From e7984f28cc40439e86b6765df7eaa3b46465cd65 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Mon, 29 Jun 2026 11:07:38 -0700 Subject: [PATCH] fix: step up OAuth scopes for under-scoped tokens in non-interactive sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_credential_for` had an asymmetry: when a stored token was missing a required scope it consulted `session_is_interactive()` and, with no TTY, returned a hard "missing required scope(s)" error instead of acquiring the scope. The no-token branch, by contrast, always ran the browser flow. So a command run from a non-TTY context (e.g. an agent harness or piped stdio) failed when it held an under-scoped token, but auto-authenticated once the token was cleared — surprising and inconsistent. Drop the interactivity gate from the under-scoped branch so it takes the same `reauthenticate(union)` path the no-token branch already uses. Missing scopes now always trigger step-up; the browser flow's 120s callback timeout bounds truly-headless hangs (identical to today's no-token behavior). The union (defaults ∪ granted ∪ required) is unchanged, so step-up still never narrows an existing grant. Simplify `plan_step_up` to Covered/Reauthenticate and remove the now-unused `MissingNonInteractive` variant, `session_is_interactive`, and `missing_scope_error`. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 85 ++++++++++++++---------------------------------- 1 file changed, 25 insertions(+), 60 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 25048c3..bedc329 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -58,7 +58,7 @@ use std::{ collections::HashMap, - io::{IsTerminal, Write}, + io::Write, net::{SocketAddr, TcpListener}, sync::Arc, time::Duration, @@ -771,18 +771,17 @@ impl AuthProvider for PkceAuthProvider { // stepping up would open the browser twice). if let Some(token) = self.existing_token(env).await? { // Decide based on what the token grants (JWT claim plus the scopes it - // was obtained with). Step-up means re-consent: the authorization - // server has no silent scope-expansion grant, so in non-interactive - // contexts we fail fast rather than hang on the callback timeout. + // was obtained with). let granted = granted_scopes(&token); - match plan_step_up(&granted, required, session_is_interactive()) { + match plan_step_up(&granted, required) { StepUp::Covered => return Ok(self.build_credential(env, &token)), - StepUp::MissingNonInteractive(missing) => { - return Err(missing_scope_error(env, &missing)); - } - // Re-consent: resolve per-env defaults only now (off the - // cached-token hot path) and request defaults ∪ already-granted ∪ - // required so step-up never drops previously-acquired scopes. + // Step-up is re-consent: the authorization server has no silent + // scope-expansion grant, so acquire the missing scopes with a + // fresh login — the same browser flow the no-token path runs + // below, rather than failing when stdio is not a TTY. Resolve + // per-env defaults only now (off the cached-token hot path) and + // request defaults ∪ already-granted ∪ required so step-up never + // drops previously-acquired scopes. StepUp::Reauthenticate => { let union = union_scopes(&self.effective_scopes(env), &granted, required); let token = self.reauthenticate(env, &union).await?; @@ -1045,14 +1044,10 @@ fn granted_scopes(token: &StoredToken) -> Vec { enum StepUp { /// The token already covers every required scope. Covered, - /// Re-authenticate (interactively) to acquire the missing scopes. The caller - /// builds the requested set (defaults ∪ granted ∪ required) only on this - /// path, so resolving per-env default scopes stays off the cached-token hot - /// path. + /// Re-authenticate to acquire the missing scopes. The caller builds the + /// requested set (defaults ∪ granted ∪ required) only on this path, so + /// resolving per-env default scopes stays off the cached-token hot path. Reauthenticate, - /// Scopes are missing but the session is non-interactive, so step-up cannot - /// prompt; carries the missing scopes for the error message. - MissingNonInteractive(Vec), } /// Decides the step-up action from what the token grants versus what the command @@ -1060,30 +1055,17 @@ enum StepUp { /// decision needs only `granted`/`required`, so the caller can avoid resolving /// defaults (potential `environments.toml` I/O) when a cached token already /// covers the requirement. -fn plan_step_up(granted: &[String], required: &[String], interactive: bool) -> StepUp { - let missing: Vec = required +fn plan_step_up(granted: &[String], required: &[String]) -> StepUp { + let covered = required .iter() - .filter(|scope| !granted.iter().any(|have| have == *scope)) - .cloned() - .collect(); - if missing.is_empty() { + .all(|scope| granted.iter().any(|have| have == scope)); + if covered { StepUp::Covered - } else if interactive { - StepUp::Reauthenticate } else { - StepUp::MissingNonInteractive(missing) + StepUp::Reauthenticate } } -/// Treats the session as interactive if any stdio stream is a TTY, so -/// redirecting one (e.g. capturing stderr) does not block a user who can still -/// complete the browser flow. -fn session_is_interactive() -> bool { - std::io::stdin().is_terminal() - || std::io::stdout().is_terminal() - || std::io::stderr().is_terminal() -} - /// Confirms a freshly (re)authenticated token actually grants `required`. /// /// Re-consent does not guarantee the authorization server grants every requested @@ -1111,20 +1093,6 @@ fn ensure_granted(env: &str, token: &StoredToken, required: &[String]) -> Result } } -/// Error returned when required scopes are missing and step-up cannot prompt. -fn missing_scope_error(env: &str, missing: &[String]) -> CliCoreError { - let display = missing.join(", "); - let hint = missing - .iter() - .map(|scope| format!("--scope {scope}")) - .collect::>() - .join(" "); - CliCoreError::message(format!( - "access token for {env:?} is missing required scope(s): {display}; \ - run `auth login --env {env} {hint}` in an interactive terminal" - )) -} - /// Returns the first claim value that is a non-empty string, in priority order. fn extract_identity(claims: &Map, priority: &[String]) -> String { priority @@ -1386,21 +1354,18 @@ mod tests { } #[test] - fn plan_step_up_covers_reauths_and_fails_non_interactive() { + fn plan_step_up_covers_or_reauthenticates() { let granted = vec!["base".to_owned(), "read".to_owned()]; let read = vec!["read".to_owned()]; let write = vec!["write".to_owned()]; // Already covered (decision needs only granted vs required). - assert_eq!(plan_step_up(&granted, &read, true), StepUp::Covered); - // Missing + interactive → reauth (the caller builds the union with - // per-env defaults only on this path). - assert_eq!(plan_step_up(&granted, &write, true), StepUp::Reauthenticate); - // Missing + non-interactive → fail fast, carrying the missing scopes. - assert_eq!( - plan_step_up(&granted, &write, false), - StepUp::MissingNonInteractive(vec!["write".to_owned()]) - ); + assert_eq!(plan_step_up(&granted, &read), StepUp::Covered); + // Missing → reauthenticate, with no interactivity gate: step-up now + // mirrors the no-token path and acquires the scope via a fresh login + // rather than failing when stdio is not a TTY. The caller builds the + // union (defaults ∪ granted ∪ required) only on this path. + assert_eq!(plan_step_up(&granted, &write), StepUp::Reauthenticate); // The union itself (defaults ∪ granted ∪ required) is covered by // union_scopes' own test. }