feat: enforce that account_ids are valid in FunctionCallPermission #7139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matklad
force-pushed
the
m/valid-account-ids
branch
5 times, most recently
from
64d0bfe
to
2396089
Compare
|
Note to self: after this is merged, look into using |
mm-near
approved these changes
Jun 30, 2022
| @@ -417,6 +430,30 @@ fn validate_add_key_action( | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn truncate_string(s: &str, limit: usize) -> String { | |||
There was a problem hiding this comment.
I'm surprised that something like this is not present in default Rust string library..
(or is it there, but we need a custom method for a reason ?)
There was a problem hiding this comment.
I am not surprised -- doing this properly is a hard and domain-specific: you probably don't want to truncate 🏳️🌈 to 🏳️ and for that you need to pull substantial amount of unicode in. And then you can truncate to the number of visual characters, or to the number of fixed-width symbols, or to the number of bytes.
| let got = truncate_string(input, limit); | ||
| assert_eq!(got, want) | ||
| } | ||
| check("hello", 0, ""); |
There was a problem hiding this comment.
can you also check:
check("", 10, "")
| block_hash: CryptoHash::default(), | ||
| }; | ||
|
|
||
| // Run the transaction & collect the logs. |
There was a problem hiding this comment.
I'd also mention -- that we expect this transaction to pass, as this is before the account id enforcement.
| } | ||
| } | ||
|
|
||
| // Re-run the transaction. |
There was a problem hiding this comment.
and comment that we expect this transaction to fail, as this is the protocol version that enforces the account ids.
matklad
force-pushed
the
m/valid-account-ids
branch
from
2396089
to
7e91585
Compare
2 tasks
near-bulldozer bot
pushed a commit
that referenced
this pull request
Sep 7, 2022
…re (#7569) # account_id_in_function_call_permission This feature controls [the check](https://github.com/near/nearcore/blob/b315192e84d388671316deaa3a17ece9d0565fd1/runtime/runtime/src/verifier.rs#L400-L405) which enforces that account id in function call permission is indeed a valid account id. Before, any string could have been used there. The primary motivation is robustness -- by restricting permissions to only valid account ids, we don't have to deal with potentially arbitrary long strings in storage. # Context - Implementation: #7139 # Testing and QA We have basic and upgradability test [here](https://github.com/near/nearcore/blob/master/integration-tests/src/tests/client/features/account_id_in_function_call_permission.rs#L18). This PR also adds a test for an extra edge case with overly long account id. This feature have been running on betanet for couple of months without problems. # Checklist - [x] Link to nightly nayduck run: https://nayduck.near.org/#/run/2667 - [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
nikurt
pushed a commit
that referenced
this pull request
Sep 7, 2022
…re (#7569) # account_id_in_function_call_permission This feature controls [the check](https://github.com/near/nearcore/blob/b315192e84d388671316deaa3a17ece9d0565fd1/runtime/runtime/src/verifier.rs#L400-L405) which enforces that account id in function call permission is indeed a valid account id. Before, any string could have been used there. The primary motivation is robustness -- by restricting permissions to only valid account ids, we don't have to deal with potentially arbitrary long strings in storage. # Context - Implementation: #7139 # Testing and QA We have basic and upgradability test [here](https://github.com/near/nearcore/blob/master/integration-tests/src/tests/client/features/account_id_in_function_call_permission.rs#L18). This PR also adds a test for an extra edge case with overly long account id. This feature have been running on betanet for couple of months without problems. # Checklist - [x] Link to nightly nayduck run: https://nayduck.near.org/#/run/2667 - [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
nikurt
pushed a commit
that referenced
this pull request
Nov 9, 2022
…re (#7569) # account_id_in_function_call_permission This feature controls [the check](https://github.com/near/nearcore/blob/b315192e84d388671316deaa3a17ece9d0565fd1/runtime/runtime/src/verifier.rs#L400-L405) which enforces that account id in function call permission is indeed a valid account id. Before, any string could have been used there. The primary motivation is robustness -- by restricting permissions to only valid account ids, we don't have to deal with potentially arbitrary long strings in storage. # Context - Implementation: #7139 # Testing and QA We have basic and upgradability test [here](https://github.com/near/nearcore/blob/master/integration-tests/src/tests/client/features/account_id_in_function_call_permission.rs#L18). This PR also adds a test for an extra edge case with overly long account id. This feature have been running on betanet for couple of months without problems. # Checklist - [x] Link to nightly nayduck run: https://nayduck.near.org/#/run/2667 - [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes https://nearinc.atlassian.net/browse/CPR-135