From f5ea731e6de511e1df793a190274e6bbca37206d Mon Sep 17 00:00:00 2001 From: Taesu <166604494+bytaesu@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:38:37 +0000 Subject: [PATCH] fix(sso): verify OIDC ID tokens with the shared step (#11935) --- .changeset/sso-id-token-claims.md | 5 ++ packages/core/package.json | 8 ++ packages/core/src/oauth2/internal.ts | 1 + .../core/src/oauth2/verify-id-token.test.ts | 11 +-- packages/core/src/oauth2/verify-id-token.ts | 73 +++++++++++-------- packages/core/tsdown.config.ts | 1 + packages/sso/src/oidc/discovery.ts | 14 ++-- packages/sso/src/oidc/id-token.test.ts | 73 ++++++++++++++++++- 8 files changed, 138 insertions(+), 48 deletions(-) create mode 100644 .changeset/sso-id-token-claims.md create mode 100644 packages/core/src/oauth2/internal.ts diff --git a/.changeset/sso-id-token-claims.md b/.changeset/sso-id-token-claims.md new file mode 100644 index 0000000000..8ea09628a5 --- /dev/null +++ b/.changeset/sso-id-token-claims.md @@ -0,0 +1,5 @@ +--- +"@better-auth/sso": patch +--- + +OIDC SSO sign-in now rejects ID tokens missing `exp` or `iat` and providers with an empty client ID, matching social sign-in. diff --git a/packages/core/package.json b/packages/core/package.json index ee0bd9d357..ba51771c99 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -104,6 +104,11 @@ "types": "./dist/oauth2/index.d.mts", "default": "./dist/oauth2/index.mjs" }, + "./oauth2/internal": { + "dev-source": "./src/oauth2/internal.ts", + "types": "./dist/oauth2/internal.d.mts", + "default": "./dist/oauth2/internal.mjs" + }, "./instrumentation": { "dev-source": "./src/instrumentation/index.ts", "types": "./dist/instrumentation/index.d.mts", @@ -158,6 +163,9 @@ "oauth2": [ "dist/oauth2/index.d.mts" ], + "oauth2/internal": [ + "dist/oauth2/internal.d.mts" + ], "instrumentation": [ "dist/instrumentation/index.d.mts" ] diff --git a/packages/core/src/oauth2/internal.ts b/packages/core/src/oauth2/internal.ts new file mode 100644 index 0000000000..24a8db2afc --- /dev/null +++ b/packages/core/src/oauth2/internal.ts @@ -0,0 +1 @@ +export { verifyIdToken } from "./verify-id-token"; diff --git a/packages/core/src/oauth2/verify-id-token.test.ts b/packages/core/src/oauth2/verify-id-token.test.ts index dc8aa83764..f237ea2175 100644 --- a/packages/core/src/oauth2/verify-id-token.test.ts +++ b/packages/core/src/oauth2/verify-id-token.test.ts @@ -1,8 +1,7 @@ import { createHash } from "node:crypto"; import type { JWK } from "jose"; import { createLocalJWKSet, exportJWK, generateKeyPair, SignJWT } from "jose"; -import { afterEach, describe, expect, it, vi } from "vitest"; -import { logger } from "../env"; +import { describe, expect, it } from "vitest"; import { apple } from "../social-providers/apple"; import { cognito } from "../social-providers/cognito"; import { facebook } from "../social-providers/facebook"; @@ -261,17 +260,12 @@ describe("verifyProviderIdToken", () => { }); describe("without a configured audience", () => { - afterEach(() => { - vi.restoreAllMocks(); - }); - it.each<[string, string | string[] | undefined]>([ ["missing", undefined], ["an empty string", ""], ["an empty list", []], ["a list of empty strings", [""]], ])("rejects a token when the audience is %s", async (_, audience) => { - const error = vi.spyOn(logger, "error").mockImplementation(() => {}); const { jwks, sign } = await makeKeyset(); const provider = providerWith({ jwks, @@ -283,9 +277,6 @@ describe("verifyProviderIdToken", () => { await expect( verifyProviderIdToken(provider, await sign({ sub: "u1" })), ).resolves.toBe(false); - expect(error).toHaveBeenCalledWith( - expect.stringContaining("no client ID"), - ); }); }); diff --git a/packages/core/src/oauth2/verify-id-token.ts b/packages/core/src/oauth2/verify-id-token.ts index b7ac9b8a17..f029d19be9 100644 --- a/packages/core/src/oauth2/verify-id-token.ts +++ b/packages/core/src/oauth2/verify-id-token.ts @@ -1,8 +1,11 @@ import type { JWTVerifyOptions } from "jose"; import { jwtVerify } from "jose"; -import { logger } from "../env"; import type { GenericEndpointContext } from "../types"; -import type { OAuthProvider, ProviderOptions } from "./oauth-provider"; +import type { + OAuthIdTokenConfig, + OAuthProvider, + ProviderOptions, +} from "./oauth-provider"; /** * Clock skew allowed on `exp`, `nbf`, and `iat`, matching openid-client and oauth4webapi. @@ -69,6 +72,44 @@ async function nonceMatches( return false; } +/** + * Verifies an ID token's signature and claims against a verification config, + * and its `nonce` when one is expected. Throws when the token is not valid. + * + * @see https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation + * @internal + */ +export async function verifyIdToken( + token: string, + config: Extract, + nonce?: string, +) { + // An empty audience would skip the audience check entirely. + if (![config.audience].flat().some(Boolean)) { + throw new Error("ID token audience is not configured"); + } + // `kid` is optional in JWS: a JWKS resolver can still select a key by algorithm, so + // key selection is left to config.jwks. + const verified = await jwtVerify( + token, + config.jwks, + getIdTokenVerifyOptions(config), + ); + if ( + nonce && + !(await nonceMatches(verified.payload.nonce, nonce, config.nonceComparison)) + ) { + throw new Error("ID token nonce does not match"); + } + // Provider-specific claim check on the now-verified payload (e.g. Google's + // hosted-domain `hd`). Standard checks have already passed, so a token that + // fails here is authentic but does not meet the provider's extra constraint. + if (config.verifyClaims && !config.verifyClaims(verified.payload)) { + throw new Error("ID token claims were rejected"); + } + return verified; +} + /** * Whether a provider can verify a client-submitted id_token. * @@ -128,33 +169,7 @@ export async function verifyProviderIdToken( if (token.split(".").length !== 3) { return config.allowOpaqueToken === true; } - // An empty audience would skip the audience check entirely. - const hasAudience = [config.audience].flat().some(Boolean); - if (!hasAudience) { - logger.error( - `[better-auth] Rejected an ID token for "${provider.id}" because the provider has no client ID to check its audience against. Set \`clientId\` on the provider.`, - ); - return false; - } - // `kid` is optional in JWS: a JWKS resolver can still select a key by algorithm, so - // key selection is left to config.jwks. - const { payload } = await jwtVerify( - token, - config.jwks, - getIdTokenVerifyOptions(config), - ); - if ( - nonce && - !(await nonceMatches(payload.nonce, nonce, config.nonceComparison)) - ) { - return false; - } - // Provider-specific claim check on the now-verified payload (e.g. Google's - // hosted-domain `hd`). Standard checks have already passed, so a token that - // fails here is authentic but does not meet the provider's extra constraint. - if (config.verifyClaims && !config.verifyClaims(payload)) { - return false; - } + await verifyIdToken(token, config, nonce); return true; } catch { return false; diff --git a/packages/core/tsdown.config.ts b/packages/core/tsdown.config.ts index cb68b41574..f69e78e14e 100644 --- a/packages/core/tsdown.config.ts +++ b/packages/core/tsdown.config.ts @@ -18,6 +18,7 @@ export default defineConfig({ "./src/context/index.ts", "./src/env/index.ts", "./src/oauth2/index.ts", + "./src/oauth2/internal.ts", "./src/api/index.ts", "./src/social-providers/index.ts", "./src/utils/*.ts", diff --git a/packages/sso/src/oidc/discovery.ts b/packages/sso/src/oidc/discovery.ts index e804e36fe5..51e7f0e743 100644 --- a/packages/sso/src/oidc/discovery.ts +++ b/packages/sso/src/oidc/discovery.ts @@ -8,13 +8,14 @@ * @see https://openid.net/specs/openid-connect-discovery-1_0.html */ +import { verifyIdToken } from "@better-auth/core/oauth2/internal"; import { classifyHost, isPublicRoutableHost, } from "@better-auth/core/utils/host"; import type { BetterFetchOption } from "@better-fetch/fetch"; import { betterFetch } from "@better-fetch/fetch"; -import { createRemoteJWKSet, customFetch, jwtVerify } from "jose"; +import { createRemoteJWKSet, customFetch } from "jose"; import type { OIDCConfig } from "../types"; import type { DiscoverOIDCConfigParams, @@ -425,8 +426,8 @@ export async function validateOIDCIdToken( token: string, jwksEndpoint: string, options: { - audience?: string | string[]; - issuer?: string | string[]; + audience: string; + issuer: string; }, isTrustedOrigin: (url: string) => boolean, ) { @@ -434,13 +435,12 @@ export async function validateOIDCIdToken( [customFetch]: (url, init) => fetchOIDCEndpointResponse("jwksEndpoint", url, init, isTrustedOrigin), }); - const verified = await jwtVerify(token, jwks, { + const verified = await verifyIdToken(token, { + jwks, audience: options.audience, issuer: options.issuer, }); - if (typeof options.audience === "string") { - assertOIDCAuthorizedParty(verified.payload, options.audience); - } + assertOIDCAuthorizedParty(verified.payload, options.audience); return verified; } diff --git a/packages/sso/src/oidc/id-token.test.ts b/packages/sso/src/oidc/id-token.test.ts index 859e58e925..b4ca86fb88 100644 --- a/packages/sso/src/oidc/id-token.test.ts +++ b/packages/sso/src/oidc/id-token.test.ts @@ -1,5 +1,6 @@ -import { describe, expect, it } from "vitest"; -import { assertOIDCAuthorizedParty } from "./discovery"; +import { exportJWK, generateKeyPair, SignJWT } from "jose"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { assertOIDCAuthorizedParty, validateOIDCIdToken } from "./discovery"; describe("OIDC authorized party validation", () => { it("accepts the configured client as azp for a multi-audience token", () => { @@ -30,3 +31,71 @@ describe("OIDC authorized party validation", () => { }, ); }); + +/** + * @see https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation + */ +describe("OIDC ID token claim validation", async () => { + const jwksEndpoint = "https://idp.example.com/jwks"; + const issuer = "https://idp.example.com"; + const clientId = "workforce-client"; + const { publicKey, privateKey } = await generateKeyPair("RS256"); + const jwks = { keys: [{ ...(await exportJWK(publicKey)), alg: "RS256" }] }; + + const signIdToken = (claims: { exp?: string; iat?: boolean }) => { + const jwt = new SignJWT({}) + .setProtectedHeader({ alg: "RS256" }) + .setIssuer(issuer) + .setAudience(clientId) + .setSubject("user-1"); + if (claims.exp) jwt.setExpirationTime(claims.exp); + if (claims.iat) jwt.setIssuedAt(); + return jwt.sign(privateKey); + }; + const validate = (token: string, audience = clientId) => + validateOIDCIdToken( + token, + jwksEndpoint, + { + audience, + issuer, + }, + () => true, + ); + + beforeEach(() => { + vi.stubGlobal( + "fetch", + vi.fn(async () => Response.json(jwks)), + ); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("accepts an ID token with exp and iat", async () => { + const token = await signIdToken({ exp: "5m", iat: true }); + + await expect(validate(token)).resolves.toMatchObject({ + payload: { sub: "user-1" }, + }); + }); + + it.each([ + { claim: "exp", claims: { iat: true } }, + { claim: "iat", claims: { exp: "5m" } }, + ])("rejects an ID token without $claim", async ({ claim, claims }) => { + const token = await signIdToken(claims); + + await expect(validate(token)).rejects.toThrow(`"${claim}" claim`); + }); + + it("rejects an ID token when the provider has no client ID", async () => { + const token = await signIdToken({ exp: "5m", iat: true }); + + await expect(validate(token, "")).rejects.toThrow( + "ID token audience is not configured", + ); + }); +});