diff --git a/src/test/rpc/RPCHandler_test.cpp b/src/test/rpc/RPCHandler_test.cpp new file mode 100644 index 0000000000..d4a9a7c232 --- /dev/null +++ b/src/test/rpc/RPCHandler_test.cpp @@ -0,0 +1,176 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace xrpl::test { + +/** + * Checks the error a busy server reports for a request it never dispatches. + * + * RPCHandler_test ──doCommand()──> RPC::fillHandler() + * │ │ + * └── fills ──> JobQueue <── reads ─┘ + * + * An overloaded server answers rpcTOO_BUSY before it reads the command name, so + * the request fields still hold whatever json type the client sent. Anything + * that runs afterwards to describe the error has to cope with that and leave + * the answer alone. + * + * @note Each testcase keeps one job-queue worker blocked for as long as it + * runs, and releases it before returning. + */ +class RPCHandler_test : public beast::unit_test::Suite +{ + /** + * How many jobs to queue to hold the server over its overload threshold. + * One job is dispatched straight away, so one spare keeps the waiting + * count above the limit. + */ + static constexpr int kOverloadJobs = rpc::tuning::kMaxJobQueueClients + 2; + + /** + * Dispatches one request on an overloaded server and checks the client is + * told the server is busy. + * + * @param params Request fields, in the form fillHandler() reads them. + */ + void + expectTooBusy(json::Value const& params) + { + using namespace jtx; + Env env{*this}; + auto& app = env.app(); + + // Only one job of this type runs at a time, so every job after the + // first stays queued until the gate opens. They also sort above + // JtClient, the priority the overload check counts from. + std::promise gate; + std::shared_future const open = gate.get_future().share(); + ScopeExit const openGate{[&gate]() { gate.set_value(); }}; + + int queued = 0; + for (int i = 0; i < kOverloadJobs; ++i) + { + if (app.getJobQueue().addJob(JtSweep, "overload", [open]() { open.wait(); })) + ++queued; + } + BEAST_EXPECT(queued == kOverloadJobs); + BEAST_EXPECT(app.getJobQueue().getJobCountGE(JtClient) > rpc::tuning::kMaxJobQueueClients); + + resource::Charge loadType = resource::kFeeReferenceRpc; + resource::Consumer consumer; + rpc::JsonContext context{ + {.j = env.journal, + .app = app, + .loadType = loadType, + .netOps = app.getOPs(), + .ledgerMaster = app.getLedgerMaster(), + .consumer = consumer, + .role = Role::USER, + .coro = {}, + .infoSub = {}, + .apiVersion = rpc::kApiVersionIfUnspecified}, + params, + {}}; + + json::Value result; + rpc::Status status; + std::string thrown; + try + { + status = rpc::doCommand(context, result); + } + catch (std::exception const& e) + { + thrown = e.what(); + } + + if (BEAST_EXPECTS(thrown.empty(), "doCommand threw: " + thrown)) + { + BEAST_EXPECT(status.type() == rpc::Status::Type::ErrorCodeI); + BEAST_EXPECT(status.toErrorCode() == RpcTooBusy); + BEAST_EXPECT(result[jss::error].asString() == "tooBusy"); + BEAST_EXPECT(result[jss::error_code].asInt() == static_cast(RpcTooBusy)); + } + } + + /** + * Checks a well-formed request on an overloaded server. This is the control + * for the two cases below: it shares their fixture and their assertions, + * and differs only in that every field it sends is a string. + */ + void + testRegisteredCommand() + { + testcase("Busy server, registered command"); + + json::Value params = json::ValueType::Object; + params[jss::command] = "ping"; + expectTooBusy(params); + } + + /** + * Checks a request whose "method" field is not a string. + */ + void + testNonStringMethod() + { + testcase("Busy server, method field is not a string"); + + // The HTTP path sets "command" from the outer method name it has + // already checked, and passes the inner request object through + // untouched, so "method" can arrive holding any json type. + json::Value params = json::ValueType::Object; + params[jss::command] = "ping"; + params[jss::method] = json::ValueType::Array; + expectTooBusy(params); + } + + /** + * Checks a request whose "command" field is not a string. + */ + void + testNonStringCommand() + { + testcase("Busy server, command field is not a string"); + + json::Value params = json::ValueType::Object; + params[jss::command] = json::ValueType::Object; + expectTooBusy(params); + } + +public: + void + run() override + { + testRegisteredCommand(); + testNonStringMethod(); + testNonStringCommand(); + } +}; + +BEAST_DEFINE_TESTSUITE(RPCHandler, rpc, xrpl); + +} // namespace xrpl::test diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index dc026c8aa2..dc362f1762 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -235,30 +235,40 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, // Resolve the span suffix / command attribute for a request that failed in // fillHandler. Returns the canonical handler name for a recognized command // (a finite, bounded set) or the literal "unknown" for a request that omits -// both fields or names an unregistered command. The raw request value is -// deliberately NOT used: the command attribute is promoted to a Prometheus -// label by the spanmetrics connector, so an attacker-controlled string would -// let arbitrary request input drive unbounded span-name / label cardinality. +// both fields, supplies one that is not a string, or names an unregistered +// command. The raw request value is deliberately NOT used: the command +// attribute is promoted to a Prometheus label by the spanmetrics connector, so +// an attacker-controlled string would let arbitrary request input drive +// unbounded span-name / label cardinality. // Resolving against the registry keeps per-command error attribution for real // commands (e.g. a submit rejected with rpcTOO_BUSY stays rpc.command.submit) // while collapsing garbage input to a single series. std::string_view resolveCommandSpanName(JsonContext const& context) { - if (!context.params.isMember(jss::command) && !context.params.isMember(jss::method)) + bool const hasCommand = context.params.isMember(jss::command); + bool const hasMethod = context.params.isMember(jss::method); + + if (!hasCommand && !hasMethod) + return rpc_span::val::unknownCommand; + + // A json array or object throws when asked for its string value, and no + // non-string field names a handler. The reply's error code is already + // decided, so naming the span must not be able to change it. + if ((hasCommand && !context.params[jss::command].isString()) || + (hasMethod && !context.params[jss::method].isString())) return rpc_span::val::unknownCommand; // fillHandler() rejects a request that supplies both fields with differing // values as rpcUNKNOWN_COMMAND. Mirror that here, or the span would be // labelled with one of the two names and misattribute the error to a // command that was never dispatched. - if (context.params.isMember(jss::command) && context.params.isMember(jss::method) && + if (hasCommand && hasMethod && context.params[jss::command].asString() != context.params[jss::method].asString()) return rpc_span::val::unknownCommand; - std::string const cmd = context.params.isMember(jss::command) - ? context.params[jss::command].asString() - : context.params[jss::method].asString(); + std::string const cmd = hasCommand ? context.params[jss::command].asString() + : context.params[jss::method].asString(); auto const* handler = getHandler(context.apiVersion, context.app.config().betaRpcApi, cmd); return (handler != nullptr) ? std::string_view{handler->name}