From 5a593b3b2fe12d6f3cbf820dfb4fa237ef89bda9 Mon Sep 17 00:00:00 2001 From: Taesu Date: Fri, 14 Aug 2026 00:35:28 +0900 Subject: [PATCH] fix(oauth-provider): validate ACR claim requests --- packages/oauth-provider/src/authorize.test.ts | 156 ++++++++++-------- packages/oauth-provider/src/claims-request.ts | 43 +++-- .../src/oidc-rs256-profile.test.ts | 16 +- 3 files changed, 128 insertions(+), 87 deletions(-) diff --git a/packages/oauth-provider/src/authorize.test.ts b/packages/oauth-provider/src/authorize.test.ts index 40cf755a1c..b4f5858de8 100644 --- a/packages/oauth-provider/src/authorize.test.ts +++ b/packages/oauth-provider/src/authorize.test.ts @@ -499,12 +499,8 @@ describe("oauth authorize - max_age (OIDC Core 1.0 §3.1.2.1)", async () => { }); }); -describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { - type AcrClaimRequest = { - essential: true; - value?: string; - values?: string[]; - }; +describe("oauth authorize - ACR requests", async () => { + type AcrClaimRequest = Record; const authServerBaseUrl = "http://localhost:3000"; const rpBaseUrl = "http://localhost:5000"; @@ -539,7 +535,7 @@ describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { }); function authorizeUrl( - acrValues: string, + acrValues: string | undefined, acrClaim?: AcrClaimRequest, scope = "openid", ) { @@ -552,7 +548,9 @@ describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { url.searchParams.set("state", "acr-state"); url.searchParams.set("code_challenge", generateRandomString(43)); url.searchParams.set("code_challenge_method", "S256"); - url.searchParams.set("acr_values", acrValues); + if (acrValues !== undefined) { + url.searchParams.set("acr_values", acrValues); + } if (acrClaim) { url.searchParams.set( "claims", @@ -563,7 +561,7 @@ describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { } async function redirectFor( - acrValues: string, + acrValues: string | undefined, acrClaim?: AcrClaimRequest, scope?: string, ) { @@ -576,68 +574,96 @@ describe("oauth authorize - acr_values (OIDC Core 1.0 §3.1.2.1)", async () => { return location; } - it("accepts the advertised level 0 ACR value", async () => { - const location = await redirectFor("0"); - const callbackRedirect = new URL(location); + describe("acr_values (OIDC Core 1.0 §3.1.2.1)", () => { + it("accepts the advertised level 0 ACR value", async () => { + const location = await redirectFor("0"); + const callbackRedirect = new URL(location); - expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( - redirectUri, - ); - expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); - expect(callbackRedirect.searchParams.get("error")).toBeNull(); - }); - - it.each<[string, AcrClaimRequest]>([ - ["value", { essential: true, value: "1" }], - ["values", { essential: true, values: ["1"] }], - ])("rejects an unsupported essential ACR %s", async (_, request) => { - const location = await redirectFor("1", request); - const callbackRedirect = new URL(location); - - expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( - redirectUri, - ); - expect(callbackRedirect.searchParams.get("error")).toBe("access_denied"); - expect(callbackRedirect.searchParams.get("state")).toBe("acr-state"); - expect(callbackRedirect.searchParams.get("code")).toBeNull(); - }); - - it("ignores essential ID Token ACR claims in OAuth-only requests", async () => { - const location = await redirectFor( - "1", - { essential: true, value: "1" }, - "profile", - ); - const callbackRedirect = new URL(location); - - expect(callbackRedirect.searchParams.get("error")).toBeNull(); - expect(callbackRedirect.searchParams.get("code")).toEqual( - expect.any(String), - ); - }); - - it("accepts the current ACR in an essential values request", async () => { - const location = await redirectFor("1", { - essential: true, - values: ["1", "0"], + expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( + redirectUri, + ); + expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); + expect(callbackRedirect.searchParams.get("error")).toBeNull(); }); - const callbackRedirect = new URL(location); - expect(callbackRedirect.searchParams.get("error")).toBeNull(); - expect(callbackRedirect.searchParams.get("code")).toEqual( - expect.any(String), - ); + it("accepts multiple values when one is supported", async () => { + const location = await redirectFor("1 0"); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( + redirectUri, + ); + expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + }); }); - it("accepts multiple requested acr values when one is supported", async () => { - const location = await redirectFor("1 0"); - const callbackRedirect = new URL(location); + describe("claims.id_token.acr (OIDC Core 1.0 §5.5.1.1)", () => { + it("accepts an unsupported voluntary claim", async () => { + const location = await redirectFor(undefined, { values: ["1"] }); + const callbackRedirect = new URL(location); - expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( - redirectUri, - ); - expect(callbackRedirect.searchParams.get("code")).toBeTruthy(); - expect(callbackRedirect.searchParams.get("error")).toBeNull(); + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + expect(callbackRedirect.searchParams.get("code")).toEqual( + expect.any(String), + ); + }); + + it.each<[string, AcrClaimRequest]>([ + ["value", { essential: true, value: "1" }], + ["values", { essential: true, values: ["1"] }], + ])("rejects an unsupported essential %s", async (_, request) => { + const location = await redirectFor(undefined, request); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.origin + callbackRedirect.pathname).toBe( + redirectUri, + ); + expect(callbackRedirect.searchParams.get("error")).toBe("access_denied"); + expect(callbackRedirect.searchParams.get("state")).toBe("acr-state"); + expect(callbackRedirect.searchParams.get("code")).toBeNull(); + }); + + it.each<[string, AcrClaimRequest]>([ + ["value", { essential: true, value: 1 }], + ["values", { essential: true, values: "1" }], + ])("rejects a malformed essential %s", async (_, request) => { + const location = await redirectFor(undefined, request); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.searchParams.get("error")).toBe( + "invalid_request", + ); + expect(callbackRedirect.searchParams.get("state")).toBe("acr-state"); + expect(callbackRedirect.searchParams.get("code")).toBeNull(); + }); + + it("ignores essential claims in OAuth-only requests", async () => { + const location = await redirectFor( + undefined, + { essential: true, value: "1" }, + "profile", + ); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + expect(callbackRedirect.searchParams.get("code")).toEqual( + expect.any(String), + ); + }); + + it("accepts the current ACR in an essential values request", async () => { + const location = await redirectFor(undefined, { + essential: true, + values: ["1", "0"], + }); + const callbackRedirect = new URL(location); + + expect(callbackRedirect.searchParams.get("error")).toBeNull(); + expect(callbackRedirect.searchParams.get("code")).toEqual( + expect.any(String), + ); + }); }); }); diff --git a/packages/oauth-provider/src/claims-request.ts b/packages/oauth-provider/src/claims-request.ts index 5a02560044..558c7710be 100644 --- a/packages/oauth-provider/src/claims-request.ts +++ b/packages/oauth-provider/src/claims-request.ts @@ -1,16 +1,25 @@ import * as z from "zod"; -const claimRequestMemberSchema = z.union([ - z.null(), - z.record(z.string(), z.unknown()), -]); +const claimRequestMemberSchema = z.record(z.string(), z.unknown()).nullable(); -const claimsRequestObjectSchema = z - .object({ - userinfo: z.record(z.string(), claimRequestMemberSchema).optional(), - id_token: z.record(z.string(), claimRequestMemberSchema).optional(), +const acrClaimRequestMemberSchema = z + .looseObject({ + essential: z.boolean().optional(), + value: z.string().optional(), + values: z.array(z.string()).optional(), }) - .passthrough(); + .nullable(); + +const idTokenClaimsRequestSchema = z + .object({ + acr: acrClaimRequestMemberSchema.optional(), + }) + .catchall(claimRequestMemberSchema); + +const claimsRequestObjectSchema = z.looseObject({ + userinfo: z.record(z.string(), claimRequestMemberSchema).optional(), + id_token: idTokenClaimsRequestSchema.optional(), +}); function parseClaimsRequestValue(value: unknown) { if (typeof value === "string") { @@ -31,13 +40,8 @@ function parseClaimsRequestObject(value: unknown) { export const claimsRequestParameterSchema = z .union([z.string(), z.record(z.string(), z.unknown())]) - .superRefine((value, ctx) => { - if (!parseClaimsRequestObject(value)) { - ctx.addIssue({ - code: "custom", - message: "claims must be a JSON object", - }); - } + .refine((value) => parseClaimsRequestObject(value) !== undefined, { + error: "claims must be a valid Claims request object", }); export function getRequestedUserInfoClaims( @@ -58,7 +62,10 @@ export function canSatisfyEssentialAcrRequest( value: unknown, currentAcr: string, ) { - const acrRequest = parseClaimsRequestObject(value)?.id_token?.acr; + const claimsRequest = parseClaimsRequestObject(value); + if (!claimsRequest) return value === undefined; + + const acrRequest = claimsRequest.id_token?.acr; if (!acrRequest || acrRequest.essential !== true) return true; if (typeof acrRequest.value === "string") { @@ -67,7 +74,7 @@ export function canSatisfyEssentialAcrRequest( if (Array.isArray(acrRequest.values)) { return acrRequest.values.includes(currentAcr); } - return true; + return acrRequest.value === undefined && acrRequest.values === undefined; } export function filterClaimsRequestUserInfoClaims( diff --git a/packages/oauth-provider/src/oidc-rs256-profile.test.ts b/packages/oauth-provider/src/oidc-rs256-profile.test.ts index e88044c845..cf2bb17e58 100644 --- a/packages/oauth-provider/src/oidc-rs256-profile.test.ts +++ b/packages/oauth-provider/src/oidc-rs256-profile.test.ts @@ -99,7 +99,17 @@ describe("OpenID Connect RS256 provider profile", async () => { ); }); - it("issues an RS256 ID token with the current ACR for unsupported acr_values", async () => { + it.each<[string, Record]>([ + ["acr_values", { acr_values: "1" }], + [ + "voluntary ACR claim", + { + claims: JSON.stringify({ + id_token: { acr: { values: ["1"] } }, + }), + }, + ], + ])("issues an RS256 ID token with the current ACR for unsupported %s", async (_, additionalParams) => { if (!oauthClient?.client_id || !oauthClient.client_secret) { throw new Error("beforeAll not run properly"); } @@ -118,9 +128,7 @@ describe("OpenID Connect RS256 provider profile", async () => { scopes, codeVerifier, nonce, - additionalParams: { - acr_values: "1", - }, + additionalParams, }); let callbackRedirectUrl = "";