From a1c5cfaf759d040b6c535ea2a1387427c77c063a Mon Sep 17 00:00:00 2001 From: Niklas Wallerstedt Date: Tue, 21 Oct 2025 18:09:59 +0200 Subject: [PATCH] fix: resolve redirect URI security vulnerability ([#299](https://github.com/sst/openauth/issues/299)) Fix malformed URL bypass where for instance redirect_uri=https:evil.com could redirect to external domains. Returns HTTP 400 for invalid redirect URIs based on default allow hook. --- bun.lockb | Bin 255216 -> 255216 bytes packages/openauth/src/issuer.ts | 52 ++- packages/openauth/test/issuer.test.ts | 89 +++++ .../test/redirect-uri-validation.test.ts | 328 ++++++++++++++++++ 4 files changed, 456 insertions(+), 13 deletions(-) create mode 100644 packages/openauth/test/redirect-uri-validation.test.ts diff --git a/bun.lockb b/bun.lockb index 7f4b865820733fdfc374b2177bd115d17dacfb6e..25c80f8d91fa07716b7b8ad1233502ef9a212c6e 100755 GIT binary patch delta 217 zcmexxlK;a={tXE(jAD}mHS{M3xNvalGchnoF)%bBL>Pf0@+cx#85tPl;35JYoE$&_ zL9l@0DvQlCUEP@{FYqv!JYhQLW=T(W14ib_3k}RSM^tifPgZEL;C#pcv4e5)LrwF| zH(ETHCqHPnV66cPi%q`UZp~>AWJv6m6rt(g0MswYpiR6FiZ^$aWb=Uz00CHtJ_NE+LUb}vHQcNM#?7Z% z!kFtVOm}=}VF4;tN(9qTLqG=SE8x<An } +export interface AllowCallbackInput { + clientID: string + redirectURI: string + audience?: string +} + /** * @internal */ @@ -416,26 +422,31 @@ export interface IssuerInput< * * - Allow if the `redirectURI` is localhost. * - Compare `redirectURI` to the request's hostname or the `x-forwarded-host` header. If they - * are from the same sub-domain level, then allow. + * share the same apex domain, then allow. + * + * :::caution[Security Notice] + * The default implementation allows ANY `redirect_uri` on the same apex domain with no per-client isolation. + * Consider implementing a custom `allow` function with strict per-client validation if your deployment has: + * - Untrusted content on subdomains (user-generated content, third-party scripts) + * - Potential XSS attack vectors + * - Multiple client applications requiring isolation + * ::: * * @example + * Recommended for production (per-client allowlist): * ```ts * { * allow: async (input, req) => { - * // Allow all clients - * return true + * const allowedRedirects = { + * 'web-client': ['https://app.example.com/callback'], + * 'mobile-client': ['https://admin.example.com/oauth'], + * } + * return allowedRedirects[input.clientID]?.includes(input.redirectURI) ?? false * } * } * ``` */ - allow?( - input: { - clientID: string - redirectURI: string - audience?: string - }, - req: Request, - ): Promise + allow?(input: AllowCallbackInput, req: Request): Promise } /** @@ -474,7 +485,7 @@ export function issuer< const allow = lazy( () => input.allow ?? - (async (input: any, req: Request) => { + (async (input: AllowCallbackInput, req: Request) => { const redir = new URL(input.redirectURI).hostname if (redir === "localhost" || redir === "127.0.0.1") { return true @@ -1032,7 +1043,6 @@ export function issuer< } : undefined, } as AuthorizationState - c.set("authorization", authorization) if (!redirect_uri) { return c.text("Missing redirect_uri", { status: 400 }) @@ -1062,6 +1072,7 @@ export function issuer< ) throw new UnauthorizedClientError(client_id, redirect_uri) await auth.set(c, "authorization", 60 * 60 * 24, authorization) + c.set("authorization", authorization) if (provider) return c.redirect(`/${provider}/authorize`) const providers = Object.keys(input.providers) if (providers.length === 1) return c.redirect(`/${providers[0]}/authorize`) @@ -1138,6 +1149,21 @@ export function issuer< app.onError(async (err, c) => { console.error(err) + + if (err instanceof UnauthorizedClientError) { + return c.json( + { error: err.error, error_description: err.description }, + 400, + ) + } + + if (err instanceof MissingParameterError) { + return c.json( + { error: err.error, error_description: err.description }, + 400, + ) + } + if (err instanceof UnknownStateError) { return auth.forward(c, await error(err, c.req.raw)) } diff --git a/packages/openauth/test/issuer.test.ts b/packages/openauth/test/issuer.test.ts index be303d77..b9814f45 100644 --- a/packages/openauth/test/issuer.test.ts +++ b/packages/openauth/test/issuer.test.ts @@ -119,6 +119,95 @@ describe("code flow", () => { }) }) +describe("error handling (same-request)", () => { + test("select() throws after authorization is set -> must redirect with OAuth error", async () => { + // Two entries pointing to the same dummy provider to trigger the select() UI + const multiProviderIssuer = issuer({ + ...issuerConfig, + providers: { + a: issuerConfig.providers.dummy, + b: issuerConfig.providers.dummy, + }, + // Force an error after state is set but before the response is returned + select: async () => { + throw new Error("boom") + }, + }) + + const client = createClient({ + issuer: "https://auth.example.com", + clientID: "web", + fetch: (a, b) => Promise.resolve(multiProviderIssuer.request(a, b)), + }) + + const { url } = await client.authorize( + "https://client.example.com/callback", + "code", + ) + + const res = await multiProviderIssuer.request(url) + + // Desired behavior: redirect to redirect_uri with server_error + expect(res.status).toBe(302) + const location = new URL(res.headers.get("location")!) + expect(location.origin + location.pathname).toBe( + "https://client.example.com/callback", + ) + expect(location.searchParams.get("error")).toBe("server_error") + }) +}) + +describe("authorization precedence", () => { + test("cookie wins over request-local when both are present", async () => { + // 1) Create a stale authorization cookie pointing to old.example.com + const staleIssuer = issuer(issuerConfig) + const staleClient = createClient({ + issuer: "https://auth.example.com", + clientID: "web", + fetch: (a, b) => Promise.resolve(staleIssuer.request(a, b)), + }) + const { url: staleUrl } = await staleClient.authorize( + "https://old.example.com/callback", + "code", + ) + const staleRes = await staleIssuer.request(staleUrl) + expect(staleRes.status).toBe(302) + const staleCookie = staleRes.headers.get("set-cookie")! + + // 2) In a new request, also create a fresh request-local authorization but throw before responding + // to trigger app.onError within the same request. Include the stale cookie in the request. + const throwingIssuer = issuer({ + ...issuerConfig, + providers: { + a: issuerConfig.providers.dummy, + b: issuerConfig.providers.dummy, + }, + select: async () => { + throw new Error("boom") + }, + }) + const freshClient = createClient({ + issuer: "https://auth.example.com", + clientID: "web", + fetch: (a, b) => Promise.resolve(throwingIssuer.request(a, b)), + }) + const { url: freshUrl } = await freshClient.authorize( + "https://new.example.com/callback", + "code", + ) + const res = await throwingIssuer.request(freshUrl, { + headers: { cookie: staleCookie }, + }) + + // If cookie has precedence, we should be redirected to the OLD callback + expect(res.status).toBe(302) + const location = new URL(res.headers.get("location")!) + expect(location.origin + location.pathname).toBe( + "https://old.example.com/callback", + ) + }) +}) + describe("client credentials flow", () => { test("success", async () => { const client = createClient({ diff --git a/packages/openauth/test/redirect-uri-validation.test.ts b/packages/openauth/test/redirect-uri-validation.test.ts new file mode 100644 index 00000000..d3ae47fb --- /dev/null +++ b/packages/openauth/test/redirect-uri-validation.test.ts @@ -0,0 +1,328 @@ +import { expect, test, describe } from "bun:test" +import { object, string } from "valibot" +import { issuer } from "../src/issuer.js" +import { createSubjects } from "../src/subject.js" +import { MemoryStorage } from "../src/storage/memory.js" +import { Provider } from "../src/provider/provider.js" + +const subjects = createSubjects({ + user: object({ + userID: string(), + }), +}) + +const storage = MemoryStorage() + +const auth = issuer({ + storage, + subjects, + // Uses default allow function + providers: { + dummy: { + type: "dummy", + init(route, ctx) { + route.get("/authorize", async (c) => { + return ctx.success(c, { + email: "foo@bar.com", + }) + }) + }, + } satisfies Provider<{ email: string }>, + }, + success: async (ctx, value) => { + if (value.provider === "dummy") { + return ctx.subject("user", { + userID: "123", + }) + } + throw new Error("Invalid provider: " + value.provider) + }, +}) + +describe("redirect_uri validation", () => { + test("does not redirect when response_type is missing", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("redirect_uri", "https:example.com") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("does not redirect when client_id is missing (code flow)", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("redirect_uri", "https:example.com") + url.searchParams.set("response_type", "code") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("does not redirect when response_type is missing but client_id present", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("redirect_uri", "https:example.com") + url.searchParams.set("client_id", "web") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("allows same-level subdomain within same apex domain", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://sub.example.com/callback") + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + const redirectTo = response.headers.get("location")! + response = await auth.request(redirectTo, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://sub.example.com") + expect(location.pathname).toBe("/callback") + expect(location.hash).toContain("access_token=") + }) + + test("rejects malformed scheme-like redirect https:evil.com", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https:evil.com") + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + const response = await auth.request(url.toString()) + // Must not redirect; malformed redirect URIs are invalid + expect(response.status).toBe(400) + }) + + test("allows same-level subdomain in code flow", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://sub.example.com/callback") + url.searchParams.set("response_type", "code") + url.searchParams.set("state", "test123") + url.searchParams.set("provider", "dummy") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + response = await auth.request(response.headers.get("location")!, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://sub.example.com") + expect(location.searchParams.has("code")).toBe(true) + }) + + test("rejects malformed scheme-like redirect https:evil.com (code flow)", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https:evil.com") + url.searchParams.set("response_type", "code") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("rejects external domain redirect (code flow)", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://evil.com/steal") + url.searchParams.set("response_type", "code") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("rejects external domain redirect", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://evil.com/steal") + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + const response = await auth.request(url.toString()) + expect(response.status).toBe(400) + }) + + test("allows different subdomain level within same apex domain", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set( + "redirect_uri", + "https://deep.sub.example.com/callback", + ) + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + const redirectTo = response.headers.get("location")! + response = await auth.request(redirectTo, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://deep.sub.example.com") + expect(location.pathname).toBe("/callback") + expect(location.hash).toContain("access_token=") + }) + + test("allows apex within same apex domain", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://example.com/callback") + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + + const redirectTo = response.headers.get("location")! + response = await auth.request(redirectTo, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://example.com") + expect(location.pathname).toBe("/callback") + expect(location.hash).toContain("access_token=") + }) + + test("allows apex within same apex domain (code flow)", async () => { + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://example.com/callback") + url.searchParams.set("response_type", "code") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + response = await auth.request(response.headers.get("location")!, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://example.com") + expect(location.pathname).toBe("/callback") + expect(location.searchParams.has("code")).toBe(true) + }) + + test("respects x-forwarded-host when deciding allow", async () => { + // Request URL host differs from forwarded host; allow should use x-forwarded-host + const url = new URL("https://internal.local/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://app.example.com/callback") + url.searchParams.set("response_type", "token") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString(), { + headers: { + "x-forwarded-host": "auth.example.com", + }, + }) + expect(response.status).toBe(302) + const redirectTo = response.headers.get("location")! + response = await auth.request(redirectTo, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://app.example.com") + expect(location.pathname).toBe("/callback") + expect(location.hash).toContain("access_token=") + }) + + test("respects x-forwarded-host (code flow)", async () => { + const url = new URL("https://internal.local/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://app.example.com/callback") + url.searchParams.set("response_type", "code") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString(), { + headers: { + "x-forwarded-host": "auth.example.com", + }, + }) + expect(response.status).toBe(302) + response = await auth.request(response.headers.get("location")!, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const location = new URL(response.headers.get("location")!) + expect(location.origin).toBe("https://app.example.com") + expect(location.pathname).toBe("/callback") + expect(location.searchParams.has("code")).toBe(true) + }) + + test("code flow requires same redirect_uri on token exchange", async () => { + // Authorize with one redirect_uri + const url = new URL("https://auth.example.com/authorize") + url.searchParams.set("client_id", "web") + url.searchParams.set("redirect_uri", "https://app.example.com/callback") + url.searchParams.set("response_type", "code") + url.searchParams.set("provider", "dummy") + url.searchParams.set("state", "test123") + + let response = await auth.request(url.toString()) + expect(response.status).toBe(302) + response = await auth.request(response.headers.get("location")!, { + headers: { + cookie: response.headers.get("set-cookie")!, + }, + }) + expect(response.status).toBe(302) + const final = new URL(response.headers.get("location")!) + const code = final.searchParams.get("code")! + + // Exchange with a different redirect_uri (same host, different path) + const tokenRes = await auth.request("https://auth.example.com/token", { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + body: new URLSearchParams({ + grant_type: "authorization_code", + code, + client_id: "web", + redirect_uri: "https://app.example.com/other", + }).toString(), + }) + expect(tokenRes.status).toBe(400) + const body = await tokenRes.json() + expect(body).toStrictEqual({ + error: "invalid_redirect_uri", + error_description: "Redirect URI mismatch", + }) + }) +})