diff --git a/.changeset/famous-banks-open.md b/.changeset/famous-banks-open.md new file mode 100644 index 0000000000..213fcc4fd1 --- /dev/null +++ b/.changeset/famous-banks-open.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +security: verify OAuth state parameter against cookie-stored nonce to prevent CSRF on cookie-backed flows diff --git a/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts b/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts index c798a584e5..9abfbda782 100644 --- a/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts +++ b/packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts @@ -989,6 +989,64 @@ describe("oauth2", async () => { expect(session.data?.user.name).toBe("OAuth2 Cookie State"); }); + it("should reject cookie-backed OAuth when callback state does not match the issued state", async () => { + const { customFetchImpl, cookieSetter } = await getTestInstance({ + plugins: [ + genericOAuth({ + config: [ + { + providerId: "test-cookie-csrf", + discoveryUrl: `http://localhost:${port}/.well-known/openid-configuration`, + clientId: clientId, + clientSecret: clientSecret, + pkce: false, + }, + ], + }), + ], + account: { + storeStateStrategy: "cookie", + }, + }); + + const victimHeaders = new Headers(); + const authClient = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: "http://localhost:3000", + fetchOptions: { + customFetchImpl, + onSuccess: cookieSetter(victimHeaders), + }, + }); + + const signInRes = await authClient.signIn.oauth2({ + providerId: "test-cookie-csrf", + callbackURL: "http://localhost:3000/dashboard", + fetchOptions: { + onSuccess: cookieSetter(victimHeaders), + }, + }); + expect(signInRes.data?.url).toBeTruthy(); + + const res = await customFetchImpl( + "http://localhost:3000/api/auth/oauth2/callback/test-cookie-csrf?code=dummy&state=attacker-controlled-state", + { + headers: victimHeaders, + redirect: "manual", + }, + ); + + expect(res.status).toBe(302); + expect(res.headers.get("location")).toContain("state_mismatch"); + + const session = await authClient.getSession({ + fetchOptions: { + headers: victimHeaders, + }, + }); + expect(session.data).toBeNull(); + }); + it("should await async mapProfileToUser", async () => { const { auth } = await getTestInstance({ plugins: [ diff --git a/packages/better-auth/src/plugins/oauth-proxy/index.ts b/packages/better-auth/src/plugins/oauth-proxy/index.ts index e8577b6a81..674187d322 100644 --- a/packages/better-auth/src/plugins/oauth-proxy/index.ts +++ b/packages/better-auth/src/plugins/oauth-proxy/index.ts @@ -375,6 +375,15 @@ export const oAuthProxy = (opts?: O) => { stateData.errorURL || ctx.context.options.onAPIError?.errorURL || `${ctx.context.baseURL}/error`; + + if ( + stateData.oauthState !== undefined && + stateData.oauthState !== statePackage.state + ) { + ctx.context.logger.error("OAuth proxy state binding mismatch"); + throw redirectOnError(ctx, errorURL, "state_mismatch"); + } + if (error) { throw redirectOnError(ctx, errorURL, error); } diff --git a/packages/better-auth/src/social.test.ts b/packages/better-auth/src/social.test.ts index 10de6f2216..8b83d4a71a 100644 --- a/packages/better-auth/src/social.test.ts +++ b/packages/better-auth/src/social.test.ts @@ -805,6 +805,7 @@ describe("signin", async () => { redirect: true, }); state = new URL(signInRes.data!.url!).searchParams.get("state") || ""; + expect(state).toBeTruthy(); await client.$fetch("/callback/google", { query: { @@ -823,6 +824,7 @@ describe("signin", async () => { expiresAt: expect.any(Number), invitedBy: "user-123", errorURL: "http://localhost:3000/api/auth/error", + oauthState: state, }); }); diff --git a/packages/better-auth/src/state.ts b/packages/better-auth/src/state.ts index 2f9c7aa74c..9303b6f263 100644 --- a/packages/better-auth/src/state.ts +++ b/packages/better-auth/src/state.ts @@ -14,6 +14,11 @@ const stateDataSchema = z.looseObject({ errorURL: z.string().optional(), newUserURL: z.string().optional(), expiresAt: z.number(), + /** + * CSRF nonce returned to the OAuth provider. When using cookie state storage, + * this must match the callback `state` query parameter. + */ + oauthState: z.string().optional(), link: z .object({ email: z.string(), @@ -61,9 +66,10 @@ export async function generateGenericState( // State data is encrypted into the cookie // no verification record created if (storeStateStrategy === "cookie") { + const payload: StateData = { ...stateData, oauthState: state }; const encryptedData = await symmetricEncrypt({ key: c.context.secretConfig, - data: JSON.stringify(stateData), + data: JSON.stringify(payload), }); const stateCookie = c.context.createAuthCookie( @@ -103,7 +109,10 @@ export async function generateGenericState( expiresAt.setMinutes(expiresAt.getMinutes() + 10); const verification = await c.context.internalAdapter.createVerificationValue({ - value: JSON.stringify(stateData), + value: JSON.stringify({ + ...stateData, + oauthState: state, + } satisfies StateData), identifier: state, expiresAt, }); @@ -166,6 +175,16 @@ export async function parseGenericState( ); } + if (!parsedData.oauthState || parsedData.oauthState !== state) { + throw new StateError( + "State mismatch: OAuth state parameter does not match stored state", + { + code: "state_security_mismatch", + details: { state }, + }, + ); + } + // Clear the cookie after successful parsing expireCookie(c, stateCookie); } else { @@ -180,6 +199,19 @@ export async function parseGenericState( parsedData = stateDataSchema.parse(JSON.parse(data.value)); + if ( + parsedData.oauthState !== undefined && + parsedData.oauthState !== state + ) { + throw new StateError( + "State mismatch: OAuth state parameter does not match stored state", + { + code: "state_security_mismatch", + details: { state }, + }, + ); + } + const stateCookie = c.context.createAuthCookie( settings?.cookieName ?? "state", );