fix: Make assorted RPC fixes - #6529
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens RPC request validation and corrects a few RPC-related behavioral issues, aligning error handling with intended semantics and improving security (e.g., HTTPS gravatar URL).
Changes:
- Add stricter type validation for several RPC request fields (string/bool) and return appropriate errors.
- Fix incorrect logic/behavior in a few RPC handlers (e.g.,
subscribetaker error code,Roleforwarded parsing,NoRippleCheckboolean operators). - Add/extend unit tests to cover the new validation and bug fixes, and update the API changelog.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/handlers/Subscribe.cpp | Validate books[].taker is a string; return actMalformed for malformed account values. |
| src/xrpld/rpc/handlers/Submit.cpp | Validate fail_hard is boolean and propagate structured validation errors via Expected. |
| src/xrpld/rpc/handlers/NoRippleCheck.cpp | Fix boolean logic by replacing bitwise & with logical &&. |
| src/xrpld/rpc/handlers/LedgerHandler.cpp | Enforce boolean typing for multiple ledger options and adjust ledger lookup gating. |
| src/xrpld/rpc/handlers/LedgerData.cpp | Enforce binary is boolean (with field-specific error). |
| src/xrpld/rpc/handlers/GetAggregatePrice.cpp | Avoid repeated ledger lookup by moving it out of the per-item loop. |
| src/xrpld/rpc/handlers/AccountInfo.cpp | Switch gravatar URL from HTTP to HTTPS. |
| src/xrpld/rpc/handlers/AccountChannels.cpp | Validate destination_account is a string before parsing. |
| src/xrpld/rpc/detail/TransactionSign.cpp | Remove redundant sle null-check in verify path (already validated earlier). |
| src/xrpld/rpc/detail/Role.cpp | Fix remaining-length calculation when parsing Forwarded to prevent buffer over-read. |
| src/test/rpc/Subscribe_test.cpp | Add coverage for invalid books[].taker types/values and expected errors. |
| src/test/rpc/Submit_test.cpp | New tests covering fail_hard type validation for both submit paths. |
| src/test/rpc/Roles_test.cpp | Add targeted cases for the Forwarded parsing fix; improve testcase labeling. |
| src/test/rpc/LedgerRPC_test.cpp | Add coverage ensuring non-boolean ledger option fields return invalid params. |
| src/test/rpc/LedgerData_test.cpp | Add coverage ensuring non-boolean binary returns the expected field error. |
| src/test/app/PayChan_test.cpp | Add coverage ensuring non-string destination_account is rejected in account_channels. |
| API-CHANGELOG.md | Document the validation/error behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6529 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.0%
=========================================
Files 1010 1010
Lines 75961 75995 +34
Branches 7392 7382 -10
=========================================
+ Hits 62330 62356 +26
- Misses 13631 13639 +8
🚀 New features to boost your workflow:
|
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit 09ce6f3.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
account_channels: Thedestination_accountfield now returns an error if the value is not a string.subscribe: Thetakerfield in thebooksarray now returns an error if the value is not a string.account_info: Theurlgravatarfield now uses HTTPS instead of HTTP.ledger: Thefull,accounts,transactions,expand,binary,owner_funds, andqueuefields now return an error if the value is not a boolean.ledger_data: Thebinaryfield now returns an error if the value is not a boolean.submit: Thefail_hardfield now returns an error if the value is not a boolean.subscribe: Thetakerfield in thebooksarray now returnsactMalformedinstead ofbadIssuerif the value is not a valid account.noripple_check: Changed bitwise&to logical&&in several places where booleans were being combinedContext of Change
AI code review
Fixes #6759
Fixes #6762
Type of Change
API Impact
Basically only validation changes, the API Changelog has been updated.
Test Plan
Everything added has tests.