diff --git a/apps/login/AGENTS.md b/apps/login/AGENTS.md index 6ca1a5c6ca..d78e2536cc 100644 --- a/apps/login/AGENTS.md +++ b/apps/login/AGENTS.md @@ -15,6 +15,12 @@ The **Login App** (`apps/login`) provides the user interface for authentication - **State**: Critical authentication state is often managed via URL parameters (Auth Requests) and cookies/sessions. - **Scope Rule**: For shared API typings and client behavior, also read `packages/AGENTS.md` and `proto/AGENTS.md`. +## Base Path & Redirects (critical — read before touching any redirect logic) +- **The login app is served under a non-root base path in production.** ZITADEL Cloud and the official container image are built with `NEXT_PUBLIC_BASE_PATH=/ui/v2/login` (see `apps/login/.env`; the value is inlined at build time by Next.js). Never assume the app lives at the domain root. +- **`constructUrl()` (`src/lib/service-url.ts`) prepends the base path and must ONLY ever receive root-relative paths** (`/loginname`, `/password?...`). Passing an absolute URL glues the base path onto it and produces a broken redirect like `https:///ui/v2/loginhttps://login.microsoftonline.com/...`. +- **Some redirect targets ARE absolute URLs**: IdP authorize URLs returned by `startIdentityProviderFlow`, OIDC callback URLs from `createCallback`, and SAML endpoints from `createResponse`. When handling a redirect value that could be absolute, branch on `isExternalUrl()` first, validate absolute URLs with `isSafeRedirectUri()`, and pass them to `NextResponse.redirect` (server) / `window.location.href` (client) untouched. Canonical pattern: `resolveLoginHint` in `src/lib/server/flow-initiation.ts` and `handleServerActionResponse` in `src/lib/client-utils.ts`. +- **An empty base path masks this whole bug class**: with `NEXT_PUBLIC_BASE_PATH` unset, `new URL("" + absoluteUrl, origin)` still parses the absolute URL correctly, so local setups without the base path won't reproduce base-path bugs. Always test redirect changes with `NEXT_PUBLIC_BASE_PATH=/ui/v2/login` set, and verify the raw `Location` header (e.g. `curl -I`), not just browser behavior. + ## Verified Nx Targets - **Dev Server**: `pnpm nx run @zitadel/login:dev` - **Build**: `pnpm nx run @zitadel/login:build` diff --git a/apps/login/src/lib/server/flow-initiation.test.ts b/apps/login/src/lib/server/flow-initiation.test.ts index e409f8989d..5c90e77357 100644 --- a/apps/login/src/lib/server/flow-initiation.test.ts +++ b/apps/login/src/lib/server/flow-initiation.test.ts @@ -533,6 +533,117 @@ describe("handleOIDCFlowInitiation — org-scoped session filtering", () => { expect(location).toContain("/password"); expect(location).not.toContain("/loginname"); }); + + test("should redirect to an absolute IdP URL as-is without prepending the base path (domain discovery auto-redirect)", async () => { + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: [], + prompt: [], + loginHint: "user@discovered-org.com", + }, + }); + + // Simulate a base path being configured, as in ZITADEL Cloud (/ui/v2/login). + mockConstructUrl.mockImplementation((_req: any, path: string) => { + return new URL(`https://example.com/ui/v2/login${path}`); + }); + + // sendLoginname resolved the hint via domain discovery to an org with a + // single external IdP and returns the absolute authorize URL of that IdP. + const idpUrl = "https://login.microsoftonline.com/tenant-id/oauth2/v2.0/authorize?client_id=xyz&state=abc"; + mockSendLoginname.mockResolvedValue({ redirect: idpUrl }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + const location = res.headers.get("location") ?? ""; + expect(location).toBe(idpUrl); + // The base path must never be glued onto an absolute URL + // (regression: https:///ui/v2/loginhttps://login.microsoftonline.com/...). + expect(location).not.toContain("/ui/v2/login"); + expect(mockConstructUrl).not.toHaveBeenCalledWith(expect.anything(), idpUrl); + }); + + test("should render an auto-submit form when loginHint resolves to a SAML POST-binding IdP", async () => { + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: [], + prompt: [], + loginHint: "user@discovered-org.com", + }, + }); + + // sendLoginname resolved the hint via domain discovery to an org whose + // single IdP is SAML with POST binding: the AuthnRequest is delivered as + // form fields, not a redirect URL. + mockSendLoginname.mockResolvedValue({ + samlData: { + url: "https://adfs.example.com/adfs/ls", + fields: { SAMLRequest: "PHNhbWxwOkF1dGhuUmVxdWVzdD4=", RelayState: "relay-123" }, + }, + }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + // No redirect: the response is an HTML page auto-posting the form to the IdP. + expect(res.headers.get("location")).toBeNull(); + expect(res.headers.get("content-type")).toContain("text/html"); + + const html = await res.text(); + expect(html).toContain('action="https://adfs.example.com/adfs/ls"'); + expect(html).toContain('name="SAMLRequest"'); + expect(html).toContain('value="PHNhbWxwOkF1dGhuUmVxdWVzdD4="'); + expect(html).toContain('name="RelayState"'); + expect(html).toContain("document.forms[0].submit()"); + }); + + test("should block unsafe SAML post URLs from loginHint resolution and fall back to /loginname", async () => { + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: [], + prompt: [], + loginHint: "user@example.com", + }, + }); + + mockSendLoginname.mockResolvedValue({ + samlData: { + url: "javascript:alert(1)", + fields: { SAMLRequest: "abc" }, + }, + }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + const location = res.headers.get("location") ?? ""; + expect(location).toContain("/loginname"); + expect(location).not.toContain("javascript:"); + }); + + test("should block unsafe absolute redirect URLs from loginHint resolution and fall back to /loginname", async () => { + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: [], + prompt: [], + loginHint: "user@example.com", + }, + }); + + mockSendLoginname.mockResolvedValue({ redirect: "javascript:alert(1)" }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + const location = res.headers.get("location") ?? ""; + expect(location).toContain("/loginname"); + expect(location).not.toContain("javascript:"); + }); }); describe("handleOIDCFlowInitiation — Prompt.LOGIN + loginHint requestId prefix", () => { @@ -634,3 +745,130 @@ describe("handleOIDCFlowInitiation — Prompt.LOGIN + loginHint requestId prefix ); }); }); + +describe("handleOIDCFlowInitiation — idp scope (urn:zitadel:iam:org:idp:id)", () => { + let mockGetAuthRequest: ReturnType; + let mockConstructUrl: ReturnType; + let mockGetActiveIdentityProviders: ReturnType; + let mockStartIdentityProviderFlow: ReturnType; + let mockIdpTypeToSlug: ReturnType; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.unstubAllEnvs(); + + const zitadel = await import("@/lib/zitadel"); + const serviceUrl = await import("@/lib/service-url"); + const authUtils = await import("@/lib/auth-utils"); + const idpLib = await import("@/lib/idp"); + + mockGetAuthRequest = vi.mocked(zitadel.getAuthRequest); + mockConstructUrl = vi.mocked(serviceUrl.constructUrl); + mockGetActiveIdentityProviders = vi.mocked(zitadel.getActiveIdentityProviders); + mockStartIdentityProviderFlow = vi.mocked(zitadel.startIdentityProviderFlow); + mockIdpTypeToSlug = vi.mocked(idpLib.idpTypeToSlug); + vi.mocked(authUtils.getValidLocaleFromUILocales).mockReturnValue(null); + + mockConstructUrl.mockImplementation((_req: any, path: string) => { + return new URL(`https://example.com${path}`); + }); + mockIdpTypeToSlug.mockReturnValue("azure"); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + test("should use the type of the scoped IdP, not the first active IdP (multi-IdP org)", async () => { + const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb"); + + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: ["urn:zitadel:iam:org:idp:id:idp-2"], + prompt: [], + loginHint: undefined, + }, + }); + + // Two active IdPs: the scope selects the SECOND one. Regression: the slug + // was previously derived from identityProviders[0].type. + mockGetActiveIdentityProviders.mockResolvedValue({ + identityProviders: [ + { id: "idp-1", type: IdentityProviderType.GITHUB }, + { id: "idp-2", type: IdentityProviderType.AZURE_AD }, + ], + }); + + mockStartIdentityProviderFlow.mockResolvedValue({ + url: "https://login.microsoftonline.com/tenant/oauth2/v2.0/authorize?client_id=xyz", + }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + expect(mockIdpTypeToSlug).toHaveBeenCalledWith(IdentityProviderType.AZURE_AD); + expect(mockIdpTypeToSlug).not.toHaveBeenCalledWith(IdentityProviderType.GITHUB); + expect(res.headers.get("location")).toBe("https://login.microsoftonline.com/tenant/oauth2/v2.0/authorize?client_id=xyz"); + }); + + test("should block unsafe IdP URLs with a 400 instead of redirecting or rendering a form", async () => { + const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb"); + + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: ["urn:zitadel:iam:org:idp:id:idp-1"], + prompt: [], + loginHint: undefined, + }, + }); + + mockGetActiveIdentityProviders.mockResolvedValue({ + identityProviders: [{ id: "idp-1", type: IdentityProviderType.SAML }], + }); + + mockStartIdentityProviderFlow.mockResolvedValue({ + url: "javascript:alert(1)", + fields: { SAMLRequest: "abc" }, + }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toContain("Unsafe redirect URI"); + }); + + test("should render the auto-submit form for a scoped SAML POST-binding IdP", async () => { + const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb"); + + mockGetAuthRequest.mockResolvedValue({ + authRequest: { + id: "abc123", + uiLocales: [], + scope: ["urn:zitadel:iam:org:idp:id:idp-1"], + prompt: [], + loginHint: undefined, + }, + }); + + mockGetActiveIdentityProviders.mockResolvedValue({ + identityProviders: [{ id: "idp-1", type: IdentityProviderType.SAML }], + }); + + mockStartIdentityProviderFlow.mockResolvedValue({ + url: "https://adfs.example.com/adfs/ls", + fields: { SAMLRequest: "PHNhbWxwOkF1dGhuUmVxdWVzdD4=", RelayState: "relay-123" }, + }); + + const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] })); + + expect(res.headers.get("location")).toBeNull(); + expect(res.headers.get("content-type")).toContain("text/html"); + const html = await res.text(); + expect(html).toContain('action="https://adfs.example.com/adfs/ls"'); + expect(html).toContain('name="SAMLRequest"'); + }); +}); diff --git a/apps/login/src/lib/server/flow-initiation.ts b/apps/login/src/lib/server/flow-initiation.ts index b5fca35c85..7a4d5c0585 100644 --- a/apps/login/src/lib/server/flow-initiation.ts +++ b/apps/login/src/lib/server/flow-initiation.ts @@ -1,5 +1,5 @@ import { getValidLocaleFromUILocales } from "@/lib/auth-utils"; -import { isSafeRedirectUri } from "@/lib/client-utils"; +import { isExternalUrl, isSafeRedirectUri } from "@/lib/client-utils"; import { getLanguageCookie, setLanguageCookie } from "@/lib/cookies"; import { shouldUILocalesOverrideCookie } from "@/lib/i18n"; @@ -53,6 +53,36 @@ function setCSPHeaders( } } +/** + * Renders a minimal HTML page that immediately POSTs the given fields to + * `url`, with a