You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds PKCE (RFC 7636) to the authorization_code grant.
This supersedes #46 and #66, which have the same diff. Their S256 transform 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_challenge and code_challenge_method at the authorize endpoint, bound to the issued code (§4.3, §4.4).
code_verifier checked at the token endpoint with hash_equals(); a mismatch returns invalid_grant (§4.5, §4.6).
Verifier length and character set validated, 43 to 128 characters (§4.1).
A per-client "Require PKCE (S256)" setting. plain stays accepted, since it is the default when the method is left out (§4.3), but it never meets that requirement.
Clients that require PKCE are refused the implicit grant. It mints no code, so there is nothing to bind a challenge to.
PKCE errors redirect back to the client with error=invalid_request instead of calling wp_die(), per RFC 6749 §4.1.2.1. Native clients cannot read an HTML error page.
code_challenge_methods_supported in the REST index and in the RFC 8414 metadata document.
wp oauth2 generate-code-challenge for testing a flow by hand.
Nothing breaks: ClientInterface is 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.
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.
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>
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>
Brings in per-client token TTL (WP-API#77), RFC 9728 protected resource metadata (WP-API#84), PHP 7.4+ support (WP-API#86) and the dynamic WP test matrix (WP-API#87).
Conflicts were between PKCE and token TTL, which both add a client meta field. Both fields are kept everywhere. In Client::update() the PKCE branch writes only the meta keys the caller supplies, while upstream always wrote token_ttl and cleared it when omitted. token_ttl now follows the same preserve-when-omitted rule as the other fields: omitting it keeps the stored value, and passing '' or null clears it. Upstream's tests clear the TTL by passing '' explicitly, so they are unaffected.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Six fixes from Copilot's review of WP-API#85:
- A non-string code_challenge or code_challenge_method (e.g. code_challenge[]=x) was treated as absent, so a client with optional PKCE got a code with no challenge. A code_challenge_method with no code_challenge was also silently ignored. Both are malformed requests and now return invalid_request, per RFC 6749 section 4.1.2.1.
- The oauth2.pkce.supported_methods filter could add a method that is then advertised in discovery but always rejected, since derive_challenge() and is_valid_challenge() only know S256 and plain. The filter result is now intersected with the built-in methods, so it can only narrow the list. Adding real extension points for custom transforms was the alternative, but RFC 7636 defines only these two methods.
- The weak_method error always said S256 was required, even when oauth2.pkce.required_methods changed the list. It now names the filtered methods.
- get_error_redirect_url() used empty() on state, which drops a valid state of "0". It now checks for null, the only value meaning "no state". The server_error redirect passes $data['state'] straight through for the same reason.
- The implicit grant's error fragment went through build_query(), which does not encode, so a description or state containing & or # corrupted the fragment. Values are now encoded first.
- Reworded the comment on the challenge-persistence guard. It described a Client subclass with the pre-PKCE arity, but such a subclass would fail to load, so the guard just fails closed if the minted code lacks the challenge.
The pre-existing success redirects still use empty() on state. They predate this PR and are left alone here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep verifier rejection disabled for non-PKCE authorization codes
inc/tokens/class-authorization-code.php:204
Rejecting a verifier by default changes the behavior of authorization codes issued without PKCE: before this PR the extra parameter was ignored, but now the same exchange returns invalid_grant. That conflicts with the stated compatibility guarantee that codes without a challenge behave as before. Default this opt-in rejection filter to false.
Preserve OAuth state unchanged in error redirects
inc/types/class-base.php:79
The new error redirect does not echo state unchanged: $state was passed through sanitize_text_field() above, which removes valid opaque values such as %41. OAuth requires the exact received state so the client can correlate the response; preserve the unslashed string for this redirect and let the URL builder encode it.
Report the configured supported PKCE methods in errors
inc/types/class-authorization-code.php:78
This message can recommend a method the server does not support. For example, the documented oauth2.pkce.supported_methods filter can leave only plain, but every unsupported-method response still says to use S256; report the actual filtered methods instead.
The error always told the client to use S256. With oauth2.pkce.supported_methods narrowed to plain, that pointed the client at a method the server rejects. The message now lists the methods from the filter.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replies to the three findings in Copilot's latest review:
Unsupported-method message: fixed in 6cd4069. It now lists the methods from oauth2.pkce.supported_methods.
Verifier rejection for non-PKCE codes: keeping the default as true. RFC 9700 section 4.8.2 says the server must reject a code_verifier when the authorization request had no code_challenge. This blocks a PKCE downgrade attack.
sanitize_text_field() on state: this line dates from 2019 and affects every redirect, not only the new error path. It should be fixed in its own PR.
Preserve exact OAuth state in PKCE error redirects
inc/types/class-base.php:79
The new PKCE error redirect does not echo state exactly: $state was passed through sanitize_text_field() on line 49, which strips valid OAuth state characters/content (for example, tag-like text). RFC 6749 requires returning the exact state received, so clients can reject this error response as a CSRF mismatch. Keep a raw unslashed scalar state for protocol round-tripping and rely on the URL builder to encode it.
The reason will be displayed to describe this comment to others. Learn more.
Not changing this here. Every token endpoint error is already a plain WP_Error, including invalid client and expired code, so the PKCE errors follow the same shape. The RFC 6749 section 5.2 format should apply to all of them, so it needs its own PR.
Each PKCE test now has a one-line docblock naming the RFC rule it checks, with an @link to that section: RFC 7636 for the verifier, challenge and verification rules, RFC 6749 for error redirects and token request parameters, RFC 9700 for the S256 requirement, the downgrade check and the implicit grant, and RFC 8414 for discovery metadata. Tests that check plugin behaviour with no RFC rule behind it, such as filters and backwards compatibility, have no link.
Going through the tests section by section showed these gaps, now covered:
- RFC 7636 section 4.4: nothing ran the whole consent flow and checked that the minted code carries the challenge. The unit tests called Authorization_Code::create() directly, so dropping the extra params when building $data in Base::handle_authorisation() went unnoticed.
- RFC 6749 section 4.1.2.1: nothing checked that a PKCE error on a request with an unregistered redirect_uri is not redirected there. Running the PKCE check before the redirect URI check would have sent the error to an attacker's URI.
- RFC 7636 section 4.6: nothing sent the S256 challenge itself as the verifier, which only fails if the stored method is applied.
- RFC 6749 section 3.2: the token endpoint tests did not cover a verifier sent only in the URL query (ignored) or in a JSON body (accepted).
- RFC 7636 section 4.6: the wrong, missing and unexpected verifier tests at the token endpoint only checked for a 400. They now also check the invalid_grant error code.
- RFC 8414 section 2: nothing checked that code_challenge_methods_supported follows the oauth2.pkce.supported_methods filter.
- The oauth2.pkce.required_methods filter was only tested narrowing the list, not allowing plain.
The consent-flow, redirect-URI and URL-query tests were each checked by making the matching change in the plugin code and seeing the test fail.
Co-Authored-By: Claude Opus 5.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
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