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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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() {