fix(rest-api): validate synapseTxId #3317
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
abtestingalpha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/rest-api/src/validations/validateKappa.ts (1)
1-1: Add input validation for the synapseTxId parameter.The function assumes that
synapseTxIdis always a string. To make the function more robust, consider adding a type check at the beginning of the function.Here's a suggested improvement:
export const validateKappa = (synapseTxId: string) => { if (typeof synapseTxId !== 'string') { return false } // ... rest of the function }This addition ensures that the function handles non-string inputs gracefully, returning
falseinstead of potentially throwing an error.packages/rest-api/src/controllers/bridgeTxStatusController.ts (2)
18-21: Ensure consistent format, but consider additional validation.The introduction of
txIdWith0xensures that thesynapseTxIdalways starts with '0x', which is a good practice for consistency. However, consider adding validation to ensure thatsynapseTxIdis a valid hexadecimal string before processing it.You could add a validation function like this:
function isValidHex(value: string): boolean { return /^(0x)?[0-9A-Fa-f]+$/.test(value); } // Then use it before processing synapseTxId if (!isValidHex(synapseTxId)) { return res.status(400).json({ error: 'Invalid synapseTxId format' }); }
Line range hint
1-93: Consider implementing comprehensive input validation.The changes to handle
synapseTxIdimprove consistency by ensuring the '0x' prefix. However, to further enhance data integrity and security, consider implementing a more comprehensive validation approach for all input parameters at the beginning of the function.You could create a separate validation function:
function validateInputs(query: any): { valid: boolean; errors: string[] } { const errors = []; if (!isValidChainId(query.destChainId)) errors.push('Invalid destChainId'); if (!isValidBridgeModule(query.bridgeModule)) errors.push('Invalid bridgeModule'); if (!isValidHex(query.synapseTxId)) errors.push('Invalid synapseTxId'); return { valid: errors.length === 0, errors }; } // Use it at the beginning of the controller const { valid, errors } = validateInputs(req.query); if (!valid) { return res.status(400).json({ errors }); }This approach would centralize input validation, making it easier to maintain and extend in the future.
packages/rest-api/src/tests/bridgeTxStatusRoute.test.ts (1)
67-79: LGTM! Consider adding more test cases for edge cases.The new test case effectively checks for invalid
synapseTxIdinput, which aligns with the PR objective of implementing validation forsynapseTxId. The use of a SQL injection attempt as an invalid input is a good security-conscious choice.Consider adding more test cases to cover other edge cases, such as:
- An empty string
- A very long string
- A string with special characters but no hex digits
This will ensure a more comprehensive validation of the
synapseTxIdparameter.packages/rest-api/src/routes/bridgeTxStatusRoute.ts (1)
138-141: LGTM: Enhanced validation forsynapseTxId.The updated validation logic for
synapseTxIdis well-implemented:
- It checks if the input is a string.
- It uses the custom
validateKappafunction to ensure it's a valid hex string.- Clear error messages are provided for each validation step.
These changes align well with the PR objectives to implement validation for
synapseTxId.Consider combining the string check and the custom validation into a single error message for a more concise user experience:
check('synapseTxId') .exists() .withMessage('synapseTxId is required') .isString() .custom((value) => validateKappa(value)) .withMessage('synapseTxId must be a valid hex string')This change would reduce the number of potential error messages while still providing clear feedback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/rest-api/src/controllers/bridgeTxStatusController.ts (1 hunks)
- packages/rest-api/src/routes/bridgeTxStatusRoute.ts (2 hunks)
- packages/rest-api/src/tests/bridgeTxStatusRoute.test.ts (1 hunks)
- packages/rest-api/src/validations/validateKappa.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/rest-api/src/validations/validateKappa.ts (1)
1-11: LGTM! The implementation is correct and secure.The
validateKappafunction correctly validates 64-character hexadecimal strings, handling both cases with and without the '0x' prefix. The implementation is secure as it strictly limits input length and format.packages/rest-api/src/controllers/bridgeTxStatusController.ts (1)
25-25: Correct usage oftxIdWith0x.The use of
txIdWith0xin theSynapse.getBridgeTxStatuscall is consistent with the earlier change and ensures that the transaction ID always has the '0x' prefix when passed to this function.packages/rest-api/src/routes/bridgeTxStatusRoute.ts (2)
8-8: LGTM: New import statement forvalidateKappa.The import statement for the custom validation function
validateKappais correctly added.
Line range hint
1-146: Summary: Successful implementation ofsynapseTxIdvalidation.The changes in this file successfully implement the validation for
synapseTxIdas outlined in the PR objectives:
- A new custom validation function
validateKappais imported.- The validation logic for
synapseTxIdis enhanced to ensure it's a string and a valid hex value.These changes improve the integrity and security of the data being handled by the
/bridgeTxStatusendpoint. The implementation is correct, and no security issues or major concerns were identified.
Deploying sanguine-fe with
|
| Latest commit: |
fa5ba7b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38bf1f9f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-validate-kappa.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3317 +/- ##
===================================================
- Coverage 31.92469% 26.98386% -4.94083%
===================================================
Files 238 178 -60
Lines 14553 11833 -2720
Branches 356 82 -274
===================================================
- Hits 4646 3193 -1453
+ Misses 9614 8357 -1257
+ Partials 293 283 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Validates
synapseTxIdfor sanitation before hitting controller.Summary by CodeRabbit
New Features
synapseTxIdis a valid hexadecimal string.Bug Fixes
synapseTxIdinputs.Tests
synapseTxId.