From 88409b0078c2bfddcc6503031fff333bfa045cd2 Mon Sep 17 00:00:00 2001 From: Taesu <166604494+bytaesu@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:41:43 -0700 Subject: [PATCH] fix(oauth-proxy): reject profile callbacks with missing or expired OAuth state (#10183) --- .changeset/tender-rivers-serve.md | 5 + .../src/plugins/oauth-proxy/index.ts | 29 ++- .../plugins/oauth-proxy/oauth-proxy.test.ts | 183 +++++++++++++++--- 3 files changed, 178 insertions(+), 39 deletions(-) create mode 100644 .changeset/tender-rivers-serve.md diff --git a/.changeset/tender-rivers-serve.md b/.changeset/tender-rivers-serve.md new file mode 100644 index 0000000000..e649e1b932 --- /dev/null +++ b/.changeset/tender-rivers-serve.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Require OAuth proxy profile callbacks to match an issued OAuth state before creating sessions. diff --git a/packages/better-auth/src/plugins/oauth-proxy/index.ts b/packages/better-auth/src/plugins/oauth-proxy/index.ts index 8bb4fb6138..8249e40a86 100644 --- a/packages/better-auth/src/plugins/oauth-proxy/index.ts +++ b/packages/better-auth/src/plugins/oauth-proxy/index.ts @@ -108,6 +108,21 @@ type PassthroughPayload = { timestamp: number; }; +const consumeOAuthProxyState = async ( + ctx: GenericEndpointContext, + state: string, +) => { + try { + await parseGenericState(ctx, state, { + skipStateCookieCheck: true, + }); + return true; + } catch (e) { + ctx.context.logger.warn("OAuth proxy state missing or invalid", e); + return false; + } +}; + const oauthProxyQuerySchema = z.object({ callbackURL: z.string().meta({ description: "The URL to redirect to after the proxy", @@ -219,6 +234,7 @@ export const oAuthProxy = (opts?: O) => { typeof payload.timestamp !== "number" || !payload.userInfo || !payload.account || + !payload.state || !payload.callbackURL ) { ctx.context.logger.error("Failed to parse OAuth proxy payload"); @@ -237,13 +253,12 @@ export const oAuthProxy = (opts?: O) => { throw redirectOnError(ctx, errorURL, "payload_expired"); } - // Clean up OAuth state - try { - await parseGenericState(ctx, payload.state, { - skipStateCookieCheck: true, - }); - } catch (e) { - ctx.context.logger.warn("Failed to clean up OAuth state", e); + const stateConsumed = await consumeOAuthProxyState( + ctx, + payload.state, + ); + if (!stateConsumed) { + throw redirectOnError(ctx, errorURL, "state_mismatch"); } let result: Awaited>; 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 e9b6ed4d09..7cbd8d6e05 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 @@ -569,11 +569,8 @@ describe("oauth-proxy", async () => { // Production instance - handles OAuth callback const production = await getTestInstance( { - plugins: [ - oAuthProxy({ - currentURL: "http://preview.example.com", - }), - ], + baseURL: "http://localhost:3000", + plugins: [oAuthProxy()], socialProviders: { google: { clientId: "test", @@ -590,7 +587,11 @@ describe("oauth-proxy", async () => { const preview = await getTestInstance( { baseURL: "http://preview.example.com", - plugins: [oAuthProxy()], + plugins: [ + oAuthProxy({ + productionURL: "http://localhost:3000", + }), + ], socialProviders: { google: { clientId: "test", @@ -603,8 +604,8 @@ describe("oauth-proxy", async () => { }, ); - // Step 1: Start OAuth on production - const res = await production.client.signIn.social( + // Step 1: Start OAuth on preview + const res = await preview.client.signIn.social( { provider: "google", callbackURL: "/dashboard", @@ -678,11 +679,8 @@ describe("oauth-proxy", async () => { it("should forward result.error verbatim instead of collapsing to user_creation_failed", async () => { const production = await getTestInstance( { - plugins: [ - oAuthProxy({ - currentURL: "http://preview.example.com", - }), - ], + baseURL: "http://localhost:3000", + plugins: [oAuthProxy()], socialProviders: { google: { clientId: "test", @@ -697,7 +695,11 @@ describe("oauth-proxy", async () => { const preview = await getTestInstance( { baseURL: "http://preview.example.com", - plugins: [oAuthProxy()], + plugins: [ + oAuthProxy({ + productionURL: "http://localhost:3000", + }), + ], socialProviders: { google: { clientId: "test", @@ -708,7 +710,7 @@ describe("oauth-proxy", async () => { { disableTestUser: true }, ); - const res = await production.client.signIn.social( + const res = await preview.client.signIn.social( { provider: "google", callbackURL: "/dashboard", @@ -1040,9 +1042,9 @@ describe("oauth-proxy", async () => { const production = await getTestInstance( { + baseURL: "http://localhost:3000", plugins: [ oAuthProxy({ - currentURL: "http://preview.example.com", secret: dedicatedSecret, }), ], @@ -1063,6 +1065,7 @@ describe("oauth-proxy", async () => { baseURL: "http://preview.example.com", plugins: [ oAuthProxy({ + productionURL: "http://localhost:3000", secret: dedicatedSecret, }), ], @@ -1078,8 +1081,8 @@ describe("oauth-proxy", async () => { }, ); - // Step 1: Start OAuth on production - const res = await production.client.signIn.social( + // Step 1: Start OAuth on preview + const res = await preview.client.signIn.social( { provider: "google", callbackURL: "/dashboard", @@ -1142,7 +1145,7 @@ describe("oauth-proxy", async () => { expect(users[0]?.email).toBe("user@email.com"); }); - it("should handle existing user on preview", async () => { + it("should reject a profile payload whose OAuth state was never issued", async () => { // Preview instance const preview = await getTestInstance( { @@ -1200,22 +1203,19 @@ describe("oauth-proxy", async () => { onError(context) { expect(context.response.status).toBe(302); const location = context.response.headers.get("location"); - expect(location).toContain("/dashboard"); + expect(location).toContain("error=state_mismatch"); }, }, ); - // User count should still be 1 (linked account, not new user) const users = await previewCtx.internalAdapter.listUsers(); expect(users.length).toBe(1); expect(users[0]?.email).toBe("user@email.com"); - // Should have linked the google account const accounts = await previewCtx.internalAdapter.findAccounts( users[0]!.id, ); - expect(accounts.length).toBe(1); - expect(accounts[0]?.providerId).toBe("google"); + expect(accounts.length).toBe(0); }); }); @@ -1319,8 +1319,7 @@ describe("oauth-proxy", async () => { expect(users[0]?.email).toBe("user@email.com"); }); - it("should handle state cleanup gracefully when verification is already deleted", async () => { - // This tests that parseGenericState errors are caught and don't break the flow + it("should reject the callback when the OAuth state is not found in the database", async () => { const { client, auth } = await getTestInstance( { plugins: [ @@ -1365,22 +1364,18 @@ describe("oauth-proxy", async () => { data: JSON.stringify(payload), }); - // The callback should still succeed even if state cleanup fails await client.$fetch( `/oauth-proxy-callback?callbackURL=/dashboard&profile=${encodeURIComponent(encryptedProfile)}`, { onError(context) { const location = context.response.headers.get("location"); - // Should redirect to dashboard, not error - expect(location).not.toContain("error=state_mismatch"); - expect(location).toContain("/dashboard"); + expect(location).toContain("error=state_mismatch"); }, }, ); - // Verify user was created const users = await internalAdapter.listUsers(); - expect(users.length).toBe(1); + expect(users.length).toBe(0); }); }); @@ -1584,6 +1579,130 @@ describe("oauth-proxy", async () => { const users = await previewCtx.internalAdapter.listUsers(); expect(users.length).toBe(1); expect(users[0]?.email).toBe("user@email.com"); + + await preview.client.$fetch( + `/oauth-proxy-callback?callbackURL=${encodeURIComponent(callbackURL!)}&profile=${encodeURIComponent(encryptedProfile!)}`, + { + onError(context) { + const location = context.response.headers.get("location"); + expect(location).toContain("error=state_mismatch"); + }, + }, + ); + }); + }); + + describe("cookie state strategy", () => { + it("should validate the state cookie during proxy callback", async () => { + const proxySecret = "shared-oauth-proxy-secret-for-stateless"; + const preview = await getTestInstance( + { + baseURL: "http://preview.example.com", + database: undefined, + secret: "preview-main-secret-for-stateless", + plugins: [ + oAuthProxy({ + productionURL: "http://localhost:3000", + secret: proxySecret, + }), + ], + socialProviders: { + google: { + clientId: "test", + clientSecret: "test", + }, + }, + }, + { + disableTestUser: true, + }, + ); + const production = await getTestInstance( + { + baseURL: "http://localhost:3000", + database: undefined, + secret: "production-main-secret-for-stateless", + plugins: [ + oAuthProxy({ + secret: proxySecret, + }), + ], + socialProviders: { + google: { + clientId: "test", + clientSecret: "test", + }, + }, + }, + { + disableTestUser: true, + }, + ); + + const previewHeaders = new Headers(); + const res = await preview.client.signIn.social( + { + provider: "google", + callbackURL: "/dashboard", + }, + { + throw: true, + onSuccess: preview.cookieSetter(previewHeaders), + }, + ); + + const encryptedState = new URL(res.url!).searchParams.get("state"); + expect(encryptedState).toBeTruthy(); + + let encryptedProfile: string | null = null; + let callbackURL: string | null = null; + await production.client.$fetch( + `/callback/google?code=test&state=${encryptedState}`, + { + onError(context) { + const location = context.response.headers.get("location"); + expect(location).toContain("/oauth-proxy-callback"); + if (location) { + const url = new URL(location); + encryptedProfile = url.searchParams.get("profile"); + callbackURL = url.searchParams.get("callbackURL"); + } + }, + }, + ); + + expect(encryptedProfile).toBeTruthy(); + expect(callbackURL).toBeTruthy(); + + const previewCtx = await preview.auth.$context; + + await preview.client.$fetch( + `/oauth-proxy-callback?callbackURL=${encodeURIComponent(callbackURL!)}&profile=${encodeURIComponent(encryptedProfile!)}`, + { + onError(context) { + const location = context.response.headers.get("location"); + expect(location).toContain("error=state_mismatch"); + }, + }, + ); + expect((await previewCtx.internalAdapter.listUsers()).length).toBe(0); + + await preview.client.$fetch( + `/oauth-proxy-callback?callbackURL=${encodeURIComponent(callbackURL!)}&profile=${encodeURIComponent(encryptedProfile!)}`, + { + headers: previewHeaders, + onError(context) { + preview.cookieSetter(previewHeaders)(context); + const location = context.response.headers.get("location"); + expect(location).not.toContain("error="); + expect(location).toContain("/dashboard"); + }, + }, + ); + + const users = await previewCtx.internalAdapter.listUsers(); + expect(users.length).toBe(1); + expect(users[0]?.email).toBe("user@email.com"); }); });