From 7be98d95de02ea387ba4744c92f7c0fae42086ee Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 29 Apr 2026 14:05:34 -0400 Subject: [PATCH] fix: Make assorted RPC fixes (#6529) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- API-CHANGELOG.md | 8 ++ src/test/app/PayChan_test.cpp | 22 +++- src/test/rpc/LedgerData_test.cpp | 10 ++ src/test/rpc/LedgerRPC_test.cpp | 17 +++ src/test/rpc/Roles_test.cpp | 17 +++ src/test/rpc/Submit_test.cpp | 98 +++++++++++++++++ src/test/rpc/Subscribe_test.cpp | 101 ++++++++++++------ src/xrpld/rpc/detail/Role.cpp | 8 +- src/xrpld/rpc/detail/TransactionSign.cpp | 6 +- .../rpc/handlers/account/AccountChannels.cpp | 4 + .../rpc/handlers/account/AccountInfo.cpp | 7 +- .../rpc/handlers/account/NoRippleCheck.cpp | 8 +- src/xrpld/rpc/handlers/ledger/Ledger.cpp | 62 ++++++++--- src/xrpld/rpc/handlers/ledger/LedgerData.cpp | 8 +- .../handlers/orderbook/GetAggregatePrice.cpp | 1 - .../rpc/handlers/subscribe/Subscribe.cpp | 4 +- src/xrpld/rpc/handlers/transaction/Submit.cpp | 18 +++- 17 files changed, 328 insertions(+), 71 deletions(-) create mode 100644 src/test/rpc/Submit_test.cpp diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index d5faaf70af..e98239d1ab 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -40,6 +40,14 @@ This section contains changes targeting a future version. - Peer Crawler: The `port` field in `overlay.active[]` now consistently returns an integer instead of a string for outbound peers. [#6318](https://github.com/XRPLF/rippled/pull/6318) - `ping`: The `ip` field is no longer returned as an empty string for proxied connections without a forwarded-for header. It is now omitted, consistent with the behavior for identified connections. [#6730](https://github.com/XRPLF/rippled/pull/6730) - gRPC `GetLedgerDiff`: Fixed error message that incorrectly said "base ledger not validated" when the desired ledger was not validated. [#6730](https://github.com/XRPLF/rippled/pull/6730) +- `account_channels`: The `destination_account` field now returns an error if the value is not a string. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `subscribe`: The `taker` field in the `books` array now returns an error if the value is not a string. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `account_info`: The `urlgravatar` field now uses HTTPS instead of HTTP. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `ledger`: The `full`, `accounts`, `transactions`, `expand`, `binary`, `owner_funds`, and `queue` fields now return an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `ledger_data`: The `binary` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `submit`: The `fail_hard` field now returns an error if the value is not a boolean. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- `subscribe`: The `taker` field in the `books` array now returns `actMalformed` instead of `badIssuer` if the value is not a valid account. [#6529](https://github.com/XRPLF/rippled/pull/6529) +- Fixed a bug in `Forwarded` HTTP header parsing where the extracted IP address could be incorrect when no comma or semicolon delimiter follows the address. This could cause the server to misidentify a client's IP address when operating behind a reverse proxy. [#6529](https://github.com/XRPLF/rippled/pull/6529) ## XRP Ledger server version 3.1.0 diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 1f4376a656..f69187c50f 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1019,7 +1019,8 @@ struct PayChan_test : public beast::unit_test::suite auto testInvalidAccountParam = [&](auto const& param) { Json::Value params; params[jss::account] = param; - auto jrr = env.rpc("json", "account_channels", to_string(params))[jss::result]; + auto const jrr = + env.rpc("json", "account_channels", to_string(params))[jss::result]; BEAST_EXPECT(jrr[jss::error] == "invalidParams"); BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'account'."); }; @@ -1031,6 +1032,25 @@ struct PayChan_test : public beast::unit_test::suite testInvalidAccountParam(Json::Value(Json::objectValue)); testInvalidAccountParam(Json::Value(Json::arrayValue)); } + { + // test destination_account non-string + auto testInvalidDestAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = alice.human(); + params[jss::destination_account] = param; + auto const jrr = + env.rpc("json", "account_channels", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'destination_account'."); + }; + + testInvalidDestAccountParam(1); + testInvalidDestAccountParam(1.1); + testInvalidDestAccountParam(true); + testInvalidDestAccountParam(Json::Value(Json::nullValue)); + testInvalidDestAccountParam(Json::Value(Json::objectValue)); + testInvalidDestAccountParam(Json::Value(Json::arrayValue)); + } { auto const r = env.rpc("account_channels", alice.human(), bob.human()); BEAST_EXPECT(r[jss::result][jss::channels].size() == 1); diff --git a/src/test/rpc/LedgerData_test.cpp b/src/test/rpc/LedgerData_test.cpp index 612899e47d..32d26f770c 100644 --- a/src/test/rpc/LedgerData_test.cpp +++ b/src/test/rpc/LedgerData_test.cpp @@ -165,6 +165,16 @@ public: BEAST_EXPECT(jrr[jss::status] == "error"); BEAST_EXPECT(jrr[jss::error_message] == "ledgerNotFound"); } + + { + // binary not a boolean + Json::Value jvParams; + jvParams[jss::binary] = "true"; + auto const jrr = env.rpc("json", "ledger_data", to_string(jvParams))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::status] == "error"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'binary', not boolean."); + } } void diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index c8c40ce0cc..48fcfd2dec 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -165,6 +165,23 @@ class LedgerRPC_test : public beast::unit_test::suite auto const ret = env.rpc("json", "ledger", "{ \"ledger_index\" : 1000000000000000 }"); checkErrorValue(ret, "invalidParams", "Invalid parameters."); } + + { + // test all boolean fields with non-boolean values + auto testBooleanField = [&](Json::StaticString const& field) { + Json::Value jvParams; + jvParams[field] = "blah"; + auto const jrr = env.rpc("json", "ledger", to_string(jvParams))[jss::result]; + checkErrorValue(jrr, "invalidParams", "Invalid parameters."); + }; + testBooleanField(jss::full); + testBooleanField(jss::accounts); + testBooleanField(jss::transactions); + testBooleanField(jss::expand); + testBooleanField(jss::binary); + testBooleanField(jss::owner_funds); + testBooleanField(jss::queue); + } } void diff --git a/src/test/rpc/Roles_test.cpp b/src/test/rpc/Roles_test.cpp index eaf6143f5d..c2b1d36a0b 100644 --- a/src/test/rpc/Roles_test.cpp +++ b/src/test/rpc/Roles_test.cpp @@ -25,6 +25,7 @@ class Roles_test : public beast::unit_test::suite void testRoles() { + testcase("Roles"); using namespace test::jtx; { @@ -224,6 +225,21 @@ class Roles_test : public beast::unit_test::suite BEAST_EXPECT(rpcRes["role"] == "proxied"); BEAST_EXPECT(rpcRes["ip"] == "::11:22:33:44:45.55.65.75"); BEAST_EXPECT(isValidIpAddress(rpcRes["ip"].asString())); + + // Test: "for=" not at the beginning AND no trailing delimiter. + // This exercises the fix for the buffer over-read bug where + // the remaining length was incorrectly calculated. + headers["Forwarded"] = "by=203.0.113.43;for=1.2.3.4"; + rpcRes = env.rpc(headers, "ping")["result"]; + BEAST_EXPECT(rpcRes["role"] == "proxied"); + BEAST_EXPECT(rpcRes["ip"] == "1.2.3.4"); + BEAST_EXPECT(isValidIpAddress(rpcRes["ip"].asString())); + + headers["Forwarded"] = "proto=https;by=proxy.example.com;for=5.6.7.8"; + rpcRes = env.rpc(headers, "ping")["result"]; + BEAST_EXPECT(rpcRes["role"] == "proxied"); + BEAST_EXPECT(rpcRes["ip"] == "5.6.7.8"); + BEAST_EXPECT(isValidIpAddress(rpcRes["ip"].asString())); } { @@ -252,6 +268,7 @@ class Roles_test : public beast::unit_test::suite testInvalidIpAddresses() { using namespace test::jtx; + testcase("Invalid IP addresses"); { Env env(*this); diff --git a/src/test/rpc/Submit_test.cpp b/src/test/rpc/Submit_test.cpp new file mode 100644 index 0000000000..ea5ef145c4 --- /dev/null +++ b/src/test/rpc/Submit_test.cpp @@ -0,0 +1,98 @@ +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace xrpl::test { + +class Submit_test : public beast::unit_test::suite +{ +public: + void + testFailHardValidation() + { + testcase("fail_hard parameter validation"); + using namespace jtx; + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + env.fund(XRP(10000), alice, bob); + env.close(); + + // Lambda to test invalid fail_hard parameter types + auto testInvalidFailHard = [&](auto const& param) { + // Test with tx_blob path + { + JTx const jt = env.jt(pay(alice, bob, XRP(1))); + auto const txBlob = strHex(jt.stx->getSerializer().slice()); + + Json::Value params; + params[jss::tx_blob] = txBlob; + params[jss::fail_hard] = param; + auto const jrr = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'fail_hard', not boolean."); + } + + // Test with tx_json path (deprecated signing) + { + Json::Value params; + params[jss::secret] = toBase58(generateSeed("alice")); + params[jss::tx_json] = pay("alice", "bob", XRP(1)); + params[jss::fail_hard] = param; + auto const jrr = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT(jrr[jss::error_message] == "Invalid field 'fail_hard', not boolean."); + } + }; + + // Test all invalid types + testInvalidFailHard("true"); + testInvalidFailHard("yes"); + testInvalidFailHard(1); + testInvalidFailHard(0); + testInvalidFailHard(1.5); + testInvalidFailHard(Json::Value(Json::objectValue)); + testInvalidFailHard(Json::Value(Json::arrayValue)); + + // Valid boolean values should work (not return invalidParams) + { + JTx const jt = env.jt(pay(alice, bob, XRP(1))); + auto const txBlob = strHex(jt.stx->getSerializer().slice()); + + Json::Value params; + params[jss::tx_blob] = txBlob; + params[jss::fail_hard] = true; + auto const jrr = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(!jrr.isMember(jss::error) || jrr[jss::error] != "invalidParams"); + } + { + JTx const jt = env.jt(pay(alice, bob, XRP(1))); + auto const txBlob = strHex(jt.stx->getSerializer().slice()); + + Json::Value params; + params[jss::tx_blob] = txBlob; + params[jss::fail_hard] = false; + auto const jrr = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(!jrr.isMember(jss::error) || jrr[jss::error] != "invalidParams"); + } + } + + void + run() override + { + testFailHardValidation(); + } +}; + +BEAST_DEFINE_TESTSUITE(Submit, rpc, xrpl); + +} // namespace xrpl::test diff --git a/src/test/rpc/Subscribe_test.cpp b/src/test/rpc/Subscribe_test.cpp index 02c4fd8c8d..0e43713c2a 100644 --- a/src/test/rpc/Subscribe_test.cpp +++ b/src/test/rpc/Subscribe_test.cpp @@ -214,16 +214,12 @@ public: // Check stream update for payment transaction BEAST_EXPECT(wsc->findMsg(5s, [&](auto const& jv) { return jv[jss::meta]["AffectedNodes"][1u]["CreatedNode"]["NewFields"] - [jss::Account] // - == Account("alice").human() && - jv[jss::transaction][jss::TransactionType] // - == jss::Payment && - jv[jss::transaction][jss::DeliverMax] // - == std::to_string(10000000000 + baseFee) && - jv[jss::transaction][jss::Fee] // - == std::to_string(baseFee) && - jv[jss::transaction][jss::Sequence] // - == 1; + [jss::Account] == Account("alice").human() && + jv[jss::transaction][jss::TransactionType] == jss::Payment && + jv[jss::transaction][jss::DeliverMax] == + std::to_string(10000000000 + baseFee) && + jv[jss::transaction][jss::Fee] == std::to_string(baseFee) && + jv[jss::transaction][jss::Sequence] == 1; })); // Check stream update for accountset transaction @@ -579,7 +575,7 @@ public: auto wsc = makeWSClient(env.app().config()); { - auto jr = env.rpc("json", method, "{}")[jss::result]; + auto const jr = env.rpc("json", method, "{}")[jss::result]; BEAST_EXPECT(jr[jss::error] == "invalidParams"); BEAST_EXPECT(jr[jss::error_message] == "Invalid parameters."); } @@ -589,7 +585,7 @@ public: jv[jss::url] = "not-a-url"; jv[jss::username] = "admin"; jv[jss::password] = "password"; - auto jr = env.rpc("json", method, to_string(jv))[jss::result]; + auto const jr = env.rpc("json", method, to_string(jv))[jss::result]; if (subscribe) { BEAST_EXPECT(jr[jss::error] == "invalidParams"); @@ -602,7 +598,7 @@ public: { Json::Value jv; jv[jss::url] = "ftp://scheme.not.supported.tld"; - auto jr = env.rpc("json", method, to_string(jv))[jss::result]; + auto const jr = env.rpc("json", method, to_string(jv))[jss::result]; if (subscribe) { BEAST_EXPECT(jr[jss::error] == "invalidParams"); @@ -614,7 +610,7 @@ public: Env env_nonadmin{*this, single_thread_io(no_admin(envconfig()))}; Json::Value jv; jv[jss::url] = "no-url"; - auto jr = env_nonadmin.rpc("json", method, to_string(jv))[jss::result]; + auto const jr = env_nonadmin.rpc("json", method, to_string(jv))[jss::result]; BEAST_EXPECT(jr[jss::error] == "noPermission"); BEAST_EXPECT(jr[jss::error_message] == "You don't have permission for this command."); } @@ -634,7 +630,7 @@ public: { Json::Value jv; jv[f] = nonArray; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "invalidParams"); BEAST_EXPECT(jr[jss::error_message] == "Invalid parameters."); } @@ -642,7 +638,7 @@ public: { Json::Value jv; jv[f] = Json::arrayValue; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "actMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Account malformed."); } @@ -652,7 +648,7 @@ public: { Json::Value jv; jv[jss::books] = nonArray; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "invalidParams"); BEAST_EXPECT(jr[jss::error_message] == "Invalid parameters."); } @@ -661,7 +657,7 @@ public: Json::Value jv; jv[jss::books] = Json::arrayValue; jv[jss::books][0u] = 1; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "invalidParams"); BEAST_EXPECT(jr[jss::error_message] == "Invalid parameters."); } @@ -672,7 +668,7 @@ public: jv[jss::books][0u] = Json::objectValue; jv[jss::books][0u][jss::taker_gets] = Json::objectValue; jv[jss::books][0u][jss::taker_pays] = Json::objectValue; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "srcCurMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Source currency is malformed."); } @@ -684,7 +680,7 @@ public: jv[jss::books][0u][jss::taker_gets] = Json::objectValue; jv[jss::books][0u][jss::taker_pays] = Json::objectValue; jv[jss::books][0u][jss::taker_pays][jss::currency] = "ZZZZ"; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "srcCurMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Source currency is malformed."); } @@ -697,7 +693,7 @@ public: jv[jss::books][0u][jss::taker_pays] = Json::objectValue; jv[jss::books][0u][jss::taker_pays][jss::currency] = "USD"; jv[jss::books][0u][jss::taker_pays][jss::issuer] = 1; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "srcIsrMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Source issuer is malformed."); } @@ -710,7 +706,7 @@ public: jv[jss::books][0u][jss::taker_pays] = Json::objectValue; jv[jss::books][0u][jss::taker_pays][jss::currency] = "USD"; jv[jss::books][0u][jss::taker_pays][jss::issuer] = Account{"gateway"}.human() + "DEAD"; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "srcIsrMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Source issuer is malformed."); } @@ -722,7 +718,7 @@ public: jv[jss::books][0u][jss::taker_pays] = Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); jv[jss::books][0u][jss::taker_gets] = Json::objectValue; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; // NOTE: this error is slightly incongruous with the // equivalent source currency error BEAST_EXPECT(jr[jss::error] == "dstAmtMalformed"); @@ -737,7 +733,7 @@ public: jv[jss::books][0u][jss::taker_pays] = Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); jv[jss::books][0u][jss::taker_gets][jss::currency] = "ZZZZ"; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; // NOTE: this error is slightly incongruous with the // equivalent source currency error BEAST_EXPECT(jr[jss::error] == "dstAmtMalformed"); @@ -753,7 +749,7 @@ public: Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); jv[jss::books][0u][jss::taker_gets][jss::currency] = "USD"; jv[jss::books][0u][jss::taker_gets][jss::issuer] = 1; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "dstIsrMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Destination issuer is malformed."); } @@ -766,7 +762,7 @@ public: Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); jv[jss::books][0u][jss::taker_gets][jss::currency] = "USD"; jv[jss::books][0u][jss::taker_gets][jss::issuer] = Account{"gateway"}.human() + "DEAD"; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "dstIsrMalformed"); BEAST_EXPECT(jr[jss::error_message] == "Destination issuer is malformed."); } @@ -779,7 +775,7 @@ public: Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); jv[jss::books][0u][jss::taker_gets] = Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "badMarket"); BEAST_EXPECT(jr[jss::error_message] == "No such market."); } @@ -788,7 +784,7 @@ public: { Json::Value jv; jv[jss::streams] = nonArray; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "invalidParams"); BEAST_EXPECT(jr[jss::error_message] == "Invalid parameters."); } @@ -797,7 +793,7 @@ public: Json::Value jv; jv[jss::streams] = Json::arrayValue; jv[jss::streams][0u] = 1; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "malformedStream"); BEAST_EXPECT(jr[jss::error_message] == "Stream malformed."); } @@ -806,10 +802,55 @@ public: Json::Value jv; jv[jss::streams] = Json::arrayValue; jv[jss::streams][0u] = "not_a_stream"; - auto jr = wsc->invoke(method, jv)[jss::result]; + auto const jr = wsc->invoke(method, jv)[jss::result]; BEAST_EXPECT(jr[jss::error] == "malformedStream"); BEAST_EXPECT(jr[jss::error_message] == "Stream malformed."); } + + if (subscribe) + { + // invalid taker - not a string + { + Json::Value jv; + jv[jss::books] = Json::arrayValue; + jv[jss::books][0u] = Json::objectValue; + jv[jss::books][0u][jss::taker_pays] = + Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); + jv[jss::books][0u][jss::taker_gets][jss::currency] = "XRP"; + jv[jss::books][0u][jss::taker] = 1; + auto const jr = wsc->invoke(method, jv)[jss::result]; + BEAST_EXPECTS(jr[jss::error] == "actMalformed", jr.toStyledString()); + BEAST_EXPECT(jr[jss::error_message] == "Account malformed."); + } + + // invalid taker - malformed account string + { + Json::Value jv; + jv[jss::books] = Json::arrayValue; + jv[jss::books][0u] = Json::objectValue; + jv[jss::books][0u][jss::taker_pays] = + Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); + jv[jss::books][0u][jss::taker_gets][jss::currency] = "XRP"; + jv[jss::books][0u][jss::taker] = "not_an_account"; + auto const jr = wsc->invoke(method, jv)[jss::result]; + BEAST_EXPECTS(jr[jss::error] == "actMalformed", jr.toStyledString()); + BEAST_EXPECT(jr[jss::error_message] == "Account malformed."); + } + + // invalid taker - account string with extra characters + { + Json::Value jv; + jv[jss::books] = Json::arrayValue; + jv[jss::books][0u] = Json::objectValue; + jv[jss::books][0u][jss::taker_pays] = + Account{"gateway"}["USD"](1).value().getJson(JsonOptions::include_date); + jv[jss::books][0u][jss::taker_gets][jss::currency] = "XRP"; + jv[jss::books][0u][jss::taker] = Account{"alice"}.human() + "DEAD"; + auto const jr = wsc->invoke(method, jv)[jss::result]; + BEAST_EXPECTS(jr[jss::error] == "actMalformed", jr.toStyledString()); + BEAST_EXPECT(jr[jss::error_message] == "Account malformed."); + } + } } void diff --git a/src/xrpld/rpc/detail/Role.cpp b/src/xrpld/rpc/detail/Role.cpp index 325b7eb3c6..f28905ba20 100644 --- a/src/xrpld/rpc/detail/Role.cpp +++ b/src/xrpld/rpc/detail/Role.cpp @@ -272,12 +272,12 @@ forwardedFor(http_request_type const& request) // We found a "for=". Scan for the end of the IP address. std::size_t const pos = [&found, &it]() { - std::size_t const pos = - std::string_view(found, it->value().end() - found).find_first_of(",;"); - if (pos != std::string_view::npos) + auto const remaining = static_cast(it->value().end() - found); + if (std::size_t const pos = std::string_view(found, remaining).find_first_of(",;"); + pos != std::string_view::npos) return pos; - return it->value().size() - forStr.size(); + return remaining; }(); return extractIpAddrFromField({found, pos}); diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 7a60dc1f2c..284279fa3f 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -575,11 +575,7 @@ transactionPreProcessImpl( if (verify) { - if (!sle) - { - // XXX Ignore transactions for accounts not created. - return rpcError(rpcSRC_ACT_NOT_FOUND); - } + // sle validity is checked above JLOG(j.trace()) << "verify: " << toBase58(calcAccountID(pk)) << " : " << toBase58(srcAddressID); diff --git a/src/xrpld/rpc/handlers/account/AccountChannels.cpp b/src/xrpld/rpc/handlers/account/AccountChannels.cpp index 1fa492a3c0..51282f699f 100644 --- a/src/xrpld/rpc/handlers/account/AccountChannels.cpp +++ b/src/xrpld/rpc/handlers/account/AccountChannels.cpp @@ -95,7 +95,11 @@ doAccountChannels(RPC::JsonContext& context) std::string strDst; if (params.isMember(jss::destination_account)) + { + if (!params[jss::destination_account].isString()) + return RPC::invalid_field_error(jss::destination_account); strDst = params[jss::destination_account].asString(); + } auto const raDstAccount = [&]() -> std::optional { return strDst.empty() ? std::nullopt : parseBase58(strDst); diff --git a/src/xrpld/rpc/handlers/account/AccountInfo.cpp b/src/xrpld/rpc/handlers/account/AccountInfo.cpp index 019ff83def..c1c6c71339 100644 --- a/src/xrpld/rpc/handlers/account/AccountInfo.cpp +++ b/src/xrpld/rpc/handlers/account/AccountInfo.cpp @@ -58,10 +58,9 @@ injectSLE(Json::Value& jv, SLE const& sle) Blob const b(hash.begin(), hash.end()); std::string md5 = strHex(makeSlice(b)); boost::to_lower(md5); - // VFALCO TODO Give a name and move this constant - // to a more visible location. Also - // shouldn't this be https? - jv[jss::urlgravatar] = str(boost::format("http://www.gravatar.com/avatar/%s") % md5); + // VFALCO TODO Give a name to this constant and move it + // to a more visible location. + jv[jss::urlgravatar] = str(boost::format("https://www.gravatar.com/avatar/%s") % md5); } } else diff --git a/src/xrpld/rpc/handlers/account/NoRippleCheck.cpp b/src/xrpld/rpc/handlers/account/NoRippleCheck.cpp index 54964be8da..d05d74170f 100644 --- a/src/xrpld/rpc/handlers/account/NoRippleCheck.cpp +++ b/src/xrpld/rpc/handlers/account/NoRippleCheck.cpp @@ -118,14 +118,14 @@ doNoRippleCheck(RPC::JsonContext& context) bool const bDefaultRipple = (sle->getFieldU32(sfFlags) & lsfDefaultRipple) != 0u; - if ((static_cast(bDefaultRipple) & static_cast(!roleGateway)) != 0) + if (bDefaultRipple && !roleGateway) { problems.append( "You appear to have set your default ripple flag even though you " "are not a gateway. This is not recommended unless you are " "experimenting"); } - else if ((static_cast(roleGateway) & static_cast(!bDefaultRipple)) != 0) + else if (roleGateway && !bDefaultRipple) { problems.append("You should immediately set your default ripple flag"); if (transactions) @@ -148,12 +148,12 @@ doNoRippleCheck(RPC::JsonContext& context) std::string problem; bool needFix = false; - if (bNoRipple & roleGateway) + if (bNoRipple && roleGateway) { problem = "You should clear the no ripple flag on your "; needFix = true; } - else if (!roleGateway & !bNoRipple) + else if (!roleGateway && !bNoRipple) { problem = "You should probably set the no ripple flag on your "; needFix = true; diff --git a/src/xrpld/rpc/handlers/ledger/Ledger.cpp b/src/xrpld/rpc/handlers/ledger/Ledger.cpp index dd2280b51c..dc5f24dffc 100644 --- a/src/xrpld/rpc/handlers/ledger/Ledger.cpp +++ b/src/xrpld/rpc/handlers/ledger/Ledger.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -42,28 +43,56 @@ Status LedgerHandler::check() { auto const& params = context_.params; + + auto getBool = [&](Json::StaticString const& field) -> Expected { + if (!params.isMember(field)) + { + return false; + } + if (!params[field].isBool()) + { + return Unexpected(rpcINVALID_PARAMS); + } + + return params[field].asBool(); + }; + + auto const full = getBool(jss::full); + auto const transactions = getBool(jss::transactions); + auto const accounts = getBool(jss::accounts); + auto const expand = getBool(jss::expand); + auto const binary = getBool(jss::binary); + auto const owner_funds = getBool(jss::owner_funds); + auto const queue = getBool(jss::queue); + + if (!full.has_value()) + return full.error(); + if (!transactions.has_value()) + return transactions.error(); + if (!accounts.has_value()) + return accounts.error(); + if (!expand.has_value()) + return expand.error(); + if (!binary.has_value()) + return binary.error(); + if (!owner_funds.has_value()) + return owner_funds.error(); + if (!queue.has_value()) + return queue.error(); + + options_ = (*full ? LedgerFill::full : 0) | (*expand ? LedgerFill::expand : 0) | + (*transactions ? LedgerFill::dumpTxrp : 0) | (*accounts ? LedgerFill::dumpState : 0) | + (*binary ? LedgerFill::binary : 0) | (*owner_funds ? LedgerFill::ownerFunds : 0) | + (*queue ? LedgerFill::dumpQueue : 0); + bool const needsLedger = params.isMember(jss::ledger) || params.isMember(jss::ledger_hash) || params.isMember(jss::ledger_index); if (!needsLedger) return Status::OK; - if (auto s = lookupLedger(ledger_, context_, result_)) return s; - bool const full = params[jss::full].asBool(); - bool const transactions = params[jss::transactions].asBool(); - bool const accounts = params[jss::accounts].asBool(); - bool const expand = params[jss::expand].asBool(); - bool const binary = params[jss::binary].asBool(); - bool const owner_funds = params[jss::owner_funds].asBool(); - bool const queue = params[jss::queue].asBool(); - - options_ = (full ? LedgerFill::full : 0) | (expand ? LedgerFill::expand : 0) | - (transactions ? LedgerFill::dumpTxrp : 0) | (accounts ? LedgerFill::dumpState : 0) | - (binary ? LedgerFill::binary : 0) | (owner_funds ? LedgerFill::ownerFunds : 0) | - (queue ? LedgerFill::dumpQueue : 0); - - if (full || accounts) + if (*full || *accounts) { // Until some sane way to get full ledgers has been implemented, // disallow retrieving all state nodes. @@ -76,7 +105,8 @@ LedgerHandler::check() } context_.loadType = binary ? Resource::feeMediumBurdenRPC : Resource::feeHeavyBurdenRPC; } - if (queue) + + if (*queue) { if (!ledger_ || !ledger_->open()) { diff --git a/src/xrpld/rpc/handlers/ledger/LedgerData.cpp b/src/xrpld/rpc/handlers/ledger/LedgerData.cpp index f0a361d951..367eaa8973 100644 --- a/src/xrpld/rpc/handlers/ledger/LedgerData.cpp +++ b/src/xrpld/rpc/handlers/ledger/LedgerData.cpp @@ -54,7 +54,13 @@ doLedgerData(RPC::JsonContext& context) return RPC::expected_field_error(jss::marker, "valid"); } - bool const isBinary = params[jss::binary].asBool(); + bool isBinary = false; + if (params.isMember(jss::binary)) + { + if (!params[jss::binary].isBool()) + return RPC::expected_field_error(jss::binary, "boolean"); + isBinary = params[jss::binary].asBool(); + } int limit = -1; if (params.isMember(jss::limit)) diff --git a/src/xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp b/src/xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp index 10359c810e..7db7c2312e 100644 --- a/src/xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp +++ b/src/xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp @@ -241,7 +241,6 @@ doGetAggregatePrice(RPC::JsonContext& context) return result; } - // Get the ledger std::shared_ptr ledger; result = RPC::lookupLedger(ledger, context); if (!ledger) diff --git a/src/xrpld/rpc/handlers/subscribe/Subscribe.cpp b/src/xrpld/rpc/handlers/subscribe/Subscribe.cpp index 9be273587d..f5e33922dc 100644 --- a/src/xrpld/rpc/handlers/subscribe/Subscribe.cpp +++ b/src/xrpld/rpc/handlers/subscribe/Subscribe.cpp @@ -248,9 +248,11 @@ doSubscribe(RPC::JsonContext& context) if (j.isMember(jss::taker)) { + if (!j[jss::taker].isString()) + return rpcError(rpcACT_MALFORMED); takerID = parseBase58(j[jss::taker].asString()); if (!takerID) - return rpcError(rpcBAD_ISSUER); + return rpcError(rpcACT_MALFORMED); } if (j.isMember(jss::domain)) diff --git a/src/xrpld/rpc/handlers/transaction/Submit.cpp b/src/xrpld/rpc/handlers/transaction/Submit.cpp index 936579c276..8b54083628 100644 --- a/src/xrpld/rpc/handlers/transaction/Submit.cpp +++ b/src/xrpld/rpc/handlers/transaction/Submit.cpp @@ -4,10 +4,12 @@ #include #include +#include #include #include #include #include +#include #include #include #include @@ -24,11 +26,15 @@ namespace xrpl { -static NetworkOPs::FailHard +static Expected getFailHard(RPC::JsonContext const& context) { + if (context.params.isMember(jss::fail_hard) && !context.params[jss::fail_hard].isBool()) + { + return Unexpected(RPC::expected_field_error(jss::fail_hard, "boolean")); + } return NetworkOPs::doFailHard( - context.params.isMember("fail_hard") && context.params["fail_hard"].asBool()); + context.params.isMember(jss::fail_hard) && context.params[jss::fail_hard].asBool()); } // { @@ -43,6 +49,8 @@ doSubmit(RPC::JsonContext& context) if (!context.params.isMember(jss::tx_blob)) { auto const failType = getFailHard(context); + if (!failType) + return failType.error(); if (context.role != Role::ADMIN && !context.app.config().canSign()) return RPC::make_error(rpcNOT_SUPPORTED, "Signing is not supported by this server."); @@ -50,7 +58,7 @@ doSubmit(RPC::JsonContext& context) auto ret = RPC::transactionSubmit( context.params, context.apiVersion, - failType, + *failType, context.role, context.ledgerMaster.getValidatedLedgerAge(), context.app, @@ -118,8 +126,10 @@ doSubmit(RPC::JsonContext& context) try { auto const failType = getFailHard(context); + if (!failType) + return failType.error(); - context.netOps.processTransaction(transaction, isUnlimited(context.role), true, failType); + context.netOps.processTransaction(transaction, isUnlimited(context.role), true, *failType); } catch (std::exception& e) {