From 635494054a4ab1b2d02f675ce60421dc039bd327 Mon Sep 17 00:00:00 2001 From: Michael Costello Date: Thu, 9 Jul 2026 12:13:52 -0400 Subject: [PATCH] fix: handle sendLoginname failures in OIDC re-auth flow When a session has expired during the OIDC login callback, loginWithOIDCAndSession calls sendLoginname to redirect the user to re-authenticate. sendLoginname can throw (e.g. "Could not get domain", "Could not find identity provider", or a propagated ConnectError), and that call was not wrapped in a try/catch, unlike the createCallback call just below it. The unhandled rejection propagated out of the route handler, where Next.js redacts server error messages in production, so Sentry only ever saw a bare digest with no context. Wrap the call in try/catch, matching the same pattern already used in apps/login/src/app/login/route.ts, and report the real error to Sentry with tags/extra so future alerts carry useful context. The console.error line intentionally logs only the error message and correlating IDs (authRequest, sessionId), not the full error object, to keep pod logs terse; the full error still reaches Sentry via captureException. Add regression tests covering: the failure no longer throws, the failure is reported to Sentry, and the existing redirect-on-success behavior is unchanged. --- apps/login/src/lib/oidc.test.ts | 90 +++++++++++++++++++++++++++++++++ apps/login/src/lib/oidc.ts | 35 ++++++++----- 2 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 apps/login/src/lib/oidc.test.ts diff --git a/apps/login/src/lib/oidc.test.ts b/apps/login/src/lib/oidc.test.ts new file mode 100644 index 000000000..89c13b992 --- /dev/null +++ b/apps/login/src/lib/oidc.test.ts @@ -0,0 +1,90 @@ +import { Session } from "@zitadel/proto/zitadel/session/v2/session_pb"; +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, test, vi } from "vitest"; + +vi.mock("@/lib/server/loginname", () => ({ + sendLoginname: vi.fn(), +})); +vi.mock("@/lib/zitadel", () => ({ + createCallback: vi.fn(), + getLoginSettings: vi.fn(), + listAuthenticationMethodTypes: vi.fn(), +})); +vi.mock("@sentry/nextjs", () => ({ + captureException: vi.fn(), + captureMessage: vi.fn(), +})); +vi.mock("./session", () => ({ + isSessionValid: vi.fn(), +})); +vi.mock("./verify-helper", () => ({ + checkMFAFactors: vi.fn(), +})); + +import { sendLoginname } from "@/lib/server/loginname"; +import * as Sentry from "@sentry/nextjs"; +import { loginWithOIDCAndSession } from "./oidc"; +import { isSessionValid } from "./session"; + +const expiredSession = { + id: "session-1", + factors: { + user: { + id: "user-1", + loginName: "user@example.com", + organizationId: "org-1", + }, + // no intent factor: user did not authenticate via an IDP + }, +} as unknown as Session; + +const baseArgs = { + serviceUrl: "https://zitadel.example.com", + authRequest: "V2_authrequest-1", + sessionId: "session-1", + sessions: [expiredSession], + sessionCookies: [], + request: new NextRequest("https://auth.example.com/oidc/login"), +}; + +describe("loginWithOIDCAndSession", () => { + beforeEach(() => { + vi.mocked(isSessionValid).mockResolvedValue(false); + vi.mocked(sendLoginname).mockReset(); + vi.mocked(Sentry.captureException).mockReset(); + }); + + test("does not throw when sendLoginname rejects for an expired session", async () => { + vi.mocked(sendLoginname).mockRejectedValue( + new Error("Could not get domain"), + ); + + await expect(loginWithOIDCAndSession(baseArgs)).resolves.not.toThrow(); + }); + + test("reports the sendLoginname failure to Sentry instead of swallowing it silently", async () => { + const error = new Error("Could not get domain"); + vi.mocked(sendLoginname).mockRejectedValue(error); + + await loginWithOIDCAndSession(baseArgs); + + expect(Sentry.captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: { flow: "oidc", stage: "reauth_sendLoginname" }, + }), + ); + }); + + test("still redirects when sendLoginname resolves with a redirect", async () => { + vi.mocked(sendLoginname).mockResolvedValue({ + redirect: "/loginname?requestId=oidc_V2_authrequest-1", + }); + + const res = await loginWithOIDCAndSession(baseArgs); + + expect(res?.status).toBe(307); + expect(res?.headers.get("location")).toContain("/loginname"); + expect(Sentry.captureException).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/login/src/lib/oidc.ts b/apps/login/src/lib/oidc.ts index 571517ae3..0487457e2 100644 --- a/apps/login/src/lib/oidc.ts +++ b/apps/login/src/lib/oidc.ts @@ -5,6 +5,7 @@ import { getLoginSettings, listAuthenticationMethodTypes, } from "@/lib/zitadel"; +import * as Sentry from "@sentry/nextjs"; import { create } from "@zitadel/client"; import { CreateCallbackRequestSchema, @@ -86,19 +87,29 @@ export async function loginWithOIDCAndSession({ requestId: `oidc_${authRequest}`, }; - const res = await sendLoginname(command); - - if (res && "redirect" in res && res?.redirect) { - // Check if the redirect URL is already a full URL - if ( - res.redirect.startsWith("http://") || - res.redirect.startsWith("https://") - ) { - return NextResponse.redirect(res.redirect); - } else { - const absoluteUrl = constructUrl(request, res.redirect); - return NextResponse.redirect(absoluteUrl.toString()); + try { + const res = await sendLoginname(command); + + if (res && "redirect" in res && res?.redirect) { + // Check if the redirect URL is already a full URL + if ( + res.redirect.startsWith("http://") || + res.redirect.startsWith("https://") + ) { + return NextResponse.redirect(res.redirect); + } else { + const absoluteUrl = constructUrl(request, res.redirect); + return NextResponse.redirect(absoluteUrl.toString()); + } } + } catch (error) { + console.error( + `Failed to execute sendLoginname (authRequest=${authRequest}, sessionId=${sessionId}): ${error instanceof Error ? error.message : error}`, + ); + Sentry.captureException(error, { + tags: { flow: "oidc", stage: "reauth_sendLoginname" }, + extra: { authRequest, sessionId }, + }); } }