mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
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 <[email protected]>
This commit is contained in:
co-authored by
Max Peintner
parent
f9995ee39c
commit
4dc89b499f
@@ -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",
|
||||
|
||||
@@ -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("/[email protected]")).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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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)) {
|
||||
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) {
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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!" };
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 = `
|
||||
<html>
|
||||
<body onload="document.forms[0].submit()">
|
||||
|
||||
@@ -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
|
||||
|
||||
Generated
+10
@@ -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/[email protected])
|
||||
@@ -975,6 +978,7 @@ packages:
|
||||
'@angular/[email protected]':
|
||||
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/[email protected]':
|
||||
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/[email protected]':
|
||||
resolution: {integrity: sha512-wMue2Sy4GAVTk6Ic4tJVcnfdau+gx2EnG7S+uAEe+TWJFqE4YoWN4/H8MSLj4eYJKxGg26lZwboEniNiNwZQ6Q==}
|
||||
|
||||
'@braintree/[email protected]':
|
||||
resolution: {integrity: sha512-jigsZK+sMF/cuiB7sERuo9V7N9jx+dhmHHnQyDSVdpZwVutaBu7WvNYqMDLSgFgfB30n452TP3vjDAvFC973mA==}
|
||||
|
||||
'@bufbuild/[email protected]':
|
||||
resolution: {integrity: sha512-c7owUswBbMmwfHPH9JRBEJu09mrXYGC33V2JQCgraWCBm74Z95AOkhDua50qiBrQnysvJkJ0p/z4MWxJqcpnIA==}
|
||||
engines: {node: '>=12'}
|
||||
@@ -13140,6 +13148,8 @@ snapshots:
|
||||
|
||||
'@balena/[email protected]': {}
|
||||
|
||||
'@braintree/[email protected]': {}
|
||||
|
||||
'@bufbuild/[email protected]':
|
||||
optional: true
|
||||
|
||||
|
||||
Reference in New Issue
Block a user