feat(token-handler): distinguish public and confidential clients #81#457
Open
dsschiramm wants to merge 2 commits into
Open
feat(token-handler): distinguish public and confidential clients #81#457dsschiramm wants to merge 2 commits into
dsschiramm wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
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 onclient.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 () { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Added tests?
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