From 303e2c43c88c27c38b16cdf84c3c9445b33ee75a Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:10:07 +0100 Subject: [PATCH 01/19] Add PKCE helper class 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/OAuth2#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. --- .phpcs.xml.dist | 1 + inc/class-pkce.php | 155 ++++++++++++++++++++++++++++++++++++++++++ plugin.php | 7 +- tests/test-pkce.php | 160 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 322 insertions(+), 1 deletion(-) create mode 100644 inc/class-pkce.php create mode 100644 tests/test-pkce.php diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index ab5e57d..330b103 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -28,5 +28,6 @@ + diff --git a/inc/class-pkce.php b/inc/class-pkce.php new file mode 100644 index 0000000..f13cf25 --- /dev/null +++ b/inc/class-pkce.php @@ -0,0 +1,155 @@ +assertSame( static::RFC_CHALLENGE, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ) ); + } + + public function test_s256_challenge_is_unpadded_base64url() { + $challenge = PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ); + + $this->assertSame( 43, strlen( $challenge ) ); + $this->assertMatchesRegularExpression( '/^[A-Za-z0-9\-_]+$/', $challenge ); + $this->assertStringNotContainsString( '=', $challenge ); + $this->assertStringNotContainsString( '+', $challenge ); + $this->assertStringNotContainsString( '/', $challenge ); + } + + public function test_plain_challenge_is_the_verifier_verbatim() { + $this->assertSame( static::RFC_VERIFIER, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + } + + public function test_derive_challenge_returns_null_for_unknown_method() { + $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 'md5' ) ); + } + + public function test_derive_challenge_is_case_sensitive() { + $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 's256' ) ); + } + + public function test_verify_true_for_matching_s256_pair() { + $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_false_for_wrong_verifier() { + $this->assertFalse( PKCE::verify( 'wrong-verifier-wrong-verifier-wrong-verifier', static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_true_for_matching_plain_pair() { + $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + } + + public function test_verify_returns_false_not_typeerror_for_unknown_method() { + $this->assertFalse( PKCE::verify( static::RFC_VERIFIER, static::RFC_CHALLENGE, 'md5' ) ); + } + + public function test_verify_returns_false_not_typeerror_for_array_verifier() { + $this->assertFalse( PKCE::verify( [ 'x' ], static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_returns_false_not_typeerror_for_array_challenge() { + $this->assertFalse( PKCE::verify( static::RFC_VERIFIER, [ 'x' ], PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_verifier_accepts_boundary_lengths() { + $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) ) ); + $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 128 ) ) ); + } + + public function test_is_valid_verifier_rejects_lengths_outside_boundary() { + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 42 ) ) ); + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 129 ) ) ); + } + + public function test_is_valid_verifier_rejects_disallowed_characters() { + $base = str_repeat( 'a', 42 ); + + $this->assertFalse( PKCE::is_valid_verifier( $base . '+' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '/' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '=' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . ' ' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '%' ) ); + } + + public function test_is_valid_verifier_rejects_trailing_newline() { + // A `$`-anchored regex would accept a trailing newline; `\z` must not. + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) . "\n" ) ); + } + + public function test_is_valid_verifier_rejects_non_string() { + $this->assertFalse( PKCE::is_valid_verifier( [ 'x' ] ) ); + } + + public function test_is_valid_challenge_for_s256_requires_exactly_43_characters() { + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_S256 ) ); + $this->assertTrue( PKCE::is_valid_challenge( str_repeat( 'a', 43 ), PKCE::METHOD_S256 ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 44 ), PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_s256_rejects_padding() { + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '=', PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_s256_rejects_verifier_only_characters() { + // '.' and '~' are valid in a verifier, but not in base64url. + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '.', PKCE::METHOD_S256 ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '~', PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_plain_uses_verifier_rules() { + $this->assertTrue( PKCE::is_valid_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_PLAIN ) ); + } + + public function test_is_valid_challenge_rejects_unknown_method() { + $this->assertFalse( PKCE::is_valid_challenge( static::RFC_CHALLENGE, 'md5' ) ); + } + + public function test_supported_methods_defaults_to_s256_and_plain() { + $this->assertSame( [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], PKCE::supported_methods() ); + } + + public function test_supported_methods_filter_removes_plain() { + $filter = function () { + return [ PKCE::METHOD_S256 ]; + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertSame( [ PKCE::METHOD_S256 ], PKCE::supported_methods() ); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + } + + public function test_generate_verifier_produces_a_valid_verifier() { + $verifier = PKCE::generate_verifier(); + $this->assertTrue( PKCE::is_valid_verifier( $verifier ) ); + } + + public function test_generate_verifier_respects_requested_length() { + $this->assertSame( 64, strlen( PKCE::generate_verifier( 64 ) ) ); + $this->assertSame( 100, strlen( PKCE::generate_verifier( 100 ) ) ); + } + + public function test_generate_verifier_clamps_to_valid_range() { + $this->assertSame( PKCE::VERIFIER_MIN_LENGTH, strlen( PKCE::generate_verifier( 10 ) ) ); + $this->assertSame( PKCE::VERIFIER_MAX_LENGTH, strlen( PKCE::generate_verifier( 500 ) ) ); + } + + public function test_generate_verifier_is_not_deterministic() { + $this->assertNotSame( PKCE::generate_verifier(), PKCE::generate_verifier() ); + } +} From f5249aab32dc24625cd89d820784f285c0764276 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:10:16 +0100 Subject: [PATCH 02/19] Bind PKCE challenge to authorization codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- inc/tokens/class-authorization-code.php | 138 ++++++++++++++++++- tests/test-authorization-code.php | 172 ++++++++++++++++++++++++ 2 files changed, 305 insertions(+), 5 deletions(-) diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index 111ee15..5c920e2 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -10,6 +10,7 @@ use WP_Error; use WP_Http; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; use WP_User; /** @@ -113,14 +114,49 @@ public function get_expiration() { return (int) $value['expiration']; } + /** + * Get the PKCE code challenge stored against this code, if any. + * + * @return string|null Code challenge, or null if this code was not issued with one. + */ + public function get_code_challenge() { + $value = $this->get_value(); + if ( empty( $value ) || empty( $value['code_challenge'] ) ) { + return null; + } + + return $value['code_challenge']; + } + + /** + * Get the PKCE code challenge method stored against this code, if any. + * + * @return string|null Code challenge method, or null if this code was not issued with a challenge. + */ + public function get_code_challenge_method() { + $value = $this->get_value(); + if ( empty( $value ) || empty( $value['code_challenge_method'] ) ) { + return null; + } + + return $value['code_challenge_method']; + } + /** * Validate the code for use. * + * @param array $args Other request arguments to validate. { + * @var string $code_verifier PKCE code verifier, if the client is using PKCE. + * } * @return bool|WP_Error True if valid, error describing problem otherwise. */ - public function validate() { + public function validate( $args = [] ) { $expiration = $this->get_expiration(); - $now = time(); + if ( is_wp_error( $expiration ) ) { + return $expiration; + } + + $now = time(); if ( $expiration <= $now ) { return new WP_Error( 'oauth2.tokens.authorization_code.validate.expired', @@ -133,6 +169,86 @@ public function validate() { ); } + $verifier_check = $this->validate_code_verifier( $args ); + if ( is_wp_error( $verifier_check ) ) { + return $verifier_check; + } + + return true; + } + + /** + * Validate a PKCE code verifier against the stored code challenge. + * + * @param array $args Request arguments, as passed to validate(). + * @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 ) { + $challenge = $this->get_code_challenge(); + $verifier = isset( $args['code_verifier'] ) && is_string( $args['code_verifier'] ) ? $args['code_verifier'] : null; + + if ( null === $challenge ) { + if ( null === $verifier ) { + return true; + } + + /** + * Filter whether a code_verifier is accepted for a code that has no stored code_challenge. + * + * A verifier arriving for a non-PKCE code is unexpected: either the + * client is misconfigured, or the code was obtained some other way. + * Rejecting it by default is the safer choice. + * + * @param bool $reject True to reject the request, the default. + * @param string $verifier Code verifier that was supplied. + */ + if ( apply_filters( 'oauth2.pkce.reject_unexpected_verifier', true, $verifier ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.unexpected_verifier', + __( 'This authorization code was not issued with a PKCE challenge.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + + return true; + } + + $method = $this->get_code_challenge_method(); + if ( null === $method ) { + // A stored challenge with no stored method means corrupted data; fail closed. + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.missing_challenge_method', + __( 'Authorization code data is not valid.', 'oauth2' ), + [ 'status' => WP_Http::BAD_REQUEST ] + ); + } + + if ( null === $verifier ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.missing_verifier', + __( 'This authorization code requires a code_verifier to be exchanged.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + + if ( ! PKCE::is_valid_verifier( $verifier ) || ! PKCE::verify( $verifier, $challenge, $method ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.invalid_verifier', + __( 'The supplied code_verifier does not match the code_challenge for this authorization code.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + return true; } @@ -184,17 +300,29 @@ public static function get_by_code( Client $client, $code ) { * * @param Client $client * @param WP_User $user + * @param array $data Optional extra data for this code. Only `code_challenge` + * and `code_challenge_method` are recognised; anything + * else is ignored, and neither `user` nor `expiration` + * can be overridden this way. * * @return Authorization_Code|WP_Error Authorization code instance, or error on failure. */ - public static function create( Client $client, WP_User $user ) { + public static function create( Client $client, WP_User $user, array $data = [] ) { $code = wp_generate_password( static::KEY_LENGTH, false ); $meta_key = static::KEY_PREFIX . $code; - $data = [ + $value = [ 'user' => (int) $user->ID, 'expiration' => time() + static::MAX_AGE, ]; - $result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $data ), true ); + + // Only store the challenge when one was actually supplied, so a + // non-PKCE code's meta shape is unchanged from before this existed. + if ( ! empty( $data['code_challenge'] ) ) { + $value['code_challenge'] = $data['code_challenge']; + $value['code_challenge_method'] = ! empty( $data['code_challenge_method'] ) ? $data['code_challenge_method'] : PKCE::METHOD_PLAIN; + } + + $result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $value ), true ); if ( ! $result ) { return new WP_Error( 'oauth2.tokens.authorization_code.create.could_not_create', diff --git a/tests/test-authorization-code.php b/tests/test-authorization-code.php index f7a2a40..ab6223a 100644 --- a/tests/test-authorization-code.php +++ b/tests/test-authorization-code.php @@ -105,4 +105,176 @@ public function test_code_is_valid_only_for_its_client() { $result = Authorization_Code::get_by_code( $other_client, $code->get_code() ); $this->assertWPError( $result ); } + + // RFC 7636 Appendix B worked example. + const RFC_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + const RFC_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + + public function test_create_without_pkce_data_stores_no_challenge() { + $code = Authorization_Code::create( $this->client, $this->user ); + $this->assertNull( $code->get_code_challenge() ); + $this->assertNull( $code->get_code_challenge_method() ); + } + + public function test_create_with_pkce_data_stores_challenge_and_method() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertSame( static::RFC_CHALLENGE, $code->get_code_challenge() ); + $this->assertSame( 'S256', $code->get_code_challenge_method() ); + } + + public function test_create_defaults_challenge_method_to_plain_when_omitted() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ 'code_challenge' => static::RFC_VERIFIER ] + ); + + $this->assertSame( 'plain', $code->get_code_challenge_method() ); + } + + public function test_create_data_cannot_override_user_or_expiration() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'user' => 999999, + 'expiration' => 1, + ] + ); + + $user = $code->get_user(); + $this->assertInstanceOf( WP_User::class, $user ); + $this->assertEquals( $this->user->ID, $user->ID ); + $this->assertGreaterThan( time(), $code->get_expiration() ); + } + + public function test_validate_with_no_args_still_passes_for_non_pkce_code() { + // Back-compat guarantee: existing callers pass no args at all. + $code = Authorization_Code::create( $this->client, $this->user ); + $this->assertTrue( $code->validate() ); + } + + public function test_validate_passes_with_correct_s256_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + } + + public function test_validate_passes_with_correct_plain_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_VERIFIER, + 'code_challenge_method' => 'plain', + ] + ); + + $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + } + + public function test_validate_fails_with_wrong_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate( [ 'code_verifier' => 'wrong-verifier-wrong-verifier-wrong-verifier' ] ); + $this->assertWPError( $result ); + $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); + } + + public function test_validate_fails_with_missing_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate(); + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_verifier', $result->get_error_code() ); + } + + public function test_validate_fails_with_malformed_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate( [ 'code_verifier' => 'too-short' ] ); + $this->assertWPError( $result ); + } + + public function test_validate_rejects_verifier_for_code_with_no_stored_challenge() { + $code = Authorization_Code::create( $this->client, $this->user ); + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.unexpected_verifier', $result->get_error_code() ); + } + + public function test_validate_allows_unexpected_verifier_when_filtered() { + $code = Authorization_Code::create( $this->client, $this->user ); + + $filter = '__return_false'; + add_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + remove_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); + + $this->assertTrue( $result ); + } + + public function test_validate_fails_closed_when_stored_method_is_missing() { + $code = Authorization_Code::create( $this->client, $this->user ); + $meta_key = Authorization_Code::KEY_PREFIX . $code->get_code(); + + // Simulate corrupted meta: a challenge with no method. + $value = get_post_meta( $this->client->get_post_id(), $meta_key, true ); + $value['code_challenge'] = static::RFC_CHALLENGE; + update_post_meta( $this->client->get_post_id(), $meta_key, $value ); + + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_challenge_method', $result->get_error_code() ); + } + + public function test_validate_with_malformed_meta_does_not_pass_expiry_check() { + $code = Authorization_Code::create( $this->client, $this->user ); + $meta_key = Authorization_Code::KEY_PREFIX . $code->get_code(); + + // Corrupt the expiration entirely; get_expiration() now returns a WP_Error. + $value = get_post_meta( $this->client->get_post_id(), $meta_key, true ); + unset( $value['expiration'] ); + update_post_meta( $this->client->get_post_id(), $meta_key, $value ); + + $result = $code->validate(); + $this->assertWPError( $result ); + } } From beb6992b33549792b2ce0fd0b21072aa97c0c15e Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:10:26 +0100 Subject: [PATCH 03/19] Add per-client PKCE requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- inc/class-client.php | 51 ++++++++++++++++++++---- tests/class-test-case.php | 25 +++++++++++- tests/test-client.php | 83 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 10 deletions(-) diff --git a/inc/class-client.php b/inc/class-client.php index bc9df85..c3c84e0 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -20,6 +20,7 @@ class Client implements ClientInterface { const TYPE_KEY = '_oauth2_client_type'; const REDIRECT_URI_KEY = '_oauth2_redirect_uri'; const CLIENT_CREDENTIALS_ENABLED_KEY = '_oauth2_client_credentials_enabled'; + const PKCE_REQUIRED_KEY = '_oauth2_pkce_required'; const AUTH_CODE_KEY_PREFIX = '_oauth2_authcode_'; const AUTH_CODE_LENGTH = 12; const CLIENT_ID_LENGTH = 12; @@ -142,6 +143,23 @@ public function is_client_credentials_enabled() { return (bool) get_post_meta( $this->get_post_id(), static::CLIENT_CREDENTIALS_ENABLED_KEY, true ); } + /** + * Check whether this client requires PKCE for the authorization_code grant. + * + * @return bool True if PKCE is required, false otherwise. + */ + public function is_pkce_required() { + $required = (bool) get_post_meta( $this->get_post_id(), static::PKCE_REQUIRED_KEY, true ); + + /** + * Filter whether PKCE is required for a client. + * + * @param bool $required True if PKCE is required for this client. + * @param Client $client Client being checked. + */ + return apply_filters( 'oauth2.pkce.required', $required, $this ); + } + /** * Get registered URI for the client. * @@ -249,11 +267,13 @@ public function check_redirect_uri( $uri ) { /** * @param WP_User $user + * @param array $data Optional extra data for the code, e.g. PKCE `code_challenge` + * and `code_challenge_method`. See Authorization_Code::create(). * * @return Authorization_Code|WP_Error */ - public function generate_authorization_code( WP_User $user ) { - return Authorization_Code::create( $this, $user ); + public function generate_authorization_code( WP_User $user, array $data = [] ) { + return Authorization_Code::create( $this, $user, $data ); } /** @@ -361,6 +381,7 @@ public static function create( $data ) { static::TYPE_KEY => $data['meta']['type'], static::CLIENT_SECRET_KEY => wp_generate_password( static::CLIENT_SECRET_LENGTH, false ), static::CLIENT_CREDENTIALS_ENABLED_KEY => ! empty( $data['meta']['client_credentials_enabled'] ) ? '1' : '', + static::PKCE_REQUIRED_KEY => ! empty( $data['meta']['pkce_required'] ) ? '1' : '', ]; foreach ( $meta as $key => $value ) { @@ -394,14 +415,28 @@ public function update( $data ) { return $post_id; } - $meta = [ - static::REDIRECT_URI_KEY => $data['meta']['callback'], - static::TYPE_KEY => $data['meta']['type'], - static::CLIENT_CREDENTIALS_ENABLED_KEY => ! empty( $data['meta']['client_credentials_enabled'] ) ? '1' : '', + // Map of meta key => data key. Built this way, rather than as a fixed + // array of values, so that a caller omitting a key leaves the existing + // meta value untouched instead of silently resetting it to empty/false. + $fields = [ + static::REDIRECT_URI_KEY => 'callback', + static::TYPE_KEY => 'type', + static::CLIENT_CREDENTIALS_ENABLED_KEY => 'client_credentials_enabled', + static::PKCE_REQUIRED_KEY => 'pkce_required', ]; + $boolean_fields = [ static::CLIENT_CREDENTIALS_ENABLED_KEY, static::PKCE_REQUIRED_KEY ]; - foreach ( $meta as $key => $value ) { - update_post_meta( $post_id, wp_slash( $key ), wp_slash( $value ) ); + foreach ( $fields as $meta_key => $data_key ) { + if ( ! array_key_exists( $data_key, $data['meta'] ) ) { + continue; + } + + $value = $data['meta'][ $data_key ]; + if ( in_array( $meta_key, $boolean_fields, true ) ) { + $value = ! empty( $value ) ? '1' : ''; + } + + update_post_meta( $post_id, wp_slash( $meta_key ), wp_slash( $value ) ); } $post = get_post( $post_id ); diff --git a/tests/class-test-case.php b/tests/class-test-case.php index 9035d85..76facb2 100644 --- a/tests/class-test-case.php +++ b/tests/class-test-case.php @@ -8,6 +8,7 @@ namespace WP\OAuth2\Tests; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; use WP_UnitTestCase; /** @@ -30,8 +31,9 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie 'meta' => array_merge( [ 'callback' => 'https://example.com/callback', - 'type' => 'web', + 'type' => 'public', 'client_credentials_enabled' => false, + 'pkce_required' => false, ], $meta_overrides ), @@ -43,4 +45,25 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie return $client; } + + /** + * Generate a matching PKCE verifier/challenge pair for use in tests. + * + * @param string $method Challenge method, `S256` or `plain`. Default `S256`. + * + * @return array { + * @var string $code_verifier Code verifier. + * @var string $code_challenge Derived code challenge. + * @var string $code_challenge_method Method used to derive the challenge. + * } + */ + protected function make_pkce_pair( $method = PKCE::METHOD_S256 ) { + $verifier = PKCE::generate_verifier(); + + return [ + 'code_verifier' => $verifier, + 'code_challenge' => PKCE::derive_challenge( $verifier, $method ), + 'code_challenge_method' => $method, + ]; + } } diff --git a/tests/test-client.php b/tests/test-client.php index f63fb51..cadebb0 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -51,7 +51,7 @@ public function test_create_stores_redirect_uri() { } public function test_create_stores_type() { - $this->assertEquals( 'web', $this->client->get_type() ); + $this->assertEquals( 'public', $this->client->get_type() ); } public function test_create_stores_secret() { @@ -217,4 +217,85 @@ public function test_check_redirect_uri_rejects_different_path() { public function test_check_redirect_uri_rejects_unregistered() { $this->assertFalse( $this->client->check_redirect_uri( 'https://evil.com/steal' ) ); } + + public function test_pkce_required_defaults_to_false() { + $this->assertFalse( $this->client->is_pkce_required() ); + } + + public function test_pkce_required_true_when_set() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $this->assertTrue( $client->is_pkce_required() ); + } + + public function test_pkce_required_filter_can_force_true() { + $filter = '__return_true'; + add_filter( 'oauth2.pkce.required', $filter ); + $this->assertTrue( $this->client->is_pkce_required() ); + remove_filter( 'oauth2.pkce.required', $filter ); + } + + public function test_update_omitting_pkce_required_preserves_existing_value() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + ], + ] ); + + $this->assertTrue( $client->is_pkce_required() ); + } + + public function test_update_omitting_client_credentials_enabled_preserves_existing_value() { + $client = $this->create_client( [ 'client_credentials_enabled' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + ], + ] ); + + $this->assertTrue( $client->is_client_credentials_enabled() ); + } + + public function test_update_can_still_explicitly_change_pkce_required() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + 'pkce_required' => false, + ], + ] ); + + $this->assertFalse( $client->is_pkce_required() ); + } + + public function test_generate_authorization_code_without_data_still_works() { + $user = $this->factory->user->create_and_get(); + $code = $this->client->generate_authorization_code( $user ); + $this->assertNull( $code->get_code_challenge() ); + } + + public function test_generate_authorization_code_stores_pkce_challenge() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair(); + + $code = $this->client->generate_authorization_code( $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $this->assertSame( $pair['code_challenge'], $code->get_code_challenge() ); + $this->assertSame( $pair['code_challenge_method'], $code->get_code_challenge_method() ); + } } From bf81aa47abe5437680705af424a0db3911201427 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:14:42 +0100 Subject: [PATCH 04/19] Validate PKCE parameters at the authorisation endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- inc/types/class-authorization-code.php | 92 ++++++++- inc/types/class-base.php | 55 +++++- inc/types/class-implicit.php | 48 +++++ tests/test-types-authorization-code.php | 240 ++++++++++++++++++++++++ 4 files changed, 433 insertions(+), 2 deletions(-) create mode 100644 tests/test-types-authorization-code.php diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index dae2470..76b1701 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -9,6 +9,7 @@ use WP_Error; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; class Authorization_Code extends Base { /** @@ -22,6 +23,79 @@ public function get_response_type_code() { return 'code'; } + /** + * Gather and validate PKCE parameters from the authorisation request. + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @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 ) { + $code_challenge = isset( $request['code_challenge'] ) && is_string( $request['code_challenge'] ) ? $request['code_challenge'] : null; + $code_challenge_method = isset( $request['code_challenge_method'] ) && is_string( $request['code_challenge_method'] ) ? $request['code_challenge_method'] : null; + + if ( null === $code_challenge ) { + if ( $client->is_pkce_required() ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.pkce_required', + __( 'This client requires a code_challenge for the authorization_code grant.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + return []; + } + + // The method defaults to 'plain' when omitted, per RFC 7636 section 4.3. + if ( null === $code_challenge_method ) { + $code_challenge_method = PKCE::METHOD_PLAIN; + } + + if ( ! in_array( $code_challenge_method, PKCE::supported_methods(), true ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.unsupported_method', + __( 'Unsupported code_challenge_method. Note that this value is case-sensitive; use S256.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + if ( ! PKCE::is_valid_challenge( $code_challenge, $code_challenge_method ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.invalid_challenge', + __( 'Invalid code_challenge.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + if ( $client->is_pkce_required() ) { + /** + * Filter the code_challenge_method values that satisfy a client's PKCE requirement. + * + * Defaults to S256 only. `plain` offers no protection against a + * malicious app on the same device reading the authorization + * request, which is exactly the threat PKCE exists to mitigate + * for public clients (RFC 9700 section 2.1.1), so it does not + * satisfy a requirement even though it remains an accepted method. + * + * @param string[] $methods Methods that satisfy "PKCE required". + */ + $required_methods = apply_filters( 'oauth2.pkce.required_methods', [ PKCE::METHOD_S256 ] ); + if ( ! in_array( $code_challenge_method, $required_methods, true ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.weak_method', + __( 'This client requires PKCE with the S256 code_challenge_method.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + } + + return [ + 'code_challenge' => $code_challenge, + 'code_challenge_method' => $code_challenge_method, + ]; + } + /** * Handles the authorization. * @@ -38,11 +112,27 @@ protected function handle_authorization_submission( $submit, Client $client, $da case 'authorize': // Generate authorization code and redirect back. $user = wp_get_current_user(); - $code = $client->generate_authorization_code( $user ); + $code = $client->generate_authorization_code( $user, $data ); if ( is_wp_error( $code ) ) { return $code; } + // Defends against a third-party Client subclass overriding + // generate_authorization_code() with the pre-PKCE arity, + // which would silently mint a code with no challenge. + if ( ! empty( $data['code_challenge'] ) && $code->get_code_challenge() !== $data['code_challenge'] ) { + $code->delete(); + wp_safe_redirect( + $this->get_error_redirect_url( + $redirect_uri, + 'server_error', + __( 'Could not persist the PKCE challenge for this authorization code.', 'oauth2' ), + ! empty( $data['state'] ) ? $data['state'] : null + ) + ); + exit; + } + $redirect_args = [ 'code' => $code->get_code(), ]; diff --git a/inc/types/class-base.php b/inc/types/class-base.php index d04031f..8b4756e 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -21,6 +21,8 @@ abstract class Base implements Type { * @var string $redirect_uri Specified redirection URI. * @var string $scope Requested scope. * @var string $state State parameter from the client. + * @var string $code_challenge PKCE code challenge, if supplied. Grant-type specific. + * @var string $code_challenge_method PKCE code challenge method, if supplied. Grant-type specific. * } * @return WP_Error|void Method should output form and exit, or return encountered error. */ @@ -65,6 +67,18 @@ public function handle_authorisation() { return $redirect_uri; } + // Gather and validate any grant-type-specific parameters (e.g. PKCE). + // This runs after the redirect URI is known to be valid, and before the + // login redirect, so a malformed request fails fast and can be reported + // back to the client rather than dying with an HTML error page. + $extra_params = $this->gather_extra_params( $client, wp_unslash( $_GET ) ); + if ( is_wp_error( $extra_params ) ) { + $error_data = $extra_params->get_error_data(); + $error_code = ! empty( $error_data['error'] ) ? $error_data['error'] : 'invalid_request'; + wp_safe_redirect( $this->get_error_redirect_url( $redirect_uri, $error_code, $extra_params->get_error_message(), $state ) ); + exit; + } + // Valid parameters, ensure the user is logged in. if ( ! is_user_logged_in() ) { $redirect = ''; @@ -105,10 +119,49 @@ public function handle_authorisation() { $submit = sanitize_text_field( wp_unslash( $_POST['wp-submit'] ) ); - $data = compact( 'redirect_uri', 'scope', 'state' ); + $data = array_merge( compact( 'redirect_uri', 'scope', 'state' ), $extra_params ); return $this->handle_authorization_submission( $submit, $client, $data ); } + /** + * Gather and validate any grant-type-specific parameters from the request. + * + * Runs on both the initial GET and the consent-form POST, since the + * authorisation form posts back to the original request URI, so $_GET is + * populated on both passes. The default implementation adds nothing; + * override to add grant-type-specific request parameters (e.g. PKCE's + * code_challenge for the authorization_code grant). + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @return array|WP_Error Extra data to merge into the $data passed to + * handle_authorization_submission(), or an error. + */ + protected function gather_extra_params( Client $client, array $request ) { + return []; + } + + /** + * Build a URL for reporting an authorisation error back to the client. + * + * @param string $redirect_uri Validated redirect URI for the client. + * @param string $error Error code, e.g. `invalid_request`. + * @param string $description Human-readable error description. + * @param string|null $state State parameter from the original request, if any. + * @return string URL to redirect the client to. + */ + protected function get_error_redirect_url( $redirect_uri, $error, $description, $state = null ) { + $args = [ + 'error' => $error, + 'error_description' => $description, + ]; + if ( ! empty( $state ) ) { + $args['state'] = $state; + } + + return add_query_arg( urlencode_deep( $args ), $redirect_uri ); + } + /** * Validate the supplied redirect URI. * diff --git a/inc/types/class-implicit.php b/inc/types/class-implicit.php index 35151f5..d727a62 100644 --- a/inc/types/class-implicit.php +++ b/inc/types/class-implicit.php @@ -22,6 +22,30 @@ public function get_response_type_code() { return 'token'; } + /** + * Refuse the implicit grant for clients that require PKCE. + * + * The implicit grant issues a token directly and mints no authorization + * code, so there is nothing for a PKCE challenge to bind to. Silently + * accepting a code_challenge here would give a false sense of protection, + * so a client marked as requiring PKCE cannot use this grant at all. + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @return array|WP_Error + */ + protected function gather_extra_params( Client $client, array $request ) { + if ( $client->is_pkce_required() ) { + return new WP_Error( + 'oauth2.types.implicit.gather_extra_params.pkce_required', + __( 'This client requires PKCE, which the implicit grant cannot support. Use the authorization_code grant instead.', 'oauth2' ), + [ 'error' => 'unauthorized_client' ] + ); + } + + return []; + } + /** * Handles the authorization. * @@ -78,4 +102,28 @@ protected function handle_authorization_submission( $submit, Client $client, $da wp_safe_redirect( $generated_redirect ); exit; } + + /** + * Build a URL for reporting an authorisation error back to the client. + * + * Access tokens are returned in the URL fragment for this grant, so + * errors are reported the same way, per RFC 6749 section 4.2.2.1. + * + * @param string $redirect_uri Validated redirect URI for the client. + * @param string $error Error code, e.g. `unauthorized_client`. + * @param string $description Human-readable error description. + * @param string|null $state State parameter from the original request, if any. + * @return string URL to redirect the client to. + */ + protected function get_error_redirect_url( $redirect_uri, $error, $description, $state = null ) { + $args = [ + 'error' => $error, + 'error_description' => $description, + ]; + if ( ! empty( $state ) ) { + $args['state'] = $state; + } + + return $redirect_uri . '#' . build_query( $args ); + } } diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php new file mode 100644 index 0000000..985dafa --- /dev/null +++ b/tests/test-types-authorization-code.php @@ -0,0 +1,240 @@ +gather_extra_params( $client, $request ); + } + + public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { + return $this->get_error_redirect_url( $redirect_uri, $error, $description, $state ); + } +} + +/** + * Exposes the protected methods under test for the implicit grant. + */ +class Test_Exposed_Implicit_Type extends Implicit { + public function gather_extra_params_public( Client $client, array $request ) { + return $this->gather_extra_params( $client, $request ); + } + + public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { + return $this->get_error_redirect_url( $redirect_uri, $error, $description, $state ); + } +} + +/** + * Test cases for PKCE parameter handling in the authorization_code grant. + */ +class Test_Types_Authorization_Code extends Test_Case { + + /** + * @var Client + */ + protected $client; + + /** + * @var Test_Exposed_Authorization_Code_Type + */ + protected $type; + + public function set_up() { + parent::set_up(); + $this->client = $this->create_client(); + $this->type = new Test_Exposed_Authorization_Code_Type(); + } + + public function test_no_challenge_returns_empty_array() { + $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + } + + public function test_valid_s256_challenge_returns_both_keys() { + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertSame( $pair['code_challenge'], $result['code_challenge'] ); + $this->assertSame( 'S256', $result['code_challenge_method'] ); + } + + public function test_unsupported_method_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => str_repeat( 'a', 43 ), + 'code_challenge_method' => 'md5', + ] + ); + + $this->assertWPError( $result ); + $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); + } + + public function test_wrongly_cased_method_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => str_repeat( 'a', 43 ), + 'code_challenge_method' => 's256', + ] + ); + + $this->assertWPError( $result ); + } + + public function test_malformed_challenge_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => 'too-short', + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertWPError( $result ); + } + + public function test_method_without_challenge_is_ignored() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge_method' => 'S256' ] + ); + + $this->assertSame( [], $result ); + } + + public function test_omitted_method_normalizes_to_plain() { + $verifier = PKCE::generate_verifier(); + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge' => $verifier ] + ); + + $this->assertSame( 'plain', $result['code_challenge_method'] ); + } + + public function test_array_valued_challenge_is_treated_as_absent() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge' => [ 'x' ] ] + ); + + $this->assertSame( [], $result ); + } + + public function test_pkce_required_client_without_challenge_is_rejected() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $result = $this->type->gather_extra_params_public( $client, [] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.pkce_required', $result->get_error_code() ); + } + + public function test_pkce_required_client_with_plain_is_rejected() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $verifier = PKCE::generate_verifier(); + $result = $this->type->gather_extra_params_public( + $client, + [ + 'code_challenge' => $verifier, + 'code_challenge_method' => 'plain', + ] + ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.weak_method', $result->get_error_code() ); + } + + public function test_pkce_required_client_with_s256_is_accepted() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->gather_extra_params_public( + $client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertIsArray( $result ); + } + + public function test_error_redirect_url_uses_query_string() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); + $this->assertStringContainsString( 'https://example.com/callback?', $url ); + $this->assertStringContainsString( 'error=invalid_request', $url ); + } + + public function test_error_redirect_url_omits_state_when_absent() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); + $this->assertStringNotContainsString( 'state=', $url ); + } + + public function test_error_redirect_url_includes_state_when_present() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', 'xyz' ); + $this->assertStringContainsString( 'state=xyz', $url ); + } +} + +/** + * Test cases for the implicit grant's PKCE-required bypass fix. + */ +class Test_Types_Implicit extends Test_Case { + + /** + * @var Client + */ + protected $client; + + /** + * @var Test_Exposed_Implicit_Type + */ + protected $type; + + public function set_up() { + parent::set_up(); + $this->client = $this->create_client(); + $this->type = new Test_Exposed_Implicit_Type(); + } + + public function test_non_pkce_client_is_allowed() { + $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + } + + public function test_pkce_required_client_is_refused() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $result = $this->type->gather_extra_params_public( $client, [] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.implicit.gather_extra_params.pkce_required', $result->get_error_code() ); + } + + public function test_error_redirect_url_uses_fragment() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'Nope.' ); + $this->assertStringContainsString( '#', $url ); + $this->assertStringContainsString( 'error=unauthorized_client', $url ); + } +} From fdb4c398f996c56f4e1d3849bfcab8e7cccecf07 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:16:07 +0100 Subject: [PATCH 05/19] Verify code_verifier at the token endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- inc/endpoints/class-token.php | 14 +++- tests/test-token-endpoint.php | 136 ++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index b853c75..0dbd91b 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -44,6 +44,11 @@ public function register_routes() { 'type' => 'string', 'validate_callback' => 'rest_validate_request_arg', ], + 'code_verifier' => [ + 'required' => false, + 'type' => 'string', + 'validate_callback' => 'rest_validate_request_arg', + ], ], ] ); @@ -127,7 +132,14 @@ public function exchange_token( WP_REST_Request $request ) { return $auth_code; } - $is_valid = $auth_code->validate(); + // Read from the body only, not get_param(), which also reads $_GET on a + // POST route — a verifier belongs in the body, not the URL, since a URL + // is far more likely to end up in access logs or a Referer header. + $body_params = $request->get_body_params(); + $json_params = (array) $request->get_json_params(); + $code_verifier = $body_params['code_verifier'] ?? $json_params['code_verifier'] ?? null; + + $is_valid = $auth_code->validate( [ 'code_verifier' => $code_verifier ] ); if ( is_wp_error( $is_valid ) ) { // Invalid request, but code itself exists, so we should delete // (and silently ignore errors). diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index d7c7180..d2c2f24 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -11,6 +11,7 @@ use WP\OAuth2\Client; use WP\OAuth2\Endpoints\Token; +use WP\OAuth2\PKCE; use WP\OAuth2\Tokens\Authorization_Code; use WP_REST_Request; use WP_REST_Server; @@ -194,6 +195,141 @@ public function test_exchange_token_deletes_code_after_use() { $this->assertWPError( $reuse ); } + // ------------------------------------------------------------------------- + // Authorization code grant, PKCE + // ------------------------------------------------------------------------- + + public function test_exchange_token_with_correct_s256_verifier() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', $pair['code_verifier'] ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 200, $response->get_status() ); + $data = $response->get_data(); + $this->assertArrayHasKey( 'access_token', $data ); + } + + public function test_exchange_token_with_correct_plain_verifier() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_PLAIN ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', $pair['code_verifier'] ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 200, $response->get_status() ); + } + + public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', 'wrong-verifier-wrong-verifier-wrong-verifier' ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + + // The code must be consumed, not just rejected once. + $retry = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $retry->set_param( 'grant_type', 'authorization_code' ); + $retry->set_param( 'client_id', $this->client->get_id() ); + $retry->set_param( 'code', $code->get_code() ); + $retry->set_param( 'code_verifier', $pair['code_verifier'] ); + + $retry_response = $this->server->dispatch( $retry ); + $this->assertEquals( 400, $retry_response->get_status() ); + } + + public function test_exchange_token_missing_verifier_for_pkce_code_fails() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_verifier_for_non_pkce_code_fails() { + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $this->client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', PKCE::generate_verifier() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_rejects_array_valued_verifier_via_schema() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', [ 'x' ] ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_non_pkce_code_with_no_verifier_still_works() { + // Back-compat: a code minted without PKCE needs no verifier at all. + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $this->client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 200, $response->get_status() ); + } + // ------------------------------------------------------------------------- // Client credentials grant // ------------------------------------------------------------------------- From ca1ec3c5e8c1a181a3402be8fbae1ba463539f4e Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:17:09 +0100 Subject: [PATCH 06/19] Add PKCE settings to the client admin screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 . 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. --- inc/admin/namespace.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index f72af1b..f5cc67e 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -174,6 +174,7 @@ function validate_parameters( $params ) { $valid['type'] = wp_kses_post( $params['type'] ); $valid['client_credentials_enabled'] = ! empty( $params['client_credentials_enabled'] ); + $valid['pkce_required'] = ! empty( $params['pkce_required'] ); // Callback is required unless this client only uses client_credentials. if ( empty( $params['callback'] ) && ! $valid['client_credentials_enabled'] ) { @@ -219,6 +220,7 @@ function handle_edit_submit( Client $consumer = null ) { 'type' => $params['type'], 'callback' => $params['callback'], 'client_credentials_enabled' => $params['client_credentials_enabled'], + 'pkce_required' => $params['pkce_required'], ], ]; @@ -233,6 +235,7 @@ function handle_edit_submit( Client $consumer = null ) { 'type' => $params['type'], 'callback' => $params['callback'], 'client_credentials_enabled' => $params['client_credentials_enabled'], + 'pkce_required' => $params['pkce_required'], ], ]; @@ -326,12 +329,21 @@ function render_edit_page() { $data[ $key ] = empty( $form_data[ $key ] ) ? '' : $form_data[ $key ]; } $data['client_credentials_enabled'] = ! empty( $form_data['client_credentials_enabled'] ); + + if ( empty( $consumer ) && empty( $form_data ) ) { + // A genuinely fresh "Add Application" page, not a failed submission + // redisplay: default new clients to requiring PKCE. + $data['pkce_required'] = true; + } else { + $data['pkce_required'] = ! empty( $form_data['pkce_required'] ); + } } else { $data['name'] = $consumer->get_name(); $data['description'] = $consumer->get_description( true ); $data['type'] = $consumer->get_type(); $data['callback'] = $consumer->get_redirect_uris(); $data['client_credentials_enabled'] = $consumer->is_client_credentials_enabled(); + $data['pkce_required'] = $consumer->is_pkce_required(); if ( is_array( $data['callback'] ) ) { $data['callback'] = implode( ',', $data['callback'] ); @@ -457,6 +469,26 @@ function render_edit_page() {

+ + + + + + +

+ +

+ + Date: Mon, 31 Aug 2026 10:17:50 +0100 Subject: [PATCH 07/19] Advertise supported PKCE methods in the REST index 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. --- inc/namespace.php | 5 +++-- tests/test-namespace.php | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/inc/namespace.php b/inc/namespace.php index 6222e57..1078047 100644 --- a/inc/namespace.php +++ b/inc/namespace.php @@ -98,11 +98,12 @@ function register_in_index( WP_REST_Response $response ) { $data = $response->get_data(); $data['authentication']['oauth2'] = [ - 'endpoints' => [ + 'endpoints' => [ 'authorization' => get_authorization_url(), 'token' => get_token_url(), ], - 'grant_types' => array_keys( get_grant_types() ), + 'grant_types' => array_keys( get_grant_types() ), + 'code_challenge_methods_supported' => PKCE::supported_methods(), ]; $response->set_data( $data ); diff --git a/tests/test-namespace.php b/tests/test-namespace.php index 340156c..54f8138 100644 --- a/tests/test-namespace.php +++ b/tests/test-namespace.php @@ -12,6 +12,7 @@ use WP\OAuth2\Client; use WP\OAuth2\ClientInterface; use WP\OAuth2\PersonalClient; +use WP\OAuth2\PKCE; use WP\OAuth2\Types\Type; use WP_REST_Response; use WP_REST_Server; @@ -118,4 +119,30 @@ public function test_register_in_index_lists_grant_types() { $this->assertArrayHasKey( 'grant_types', $data['authentication']['oauth2'] ); $this->assertContains( 'authorization_code', $data['authentication']['oauth2']['grant_types'] ); } + + public function test_register_in_index_lists_pkce_methods() { + $response = new WP_REST_Response( [] ); + $response = register_in_index( $response ); + + $data = $response->get_data(); + $this->assertSame( + [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], + $data['authentication']['oauth2']['code_challenge_methods_supported'] + ); + } + + public function test_register_in_index_tracks_pkce_methods_filter() { + $filter = function () { + return [ PKCE::METHOD_S256 ]; + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $response = new WP_REST_Response( [] ); + $response = register_in_index( $response ); + $data = $response->get_data(); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertSame( [ PKCE::METHOD_S256 ], $data['authentication']['oauth2']['code_challenge_methods_supported'] ); + } } From 078552b4b42048b18a833994906577480fa7319a Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:19:12 +0100 Subject: [PATCH 08/19] Add WP-CLI command to generate PKCE pairs 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. --- inc/namespace.php | 6 +++ inc/utilities/class-command.php | 93 +++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 inc/utilities/class-command.php diff --git a/inc/namespace.php b/inc/namespace.php index 1078047..f2d161c 100644 --- a/inc/namespace.php +++ b/inc/namespace.php @@ -8,6 +8,7 @@ namespace WP\OAuth2; use WP\OAuth2\Types\Type; +use WP_CLI; use WP_REST_Response; function bootstrap() { @@ -28,6 +29,11 @@ function bootstrap() { add_action( 'init', __NAMESPACE__ . '\\rest_oauth2_load_authorize_page' ); add_action( 'admin_menu', __NAMESPACE__ . '\\Admin\\register' ); Admin\Profile\bootstrap(); + + // WP-CLI. + if ( class_exists( __NAMESPACE__ . '\\Utilities\\Command' ) ) { + WP_CLI::add_command( 'oauth2', __NAMESPACE__ . '\\Utilities\\Command' ); + } } /** diff --git a/inc/utilities/class-command.php b/inc/utilities/class-command.php new file mode 100644 index 0000000..15923aa --- /dev/null +++ b/inc/utilities/class-command.php @@ -0,0 +1,93 @@ +] + * : Use this code verifier instead of generating a random one. + * + * [--length=] + * : Length of the randomly generated code verifier. Ignored if a verifier is given. + * --- + * default: 64 + * --- + * + * [--method=] + * : Code challenge method to derive the challenge with. + * --- + * default: S256 + * options: + * - S256 + * - plain + * --- + * + * [--format=] + * : Render output in a particular format. + * --- + * default: table + * options: + * - table + * - json + * - csv + * - yaml + * --- + * + * ## EXAMPLES + * + * wp oauth2 generate-code-challenge + * wp oauth2 generate-code-challenge --method=plain + * + * @alias generate-code-challenge + * + * @param array $args Positional arguments. + * @param array $assoc_args Associative arguments. + */ + public function generate_code_challenge( $args, $assoc_args ) { + $method = $assoc_args['method']; + + if ( ! empty( $args[0] ) ) { + $verifier = $args[0]; + + if ( ! PKCE::is_valid_verifier( $verifier ) ) { + WP_CLI::error( 'The supplied verifier must be 43-128 characters from [A-Za-z0-9-._~].' ); + } + } else { + $length = (int) $assoc_args['length']; + if ( $length < PKCE::VERIFIER_MIN_LENGTH || $length > PKCE::VERIFIER_MAX_LENGTH ) { + WP_CLI::error( sprintf( 'Length must be between %d and %d.', PKCE::VERIFIER_MIN_LENGTH, PKCE::VERIFIER_MAX_LENGTH ) ); + } + + $verifier = PKCE::generate_verifier( $length ); + } + + $challenge = PKCE::derive_challenge( $verifier, $method ); + + $items = [ + [ + 'code_verifier' => $verifier, + 'code_challenge' => $challenge, + 'code_challenge_method' => $method, + ], + ]; + + Utils\format_items( $assoc_args['format'], $items, [ 'code_verifier', 'code_challenge', 'code_challenge_method' ] ); + } +} From f966cecf1f5f777aa007197144d52f83de98e152 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 31 Aug 2026 10:20:05 +0100 Subject: [PATCH 09/19] Document PKCE support 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. --- README.md | 48 ++++++++++++++++++++++++++++++++++++++++ inc/types/class-base.php | 9 +++++--- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 5950059..4f64a79 100755 --- a/README.md +++ b/README.md @@ -6,6 +6,54 @@ This plugin uses the OAuth 2 protocol to allow delegated authorization; that is, This plugin only supports WordPress >= 4.8. +## Proof Key for Code Exchange (PKCE) + +The plugin supports PKCE ([RFC 7636](https://tools.ietf.org/html/rfc7636)) for +the `authorization_code` grant, as protection against authorization code +interception. To use it, add two extra parameters to the initial +authorization request: + +* `code_challenge` (required) +* `code_challenge_method` (optional, defaults to `plain`; use `S256`) + +`S256` derives the challenge from a code verifier by SHA-256 hashing it, then +base64url-encoding the digest with no padding. For example: + +``` +code_verifier = dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk +code_challenge = E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM +``` + +This is **not** the same as `base64_encode( hash( 'sha256', $verifier ) )` — +that encodes the hex digest with standard base64, which is a different, wrong +value that RFC 7636's own worked example above will catch. + +When exchanging the code for a token, pass the original `code_verifier` as an +extra parameter to the token endpoint. The server derives the challenge from +it the same way and checks it matches the one supplied at authorization time. + +A client can be marked to require PKCE from its edit screen under +**Users → Applications**. When required, only `S256` satisfies the +requirement — `plain` remains an accepted method generally, but does not +count as PKCE having been used, since it offers no protection against a +malicious app on the same device reading the authorization request. + +Filters: `oauth2.pkce.supported_methods` (accepted `code_challenge_method` +values, default `S256` and `plain`), `oauth2.pkce.required_methods` (methods +that satisfy "PKCE required", default `S256` only), and `oauth2.pkce.required` +(override whether PKCE is required for a given client). + +## CLI Commands + +### PKCE + +Generate a random code verifier and its matching code challenge, for manually +testing a PKCE flow: + +``` +wp oauth2 generate-code-challenge +``` + ## Contributors Welcome! This plugin works and is in use in several production environments, but the user experience and documentation could be substantially improved. We welcome input and contributions to make this tool better! diff --git a/inc/types/class-base.php b/inc/types/class-base.php index 8b4756e..78574b0 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -222,7 +222,10 @@ protected function get_nonce_action( Client $client ) { * @param array $redirect_args Redirect args. * @param boolean $authorized True if authorized, false otherwise. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. May include PKCE's + * `code_challenge`/`code_challenge_method` for the + * authorization_code grant; never `code_verifier`, + * which is only ever supplied at the token endpoint. */ protected function filter_redirect_args( $redirect_args, $authorized, Client $client, $data ) { if ( ! $authorized ) { @@ -231,7 +234,7 @@ protected function filter_redirect_args( $redirect_args, $authorized, Client $cl * * @param array $redirect_args Redirect args. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. See filter_redirect_args(). */ return apply_filters( 'oauth2.redirect_args.cancelled', $redirect_args, $client, $data ); } @@ -241,7 +244,7 @@ protected function filter_redirect_args( $redirect_args, $authorized, Client $cl * * @param array $redirect_args Redirect args. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. See filter_redirect_args(). */ return apply_filters( 'oauth2.redirect_args.authorized', $redirect_args, $client, $data ); } From a94f613cfabc78d5c5dc4ee31bbd3c70c1ea5053 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Tue, 1 Sep 2026 12:38:49 +0100 Subject: [PATCH 10/19] Trigger CI checks Co-authored-by: CommandCodeBot From 0c481d659252c05eadcb77b0ecdfdba1506b0a0c Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Tue, 1 Sep 2026 15:05:11 +0100 Subject: [PATCH 11/19] Fix CI failures after merging main 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. --- inc/class-client.php | 2 +- tests/test-token-endpoint.php | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/inc/class-client.php b/inc/class-client.php index c3c84e0..ef29e94 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -418,7 +418,7 @@ public function update( $data ) { // Map of meta key => data key. Built this way, rather than as a fixed // array of values, so that a caller omitting a key leaves the existing // meta value untouched instead of silently resetting it to empty/false. - $fields = [ + $fields = [ static::REDIRECT_URI_KEY => 'callback', static::TYPE_KEY => 'type', static::CLIENT_CREDENTIALS_ENABLED_KEY => 'client_credentials_enabled', diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index d2c2f24..354308a 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -256,7 +256,9 @@ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $response = $this->server->dispatch( $request ); $this->assertEquals( 400, $response->get_status() ); - // The code must be consumed, not just rejected once. + // The code must be consumed, not just rejected once: it's deleted along + // with the failed attempt, so re-using it (even with the right verifier + // this time) now fails as an unknown code, not as a bad verifier. $retry = new WP_REST_Request( 'POST', '/oauth2/access_token' ); $retry->set_param( 'grant_type', 'authorization_code' ); $retry->set_param( 'client_id', $this->client->get_id() ); @@ -264,7 +266,7 @@ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $retry->set_param( 'code_verifier', $pair['code_verifier'] ); $retry_response = $this->server->dispatch( $retry ); - $this->assertEquals( 400, $retry_response->get_status() ); + $this->assertEquals( 404, $retry_response->get_status() ); } public function test_exchange_token_missing_verifier_for_pkce_code_fails() { From 0257a457936d6ccfb83cabaa760826e60617a1fe Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Tue, 1 Sep 2026 16:06:17 +0100 Subject: [PATCH 12/19] Fix PKCE error redirects silently landing on wp-admin instead of the 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/). --- inc/types/class-authorization-code.php | 3 +- inc/types/class-base.php | 3 +- tests/test-types-authorization-code.php | 46 +++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index 76b1701..93dac36 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -122,7 +122,8 @@ protected function handle_authorization_submission( $submit, Client $client, $da // which would silently mint a code with no challenge. if ( ! empty( $data['code_challenge'] ) && $code->get_code_challenge() !== $data['code_challenge'] ) { $code->delete(); - wp_safe_redirect( + // phpcs:ignore WordPress.Security.SafeRedirect -- Intentionally external redirect, secured via client registration. + wp_redirect( $this->get_error_redirect_url( $redirect_uri, 'server_error', diff --git a/inc/types/class-base.php b/inc/types/class-base.php index 78574b0..ebb5872 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -75,7 +75,8 @@ public function handle_authorisation() { if ( is_wp_error( $extra_params ) ) { $error_data = $extra_params->get_error_data(); $error_code = ! empty( $error_data['error'] ) ? $error_data['error'] : 'invalid_request'; - wp_safe_redirect( $this->get_error_redirect_url( $redirect_uri, $error_code, $extra_params->get_error_message(), $state ) ); + // phpcs:ignore WordPress.Security.SafeRedirect -- Intentionally external redirect, secured via client registration. + wp_redirect( $this->get_error_redirect_url( $redirect_uri, $error_code, $extra_params->get_error_message(), $state ) ); exit; } diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php index 985dafa..7861da9 100644 --- a/tests/test-types-authorization-code.php +++ b/tests/test-types-authorization-code.php @@ -14,6 +14,13 @@ use WP\OAuth2\Types\Authorization_Code; use WP\OAuth2\Types\Implicit; +/** + * Thrown from a 'wp_redirect' filter to capture the final redirect location + * and escape before the real exit(), without swallowing any other exception + * a bug elsewhere in the request might throw. + */ +class Redirect_Interrupt extends \RuntimeException {} + /** * Exposes the protected methods under test without going through * $_GET/$_POST/exit, so they can be driven with plain arrays. @@ -197,6 +204,45 @@ public function test_error_redirect_url_includes_state_when_present() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', 'xyz' ); $this->assertStringContainsString( 'state=xyz', $url ); } + + /** + * Regression test: an authorize-time PKCE error must redirect to the + * client's registered callback, not silently fall back to wp-admin. + * + * wp_safe_redirect() rejects a callback on a host other than the current + * site (there is no allowed_redirect_hosts filter registered anywhere in + * this plugin) and substitutes admin_url() instead, so the client would + * never learn the request failed. wp_redirect() and wp_safe_redirect() + * both funnel through the 'wp_redirect' filter, so hooking it captures + * the final location actually sent, whichever function was used, and + * lets the test escape before handle_authorisation()'s exit. + */ + public function test_pkce_error_redirects_to_client_callback_not_wp_admin() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $_GET['client_id'] = $client->get_id(); + $captured_location = null; + + add_filter( + 'wp_redirect', + function ( $location ) use ( &$captured_location ) { + $captured_location = $location; + throw new Redirect_Interrupt(); + } + ); + + try { + ( new Authorization_Code() )->handle_authorisation(); + $this->fail( 'Expected handle_authorisation() to redirect on a PKCE error.' ); + } catch ( Redirect_Interrupt $e ) { + // Expected: escapes before the real exit(); see filter above. + } + + unset( $_GET['client_id'] ); + + $this->assertNotNull( $captured_location ); + $this->assertStringStartsWith( 'https://example.com/callback', $captured_location ); + $this->assertStringContainsString( 'error=invalid_request', $captured_location ); + } } /** From c827fbadcd597cc801414cebc50d8d675c86ba93 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Thu, 17 Sep 2026 18:22:43 +0100 Subject: [PATCH 13/19] Advertise PKCE methods in the RFC 8414 metadata document 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 --- inc/well-known/namespace.php | 1 + tests/test-well-known.php | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/inc/well-known/namespace.php b/inc/well-known/namespace.php index 53eec08..f4bf61d 100644 --- a/inc/well-known/namespace.php +++ b/inc/well-known/namespace.php @@ -134,6 +134,7 @@ function get_authorization_server_metadata() { 'grant_types_supported' => get_grant_types_supported(), 'response_types_supported' => get_response_types_supported(), 'token_endpoint_auth_methods_supported' => [ 'none', 'client_secret_post', 'client_secret_basic' ], + 'code_challenge_methods_supported' => OAuth2\PKCE::supported_methods(), ]; /** diff --git a/tests/test-well-known.php b/tests/test-well-known.php index ecae459..4cc0c54 100644 --- a/tests/test-well-known.php +++ b/tests/test-well-known.php @@ -107,6 +107,11 @@ public function test_metadata_advertises_the_token_endpoint() { $this->assertStringContainsString( 'oauth2/access_token', $metadata['token_endpoint'] ); } + public function test_metadata_advertises_the_pkce_challenge_methods() { + $metadata = get_authorization_server_metadata(); + $this->assertContains( 'S256', $metadata['code_challenge_methods_supported'] ); + } + public function test_metadata_is_filterable() { add_filter( 'oauth2.well_known_authorization_server_metadata', function ( $metadata ) { $metadata['service_documentation'] = 'https://example.org/docs'; From 0b195894cc13989df48e1b47a0f38c336b8937c7 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Thu, 17 Sep 2026 18:52:20 +0100 Subject: [PATCH 14/19] Apply review feedback on PKCE Four changes from review on WP-API/OAuth2#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 --- inc/admin/namespace.php | 6 +- inc/endpoints/class-token.php | 2 +- inc/tokens/class-authorization-code.php | 14 ++-- inc/types/class-authorization-code.php | 96 +++++++++++++++---------- inc/types/class-base.php | 8 +-- inc/types/class-implicit.php | 6 +- tests/test-authorization-code.php | 14 ++-- tests/test-types-authorization-code.php | 40 +++++------ 8 files changed, 102 insertions(+), 84 deletions(-) diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index f5cc67e..562b952 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -471,7 +471,7 @@ function render_edit_page() { - +

- +

diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index 0dbd91b..709f117 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -139,7 +139,7 @@ public function exchange_token( WP_REST_Request $request ) { $json_params = (array) $request->get_json_params(); $code_verifier = $body_params['code_verifier'] ?? $json_params['code_verifier'] ?? null; - $is_valid = $auth_code->validate( [ 'code_verifier' => $code_verifier ] ); + $is_valid = $auth_code->validate( $code_verifier ); if ( is_wp_error( $is_valid ) ) { // Invalid request, but code itself exists, so we should delete // (and silently ignore errors). diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index 5c920e2..c2ed3a6 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -145,12 +145,10 @@ public function get_code_challenge_method() { /** * Validate the code for use. * - * @param array $args Other request arguments to validate. { - * @var string $code_verifier PKCE code verifier, if the client is using PKCE. - * } + * @param string|null $code_verifier PKCE code verifier, if the client is using PKCE. * @return bool|WP_Error True if valid, error describing problem otherwise. */ - public function validate( $args = [] ) { + public function validate( $code_verifier = null ) { $expiration = $this->get_expiration(); if ( is_wp_error( $expiration ) ) { return $expiration; @@ -169,7 +167,7 @@ public function validate( $args = [] ) { ); } - $verifier_check = $this->validate_code_verifier( $args ); + $verifier_check = $this->validate_code_verifier( $code_verifier ); if ( is_wp_error( $verifier_check ) ) { return $verifier_check; } @@ -180,13 +178,13 @@ public function validate( $args = [] ) { /** * Validate a PKCE code verifier against the stored code challenge. * - * @param array $args Request arguments, as passed to validate(). + * @param string|null $verifier Code verifier supplied at the token endpoint. * @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 ) { + protected function validate_code_verifier( $verifier ) { $challenge = $this->get_code_challenge(); - $verifier = isset( $args['code_verifier'] ) && is_string( $args['code_verifier'] ) ? $args['code_verifier'] : null; + $verifier = is_string( $verifier ) ? $verifier : null; if ( null === $challenge ) { if ( null === $verifier ) { diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index 93dac36..50eccd9 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -24,37 +24,35 @@ public function get_response_type_code() { } /** - * Gather and validate PKCE parameters from the authorisation request. + * Validate the PKCE parameters on the authorisation request. * * @param Client $client Client being authorised. * @param array $request Unslashed request parameters, from $_GET. * @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 ) { + protected function validate_extra_params( Client $client, array $request ) { $code_challenge = isset( $request['code_challenge'] ) && is_string( $request['code_challenge'] ) ? $request['code_challenge'] : null; $code_challenge_method = isset( $request['code_challenge_method'] ) && is_string( $request['code_challenge_method'] ) ? $request['code_challenge_method'] : null; - if ( null === $code_challenge ) { - if ( $client->is_pkce_required() ) { - return new WP_Error( - 'oauth2.types.authorization_code.gather_extra_params.pkce_required', - __( 'This client requires a code_challenge for the authorization_code grant.', 'oauth2' ), - [ 'error' => 'invalid_request' ] - ); - } + // The method defaults to 'plain' when a challenge arrives without one, + // per RFC 7636 section 4.3. + if ( null !== $code_challenge && null === $code_challenge_method ) { + $code_challenge_method = PKCE::METHOD_PLAIN; + } - return []; + $requirement = $this->check_pkce_requirement( $client, $code_challenge, $code_challenge_method ); + if ( is_wp_error( $requirement ) ) { + return $requirement; } - // The method defaults to 'plain' when omitted, per RFC 7636 section 4.3. - if ( null === $code_challenge_method ) { - $code_challenge_method = PKCE::METHOD_PLAIN; + if ( null === $code_challenge ) { + return []; } if ( ! in_array( $code_challenge_method, PKCE::supported_methods(), true ) ) { return new WP_Error( - 'oauth2.types.authorization_code.gather_extra_params.unsupported_method', + 'oauth2.types.authorization_code.validate_extra_params.unsupported_method', __( 'Unsupported code_challenge_method. Note that this value is case-sensitive; use S256.', 'oauth2' ), [ 'error' => 'invalid_request' ] ); @@ -62,40 +60,62 @@ protected function gather_extra_params( Client $client, array $request ) { if ( ! PKCE::is_valid_challenge( $code_challenge, $code_challenge_method ) ) { return new WP_Error( - 'oauth2.types.authorization_code.gather_extra_params.invalid_challenge', + 'oauth2.types.authorization_code.validate_extra_params.invalid_challenge', __( 'Invalid code_challenge.', 'oauth2' ), [ 'error' => 'invalid_request' ] ); } - if ( $client->is_pkce_required() ) { - /** - * Filter the code_challenge_method values that satisfy a client's PKCE requirement. - * - * Defaults to S256 only. `plain` offers no protection against a - * malicious app on the same device reading the authorization - * request, which is exactly the threat PKCE exists to mitigate - * for public clients (RFC 9700 section 2.1.1), so it does not - * satisfy a requirement even though it remains an accepted method. - * - * @param string[] $methods Methods that satisfy "PKCE required". - */ - $required_methods = apply_filters( 'oauth2.pkce.required_methods', [ PKCE::METHOD_S256 ] ); - if ( ! in_array( $code_challenge_method, $required_methods, true ) ) { - return new WP_Error( - 'oauth2.types.authorization_code.gather_extra_params.weak_method', - __( 'This client requires PKCE with the S256 code_challenge_method.', 'oauth2' ), - [ 'error' => 'invalid_request' ] - ); - } - } - return [ 'code_challenge' => $code_challenge, 'code_challenge_method' => $code_challenge_method, ]; } + /** + * Check a request against the client's PKCE requirement. + * + * @param Client $client Client being authorised. + * @param string|null $code_challenge Code challenge from the request, if any. + * @param string|null $code_challenge_method Challenge method from the request, if any. + * @return true|WP_Error True if the request meets the requirement, error otherwise. + */ + protected function check_pkce_requirement( Client $client, $code_challenge, $code_challenge_method ) { + if ( ! $client->is_pkce_required() ) { + return true; + } + + if ( null === $code_challenge ) { + return new WP_Error( + 'oauth2.types.authorization_code.check_pkce_requirement.pkce_required', + __( 'This client requires a code_challenge for the authorization_code grant.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + /** + * Filter the code_challenge_method values that satisfy a client's PKCE requirement. + * + * Defaults to S256 only. `plain` offers no protection against a + * malicious app on the same device reading the authorization + * request, which is exactly the threat PKCE exists to mitigate + * for public clients (RFC 9700 section 2.1.1), so it does not + * satisfy a requirement even though it remains an accepted method. + * + * @param string[] $methods Methods that satisfy "PKCE required". + */ + $required_methods = apply_filters( 'oauth2.pkce.required_methods', [ PKCE::METHOD_S256 ] ); + if ( ! in_array( $code_challenge_method, $required_methods, true ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.check_pkce_requirement.weak_method', + __( 'This client requires PKCE with the S256 code_challenge_method.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + return true; + } + /** * Handles the authorization. * diff --git a/inc/types/class-base.php b/inc/types/class-base.php index ebb5872..be17e47 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -67,11 +67,11 @@ public function handle_authorisation() { return $redirect_uri; } - // Gather and validate any grant-type-specific parameters (e.g. PKCE). + // Validate any grant-type-specific parameters (e.g. PKCE). // This runs after the redirect URI is known to be valid, and before the // login redirect, so a malformed request fails fast and can be reported // back to the client rather than dying with an HTML error page. - $extra_params = $this->gather_extra_params( $client, wp_unslash( $_GET ) ); + $extra_params = $this->validate_extra_params( $client, wp_unslash( $_GET ) ); if ( is_wp_error( $extra_params ) ) { $error_data = $extra_params->get_error_data(); $error_code = ! empty( $error_data['error'] ) ? $error_data['error'] : 'invalid_request'; @@ -125,7 +125,7 @@ public function handle_authorisation() { } /** - * Gather and validate any grant-type-specific parameters from the request. + * Validate any grant-type-specific parameters, and return the ones to keep. * * Runs on both the initial GET and the consent-form POST, since the * authorisation form posts back to the original request URI, so $_GET is @@ -138,7 +138,7 @@ public function handle_authorisation() { * @return array|WP_Error Extra data to merge into the $data passed to * handle_authorization_submission(), or an error. */ - protected function gather_extra_params( Client $client, array $request ) { + protected function validate_extra_params( Client $client, array $request ) { return []; } diff --git a/inc/types/class-implicit.php b/inc/types/class-implicit.php index d727a62..413b623 100644 --- a/inc/types/class-implicit.php +++ b/inc/types/class-implicit.php @@ -32,12 +32,12 @@ public function get_response_type_code() { * * @param Client $client Client being authorised. * @param array $request Unslashed request parameters, from $_GET. - * @return array|WP_Error + * @return array|WP_Error Empty array if the grant is allowed, error otherwise. */ - protected function gather_extra_params( Client $client, array $request ) { + protected function validate_extra_params( Client $client, array $request ) { if ( $client->is_pkce_required() ) { return new WP_Error( - 'oauth2.types.implicit.gather_extra_params.pkce_required', + 'oauth2.types.implicit.validate_extra_params.pkce_required', __( 'This client requires PKCE, which the implicit grant cannot support. Use the authorization_code grant instead.', 'oauth2' ), [ 'error' => 'unauthorized_client' ] ); diff --git a/tests/test-authorization-code.php b/tests/test-authorization-code.php index ab6223a..e65844f 100644 --- a/tests/test-authorization-code.php +++ b/tests/test-authorization-code.php @@ -172,7 +172,7 @@ public function test_validate_passes_with_correct_s256_verifier() { ] ); - $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + $this->assertTrue( $code->validate( static::RFC_VERIFIER ) ); } public function test_validate_passes_with_correct_plain_verifier() { @@ -185,7 +185,7 @@ public function test_validate_passes_with_correct_plain_verifier() { ] ); - $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + $this->assertTrue( $code->validate( static::RFC_VERIFIER ) ); } public function test_validate_fails_with_wrong_verifier() { @@ -198,7 +198,7 @@ public function test_validate_fails_with_wrong_verifier() { ] ); - $result = $code->validate( [ 'code_verifier' => 'wrong-verifier-wrong-verifier-wrong-verifier' ] ); + $result = $code->validate( 'wrong-verifier-wrong-verifier-wrong-verifier' ); $this->assertWPError( $result ); $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); } @@ -228,13 +228,13 @@ public function test_validate_fails_with_malformed_verifier() { ] ); - $result = $code->validate( [ 'code_verifier' => 'too-short' ] ); + $result = $code->validate( 'too-short' ); $this->assertWPError( $result ); } public function test_validate_rejects_verifier_for_code_with_no_stored_challenge() { $code = Authorization_Code::create( $this->client, $this->user ); - $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + $result = $code->validate( static::RFC_VERIFIER ); $this->assertWPError( $result ); $this->assertEquals( 'oauth2.tokens.authorization_code.validate.unexpected_verifier', $result->get_error_code() ); @@ -245,7 +245,7 @@ public function test_validate_allows_unexpected_verifier_when_filtered() { $filter = '__return_false'; add_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); - $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + $result = $code->validate( static::RFC_VERIFIER ); remove_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); $this->assertTrue( $result ); @@ -260,7 +260,7 @@ public function test_validate_fails_closed_when_stored_method_is_missing() { $value['code_challenge'] = static::RFC_CHALLENGE; update_post_meta( $this->client->get_post_id(), $meta_key, $value ); - $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + $result = $code->validate( static::RFC_VERIFIER ); $this->assertWPError( $result ); $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_challenge_method', $result->get_error_code() ); } diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php index 7861da9..7b8d7ee 100644 --- a/tests/test-types-authorization-code.php +++ b/tests/test-types-authorization-code.php @@ -26,8 +26,8 @@ class Redirect_Interrupt extends \RuntimeException {} * $_GET/$_POST/exit, so they can be driven with plain arrays. */ class Test_Exposed_Authorization_Code_Type extends Authorization_Code { - public function gather_extra_params_public( Client $client, array $request ) { - return $this->gather_extra_params( $client, $request ); + public function validate_extra_params_public( Client $client, array $request ) { + return $this->validate_extra_params( $client, $request ); } public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { @@ -39,8 +39,8 @@ public function get_error_redirect_url_public( $redirect_uri, $error, $descripti * Exposes the protected methods under test for the implicit grant. */ class Test_Exposed_Implicit_Type extends Implicit { - public function gather_extra_params_public( Client $client, array $request ) { - return $this->gather_extra_params( $client, $request ); + public function validate_extra_params_public( Client $client, array $request ) { + return $this->validate_extra_params( $client, $request ); } public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { @@ -70,12 +70,12 @@ public function set_up() { } public function test_no_challenge_returns_empty_array() { - $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + $this->assertSame( [], $this->type->validate_extra_params_public( $this->client, [] ) ); } public function test_valid_s256_challenge_returns_both_keys() { $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => $pair['code_challenge'], @@ -88,7 +88,7 @@ public function test_valid_s256_challenge_returns_both_keys() { } public function test_unsupported_method_is_rejected() { - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => str_repeat( 'a', 43 ), @@ -101,7 +101,7 @@ public function test_unsupported_method_is_rejected() { } public function test_wrongly_cased_method_is_rejected() { - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => str_repeat( 'a', 43 ), @@ -113,7 +113,7 @@ public function test_wrongly_cased_method_is_rejected() { } public function test_malformed_challenge_is_rejected() { - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => 'too-short', @@ -125,7 +125,7 @@ public function test_malformed_challenge_is_rejected() { } public function test_method_without_challenge_is_ignored() { - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge_method' => 'S256' ] ); @@ -135,7 +135,7 @@ public function test_method_without_challenge_is_ignored() { public function test_omitted_method_normalizes_to_plain() { $verifier = PKCE::generate_verifier(); - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => $verifier ] ); @@ -144,7 +144,7 @@ public function test_omitted_method_normalizes_to_plain() { } public function test_array_valued_challenge_is_treated_as_absent() { - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => [ 'x' ] ] ); @@ -154,16 +154,16 @@ public function test_array_valued_challenge_is_treated_as_absent() { public function test_pkce_required_client_without_challenge_is_rejected() { $client = $this->create_client( [ 'pkce_required' => true ] ); - $result = $this->type->gather_extra_params_public( $client, [] ); + $result = $this->type->validate_extra_params_public( $client, [] ); $this->assertWPError( $result ); - $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.pkce_required', $result->get_error_code() ); + $this->assertEquals( 'oauth2.types.authorization_code.check_pkce_requirement.pkce_required', $result->get_error_code() ); } public function test_pkce_required_client_with_plain_is_rejected() { $client = $this->create_client( [ 'pkce_required' => true ] ); $verifier = PKCE::generate_verifier(); - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $client, [ 'code_challenge' => $verifier, @@ -172,13 +172,13 @@ public function test_pkce_required_client_with_plain_is_rejected() { ); $this->assertWPError( $result ); - $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.weak_method', $result->get_error_code() ); + $this->assertEquals( 'oauth2.types.authorization_code.check_pkce_requirement.weak_method', $result->get_error_code() ); } public function test_pkce_required_client_with_s256_is_accepted() { $client = $this->create_client( [ 'pkce_required' => true ] ); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); - $result = $this->type->gather_extra_params_public( + $result = $this->type->validate_extra_params_public( $client, [ 'code_challenge' => $pair['code_challenge'], @@ -267,15 +267,15 @@ public function set_up() { } public function test_non_pkce_client_is_allowed() { - $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + $this->assertSame( [], $this->type->validate_extra_params_public( $this->client, [] ) ); } public function test_pkce_required_client_is_refused() { $client = $this->create_client( [ 'pkce_required' => true ] ); - $result = $this->type->gather_extra_params_public( $client, [] ); + $result = $this->type->validate_extra_params_public( $client, [] ); $this->assertWPError( $result ); - $this->assertEquals( 'oauth2.types.implicit.gather_extra_params.pkce_required', $result->get_error_code() ); + $this->assertEquals( 'oauth2.types.implicit.validate_extra_params.pkce_required', $result->get_error_code() ); } public function test_error_redirect_url_uses_fragment() { From 40696fea341ce3568a7357a9c86ef536653876bb Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 23 Sep 2026 12:40:40 +0100 Subject: [PATCH 15/19] Apply Copilot review feedback on PKCE error handling Six fixes from Copilot's review of WP-API/OAuth2#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 --- inc/class-pkce.php | 6 ++- inc/types/class-authorization-code.php | 32 ++++++++++--- inc/types/class-base.php | 2 +- inc/types/class-implicit.php | 4 +- tests/test-pkce.php | 11 +++++ tests/test-types-authorization-code.php | 64 +++++++++++++++++++++++-- 6 files changed, 104 insertions(+), 15 deletions(-) diff --git a/inc/class-pkce.php b/inc/class-pkce.php index f13cf25..1721ca2 100644 --- a/inc/class-pkce.php +++ b/inc/class-pkce.php @@ -30,12 +30,16 @@ public static function supported_methods() { /** * Filter the PKCE code challenge methods this site accepts. * + * Only the built-in `S256` and `plain` methods can be enabled; any + * other value is dropped, since no challenge can be derived for it. * Comparisons against these values are case-sensitive, per RFC 7636 * section 4.3. * * @param string[] $methods Supported `code_challenge_method` values. */ - return apply_filters( 'oauth2.pkce.supported_methods', [ static::METHOD_S256, static::METHOD_PLAIN ] ); + $methods = apply_filters( 'oauth2.pkce.supported_methods', [ static::METHOD_S256, static::METHOD_PLAIN ] ); + + return array_values( array_intersect( (array) $methods, [ static::METHOD_S256, static::METHOD_PLAIN ] ) ); } /** diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index 50eccd9..33c639f 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -32,8 +32,27 @@ public function get_response_type_code() { * through to the minted code, or an error. */ protected function validate_extra_params( Client $client, array $request ) { - $code_challenge = isset( $request['code_challenge'] ) && is_string( $request['code_challenge'] ) ? $request['code_challenge'] : null; - $code_challenge_method = isset( $request['code_challenge_method'] ) && is_string( $request['code_challenge_method'] ) ? $request['code_challenge_method'] : null; + foreach ( [ 'code_challenge', 'code_challenge_method' ] as $key ) { + if ( isset( $request[ $key ] ) && ! is_string( $request[ $key ] ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.validate_extra_params.malformed', + /* translators: %s: request parameter name */ + sprintf( __( 'The %s parameter must be a single string.', 'oauth2' ), $key ), + [ 'error' => 'invalid_request' ] + ); + } + } + + $code_challenge = $request['code_challenge'] ?? null; + $code_challenge_method = $request['code_challenge_method'] ?? null; + + if ( null === $code_challenge && null !== $code_challenge_method ) { + return new WP_Error( + 'oauth2.types.authorization_code.validate_extra_params.method_without_challenge', + __( 'A code_challenge_method was supplied without a code_challenge.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } // The method defaults to 'plain' when a challenge arrives without one, // per RFC 7636 section 4.3. @@ -108,7 +127,8 @@ protected function check_pkce_requirement( Client $client, $code_challenge, $cod if ( ! in_array( $code_challenge_method, $required_methods, true ) ) { return new WP_Error( 'oauth2.types.authorization_code.check_pkce_requirement.weak_method', - __( 'This client requires PKCE with the S256 code_challenge_method.', 'oauth2' ), + /* translators: %s: comma-separated list of code_challenge_method values */ + sprintf( __( 'This client requires PKCE with one of these code_challenge_method values: %s.', 'oauth2' ), implode( ', ', $required_methods ) ), [ 'error' => 'invalid_request' ] ); } @@ -137,9 +157,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da return $code; } - // Defends against a third-party Client subclass overriding - // generate_authorization_code() with the pre-PKCE arity, - // which would silently mint a code with no challenge. + // Fail closed if the minted code does not carry the requested challenge. if ( ! empty( $data['code_challenge'] ) && $code->get_code_challenge() !== $data['code_challenge'] ) { $code->delete(); // phpcs:ignore WordPress.Security.SafeRedirect -- Intentionally external redirect, secured via client registration. @@ -148,7 +166,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da $redirect_uri, 'server_error', __( 'Could not persist the PKCE challenge for this authorization code.', 'oauth2' ), - ! empty( $data['state'] ) ? $data['state'] : null + $data['state'] ) ); exit; diff --git a/inc/types/class-base.php b/inc/types/class-base.php index d327c5a..9d5cb82 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -156,7 +156,7 @@ protected function get_error_redirect_url( $redirect_uri, $error, $description, 'error' => $error, 'error_description' => $description, ]; - if ( ! empty( $state ) ) { + if ( null !== $state ) { $args['state'] = $state; } diff --git a/inc/types/class-implicit.php b/inc/types/class-implicit.php index 413b623..a236cec 100644 --- a/inc/types/class-implicit.php +++ b/inc/types/class-implicit.php @@ -120,10 +120,10 @@ protected function get_error_redirect_url( $redirect_uri, $error, $description, 'error' => $error, 'error_description' => $description, ]; - if ( ! empty( $state ) ) { + if ( null !== $state ) { $args['state'] = $state; } - return $redirect_uri . '#' . build_query( $args ); + return $redirect_uri . '#' . build_query( urlencode_deep( $args ) ); } } diff --git a/tests/test-pkce.php b/tests/test-pkce.php index 9b6b113..73a7b71 100644 --- a/tests/test-pkce.php +++ b/tests/test-pkce.php @@ -128,6 +128,17 @@ public function test_supported_methods_defaults_to_s256_and_plain() { $this->assertSame( [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], PKCE::supported_methods() ); } + public function test_supported_methods_filter_drops_unknown_methods() { + $filter = function ( $methods ) { + return array_merge( $methods, [ 'S512' ] ); + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertSame( [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], PKCE::supported_methods() ); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + } + public function test_supported_methods_filter_removes_plain() { $filter = function () { return [ PKCE::METHOD_S256 ]; diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php index 7b8d7ee..8dd2e49 100644 --- a/tests/test-types-authorization-code.php +++ b/tests/test-types-authorization-code.php @@ -124,13 +124,14 @@ public function test_malformed_challenge_is_rejected() { $this->assertWPError( $result ); } - public function test_method_without_challenge_is_ignored() { + public function test_method_without_challenge_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge_method' => 'S256' ] ); - $this->assertSame( [], $result ); + $this->assertWPError( $result ); + $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } public function test_omitted_method_normalizes_to_plain() { @@ -143,13 +144,27 @@ public function test_omitted_method_normalizes_to_plain() { $this->assertSame( 'plain', $result['code_challenge_method'] ); } - public function test_array_valued_challenge_is_treated_as_absent() { + public function test_array_valued_challenge_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, [ 'code_challenge' => [ 'x' ] ] ); - $this->assertSame( [], $result ); + $this->assertWPError( $result ); + $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); + } + + public function test_array_valued_method_is_rejected() { + $result = $this->type->validate_extra_params_public( + $this->client, + [ + 'code_challenge' => str_repeat( 'a', 43 ), + 'code_challenge_method' => [ 'S256' ], + ] + ); + + $this->assertWPError( $result ); + $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } public function test_pkce_required_client_without_challenge_is_rejected() { @@ -175,6 +190,29 @@ public function test_pkce_required_client_with_plain_is_rejected() { $this->assertEquals( 'oauth2.types.authorization_code.check_pkce_requirement.weak_method', $result->get_error_code() ); } + public function test_weak_method_error_names_the_filtered_required_methods() { + $filter = function () { + return [ PKCE::METHOD_PLAIN ]; + }; + add_filter( 'oauth2.pkce.required_methods', $filter ); + + $client = $this->create_client( [ 'pkce_required' => true ] ); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->validate_extra_params_public( + $client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + remove_filter( 'oauth2.pkce.required_methods', $filter ); + + $this->assertWPError( $result ); + $this->assertStringContainsString( 'plain', $result->get_error_message() ); + $this->assertStringNotContainsString( 'S256', $result->get_error_message() ); + } + public function test_pkce_required_client_with_s256_is_accepted() { $client = $this->create_client( [ 'pkce_required' => true ] ); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -205,6 +243,11 @@ public function test_error_redirect_url_includes_state_when_present() { $this->assertStringContainsString( 'state=xyz', $url ); } + public function test_error_redirect_url_keeps_zero_state() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', '0' ); + $this->assertStringContainsString( 'state=0', $url ); + } + /** * Regression test: an authorize-time PKCE error must redirect to the * client's registered callback, not silently fall back to wp-admin. @@ -283,4 +326,17 @@ public function test_error_redirect_url_uses_fragment() { $this->assertStringContainsString( '#', $url ); $this->assertStringContainsString( 'error=unauthorized_client', $url ); } + + public function test_error_redirect_url_encodes_fragment_values() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'A & B #c', 'x&y=z' ); + + parse_str( wp_parse_url( $url, PHP_URL_FRAGMENT ), $args ); + $this->assertSame( 'A & B #c', $args['error_description'] ); + $this->assertSame( 'x&y=z', $args['state'] ); + } + + public function test_error_redirect_url_keeps_zero_state() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'Nope.', '0' ); + $this->assertStringContainsString( 'state=0', $url ); + } } From 6cd40690c422c6513922d3cd5f4ff6d8422b5800 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 23 Sep 2026 12:49:04 +0100 Subject: [PATCH 16/19] Name the supported PKCE methods in the unsupported-method error 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 --- inc/types/class-authorization-code.php | 6 ++++-- tests/test-types-authorization-code.php | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index 33c639f..e9bbc33 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -69,10 +69,12 @@ protected function validate_extra_params( Client $client, array $request ) { return []; } - if ( ! in_array( $code_challenge_method, PKCE::supported_methods(), true ) ) { + $supported_methods = PKCE::supported_methods(); + if ( ! in_array( $code_challenge_method, $supported_methods, true ) ) { return new WP_Error( 'oauth2.types.authorization_code.validate_extra_params.unsupported_method', - __( 'Unsupported code_challenge_method. Note that this value is case-sensitive; use S256.', 'oauth2' ), + /* translators: %s: comma-separated list of code_challenge_method values */ + sprintf( __( 'Unsupported code_challenge_method. Use one of: %s. This value is case-sensitive.', 'oauth2' ), implode( ', ', $supported_methods ) ), [ 'error' => 'invalid_request' ] ); } diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php index 8dd2e49..104eb31 100644 --- a/tests/test-types-authorization-code.php +++ b/tests/test-types-authorization-code.php @@ -100,6 +100,28 @@ public function test_unsupported_method_is_rejected() { $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } + public function test_unsupported_method_error_names_the_filtered_supported_methods() { + $filter = function () { + return [ PKCE::METHOD_PLAIN ]; + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->validate_extra_params_public( + $this->client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertWPError( $result ); + $this->assertStringContainsString( 'plain', $result->get_error_message() ); + $this->assertStringNotContainsString( 'S256', $result->get_error_message() ); + } + public function test_wrongly_cased_method_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, From 3d64684386034c30741b2ad98a0b27bc3f594ee5 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 23 Sep 2026 19:39:43 +0100 Subject: [PATCH 17/19] Link PKCE tests to the RFC sections they cover, and fill gaps 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 --- tests/test-authorization-code.php | 67 ++++++++ tests/test-client.php | 5 + tests/test-pkce.php | 110 +++++++++++++ tests/test-token-endpoint.php | 82 ++++++++++ tests/test-types-authorization-code.php | 196 ++++++++++++++++++++++++ tests/test-well-known.php | 20 +++ 6 files changed, 480 insertions(+) diff --git a/tests/test-authorization-code.php b/tests/test-authorization-code.php index e65844f..3a8583a 100644 --- a/tests/test-authorization-code.php +++ b/tests/test-authorization-code.php @@ -10,6 +10,7 @@ require_once __DIR__ . '/class-test-case.php'; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; use WP\OAuth2\Tokens\Authorization_Code; use WP_User; @@ -116,6 +117,11 @@ public function test_create_without_pkce_data_stores_no_challenge() { $this->assertNull( $code->get_code_challenge_method() ); } + /** + * RFC 7636 section 4.4: the server stores the challenge and method with the authorization code. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4 + */ public function test_create_with_pkce_data_stores_challenge_and_method() { $code = Authorization_Code::create( $this->client, @@ -130,6 +136,11 @@ public function test_create_with_pkce_data_stores_challenge_and_method() { $this->assertSame( 'S256', $code->get_code_challenge_method() ); } + /** + * RFC 7636 section 4.3: the method is "S256" or "plain", and defaults to "plain" when omitted. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 + */ public function test_create_defaults_challenge_method_to_plain_when_omitted() { $code = Authorization_Code::create( $this->client, @@ -162,6 +173,11 @@ public function test_validate_with_no_args_still_passes_for_non_pkce_code() { $this->assertTrue( $code->validate() ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_validate_passes_with_correct_s256_verifier() { $code = Authorization_Code::create( $this->client, @@ -175,6 +191,11 @@ public function test_validate_passes_with_correct_s256_verifier() { $this->assertTrue( $code->validate( static::RFC_VERIFIER ) ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_validate_passes_with_correct_plain_verifier() { $code = Authorization_Code::create( $this->client, @@ -188,6 +209,11 @@ public function test_validate_passes_with_correct_plain_verifier() { $this->assertTrue( $code->validate( static::RFC_VERIFIER ) ); } + /** + * RFC 7636 section 4.6: a verifier that does not match the stored challenge gets invalid_grant. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_validate_fails_with_wrong_verifier() { $code = Authorization_Code::create( $this->client, @@ -203,6 +229,32 @@ public function test_validate_fails_with_wrong_verifier() { $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * Sending the S256 challenge itself as the verifier must fail, since the + * server hashes it rather than comparing it as plain. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ + public function test_validate_fails_when_the_s256_challenge_is_sent_as_the_verifier() { + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $this->user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => PKCE::METHOD_S256, + ] ); + + $result = $code->validate( $pair['code_challenge'] ); + + $this->assertWPError( $result ); + $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); + } + + /** + * RFC 7636 section 4.5: the client sends the code_verifier with the token request. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 + */ public function test_validate_fails_with_missing_verifier() { $code = Authorization_Code::create( $this->client, @@ -218,6 +270,11 @@ public function test_validate_fails_with_missing_verifier() { $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_verifier', $result->get_error_code() ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_validate_fails_with_malformed_verifier() { $code = Authorization_Code::create( $this->client, @@ -232,6 +289,11 @@ public function test_validate_fails_with_malformed_verifier() { $this->assertWPError( $result ); } + /** + * RFC 9700 section 4.8.2: a code_verifier for a code issued without a code_challenge must be rejected. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-4.8.2 + */ public function test_validate_rejects_verifier_for_code_with_no_stored_challenge() { $code = Authorization_Code::create( $this->client, $this->user ); $result = $code->validate( static::RFC_VERIFIER ); @@ -240,6 +302,11 @@ public function test_validate_rejects_verifier_for_code_with_no_stored_challenge $this->assertEquals( 'oauth2.tokens.authorization_code.validate.unexpected_verifier', $result->get_error_code() ); } + /** + * RFC 9700 section 4.8.2: a code_verifier for a code issued without a code_challenge must be rejected. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-4.8.2 + */ public function test_validate_allows_unexpected_verifier_when_filtered() { $code = Authorization_Code::create( $this->client, $this->user ); diff --git a/tests/test-client.php b/tests/test-client.php index 57f9bc7..e62e16e 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -286,6 +286,11 @@ public function test_generate_authorization_code_without_data_still_works() { $this->assertNull( $code->get_code_challenge() ); } + /** + * RFC 7636 section 4.4: the server stores the challenge and method with the authorization code. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4 + */ public function test_generate_authorization_code_stores_pkce_challenge() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair(); diff --git a/tests/test-pkce.php b/tests/test-pkce.php index 73a7b71..ae4101b 100644 --- a/tests/test-pkce.php +++ b/tests/test-pkce.php @@ -20,10 +20,20 @@ class Test_PKCE extends Test_Case { const RFC_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; const RFC_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + /** + * RFC 7636 Appendix B: the worked S256 example. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#appendix-B + */ public function test_derive_challenge_matches_rfc_7636_test_vector() { $this->assertSame( static::RFC_CHALLENGE, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_s256_challenge_is_unpadded_base64url() { $challenge = PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ); @@ -34,26 +44,56 @@ public function test_s256_challenge_is_unpadded_base64url() { $this->assertStringNotContainsString( '/', $challenge ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_plain_challenge_is_the_verifier_verbatim() { $this->assertSame( static::RFC_VERIFIER, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_derive_challenge_returns_null_for_unknown_method() { $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 'md5' ) ); } + /** + * RFC 7636 section 4.3: the method is "S256" or "plain", and defaults to "plain" when omitted. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 + */ public function test_derive_challenge_is_case_sensitive() { $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 's256' ) ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_verify_true_for_matching_s256_pair() { $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.6: a verifier that does not match the stored challenge gets invalid_grant. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_verify_false_for_wrong_verifier() { $this->assertFalse( PKCE::verify( 'wrong-verifier-wrong-verifier-wrong-verifier', static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_verify_true_for_matching_plain_pair() { $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); } @@ -70,16 +110,31 @@ public function test_verify_returns_false_not_typeerror_for_array_challenge() { $this->assertFalse( PKCE::verify( static::RFC_VERIFIER, [ 'x' ], PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_is_valid_verifier_accepts_boundary_lengths() { $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) ) ); $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 128 ) ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_is_valid_verifier_rejects_lengths_outside_boundary() { $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 42 ) ) ); $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 129 ) ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_is_valid_verifier_rejects_disallowed_characters() { $base = str_repeat( 'a', 42 ); @@ -90,36 +145,71 @@ public function test_is_valid_verifier_rejects_disallowed_characters() { $this->assertFalse( PKCE::is_valid_verifier( $base . '%' ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_is_valid_verifier_rejects_trailing_newline() { // A `$`-anchored regex would accept a trailing newline; `\z` must not. $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) . "\n" ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_is_valid_verifier_rejects_non_string() { $this->assertFalse( PKCE::is_valid_verifier( [ 'x' ] ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_is_valid_challenge_for_s256_requires_exactly_43_characters() { $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_S256 ) ); $this->assertTrue( PKCE::is_valid_challenge( str_repeat( 'a', 43 ), PKCE::METHOD_S256 ) ); $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 44 ), PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_is_valid_challenge_for_s256_rejects_padding() { $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '=', PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_is_valid_challenge_for_s256_rejects_verifier_only_characters() { // '.' and '~' are valid in a verifier, but not in base64url. $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '.', PKCE::METHOD_S256 ) ); $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '~', PKCE::METHOD_S256 ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_is_valid_challenge_for_plain_uses_verifier_rules() { $this->assertTrue( PKCE::is_valid_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_PLAIN ) ); } + /** + * RFC 7636 section 4.2: S256 is BASE64URL(SHA256(verifier)) with no padding, and plain is the verifier itself. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.2 + */ public function test_is_valid_challenge_rejects_unknown_method() { $this->assertFalse( PKCE::is_valid_challenge( static::RFC_CHALLENGE, 'md5' ) ); } @@ -150,21 +240,41 @@ public function test_supported_methods_filter_removes_plain() { remove_filter( 'oauth2.pkce.supported_methods', $filter ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_generate_verifier_produces_a_valid_verifier() { $verifier = PKCE::generate_verifier(); $this->assertTrue( PKCE::is_valid_verifier( $verifier ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_generate_verifier_respects_requested_length() { $this->assertSame( 64, strlen( PKCE::generate_verifier( 64 ) ) ); $this->assertSame( 100, strlen( PKCE::generate_verifier( 100 ) ) ); } + /** + * RFC 7636 section 4.1: a code verifier is 43-128 characters from the unreserved set. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + */ public function test_generate_verifier_clamps_to_valid_range() { $this->assertSame( PKCE::VERIFIER_MIN_LENGTH, strlen( PKCE::generate_verifier( 10 ) ) ); $this->assertSame( PKCE::VERIFIER_MAX_LENGTH, strlen( PKCE::generate_verifier( 500 ) ) ); } + /** + * RFC 7636 section 7.1: the verifier must have enough entropy to be impractical to guess. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-7.1 + */ public function test_generate_verifier_is_not_deterministic() { $this->assertNotSame( PKCE::generate_verifier(), PKCE::generate_verifier() ); } diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index 0dcfb4c..e63d1c0 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -200,6 +200,11 @@ public function test_exchange_token_deletes_code_after_use() { // Authorization code grant, PKCE // ------------------------------------------------------------------------- + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_exchange_token_with_correct_s256_verifier() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -221,6 +226,11 @@ public function test_exchange_token_with_correct_s256_verifier() { $this->assertArrayHasKey( 'access_token', $data ); } + /** + * RFC 7636 section 4.6: the server derives the challenge from the verifier with the stored method and compares. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_exchange_token_with_correct_plain_verifier() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair( PKCE::METHOD_PLAIN ); @@ -240,6 +250,11 @@ public function test_exchange_token_with_correct_plain_verifier() { $this->assertEquals( 200, $response->get_status() ); } + /** + * RFC 7636 section 4.6: a verifier that does not match the stored challenge gets invalid_grant. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.6 + */ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -256,6 +271,7 @@ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $response = $this->server->dispatch( $request ); $this->assertEquals( 400, $response->get_status() ); + $this->assertSame( 'invalid_grant', $response->get_data()['data']['error'] ); // The code must be consumed, not just rejected once: it's deleted along // with the failed attempt, so re-using it (even with the right verifier @@ -270,6 +286,11 @@ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $this->assertEquals( 404, $retry_response->get_status() ); } + /** + * RFC 7636 section 4.5: the client sends the code_verifier with the token request. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 + */ public function test_exchange_token_missing_verifier_for_pkce_code_fails() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -285,8 +306,14 @@ public function test_exchange_token_missing_verifier_for_pkce_code_fails() { $response = $this->server->dispatch( $request ); $this->assertEquals( 400, $response->get_status() ); + $this->assertSame( 'invalid_grant', $response->get_data()['data']['error'] ); } + /** + * RFC 9700 section 4.8.2: a code_verifier for a code issued without a code_challenge must be rejected. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-4.8.2 + */ public function test_exchange_token_verifier_for_non_pkce_code_fails() { $user = $this->factory->user->create_and_get(); $code = Authorization_Code::create( $this->client, $user ); @@ -299,8 +326,14 @@ public function test_exchange_token_verifier_for_non_pkce_code_fails() { $response = $this->server->dispatch( $request ); $this->assertEquals( 400, $response->get_status() ); + $this->assertSame( 'invalid_grant', $response->get_data()['data']['error'] ); } + /** + * RFC 6749 section 3.2: token requests are POSTed, and a parameter must not be sent more than once. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-3.2 + */ public function test_exchange_token_rejects_array_valued_verifier_via_schema() { $user = $this->factory->user->create_and_get(); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -333,6 +366,55 @@ public function test_exchange_token_non_pkce_code_with_no_verifier_still_works() $this->assertEquals( 200, $response->get_status() ); } + /** + * RFC 6749 section 3.2: token requests are POSTed, so a verifier in the URL query is ignored. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-3.2 + */ + public function test_exchange_token_ignores_verifier_in_the_url_query() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_body_params( [ + 'grant_type' => 'authorization_code', + 'client_id' => $this->client->get_id(), + 'code' => $code->get_code(), + ] ); + $request->set_query_params( [ 'code_verifier' => $pair['code_verifier'] ] ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 400, $response->get_status() ); + $this->assertSame( 'oauth2.tokens.authorization_code.validate.missing_verifier', $response->get_data()['code'] ); + } + + public function test_exchange_token_accepts_verifier_in_a_json_body() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_header( 'Content-Type', 'application/json' ); + $request->set_body( wp_json_encode( [ + 'grant_type' => 'authorization_code', + 'client_id' => $this->client->get_id(), + 'code' => $code->get_code(), + 'code_verifier' => $pair['code_verifier'], + ] ) ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 200, $response->get_status() ); + } + // ------------------------------------------------------------------------- // Client credentials grant // ------------------------------------------------------------------------- diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php index 104eb31..3d2e9df 100644 --- a/tests/test-types-authorization-code.php +++ b/tests/test-types-authorization-code.php @@ -73,6 +73,11 @@ public function test_no_challenge_returns_empty_array() { $this->assertSame( [], $this->type->validate_extra_params_public( $this->client, [] ) ); } + /** + * RFC 7636 section 4.4: the server stores the challenge and method with the authorization code. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4 + */ public function test_valid_s256_challenge_returns_both_keys() { $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); $result = $this->type->validate_extra_params_public( @@ -87,6 +92,11 @@ public function test_valid_s256_challenge_returns_both_keys() { $this->assertSame( 'S256', $result['code_challenge_method'] ); } + /** + * RFC 7636 section 4.4.1: an unsupported transform method gets an invalid_request error. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 + */ public function test_unsupported_method_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -100,6 +110,11 @@ public function test_unsupported_method_is_rejected() { $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } + /** + * RFC 7636 section 4.4.1: an unsupported transform method gets an invalid_request error. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 + */ public function test_unsupported_method_error_names_the_filtered_supported_methods() { $filter = function () { return [ PKCE::METHOD_PLAIN ]; @@ -122,6 +137,11 @@ public function test_unsupported_method_error_names_the_filtered_supported_metho $this->assertStringNotContainsString( 'S256', $result->get_error_message() ); } + /** + * RFC 7636 section 4.3: the method is "S256" or "plain", and defaults to "plain" when omitted. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 + */ public function test_wrongly_cased_method_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -134,6 +154,11 @@ public function test_wrongly_cased_method_is_rejected() { $this->assertWPError( $result ); } + /** + * RFC 6749 section 4.1.2.1: an invalid, repeated or malformed parameter gets an invalid_request redirect. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_malformed_challenge_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -146,6 +171,11 @@ public function test_malformed_challenge_is_rejected() { $this->assertWPError( $result ); } + /** + * RFC 6749 section 4.1.2.1: an invalid, repeated or malformed parameter gets an invalid_request redirect. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_method_without_challenge_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -156,6 +186,11 @@ public function test_method_without_challenge_is_rejected() { $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } + /** + * RFC 7636 section 4.3: the method is "S256" or "plain", and defaults to "plain" when omitted. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 + */ public function test_omitted_method_normalizes_to_plain() { $verifier = PKCE::generate_verifier(); $result = $this->type->validate_extra_params_public( @@ -166,6 +201,11 @@ public function test_omitted_method_normalizes_to_plain() { $this->assertSame( 'plain', $result['code_challenge_method'] ); } + /** + * RFC 6749 section 4.1.2.1: an invalid, repeated or malformed parameter gets an invalid_request redirect. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_array_valued_challenge_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -176,6 +216,11 @@ public function test_array_valued_challenge_is_rejected() { $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } + /** + * RFC 6749 section 4.1.2.1: an invalid, repeated or malformed parameter gets an invalid_request redirect. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_array_valued_method_is_rejected() { $result = $this->type->validate_extra_params_public( $this->client, @@ -189,6 +234,11 @@ public function test_array_valued_method_is_rejected() { $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); } + /** + * RFC 7636 section 4.4.1: a server that requires PKCE returns invalid_request when the challenge is missing. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1 + */ public function test_pkce_required_client_without_challenge_is_rejected() { $client = $this->create_client( [ 'pkce_required' => true ] ); $result = $this->type->validate_extra_params_public( $client, [] ); @@ -197,6 +247,11 @@ public function test_pkce_required_client_without_challenge_is_rejected() { $this->assertEquals( 'oauth2.types.authorization_code.check_pkce_requirement.pkce_required', $result->get_error_code() ); } + /** + * RFC 9700 section 2.1.1: S256 is the method to use, since plain does not protect the challenge. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-2.1.1 + */ public function test_pkce_required_client_with_plain_is_rejected() { $client = $this->create_client( [ 'pkce_required' => true ] ); $verifier = PKCE::generate_verifier(); @@ -212,6 +267,11 @@ public function test_pkce_required_client_with_plain_is_rejected() { $this->assertEquals( 'oauth2.types.authorization_code.check_pkce_requirement.weak_method', $result->get_error_code() ); } + /** + * RFC 9700 section 2.1.1: S256 is the method to use, since plain does not protect the challenge. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-2.1.1 + */ public function test_weak_method_error_names_the_filtered_required_methods() { $filter = function () { return [ PKCE::METHOD_PLAIN ]; @@ -235,6 +295,11 @@ public function test_weak_method_error_names_the_filtered_required_methods() { $this->assertStringNotContainsString( 'S256', $result->get_error_message() ); } + /** + * RFC 7636 section 4.3: the method is "S256" or "plain", and defaults to "plain" when omitted. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 + */ public function test_pkce_required_client_with_s256_is_accepted() { $client = $this->create_client( [ 'pkce_required' => true ] ); $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); @@ -249,22 +314,42 @@ public function test_pkce_required_client_with_s256_is_accepted() { $this->assertIsArray( $result ); } + /** + * RFC 6749 section 4.1.2.1: the error redirect goes to the client and carries the exact state it sent. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_error_redirect_url_uses_query_string() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); $this->assertStringContainsString( 'https://example.com/callback?', $url ); $this->assertStringContainsString( 'error=invalid_request', $url ); } + /** + * RFC 6749 section 4.1.2.1: the error redirect goes to the client and carries the exact state it sent. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_error_redirect_url_omits_state_when_absent() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); $this->assertStringNotContainsString( 'state=', $url ); } + /** + * RFC 6749 section 4.1.2.1: the error redirect goes to the client and carries the exact state it sent. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_error_redirect_url_includes_state_when_present() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', 'xyz' ); $this->assertStringContainsString( 'state=xyz', $url ); } + /** + * RFC 6749 section 4.1.2.1: the error redirect goes to the client and carries the exact state it sent. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ public function test_error_redirect_url_keeps_zero_state() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', '0' ); $this->assertStringContainsString( 'state=0', $url ); @@ -281,6 +366,8 @@ public function test_error_redirect_url_keeps_zero_state() { * both funnel through the 'wp_redirect' filter, so hooking it captures * the final location actually sent, whichever function was used, and * lets the test escape before handle_authorisation()'s exit. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 */ public function test_pkce_error_redirects_to_client_callback_not_wp_admin() { $client = $this->create_client( [ 'pkce_required' => true ] ); @@ -308,6 +395,95 @@ function ( $location ) use ( &$captured_location ) { $this->assertStringStartsWith( 'https://example.com/callback', $captured_location ); $this->assertStringContainsString( 'error=invalid_request', $captured_location ); } + + public function test_required_methods_filter_can_allow_plain() { + add_filter( 'oauth2.pkce.required_methods', function () { + return [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ]; + } ); + + $client = $this->create_client( [ 'pkce_required' => true ] ); + $verifier = PKCE::generate_verifier(); + $result = $this->type->validate_extra_params_public( + $client, + [ + 'code_challenge' => $verifier, + 'code_challenge_method' => 'plain', + ] + ); + + $this->assertSame( 'plain', $result['code_challenge_method'] ); + } + + /** + * RFC 6749 section 4.1.2.1: with an invalid redirect URI, the server must not redirect. + * + * A PKCE error on a request whose redirect_uri is not registered must not + * send the user, or the error, to that URI. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1 + */ + public function test_pkce_error_is_not_redirected_to_an_unregistered_uri() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $_GET = [ + 'client_id' => $client->get_id(), + 'redirect_uri' => 'https://attacker.example/callback', + ]; + add_filter( 'wp_redirect', function () { + throw new Redirect_Interrupt(); + } ); + + $result = ( new Authorization_Code() )->handle_authorisation(); + $_GET = []; + + $this->assertWPError( $result ); + $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.invalid_redirect_uri', $result->get_error_code() ); + } + + /** + * RFC 7636 section 4.4: the server stores the challenge and method with the authorization code. + * + * Runs the whole consent flow, so the challenge has to survive from the + * authorize request, through the consent form post, into the minted code. + * + * @link https://datatracker.ietf.org/doc/html/rfc7636#section-4.4 + */ + public function test_consent_flow_mints_a_code_bound_to_the_challenge() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + wp_set_current_user( $this->factory->user->create() ); + + $_GET = [ + 'client_id' => $client->get_id(), + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ]; + $_POST = [ + '_wpnonce' => wp_create_nonce( sprintf( 'oauth2_authorize:%s', $client->get_id() ) ), + 'wp-submit' => 'authorize', + ]; + + $location = null; + add_filter( 'wp_redirect', function ( $url ) use ( &$location ) { + $location = $url; + throw new Redirect_Interrupt(); + } ); + + try { + ( new Authorization_Code() )->handle_authorisation(); + $this->fail( 'Expected a redirect back to the client.' ); + } catch ( Redirect_Interrupt $e ) { + // Expected. + } finally { + $_GET = []; + $_POST = []; + } + + parse_str( wp_parse_url( $location, PHP_URL_QUERY ), $args ); + $code = $client->get_authorization_code( $args['code'] ); + + $this->assertSame( $pair['code_challenge'], $code->get_code_challenge() ); + $this->assertSame( 'S256', $code->get_code_challenge_method() ); + } } /** @@ -335,6 +511,11 @@ public function test_non_pkce_client_is_allowed() { $this->assertSame( [], $this->type->validate_extra_params_public( $this->client, [] ) ); } + /** + * RFC 9700 section 2.1.2: the implicit grant cannot bind a code_challenge, so PKCE clients must use the code grant. + * + * @link https://datatracker.ietf.org/doc/html/rfc9700#section-2.1.2 + */ public function test_pkce_required_client_is_refused() { $client = $this->create_client( [ 'pkce_required' => true ] ); $result = $this->type->validate_extra_params_public( $client, [] ); @@ -343,12 +524,22 @@ public function test_pkce_required_client_is_refused() { $this->assertEquals( 'oauth2.types.implicit.validate_extra_params.pkce_required', $result->get_error_code() ); } + /** + * RFC 6749 section 4.2.2.1: implicit grant errors go in the redirect URI fragment. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 + */ public function test_error_redirect_url_uses_fragment() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'Nope.' ); $this->assertStringContainsString( '#', $url ); $this->assertStringContainsString( 'error=unauthorized_client', $url ); } + /** + * RFC 6749 section 4.2.2.1: implicit grant errors go in the redirect URI fragment. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 + */ public function test_error_redirect_url_encodes_fragment_values() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'A & B #c', 'x&y=z' ); @@ -357,6 +548,11 @@ public function test_error_redirect_url_encodes_fragment_values() { $this->assertSame( 'x&y=z', $args['state'] ); } + /** + * RFC 6749 section 4.2.2.1: implicit grant errors go in the redirect URI fragment, with the exact state. + * + * @link https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1 + */ public function test_error_redirect_url_keeps_zero_state() { $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'Nope.', '0' ); $this->assertStringContainsString( 'state=0', $url ); diff --git a/tests/test-well-known.php b/tests/test-well-known.php index 4cc0c54..154febc 100644 --- a/tests/test-well-known.php +++ b/tests/test-well-known.php @@ -107,11 +107,31 @@ public function test_metadata_advertises_the_token_endpoint() { $this->assertStringContainsString( 'oauth2/access_token', $metadata['token_endpoint'] ); } + /** + * RFC 8414 section 2: the metadata lists code_challenge_methods_supported. + * + * @link https://datatracker.ietf.org/doc/html/rfc8414#section-2 + */ public function test_metadata_advertises_the_pkce_challenge_methods() { $metadata = get_authorization_server_metadata(); $this->assertContains( 'S256', $metadata['code_challenge_methods_supported'] ); } + /** + * RFC 8414 section 2: the metadata lists code_challenge_methods_supported. + * + * @link https://datatracker.ietf.org/doc/html/rfc8414#section-2 + */ + public function test_metadata_pkce_methods_follow_the_supported_methods_filter() { + add_filter( 'oauth2.pkce.supported_methods', function () { + return [ 'S256' ]; + } ); + + $metadata = get_authorization_server_metadata(); + + $this->assertSame( [ 'S256' ], $metadata['code_challenge_methods_supported'] ); + } + public function test_metadata_is_filterable() { add_filter( 'oauth2.well_known_authorization_server_metadata', function ( $metadata ) { $metadata['service_documentation'] = 'https://example.org/docs'; From 384e2f1c2fe87416ca73ca91d3195f95722acd42 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 7 Oct 2026 14:23:47 +0100 Subject: [PATCH 18/19] Map PKCE token errors through the RFC 6749 error format Upstream #89 added a token endpoint filter that turns WP_Error responses into RFC 6749 section 5.2 errors. It uses the `error` key in the error data when present, otherwise a fixed map, and falls back to `invalid_request` for any other 4xx. The PKCE verifier errors already set `error => invalid_grant`, so they come through unchanged. The fail-closed error for a stored challenge with no stored method did not, so after the merge it would have surfaced as `invalid_request`. The problem is the authorization code, not the request, so it now sets `invalid_grant`, matching how #89 maps the other corrupted-code error (`get_user.invalid_data`). The array-valued verifier test only checked for a 400. With #89 in place the schema rejection becomes a proper `invalid_request` OAuth error, so the test now asserts that with the shared helper. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/tokens/class-authorization-code.php | 5 ++++- tests/test-authorization-code.php | 1 + tests/test-token-endpoint.php | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index c2ed3a6..0c3262e 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -221,7 +221,10 @@ protected function validate_code_verifier( $verifier ) { return new WP_Error( 'oauth2.tokens.authorization_code.validate.missing_challenge_method', __( 'Authorization code data is not valid.', 'oauth2' ), - [ 'status' => WP_Http::BAD_REQUEST ] + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] ); } diff --git a/tests/test-authorization-code.php b/tests/test-authorization-code.php index 3a8583a..3b8f3ac 100644 --- a/tests/test-authorization-code.php +++ b/tests/test-authorization-code.php @@ -330,6 +330,7 @@ public function test_validate_fails_closed_when_stored_method_is_missing() { $result = $code->validate( static::RFC_VERIFIER ); $this->assertWPError( $result ); $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_challenge_method', $result->get_error_code() ); + $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); } public function test_validate_with_malformed_meta_does_not_pass_expiry_check() { diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index 4a11ad3..31c689a 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -351,7 +351,7 @@ public function test_exchange_token_rejects_array_valued_verifier_via_schema() { $request->set_param( 'code_verifier', [ 'x' ] ); $response = $this->server->dispatch( $request ); - $this->assertEquals( 400, $response->get_status() ); + $this->assertOAuthError( $response, 'invalid_request', 400 ); } public function test_exchange_token_non_pkce_code_with_no_verifier_still_works() { From f46c09e4e225fc576209e181fe65ee54fd0860d7 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 7 Oct 2026 14:27:13 +0100 Subject: [PATCH 19/19] Expect a 400 when a burned PKCE code is reused Upstream #89 sends every token endpoint error except a 401 invalid_client as a 400, as RFC 6749 section 5.2 requires. Reusing an unknown code used to return 404, so the burn test's retry assertion broke after the merge and failed across the whole CI matrix. The retry exists to prove the code was deleted, so it fails as an unknown code rather than as a bad verifier. The status no longer tells those apart, so the test now checks the invalid_grant OAuth error and the unknown-code WP_Error code. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test-token-endpoint.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index 31c689a..e640b52 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -285,7 +285,8 @@ public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { $retry->set_param( 'code_verifier', $pair['code_verifier'] ); $retry_response = $this->server->dispatch( $retry ); - $this->assertEquals( 404, $retry_response->get_status() ); + $this->assertOAuthError( $retry_response, 'invalid_grant', 400 ); + $this->assertSame( 'oauth2.client.check_authorization_code.invalid_code', $retry_response->get_data()['code'] ); } /**