From eba5d19377cf0fb6f010e5c413e2079431bf4078 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 12 Jul 2024 17:58:17 -0400 Subject: [PATCH 1/3] Improve error handling in some RPC commands --- Builds/CMake/RippledCore.cmake | 2 +- src/ripple/rpc/handlers/AccountChannels.cpp | 3 + .../rpc/handlers/AccountCurrenciesHandler.cpp | 24 +++++--- src/ripple/rpc/handlers/AccountInfo.cpp | 9 +++ src/ripple/rpc/handlers/AccountLines.cpp | 3 + src/ripple/rpc/handlers/AccountObjects.cpp | 17 ++++-- src/ripple/rpc/handlers/AccountOffers.cpp | 13 +++-- src/ripple/rpc/handlers/AccountTx.cpp | 9 ++- src/ripple/rpc/handlers/NoRippleCheck.cpp | 6 +- src/test/app/PayChan_test.cpp | 19 ++++++ src/test/rpc/AccountCurrencies_test.cpp | 47 ++++++++++++++- src/test/rpc/AccountInfo_test.cpp | 47 ++++++++++++++- ...inesRPC_test.cpp => AccountLines_test.cpp} | 23 +++++++- src/test/rpc/AccountObjects_test.cpp | 58 ++++++++++++++++++- src/test/rpc/AccountOffers_test.cpp | 33 ++++++++++- src/test/rpc/AccountTx_test.cpp | 26 ++++++++- src/test/rpc/NoRippleCheck_test.cpp | 25 +++++++- 17 files changed, 329 insertions(+), 35 deletions(-) rename src/test/rpc/{AccountLinesRPC_test.cpp => AccountLines_test.cpp} (98%) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 6b7b2aae683..44d7061d739 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1119,7 +1119,7 @@ if (tests) #]===============================] src/test/rpc/AccountCurrencies_test.cpp src/test/rpc/AccountInfo_test.cpp - src/test/rpc/AccountLinesRPC_test.cpp + src/test/rpc/AccountLines_test.cpp src/test/rpc/AccountObjects_test.cpp src/test/rpc/AccountOffers_test.cpp src/test/rpc/AccountSet_test.cpp diff --git a/src/ripple/rpc/handlers/AccountChannels.cpp b/src/ripple/rpc/handlers/AccountChannels.cpp index ebd89b04418..da751d0dae2 100644 --- a/src/ripple/rpc/handlers/AccountChannels.cpp +++ b/src/ripple/rpc/handlers/AccountChannels.cpp @@ -71,6 +71,9 @@ doAccountChannels(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) diff --git a/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp b/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp index 45dc8b545ca..0114e25c712 100644 --- a/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp +++ b/src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp @@ -33,19 +33,29 @@ doAccountCurrencies(RPC::JsonContext& context) { auto& params = context.params; + if (!(params.isMember(jss::account) || params.isMember(jss::ident))) + return RPC::missing_field_error(jss::account); + + std::string strIdent; + if (params.isMember(jss::account)) + { + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + strIdent = params[jss::account].asString(); + } + else if (params.isMember(jss::ident)) + { + if (!params[jss::ident].isString()) + return RPC::invalid_field_error(jss::ident); + strIdent = params[jss::ident].asString(); + } + // Get the current ledger std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) return result; - if (!(params.isMember(jss::account) || params.isMember(jss::ident))) - return RPC::missing_field_error(jss::account); - - std::string const strIdent( - params.isMember(jss::account) ? params[jss::account].asString() - : params[jss::ident].asString()); - // Get info on account. auto id = parseBase58(strIdent); if (!id) diff --git a/src/ripple/rpc/handlers/AccountInfo.cpp b/src/ripple/rpc/handlers/AccountInfo.cpp index bd2184f49a3..b4d31f6187c 100644 --- a/src/ripple/rpc/handlers/AccountInfo.cpp +++ b/src/ripple/rpc/handlers/AccountInfo.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -53,9 +54,17 @@ doAccountInfo(RPC::JsonContext& context) std::string strIdent; if (params.isMember(jss::account)) + { + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); strIdent = params[jss::account].asString(); + } else if (params.isMember(jss::ident)) + { + if (!params[jss::ident].isString()) + return RPC::invalid_field_error(jss::ident); strIdent = params[jss::ident].asString(); + } else return RPC::missing_field_error(jss::account); diff --git a/src/ripple/rpc/handlers/AccountLines.cpp b/src/ripple/rpc/handlers/AccountLines.cpp index 3bfcd225b14..3f7e154690b 100644 --- a/src/ripple/rpc/handlers/AccountLines.cpp +++ b/src/ripple/rpc/handlers/AccountLines.cpp @@ -80,6 +80,9 @@ doAccountLines(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) diff --git a/src/ripple/rpc/handlers/AccountObjects.cpp b/src/ripple/rpc/handlers/AccountObjects.cpp index 2531cd03115..f228272f640 100644 --- a/src/ripple/rpc/handlers/AccountObjects.cpp +++ b/src/ripple/rpc/handlers/AccountObjects.cpp @@ -54,17 +54,19 @@ doAccountNFTs(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); - std::shared_ptr ledger; - auto result = RPC::lookupLedger(ledger, context); - if (ledger == nullptr) - return result; + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); auto id = parseBase58(params[jss::account].asString()); if (!id) { - RPC::inject_error(rpcACT_MALFORMED, result); - return result; + return rpcError(rpcACT_MALFORMED); } + + std::shared_ptr ledger; + auto result = RPC::lookupLedger(ledger, context); + if (ledger == nullptr) + return result; auto const accountID{id.value()}; if (!ledger->exists(keylet::account(accountID))) @@ -167,6 +169,9 @@ doAccountObjects(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (ledger == nullptr) diff --git a/src/ripple/rpc/handlers/AccountOffers.cpp b/src/ripple/rpc/handlers/AccountOffers.cpp index 867f888e241..2be50e16880 100644 --- a/src/ripple/rpc/handlers/AccountOffers.cpp +++ b/src/ripple/rpc/handlers/AccountOffers.cpp @@ -60,6 +60,9 @@ doAccountOffers(RPC::JsonContext& context) if (!params.isMember(jss::account)) return RPC::missing_field_error(jss::account); + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + std::shared_ptr ledger; auto result = RPC::lookupLedger(ledger, context); if (!ledger) @@ -84,7 +87,7 @@ doAccountOffers(RPC::JsonContext& context) return *err; if (limit == 0) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::limit); Json::Value& jsonOffers(result[jss::offers] = Json::arrayValue); std::vector> offers; @@ -101,13 +104,13 @@ doAccountOffers(RPC::JsonContext& context) std::stringstream marker(params[jss::marker].asString()); std::string value; if (!std::getline(marker, value, ',')) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); if (!startAfter.parseHex(value)) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); if (!std::getline(marker, value, ',')) - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); try { @@ -115,7 +118,7 @@ doAccountOffers(RPC::JsonContext& context) } catch (boost::bad_lexical_cast&) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::marker); } // We then must check if the object pointed to by the marker is actually diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index 7fe7472721f..89d4f49a519 100644 --- a/src/ripple/rpc/handlers/AccountTx.cpp +++ b/src/ripple/rpc/handlers/AccountTx.cpp @@ -426,12 +426,12 @@ doAccountTxJson(RPC::JsonContext& context) if (context.apiVersion > 1u && params.isMember(jss::binary) && !params[jss::binary].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::binary); } if (context.apiVersion > 1u && params.isMember(jss::forward) && !params[jss::forward].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::forward); } args.limit = params.isMember(jss::limit) ? params[jss::limit].asUInt() : 0; @@ -440,7 +440,10 @@ doAccountTxJson(RPC::JsonContext& context) params.isMember(jss::forward) && params[jss::forward].asBool(); if (!params.isMember(jss::account)) - return rpcError(rpcINVALID_PARAMS); + return RPC::missing_field_error(jss::account); + + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); auto const account = parseBase58(params[jss::account].asString()); diff --git a/src/ripple/rpc/handlers/NoRippleCheck.cpp b/src/ripple/rpc/handlers/NoRippleCheck.cpp index 91d69df96bd..6cb206e2530 100644 --- a/src/ripple/rpc/handlers/NoRippleCheck.cpp +++ b/src/ripple/rpc/handlers/NoRippleCheck.cpp @@ -66,6 +66,10 @@ doNoRippleCheck(RPC::JsonContext& context) if (!params.isMember("role")) return RPC::missing_field_error("role"); + + if (!params[jss::account].isString()) + return RPC::invalid_field_error(jss::account); + bool roleGateway = false; { std::string const role = params["role"].asString(); @@ -90,7 +94,7 @@ doNoRippleCheck(RPC::JsonContext& context) if (context.apiVersion > 1u && params.isMember(jss::transactions) && !params[jss::transactions].isBool()) { - return rpcError(rpcINVALID_PARAMS); + return RPC::invalid_field_error(jss::transactions); } std::shared_ptr ledger; diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index a479e43b170..4bd23a4edb8 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -873,6 +873,25 @@ struct PayChan_test : public beast::unit_test::suite auto const chan1Str = to_string(channel(alice, bob, env.seq(alice))); env(create(alice, bob, channelFunds, settleDelay, pk)); env.close(); + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto 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'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } { auto const r = env.rpc("account_channels", alice.human(), bob.human()); diff --git a/src/test/rpc/AccountCurrencies_test.cpp b/src/test/rpc/AccountCurrencies_test.cpp index c3e46a3e66c..64b73743024 100644 --- a/src/test/rpc/AccountCurrencies_test.cpp +++ b/src/test/rpc/AccountCurrencies_test.cpp @@ -39,6 +39,7 @@ class AccountCurrencies_test : public beast::unit_test::suite { // invalid ledger (hash) Json::Value params; + params[jss::account] = Account{"bob"}.human(); params[jss::ledger_hash] = 1; auto const result = env.rpc( "json", @@ -56,6 +57,50 @@ class AccountCurrencies_test : public beast::unit_test::suite result[jss::error_message] == "Missing field 'account'."); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", + "account_currencies", + to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + + { + // test ident non-string + auto testInvalidIdentParam = [&](auto const& param) { + Json::Value params; + params[jss::ident] = param; + auto jrr = env.rpc( + "json", + "account_currencies", + to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'ident'."); + }; + + testInvalidIdentParam(1); + testInvalidIdentParam(1.1); + testInvalidIdentParam(true); + testInvalidIdentParam(Json::Value(Json::nullValue)); + testInvalidIdentParam(Json::Value(Json::objectValue)); + testInvalidIdentParam(Json::Value(Json::arrayValue)); + } + { Json::Value params; params[jss::account] = @@ -198,6 +243,6 @@ class AccountCurrencies_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountCurrencies, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountCurrencies, rpc, ripple); } // namespace ripple diff --git a/src/test/rpc/AccountInfo_test.cpp b/src/test/rpc/AccountInfo_test.cpp index ff23a5e53c1..2d6832cee05 100644 --- a/src/test/rpc/AccountInfo_test.cpp +++ b/src/test/rpc/AccountInfo_test.cpp @@ -36,6 +36,7 @@ class AccountInfo_test : public beast::unit_test::suite void testErrors() { + testcase("Errors"); using namespace jtx; Env env(*this); { @@ -78,12 +79,53 @@ class AccountInfo_test : public beast::unit_test::suite BEAST_EXPECT( info[jss::result][jss::error_message] == "Account malformed."); } + { + // Cannot pass a non-string into the `account` param + + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_info", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { + // Cannot pass a non-string into the `ident` param + + auto testInvalidIdentParam = [&](auto const& param) { + Json::Value params; + params[jss::ident] = param; + auto jrr = env.rpc( + "json", "account_info", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'ident'."); + }; + + testInvalidIdentParam(1); + testInvalidIdentParam(1.1); + testInvalidIdentParam(true); + testInvalidIdentParam(Json::Value(Json::nullValue)); + testInvalidIdentParam(Json::Value(Json::objectValue)); + testInvalidIdentParam(Json::Value(Json::arrayValue)); + } } // Test the "signer_lists" argument in account_info. void testSignerLists() { + testcase("Signer lists"); using namespace jtx; Env env(*this); Account const alice{"alice"}; @@ -205,6 +247,7 @@ class AccountInfo_test : public beast::unit_test::suite void testSignerListsApiVersion2() { + testcase("Signer lists APIv2"); using namespace jtx; Env env{*this}; Account const alice{"alice"}; @@ -326,6 +369,7 @@ class AccountInfo_test : public beast::unit_test::suite void testSignerListsV2() { + testcase("Signer lists v2"); using namespace jtx; Env env(*this); Account const alice{"alice"}; @@ -515,6 +559,7 @@ class AccountInfo_test : public beast::unit_test::suite void testAccountFlags(FeatureBitset const& features) { + testcase("Account flags"); using namespace jtx; Env env(*this, features); @@ -652,7 +697,7 @@ class AccountInfo_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountInfo, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountInfo, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLines_test.cpp similarity index 98% rename from src/test/rpc/AccountLinesRPC_test.cpp rename to src/test/rpc/AccountLines_test.cpp index 04688156d12..c02cc25e1a8 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLines_test.cpp @@ -27,7 +27,7 @@ namespace ripple { namespace RPC { -class AccountLinesRPC_test : public beast::unit_test::suite +class AccountLines_test : public beast::unit_test::suite { public: void @@ -55,6 +55,25 @@ class AccountLinesRPC_test : public beast::unit_test::suite lines[jss::result][jss::error_message] == RPC::make_error(rpcACT_MALFORMED)[jss::error_message]); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_lines", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } Account const alice{"alice"}; { // account_lines on an unfunded account. @@ -1474,7 +1493,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountLinesRPC, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountLines, rpc, ripple); } // namespace RPC } // namespace ripple diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 17217f2c880..46b092c4a6d 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -123,8 +123,30 @@ class AccountObjects_test : public beast::unit_test::suite // test error on no account { - auto resp = env.rpc("json", "account_objects"); - BEAST_EXPECT(resp[jss::error_message] == "Syntax error."); + Json::Value params; + auto resp = env.rpc("json", "account_objects", to_string(params)); + BEAST_EXPECT( + resp[jss::result][jss::error_message] == + "Missing field 'account'."); + } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_objects", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); } // test error on malformed account string. { @@ -1032,6 +1054,35 @@ class AccountObjects_test : public beast::unit_test::suite BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hashes), 0)); } + void + testAccountNFTs() + { + testcase("account_nfts"); + + using namespace jtx; + Env env(*this); + + // test validation + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_nfts", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + } + void run() override { @@ -1039,10 +1090,11 @@ class AccountObjects_test : public beast::unit_test::suite testUnsteppedThenStepped(); testUnsteppedThenSteppedWithNFTs(); testObjectTypes(); + testAccountNFTs(); } }; -BEAST_DEFINE_TESTSUITE(AccountObjects, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountObjects, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index d94442ea8e5..635dd1a63b4 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -37,6 +37,8 @@ class AccountOffers_test : public beast::unit_test::suite void testNonAdminMinLimit() { + testcase("Non-Admin Min Limit"); + using namespace jtx; Env env{*this, envconfig(no_admin)}; Account const gw("G1"); @@ -81,6 +83,9 @@ class AccountOffers_test : public beast::unit_test::suite void testSequential(bool asAdmin) { + testcase( + std::string("Sequential - ") + (asAdmin ? "admin" : "non-admin")); + using namespace jtx; Env env{*this, asAdmin ? envconfig() : envconfig(no_admin)}; Account const gw("G1"); @@ -215,6 +220,8 @@ class AccountOffers_test : public beast::unit_test::suite void testBadInput() { + testcase("Bad input"); + using namespace jtx; Env env(*this); Account const gw("G1"); @@ -233,6 +240,26 @@ class AccountOffers_test : public beast::unit_test::suite BEAST_EXPECT(jrr[jss::error_message] == "Syntax error."); } + { + // test account non-string + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_offers", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { // empty string account Json::Value jvParams; @@ -282,7 +309,9 @@ class AccountOffers_test : public beast::unit_test::suite jvParams.toStyledString())[jss::result]; BEAST_EXPECT(jrr[jss::error] == "invalidParams"); BEAST_EXPECT(jrr[jss::status] == "error"); - BEAST_EXPECT(jrr[jss::error_message] == "Invalid parameters."); + BEAST_EXPECTS( + jrr[jss::error_message] == "Invalid field 'marker'.", + jrr.toStyledString()); } { @@ -326,7 +355,7 @@ class AccountOffers_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(AccountOffers, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountOffers, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 6601996925e..2cec59d7198 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -109,6 +109,7 @@ class AccountTx_test : public beast::unit_test::suite void testParameters(unsigned int apiVersion) { + testcase("Parameters APIv" + std::to_string(apiVersion)); using namespace test::jtx; Env env(*this); @@ -353,6 +354,25 @@ class AccountTx_test : public beast::unit_test::suite env.rpc("json", "account_tx", to_string(p)), rpcLGR_IDX_MALFORMED)); } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + auto jrr = env.rpc( + "json", "account_tx", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } // test binary and forward for bool/non bool values { Json::Value p{jParms}; @@ -388,6 +408,8 @@ class AccountTx_test : public beast::unit_test::suite void testContents() { + testcase("Contents"); + // Get results for all transaction types that can be associated // with an account. Start by generating all transaction types. using namespace test::jtx; @@ -600,6 +622,8 @@ class AccountTx_test : public beast::unit_test::suite void testAccountDelete() { + testcase("AccountDelete"); + // Verify that if an account is resurrected then the account_tx RPC // command still recovers all transactions on that account before // and after resurrection. @@ -740,7 +764,7 @@ class AccountTx_test : public beast::unit_test::suite testAccountDelete(); } }; -BEAST_DEFINE_TESTSUITE(AccountTx, app, ripple); +BEAST_DEFINE_TESTSUITE(AccountTx, rpc, ripple); } // namespace test } // namespace ripple diff --git a/src/test/rpc/NoRippleCheck_test.cpp b/src/test/rpc/NoRippleCheck_test.cpp index 3d34f55c90d..9858adf8466 100644 --- a/src/test/rpc/NoRippleCheck_test.cpp +++ b/src/test/rpc/NoRippleCheck_test.cpp @@ -64,6 +64,27 @@ class NoRippleCheck_test : public beast::unit_test::suite BEAST_EXPECT(result[jss::error_message] == "Missing field 'role'."); } + // test account non-string + { + auto testInvalidAccountParam = [&](auto const& param) { + Json::Value params; + params[jss::account] = param; + params[jss::role] = "user"; + auto jrr = env.rpc( + "json", "noripple_check", to_string(params))[jss::result]; + BEAST_EXPECT(jrr[jss::error] == "invalidParams"); + BEAST_EXPECT( + jrr[jss::error_message] == "Invalid field 'account'."); + }; + + testInvalidAccountParam(1); + testInvalidAccountParam(1.1); + testInvalidAccountParam(true); + testInvalidAccountParam(Json::Value(Json::nullValue)); + testInvalidAccountParam(Json::Value(Json::objectValue)); + testInvalidAccountParam(Json::Value(Json::arrayValue)); + } + { // invalid role field Json::Value params; params[jss::account] = alice.human(); @@ -369,12 +390,12 @@ class NoRippleCheckLimits_test : public beast::unit_test::suite } }; -BEAST_DEFINE_TESTSUITE(NoRippleCheck, app, ripple); +BEAST_DEFINE_TESTSUITE(NoRippleCheck, rpc, ripple); // These tests that deal with limit amounts are slow because of the // offer/account setup, so making them manual -- the additional coverage // provided by them is minimal -BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(NoRippleCheckLimits, app, ripple, 1); +BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(NoRippleCheckLimits, rpc, ripple, 1); } // namespace ripple From c157816017ae869c4fb6ca3e2bcac97e5a33ef65 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Mon, 22 Jul 2024 15:47:01 -0400 Subject: [PATCH 2/3] Use error codes throughout fast Base58 implementation --- src/ripple/protocol/impl/b58_utils.h | 24 +++++++------- src/ripple/protocol/impl/token_errors.h | 1 + src/ripple/protocol/impl/tokens.cpp | 26 ++++++++++++--- src/test/basics/base58_test.cpp | 42 +++++++++++++++++++++++-- 4 files changed, 75 insertions(+), 18 deletions(-) diff --git a/src/ripple/protocol/impl/b58_utils.h b/src/ripple/protocol/impl/b58_utils.h index c3bb0c03750..1e7519f0eb0 100644 --- a/src/ripple/protocol/impl/b58_utils.h +++ b/src/ripple/protocol/impl/b58_utils.h @@ -21,6 +21,7 @@ #define RIPPLE_PROTOCOL_B58_UTILS_H_INCLUDED #include +#include #include #include @@ -71,12 +72,12 @@ carrying_add(std::uint64_t a, std::uint64_t b) // (i.e a[0] is the 2^0 coefficient, a[n] is the 2^(64*n) coefficient) // panics if overflows (this is a specialized adder for b58 decoding. // it should never overflow). -inline void +[[nodiscard]] inline TokenCodecErrc inplace_bigint_add(std::span a, std::uint64_t b) { if (a.size() <= 1) { - ripple::LogicError("Input span too small for inplace_bigint_add"); + return TokenCodecErrc::inputTooSmall; } std::uint64_t carry; @@ -86,28 +87,29 @@ inplace_bigint_add(std::span a, std::uint64_t b) { if (!carry) { - return; + return TokenCodecErrc::success; } std::tie(v, carry) = carrying_add(v, 1); } if (carry) { - LogicError("Overflow in inplace_bigint_add"); + return TokenCodecErrc::overflowAdd; } + return TokenCodecErrc::success; } -inline void +[[nodiscard]] inline TokenCodecErrc inplace_bigint_mul(std::span a, std::uint64_t b) { if (a.empty()) { - LogicError("Empty span passed to inplace_bigint_mul"); + return TokenCodecErrc::inputTooSmall; } auto const last_index = a.size() - 1; if (a[last_index] != 0) { - LogicError("Non-zero element in inplace_bigint_mul last index"); + return TokenCodecErrc::inputTooLarge; } std::uint64_t carry = 0; @@ -116,7 +118,9 @@ inplace_bigint_mul(std::span a, std::uint64_t b) std::tie(coeff, carry) = carrying_mul(coeff, b, carry); } a[last_index] = carry; + return TokenCodecErrc::success; } + // divide a "big uint" value inplace and return the mod // numerator is stored so smallest coefficients come first [[nodiscard]] inline std::uint64_t @@ -166,11 +170,7 @@ inplace_bigint_div_rem(std::span numerator, std::uint64_t divisor) b58_10_to_b58_be(std::uint64_t input) { constexpr std::uint64_t B_58_10 = 430804206899405824; // 58^10; - if (input >= B_58_10) - { - LogicError("Input to b58_10_to_b58_be equals or exceeds 58^10."); - } - + assert(input < B_58_10); constexpr std::size_t resultSize = 10; std::array result{}; int i = 0; diff --git a/src/ripple/protocol/impl/token_errors.h b/src/ripple/protocol/impl/token_errors.h index 59b09974149..23a46bd1c5e 100644 --- a/src/ripple/protocol/impl/token_errors.h +++ b/src/ripple/protocol/impl/token_errors.h @@ -32,6 +32,7 @@ enum class TokenCodecErrc { mismatchedTokenType, mismatchedChecksum, invalidEncodingChar, + overflowAdd, unknown, }; } diff --git a/src/ripple/protocol/impl/tokens.cpp b/src/ripple/protocol/impl/tokens.cpp index 8445eec38ca..a166ac733cd 100644 --- a/src/ripple/protocol/impl/tokens.cpp +++ b/src/ripple/protocol/impl/tokens.cpp @@ -467,6 +467,11 @@ b256_to_b58_be(std::span input, std::span out) { continue; } + static constexpr std::uint64_t B_58_10 = 430804206899405824; // 58^10; + if (base_58_10_coeff[i] >= B_58_10) + { + return Unexpected(TokenCodecErrc::inputTooLarge); + } std::array const b58_be = ripple::b58_fast::detail::b58_10_to_b58_be(base_58_10_coeff[i]); std::size_t to_skip = 0; @@ -565,10 +570,23 @@ b58_to_b256_be(std::string_view input, std::span out) for (int i = 1; i < num_b_58_10_coeffs; ++i) { std::uint64_t const c = b_58_10_coeff[i]; - ripple::b58_fast::detail::inplace_bigint_mul( - std::span(&result[0], cur_result_size + 1), B_58_10); - ripple::b58_fast::detail::inplace_bigint_add( - std::span(&result[0], cur_result_size + 1), c); + + { + auto code = ripple::b58_fast::detail::inplace_bigint_mul( + std::span(&result[0], cur_result_size + 1), B_58_10); + if (code != TokenCodecErrc::success) + { + return Unexpected(code); + } + } + { + auto code = ripple::b58_fast::detail::inplace_bigint_add( + std::span(&result[0], cur_result_size + 1), c); + if (code != TokenCodecErrc::success) + { + return Unexpected(code); + } + } if (result[cur_result_size] != 0) { cur_result_size += 1; diff --git a/src/test/basics/base58_test.cpp b/src/test/basics/base58_test.cpp index 6f3d495d7a9..8b79d2729d7 100644 --- a/src/test/basics/base58_test.cpp +++ b/src/test/basics/base58_test.cpp @@ -177,6 +177,7 @@ class base58_test : public beast::unit_test::suite constexpr std::size_t iters = 100000; auto eng = randEngine(); std::uniform_int_distribution dist; + std::uniform_int_distribution dist1(1); for (int i = 0; i < iters; ++i) { std::uint64_t const d = dist(eng); @@ -209,12 +210,31 @@ class base58_test : public beast::unit_test::suite auto const refAdd = boostBigInt + d; - b58_fast::detail::inplace_bigint_add( + auto const result = b58_fast::detail::inplace_bigint_add( std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::success); auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); BEAST_EXPECT(refAdd == foundAdd); } for (int i = 0; i < iters; ++i) + { + std::uint64_t const d = dist1(eng); + // Force overflow + std::vector bigInt( + 5, std::numeric_limits::max()); + + auto const boostBigInt = multiprecision_utils::toBoostMP( + std::span(bigInt.data(), bigInt.size())); + + auto const refAdd = boostBigInt + d; + + auto const result = b58_fast::detail::inplace_bigint_add( + std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::overflowAdd); + auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); + BEAST_EXPECT(refAdd != foundAdd); + } + for (int i = 0; i < iters; ++i) { std::uint64_t const d = dist(eng); auto bigInt = multiprecision_utils::randomBigInt(/* minSize */ 2); @@ -226,11 +246,29 @@ class base58_test : public beast::unit_test::suite auto const refMul = boostBigInt * d; - b58_fast::detail::inplace_bigint_mul( + auto const result = b58_fast::detail::inplace_bigint_mul( std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::success); auto const foundMul = multiprecision_utils::toBoostMP(bigInt); BEAST_EXPECT(refMul == foundMul); } + for (int i = 0; i < iters; ++i) + { + std::uint64_t const d = dist1(eng); + // Force overflow + std::vector bigInt( + 5, std::numeric_limits::max()); + auto const boostBigInt = multiprecision_utils::toBoostMP( + std::span(bigInt.data(), bigInt.size())); + + auto const refMul = boostBigInt * d; + + auto const result = b58_fast::detail::inplace_bigint_mul( + std::span(bigInt.data(), bigInt.size()), d); + BEAST_EXPECT(result == TokenCodecErrc::inputTooLarge); + auto const foundMul = multiprecision_utils::toBoostMP(bigInt); + BEAST_EXPECT(refMul != foundMul); + } } void From e6ef0fc26cb8d4db25075eaa1fe21fcc7f984751 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 22 Jul 2024 18:08:16 -0400 Subject: [PATCH 3/3] Set version to 2.2.1 --- src/ripple/protocol/impl/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ripple/protocol/impl/BuildInfo.cpp b/src/ripple/protocol/impl/BuildInfo.cpp index 8a83011e906..e6359cd3a52 100644 --- a/src/ripple/protocol/impl/BuildInfo.cpp +++ b/src/ripple/protocol/impl/BuildInfo.cpp @@ -33,7 +33,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "2.2.0" +char const* const versionString = "2.2.1" // clang-format on #if defined(DEBUG) || defined(SANITIZER)