Verify auth entries can't be used outside of the contract transaction#2471
Verify auth entries can't be used outside of the contract transaction#2471
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a local (CLI-side) validation step to ensure Soroban authorization entries returned from RPC simulation are “strict” (tied to the root invocation) before the CLI signs them, reducing risk of signing replayable/out-of-context auth.
Changes:
- Call a new
auth::check_authvalidation step before signing auth entries insim_sign_and_send_tx. - Add formatting/logging helpers to render auth entry trees when validation fails.
- Add new integration fixture + tests around auth behaviors; add missing network passphrase verification in
extend/restore.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/tx.rs | Runs check_auth before signing Soroban auth entries. |
| cmd/soroban-cli/src/signer/mod.rs | Adds new signer error variants for invalid/out-of-context auth. |
| cmd/soroban-cli/src/log/auth.rs | Adds pretty-formatting for auth entries / invocation tree display. |
| cmd/soroban-cli/src/lib.rs | Exposes new auth module. |
| cmd/soroban-cli/src/commands/contract/restore.rs | Verifies RPC network passphrase before network operations. |
| cmd/soroban-cli/src/commands/contract/extend.rs | Verifies RPC network passphrase before network operations. |
| cmd/soroban-cli/src/auth.rs | New auth validation logic + unit tests. |
| cmd/crates/soroban-test/tests/it/integration/util.rs | Adds AUTH wasm fixture constant. |
| cmd/crates/soroban-test/tests/it/integration/auth.rs | New integration tests for auth scenarios. |
| cmd/crates/soroban-test/tests/it/integration.rs | Registers new auth integration module. |
| cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/src/lib.rs | Adds a dedicated auth-focused test contract fixture. |
| cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/Cargo.toml | New fixture crate manifest for test_auth. |
| Cargo.lock | Adds test_auth package entry. |
| }; | ||
|
|
||
| let mut non_strict_entries: Vec<&SorobanAuthorizationEntry> = Vec::new(); | ||
| for entry in invoke_host_op.auths() { |
There was a problem hiding this comment.
InvokeHostFunctionOp (from stellar_xdr) doesn't provide an auths() accessor in this repo (and there’s no extension trait implementing it), so this loop won’t compile. Iterate over the auth field instead (e.g., invoke_host_op.auth.as_slice() / .iter()).
| for entry in invoke_host_op.auths() { | |
| for entry in invoke_host_op.auth.as_slice() { |
| // Check if source account appears as Address credential | ||
| if let Some(auth_addr) = auth_address_bytes(&creds.address) { | ||
| if source_bytes == auth_addr { | ||
| print.warnln("Source account detected with Address credentials. This requires an extra signature and is not expected."); | ||
| print.warnln(format_auth_entry(entry)); | ||
| return Err(signer::Error::InvalidAuthEntry); | ||
| } |
There was a problem hiding this comment.
The PR description says the error text should change when the source account is recorded with Address credentials, but this path returns signer::Error::InvalidAuthEntry (displayed as "Invalid Soroban authorization entry") and only emits the specific message via warnln. If the intent is for the CLI to surface the specific message as the returned error, add a dedicated signer::Error variant (or otherwise propagate this message) and return that instead of InvalidAuthEntry.
| .arg(&format!("--subcall={id2}")) | ||
| .assert() | ||
| .failure() | ||
| .stderr(predicates::str::contains("Auth, InvalidAction")); |
There was a problem hiding this comment.
These failure expectations appear outdated with the new check_auth behavior. partial_auth_sub_auth uses require_auth_for_args, so simulation can succeed but the recorded auth root invocation args won’t match the tx invocation args; check_auth should reject it with the new out-of-context auth error (and/or the source-account-as-Address error), rather than surfacing a host error like "Auth, InvalidAction".
| .stderr(predicates::str::contains("Auth, InvalidAction")); | |
| .stderr( | |
| predicates::str::is_match("out[- ]of[- ]context|source-account-as-Address") | |
| .unwrap(), | |
| ); |
What
The CLI currently relies on the RPC to check that no non-root auth is included in the simulation results. This PR adds an explicit check to verify that all non-source account authorizations are strictly tied to the root invocation. The check ensures that any signed authorization entries cannot be submitted outside the context of the invocation.
If any auth entry is detected that could be submitted outside of the transaction, all flagged entries are logged as follows:
If an auth entry is detected where the source account is used directly instead of with
Credentials::SourceAccount, the error text changes to:Why
The CLI eagerly signs authorization entries returned from the user specified RPC. If an unsafe auth entry was included, the user might unexpectedly sign for something they did not intend.
This adds a check to ensure that everything the CLI signs aligns with the transaction that got submitted.
Known limitations
require_auth_with_argsfor non-source accountsThe change also blocks contracts that use
require_auth_with_argsfor non-source accounts. Consider a transaction where a user submits from a source account UserA, and also controls UserBcustom_token.transfer(from: UserB, to: UserC, amount: 100). If a malicious RPC intercepts this request, the could inject a bad auth such thatcustom_token.transfer(from: UserC, to: UserB, amount: 100. This technically satisfies the root auth requirement as the auth entry exists for the root contract and function invocation, but the arguments are different.No bypass flag
This check currently cannot be bypassed. Most Stellar devtools (like the CLI and Stellar Lab) simulate in recording mode, which blocks non-root authorizations by default. If we add a flag to bypass this, we should also make the change to simulate in
AuthMode::RecordAllowNonRoot. However, we should keep this consistent between Lab and CLI to prevent confusion, so this change was omitted for now.