Add PKCE support (RFC 7636) - #85
Open
roborourke wants to merge 14 commits into
Open
roborourke wants to merge 14 commits into
roborourke wants to merge 14 commits into
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.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
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.
…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/).
The REST index already carries code_challenge_methods_supported, but the well-known authorization server metadata endpoint landed after this branch was written and did not. RFC 8414 section 2 defines that field as the standard discovery point for PKCE, so a client that reads the metadata document rather than the WordPress REST index could not tell that the server supports S256. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
roborourke
commented
Sep 17, 2026
Comment on lines
+485
to
+489
| <?php esc_html_e( 'Require this application to use PKCE with the S256 code_challenge_method for the authorization_code grant.', 'oauth2' ); ?> | ||
| </label> | ||
| <p class="description"> | ||
| <?php esc_html_e( 'Recommended for public clients such as single-page apps, desktop apps, and mobile apps, which cannot keep a client secret confidential. Only S256 satisfies this requirement; plain does not.', 'oauth2' ); ?> | ||
| </p> |
Contributor
Author
There was a problem hiding this comment.
These can be reworded to be more user friendly, it's not necessary for someone to know about S256, and implementation specifics. Just "Require PKCE", and the first sentence of the description is enough.
Contributor
Author
There was a problem hiding this comment.
Done. The field is now just "Require PKCE", and the description is the first sentence only.
| * @return true|WP_Error True if valid (including when this code has no | ||
| * PKCE challenge and none was supplied), error otherwise. | ||
| */ | ||
| protected function validate_code_verifier( $args ) { |
Contributor
Author
There was a problem hiding this comment.
We only need the verifier here so it's not necessary to accept an array, it's better to pass a specific named argument to simplify the function call.
Contributor
Author
There was a problem hiding this comment.
Changed to validate( $code_verifier ). validate_code_verifier() takes the same named argument now.
| * @return array|WP_Error `code_challenge`/`code_challenge_method` to carry | ||
| * through to the minted code, or an error. | ||
| */ | ||
| protected function gather_extra_params( Client $client, array $request ) { |
Contributor
Author
There was a problem hiding this comment.
This function name is too generic given what it actually does, the base class defines it as gathering grant type parameters rather being anything generic, but it also does the validation. The name indicates it's a getter only.
Contributor
Author
There was a problem hiding this comment.
Renamed to validate_extra_params() in the base class and both grant types, since it rejects a bad request as well as returning the params. Error codes moved with it.
| ); | ||
| } | ||
|
|
||
| if ( $client->is_pkce_required() ) { |
Contributor
Author
There was a problem hiding this comment.
If we're using this twice could it be an early return gate in this function instead?
Contributor
Author
There was a problem hiding this comment.
Yes. Both checks now live in check_pkce_requirement(), called once as a gate before the challenge itself is validated.
Four changes from review on WP-API#85: - The client screen said "Require PKCE (S256)" and spelled out the S256 transform in both the label and the description. An admin choosing the setting does not need the implementation detail, so the field is now "Require PKCE" with a one-sentence description. - Authorization_Code::validate() took an $args array to carry one value. It now takes $code_verifier directly, so callers do not have to know the array key. validate_code_verifier() takes the same named argument. - Renamed Base::gather_extra_params() to validate_extra_params(). The old name read as a getter, but the method rejects a bad request as well as returning the parameters to keep. - The client's PKCE requirement was checked in two places inside validate_extra_params(). Both checks now live in check_pkce_requirement(), called once as a gate before the challenge itself is validated. Error codes move with the method names, and the tests follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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 (RFC 7636) to the
authorization_codegrant.This supersedes #46 and #66, which have the same diff. Their
S256transform base64-encodes the hex digest, giving 88 characters. RFC 7636 §4.2 asks for unpadded base64url of the raw 32 bytes, so 43 characters. A standards-compliant client cannot finish that flow. This branch reimplements PKCE and checks the transform against the worked example in Appendix B.What it adds:
code_challengeandcode_challenge_methodat the authorize endpoint, bound to the issued code (§4.3, §4.4).code_verifierchecked at the token endpoint withhash_equals(); a mismatch returnsinvalid_grant(§4.5, §4.6).plainstays accepted, since it is the default when the method is left out (§4.3), but it never meets that requirement.error=invalid_requestinstead of callingwp_die(), per RFC 6749 §4.1.2.1. Native clients cannot read an HTML error page.code_challenge_methods_supportedin the REST index and in the RFC 8414 metadata document.wp oauth2 generate-code-challengefor testing a flow by hand.Nothing breaks:
ClientInterfaceis unchanged, and codes issued without a challenge behave as before.This matters here because the token endpoint does not authenticate clients on this grant, so PKCE is the only defence against a stolen code. #83 fixes that gap for confidential clients.
Originally opened as humanmade#1.
🤖 Generated with Claude Code