Add PKCE support (RFC 7636) - #1
Open
roborourke wants to merge 13 commits into
Open
Conversation
RFC 7636 requires the S256 code challenge to be base64url (unpadded) of the raw 32-byte SHA-256 digest. The upstream PKCE PR (WP-API#46) instead base64-encodes the hex digest with standard base64, which produces an 88-character value no compliant client can ever match. Centralising the transform in one class stops the token endpoint and the WP-CLI helper from being able to drift from each other, and gives it one place to unit test against the RFC's own Appendix B test vector. Method comparison is case-sensitive per RFC 7636 section 4.3, and challenge validation is method-aware (S256 challenges are always 43 base64url characters; plain challenges follow the verifier's ABNF). Every comparison fails closed rather than raising a PHP 8 TypeError on unrecognised input, since hash_equals() requires string arguments. While bumping the PHP floor comment in plugin.php's header, since composer.json already requires >=7.4 and the header still said 5.6.
Authorization_Code::create() now accepts an optional $data array, whitelisting only code_challenge and code_challenge_method rather than merging the caller's array over the stored value — the upstream PR does an array_merge() with caller data last, so a future caller could overwrite the stored user or expiration. The challenge is only stored when one is actually supplied, so a non-PKCE code's meta shape is unchanged from before this existed. validate() gains a code_verifier check: a code minted with a challenge requires a matching verifier; a code minted without one rejects a verifier by default (behind a filter), since that is the signature of a code obtained some other way rather than a legitimate omission. Every access to the supplied args is guarded, so calling validate() with no args at all — what every existing caller does — keeps returning true for a non-PKCE code. Also fixes a latent bug get_expiration() can return a WP_Error, but validate() compared it directly against time() with <=. On PHP 8 that object-to-int comparison treats the WP_Error as greater than any timestamp, so a code with corrupted meta was passing the expiry check.
Client::is_pkce_required() reads a new _oauth2_pkce_required meta key,
wrapped in an oauth2.pkce.required filter so a site can force it for
every client. generate_authorization_code() gains an optional $data
parameter to carry the PKCE fields through to the stored code; adding
an optional parameter to an implementation is not a BC break (PHP
permits an implementing method to accept more optional arguments than
its interface declares), so ClientInterface and PersonalClient are
untouched.
update()'s meta loop previously wrote every field unconditionally from
$data['meta'], coercing an absent key to false — so any partial update
silently disabled client_credentials_enabled, and would have done the
same to the new PKCE flag. Rebuilt as a map of meta key to data key,
skipping any key the caller didn't pass, so an update that omits a
field leaves it as it was.
Also fixes the shared test helper's client type, 'web', which is not
one of the two values ('public'/'private') the admin UI ever writes.
Harmless today since nothing branches on it, but it stops being
harmless the day client-type-based auth (upstream OAuth2#36) lands.
Adds a gather_extra_params() seam to Types\Base, called after the redirect URI is validated (so an error has somewhere safe to report to) and before the login redirect (no point sending a user through login for an already-malformed request). It runs on both the initial GET and the consent-form POST, since the authorisation form posts back to the original request URI. A brand-new protected method is the only backwards-compatible way to add this: widening an existing method like get_nonce_action() would fatal any subclass overriding it with the old arity. Types\Authorization_Code implements the hook to validate code_challenge and code_challenge_method: defaults the method to 'plain' when omitted per RFC 7636 section 4.3, rejects unsupported or wrongly-cased methods, validates the challenge shape, and requires S256 specifically when the client has PKCE required (plain offers no protection against a malicious app on the same device reading the request, which is exactly the threat PKCE exists to mitigate per RFC 9700 section 2.1.1). Types\Implicit inherits none of this — it mints no code, so there is nothing for a challenge to bind to — but now explicitly refuses a PKCE-required client instead of silently ignoring the requirement, which is the one bypass the upstream PR left wide open. PKCE errors at the authorisation endpoint redirect to the client with an error/error_description query pair (fragment, for the implicit grant) rather than the existing wp_die(), per RFC 7636 section 4.4.1 — important in practice because PKCE exists for native and mobile clients, which cannot parse or display an HTML error page. The rule this follows generally, not just for PKCE: an error before the redirect URI is validated dies; an error after it redirects.
Declares code_verifier in the /oauth2/access_token route schema and passes it through to Authorization_Code::validate(). Read via get_body_params()/get_json_params() rather than get_param(), which also reads $_GET on a POST route — a verifier landing in the URL is far more likely to end up in access logs or a Referer header than one kept in the body.
Adds a "Require PKCE (S256)" checkbox, following the existing client_credentials_enabled field's trail through validate_parameters(), both meta arrays in handle_edit_submit(), the $data hydration in render_edit_page(), and a new <tr>. Labelled explicitly as S256, not just "PKCE", since plain does not satisfy the requirement. New clients default to checked, but only on a genuinely fresh "Add Application" page — keyed on empty($consumer) && empty($form_data), not empty($consumer) alone, since the same hydration branch also handles redisplaying a failed submission, where empty($consumer) is still true but the box should reflect what was actually submitted.
Adds code_challenge_methods_supported to the oauth2 entry in the REST index response, alongside the existing grant_types. This is the discovery half of what a PKCE-aware client expects from a compliant server, and it tracks the oauth2.pkce.supported_methods filter rather than hardcoding the default.
wp oauth2 generate-code-challenge derives a code_challenge from a code_verifier (randomly generated, or supplied) using PKCE::, so there is exactly one implementation of the transform in the plugin rather than a second copy that could drift from the one the token endpoint checks against. Useful for manually exercising the authorization_code + PKCE flow without a full client.
Adds a README section covering the code_challenge/code_challenge_method parameters, the S256 transform with the RFC 7636 worked example as a value a client implementer can check their own code against, the S256-only rule when PKCE is required, the three filters, and the WP-CLI helper. Also notes the redirect_args filters' $data now carries PKCE fields.
This was referenced Aug 31, 2026
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
# Conflicts: # inc/tokens/class-authorization-code.php # inc/types/class-implicit.php
WPCS 3.4.1's alignment sniff wants the = signs lined up across the two adjacent assignments in Client::update() (fields, boolean_fields); main's changes elsewhere didn't touch this code so it went unchecked until now. The wrong-verifier test asserted 400 on retry, but the code is already deleted after the first failed attempt (delete-on-validation-failure, which is deliberate: one wrong verifier should burn the code). Retrying a deleted code is "not found", not "bad request" - fixed the assertion to 404 to match get_by_code()'s actual behavior, confirmed by the CI run itself.
Collaborator
Author
|
@joehoyle ideally this goes back upstream, but happy for me to run with this fork for a bit? I want to add DCR as well. |
8 tasks
…client Both authorize-time PKCE error paths used wp_safe_redirect(), which rejects a redirect target whose host isn't the current site (no allowed_redirect_hosts filter is registered anywhere in this plugin) and silently substitutes admin_url() instead. Any client on a foreign host - the normal case - never learned its PKCE request had failed; the user was just dumped on wp-admin. The success path already gets this right at class-authorization-code.php:166-167, using wp_redirect() with a phpcs ignore, because validate_redirect_uri() has already confirmed the URI is the client's own pre-registered callback by the time either redirect fires. Apply the same fix to the two PKCE error-redirect sites this branch added. The regression test hooks the 'wp_redirect' filter - which both wp_redirect() and wp_safe_redirect() funnel through - to capture the real destination and throw before handle_authorisation()'s exit(), so it can exercise the actual method instead of a bypass helper. Verified it fails against the pre-fix code with the exact predicted symptom (lands on http://example.org/wp-admin/).
Member
|
Mmm I didn't realize we even had a fork, IMO everything should be upstream, I can review / merge there. |
Collaborator
Author
|
Checked this branch's S256 PKCE flow with oauth2c, an independent Go OAuth client, against a real WordPress Playground instance. It ran the full flow over real HTTP: login, consent, code exchange with its own PKCE verifier, and the token it got back worked against While checking this I found two redirect bugs and fixed one here:
Also filed #15 (token endpoint doesn't return RFC 6749-shaped errors) and #14 (no RFC 8414 discovery document, so a compliant client can't find |
Collaborator
Author
|
@joehoyle I just made the fork to pick up on the PKCE support review and PR on Ryan's behalf. I'll get this in order, and make sure it has relevant props from the upstream PRs and then open it against that. |
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
2 participants
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.
Adds PKCE for the
authorization_codegrant. This matters more than usual here: the token endpoint does not authenticate clients on this grant at all (noclient_secretcheck), so PKCE is currently the only defence against a stolen authorization code.Two PKCE PRs are open upstream, WP-API#46 and WP-API#66 (identical diffs). Their S256 transform is wrong: it base64-encodes the hex digest with standard base64, giving an 88-character value, where RFC 7636 wants 43-character unpadded base64url of the raw bytes. No compliant client could complete a PKCE flow against it. This PR reimplements PKCE from scratch with that fixed and tested against the RFC's own Appendix B vector, plus:
plainstays an accepted method (it's the RFC default when the method is omitted) but never satisfies the requirement, since it gives a public client no real protection.error=invalid_requestinstead of the existingwp_die(), since native/mobile clients can't read an HTML error page. This follows the endpoint's existing behaviour more generally: an error before the redirect URI is validated dies, one after it redirects.code_challenge_methods_supportedadded to the REST index for discovery.wp oauth2 generate-code-challengecommand for manually testing a flow.No breaking changes:
ClientInterfaceis untouched (PHP allows an implementation to accept more optional args than the interface declares), and non-PKCE codes behave exactly as before.Known gaps this PR does not fix, filed as separate issues: #4 (no client authentication on the
authorization_codegrant at all — PKCE is not a substitute for confidential clients), #5 (token endpoint errors are WP REST shaped, not RFC 6749 §5.2). Also filed: #6, #7, #8, #9, #10, and the requested integration-testing follow-on, #2.Related to WP-API#18, WP-API#46, WP-API#66.