fix: Select the reply envelope from a ripplerpc the server supports

The `ripplerpc` version is read in three places: twice inside the dispatch
loop to pick the error shape, and once after it to derive the HTTP status
by re-reading the version off the finished reply. Replace all three with
one `shapeReply` keyed by an `RpcVersion` enum naming the three envelopes.
The version is read once per request and passed in, and the status is
returned rather than read back, since only the function that wrote the
shape knows where the code went.

The version is now matched exactly against the three valid spellings, where
the `ripplerpc` string was compared with `>=` against "2.0" and "3.0", so
"abc" read as version 3 and "10.0" as version 1. Junk from an
unauthenticated client therefore chose an envelope, and at version 3 chose
real HTTP error codes. Anything but the three exact values is rejected with
a 400, the malformed-RPC fee and -32602, which is a break for API versions
1 and 2 on the JSON-RPC transport, listed under a breaking-changes heading
of its own in the changelog.

Hoisting the log statement above the shape branch fixes a second defect. It
read `error_message` after the version 2 and 3 branch renamed that member
to `message`, and reading a missing member non-const puts it back as an
explicit null, so an error reply carried `"error_message": null` exactly
when the `Server` log partition wrote at debug.

.github/scripts/levelization/results/ordering.txt

@@ -177,6 +177,7 @@ test.server > xrpld.core
 test.server > xrpld.rpc
 test.server > xrpl.json
 test.server > xrpl.protocol
+test.server > xrpl.resource
 test.server > xrpl.server
 test.unit_test > xrpl.basics
 test.unit_test > xrpl.protocol

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. An `[rpc_startup]` line that does not parse is reported by its position in the section and the parser's message, never by its text, and no command runs for it.
@@ -31,6 +35,8 @@ This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-R
 - A WebSocket frame that does not parse, or exceeds the request size limit, is answered `{"type": "error", "error": "jsonInvalid", "size": <bytes>}`. The frame's body is reported by size rather than echoed back in a `value` member, since a body that does not parse has no fields to mask. A client that read `value` gets `size` instead.
 - 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
 

include/xrpl/protocol/JsonRpc.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <xrpl/json/json_forwards.h>
+
 #include <string_view>
 
 namespace xrpl::rpc {
@@ -17,4 +19,25 @@ namespace xrpl::rpc {
  */
 inline constexpr std::string_view kJsonRpcVersion{"2.0"};
 
+/**
+ * Codes for the `code` member of a JSON-RPC error object.
+ *
+ * The specification reserves -32768 to -32000 for the protocol and leaves
+ * -32000 to -32099 of it to the implementation.
+ *
+ * kJsonRpcServerError is the code for an error an XRPL handler reports.
+ *
+ * The codes from kJsonRpcServerOverloaded on lie outside the
+ * implementation-defined sub-range, which the specification does not allow.
+ * They are the codes every shipped version reports, so moving one breaks the
+ * clients matching on it.
+ */
+inline constexpr json::Int kJsonRpcServerError{-32000};
+inline constexpr json::Int kJsonRpcInvalidRequest{-32600};
+inline constexpr json::Int kJsonRpcMethodNotFound{-32601};
+inline constexpr json::Int kJsonRpcInvalidParams{-32602};
+inline constexpr json::Int kJsonRpcServerOverloaded{-32604};
+inline constexpr json::Int kJsonRpcForbidden{-32605};
+inline constexpr json::Int kJsonRpcWrongVersion{-32606};
+
 }  // namespace xrpl::rpc

include/xrpl/server/detail/JSONRPCUtil.h

@@ -13,6 +13,8 @@ namespace xrpl {
  * credential this library cannot mask, so the caller logs it masked.
  *
  * A 401 with an empty body is answered with the fixed authentication page.
+ * The status line carries the phrase Beast's registry gives @p nStatus, except
+ * for 401 and 503, which carry a phrase of this server's own.
  *
  * @param nStatus The HTTP status code.
  * @param strMsg The body.

src/libxrpl/server/JSONRPCUtil.cpp

@@ -6,7 +6,10 @@
 #include <xrpl/protocol/BuildInfo.h>
 #include <xrpl/protocol/SystemParameters.h>
 
+#include <boost/beast/http/status.hpp>
+
 #include <ctime>
+#include <format>
 #include <string>
 
 namespace xrpl {
@@ -72,42 +75,29 @@ httpReply(int nStatus, std::string const& content, json::Output const& output, b
         return;
     }
 
-    // NOLINTNEXTLINE(bugprone-switch-missing-default-case)
     switch (nStatus)
     {
+        // The status every successful reply carries, so a literal rather than a format call.
         case 200:
             output("HTTP/1.1 200 OK\r\n");
             break;
-        case 202:
-            output("HTTP/1.1 202 Accepted\r\n");
-            break;
-        case 400:
-            output("HTTP/1.1 400 Bad Request\r\n");
-            break;
+        // Two statuses this server phrases itself rather than taking from the registry.
         case 401:
             output("HTTP/1.1 401 Authorization Required\r\n");
             break;
-        case 403:
-            output("HTTP/1.1 403 Forbidden\r\n");
-            break;
-        case 404:
-            output("HTTP/1.1 404 Not Found\r\n");
-            break;
-        case 405:
-            output("HTTP/1.1 405 Method Not Allowed\r\n");
-            break;
-        case 429:
-            output("HTTP/1.1 429 Too Many Requests\r\n");
-            break;
-        case 500:
-            output("HTTP/1.1 500 Internal Server Error\r\n");
-            break;
-        case 501:
-            output("HTTP/1.1 501 Not Implemented\r\n");
-            break;
         case 503:
             output("HTTP/1.1 503 Server is overloaded\r\n");
             break;
+        default:
+            // A reply whose first line is a header is not an HTTP response. Beast knows the whole
+            // registry, so a status the error table gains needs no case here.
+            output(
+                std::format(
+                    "HTTP/1.1 {} {}\r\n",
+                    nStatus,
+                    boost::beast::http::obsolete_reason(
+                        static_cast<boost::beast::http::status>(nStatus))));
+            break;
     }
 
     output(getHTTPHeaderTimestamp());

src/test/server/ServerStatus_test.cpp

@@ -5,11 +5,13 @@
 #include <test/jtx/WSClient.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/envconfig.h>
+#include <test/jtx/pay.h>
 
 #include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/rpc/detail/MaskSecrets.h>
 
 #include <xrpl/basics/base64.h>
+#include <xrpl/beast/net/IPEndpoint.h>
 #include <xrpl/beast/test/yield_to.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/beast/utility/Journal.h>
@@ -19,8 +21,13 @@
 #include <xrpl/json/to_string.h>
 #include <xrpl/protocol/ApiVersion.h>
 #include <xrpl/protocol/ErrorCodes.h>
+#include <xrpl/protocol/JsonRpc.h>
 #include <xrpl/protocol/Seed.h>
 #include <xrpl/protocol/jss.h>
+#include <xrpl/resource/Charge.h>
+#include <xrpl/resource/Consumer.h>
+#include <xrpl/resource/ResourceManager.h>
+#include <xrpl/resource/detail/Tuning.h>
 #include <xrpl/server/LoadFeeTrack.h>
 #include <xrpl/server/NetworkOPs.h>
 
@@ -249,6 +256,25 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
         return reply;
     }
 
+    /**
+     * Charges the inbound resource entry of @p ip past the drop threshold.
+     *
+     * The balance sums charges over the decay window, so one large charge
+     * stands in for thousands of requests.
+     *
+     * @param env The environment whose resource manager holds the entry.
+     * @param ip The address the entry is keyed by.
+     */
+    static void
+    overloadEndpoint(test::jtx::Env& env, std::string const& ip)
+    {
+        auto usage =
+            env.app().getResourceManager().newInboundEndpoint(beast::ip::Endpoint::fromString(ip));
+        usage.charge(
+            resource::Charge{
+                2 * resource::kDropThreshold * resource::kDecayWindowSeconds, "test overload"});
+    }
+
     void
     doWSRequest(
         test::jtx::Env& env,
@@ -1254,6 +1280,332 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
         return to_string(jv);
     }
 
+    /**
+     * An overloaded endpoint sheds a request unless the connection holds a
+     * privileged role, whose resource entry is exempt.
+     *
+     * @param yield The coroutine the requests run on.
+     */
+    void
+    testPrivilegedRequestIsNotShed(boost::asio::yield_context& yield)
+    {
+        testcase("A privileged request is not shed from an overloaded endpoint");
+
+        using namespace test::jtx;
+
+        boost::system::error_code ec;
+
+        json::Value ping;
+        ping[jss::method] = "ping";
+
+        // Without privilege the request is shed, which is what the case below contrasts.
+        {
+            Env env{*this, envconfig(noAdmin)};
+            auto const ip = env.app().config()[Sections::kPortRpc].get<std::string>(Keys::kIp);
+            // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
+            overloadEndpoint(env, *ip);
+
+            Response resp;
+            doHTTPRequest(env, yield, false, resp, ec, to_string(ping));
+            BEAST_EXPECT(resp.result() == boost::beast::http::status::service_unavailable);
+            BEAST_EXPECT(resp.body() == "Server is overloaded\r\n");
+        }
+
+        // With privilege it is answered, the connection holding an exempt resource entry.
+        {
+            Env env{*this};
+            auto const ip = env.app().config()[Sections::kPortRpc].get<std::string>(Keys::kIp);
+            // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
+            overloadEndpoint(env, *ip);
+
+            Response resp;
+            auto const reply = postAndParse(env, yield, resp, ec, to_string(ping));
+            BEAST_EXPECT(resp.result() == kOk);
+            BEAST_EXPECT(reply[jss::result][jss::status] == jss::success);
+        }
+    }
+
+    /**
+     * `ripplerpc` is refused when it is not a string and echoed back at every
+     * version the server serves. An entry naming an `api_version` the server
+     * cannot serve is returned under `request`.
+     *
+     * @param yield The coroutine the requests run on.
+     */
+    void
+    testRipplerpcVersions(boost::asio::yield_context& yield)
+    {
+        testcase("RPC client sends assorted ripplerpc versions");
+
+        using namespace test::jtx;
+        Env env{*this};
+
+        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;
+            json::Value jv;
+            jv[jss::method] = "ping";
+            jv[jss::params] = json::ValueType::Array;
+            jv[jss::params][0u] = json::ValueType::Object;
+            jv[jss::params][0u][jss::ripplerpc] = 2;
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == kBadRequest);
+            BEAST_EXPECT(resp.body() == "ripplerpc is not a string\r\n");
+        }
+
+        // Each supported version is accepted and echoed back.
+        for (auto const version :
+             {rpc::kRippleRpcVersion1, rpc::kRippleRpcVersion2, rpc::kRippleRpcVersion3})
+        {
+            Response resp;
+            auto const reply =
+                postAndParse(env, yield, resp, ec, makeRippleRpcRequest("ping", version), version);
+            BEAST_EXPECTS(resp.result() == kOk, version);
+            BEAST_EXPECTS(reply[jss::ripplerpc] == version, version);
+            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}] == "<masked>", 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.
+        {
+            Response resp;
+            json::Value entry(json::ValueType::Object);
+            entry[jss::method] = "ping";
+            entry[jss::api_version] = 99u;
+            entry[jss::secret] = "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(resp.result() == kOk);
+            BEAST_EXPECT(reply.isArray() && reply.size() == 1);
+
+            auto const& echoed = reply[0u][jss::request];
+            BEAST_EXPECT(echoed[jss::method] == "ping");
+            BEAST_EXPECT(echoed[jss::api_version] == 99u);
+            BEAST_EXPECT(echoed[jss::secret] == "<masked>");
+            BEAST_EXPECT(!reply[0u].isMember(jss::method));
+
+            auto const& error = reply[0u][jss::error][jss::error];
+            BEAST_EXPECT(error[jss::code] == rpc::kJsonRpcWrongVersion);
+            BEAST_EXPECT(error[jss::message] == jss::invalid_API_version);
+        }
+    }
+
+    /**
+     * Two `method: "batch"` entry rejections answer the shape shipped versions
+     * report. A null `method` reports `-32601`, which the specification would
+     * call an invalid request rather than a method it could not find; a lone
+     * request is answered with text, so only an entry carries the code at all.
+     * An entry that is not an object is echoed under `request` with the same
+     * code and `Method not found`.
+     *
+     * @param yield The coroutine the requests run on.
+     */
+    void
+    testLegacyBatchEntryRejections(boost::asio::yield_context& yield)
+    {
+        testcase("A rejected batch entry reports the code shipped versions report");
+
+        using namespace test::jtx;
+        Env env{*this};
+        boost::system::error_code ec;
+
+        // A null `method`: the entry is echoed with the error beside its own members.
+        {
+            json::Value entry(json::ValueType::Object);
+            entry[jss::method] = json::ValueType::Null;
+
+            json::Value jv;
+            jv[jss::method] = "batch";
+            jv[jss::params] = json::ValueType::Array;
+            jv[jss::params][0u] = entry;
+
+            Response resp;
+            auto const reply = postAndParse(env, yield, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == kOk);
+            BEAST_EXPECT(reply.isArray() && reply.size() == 1);
+
+            auto const& error = reply[0u][jss::error][jss::error];
+            BEAST_EXPECT(error[jss::code] == rpc::kJsonRpcMethodNotFound);
+            BEAST_EXPECT(error[jss::code] == -32601);
+            BEAST_EXPECT(error[jss::message] == "Null method");
+        }
+
+        // An entry that is not an object: echoed under `request`, having no members of its own.
+        {
+            json::Value jv;
+            jv[jss::method] = "batch";
+            jv[jss::params] = json::ValueType::Array;
+            jv[jss::params][0u] = 7;
+
+            Response resp;
+            auto const reply = postAndParse(env, yield, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == kOk);
+            BEAST_EXPECT(reply.isArray() && reply.size() == 1);
+            BEAST_EXPECT(reply[0u][jss::request] == 7);
+
+            auto const& error = reply[0u][jss::error][jss::error];
+            BEAST_EXPECT(error[jss::code] == rpc::kJsonRpcMethodNotFound);
+            BEAST_EXPECT(error[jss::message] == "Method not found");
+        }
+    }
+
+    /**
+     * 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<boost::beast::http::string_body> 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);
+        }
+    }
+
     /**
      * Four of the five handlers that report a bare token carry a code and
      * message with it: `transaction_entry`, `ledger_entry`, `submit` and
@@ -1542,6 +1894,54 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
         }
     }
 
+    /**
+     * Every reply begins with a status line, including one whose status the
+     * status-line switch does not spell out. `highFee` reports 402, one of
+     * the two statuses the switch names no case for, and a reply that begins
+     * with a header instead is not an HTTP response at all.
+     *
+     * @param yield The coroutine the request runs on.
+     */
+    void
+    testUncommonHttpStatus(boost::asio::yield_context& yield)
+    {
+        testcase("A reply names an HTTP status the status-line switch does not spell out");
+
+        using namespace test::jtx;
+        Env env{*this, envconfig([](std::unique_ptr<Config> cfg) {
+                    cfg->loadFromString(std::string("[") + Sections::kSigningSupport + "]\ntrue");
+                    return cfg;
+                })};
+
+        Account const alice{"alice"};
+        Account const bob{"bob"};
+        env.fund(XRP(10000), alice, bob);
+        env.close();
+
+        // A fee ceiling of zero covers no fee at all, which is what `highFee` reports. The
+        // `ripplerpc: "3.0"` envelope derives the HTTP status from the code.
+        json::Value params(json::ValueType::Object);
+        params[jss::ripplerpc] = rpc::kRippleRpcVersion3;
+        params[jss::secret] = toBase58(generateSeed("alice"));
+        params[jss::fee_mult_max] = 0;
+        params[jss::tx_json] = pay(alice, bob, XRP(1));
+
+        json::Value jv;
+        jv[jss::method] = "sign";
+        jv[jss::params] = json::ValueType::Array;
+        jv[jss::params][0u] = params;
+
+        Response resp;
+        boost::system::error_code ec;
+        auto const reply = postAndParse(env, yield, resp, ec, to_string(jv));
+
+        // The reply parsed as a response, which is what a missing status line breaks.
+        BEAST_EXPECT(!ec);
+        BEAST_EXPECT(resp.result_int() == rpc::errorCodeHttpStatus(RpcHighFee));
+        BEAST_EXPECT(resp.result() == boost::beast::http::status::payment_required);
+        BEAST_EXPECT(reply[jss::error][jss::error] == "highFee");
+    }
+
     /**
      * A credential the server echoes back is masked, on every path that echoes.
      *
@@ -2055,8 +2455,13 @@ public:
             testTheLoggedReplyIsMaskedOnlyWhenItCarriesACredential(yield);
             testInternalErrorIsReportedOnBothTransports(yield);
             testNoCredentialReachesTheLogAtTrace(yield);
+            testRipplerpcVersions(yield);
+            testPrivilegedRequestIsNotShed(yield);
+            testLegacyBatchEntryRejections(yield);
+            testAnErrorReplyDoesNotFollowTheLogLevel(yield);
             testHandlerErrorsCarryCodes(yield);
             testGainedStatusesStayOffLegacyEnvelope(yield);
+            testUncommonHttpStatus(yield);
             testStatusNotOkay(yield);
         });
 

src/tests/libxrpl/server/JSONRPCUtil.cpp

@@ -0,0 +1,105 @@
+#include <xrpl/server/detail/JSONRPCUtil.h>
+
+#include <xrpl/beast/utility/Journal.h>
+#include <xrpl/protocol/ErrorCodes.h>
+
+#include <gtest/gtest.h>
+
+#include <set>
+#include <string>
+#include <string_view>
+
+using namespace xrpl;
+
+namespace {
+
+/**
+ * The reply httpReply writes, collected as one string.
+ *
+ * @param status The HTTP status to write a reply for.
+ * @return The whole reply, headers and body.
+ */
+std::string
+reply(int status)
+{
+    std::string out;
+    beast::Journal const journal{beast::Journal::getNullSink()};
+    httpReply(status, "{}", [&out](std::string_view s) { out += s; }, journal);
+    return out;
+}
+
+/**
+ * The reply's status line, without the trailing CRLF.
+ *
+ * @param reply A whole HTTP reply.
+ * @return The first line, or the whole reply when it holds no CRLF.
+ */
+std::string_view
+statusLine(std::string const& reply)
+{
+    auto const end = reply.find("\r\n");
+    return std::string_view{reply}.substr(0, end == std::string::npos ? reply.size() : end);
+}
+
+}  // namespace
+
+TEST(JSONRPCUtil, status_line_names_the_status)
+{
+    // Every status this server sends but the two the last test names. The phrase comes from the
+    // registry but for the two below.
+    EXPECT_EQ(statusLine(reply(200)), "HTTP/1.1 200 OK");
+    EXPECT_EQ(statusLine(reply(202)), "HTTP/1.1 202 Accepted");
+    EXPECT_EQ(statusLine(reply(400)), "HTTP/1.1 400 Bad Request");
+    EXPECT_EQ(statusLine(reply(403)), "HTTP/1.1 403 Forbidden");
+    EXPECT_EQ(statusLine(reply(404)), "HTTP/1.1 404 Not Found");
+    EXPECT_EQ(statusLine(reply(405)), "HTTP/1.1 405 Method Not Allowed");
+    EXPECT_EQ(statusLine(reply(429)), "HTTP/1.1 429 Too Many Requests");
+    EXPECT_EQ(statusLine(reply(500)), "HTTP/1.1 500 Internal Server Error");
+    EXPECT_EQ(statusLine(reply(501)), "HTTP/1.1 501 Not Implemented");
+
+    // The two whose phrase is this server's own, not the registry's.
+    EXPECT_EQ(statusLine(reply(401)), "HTTP/1.1 401 Authorization Required");
+    EXPECT_EQ(statusLine(reply(503)), "HTTP/1.1 503 Server is overloaded");
+}
+
+TEST(JSONRPCUtil, every_status_the_error_table_names_gets_a_status_line)
+{
+    // httpReply sees a status, not the table, so only this can check every status the table names.
+    std::set<int> statuses;
+    for (int i = RpcBadSyntax; i <= RpcLast; ++i)
+    {
+        auto const& info = rpc::getErrorInfo(static_cast<ErrorCodeI>(i));
+
+        // Gaps in the table name no error, so they report no status of their own.
+        if (info.code == RpcUnknown)
+            continue;
+
+        statuses.insert(info.httpStatus);
+    }
+    EXPECT_FALSE(statuses.empty());
+
+    for (int const status : statuses)
+    {
+        auto const line = std::string{statusLine(reply(status))};
+        auto const expectedPrefix = "HTTP/1.1 " + std::to_string(status) + " ";
+        EXPECT_TRUE(line.starts_with(expectedPrefix)) << "status: " << status << ", line: " << line;
+
+        // A placeholder phrase means the table names something that is not a status.
+        EXPECT_GT(line.size(), expectedPrefix.size()) << "status: " << status;
+        EXPECT_EQ(line.find("unknown-status"), std::string::npos) << "status: " << status;
+    }
+}
+
+TEST(JSONRPCUtil, statuses_without_a_case_take_the_registry_phrase)
+{
+    // The two statuses the error table names and the switch spells no case for.
+    EXPECT_EQ(rpc::errorCodeHttpStatus(RpcHighFee), 402);
+    EXPECT_EQ(statusLine(reply(402)), "HTTP/1.1 402 Payment Required");
+
+    EXPECT_EQ(rpc::errorCodeHttpStatus(RpcDbDeserialization), 502);
+    EXPECT_EQ(statusLine(reply(502)), "HTTP/1.1 502 Bad Gateway");
+
+    // A number the registry does not know still gets a line, with the placeholder phrase the table
+    // test above refuses.
+    EXPECT_EQ(statusLine(reply(999)), "HTTP/1.1 999 <unknown-status>");
+}

src/xrpld/rpc/detail/ServerHandler.cpp

@@ -29,6 +29,7 @@
 #include <xrpl/protocol/ApiVersion.h>
 #include <xrpl/protocol/BuildInfo.h>
 #include <xrpl/protocol/ErrorCodes.h>
+#include <xrpl/protocol/JsonRpc.h>
 #include <xrpl/protocol/RPCErr.h>
 #include <xrpl/protocol/SystemParameters.h>
 #include <xrpl/protocol/jss.h>
@@ -67,6 +68,7 @@
 #include <map>
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <ostream>
 #include <string>
 #include <string_view>
@@ -597,10 +599,40 @@ makeJsonError(json::Int code, json::Value&& message)
     return r;
 }
 
-constexpr json::Int kMethodNotFound = -32601;
-constexpr json::Int kServerOverloaded = -32604;
-constexpr json::Int kForbidden = -32605;
-constexpr json::Int kWrongVersion = -32606;
+// 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>
+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.
@@ -627,6 +659,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<ErrorCodeI>(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,
@@ -670,16 +780,22 @@ 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)
     {
         json::Value const& jsonRPC = batch ? jsonOrig[jss::params][i] : jsonOrig;
 
+        // Only an entry of a batch can be a non-object; a lone request was checked before the loop.
+        // Inline rather than through `reject` below: an entry with no members carries the copy
+        // under `request` whatever a caller asks for.
         if (!jsonRPC.isObject())
         {
             json::Value r(json::ValueType::Object);
             r[jss::request] = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kMethodNotFound, "Method not found");
+            r[jss::error] = makeJsonError(rpc::kJsonRpcMethodNotFound, "Method not found");
             reply.append(r);
             continue;
         }
@@ -698,17 +814,45 @@ ServerHandler::processRequest(
             apiVersion = rpc::getAPIVersionNumber(jsonRPC, app_.config().betaRpcApi);
         }
 
+        // Answers a request rejected before it reaches a handler. A lone request receives the
+        // HTTP status and `message` as the whole body; a batch entry receives an error object
+        // beside a copy of the entry, one reply array having no other place to name the entry that
+        // failed. `wrapRequest` puts that copy under `request` instead.
+        //
+        // Returns whether the loop continues, which it does only for a batch.
+        auto const reject =
+            [&](int status, json::Int code, char const* message, bool wrapRequest = false) {
+                if (!batch)
+                {
+                    httpReply(status, message, output, rpcJ);
+                    return false;
+                }
+                json::Value r(json::ValueType::Object);
+                if (!wrapRequest)
+                {
+                    r = rpc::maskSecrets(jsonRPC);
+                }
+                else
+                {
+                    r[jss::request] = rpc::maskSecrets(jsonRPC);
+                }
+                r[jss::error] = makeJsonError(code, message);
+                reply.append(std::move(r));
+                return true;
+            };
+
         if (apiVersion == rpc::kApiInvalidVersion)
         {
-            if (!batch)
+            // An object-shaped rejection returns this entry under `request`, where a client
+            // correlating by `reply[i].request` finds it.
+            if (!reject(
+                    400,
+                    rpc::kJsonRpcWrongVersion,
+                    jss::invalid_API_version.cStr(),
+                    /*wrapRequest=*/true))
             {
-                httpReply(400, jss::invalid_API_version.cStr(), output, rpcJ);
                 return;
             }
-            json::Value r(json::ValueType::Object);
-            r[jss::request] = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kWrongVersion, jss::invalid_API_version.cStr());
-            reply.append(r);
             continue;
         }
 
@@ -732,54 +876,31 @@ ServerHandler::processRequest(
             role = requestRole(required, port, json::ValueType::Object, remoteIPAddress, user);
         }
 
-        resource::Consumer usage;
-        if (isUnlimited(role))
+        auto usage =
+            requestInboundEndpoint(resourceManager_, remoteIPAddress, role, user, forwardedFor);
+
+        // An overloaded server sheds the request without charging for it; disconnect() has already
+        // accounted for the load that got it here.
+        if (!isUnlimited(role) && usage.disconnect(journal_))
         {
-            usage = resourceManager_.newUnlimitedEndpoint(remoteIPAddress);
-        }
-        else
-        {
-            usage = resourceManager_.newInboundEndpoint(
-                remoteIPAddress, role == Role::PROXY, forwardedFor);
-            if (usage.disconnect(journal_))
-            {
-                if (!batch)
-                {
-                    httpReply(503, "Server is overloaded", output, rpcJ);
-                    return;
-                }
-                json::Value r = rpc::maskSecrets(jsonRPC);
-                r[jss::error] = makeJsonError(kServerOverloaded, "Server is overloaded");
-                reply.append(r);
-                continue;
-            }
+            if (!reject(503, rpc::kJsonRpcServerOverloaded, "Server is overloaded"))
+                return;
+            continue;
         }
 
         if (role == Role::FORBID)
         {
             usage.charge(resource::kFeeMalformedRpc);
-            if (!batch)
-            {
-                httpReply(403, "Forbidden", output, rpcJ);
+            if (!reject(403, rpc::kJsonRpcForbidden, "Forbidden"))
                 return;
-            }
-            json::Value r = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kForbidden, "Forbidden");
-            reply.append(r);
             continue;
         }
 
         if (!jsonRPC.isMember(jss::method) || jsonRPC[jss::method].isNull())
         {
             usage.charge(resource::kFeeMalformedRpc);
-            if (!batch)
-            {
-                httpReply(400, "Null method", output, rpcJ);
+            if (!reject(400, rpc::kJsonRpcMethodNotFound, "Null method"))
                 return;
-            }
-            json::Value r = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kMethodNotFound, "Null method");
-            reply.append(r);
             continue;
         }
 
@@ -787,14 +908,8 @@ ServerHandler::processRequest(
         if (!method.isString())
         {
             usage.charge(resource::kFeeMalformedRpc);
-            if (!batch)
-            {
-                httpReply(400, "method is not string", output, rpcJ);
+            if (!reject(400, rpc::kJsonRpcMethodNotFound, "method is not string"))
                 return;
-            }
-            json::Value r = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kMethodNotFound, "method is not string");
-            reply.append(r);
             continue;
         }
 
@@ -802,14 +917,8 @@ ServerHandler::processRequest(
         if (strMethod.empty())
         {
             usage.charge(resource::kFeeMalformedRpc);
-            if (!batch)
-            {
-                httpReply(400, "method is empty", output, rpcJ);
+            if (!reject(400, rpc::kJsonRpcMethodNotFound, "method is empty"))
                 return;
-            }
-            json::Value r = rpc::maskSecrets(jsonRPC);
-            r[jss::error] = makeJsonError(kMethodNotFound, "method is empty");
-            reply.append(r);
             continue;
         }
 
@@ -849,24 +958,29 @@ ServerHandler::processRequest(
             params = jsonRPC;
         }
 
-        std::string ripplerpc{rpc::kRippleRpcVersion1};
+        RpcVersion envelope = kRpcVersionIfUnspecified;
         if (params.isMember(jss::ripplerpc))
         {
+            // 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);
-                if (!batch)
-                {
-                    httpReply(400, "ripplerpc is not a string", output, rpcJ);
+                if (!reject(400, rpc::kJsonRpcMethodNotFound, "ripplerpc is not a string"))
                     return;
-                }
-
-                json::Value r = rpc::maskSecrets(jsonRPC);
-                r[jss::error] = makeJsonError(kMethodNotFound, "ripplerpc is not a string");
-                reply.append(r);
                 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;
         }
 
         /**
@@ -926,58 +1040,9 @@ ServerHandler::processRequest(
         if (usage.warn())
             result[jss::warning] = jss::load;
 
-        json::Value r(json::ValueType::Object);
-        if (ripplerpc >= rpc::kRippleRpcVersion2)
-        {
-            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))
         {
@@ -990,25 +1055,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() >= rpc::kRippleRpcVersion3)
-        {
-            // 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<ErrorCodeI>(errCode));
-            }
-        }
-        // Return OK.
-        return 200;
-    }();
-
     auto response = to_string(reply);
 
     rpcTime_.notify(