Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix account_objects to disallow filtering by types that an account ca… #5056

Merged
merged 6 commits into from
Jul 29, 2024
Merged

fix account_objects to disallow filtering by types that an account ca… #5056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f4fa4b
fix account_objects to disallow filtering by types that an account ca…
yinyiqian1 Jun 24, 2024
b8b4e88
add NegativeUNL as invalid and resolve switch warning
yinyiqian1 Jun 25, 2024
6b9c140
[FOLD] Suggestions for testing invalid account_objects types
scottschurr Jul 3, 2024
2e6e3f7
[FOLD] Suggested drive-by lambda name fixes
scottschurr Jul 4, 2024
4a6b6fd
Merge branch 'develop' into br_fix_invalid_type
yinyiqian1 Jul 24, 2024
849276d
Merge branch 'develop' into br_fix_invalid_type
ximinez Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 80 additions & 67 deletions src/test/rpc/AccountObjects_test.cpp
View file
Open in desktop
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
Open in desktop
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:
yinyiqian1 marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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))
yinyiqian1 marked this conversation as resolved.
Show resolved Hide resolved
return RPC::invalid_field_error(jss::type);

if (rpcStatus)
{
result.clear();
Expand Down
Loading