Repository navigation
Conversation
Public clients (token_endpoint_auth_method: 'none') without explicit auth-method provenance are now allowed when the request carries a non-empty client_secret or Basic header. The presented credential is ignored — the server never issued one, so skipping validation does not weaken security (PKCE and refresh rotation protect public clients). This restores 0.8.x behavior for clients whose integration platforms send credentials unconditionally, and prevents ~600 existing grants from breaking on upgrade. Fixes cloudflare#404
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Bug fix has no changeset
Repository rules require a changeset for bug fixes. This PR changes only the provider implementation, leaving no release entry.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (client.tokenEndpointAuthMethod === 'none' && isSecretMethod(presentedMethod)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🔍 Public-client tests still expect rejection
Existing tests expect 401 invalid_client for unmarked public clients presenting a non-empty secret. The new behavior accepts those requests, so the suite and compatibility coverage need updating.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Allow public clients that receive a spurious credential. Ignoring a | ||
| // secret the server never issued does not weaken public-client security, | ||
| // which rests on PKCE and refresh rotation (RFC 6749 §2.1). | ||
| if (client.tokenEndpointAuthMethod === 'none' && isSecretMethod(presentedMethod)) { | ||
| return true; |
There was a problem hiding this comment.
🔍 Stored-client documentation contradicts new behavior
The authMethodExplicit storage note says public clients never receive compatibility. It now needs to distinguish unmarked public clients from explicit registrations.
Was this helpful? React with 👍 or 👎 to provide feedback.
The normalization that overrides presentedAuthMethod to 'none' for backward-compat public clients was also firing for CIMD clients whose tokenEndpointAuthMethod is 'none' and authMethodExplicit is absent. This caused valid private_key_jwt assertions from CIMD clients to skip verifyClientAssertion, silently accepting unauthenticated requests. - Extract isCimdClient bool from the existing inline expression so it can be reused without duplication. - Move the presentedAuthMethod normalization to AFTER the isClientAuthMethodAllowed check (it was previously before, meaning it ran even for clients that would be rejected). - Gate normalization on !isCimdClient so CIMD assertion flows are never affected.
| !isCimdClient && | ||
| registeredAuthMethod === 'none' && | ||
| presentedAuthMethod !== 'none' && | ||
| clientInfo.authMethodExplicit === undefined |
There was a problem hiding this comment.
🔍 Mixed-method CIMD assertion needs a regression test
A CIMD document offering none and private_key_jwt exercises the new normalization boundary. Add a token request with an invalid assertion to pin verification on that path.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Restores backward compatibility for public clients (
token_endpoint_auth_method: "none") that send aclient_secretor BasicAuthorizationheader at/token. Since 1.x (#276, #287), these requests are rejected with401 invalid_client(token_endpoint_auth_method_mismatch), breaking integrations that unconditionally include credentials.Empty
client_secret=is already handled (filtered as omitted per RFC 6749 §3.2). This PR extends backward compatibility to non-empty credentials on public clients without explicit auth-method provenance.Changes
src/oauth-provider.ts:isClientAuthMethodAllowed: add backward compat case fornoneclients receiving a secret method whenauthMethodExplicitis absentpresentedAuthMethodto'none'after the compat check so downstream secret validation is skipped (the server never issued a secret)Security rationale
Ignoring a credential the server never issued does not weaken public-client security, which rests on PKCE and refresh rotation (RFC 6749 §2.1). This matches 0.8.x behavior.
Test plan
client_secret=valuein POST body → accepted asnonenoneclient_secret=→ accepted (existing behavior)authMethodExplicitset is NOT compat-allowed (only legacy records)Fixes #404