Skip to content

Commit 43b5dfe

Browse files
committed
fix(authorize): self-heal to /login on createCallback FAILED_PRECONDITION (stale grant after logout, not signin_failed)
1 parent 27c41d8 commit 43b5dfe

2 files changed

Lines changed: 129 additions & 4 deletions

File tree

app/resources/authorize/authorize.service.ts

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,23 @@ const DEAD_SESSION_CODES: ReadonlySet<ProviderError['code']> = new Set([
4646
'PERMISSION_DENIED',
4747
]);
4848

49+
// createCallback codes that mean THIS session can no longer finalize THIS auth request, even
50+
// though getSession reported it alive — a stale OIDC grant after RP-initiated logout (cloud-portal
51+
// revokes the grant but auth-ui's `sessions` cookie outlives it). Re-authenticate instead of
52+
// dead-ending on signin_failed: a fresh login mints a new session that completes the callback
53+
// (confirmed in staging — this fails ONLY for old/stale sessions; a fresh login always works, so
54+
// self-healing here cannot create a redirect loop).
55+
// • FAILED_PRECONDITION — the exact code observed in staging logs for this failure.
56+
// • ALREADY_DONE — mappers.ts normalizeError maps the SAME underlying gRPC FailedPrecondition
57+
// (code 9) to ALREADY_DONE instead of FAILED_PRECONDITION whenever Zitadel's error message
58+
// matches /verified|already/i (e.g. "auth request already handled"). createCallback goes
59+
// through that same generic mapper, so a stale grant can surface as either code depending on
60+
// Zitadel's wording — both must self-heal the same way.
61+
const DEAD_CALLBACK_CODES: ReadonlySet<ProviderError['code']> = new Set([
62+
'FAILED_PRECONDITION',
63+
'ALREADY_DONE',
64+
]);
65+
4966
// Freshness window for honoring a query-handed-back sessionId against a prompt=login request.
5067
// The post-auth finalize redirect is near-immediate (the ceremony hands the just-authenticated
5168
// session id straight back to /authorize), so 2 minutes generously covers redirect latency +
@@ -220,7 +237,9 @@ async function healStaleEntry(
220237
async function runCallback(
221238
provider: AuthProvider,
222239
rawId: string,
223-
entry: SessionEntry
240+
entry: SessionEntry,
241+
list: SessionEntry[],
242+
requestId: string
224243
): Promise<AuthorizeOutcome> {
225244
try {
226245
const { callbackUrl } = await provider.createCallback(rawId, {
@@ -231,11 +250,19 @@ async function runCallback(
231250
logAuthEvent('oidc_callback', 'success', { requestId: rawId, sessionId: entry.id });
232251
return { kind: 'redirect', location: callbackUrl };
233252
} catch (err) {
253+
const code = err instanceof ProviderError ? err.code : undefined;
234254
logAuthEvent('oidc_callback', 'failure', {
235255
requestId: rawId,
236256
sessionId: entry.id,
237-
code: err instanceof ProviderError ? err.code : 'UNKNOWN',
257+
code: code ?? 'UNKNOWN',
238258
});
259+
// A DEAD_CALLBACK_CODES code means the session is a stale post-logout grant, not a genuine
260+
// failure: self-heal to /login (prune + re-auth) instead of dead-ending on signin_failed. Any
261+
// other code (transient/unknown) keeps the conservative existing behavior — surface the error
262+
// page rather than guessing that a re-login will help.
263+
if (code && DEAD_CALLBACK_CODES.has(code)) {
264+
return healStaleEntry(list, entry, requestId, rawId);
265+
}
239266
return { kind: 'error-redirect', code: 'signin_failed' };
240267
}
241268
}
@@ -339,7 +366,7 @@ async function resolveOidc(
339366
const mustReauth =
340367
authRequest.prompt.includes('login') &&
341368
!primaryFresh(gate.session.factors, nowMs, await freshLoginWindowMs(provider, entry));
342-
if (!mustReauth) return runCallback(provider, rawId, entry);
369+
if (!mustReauth) return runCallback(provider, rawId, entry, list, requestId);
343370
// else: stale prompt=login → do NOT finalize; fall through to decideAuthorize below.
344371
}
345372
}
@@ -365,7 +392,7 @@ async function resolveOidc(
365392
// 'callback', so this branch is unreachable under prompt=login (only none/default reuse).
366393
const healed = await healIfSessionDead(provider, list, entry, requestId, rawId);
367394
if ('kind' in healed) return healed;
368-
return runCallback(provider, rawId, entry);
395+
return runCallback(provider, rawId, entry, list, requestId);
369396
}
370397
if (decision.target === 'error') {
371398
if (decision.error === 'NO_ACTIVE_SESSION') {
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// cypress/component/resources/authorize/stale-grant-callback.cy.ts
2+
//
3+
// Regression coverage for the post-logout STALE OIDC GRANT bug: after RP-initiated logout
4+
// (cloud-portal), auth-ui's `sessions` cookie can hold a session whose Zitadel SESSION is still
5+
// alive but whose OIDC GRANT is gone. getSession succeeds (so healIfSessionDead's dead-session
6+
// check never fires) and the flow proceeds to createCallback, which Zitadel rejects with
7+
// FAILED_PRECONDITION (confirmed in staging logs) — or ALREADY_DONE, the sibling code the same
8+
// gRPC FailedPrecondition maps to depending on Zitadel's error message (see mappers.ts). Before
9+
// the fix, ANY createCallback failure dead-ended on `signin_failed`; now DEAD_CALLBACK_CODES
10+
// self-heals (prune + /login) exactly like the already-covered dead-session case in logout.cy.ts,
11+
// while a genuinely transient/unknown code (e.g. UNAVAILABLE) still surfaces the error page.
12+
//
13+
// Node-bound: resolveAuthorize reads a real signed `sessions` cookie off a Request (Fetch spec
14+
// forbids a Cookie header in the browser), so this runs through the cy.task node-spec harness.
15+
import { callService, type AuditEvent } from '../../../support/node/call-service';
16+
17+
const SESSION = { id: 'sess-stale-grant-1', token: 'tok-stale-grant-1' };
18+
const COOKIE = [{ id: SESSION.id, token: SESSION.token, loginName: 'alice@acme.test' }];
19+
20+
function authRequestSeed() {
21+
return { authRequests: { req1: { id: 'req1', clientId: 'client1', scopes: [], prompt: [] } } };
22+
}
23+
24+
describe('resolveOidc — stale OIDC grant after logout (createCallback failure on an ALIVE session)', () => {
25+
it('FAILED_PRECONDITION self-heals to /login (prunes the stale entry) — NOT signin_failed', () => {
26+
callService({
27+
fn: 'resolveAuthorize',
28+
provider: 'fresh',
29+
seed: authRequestSeed(),
30+
liveSessions: [SESSION],
31+
callbackResults: { [SESSION.id]: { mode: 'throw', code: 'FAILED_PRECONDITION' } },
32+
request: {
33+
url: 'http://localhost/id/authorize?authRequest=req1',
34+
sessions: COOKIE,
35+
},
36+
}).then((v) => {
37+
expect(v.response?.status).to.equal(302);
38+
const loc = v.response?.location ?? '';
39+
expect(loc).to.include('/login');
40+
expect(loc).to.include('requestId=oidc_req1');
41+
expect(loc).to.not.include('/error');
42+
expect(loc).to.not.include('client.acme.test/callback');
43+
// Stale entry pruned from the re-signed cookie (same self-heal shape as the dead-session case).
44+
expect(v.response?.cookieEntries?.some((e) => e.id === SESSION.id) ?? false).to.equal(false);
45+
// Traceable self-heal event — proves this took the heal path, not a bare error redirect.
46+
const stale = v.audit.find((e: AuditEvent) => e.event === 'session_stale');
47+
expect(stale !== undefined, 'session_stale event').to.equal(true);
48+
expect(stale?.outcome).to.equal('success');
49+
expect(stale?.sessionId).to.equal(SESSION.id);
50+
// The failed createCallback attempt is still logged (diagnosability), just no longer terminal.
51+
const failure = v.audit.find(
52+
(e: AuditEvent) => e.event === 'oidc_callback' && e.outcome === 'failure'
53+
);
54+
expect(failure !== undefined, 'oidc_callback failure event').to.equal(true);
55+
expect(failure?.code).to.equal('FAILED_PRECONDITION');
56+
});
57+
});
58+
59+
it('ALREADY_DONE (the sibling code for the same underlying gRPC FailedPrecondition) also self-heals to /login', () => {
60+
callService({
61+
fn: 'resolveAuthorize',
62+
provider: 'fresh',
63+
seed: authRequestSeed(),
64+
liveSessions: [SESSION],
65+
callbackResults: { [SESSION.id]: { mode: 'throw', code: 'ALREADY_DONE' } },
66+
request: {
67+
url: 'http://localhost/id/authorize?authRequest=req1',
68+
sessions: COOKIE,
69+
},
70+
}).then((v) => {
71+
expect(v.response?.status).to.equal(302);
72+
const loc = v.response?.location ?? '';
73+
expect(loc).to.include('/login');
74+
expect(loc).to.not.include('/error');
75+
});
76+
});
77+
78+
it('a TRANSIENT createCallback failure (UNAVAILABLE) on an alive session still surfaces signin_failed — no silent re-login', () => {
79+
callService({
80+
fn: 'resolveAuthorize',
81+
provider: 'fresh',
82+
seed: authRequestSeed(),
83+
liveSessions: [SESSION],
84+
callbackResults: { [SESSION.id]: { mode: 'throw', code: 'UNAVAILABLE' } },
85+
request: {
86+
url: 'http://localhost/id/authorize?authRequest=req1',
87+
sessions: COOKIE,
88+
},
89+
}).then((v) => {
90+
expect(v.response?.status).to.equal(302);
91+
const loc = v.response?.location ?? '';
92+
expect(loc).to.include('/error');
93+
expect(loc).to.include('code=signin_failed');
94+
expect(loc).to.not.include('/login');
95+
expect(v.audit.some((e: AuditEvent) => e.event === 'session_stale')).to.equal(false);
96+
});
97+
});
98+
});

0 commit comments

Comments
 (0)