Drop non-strict account parsing #3330
Assignees
Labels
API Change
Feature Request
Used to indicate requests to add new features
Tech Debt
Non-urgent improvements
Will Need Documentation
Milestone
Comments
mDuo13
added
API Change
Feature Request
|
@drlongle could you add a comment here re: your thoughts / questions? |
|
I would like to know why public keys should not be accepted. Is it because we want to simplify the API and argument parsing (i.e. guessing whether a provided argument is an address or a public key). @mDuo13 Can you clarify? Thanks. |
7 tasks
|
drlongle: What is the rationale for not accepting public key to look up account? Do we have a consensus about that? mDuo13: There is no security issue with using public keys, but for simplicity and consistency, it makes the most sense to accept only one data type per field. I still think the ticket is relevant and worth doing |
drlongle
added a commit
to drlongle/rippled
that referenced
this issue
Mar 1, 2023
drlongle
added a commit
to drlongle/rippled
that referenced
this issue
Mar 30, 2023
drlongle
added a commit
to drlongle/rippled
that referenced
this issue
Mar 31, 2023
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)
|
Resolved by #4404, expected to be released in v1.11, target release date ~June 2023. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
Relating to #3329, I think non-strict account parsing should be removed.
Motivation
Accepting secret values in the API just encourages poor operational security and makes the API definition more ambiguous.
Solution
This is a breaking change requiring an API version bump.[Core Ledger team was not able to find an example of a real app relying on non-strict account parsing.]All API fields that accept account addresses in base58 should not accept passphrases, secret keys, public keys, RFC-1751 phrases, or other variant forms. (X-Address support would be fine though.)
This applies to all fields named
account, as well as some other cases likepeer,taker,issuer, members ofhotwallets, etc. I haven't extensively tested which of these fields accept secrets/etc. but the answer should be "none of them."After doing this, the
strictparameter can be removed from all API methods that have it.The text was updated successfully, but these errors were encountered: