From be32012ca3507a62371d1baa09cdacd5123a99bf Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Sun, 31 May 2026 11:38:52 +0100 Subject: [PATCH] fix(oauth): validate redirect_uri schemes in oidc-provider and mcp (#9838) --- .../oauth-redirect-uri-scheme-validation.md | 11 ++++ .cspell/tech-terms.txt | 1 + .../better-auth/src/client/fetch-plugins.ts | 7 ++- packages/better-auth/src/plugins/mcp/index.ts | 12 +++- .../src/plugins/oidc-provider/index.ts | 21 +++++-- .../oidc-provider/redirect-uri.test.ts | 57 +++++++++++++++++++ .../better-auth/src/plugins/one-tap/client.ts | 11 +++- .../src/plugins/two-factor/client.ts | 7 ++- packages/core/src/utils/redirect-uri.ts | 45 +++++++++++++++ packages/core/src/utils/url.test.ts | 49 ++++++++++++++++ packages/core/src/utils/url.ts | 25 ++++++++ packages/oauth-provider/src/types/zod.ts | 38 +------------ 12 files changed, 240 insertions(+), 44 deletions(-) create mode 100644 .changeset/oauth-redirect-uri-scheme-validation.md create mode 100644 packages/better-auth/src/plugins/oidc-provider/redirect-uri.test.ts create mode 100644 packages/core/src/utils/redirect-uri.ts create mode 100644 packages/core/src/utils/url.test.ts diff --git a/.changeset/oauth-redirect-uri-scheme-validation.md b/.changeset/oauth-redirect-uri-scheme-validation.md new file mode 100644 index 0000000000..e8f740e50a --- /dev/null +++ b/.changeset/oauth-redirect-uri-scheme-validation.md @@ -0,0 +1,11 @@ +--- +"better-auth": patch +--- + +Validate the scheme of OAuth `redirect_uris` in the `oidc-provider` and `mcp` plugins. + +Both plugins previously accepted any string as a `redirect_uri` at registration. They now reject the `javascript:`, `data:`, and `vbscript:` schemes, which are never valid OAuth redirect targets. The `@better-auth/oauth-provider` package already applied this check, so this change brings the two older plugins in line with it. + +The redirect-URI scheme policy now lives in `@better-auth/core` as a single `SafeUrlSchema` and an `isSafeUrlScheme` helper, and the OAuth provider plugins share that one implementation. The client navigation helpers (`redirectPlugin`, one-tap, and two-factor) also skip navigation when the target uses one of these schemes. + +The change is non-breaking. The `http`, `https`, loopback, and custom application schemes still register unchanged. Both `oidc-provider` and `mcp` are on the migration path to `@better-auth/oauth-provider`, which remains the route to its stricter HTTPS-or-loopback policy. diff --git a/.cspell/tech-terms.txt b/.cspell/tech-terms.txt index c9f7bd0f52..544a32c570 100644 --- a/.cspell/tech-terms.txt +++ b/.cspell/tech-terms.txt @@ -65,3 +65,4 @@ Oligo febf fdff fffe +vbscript diff --git a/packages/better-auth/src/client/fetch-plugins.ts b/packages/better-auth/src/client/fetch-plugins.ts index 8096e0aae5..9c16c1bba6 100644 --- a/packages/better-auth/src/client/fetch-plugins.ts +++ b/packages/better-auth/src/client/fetch-plugins.ts @@ -1,3 +1,4 @@ +import { isSafeUrlScheme } from "@better-auth/core/utils/url"; import type { BetterFetchPlugin } from "@better-fetch/fetch"; export const redirectPlugin = { @@ -5,7 +6,11 @@ export const redirectPlugin = { name: "Redirect", hooks: { onSuccess(context) { - if (context.data?.url && context.data?.redirect) { + if ( + context.data?.url && + context.data?.redirect && + isSafeUrlScheme(context.data.url) + ) { if (typeof window !== "undefined" && window.location) { if (window.location) { try { diff --git a/packages/better-auth/src/plugins/mcp/index.ts b/packages/better-auth/src/plugins/mcp/index.ts index 9d985fe451..9b8f8c0759 100644 --- a/packages/better-auth/src/plugins/mcp/index.ts +++ b/packages/better-auth/src/plugins/mcp/index.ts @@ -9,6 +9,7 @@ import { } from "@better-auth/core/api"; import { isProduction, logger } from "@better-auth/core/env"; import { safeJSONParse } from "@better-auth/core/utils/json"; +import { isSafeUrlScheme } from "@better-auth/core/utils/url"; import { getWebcryptoSubtle } from "@better-auth/utils"; import { base64 } from "@better-auth/utils/base64"; import { createHash } from "@better-auth/utils/hash"; @@ -129,7 +130,16 @@ export const getMCPProtectedResourceMetadata = ( }; const registerMcpClientBodySchema = z.object({ - redirect_uris: z.array(z.string()), + // This plugin is migrating to @better-auth/oauth-provider (see the deprecation + // notice in docs/plugins/mcp). It gets only the non-breaking guard that rejects + // code-execution schemes here; full https-or-loopback parity comes from + // @better-auth/oauth-provider's SafeUrlSchema, not from tightening this plugin. + redirect_uris: z.array( + z.string().refine(isSafeUrlScheme, { + message: + "redirect_uri cannot use a javascript:, data:, or vbscript: scheme", + }), + ), token_endpoint_auth_method: z .enum(["none", "client_secret_basic", "client_secret_post"]) .default("client_secret_basic") diff --git a/packages/better-auth/src/plugins/oidc-provider/index.ts b/packages/better-auth/src/plugins/oidc-provider/index.ts index 3d45c7c62b..87e46447c1 100644 --- a/packages/better-auth/src/plugins/oidc-provider/index.ts +++ b/packages/better-auth/src/plugins/oidc-provider/index.ts @@ -8,6 +8,7 @@ import { } from "@better-auth/core/api"; import { getCurrentAuthContext } from "@better-auth/core/context"; import { deprecate } from "@better-auth/core/utils/deprecate"; +import { isSafeUrlScheme } from "@better-auth/core/utils/url"; import { base64 } from "@better-auth/utils/base64"; import { createHash } from "@better-auth/utils/hash"; import type { OpenAPIParameter } from "better-call"; @@ -144,10 +145,22 @@ const oAuthConsentBodySchema = z.object({ const oAuth2TokenBodySchema = z.record(z.any(), z.any()); const registerOAuthApplicationBodySchema = z.object({ - redirect_uris: z.array(z.string()).meta({ - description: - 'A list of redirect URIs. Eg: ["https://client.example.com/callback"]', - }), + // This plugin is deprecated and removed in the next major. It gets only the + // non-breaking guard that rejects code-execution schemes (javascript:, data:, + // vbscript:). The stricter https-or-loopback policy lives in + // @better-auth/oauth-provider via SafeUrlSchema; migrating there is the path to + // full parity, so we do not tighten this plugin further. + redirect_uris: z + .array( + z.string().refine(isSafeUrlScheme, { + message: + "redirect_uri cannot use a javascript:, data:, or vbscript: scheme", + }), + ) + .meta({ + description: + 'A list of redirect URIs. Eg: ["https://client.example.com/callback"]', + }), token_endpoint_auth_method: z .enum(["none", "client_secret_basic", "client_secret_post"]) .meta({ diff --git a/packages/better-auth/src/plugins/oidc-provider/redirect-uri.test.ts b/packages/better-auth/src/plugins/oidc-provider/redirect-uri.test.ts new file mode 100644 index 0000000000..8ed6272e65 --- /dev/null +++ b/packages/better-auth/src/plugins/oidc-provider/redirect-uri.test.ts @@ -0,0 +1,57 @@ +/** + * @see https://github.com/better-auth/better-auth/security/advisories/GHSA-86j7-9j95-vpqj + */ +import { describe, expect, it } from "vitest"; +import { createAuthClient } from "../../client"; +import { getTestInstance } from "../../test-utils/test-instance"; +import { oidcProvider } from "."; +import { oidcClient } from "./client"; + +async function getClient() { + const { customFetchImpl, signInWithTestUser } = await getTestInstance({ + baseURL: "http://localhost:3000", + plugins: [ + oidcProvider({ + loginPage: "/login", + consentPage: "/consent", + }), + ], + }); + const { headers } = await signInWithTestUser(); + return createAuthClient({ + plugins: [oidcClient()], + baseURL: "http://localhost:3000", + fetchOptions: { customFetchImpl, headers }, + }); +} + +describe("oidc-provider redirect_uri scheme validation", () => { + it("rejects javascript:, data:, and vbscript: redirect URIs at registration", async () => { + const client = await getClient(); + for (const uri of [ + "javascript:fetch('/api/auth/get-session')//", + "data:text/html,", + "vbscript:msgbox(1)", + ]) { + const reg = await client.oauth2.register({ + client_name: "Evil App", + redirect_uris: [uri], + }); + expect(reg.error?.status).toBe(400); + expect(reg.data?.client_id).toBeUndefined(); + } + }); + + it("still accepts https and loopback http redirect URIs", async () => { + const client = await getClient(); + const reg = await client.oauth2.register({ + client_name: "Good App", + redirect_uris: [ + "https://client.example.com/callback", + "http://localhost:3000/callback", + ], + }); + expect(reg.error).toBeNull(); + expect(reg.data?.client_id).toBeDefined(); + }); +}); diff --git a/packages/better-auth/src/plugins/one-tap/client.ts b/packages/better-auth/src/plugins/one-tap/client.ts index 064c1c5298..8a66bb149a 100644 --- a/packages/better-auth/src/plugins/one-tap/client.ts +++ b/packages/better-auth/src/plugins/one-tap/client.ts @@ -3,6 +3,7 @@ import type { BetterAuthClientPlugin, ClientFetchOption, } from "@better-auth/core"; +import { isSafeUrlScheme } from "@better-auth/core/utils/url"; import { PACKAGE_VERSION } from "../../version"; declare global { @@ -257,7 +258,10 @@ export const oneTapClient = (options: GoogleOneTapOptions) => { }); if ((!opts?.fetchOptions && !fetchOptions) || opts?.callbackURL) { - window.location.href = opts?.callbackURL ?? "/"; + const target = opts?.callbackURL ?? "/"; + if (isSafeUrlScheme(target)) { + window.location.href = target; + } } } @@ -303,7 +307,10 @@ export const oneTapClient = (options: GoogleOneTapOptions) => { }); if ((!opts?.fetchOptions && !fetchOptions) || opts?.callbackURL) { - window.location.href = opts?.callbackURL ?? "/"; + const target = opts?.callbackURL ?? "/"; + if (isSafeUrlScheme(target)) { + window.location.href = target; + } } } diff --git a/packages/better-auth/src/plugins/two-factor/client.ts b/packages/better-auth/src/plugins/two-factor/client.ts index 0173e36ce1..5310b7f7ca 100644 --- a/packages/better-auth/src/plugins/two-factor/client.ts +++ b/packages/better-auth/src/plugins/two-factor/client.ts @@ -1,4 +1,5 @@ import type { BetterAuthClientPlugin } from "@better-auth/core"; +import { isSafeUrlScheme } from "@better-auth/core/utils/url"; import { PACKAGE_VERSION } from "../../version"; import type { twoFactor as twoFa } from "."; import { TWO_FACTOR_ERROR_CODES } from "./error-code"; @@ -68,7 +69,11 @@ export const twoFactorClient = ( } // fallback for when `onTwoFactorRedirect` is not used and only `twoFactorPage` is provided - if (options?.twoFactorPage && typeof window !== "undefined") { + if ( + options?.twoFactorPage && + typeof window !== "undefined" && + isSafeUrlScheme(options.twoFactorPage) + ) { window.location.href = options.twoFactorPage; } } diff --git a/packages/core/src/utils/redirect-uri.ts b/packages/core/src/utils/redirect-uri.ts new file mode 100644 index 0000000000..ec087eeb1d --- /dev/null +++ b/packages/core/src/utils/redirect-uri.ts @@ -0,0 +1,45 @@ +import * as z from "zod"; +import { isLoopbackHost } from "./host"; +import { DANGEROUS_URL_SCHEMES } from "./url"; + +/** + * Zod schema for OAuth redirect URIs and other developer-supplied URLs that the + * server stores and later hands back to a browser. + * + * - Rejects dangerous schemes (`javascript:`, `data:`, `vbscript:`). + * - Requires HTTPS, except for loopback hosts (`127.0.0.0/8`, `[::1]`, + * `*.localhost` per RFC 6761), where HTTP is allowed for local development. + * - Allows custom schemes for mobile apps (e.g. `myapp://callback`). + * + * This is the single source of truth for redirect-URI validation across the + * OAuth provider plugins. Consume it from `@better-auth/core/utils/redirect-uri` + * rather than re-implementing the scheme policy per plugin. + */ +export const SafeUrlSchema = z.url().superRefine((val, ctx) => { + if (!URL.canParse(val)) { + ctx.addIssue({ + code: "custom", + message: "URL must be parseable", + fatal: true, + }); + return z.NEVER; + } + + const u = new URL(val); + + if (DANGEROUS_URL_SCHEMES.includes(u.protocol)) { + ctx.addIssue({ + code: "custom", + message: "URL cannot use javascript:, data:, or vbscript: scheme", + }); + return; + } + + if (u.protocol === "http:" && !isLoopbackHost(u.host)) { + ctx.addIssue({ + code: "custom", + message: + "Redirect URI must use HTTPS (HTTP allowed only for loopback hosts)", + }); + } +}); diff --git a/packages/core/src/utils/url.test.ts b/packages/core/src/utils/url.test.ts new file mode 100644 index 0000000000..cd9a882e7b --- /dev/null +++ b/packages/core/src/utils/url.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from "vitest"; +import { SafeUrlSchema } from "./redirect-uri"; +import { isSafeUrlScheme } from "./url"; + +describe("isSafeUrlScheme", () => { + it("rejects code-execution schemes", () => { + expect(isSafeUrlScheme("javascript:alert(1)")).toBe(false); + expect(isSafeUrlScheme("data:text/html,")).toBe( + false, + ); + expect(isSafeUrlScheme("vbscript:msgbox(1)")).toBe(false); + }); + + it("normalizes the scheme before checking (mixed case is still blocked)", () => { + expect(isSafeUrlScheme("JavaScript:alert(1)")).toBe(false); + expect(isSafeUrlScheme("JAVASCRIPT:alert(1)")).toBe(false); + }); + + it("allows http(s), relative paths, and custom app schemes", () => { + expect(isSafeUrlScheme("https://example.com/callback")).toBe(true); + expect(isSafeUrlScheme("http://localhost:3000/callback")).toBe(true); + expect(isSafeUrlScheme("/dashboard")).toBe(true); + expect(isSafeUrlScheme("myapp://callback")).toBe(true); + }); +}); + +describe("SafeUrlSchema", () => { + it("rejects dangerous schemes", () => { + expect(SafeUrlSchema.safeParse("javascript:alert(1)").success).toBe(false); + expect(SafeUrlSchema.safeParse("data:text/html,x").success).toBe(false); + expect(SafeUrlSchema.safeParse("vbscript:x").success).toBe(false); + }); + + it("requires https for non-loopback hosts", () => { + expect(SafeUrlSchema.safeParse("http://example.com/cb").success).toBe( + false, + ); + expect(SafeUrlSchema.safeParse("https://example.com/cb").success).toBe( + true, + ); + }); + + it("allows http for loopback hosts", () => { + expect(SafeUrlSchema.safeParse("http://localhost:3000/cb").success).toBe( + true, + ); + expect(SafeUrlSchema.safeParse("http://127.0.0.1/cb").success).toBe(true); + }); +}); diff --git a/packages/core/src/utils/url.ts b/packages/core/src/utils/url.ts index a895ef4ccf..da85d4b4fc 100644 --- a/packages/core/src/utils/url.ts +++ b/packages/core/src/utils/url.ts @@ -41,3 +41,28 @@ export function normalizePathname( return pathname; } + +/** + * Schemes that execute or embed code when navigated to or accepted as a + * redirect target. These are never safe as an OAuth `redirect_uri` or as a + * client-side navigation target (`window.location.href`, `location.assign`, ...). + */ +export const DANGEROUS_URL_SCHEMES = ["javascript:", "data:", "vbscript:"]; + +/** + * Returns `false` only when `value` is an absolute URL using a dangerous scheme + * (`javascript:`, `data:`, `vbscript:`). Relative URLs (e.g. `/dashboard`) and + * safe absolute schemes (`http`, `https`, custom app schemes such as + * `myapp://`) return `true`. + * + * Use this to guard browser navigation sinks and any redirect target that may + * originate from untrusted input. It is intentionally narrow: it blocks code + * execution schemes without rejecting relative paths or mobile deep links. + */ +export function isSafeUrlScheme(value: string): boolean { + if (!URL.canParse(value)) { + // Relative URLs carry no scheme to abuse. + return true; + } + return !DANGEROUS_URL_SCHEMES.includes(new URL(value).protocol); +} diff --git a/packages/oauth-provider/src/types/zod.ts b/packages/oauth-provider/src/types/zod.ts index a92d301cac..951160cc93 100644 --- a/packages/oauth-provider/src/types/zod.ts +++ b/packages/oauth-provider/src/types/zod.ts @@ -1,8 +1,5 @@ -import { isLoopbackHost } from "@better-auth/core/utils/host"; import * as z from "zod"; -const DANGEROUS_SCHEMES = ["javascript:", "data:", "vbscript:"]; - /** * Runtime schema for OAuthAuthorizationQuery. * Uses passthrough to tolerate fields added by future extensions (PAR, FPA, etc.) @@ -45,36 +42,7 @@ export const verificationValueSchema = z .passthrough(); /** - * Reusable URL validation for OAuth redirect URIs. - * - Blocks dangerous schemes (javascript:, data:, vbscript:) - * - For http/https: requires HTTPS (HTTP allowed only for loopback hosts: 127.0.0.0/8, [::1], *.localhost per RFC 6761) - * - Allows custom schemes for mobile apps (e.g., myapp://callback) + * Re-exported from `@better-auth/core` so every OAuth provider plugin shares one + * redirect-URI scheme policy. See `@better-auth/core/utils/redirect-uri`. */ -export const SafeUrlSchema = z.url().superRefine((val, ctx) => { - if (!URL.canParse(val)) { - ctx.addIssue({ - code: "custom", - message: "URL must be parseable", - fatal: true, - }); - return z.NEVER; - } - - const u = new URL(val); - - if (DANGEROUS_SCHEMES.includes(u.protocol)) { - ctx.addIssue({ - code: "custom", - message: "URL cannot use javascript:, data:, or vbscript: scheme", - }); - return; - } - - if (u.protocol === "http:" && !isLoopbackHost(u.host)) { - ctx.addIssue({ - code: "custom", - message: - "Redirect URI must use HTTPS (HTTP allowed only for loopback hosts)", - }); - } -}); +export { SafeUrlSchema } from "@better-auth/core/utils/redirect-uri";