fix(authenticator): redirect failed /auth/callback into the SPA with auth_error

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 <jwt>` + `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=<reason>` 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 |

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=<reason>` 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=<email>` 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=<reason>` (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). |

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=<reason>` (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",

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",

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;

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<String>,
     #[serde(default)]
     error: Option<String>,
+    /// 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<String>,
+}
+
+/// 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=<reason>` 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<CallbackParams>,
 ) -> 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");
+    }
 }

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=<reason>` (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);

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.

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=<reason>` 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}");
+    }
+}

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");
 }
 

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;