From b18e3b8fdfe3d6a03bfdf18b34ed8667459d85dd Mon Sep 17 00:00:00 2001 From: Taesu <166604494+bytaesu@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:07:35 +0900 Subject: [PATCH] fix(oauth-proxy): run the standard callback checks on proxied callbacks (#11930) --- .changeset/oauth-proxy-callback-checks.md | 5 + .../better-auth/src/api/routes/callback.ts | 98 ++---------- packages/better-auth/src/oauth2/callback.ts | 107 ++++++++++++++ .../src/plugins/oauth-proxy/index.ts | 90 +++--------- .../plugins/oauth-proxy/oauth-proxy.test.ts | 139 ++++++++++++++++++ 5 files changed, 282 insertions(+), 157 deletions(-) create mode 100644 .changeset/oauth-proxy-callback-checks.md create mode 100644 packages/better-auth/src/oauth2/callback.ts diff --git a/.changeset/oauth-proxy-callback-checks.md b/.changeset/oauth-proxy-callback-checks.md new file mode 100644 index 0000000000..5ed14b41d4 --- /dev/null +++ b/.changeset/oauth-proxy-callback-checks.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +OAuth callbacks completed through the OAuth proxy now run the same checks as the standard OAuth callback, including issuer and ID token nonce validation. diff --git a/packages/better-auth/src/api/routes/callback.ts b/packages/better-auth/src/api/routes/callback.ts index fd41096ec4..49a2d18ed3 100644 --- a/packages/better-auth/src/api/routes/callback.ts +++ b/packages/better-auth/src/api/routes/callback.ts @@ -1,15 +1,9 @@ import { createAuthEndpoint } from "@better-auth/core/api"; -import type { AccountKey } from "@better-auth/core/db"; -import type { OAuth2Tokens } from "@better-auth/core/oauth2"; -import { safeJSONParse } from "@better-auth/core/utils/json"; import { appendQueryParams } from "@better-auth/core/utils/url"; import * as z from "zod"; import { getAwaitableValue } from "../../context/helpers"; import { setSessionCookie } from "../../cookies"; -import { - resolveOAuthAccountKey, - toOAuthProfileRecord, -} from "../../oauth2/account-key"; +import { exchangeOAuthCallback } from "../../oauth2/callback"; import { missingEmailLogMessage, OAUTH_CALLBACK_ERROR_CODES, @@ -171,88 +165,18 @@ export const callbackOAuth = createAuthEndpoint( throw redirectOnError(OAUTH_CALLBACK_ERROR_CODES.PROVIDER_NOT_FOUND); } - // RFC 9207: validate authorization server issuer identifier. - // Only validated when the provider sends the iss parameter; - // older OAuth servers that don't support RFC 9207 omit it. - if (iss && provider.issuer && iss !== provider.issuer) { - c.context.logger.error("OAuth issuer mismatch", { - expected: provider.issuer, - received: iss, - }); - throw redirectOnError(OAUTH_CALLBACK_ERROR_CODES.ISSUER_MISMATCH); - } - - // Fail closed: a provider that requires id_token nonce binding must carry - // an expected nonce recovered from state. If it is absent (a flow minted - // before binding was enabled, or a dropped state field), the redirect - // cannot enforce the binding, so refuse rather than trust an unbound - // id_token. - if (provider.requiresIdTokenNonce && !idTokenNonce) { - c.context.logger.error( - "OAuth id_token nonce binding required but no expected nonce was found in state", - { providerId: provider.id }, - ); - throw redirectOnError(OAUTH_CALLBACK_ERROR_CODES.NONCE_BINDING_MISSING); - } - - let tokens: OAuth2Tokens | null; - try { - tokens = await provider.validateAuthorizationCode({ - code: code, - codeVerifier, - deviceId: device_id, - redirectURI: `${c.context.baseURL}${getOAuthCallbackPath(provider)}`, - }); - } catch (e) { - c.context.logger.error("", e); - throw redirectOnError(OAUTH_CALLBACK_ERROR_CODES.INVALID_CODE); - } - if (!tokens) { - throw redirectOnError(OAUTH_CALLBACK_ERROR_CODES.INVALID_CODE); - } - const parsedUserData = userData - ? safeJSONParse<{ - name?: { - firstName?: string; - lastName?: string; - }; - email?: string; - }>(userData) - : null; - - const providerResult = await provider.getUserInfo({ - ...tokens, - ...(idTokenNonce ? { expectedIdTokenNonce: idTokenNonce } : {}), - /** - * The user object from the provider - * This is only available for some providers like Apple - */ - user: parsedUserData ?? undefined, + const exchanged = await exchangeOAuthCallback(c, provider, { + code, + codeVerifier, + iss, + deviceId: device_id, + user: userData, + idTokenNonce, }); - if (!providerResult?.user) { - c.context.logger.error("Unable to get user info"); - return redirectOnError( - OAUTH_CALLBACK_ERROR_CODES.UNABLE_TO_GET_USER_INFO, - ); - } - const userInfo = providerResult.user; - const providerProfile = toOAuthProfileRecord(providerResult.data); - let accountKey: AccountKey; - try { - accountKey = await resolveOAuthAccountKey( - provider, - tokens, - providerResult.data, - ); - } catch (error) { - c.context.logger.error("Unable to derive provider account identity", { - providerId: provider.id, - error, - }); - return redirectOnError( - OAUTH_CALLBACK_ERROR_CODES.UNABLE_TO_GET_USER_INFO, - ); + if (exchanged.error) { + throw redirectOnError(exchanged.error); } + const { tokens, userInfo, providerProfile, accountKey } = exchanged; const { accountId } = accountKey; if (!callbackURL) { diff --git a/packages/better-auth/src/oauth2/callback.ts b/packages/better-auth/src/oauth2/callback.ts new file mode 100644 index 0000000000..edb432ac81 --- /dev/null +++ b/packages/better-auth/src/oauth2/callback.ts @@ -0,0 +1,107 @@ +import type { GenericEndpointContext } from "@better-auth/core"; +import type { OAuth2Tokens, OAuthProvider } from "@better-auth/core/oauth2"; +import { safeJSONParse } from "@better-auth/core/utils/json"; +import { resolveOAuthAccountKey, toOAuthProfileRecord } from "./account-key"; +import { OAUTH_CALLBACK_ERROR_CODES } from "./errors"; +import { getOAuthCallbackPath } from "./utils"; + +interface OAuthCallbackInput { + code: string; + codeVerifier: string; + iss?: string | undefined; + deviceId?: string | undefined; + user?: string | undefined; + idTokenNonce?: string | undefined; +} + +/** + * Turns a provider callback into a verified identity. It checks that the + * callback answers this authorization request, exchanges the code, and + * resolves the user and account key. + * + * Every route that completes a provider callback goes through here, so the + * routes run the same checks. + */ +export async function exchangeOAuthCallback( + ctx: GenericEndpointContext, + provider: OAuthProvider, + callback: OAuthCallbackInput, +) { + // RFC 9207: validate authorization server issuer identifier. + // Only validated when the provider sends the iss parameter; + // older OAuth servers that don't support RFC 9207 omit it. + if (callback.iss && provider.issuer && callback.iss !== provider.issuer) { + ctx.context.logger.error("OAuth issuer mismatch", { + expected: provider.issuer, + received: callback.iss, + }); + return { error: OAUTH_CALLBACK_ERROR_CODES.ISSUER_MISMATCH }; + } + + // Fail closed: a provider that requires id_token nonce binding must carry + // an expected nonce recovered from state. If it is absent (a flow minted + // before binding was enabled, or a dropped state field), the redirect + // cannot enforce the binding, so refuse rather than trust an unbound + // id_token. + if (provider.requiresIdTokenNonce && !callback.idTokenNonce) { + ctx.context.logger.error( + "OAuth id_token nonce binding required but no expected nonce was found in state", + { providerId: provider.id }, + ); + return { error: OAUTH_CALLBACK_ERROR_CODES.NONCE_BINDING_MISSING }; + } + + // A custom provider can throw before returning a promise, so await inside + // `try` to catch both. + let tokens: OAuth2Tokens | null; + try { + tokens = await provider.validateAuthorizationCode({ + code: callback.code, + codeVerifier: callback.codeVerifier, + deviceId: callback.deviceId, + redirectURI: `${ctx.context.baseURL}${getOAuthCallbackPath(provider)}`, + }); + } catch (error) { + ctx.context.logger.error("Failed to validate authorization code", error); + return { error: OAUTH_CALLBACK_ERROR_CODES.INVALID_CODE }; + } + if (!tokens) { + return { error: OAUTH_CALLBACK_ERROR_CODES.INVALID_CODE }; + } + + const result = await provider.getUserInfo({ + ...tokens, + expectedIdTokenNonce: callback.idTokenNonce, + /** + * The user object from the provider + * This is only available for some providers like Apple + */ + user: safeJSONParse(callback.user) ?? undefined, + }); + if (!result?.user) { + ctx.context.logger.error("Unable to get user info"); + return { error: OAUTH_CALLBACK_ERROR_CODES.UNABLE_TO_GET_USER_INFO }; + } + + const accountKey = await resolveOAuthAccountKey( + provider, + tokens, + result.data, + ).catch((error) => { + ctx.context.logger.error("Unable to derive provider account identity", { + providerId: provider.id, + error, + }); + return null; + }); + if (!accountKey) { + return { error: OAUTH_CALLBACK_ERROR_CODES.UNABLE_TO_GET_USER_INFO }; + } + + return { + tokens, + userInfo: result.user, + providerProfile: toOAuthProfileRecord(result.data), + accountKey, + }; +} diff --git a/packages/better-auth/src/plugins/oauth-proxy/index.ts b/packages/better-auth/src/plugins/oauth-proxy/index.ts index 0798823b1b..aaea4e4072 100644 --- a/packages/better-auth/src/plugins/oauth-proxy/index.ts +++ b/packages/better-auth/src/plugins/oauth-proxy/index.ts @@ -7,10 +7,7 @@ import { createAuthEndpoint, createAuthMiddleware, } from "@better-auth/core/api"; -import type { AccountKey } from "@better-auth/core/db"; import { accountSchema, userSchema } from "@better-auth/core/db"; -import type { OAuth2Tokens } from "@better-auth/core/oauth2"; -import { safeJSONParse } from "@better-auth/core/utils/json"; import { defu } from "defu"; import * as z from "zod"; import { originCheck } from "../../api"; @@ -21,16 +18,12 @@ import { parseSetCookieHeader } from "../../cookies/cookie-utils"; import { symmetricDecrypt, symmetricEncrypt } from "../../crypto"; import type { EncryptionPurpose } from "../../crypto/purpose"; import { derivePurposeKey } from "../../crypto/purpose"; -import { - resolveOAuthAccountKey, - toOAuthProfileRecord, -} from "../../oauth2/account-key"; +import { exchangeOAuthCallback } from "../../oauth2/callback"; import { redirectOnError } from "../../oauth2/errors"; import { handleOAuthUserInfo, linkOAuthAccount, } from "../../oauth2/link-account"; -import { getOAuthCallbackPath } from "../../oauth2/utils"; import type { StateData } from "../../state"; import { getAuthStateVerificationIdentifier, @@ -165,6 +158,8 @@ const oauthCallbackQuerySchema = z.object({ code: z.string().optional(), error: z.string().optional(), user: z.string().optional(), + iss: z.string().optional(), + device_id: z.string().optional(), }); export const oAuthProxy = (opts?: O) => { @@ -455,7 +450,13 @@ export const oAuthProxy = (opts?: O) => { ); return; } - const { code, error, user: userData } = query.data; + const { + code, + error, + user: userData, + iss, + device_id: deviceId, + } = query.data; // Decrypt state to get codeVerifier and callbackURL let stateData: StateData; @@ -509,74 +510,23 @@ export const oAuthProxy = (opts?: O) => { throw redirectOnError(ctx, errorURL, "oauth_provider_not_found"); } - // Exchange code for tokens - let tokens: OAuth2Tokens | null; - try { - tokens = await provider.validateAuthorizationCode({ - code, - codeVerifier: stateData.codeVerifier, - redirectURI: `${ctx.context.baseURL}${getOAuthCallbackPath(provider)}`, - }); - } catch (e) { - ctx.context.logger.error( - "Failed to validate authorization code", - e, - ); - throw redirectOnError(ctx, errorURL, "invalid_code"); - } - - if (!tokens) { - throw redirectOnError(ctx, errorURL, "invalid_code"); - } - - const parsedUserData = userData - ? safeJSONParse<{ - name?: { - firstName?: string; - lastName?: string; - }; - email?: string; - }>(userData) - : null; - - // Get user info from provider - const userInfoResult = await provider.getUserInfo({ - ...tokens, - /** - * The user object from the provider - * This is only available for some providers like Apple - */ - user: parsedUserData ?? undefined, + const exchanged = await exchangeOAuthCallback(ctx, provider, { + code, + codeVerifier: stateData.codeVerifier, + iss, + deviceId, + user: userData, + idTokenNonce: stateData.idTokenNonce, }); - const userInfo = userInfoResult?.user; - - if (!userInfo) { - ctx.context.logger.error("Unable to get user info from provider"); - throw redirectOnError(ctx, errorURL, "unable_to_get_user_info"); + if (exchanged.error) { + throw redirectOnError(ctx, errorURL, exchanged.error); } + const { tokens, userInfo, providerProfile, accountKey } = exchanged; if (!userInfo.email) { ctx.context.logger.error("Provider did not return email"); throw redirectOnError(ctx, errorURL, "email_not_found"); } - let accountKey: AccountKey; - try { - accountKey = await resolveOAuthAccountKey( - provider, - tokens, - userInfoResult.data, - ); - } catch (error) { - ctx.context.logger.error( - "Unable to derive provider account identity", - { - providerId: provider.id, - error, - }, - ); - throw redirectOnError(ctx, errorURL, "unable_to_get_user_info"); - } - const providerProfile = toOAuthProfileRecord(userInfoResult.data); const proxyCallbackURL = new URL(stateData.callbackURL); const finalCallbackURL = 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 876eaec684..fc3317a4f4 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 @@ -2437,3 +2437,142 @@ describe("oauth-proxy current URL trust", () => { } }); }); + +describe("oauth-proxy callback binding", () => { + const providerId = "test-idp"; + + function createProvider( + options: Partial< + Pick< + OAuthProvider, + "issuer" | "requiresIdTokenNonce" | "validateAuthorizationCode" + > + > = {}, + ) { + return { + id: providerId, + name: "Test IdP", + issuer: options.issuer, + requiresIdTokenNonce: options.requiresIdTokenNonce, + accountSubject: () => "test-subject", + createAuthorizationURL: vi.fn( + ({ state }) => + new URL(`https://idp.example.com/authorize?state=${state}`), + ), + validateAuthorizationCode: + options.validateAuthorizationCode ?? + (async () => ({ accessToken: "test-access-token" })), + getUserInfo: vi.fn(async () => ({ + user: { + email: "user@idp.example.com", + name: "Test User", + emailVerified: true, + }, + data: { sub: "test-subject" }, + })), + } satisfies OAuthProvider; + } + + function withProvider(provider: OAuthProvider) { + return { + id: "test-idp-provider", + init: (ctx: { socialProviders: OAuthProvider[] }) => ({ + context: { socialProviders: [provider, ...ctx.socialProviders] }, + }), + }; + } + + /** + * Starts sign-in on a preview instance and completes the callback on + * production, returning where production redirects. + */ + async function completeProxiedCallback({ + preview = createProvider(), + production = createProvider(), + query = "", + }: { + preview?: OAuthProvider; + production?: OAuthProvider; + query?: string; + } = {}) { + const productionInstance = await getTestInstance( + { + baseURL: "http://localhost:3000", + plugins: [oAuthProxy(), withProvider(production)], + }, + { disableTestUser: true }, + ); + const previewInstance = await getTestInstance( + { + baseURL: "http://preview.example.com", + plugins: [ + oAuthProxy({ productionURL: "http://localhost:3000" }), + withProvider(preview), + ], + }, + { disableTestUser: true }, + ); + const { url } = await previewInstance.client.signIn.social( + { provider: providerId, callbackURL: "/dashboard" }, + { throw: true }, + ); + const state = new URL(url!).searchParams.get("state"); + + const redirect: { location: string | null } = { location: null }; + await productionInstance.client.$fetch( + `/callback/${providerId}?code=test&state=${state}${query}`, + { + onError(context) { + redirect.location = context.response.headers.get("location"); + }, + }, + ); + if (!redirect.location) { + throw new Error("OAuth proxy callback did not redirect"); + } + return new URL(redirect.location); + } + + it("rejects a callback whose iss does not match the provider issuer", async () => { + const location = await completeProxiedCallback({ + production: createProvider({ issuer: "https://idp.example.com" }), + query: "&iss=https://attacker.example", + }); + + expect(location.searchParams.get("error")).toBe("issuer_mismatch"); + }); + + it("rejects a callback when the provider requires a nonce the state lacks", async () => { + const location = await completeProxiedCallback({ + production: createProvider({ requiresIdTokenNonce: true }), + }); + + expect(location.searchParams.get("error")).toBe("nonce_binding_missing"); + }); + + it("passes the nonce issued at sign-in to the provider's getUserInfo", async () => { + const preview = createProvider({ requiresIdTokenNonce: true }); + const production = createProvider({ requiresIdTokenNonce: true }); + + await completeProxiedCallback({ preview, production }); + + const issuedNonce = + preview.createAuthorizationURL.mock.calls[0]?.[0].idTokenNonce; + expect(issuedNonce).toEqual(expect.any(String)); + expect(production.getUserInfo).toHaveBeenCalledWith( + expect.objectContaining({ expectedIdTokenNonce: issuedNonce }), + ); + }); + + it("redirects with invalid_code when the code exchange throws synchronously", async () => { + const location = await completeProxiedCallback({ + production: createProvider({ + validateAuthorizationCode: () => { + throw new Error("token endpoint misconfigured"); + }, + }), + }); + + expect(location.searchParams.get("error")).toBe("invalid_code"); + }); +});