Skip to content

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

Open
dsschiramm wants to merge 2 commits into
node-oauth:masterfrom
dsschiramm:feat/public-confidential-client-distinction
Open

feat(token-handler): distinguish public and confidential clients #81#457
dsschiramm wants to merge 2 commits into
node-oauth:masterfrom
dsschiramm:feat/public-confidential-client-distinction

Conversation

@dsschiramm

Copy link
Copy Markdown

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


try {
const client = await this.model.getClient(credentials.clientId, credentials.clientSecret);
const client = await this.model.getClient(credentials.clientId, credentials.clientSecret ?? null);
Comment on lines 193 to 197
if (pkce.isPKCERequest({ grantType, codeVerifier })) {
if (request.body.client_id) {
return { clientId: request.body.client_id };
}
}
.catch(should.fail);
});

it('should call `model.getClient()` with no secret is provided (public client)', function () {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants