Validate auth entries before signing#2530
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens soroban-cli transaction signing by explicitly validating each Address-credential Soroban auth entry against the transaction’s host function before signing, rejecting entries that are unsafe to replay or malformed, and improving the error output by rendering the offending auth entry inline.
Changes:
- Added a host-function vs auth-root-invocation classifier and enforced “strict” auth validation in
sign_soroban_authorizations. - Introduced pretty-print formatting for auth entries to improve CLI error messages.
- Added coverage via a new auth fixture contract + integration tests, and added missing RPC network-passphrase verification in extend/restore commands.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/tx.rs | Updates comment to reflect new validation behavior prior to signing. |
| cmd/soroban-cli/src/signer/validation.rs | Adds auth root-invocation classification logic + unit tests. |
| cmd/soroban-cli/src/signer/mod.rs | Enforces strict validation before signing; adds new errors and source-account credential guard; adds unit tests. |
| cmd/soroban-cli/src/log/auth.rs | Replaces prior debug helper with structured formatting for auth entries. |
| cmd/soroban-cli/src/commands/contract/restore.rs | Adds verify_network_passphrase call on RPC client. |
| cmd/soroban-cli/src/commands/contract/extend.rs | Adds verify_network_passphrase call on RPC client. |
| cmd/crates/soroban-test/tests/it/integration/util.rs | Adds AUTH fixture constant. |
| cmd/crates/soroban-test/tests/it/integration/auth.rs | Adds integration coverage for strict vs non-strict/non-root auth scenarios. |
| cmd/crates/soroban-test/tests/it/integration.rs | Wires in the new auth integration test module. |
| cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/src/lib.rs | Adds a Soroban test contract to generate various auth-tree shapes. |
| cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/Cargo.toml | Adds the new test fixture crate manifest. |
| Cargo.lock | Records the new test_auth fixture crate dependency. |
There was a problem hiding this comment.
I haven't finished reviewing this, but it's not obvious to me that this is the appropriate action, it's rather restrictive, especially in the context of the known limitations in the pr description.
I think given the stellar-cli is a developer tool it would be more appropriate to do the following instead of rejecting outright:
-
validate the safe case and auto sign in that case (most commands other than invoke should have relatively predictable auth entries required)
-
identify the unsafe cases and display those to the user to confirm if they wish to continue
And for a tool that has narrower use, such as a wallet cli rather than a developer focused cli, to provide narrower restrictions like this to protect its use case.
Thanks for the feedback @leighmcculloch ! This PR is basically step 1, identify "safe" auth. In my mind, I defined this as any auth entry that is tied to the root invocation exactly. Since the idea of this is not to limit user actions, we can assume the user input the contract invocation as intended. Thus, even if the auth could be detached, the only way its valid is if the exact contract invocation the user intended was the root invocation. To add, I'd be shocked if this was actually restrictive. The only use case it blocks is non-source accounts signing Note - updated to add a bypass flag and approval mechanism. Created a follow up issue to support non-root auth: #2574 |
|
Claude encountered an error —— View job Claude PR Review
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @mootz12's task in 2m 50s —— View job Claude PR ReviewI went through the auth-validation flow end-to-end. The classifier is sound and the test coverage of the strict/non-strict/source-account/upload-wasm matrix is solid. Most of my concerns are about the prompt/approval UX and a few sharp edges in
Blocking issues1. validation::AuthStyle::NonStrict => {
if !skip_approval {
confirm_non_strict_authorization(raw_auth)?;
}
}
2. ScAddress::MuxedAccount(_) => todo!("muxed accounts are not supported"),
ScAddress::ClaimableBalance(_) => todo!("claimable balance not supported"),
ScAddress::LiquidityPool(_) => todo!("liquidity pool not supported"),leighmcculloch flagged this earlier and you agreed but deferred. Since this PR's whole premise is "the RPC might return adversarial auth entries", leaving three panic paths reachable from RPC output is at odds with the threat model. A malformed credential address from a hostile simulation result panics the CLI instead of failing gracefully. Returning Worth fixing before merge3. Signer lookup keeps iterating after a match — let mut signer: Option<&Signer> = None;
for s in signers {
if auth_address_bytes == &s.get_public_key()?.0 {
signer = Some(s);
}
}No 4. SorobanAuthorizedFunction::CreateContractHostFn(_)
| SorobanAuthorizedFunction::CreateContractV2HostFn(_) => {
let _ = writeln!(result, "{prefix} CreateContract");
}If 5.
Nits / lower priority6. Cross-variant mismatch returns A 7. This ignores 8. Good change on its own, but it's not mentioned in the PR description and doesn't belong to the auth-validation theme. Either call it out in the description or split into a follow-up PR — if the auth changes get reverted later, you don't want to lose the passphrase checks with them. Things I liked
|
|
Claude finished @mootz12's task in 2m 23s —— View job Claude PR ReviewRe-reviewed against the latest push (
Resolved since last review
Still outstanding (blocking)
ScAddress::MuxedAccount(_) => todo!("muxed accounts are not supported"),
ScAddress::ClaimableBalance(_) => todo!("claimable balance not supported"),
ScAddress::LiquidityPool(_) => todo!("liquidity pool not supported"),I called this out last round and the comment was acknowledged but deferred to #2534. With this PR's whole premise being "the RPC might return adversarial auth entries", I think it's worth promoting this from "follow-up" to "blocking" — the rest of the PR now guards against unexpected RPC output (strict-match check, source-account-as-credential guard, force-disabled non-interactive failure), but a malicious or buggy simulation response that includes one of these three address kinds still panics the CLI with a Rust backtrace instead of erroring out. The fix is mechanical: each arm becomes one line returning an Worth fixing before merge
Two options, either is fine:
I'd lean toward the first — the help text already says Nits / lower priorityCross-variant mismatch still surfaces as Carried over from prior round. You've already defended the choice ("valid, just not strict") and I think that's defensible, but the user-facing prompt text doesn't differentiate "same op, different args" from "structurally wrong op kind". Not a merge blocker — flagging here for visibility in case the warning text is revisited.
Unchanged from last round. Good change on its own, just not part of the auth-validation theme. If the auth changes ever get reverted, you don't want to lose the passphrase checks with them. Things I liked in this revision
|
What
The CLI currently relies on the RPC to check that no non-root auths are included in simulation results. This PR adds an explicit, per-entry validation step inside
sign_soroban_authorizationsthat classifies everyAddress-credential auth entry against the transaction's host function before signing. Entries that don't match the host function exactly require approval. This approval can be bypassed with a--forceflag.Example output:
If the CLI is invoked in a non-interactive location and the force flag is not preset, it will fail:
Why
The CLI eagerly signs authorization entries returned from the user-specified RPC. If an unsafe auth entry is included, the user might unexpectedly sign for something they did not intend. This check ensures everything the CLI signs automatically is bound to the exact host function invocation in the transaction.
Close https://github.com/stellar/stellar-cli-internal/issues/50
Known limitations
require_auth_for_argsfor non-source accountsThe check flags contracts that use
require_auth_for_args(custom_args)at the root for non-source accounts. The auth tree's root carriescustom_args, not the host function's args, so the strict-match check fails even though the auth is genuinely rooted at the operation. A tampered auth entry with the same custom args at root could otherwise be signed and replayed. Source-account auth viaSorobanCredentials::SourceAccountis unaffected.