diff --git a/docs/components/backend/authenticator/DESIGN.md b/docs/components/backend/authenticator/DESIGN.md index 576820904..d4187873c 100644 --- a/docs/components/backend/authenticator/DESIGN.md +++ b/docs/components/backend/authenticator/DESIGN.md @@ -496,6 +496,8 @@ Implements the API declared in [PRD section 7.1](./PRD.md#71-public-api-surface) Exchange response contract (the load-bearing part): `200` + `X-Gateway-Jwt: Bearer ` + `Cache-Control: max-age = min(authz_cache_max_age, jwt_exp - now - 60 s)`; `401` (no/expired session) + `Cache-Control: no-store`; any other status is treated by the gateway as "authenticator unavailable" and fails closed. +Callback failure contract (issue #2032): `GET /auth/callback` is a browser-facing IdP redirect target, so its failures redirect (`302`) to `default_return_to` with a fixed `auth_error=` query parameter instead of answering problem+json the user cannot act on. Reasons: `state_expired` (unknown, expired -- the 300 s login-state TTL -- or already-consumed state), `idp_error` (the IdP redirected back with `error=`), `invalid_callback` (missing `code`/`state`), `exchange_failed` (code exchange / id_token validation), `access_denied` (no tenant resolved, no matching person in Identity, or a view-as override naming an unknown person -- still a denial, never a fallback to the caller). The reason strings are a fixed vocabulary -- nothing IdP- or caller-supplied reaches the `Location` header. The SPA consumes `auth_error`, restarts the login for the retryable reasons behind a retry loop guard, and shows an error screen for `access_denied` or repeated failures. Rate-limit (`429`) and internal (`5xx`) responses stay problem+json. + ### 3.4 Internal Dependencies | Dependency Module | Interface Used | Purpose | diff --git a/docs/components/backend/authenticator/PRD.md b/docs/components/backend/authenticator/PRD.md index 5992b9b4f..da6195f09 100644 --- a/docs/components/backend/authenticator/PRD.md +++ b/docs/components/backend/authenticator/PRD.md @@ -174,6 +174,8 @@ Defined in the [parent backend PRD](../specs/PRD.md) as `cpt-insightspec-actor-o The system **MUST** implement OIDC authorization code flow with PKCE as a confidential client. The authenticator **MUST** generate `state`, `nonce`, and PKCE verifier per login attempt and validate them on callback. The browser **MUST NOT** receive or transmit the IdP code, ID token, access token, or refresh token at any point. +A failed callback **MUST NOT** dead-end the browser on an error document: `/auth/callback` is an IdP redirect target with no page loaded, so every browser-facing failure (expired, unknown, or replayed `state`; IdP-reported error; missing parameters; code-exchange failure; a denied person) **MUST** redirect (302) back to the SPA with a fixed `auth_error=` query parameter, allowing the SPA to restart the login from scratch (issue #2032). Rate-limit (429) and internal (5xx) responses stay problem+json. + The new session token issued at the end of a successful callback **MUST** be generated server-side from a CSPRNG and **MUST NOT** be derived from, or equal to, any value present in the incoming request (cookies, headers, query). Any `__Host-sid` cookie present on the `/auth/callback` request **MUST** be ignored; if its value maps to a live session in Redis, that session **MUST** be revoked before the new session is created. This prevents session fixation where an attacker plants a known token before the victim logs in. At login the system **MUST** resolve the authenticated person via Identity Service (`sub` to `person_id` plus tenant memberships) and **MUST** fetch access-control claims once, from the permissions service when it exists; until then the configured `authenticator.default_roles` apply. @@ -552,7 +554,7 @@ If Redis is unreachable, `/internal/authz` and `/auth/*` mutations **MUST** fail | Method | Path | Purpose | |--------|------|---------| | GET | `/auth/login` | Start OIDC flow; 302 to IdP. Optional `__override=` view-as target, honored only when `override_enabled` (5.16). | -| GET | `/auth/callback` | OIDC callback; creates session + linked JWT; sets cookie; 302 to SPA. | +| GET | `/auth/callback` | OIDC callback; creates session + linked JWT; sets cookie; 302 to SPA. Failures also 302 to the SPA, with `auth_error=` (5.1). | | POST | `/auth/refresh` | Rotate cookie, extend session TTL; return `{expires_at, refresh_at}`. | | POST | `/auth/logout` | Revoke current session; clear cookie; return RP-logout URL. | | GET | `/auth/me` | Current user, tenants, plus `{expires_at, refresh_at}`; `impersonator_email` on view-as sessions (5.16). | diff --git a/docs/components/backend/authenticator/openapi.json b/docs/components/backend/authenticator/openapi.json index a0f96933c..69c532dce 100644 --- a/docs/components/backend/authenticator/openapi.json +++ b/docs/components/backend/authenticator/openapi.json @@ -145,7 +145,7 @@ "operationId": "authenticator.callback", "responses": { "302": { - "description": "Redirect to the SPA with the session cookie set" + "description": "Redirect to the SPA: with the session cookie set on success, or with `auth_error=` (state_expired, idp_error, invalid_callback, exchange_failed, access_denied) on a failed login so the SPA can restart the flow" } }, "summary": "Complete login: exchange the code and set the session cookie", diff --git a/src/backend/clippy.toml b/src/backend/clippy.toml index a08ab5c80..87393283a 100644 --- a/src/backend/clippy.toml +++ b/src/backend/clippy.toml @@ -41,7 +41,7 @@ doc-valid-idents = [ # ─── Clippy default list (preserved verbatim) ──────────────────── "..", # ─── Auth & identity protocols ─────────────────────────────────── - "OAuth", "OAuth2", "OIDC", "OpenID", "PKCE", + "IdP", "OAuth", "OAuth2", "OIDC", "OpenID", "PKCE", "JWT", "JWTs", "JWS", "JWE", # ─── Wire protocols & RPC ──────────────────────────────────────── "gRPC", "HTTPS", "TLS", "mTLS", "WebSocket", "WebSockets", diff --git a/src/backend/services/authenticator/src/api/error.rs b/src/backend/services/authenticator/src/api/error.rs index 8f7c858f8..bdf65ad2c 100644 --- a/src/backend/services/authenticator/src/api/error.rs +++ b/src/backend/services/authenticator/src/api/error.rs @@ -5,10 +5,6 @@ use toolkit_canonical_errors::resource_error; -/// Failures resolving / creating the request's internal person. -#[resource_error("gts.cf.insight.authenticator.person.v1~")] -pub struct PersonError; - /// OIDC handshake failures (state/nonce/exchange/id_token validation). #[resource_error("gts.cf.insight.authenticator.oidc.v1~")] pub struct OidcError; diff --git a/src/backend/services/authenticator/src/api/handlers.rs b/src/backend/services/authenticator/src/api/handlers.rs index fecb9017e..caa28239b 100644 --- a/src/backend/services/authenticator/src/api/handlers.rs +++ b/src/backend/services/authenticator/src/api/handlers.rs @@ -25,7 +25,7 @@ use serde::Deserialize; use uuid::Uuid; use crate::api::AppState; -use crate::api::error::{OidcError, PersonError, SessionError}; +use crate::api::error::{OidcError, SessionError}; use crate::audit::AuditEvent; use crate::cookie; use crate::identity::PersonResolution; @@ -140,6 +140,33 @@ pub struct CallbackParams { state: Option, #[serde(default)] error: Option, + /// The IdP's human-readable detail (e.g. Entra's `AADSTS…` codes) — the + /// only place the failure cause survives now that the browser gets a + /// redirect instead of a problem body. + #[serde(default)] + error_description: Option, +} + +/// Bounce a failed callback back into the SPA (#2032). The browser arrives +/// here on an IdP redirect with no page loaded, so problem+json would dead-end +/// the login on raw JSON. Redirect to `default_return_to` with a fixed +/// `auth_error=` instead — the SPA restarts the login (loop-guarded) +/// or shows an error screen. `reason` must be one of the fixed codes; nothing +/// IdP- or caller-supplied may reach the Location header. +fn login_error_redirect(default_return_to: &str, reason: &str) -> Response { + let sep = if default_return_to.contains('?') { + '&' + } else { + '?' + }; + build_response( + StatusCode::FOUND, + vec![( + LOCATION.clone(), + format!("{default_return_to}{sep}auth_error={reason}"), + )], + Body::empty(), + ) } /// Complete login: validate state, exchange the code, guard against session @@ -155,16 +182,19 @@ pub async fn callback( Query(params): Query, ) -> Response { if let Some(err) = params.error { - return OidcError::invalid_argument() - .with_field_violation("error", err, "IDP_ERROR") - .create() - .into_response(); + // Client-reachable log path: strip control characters so the + // IdP-supplied values cannot forge log lines, and cap the lengths. + let sanitize = + |v: &str| -> String { v.chars().filter(|c| !c.is_control()).take(200).collect() }; + tracing::warn!( + error = %sanitize(&err), + error_description = %sanitize(params.error_description.as_deref().unwrap_or("")), + "IdP reported an error at /auth/callback" + ); + return login_error_redirect(&state.cfg.default_return_to, "idp_error"); } let (Some(code), Some(oidc_state)) = (params.code, params.state) else { - return OidcError::invalid_argument() - .with_field_violation("state", "missing code or state", "MISSING") - .create() - .into_response(); + return login_error_redirect(&state.cfg.default_return_to, "invalid_callback"); }; // Layer-2 bucket keyed by the presented `state` @@ -187,10 +217,10 @@ pub async fn callback( let login_state = match state.sessions.take_login_state(&oidc_state).await { Ok(Some(ls)) => ls, Ok(None) => { - return OidcError::invalid_argument() - .with_field_violation("state", "unknown or expired state", "STATE_MISMATCH") - .create() - .into_response(); + // Expired (the 300 s login-state TTL), unknown, or already-consumed + // (replayed callback) state. A fresh login fixes all three, so + // bounce to the SPA instead of dead-ending (#2032). + return login_error_redirect(&state.cfg.default_return_to, "state_expired"); } Err(e) => return internal_problem("login_state_take", &e), }; @@ -212,10 +242,7 @@ pub async fn callback( error = format!("{e:#}"), "oidc code exchange / id_token validation failed" ); - return OidcError::invalid_argument() - .with_field_violation("code", "token exchange failed", "EXCHANGE_FAILED") - .create() - .into_response(); + return login_error_redirect(&state.cfg.default_return_to, "exchange_failed"); } }; @@ -232,10 +259,7 @@ pub async fn callback( email = %idp.identity.email, "login denied: id_token carried no tenant and no default_tenant_id is set" ); - return PersonError::permission_denied() - .with_reason("tenant_unresolved") - .create() - .into_response(); + return login_error_redirect(&state.cfg.default_return_to, "access_denied"); } // Session-fixation guard: never reuse an incoming session; revoke any live @@ -275,10 +299,7 @@ pub async fn callback( "idp_sub": idp.identity.sub, }), }); - return PersonError::permission_denied() - .with_reason("unknown_person") - .create() - .into_response(); + return login_error_redirect(&state.cfg.default_return_to, "access_denied"); } Err(e) => return internal_problem("person_resolution", &e), }; @@ -488,12 +509,12 @@ async fn resolve_override( "override_email": target_email, }), }); - Err(Box::new( - PersonError::permission_denied() - .with_reason("override_unknown_person") - .create() - .into_response(), - )) + // Denied, never a fallback to the caller (PRD 5.16) — but still a + // browser-facing callback failure, so it bounces like the rest. + Err(Box::new(login_error_redirect( + &state.cfg.default_return_to, + "access_denied", + ))) } Err(e) => Err(Box::new(internal_problem("person_resolution", &e))), } @@ -1598,4 +1619,25 @@ mod tests { let token = format!("aGVhZGVy.{payload}.c2ln"); assert_eq!(jwt_exp(&token), Some(4_000_000_000)); } + + fn location_of(resp: &Response) -> String { + resp.headers() + .get(LOCATION) + .and_then(|h| h.to_str().ok()) + .unwrap_or_default() + .to_owned() + } + + #[test] + fn login_error_redirect_bounces_into_the_spa() { + let resp = login_error_redirect("/", "state_expired"); + assert_eq!(resp.status(), StatusCode::FOUND); + assert_eq!(location_of(&resp), "/?auth_error=state_expired"); + } + + #[test] + fn login_error_redirect_appends_to_an_existing_query() { + let resp = login_error_redirect("/app?tab=home", "access_denied"); + assert_eq!(location_of(&resp), "/app?tab=home&auth_error=access_denied"); + } } diff --git a/src/backend/services/authenticator/src/api/mod.rs b/src/backend/services/authenticator/src/api/mod.rs index 1a9c7be78..6b50811f8 100644 --- a/src/backend/services/authenticator/src/api/mod.rs +++ b/src/backend/services/authenticator/src/api/mod.rs @@ -86,7 +86,10 @@ fn register_auth_routes(router: Router, openapi: &dyn OpenApiRegistry) -> Router .public() .no_content_response( StatusCode::FOUND, - "Redirect to the SPA with the session cookie set", + "Redirect to the SPA: with the session cookie set on success, or \ + with `auth_error=` (state_expired, idp_error, \ + invalid_callback, exchange_failed, access_denied) on a failed \ + login so the SPA can restart the flow", ) .handler(handlers::callback) .register(router, openapi); diff --git a/src/backend/services/authenticator/src/config.rs b/src/backend/services/authenticator/src/config.rs index 3693ab17c..a501a7a49 100644 --- a/src/backend/services/authenticator/src/config.rs +++ b/src/backend/services/authenticator/src/config.rs @@ -407,6 +407,19 @@ impl AuthenticatorConfig { anyhow::ensure!(!value.trim().is_empty(), "{name} is required (empty)"); } + // `default_return_to` lands verbatim in Location headers (login + // fallback and every `auth_error` bounce). A non-site-relative value + // would open-redirect on our own config, and a `#` fragment would hide + // `auth_error=` from the SPA's query parsing — defeating its login + // retry loop guard. + anyhow::ensure!( + self.default_return_to.starts_with('/') + && !self.default_return_to.starts_with("//") + && !self.default_return_to.contains('#') + && !self.default_return_to.chars().any(char::is_control), + "default_return_to must be a site-relative path without a fragment" + ); + // Service tokens: if any service is registered, the token endpoint must // know the `aud` it expects on assertions (its own URL). A registry // entry with zero public keys can never authenticate — reject it early. diff --git a/src/backend/services/authenticator/tests/e2e_login_loop.rs b/src/backend/services/authenticator/tests/e2e_login_loop.rs index 3d35cfa62..03c906f78 100644 --- a/src/backend/services/authenticator/tests/e2e_login_loop.rs +++ b/src/backend/services/authenticator/tests/e2e_login_loop.rs @@ -246,3 +246,47 @@ async fn full_login_exchange_logout_loop() { "401 must never be cached" ); } + +/// Failed callbacks bounce back into the SPA (#2032): the browser lands on +/// `/auth/callback` from an IdP redirect, so a problem+json answer would +/// dead-end the login on raw JSON. Every browser-facing failure must 302 to +/// `default_return_to` with a fixed `auth_error=` the SPA consumes to +/// restart the flow. +#[tokio::test] +#[ignore = "requires a running authenticator + fakeidp + Redis stack"] +async fn failed_callback_redirects_into_the_spa_with_auth_error() { + let auth_base = env("AUTH_BASE", "http://localhost:8083"); + let http = client(); + + // Distinct `state` values per case AND per run: the per-state callback + // rate-limit bucket (5-burst, 10/min refill) must not couple these + // requests — or trip on rapid suite re-runs against the same stack. + let run = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis()) + .unwrap_or_default(); + let cases = [ + ( + format!("code=x&state=e2e-auth-error-unknown-{run}"), + "/?auth_error=state_expired", + ), + ( + format!("error=access_denied&state=e2e-auth-error-idp-{run}"), + "/?auth_error=idp_error", + ), + ( + format!("state=e2e-auth-error-no-code-{run}"), + "/?auth_error=invalid_callback", + ), + ]; + for (query, expected_location) in cases { + let resp = http + .get(format!("{auth_base}/auth/callback?{query}")) + .send() + .await + .unwrap(); + assert_eq!(resp.status().as_u16(), 302, "{query} must redirect"); + let location = resp.headers()[reqwest::header::LOCATION].to_str().unwrap(); + assert_eq!(location, expected_location, "for {query}"); + } +} diff --git a/src/backend/services/authenticator/tests/e2e_override.rs b/src/backend/services/authenticator/tests/e2e_override.rs index 4e980601f..1f4ee0245 100644 --- a/src/backend/services/authenticator/tests/e2e_override.rs +++ b/src/backend/services/authenticator/tests/e2e_override.rs @@ -344,13 +344,19 @@ async fn override_with_unknown_target_is_denied() { let user = env("E2E_USER", "dev@company.nonpresent"); let http = client(); - // The identity stub 404s emails prefixed `unknown-` (test seam). + // The identity stub 404s emails prefixed `unknown-` (test seam). The + // denial is an auth_error bounce back into the SPA (#2032), never a + // fallback to the caller's own identity. let cb = login_flow(&http, &auth_base, &user, Some("unknown-nobody@example.com")).await; assert_eq!( cb.status(), - 403, + 302, "an unknown override target must be denied" ); + assert_eq!( + cb.headers()[reqwest::header::LOCATION].to_str().unwrap(), + "/?auth_error=access_denied" + ); assert!(cookie_from(&cb).is_none(), "no session may be minted"); } diff --git a/src/backend/services/authenticator/tests/e2e_ratelimit.rs b/src/backend/services/authenticator/tests/e2e_ratelimit.rs index 33f96a798..5b07c9cfb 100644 --- a/src/backend/services/authenticator/tests/e2e_ratelimit.rs +++ b/src/backend/services/authenticator/tests/e2e_ratelimit.rs @@ -145,8 +145,9 @@ async fn refresh_and_callback_buckets_trip_past_burst() { let (status, _) = refresh(&http, &auth_base, &other, &other_csrf).await; assert_eq!(status, 200, "another session must have its own bucket"); - // 3. Callback bucket: hammering one (bogus) state flips from 400 - // (unknown state) to 429 once the per-state bucket empties. + // 3. Callback bucket: hammering one (bogus) state flips from 302 (the + // unknown-state auth_error redirect, #2032) to 429 once the per-state + // bucket empties. let mut saw_429 = false; for _ in 1..=8 { let resp = http @@ -157,7 +158,7 @@ async fn refresh_and_callback_buckets_trip_past_burst() { .await .unwrap(); match resp.status().as_u16() { - 400 => {} + 302 => {} 429 => { saw_429 = true; break;