From 051506b5bb603f8122bd41185ed7994d74b45aac Mon Sep 17 00:00:00 2001 From: Cameron Waldron <1000178+CamW@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:03:46 +0200 Subject: [PATCH] fix(login): prevent crash on a stale session cookie (#12423) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Which Problems Are Solved The `sessions` cookie lifetime is independent of Zitadel's server-side session validity, so a cookie can reference an invalid session. In that case `getSession` throws `not_found` (QUERY-SFeaa). - `loadMostRecentSession` lets that error propagate instead of treating it as "no session", crashing the server render of every page that calls it without its own guard. - Most visibly, the "forgot password" flow landing on `/password/set` throws `[not_found] Session does not exist (QUERY-SFeaa)` (gRPC code 5, HTTP 404) and fails to render. - 12 of the 16 call sites are unguarded and share this crash. # How the Problems Are Solved Catch the error in `loadMostRecentSession` and return `undefined`, unifying "cookie references a dropped session" with the existing "no cookie" case. Every call site already handles an undefined session, either with a graceful fallback or by throwing its own explicit error. ```ts return getSession({ serviceConfig, sessionId: recent.id, sessionToken: recent.token }) .then((resp: GetSessionResponse) => resp.session) .catch((error) => { console.warn("[Session] Could not load most recent session", error); return undefined; }); ``` All `loadMostRecentSession` call sites — **Previous** = behaviour when the cookie referenced an invalid session; **Change** = what this PR does to that call site: | # | Call site | Change in this PR | Previous behaviour (invalid session) | New behaviour | |---|-----------|-------------------|--------------------------------------|---------------| | — | `lib/session.ts` — `loadMostRecentSession` | **Added `.catch` → `undefined`** (core fix) | Threw `not_found` to caller | Resolves to `undefined` | | 1 | `password/set/page.tsx` | none | **Crash** (the reported bug) | Falls back to login-name flow | | 2 | `password/change/page.tsx` | none | **Crash** | `undefined` → `failedLoading` alert if no user | | 3 | `passkey/set/page.tsx` | none | **Crash** | `undefined` → `userId` fallback / `unknownContext` | | 4 | `otp/[method]/page.tsx` | none | **Crash** | `undefined` → `unknownContext` alert | | 5 | `otp/[method]/set/page.tsx` | none | **Crash** (raw `ConnectError`) | `undefined` → own `throw "No session found"` | | 6 | `mfa/page.tsx` | none | **Crash** | `undefined` → `verify.noResults` alert | | 7 | `mfa/set/page.tsx` | none | **Crash** (raw `ConnectError`) | `undefined` → own `throw "Could not get user id from session"` | | 8 | `authenticator/set/page.tsx` | none | **Crash** (raw `ConnectError`) | `undefined` → own throw / `unknownContext` | | 9 | `signedin/page.tsx` | none | **Crash** | `undefined` → renders with `loginName` fallback | | 10 | `u2f/set/page.tsx` | none | **Crash** | `undefined` → `unknownContext` alert | | 11 | `u2f/page.tsx` | none | **Crash** | `undefined` → renders on `loginName` | | 12 | `lib/server/verify.ts` — `verifyTOTP` | none | **Crash** (raw `ConnectError`) | `undefined` → own `throw "No user id found in session."` | | 13 | `password/page.tsx` | **Removed redundant `try/catch`** | Caught locally → `undefined` | `undefined` — unchanged | | 14 | `verify/page.tsx` | **Removed redundant `.catch`** ¹ | Caught locally → `undefined` | `undefined` — unchanged | | 15 | `verify/success/page.tsx` | **Removed redundant `.catch`** | Caught locally → `undefined` | `undefined` — unchanged | | 16 | `passkey/page.tsx` | **Removed redundant `.catch`** (login-name path) | Caught locally → `undefined` | `undefined` — unchanged | Rows 1–12 were unguarded and crashed; the central change fixes them. Rows 13–16 already handled the error locally. # Additional Changes - This makes the four callers that wrapped the call in their own try/catch or `.catch` redundant, so remove that now-dead handling (password, verify, verify/success, passkey). - Add unit tests covering `loadMostRecentSession`: no cookie, success, and the stale-cookie rejection path. ¹ `verify/page.tsx`'s `.catch` also eagerly fetched `loginSettings`; that is already covered by the page's own lazy fetch, so behaviour is unchanged (one cosmetic edge case under `ignoreUnknownUsernames` with both `userId` and `loginName` present — no functional impact). # Additional Context - Session *expiry* alone does not trigger this: expired rows are not removed from the session projection, so `getSession` still returns them. The `not_found` specifically means the session was **removed** server-side (logout, admin/API termination, or a user/org/instance cascade) while the browser still held the cookie. - Mirrors the pattern upstream already applies inline in `verify/page.tsx` (*"ignore error, as we might not have a session yet"*), lifted to the shared function. - **Related issue (not in this PR):** the sibling `loadSessionById` helpers on the `?sessionId=` path (`passkey`, `otp`, `mfa`, `mfa/set`, `signedin`, `authenticator/set`, `u2f`) call `getSession` directly and share the same defect. Lower practical exposure (short-lived in-flow URLs; a clean logout clears the cookie), so a follow-up PR should lift those into a shared `loadSessionById` in `lib/session.ts`. - Prior art: upstream added an equivalent guard to verify/page.tsx in #11130, but the other call sites were never given the same treatment — this PR resolves that inconsistency at the source. Error trace: ``` {"level":"error","message":"⨯ Error [ConnectError]: [not_found] Session does not exist (QUERY-SFeaa)\n at R (.next/server/chunks/ssr/_0uqqxk7._.js:3:25490)\n at (.next/server/chunks/ssr/_0uqqxk7._.js:3:26477)\n at next (.next/server/chunks/ssr/_0uqqxk7._.js:3:68112)\n at async (.next/server/chunks/ssr/_0uqqxk7._.js:10:31326)\n at async (.next/server/chunks/ssr/_0uqqxk7._.js:10:33734)\n at async Object.unary (.next/server/chunks/ssr/_0uqqxk7._.js:3:66838)\n at async Object.getSession (.next/server/chunks/ssr/_0uqqxk7._.js:3:72371)\n at async o (.next/server/chunks/ssr/[root-of-the-server]__0msfcq_._.js:1:7440) {\n rawMessage: 'Session does not exist (QUERY-SFeaa)',\n code: 5,\n metadata: Headers {\n 'accept-encoding': 'gzip',\n 'alt-svc': 'h3=\":443\"; ma=2592000',\n 'content-length': '240',\n 'content-type': 'application/json',\n date: 'Wed, 08 Jul 2026 09:06:09 GMT',\n 'strict-transport-security': 'max-age=63072000; includeSubDomains; preload',\n vary: 'Origin',\n },\n details: [\n {\n type: 'zitadel.v1.ErrorDetail',\n value: [Uint8Array],\n debug: [Object]\n }\n ],\n cause: undefined,\n httpStatus: 404,\n isUserError: true,\n digest: '634020356'\n}","timestamp":"2026-07-08T09:06:09.969Z"} ``` --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Marco A. Co-authored-by: Liam Neville --- apps/login/src/app/(login)/passkey/page.tsx | 3 - apps/login/src/app/(login)/password/page.tsx | 20 ++--- apps/login/src/app/(login)/verify/page.tsx | 7 -- .../src/app/(login)/verify/success/page.tsx | 6 +- apps/login/src/lib/session.test.ts | 83 ++++++++++++++++++- apps/login/src/lib/session.ts | 23 ++++- 6 files changed, 110 insertions(+), 32 deletions(-) diff --git a/apps/login/src/app/(login)/passkey/page.tsx b/apps/login/src/app/(login)/passkey/page.tsx index c236d626f..eda5093b3 100644 --- a/apps/login/src/app/(login)/passkey/page.tsx +++ b/apps/login/src/app/(login)/passkey/page.tsx @@ -40,9 +40,6 @@ export default async function Page(props: { searchParams: Promise { - // ignore error - return undefined; }); } diff --git a/apps/login/src/app/(login)/password/page.tsx b/apps/login/src/app/(login)/password/page.tsx index 3962285e4..0f6db975f 100644 --- a/apps/login/src/app/(login)/password/page.tsx +++ b/apps/login/src/app/(login)/password/page.tsx @@ -33,19 +33,13 @@ export default async function Page(props: { searchParams: Promise }) { loginName, organization, }, - }).catch(async (error) => { - loginSettings = await getLoginSettings({ serviceConfig, organization }); - if (!loginSettings?.ignoreUnknownUsernames) { - console.error("loadMostRecentSession failed", error); - } - // ignore error, as we might not have a session yet - return undefined; }); } else if ("userId" in searchParams && userId) { const userResponse = await getUserByID({ serviceConfig, userId }); diff --git a/apps/login/src/app/(login)/verify/success/page.tsx b/apps/login/src/app/(login)/verify/success/page.tsx index 17f46653e..58db5165b 100644 --- a/apps/login/src/app/(login)/verify/success/page.tsx +++ b/apps/login/src/app/(login)/verify/success/page.tsx @@ -18,11 +18,7 @@ export default async function Page(props: { searchParams: Promise }) { const branding = await getBrandingSettings({ serviceConfig, organization }); - const sessionFactors = await loadMostRecentSession({ serviceConfig, sessionParams: { loginName, organization } }).catch( - (error) => { - console.warn("Error loading session:", error); - }, - ); + const sessionFactors = await loadMostRecentSession({ serviceConfig, sessionParams: { loginName, organization } }); const id = userId ?? sessionFactors?.factors?.user?.id; diff --git a/apps/login/src/lib/session.test.ts b/apps/login/src/lib/session.test.ts index a8b18985c..e64f4f59e 100644 --- a/apps/login/src/lib/session.test.ts +++ b/apps/login/src/lib/session.test.ts @@ -13,10 +13,12 @@ * - Edge cases like sessions without expiration date */ +import { Code, ConnectError } from "@connectrpc/connect"; import { timestampDate } from "@zitadel/client"; import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { isSessionValid } from "./session"; +import * as cookiesModule from "./cookies"; +import { isSessionValid, loadMostRecentSession } from "./session"; import * as verifyHelperModule from "./verify-helper"; import * as zitadelModule from "./zitadel"; @@ -28,6 +30,11 @@ vi.mock("./zitadel", () => ({ listAuthenticationMethodTypes: vi.fn(), getLoginSettings: vi.fn(), getUserByID: vi.fn(), + getSession: vi.fn(), +})); + +vi.mock("./cookies", () => ({ + getMostRecentCookieWithLoginname: vi.fn(), })); vi.mock("./verify-helper", () => ({ @@ -1447,3 +1454,77 @@ describe("isSessionValid", () => { }); }); }); + +describe("loadMostRecentSession", () => { + const serviceConfig = { baseUrl: "https://zitadel-abc123.zitadel.cloud" }; + const sessionParams = { loginName: "test@example.com", organization: "test-org-id" }; + const cookie = { id: "session-id", token: "session-token", loginName: "test@example.com", organization: "test-org-id" }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("returns undefined without calling getSession when no cookie is found", async () => { + vi.mocked(cookiesModule.getMostRecentCookieWithLoginname).mockResolvedValue(undefined as any); + + const result = await loadMostRecentSession({ serviceConfig, sessionParams }); + + expect(result).toBeUndefined(); + expect(zitadelModule.getSession).not.toHaveBeenCalled(); + }); + + test("returns the session when getSession succeeds", async () => { + const session = { id: "session-id", factors: { user: { id: "user-id" } } }; + vi.mocked(cookiesModule.getMostRecentCookieWithLoginname).mockResolvedValue(cookie as any); + vi.mocked(zitadelModule.getSession).mockResolvedValue({ session } as any); + + const result = await loadMostRecentSession({ serviceConfig, sessionParams }); + + expect(result).toBe(session); + expect(zitadelModule.getSession).toHaveBeenCalledWith({ + serviceConfig, + sessionId: cookie.id, + sessionToken: cookie.token, + }); + }); + + test("returns undefined instead of throwing when getSession rejects with NotFound (stale cookie)", async () => { + // The `sessions` cookie can outlive the server-side session, so getSession may reject with + // a NotFound ConnectError. loadMostRecentSession must treat that the same as no session. + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const notFound = new ConnectError("Session does not exist (QUERY-SFeaa)", Code.NotFound); + vi.mocked(cookiesModule.getMostRecentCookieWithLoginname).mockResolvedValue(cookie as any); + vi.mocked(zitadelModule.getSession).mockRejectedValue(notFound); + + const result = await loadMostRecentSession({ serviceConfig, sessionParams }); + + expect(result).toBeUndefined(); + expect(consoleSpy).toHaveBeenCalledWith("[Session] Could not load most recent session", notFound); + consoleSpy.mockRestore(); + }); + + test("re-throws non-NotFound ConnectErrors (real failures must propagate)", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const unavailable = new ConnectError("backend unavailable", Code.Unavailable); + vi.mocked(cookiesModule.getMostRecentCookieWithLoginname).mockResolvedValue(cookie as any); + vi.mocked(zitadelModule.getSession).mockRejectedValue(unavailable); + + await expect(loadMostRecentSession({ serviceConfig, sessionParams })).rejects.toBe(unavailable); + expect(consoleSpy).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + test("re-throws non-ConnectError rejections (only coded NotFound is treated as no session)", async () => { + // The guard is `instanceof ConnectError && code === NotFound`; anything that is not a + // ConnectError (e.g. an unexpected runtime error) must not be mistaken for a stale + // cookie and must propagate. + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const unexpected = new Error("boom"); + vi.mocked(cookiesModule.getMostRecentCookieWithLoginname).mockResolvedValue(cookie as any); + vi.mocked(zitadelModule.getSession).mockRejectedValue(unexpected); + + await expect(loadMostRecentSession({ serviceConfig, sessionParams })).rejects.toBe(unexpected); + expect(consoleSpy).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/apps/login/src/lib/session.ts b/apps/login/src/lib/session.ts index 540fe78dc..b43f1446d 100644 --- a/apps/login/src/lib/session.ts +++ b/apps/login/src/lib/session.ts @@ -29,9 +29,26 @@ export async function loadMostRecentSession({ return undefined; } - return getSession({ serviceConfig, sessionId: recent.id, sessionToken: recent.token }).then( - (resp: GetSessionResponse) => resp.session, - ); + return getSession({ serviceConfig, sessionId: recent.id, sessionToken: recent.token }) + .then((resp: GetSessionResponse) => resp.session) + .catch(async (error) => { + const { Code, ConnectError } = await import("@connectrpc/connect"); + const isNotFound = error instanceof ConnectError && error.code === Code.NotFound; + + // The `sessions` cookie has no maxAge, so it can outlive the server-side session + // and reference one the session projection no longer holds — e.g. the user logged out, an + // admin/API terminated the session, or the user/org/instance was removed. In that + // case getSession throws `not_found`. Treat it like the no-cookie case above and + // return undefined instead of letting the error crash the caller's render. Every + // call site already handles an undefined session — either with a graceful fallback + // or by throwing its own explicit "no session" error. + if (isNotFound) { + console.warn("[Session] Could not load most recent session", error); + return undefined; + } + + throw error; + }); } /**