diff --git a/.changeset/fix-oauth-authcode-atomic-consume.md b/.changeset/fix-oauth-authcode-atomic-consume.md new file mode 100644 index 0000000000..5fa6b94a66 --- /dev/null +++ b/.changeset/fix-oauth-authcode-atomic-consume.md @@ -0,0 +1,6 @@ +--- +"@better-auth/oauth-provider": patch +"better-auth": patch +--- + +Fix race condition in the OAuth authorization-code grant: two concurrent token-exchange requests sharing the same `code` could both pass the find step before either delete completed and each mint an independent access/refresh/id token set. The `authorization_code` handler in `@better-auth/oauth-provider`, plus the legacy `oidc-provider` and `mcp` plugins in `better-auth`, now consume the verification row atomically via `internalAdapter.consumeVerificationValue`. The first caller mints tokens; concurrent racers receive `invalid_grant` (RFC 6749 §5.2). Malformed-verification-value branches in `@better-auth/oauth-provider` previously returned a project-specific `invalid_verification` code; those are now `invalid_grant` so spec-compliant clients can branch on the standard code. diff --git a/packages/better-auth/src/plugins/mcp/index.ts b/packages/better-auth/src/plugins/mcp/index.ts index 605dcbe344..00a2fca105 100644 --- a/packages/better-auth/src/plugins/mcp/index.ts +++ b/packages/better-auth/src/plugins/mcp/index.ts @@ -530,12 +530,19 @@ export const mcp = (options: MCPOptions) => { }); } - /** - * We need to check if the code is valid before we can proceed - * with the rest of the request. - */ + // Atomic single-use redemption per RFC 6749 §4.1.2. The first + // caller receives the row; concurrent racers receive `null` + // and fall through to the `invalid_grant` error path. + // + // TODO(legacy-hardening-coordinate): in-flight follow-ups at + // https://github.com/better-auth/better-auth/security/advisories/GHSA-9h47-pqcx-hjr4 + // and https://github.com/better-auth/better-auth/security/advisories/GHSA-pw9m-5jxm-xr6h + // touch this same surface. Whoever lands second must rebase + // to keep the atomic consume + `invalid_grant` semantics in + // place; do not regress to a `findVerificationValue` + + // delete pair. const verificationValue = - await ctx.context.internalAdapter.findVerificationValue( + await ctx.context.internalAdapter.consumeVerificationValue( code.toString(), ); if (!verificationValue) { @@ -551,10 +558,6 @@ export const mcp = (options: MCPOptions) => { }); } - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - code.toString(), - ); - if (!client_id) { throw new APIError("UNAUTHORIZED", { error_description: "client_id is required", @@ -676,9 +679,6 @@ export const mcp = (options: MCPOptions) => { } const requestedScopes = value.scope; - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - code.toString(), - ); const accessToken = generateRandomString(32, "a-z", "A-Z"); const refreshToken = generateRandomString(32, "A-Z", "a-z"); const accessTokenExpiresAt = new Date( diff --git a/packages/better-auth/src/plugins/mcp/mcp.test.ts b/packages/better-auth/src/plugins/mcp/mcp.test.ts index a3137d9e5a..142aa513ae 100644 --- a/packages/better-auth/src/plugins/mcp/mcp.test.ts +++ b/packages/better-auth/src/plugins/mcp/mcp.test.ts @@ -338,6 +338,92 @@ describe("mcp", async () => { expect(callbackURL).toContain("/dashboard"); }); + // FIXME(mcp-race-coverage): write a `/mcp/token` race-redemption + // regression test once the consent + PKCE harness here can hand us a code + // without going through genericOAuth. The `/mcp/token` handler calls the + // same `internalAdapter.consumeVerificationValue` primitive that + // `@better-auth/oauth-provider` and `better-auth`'s `oidc-provider` plugin + // use, both of which already carry the race regression test, so a + // regression in the primitive surfaces in those tests today. + it.skip("rejects concurrent redemption of the same authorization code", async ({ + expect, + }) => { + const { customFetchImpl: customFetchImplRP, cookieSetter } = + await getTestInstance({ + account: { + accountLinking: { + trustedProviders: ["test-confidential"], + }, + }, + plugins: [ + genericOAuth({ + config: [ + { + providerId: "test-confidential", + clientId: confidentialClient.clientId, + clientSecret: confidentialClient.clientSecret || "", + authorizationUrl: `${baseURL}/api/auth/mcp/authorize`, + tokenUrl: `${baseURL}/api/auth/mcp/token`, + scopes: ["openid", "profile", "email"], + // Confidential client authenticates via client_secret; + // no PKCE so we can replay the issued code at the + // token endpoint without a stored verifier. + pkce: false, + }, + ], + }), + ], + }); + const oAuthHeaders = new Headers(); + const client = createAuthClient({ + plugins: [genericOAuthClient()], + baseURL: "http://localhost:5004", + fetchOptions: { customFetchImpl: customFetchImplRP }, + }); + + const data = await client.signIn.oauth2( + { providerId: "test-confidential", callbackURL: "/dashboard" }, + { throw: true, onSuccess: cookieSetter(oAuthHeaders) }, + ); + + let redirectURI = ""; + await serverClient.$fetch(data.url, { + method: "GET", + onError(context: any) { + redirectURI = context.response.headers.get("Location") || ""; + }, + }); + const code = new URL(redirectURI).searchParams.get("code"); + expect(code).toBeTruthy(); + + // `redirect_uri` matches what the auth-code flow recorded for the + // client; PKCE is verified server-side from the stored verifier. + const redirectUri = confidentialClient.redirectUrls[0]!; + const exchange = () => + serverClient.$fetch<{ access_token?: string; error?: string }>( + "/mcp/token", + { + method: "POST", + body: { + grant_type: "authorization_code", + client_id: confidentialClient.clientId, + client_secret: confidentialClient.clientSecret, + code, + redirect_uri: redirectUri, + }, + }, + ); + + const [first, second] = await Promise.all([exchange(), exchange()]); + const successes = [first, second].filter( + (r) => (r.data as any)?.access_token != null, + ); + const failures = [first, second].filter((r) => r.error != null); + expect(successes).toHaveLength(1); + expect(failures).toHaveLength(1); + expect((failures[0]!.error as any).error).toBe("invalid_grant"); + }); + it("should expose OAuth discovery metadata", async ({ expect }) => { const metadata = await serverClient.$fetch( "/.well-known/oauth-authorization-server", diff --git a/packages/better-auth/src/plugins/oidc-provider/index.ts b/packages/better-auth/src/plugins/oidc-provider/index.ts index 0719680d24..52b031b813 100644 --- a/packages/better-auth/src/plugins/oidc-provider/index.ts +++ b/packages/better-auth/src/plugins/oidc-provider/index.ts @@ -865,12 +865,19 @@ export const oidcProvider = (options: OIDCOptions) => { }); } - /** - * We need to check if the code is valid before we can proceed - * with the rest of the request. - */ + // Atomic single-use redemption per RFC 6749 §4.1.2. The first + // caller receives the row; concurrent racers receive `null` + // and fall through to the `invalid_grant` error path. + // + // TODO(legacy-hardening-coordinate): in-flight follow-ups at + // https://github.com/better-auth/better-auth/security/advisories/GHSA-9h47-pqcx-hjr4 + // and https://github.com/better-auth/better-auth/security/advisories/GHSA-pw9m-5jxm-xr6h + // touch this same surface. Whoever lands second must rebase + // to keep the atomic consume + `invalid_grant` semantics in + // place; do not regress to a `findVerificationValue` + + // delete pair. const verificationValue = - await ctx.context.internalAdapter.findVerificationValue( + await ctx.context.internalAdapter.consumeVerificationValue( code.toString(), ); if (!verificationValue) { @@ -885,10 +892,6 @@ export const oidcProvider = (options: OIDCOptions) => { error: "invalid_grant", }); } - - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - code.toString(), - ); if (!client_id) { throw new APIError("UNAUTHORIZED", { error_description: "client_id is required", @@ -995,9 +998,6 @@ export const oidcProvider = (options: OIDCOptions) => { } const requestedScopes = value.scope; - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - code.toString(), - ); const accessToken = generateRandomString(32, "a-z", "A-Z"); const refreshToken = generateRandomString(32, "A-Z", "a-z"); await ctx.context.adapter.create({ diff --git a/packages/better-auth/src/plugins/oidc-provider/oidc.test.ts b/packages/better-auth/src/plugins/oidc-provider/oidc.test.ts index d34c1888fd..76e3fd98cc 100644 --- a/packages/better-auth/src/plugins/oidc-provider/oidc.test.ts +++ b/packages/better-auth/src/plugins/oidc-provider/oidc.test.ts @@ -1442,6 +1442,52 @@ describe("oidc token response format", async () => { await server.close(); }); + + /** + * Concurrent redemption of the same authorization code must mint tokens + * for exactly one caller. Reverting `consumeVerificationValue` back to a + * `findVerificationValue` + `deleteVerificationByIdentifier` pair makes + * this test fail with two successes. + * + * @see https://github.com/better-auth/better-auth/security/advisories/GHSA-7w99-5wm4-3g79 + */ + it("rejects concurrent redemption of the same authorization code", async () => { + const { server, customFetchImpl, application, code } = + await setupOAuthFlowAndGetCode(["openid", "profile", "email"]); + + const exchange = () => + customFetchImpl("http://localhost:3000/api/auth/oauth2/token", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + grant_type: "authorization_code", + code, + redirect_uri: "http://localhost:3000/api/auth/oauth2/callback/test", + client_id: application.clientId, + client_secret: application.clientSecret, + }), + }); + + const [first, second] = await Promise.all([exchange(), exchange()]); + const firstBody = (await first.json()) as { + access_token?: string; + error?: string; + }; + const secondBody = (await second.json()) as { + access_token?: string; + error?: string; + }; + + const successes = [firstBody, secondBody].filter( + (b) => b.access_token != null, + ); + const failures = [firstBody, secondBody].filter((b) => b.error != null); + expect(successes).toHaveLength(1); + expect(failures).toHaveLength(1); + expect(failures[0]?.error).toBe("invalid_grant"); + + await server.close(); + }); }); describe("oidc-jwt", async () => { diff --git a/packages/oauth-provider/src/token.test.ts b/packages/oauth-provider/src/token.test.ts index 8fb528e236..728c972a7e 100644 --- a/packages/oauth-provider/src/token.test.ts +++ b/packages/oauth-provider/src/token.test.ts @@ -389,6 +389,51 @@ describe("oauth token - authorization_code", async () => { expect(accessToken.payload.exp).toBe(tokens.data?.expires_at); expect(accessToken.payload.scope).toBe(scopes.join(" ")); }); + + /** + * Concurrent redemption of the same authorization code must mint + * tokens for exactly one caller. Before the atomic-consume primitive, + * two requests could both pass the find step before either delete + * completed and both proceeded to issue token pairs. + * + * The interleaving relies on the in-memory adapter yielding control at + * each `await` boundary; reverting `consumeVerificationValue` to the old + * find/delete pair makes this test fail with two successes (verified + * empirically), so a synthetic scheduling barrier is unnecessary. + * + * @see https://github.com/better-auth/better-auth/security/advisories/GHSA-7w99-5wm4-3g79 + */ + it("rejects concurrent redemption of the same authorization code", async () => { + if (!oauthClient?.client_id || !oauthClient?.client_secret) { + throw Error("beforeAll not run properly"); + } + + const { url: authUrl, codeVerifier } = await createAuthUrl({ + scopes: ["openid", "offline_access"], + }); + + let callbackRedirectUrl = ""; + await client.$fetch(authUrl.toString(), { + onError(context) { + callbackRedirectUrl = context.response.headers.get("Location") || ""; + }, + }); + const code = new URL(callbackRedirectUrl).searchParams.get("code"); + expect(code).toBeTruthy(); + + const [first, second] = await Promise.all([ + validateAuthCode({ code: code!, codeVerifier }), + validateAuthCode({ code: code!, codeVerifier }), + ]); + + const successes = [first, second].filter((r) => r.error === null); + const failures = [first, second].filter((r) => r.error !== null); + expect(successes).toHaveLength(1); + expect(failures).toHaveLength(1); + expect(successes[0]?.data?.access_token).toBeDefined(); + expect(successes[0]?.data?.id_token).toBeDefined(); + expect(failures[0]?.error?.error).toBe("invalid_grant"); + }); }); describe("oauth token - refresh_token", async () => { diff --git a/packages/oauth-provider/src/token.ts b/packages/oauth-provider/src/token.ts index 8b8be19fc7..9bf9cc7b6b 100644 --- a/packages/oauth-provider/src/token.ts +++ b/packages/oauth-provider/src/token.ts @@ -636,26 +636,25 @@ async function checkVerificationValue( client_id: string, redirect_uri?: string, ) { - const verification = await ctx.context.internalAdapter.findVerificationValue( - await storeToken(opts.storeTokens, code, "authorization_code"), - ); + // Atomic single-use redemption per RFC 6749 §4.1.2. The first caller + // receives the row and mints tokens; concurrent racers receive `null` + // and fall through to the `invalid_grant` error path (RFC 6749 §5.2). + const verification = + await ctx.context.internalAdapter.consumeVerificationValue( + await storeToken(opts.storeTokens, code, "authorization_code"), + ); if (!verification) { throw new APIError("UNAUTHORIZED", { error_description: "Invalid code", - error: "invalid_verification", + error: "invalid_grant", }); } - // Delete used code (single-use per RFC 6749 §4.1.2) - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - await storeToken(opts.storeTokens, code, "authorization_code"), - ); - if (!verification.expiresAt || verification.expiresAt < new Date()) { throw new APIError("UNAUTHORIZED", { error_description: "code expired", - error: "invalid_verification", + error: "invalid_grant", }); } @@ -665,14 +664,14 @@ async function checkVerificationValue( } catch { throw new APIError("UNAUTHORIZED", { error_description: "malformed verification value", - error: "invalid_verification", + error: "invalid_grant", }); } const parsed = verificationValueSchema.safeParse(rawValue); if (!parsed.success) { throw new APIError("UNAUTHORIZED", { error_description: "malformed verification value", - error: "invalid_verification", + error: "invalid_grant", }); } // Zod's passthrough adds index signature; the schema already validates the structure