-
Couldn't load subscription status.
- Fork 1.6k
fix: account_tx limit parameter validation for malformed values
#5891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
account_tx limit parameter validation for malformed values
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5891 +/- ##
=========================================
- Coverage 79.5% 79.5% -0.0%
=========================================
Files 817 817
Lines 72210 72210
Branches 8280 8278 -2
=========================================
- Hits 57398 57387 -11
- Misses 14812 14823 +11
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is fine. I believe in Clio we have the same behaviour.
src/xrpld/rpc/handlers/AccountTx.cpp
Outdated
| if (auto err = RPC::readLimitField(limit, RPC::Tuning::accountTx, context)) | ||
| return *err; | ||
|
|
||
| args.limit = limit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why not directly use args.limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point - fixed
src/xrpld/rpc/handlers/AccountTx.cpp
Outdated
| } | ||
|
|
||
| unsigned int limit; | ||
| if (auto err = RPC::readLimitField(limit, RPC::Tuning::accountTx, context)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: err can be const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - fixed
src/xrpld/rpc/handlers/AccountTx.cpp
Outdated
| if (args.limit == 0) | ||
| return rpcError(rpcINVALID_PARAMS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (args.limit == 0) | |
| return rpcError(rpcINVALID_PARAMS); | |
| auto const& jvLimit = params[jss::limit]; | |
| if (jvLimit.asUInt() == 0) | |
| return rpcError(rpcINVALID_PARAMS); |
This should work, since the RPC::readLimitField will set args.limit to the minimum value of the accountTx's LimitRange. You wouldn't need to check that it's a uint anymore, since the readLimitField already has done so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding it directly into readLimitField, but unsure if that'll break something in a separate RPC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give it a shot, I'd say - assuming test coverage is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this does change the error response for at least one RPC method, I'd like @godexsoft to take a second look.
Co-authored-by: mvadari <[email protected]>
Co-authored-by: mvadari <[email protected]>
782630f to
24a8364
Compare
High Level Overview of Change
Fixed
account_txRPC method to properly validate malformedlimitparameter values. Previously, invalid values like0,1.2,"10",true,false,-1,[],{}, etc. were either accepted without errors or caused internal errors. Now all malformed values correctly returninvalidParamserror.Fixes #5138
Context of Change
This is a bug fix for improper parameter validation in the
account_txRPC method. The bug has existed since the method was implemented - the original code directly calledasUInt()on the limit parameter without proper type validation, which bypassed type checking and caused implicit conversions or internal errors.The fix follows the established pattern used by other RPC methods (like
account_lines) by:readLimitFieldhelper function for consistent behaviorType of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Before / After
Before (incorrect behavior):
Request with limit = 0:
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 0}]}Response: Success with transactions (incorrect - should reject invalid limit)
Request with limit = "10":
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": "10"}]}Response: Success (incorrect - should reject non-integer)
Request with limit = -1:
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": -1}]}Response: Internal error (incorrect error type)
After (correct behavior):
Request with limit = 0:
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 0}]}Response:
{ "result": { "error": "invalidParams", "error_code": 31, "error_message": "Invalid parameters.", "status": "error" } }Request with limit = "10", 1.2, true, false, -1, [], {}, etc.:
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": "10"}]}Response:
{ "result": { "error": "invalidParams", "error_code": 31, "error_message": "Invalid field 'limit', not unsigned integer.", "status": "error" } }Valid request with limit = 10:
{"method": "account_tx", "params": [{"account": "rpU4XtUSB7vx7hnDbkJ7pdbqAHEEoxTL33", "limit": 10}]}Response: Success with up to 10 transactions
Test Plan
Added comprehensive test coverage in
AccountTx_test.cppfor all malformed limit values from the issue:limit = 0→ returnsrpcINVALID_PARAMSlimit = 1.2,"10",true,false,-1,[],{},"malformed",["limit"],{"limit": 10}→ returnexpected_field_errorwith message "Invalid field 'limit', not unsigned integer."limit = 10(valid) → works correctlyThe tests verify that:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.