feat: add refinement for transaction notes #2377
Conversation
WalkthroughThis pull request introduces HTML sanitization by adding the necessary dependency and its type definitions. Changes to the transaction proposal tests include adding an "origin" property with potentially unsafe content and updating the expected output. A new test ensures that the note mapper correctly sanitizes malicious input. The mapper itself is updated to accept multiple transaction types and to sanitize the note content before returning it. Finally, the transactions service now injects the note mapper and includes a method to verify and parse the "origin" property, handling invalid JSON accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransactionsService
participant NoteMapper
Client->>TransactionsService: proposeTransaction(proposeTransactionDto)
TransactionsService->>TransactionsService: verifyOrigin(origin)
alt Valid JSON in origin
TransactionsService->>NoteMapper: mapTxNote(parsedOrigin)
NoteMapper-->>TransactionsService: sanitized note
TransactionsService->>TransactionsService: Append sanitized note to origin
else Invalid JSON
TransactionsService->>TransactionsService: Catch error, set origin to null
end
TransactionsService-->>Client: Return JSON string (or null) of origin
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (1)
15-15: Consider configuring sanitizeHtml options.The default sanitizeHtml options might be too permissive. Consider explicitly configuring allowed tags and attributes based on your requirements.
- return sanitizeHtml(origin.note as string); + return sanitizeHtml(origin.note as string, { + allowedTags: [], // Disallow all HTML tags + allowedAttributes: {}, // Disallow all attributes + disallowedTagsMode: 'recursiveEscape' + });src/routes/transactions/transactions.service.ts (1)
439-441: Consider error handling for verifyOrigin.While the code is functionally correct, it might benefit from explicit error handling to provide better feedback when origin verification fails.
Consider adding error handling:
- args.proposeTransactionDto.origin = this.verifyOrigin( - args.proposeTransactionDto, - ); + try { + args.proposeTransactionDto.origin = this.verifyOrigin( + args.proposeTransactionDto, + ); + } catch (error) { + this.loggingService.warn('Failed to verify transaction origin', { + error, + chainId: args.chainId, + safeAddress: args.safeAddress, + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)src/routes/transactions/__tests__/controllers/propose-transaction.transactions.controller.spec.ts(2 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts(1 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts(1 hunks)src/routes/transactions/transactions.service.ts(4 hunks)
🔇 Additional comments (7)
package.json (1)
57-57:✅ Verification successful
Verify the sanitize-html package version.
Let's confirm if version ^2.14.0 is valid and check for any known security advisories.
🌐 Web query:
What is the latest version of sanitize-html package and are there any known security advisories?💡 Result:
The latest version of
sanitize-htmlis 2.14.0, released in December 2024[1]. This package provides HTML sanitization with customizable rules for allowed tags/attributes and is widely used to clean user-generated HTML content[1][4].Security Advisories
Several vulnerabilities have been identified in older versions:
Vulnerability Affected Versions Fix Version Details Information Exposure <2.12.1 2.12.1 Allowed style attribute enumeration of server files when used on backend[3][5] ReDoS <2.7.1 2.7.1 Regular Expression Denial of Service via HTML comment removal[3] XSS via Math/SVG N/A (Sanitize library) Sanitize 5.2.1 Related advisory shows risks when allowing certain elements like <math>/<svg>[2]IDN Bypass All versions Patched Could bypass allowedIframeHostnameswith internationalized domain names[3]Attribute Merging All versions Ongoing Incomplete attributes might merge dangerously (e.g., SRC=)[3]For secure usage:
- Upgrade to ≥v2.14.0[1]
- Avoid allowing risky elements like
<math>,<svg>, or<style>[2][6]- Use strict configurations:
sanitizeHtml(dirtyHtml, { allowedTags: ['b', 'i', 'a', 'p'], allowedAttributes: {a: ['href']}, allowProtocolRelative: false });- Consider client-side sanitizers like DOMPurify for additional protection[6][9]
The package remains actively maintained with security updates addressing these issues[1][3][5].
Citations:
- 1: https://www.npmjs.com/package/sanitize-html
- 2: GHSA-p4x4-rw2p-8j8m
- 3: https://advisories.gitlab.com/pkg/npm/sanitize-html/
- 4: https://classic.yarnpkg.com/en/package/sanitize-html
- 5: GHSA-rm97-x556-q36h
- 6: https://www.sonarsource.com/blog/sanitize-client-side-why-server-side-html-sanitization-is-doomed-to-fail/
- 7: https://pypi.org/project/html-sanitizer/
- 8: https://learn.microsoft.com/en-us/answers/questions/1520400/how-to-sanitize-input-in-an-application-that-is-of
- 9: https://npm-compare.com/dompurify,sanitize-html,xss
- 10: https://www.nuget.org/packages/HtmlSanitizer/
Sanitize-html package version is verified as current.
After checking, the sanitize-html version specified in package.json ("^2.14.0") is indeed the latest release (as of December 2024), and the use of the caret (^) ensures that minor security patches are applied automatically. The addition of "@types/sanitize-html" in devDependencies aligns with best practices. No further action is required regarding this dependency.
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts (1)
19-34: Great test coverage for XSS prevention!The test cases effectively cover both direct and encoded XSS attack vectors, ensuring proper sanitization.
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (1)
8-10: LGTM! Good type safety improvement.The method signature now correctly handles both MultisigTransaction and ProposeTransactionDto types.
src/routes/transactions/__tests__/controllers/propose-transaction.transactions.controller.spec.ts (2)
110-115: LGTM! Good integration test coverage.The test properly verifies that malicious content in the origin note is sanitized in the response.
180-180: LGTM! Correct assertion for sanitized content.The test correctly expects an empty string as the result of sanitizing malicious content.
src/routes/transactions/transactions.service.ts (2)
41-41: LGTM! Clean dependency injection setup.The
MultisigTransactionNoteMapperis properly imported and injected as a private readonly dependency.Also applies to: 55-55
543-558: 🛠️ Refactor suggestion✅ Verification successful
Enhance error handling in verifyOrigin.
The current implementation silently catches all errors and returns null. This might hide important issues.
Consider these improvements:
private verifyOrigin(transaction: ProposeTransactionDto): string | null { if (transaction.origin) { try { const note = this.multisigTransactionNoteMapper.mapTxNote(transaction); const origin = JSON.parse(transaction.origin); + if (typeof origin !== 'object' || origin === null) { + throw new Error('Origin must be a JSON object'); + } origin.note = note; return JSON.stringify(origin); - } catch { - // If the origin is not a valid JSON, we return null + } catch (error) { + // Log specific error for debugging + this.loggingService.debug('Invalid transaction origin', { + error, + origin: transaction.origin, + }); + return null; } } return null; }The improvements:
- Validate that parsed JSON is an object
- Add specific error logging for debugging
Let's verify the usage of logging service in the class:
🏁 Script executed:
#!/bin/bash # Verify logging service usage patterns rg "loggingService\." -A 2Length of output: 40291
Enhanced error handling in verifyOrigin verified as consistent with logging usage.
The proposed changes correctly validate that the parsed JSON is an object and log detailed debug information when parsing fails. Notably, using
this.loggingService.debugfor tracking invalid JSON aligns with other parts of the codebase where debug-level logs are used for non-critical issues. The grep output confirms that the logging service is widely used (with debug, info, warn, and error), and using debug here is acceptable.Changes to be made:
- Add a check to ensure the parsed JSON is an object and not null.
- In the catch block, log the error and the raw
transaction.originusingthis.loggingService.debug.- Continue to return
nullupon failure to parse.private verifyOrigin(transaction: ProposeTransactionDto): string | null { if (transaction.origin) { try { const note = this.multisigTransactionNoteMapper.mapTxNote(transaction); const origin = JSON.parse(transaction.origin); + if (typeof origin !== 'object' || origin === null) { + throw new Error('Origin must be a JSON object'); + } origin.note = note; return JSON.stringify(origin); - } catch { - // If the origin is not a valid JSON, we return null + } catch (error) { + // Log specific error for debugging + this.loggingService.debug('Invalid transaction origin', { + error, + origin: transaction.origin, + }); + return null; } } return null; }
PooyaRaki
force-pushed
the
feat/txNote
branch
from
047235a to
26f1663
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (1)
15-15: Remove redundant type assertion.The type assertion
as stringis unnecessary since we already checktypeof origin.note === 'string'.- return sanitizeHtml(origin.note as string, { + return sanitizeHtml(origin.note, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)src/routes/transactions/__tests__/controllers/propose-transaction.transactions.controller.spec.ts(2 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts(1 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts(1 hunks)src/routes/transactions/transactions.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts
- package.json
- src/routes/transactions/tests/controllers/propose-transaction.transactions.controller.spec.ts
- src/routes/transactions/transactions.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: tests (test:e2e:cov)
- GitHub Check: tests (test:cov)
🔇 Additional comments (3)
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (3)
3-4: LGTM! Necessary imports added.The new imports support HTML sanitization and expanded type handling.
15-19: LGTM! Robust HTML sanitization implementation.The sanitization configuration follows security best practices by:
- Stripping all HTML tags and attributes
- Preventing iframe injection
- Using a restrictive allowlist approach
8-10:❓ Verification inconclusive
Verify type compatibility of the union type.
The method now accepts both
MultisigTransactionandProposeTransactionDto. Please ensure both types have compatibleoriginproperties to prevent runtime errors.
🏁 Script executed:
#!/bin/bash # Description: Verify that both types have compatible 'origin' property # Check MultisigTransaction definition echo "Checking MultisigTransaction definition:" ast-grep --pattern 'interface MultisigTransaction { $$$ origin: $_; $$$ }' # Check ProposeTransactionDto definition echo "Checking ProposeTransactionDto definition:" ast-grep --pattern 'interface ProposeTransactionDto { $$$ origin: $_; $$$ }'Length of output: 353
Action: Verify ‘origin’ Property Compatibility
Our initial automated AST grep didn’t return explicit definitions for the
originproperty in either theMultisigTransactionorProposeTransactionDtotypes. Please double-check that both types include anoriginproperty of a compatible type (e.g., both as a string, number, etc.) to avoid any runtime issues. In case these definitions are declared differently (for instance, as classes or via type aliases), manually verify their signatures to ensure they align.
- Confirm that
MultisigTransactiondefinesoriginwith the intended type.- Confirm that
ProposeTransactionDtodefinesoriginwith a compatible type.
PooyaRaki
force-pushed
the
feat/txNote
branch
from
26f1663 to
d1623f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (1)
15-19: Consider adding security logging.For security monitoring purposes, consider logging attempts to inject malicious content.
return sanitizeHtml(origin.note as string, { allowedAttributes: {}, allowedTags: [], allowedIframeHostnames: [], + transformTags: { + '*': (tagName, attribs) => { + if (Object.keys(attribs).length > 0 || tagName !== '') { + this.logger.warn('Attempted HTML injection in transaction note', { + tagName, + attribs, + }); + } + return { tagName: '', attribs: {} }; + }, + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)src/routes/transactions/__tests__/controllers/propose-transaction.transactions.controller.spec.ts(2 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts(1 hunks)src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts(1 hunks)src/routes/transactions/transactions.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/transactions/tests/controllers/propose-transaction.transactions.controller.spec.ts
- src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.spec.ts
- package.json
- src/routes/transactions/transactions.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (test:e2e:cov)
- GitHub Check: tests (test:cov)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: tests (test:e2e:cov)
- GitHub Check: tests (test:cov)
🔇 Additional comments (3)
src/routes/transactions/mappers/multisig-transactions/multisig-transaction-note.mapper.ts (3)
8-10: LGTM! Good type safety with union type.The method signature change to accept both
MultisigTransactionandProposeTransactionDtomaintains type safety while providing more flexibility.
15-19: LGTM! Secure sanitization configuration.The HTML sanitization is implemented with appropriately strict security controls:
- No HTML tags allowed
- No attributes allowed
- No iframes allowed
This configuration effectively prevents XSS attacks.
3-4:❓ Verification inconclusive
Verify the sanitize-html package version for security.
The addition of
sanitize-htmlfor HTML sanitization is a good security practice. Let's ensure we're using a secure version.
🏁 Script executed:
#!/bin/bash # Check the installed version and latest version of sanitize-html npm list sanitize-html npm view sanitize-html versions --json | jq -r '.[-1]' # Check for known vulnerabilities npm auditLength of output: 477
Action Needed: Verify the
sanitize-htmlDependency Installation and Lockfile SetupIt appears that while the code imports
sanitize-html, our initial checks indicate that the package does not seem to be explicitly installed (as shown by the empty output fromnpm list sanitize-html), and npm audit was unable to run due to a missing lockfile. Please perform the following:
- Dependency Verification:
Ensure thatsanitize-htmlis listed in your package.json. If it's missing, add it by running:npm install sanitize-html- Lockfile Generation:
Since npm audit failed because no lockfile exists, generate one with:npm i --package-lock-only- Security Check:
Confirm that the installed version (currently the latest available is 2.14.0) is secure and free of vulnerabilities.After addressing these points, please re-run your audit to ensure that all security issues are resolved.
Summary
This PR improves and clarifies transaction notes for better accuracy and readability.
Changes
Summary by CodeRabbit