From 4dc89b499f233f99ff7e9089b8c73336c8dac1d4 Mon Sep 17 00:00:00 2001 From: Livio Spring <9405495+livio-a@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:19:40 +0200 Subject: [PATCH] fix(login): allow custom protocols for native apps again (#12332) # Which Problems Are Solved - Native apps (mobile/desktop) use custom protocol schemes (e.g. `myapp://callback`, `io.zitadel.app://auth`) as OAuth/OIDC redirect URIs. The previous redirect-safety check in the login UI only allowed `http:`/`https:` absolute URLs, so these custom-scheme callbacks were wrongly rejected and native app login flows were broken. - Even for valid external redirects, `router.push()` triggers an RSC prefetch `fetch()` that is blocked by the CSP `connect-src 'self'` directive for non-same-origin URLs, so navigation to external/custom-protocol targets failed. - SAML and OIDC callback URLs returned from the backend were not consistently validated before being used for redirects/POSTs across the various flow entry points. # How the Problems Are Solved - Reworked `isSafeRedirectUri` to use `@braintree/sanitize-url` instead of a strict `http/https`-only allowlist. Custom protocol schemes are now permitted, while dangerous schemes (`javascript:`, `data:`, plus an explicit block of `file:`, `blob:`, `about:`) are still rejected. - Added `isExternalUrl` helper to distinguish internal relative paths (`/...`) from external/custom-protocol targets. - In `handleServerActionResponse`, external/custom-protocol redirects now navigate via `window.location.href` (full navigation) instead of `router.push()`, avoiding the CSP-blocked RSC prefetch. Internal paths continue to use `router.push()`. - Added consistent redirect-URI validation for SAML and OIDC callbacks across all relevant entry points (`oidc.ts`, `saml.ts`, `server/flow-initiation.ts` including the `prompt=none` path, and the SAML `samlData` POST case), blocking unsafe URLs with a clear warning and error response. # Additional Changes - Added `@braintree/sanitize-url` (`^7.1.2`) dependency to `apps/login` (`package.json` / `pnpm-lock.yaml`). - Added unit tests for `isExternalUrl` covering relative paths, absolute HTTP/HTTPS URLs, custom protocol schemes, and protocol-relative URLs. - Annotated the intentional `window.location.href` navigation with a CodeQL suppression comment, documenting that the URL is validated by `isSafeRedirectUri` beforehand. - Minor formatting cleanup in `console` JWT provider component. # Additional Context - This restores behavior that previously worked for native apps but regressed when redirect validation was tightened to `http`/`https` only. --------- Co-authored-by: Max Peintner --- apps/login/package.json | 1 + apps/login/src/lib/client-utils.test.ts | 34 +++++++++++- apps/login/src/lib/client-utils.ts | 53 ++++++++++++++----- apps/login/src/lib/oidc.ts | 4 ++ apps/login/src/lib/saml.ts | 8 +++ apps/login/src/lib/server/flow-initiation.ts | 17 ++++++ .../provider-jwt/provider-jwt.component.ts | 2 +- pnpm-lock.yaml | 10 ++++ 8 files changed, 113 insertions(+), 16 deletions(-) diff --git a/apps/login/package.json b/apps/login/package.json index 71ee968f5..15fd3976d 100644 --- a/apps/login/package.json +++ b/apps/login/package.json @@ -23,6 +23,7 @@ "*": "prettier --write --ignore-unknown" }, "dependencies": { + "@braintree/sanitize-url": "^7.1.2", "@connectrpc/connect": "^2.1.1", "@connectrpc/connect-node": "^2.1.1", "@headlessui/react": "^2.2.10", diff --git a/apps/login/src/lib/client-utils.test.ts b/apps/login/src/lib/client-utils.test.ts index 0aa57cb2d..4653804fe 100644 --- a/apps/login/src/lib/client-utils.test.ts +++ b/apps/login/src/lib/client-utils.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; -import { isSafeRedirectUri } from "./client-utils"; +import { isExternalUrl, isSafeRedirectUri } from "./client-utils"; describe("isSafeRedirectUri", () => { beforeEach(() => { @@ -42,3 +42,35 @@ describe("isSafeRedirectUri", () => { expect(await isSafeRedirectUri("http://%")).toBe(false); }); }); + +describe("isExternalUrl", () => { + test("should return false for relative paths", () => { + expect(isExternalUrl("/dashboard")).toBe(false); + expect(isExternalUrl("/ui/console")).toBe(false); + expect(isExternalUrl("/")).toBe(false); + expect(isExternalUrl("/password?loginName=user@example.com")).toBe(false); + expect(isExternalUrl("/signedin?sessionId=123&organization=456")).toBe(false); + }); + + test("should return true for absolute HTTPS URLs", () => { + expect(isExternalUrl("https://example.com/callback")).toBe(true); + expect(isExternalUrl("https://my-zitadel.com/ui/console")).toBe(true); + }); + + test("should return true for absolute HTTP URLs", () => { + expect(isExternalUrl("http://localhost:3000/callback")).toBe(true); + expect(isExternalUrl("http://example.com")).toBe(true); + }); + + test("should return true for custom protocol schemes (native apps)", () => { + expect(isExternalUrl("myapp://callback")).toBe(true); + expect(isExternalUrl("com.example.app://oauth/callback")).toBe(true); + expect(isExternalUrl("io.zitadel.app://auth")).toBe(true); + expect(isExternalUrl("flutter-app://callback?code=abc&state=xyz")).toBe(true); + }); + + test("should return true for protocol-relative URLs", () => { + expect(isExternalUrl("//evil.com")).toBe(true); + expect(isExternalUrl("//example.com/path")).toBe(true); + }); +}); diff --git a/apps/login/src/lib/client-utils.ts b/apps/login/src/lib/client-utils.ts index f96e417f9..43437caad 100644 --- a/apps/login/src/lib/client-utils.ts +++ b/apps/login/src/lib/client-utils.ts @@ -1,3 +1,5 @@ +import { sanitizeUrl } from "@braintree/sanitize-url"; + export type ServerActionResponse = | { redirect: string } | { error: string } @@ -17,7 +19,16 @@ export function handleServerActionResponse( if ("redirect" in response && response.redirect) { if (isSafeRedirectUri(response.redirect)) { - router.push(response.redirect); + if (typeof window !== "undefined" && isExternalUrl(response.redirect)) { + // External/custom-protocol URLs: use window.location for full navigation. + // router.push() would trigger an RSC prefetch fetch() that gets blocked + // by CSP connect-src 'self' for non-same-origin URLs. + // CodeQL: This is safe — the URL is validated by isSafeRedirectUri above, + // which blocks javascript:, data:, file:, blob:, and about: schemes. + window.location.href = response.redirect; // lgtm[js/client-side-unvalidated-url-redirection] + } else { + router.push(response.redirect); + } return true; } else { console.warn("handleServerActionResponse: Blocked unsafe redirect URI:", response.redirect); @@ -27,8 +38,14 @@ export function handleServerActionResponse( } if ("samlData" in response && response.samlData) { - setSamlData(response.samlData); - return true; + if (isSafeRedirectUri(response.samlData.url)) { + setSamlData(response.samlData); + return true; + } else { + console.warn("handleServerActionResponse: Blocked unsafe SAML post URL:", response.samlData.url); + setError("Unsafe redirect URI was blocked"); + return true; + } } if ("error" in response && response.error) { @@ -39,6 +56,20 @@ export function handleServerActionResponse( return false; } +/** + * Returns true if the URL is external (absolute URL or custom protocol). + * Relative paths (starting with "/" but not "//") are considered internal. + */ +export function isExternalUrl(url: string): boolean { + if (url.startsWith("/") && !url.startsWith("//")) { + return false; + } + return true; +} + +const SANITIZE_BLANK = "about:blank"; +const EXTRA_BLOCKED = new Set(["file:", "blob:", "about:"]); + /** * Validates whether a given redirect URI is safe. * Safe URIs are either relative paths or absolute URLs matching the current host. @@ -52,19 +83,13 @@ export function isSafeRedirectUri(uri: string): boolean { return true; } - // 2. Check absolute URLs for safe protocols (http/https) - // We allow external domains, but strictly forbid javascript:/data: etc. + const sanitized = sanitizeUrl(uri); + if (sanitized === SANITIZE_BLANK) return false; + try { - const parsedUri = new URL(uri); - - // Only allow http(s) protocols - if (parsedUri.protocol !== "http:" && parsedUri.protocol !== "https:") { - return false; - } - - return true; + const parsed = new URL(sanitized); + return !EXTRA_BLOCKED.has(parsed.protocol); } catch { - // If it can't be parsed as a URL and didn't start with /, it's unsafe return false; } } diff --git a/apps/login/src/lib/oidc.ts b/apps/login/src/lib/oidc.ts index d6ce17b6f..76a1601d1 100644 --- a/apps/login/src/lib/oidc.ts +++ b/apps/login/src/lib/oidc.ts @@ -65,6 +65,10 @@ export async function loginWithOIDCAndSession({ }), }); if (callbackUrl) { + if (!isSafeRedirectUri(callbackUrl)) { + console.warn("loginWithOIDCAndSession: Blocked unsafe OIDC callback URL:", callbackUrl); + return { error: "Unsafe redirect URI was blocked" }; + } return { redirect: callbackUrl }; } else { return { error: "An error occurred!" }; diff --git a/apps/login/src/lib/saml.ts b/apps/login/src/lib/saml.ts index 1a5ea4a97..38443937f 100644 --- a/apps/login/src/lib/saml.ts +++ b/apps/login/src/lib/saml.ts @@ -73,8 +73,16 @@ export async function loginWithSAMLAndSession({ }), }); if (url && binding.case === "redirect") { + if (!isSafeRedirectUri(url)) { + console.warn("loginWithSAMLAndSession: Blocked unsafe SAML redirect URL:", url); + return { error: "Unsafe redirect URI was blocked" }; + } return { redirect: url }; } else if (url && binding.case === "post") { + if (!isSafeRedirectUri(url)) { + console.warn("loginWithSAMLAndSession: Blocked unsafe SAML post URL:", url); + return { error: "Unsafe redirect URI was blocked" }; + } return { samlData: { url, diff --git a/apps/login/src/lib/server/flow-initiation.ts b/apps/login/src/lib/server/flow-initiation.ts index cb268e926..05b7f61ec 100644 --- a/apps/login/src/lib/server/flow-initiation.ts +++ b/apps/login/src/lib/server/flow-initiation.ts @@ -1,4 +1,5 @@ import { getValidLocaleFromUILocales } from "@/lib/auth-utils"; +import { isSafeRedirectUri } from "@/lib/client-utils"; import { getLanguageCookie, setLanguageCookie } from "@/lib/cookies"; import { shouldUILocalesOverrideCookie } from "@/lib/i18n"; @@ -306,6 +307,10 @@ export async function handleOIDCFlowInitiation(params: FlowInitiationParams): Pr }), }); + if (!isSafeRedirectUri(callbackUrl)) { + logger.warn("Blocked unsafe OIDC callback URL (prompt=none)", { callbackUrl }); + return NextResponse.json({ error: "Unsafe redirect URI was blocked" }, { status: 400 }); + } const callbackResponse = NextResponse.redirect(callbackUrl); setCSPHeaders(callbackResponse, serviceConfig, securitySettings); return callbackResponse; @@ -347,6 +352,10 @@ export async function handleOIDCFlowInitiation(params: FlowInitiationParams): Pr }), }); if (callbackUrl) { + if (!isSafeRedirectUri(callbackUrl)) { + logger.warn("Blocked unsafe OIDC callback URL", { callbackUrl }); + return NextResponse.json({ error: "Unsafe redirect URI was blocked" }, { status: 400 }); + } return NextResponse.redirect(callbackUrl); } else { logger.info("could not create callback, redirect user to choose other account"); @@ -446,8 +455,16 @@ export async function handleSAMLFlowInitiation(params: FlowInitiationParams): Pr }); if (url && binding.case === "redirect") { + if (!isSafeRedirectUri(url)) { + logger.warn("Blocked unsafe SAML redirect URL", { url }); + return NextResponse.json({ error: "Unsafe redirect URI was blocked" }, { status: 400 }); + } return NextResponse.redirect(url); } else if (url && binding.case === "post") { + if (!isSafeRedirectUri(url)) { + logger.warn("Blocked unsafe SAML post URL", { url }); + return NextResponse.json({ error: "Unsafe redirect URI was blocked" }, { status: 400 }); + } const html = ` diff --git a/console/src/app/modules/providers/provider-jwt/provider-jwt.component.ts b/console/src/app/modules/providers/provider-jwt/provider-jwt.component.ts index 6bbf7089b..4e173669b 100644 --- a/console/src/app/modules/providers/provider-jwt/provider-jwt.component.ts +++ b/console/src/app/modules/providers/provider-jwt/provider-jwt.component.ts @@ -117,7 +117,7 @@ export class ProviderJWTComponent { issuer: new UntypedFormControl('', [requiredValidator]), jwtEndpoint: new UntypedFormControl('', [requiredValidator]), keysEndpoint: new UntypedFormControl('', [requiredValidator]), - audience: new UntypedFormControl('' ), + audience: new UntypedFormControl(''), }); this.authService diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0198fd44c..c07c73791 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -208,6 +208,9 @@ importers: apps/login: dependencies: + '@braintree/sanitize-url': + specifier: ^7.1.2 + version: 7.1.2 '@connectrpc/connect': specifier: ^2.1.1 version: 2.1.1(@bufbuild/protobuf@2.12.0) @@ -975,6 +978,7 @@ packages: '@angular/animations@21.2.14': resolution: {integrity: sha512-9WLnsJE0xqtd1rVtHMvsAUxFy3OdPks4bdmUIqyw23X/je7ytUALAGWNadffcZBwRpa1A6TUnLr9X4+Draz3kw==} engines: {node: ^20.19.0 || ^22.12.0 || >=24.0.0} + deprecated: '@angular/animations is deprecated. Use `animate.enter` and `animate.leave` instead. For more information see: https://v22.angular.dev/guide/animations.' peerDependencies: '@angular/core': 21.2.14 @@ -1105,6 +1109,7 @@ packages: '@angular/platform-browser-dynamic@21.2.14': resolution: {integrity: sha512-m5U4zX8JFnxTAIGpsBXIAyefSmYqdORY/OfHC0aMmZovuFCbXXIYqYRQDBB7+YVNpSDSHllCrKEZFu/CC6dq3g==} engines: {node: ^20.19.0 || ^22.12.0 || >=24.0.0} + deprecated: '@angular/platform-browser-dynamic is deprecated. Use `@angular/platform-browser` instead.' peerDependencies: '@angular/common': 21.2.14 '@angular/compiler': 21.2.14 @@ -1308,6 +1313,9 @@ packages: '@balena/dockerignore@1.0.2': resolution: {integrity: sha512-wMue2Sy4GAVTk6Ic4tJVcnfdau+gx2EnG7S+uAEe+TWJFqE4YoWN4/H8MSLj4eYJKxGg26lZwboEniNiNwZQ6Q==} + '@braintree/sanitize-url@7.1.2': + resolution: {integrity: sha512-jigsZK+sMF/cuiB7sERuo9V7N9jx+dhmHHnQyDSVdpZwVutaBu7WvNYqMDLSgFgfB30n452TP3vjDAvFC973mA==} + '@bufbuild/buf-darwin-arm64@1.70.0': resolution: {integrity: sha512-c7owUswBbMmwfHPH9JRBEJu09mrXYGC33V2JQCgraWCBm74Z95AOkhDua50qiBrQnysvJkJ0p/z4MWxJqcpnIA==} engines: {node: '>=12'} @@ -13140,6 +13148,8 @@ snapshots: '@balena/dockerignore@1.0.2': {} + '@braintree/sanitize-url@7.1.2': {} + '@bufbuild/buf-darwin-arm64@1.70.0': optional: true