mirror of
https://github.com/better-auth/better-auth.git
synced 2026-05-26 08:56:40 -05:00
fix: cookie store strategy should verify oauth state (#8949)
Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com> Co-authored-by: Gustavo Valverde <g.valverde02@gmail.com>
This commit is contained in:
5
.changeset/famous-banks-open.md
Normal file
5
.changeset/famous-banks-open.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"better-auth": patch
|
||||
---
|
||||
|
||||
security: verify OAuth state parameter against cookie-stored nonce to prevent CSRF on cookie-backed flows
|
||||
@@ -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: [
|
||||
|
||||
@@ -375,6 +375,15 @@ export const oAuthProxy = <O extends OAuthProxyOptions>(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);
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user