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 fc58d81af8..1efcea372d 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -158,6 +158,42 @@ fillHandler(JsonContext& context, Handler const*& result) return RpcSuccess; } +/** + * Names the reason a command failed, for the span's error description. + * + * jss::error holds the error token, and an old-style handler reports it there + * and nowhere else. A failure the reply does not name is described by the + * status's own error code, which is the only reason left to report. Every + * token is a compile-time string, so no request text reaches the description. + * + * @param status What the handler returned. + * @param result The reply the handler filled in. The returned view can point + * into it, so result must outlive the view. + * @param replyHasError The caller's containsError(result), passed in so the + * reply is not searched twice. + * @return The error token, or "error" where neither source carries one. + */ +std::string_view +errorDescription(Status const& status, json::Value const& result, bool replyHasError) +{ + if (replyHasError && result[jss::error].isString()) + { + // asCString() asserts the type, then hands back the stored pointer + // unchecked, and a string-typed json::Value may hold a null one. Both + // checks are needed before that pointer becomes a view. + if (char const* const token = result[jss::error].asCString(); token != nullptr) + return token; + } + + // A TER or a bare integer code has no token in the error registry, so + // reading one would name an unrelated error. getErrorInfo() returns a + // reference into a static table, so its token outlives this call. + if (status.type() == Status::Type::ErrorCodeI) + return getErrorInfo(status.toErrorCode()).token.cStr(); + + return rpc_span::val::error; +} + Status callMethod(JsonContext& context, Handler::Method method, std::string_view name, json::Value& result) { @@ -190,23 +226,33 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, JLOG(context.j.debug()) << "RPC call " << name << " completed in " << ((end - start).count() / 1000000000.0) << "seconds"; perfLog.rpcFinish(name, curId); - span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str()); - // Status::operator bool() returns true when there IS an error - // (code_ != OK), so the ternary correctly maps error->error, ok->success. - span.setAttribute( - rpc_span::attr::rpcStatus, - ret ? std::string_view{rpc_span::val::error} - : std::string_view{rpc_span::val::success}); - // Reflect the result in the OTel span status, not just the attribute, - // so non-exception RPC errors (rpcTOO_BUSY, rpcNO_PERMISSION, ...) are - // visible to {status.code=error} queries. - if (ret) + // Everything in here only feeds the span, and searching the reply is + // not free, so a null guard pays for none of it. setError() and + // setAttribute() are no-ops on a null guard, but their arguments are + // not: with telemetry compiled out operator bool() is a constant false. + if (span) { - span.setError(rpc_span::val::error); - } - else - { - span.setOk(); + // Read after the handler ran, because a handler may raise its own + // load type (pathfind charges a heavy burden). + span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str()); + // An old-style handler reports its error in the reply, not in the + // Status: byRef() returns a default Status whatever happened. + // Reading both covers every handler. Status::operator bool() is + // true when there IS an error. + bool const replyHasError = containsError(result); + bool const failed = static_cast(ret) || replyHasError; + // Two values only. rpc_status is a spanmetrics dimension, so every + // value it can take becomes a Prometheus label and a metric series. + span.setAttribute( + rpc_span::attr::rpcStatus, + failed ? std::string_view{rpc_span::val::error} + : std::string_view{rpc_span::val::success}); + // Error so a failed call answers {status.code=error}, for the codes + // that never throw (rpcTOO_BUSY, rpcNO_PERMISSION, ...). Success + // stays Unset: the spec reserves Ok for an operator asserting + // verified success, and a tool may read it as suppressing errors. + if (failed) + span.setError(errorDescription(ret, result, replyHasError)); } return ret; } @@ -234,32 +280,37 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, #ifdef XRPL_ENABLE_TELEMETRY // 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. -// 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. +// fillHandler. The name comes from the handler registry, so only a registered +// handler name or the "unknown" label can reach the span; request text never +// does. That bounded set also bounds the Prometheus label the spanmetrics +// connector derives from it, and a real command still keeps its own error +// attribution: a submit rejected with rpcTOO_BUSY stays rpc.command.submit. 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} diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 9c3168cb5c..842193eeef 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -391,6 +391,10 @@ ServerHandler::onWSMessage( // Fresh root so each WS message is its own trace. auto span = ScopedSpanGuard::freshRoot( TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); + // rpc_status is a span-metrics dimension, so leaving it unset emits a + // series with a blank label and hides this failure from any query that + // selects on error. + span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error); span.setError(rpc_span::val::invalidJson); json::Value jvResult(json::ValueType::Object);