diff --git a/contracts/smart-account/src/auth/core/authorizer.rs b/contracts/smart-account/src/auth/core/authorizer.rs index f82ff0d..1d2350d 100644 --- a/contracts/smart-account/src/auth/core/authorizer.rs +++ b/contracts/smart-account/src/auth/core/authorizer.rs @@ -91,10 +91,23 @@ impl Authorizer { { let res = SmartAccountPluginClient::new(env, &plugin) .try_on_auth(&env.current_contract_address(), auth_contexts); + // Plugin-result classification (Soroban 22 try_* semantics): + // Ok(Ok(_)) — plugin returned normally → continue + // Ok(Err(_)) — return-value ABI decode failure → SKIP (fail-open) + // Err(Ok(_)) — contracterror OR panic!/unwrap/unreachable → REJECT (fail-closed) + // Err(Err(_)) — host trap (archival, budget, missing code) → SKIP (fail-open) + // + // Note: any `panic!` from the plugin — including an unwrap that + // fires or an arithmetic overflow — lands in the Err(Ok(_)) + // branch and blocks auth. Only environmental host traps + // (storage archival, budget exhaustion, stack overflow, + // contract-not-found) reach Err(Err(_)) and are silently + // skipped. Plugins that want to stay enforceable should + // manage their own TTL and avoid unbounded work. match res { // Plugin executed successfully Ok(Ok(_)) => {} - // Plugin return value conversion failure (ABI mismatch) + // Plugin return value ABI decode failure // Treat as technical failure: log and continue Ok(Err(_)) => { env.events().publish( @@ -105,7 +118,8 @@ impl Authorizer { }, ); } - // Plugin intentionally rejected (contracterror / panic_with_error!) + // Plugin intentionally rejected (contracterror) or panicked + // (panic!, panic_with_error!, unwrap, arithmetic overflow, etc.) Err(Ok(_)) => { env.events().publish( (TOPIC_PLUGIN, &plugin, VERB_AUTH_FAILED), @@ -116,8 +130,10 @@ impl Authorizer { ); return Err(Error::PluginOnAuthFailed); } - // Plugin had a technical failure (panic!, host trap, TTL expiry) - // Non-blocking: log and continue to next plugin + // Host-level failure: storage archival, budget exhaustion, + // contract not found, stack overflow. Non-blocking: log + // and continue to next plugin so an unreachable plugin + // cannot brick the account. Err(Err(_)) => { env.events().publish( (TOPIC_PLUGIN, &plugin, VERB_AUTH_FAILED), diff --git a/contracts/smart-account/src/tests/plugin_test.rs b/contracts/smart-account/src/tests/plugin_test.rs index 7c9024f..94e349e 100644 --- a/contracts/smart-account/src/tests/plugin_test.rs +++ b/contracts/smart-account/src/tests/plugin_test.rs @@ -247,3 +247,62 @@ fn test_max_plugins_limit() { assert_eq!(err, Error::MaxPluginsReached); } + +// ----------------------------------------------------------------------------- +// Plugin that crashes via plain `panic!` — Soroban 22 classifies this as +// Err(Ok(_)), i.e. fail-closed. The authorizer comment previously grouped +// `panic!` with host traps in the fail-open branch, which was inaccurate. +// This test pins the actual behaviour so future refactors cannot regress it. +// ----------------------------------------------------------------------------- + +mod panicking_plugin { + use soroban_sdk::{auth::Context, contract, contractimpl, Address, Env, Vec}; + + #[contract] + pub struct PlainPanicPlugin; + + #[contractimpl] + impl PlainPanicPlugin { + pub fn on_install(_env: &Env, _source: Address) {} + pub fn on_uninstall(_env: &Env, _source: Address) {} + #[allow(clippy::panic)] + pub fn on_auth(_env: &Env, _source: Address, _contexts: Vec) { + panic!("buggy plugin — this should block auth, not be skipped"); + } + } +} + +#[test] +fn test_plugin_plain_panic_is_fail_closed() { + let env = setup(); + env.mock_all_auths(); + + let admin = Ed25519TestSigner::generate(SignerRole::Admin); + let smart_account_id = env.register( + SmartAccount, + ( + vec![&env, admin.into_signer(&env)], + Vec::
::new(&env), + ), + ); + + let plugin_id = env.register(panicking_plugin::PlainPanicPlugin, ()); + env.as_contract(&smart_account_id, || { + SmartAccount::install_plugin(&env, plugin_id.clone()) + }) + .unwrap(); + + let payload = BytesN::random(&env); + let (admin_key, admin_proof) = admin.sign(&env, &payload); + let auth_payloads = SignatureProofs(soroban_sdk::map![&env, (admin_key, admin_proof)]); + + let result = env.try_invoke_contract_check_auth::( + &smart_account_id, + &payload, + auth_payloads.into_val(&env), + &vec![&env, get_token_auth_context(&env)], + ); + + // plain `panic!` → Err(Ok(_)) classification → PluginOnAuthFailed. + assert_eq!(result, Err(Ok(Error::PluginOnAuthFailed))); +}