ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-10-11 06:08:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Keep the connection's identity for every entry of a batch

Browse Source 
`X-User` and the forwarded-for address are assigned by the connection's
header, so they belong to the connection rather than to one entry of a
`method: "batch"` body. The loop clears them in place for an entry whose
own role is neither identified nor proxied, and a `std::string_view`
shortened in place stays shortened, so the first such entry decides them
for every entry after it. A later entry's role is read from that same
value, so it is demoted along with the username.

Read them into two entry-scoped views instead and hand those to the
handler's context, so the clearing reaches only the entry it belongs to. A
lone request is unaffected, there being no entry after it.

What a client can observe: a body whose first entry presents admin
credentials made the second report no username, where the same entry sent
alone reports the connection's. Every entry of one body now reports the
role and username it would report alone.
This commit is contained in:
Bart
2026-09-20 10:45:16 +02:00
parent 373e1dc4c2
commit 96d8be6d83
3 changed files with 79 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
API-CHANGELOG.md
View File

@@ -38,6 +38,7 @@ This version is supported by all `xrpld` versions. For WebSocket and HTTP JSON-R
- 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.
- A body the server rejects before it reads a request out of it now says which of four things was wrong. A body over the size limit answers `Request is too large`. A body that parses to `{}`, `[]` or `null` answers `Request is empty`. A body that parses to a non-empty array answers `Request is not a JSON object`; that is the only other document the parser accepts at the top level. All three previously answered `Unable to parse request: ` with nothing after the colon, the parser having recorded no error for them. Any other body does not parse, which includes one that is only whitespace and one whose top-level value is a string, number or boolean; it answers `Unable to parse request: ` followed by the parser's own reason, as it did before. The status is 400 for all four, as before, and this reaches every API version.
- `batch`: An entry that is not identified through a secure gateway no longer clears the connection's `X-User` and forwarded-for values for the entries after it, so every entry of one body reports the role and username it would have reported on its own.
## XRP Ledger server version 3.5.0

73
src/test/server/ServerStatus_test.cpp
View File

@@ -134,6 +134,17 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
return req;
}
/**
* Build an HTTP/1.1 request for the server's root. It is a GET when `body`
* is empty and otherwise a JSON POST carrying it.
*
* @param host The host name or address the `Host` header names.
* @param port The port the `Host` header names.
* @param body The request body, empty for a GET.
* @param fields Headers inserted by name, so a name Beast has no
* enumerator for, such as `X-User`, is carried too.
* @return The request, with its payload prepared.
*/
static auto
makeHTTPRequest(
std::string const& host,
@@ -147,8 +158,10 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
req.target("/");
req.version(11);
// By name: `name()` answers `field::unknown` for a header Beast has no enumerator for,
// such as `X-User`, and inserting that asserts.
for (auto const& f : fields)
req.insert(f.name(), f.value());
req.insert(f.name_string(), f.value());
req.insert("Host", host + ":" + std::to_string(port));
req.insert("User-Agent", "test");
if (body.empty())
@@ -1648,6 +1661,63 @@ class ServerStatus_test : public beast::unit_test::Suite, public beast::test::En
}
}
/**
* Header-assigned values belong to the connection, not to one entry of a
* batch, so an entry whose own role is neither identified nor proxied
* must not clear them for the entries after it.
*
* @param yield The coroutine the requests run on.
*/
void
testBatchIdentity(boost::asio::yield_context& yield)
{
testcase("A batch entry keeps the connection's identity");
using namespace test::jtx;
// Both an admin net and a secure gateway, so one entry can be admin while the next is
// identified by the header.
Env env{*this, envconfig([](std::unique_ptr<Config> cfg) {
(*cfg)[Sections::kPortRpc].set(Keys::kAdminUser, "u");
(*cfg)[Sections::kPortRpc].set(Keys::kAdminPassword, "p");
(*cfg)[Sections::kPortRpc].set(Keys::kSecureGateway, getEnvLocalhostAddr());
return cfg;
})};
boost::system::error_code ec;
MyFields fields;
fields.insert("X-User", "xrposhi");
fields.insert("X-Forwarded-For", "203.0.113.9");
// The first entry is admin, which is the role the clearing keys off; the second presents
// no credentials and keeps the connection's `X-User` and forwarded-for address.
json::Value credentials(json::ValueType::Object);
credentials["admin_user"] = "u";
credentials["admin_password"] = "p";
json::Value batch;
batch[jss::method] = "batch";
batch[jss::params] = json::ValueType::Array;
batch[jss::params][0u][jss::method] = "ping";
batch[jss::params][0u][jss::params] = json::ValueType::Array;
batch[jss::params][0u][jss::params][0u] = credentials;
batch[jss::params][1u][jss::method] = "ping";
Response resp;
auto const reply = postAndParse(env, yield, resp, ec, to_string(batch), {}, fields);
BEAST_EXPECT(resp.result() == kOk);
BEAST_EXPECT(reply.isArray() && reply.size() == 2);
BEAST_EXPECT(reply[0u][jss::result][jss::role] == "admin");
// The second entry reports what it reports as a request of its own.
auto const& ping = reply[1u][jss::result];
BEAST_EXPECT(ping[jss::role] == "identified");
BEAST_EXPECT(ping["username"] == "xrposhi");
BEAST_EXPECT(ping[jss::ip] == "203.0.113.9");
}
/**
* The five handlers that report a bare token carry a code and message with
* it.
@@ -2435,6 +2505,7 @@ public:
testPrivilegedRequestIsNotShed(yield);
testLegacyBatchEntryRejections(yield);
testAnErrorReplyDoesNotFollowTheLogLevel(yield);
testBatchIdentity(yield);
testHandlerErrorsCarryCodes(yield);
testGainedStatusesStayOffLegacyEnvelope(yield);
testUncommonHttpStatus(yield);

16
src/xrpld/rpc/detail/ServerHandler.cpp
View File

@@ -1001,15 +1001,11 @@ ServerHandler::processRequest(
envelope = *parsed;
}
/**
* Clear header-assigned values if not positively identified from a
* secureGateway.
*/
if (role != Role::IDENTIFIED && role != Role::PROXY)
{
forwardedFor.remove_suffix(forwardedFor.size());
user.remove_suffix(user.size());
}
// Header-assigned values belong to the connection, so an entry not identified from a
// secureGateway drops them for itself only rather than clearing them in place.
bool const identified = role == Role::IDENTIFIED || role == Role::PROXY;
std::string_view const entryForwardedFor = identified ? forwardedFor : std::string_view{};
std::string_view const entryUser = identified ? user : std::string_view{};
JLOG(journal_.debug()) << "Query: " << strMethod << rpc::loggable(params);
@@ -1031,7 +1027,7 @@ ServerHandler::processRequest(
.infoSub = InfoSub::pointer(),
.apiVersion = apiVersion},
params,
{.user = user, .forwardedFor = forwardedFor}};
{.user = entryUser, .forwardedFor = entryForwardedFor}};
json::Value result;
auto start = std::chrono::system_clock::now();