diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index a04f2653285..e5df7b4f49e 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -53,6 +53,7 @@ This section contains changes targeting a future version. - `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) - `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529) - Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `tx_history`: The `start` field now returns `invalidParams` if the value is not a non-negative integer. Previously, values that could not be converted returned an `internal` error, and strings, booleans, and `null` were silently coerced. [#7891](https://github.com/XRPLF/rippled/pull/7891) ## XRP Ledger server version 3.1.0 diff --git a/src/test/rpc/TransactionHistory_test.cpp b/src/test/rpc/TransactionHistory_test.cpp index 6227a1b75bb..e581cfe8892 100644 --- a/src/test/rpc/TransactionHistory_test.cpp +++ b/src/test/rpc/TransactionHistory_test.cpp @@ -42,6 +42,38 @@ class TransactionHistory_test : public beast::unit_test::Suite BEAST_EXPECT(result[jss::error] == "noPermission"); BEAST_EXPECT(result[jss::status] == "error"); } + + { + // a malformed start must report invalidParams, not internal + json::Value objStart{json::ValueType::Object}; + objStart["nope"] = 0; + json::Value arrStart{json::ValueType::Array}; + arrStart.append(0); + + // Note that a whole number beyond the range of a uint, e.g. + // 4294967296, is rejected by the json parser itself, so it never + // reaches the handler and is not covered here. + boost::container::static_vector const badStarts{ + json::Value{-1}, // negative + json::Value{"abc"}, // not a number + json::Value{"5"}, // numeric, but a string + json::Value{1.5}, // not a whole number + json::Value{1e30}, // beyond the range of a uint + json::Value{true}, // bool + json::Value{}, // null + objStart, + arrStart, + }; + + for (auto const& badStart : badStarts) + { + json::Value params{json::ValueType::Object}; + params[jss::start] = badStart; + auto const result = env.client().invoke("tx_history", params)[jss::result]; + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::status] == "error"); + } + } } void diff --git a/src/xrpld/rpc/handlers/transaction/TxHistory.cpp b/src/xrpld/rpc/handlers/transaction/TxHistory.cpp index a45046773cc..560cb3aa157 100644 --- a/src/xrpld/rpc/handlers/transaction/TxHistory.cpp +++ b/src/xrpld/rpc/handlers/transaction/TxHistory.cpp @@ -26,7 +26,14 @@ doTxHistory(RPC::JsonContext& context) if (!context.params.isMember(jss::start)) return rpcError(RpcInvalidParams); - unsigned int const startIndex = context.params[jss::start].asUInt(); + // Guard the conversion: json::Value::asUInt() throws for negative, + // out-of-range, non-numeric or non-scalar values, which would surface as a + // generic internal error instead of invalid parameters. + auto const& jvStart = context.params[jss::start]; + if (!jvStart.isUInt() && (!jvStart.isInt() || jvStart.asInt() < 0)) + return rpcError(RpcInvalidParams); + + unsigned int const startIndex = jvStart.asUInt(); if ((startIndex > 10000) && (!isUnlimited(context.role))) return rpcError(RpcNoPermission);