Skip to content

Commit

Permalink
Disallow filtering account_objects by unsupported types (#5056)
Browse files Browse the repository at this point in the history 
* `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
  • Loading branch information
@yinyiqian1
yinyiqian1 authored Jul 29, 2024
1 parent 21a0a64 commit d54151e
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 67 deletions.
147 changes: 80 additions & 67 deletions src/test/rpc/AccountObjects_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,11 +558,11 @@ class AccountObjects_test : public beast::unit_test::suite
Env env(*this, features);

// Make a lambda we can use to get "account_objects" easily.
auto acct_objs = [&env](
AccountID const& acct,
std::optional<Json::StaticString> const& type,
std::optional<std::uint16_t> limit = std::nullopt,
std::optional<std::string> marker = std::nullopt) {
auto acctObjs = [&env](
AccountID const& acct,
std::optional<Json::StaticString> const& type,
std::optional<std::uint16_t> limit = std::nullopt,
std::optional<std::string> marker = std::nullopt) {
Json::Value params;
params[jss::account] = to_string(acct);
if (type)
Expand All @@ -576,41 +576,51 @@ class AccountObjects_test : public beast::unit_test::suite
};

// 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::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::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)};
env(token::mint(gw, 0u), txflags(tfTransferable));
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);
Expand All @@ -626,8 +636,8 @@ class AccountObjects_test : public beast::unit_test::suite
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);
Expand All @@ -639,8 +649,8 @@ class AccountObjects_test : public beast::unit_test::suite
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());
Expand All @@ -652,8 +662,8 @@ class AccountObjects_test : public beast::unit_test::suite
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());
Expand All @@ -674,8 +684,8 @@ class AccountObjects_test : public beast::unit_test::suite
}
{
// 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());
Expand All @@ -688,7 +698,7 @@ class AccountObjects_test : public beast::unit_test::suite
Env scEnv(*this, envconfig(port_increment, 3), features);
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;
Expand All @@ -697,9 +707,9 @@ class AccountObjects_test : public beast::unit_test::suite
};

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(
Expand Down Expand Up @@ -735,7 +745,7 @@ class AccountObjects_test : public beast::unit_test::suite
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;
Expand All @@ -746,8 +756,8 @@ class AccountObjects_test : public beast::unit_test::suite
{
// 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];
Expand All @@ -759,8 +769,8 @@ class AccountObjects_test : public beast::unit_test::suite
{
// 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];
Expand Down Expand Up @@ -792,7 +802,7 @@ class AccountObjects_test : public beast::unit_test::suite
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;
Expand All @@ -802,9 +812,9 @@ class AccountObjects_test : public beast::unit_test::suite

{
// 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];
Expand All @@ -823,8 +833,8 @@ class AccountObjects_test : public beast::unit_test::suite
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());
Expand All @@ -848,8 +858,8 @@ class AccountObjects_test : public beast::unit_test::suite
}
{
// 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());
Expand All @@ -870,8 +880,8 @@ class AccountObjects_test : public beast::unit_test::suite
}
{
// 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());
Expand All @@ -882,8 +892,8 @@ class AccountObjects_test : public beast::unit_test::suite
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];
Expand All @@ -898,8 +908,8 @@ class AccountObjects_test : public beast::unit_test::suite
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());
Expand Down Expand Up @@ -927,7 +937,7 @@ class AccountObjects_test : public beast::unit_test::suite
std::uint32_t const expectedAccountObjects{
static_cast<std::uint32_t>(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<std::string> gotLedgerTypes;
Expand All @@ -950,7 +960,7 @@ class AccountObjects_test : public beast::unit_test::suite
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);
Expand All @@ -971,7 +981,7 @@ class AccountObjects_test : public beast::unit_test::suite
auto expectObjects =
[&](Json::Value const& resp,
std::vector<std::string> const& types) -> bool {
if (!acct_objs_is_size(resp, types.size()))
if (!acctObjsIsSize(resp, types.size()))
return false;
std::vector<std::string> typesOut;
getTypes(resp, typesOut);
Expand All @@ -985,13 +995,13 @@ class AccountObjects_test : public beast::unit_test::suite
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<std::string> typesOut;
getTypes(resp, typesOut);
// request next two objects
resp = acct_objs(
resp = acctObjs(
amm.ammAccount(),
std::nullopt,
10,
Expand All @@ -1005,19 +1015,26 @@ class AccountObjects_test : public beast::unit_test::suite
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(),
jss::RippleState.c_str(),
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)
Expand All @@ -1027,11 +1044,7 @@ class AccountObjects_test : public beast::unit_test::suite
}

// 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
Expand Down
16 changes: 16 additions & 0 deletions src/xrpld/rpc/detail/RPCHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,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");
Expand Down
9 changes: 9 additions & 0 deletions src/xrpld/rpc/detail/RPCHelpers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ setVersion(Object& parent, unsigned int apiVersion, bool betaEnabled)
std::pair<RPC::Status, LedgerEntryType>
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
*
Expand Down
3 changes: 3 additions & 0 deletions src/xrpld/rpc/handlers/AccountObjects.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,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();
Expand Down

0 comments on commit d54151e

Please sign in to comment.