From f04b4e066f71302c29f126cbc62bedc16d974ab4 Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Mon, 29 Jul 2024 16:30:02 -0400 Subject: [PATCH] Disallow filtering account_objects by unsupported types (#5056) * `account_objects` returns an invalid field error if `type` is not supported. This includes objects an account can't own, or which are unsupported by `account_objects` * Includes: * Amendments * Directory Node * Fee Settings * Ledger Hashes * Negative UNL --- src/test/rpc/AccountObjects_test.cpp | 159 ++++++++++++---------- src/xrpld/rpc/detail/RPCHelpers.cpp | 16 +++ src/xrpld/rpc/detail/RPCHelpers.h | 9 ++ src/xrpld/rpc/handlers/AccountObjects.cpp | 3 + 4 files changed, 114 insertions(+), 73 deletions(-) diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index b1ae77245..d64137ca6 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -578,11 +578,11 @@ public: Env env(*this, features); // Make a lambda we can use to get "account_objects" easily. - auto acct_objs = [&env]( - AccountID const& acct, - std::optional const& type, - std::optional limit = std::nullopt, - std::optional marker = std::nullopt) { + auto acctObjs = [&env]( + AccountID const& acct, + std::optional const& type, + std::optional limit = std::nullopt, + std::optional marker = std::nullopt) { Json::Value params; params[jss::account] = to_string(acct); if (type) @@ -596,34 +596,44 @@ public: }; // Make a lambda that easily identifies the size of account objects. - auto acct_objs_is_size = [](Json::Value const& resp, unsigned size) { + auto acctObjsIsSize = [](Json::Value const& resp, unsigned size) { return resp[jss::result][jss::account_objects].isArray() && (resp[jss::result][jss::account_objects].size() == size); }; + // Make a lambda that checks if the response has error for invalid type + auto acctObjsTypeIsInvalid = [](Json::Value const& resp) { + return resp[jss::result].isMember(jss::error) && + resp[jss::result][jss::error_message] == + "Invalid field \'type\'."; + }; + env.fund(XRP(10000), gw, alice); env.close(); // Since the account is empty now, all account objects should come // back empty. - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::account), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amendments), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::check), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::deposit_preauth), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::directory), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::escrow), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::fee), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hashes), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::nft_page), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::offer), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::payment_channel), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::signer_list), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::state), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::ticket), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::uri_token), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hook), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amm), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::did), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::account), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::check), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::deposit_preauth), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::escrow), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::nft_page), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::offer), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::payment_channel), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::signer_list), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::state), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::ticket), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::uri_token), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::hook), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::amm), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::did), 0)); + + // we expect invalid field type reported for the following types + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::amendments))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::directory))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::fee))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::hashes))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::NegativeUNL))); // gw mints an NFT so we can find it. uint256 const nftID{token::getNextID(env, gw, 0u, tfTransferable)}; @@ -631,8 +641,8 @@ public: env.close(); { // Find the NFToken page and make sure it's the right one. - Json::Value const resp = acct_objs(gw, jss::nft_page); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::nft_page); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& nftPage = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(nftPage[sfNFTokens.jsonName].size() == 1); @@ -648,8 +658,8 @@ public: env.close(); { // Find the trustline and make sure it's the right one. - Json::Value const resp = acct_objs(gw, jss::state); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::state); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& state = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(state[sfBalance.jsonName][jss::value].asInt() == -5); @@ -661,8 +671,8 @@ public: env.close(); { // Find the check. - Json::Value const resp = acct_objs(gw, jss::check); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::check); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& check = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(check[sfAccount.jsonName] == gw.human()); @@ -674,8 +684,8 @@ public: env.close(); { // Find the preauthorization. - Json::Value const resp = acct_objs(gw, jss::deposit_preauth); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::deposit_preauth); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& preauth = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(preauth[sfAccount.jsonName] == gw.human()); @@ -696,8 +706,8 @@ public: } { // Find the escrow. - Json::Value const resp = acct_objs(gw, jss::escrow); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::escrow); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& escrow = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(escrow[sfAccount.jsonName] == gw.human()); @@ -713,7 +723,7 @@ public: features | FeatureBitset{featureXChainBridge}); x.createScBridgeObjects(scEnv); - auto scenv_acct_objs = [&](Account const& acct, char const* type) { + auto scEnvAcctObjs = [&](Account const& acct, char const* type) { Json::Value params; params[jss::account] = acct.human(); params[jss::type] = type; @@ -722,9 +732,9 @@ public: }; Json::Value const resp = - scenv_acct_objs(Account::master, jss::bridge); + scEnvAcctObjs(Account::master, jss::bridge); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& acct_bridge = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT( @@ -763,7 +773,7 @@ public: scEnv(xchain_create_claim_id(x.scBob, x.jvb, x.reward, x.mcBob)); scEnv.close(); - auto scenv_acct_objs = [&](Account const& acct, char const* type) { + auto scEnvAcctObjs = [&](Account const& acct, char const* type) { Json::Value params; params[jss::account] = acct.human(); params[jss::type] = type; @@ -774,8 +784,8 @@ public: { // Find the xchain sequence number for Andrea. Json::Value const resp = - scenv_acct_objs(x.scAlice, jss::xchain_owned_claim_id); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + scEnvAcctObjs(x.scAlice, jss::xchain_owned_claim_id); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& xchain_seq = resp[jss::result][jss::account_objects][0u]; @@ -787,8 +797,8 @@ public: { // and the one for Bob Json::Value const resp = - scenv_acct_objs(x.scBob, jss::xchain_owned_claim_id); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + scEnvAcctObjs(x.scBob, jss::xchain_owned_claim_id); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& xchain_seq = resp[jss::result][jss::account_objects][0u]; @@ -823,7 +833,7 @@ public: x.signers[0])); scEnv.close(); - auto scenv_acct_objs = [&](Account const& acct, char const* type) { + auto scEnvAcctObjs = [&](Account const& acct, char const* type) { Json::Value params; params[jss::account] = acct.human(); params[jss::type] = type; @@ -833,9 +843,9 @@ public: { // Find the xchain_create_account_claim_id - Json::Value const resp = scenv_acct_objs( + Json::Value const resp = scEnvAcctObjs( Account::master, jss::xchain_owned_create_account_claim_id); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& xchain_create_account_claim_id = resp[jss::result][jss::account_objects][0u]; @@ -854,8 +864,8 @@ public: env.close(); { // Find the offer. - Json::Value const resp = acct_objs(gw, jss::offer); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::offer); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& offer = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(offer[sfAccount.jsonName] == gw.human()); @@ -879,8 +889,8 @@ public: } { // Find the payment channel. - Json::Value const resp = acct_objs(gw, jss::payment_channel); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::payment_channel); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& payChan = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(payChan[sfAccount.jsonName] == gw.human()); @@ -901,8 +911,8 @@ public: } { // Find the DID. - Json::Value const resp = acct_objs(gw, jss::did); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::did); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& did = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(did[sfAccount.jsonName] == gw.human()); @@ -913,8 +923,8 @@ public: env.close(); { // Find the signer list. - Json::Value const resp = acct_objs(gw, jss::signer_list); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::signer_list); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& signerList = resp[jss::result][jss::account_objects][0u]; @@ -929,8 +939,8 @@ public: env.close(); { // Find the ticket. - Json::Value const resp = acct_objs(gw, jss::ticket); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::ticket); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& ticket = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(ticket[sfAccount.jsonName] == gw.human()); @@ -951,8 +961,8 @@ public: { // Find the uri token. std::string const uri(maxTokenURILength, '?'); - Json::Value const resp = acct_objs(gw, jss::uri_token); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::uri_token); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& uritoken = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(uritoken[sfOwner.jsonName] == gw.human()); @@ -979,8 +989,8 @@ public: } { // Find the hook. - Json::Value const resp = acct_objs(gw, jss::hook); - BEAST_EXPECT(acct_objs_is_size(resp, 1)); + Json::Value const resp = acctObjs(gw, jss::hook); + BEAST_EXPECT(acctObjsIsSize(resp, 1)); auto const& hook = resp[jss::result][jss::account_objects][0u]; BEAST_EXPECT(hook[sfAccount.jsonName] == gw.human()); } @@ -1007,7 +1017,7 @@ public: std::uint32_t const expectedAccountObjects{ static_cast(std::size(expectedLedgerTypes))}; - if (BEAST_EXPECT(acct_objs_is_size(resp, expectedAccountObjects))) + if (BEAST_EXPECT(acctObjsIsSize(resp, expectedAccountObjects))) { auto const& aobjs = resp[jss::result][jss::account_objects]; std::vector gotLedgerTypes; @@ -1030,7 +1040,7 @@ public: params[jss::type] = jss::escrow; auto resp = env.rpc("json", "account_objects", to_string(params)); - if (BEAST_EXPECT(acct_objs_is_size(resp, 1u))) + if (BEAST_EXPECT(acctObjsIsSize(resp, 1u))) { auto const& aobjs = resp[jss::result][jss::account_objects]; BEAST_EXPECT(aobjs[0u]["LedgerEntryType"] == jss::Escrow); @@ -1051,7 +1061,7 @@ public: auto expectObjects = [&](Json::Value const& resp, std::vector const& types) -> bool { - if (!acct_objs_is_size(resp, types.size())) + if (!acctObjsIsSize(resp, types.size())) return false; std::vector typesOut; getTypes(resp, typesOut); @@ -1065,13 +1075,13 @@ public: BEAST_EXPECT(lines[jss::lines].size() == 3); // request AMM only, doesn't depend on the limit BEAST_EXPECT( - acct_objs_is_size(acct_objs(amm.ammAccount(), jss::amm), 1)); + acctObjsIsSize(acctObjs(amm.ammAccount(), jss::amm), 1)); // request first two objects - auto resp = acct_objs(amm.ammAccount(), std::nullopt, 2); + auto resp = acctObjs(amm.ammAccount(), std::nullopt, 2); std::vector typesOut; getTypes(resp, typesOut); // request next two objects - resp = acct_objs( + resp = acctObjs( amm.ammAccount(), std::nullopt, 10, @@ -1085,7 +1095,7 @@ public: jss::RippleState.c_str(), jss::RippleState.c_str()})); // filter by state: there are three trustlines - resp = acct_objs(amm.ammAccount(), jss::state, 10); + resp = acctObjs(amm.ammAccount(), jss::state, 10); BEAST_EXPECT(expectObjects( resp, {jss::RippleState.c_str(), @@ -1093,11 +1103,18 @@ public: jss::RippleState.c_str()})); // AMM account doesn't own offers BEAST_EXPECT( - acct_objs_is_size(acct_objs(amm.ammAccount(), jss::offer), 0)); + acctObjsIsSize(acctObjs(amm.ammAccount(), jss::offer), 0)); // gw account doesn't own AMM object - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amm), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::amm), 0)); } + // we still expect invalid field type reported for the following types + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::amendments))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::directory))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::fee))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::hashes))); + BEAST_EXPECT(acctObjsTypeIsInvalid(acctObjs(gw, jss::NegativeUNL))); + // Run up the number of directory entries so gw has two // directory nodes. for (int d = 1'000'032; d >= 1'000'000; --d) @@ -1107,11 +1124,7 @@ public: } // Verify that the non-returning types still don't return anything. - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::account), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::amendments), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::directory), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::fee), 0)); - BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hashes), 0)); + BEAST_EXPECT(acctObjsIsSize(acctObjs(gw, jss::account), 0)); } void diff --git a/src/xrpld/rpc/detail/RPCHelpers.cpp b/src/xrpld/rpc/detail/RPCHelpers.cpp index ff0d05486..a522261aa 100644 --- a/src/xrpld/rpc/detail/RPCHelpers.cpp +++ b/src/xrpld/rpc/detail/RPCHelpers.cpp @@ -1118,6 +1118,22 @@ chooseLedgerEntryType(Json::Value const& params) return result; } +bool +isAccountObjectsValidType(LedgerEntryType const& type) +{ + switch (type) + { + case LedgerEntryType::ltAMENDMENTS: + case LedgerEntryType::ltDIR_NODE: + case LedgerEntryType::ltFEE_SETTINGS: + case LedgerEntryType::ltLEDGER_HASHES: + case LedgerEntryType::ltNEGATIVE_UNL: + return false; + default: + return true; + } +} + beast::SemanticVersion const firstVersion("1.0.0"); beast::SemanticVersion const goodVersion("1.0.0"); beast::SemanticVersion const lastVersion("1.0.0"); diff --git a/src/xrpld/rpc/detail/RPCHelpers.h b/src/xrpld/rpc/detail/RPCHelpers.h index 046e6b0de..c516fdd1d 100644 --- a/src/xrpld/rpc/detail/RPCHelpers.h +++ b/src/xrpld/rpc/detail/RPCHelpers.h @@ -251,6 +251,15 @@ setVersion(Object& parent, unsigned int apiVersion, bool betaEnabled) std::pair chooseLedgerEntryType(Json::Value const& params); +/** + * Check if the type is a valid filtering type for account_objects method + * + * Since Amendments, DirectoryNode, FeeSettings, LedgerHashes can not be + * owned by an account, this function will return false in these situations. + */ +bool +isAccountObjectsValidType(LedgerEntryType const& type); + /** * Retrieve the api version number from the json value * diff --git a/src/xrpld/rpc/handlers/AccountObjects.cpp b/src/xrpld/rpc/handlers/AccountObjects.cpp index fb3eca7ca..c7d27af5a 100644 --- a/src/xrpld/rpc/handlers/AccountObjects.cpp +++ b/src/xrpld/rpc/handlers/AccountObjects.cpp @@ -243,6 +243,9 @@ doAccountObjects(RPC::JsonContext& context) { auto [rpcStatus, type] = RPC::chooseLedgerEntryType(params); + if (!RPC::isAccountObjectsValidType(type)) + return RPC::invalid_field_error(jss::type); + if (rpcStatus) { result.clear();