From c587cf5edf8287a2d89ec44c23707d641969d5cb Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:06:07 +0100 Subject: [PATCH 1/4] fix(rpc): Do not let span naming change the RPC error a client sees resolveCommandSpanName() converted command/method to a string with no type check. json::Value::asString() throws for an array or an object, so a request whose nested method is [] reached that conversion and the throw replaced a clean tooBusy reply with internal. The overloaded path is the only way in: fillHandler() returns tooBusy before anything has read those fields, and every other exit either converted them itself or means neither field is present. The effect is that the error code a client receives depends on whether telemetry was compiled in, which telemetry must never do. The span now falls back to its existing unknown-command label when either present field is not a string. The WebSocket path already validates both fields before dispatch, so it is left alone. The test drives a genuinely overloaded job queue, reading the threshold from the production constant rather than copying it, and asserts the client still gets tooBusy. It lives in the Beast tree because doCommand is daemon code and needs jtx, which the gtest binary cannot reach. --- src/test/rpc/RPCHandler_test.cpp | 176 ++++++++++++++++++++++++++++ src/xrpld/rpc/detail/RPCHandler.cpp | 28 +++-- 2 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 src/test/rpc/RPCHandler_test.cpp 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} From a2ee20b88f160bde4315b0a8578a29844d6d9286 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:22:15 +0100 Subject: [PATCH 2/4] fix(telemetry): Read the RPC span status from the reply, not the Status callMethod decided the rpc.command span's status from the Status the handler returned. Handler.cpp registers 70 of its 72 methods through byRef(), which returns a default Status whatever happened, because an old-style handler reports its error in the reply body instead. So a failed account_info or a refused path_find came out with rpc_status=success and span status Ok. That is the opposite of what the comment above the code claimed it did, and it left a {status.code=error} query blind to every non-throwing RPC error. The status now comes from the reply as well as the Status, which covers both handler styles. byRef() is unchanged and identical to develop: it behaves correctly for its own purpose, and no RPC reply changes. Only telemetry was reading the wrong signal. setOk() is dropped rather than moved. The specification reserves Ok for an operator asserting verified success and warns that a tool may treat it as suppressing errors, so a successful call now leaves the status Unset. No test accompanies this: the Beast tree has no telemetry fixture and the in-memory span exporter is linked only into xrpl_tests, which cannot reach daemon code. Asserting the exported status needs that infrastructure first. --- src/xrpld/rpc/detail/RPCHandler.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index dc362f1762..ca163b22af 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -190,23 +190,21 @@ 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); - // Status::operator bool() returns true when there IS an error - // (code_ != OK), so the ternary correctly maps error->error, ok->success. + // 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 failed = static_cast(ret) || containsError(result); 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) - { + 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(rpc_span::val::error); - } - else - { - span.setOk(); - } return ret; } catch (std::exception& e) From 89fc3ba450f5bd3757fd21ed99abb4132137305b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:29:58 +0100 Subject: [PATCH 3/4] fix(telemetry): Name the reason an RPC failed in the span status The status description was the fixed string error, so a trace recorded that a request failed but not why. It now carries the error token from the reply, falling back to the status's own error code and then to the old string. Every source is a compile-time literal from the error registry, so no request text reaches it. The rpc_status attribute stays at success and error: that one is a span-metrics dimension, and widening it would mint a series per token per command per node. The work is gated on the span being live, so a build with telemetry compiled out or a disabled guard pays nothing, matching how the neighbouring helper is handled. asCString() is null-checked as well as type-checked, because it returns the raw pointer where asString() guards it. The comment above resolveCommandSpanName now states the invariant rather than how it could be abused. --- src/xrpld/rpc/detail/RPCHandler.cpp | 91 +++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 25 deletions(-) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index ca163b22af..363938367a 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,21 +226,31 @@ 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); - // 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 failed = static_cast(ret) || containsError(result); - 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(rpc_span::val::error); + // 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) + { + // 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; } catch (std::exception& e) @@ -231,16 +277,11 @@ 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, 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. +// 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) { From 35db8610cc46064a13c5e658c0c101a74318ab0e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:21 +0100 Subject: [PATCH 4/4] fix(telemetry): Set rpc_status on the invalid-JSON websocket span rpc_status is a span-metrics dimension, so leaving it unset on this path emitted a series with a blank label. Any query selecting on error missed the failure entirely. Co-Authored-By: Claude Opus 5 (1M context) --- src/xrpld/rpc/detail/ServerHandler.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 20587e53e5..325c9c1224 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -352,6 +352,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);