From 73550a4bfc65c661cbf475540cd2ed692fc68aec 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 | 251 +++++++++++++++++--------- 2 files changed, 192 insertions(+), 88 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountTx.cpp b/src/ripple/rpc/handlers/AccountTx.cpp index f65657d92..bfbc76362 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 85dd2978d..d9215086d 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -111,7 +111,7 @@ class AccountTx_test : public beast::unit_test::suite }; void - testParameters() + testParameters(unsigned int apiVersion) { using namespace test::jtx; @@ -146,104 +146,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)))); - - 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))); - } + env.rpc("json", "account_tx", to_string(jParms)), + rpcINVALID_PARAMS)); - // 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] = "0xDEADBEEF"; - 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))); + 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 + { + 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))); + } + + // 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)))); + + 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)))); + } + + // Ledger Sequence + { + Json::Value p{jParms}; + + p[jss::ledger_index] = env.closed()->info().seq; + BEAST_EXPECT( + hasTxs(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)))); + + 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.current()->info().seq + 1; + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcLGR_NOT_FOUND)); + } + + // Ledger Hash + { + 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_hash] = + to_string(env.closed()->info().parentHash); + BEAST_EXPECT( + noTxs(env.rpc("json", "account_tx", to_string(p)))); + } } - - // Ledger index max only + else { - Json::Value p{jParms}; - p[jss::ledger_index_max] = -1; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + // 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_index_max] = env.current()->info().seq; - 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_index_max] = env.closed()->info().seq; - BEAST_EXPECT(hasTxs(env.rpc("json", "account_tx", to_string(p)))); + BEAST_EXPECT(isErr( + env.rpc("json", "account_tx", to_string(p)), + rpcINVALID_PARAMS)); + } - p[jss::ledger_index_max] = env.closed()->info().seq - 1; - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); - } + // 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)); - // Ledger Sequence - { - Json::Value p{jParms}; + 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] = env.closed()->info().seq; - 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)); + } - p[jss::ledger_index] = env.closed()->info().seq - 1; - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); - - 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.current()->info().seq + 1; - BEAST_EXPECT(isErr( - env.rpc("json", "account_tx", to_string(p)), rpcLGR_NOT_FOUND)); - } - - // Ledger Hash - { - 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_hash] = to_string(env.closed()->info().parentHash); - BEAST_EXPECT(noTxs(env.rpc("json", "account_tx", to_string(p)))); + // 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)); + } } } @@ -780,7 +854,12 @@ public: void run() override { - testParameters(); + for (auto testVersion = RPC::apiMinimumSupportedVersion; + testVersion <= RPC::apiBetaVersion; + ++testVersion) + { + testParameters(testVersion); + } testContents(); testAccountDelete(); }