Skip to content

Commit

Permalink
fix: reject invalid markers in account_objects RPC calls (XRPLF#5046)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yinyiqian1 @vlntb
yinyiqian1 authored and vlntb committed Nov 19, 2024
1 parent bab5e65 commit 8b6b092
Show file tree
Hide file tree
Showing 3 changed files with 190 additions and 13 deletions.
168 changes: 168 additions & 0 deletions src/test/rpc/AccountObjects_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,173 @@ 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"};
Account const carol{"carol"};
env.fund(XRP(10000), alice, bob, carol);

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

for (unsigned i = 0; i < 10; i++)
env(token::mint(carol, 0));

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);
}

// this lambda function is used to check invalid marker response.
auto testInvalidMarker = [&](std::string& marker) {
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::ledger_index] = jss::validated;
params[jss::marker] = marker;
Json::Value const resp =
env.rpc("json", "account_objects", to_string(params));
return resp[jss::result][jss::error_message] ==
"Invalid field \'marker\'.";
};

auto const markerStr = marker.asString();
auto const& idx = markerStr.find(',');
auto const dirIndex = markerStr.substr(0, idx);
auto const entryIndex = markerStr.substr(idx + 1);

// test account_objects with an invalid marker that contains no ','
{
std::string s = dirIndex + entryIndex;
BEAST_EXPECT(testInvalidMarker(s));
}

// test invalid marker by adding invalid string after the maker:
// "dirIndex,entryIndex,1234"
{
std::string s = markerStr + ",1234";
BEAST_EXPECT(testInvalidMarker(s));
}

// test account_objects with an invalid marker containing invalid
// dirIndex by replacing some characters from the dirIndex.
{
std::string s = markerStr;
s.replace(0, 7, "FFFFFFF");
BEAST_EXPECT(testInvalidMarker(s));
}

// test account_objects with an invalid marker containing invalid
// entryIndex by replacing some characters from the entryIndex.
{
std::string s = entryIndex;
s.replace(0, 7, "FFFFFFF");
s = dirIndex + ',' + s;
BEAST_EXPECT(testInvalidMarker(s));
}

// test account_objects with an invalid marker containing invalid
// dirIndex with marker: ",entryIndex"
{
std::string s = ',' + entryIndex;
BEAST_EXPECT(testInvalidMarker(s));
}

// test account_objects with marker: "0,entryIndex", this is still
// valid, because when dirIndex = 0, we will use root key to find
// dir.
{
std::string s = "0," + entryIndex;
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));
auto& accountObjects = resp[jss::result][jss::account_objects];
BEAST_EXPECT(!resp[jss::result].isMember(jss::error));
BEAST_EXPECT(accountObjects.size() == limit);
}

// test account_objects with an invalid marker containing invalid
// entryIndex with marker: "dirIndex,"
{
std::string s = dirIndex + ',';
BEAST_EXPECT(testInvalidMarker(s));
}

// test account_objects with an invalid marker containing invalid
// entryIndex with marker: "dirIndex,0"
{
std::string s = dirIndex + ",0";
BEAST_EXPECT(testInvalidMarker(s));
}

// continue getting account_objects with valid marker. This will be the
// last page, so response will not contain any 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(
accountObjects.size() == accountObjectSize - limit * 2);
BEAST_EXPECT(!resp[jss::result].isMember(jss::marker));
}

// test account_objects when the account only have nft pages, but
// provided invalid entry index.
{
Json::Value params;
params[jss::account] = carol.human();
params[jss::limit] = 10;
params[jss::marker] = "0," + entryIndex;
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(accountObjects.size() == 0);
}
}

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

Expand Down
18 changes: 16 additions & 2 deletions src/xrpld/rpc/detail/RPCHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ getAccountObjects(
std::uint32_t const limit,
Json::Value& jvResult)
{
// check if dirIndex is valid
if (!dirIndex.isZero() && !ledger.read({ltDIR_NODE, dirIndex}))
return false;

auto typeMatchesFilter = [](std::vector<LedgerEntryType> const& typeFilter,
LedgerEntryType ledgerType) {
auto it = std::find(typeFilter.begin(), typeFilter.end(), ledgerType);
Expand Down Expand Up @@ -249,8 +253,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;

// non-zero dirIndex validity was checked in the beginning of this
// function; by this point, it should be zero. 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
17 changes: 6 additions & 11 deletions src/xrpld/rpc/handlers/AccountObjects.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,18 +270,15 @@ doAccountObjects(RPC::JsonContext& context)
if (!marker.isString())
return RPC::expected_field_error(jss::marker, "string");

std::stringstream ss(marker.asString());
std::string s;
if (!std::getline(ss, s, ','))
auto const& markerStr = marker.asString();
auto const& idx = markerStr.find(',');
if (idx == std::string::npos)
return RPC::invalid_field_error(jss::marker);

if (!dirIndex.parseHex(s))
if (!dirIndex.parseHex(markerStr.substr(0, idx)))
return RPC::invalid_field_error(jss::marker);

if (!std::getline(ss, s, ','))
return RPC::invalid_field_error(jss::marker);

if (!entryIndex.parseHex(s))
if (!entryIndex.parseHex(markerStr.substr(idx + 1)))
return RPC::invalid_field_error(jss::marker);
}

Expand All @@ -293,9 +290,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

0 comments on commit 8b6b092

Please sign in to comment.