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

feat: enforce that account_ids are valid in FunctionCallPermission #7139

Merged
merged 2 commits into from
Jun 30, 2022
Merged

feat: enforce that account_ids are valid in FunctionCallPermission #7139

merged 2 commits into from
Jun 30, 2022

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Jun 29, 2022

@matklad matklad force-pushed the m/valid-account-ids branch 5 times, most recently from 64d0bfe to 2396089 Compare June 29, 2022 16:14
@matklad matklad marked this pull request as ready for review June 29, 2022 17:41
@matklad matklad requested a review from a team as a code owner June 29, 2022 17:41
@matklad
Copy link
Contributor Author

matklad commented Jun 29, 2022

Note to self: after this is merged, look into using serde_repr to reduce the serialization boilerplate, and into adding TestEnv::upgrade_protocol

mm-near
mm-near approved these changes Jun 30, 2022
View reviewed changes
runtime/runtime/src/verifier.rs
@@ -417,6 +430,30 @@ fn validate_add_key_action(
Ok(())
}

fn truncate_string(s: &str, limit: usize) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

runtime/runtime/src/verifier.rs
let got = truncate_string(input, limit);
assert_eq!(got, want)
}
check("hello", 0, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also check:

check("", 10, "")

integration-tests/src/tests/client/process_blocks.rs Outdated
block_hash: CryptoHash::default(),
};

// Run the transaction & collect the logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also mention -- that we expect this transaction to pass, as this is before the account id enforcement.

integration-tests/src/tests/client/process_blocks.rs Outdated
}
}

// Re-run the transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

and comment that we expect this transaction to fail, as this is the protocol version that enforces the account ids.

@near-bulldozer near-bulldozer bot merged commit db84d6d into near:master Jun 30, 2022
@matklad matklad deleted the m/valid-account-ids branch June 30, 2022 18:24
@matklad matklad mentioned this pull request Jul 18, 2022
Open
@matklad matklad mentioned this pull request Sep 6, 2022
Merged
2 tasks
near-bulldozer bot pushed a commit that referenced this pull request Sep 7, 2022
@matklad
feat: stabilize account_id_in_function_call_permission protocol featu…
85f1479 
…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
@matklad @nikurt
feat: stabilize account_id_in_function_call_permission protocol featu…
930b464 
…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
@matklad @nikurt
feat: stabilize account_id_in_function_call_permission protocol featu…
3cba614 
…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
Reviewers

@mm-near mm-near mm-near approved these changes

@Longarithm Longarithm Awaiting requested review from Longarithm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matklad @mm-near