From a4266fa7fc011c901e1fd4207d6f6e82d35472f7 Mon Sep 17 00:00:00 2001 From: "Marco A." Date: Fri, 14 Aug 2026 06:12:33 +0200 Subject: [PATCH] Merge commit from fork * fix(login): strip returnCode from browser supplied session challenges The updateOrCreateSession server action forwarded the browser's challenges object to the session API verbatim and returned the resulting challenges in its response. A party who knew only a victim's login name could create an identify-only session, request OTP-Email and OTP-SMS challenges with the returnCode delivery type, and read both plaintext codes straight out of the server action response. sanitizeChallenges forces returnCode off before anything is forwarded. It is applied in createSessionAndUpdateCookie and setSessionAndUpdateCookie rather than in the action itself, so every caller including sendPasskey is covered by one gate. An OTP-Email returnCode request is rewritten to sendCode rather than dropped, so the legitimate flow still completes and the code is delivered to the user; WebAuthN challenges carry no code and pass through untouched. The action response is also narrowed to the WebAuthN challenge. That is redundant while the sanitizer holds, but it keeps "no OTP code crosses this boundary" a local, readable property. Co-Authored-By: Claude Opus 5 * test(login): assert session challenges are sanitized before reaching the API challenges.test.ts covers what sanitizeChallenges does, but nothing asserted that cookie.ts actually calls it. A refactor could drop either call site and the whole suite would still pass, silently reopening the hole. Covers both entry points to the session API by asserting on the challenges the mocked client receives, so the test fails if the sanitizer is ever bypassed. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- apps/login/src/lib/server/challenges.test.ts | 82 +++++++++++++++ apps/login/src/lib/server/challenges.ts | 44 ++++++++ apps/login/src/lib/server/cookie.test.ts | 101 +++++++++++++++++++ apps/login/src/lib/server/cookie.ts | 5 +- apps/login/src/lib/server/session.ts | 12 ++- 5 files changed, 239 insertions(+), 5 deletions(-) create mode 100644 apps/login/src/lib/server/challenges.test.ts create mode 100644 apps/login/src/lib/server/challenges.ts create mode 100644 apps/login/src/lib/server/cookie.test.ts diff --git a/apps/login/src/lib/server/challenges.test.ts b/apps/login/src/lib/server/challenges.test.ts new file mode 100644 index 000000000..9d2ef58cf --- /dev/null +++ b/apps/login/src/lib/server/challenges.test.ts @@ -0,0 +1,82 @@ +import { create } from "@zitadel/client"; +import { RequestChallengesSchema } from "@zitadel/proto/zitadel/session/v2/challenge_pb"; +import { assert, describe, expect, it } from "vitest"; +import { sanitizeChallenges } from "./challenges"; + +describe("sanitizeChallenges", () => { + it("returns undefined when no challenges are requested", () => { + expect(sanitizeChallenges(undefined)).toBeUndefined(); + }); + + it("forces returnCode off for an OTP SMS challenge", () => { + const challenges = create(RequestChallengesSchema, { + otpSms: { returnCode: true }, + }); + + expect(sanitizeChallenges(challenges)?.otpSms?.returnCode).toBe(false); + }); + + it("leaves an OTP SMS challenge that already sends the code alone", () => { + const challenges = create(RequestChallengesSchema, { + otpSms: {}, + }); + + expect(sanitizeChallenges(challenges)?.otpSms?.returnCode).toBe(false); + }); + + it("rewrites an OTP email returnCode challenge to sendCode", () => { + const challenges = create(RequestChallengesSchema, { + otpEmail: { deliveryType: { case: "returnCode", value: {} } }, + }); + + const deliveryType = sanitizeChallenges(challenges)?.otpEmail?.deliveryType; + + assert(deliveryType?.case === "sendCode"); + // no url template: Zitadel falls back to its default verification url + expect(deliveryType.value.urlTemplate).toBeUndefined(); + }); + + it("preserves the url template of an OTP email sendCode challenge", () => { + const urlTemplate = "https://example.com/otp/verify?code={{.Code}}"; + const challenges = create(RequestChallengesSchema, { + otpEmail: { deliveryType: { case: "sendCode", value: { urlTemplate } } }, + }); + + const deliveryType = sanitizeChallenges(challenges)?.otpEmail?.deliveryType; + + assert(deliveryType?.case === "sendCode"); + expect(deliveryType.value.urlTemplate).toBe(urlTemplate); + }); + + it("passes a WebAuthN challenge through untouched", () => { + const challenges = create(RequestChallengesSchema, { + webAuthN: { domain: "login.example.com", userVerificationRequirement: 1 }, + }); + + expect(sanitizeChallenges(challenges)?.webAuthN).toEqual(challenges.webAuthN); + }); + + it("strips returnCode from both OTP methods requested together", () => { + const challenges = create(RequestChallengesSchema, { + otpSms: { returnCode: true }, + otpEmail: { deliveryType: { case: "returnCode", value: {} } }, + }); + + const sanitized = sanitizeChallenges(challenges); + + expect(sanitized?.otpSms?.returnCode).toBe(false); + expect(sanitized?.otpEmail?.deliveryType?.case).toBe("sendCode"); + }); + + it("does not mutate the caller's challenges", () => { + const challenges = create(RequestChallengesSchema, { + otpSms: { returnCode: true }, + otpEmail: { deliveryType: { case: "returnCode", value: {} } }, + }); + + sanitizeChallenges(challenges); + + expect(challenges.otpSms?.returnCode).toBe(true); + expect(challenges.otpEmail?.deliveryType?.case).toBe("returnCode"); + }); +}); diff --git a/apps/login/src/lib/server/challenges.ts b/apps/login/src/lib/server/challenges.ts new file mode 100644 index 000000000..e407657ee --- /dev/null +++ b/apps/login/src/lib/server/challenges.ts @@ -0,0 +1,44 @@ +import { create } from "@zitadel/client"; +import { + RequestChallenges, + RequestChallenges_OTPEmail_SendCodeSchema, +} from "@zitadel/proto/zitadel/session/v2/challenge_pb"; + +/** + * Strips the `returnCode` delivery type from a client-supplied challenge request. + * + * `returnCode` makes Zitadel return the freshly generated OTP in plaintext in the response + * body instead of sending it to the user's mailbox or phone. It is a legitimate API feature + * for callers that deliver the code themselves, but this app proxies requests on behalf of + * an untrusted browser: anything it forwards is attacker-controllable, and anything it + * returns is attacker-readable. + * + * Without this, a party who knows only a victim's login name could request both OTP-Email + * and OTP-SMS challenges on an identify-only session, read both codes straight out of the + * server-action response and submit them back as two verified factors — GHSA-3gwm-5wx8-4gm6. + * + * OTP challenges are rewritten to their out-of-band equivalent rather than dropped, so the + * legitimate 2FA flow still completes and the code reaches the actual user. WebAuthN + * challenges carry no code and are passed through untouched. + */ +export function sanitizeChallenges(challenges?: RequestChallenges): RequestChallenges | undefined { + if (!challenges) { + return challenges; + } + + const sanitized = { ...challenges }; + + if (sanitized.otpSms) { + sanitized.otpSms = { ...sanitized.otpSms, returnCode: false }; + } + + if (sanitized.otpEmail?.deliveryType?.case === "returnCode") { + sanitized.otpEmail = { + ...sanitized.otpEmail, + // fall back to the default Zitadel verification url + deliveryType: { case: "sendCode", value: create(RequestChallenges_OTPEmail_SendCodeSchema, {}) }, + }; + } + + return sanitized; +} diff --git a/apps/login/src/lib/server/cookie.test.ts b/apps/login/src/lib/server/cookie.test.ts new file mode 100644 index 000000000..570b37d28 --- /dev/null +++ b/apps/login/src/lib/server/cookie.test.ts @@ -0,0 +1,101 @@ +import { create } from "@zitadel/client"; +import { RequestChallengesSchema } from "@zitadel/proto/zitadel/session/v2/challenge_pb"; +import { beforeEach, describe, expect, test, vi } from "vitest"; + +vi.mock("next/headers", () => ({ + headers: vi.fn(), +})); + +vi.mock("../service-url", () => ({ + getServiceConfig: vi.fn(), +})); + +vi.mock("@/lib/zitadel", () => ({ + createSessionForUserIdAndIdpIntent: vi.fn(), + createSessionFromChecksAndChallenges: vi.fn(), + getSecuritySettings: vi.fn(), + getSession: vi.fn(), + setSession: vi.fn(), +})); + +vi.mock("@/lib/cookies", () => ({ + addSessionToCookie: vi.fn(), + updateSessionCookie: vi.fn(), +})); + +import { addSessionToCookie, updateSessionCookie } from "@/lib/cookies"; +import { createSessionFromChecksAndChallenges, getSecuritySettings, getSession, setSession } from "@/lib/zitadel"; +import { headers } from "next/headers"; +import { getServiceConfig } from "../service-url"; +import { createSessionAndUpdateCookie, setSessionAndUpdateCookie } from "./cookie"; + +// A challenge request as an attacker-controlled browser could construct it: both OTP methods +// asking for the code to come back in the response instead of being sent to the user. +function returnCodeChallenges() { + return create(RequestChallengesSchema, { + otpSms: { returnCode: true }, + otpEmail: { deliveryType: { case: "returnCode", value: {} } }, + }); +} + +function expectSanitized(challenges: any) { + expect(challenges.otpSms.returnCode).toBe(false); + expect(challenges.otpEmail.deliveryType.case).toBe("sendCode"); +} + +describe("session challenge sanitization", () => { + beforeEach(() => { + vi.clearAllMocks(); + + vi.mocked(headers).mockResolvedValue({} as any); + vi.mocked(getServiceConfig).mockReturnValue({ serviceConfig: {} } as any); + vi.mocked(getSecuritySettings).mockResolvedValue({ embeddedIframe: { enabled: false } } as any); + vi.mocked(getSession).mockResolvedValue({ + session: { + id: "sessionId", + factors: { user: { loginName: "victim@example.com", organizationId: "orgId" } }, + }, + } as any); + vi.mocked(addSessionToCookie).mockResolvedValue(undefined as any); + vi.mocked(updateSessionCookie).mockResolvedValue(undefined as any); + }); + + test("createSessionAndUpdateCookie strips returnCode before calling the API", async () => { + vi.mocked(createSessionFromChecksAndChallenges).mockResolvedValue({ + sessionId: "sessionId", + sessionToken: "sessionToken", + } as any); + + await createSessionAndUpdateCookie({ + checks: {} as any, + requestId: undefined, + challenges: returnCodeChallenges(), + }); + + expect(createSessionFromChecksAndChallenges).toHaveBeenCalledTimes(1); + expectSanitized(vi.mocked(createSessionFromChecksAndChallenges).mock.calls[0][0].challenges); + }); + + test("setSessionAndUpdateCookie strips returnCode before calling the API", async () => { + vi.mocked(setSession).mockResolvedValue({ + sessionToken: "sessionToken", + details: {}, + } as any); + + await setSessionAndUpdateCookie({ + recentCookie: { + id: "sessionId", + token: "sessionToken", + loginName: "victim@example.com", + creationTs: "", + expirationTs: "", + changeTs: "", + }, + challenges: returnCodeChallenges(), + lifetime: {} as any, + }); + + expect(setSession).toHaveBeenCalledTimes(1); + expectSanitized(vi.mocked(setSession).mock.calls[0][0].challenges); + }); +}); diff --git a/apps/login/src/lib/server/cookie.ts b/apps/login/src/lib/server/cookie.ts index 801654036..930ac566f 100644 --- a/apps/login/src/lib/server/cookie.ts +++ b/apps/login/src/lib/server/cookie.ts @@ -2,6 +2,7 @@ import { addSessionToCookie, updateSessionCookie } from "@/lib/cookies"; import { createLogger } from "@/lib/logger"; +import { sanitizeChallenges } from "@/lib/server/challenges"; import { createSessionForUserIdAndIdpIntent, createSessionFromChecksAndChallenges, @@ -67,7 +68,7 @@ export async function createSessionAndUpdateCookie(command: { serviceConfig, checks: command.checks, lifetime: sessionLifetime, - challenges: command.challenges, + challenges: sanitizeChallenges(command.challenges), }); if (createdSession) { @@ -211,7 +212,7 @@ export async function setSessionAndUpdateCookie(command: { serviceConfig, sessionId: command.recentCookie.id, sessionToken: command.recentCookie.token, - challenges: command.challenges, + challenges: sanitizeChallenges(command.challenges), checks: command.checks, lifetime: command.lifetime, }) diff --git a/apps/login/src/lib/server/session.ts b/apps/login/src/lib/server/session.ts index f694283ae..7a33481ce 100644 --- a/apps/login/src/lib/server/session.ts +++ b/apps/login/src/lib/server/session.ts @@ -12,7 +12,7 @@ import { listUsers, } from "@/lib/zitadel"; import { create, Duration } from "@zitadel/client"; -import { RequestChallenges } from "@zitadel/proto/zitadel/session/v2/challenge_pb"; +import { Challenges, RequestChallenges } from "@zitadel/proto/zitadel/session/v2/challenge_pb"; import { Session } from "@zitadel/proto/zitadel/session/v2/session_pb"; import { Checks, ChecksSchema } from "@zitadel/proto/zitadel/session/v2/session_service_pb"; import { getTranslations } from "next-intl/server"; @@ -275,11 +275,17 @@ export async function updateOrCreateSession(options: UpdateSessionCommand) { } } + // @ts-ignore + const challengeResponse: Challenges | undefined = session.challenges; + return { sessionId: session.id, factors: session.factors, - // @ts-ignore - challenges: session.challenges, + // Only the WebAuthN challenge may reach the browser. `sanitizeChallenges` already stops + // `returnCode` from being requested, so `otpSms`/`otpEmail` should always be unset here — + // keeping the projection explicit means no OTP code can leak through this boundary even + // if that ever regresses (GHSA-3gwm-5wx8-4gm6). + challenges: challengeResponse?.webAuthN ? { webAuthN: challengeResponse.webAuthN } : undefined, authMethods, }; }