mirror of
https://github.com/better-auth/better-auth.git
synced 2026-10-20 20:26:55 -05:00
Merge commit from fork
The `authorization_code` grant's verification step was a `findOne` + `deleteOne` pair, so two concurrent `POST /oauth2/token` requests sharing the same `code` both pass the find, both delete, and both mint independent access/refresh/id token sets: a CAS gap that lets an authorization code be redeemed twice. The legacy `oidc-provider` and `mcp` plugins in `better-auth` share the same primitive on their `authorization_code` paths and have the same gap. All three call sites now use `internalAdapter.consumeVerificationValue` (the atomic primitive added in better-auth#9560 and renamed in better-auth#9568): the first concurrent caller receives the row and mints tokens, subsequent racers receive `null`. The consumed and expired paths return RFC 6749 §5.2 `invalid_grant` instead of the better-auth-internal `invalid_verification`, so spec-compliant clients can branch on the standard code. The redundant second `deleteVerificationByIdentifier` call after PKCE validation in the legacy paths is removed. Closes GHSA-7w99-5wm4-3g79. Co-authored-by: chdanielmueller <4051999+chdanielmueller@users.noreply.github.com>
This commit is contained in:
co-authored by
chdanielmueller
parent
c6918ecc9e
commit
b4bc65a007
@@ -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.
|
||||
@@ -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(
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user