From 1dbf5bb59de5d628f0d07d5e846eba8287b831d7 Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Wed, 10 Jun 2026 20:50:35 -0700 Subject: [PATCH] fix: harden trusted request context (#9990) --- .changeset/trusted-request-context.md | 7 + demo/expo/src/lib/auth.ts | 6 +- demo/nextjs/lib/auth.ts | 2 +- .../src/api/middlewares/origin-check.test.ts | 194 ++++++++++++++++++ .../src/api/middlewares/origin-check.ts | 24 ++- .../better-auth/src/api/rate-limiter/index.ts | 28 ++- .../src/api/rate-limiter/rate-limiter.test.ts | 35 +++- packages/better-auth/src/auth/base.ts | 35 ++-- .../src/plugins/captcha/captcha.test.ts | 93 +++++++++ .../better-auth/src/plugins/captcha/index.ts | 8 +- .../better-auth/src/plugins/captcha/types.ts | 21 ++ .../verify-handlers/cloudflare-turnstile.ts | 28 ++- .../verify-handlers/google-recaptcha.ts | 32 ++- .../plugins/oauth-proxy/oauth-proxy.test.ts | 120 ++++++++++- .../src/plugins/oauth-proxy/utils.ts | 32 ++- .../better-auth/src/plugins/one-tap/client.ts | 20 +- .../better-auth/src/plugins/one-tap/index.ts | 11 + .../src/plugins/one-tap/one-tap.test.ts | 48 +++++ packages/core/src/utils/host.test.ts | 34 +++ packages/core/src/utils/host.ts | 15 ++ packages/core/src/utils/url.test.ts | 41 +++- packages/core/src/utils/url.ts | 14 +- packages/expo/src/routes.ts | 39 +++- packages/expo/test/expo.test.ts | 56 +++++ 24 files changed, 887 insertions(+), 56 deletions(-) create mode 100644 .changeset/trusted-request-context.md diff --git a/.changeset/trusted-request-context.md b/.changeset/trusted-request-context.md new file mode 100644 index 0000000000..a4fd571570 --- /dev/null +++ b/.changeset/trusted-request-context.md @@ -0,0 +1,7 @@ +--- +"better-auth": patch +"@better-auth/core": patch +"@better-auth/expo": patch +--- + +Hardens how requests are trusted across several flows. Rate limiting is now enforced even when a client IP cannot be determined, instead of being skipped. When `baseURL` is not configured, password-reset and verification links use the current request's host rather than the host of the first request the server handled, and a request-scoped `trustedOrigins` callback no longer affects other concurrent requests. The OAuth proxy, Google One Tap, and the Expo authorization proxy reject redirect and callback targets that are not in `trustedOrigins`. Google reCAPTCHA and Cloudflare Turnstile accept optional `expectedAction` and `allowedHostnames` to reject tokens minted for a different action or hostname. Server-side fetches reject additional reserved IPv6 ranges, and malformed redirect parameters return a 400 instead of a 500. diff --git a/demo/expo/src/lib/auth.ts b/demo/expo/src/lib/auth.ts index 77d7d357cd..08a097e749 100644 --- a/demo/expo/src/lib/auth.ts +++ b/demo/expo/src/lib/auth.ts @@ -20,5 +20,9 @@ export const auth = betterAuth({ clientSecret: process.env.GOOGLE_CLIENT_SECRET!, }, }, - trustedOrigins: ["exp://"], + // Trust the app-specific scheme only. The expo() plugin adds the broad + // `exp://` development scheme automatically when NODE_ENV is development; + // trusting it in production could leak the session cookie to a deep link + // the app does not control. + trustedOrigins: ["better-auth://"], }); diff --git a/demo/nextjs/lib/auth.ts b/demo/nextjs/lib/auth.ts index d2e7adeb35..63d18ff22f 100644 --- a/demo/nextjs/lib/auth.ts +++ b/demo/nextjs/lib/auth.ts @@ -440,7 +440,7 @@ const authOptions = { trustedOrigins: [ "https://*.better-auth.com", "https://better-auth-demo-*-better-auth.vercel.app", - "exp://", + "better-auth://", "com.better-auth.demo:/", "https://appleid.apple.com", ], diff --git a/packages/better-auth/src/api/middlewares/origin-check.test.ts b/packages/better-auth/src/api/middlewares/origin-check.test.ts index a1337ad1e5..a38ad58fbe 100644 --- a/packages/better-auth/src/api/middlewares/origin-check.test.ts +++ b/packages/better-auth/src/api/middlewares/origin-check.test.ts @@ -654,6 +654,98 @@ describe("origin check middleware", async () => { ); expect(blocked.error?.status).toBe(403); }); + + it("should not skip origin check for a path that only shares a prefix with a skip path", async () => { + const { client } = await getTestInstance({ + trustedOrigins: ["https://trusted-site.com"], + advanced: { + disableOriginCheck: false, + }, + plugins: [ + { + id: "test", + init() { + return { + context: { + skipOriginCheck: ["/public/data"], + }, + }; + }, + endpoints: { + siblingEndpoint: createAuthEndpoint( + "/public/database", + { + method: "GET", + query: z.object({ + callbackURL: z.string(), + }), + use: [originCheck((c) => c.query.callbackURL)], + }, + async (c) => c.query.callbackURL, + ), + }, + }, + ], + }); + + // "/public/database" only shares a prefix with the configured skip path + // "/public/data"; it must still have its callbackURL validated. + const blocked = await client.$fetch( + "/public/database?callbackURL=https://malicious.com", + ); + expect(blocked.error?.status).toBe(403); + }); + + it("should reject a non-string redirect parameter with 400, not 500", async () => { + const { auth } = await getTestInstance({ + trustedOrigins: ["http://localhost:3000"], + emailAndPassword: { + enabled: true, + }, + advanced: { + disableCSRFCheck: false, + disableOriginCheck: false, + }, + }); + + // An object callbackURL reaches the global origin-check middleware before + // endpoint schema validation; it must produce a controlled 400. + const objectBodyRequest = new Request( + "http://localhost:3000/api/auth/sign-in/email", + { + method: "POST", + headers: { + "content-type": "application/json", + origin: "http://localhost:3000", + }, + body: JSON.stringify({ + email: "test@test.com", + password: "password12345", + callbackURL: { object: true }, + }), + }, + ); + const objectRes = await auth.handler(objectBodyRequest); + expect(objectRes.status).toBe(400); + + // Duplicate query callbackURL parameters arrive as an array. + const duplicateQueryRequest = new Request( + "http://localhost:3000/api/auth/sign-in/email?callbackURL=/a&callbackURL=/b", + { + method: "POST", + headers: { + "content-type": "application/json", + origin: "http://localhost:3000", + }, + body: JSON.stringify({ + email: "test@test.com", + password: "password12345", + }), + }, + ); + const arrayRes = await auth.handler(duplicateQueryRequest); + expect(arrayRes.status).toBe(400); + }); }); describe("trusted origins with baseURL inferred from request", async () => { @@ -1069,3 +1161,105 @@ describe("disableCSRFCheck and disableOriginCheck separation", async () => { expect(res.data?.user).toBeDefined(); }); }); + +describe("request-scoped trusted origin isolation", async () => { + it("does not let one request's trusted origins bleed into a concurrent request", async () => { + let releaseA!: () => void; + const aPaused = new Promise((resolve) => { + releaseA = resolve; + }); + let signalAtPause!: () => void; + const aReachedPause = new Promise((resolve) => { + signalAtPause = resolve; + }); + + const { auth } = await getTestInstance({ + baseURL: "http://localhost:3000", + emailAndPassword: { enabled: true }, + advanced: { disableCSRFCheck: false, disableOriginCheck: false }, + trustedOrigins: (request) => + request?.headers.get("x-tenant") === "a" + ? ["https://a.example"] + : ["https://b.example"], + plugins: [ + { + id: "pause-tenant-a", + onRequest: async (request) => { + if (request.headers.get("x-tenant") === "a") { + signalAtPause(); + await aPaused; + } + return undefined; + }, + }, + ], + }); + + const makeRequest = (tenant: "a" | "b") => + new Request("http://localhost:3000/api/auth/sign-in/email", { + method: "POST", + headers: { + "content-type": "application/json", + "x-tenant": tenant, + // Tenant A carries tenant B's origin: only the shared-context + // bleed would let A trust it. + origin: "https://b.example", + cookie: "x=1", + }, + body: JSON.stringify({ + email: `${tenant}@example.com`, + password: "password1234", + }), + }); + + // Start A; it pauses inside onRequest after base.ts resolved A's origins. + const aResponsePromise = auth.handler(makeRequest("a")); + await aReachedPause; + // Run B to completion so it mutates any shared trust state. + await auth.handler(makeRequest("b")); + releaseA(); + + const aResponse = await aResponsePromise; + // A must reject tenant B's origin; b.example is not in A's trusted list. + expect(aResponse.status).toBe(403); + }); +}); + +describe("inferred baseURL is not persisted across requests", async () => { + it("does not reuse one request's host for a later request's token links", async () => { + const resetURLs: string[] = []; + const { auth, testUser } = await getTestInstance({ + baseURL: undefined, + emailAndPassword: { + enabled: true, + sendResetPassword: async ({ url }) => { + resetURLs.push(url); + }, + }, + }); + + // First request arrives from one host. + await auth.handler( + new Request("https://untrusted.example/api/auth/request-password-reset", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ email: testUser.email, redirectTo: "/" }), + }), + ); + + // A later request from the legitimate host must build its link from its + // own host, not the host carried by the first request. + await auth.handler( + new Request("https://app.example/api/auth/request-password-reset", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ email: testUser.email, redirectTo: "/" }), + }), + ); + + const legitURL = resetURLs.at(-1); + expect(legitURL).toBeDefined(); + expect(legitURL).toContain("https://app.example"); + expect(legitURL).not.toContain("untrusted.example"); + }); +}); diff --git a/packages/better-auth/src/api/middlewares/origin-check.ts b/packages/better-auth/src/api/middlewares/origin-check.ts index fe7ece5796..42a437ec86 100644 --- a/packages/better-auth/src/api/middlewares/origin-check.ts +++ b/packages/better-auth/src/api/middlewares/origin-check.ts @@ -31,9 +31,16 @@ function shouldSkipOriginCheck(ctx: GenericEndpointContext): boolean { try { const basePath = new URL(ctx.context.baseURL).pathname; const currentPath = normalizePathname(ctx.request.url, basePath); - return skipOriginCheck.some((skipPath) => - currentPath.startsWith(skipPath), - ); + // Match only an exact path or a slash-boundary child, so a configured + // skip path like "/public/data" does not also skip "/public/database" + // or "/public/data-delete" and silently disable origin/CSRF checks there. + return skipOriginCheck.some((skipPath) => { + const normalizedSkipPath = skipPath.replace(/\/+$/, ""); + return ( + currentPath === normalizedSkipPath || + currentPath.startsWith(`${normalizedSkipPath}/`) + ); + }); } catch { // } @@ -79,7 +86,7 @@ export const originCheckMiddleware = createAuthMiddleware(async (ctx) => { const newUserCallbackURL = body?.newUserCallbackURL; const validateURL = ( - url: string | undefined, + url: unknown, label: | "origin" | "callbackURL" @@ -90,6 +97,15 @@ export const originCheckMiddleware = createAuthMiddleware(async (ctx) => { if (!url) { return; } + // These values are read from the raw body/query before endpoint schema + // validation. A JSON object/array body or duplicate query parameter + // yields a non-string here; reject it as a controlled 400 instead of + // letting a string method throw an uncontrolled 500. Never String()-coerce. + if (typeof url !== "string") { + throw APIError.fromStatus("BAD_REQUEST", { + message: `Invalid ${label}: expected a string`, + }); + } const isTrustedOrigin = ctx.context.isTrustedOrigin(url, { allowRelativePaths: label !== "origin", }); diff --git a/packages/better-auth/src/api/rate-limiter/index.ts b/packages/better-auth/src/api/rate-limiter/index.ts index ed9b168d95..1f4d1314b4 100644 --- a/packages/better-auth/src/api/rate-limiter/index.ts +++ b/packages/better-auth/src/api/rate-limiter/index.ts @@ -159,23 +159,33 @@ function getRateLimitStorage( let ipWarningLogged = false; +// Sentinel IP segment for the shared rate-limit bucket used when no trusted +// client IP can be derived. It is not a valid IP, so it never collides with a +// real client IP key. +const NO_TRUSTED_IP_KEY = "no-trusted-ip"; + async function resolveRateLimitConfig(req: Request, ctx: AuthContext) { const basePath = new URL(ctx.baseURL).pathname; const path = normalizePathname(req.url, basePath); let currentWindow = ctx.rateLimit.window; let currentMax = ctx.rateLimit.max; const ip = getIp(req, ctx.options); - if (!ip) { - if (!ipWarningLogged) { - ctx.logger.warn( - "Rate limiting skipped: could not determine client IP address. " + - "Ensure your runtime forwards a trusted client IP header and configure `advanced.ipAddress.ipAddressHeaders` if needed.", - ); - ipWarningLogged = true; - } + if (!ip && ctx.options.advanced?.ipAddress?.disableIpTracking) { + // IP tracking is explicitly disabled; per-IP rate limiting does not apply. return null; } - const key = createRateLimitKey(ip, path); + if (!ip && !ipWarningLogged) { + ctx.logger.warn( + "Rate limiting could not determine a client IP and is falling back to a " + + "single shared per-path bucket. Ensure your runtime forwards a trusted " + + "client IP header and configure `advanced.ipAddress.ipAddressHeaders` if needed.", + ); + ipWarningLogged = true; + } + // Fail closed when no client IP can be derived: key on a shared per-path + // bucket and still enforce the limit, instead of skipping rate limiting + // entirely (which let a client omit the IP header to bypass the limit). + const key = createRateLimitKey(ip ?? NO_TRUSTED_IP_KEY, path); const specialRules = getDefaultSpecialRules(); const specialRule = specialRules.find((rule) => rule.pathMatcher(path)); diff --git a/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts b/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts index 8e49ea94d0..a8efc70251 100644 --- a/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts +++ b/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts @@ -322,8 +322,9 @@ describe("should work in development/test environment", () => { describe("missing client IP warning", () => { const warningMessage = - "Rate limiting skipped: could not determine client IP address. " + - "Ensure your runtime forwards a trusted client IP header and configure `advanced.ipAddress.ipAddressHeaders` if needed."; + "Rate limiting could not determine a client IP and is falling back to a " + + "single shared per-path bucket. Ensure your runtime forwards a trusted " + + "client IP header and configure `advanced.ipAddress.ipAddressHeaders` if needed."; afterEach(() => { vi.unstubAllEnvs(); @@ -362,6 +363,36 @@ describe("missing client IP warning", () => { ), ).toBe(false); }); + + it("should fail closed and still enforce the limit when no client IP is available", async () => { + vi.stubEnv("NODE_ENV", "production"); + vi.stubEnv("TEST", "false"); + vi.resetModules(); + + const { getTestInstance: getTestInstanceReloaded } = await import( + "../../test-utils/test-instance" + ); + const { client } = await getTestInstanceReloaded({ + rateLimit: { + enabled: true, + window: 10, + max: 3, + }, + }); + + // With no derivable client IP, requests share one per-path bucket and + // must still be limited rather than skipped (the previous fail-open let a + // client omit the IP header to bypass rate limiting entirely). + let limited = false; + for (let i = 0; i < 6; i++) { + const res = await client.getSession(); + if (res.error?.status === 429) { + limited = true; + break; + } + } + expect(limited).toBe(true); + }); }); describe("IPv6 address normalization and rate limiting", () => { diff --git a/packages/better-auth/src/auth/base.ts b/packages/better-auth/src/auth/base.ts index 5dc99d7a03..a55719cb3a 100644 --- a/packages/better-auth/src/auth/base.ts +++ b/packages/better-auth/src/auth/base.ts @@ -42,13 +42,18 @@ export const createBetterAuth = ( resolveDynamicTrustedProxyHeaders(ctx.options), ); } else { - handlerCtx = ctx; - // Static config with no baseURL: memoize on the shared ctx from - // the first request. A concurrent-first-requests race is - // harmless since both writes resolve to the same value. Cloning - // via `Object.create` (as the dynamic branch does) would break - // downstream references that depend on `ctx.options` being - // mutated in place. + // Resolve request-derived state on a per-request clone so it never + // mutates the shared context. This isolates a request-dependent + // `trustedOrigins`/`trustedProviders` callback from concurrent + // requests, and (for the no-baseURL case) stops the first request's + // host from being memoized onto the shared context, where it would + // be reused for every later request's token links. + handlerCtx = Object.create( + Object.getPrototypeOf(ctx), + Object.getOwnPropertyDescriptors(ctx), + ) as AuthContext; + + let trustOptions = ctx.options; if (!ctx.options.baseURL) { const baseURL = getBaseURL( undefined, @@ -57,21 +62,25 @@ export const createBetterAuth = ( undefined, ctx.options.advanced?.trustedProxyHeaders, ); - if (baseURL) { - ctx.baseURL = baseURL; - ctx.options.baseURL = getOrigin(ctx.baseURL) || undefined; - } else { + if (!baseURL) { throw new BetterAuthError( "Could not get base URL from request. Please provide a valid base URL.", ); } + handlerCtx.baseURL = baseURL; + handlerCtx.options = { + ...ctx.options, + baseURL: getOrigin(baseURL) || undefined, + }; + trustOptions = handlerCtx.options; } + handlerCtx.trustedOrigins = await getTrustedOrigins( - ctx.options, + trustOptions, request, ); handlerCtx.trustedProviders = await getTrustedProviders( - ctx.options, + trustOptions, request, ); } diff --git a/packages/better-auth/src/plugins/captcha/captcha.test.ts b/packages/better-auth/src/plugins/captcha/captcha.test.ts index da62b6dabe..685619b001 100644 --- a/packages/better-auth/src/plugins/captcha/captcha.test.ts +++ b/packages/better-auth/src/plugins/captcha/captcha.test.ts @@ -518,4 +518,97 @@ describe("captcha", async () => { }); }); }); + + describe("action and hostname binding", async () => { + it("rejects a Turnstile token whose action does not match expectedAction", async () => { + const { client } = await getTestInstance({ + plugins: [ + captcha({ + provider: "cloudflare-turnstile", + secretKey: "xx-secret-key", + expectedAction: "login", + }), + ], + }); + mockBetterFetch.mockResolvedValue({ + data: { success: true, action: "signup", hostname: "myapp.com" }, + }); + const res = await client.signIn.email({ + email: "test@test.com", + password: "test123456", + fetchOptions: { headers: { "x-captcha-response": "token" } }, + }); + expect(res.error?.status).toBe(403); + }); + + it("rejects a Turnstile token from a hostname outside allowedHostnames", async () => { + const { client } = await getTestInstance({ + plugins: [ + captcha({ + provider: "cloudflare-turnstile", + secretKey: "xx-secret-key", + allowedHostnames: ["myapp.com"], + }), + ], + }); + mockBetterFetch.mockResolvedValue({ + data: { success: true, hostname: "untrusted.example" }, + }); + const res = await client.signIn.email({ + email: "test@test.com", + password: "test123456", + fetchOptions: { headers: { "x-captcha-response": "token" } }, + }); + expect(res.error?.status).toBe(403); + }); + + it("rejects a reCAPTCHA v3 token whose action does not match expectedAction", async () => { + const { client } = await getTestInstance({ + plugins: [ + captcha({ + provider: "google-recaptcha", + secretKey: "xx-secret-key", + expectedAction: "login", + }), + ], + }); + mockBetterFetch.mockResolvedValue({ + data: { + success: true, + score: 0.9, + action: "signup", + hostname: "myapp.com", + challenge_ts: "2022-02-28T15:14:30.096Z", + }, + }); + const res = await client.signIn.email({ + email: "test@test.com", + password: "test123456", + fetchOptions: { headers: { "x-captcha-response": "token" } }, + }); + expect(res.error?.status).toBe(403); + }); + + it("accepts a token when action and hostname match", async () => { + const { client, testUser } = await getTestInstance({ + plugins: [ + captcha({ + provider: "cloudflare-turnstile", + secretKey: "xx-secret-key", + expectedAction: "login", + allowedHostnames: ["myapp.com"], + }), + ], + }); + mockBetterFetch.mockResolvedValue({ + data: { success: true, action: "login", hostname: "myapp.com" }, + }); + const res = await client.signIn.email({ + email: testUser.email, + password: testUser.password, + fetchOptions: { headers: { "x-captcha-response": "token" } }, + }); + expect(res.error?.status).not.toBe(403); + }); + }); }); diff --git a/packages/better-auth/src/plugins/captcha/index.ts b/packages/better-auth/src/plugins/captcha/index.ts index af8c767a97..0dc91d0e7d 100644 --- a/packages/better-auth/src/plugins/captcha/index.ts +++ b/packages/better-auth/src/plugins/captcha/index.ts @@ -86,13 +86,19 @@ export const captcha = (options: CaptchaOptions) => }; if (options.provider === Providers.CLOUDFLARE_TURNSTILE) { - return await verifyHandlers.cloudflareTurnstile(handlerParams); + return await verifyHandlers.cloudflareTurnstile({ + ...handlerParams, + expectedAction: options.expectedAction, + allowedHostnames: options.allowedHostnames, + }); } if (options.provider === Providers.GOOGLE_RECAPTCHA) { return await verifyHandlers.googleRecaptcha({ ...handlerParams, minScore: options.minScore, + expectedAction: options.expectedAction, + allowedHostnames: options.allowedHostnames, }); } diff --git a/packages/better-auth/src/plugins/captcha/types.ts b/packages/better-auth/src/plugins/captcha/types.ts index a3d191a5eb..d8cf1ec52a 100644 --- a/packages/better-auth/src/plugins/captcha/types.ts +++ b/packages/better-auth/src/plugins/captcha/types.ts @@ -11,10 +11,31 @@ export interface BaseCaptchaOptions { export interface GoogleRecaptchaOptions extends BaseCaptchaOptions { provider: typeof Providers.GOOGLE_RECAPTCHA; minScore?: number | undefined; + /** + * Expected reCAPTCHA v3 `action`. When set, a verification whose action does + * not match is rejected, preventing a token minted for another action on the + * same site key from being replayed against this endpoint. + */ + expectedAction?: string | undefined; + /** + * Allow-list of hostnames the token must have been issued for. When set, a + * verification reporting a different hostname is rejected. + */ + allowedHostnames?: string[] | undefined; } export interface CloudflareTurnstileOptions extends BaseCaptchaOptions { provider: typeof Providers.CLOUDFLARE_TURNSTILE; + /** + * Expected Turnstile `action`. When set, a verification whose action does + * not match is rejected, preventing cross-context token reuse. + */ + expectedAction?: string | undefined; + /** + * Allow-list of hostnames the token must have been issued for. When set, a + * verification reporting a different or missing hostname is rejected. + */ + allowedHostnames?: string[] | undefined; } export interface HCaptchaOptions extends BaseCaptchaOptions { diff --git a/packages/better-auth/src/plugins/captcha/verify-handlers/cloudflare-turnstile.ts b/packages/better-auth/src/plugins/captcha/verify-handlers/cloudflare-turnstile.ts index 4d6bc490f6..a6f8c3c9d4 100644 --- a/packages/better-auth/src/plugins/captcha/verify-handlers/cloudflare-turnstile.ts +++ b/packages/better-auth/src/plugins/captcha/verify-handlers/cloudflare-turnstile.ts @@ -7,6 +7,8 @@ type Params = { secretKey: string; captchaResponse: string; remoteIP?: string | undefined; + expectedAction?: string | undefined; + allowedHostnames?: string[] | undefined; }; type SiteVerifyResponse = { @@ -29,6 +31,8 @@ export const cloudflareTurnstile = async ({ captchaResponse, secretKey, remoteIP, + expectedAction, + allowedHostnames, }: Params) => { const response = await betterFetch(siteVerifyURL, { method: "POST", @@ -44,12 +48,32 @@ export const cloudflareTurnstile = async ({ throw new Error(INTERNAL_ERROR_CODES.SERVICE_UNAVAILABLE.message); } - if (!response.data.success) { - return middlewareResponse({ + const verificationFailed = () => + middlewareResponse({ message: EXTERNAL_ERROR_CODES.VERIFICATION_FAILED.message, code: EXTERNAL_ERROR_CODES.VERIFICATION_FAILED.code, status: 403, }); + + if (!response.data.success) { + return verificationFailed(); + } + + // When configured, bind the token to the expected action and to an + // allow-list of hostnames so a token issued for a different action or host + // (e.g. under a shared widget or "Any Hostname") cannot be reused here. + if (expectedAction && response.data.action !== expectedAction) { + return verificationFailed(); + } + if ( + allowedHostnames && + allowedHostnames.length > 0 && + !( + response.data.hostname && + allowedHostnames.includes(response.data.hostname) + ) + ) { + return verificationFailed(); } return undefined; diff --git a/packages/better-auth/src/plugins/captcha/verify-handlers/google-recaptcha.ts b/packages/better-auth/src/plugins/captcha/verify-handlers/google-recaptcha.ts index 8643a0f802..dc82fe91af 100644 --- a/packages/better-auth/src/plugins/captcha/verify-handlers/google-recaptcha.ts +++ b/packages/better-auth/src/plugins/captcha/verify-handlers/google-recaptcha.ts @@ -9,12 +9,15 @@ type Params = { captchaResponse: string; minScore?: number | undefined; remoteIP?: string | undefined; + expectedAction?: string | undefined; + allowedHostnames?: string[] | undefined; }; type SiteVerifyResponse = { success: boolean; challenge_ts: string; hostname: string; + action?: string | undefined; "error-codes": | Array< | "missing-input-secret" @@ -43,6 +46,8 @@ export const googleRecaptcha = async ({ secretKey, minScore = 0.5, remoteIP, + expectedAction, + allowedHostnames, }: Params) => { const response = await betterFetch( siteVerifyURL, @@ -61,15 +66,32 @@ export const googleRecaptcha = async ({ throw new Error(INTERNAL_ERROR_CODES.SERVICE_UNAVAILABLE.message); } - if ( - !response.data.success || - (isV3(response.data) && response.data.score < minScore) - ) { - return middlewareResponse({ + const verificationFailed = () => + middlewareResponse({ message: EXTERNAL_ERROR_CODES.VERIFICATION_FAILED.message, code: EXTERNAL_ERROR_CODES.VERIFICATION_FAILED.code, status: 403, }); + + if ( + !response.data.success || + (isV3(response.data) && response.data.score < minScore) + ) { + return verificationFailed(); + } + + // When configured, bind the token to the expected v3 action and to an + // allow-list of hostnames so a token minted for a different action or site + // cannot be replayed against this endpoint. + if (expectedAction && response.data.action !== expectedAction) { + return verificationFailed(); + } + if ( + allowedHostnames && + allowedHostnames.length > 0 && + !allowedHostnames.includes(response.data.hostname) + ) { + return verificationFailed(); } return undefined; diff --git a/packages/better-auth/src/plugins/oauth-proxy/oauth-proxy.test.ts b/packages/better-auth/src/plugins/oauth-proxy/oauth-proxy.test.ts index 0375a26a47..e9b6ed4d09 100644 --- a/packages/better-auth/src/plugins/oauth-proxy/oauth-proxy.test.ts +++ b/packages/better-auth/src/plugins/oauth-proxy/oauth-proxy.test.ts @@ -1,7 +1,15 @@ import type { GoogleProfile } from "@better-auth/core/social-providers"; import { HttpResponse, http } from "msw"; import { setupServer } from "msw/node"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + afterAll, + afterEach, + beforeAll, + describe, + expect, + it, + vi, +} from "vitest"; import { parseJSON } from "../../client/parser"; import { signJWT, symmetricDecrypt, symmetricEncrypt } from "../../crypto"; import { getTestInstance } from "../../test-utils/test-instance"; @@ -1632,3 +1640,113 @@ describe("oauth-proxy", async () => { }); }); }); + +describe("oauth-proxy current URL trust", () => { + it("does not use an untrusted request origin as the proxy callback receiver", async () => { + const { auth } = await getTestInstance({ + baseURL: "https://myapp.com", + plugins: [oAuthProxy({ productionURL: "https://login.myapp.com" })], + socialProviders: { + google: { clientId: "test", clientSecret: "test" }, + }, + }); + + // Sign-in initiated from a request host that is not + // a trusted origin. + const signInResponse = await auth.handler( + new Request("https://untrusted.example/api/auth/sign-in/social", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ provider: "google", callbackURL: "/dashboard" }), + }), + ); + const { url } = (await signInResponse.json()) as { url: string }; + const state = new URL(url).searchParams.get("state"); + + const callbackResponse = await auth.handler( + new Request( + `https://login.myapp.com/api/auth/callback/google?code=test&state=${state}`, + { method: "GET" }, + ), + ); + const location = callbackResponse.headers.get("location"); + expect(location).toBeTruthy(); + // Falls back to the configured base URL, never the untrusted request origin. + expect(location).not.toContain("untrusted.example"); + expect(location).toContain( + "https://myapp.com/api/auth/oauth-proxy-callback", + ); + }); + + it("uses an explicitly trusted request origin as the proxy callback receiver", async () => { + const { auth } = await getTestInstance({ + baseURL: "https://myapp.com", + trustedOrigins: ["https://preview.myapp.com"], + plugins: [oAuthProxy({ productionURL: "https://login.myapp.com" })], + socialProviders: { + google: { clientId: "test", clientSecret: "test" }, + }, + }); + + const signInResponse = await auth.handler( + new Request("https://preview.myapp.com/api/auth/sign-in/social", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ provider: "google", callbackURL: "/dashboard" }), + }), + ); + const { url } = (await signInResponse.json()) as { url: string }; + const state = new URL(url).searchParams.get("state"); + + const callbackResponse = await auth.handler( + new Request( + `https://login.myapp.com/api/auth/callback/google?code=test&state=${state}`, + { method: "GET" }, + ), + ); + const location = callbackResponse.headers.get("location"); + expect(location).toContain( + "https://preview.myapp.com/api/auth/oauth-proxy-callback", + ); + }); + + it("falls back to the base URL when the platform vendor value is not a URL", async () => { + // AWS Lambda / GCP / Azure expose a bare function name, not a URL. + vi.stubEnv("AWS_LAMBDA_FUNCTION_NAME", "my-lambda-function"); + try { + const { auth } = await getTestInstance({ + baseURL: "https://myapp.com", + plugins: [oAuthProxy({ productionURL: "https://login.myapp.com" })], + socialProviders: { + google: { clientId: "test", clientSecret: "test" }, + }, + }); + + const signInResponse = await auth.handler( + new Request("https://untrusted.example/api/auth/sign-in/social", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + provider: "google", + callbackURL: "/dashboard", + }), + }), + ); + const { url } = (await signInResponse.json()) as { url: string }; + const state = new URL(url).searchParams.get("state"); + + const callbackResponse = await auth.handler( + new Request( + `https://login.myapp.com/api/auth/callback/google?code=test&state=${state}`, + { method: "GET" }, + ), + ); + const location = callbackResponse.headers.get("location"); + expect(location).toContain( + "https://myapp.com/api/auth/oauth-proxy-callback", + ); + } finally { + vi.unstubAllEnvs(); + } + }); +}); diff --git a/packages/better-auth/src/plugins/oauth-proxy/utils.ts b/packages/better-auth/src/plugins/oauth-proxy/utils.ts index 37326559b9..4aee094da6 100644 --- a/packages/better-auth/src/plugins/oauth-proxy/utils.ts +++ b/packages/better-auth/src/plugins/oauth-proxy/utils.ts @@ -26,17 +26,39 @@ function getVendorBaseURL() { } /** - * Resolve the current URL from various sources + * Resolve the current URL from various sources. + * + * The request URL host can come from an untrusted source (`Host` / forwarded host), + * and this origin becomes the receiver for the encrypted OAuth profile replay. + * So a request-derived origin is only honored when it is an explicitly trusted + * origin; otherwise resolution falls back to the configured platform/base URL, + * never the raw request host. An explicit `opts.currentURL` and the vendor/base + * URLs are configured by the developer and trusted as-is. */ export function resolveCurrentURL( ctx: GenericEndpointContext, opts?: OAuthProxyOptions, ) { + if (opts?.currentURL) { + return new URL(opts.currentURL); + } + + const requestURL = ctx.request?.url; + if (requestURL) { + const origin = getOrigin(requestURL); + if (origin && ctx.context.isTrustedOrigin(origin)) { + return new URL(requestURL); + } + } + + // getVendorBaseURL() returns a full URL on some platforms (Vercel, Netlify, + // Render) but a bare function name on others (AWS Lambda, GCP, Azure). Only + // use it when it parses as a URL; otherwise fall back to the base URL. + const vendorBaseURL = getVendorBaseURL(); return new URL( - opts?.currentURL || - ctx.request?.url || - getVendorBaseURL() || - ctx.context.baseURL, + vendorBaseURL && getOrigin(vendorBaseURL) + ? vendorBaseURL + : ctx.context.baseURL, ); } diff --git a/packages/better-auth/src/plugins/one-tap/client.ts b/packages/better-auth/src/plugins/one-tap/client.ts index 8a66bb149a..e2b2802333 100644 --- a/packages/better-auth/src/plugins/one-tap/client.ts +++ b/packages/better-auth/src/plugins/one-tap/client.ts @@ -250,13 +250,19 @@ export const oneTapClient = (options: GoogleOneTapOptions) => { } async function callback(idToken: string) { - await $fetch("/one-tap/callback", { + const res = await $fetch("/one-tap/callback", { method: "POST", - body: { idToken }, + body: { idToken, callbackURL: opts?.callbackURL }, ...opts?.fetchOptions, ...fetchOptions, }); + // The server validates callbackURL against trustedOrigins; do + // not navigate if it rejected the request. + if (res?.error) { + return; + } + if ((!opts?.fetchOptions && !fetchOptions) || opts?.callbackURL) { const target = opts?.callbackURL ?? "/"; if (isSafeUrlScheme(target)) { @@ -299,13 +305,19 @@ export const oneTapClient = (options: GoogleOneTapOptions) => { } async function callback(idToken: string) { - await $fetch("/one-tap/callback", { + const res = await $fetch("/one-tap/callback", { method: "POST", - body: { idToken }, + body: { idToken, callbackURL: opts?.callbackURL }, ...opts?.fetchOptions, ...fetchOptions, }); + // The server validates callbackURL against trustedOrigins; do + // not navigate if it rejected the request. + if (res?.error) { + return; + } + if ((!opts?.fetchOptions && !fetchOptions) || opts?.callbackURL) { const target = opts?.callbackURL ?? "/"; if (isSafeUrlScheme(target)) { diff --git a/packages/better-auth/src/plugins/one-tap/index.ts b/packages/better-auth/src/plugins/one-tap/index.ts index 110841439a..52f3c432c6 100644 --- a/packages/better-auth/src/plugins/one-tap/index.ts +++ b/packages/better-auth/src/plugins/one-tap/index.ts @@ -38,6 +38,17 @@ const oneTapCallbackBodySchema = z.object({ description: "Google ID token, which the client obtains from the One Tap API", }), + /** + * Sent so the global origin-check middleware validates the post-login + * redirect target against `trustedOrigins`. Without it the client performs + * an unvalidated `window.location` redirect, which is an open redirect. + */ + callbackURL: z + .string() + .meta({ + description: "URL to redirect to after a successful sign-in", + }) + .optional(), }); export const oneTap = (options?: OneTapOptions | undefined) => diff --git a/packages/better-auth/src/plugins/one-tap/one-tap.test.ts b/packages/better-auth/src/plugins/one-tap/one-tap.test.ts index 018da4dce8..8968f39289 100644 --- a/packages/better-auth/src/plugins/one-tap/one-tap.test.ts +++ b/packages/better-auth/src/plugins/one-tap/one-tap.test.ts @@ -367,3 +367,51 @@ describe("one-tap implicit linking gate", async () => { expect(accounts.length).toBe(0); }); }); + +describe("one-tap callbackURL origin validation", async () => { + const googleProvider = { + clientId: "test-client", + clientSecret: "test-secret", + enabled: true, + }; + + it("rejects an untrusted callbackURL via the global origin check", async () => { + const { client } = await getTestInstance({ + socialProviders: { google: googleProvider }, + plugins: [oneTap()], + advanced: { disableOriginCheck: false }, + }); + + const res = await client.$fetch<{ + data: unknown; + error: { status: number } | null; + }>("/one-tap/callback", { + method: "POST", + body: { + idToken: "stub-id-token", + callbackURL: "https://untrusted.example/callback", + }, + }); + + expect(res.error?.status).toBe(403); + }); + + it("accepts a relative callbackURL (origin check passes)", async () => { + const { client } = await getTestInstance({ + socialProviders: { google: googleProvider }, + plugins: [oneTap()], + advanced: { disableOriginCheck: false }, + }); + + const res = await client.$fetch<{ + data: unknown; + error: { status: number } | null; + }>("/one-tap/callback", { + method: "POST", + body: { idToken: "stub-id-token", callbackURL: "/dashboard" }, + }); + + // The origin check must not block a same-app relative redirect target. + expect(res.error?.status).not.toBe(403); + }); +}); diff --git a/packages/core/src/utils/host.test.ts b/packages/core/src/utils/host.test.ts index 6c65599ad1..100f0a4e76 100644 --- a/packages/core/src/utils/host.test.ts +++ b/packages/core/src/utils/host.test.ts @@ -233,6 +233,40 @@ describe("Host Classification", () => { expect(classifyHost("2001:0:0:0:0:0:5601:5601").kind).toBe("reserved"); }); }); + + describe("IANA special-purpose ranges (not globally reachable)", () => { + // @see https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml + it("should flag 64:ff9b:1::/48 local-use translation (RFC 8215)", () => { + expect(classifyHost("64:ff9b:1::").kind).toBe("reserved"); + expect(classifyHost("64:ff9b:1::a9fe:a9fe").kind).toBe("reserved"); + expect(isPublicRoutableHost("64:ff9b:1::")).toBe(false); + }); + + it("should flag 2001:2::/48 benchmarking (RFC 5180)", () => { + expect(classifyHost("2001:2::").kind).toBe("benchmarking"); + expect(classifyHost("2001:2::1").kind).toBe("benchmarking"); + expect(isPublicRoutableHost("2001:2::1")).toBe(false); + }); + + it("should flag 3fff::/20 documentation (RFC 9637)", () => { + expect(classifyHost("3fff::").kind).toBe("documentation"); + expect(classifyHost("3fff:0fff::1").kind).toBe("documentation"); + expect(isPublicRoutableHost("3fff::")).toBe(false); + }); + + it("should flag 5f00::/16 SRv6 SIDs (RFC 9602)", () => { + expect(classifyHost("5f00::").kind).toBe("reserved"); + expect(classifyHost("5f00:abcd::1").kind).toBe("reserved"); + expect(isPublicRoutableHost("5f00::")).toBe(false); + }); + + it("should keep globally-reachable hosts outside these blocks public", () => { + // 2001:20::/28 (ORCHIDv2) is globally reachable; an address one + // /48 outside the benchmarking block must stay public. + expect(isPublicRoutableHost("2001:20::1")).toBe(true); + expect(classifyHost("2001:2:1::").kind).toBe("public"); + }); + }); }); describe("IPv4-Mapped IPv6 Handling", () => { diff --git a/packages/core/src/utils/host.ts b/packages/core/src/utils/host.ts index 6f6c6c3082..7cb06af81d 100644 --- a/packages/core/src/utils/host.ts +++ b/packages/core/src/utils/host.ts @@ -235,6 +235,10 @@ function classifyIPv6(expanded: string): HostKind { if (expanded.startsWith("2001:0db8:")) return "documentation"; + // 2001:2::/48 — Benchmarking (RFC 5180). A specific non-globally-reachable + // block inside the otherwise-mixed 2001::/23 protocol-assignments space. + if (expanded.startsWith("2001:0002:0000:")) return "benchmarking"; + if (expanded.startsWith("2002:")) { const embedded = extractEmbeddedIPv4(expanded, 1); if (embedded && classifyIPv4(embedded) !== "public") return "reserved"; @@ -247,6 +251,10 @@ function classifyIPv6(expanded: string): HostKind { return "reserved"; } + // 64:ff9b:1::/48 — Local-Use IPv4/IPv6 Translation (RFC 8215). Distinct from + // the well-known NAT64 /96 prefix above and not globally reachable. + if (expanded.startsWith("0064:ff9b:0001:")) return "reserved"; + if (expanded.startsWith("2001:0000:")) { const embedded = extractEmbeddedIPv4(expanded, 6, { xor: true }); if (embedded && classifyIPv4(embedded) !== "public") return "reserved"; @@ -255,6 +263,13 @@ function classifyIPv6(expanded: string): HostKind { if (expanded.startsWith("0100:0000:0000:0000:")) return "reserved"; + // 3fff::/20 — Documentation (RFC 9637). The /20 fixes the first 16 bits to + // `3fff` and the next nibble to 0, so only `3fff:0xxx` is in range. + if (expanded.startsWith("3fff:0")) return "documentation"; + + // 5f00::/16 — SRv6 SIDs (RFC 9602), not globally reachable. + if (expanded.startsWith("5f00:")) return "reserved"; + return "public"; } diff --git a/packages/core/src/utils/url.test.ts b/packages/core/src/utils/url.test.ts index f146471a59..d027f78565 100644 --- a/packages/core/src/utils/url.test.ts +++ b/packages/core/src/utils/url.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { SafeUrlSchema } from "./redirect-uri"; -import { isSafeUrlScheme } from "./url"; +import { isSafeUrlScheme, normalizePathname } from "./url"; describe("isSafeUrlScheme", () => { it("rejects code-execution schemes", () => { @@ -24,6 +24,45 @@ describe("isSafeUrlScheme", () => { }); }); +describe("normalizePathname", () => { + it("strips the basePath prefix", () => { + expect( + normalizePathname("http://localhost:3000/api/auth/sign-in", "/api/auth"), + ).toBe("/sign-in"); + }); + + it("canonicalizes a trailing-slash basePath", () => { + // A baseURL of "https://app.com/api/auth/" yields basePath "/api/auth/". + // Without canonicalization the prefix never matches and the full path + // leaks through to disabledPaths / rate-limit special-rule matching. + expect( + normalizePathname("http://localhost:3000/api/auth/sign-in", "/api/auth/"), + ).toBe("/sign-in"); + expect( + normalizePathname("http://localhost:3000/api/auth", "/api/auth/"), + ).toBe("/"); + }); + + it("treats '/' and empty basePath as no prefix", () => { + expect(normalizePathname("http://localhost:3000/sign-in/", "/")).toBe( + "/sign-in", + ); + expect(normalizePathname("http://localhost:3000/sign-in", "")).toBe( + "/sign-in", + ); + }); + + it("does not strip a basePath that is only a string prefix of the path", () => { + expect( + normalizePathname("http://localhost:3000/api/authevil/x", "/api/auth"), + ).toBe("/api/authevil/x"); + }); + + it("returns '/' for a malformed URL", () => { + expect(normalizePathname("not a url", "/api/auth")).toBe("/"); + }); +}); + describe("SafeUrlSchema", () => { it("rejects dangerous schemes", () => { expect(SafeUrlSchema.safeParse("javascript:alert(1)").success).toBe(false); diff --git a/packages/core/src/utils/url.ts b/packages/core/src/utils/url.ts index 13b9a74c74..63326cb297 100644 --- a/packages/core/src/utils/url.ts +++ b/packages/core/src/utils/url.ts @@ -25,18 +25,24 @@ export function normalizePathname( return "/"; } - if (basePath === "/" || basePath === "") { + // Canonicalize the basePath the same way as the request pathname. A baseURL + // with a trailing slash yields a basePath like "/api/auth/"; without this it + // would never match the slash-stripped pathname and the prefix would leak + // through to disabledPaths and rate-limit special-rule matching. + const normalizedBasePath = basePath.replace(/\/+$/, ""); + + if (normalizedBasePath === "") { return pathname; } // Check for exact match or proper path boundary (basePath followed by "/" or end) // This prevents "/api/auth" from matching "/api/authevil/..." - if (pathname === basePath) { + if (pathname === normalizedBasePath) { return "/"; } - if (pathname.startsWith(basePath + "/")) { - return pathname.slice(basePath.length).replace(/\/+$/, "") || "/"; + if (pathname.startsWith(normalizedBasePath + "/")) { + return pathname.slice(normalizedBasePath.length).replace(/\/+$/, "") || "/"; } return pathname; diff --git a/packages/expo/src/routes.ts b/packages/expo/src/routes.ts index 42d93fe051..13ec06d337 100644 --- a/packages/expo/src/routes.ts +++ b/packages/expo/src/routes.ts @@ -13,6 +13,41 @@ export const expoAuthorizationProxy = createAuthEndpoint( metadata: HIDE_METADATA, }, async (ctx) => { + const { authorizationURL } = ctx.query; + + // This endpoint sets an OAuth state cookie and redirects, so the target + // must be an external provider authorization endpoint. Reject malformed + // or non-https targets and any same-origin Better Auth URL: a same-origin + // target would allow a state cookie to be planted and a + // login-CSRF / session-fixation flow through the auth domain. + // FIXME(next): bind the redirect to a server-generated, signed proxy + // token (or validate against the configured provider authorization + // endpoints) to also close redirects to unrelated external https hosts. + // + // A fragment is never part of a valid OAuth authorization endpoint, and a + // bare trailing "#" parses to an empty url.hash, so reject on the raw value. + if (authorizationURL.includes("#")) { + throw new APIError("BAD_REQUEST", { + message: "Invalid authorizationURL", + }); + } + let url: URL; + try { + url = new URL(authorizationURL); + } catch { + throw new APIError("BAD_REQUEST", { + message: "Invalid authorizationURL", + }); + } + if ( + url.protocol !== "https:" || + url.origin === new URL(ctx.context.baseURL).origin + ) { + throw new APIError("BAD_REQUEST", { + message: "Invalid authorizationURL", + }); + } + const { oauthState } = ctx.query; if (oauthState) { const oauthStateCookie = ctx.context.createAuthCookie("oauth_state", { @@ -23,11 +58,9 @@ export const expoAuthorizationProxy = createAuthEndpoint( oauthState, oauthStateCookie.attributes, ); - return ctx.redirect(ctx.query.authorizationURL); + return ctx.redirect(authorizationURL); } - const { authorizationURL } = ctx.query; - const url = new URL(authorizationURL); const state = url.searchParams.get("state"); if (!state) { throw new APIError("BAD_REQUEST", { diff --git a/packages/expo/test/expo.test.ts b/packages/expo/test/expo.test.ts index f17e4b96c1..dd556c5518 100644 --- a/packages/expo/test/expo.test.ts +++ b/packages/expo/test/expo.test.ts @@ -1427,3 +1427,59 @@ describe("ExpoOnlineManager duplicate notification prevention", () => { expect(listener).toHaveBeenCalledTimes(2); }); }); + +describe("expo authorization proxy", async () => { + const { auth } = await getTestInstance({ + baseURL: "https://app.example", + socialProviders: { + google: { clientId: "test", clientSecret: "test" }, + }, + plugins: [expo()], + }); + + const proxy = (authorizationURL: string) => + auth.handler( + new Request( + `https://app.example/api/auth/expo-authorization-proxy?authorizationURL=${encodeURIComponent( + authorizationURL, + )}`, + ), + ); + + it("rejects a same-origin authorizationURL (login-CSRF bounce)", async () => { + const res = await proxy( + "https://app.example/api/auth/callback/google?state=x", + ); + expect(res.status).toBe(400); + }); + + it("rejects a non-https authorizationURL", async () => { + const res = await proxy( + "http://accounts.google.com/o/oauth2/v2/auth?state=x", + ); + expect(res.status).toBe(400); + }); + + it("rejects a malformed authorizationURL", async () => { + const res = await proxy("not-a-url"); + expect(res.status).toBe(400); + }); + + it("rejects an authorizationURL with a fragment", async () => { + const withFragment = await proxy( + "https://accounts.google.com/o/oauth2/v2/auth?state=x#fragment", + ); + expect(withFragment.status).toBe(400); + const bareFragment = await proxy( + "https://accounts.google.com/o/oauth2/v2/auth?state=x#", + ); + expect(bareFragment.status).toBe(400); + }); + + it("redirects to an external https provider authorization endpoint", async () => { + const target = + "https://accounts.google.com/o/oauth2/v2/auth?client_id=x&state=abc"; + const res = await proxy(target); + expect(res.headers.get("location")).toBe(target); + }); +});