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

[dsschiramm]

Summary

Closes the gap described in #81: the library had no distinction between public and confidential clients (RFC 6749 §2.1). Whether a client_secret was required depended only on the grant_type and the requireClientAuthentication option, not on the client. A public client could therefore be forced to send a secret it cannot keep, and a confidential client could skip its secret on any authorization_code request that used PKCE. This PR adds an optional type attribute ('public' | 'confidential') to the client returned by model.getClient(). The token endpoint decides from that type whether a secret is required. An explicit requireClientAuthentication entry for a grant type still takes precedence for all clients.

Confidential clients, and clients without type, must authenticate with their secret on every token request. PKCE is additive and never replaces the secret. Public clients authenticate without a secret. They must use PKCE for authorization_code, even if they send a secret or requireClientAuthentication waives it. They may use refresh_token without a secret, unless requireClientAuthentication.refresh_token is set. The client_credentials grant type cannot be used with public clients.

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

Obs.: Requests without a secret that no client type could accept are still rejected before model.getClient() is called, as before. When model.getClient() is called without a secret, it receives null instead of undefined.

Obs.: A type other than 'public' or 'confidential' (e.g. 'Confidential' or '') throws a ServerError instead of silently falling back. null counts as no type.

Breaking change: clients without type are treated as confidential. Previously, an authorization_code request with a code_verifier skipped the secret check for every client. Such requests are now rejected without a secret. Migration: return type: 'public' from model.getClient() for clients that use PKCE without a secret (SPAs, native apps).

Linked issue(s)

#81

Involved parts of the project

• lib/handlers/token-handler.js — getClient(), getClientCredentials(), handleGrantType(), isClientAuthenticationRequired(), isClientSecretOptional()
• lib/model.js — ClientData typedef (type property documented), getClient remarks
• index.d.ts — Client interface (type?: 'public' | 'confidential'), requireClientAuthentication docs
• lib/server.js, docs/api/ — requireClientAuthentication docs
• docs/guide/pkce.md — PKCE behavior per client type
• Affected flow: token endpoint (/token) client authentication, all grant types

Added tests?

• test/integration/handlers/token-handler_test.js: end-to-end tests through TokenHandler.handle() for public, confidential and untyped clients:
  • public client with PKCE and no client_secret succeeds; without PKCE it is rejected
  • public client redeeming a code issued without a code_challenge is rejected, also with a secret or under { authorization_code: false }
  • public client with client_credentials is rejected, also under { client_credentials: false }
  • confidential and untyped clients without client_secret are rejected, also with PKCE
  • explicit requireClientAuthentication entries override the client type
  • wrong secrets in the body and in the Basic header (401)
  • codes and refresh tokens issued to another client
  • requests without a secret that no client type could accept are rejected before model.getClient() is called
  • unknown type values throw a ServerError
• test/unit/handlers/token-handler_test.js: model.getClient() is called with null (not undefined) when no secret is provided.

OAuth2 standard

• RFC 6749 §2.1 (Client Types): public clients cannot keep a secret, confidential clients can. This PR adds an optional client.type ('public' | 'confidential') to the model contract. When unset, the client is treated as confidential (fail-closed).
• RFC 6749 §4.1.3 (Access Token Request): confidential clients must authenticate at the token endpoint; public clients are not required to.
• RFC 6749 §4.4 (Client Credentials Grant): only for confidential clients.
• RFC 7636 (PKCE) and RFC 9700 §2.1.1: public clients must use PKCE; PKCE does not replace client authentication.

Reproduction

// model implementation
async function getClient(clientId, clientSecret) {
  const client = await db.findClient(clientId);
  if (!client) return null;
  // a provided secret must always match, also for public clients
  if (clientSecret !== null && client.secret !== clientSecret) return null;
  return { id: client.id, grants: client.grants, type: client.type }; // 'public' or 'confidential'
}

// POST /token, grant_type=authorization_code
// public client, code_verifier, no client_secret       -> succeeds
// public client, no code_verifier                      -> rejected (InvalidClientError)
// confidential client, code_verifier, no client_secret -> rejected (InvalidClientError)
// client without type, code_verifier, no client_secret -> rejected (InvalidClientError)
//
// POST /token, grant_type=client_credentials
// public client                                        -> rejected (UnauthorizedClientError)

[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.

[dsschiramm]

Hi @jankapunkt, thank you for the review; I’ve made adjustments based on the points you mentioned.

1. Public clients and client_credentials
handleGrantType now rejects client_credentials for type: 'public' with UnauthorizedClientError, even when the grant is listed in client.grants and even if requireClientAuthentication.client_credentials is false.

2. Precedence
The unconditional public exemption is gone. isClientAuthenticationRequired(grantType, client, isPkce) now resolves in this order:

• An explicit requireClientAuthentication[grantType] entry always wins, for every client type.
• public: authorization_code needs PKCE and refresh_token needs no secret. Every other grant requires authentication.
• confidential: a secret is always required.
• No type: same behavior as master.

A supplied secret is always passed to model.getClient() for verification. The docs example now validates it for public clients too, so a wrong secret is rejected.

3. PKCE no longer waives the secret for confidential clients
For type: 'confidential', PKCE is additive and a missing secret is rejected. For legacy clients without type, I kept master's behavior, where PKCE on authorization_code waives the secret. That avoids a breaking change for existing deployments. The JSDoc and the PKCE guide document this, and recommend setting type: 'confidential' to get strict enforcement.

4. PKCE for public clients
A public client on authorization_code without a code_verifier is now rejected, which matches RFC 9700 §2.1.1. requirePKCE is not needed for that.

Compatibility and docs

• A missing or empty secret now reaches model.getClient() as null, which matches the documented nullable argument. I dropped the "no behavior change" claim.
• I rewrote the client authentication section of docs/guide/[pkce.md](file:///C:/Users/dssch/Documents/Projects/projects-opensource/node-oauth2-server/docs/guide/pkce.md) to describe the client types, the precedence, and the legacy behavior.

Tests
I added tests through TokenHandler.handle() that cover every row of your table:

• public client with S256 PKCE
• confidential client with and without a secret
• client_credentials rejected for public clients
• explicit requireClientAuthentication
• wrong credentials in the body and in the Basic header (401)
• codes and refresh tokens that belong to another client
• legacy clients

The full suite passes.

I think this covers them, but I'm happy to adjust anything you'd like to see done differently.

[dhensby]

Thanks for the update @dsschiramm, and for working through Jan's review so carefully. Jan's points look addressed to me:

• public clients can't use client_credentials
• an explicit requireClientAuthentication entry wins
• with type: 'confidential', PKCE no longer stands in for the secret

The end-to-end tests through TokenHandler.handle() for every row of his table are great.

On Jan's question from July: I agree PKCE is additive. It binds the code to the request that obtained it, but it's never a substitute for client authentication, which is exactly the public/confidential question this PR is about.

I re-tested the current head (cccaf7a) against master, and a few things aren't covered yet.

1. Clients without type don't behave quite like master

The description and your reply say untyped clients should behave as before. Two differences:

Requests without a secret now reach model.getClient() instead of being rejected up front. On master, a secretless password, client_credentials, refresh_token or non-PKCE authorization_code request gets 400 invalid_client before the model is called. Now getClient(id, null) is called first (token-handler.js:139; the early rejection is gone from getClientCredentials() at :195-199). A model that compares the secret without a null guard, e.g. crypto.timingSafeEqual(Buffer.from(secret), …) or bcrypt.compare(secret, hash), now throws, and the client gets 503 server_error instead of 400 invalid_client.

Could you keep the up-front rejection whenever nothing could make the request acceptable? That is: no secret, no explicit requireClientAuthentication[grant] entry, and the request isn't authorization_code + code_verifier or refresh_token.

requireClientAuthentication: { authorization_code: true } now means something different for untyped clients. On master, an explicit true behaved exactly like leaving the key unset. Now the early return at :317-318 skips the legacy PKCE waiver, so an untyped client doing PKCE without a secret gets invalid_client where master issued a token. I think the stricter behaviour is what someone writing true wants, so I'm happy to keep it. It does need a test with an untyped client (the existing explicit-true tests only use public clients) and a mention in the docs.

2. "Public clients must use PKCE" only holds when they don't send a secret

The rule lives inside the secret-presence check (:153, :322). So a type: 'public' client that sends any client_secret, or any public client under { authorization_code: false }, can still redeem a code that was issued without a code_challenge, with no verifier. Enforcing it per client is a one-liner at :239:

requirePKCE: this.requirePKCE === true || client.type === 'public',

verifyPKCE then rejects with invalid_grant ("issued without a code_challenge"), which is also a clearer error.

3. Unrecognised type values silently get the legacy behaviour

'Confidential', 'CONFIDENTIAL' (e.g. a DB enum passed straight through) or '' fall through to the untyped path, so PKCE still waives the secret. Could you throw a ServerError when type is set but isn't 'public' or 'confidential', the way :145-151 does for a bad grants?

4. Docs and types

The pkce.md rewrite is much better, but a few places still describe the old model:

• lib/server.js:43 and :187, which generate docs/api/server.md, and index.d.ts:235-238 still say per-client enforcement must happen in getClient().
• docs/api/ needs regenerating (npm run docs:api).
• The PR description still says untyped clients are fail-closed with "no behavior change", and still shows the example model Jan flagged.

5. Commit history

We merge with merge commits, so the branch history lands on master as-is. Could you rebase onto master and squash into one commit, or a few atomic ones, following Conventional Commits? Right now there are eight merges from master and a "Potential fix for pull request finding" commit, which commitlint will reject once #335 lands. Please also leave #81 out of the commit subject and link it from the description instead.

Jan and I will also confirm how this lines up with #453, which reworks the same client-authentication code. That doesn't change anything above. Thanks again, the core of this is good.

[dsschiramm]

Thanks @dhensby for the detailed re-test, and to you and @jankapunkt for your patience. I agree that PKCE is additive, and I took that further than you suggested: clients without type are now treated as confidential, so there's no ambiguity about how they behave. PKCE never stands in for the secret. Without an explicit requireClientAuthentication entry, only type: 'public' clients can omit it. This is a breaking change for untyped clients that used PKCE without a secret.

1. Clients without type

• The up-front rejection is back. A request without a secret gets 400 invalid_client before model.getClient() is called, unless the grant has an explicit false entry, or has no entry and is authorization_code + code_verifier or refresh_token. A Basic header with an empty password gets the same Missing parameter: client_secret as master. Tests assert that getClient is not called in these cases.
• Since untyped clients are now confidential, an explicit { authorization_code: true } no longer changes whether they're accepted. Without a secret they're always rejected, just before the model call instead of after. So the untyped-specific test and docs note were no longer needed. The tests cover untyped PKCE with and without a secret, and with { authorization_code: false }.

2. PKCE for public clients
I applied your one-liner. A public client can't redeem a code issued without a code_challenge, whether it sends a secret or runs under { authorization_code: false }. Both cases have tests that expect invalid_grant.

3. Unknown type values
A type other than 'public' or 'confidential' throws a ServerError. 'Confidential', 'CONFIDENTIAL' and '' are covered by tests. null counts as no type, since a nullable column usually means "not set". Let me know if you'd rather it throw.

4. Docs and types
The points you raised were addressed.

5. Commit history
Squashed into a single Conventional Commit on top of current master, with no merges and no #81 in the subject.