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}
| // Before we attempt to sign, validate the auth entry is strict | ||
| match validation::classify_auth_invocation(&body.host_function, &raw_auth.root_invocation) { | ||
| validation::AuthStyle::Strict => {} | ||
| validation::AuthStyle::NonStrict => { | ||
| if !skip_approval { | ||
| confirm_non_strict_authorization(raw_auth)?; | ||
| } | ||
| } | ||
| validation::AuthStyle::Invalid => { | ||
| return Err(Error::InvalidAuthEntry { | ||
| reason: "authorization entry is not expected for the transaction".to_string(), | ||
| auth_entry_str: format_auth_entry(raw_auth), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
With --force, non-strict entries are signed silently — no audit trail of what got approved. confirm_non_strict_authorization is the only place that prints the entry, and it's gated behind !skip_approval. So a user running with --force (or in CI) sees nothing about which auth entries failed the strict check before being signed.
Suggest always logging the non-strict entry at warn level, and only adding the interactive prompt when not forced. That way --force users still get a record in their logs.
validation::AuthStyle::NonStrict => {
let print = Print::new(false);
print.warnln("Authorization entry does not match the current contract call:");
print.println(format_auth_entry(raw_auth));
if !skip_approval {
confirm_non_strict_authorization_prompt(&print)?;
} else {
print.warnln("Signing anyway because --force was provided.");
}
}
There was a problem hiding this comment.
This is good feedback! I like having a warning message.
| let mut signer: Option<&Signer> = None; | ||
| for s in signers { | ||
| if needle == &s.get_public_key()?.0 { | ||
| if auth_address_bytes == &s.get_public_key()?.0 { | ||
| signer = Some(s); | ||
| } | ||
| } |
There was a problem hiding this comment.
Two minor issues here:
- No
breakafter assigning the signer — the loop keeps iterating and re-callingget_public_key()?on remaining signers. get_public_key()can hit external systems forLedger/SecureStoresigners, so iterating all of them per auth entry is more expensive than necessary.
Suggest:
let signer = signers
.iter()
.find(|s| s.get_public_key().ok().map(|pk| pk.0) == Some(*auth_address_bytes));or just add a break after assignment. (You'll lose the ? propagation on get_public_key errors; if that matters, keep the explicit loop but break once matched.)
| } | ||
|
|
||
| fn confirm_non_strict_authorization(auth: &SorobanAuthorizationEntry) -> Result<(), Error> { | ||
| let print = Print::new(false); |
There was a problem hiding this comment.
Print::new(false) hard-codes quiet=false, so the safety warning ignores the global --quiet flag. That's probably the intended behavior for a safety prompt (you want the warning to show even in quiet mode), but it's inconsistent with the rest of the CLI's print plumbing — worth a one-line comment justifying the choice so a future reader doesn't "fix" it by threading quiet through.
| SorobanAuthorizedFunction::CreateContractHostFn(_) | ||
| | SorobanAuthorizedFunction::CreateContractV2HostFn(_) => { | ||
| let _ = writeln!(result, "{prefix} CreateContract"); | ||
| } |
There was a problem hiding this comment.
When the prompt is shown for a non-strict CreateContract / CreateContractV2 auth entry, the user just sees the word CreateContract with no further detail (no wasm hash, no preimage, no constructor args). That's the same information they'd need to evaluate whether to approve.
Since CreateContractArgsV2 carries contract_id_preimage, executable, and constructor_args, it's worth at least formatting the wasm hash and constructor args here (mirroring how ContractFn is printed). Otherwise the approval prompt for a create-contract auth is essentially "do you want to sign this?" with no context.
| let is_strict = match (source_host_fn, &auth_invocation.function) { | ||
| (HostFunction::InvokeContract(op), SorobanAuthorizedFunction::ContractFn(args)) => { | ||
| args == op | ||
| } | ||
| ( | ||
| HostFunction::CreateContract(op), | ||
| SorobanAuthorizedFunction::CreateContractHostFn(args), | ||
| ) => args == op, | ||
| ( | ||
| HostFunction::CreateContractV2(op), | ||
| SorobanAuthorizedFunction::CreateContractV2HostFn(args), | ||
| ) => args == op, | ||
| _ => false, | ||
| }; | ||
|
|
||
| if is_strict { | ||
| AuthStyle::Strict | ||
| } else { | ||
| AuthStyle::NonStrict | ||
| } |
There was a problem hiding this comment.
Echoing the earlier Copilot comment now that I see the full classifier: with (HostFunction::CreateContract(_), SorobanAuthorizedFunction::ContractFn(_)) or any other cross-variant pair, this falls into the _ => false arm and returns NonStrict. The doc string says NonStrict is "the same kind of operation but doesn't match exactly" — a ContractFn auth attached to a CreateContract host fn isn't that; it's structurally wrong.
You acknowledged in the earlier thread that "it is valid, just not strict." That's defensible, but currently the prompt the user sees will say "Authorization entry does not match the current contract call" without distinguishing "same op, different args" from "completely wrong op kind." A user might reasonably approve the former and never the latter. Worth either:
- Returning
Invalidon cross-variant mismatch (clearer error), or - Surfacing the distinction in the warning text so users have a real signal.
| client | ||
| .verify_network_passphrase(Some(&network.network_passphrase)) | ||
| .await?; |
There was a problem hiding this comment.
This verify_network_passphrase addition (also in restore.rs) is unrelated to the rest of this PR's auth-validation theme. It looks like a good change on its own, but the PR description doesn't mention it. Consider splitting it into a separate PR (or at least calling it out in the description) so it doesn't get lost under the auth-validation review and so it can be reverted independently if needed.
| /// Skip confirmation prompts when signing | ||
| #[arg(long, help_heading = HEADING_SIGNING)] | ||
| pub force: bool, | ||
| } |
There was a problem hiding this comment.
--force is now attached to sign_with::Args, which flattens into ~50 subcommands per the diff in FULL_HELP_DOCS.md — including tx sign, tx send, etc. where there are no Soroban auth confirmation prompts. The flag is therefore a silent no-op on most of the commands that advertise it.
Two small suggestions:
- Tighten the help text so users know when it actually does something, e.g.
Skip auth-entry approval prompts (only applies to commands that invoke Soroban contracts). - Consider whether the flag belongs on
sign_with::Argsat all vs. only on the commands that actually callsign_soroban_authorizations.
Not blocking, but worth a thought before merge so the help output stays honest.
There was a problem hiding this comment.
I wonder if we should use a better more explicit name for this option. --force can have different meaning on different commands. Maybe --confirm-signing?
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.