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, }; }