Add a Playground e2e check for the PKCE flow - #90
Closed
roborourke wants to merge 18 commits into
Closed
roborourke wants to merge 18 commits into
roborourke wants to merge 18 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>
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>
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>
The PHPUnit tests call the grant types and token endpoint directly, so they never cover a real browser-style round trip: cookie login, the consent form and its nonce, the redirect back to the client, the token exchange over HTTP, and using the token on the REST API. This adds a harness that does exactly that against WordPress Playground. tests/e2e/run.sh starts Playground with the plugin mounted and a blueprint that activates it and creates two clients, one with PKCE required and one without. setup.php writes their IDs to a JSON file in the web root so the checks can read them over HTTP. pkce.py then runs 23 checks: the S256 happy path end to end, wrong, missing and URL-only verifiers, code reuse and cross-client redemption, the downgrade case of a verifier sent for a non-PKCE code, malformed authorize parameters, the implicit grant refusal, and cancel on the consent screen. pkce.py uses only the Python standard library, so the CI job needs nothing beyond the Node and Python that ubuntu-latest already ships. Playwright was the other option, but none of these checks need a browser engine, and plain HTTP lets every redirect be inspected without following it. run.sh waits for Playground's "Ready!" log line as well as the clients file. Playground answers some requests while it is still booting, so polling the file alone let the checks start against a server that then stopped responding. The code-reuse checks accept 400 or 404, since an unknown code is currently a 404 and WP-API#89 changes it to the 400 that RFC 6749 expects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Moved to humanmade#19 so it can build on the PKCE branch. It will come back here once #85 merges. |
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.
This adds an end-to-end check that runs the PKCE flow over real HTTP against WordPress Playground.
It depends on #85, so this PR also shows the PKCE commits until #85 merges. Only the last commit is new.
tests/e2e/run.shstarts Playground with the plugin mounted. A blueprint activates the plugin and creates two clients: one that requires PKCE and one that does not.tests/e2e/pkce.pythen logs in with a password, submits the consent form, exchanges the code for a token, and calls/wp/v2/users/mewith the token. It never follows redirects, so it checks every redirect target.It runs 23 checks. They cover the S256 flow from start to finish, wrong, missing and URL-only verifiers, code reuse, and a verifier sent for a code without PKCE. They also cover bad authorize parameters, the implicit grant refusal and the cancel button.
The script needs only Python 3 and Node. A new
E2E (Playground)job runs it in CI. To run it locally, usebash tests/e2e/run.sh.🤖 Generated with Claude Code