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 with invalid marker do not return an error #5046

Merged
merged 10 commits into from
Oct 29, 2024
Merged

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
10 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
1bc0bc9
Modify the way parsing marker string
yinyiqian1 Oct 14, 2024
141349c
Add more tests
yinyiqian1 Oct 15, 2024
d75eed3
Merge branch 'develop' into br_fix_account_obj_marker
Bronek Oct 16, 2024
66857f0
update unit tests
yinyiqian1 Oct 16, 2024
3a5a707
update unit test
yinyiqian1 Oct 17, 2024
617d4dd
Merge branch 'develop' into br_fix_account_obj_marker
yinyiqian1 Oct 25, 2024
8f1da51
Merge branch 'develop' into br_fix_account_obj_marker
ximinez Oct 29, 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;
Bronek marked this conversation as resolved.
Show resolved Hide resolved
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";
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you pls also test when this gets corrupted not by ,0 but by a , alone ? i.e. valid dirIndex followed by a coma, then end of string. Also reverse, i.e. string starts with a coma, followed by valid entryIndex, then end of string. Reading up the implementation these both should return the same kind of error, but you need to have unit tests so that the code (which I think is good at this moment) does not degrade due to bad changes in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. In the recent commit, I added a lambda function to test invalid marker situations. And also added more invalid marker cases.


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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

really nice improvement in parsing here ! 👍


// 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