fix: Validate the tx_history start parameter - #7891
Draft
SoubeDev wants to merge 2 commits into
Draft
Conversation
doTxHistory read the start parameter with an unguarded asUInt(), which throws for negative, out-of-range, non-numeric and non-scalar values. The throw was swallowed by callMethod's catch-all, so a malformed start produced a generic internal error rather than invalidParams, and the request was recorded as a server fault in the perf log. Guard the conversion with the same isUInt()/isInt() check that readLimitField already applies to the limit field, and return invalidParams when it fails. This makes the handler stricter: a start given as a numeric string, null, or a bool was previously coerced and is now rejected. tx_history is registered for API version 1 only and is retired in version 2, so the affected surface is small. Fixes XRPLF#6769
The entry was added with a placeholder because the pull request did not exist when the fix was committed. Point it at XRPLF#7891.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
tx_historyreturned a genericinternalerror when itsstartparameter wasmalformed. This adds a type guard so the handler returns
invalidParamsinstead,matching how every other RPC handler treats an unsigned-integer field.
Fixes #6769.
Context of Change
doTxHistoryread its only parameter with an unguarded conversion:json::Value::asUInt()throws for several values a client can send(
src/libxrpl/json/json_value.cpp:630-671):startasUInt()behavior-1(negativeInt)Throw<json::Error>— "Negative integer can not be converted..."1e30(Realout of range)Throw<json::Error>— "Real out of unsigned integer range"{}/[]Throw<json::Error>— "Type is not convertible to uint""abc"beast::lexicalCastThrow→BadLexicalCastThe throw escaped into
callMethod's blanketcatch (std::exception&)(
src/xrpld/rpc/detail/RPCHandler.cpp:177-187), which injectsRpcInternal. So baduser input was reported as a server fault: the client got
internal(code 73) ratherthan
invalidParams(code 31), and the request was recorded viaperfLog.rpcErrorasthough the server had failed.
A second, quieter group of values never threw at all —
"5",null,trueand1.5were silently coerced to a number and the request proceeded.This is long-standing rather than a recent regression; the unguarded conversion is
present as far back as the handler's history goes, predating the repo-wide
clang-formatpass in 50760c6 (2020-04-17).The fix reuses the predicate
readLimitFieldalready applies tolimit(
src/xrpld/rpc/detail/RPCHelpers.cpp:126-127), so the two are consistent:The guard is placed before the existing
startIndex > 10000role check so a malformedvalue can never reach it.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)I have checked "Breaking change" deliberately, and want to flag the nuance for
reviewers rather than bury it.
Making the throwing cases return
invalidParamsinstead ofinternalis notbreaking — those requests already failed.
What is breaking is that
startvalues which were previously coerced are nowrejected:
"5"(a numeric string),null,true/false, and non-whole reals like1.5. A client relying on any of those getsinvalidParamswhere it used to get aresult.
The usual remedy — gating the stricter behavior behind the next
api_version— is notavailable here.
tx_historyis registeredminApiVer = 1, maxApiVer = 1(
src/xrpld/rpc/detail/Handler.cpp:323-328) and is listed under "Removed methods" inAPI-VERSION-2.md, so there is no later version to gate against. The options were tomatch
readLimitField's strictness or to keep accepting numeric strings indefinitely;I chose consistency with the rest of the RPC layer, on the grounds that the affected
surface is a single v1-only command. Happy to switch to a lenient variant that still
accepts numeric strings if reviewers prefer.
API-CHANGELOG.mdhas been updated underUnreleased→Bugfixes.Before / After
Request:
{ "command": "tx_history", "start": -1 }Before:
{ "error": "internal", "error_code": 73, "error_message": "Internal error.", "status": "error" }After:
{ "error": "invalidParams", "error_code": 31, "error_message": "Invalid parameters.", "status": "error" }Valid input is unaffected:
startas a non-negative integer behaves exactly asbefore, including the existing
noPermissionresponse for non-admin callers above10000.
Test Plan
testBadInput()insrc/test/rpc/TransactionHistory_test.cppgains a table-drivenblock covering nine malformed values —
-1,"abc","5",1.5,1e30,true,null, an object, and an array — each assertinginvalidParams/status == "error".To confirm the tests actually exercise the change rather than passing vacuously, I
reverted the handler, rebuilt, and re-ran: 13 assertions fail without the fix and 0
with it. The count decomposes exactly as the two old behaviors predict — the five
throwing inputs previously returned
internal, failing theerrorassertion butpassing
status == "error"(1 each), while the four silently-coerced inputspreviously succeeded, failing both (2 each).
Results on macOS (arm64, apple-clang 17, Debug):
RPCCallis included because it covers the command-line parser, which this changedeliberately does not touch — see Future Tasks.
Future Tasks
RPCParser::parseTxHistory(src/xrpld/rpc/detail/RPCCall.cpp:1132) has the sameunguarded
asUInt()on the command-line path, soxrpld tx_history abcstill throwsand surfaces as
internalviaRPCCall.cpp:1804-1809. Three cases inRPCCall_test.cpp:5477-5500already assert this, each marked// Note: this really shouldn't throw, but does at the moment.I left it out to keepthis PR scoped to the reported issue; it is a small follow-up if reviewers would like
it addressed.