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

Jump to bottom

fix account_objects with invalid marker do not return an error #5046

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

fix account_objects with invalid marker do not return an error #5046

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
627655e
fix account_objects with invalid marker
yinyiqian1 Oct 4, 2024
fb37fc7
Add comment
yinyiqian1 Oct 9, 2024
029c474
add more tests
yinyiqian1 Oct 10, 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
99 changes: 99 additions & 0 deletions src/test/rpc/AccountObjects_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,104 @@ class AccountObjects_test : public beast::unit_test::suite
}
}

void
testAccountObjectMarker()
{
testcase("AccountObjectMarker");

using namespace jtx;
Env env(*this);

Account const alice{"alice"};
Account const bob{"bob"};
env.fund(XRP(10000), alice, bob);

unsigned const accountObjectSize = 30;
for (unsigned i = 0; i < accountObjectSize; i++)
env(check::create(alice, bob, XRP(10)));

env.close();

unsigned const limit = 11;
Json::Value marker;
// test account_objects with a limit and update marker
{
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::ledger_index] = "validated";
auto resp = env.rpc("json", "account_objects", to_string(params));
auto& accountObjects = resp[jss::result][jss::account_objects];
marker = resp[jss::result][jss::marker];
BEAST_EXPECT(!resp[jss::result].isMember(jss::error));
BEAST_EXPECT(accountObjects.size() == limit);
}

// test account_objects with valid marker and update marker
{
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::marker] = marker;
params[jss::ledger_index] = "validated";
auto resp = env.rpc("json", "account_objects", to_string(params));
auto& accountObjects = resp[jss::result][jss::account_objects];
marker = resp[jss::result][jss::marker];
BEAST_EXPECT(!resp[jss::result].isMember(jss::error));
BEAST_EXPECT(accountObjects.size() == limit);
}

// continue getting account_objects with valid marker
{
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::marker] = marker;
params[jss::ledger_index] = "validated";
auto resp = env.rpc("json", "account_objects", to_string(params));
auto& accountObjects = resp[jss::result][jss::account_objects];
BEAST_EXPECT(!resp[jss::result].isMember(jss::error));
BEAST_EXPECT(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add here check that this is the last page:

BEAST_EXPECT(!resp[jss::result].isMember(jss::marker));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

accountObjects.size() == accountObjectSize - limit * 2);
}

// test account_objects with an invalid marker containing invalid
// dirIndex
{
std::string s = marker.asString();
s.replace(0, 7, "FFFFFFF");
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::marker] = s;
params[jss::ledger_index] = "validated";
auto resp = env.rpc("json", "account_objects", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field \'marker\'.");
}

// test account_objects with an invalid marker containing invalid
// entryIndex
{
// construct an invalid entry
std::stringstream ss(marker.asString());
std::string s;
std::getline(ss, s, ',');
s = s + ",0";

Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::marker] = s;
params[jss::ledger_index] = "validated";
auto resp = env.rpc("json", "account_objects", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Invalid field \'marker\'.");
}
}

void
run() override
{
Expand All @@ -1229,6 +1327,7 @@ class AccountObjects_test : public beast::unit_test::suite
testObjectTypes();
testNFTsMarker();
testAccountNFTs();
testAccountObjectMarker();
}
};

Expand Down
14 changes: 12 additions & 2 deletions src/xrpld/rpc/detail/RPCHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,18 @@ getAccountObjects(
if (!dir)
{
// it's possible the user had nftoken pages but no
// directory entries
return mlimit < limit;
// directory entries. If there's no nftoken page, we will
// give empty array for account_objects.
if (mlimit >= limit)
jvResult[jss::account_objects] = Json::arrayValue;

yinyiqian1 marked this conversation as resolved.
Show resolved Hide resolved
// non-zero dirIndex validity was checked in the caller function;
// by this point, it should be zero.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as commented elsewhere, I would prefer if this function did not rely on the validation of dirIndex happening in the caller, but rather move that check into the body of getAccountObjects

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

// This function returns true regardless of nftoken page presence;
// if absent, account_objects is already set as an empty array.
// Notice we will only return false in this function when entryIndex
// can not be found, indicating an invalid marker error.
return true;
}

std::uint32_t i = 0;
Expand Down
8 changes: 5 additions & 3 deletions src/xrpld/rpc/handlers/AccountObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,10 @@ doAccountObjects(RPC::JsonContext& context)
return RPC::invalid_field_error(jss::marker);
}

// check if dirIndex is valid
if (!dirIndex.isZero() && !ledger->read({ltDIR_NODE, dirIndex}))
return RPC::invalid_field_error(jss::marker);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since RPC::getAccountObjects returning false would have the same result, I would move this check into the body of RPC::getAccountObjects; this will keep the checks (other than the marker format, in the code segment above) in that one function only, which is more robust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated


if (!RPC::getAccountObjects(
*ledger,
accountID,
Expand All @@ -291,9 +295,7 @@ doAccountObjects(RPC::JsonContext& context)
entryIndex,
limit,
result))
{
result[jss::account_objects] = Json::arrayValue;
}
return RPC::invalid_field_error(jss::marker);

result[jss::account] = toBase58(accountID);
context.loadType = Resource::feeMediumBurdenRPC;
Expand Down
Loading