diff --git a/backend/security/src/routes/security.ts b/backend/security/src/routes/security.ts index 228b9852..656588ad 100644 --- a/backend/security/src/routes/security.ts +++ b/backend/security/src/routes/security.ts @@ -120,7 +120,25 @@ function sendError(res: Response, err: unknown): void { return } if (name === 'AuthentikUnavailableError' || name === 'UnsupportedFlowStageError') { - res.status(401).json({ error: 'Authentication unavailable', code: 'PROVIDER_UNAVAILABLE' }) + // 503, NOT 401. The identity provider being unreachable/too slow is a + // SERVICE condition, not a statement about the caller's credentials. + // Returning 401 told every client "those credentials failed" for what is + // actually an outage: the login UI has no way to tell a real rejection + // from a provider stall, so a platform incident renders to users as "your + // password is wrong" — they change a password that was never the problem, + // and the outage stays invisible in auth-failure metrics. + // + // This aligns three things that had drifted apart: the contract's own + // status map already declares PROVIDER_UNAVAILABLE -> 503 + // (tests/security-api/referenceApp.ts), and the legacy /api/auth/* route + // already returns 503 for both of these errors (routes/auth.ts). This + // route — the one the login form actually calls — was the sole outlier. + // + // Retry-After marks it explicitly retryable; both errors are transient + // from the caller's point of view (an unavailable provider recovers, and + // an undriveable flow stage is retryable via the SSO button). + res.setHeader('Retry-After', '5') + res.status(503).json({ error: 'Authentication unavailable', code: 'PROVIDER_UNAVAILABLE' }) return } console.error('[security] unhandled error:', err) diff --git a/backend/security/src/services/authentikPassword.ts b/backend/security/src/services/authentikPassword.ts index 63548a93..f6162265 100644 --- a/backend/security/src/services/authentikPassword.ts +++ b/backend/security/src/services/authentikPassword.ts @@ -39,22 +39,170 @@ import { logger } from '../lib/logger' const AUTHENTIK_FLOW_TIMEOUT_MS = Number(process.env.AUTHENTIK_FLOW_TIMEOUT_MS) || 10000 +/** + * WHOLE-REQUEST budget (ms) for one server-brokered sign-in/sign-up. + * + * The per-hop cap above bounds each INDIVIDUAL fetch, but a login is a CHAIN: + * 2-3 flow-executor stages (each following up to 10 redirects) followed by the + * authorize→code chain (up to 10 more hops). Every hop used to get a fresh 10s + * budget, so the server's worst case ran to minutes while the browser gives the + * whole call only LOGIN_TIMEOUT_MS (15s — frontend/src/services/api.ts). The + * client therefore always aborted first, and the user got a bare + * "timeout of 15000ms exceeded" with no status and no message, while the + * labelled server-side diagnostics ("hop timed out", naming the exact stage) + * were still waiting to be produced and never reached anyone. + * + * Bounding the CHAIN — not just each link — is what makes the server answer + * first. This MUST stay below the client's bound so a stalled sign-in surfaces + * as a real, logged, labelled HTTP response instead of a blind client-side + * abort: 40s here against the client's 45s. The margin is for the response + * trip, so retune the pair together and never let this cross above it. + * + * Sized to the documented slow path (16-30s), not to what sign-in SHOULD cost + * — a budget below the real worst case rejects logins that would have + * succeeded. Tighten both once the underlying slow hop is fixed. + * Overridable via AUTHENTIK_LOGIN_DEADLINE_MS. + */ +const AUTHENTIK_LOGIN_DEADLINE_MS = + Number(process.env.AUTHENTIK_LOGIN_DEADLINE_MS) || 40000 + +/** + * A hop slower than this is reported at WARN, with its label, even when the + * login ultimately succeeds. + * + * Per-hop timings already existed — at `logger.debug`. LOG_LEVEL defaults to + * `info` and is not set anywhere in the chart, so in production that detail has + * always been switched OFF, and answering "which hop is slow?" required either + * a config change or a redeploy. That is why this path has accumulated timeout + * band-aids (#362, #371) instead of a diagnosis: the evidence was never in the + * logs when the incident happened. + * + * A slow-hop threshold is the standard fix (a slow-query log): silent on the + * fast path, fully detailed exactly when something is wrong — no LOG_LEVEL + * change, no redeploy, no spam. Overridable via AUTHENTIK_SLOW_HOP_WARN_MS. + */ +const AUTHENTIK_SLOW_HOP_WARN_MS = + Number(process.env.AUTHENTIK_SLOW_HOP_WARN_MS) || 1000 + +/** + * A whole sign-in slower than this is reported at WARN even though it + * SUCCEEDED. Set above the ~5.5s fast path, well below the client bound — the + * gap between them is precisely the band that was silently eating sign-in. + */ +const AUTHENTIK_LOGIN_WARN_MS = + Number(process.env.AUTHENTIK_LOGIN_WARN_MS) || 8000 + /** Monotonic-ish elapsed helper for the per-step timing logs. */ function since(startMs: number): number { return Math.round(Date.now() - startMs) } +/** + * Record one hop's cost. Always available at debug; promoted to warn when the + * hop is slow enough to be the thing worth looking at. + * + * Leading hypothesis for what this will show, stated so the logs can refute it: + * these hops all target the same in-cluster origin, and this pod's own + * `dnsConfig` (deploy/helm/.../security.yaml) documents CoreDNS "intermittently + * stalls lookups in 5s/10s retry multiples", capped to ~2s by timeout:1 / + * attempts:2 — with the note that this service "resolves authentik-server on + * every auth flow". A DNS lookup happens per NEW CONNECTION, and the leaked + * response bodies (see drainBody) forced a new connection per hop, so the stall + * was multiplied by hop count: ~6 hops x ~2s is most of the observed 16-30s. + * If that is right, draining bodies lets undici reuse one keep-alive socket per + * origin and the stalls collapse to at most one. `elapsedMs` per labelled hop + * is what confirms or kills that; if connect/DNS time still dominates, the next + * step is an explicit keep-alive dispatcher pinned to the Authentik origin. + */ +function recordHop( + label: string, + elapsedMs: number, + fields: Record = {} +): void { + const entry = { label, elapsedMs, ...fields } + if (elapsedMs >= AUTHENTIK_SLOW_HOP_WARN_MS) { + logger.warn(entry, 'authentikPassword: SLOW hop') + return + } + logger.debug(entry, 'authentikPassword: hop') +} + +/** + * A wall-clock budget shared by every hop of ONE login/signup attempt. + * + * Each hop asks for `hopBudget()`, which is the smaller of the per-hop cap and + * whatever is actually left — so the chain can never outlive the whole-request + * deadline no matter how many redirects Authentik asks us to follow. + */ +class Deadline { + private readonly expiresAt: number + private readonly budgetMs: number + + constructor(budgetMs: number = AUTHENTIK_LOGIN_DEADLINE_MS) { + this.budgetMs = budgetMs + this.expiresAt = Date.now() + budgetMs + } + + remainingMs(): number { + return this.expiresAt - Date.now() + } + + /** Per-hop allowance: never more than the cap, never more than what's left. */ + hopBudget(cap: number = AUTHENTIK_FLOW_TIMEOUT_MS): number { + return Math.min(cap, this.remainingMs()) + } + + /** + * Throw a labelled error if the whole-request budget is already spent, so the + * chain stops at a named stage instead of starting a hop it cannot finish. + */ + assertLive(label: string): void { + if (this.remainingMs() > 0) return + logger.error( + { label, budgetMs: this.budgetMs }, + 'authentikPassword: login deadline exceeded' + ) + throw new AuthentikUnavailableError( + `sign-in exceeded its ${this.budgetMs}ms budget before ${label}` + ) + } +} + +/** + * Release the connection behind a response whose body we are going to discard. + * + * undici (Node's fetch) keeps the underlying socket checked out until the body + * is consumed or cancelled. Every redirect hop below reads only `location` and + * moves on, so without this the socket for each hop stayed pinned for the rest + * of the request — and subsequent hops to the SAME origin queued behind the + * leaked ones. That is exactly the shape of the intermittent multi-second + * stalls this module keeps getting timeout band-aids for: not one slow hop, but + * hops waiting on connections their own predecessors never gave back. + */ +function drainBody(res: Response): void { + // `.cancel()` rejects if the body is already disturbed/locked; either way the + // connection is no longer ours to hold, so the outcome is not interesting. + void res.body?.cancel().catch(() => undefined) +} + /** * `fetch` with a hard AbortController deadline. On timeout the AbortError is * normalised to a labelled AuthentikUnavailableError carrying the elapsed time * and the target, so prod logs pinpoint exactly which hop stalled. + * + * Pass a `Deadline` for anything on the login/signup chain so the hop's + * allowance is clamped to the whole-request budget rather than getting a fresh + * full-length one. */ async function fetchWithTimeout( url: string, init: RequestInit, label: string, - timeoutMs: number = AUTHENTIK_FLOW_TIMEOUT_MS + deadline?: Deadline, + cap: number = AUTHENTIK_FLOW_TIMEOUT_MS ): Promise { + if (deadline) deadline.assertLive(label) + const timeoutMs = deadline ? deadline.hopBudget(cap) : cap const controller = new AbortController() const started = Date.now() const timer = setTimeout(() => controller.abort(), timeoutMs) @@ -171,7 +319,8 @@ export async function flowRequest( base: string, slug: string, jar: CookieJar, - body?: Record + body?: Record, + deadline?: Deadline ): Promise { // Authentik commonly answers the first executor request with a 302 that // establishes the session cookie (Location points back into the flow), so @@ -201,7 +350,8 @@ export async function flowRequest( res = await fetchWithTimeout( url, { method, headers, body: payload, redirect: 'manual' }, - `flow.step slug=${slug} hop=${hop} ${method}` + `flow.step slug=${slug} hop=${hop} ${method}`, + deadline ) } catch (err) { if (err instanceof AuthentikUnavailableError) throw err @@ -209,14 +359,20 @@ export async function flowRequest( `Authentik unreachable at ${base}: ${(err as Error).message}` ) } - logger.debug( - { slug, hop, method, status: res.status, elapsedMs: since(stepStart) }, - 'authentikPassword: flow.step' - ) + recordHop(`flow.step slug=${slug} hop=${hop} ${method}`, since(stepStart), { + slug, + hop, + method, + status: res.status, + }) jar.absorb(res) const loc = res.headers.get('location') if ([301, 302, 303, 307, 308].includes(res.status) && loc) { + // Only `location` is wanted from a redirect — hand the socket back before + // issuing the next hop, or it stays checked out for the whole request and + // the following hops queue behind it (see drainBody). + drainBody(res) const nextUrl = new URL(loc, url) // The jar carries authentik_session/authentik_csrf — never present those // cookies to any host other than Authentik itself. @@ -279,10 +435,19 @@ export async function authentikPasswordLogin( logger.info({ email }, 'authentikPassword: login start') try { const user = await authentikPasswordLoginInner(email, password) - logger.info( - { email, elapsedMs: since(loginStart) }, - 'authentikPassword: login succeeded' - ) + const elapsedMs = since(loginStart) + // A login that SUCCEEDS at 25s is the failure mode that broke sign-in: it + // never errors, so nothing alerts, and it only becomes visible once a + // client bound trips underneath it. Report a slow success as loudly as a + // slow hop — the per-hop WARNs above then say which stage owned the time. + if (elapsedMs >= AUTHENTIK_LOGIN_WARN_MS) { + logger.warn( + { email, elapsedMs, thresholdMs: AUTHENTIK_LOGIN_WARN_MS }, + 'authentikPassword: SLOW login (succeeded)' + ) + } else { + logger.info({ email, elapsedMs }, 'authentikPassword: login succeeded') + } return user } catch (err) { logger.error( @@ -319,9 +484,11 @@ async function authentikPasswordLoginInner( const base = authentikBaseUrl() const slug = authFlowSlug() const jar = new CookieJar() + // One budget for the WHOLE chain — flow stages AND the authorize hops below. + const deadline = new Deadline() // ── Drive the authentication flow ───────────────────────────────────────── - let challenge = await flowRequest(base, slug, jar) + let challenge = await flowRequest(base, slug, jar, undefined, deadline) const MAX_STEPS = 6 let authenticated = false @@ -340,9 +507,15 @@ async function authentikPasswordLoginInner( } // Combined identification+password stage if (challenge.password_fields) body.password = password - challenge = await flowRequest(base, slug, jar, body) + challenge = await flowRequest(base, slug, jar, body, deadline) } else if (component === 'ak-stage-password') { - challenge = await flowRequest(base, slug, jar, { component, password }) + challenge = await flowRequest( + base, + slug, + jar, + { component, password }, + deadline + ) } else if (component === 'ak-stage-access-denied') { throw new InvalidCredentialsError() } else { @@ -362,8 +535,9 @@ async function authentikPasswordLoginInner( } } - // Complete OIDC code+PKCE with the now-authenticated Authentik session. - return completeOidcWithSession(base, jar) + // Complete OIDC code+PKCE with the now-authenticated Authentik session, on + // whatever is LEFT of the login budget rather than a fresh one. + return completeOidcWithSession(base, jar, deadline) } /** @@ -400,7 +574,8 @@ function toInternalAuthorizeUrl(externalUrl: string, base: string): string { export async function completeOidcWithSession( base: string, - jar: CookieJar + jar: CookieJar, + deadline?: Deadline ): Promise { const state = generators.state() const { url: authorizeUrl, codeVerifier } = oidcService.generateAuthUrl(state) @@ -422,7 +597,8 @@ export async function completeOidcWithSession( headers: { Cookie: jar.header(), Accept: 'application/json' }, redirect: 'manual', }, - `authorize.hop hop=${hop}` + `authorize.hop hop=${hop}`, + deadline ) } catch (err) { if (err instanceof AuthentikUnavailableError) throw err @@ -430,11 +606,15 @@ export async function completeOidcWithSession( `Authorize request failed: ${(err as Error).message}` ) } - logger.debug( - { hop, status: res.status, elapsedMs: since(hopStart) }, - 'authentikPassword: authorize.hop' - ) + recordHop(`authorize.hop hop=${hop}`, since(hopStart), { + hop, + status: res.status, + }) jar.absorb(res) + // Nothing in this chain ever reads an authorize response BODY — only its + // status, cookies and `location`. Release the socket now so the next hop + // doesn't queue behind it (see drainBody). + drainBody(res) const next = res.headers.get('location') if (!next) { @@ -477,7 +657,66 @@ export async function completeOidcWithSession( 'authentikPassword: authorize chain resolved to code; entering token exchange' ) // Token exchange + user sync — identical to the redirect callback path. - return oidcService.handleCallback(code, returnedState || state, codeVerifier) + // + // This stage is openid-client's, not ours, so it obeys OIDC_HTTP_TIMEOUT_MS + // (15s) PER CALL and makes two (token, then userinfo) — on its own it can + // outlast the whole login budget several times over and put us right back to + // the client aborting first. Hold it to what is left of the budget so the + // server still answers inside the browser's window with a stage-labelled + // error. The underlying HTTP call may run on in the background; the point is + // to stop WAITING on it, not to pretend it was cancelled. + const exchangeStart = Date.now() + const exchange = oidcService.handleCallback( + code, + returnedState || state, + codeVerifier + ) + const bounded = deadline + ? withDeadline(exchange, deadline, 'oidc.tokenExchange') + : exchange + // Timed like any other hop: this stage is two openid-client round-trips + // (token, then userinfo) and is just as capable of being THE slow one, so it + // must not be the one stage missing from the timing breakdown. + return bounded.finally(() => + recordHop('oidc.tokenExchange', since(exchangeStart)) + ) +} + +/** + * Resolve with `work`, or reject with a labelled AuthentikUnavailableError once + * the shared budget is spent — whichever happens first. + */ +function withDeadline( + work: Promise, + deadline: Deadline, + label: string +): Promise { + const remaining = deadline.remainingMs() + if (remaining <= 0) { + // Do not leave the already-started work as an unhandled rejection. + void work.catch(() => undefined) + deadline.assertLive(label) + } + let timer: NodeJS.Timeout + const expiry = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + logger.error( + { label, remainingMs: remaining }, + 'authentikPassword: stage exceeded the remaining login budget' + ) + reject( + new AuthentikUnavailableError( + `${label} exceeded the remaining ${remaining}ms of the sign-in budget` + ) + ) + }, remaining) + if (typeof timer.unref === 'function') timer.unref() + }) + // When the timer wins the race, `work` is still in flight; a later rejection + // from it would otherwise surface as an unhandled rejection and (under + // Node's default) take the process down. + void work.catch(() => undefined) + return Promise.race([work, expiry]).finally(() => clearTimeout(timer)) } /** Thrown when an account already exists for the email (signup conflict). */ @@ -551,12 +790,15 @@ async function authentikSignupInner(input: AuthentikSignupInput): Promise const base = authentikBaseUrl() const slug = enrollmentFlowSlug() const jar = new CookieJar() + // Signup drives the same multi-hop chain as login and is bounded by the same + // client-side LOGIN_TIMEOUT_MS, so it gets the same whole-request budget. + const deadline = new Deadline() // Derive a username from the local-part when the caller did not supply one. const username = input.username || input.email.split('@')[0].replace(/[^a-zA-Z0-9_.-]/g, '') || input.email - let challenge = await flowRequest(base, slug, jar) + let challenge = await flowRequest(base, slug, jar, undefined, deadline) const MAX_STEPS = 8 let enrolled = false @@ -582,11 +824,11 @@ async function authentikSignupInner(input: AuthentikSignupInput): Promise if (input.firstName || input.lastName) { body.name = [input.firstName, input.lastName].filter(Boolean).join(' ') } - challenge = await flowRequest(base, slug, jar, body) + challenge = await flowRequest(base, slug, jar, body, deadline) } else if (component === 'ak-stage-user-login' || component === 'ak-stage-user-write') { // Non-interactive stages that occasionally surface a challenge — re-POST // the bare component to advance. - challenge = await flowRequest(base, slug, jar, { component }) + challenge = await flowRequest(base, slug, jar, { component }, deadline) } else if (component === 'ak-stage-access-denied') { throw new EnrollmentConflictError() } else { @@ -616,8 +858,9 @@ async function authentikSignupInner(input: AuthentikSignupInput): Promise throw new UnsupportedFlowStageError(last) } - // Enrollment auto-logged-in → complete OIDC + sync via the shared path. - return completeOidcWithSession(base, jar) + // Enrollment auto-logged-in → complete OIDC + sync via the shared path, on + // whatever is LEFT of the signup budget rather than a fresh one. + return completeOidcWithSession(base, jar, deadline) } /** Thrown when the identity store has no account for the address. */ diff --git a/backend/security/tests/authentik-password-login.test.ts b/backend/security/tests/authentik-password-login.test.ts index 8c14f167..d5978d32 100644 --- a/backend/security/tests/authentik-password-login.test.ts +++ b/backend/security/tests/authentik-password-login.test.ts @@ -307,4 +307,208 @@ describe('authentikPasswordLogin()', () => { authentikPasswordLogin('e2e@test.local', 'pw') ).rejects.toBeInstanceOf(UnsupportedFlowStageError) }) + + // ── Whole-request budget ────────────────────────────────────────────────── + // + // The per-hop timeout bounds each individual fetch, but a login is a CHAIN. + // Every hop used to get a FRESH full-length budget, so the server's worst + // case ran far past the browser's own LOGIN_TIMEOUT_MS — the client aborted + // first and the user got a bare "timeout of 15000ms exceeded" with no status + // and no message, while the labelled server-side diagnostics never got to + // exist. These pin the chain-level bound that makes the server answer first. + + it('abandons the chain with a labelled error once the whole-request budget is spent', async () => { + process.env.AUTHENTIK_LOGIN_DEADLINE_MS = '150' + jest.resetModules() + const { authentikPasswordLogin: login, AuthentikUnavailableError: Unavailable } = + require('../src/services/authentikPassword') + + // Each hop is individually well under the per-hop cap; it is their SUM + // that blows the budget. Authentik keeps redirecting inside the flow. + fetchMock.mockImplementation( + async () => + new Promise(resolve => + setTimeout( + () => + resolve( + mkRes({ + status: 302, + location: + 'http://auth.example.test/api/v3/flows/executor/default-authentication-flow/?query=', + }) + ), + 60 + ) + ) + ) + + const err = await login('e2e@test.local', 'pw').catch((e: Error) => e) + + expect(err).toBeInstanceOf(Unavailable) + // The message names the budget and the stage, so prod logs point at the + // stall instead of the client reporting an anonymous abort. + expect((err as Error).message).toMatch(/150ms budget before flow\.step/) + delete process.env.AUTHENTIK_LOGIN_DEADLINE_MS + }) + + it('clamps a hop to the time remaining rather than giving it a fresh full timeout', async () => { + process.env.AUTHENTIK_LOGIN_DEADLINE_MS = '400' + process.env.AUTHENTIK_FLOW_TIMEOUT_MS = '10000' + jest.resetModules() + const { authentikPasswordLogin: login } = require('../src/services/authentikPassword') + + // First hop burns most of the budget, then a hop hangs forever. Without + // clamping, the hang would get the full 10s per-hop cap and outlive the + // browser; with it, the abort fires within what is LEFT of the 400ms. + fetchMock + .mockImplementationOnce( + async () => + new Promise(resolve => + setTimeout( + () => resolve(mkRes({ json: { component: 'ak-stage-identification' } })), + 250 + ) + ) + ) + .mockImplementationOnce( + (_url: string, init: RequestInit) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => { + const e = new Error('aborted') + e.name = 'AbortError' + reject(e) + }) + }) + ) + + const started = Date.now() + await expect(login('e2e@test.local', 'pw')).rejects.toThrow(/timed out/) + // Comfortably below the 10s per-hop cap — proof the clamp, not the cap, won. + expect(Date.now() - started).toBeLessThan(3000) + + delete process.env.AUTHENTIK_LOGIN_DEADLINE_MS + delete process.env.AUTHENTIK_FLOW_TIMEOUT_MS + }) + + it('bounds the token exchange by the remaining budget, not openid-client’s own timeout', async () => { + // handleCallback is openid-client's stage: OIDC_HTTP_TIMEOUT_MS (15s) per + // call, twice (token + userinfo). Left alone it outlasts the whole login + // budget on its own and the browser aborts first again. + process.env.AUTHENTIK_LOGIN_DEADLINE_MS = '300' + jest.resetModules() + const { authentikPasswordLogin: login } = require('../src/services/authentikPassword') + const { oidcService: oidc } = require('../src/services/oidc') + + oidc.handleCallback.mockReturnValueOnce(new Promise(() => {})) // never settles + + fetchMock + .mockResolvedValueOnce( + mkRes({ json: { component: 'ak-stage-identification' } }) + ) + .mockResolvedValueOnce(mkRes({ json: { component: 'ak-stage-password' } })) + .mockResolvedValueOnce( + mkRes({ json: { component: 'xak-flow-redirect', to: '/' } }) + ) + .mockResolvedValueOnce( + mkRes({ status: 302, location: `${REDIRECT_URI}?code=c8&state=st` }) + ) + + const started = Date.now() + await expect(login('e2e@test.local', 'pw')).rejects.toThrow( + /oidc\.tokenExchange exceeded the remaining/ + ) + // Well inside the browser's 15s bound instead of openid-client's 2x15s. + expect(Date.now() - started).toBeLessThan(3000) + + delete process.env.AUTHENTIK_LOGIN_DEADLINE_MS + }) + + it('reports a slow hop at WARN, naming the stage, on a login that still succeeds', async () => { + // The per-hop timings existed only at logger.debug, and LOG_LEVEL defaults + // to info in prod — so the one piece of evidence needed to find the slow + // hop was switched off exactly when it mattered. A slow SUCCESS must be + // visible without a LOG_LEVEL change or a redeploy. + process.env.AUTHENTIK_SLOW_HOP_WARN_MS = '50' + jest.resetModules() + const { authentikPasswordLogin: login } = require('../src/services/authentikPassword') + const { logger } = require('../src/lib/logger') + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => undefined) + + const slow = (res: Response) => + new Promise(resolve => setTimeout(() => resolve(res), 80)) + + fetchMock + .mockImplementationOnce(() => + slow(mkRes({ json: { component: 'ak-stage-identification' } })) + ) + .mockResolvedValueOnce(mkRes({ json: { component: 'ak-stage-password' } })) + .mockResolvedValueOnce( + mkRes({ json: { component: 'xak-flow-redirect', to: '/' } }) + ) + .mockResolvedValueOnce( + mkRes({ status: 302, location: `${REDIRECT_URI}?code=c7&state=st` }) + ) + + // The login SUCCEEDS — the warning is the whole point, not an error path. + await expect(login('e2e@test.local', 'pw')).resolves.toBeDefined() + + const slowHops = warn.mock.calls.filter( + ([, msg]) => msg === 'authentikPassword: SLOW hop' + ) + expect(slowHops).toHaveLength(1) + // Names the exact stage, so prod logs point at the culprit directly. + expect((slowHops[0][0] as any).label).toMatch(/flow\.step .*hop=0 GET/) + expect((slowHops[0][0] as any).elapsedMs).toBeGreaterThanOrEqual(50) + + warn.mockRestore() + delete process.env.AUTHENTIK_SLOW_HOP_WARN_MS + }) + + it('releases the response body of every hop it only reads headers from', async () => { + // undici keeps the socket checked out until the body is consumed or + // cancelled. Redirect hops read only `location`, and authorize hops never + // read a body at all — so an un-cancelled body pins that connection for the + // rest of the request and later hops queue behind their own predecessors. + const cancels: string[] = [] + const withBody = (label: string, res: Response) => + Object.defineProperty(res, 'body', { + value: { + cancel: async () => { + cancels.push(label) + }, + }, + configurable: true, + }) + + fetchMock + // flow hop that only yields a Location + .mockResolvedValueOnce( + withBody( + 'flow-redirect', + mkRes({ + status: 302, + location: + 'http://auth.example.test/api/v3/flows/executor/default-authentication-flow/?query=', + }) + ) + ) + .mockResolvedValueOnce( + mkRes({ json: { component: 'ak-stage-identification' } }) + ) + .mockResolvedValueOnce(mkRes({ json: { component: 'ak-stage-password' } })) + .mockResolvedValueOnce( + mkRes({ json: { component: 'xak-flow-redirect', to: '/' } }) + ) + // authorize hop — body never read on any path + .mockResolvedValueOnce( + withBody( + 'authorize', + mkRes({ status: 302, location: `${REDIRECT_URI}?code=c9&state=st` }) + ) + ) + + await authentikPasswordLogin('e2e@test.local', 'pw123') + + expect(cancels).toEqual(['flow-redirect', 'authorize']) + }) }) diff --git a/backend/security/tests/security-routes.test.ts b/backend/security/tests/security-routes.test.ts index f2a47312..5be5c43f 100644 --- a/backend/security/tests/security-routes.test.ts +++ b/backend/security/tests/security-routes.test.ts @@ -91,6 +91,29 @@ describe('POST /session (password login)', () => { expect(res.status).toBe(401) expect(res.body.code).toBeDefined() }) + + // A provider outage is a SERVICE condition, not a credential verdict. These + // used to return 401, which made an incident indistinguishable from a typo: + // the login UI could only hedge ("wrong password, OR the service is down"), + // and the outage hid inside auth-failure metrics. + for (const name of ['AuthentikUnavailableError', 'UnsupportedFlowStageError']) { + it(`maps ${name} to 503 PROVIDER_UNAVAILABLE, not 401`, async () => { + const err = new Error('provider is having a bad day') + err.name = name + const p = fakeProvider({ passwordLogin: jest.fn().mockRejectedValue(err) }) + const res = await request(makeApp(p)) + .post('/api/v1/security/session') + .send({ email: 'x', password: 'y' }) + + expect(res.status).toBe(503) + expect(res.body.code).toBe('PROVIDER_UNAVAILABLE') + // Marked retryable so clients (and probes) treat it as transient. + expect(res.headers['retry-after']).toBeDefined() + // The provider's raw message can name internal hosts/flow slugs — it + // must not be echoed to an unauthenticated caller. + expect(JSON.stringify(res.body)).not.toContain('bad day') + }) + } }) describe('GET /session (me) — bearer enforcement', () => { @@ -134,7 +157,13 @@ describe('social login boundary', () => { expect(res.status).toBe(302) expect(res.headers.location).toBe('/api/auth/idp/application/o/authorize/?x=1') expect(res.headers.location).not.toMatch(/auth\.fuzefront\.com/) - expect(res.headers['set-cookie'].join(';')).toMatch(/sec_social_state=/) + // `set-cookie` is typed `string | string[]` (supertest gives an array when + // several are set, a bare string for one). Normalise rather than assuming + // the array shape — the unguarded `.join` failed to COMPILE, which took the + // whole suite down with it, so nothing in this file has been running. + expect([res.headers['set-cookie']].flat().join(';')).toMatch( + /sec_social_state=/ + ) }) it('callback 302s back to the app with a FuzeFront opaque ?code= (no token in URL)', async () => { const res = await request(makeApp(fakeProvider())).get('/api/v1/security/social/callback?code=prov&state=st') diff --git a/frontend/src/__tests__/LoginPage.submit-resilience.test.tsx b/frontend/src/__tests__/LoginPage.submit-resilience.test.tsx index 51e72bd9..0aa06be9 100644 --- a/frontend/src/__tests__/LoginPage.submit-resilience.test.tsx +++ b/frontend/src/__tests__/LoginPage.submit-resilience.test.tsx @@ -124,7 +124,13 @@ describe('LoginPage — credentials submit resilience (fail-fast + clear errors) expect(screen.queryByText(/^signing in…$/i)).not.toBeInTheDocument() }) - it('shows an ambiguous-401 message (not a flat "wrong password" accusation) and un-sticks the button', async () => { + // The Security API used to answer 401 for BOTH a rejected credential and a + // provider outage, so this message had to hedge across the two ("wrong + // password, OR the service is down") — telling users during an incident that + // their password might be at fault. The API now separates them (503 + // PROVIDER_UNAVAILABLE vs 401), so each case says one true thing. + + it('shows a plain rejected-credentials message on 401 and un-sticks the button', async () => { const unauthorizedErr: any = new Error('Request failed with status code 401') unauthorizedErr.response = { status: 401, data: { error: 'Unauthorized' } } vi.mocked(authAPI.login).mockRejectedValue(unauthorizedErr) @@ -134,9 +140,39 @@ describe('LoginPage — credentials submit resilience (fail-fast + clear errors) await waitFor(() => { expect( - screen.getByText(/incorrect email or password, or the sign-in service is temporarily unavailable/i) + screen.getByText(/incorrect email or password/i) + ).toBeInTheDocument() + }) + // No longer hedged against an outage — 401 now means only one thing. + expect( + screen.queryByText(/temporarily unavailable/i) + ).not.toBeInTheDocument() + + const signIn = screen.getByRole('button', { name: /^sign in$/i }) + expect(signIn).not.toBeDisabled() + expect(screen.queryByText(/^signing in…$/i)).not.toBeInTheDocument() + }) + + it('never blames the user’s credentials on a 503 provider outage', async () => { + const outageErr: any = new Error('Request failed with status code 503') + outageErr.response = { + status: 503, + data: { error: 'Authentication unavailable', code: 'PROVIDER_UNAVAILABLE' }, + } + vi.mocked(authAPI.login).mockRejectedValue(outageErr) + + render() + await fillAndSubmit() + + await waitFor(() => { + expect( + screen.getByText(/sign-in service is temporarily unavailable/i) ).toBeInTheDocument() }) + // The whole point: an incident must not read as a credentials problem. + expect( + screen.queryByText(/incorrect email or password/i) + ).not.toBeInTheDocument() const signIn = screen.getByRole('button', { name: /^sign in$/i }) expect(signIn).not.toBeDisabled() diff --git a/frontend/src/pages/LoginPage.tsx b/frontend/src/pages/LoginPage.tsx index 830a5025..7151c91c 100644 --- a/frontend/src/pages/LoginPage.tsx +++ b/frontend/src/pages/LoginPage.tsx @@ -224,12 +224,24 @@ function LoginPage() { // NOT "you typed the wrong password"; word it as a service condition. errorMessage = 'Sign-in is taking longer than expected — the service may be busy. Please try again.' - } else if (status === 401) { - // A 401 here is genuinely ambiguous: real bad credentials OR a slow - // auth hop that got cut short server-side. Don't wrongly accuse the - // user of a typo when it may be a transient service blip. + } else if (status === 503) { + // The Security API now distinguishes a provider outage from a rejected + // credential (503 PROVIDER_UNAVAILABLE vs 401). It previously returned + // 401 for BOTH, which forced the hedged wording below and told users + // their password might be wrong during what was actually an incident. + // With an unambiguous signal, say the true thing and nothing else — do + // not mention credentials at all. errorMessage = - 'Incorrect email or password, or the sign-in service is temporarily unavailable. Please try again.' + err.response?.data?.code === 'PROVIDER_UNAVAILABLE' + ? 'The sign-in service is temporarily unavailable. Your details are fine — please try again in a moment.' + : err.response?.data?.error || + 'The sign-in service is temporarily unavailable. Please try again in a moment.' + } else if (status === 401) { + // Now genuinely means "these credentials were rejected" — the provider + // outage case moved to 503 above. Still worded without certainty about + // WHICH field is wrong, which is deliberate: naming the field tells an + // attacker whether the email exists. + errorMessage = 'Incorrect email or password. Please try again.' } else if (isNetworkError) { errorMessage = (err.message || 'Authentication failed') + diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index cd52e81a..0be50c40 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -65,14 +65,25 @@ const api = axios.create({ timeout: 30000, // 30 second timeout }) -// Dedicated, SHORTER timeout for the login/signup submit path. Prod reality: -// the auth chain normally answers in ~5.5s, but an intermittent slow hop can -// stretch it to 16-30s before the shared 30s client timeout ever trips — that -// entire window the submit button sat on "Signing in…" with zero feedback. -// Bounding just this call lets a slow attempt fail fast with a clear message -// instead of hanging to the full 30s. Env-overridable for tuning without a -// redeploy of the timeout value itself. -const LOGIN_TIMEOUT_MS = Number(import.meta.env.VITE_LOGIN_TIMEOUT_MS) || 15000 +// Dedicated timeout for the login/signup submit path. Prod reality: the auth +// chain normally answers in ~5.5s, but an intermittent slow hop stretches it +// well past that. +// +// This was 15s, chosen to fail FAST on a slow attempt rather than leave the +// submit button on "Signing in…". That traded the wrong way: the documented +// slow path runs 16-30s, so a 15s bound did not fail slow sign-ins fast — it +// failed sign-ins that would otherwise have SUCCEEDED, and sign-in broke +// outright. A bound below the known worst case is not a safety net, it is an +// outage. +// +// 45s covers the documented 16-30s worst case with real margin. The wait is +// not pleasant, but it completes; LoginPage shows a "still working…" hint at +// 8s so a slow-but-succeeding attempt never looks frozen. The server's own +// budget (AUTHENTIK_LOGIN_DEADLINE_MS, 40s) sits UNDER this so the server +// still answers first with a labelled error instead of being raced — keep +// that ordering if either value is retuned. Env-overridable so the pair can +// be tightened again, without a rebuild, once the slow hop is actually fixed. +const LOGIN_TIMEOUT_MS = Number(import.meta.env.VITE_LOGIN_TIMEOUT_MS) || 45000 // Add request timing and enhanced logging api.interceptors.request.use( diff --git a/packages/security/openapi.yaml b/packages/security/openapi.yaml index eaffd80a..e454ebdc 100644 --- a/packages/security/openapi.yaml +++ b/packages/security/openapi.yaml @@ -131,6 +131,8 @@ paths: $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' + '503': + $ref: '#/components/responses/ServiceUnavailable' x-pagination: exempt x-pagination-reason: Singleton session-creation action; returns one session. get: @@ -610,6 +612,8 @@ paths: application/json: schema: $ref: '#/components/schemas/ErrorBody' + '503': + $ref: '#/components/responses/ServiceUnavailable' x-pagination: exempt x-pagination-reason: Singleton account-creation action. # ─────────────────────────── AuthN: capabilities ────────────────────────── @@ -1499,6 +1503,22 @@ components: application/json: schema: $ref: '#/components/schemas/ErrorBody' + ServiceUnavailable: + description: >- + The identity provider is unreachable, too slow, or presented a flow this + API cannot drive server-side (`code: PROVIDER_UNAVAILABLE`). This is a + SERVICE condition and says nothing about the caller's credentials — + clients MUST NOT surface it as an authentication failure. Retryable; + `Retry-After` indicates how long to wait. + headers: + Retry-After: + description: Seconds to wait before retrying. + schema: + type: integer + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorBody' schemas: # ── AuthN request/response shapes (kept compatible with today's @fuzefront/auth) ── LoginRequest: