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

APIv2(account_info): handle invalid "signer_lists" value #4585

Merged
merged 5 commits into from
Jun 30, 2023
Merged

APIv2(account_info): handle invalid "signer_lists" value #4585

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33fca64
accountinfo invalid signer_list -> invalidParams
PeterChen13579 Jun 26, 2023
7c970db
small change for scott's comment
PeterChen13579 Jun 29, 2023
d626a6b
comment change
PeterChen13579 Jun 29, 2023
a15df56
clang format
PeterChen13579 Jun 29, 2023
c286cb7
Merge branch 'develop' into AccountTxNoAccountError
intelliot Jun 30, 2023
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
9 changes: 9 additions & 0 deletions src/ripple/rpc/handlers/AccountInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ doAccountInfo(RPC::JsonContext& context)
}
result[jss::account_flags] = std::move(acctFlags);

// The document states that signer_lists is a bool, however
// assigning any string value works. Do not allow this
// This for api v2 onwards
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tiniest of nits: The comment says "... allow this This for ...". Consider removing the capitalized "This".

if (!params[jss::signer_lists].isBool() && context.apiVersion > 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to check the context.apiVersion here? We assume that all the validators are running the latest version of the rippled codebase. Otherwise, the laggards will be incompatible with the rest of the network right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a situation where we don't need to worry about validators very much, at least on the main net. This is an RPC query. It's not typical for main net validators to be required also handle very many RPC queries.

The context.api can be a value passed in by the caller as shown in the test where it says "\"api_version\": 2. So, yes, we should check the api_version. It can be different from one RPC call to the next.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see okay, thanks for the clarification 👍
I'm guessing the version of rippled software is preserved in a different variable right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the software version (e.g., 1.12.0-b1) is elsewhere. The code never needs to check the software version because it is compiled into the binary with whatever software version it has. The api_version is a different kind of beast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see, okay

{
RPC::inject_error(rpcINVALID_PARAMS, result);
return result;
}

// Return SignerList(s) if that is requested.
if (params.isMember(jss::signer_lists) &&
params[jss::signer_lists].asBool())
Expand Down
10 changes: 10 additions & 0 deletions src/test/rpc/AccountInfo_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ class AccountInfo_test : public beast::unit_test::suite
"\"api_version\": 2, \"account\": \"" + alice.human() + "\", " +
"\"signer_lists\": true }";

auto const withSignersAsString = std::string("{ ") +
"\"api_version\": 2, \"account\": \"" + alice.human() + "\", " +
"\"signer_lists\": asdfggh }";

// Alice has no SignerList yet.
{
// account_info without the "signer_lists" argument.
Expand Down Expand Up @@ -263,6 +267,12 @@ class AccountInfo_test : public beast::unit_test::suite
auto const& entry0 = signerEntries[0u][sfSignerEntry.jsonName];
BEAST_EXPECT(entry0[sfSignerWeight.jsonName] == 3);
}
{
// account_info with "signer_lists" as not bool should error out
auto const info = env.rpc("json", "account_info", withSignersAsString);
BEAST_EXPECT(info[jss::status] == "error");
BEAST_EXPECT(info[jss::error] == "invalidParams");
}

// Give alice a big signer list
Account const demon{"demon"};
Expand Down