Skip to content

feat: add requirePKCE option to enforce PKCE on the authorization code grant#449

Open
dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:feat/require-pkce
Open

feat: add requirePKCE option to enforce PKCE on the authorization code grant#449
dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:feat/require-pkce

Conversation

@dhensby

@dhensby dhensby commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

What

Adds a requirePKCE server option that enforces PKCE on the authorization_code grant.

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_challenge was present — a client could skip PKCE entirely, and the only way to force it was a workaround that throws in model.saveAuthorizationCode.

Behaviour

requirePKCE defaults to false (no change to existing behaviour). When true:

  • Authorize: requests without a code_challenge are rejected with InvalidRequestError — so no PKCE-less codes are ever issued.
  • Token: authorization codes that were issued without a code_challenge are rejected with InvalidGrantError — 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); requirePKCE layers the "PKCE is mandatory" enforcement on top.

Implementation

  • Plumbed requirePKCE through server.jsauthorize-handler + token-handlerauthorization-code-grant-type, mirroring the existing enablePlainPKCE option.
  • Documented in the constructor JSDoc and index.d.ts (AuthorizeOptions + TokenOptions).
  • Compliance tests in 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

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

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 requirePKCE plumbing from OAuth2Server options into authorize + token handling paths.
  • Enforces PKCE at /authorize (reject missing code_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 thread lib/handlers/authorize-handler.js
Comment thread lib/grant-types/authorization-code-grant-type.js Outdated
Comment thread test/compliance/pkce_test.js Outdated
@jankapunkt

jankapunkt commented Jun 17, 2026

Copy link
Copy Markdown
Member

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

I would add this paragraph to the documentation, unless this is already covered by #448

@dhensby dhensby force-pushed the feat/require-pkce branch from ea917ab to 0837585 Compare June 17, 2026 12:25
@dhensby

dhensby commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Good shout. It isn't covered by #448 — that PR only adds the "PKCE vs client authentication & refresh tokens" section, nothing about requiring PKCE. So I've added a ## Requiring PKCE section to docs/guide/pkce.md in 96af831 that documents the option and folds in this rationale (OAuth 2.1 mandates PKCE for the authorization code grant; RFC 9700 §2.1.1 recommends servers require it, incl. the downgrade-attack motivation), plus what requirePKCE: true rejects at each endpoint.

Also addressed Copilot's three notes in the same fixup: code_challenge-missing now uses the standard Missing parameter: phrasing at /authorize, and the no-stored-challenge token rejection now reads Invalid grant: authorization code was issued without a code_challenge`` (with the test updated to match).

…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 dhensby force-pushed the feat/require-pkce branch from 96af831 to b08d51c Compare June 17, 2026 14:38
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.

An option to require PKCE parameters

3 participants