feat: add requirePKCE option to enforce PKCE on the authorization code grant#449
Open
dhensby wants to merge 1 commit into
Open
feat: add requirePKCE option to enforce PKCE on the authorization code grant#449dhensby wants to merge 1 commit into
requirePKCE option to enforce PKCE on the authorization code grant#449dhensby wants to merge 1 commit into
Conversation
…ode grant OAuth 2.1 and RFC 9700 (Security BCP) §2.1.1 require/recommend PKCE for all authorization code flows; previously PKCE was opt-in per request. When `requirePKCE` is enabled (default `false`): - the authorize endpoint rejects requests without a `code_challenge` (InvalidRequestError), so no PKCE-less authorization codes are issued; and - the token endpoint rejects authorization codes issued without a `code_challenge` (InvalidGrantError), closing the gap for pre-existing codes. Plumbed through the server to the authorize and token handlers (mirroring `enablePlainPKCE`), documented in JSDoc/typings, and covered by compliance tests. Existing behaviour is unchanged when the option is off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dhensby
force-pushed
the
feat/require-pkce
branch
from
34c8162 to
ea917ab
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new server option, requirePKCE, to enforce PKCE for the authorization_code grant across both the authorization and token endpoints, aligning the library with OAuth 2.1 / OAuth 2.0 Security BCP guidance.
Changes:
- Introduces
requirePKCEplumbing fromOAuth2Serveroptions into authorize + token handling paths. - Enforces PKCE at
/authorize(reject missingcode_challenge) and at/token(reject codes issued without a stored challenge). - Documents the new option and adds compliance tests covering the new behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/compliance/pkce_test.js | Adds compliance tests validating requirePKCE behavior on authorize + token flows. |
| lib/server.js | Documents the new requirePKCE constructor option in JSDoc. |
| lib/handlers/token-handler.js | Threads requirePKCE into grant type instantiation options. |
| lib/handlers/authorize-handler.js | Rejects authorize requests missing code_challenge when requirePKCE is enabled. |
| lib/grant-types/authorization-code-grant-type.js | Rejects token exchanges for codes lacking a stored PKCE challenge when requirePKCE is enabled. |
| index.d.ts | Exposes requirePKCE in TypeScript option types. |
| docs/api/server.md | Documents requirePKCE in the server API docs table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+106
to
+108
| if (this.requirePKCE && !codeChallenge) { | ||
| throw new InvalidRequestError('Invalid request: `code_challenge` is required'); | ||
| } |
| else { | ||
| if (this.requirePKCE) { | ||
| // PKCE is required, but the authorization code was not associated with a `code_challenge`. | ||
| throw new InvalidGrantError('Invalid grant: code verifier is required'); |
Comment on lines
+764
to
+766
| (error !== null).should.equal(true); | ||
| error.should.be.an.instanceOf(InvalidGrantError); | ||
| error.message.should.equal('Invalid grant: code verifier is required'); |
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.
What
Adds a
requirePKCEserver option that enforces PKCE on theauthorization_codegrant.Closes #179.
Why
OAuth 2.1 makes PKCE mandatory for the authorization code grant (all clients), and RFC 9700 (OAuth 2.0 Security BCP) §2.1.1 / §4.8 recommend authorization servers require it (PKCE downgrade attack). Previously this library only enforced PKCE if a
code_challengewas present — a client could skip PKCE entirely, and the only way to force it was a workaround that throws inmodel.saveAuthorizationCode.Behaviour
requirePKCEdefaults tofalse(no change to existing behaviour). Whentrue:code_challengeare rejected withInvalidRequestError— so no PKCE-less codes are ever issued.code_challengeare rejected withInvalidGrantError— covering codes issued before the option was enabled or via other paths.The library already implemented the RFC 7636 §4.6 downgrade mitigation (challenge ⇒ verifier required; a verifier with no challenge is rejected);
requirePKCElayers the "PKCE is mandatory" enforcement on top.Implementation
requirePKCEthroughserver.js→authorize-handler+token-handler→authorization-code-grant-type, mirroring the existingenablePlainPKCEoption.index.d.ts(AuthorizeOptions+TokenOptions).test/compliance/pkce_test.js: authorize rejects without a challenge, allows with one, and token rejects a no-challenge code.Full suite green (461 passing), lint clean. Strong candidate to become the default in v6 to align with OAuth 2.1.
🤖 Generated with Claude Code