diff --git a/.changeset/oidc-offline-access-nonce.md b/.changeset/oidc-offline-access-nonce.md new file mode 100644 index 0000000000..b3ab205a05 --- /dev/null +++ b/.changeset/oidc-offline-access-nonce.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +Allows confidential OIDC clients that have opted out of PKCE to request `offline_access` when the authorization request includes both `openid` and `nonce`. diff --git a/docs/content/docs/plugins/oauth-provider.mdx b/docs/content/docs/plugins/oauth-provider.mdx index 9630e6e18a..74a271f720 100644 --- a/docs/content/docs/plugins/oauth-provider.mdx +++ b/docs/content/docs/plugins/oauth-provider.mdx @@ -1354,7 +1354,7 @@ By default, PKCE is required for all clients. This provides maximum security and **PKCE is always required for:** * Public clients (native/user-agent-based applications) -* Any authorization request with the `offline_access` scope (refresh tokens) +* Authorization requests with the `offline_access` scope, unless a confidential client has opted out of PKCE and the OIDC request includes both `openid` and `nonce` #### Per-Client PKCE Configuration @@ -1379,7 +1379,7 @@ The `require_pkce` field: * Defaults to `true` (PKCE required) * Only applies to confidential clients * Ignored for public clients (PKCE always required) -* Ignored for `offline_access` scope (PKCE always required) +* Requires an OIDC request with both `openid` and `nonce` when `offline_access` is requested without PKCE #### Dynamic Client Registration PKCE Configuration @@ -1392,7 +1392,7 @@ oauthProvider({ }) ``` -This only applies to confidential clients created through dynamic client registration. Public clients and authorization requests with the `offline_access` scope still require PKCE. +This only applies to confidential clients created through dynamic client registration. Public clients still require PKCE. Confidential OIDC clients that request `offline_access` without PKCE must send both `openid` and `nonce`. **When to use `require_pkce: false`:** diff --git a/packages/oauth-provider/src/authorize.ts b/packages/oauth-provider/src/authorize.ts index 9b2e012623..a43fcf9761 100644 --- a/packages/oauth-provider/src/authorize.ts +++ b/packages/oauth-provider/src/authorize.ts @@ -583,8 +583,11 @@ export async function authorizeEndpoint( } } - // Check if PKCE is required for this client and scope - const pkceRequired = isPKCERequired(client, requestedScopes); + // Check if PKCE is required for this client and authorization request + const pkceRequired = isPKCERequired(client, { + scopes: requestedScopes, + nonce: query.nonce, + }); // Validate PKCE parameters if required if (pkceRequired) { diff --git a/packages/oauth-provider/src/pkce-optional.test.ts b/packages/oauth-provider/src/pkce-optional.test.ts index 0f9e6d3893..577b4641a7 100644 --- a/packages/oauth-provider/src/pkce-optional.test.ts +++ b/packages/oauth-provider/src/pkce-optional.test.ts @@ -6,6 +6,7 @@ import { } from "better-auth/oauth2"; import { jwt } from "better-auth/plugins/jwt"; import { getTestInstance } from "better-auth/test"; +import { decodeJwt } from "jose"; import { beforeAll, describe, expect, it, vi } from "vitest"; import { oauthProviderClient } from "./client"; import { oauthProvider } from "./oauth"; @@ -465,8 +466,7 @@ describe("PKCE optional - offline_access scope", async () => { confidentialClient = confResponse; }); - it("offline_access without PKCE should fail even with requirePKCE: false", async () => { - // Try to authorize with offline_access but without PKCE + it("offline_access without PKCE or OIDC nonce should fail even with requirePKCE: false", async () => { const authUrl = new URL(`${authServerBaseUrl}/api/auth/oauth2/authorize`); authUrl.searchParams.set("client_id", confidentialClient.client_id); authUrl.searchParams.set("redirect_uri", redirectUri); @@ -483,10 +483,96 @@ describe("PKCE optional - offline_access scope", async () => { expect(errorRedirect).toContain("error=invalid_request"); expect(errorRedirect).toContain( - "pkce+is+required+when+requesting+offline_access+scope", + "pkce+or+OIDC+nonce+is+required+when+requesting+offline_access+scope", ); }); + it("offline_access without PKCE should succeed for confidential OIDC requests with nonce", async () => { + const nonce = "offline-access-nonce"; + const authUrl = await createAuthorizationURL({ + id: providerId, + options: { + clientId: confidentialClient.client_id, + clientSecret: confidentialClient.client_secret, + }, + redirectURI: redirectUri, + state: "123", + scopes: ["openid", "offline_access"], + responseType: "code", + authorizationEndpoint: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + nonce, + }); + + let callbackUrl = ""; + await authenticatedClient.$fetch(authUrl.toString(), { + onError(context) { + callbackUrl = context.response.headers.get("Location") || ""; + }, + }); + + expect(callbackUrl).toContain(redirectUri); + expect(callbackUrl).toContain("code="); + expect(callbackUrl).not.toContain("error="); + + const url = resolveUrl(callbackUrl, authServerBaseUrl); + const code = url.searchParams.get("code"); + expect(code).toBeDefined(); + + const { body, headers } = await authorizationCodeRequest({ + code: code!, + redirectURI: redirectUri, + options: { + clientId: confidentialClient.client_id, + clientSecret: confidentialClient.client_secret, + redirectURI: redirectUri, + }, + }); + + const tokenResponse = await authenticatedClient.$fetch<{ + access_token?: string; + id_token?: string; + refresh_token?: string; + }>("/oauth2/token", { + method: "POST", + body, + headers, + }); + + expect(tokenResponse.data?.access_token).toBeDefined(); + expect(tokenResponse.data?.id_token).toBeDefined(); + expect(tokenResponse.data?.refresh_token).toBeDefined(); + expect(decodeJwt(tokenResponse.data!.id_token!).nonce).toBe(nonce); + }); + + it("offline_access without PKCE should fail for non-OIDC requests with nonce", async () => { + const authUrl = await createAuthorizationURL({ + id: providerId, + options: { + clientId: confidentialClient.client_id, + clientSecret: confidentialClient.client_secret, + }, + redirectURI: redirectUri, + state: "123", + scopes: ["offline_access"], + responseType: "code", + authorizationEndpoint: `${authServerBaseUrl}/api/auth/oauth2/authorize`, + nonce: "unused-without-openid", + }); + + let errorRedirect = ""; + await authenticatedClient.$fetch(authUrl.toString(), { + onError(context) { + errorRedirect = context.response.headers.get("Location") || ""; + }, + }); + + expect(errorRedirect).toContain("error=invalid_request"); + expect(errorRedirect).toContain( + "pkce+or+OIDC+nonce+is+required+when+requesting+offline_access+scope", + ); + expect(errorRedirect).not.toContain("code="); + }); + it("offline_access with PKCE should succeed", async () => { const codeVerifier = generateRandomString(64); const authUrl = await createAuthorizationURL({ diff --git a/packages/oauth-provider/src/token.ts b/packages/oauth-provider/src/token.ts index a564d1c270..48c9316b3d 100644 --- a/packages/oauth-provider/src/token.ts +++ b/packages/oauth-provider/src/token.ts @@ -1237,8 +1237,11 @@ async function handleAuthorizationCodeGrant( const requestedScopes = (verificationValue.query?.scope as string)?.split(" ") || []; - // Check if PKCE is required for this client - const pkceRequired = isPKCERequired(client, requestedScopes); + // Check if PKCE is required for this client and authorization request + const pkceRequired = isPKCERequired(client, { + scopes: requestedScopes, + nonce: verificationValue.query?.nonce, + }); // Validate credentials based on requirements if (pkceRequired) { diff --git a/packages/oauth-provider/src/types/index.ts b/packages/oauth-provider/src/types/index.ts index 5ddd321eb0..ad8341c3e5 100644 --- a/packages/oauth-provider/src/types/index.ts +++ b/packages/oauth-provider/src/types/index.ts @@ -717,7 +717,8 @@ export interface OAuthOptions< * * This is server-owned registration policy. Dynamic client registration does * not accept `require_pkce` from the client request, and public clients or - * authorization requests with `offline_access` still require PKCE. + * authorization requests with `offline_access` still require PKCE unless the + * confidential OIDC request includes both `openid` and `nonce`. * * @default true */ diff --git a/packages/oauth-provider/src/types/oauth.ts b/packages/oauth-provider/src/types/oauth.ts index e8450d6e12..ea6cf928ee 100644 --- a/packages/oauth-provider/src/types/oauth.ts +++ b/packages/oauth-provider/src/types/oauth.ts @@ -392,8 +392,9 @@ export interface OAuthClient { * * @default true * - * Note: PKCE is always required for public clients and when - * requesting offline_access scope, regardless of this setting. + * Note: PKCE is always required for public clients. When requesting + * offline_access without PKCE, confidential OIDC clients must send both + * `openid` and `nonce`. */ require_pkce?: boolean; /** diff --git a/packages/oauth-provider/src/utils/index.ts b/packages/oauth-provider/src/utils/index.ts index b60d16a06d..01ae835f6c 100644 --- a/packages/oauth-provider/src/utils/index.ts +++ b/packages/oauth-provider/src/utils/index.ts @@ -915,17 +915,24 @@ export function removeMaxAgeFromQuery(query: URLSearchParams) { enum PKCERequirementErrors { PUBLIC_CLIENT = "pkce is required for public clients", - OFFLINE_ACCESS_SCOPE = "pkce is required when requesting offline_access scope", + OFFLINE_ACCESS_SCOPE = "pkce or OIDC nonce is required when requesting offline_access scope", CLIENT_REQUIRE_PKCE = "pkce is required for this client", } + +interface AuthorizationPKCEContext { + scopes?: string[]; + nonce?: string; +} + /** - * Determines if PKCE is required for a given client and scope. + * Determines if PKCE is required for a given client and authorization request. * * PKCE is always required for: * 1. Public clients (cannot securely store client_secret) - * 2. Requests with offline_access scope (refresh token security) + * 2. Requests with offline_access scope unless a confidential OIDC request + * already uses nonce as its authorization-code injection countermeasure. * - * For confidential clients without offline_access: + * For confidential clients: * - Uses client.requirePKCE if set (defaults to true) * * Returns false if PKCE is not required, or the reason it is required. @@ -934,7 +941,7 @@ enum PKCERequirementErrors { */ export function isPKCERequired( client: SchemaClient, - requestedScopes?: string[], + request?: AuthorizationPKCEContext, ): false | PKCERequirementErrors { // Determine if client is public const isPublicClient = @@ -948,8 +955,13 @@ export function isPKCERequired( return PKCERequirementErrors.PUBLIC_CLIENT; } - // PKCE always required for offline_access scope (refresh tokens) - if (requestedScopes?.includes("offline_access")) { + const requestScopes = request?.scopes ?? []; + const isOpenIdRequest = requestScopes.includes("openid"); + const hasNonce = + typeof request?.nonce === "string" && request.nonce.length > 0; + const hasOidcNonce = isOpenIdRequest && hasNonce; + + if (requestScopes.includes("offline_access") && !hasOidcNonce) { return PKCERequirementErrors.OFFLINE_ACCESS_SCOPE; }