diff --git a/crates/ironclaw_auth/src/engine/exchange.rs b/crates/ironclaw_auth/src/engine/exchange.rs index c64ce5379c3..b9c9c5dda44 100644 --- a/crates/ironclaw_auth/src/engine/exchange.rs +++ b/crates/ironclaw_auth/src/engine/exchange.rs @@ -96,7 +96,7 @@ impl AuthEngine { &recipe, &response.body, &request.scopes, - ScopeClamp::ToRequested, + ScopeClamp::ToRecipeCeiling, ) .inspect_err(|_| { tracing::debug!(vendor, "token response extraction failed"); @@ -401,13 +401,13 @@ fn token_request_headers_and_body( (headers, form.finish().into_bytes()) } -/// Whether the A6 exchange-scope clamp (store `granted ∩ requested`) applies. -/// Only the initial authorization-code exchange clamps; refresh preserves the -/// granted set so a vendor's rotation response is recorded as-is (the account's -/// scopes were already clamped at exchange time). +/// Whether the A6 exchange-scope clamp (store `granted ∩ recipe ceiling`) +/// applies. Only the initial authorization-code exchange clamps; refresh +/// preserves the granted set so a vendor's rotation response is recorded as-is +/// (the account's scopes were already clamped at exchange time). #[derive(Clone, Copy)] pub(super) enum ScopeClamp { - ToRequested, + ToRecipeCeiling, PreserveGranted, } @@ -454,37 +454,55 @@ pub(super) fn extract_token_response( .collect::, _>>() .map_err(|_| AuthProductError::TokenExchangeFailed)?; match clamp { - // A6 · Enforce granted ⊆ requested on the echoed-scope path - // (RFC 9700 §2.3). Store only scopes the vendor granted AND - // the user requested: drop any scope granted beyond the - // request (stop over-claiming an unrequested scope), and - // never widen a narrower-than-requested grant back up to - // the full requested set. Generic and spec-agnostic — every - // vendor gets the same clamp. Applied only on the initial - // exchange; refresh preserves the granted set. - ScopeClamp::ToRequested => { + // A6 · Clamp the echoed grant to the recipe's declared + // scope ceiling (RFC 9700 §2.3): a scope no recipe ever + // declared is dropped (no over-claim). A scope granted + // beyond THIS flow's request but within the ceiling is + // kept — vendors with cumulative grants (opted into via + // recipe data, e.g. Google's `include_granted_scopes` + // authorize param) echo previously granted scopes on + // every exchange, and the stored account is shared by + // every extension using the vendor, so discarding them + // would silently sign the other extensions out (the + // gmail → google-docs regression). The per-flow request + // still drives the authorize URL and the downgrade warn + // below; it is not the storage bound. Generic and + // spec-agnostic — every vendor gets the same clamp. + // Applied only on the initial exchange; refresh + // preserves the granted set. + ScopeClamp::ToRecipeCeiling => { let clamped: Vec = granted .iter() - .filter(|scope| requested_scopes.contains(scope)) + .filter(|scope| { + recipe + .scopes + .iter() + .any(|ceiling| ceiling == scope.as_str()) + }) .cloned() .collect(); - let over_granted = granted.len().saturating_sub(clamped.len()); - if over_granted > 0 || clamped.len() < requested_scopes.len() { + let outside_ceiling = granted.len().saturating_sub(clamped.len()); + let missing_requested = requested_scopes + .iter() + .filter(|scope| !clamped.contains(scope)) + .count(); + if outside_ceiling > 0 || missing_requested > 0 { // Count-only guard log — never the scope values or - // the response body; the stored grant is the - // intersection, never wider than either side. + // the response body; the stored grant is never + // wider than granted ∩ ceiling. tracing::warn!( requested_scope_count = requested_scopes.len(), granted_scope_count = clamped.len(), - over_granted_scope_count = over_granted, - "oauth exchange granted a scope set differing from requested; storing granted ∩ requested (never wider than requested or granted)" + outside_ceiling_scope_count = outside_ceiling, + missing_requested_scope_count = missing_requested, + "oauth exchange grant differs from this flow's request; storing granted ∩ recipe ceiling" ); } if clamped.is_empty() { - // The vendor echoed only scopes outside the request - // — no legitimate granted scope to store, so fall - // through to the missing-scope behavior exactly as - // an omitted scope would. + // The vendor echoed only scopes outside every + // declared ceiling — no legitimate granted scope + // to store, so fall through to the missing-scope + // behavior exactly as an omitted scope would. scopes_when_grant_absent(extraction.missing, requested_scopes)? } else { clamped diff --git a/crates/ironclaw_auth/src/fakes.rs b/crates/ironclaw_auth/src/fakes.rs index b3fa5174759..fa53878ba94 100644 --- a/crates/ironclaw_auth/src/fakes.rs +++ b/crates/ironclaw_auth/src/fakes.rs @@ -1135,6 +1135,49 @@ impl SecretCleanupService for InMemoryAuthProductServices { let mut state = self.lock_state(); let quarantines = state.quarantines.clone(); let mut report = SecretCleanupReport::default(); + // A3 · Cancel the provider's pending flows BEFORE enumerating + // accounts, mirroring the durable store's order (which closes the + // callback/removal race there — the fake serializes the whole + // cleanup under one lock, so here the order is fidelity only). + // Owner decision 2026-07-15: cancel on both Deactivate and + // Uninstall (any provider-selected cleanup). Idempotent. + // + // F2 · Any matched flow whose `TurnGateResume` continuation has not + // been acknowledged — freshly canceled here or already terminal — is + // reported so the composition layer denies its blocked turn gate + // instead of leaving the turn parked. `mark_continuation_dispatched` + // makes the handoff emit-once across cleanup retries. + if let Some(provider) = request.provider.as_ref() { + for record in state.flows.values_mut() { + if &record.provider != provider + || !flow_matches_credential_owner(&record.scope, &request.scope) + { + continue; + } + if !crate::is_terminal_status(record.status) { + record.status = AuthFlowStatus::Canceled; + record.error = Some(crate::AuthErrorCode::Canceled); + record.updated_at = Utc::now(); + } + if record.continuation_emitted_at.is_none() + && matches!( + record.continuation, + crate::AuthContinuationRef::TurnGateResume { .. } + ) + { + report + .canceled_turn_gate_continuations + .push(AuthContinuationEvent { + flow_id: record.id, + scope: record.scope.clone(), + continuation: record.continuation.clone(), + provider: record.provider.clone(), + credential_account_id: record.credential_account_id, + emitted_at: Utc::now(), + }); + } + } + } // Credential-owner granularity, not full scope equality: cleanup // callers mint a fresh invocation (and often a different thread), so // exact matching could never find the account the flow stored. @@ -1189,49 +1232,6 @@ impl SecretCleanupService for InMemoryAuthProductServices { report.retained_accounts.push(account.id); } } - // A3 · Removal/disconnect cancels pending flows (RFC 9700 §4.7.1 + - // RFC 7009 §1): a provider-selected cleanup cancels EVERY non-terminal - // flow for the credential-owner + provider so a late provider callback - // can no longer mint a credential for a torn-down extension. Owner - // decision 2026-07-15: cancel on both Deactivate and Uninstall (any - // provider-selected cleanup). Idempotent. - // - // F2 · Any matched flow whose `TurnGateResume` continuation has not - // been acknowledged — freshly canceled here or already terminal — is - // reported so the composition layer denies its blocked turn gate - // instead of leaving the turn parked. `mark_continuation_dispatched` - // makes the handoff emit-once across cleanup retries. - if let Some(provider) = request.provider.as_ref() { - for record in state.flows.values_mut() { - if &record.provider != provider - || !flow_matches_credential_owner(&record.scope, &request.scope) - { - continue; - } - if !crate::is_terminal_status(record.status) { - record.status = AuthFlowStatus::Canceled; - record.error = Some(crate::AuthErrorCode::Canceled); - record.updated_at = Utc::now(); - } - if record.continuation_emitted_at.is_none() - && matches!( - record.continuation, - crate::AuthContinuationRef::TurnGateResume { .. } - ) - { - report - .canceled_turn_gate_continuations - .push(AuthContinuationEvent { - flow_id: record.id, - scope: record.scope.clone(), - continuation: record.continuation.clone(), - provider: record.provider.clone(), - credential_account_id: record.credential_account_id, - emitted_at: Utc::now(), - }); - } - } - } Ok(report) } } diff --git a/crates/ironclaw_auth/tests/auth_engine_contract.rs b/crates/ironclaw_auth/tests/auth_engine_contract.rs index 111cf95d25c..9b271d2c21c 100644 --- a/crates/ironclaw_auth/tests/auth_engine_contract.rs +++ b/crates/ironclaw_auth/tests/auth_engine_contract.rs @@ -289,6 +289,34 @@ fn manifest_recipe(package: &str, vendor: &str) -> ResolvedVendorAuthRecipe { } } +/// The unified shared-vendor recipe the production resolver builds: recipes +/// for one vendor are identical except `scopes`/`display_name`, and the scope +/// ceiling is the union across every declaring manifest +/// (`ironclaw_extension_host::unified_vendor_recipes`, overview §3.2). This +/// test-local mirror unions the real bundled manifests the same way so the +/// engine suite exercises the ceiling production actually resolves. +fn unified_manifest_recipe(vendor: &str, packages: &[&str]) -> ResolvedVendorAuthRecipe { + let mut packages = packages.iter(); + let first = packages + .next() + .expect("unified recipe needs at least one package"); + let mut unified = manifest_recipe(first, vendor); + for package in packages { + let next = manifest_recipe(package, vendor); + let (VendorAuthRecipe::Oauth2Code(unified_recipe), VendorAuthRecipe::Oauth2Code(incoming)) = + (&mut unified.recipe, &next.recipe) + else { + panic!("unified_manifest_recipe unions oauth2_code recipes only"); + }; + for scope in &incoming.scopes { + if !unified_recipe.scopes.contains(scope) { + unified_recipe.scopes.push(scope.clone()); + } + } + } + unified +} + fn synthetic_recipe(vendor: &str, toml_text: &str) -> ResolvedVendorAuthRecipe { let recipe: VendorAuthRecipe = toml::from_str(toml_text).expect("synthetic recipe parses"); ResolvedVendorAuthRecipe { @@ -667,10 +695,11 @@ async fn pointer_extraction_reads_nested_fields_and_scope_fallback() { .engine .exchange_callback( context, - // Request both granted scopes so the A6 clamp (granted ∩ requested) - // is a no-op here and this case keeps proving comma-separated - // multi-scope pointer extraction. The over-claim clamp itself is - // covered by `exchange_clamps_echoed_scopes_to_granted_intersect_requested`. + // Request both granted scopes so the A6 clamp (granted ∩ recipe + // ceiling) is a no-op here and this case keeps proving + // comma-separated multi-scope pointer extraction. The over-claim + // clamp itself is covered by + // `exchange_clamps_echoed_scopes_to_recipe_ceiling`. callback_request( "slack", vec![ @@ -733,13 +762,14 @@ async fn pointer_extraction_reads_nested_fields_and_scope_fallback() { } /// A6 · Scope downgrade / over-claim on the echoed-scope path (RFC 9700 §2.3). -/// The vendor echoes a scope set that both over-grants a scope the user never -/// requested (`chat:write`) and omits one that was requested (`channels:read`). -/// The stored grant must be exactly `granted ∩ requested` — the over-granted -/// scope is dropped (no over-claim) and the grant is never widened to the full -/// requested set. Generic clamp, no vendor branch. +/// The vendor echoes a scope set that both over-grants a scope no recipe ever +/// declared (`admin.conversations:write`, outside the ceiling) and omits one +/// that was requested (`channels:read`). The stored grant must be exactly +/// `granted ∩ recipe ceiling` — the outside-ceiling scope is dropped (no +/// over-claim) and the grant is never widened to the requested set. Generic +/// clamp, no vendor branch. #[tokio::test] -async fn exchange_clamps_echoed_scopes_to_granted_intersect_requested() { +async fn exchange_clamps_echoed_scopes_to_recipe_ceiling() { let harness = Harness::new(vec![manifest_recipe("slack", "slack")]); let scope = test_scope(); harness.server.script( @@ -752,9 +782,9 @@ async fn exchange_clamps_echoed_scopes_to_granted_intersect_requested() { "authed_user": { "id": "U100", "access_token": "xoxp-clamp-token", - // Over-grants chat:write (not requested) and omits channels:read - // (requested) — both within the recipe ceiling. - "scope": "search:read,chat:write" + // Grants a scope outside the recipe ceiling (dropped) and + // omits channels:read (requested — never widened back in). + "scope": "search:read,admin.conversations:write" } }), ); @@ -775,11 +805,77 @@ async fn exchange_clamps_echoed_scopes_to_granted_intersect_requested() { assert_eq!( exchange.scopes, vec![ProviderScope::new("search:read").unwrap()], - "stored grant is granted ∩ requested: chat:write over-grant dropped, \ + "stored grant is granted ∩ ceiling: the undeclared scope is dropped, \ channels:read never widened in" ); } +/// The shared-vendor cumulative-grant regression (gmail → google-docs +/// sign-out): several extensions share one vendor account, each connect +/// requests only its own extension's scopes, and a cumulative-grant vendor +/// (recipe data: Google's `include_granted_scopes` authorize param) echoes +/// previously granted scopes on every exchange. The clamp must keep every +/// echoed scope inside the UNIFIED recipe ceiling — clamping to this flow's +/// request would strip the first extension's scopes from the shared account +/// and sign it out. +#[tokio::test] +async fn exchange_preserves_cumulative_grant_within_unified_ceiling() { + // The production resolver unions scopes across every manifest declaring + // the vendor (`unified_vendor_recipes`); mirror that union over the real + // gmail + google-docs manifests. + let harness = Harness::new(vec![unified_manifest_recipe( + "google", + &["gmail", "google-docs"], + )]); + let scope = test_scope(); + let docs_scopes = vec![ + ProviderScope::new("https://www.googleapis.com/auth/documents").unwrap(), + ProviderScope::new("https://www.googleapis.com/auth/documents.readonly").unwrap(), + ]; + // The docs connect happens second: gmail's scopes were granted earlier, + // so Google echoes the cumulative grant alongside the newly consented + // docs scopes (space-separated, as Google returns them). + let cumulative_grant = [ + "https://www.googleapis.com/auth/documents", + "https://www.googleapis.com/auth/documents.readonly", + "https://www.googleapis.com/auth/gmail.readonly", + "https://www.googleapis.com/auth/gmail.send", + "https://www.googleapis.com/auth/gmail.modify", + ] + .join(" "); + harness.server.script( + "https://oauth2.googleapis.com/token", + 200, + serde_json::json!({ + "access_token": "ya29-cumulative", + "refresh_token": "1//refresh-cumulative", + "expires_in": 3599, + "scope": cumulative_grant + }), + ); + let exchange = harness + .engine + .exchange_callback( + exchange_context(&scope), + callback_request("google", docs_scopes), + ) + .await + .expect("google exchange"); + let stored: Vec<&str> = exchange.scopes.iter().map(|s| s.as_str()).collect(); + assert_eq!( + stored, + vec![ + "https://www.googleapis.com/auth/documents", + "https://www.googleapis.com/auth/documents.readonly", + "https://www.googleapis.com/auth/gmail.readonly", + "https://www.googleapis.com/auth/gmail.send", + "https://www.googleapis.com/auth/gmail.modify", + ], + "the cumulative grant within the unified ceiling is preserved — \ + gmail's scopes survive the google-docs connect" + ); +} + #[tokio::test] async fn missing_scope_without_fallback_fails_the_exchange() { // Google's recipe extracts `/scope` with the default `reject` behavior. @@ -1347,10 +1443,17 @@ async fn dcr_vendor_registers_once_and_runs_standard_oauth_afterwards() { // The token exchange then runs the standard oauth2_code flow against the // discovered token endpoint, carrying the RFC 8707 resource indicator. + // Notion rotates single-use refresh tokens on ~1h access tokens; the + // recipe must capture both or the credential silently dies at expiry + // (the A4 regression). harness.server.script( "https://mcp.notion.com/discovered/token", 200, - serde_json::json!({ "access_token": "notion-access", "expires_in": 3600 }), + serde_json::json!({ + "access_token": "notion-access", + "refresh_token": "notion-refresh-1", + "expires_in": 3600 + }), ); let exchange = harness .engine @@ -1361,6 +1464,15 @@ async fn dcr_vendor_registers_once_and_runs_standard_oauth_afterwards() { .await .expect("notion exchange"); assert!(exchange.provider_identity.is_none()); + let refresh_secret = exchange + .refresh_secret + .as_ref() + .expect("notion recipe captures the rotating refresh token (A4)"); + assert_eq!( + harness.secret_value(&scope.resource, refresh_secret).await, + Some("notion-refresh-1".to_string()), + "the captured refresh token is stored for the refresh path" + ); let token_request = &harness .server .requests_for("https://mcp.notion.com/discovered/token")[0]; @@ -1376,6 +1488,35 @@ async fn dcr_vendor_registers_once_and_runs_standard_oauth_afterwards() { ); } +/// A4 pin on the REAL bundled manifest: Notion issues ~1h access tokens with +/// single-use rotating refresh tokens, so its recipe must declare the +/// `refresh_token`/`expires_in` capture pointers and the rotation flag. The +/// pointer-driven engine captures nothing that is not declared — without +/// these the token is stored non-expiring and every Notion connection dies +/// at the first expiry with no recovery. +#[test] +fn notion_recipe_declares_refresh_and_expiry_capture() { + let resolved = manifest_recipe("notion-mcp", "notion"); + let VendorAuthRecipe::Oauth2Code(recipe) = &resolved.recipe else { + panic!("notion declares oauth2_code"); + }; + assert!( + recipe.token_response.refresh_token.is_some(), + "notion token_response must capture /refresh_token" + ); + assert!( + recipe.token_response.expires_in.is_some(), + "notion token_response must capture /expires_in (else stored non-expiring)" + ); + assert!( + recipe + .refresh + .as_ref() + .is_some_and(|refresh| refresh.rotates_refresh_token), + "notion refresh tokens rotate single-use; the recipe must declare it" + ); +} + // --------------------------------------------------------------------------- // AUTH-3: flow state machine — exactly-once callback, cross-flow rejection // --------------------------------------------------------------------------- diff --git a/crates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rs b/crates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rs index d11aae57073..9c82907fb8b 100644 --- a/crates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rs +++ b/crates/ironclaw_auth/tests/auth_product_contract/cleanup_contract.rs @@ -781,6 +781,15 @@ async fn completed_unacknowledged_turn_gate_cleanup_emits_once_then_converges() assert_eq!(report.canceled_turn_gate_continuations.len(), 1); let event = &report.canceled_turn_gate_continuations[0]; assert_eq!(event.flow_id, flow.id); + // Callback-wins invariant (removal/callback race): a credential minted by + // a flow that completed before — or raced ahead of — the removal must not + // survive the cleanup. The account scan runs AFTER flow cancellation, so + // a mint that beat the cancellation is still swept here. + assert_eq!( + report.revoked_accounts, + vec![account.id], + "the completed flow's credential is revoked by the same cleanup" + ); services .mark_continuation_dispatched(&owner, flow.id, event.emitted_at) diff --git a/crates/ironclaw_first_party_extensions/assets/notion-mcp/manifest.toml b/crates/ironclaw_first_party_extensions/assets/notion-mcp/manifest.toml index 2034bcb5f33..e45d0fa2496 100644 --- a/crates/ironclaw_first_party_extensions/assets/notion-mcp/manifest.toml +++ b/crates/ironclaw_first_party_extensions/assets/notion-mcp/manifest.toml @@ -27,6 +27,15 @@ authorization_endpoint = "https://mcp.notion.com/authorize" token_endpoint = "https://mcp.notion.com/token" scopes = [] +# Notion MCP is OAuth 2.1 with a dynamically registered public client: +# access tokens expire (~1h) and refresh tokens rotate single-use. Without +# the refresh/expiry captures below the engine stores the token as +# non-expiring and every Notion connection dies within the hour. +[auth.notion.refresh] +rotates_refresh_token = true + [auth.notion.token_response] access_token = "/access_token" +refresh_token = "/refresh_token" +expires_in = "/expires_in" scope = { path = "/scope", missing = "fallback_to_requested" } diff --git a/crates/ironclaw_product_workflow/src/auth_continuation.rs b/crates/ironclaw_product_workflow/src/auth_continuation.rs index 848c5e51fda..0def9805eeb 100644 --- a/crates/ironclaw_product_workflow/src/auth_continuation.rs +++ b/crates/ironclaw_product_workflow/src/auth_continuation.rs @@ -93,7 +93,14 @@ impl ProductAuthTurnGateResumeDispatcher { &self, event: AuthContinuationEvent, ) -> Result { - self.dispatch_turn_gate(event, None, false).await + // Tolerate an already-settled gate exactly like the deny path does: + // a completed continuation is re-dispatched whenever the durable + // `continuation_emitted_at` fence was not stamped (e.g. the fan-out + // sweep was incomplete and the whole dispatch stays retryable). On + // replay the primary run has typically already resumed — its gate is + // no longer the blocked gate — and that is the settled outcome this + // continuation wanted, not an error to retry forever. + self.dispatch_turn_gate(event, None, true).await } async fn dispatch_turn_gate( @@ -683,6 +690,46 @@ mod tests { assert!(coordinator.resumes().is_empty()); } + /// A replayed RESUME continuation whose gate already settled converges as + /// a no-op instead of erroring forever. Completed continuations replay + /// whenever the durable `continuation_emitted_at` fence was not stamped — + /// e.g. the blocked-run fan-out sweep was incomplete and the whole + /// dispatch stayed retryable; by then the primary run has typically + /// resumed (or re-blocked on a NEW gate), which is the settled outcome the + /// continuation wanted. + #[tokio::test] + async fn resume_continuation_leaves_settled_gate_untouched() { + let coordinator = Arc::new(RecordingTurnCoordinator::default()); + let dispatcher = ProductAuthTurnGateResumeDispatcher::new(coordinator.clone()); + let run_id = TurnRunId::new(); + // Re-blocked on a NEW gate: the replayed resume for the old gate must + // not touch it. + coordinator.set_state(run_state( + run_id, + TurnStatus::BlockedAuth, + Some("gate:new-auth"), + )); + dispatcher + .dispatch_auth_continuation(scoped_event(AuthContinuationRef::TurnGateResume { + turn_run_ref: TurnRunRef::new(run_id.to_string()).unwrap(), + gate_ref: AuthGateRef::new("gate:stale-auth").unwrap(), + })) + .await + .expect("a replayed resume for a superseded gate converges"); + + // Already resumed (no longer blocked at all): same convergence. + coordinator.set_state(run_state(run_id, TurnStatus::Queued, None)); + dispatcher + .dispatch_auth_continuation(scoped_event(AuthContinuationRef::TurnGateResume { + turn_run_ref: TurnRunRef::new(run_id.to_string()).unwrap(), + gate_ref: AuthGateRef::new("gate:stale-auth").unwrap(), + })) + .await + .expect("a replayed resume for an already-resumed run converges"); + + assert!(coordinator.resumes().is_empty()); + } + #[tokio::test] async fn turn_gate_continuation_uses_subject_scope_and_original_actor() { let coordinator = Arc::new(RecordingTurnCoordinator::default()); @@ -738,17 +785,15 @@ mod tests { gate_ref: AuthGateRef::new("gate:auth").unwrap(), }); - let err = dispatcher + // The safety property is side-effect freedom: an auth continuation + // must never resolve a non-auth gate. It converges as a settled no-op + // (replay-tolerant) instead of erroring forever — the run left + // BlockedAuth, so this continuation's business is done. + dispatcher .dispatch_turn_gate_resume(event) .await - .expect_err("non-auth gates must not resume through auth continuation"); + .expect("a non-auth-blocked run converges without a resume"); - assert!(matches!( - err, - ProductWorkflowError::AuthContinuationRejected { - kind: AuthContinuationRejectionKind::UnauthorizedBlockedGate - } - )); assert!(coordinator.resumes().is_empty()); } @@ -767,17 +812,15 @@ mod tests { gate_ref: AuthGateRef::new("gate:auth").unwrap(), }); - let err = dispatcher + // The safety property is side-effect freedom: a stale continuation + // must never resolve a DIFFERENT gate. It converges as a settled + // no-op (replay-tolerant) instead of erroring forever — the gate it + // was minted for is gone. + dispatcher .dispatch_turn_gate_resume(event) .await - .expect_err("stale auth gate callbacks must not resume a different gate"); + .expect("a superseded gate converges without a resume"); - assert!(matches!( - err, - ProductWorkflowError::AuthContinuationRejected { - kind: AuthContinuationRejectionKind::UnauthorizedBlockedGate - } - )); assert!(coordinator.resumes().is_empty()); } diff --git a/crates/ironclaw_reborn_composition/src/blocked_auth_resume.rs b/crates/ironclaw_reborn_composition/src/blocked_auth_resume.rs index 45e843d62a6..7eb32fca599 100644 --- a/crates/ironclaw_reborn_composition/src/blocked_auth_resume.rs +++ b/crates/ironclaw_reborn_composition/src/blocked_auth_resume.rs @@ -15,9 +15,11 @@ //! Ordering matters: the decorator runs at continuation-dispatch time, which //! is strictly after `complete_oauth_callback` committed the credential //! account, so resumed runs re-running `extension_activate` find their -//! requirements satisfied. Fan-out is best-effort per run (log and continue) -//! and idempotent per (flow, run); the primary continuation's result is -//! returned unchanged. +//! requirements satisfied. Fan-out is idempotent per (flow, run), and an +//! incomplete sweep returns an error so the durable continuation remains +//! undispatched and a re-drive (flow reconcile / lifecycle cleanup) retries +//! it — resumed runs leave `BlockedAuth` and are skipped on replay, and the +//! primary dispatcher settles idempotently on an already-settled gate. //! //! Scope safety mirrors the deleted read model: the scan is bounded to the //! completed flow's `tenant_id` + explicit owner `user_id`, so this can never @@ -96,17 +98,26 @@ impl BlockedAuthResumeFanout { } } - async fn fan_out(&self, event: &AuthContinuationEvent) { + /// Sweep the caller's other provider-blocked runs. An incomplete sweep — + /// unreadable turn snapshot, or any retryable resume failure — returns an + /// error so the caller leaves the durable continuation UNDISPATCHED and a + /// re-drive (the browser's flow reconcile, or lifecycle cleanup) retries + /// the whole dispatch. Retries are safe end-to-end: already-resumed runs + /// have left `BlockedAuth` and are skipped here, and the primary + /// dispatcher settles idempotently on a no-longer-blocked gate. + async fn fan_out(&self, event: &AuthContinuationEvent) -> Result<(), AuthProductError> { let primary_run_id = primary_run_id(&event.continuation); - // silent-ok: snapshot source logs underlying read failures; auth - // continuation fan-out is best-effort. let Some(snapshot) = self.snapshot_source.snapshot().await else { - tracing::debug!("blocked-auth fan-out skipped: no turn snapshot available"); - return; + tracing::warn!( + flow_id = %event.flow_id, + "blocked-auth fan-out could not read the turn snapshot; keeping the continuation retryable" + ); + return Err(AuthProductError::BackendUnavailable); }; let tenant_id = &event.scope.resource.tenant_id; let user_id = &event.scope.resource.user_id; let mut resumed = 0usize; + let mut incomplete = false; for run in &snapshot.runs { if run.status != TurnStatus::BlockedAuth { continue; @@ -175,12 +186,16 @@ impl BlockedAuthResumeFanout { match self.turn_coordinator.resume_turn(request).await { Ok(_) => resumed += 1, Err(error) => { + // Keep sweeping so one failing run does not starve the + // rest, but report the sweep incomplete: the continuation + // must stay undispatched so a re-drive retries this run. tracing::warn!( run_id = %run.run_id, flow_id = %event.flow_id, %error, - "blocked-auth fan-out failed to resume a parked run" + "blocked-auth fan-out failed to resume a parked run; keeping the continuation retryable" ); + incomplete = true; } } } @@ -192,6 +207,10 @@ impl BlockedAuthResumeFanout { "blocked-auth fan-out resumed additional parked runs" ); } + if incomplete { + return Err(AuthProductError::BackendUnavailable); + } + Ok(()) } } @@ -216,9 +235,11 @@ impl RebornAuthContinuationDispatcher for BlockedAuthResumeFanout { // Fan out regardless of the primary outcome: the credential account // exists once this event is emitted, and the caller's other parked // runs deserve the resume even if the primary run's own resume hit a - // conflict. - self.fan_out(&event).await; - primary + // conflict. Either failure keeps the continuation undispatched (the + // caller skips `mark_continuation_dispatched`), so a re-drive retries + // both legs; replays are idempotent on both. + let sweep = self.fan_out(&event).await; + primary.and(sweep) } async fn dispatch_canceled_auth_continuation( @@ -292,7 +313,10 @@ mod tests { #[derive(Default)] struct RecordingTurnCoordinator { resumed: Mutex>, - fail_resumes: bool, + /// Fail this many resume attempts before succeeding — models a + /// transient coordinator/store outage that a re-driven dispatch + /// recovers from. + fail_next_resumes: Mutex, } #[async_trait] @@ -312,10 +336,14 @@ mod tests { &self, request: ResumeTurnRequest, ) -> Result { - if self.fail_resumes { - return Err(TurnError::Unavailable { - reason: "resume backend down".to_string(), - }); + { + let mut fail_next = self.fail_next_resumes.lock().expect("fail counter"); + if *fail_next > 0 { + *fail_next -= 1; + return Err(TurnError::Unavailable { + reason: "resume backend down".to_string(), + }); + } } let run_id = request.run_id; self.resumed.lock().expect("resume lock").push(request); @@ -509,7 +537,7 @@ mod tests { fn fanout_with( snapshot: TurnPersistenceSnapshot, - fail_resumes: bool, + fail_next_resumes: usize, ) -> ( BlockedAuthResumeFanout, Arc, @@ -520,7 +548,7 @@ mod tests { }); let coordinator = Arc::new(RecordingTurnCoordinator { resumed: Mutex::new(Vec::new()), - fail_resumes, + fail_next_resumes: Mutex::new(fail_next_resumes), }); let fanout = BlockedAuthResumeFanout::new( inner.clone(), @@ -557,7 +585,7 @@ mod tests { ], ..Default::default() }; - let (fanout, coordinator, inner) = fanout_with(snapshot, false); + let (fanout, coordinator, inner) = fanout_with(snapshot, 0); fanout .dispatch_auth_continuation(event( @@ -594,7 +622,7 @@ mod tests { runs: vec![first.clone(), second.clone()], ..Default::default() }; - let (fanout, coordinator, _inner) = fanout_with(snapshot, false); + let (fanout, coordinator, _inner) = fanout_with(snapshot, 0); fanout .dispatch_auth_continuation(event("slack", AuthContinuationRef::SetupOnly)) @@ -612,22 +640,51 @@ mod tests { ); } + /// An incomplete sweep keeps the continuation retryable: the first + /// dispatch hits a transient resume failure and must error (so the caller + /// never stamps `continuation_emitted_at`); the re-driven dispatch retries + /// the sweep and resumes the still-parked run. Regression for the + /// best-effort weakening where a transient coordinator error permanently + /// stranded every other parked run (mega-PR review finding). #[tokio::test] - async fn fan_out_failures_do_not_poison_the_primary_result() { + async fn incomplete_fan_out_keeps_the_continuation_retryable() { let waiting = blocked_run(OWNER, TurnRunId::new(), TurnId::new(), slack_requirement()); let snapshot = TurnPersistenceSnapshot { turns: vec![parent_turn(OWNER, &waiting)], runs: vec![waiting], ..Default::default() }; - let (fanout, coordinator, inner) = fanout_with(snapshot, true); + let (fanout, coordinator, inner) = fanout_with(snapshot, 1); - fanout + let error = fanout .dispatch_auth_continuation(event("slack", AuthContinuationRef::SetupOnly)) .await - .expect("resume failures stay best-effort"); + .expect_err("an incomplete sweep must keep the continuation retryable"); + assert!( + matches!(error, AuthProductError::BackendUnavailable), + "incomplete sweep surfaces as retryable backend unavailability" + ); + assert!( + coordinator.resumed.lock().expect("resumed").is_empty(), + "the failed attempt resumed nothing" + ); - assert_eq!(inner.events.lock().expect("events").len(), 1); - assert!(coordinator.resumed.lock().expect("resumed").is_empty()); + // The continuation was never marked dispatched, so a re-drive (flow + // reconcile / lifecycle cleanup) retries the whole dispatch; the + // parked run is still BlockedAuth in the snapshot and now resumes. + fanout + .dispatch_auth_continuation(event("slack", AuthContinuationRef::SetupOnly)) + .await + .expect("the retried sweep completes"); + assert_eq!( + inner.events.lock().expect("events").len(), + 2, + "the primary dispatch replayed alongside the retried sweep" + ); + assert_eq!( + coordinator.resumed.lock().expect("resumed").len(), + 1, + "the parked run resumed exactly once" + ); } } diff --git a/crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs b/crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs index 18023534203..9b7d7973437 100644 --- a/crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs +++ b/crates/ironclaw_reborn_composition/src/extension_host/extension_installation_store.rs @@ -302,9 +302,13 @@ struct WireManifestRecord { source: WireManifestSource, #[serde(default, skip_serializing_if = "Option::is_none")] manifest_hash: Option, - /// The compiled contract (extension-runtime REC-1/REC-2). Loads rebuild - /// from it without reparsing `raw_toml`; absent only on legacy records, - /// which backfill by compiling once at load. + /// The compiled contract (extension-runtime REC-1/REC-2). Every record is + /// persisted with it and loads rebuild from it without reparsing + /// `raw_toml`. This is a blank-slate schema: there is no pre-PR record to + /// migrate, so an absent `resolved` is a corrupt/unexpected row and the + /// load fails loud (see `into_manifest_record`) — it is never backfilled. + /// The field stays `Option` only so a truncated/garbled row deserializes + /// far enough to be rejected with a clear error rather than a serde panic. #[serde(default, skip_serializing_if = "Option::is_none")] resolved: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] diff --git a/crates/ironclaw_reborn_composition/src/factory/auth_tests.rs b/crates/ironclaw_reborn_composition/src/factory/auth_tests.rs index 429c5fd8606..6124440cdfa 100644 --- a/crates/ironclaw_reborn_composition/src/factory/auth_tests.rs +++ b/crates/ironclaw_reborn_composition/src/factory/auth_tests.rs @@ -662,7 +662,7 @@ async fn local_dev_google_oauth_backend_accepts_optional_client_secret_config() } #[tokio::test] -async fn oauth_callback_with_stale_gate_maps_to_terminal_invalid_request() { +async fn oauth_callback_with_stale_gate_converges_without_resuming() { let dir = tempfile::tempdir().expect("tempdir"); let services = build_reborn_services( RebornBuildInput::local_dev("local-dev-auth-stale-owner", dir.path().join("local-dev")) @@ -697,13 +697,34 @@ async fn oauth_callback_with_stale_gate_maps_to_terminal_invalid_request() { ) .await; - let error = product_auth - .handle_oauth_callback(authorized_request(auth_scope, flow_id)) + // A continuation for a superseded gate converges as a settled no-op: the + // credential is minted (that part is real work the user completed) and + // the continuation is acknowledged, but the run's CURRENT gate must not + // be resumed by a stale reference. Erroring here instead used to leave + // the completed flow permanently unacknowledged and the reconcile loop + // hammering a non-retryable failure. + product_auth + .handle_oauth_callback(authorized_request(auth_scope.clone(), flow_id)) .await - .expect_err("stale auth gate should not resume"); + .expect("a stale-gate continuation converges without resuming"); - assert_eq!(error.code, AuthErrorCode::InvalidRequest); - assert!(!error.retryable); + let state = turn_coordinator + .get_run_state(GetRunStateRequest { + scope: scope.clone(), + run_id, + }) + .await + .expect("run state"); + assert_eq!( + state.status, + TurnStatus::BlockedAuth, + "the run stays parked on its CURRENT gate" + ); + assert_eq!( + state.gate_ref.as_ref().map(|gate| gate.as_str()), + Some("gate:current-auth"), + "the stale continuation never touched the current gate" + ); } #[tokio::test] diff --git a/crates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rs b/crates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rs index cc64724aa30..c145fafaf4c 100644 --- a/crates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rs +++ b/crates/ironclaw_reborn_composition/src/product_auth/durable/cleanup.rs @@ -19,6 +19,60 @@ where request: SecretCleanupRequest, ) -> Result { let mut report = SecretCleanupReport::default(); + // A3 · Cancel the provider's pending flows BEFORE enumerating + // accounts (RFC 9700 §4.7.1 + RFC 7009 §1). Ordering closes the + // callback/removal race: a callback racing this cleanup either loses + // — its flow is canceled first, so `complete_oauth_callback` rejects + // before writing an account — or wins and completes first, in which + // case the account it minted already exists when the scan below runs + // and is revoked like any other. Scanning accounts first left a + // window where a callback completing between the scan and the flow + // cancellation minted a credential that survived removal. + // + // Owner decision 2026-07-15: cancel on both Deactivate and Uninstall. + // Shared-vendor safe by construction — the removal caller only + // selects a provider exclusive to the removed extension. Idempotent: + // a concurrently terminal flow is skipped, never an error. + // + // F2 · Any enumerated flow whose `TurnGateResume` continuation was + // never acknowledged — freshly canceled here or already terminal — is + // reported so the composition layer denies its blocked turn gate + // instead of leaving the turn parked. `mark_continuation_dispatched` + // makes the handoff emit-once across cleanup retries. + if let Some(provider) = request.provider.as_ref() { + for flow in self + .lifecycle_flows_for_owner_provider(&request.scope.resource, provider) + .await? + { + let canceled = match flow.status { + status if ironclaw_auth::is_terminal_status(status) => flow, + _ => match self.cancel_flow(&flow.scope, flow.id).await { + Ok(canceled) => canceled, + Err(AuthProductError::Canceled) + | Err(AuthProductError::FlowAlreadyTerminal) + | Err(AuthProductError::UnknownOrExpiredFlow) => flow, + Err(error) => return Err(error), + }, + }; + if canceled.continuation_emitted_at.is_none() + && matches!( + canceled.continuation, + AuthContinuationRef::TurnGateResume { .. } + ) + { + report + .canceled_turn_gate_continuations + .push(AuthContinuationEvent { + flow_id: canceled.id, + scope: canceled.scope.clone(), + continuation: canceled.continuation.clone(), + provider: canceled.provider.clone(), + credential_account_id: canceled.credential_account_id, + emitted_at: Utc::now(), + }); + } + } + } // Credential-owner granularity, not full scope equality: lifecycle and // disconnect callers mint a fresh `invocation_id` (and often arrive // from a different thread), so an exact-scope lookup could never find @@ -89,54 +143,6 @@ where self.purge_secret_handle(¤t.scope.resource, h).await; } } - // A3 · Removal/disconnect cancels pending flows (RFC 9700 §4.7.1 + - // RFC 7009 §1). A provider-selected cleanup cancels EVERY non-terminal - // flow for the credential-owner + provider so a late provider callback - // can no longer mint a credential for a torn-down extension. Owner - // decision 2026-07-15: cancel on both Deactivate and Uninstall. Shared- - // vendor safe by construction — the removal caller only selects a - // provider exclusive to the removed extension. Idempotent: a concurrently - // terminal flow is skipped, never an error. - // - // F2 · Any enumerated flow whose `TurnGateResume` continuation was - // never acknowledged — freshly canceled here or already terminal — is - // reported so the composition layer denies its blocked turn gate - // instead of leaving the turn parked. `mark_continuation_dispatched` - // makes the handoff emit-once across cleanup retries. - if let Some(provider) = request.provider.as_ref() { - for flow in self - .lifecycle_flows_for_owner_provider(&request.scope.resource, provider) - .await? - { - let canceled = match flow.status { - status if ironclaw_auth::is_terminal_status(status) => flow, - _ => match self.cancel_flow(&flow.scope, flow.id).await { - Ok(canceled) => canceled, - Err(AuthProductError::Canceled) - | Err(AuthProductError::FlowAlreadyTerminal) - | Err(AuthProductError::UnknownOrExpiredFlow) => flow, - Err(error) => return Err(error), - }, - }; - if canceled.continuation_emitted_at.is_none() - && matches!( - canceled.continuation, - AuthContinuationRef::TurnGateResume { .. } - ) - { - report - .canceled_turn_gate_continuations - .push(AuthContinuationEvent { - flow_id: canceled.id, - scope: canceled.scope.clone(), - continuation: canceled.continuation.clone(), - provider: canceled.provider.clone(), - credential_account_id: canceled.credential_account_id, - emitted_at: Utc::now(), - }); - } - } - } Ok(report) } } diff --git a/crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs b/crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs index e603fae434f..5d07ea5007d 100644 --- a/crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs +++ b/crates/ironclaw_reborn_composition/src/product_auth/durable/flows.rs @@ -158,8 +158,21 @@ where record.pkce_verifier_hash = Some(exchange.pkce_verifier_hash); record.credential_account_id = Some(account_id); record.updated_at = now; - self.write_flow(scope, &record, CasExpectation::Version(version)) - .await?; + if let Err(error) = self + .write_flow(scope, &record, CasExpectation::Version(version)) + .await + { + // The exchange already minted/updated the credential account, but + // the flow's completion write lost a CAS race — e.g. a concurrent + // lifecycle cancel from extension removal on another replica. A + // live credential must not outlive its flow: revoke it + // best-effort (clearing its secret handles) so a torn-down + // extension cannot retain a token minted mid-removal, then + // surface the original conflict. + self.compensate_unanchored_callback_account(scope, account_id) + .await; + return Err(error); + } Ok(record) } @@ -499,6 +512,58 @@ impl FilesystemAuthProductServices where F: RootFilesystem + 'static, { + /// Best-effort compensation for a callback whose account write committed + /// but whose flow-completion write lost a CAS race — typically a + /// concurrent lifecycle cancel while an extension is being removed. + /// Revokes the account and purges its secret handles so the credential + /// cannot outlive its canceled flow. Failures are logged, never + /// propagated: the caller surfaces the original conflict, and the + /// lifecycle cleanup's account scan (which now runs AFTER flow + /// cancellation) remains the durable backstop. + async fn compensate_unanchored_callback_account( + &self, + scope: &ironclaw_auth::AuthProductScope, + account_id: CredentialAccountId, + ) { + let lock = self.lock_for(format!("account:{account_id}")); + let _guard = lock.lock().await; + let (mut account, version) = match self.read_account(scope, account_id).await { + Ok(Some(found)) => found, + Ok(None) => return, + Err(error) => { + tracing::warn!( + %account_id, + error_code = ?error.code(), + "callback compensation could not read the just-minted account" + ); + return; + } + }; + let purge_access = account.access_secret.take(); + let purge_refresh = account.refresh_secret.take(); + account.status = CredentialAccountStatus::Revoked; + account.updated_at = Utc::now(); + if let Err(error) = self + .write_account(&account, CasExpectation::Version(version)) + .await + { + tracing::warn!( + %account_id, + error_code = ?error.code(), + "callback compensation could not revoke the just-minted account" + ); + return; + } + if let Some(handle) = &purge_access { + self.purge_secret_handle(&account.scope.resource, handle) + .await; + } + if let Some(handle) = &purge_refresh { + self.purge_secret_handle(&account.scope.resource, handle) + .await; + } + } + async fn resolve_callback_account( &self, flow_id: AuthFlowId, diff --git a/docs/reborn/auth/recipe-parity-checklist.md b/docs/reborn/auth/recipe-parity-checklist.md index 9530a85f90a..d8e13a06d52 100644 --- a/docs/reborn/auth/recipe-parity-checklist.md +++ b/docs/reborn/auth/recipe-parity-checklist.md @@ -56,23 +56,30 @@ Legend: ✅ all boxes ticked · ⏳ in progress · ⚪ N/A · 🔴 known-broken --- -## Notion 🟢 +## Notion 🟢 (re-verified on THIS branch — 2026-07-15 audit correction) -Recipe = `oauth/notion_oauth.rs:10-20` (`HostOAuthProviderSpec`, `TokenResponseShape::Standard`, -`ExchangeScopePolicy::FallbackToRequested`, token endpoint `https://mcp.notion.com/token`). -Manifest `assets/notion-mcp/manifest.toml` declares credentials via `runtime_credentials` only. -_(Main-provenance names; on the rollup the equivalent is the `[auth.notion]` bundled recipe → -`OAuth2CodeRecipe` executed by `ironclaw_auth`'s `AuthEngine`.)_ +Recipe = the `[auth.notion]` bundled TOML (`assets/notion-mcp/manifest.toml`) → +`OAuth2CodeRecipe` executed by `ironclaw_auth`'s `AuthEngine` (DCR — no +`client_credentials` block). _Audit correction: the previous 🟢 here rested on +main's auto-parsing `Standard` token shape, which did **not** survive the +merge — the pointer-driven engine captures only declared pointers, and the +bundled recipe declared none for refresh/expiry, so every Notion connection +died ~1h after connect. Fixed by declaring the captures in the manifest; +provenance below is this branch._ - [x] **Handshake** — PKCE-S256 + host `state` + HTTPS-only token endpoint (inherited from §S H1-H5). -- [x] **Token lifecycle — captures `refresh_token` + `expires_in`** (A4). The shared `Standard` - shape parses both (`oauth_provider_client.rs:632-644,708-723`); `store_token_pair:404-413` - computes `access_expires_at` from `expires_in`. **Proven:** `product_auth_providers.rs` - `compose_provider_client_routes_notion_to_configured_provider_spec:407` (asserts the composed - Notion client carries refresh + expiry) + `oauth_provider_client/tests.rs` token-sink tests. +- [x] **Token lifecycle — captures `refresh_token` + `expires_in`** (A4). + `[auth.notion.token_response]` declares `/refresh_token` + `/expires_in`; + `[auth.notion.refresh] rotates_refresh_token = true`. **Proven:** + `auth_engine_contract::notion_recipe_declares_refresh_and_expiry_capture` + (pin on the real manifest) + + `auth_engine_contract::dcr_vendor_registers_once_and_runs_standard_oauth_afterwards` + (exchange captures and stores the rotating refresh token). - [x] **Non-expiring guard** — absent/`0` `expires_in` → non-expiring, not already-expired - (`store_token_pair:409` `.filter(|secs| *secs > 0)`). Proven by the same tests. _(On the rollup: - `exchange.rs::store_token_pair` `.filter(|seconds| *seconds > 0)`.)_ + (`engine/exchange.rs::store_token_pair` `.filter(|seconds| *seconds > 0)`). +- [ ] **A16 (now non-latent)** — with refresh live, a Notion DCR client expiry + surfaces as `invalid_client` on refresh; the engine does not yet re-register. + Tracked backlog, sequenced behind this fix. - flow-lifecycle & engine-hardening: inherit §S. ## Google 🟢 (gmail, drive, calendar, docs, sheets, slides)