fix(authorize): don't self-heal on ALREADY_DONE — it's a double-callback, not a stale grant

The createCallback self-heal (43b5dfe) put both FAILED_PRECONDITION and
ALREADY_DONE in DEAD_CALLBACK_CODES, pruning the session and bouncing to /login
on either. But mappers.ts maps a code-9 FailedPrecondition whose message matches
/verified|already/i ("auth request already handled") to ALREADY_DONE — the
double-callback case (same requestId re-submitted after it was already finalized:
browser back+reload, duplicate tab, request race), where the session is still
valid. Self-healing there destroys a good session and re-threads the
already-consumed requestId back to /authorize, risking a self-heal loop that
keeps destroying each freshly-minted session.

Only FAILED_PRECONDITION was the staging-confirmed stale-grant code, so narrow
DEAD_CALLBACK_CODES to it. ALREADY_DONE now falls through to the conservative
signin_failed error page with the session left intact — the strictly-safe
pre-fix behavior for that case. The confirmed logout -> re-login self-heal
(FAILED_PRECONDITION) is unchanged.

Flip the stale-grant-callback ALREADY_DONE test to assert the corrected behavior
(no prune, signin_failed, no session_stale event).

Audit finding H-2 (adversarially verified).

Author: yahyafakhroji

app/resources/authorize/authorize.service.ts

@@ -109,18 +109,19 @@ async function probeSession(provider: AuthProvider, entry: SessionEntry): Promis
 // though getSession reported it alive — a stale OIDC grant after RP-initiated logout (cloud-portal
 // revokes the grant but auth-ui's `sessions` cookie outlives it). Re-authenticate instead of
 // dead-ending on signin_failed: a fresh login mints a new session that completes the callback
-// (confirmed in staging — this fails ONLY for old/stale sessions; a fresh login always works, so
-// self-healing here cannot create a redirect loop).
-//   • FAILED_PRECONDITION — the exact code observed in staging logs for this failure.
-//   • ALREADY_DONE — mappers.ts normalizeError maps the SAME underlying gRPC FailedPrecondition
-//     (code 9) to ALREADY_DONE instead of FAILED_PRECONDITION whenever Zitadel's error message
-//     matches /verified|already/i (e.g. "auth request already handled"). createCallback goes
-//     through that same generic mapper, so a stale grant can surface as either code depending on
-//     Zitadel's wording — both must self-heal the same way.
-const DEAD_CALLBACK_CODES: ReadonlySet<ProviderError['code']> = new Set([
-  'FAILED_PRECONDITION',
-  'ALREADY_DONE',
-]);
+// (confirmed in staging — this fails ONLY for old/stale sessions; a fresh login always works).
+//
+// SCOPED TO FAILED_PRECONDITION ONLY — the exact code observed in staging logs for the stale-grant
+// failure. We deliberately do NOT self-heal on ALREADY_DONE: mappers.ts maps a code-9
+// FailedPrecondition whose message matches /verified|already/i ("auth request already handled") to
+// ALREADY_DONE, which is the DOUBLE-CALLBACK case — the SAME requestId re-submitted after it was
+// already finalized (browser back+reload, duplicate tab, request race). There the session is still
+// perfectly valid; pruning it and redirecting to /login (which re-threads the already-consumed
+// requestId straight back to /authorize) would destroy a good session and risk a self-heal loop
+// that keeps destroying each freshly-minted session. ALREADY_DONE therefore falls through to the
+// conservative signin_failed path below with the session left intact — the pre-fix behavior for
+// this case, which was strictly safe.
+const DEAD_CALLBACK_CODES: ReadonlySet<ProviderError['code']> = new Set(['FAILED_PRECONDITION']);
 
 // Freshness window for honoring a query-handed-back sessionId against a prompt=login request.
 // The post-auth finalize redirect is near-immediate (the ceremony hands the just-authenticated

cypress/component/resources/authorize/stale-grant-callback.cy.ts

@@ -4,11 +4,17 @@
 // (cloud-portal), auth-ui's `sessions` cookie can hold a session whose Zitadel SESSION is still
 // alive but whose OIDC GRANT is gone. getSession succeeds (so healIfSessionDead's dead-session
 // check never fires) and the flow proceeds to createCallback, which Zitadel rejects with
-// FAILED_PRECONDITION (confirmed in staging logs) — or ALREADY_DONE, the sibling code the same
-// gRPC FailedPrecondition maps to depending on Zitadel's error message (see mappers.ts). Before
-// the fix, ANY createCallback failure dead-ended on `signin_failed`; now DEAD_CALLBACK_CODES
-// self-heals (prune + /login) exactly like the already-covered dead-session case in logout.cy.ts,
-// while a genuinely transient/unknown code (e.g. UNAVAILABLE) still surfaces the error page.
+// FAILED_PRECONDITION (confirmed in staging logs). Before the fix, ANY createCallback failure
+// dead-ended on `signin_failed`; now FAILED_PRECONDITION self-heals (prune + /login) exactly like
+// the already-covered dead-session case in logout.cy.ts.
+//
+// IMPORTANT — ALREADY_DONE is NOT self-healed. mappers.ts maps a code-9 FailedPrecondition whose
+// message matches /verified|already/i ("auth request already handled") to ALREADY_DONE, which is
+// the DOUBLE-CALLBACK case: the SAME requestId re-submitted after it was already finalized (browser
+// back+reload, duplicate tab), where the session is still valid. Self-healing there would prune a
+// good session and re-thread the already-consumed requestId, risking a redirect loop — so
+// ALREADY_DONE, like any transient/unknown code (e.g. UNAVAILABLE), surfaces the conservative
+// signin_failed error page with the session left intact.
 //
 // Node-bound: resolveAuthorize reads a real signed `sessions` cookie off a Request (Fetch spec
 // forbids a Cookie header in the browser), so this runs through the cy.task node-spec harness.
@@ -56,7 +62,7 @@ describe('resolveOidc — stale OIDC grant after logout (createCallback failure
     });
   });
 
-  it('ALREADY_DONE (the sibling code for the same underlying gRPC FailedPrecondition) also self-heals to /login', () => {
+  it('ALREADY_DONE (double-callback: the same auth request re-submitted after it was already finalized) does NOT self-heal — surfaces signin_failed with the still-valid session left intact', () => {
     callService({
       fn: 'resolveAuthorize',
       provider: 'fresh',
@@ -70,8 +76,18 @@ describe('resolveOidc — stale OIDC grant after logout (createCallback failure
     }).then((v) => {
       expect(v.response?.status).to.equal(302);
       const loc = v.response?.location ?? '';
-      expect(loc).to.include('/login');
-      expect(loc).to.not.include('/error');
+      // Conservative error page, NOT a self-heal to /login: a double-callback must not destroy the
+      // still-valid session or re-thread the already-consumed requestId (which risks a heal loop).
+      expect(loc).to.include('/error');
+      expect(loc).to.include('code=signin_failed');
+      expect(loc).to.not.include('/login');
+      // No self-heal event and no cookie prune — the valid session is preserved (no Set-Cookie rewrite).
+      expect(v.audit.some((e: AuditEvent) => e.event === 'session_stale')).to.equal(false);
+      // The failed createCallback is still logged for diagnosability, just no longer terminal-to-heal.
+      const failure = v.audit.find(
+        (e: AuditEvent) => e.event === 'oidc_callback' && e.outcome === 'failure'
+      );
+      expect(failure?.code).to.equal('ALREADY_DONE');
     });
   });