feat: implement session key authorization on execute#143
Open
blessme247 wants to merge 6 commits intoancore-org:mainfrom
Open
feat: implement session key authorization on execute#143blessme247 wants to merge 6 commits intoancore-org:mainfrom
blessme247 wants to merge 6 commits intoancore-org:mainfrom
Conversation
|
@blessme247 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdated docs and unit-test typings in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Account as AccountContract
participant Ledger as Ledger/Storage
participant Target as TargetContract
Client->>Account: call execute(caller, target, method, args[, session_key_pub])
Account->>Ledger: read Owner, Nonce
alt caller == Owner
Account->>Target: cross-contract call(method, args)
Target-->>Account: result
else caller != Owner
Account->>Ledger: lookup SessionKey by public_key
Ledger-->>Account: SessionKey { expires_at, permissions }
alt valid & has_permission & not_expired
Account->>Target: cross-contract call(method, args)
Target-->>Account: result
else invalid/expired/insufficient
Account-->>Client: return session-key rejection error
end
end
Account->>Ledger: increment/update nonce
Account-->>Client: return success or propagated error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/account/src/lib.rs (2)
203-206: Consider using checked arithmetic for nonce increment.While u64 overflow is practically impossible, using
checked_addorsaturating_addwould be more defensive:- env.storage() - .instance() - .set(&DataKey::Nonce, &(current_nonce + 1)); + let new_nonce = current_nonce.checked_add(1).expect("nonce overflow"); + env.storage() + .instance() + .set(&DataKey::Nonce, &new_nonce);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 203 - 206, The nonce increment should use checked arithmetic to avoid overflow: replace the direct +(current_nonce + 1) with current_nonce.checked_add(1) (or .saturating_add(1)) and handle the None case from checked_add by deciding on behavior (e.g., saturate, panic with a clear error, or log and return an error) before calling env.storage().instance().set with DataKey::Nonce; update the logic around current_nonce, DataKey::Nonce, and the env.storage().instance().set call to use that safe increment.
276-291: Unusedsetup()helper function.The
setup()helper is defined but never used — all tests create their own inline setup. Consider either:
- Removing this unused code
- Refactoring tests to use this helper for consistency
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 276 - 291, The helper function setup() is unused; either delete the entire setup() function (removing the Env/AncoreAccountClient/Address creation and client.initialize(&owner) call) or refactor existing tests to call setup() instead of duplicating inline setup logic—update tests to accept the returned (Env, AncoreAccountClient<'static>, Address) and use those values in place of their current per-test setup so the helper is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/account/src/lib.rs`:
- Around line 513-550: The test test_execute_with_valid_session_key incorrectly
uses mock_all_auths() and a session key unrelated to the caller, so it doesn't
verify caller↔session-key binding; update the test to remove or limit
mock_all_auths() and add two checks using the contract methods add_session_key
and execute: (1) construct a caller whose public key corresponds to the
session_pk (or simulate a valid signature) and assert execute(...) returns true
and nonce increments; (2) use a different caller or a different session_pk and
assert execute(...) fails (returns false or errors) and nonce does not change;
reference functions: test_execute_with_valid_session_key, mock_all_auths,
add_session_key, execute, get_nonce to locate and implement these assertions.
- Around line 157-166: The TypeScript client still calls execute with the old
4-arg signature; update the client method that calls execute (the execute
wrapper in account-contract.ts) to pass the new first parameter caller (so call
execute(caller, to, fn, args, expectedNonce)); adjust any callers/construction
of that wrapper to supply the caller Address/BytesN<32> value and ensure
parameter ordering matches the Rust signature (caller, to, function, _args,
expected_nonce).
---
Nitpick comments:
In `@contracts/account/src/lib.rs`:
- Around line 203-206: The nonce increment should use checked arithmetic to
avoid overflow: replace the direct +(current_nonce + 1) with
current_nonce.checked_add(1) (or .saturating_add(1)) and handle the None case
from checked_add by deciding on behavior (e.g., saturate, panic with a clear
error, or log and return an error) before calling env.storage().instance().set
with DataKey::Nonce; update the logic around current_nonce, DataKey::Nonce, and
the env.storage().instance().set call to use that safe increment.
- Around line 276-291: The helper function setup() is unused; either delete the
entire setup() function (removing the Env/AncoreAccountClient/Address creation
and client.initialize(&owner) call) or refactor existing tests to call setup()
instead of duplicating inline setup logic—update tests to accept the returned
(Env, AncoreAccountClient<'static>, Address) and use those values in place of
their current per-test setup so the helper is actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92c5f612-064d-4c25-8c66-dbf6dff08e0a
📒 Files selected for processing (11)
contracts/account/src/lib.rscontracts/account/test_snapshots/test/test_execute_emits_event.1.jsoncontracts/account/test_snapshots/test/test_execute_non_owner_without_session_key_rejected.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_expired_session_key.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_revoked_session_key.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_session_key_at_exact_expiry_boundary.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_session_key_with_wrong_permission.1.jsoncontracts/account/test_snapshots/test/test_execute_session_key_increments_nonce_consecutively.1.jsoncontracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.jsoncontracts/account/test_snapshots/test/test_execute_with_valid_session_key.1.json
contracts/account/src/lib.rs
Outdated
Comment on lines
157
to
166
| pub fn execute( | ||
| env: Env, | ||
| caller: Address, | ||
| to: Address, | ||
| function: soroban_sdk::Symbol, | ||
| _args: Vec<soroban_sdk::Val>, | ||
| session_key: Option<BytesN<32>>, | ||
| expected_nonce: u64, | ||
| required_permission: u32, | ||
| ) -> Result<bool, ContractError> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current TypeScript client implementation
fd -e ts "account-contract" --exec cat {}Repository: ancore-org/ancore
Length of output: 16196
🏁 Script executed:
cat -n contracts/account/src/lib.rs | sed -n '150,180p'Repository: ancore-org/ancore
Length of output: 1883
🏁 Script executed:
cd contracts/account/src && grep -n "session_key\|required_permission" lib.rs | head -20Repository: ancore-org/ancore
Length of output: 1517
🏁 Script executed:
cat -n contracts/account/src/lib.rs | sed -n '165,172p'Repository: ancore-org/ancore
Length of output: 345
Breaking change: TypeScript client is missing the new caller parameter.
The execute() signature has changed. The Rust contract now expects 5 parameters:
caller(new)tofunction_argsexpected_nonce
The TypeScript client in packages/account-abstraction/src/account-contract.ts still uses the old 4-argument signature and will fail at runtime:
execute(to, fn, args, expectedNonce) // Missing: callerUpdate the client method to pass the caller as the first argument.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/account/src/lib.rs` around lines 157 - 166, The TypeScript client
still calls execute with the old 4-arg signature; update the client method that
calls execute (the execute wrapper in account-contract.ts) to pass the new first
parameter caller (so call execute(caller, to, fn, args, expectedNonce)); adjust
any callers/construction of that wrapper to supply the caller Address/BytesN<32>
value and ensure parameter ordering matches the Rust signature (caller, to,
function, _args, expected_nonce).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/account/src/lib.rs (1)
165-199:⚠️ Potential issue | 🔴 CriticalCritical: Session key authorization is not implemented.
The docstring describes dual auth paths (owner vs session-key) and parameters
session_key: Option<BytesN<32>>andrequired_permission: u32, but:
Signature mismatch: The function signature only has
caller,to,function,_args,expected_nonce— missingsession_keyandrequired_permissionparameters that the PR objectives and docstring describe.No authorization logic: Neither
caller.require_auth()nor any session key validation is performed. Thecallerandownervariables are fetched but never used (confirmed by pipeline warnings).Security vulnerability: Without any
require_auth()call, anyone can callexecute()and increment the nonce, which breaks replay protection and authorization.The implementation should:
- Add
session_key: Option<BytesN<32>>andrequired_permission: u32parameters- Call
caller.require_auth()for both paths- If
caller == owner, authorize directly- If
caller != owner, validate session key exists, is not expired, and has the required permission🔧 Suggested implementation outline
pub fn execute( env: Env, caller: Address, to: Address, function: soroban_sdk::Symbol, _args: Vec<soroban_sdk::Val>, expected_nonce: u64, + session_key: Option<BytesN<32>>, + required_permission: u32, ) -> Result<bool, ContractError> { let owner = Self::get_owner(env.clone())?; // ── Nonce check ────────────────────────────────────────────────────── let current_nonce: u64 = Self::get_nonce(env.clone())?; if expected_nonce != current_nonce { - panic!("Invalid nonce"); + return Err(ContractError::InvalidNonce); } + // ── Authorization ──────────────────────────────────────────────────── + caller.require_auth(); + + if caller != owner { + // Session-key path + let pk = session_key.ok_or(ContractError::Unauthorized)?; + let sk = Self::get_session_key(env.clone(), pk) + .ok_or(ContractError::SessionKeyNotFound)?; + + if sk.expires_at <= env.ledger().timestamp() { + return Err(ContractError::SessionKeyExpired); + } + if !sk.permissions.contains(&required_permission) { + return Err(ContractError::InsufficientPermission); + } + } // ── Increment nonce ────────────────────────────────────────────────── env.storage() .instance() .set(&DataKey::Nonce, &(current_nonce + 1));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 165 - 199, The execute function currently lacks the session_key and required_permission parameters and any auth checks; update execute to accept session_key: Option<BytesN<32>> and required_permission: u32, then enforce authorization before mutating state by calling caller.require_auth() and branching: if caller == Self::get_owner(env.clone())? then allow; else validate the provided session_key exists, is not expired, and includes required_permission (use whatever session storage/struct you have for session entries) and call caller.require_auth() for the caller as well; only after successful auth perform the nonce compare using Self::get_nonce, increment the nonce in storage (DataKey::Nonce), extend TTL, and emit events::executed with the transaction details — ensure missing/invalid session_key or insufficient permission returns appropriate ContractError rather than allowing state changes.
🧹 Nitpick comments (1)
contracts/account/src/lib.rs (1)
302-316: Unused helper functionsetup().The
setup()helper is defined but never called in any test. Consider removing it or using it to reduce boilerplate in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 302 - 316, The helper function setup() is defined but never used; either remove it or update tests to call it to avoid duplication: delete the setup() function (references: setup, AncoreAccountClient, initialize, Address::generate) if you prefer inline setups, or replace repeated test boilerplate by calling setup() and destructuring its return (Env, AncoreAccountClient<'static>, Address) in each test, then invoke client.initialize(&owner) only once via the helper to keep tests concise and correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/account/src/lib.rs`:
- Around line 504-505: The execute() function currently has the wrong signature
(5 args) while tests call it with session_key: Option<BytesN<32>> and
required_permission: u32 (7 args total); update the execute function signature
to accept the two additional parameters named session_key: Option<BytesN<32>>
and required_permission: u32 (preserving existing parameters) in the execute
declaration and all internal calls, and then update its implementation to use or
forward these values appropriately (or explicitly ignore them with clear TODOs)
so all call sites (including tests) compile against the new execute(session_key,
required_permission) signature.
- Around line 294-297: The import list includes an unused symbol Ledger from
soroban_sdk::testutils; remove Ledger from the nested use (the line containing
testutils::{Address as _, Events, Ledger}) so it reads testutils::{Address as _,
Events} and verify no other references to Ledger remain in the module (adjust
formatting/commas as needed).
- Around line 181-183: Replace the panic with returning the structured
ContractError::InvalidNonce so the contract returns a proper Err rather than
panicking: where the code compares expected_nonce and current_nonce (the block
containing if expected_nonce != current_nonce { panic!("Invalid nonce"); }),
change it to return the contract error (e.g.,
Err(ContractError::InvalidNonce.into()) or Err(ContractError::InvalidNonce { }
depending on your error type shape) so the function returns a Result::Err that
matches the test expectation.
- Around line 454-456: The call to client.execute(&to, &function, &args, &0u64)
is missing the required caller argument; update the call to supply the caller
(e.g., client.execute(&to, &function, &args, &caller, &0u64)) inserting the
caller value between &args and &0u64, and if a caller variable doesn't exist
create one of the appropriate type (or use &env.invoker()/appropriate
Address/Val) so the signature of client.execute matches its 5 parameters.
---
Outside diff comments:
In `@contracts/account/src/lib.rs`:
- Around line 165-199: The execute function currently lacks the session_key and
required_permission parameters and any auth checks; update execute to accept
session_key: Option<BytesN<32>> and required_permission: u32, then enforce
authorization before mutating state by calling caller.require_auth() and
branching: if caller == Self::get_owner(env.clone())? then allow; else validate
the provided session_key exists, is not expired, and includes
required_permission (use whatever session storage/struct you have for session
entries) and call caller.require_auth() for the caller as well; only after
successful auth perform the nonce compare using Self::get_nonce, increment the
nonce in storage (DataKey::Nonce), extend TTL, and emit events::executed with
the transaction details — ensure missing/invalid session_key or insufficient
permission returns appropriate ContractError rather than allowing state changes.
---
Nitpick comments:
In `@contracts/account/src/lib.rs`:
- Around line 302-316: The helper function setup() is defined but never used;
either remove it or update tests to call it to avoid duplication: delete the
setup() function (references: setup, AncoreAccountClient, initialize,
Address::generate) if you prefer inline setups, or replace repeated test
boilerplate by calling setup() and destructuring its return (Env,
AncoreAccountClient<'static>, Address) in each test, then invoke
client.initialize(&owner) only once via the helper to keep tests concise and
correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 601059bd-9c38-4088-8cc8-92de7bc98e12
📒 Files selected for processing (4)
contracts/account/src/lib.rscontracts/account/test_snapshots/test/test_execute_emits_event.1.jsoncontracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.jsoncontracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.json
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/account/test_snapshots/test/test_execute_emits_event.1.json
- contracts/account/test_snapshots/test/test_execute_rejects_invalid_nonce.1.json
- contracts/account/test_snapshots/test/test_execute_validates_nonce_then_increments.1.json
Comment on lines
+294
to
+297
| use soroban_sdk::{ | ||
| testutils::{Address as _, Events, Ledger}, | ||
| Address, Env, | ||
| }; |
There was a problem hiding this comment.
Remove unused import Ledger.
Pipeline reports: unused import 'Ledger'. This import is not used in the test module.
🧹 Proposed fix
use soroban_sdk::{
- testutils::{Address as _, Events, Ledger},
+ testutils::{Address as _, Events},
Address, Env,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use soroban_sdk::{ | |
| testutils::{Address as _, Events, Ledger}, | |
| Address, Env, | |
| }; | |
| use soroban_sdk::{ | |
| testutils::{Address as _, Events}, | |
| Address, Env, | |
| }; |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 295-295: Rust warning: unused import Ledger (#[warn(unused_imports)]).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/account/src/lib.rs` around lines 294 - 297, The import list
includes an unused symbol Ledger from soroban_sdk::testutils; remove Ledger from
the nested use (the line containing testutils::{Address as _, Events, Ledger})
so it reads testutils::{Address as _, Events} and verify no other references to
Ledger remain in the module (adjust formatting/commas as needed).
Contributor
Author
|
resolved the code rabbit security issues @wheval |
Contributor
Author
|
Hi @wheval, I've resolved the security issues flagged by coderabbit, tried resolving the CI issues as well but I kept on getting build issues from other contributor's commits. Kindly help me review |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Contract — Add Session Key Authorization to execute()
Allow execute() to be called by session keys (not just the owner). Validate session key expiration and permissions before allowing execution.
Type of Change
Security Impact
Testing
Test Coverage
Manual Testing Steps
Breaking Changes
execute() now has 3 new parameters which are session_key: Option<BytesN<32>>, public key of registered session key
expected_nonce: u64, // NEW: replay protection
required_permission: u32, // NEW: permission code to check
The nonce is validated before any auth check so that callers with a stale nonce get a clear InvalidNonce error rather than having auth succeed and then the nonce fail silently.
Checklist
For High-Security Changes
Related Issues
Closes #54
Related to #
Additional Context
Reviewer Notes
Summary by CodeRabbit
Documentation
Tests