diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index ca2b049891..88f776b8a1 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -24,6 +24,10 @@ This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-R ## Unreleased +### Breaking changes + +- The `ripplerpc` request field, which selects the shape of the JSON-RPC reply envelope, is now validated, and a value that is not exactly `"1.0"`, `"2.0"` or `"3.0"` is rejected. This reaches the JSON-RPC transport at every API version. It does not reach a WebSocket session, which reads the field only to echo it back. A request sending `"2"`, `" 2.0"`, `"2.00"`, `"02.0"`, `"2.0.0"`, `"10.0"` or any other text, such as `"abc"` or `"x2"`, gets a working reply today. After this ships, such a request sent alone gets HTTP 400 with `ripplerpc is not a supported version`, charged as a malformed request, and such an entry of a `"method": "batch"` body gets that error in its own reply while the batch answers 200. A client that spells the version loosely must therefore be corrected to one of the three exact values, or omit the field. Previously the value was compared as a string, which both accepted values that name no version and ordered multi-digit versions incorrectly: `"abc"` and `"x2"` sorted above `"3.0"` and so selected version 3, and `"10.0"` sorted below `"2.0"` and so selected version 1. Requests that send one of the three supported values, or omit the field, are unaffected. + ### Bugfixes - A request echoed back in an error reply now has every credential-bearing field masked: `admin_password`, `admin_user`, `passphrase`, `password`, `secret`, `seed`, `seed_hex`, `url_password`, `url_username` and `username`. Nesting no longer matters, so a credential inside `params` is masked too. The same masking is applied to every request and reply written to the log, and it covers six further names that only a reply carries: `master_key`, `master_seed`, `master_seed_hex`, `validation_key`, `validation_private_key` and `validation_seed`, which is how `wallet_propose` and `validation_create` used to write a live private key to the log. A request or reply written to the log is truncated at 10,000 characters. @@ -32,6 +36,7 @@ This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-R - Four error codes that named no HTTP status of their own, and so answered 200 on a reply reporting an error, now name one: `actMalformed`, `alreadyMultisig` and `alreadySingleSig` answer 400, and `actNotFound` answers 404. **No shipped envelope reports these four.** A request sending `ripplerpc: "3.0"` still receives 200 for all four, as it always has, so `account_info` on a malformed account or one the ledger does not hold answers 200 exactly as before. - `submit`, `simulate`, `transaction_entry`, `ledger_entry` and `ledger_accept`: Errors from these methods now include `error_code` and `error_message` alongside the `error` token, as every other method already did. Each error now answers the status its code names: 400 for a malformed request, 404 for `transactionNotFound`, 500 for an internal failure, and 501 for `notYetImplemented` and `notStandAlone`. That status change reaches only a request sending `ripplerpc: "3.0"`, which is the envelope that derives the status from the error. With `ripplerpc` `"1.0"` the status stays 200 and the two new members appear beside `error`; with `"2.0"` the status stays 200, `error_code` appears, and the `code` and `message` members carry the code and the message rather than null, since that envelope copies them from `error_code` and `error_message` and drops `error_message`. - A reply reporting HTTP 402 or 502 now carries a status line. Those two statuses named no case in the switch that writes one, so such a reply began with a header instead and did not parse as an HTTP response at all. Both are reachable at any API version with `ripplerpc: "3.0"`, which derives the status from the error code: 402 through `highFee` from `sign`, `sign_for` or `submit` with a low `fee_mult_max`, and 502 through `dbDeserialization` from `tx`. The eleven statuses that already named a case report the same phrase they always have. +- An error reply to a request sending `ripplerpc: "2.0"` or `"3.0"` no longer carries a stray `"error_message": null` beside the error it reports. The member appeared only when the `Server` log partition was set to debug or lower, because the log statement read `error_message` after the reply had renamed it to `message`, and reading it put it back as null. So the reply a client received depended on the server's log level, and the log line itself printed an empty message. Both are fixed. ## XRP Ledger server version 3.5.0 diff --git a/src/test/server/ServerStatus_test.cpp b/src/test/server/ServerStatus_test.cpp index 6a752a1170..ea0cc482ce 100644 --- a/src/test/server/ServerStatus_test.cpp +++ b/src/test/server/ServerStatus_test.cpp @@ -1342,6 +1342,41 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En boost::system::error_code ec; + // A value naming no supported version is rejected rather than coerced. + for (auto const* version : { + "0.0", // unsupported major + "4.0", // + "10.0", // would sort below "3.0" lexicographically + "02.0", // leading zero + "2.00", // non-canonical minor + "2", // missing minor + "2.", // + ".0", // missing major + "2.x", // non-numeric minor + "2x", // trailing junk + "x2", // + "abc", // + "-1.0", // signs are not part of a version + "+2.0", // + " 2.0", // surrounding whitespace + "2.0 ", // + "2.0.0", // patch component is not accepted + "2e0", // + "2.7", // no version defines a nonzero minor + "2.99", // + "3.99", // + "2.01", // leading zero in the minor + "1.4294967295", // + "999999999999999999999999999.0", // too long to name a version + "1.999999999999999999999999999", // too long to name a version + }) + { + Response resp; + doHTTPRequest(env, yield, false, resp, ec, makeRippleRpcRequest("ping", version)); + BEAST_EXPECTS(resp.result() == kBadRequest, version); + BEAST_EXPECTS(resp.body() == "ripplerpc is not a supported version\r\n", version); + } + // A non-string is reported separately, before any version parsing. { Response resp; @@ -1367,6 +1402,91 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En BEAST_EXPECTS(reply[jss::result][jss::status] == jss::success, version); } + // Every version reports success the same way, so only the error envelope + // distinguishes them. `account_info` without an account fails in the handler, + // carrying an `error_code` that version 3 maps onto an HTTP status. + { + Response resp; + doHTTPRequest( + env, + yield, + false, + resp, + ec, + makeRippleRpcRequest("account_info", rpc::kRippleRpcVersion1)); + BEAST_EXPECT(resp.result() == kOk); + + json::Value reply; + BEAST_EXPECT(json::Reader{}.parse(resp.body(), reply)); + BEAST_EXPECT(reply[jss::result][jss::status] == jss::error); + BEAST_EXPECT(reply[jss::result].isMember(jss::error_message)); + BEAST_EXPECT(reply[jss::result].isMember(jss::request)); + BEAST_EXPECT(!reply.isMember(jss::error)); + } + + // Versions 2 and 3 share an error envelope, so one loop covers both. + for (auto const version : {rpc::kRippleRpcVersion2, rpc::kRippleRpcVersion3}) + { + Response resp; + auto const reply = postAndParse( + env, yield, resp, ec, makeRippleRpcRequest("account_info", version), version); + BEAST_EXPECTS(reply[jss::error][jss::status] == jss::error, version); + BEAST_EXPECTS(reply[jss::error].isMember(jss::message), version); + BEAST_EXPECTS(!reply[jss::error].isMember(jss::error_message), version); + BEAST_EXPECTS(!reply[jss::error].isMember(jss::request), version); + BEAST_EXPECTS(!reply.isMember(jss::request), version); + + auto const expected = version == rpc::kRippleRpcVersion3 ? kBadRequest : kOk; + BEAST_EXPECTS(resp.result() == expected, version); + } + + // A batch entry rejected before dispatch is echoed back, so it is masked too. + { + Response resp; + json::Value entry(json::ValueType::Object); + entry[jss::method] = "ping"; + entry[jss::ripplerpc] = "10.0"; + for (auto const field : rpc::kCredentialFields) + entry[std::string{field}] = "sensitive"; + + json::Value jv; + jv[jss::method] = "batch"; + jv[jss::params] = json::ValueType::Array; + jv[jss::params][0u] = entry; + auto const reply = postAndParse(env, yield, resp, ec, to_string(jv)); + BEAST_EXPECT(reply.isArray() && reply.size() == 1); + for (auto const field : rpc::kCredentialFields) + BEAST_EXPECTS(reply[0u][std::string{field}] == "", std::string{field}); + } + + // A rejected entry does not decide the version for the rest, and the batch returns 200. + { + Response resp; + json::Value jv; + jv[jss::method] = "batch"; + jv[jss::params] = json::ValueType::Array; + for (auto const& [index, version] : + {std::pair{0u, "2.0"}, std::pair{1u, "abc"}, std::pair{2u, "1.0"}}) + { + json::Value entry(json::ValueType::Object); + entry[jss::method] = "ping"; + entry[jss::ripplerpc] = version; + entry[jss::id] = index; + jv[jss::params][index] = entry; + } + auto const reply = postAndParse(env, yield, resp, ec, to_string(jv)); + BEAST_EXPECT(resp.result() == kOk); + BEAST_EXPECT(reply.isArray() && reply.size() == 3); + BEAST_EXPECT(reply[0u][jss::result][jss::status] == jss::success); + BEAST_EXPECT(reply[2u][jss::result][jss::status] == jss::success); + // A version the server cannot honor is a bad parameter, not a missing method. + BEAST_EXPECT( + reply[1u][jss::error][jss::error][jss::message] == + "ripplerpc is not a supported version"); + BEAST_EXPECT(reply[1u][jss::error][jss::error][jss::code] == -32602); + BEAST_EXPECT(reply[1u][jss::ripplerpc] == "abc"); + } + // An entry naming an `api_version` the server cannot serve is returned under `request`, // rather than spliced beside the error the way other rejections are. { @@ -1455,6 +1575,37 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En } } + /** + * What a client receives cannot depend on how the server logs. + * + * Reading an absent member off a non-const value inserts it as an explicit + * null, so reading `error_message` after the rename to `message` puts + * `"error_message": null` back into the reply. The Env below logs at debug, + * which is what arms these assertions. + * + * @param yield The coroutine the requests run on. + */ + void + testAnErrorReplyDoesNotFollowTheLogLevel(boost::asio::yield_context& yield) + { + testcase("An error reply is the same at every log level"); + + using namespace test::jtx; + Env env{*this, envconfig(), nullptr, beast::Severity::Debug}; + + boost::system::error_code ec; + + // `account_info` naming no account fails in the handler, where both members come from. + for (auto const version : {rpc::kRippleRpcVersion2, rpc::kRippleRpcVersion3}) + { + boost::beast::http::response resp; + auto const reply = postAndParse( + env, yield, resp, ec, makeRippleRpcRequest("account_info", version), version); + BEAST_EXPECTS(reply[jss::error].isMember(jss::message), version); + BEAST_EXPECTS(!reply[jss::error].isMember(jss::error_message), version); + } + } + /** * The five handlers that report a bare token carry a code and message with * it. @@ -2241,6 +2392,7 @@ public: testRipplerpcVersions(yield); testPrivilegedRequestIsNotShed(yield); testLegacyBatchEntryRejections(yield); + testAnErrorReplyDoesNotFollowTheLogLevel(yield); testHandlerErrorsCarryCodes(yield); testGainedStatusesStayOffLegacyEnvelope(yield); testUncommonHttpStatus(yield); diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 8e304b4596..9a2b5a14dd 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -68,6 +68,7 @@ #include #include #include +#include #include #include #include @@ -594,6 +595,41 @@ makeJsonError(json::Int code, json::Value&& message) return r; } +// Version of the JSON-RPC reply envelope, selected by the `ripplerpc` request parameter. It shapes +// a completed result into a reply; no handler observes it. +enum class RpcVersion { + // Errors are returned under `result`, keyed by `error_message`, and echo the (secret-masked) + // request. HTTP status is always 200. + V1, + // Errors are returned under `error`, keyed by `message`, and the request is not echoed. HTTP + // status is always 200. + V2, + // As V2, but the error code selects a 4xx/5xx HTTP status. + V3, +}; + +constexpr RpcVersion kRpcVersionIfUnspecified = RpcVersion::V1; + +/** + * Maps a `ripplerpc` value onto the envelope version it selects. + * + * The value is unauthenticated, so anything but an exact match is refused. + * + * @param value The `ripplerpc` member, as sent. + * @return The envelope version, or nullopt for the caller to reject. + */ +static std::optional +rpcVersion(std::string_view value) +{ + if (value == rpc::kRippleRpcVersion1) + return RpcVersion::V1; + if (value == rpc::kRippleRpcVersion2) + return RpcVersion::V2; + if (value == rpc::kRippleRpcVersion3) + return RpcVersion::V3; + return std::nullopt; +} + /** * The HTTP status the `ripplerpc: "3.0"` envelope reports for @p code. * @@ -619,6 +655,84 @@ legacyHttpStatus(ErrorCodeI code) } } +/** + * Shapes a handler @p result into the reply envelope for @p version, appending + * it to @p reply. + * + * @param version The envelope the reply takes. + * @param result A handler result, consumed. + * @param request The request's parameters, echoed on a version 1 error only, + * and where the `jsonrpc`, `ripplerpc` and `id` members are read. + * @param batch Whether to append to @p reply rather than assign it. + * @param reply The reply this writes. + * @param journal Where an error is logged. + * @return The HTTP status the reply is sent with. Only version 3 derives it + * from the error code; the others report 200. A batch discards it. + */ +static int +shapeReply( + RpcVersion version, + json::Value result, + json::Value const& request, + bool batch, + json::Value& reply, + beast::Journal journal) +{ + json::Value r(json::ValueType::Object); + int status = 200; + + if (!result.isMember(jss::error)) + { + result[jss::status] = jss::success; + r[jss::result] = std::move(result); + } + else + { + // Read through a const reference: the non-const `operator[]` inserts a null member for an + // absent name, and the reply below is built out of `result`. + json::Value const& reported = result; + + JLOG(journal.debug()) << "rpcError: " << reported[jss::error] << ": " + << reported[jss::error_message]; + + if (version == RpcVersion::V3 && reported[jss::error_code].isInt()) + status = legacyHttpStatus(static_cast(reported[jss::error_code].asInt())); + + result[jss::status] = jss::error; + + if (version == RpcVersion::V1) + { + result[jss::request] = rpc::maskSecrets(request); + r[jss::result] = std::move(result); + } + else + { + result[jss::code] = result[jss::error_code]; + result[jss::message] = result[jss::error_message]; + result.removeMember(jss::error_message); + r[jss::error] = std::move(result); + } + } + + if (request.isMember(jss::jsonrpc)) + r[jss::jsonrpc] = request[jss::jsonrpc]; + if (request.isMember(jss::ripplerpc)) + r[jss::ripplerpc] = request[jss::ripplerpc]; + if (request.isMember(jss::id)) + r[jss::id] = request[jss::id]; + + if (batch) + { + reply.append(std::move(r)); + } + else + { + reply = std::move(r); + } + + return status; +} + void ServerHandler::processRequest( Port const& port, @@ -662,6 +776,9 @@ ServerHandler::processRequest( } json::Value reply(batch ? json::ValueType::Array : json::ValueType::Object); + // Only a lone request selects the HTTP status: a batch may mix versions and reports each + // entry's outcome in its own reply, so the batch itself always succeeds. + int httpStatus = 200; auto const start(std::chrono::high_resolution_clock::now()); for (unsigned i = 0; i < size; ++i) { @@ -837,10 +954,11 @@ ServerHandler::processRequest( params = jsonRPC; } - std::string ripplerpc = "1.0"; + RpcVersion envelope = kRpcVersionIfUnspecified; if (params.isMember(jss::ripplerpc)) { - // Not a method-not-found condition, but it is the code shipped versions report. + // A `ripplerpc` the server cannot honor is a bad parameter, so the second check reports + // that code. The first keeps the method-not-found code shipped versions report. if (!params[jss::ripplerpc].isString()) { usage.charge(resource::kFeeMalformedRpc); @@ -848,7 +966,17 @@ ServerHandler::processRequest( return; continue; } - ripplerpc = params[jss::ripplerpc].asString(); + + auto const parsed = rpcVersion(params[jss::ripplerpc].asString()); + if (!parsed) + { + usage.charge(resource::kFeeMalformedRpc); + if (!reject( + 400, rpc::kJsonRpcInvalidParams, "ripplerpc is not a supported version")) + return; + continue; + } + envelope = *parsed; } /** @@ -905,58 +1033,9 @@ ServerHandler::processRequest( if (usage.warn()) result[jss::warning] = jss::load; - json::Value r(json::ValueType::Object); - if (ripplerpc >= "2.0") - { - if (result.isMember(jss::error)) - { - result[jss::status] = jss::error; - result["code"] = result[jss::error_code]; - result["message"] = result[jss::error_message]; - result.removeMember(jss::error_message); - JLOG(journal_.debug()) - << "rpcError: " << result[jss::error] << ": " << result[jss::error_message]; - r[jss::error] = std::move(result); - } - else - { - result[jss::status] = jss::success; - r[jss::result] = std::move(result); - } - } - else - { - // Always report "status". On an error report the request as - // received. - if (result.isMember(jss::error)) - { - result[jss::status] = jss::error; - result[jss::request] = rpc::maskSecrets(params); - - JLOG(journal_.debug()) - << "rpcError: " << result[jss::error] << ": " << result[jss::error_message]; - } - else - { - result[jss::status] = jss::success; - } - r[jss::result] = std::move(result); - } - - if (params.isMember(jss::jsonrpc)) - r[jss::jsonrpc] = params[jss::jsonrpc]; - if (params.isMember(jss::ripplerpc)) - r[jss::ripplerpc] = params[jss::ripplerpc]; - if (params.isMember(jss::id)) - r[jss::id] = params[jss::id]; - if (batch) - { - reply.append(std::move(r)); - } - else - { - reply = std::move(r); - } + int const status = shapeReply(envelope, std::move(result), params, batch, reply, journal_); + if (!batch) + httpStatus = status; if (reply.isMember(jss::result) && reply[jss::result].isMember(jss::result)) { @@ -969,25 +1048,6 @@ ServerHandler::processRequest( } } - // If we're returning an error_code, use that to determine the HTTP status. - int const httpStatus = [&reply] { - // This feature is enabled with ripplerpc version 3.0 and above. - // Before ripplerpc version 3.0 always return 200. - if (reply.isMember(jss::ripplerpc) && reply[jss::ripplerpc].isString() && - reply[jss::ripplerpc].asString() >= "3.0") - { - // If there's an error_code, use that to determine the HTTP Status. - if (reply.isMember(jss::error) && reply[jss::error].isMember(jss::error_code) && - reply[jss::error][jss::error_code].isInt()) - { - int const errCode = reply[jss::error][jss::error_code].asInt(); - return legacyHttpStatus(static_cast(errCode)); - } - } - // Return OK. - return 200; - }(); - auto response = to_string(reply); rpcTime_.notify(