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.

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

account_objects CLI usage statement is wrong (Version: 1.5.0) #3329

Closed
mDuo13 opened this issue Apr 2, 2020 · 2 comments · Fixed by #4404
Closed

account_objects CLI usage statement is wrong (Version: 1.5.0) #3329

mDuo13 opened this issue Apr 2, 2020 · 2 comments · Fixed by #4404
Labels
API Change Bug Feature Request Used to indicate requests to add new features Good First Issue Great issue for a new contributor

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented Apr 2, 2020

Issue Description

(Discovered while documenting #3196.) The usage statement for the account_objects method is wrong. [Source code]

The account_objects method does not support the strict parameter, but the commandline implies it does.

Steps to Reproduce

Start rippled in stand-alone mode, then run this command:

rippled account_objects masterpassphrase current strict

Expected Result

The result should be an actMalformed error, similar to what you get if you do account_info with the same arguments.

{
   "result" : {
      "error" : "actMalformed",
      "error_code" : 35,
      "error_message" : "Account malformed.",
      "request" : {
         "account" : "masterpassphrase",
         "api_version" : 1,
         "command" : "account_objects",
         "strict" : 1
      },
      "status" : "error"
   }
}

The "easy" fix is to correct the usage statement not to mention the strict parameter, which is not supported in any interface for account_objects. But my preferred fix is to add strict support to account_objects for consistency with other account_* methods.

Actual Result

The server returns a successful response for the genesis account:

{
   "result" : {
      "account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
      "account_objects" : [],
      "ledger_current_index" : 3,
      "status" : "success",
      "validated" : false
   }
}
@mDuo13 mDuo13 added API Change Good First Issue Great issue for a new contributor Bug Feature Request Used to indicate requests to add new features labels Apr 2, 2020
@intelliot
Copy link
Collaborator

Considering #3330, for account_objects specifically, I think a reasonable path forward would be to change it so that strict behavior is the default (i.e. the account field should only accept a public key or XRP Ledger address). Allowing a secret or passphrase is a bug (@nbougalis agrees) and I doubt there are many (if any) apps relying on this behavior, so it's OK to fix this without bumping the api_version. (Of course, if someone prefers to bump the api_version, that's fine too.)

@drlongle
Copy link
Contributor

I created PR 4404 to address #4337. Coincidentally, I also address this issue.

@intelliot intelliot linked a pull request Feb 2, 2023 that will close this issue
7 tasks
intelliot pushed a commit that referenced this issue May 17, 2023
The API would allow seeds (and public keys) to be used in place of
accounts at several locations in the API. For example, when calling
account_info, you could pass `"account": "foo"`. The string "foo" is
treated like a seed, so the method returns `actNotFound` (instead of
`actMalformed`, as most developers would expect). In the early days,
this was a convenience to make testing easier. However, it allows for
poor security practices, so it is no longer a good idea. Allowing a
secret or passphrase is now considered a bug. Previously, it was
controlled by the `strict` option on some methods. With this commit,
since the API does not interpret `account` as `seed`, the option
`strict` is no longer needed and is removed.

Removing this behavior from the API is a [breaking
change](https://xrpl.org/request-formatting.html#breaking-changes). One
could argue that it shouldn't be done without bumping the API version;
however, in this instance, there is no evidence that anyone is using the
API in the "legacy" way. Furthermore, it is a potential security hole,
as it allows users to send secrets to places where they are not needed,
where they could end up in logs, error messages, etc. There's no reason
to take such a risk with a seed/secret, since only the public address is
needed.

Resolves: #3329, #3330, #4337

BREAKING CHANGE: Remove non-strict account parsing (#3330)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Bug Feature Request Used to indicate requests to add new features Good First Issue Great issue for a new contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants