fix(login): prevent crash on a stale session cookie (#12423)

# 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 <unknown> (.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. <marco@zitadel.com> Co-authored-by: Liam Neville <liam@zitadel.com>
2026-08-17 16:35:14 -05:00 · 2026-07-14 09:03:46 +00:00
parent aa76c3be27
commit 051506b5bb
6 changed files with 110 additions and 32 deletions
@@ -40,9 +40,6 @@ export default async function Page(props: { searchParams: Promise<Record<string
    sessionFactors = await loadMostRecentSession({
      serviceConfig,
      sessionParams: { loginName, organization },
-    }).catch(() => {
-      // ignore error
-      return undefined;
    });
  }

@@ -33,19 +33,13 @@ export default async function Page(props: { searchParams: Promise<Record<string
  }

  // also allow no session to be found (ignoreUnkownUsername)
-  let sessionFactors;
-  try {
-    sessionFactors = await loadMostRecentSession({
-      serviceConfig,
-      sessionParams: {
-        loginName,
-        organization,
-      },
-    });
-  } catch (error) {
-    // ignore error to continue to show the password form
-    console.warn(error);
-  }
+  const sessionFactors = await loadMostRecentSession({
+    serviceConfig,
+    sessionParams: {
+      loginName,
+      organization,
+    },
+  });

  const branding = await getBrandingSettings({
    serviceConfig,
@@ -43,13 +43,6 @@ export default async function Page(props: { searchParams: Promise<any> }) {
        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 });
@@ -18,11 +18,7 @@ export default async function Page(props: { searchParams: Promise<any> }) {

  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;

@@ -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();
+  });
+});
@@ -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;
+    });
 }

 /**