feat(token-handler): distinguish public and confidential clients #81

[dsschiramm]

Summary

RFC 6749 4.1.3 requires public clients be allowed to omit client_secret at the token endpoint. getClientCredentials() previously rejected requests missing a secret before the model was ever consulted, so client.type could never be checked. Moved checking the secret requirement to after the client is fetched so it can be decided based on client.type.

Obs.: Unified the error message to prevent leaking whether a client exists. Both unknown clients and clients missing their required secret now return the same InvalidClientError("Invalid client: client is invalid").

Linked issue(s)

#81

Involved parts of the project

• lib/handlers/token-handler.js — getClient(), getClientCredentials(), isClientAuthenticationRequired()
• lib/model.js — ClientData typedef (type property documented)
• index.d.ts — Client interface (type?: 'public' | 'confidential')
• Affected flow: token endpoint (/token) client authentication, all grant types

Added tests?

• test/integration/handlers/token-handler_test.js: public client with no client_secret succeeds; confidential client (explicit type) with no client_secret throws; confidential client (default, no type set) with no client_secret throws — verifies fail-closed default.
• test/unit/handlers/token-handler_test.js: model.getClient() is called with null (not undefined) when no secret is provided.

OAuth2 standard

RFC 6749 §4.1.3 (Access Token Request) — public clients are not required to authenticate with a client_secret; confidential clients are. This PR adds an optional client.type ('public' | 'confidential') to the model contract, defaulting to 'confidential' when unset (fail-closed — existing integrations that never set type keep requiring a secret, no behavior change for them).

Reproduction

// model implementation
async function getClient(clientId, clientSecret) {
  const client = await db.findClient(clientId);
  if (!client) return null;
  if (client.type !== 'public' && client.secret !== clientSecret) return null;
  return client;
}

// POST /token
// (no client_secret) -> now succeeds if client.type === 'public'
// confidential client omitting client_secret -> still rejected (InvalidClientError)

[Copilot]

Pull request overview

This PR updates the token endpoint client-authentication behavior to distinguish public vs confidential OAuth clients (RFC 6749 §4.1.3), allowing public clients to omit client_secret while keeping a fail-closed default for clients without an explicit type.

Changes:

• Move the “client_secret required” decision to after model.getClient() so it can be based on client.type.
• Extend the model/client contract to include type?: 'public' | 'confidential' (defaulting to confidential behavior when unset).
• Add/adjust unit and integration tests covering public vs confidential clients with missing client_secret.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
lib/handlers/token-handler.js	Defers secret requirement check until after client lookup; adds client.type handling.
lib/model.js	Documents the new ClientData.type attribute.
index.d.ts	Adds type?: 'public' | 'confidential' to the Client TypeScript interface.
test/integration/handlers/token-handler_test.js	Adds integration coverage for public/confidential behavior when client_secret is omitted.
test/unit/handlers/token-handler_test.js	Adds a unit assertion that model.getClient() is called with null when the secret is omitted.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jankapunkt]

Can you please elaborate why this is now fully omitted? The role of PKCE is especially to protect public clients that cannot safely keep a secret.

[dsschiramm]

The !isPkce condition has been relocated. Previously, getClientCredentials had two early-return flows (one for PKCE requests and another for !isClientAuthenticationRequired) that both returned { clientId } without the secret. I consolidated both into a single return, because the decision regarding whether the secret is required or if the PKCE/public-client status excuse it, now take place in getClient after model.getClient() executes, since that's the only place client.type (public vs confidential) is known. Therefore, the check has moved but has not disappeared: getClient still evaluates isClientAuthenticationRequired(grantType, client) && !credentials.clientSecret && !isPkce before accepting a client without a secret, and isClientAuthenticationRequired checks if client is public.

[dsschiramm]

This change in the validation location is also the reason why I unified the error with the !client case: both throw the same InvalidClientError('Invalid client: client is invalid'). Otherwise a client_id that exists but lacks its required secret would be distinguishable from one that doesn't exist.

[jankapunkt]

I'm generally positive for this but we need to keep a close eye on the PKCE. What do you think @dhensby?

[jankapunkt]

hey @dsschiramm I have made my agents to do a review with focus on edge cases and exploiting existing code to obtain token by circumventing current guards. Here is the review:

---

I recommend requesting changes before merging. The direction is sound, but the implementation applies the public-client exemption too broadly and leaves the confidential-client/PKCE distinction incomplete.

I reviewed commit a05991d, ran the existing tests (468 passing), checked formatting/lint for the changed JavaScript files, and exercised additional token-endpoint scenarios.

1. High priority: public clients can use client_credentials without authentication

Location: isClientAuthenticationRequired(), line 307.

if (client && client.type === 'public') return false;

This exemption applies to every grant type. If a model returns:

{ id: 'example', type: 'public', grants: ['client_credentials'] }

the token endpoint can issue an access token using only client_id and grant_type. I reproduced this through TokenHandler.handle().

The condition requires the public client to have that grant configured; it does not affect every public client automatically. Nevertheless, the library now explicitly knows the client is public and still permits a prohibited combination. RFC 6749 §4.4 restricts this grant to confidential clients. rfc-editor.org

Requested change: explicitly reject client_credentials for public clients, even when listed in client.grants. Add a token-endpoint regression test.

2. High priority: type: 'public' overrides an explicit authentication requirement

The same early return executes before checking requireClientAuthentication. Consequently:

requireClientAuthentication: { authorization_code: true }

does not require authentication when the model returns type: 'public'. I reproduced token issuance without a secret under that configuration.

This also exposes a problem with the PR’s interpretation of §4.1.3: authentication is required for confidential clients and for clients issued credentials or assigned other authentication requirements. The section also requires validation when client authentication is supplied. “Public” therefore cannot universally mean “ignore authentication requirements.” rfc-editor.org

Requested change: define and test the precedence between client type, registered authentication requirements, and explicit grant-level configuration.

The PR description’s example model also skips secret validation for every public client:

if (client.type !== 'public' && client.secret !== clientSecret) return null;

That example needs qualification or correction: it can silently accept incorrect supplied credentials. The library delegates credential verification to the model, so the model contract matters here.

3. Important existing gap: PKCE still bypasses the confidential-client secret check

Location: getClient(), line 153.

if (this.isClientAuthenticationRequired(grantType, client)
    && !credentials.clientSecret
    && !isPkce) {

A confidential client with valid PKCE can still receive tokens without a secret if model.getClient() returns it. I reproduced that scenario.

This behavior predates the PR, and the existing model documentation explicitly makes applications responsible for rejecting such confidential clients. It is therefore not a newly introduced, unconditional authentication bypass.

However, it undermines the proposed promise that confidential clients default to requiring authentication. PKCE proves possession of the verifier for a particular authorization transaction; it does not satisfy the confidential client’s separate authentication requirement. rfc-editor.org

Requested change: use the new client distinction to separate authentication policy from PKCE validation. For explicitly confidential clients, PKCE must not waive authentication. Handle legacy clients without type through an explicit compatibility decision.

4. Public-client support should address PKCE enforcement

With the default configuration, I also reproduced authorization-code token issuance for a public client without either a secret or PKCE.

That is not, by itself, a violation of the original §4.1.3. However, RFC 9700 §2.1.1 now requires public clients to use PKCE for authorization-code flows. datatracker.ietf.org

The project already has requirePKCE. I would enforce PKCE for explicitly public authorization-code clients, or clearly document the required deployment configuration. Avoid presenting the new flag alone as sufficient secure public-client support.

Compatibility and documentation

Two smaller changes also deserve attention:

• undefined becomes null. Normalizing the missing secret is consistent with the documented nullable argument, but can affect existing models that distinguish those values or use default parameters. The “no behavior change” claim is too strong.
• The PKCE guide becomes outdated. It still says the library does not model public/confidential clients and recommends a grant-wide authentication exemption for public-client refreshes. Update it alongside this feature.

Tests I would require before approval

The added tests check client retrieval, but do not establish the behavior of a complete token exchange.

Scenario	Expected result
Public authorization-code client, valid S256 PKCE, no secret	Success
Confidential client, valid PKCE, missing secret	Reject
Confidential client, valid secret and PKCE	Success
Public client using client_credentials	Reject
Explicit authentication requirement, missing credentials	Reject
Supplied incorrect credentials	Reject
Authorization code or refresh token belonging to another client	Reject
Legacy client without type	Documented compatibility behavior

Moving the decision until after model lookup is sensible, and obtaining type from the model rather than the request is correct. The merge blocker is the unconditional public-client exemption; the PKCE interaction needs an explicit, tested policy before this becomes a dependable client-type API.

---

I also have them generated the respective tests and can also work on a fix for them. If you add me to your fork repository then I can directly commit the tests and the fixes.