From d34e8be3162a5e89e268a5f053de499e9560280f Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Thu, 29 Jun 2023 11:41:13 -0400 Subject: [PATCH] APIv2: add error messages for account_tx (#4571) Certain inputs for the AccountTx method should return an error. In other words, an invalid request from a user or client now results in an error message. Since this can change the response from the API, it is an API breaking change. This commit maintains backward compatibility by keeping the existing behavior for existing requests. When clients specify "api_version": 2, they will be able to get the updated error messages. Update unit tests to check the error based on the API version. * Fix #4288 * Fix #4545 --- src/ripple/rpc/handlers/AccountTx.cpp | 29 +++- src/test/rpc/AccountTx_test.cpp | 237 +++++++++++++++++--------- 2 files changed, 185 insertions(+), 81 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index f65657d92ea..bfbc76362a3 100644 --- a/src/ripple/rpc/handlers/AccountTx.cpp +++ b/src/ripple/rpc/handlers/AccountTx.cpp @@ -53,9 +53,23 @@ using LedgerSpecifier = RelationalDatabase::LedgerSpecifier; // parses args into a ledger specifier, or returns a Json object on error std::variant, Json::Value> -parseLedgerArgs(Json::Value const& params) +parseLedgerArgs(RPC::Context& context, Json::Value const& params) { Json::Value response; + // if ledger_index_min or max is specified, then ledger_hash or ledger_index + // should not be specified. Error out if it is + if (context.apiVersion > 1) + { + if ((params.isMember(jss::ledger_index_min) || + params.isMember(jss::ledger_index_max)) && + (params.isMember(jss::ledger_hash) || + params.isMember(jss::ledger_index))) + { + RPC::Status status{rpcINVALID_PARAMS, "invalidParams"}; + status.inject(response); + return response; + } + } if (params.isMember(jss::ledger_index_min) || params.isMember(jss::ledger_index_max)) { @@ -145,6 +159,17 @@ getLedgerRange( using T = std::decay_t; if constexpr (std::is_same_v) { + // if ledger_index_min or ledger_index_max is out of + // valid ledger range, error out. exclude -1 as + // it is a valid input + if (context.apiVersion > 1) + { + if ((ls.max > uValidatedMax && ls.max != -1) || + (ls.min < uValidatedMin && ls.min != 0)) + { + return rpcLGR_IDX_MALFORMED; + } + } if (ls.min > uValidatedMin) { uLedgerMin = ls.min; @@ -379,7 +404,7 @@ doAccountTxJson(RPC::JsonContext& context) args.account = *account; - auto parseRes = parseLedgerArgs(params); + auto parseRes = parseLedgerArgs(context, params); if (auto jv = std::get_if(&parseRes)) { return *jv; diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 75147875d1a..2e09ad93b86 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -108,7 +108,7 @@ class AccountTx_test : public beast::unit_test::suite }; void - testParameters() + testParameters(unsigned int apiVersion) { using namespace test::jtx; @@ -143,104 +143,178 @@ class AccountTx_test : public beast::unit_test::suite }; Json::Value jParms; + jParms[jss::api_version] = apiVersion; - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(jParms)), - rpcINVALID_PARAMS)); - - jParms[jss::account] = "0xDEADBEEF"; - - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(jParms)), - rpcACT_MALFORMED)); - - jParms[jss::account] = A1.human(); - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(jParms)))); - - // Ledger min/max index + if (apiVersion < 2) { - Json::Value p{jParms}; - p[jss::ledger_index_min] = -1; - p[jss::ledger_index_max] = -1; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); - - p[jss::ledger_index_min] = 0; - p[jss::ledger_index_max] = 100; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(jParms)), + rpcINVALID_PARAMS)); - p[jss::ledger_index_min] = 1; - p[jss::ledger_index_max] = 2; - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); + jParms[jss::account] = "0xDEADBEEF"; - p[jss::ledger_index_min] = 2; - p[jss::ledger_index_max] = 1; BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(p)), - (RPC::apiMaximumSupportedVersion == 1 ? rpcLGR_IDXS_INVALID - : rpcINVALID_LGR_RANGE))); - } + env.rpc("json", "account_tx", to_string(jParms)), + rpcACT_MALFORMED)); - // Ledger index min only - { - Json::Value p{jParms}; - p[jss::ledger_index_min] = -1; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + jParms[jss::account] = A1.human(); + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(jParms)))); - p[jss::ledger_index_min] = 1; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + // Ledger min/max index + { + Json::Value p{jParms}; + p[jss::ledger_index_min] = -1; + p[jss::ledger_index_max] = -1; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = 0; + p[jss::ledger_index_max] = 100; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = 1; + p[jss::ledger_index_max] = 2; + BEAST_EXPECT( + noTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = 2; + p[jss::ledger_index_max] = 1; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + (RPC::apiMaximumSupportedVersion == 1 + ? rpcLGR_IDXS_INVALID + : rpcINVALID_LGR_RANGE))); + } - p[jss::ledger_index_min] = env.current()->info().seq; - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(p)), - (RPC::apiMaximumSupportedVersion == 1 ? rpcLGR_IDXS_INVALID - : rpcINVALID_LGR_RANGE))); - } + // Ledger index min only + { + Json::Value p{jParms}; + p[jss::ledger_index_min] = -1; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = 1; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = env.current()->info().seq; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + (RPC::apiMaximumSupportedVersion == 1 + ? rpcLGR_IDXS_INVALID + : rpcINVALID_LGR_RANGE))); + } - // Ledger index max only - { - Json::Value p{jParms}; - p[jss::ledger_index_max] = -1; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + // Ledger index max only + { + Json::Value p{jParms}; + p[jss::ledger_index_max] = -1; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_max] = env.current()->info().seq; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_max] = 3; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_max] = env.closed()->info().seq; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_max] = env.closed()->info().seq - 1; + BEAST_EXPECT( + noTxs(env.rpc("json", "account_tx", to_string(p)))); + } - p[jss::ledger_index_max] = env.current()->info().seq; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + // Ledger Sequence + { + Json::Value p{jParms}; - p[jss::ledger_index_max] = env.closed()->info().seq; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + p[jss::ledger_index] = env.closed()->info().seq; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); - p[jss::ledger_index_max] = env.closed()->info().seq - 1; - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); - } + p[jss::ledger_index] = env.closed()->info().seq - 1; + BEAST_EXPECT( + noTxs(env.rpc("json", "account_tx", to_string(p)))); - // Ledger Sequence - { - Json::Value p{jParms}; + p[jss::ledger_index] = env.current()->info().seq; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcLGR_NOT_VALIDATED)); - p[jss::ledger_index] = env.closed()->info().seq; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + p[jss::ledger_index] = env.current()->info().seq + 1; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcLGR_NOT_FOUND)); + } - p[jss::ledger_index] = env.closed()->info().seq - 1; - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); + // Ledger Hash + { + Json::Value p{jParms}; - p[jss::ledger_index] = env.current()->info().seq; - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(p)), - rpcLGR_NOT_VALIDATED)); + p[jss::ledger_hash] = to_string(env.closed()->info().hash); + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); - p[jss::ledger_index] = env.current()->info().seq + 1; - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(p)), rpcLGR_NOT_FOUND)); + p[jss::ledger_hash] = + to_string(env.closed()->info().parentHash); + BEAST_EXPECT( + noTxs(env.rpc("json", "account_tx", to_string(p)))); + } } - - // Ledger Hash + else { - Json::Value p{jParms}; + // Ledger index max/min/index all specified + // ERRORS out with invalid Parenthesis + { + jParms[jss::account] = "0xDEADBEEF"; + jParms[jss::account] = A1.human(); + Json::Value p{jParms}; - p[jss::ledger_hash] = to_string(env.closed()->info().hash); - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + p[jss::ledger_index_max] = -1; + p[jss::ledger_index_min] = -1; + p[jss::ledger_index] = -1; - p[jss::ledger_hash] = to_string(env.closed()->info().parentHash); - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcINVALID_PARAMS)); + } + + // Ledger index min/max only + { + Json::Value p{jParms}; + p[jss::ledger_index_max] = 100; + p[jss::ledger_index_min] = 0; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcLGR_IDX_MALFORMED)); + + p[jss::ledger_index_max] = -1; + p[jss::ledger_index_min] = -1; + BEAST_EXPECT( + hasTxs(env.rpc("json", "account_tx", to_string(p)))); + + p[jss::ledger_index_min] = 2; + p[jss::ledger_index_max] = 1; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcINVALID_LGR_RANGE)); + } + + // Ledger index max only + { + Json::Value p{jParms}; + p[jss::ledger_index_max] = env.current()->info().seq; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcLGR_IDX_MALFORMED)); + } } } @@ -593,7 +667,12 @@ class AccountTx_test : public beast::unit_test::suite void run() override { - testParameters(); + for (auto testVersion = RPC::apiMinimumSupportedVersion; + testVersion <= RPC::apiBetaVersion; + ++testVersion) + { + testParameters(testVersion); + } testContents(); testAccountDelete(); }