From e1cfd575cd7d0f062acdf4fd4cb08a07d08bade4 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Thu, 17 Sep 2026 15:00:17 +0100 Subject: [PATCH 1/3] Declare the client authentication contract on ClientInterface The token endpoint calls check_secret() and is_client_credentials_enabled() on whatever get_client() returns, but neither method was on ClientInterface and PersonalClient implemented neither. A POST to /oauth2/access_token with grant_type=client_credentials and client_id=__personal_access_token reached handle_client_credentials(), called an undefined method on the singleton and fataled. That is an unauthenticated 500 on a public endpoint. Put both methods on the interface, along with requires_secret() which the authorization_code grant needs next, so the endpoint can rely on them. Give PersonalClient implementations that all return false: it has no secret, it is issued to a user rather than a client, and it must never authenticate as one. Adding methods to a published interface breaks any third-party class that implements it. That is the lesser evil here, since such a class already fatals on this path; failing at declaration time is louder but honest. Co-Authored-By: Claude Opus 5 --- inc/class-clientinterface.php | 25 +++++++++++++++++++++ inc/class-personalclient.php | 29 +++++++++++++++++++++++++ tests/test-personalclient.php | 41 +++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 tests/test-personalclient.php diff --git a/inc/class-clientinterface.php b/inc/class-clientinterface.php index 9b42081..b492b97 100644 --- a/inc/class-clientinterface.php +++ b/inc/class-clientinterface.php @@ -47,6 +47,31 @@ public function get_type(); */ public function get_secret(); + /** + * Check whether the client must authenticate with its secret. + * + * @link https://tools.ietf.org/html/rfc6749#section-3.2.1 + * + * @return bool True if the secret must be verified, false otherwise. + */ + public function requires_secret(); + + /** + * Check if the provided secret matches the client's secret. + * + * @param string $secret Secret to check. + * + * @return bool True if the secret matches, false otherwise. + */ + public function check_secret( $secret ); + + /** + * Check whether the client_credentials grant is enabled for this client. + * + * @return bool True if enabled, false otherwise. + */ + public function is_client_credentials_enabled(); + /** * Get registered URI for the client. * diff --git a/inc/class-personalclient.php b/inc/class-personalclient.php index 6105983..694a13d 100644 --- a/inc/class-personalclient.php +++ b/inc/class-personalclient.php @@ -93,6 +93,35 @@ public function get_secret() { return ''; } + /** + * Check whether the client must authenticate with its secret. + * + * @return bool Always false: personal tokens have no secret to check. + */ + public function requires_secret() { + return false; + } + + /** + * Check if the provided secret matches the client's secret. + * + * @param string $secret Secret to check. + * + * @return bool Always false: personal tokens cannot authenticate as a client. + */ + public function check_secret( $secret ) { + return false; + } + + /** + * Check whether the client_credentials grant is enabled for this client. + * + * @return bool Always false: personal tokens are issued to a user, not a client. + */ + public function is_client_credentials_enabled() { + return false; + } + /** * Get registered URI for the client. * diff --git a/tests/test-personalclient.php b/tests/test-personalclient.php new file mode 100644 index 0000000..5431fc5 --- /dev/null +++ b/tests/test-personalclient.php @@ -0,0 +1,41 @@ +client = PersonalClient::get_instance(); + } + + public function test_requires_secret_is_false() { + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_check_secret_is_false_for_any_value() { + $this->assertFalse( $this->client->check_secret( '' ) ); + $this->assertFalse( $this->client->check_secret( 'anything' ) ); + } + + public function test_client_credentials_grant_is_disabled() { + $this->assertFalse( $this->client->is_client_credentials_enabled() ); + } +} From ca0ba9b472e780d01a93eb3c43c73ec14ce0930f Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Thu, 17 Sep 2026 15:00:27 +0100 Subject: [PATCH 2/3] Add Client::requires_secret() for confidential clients The admin form has always asked whether a client is Private or Public, and described Private with the RFC 6749 section 2.1 wording for a confidential client. Nothing ever read the answer back: get_type() had exactly one caller, the code that re-populates that same form. Expose the answer as requires_secret(), taken from rmccue's WIP in #36. Only an explicit 'private' counts. Clients created before the meta existed return '', and the test suite's own helper stores 'web', so keying off anything looser would lock out installs that never made the choice. Make it filterable on 'oauth2.client.requires_secret' so a site can demand authentication from clients the admin never marked Private, or exempt one client while an integration is updated. Enforcing this is a breaking change for anyone whose Private client never sent its secret, and the escape hatch should land with it rather than after the first support ticket. Co-Authored-By: Claude Opus 5 --- inc/class-client.php | 22 ++++++++++++++++++++++ tests/test-client.php | 27 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/inc/class-client.php b/inc/class-client.php index bc9df85..e9a8303 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -122,6 +122,28 @@ public function get_secret() { return get_post_meta( $this->get_post_id(), static::CLIENT_SECRET_KEY, true ); } + /** + * Check whether the client must authenticate with its secret. + * + * Clients declared as private are confidential clients in RFC 6749 terms, + * so they have to prove they hold the secret they were issued. + * + * @link https://tools.ietf.org/html/rfc6749#section-3.2.1 + * + * @return bool True if the secret must be verified, false otherwise. + */ + public function requires_secret() { + $requires_secret = ( 'private' === $this->get_type() ); + + /** + * Filter whether a client must authenticate with its secret. + * + * @param bool $requires_secret Whether the secret must be verified. + * @param Client $client Client being checked. + */ + return (bool) apply_filters( 'oauth2.client.requires_secret', $requires_secret, $this ); + } + /** * Check if the provided secret matches the client's secret. * diff --git a/tests/test-client.php b/tests/test-client.php index f63fb51..43d91a7 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -153,6 +153,33 @@ public function test_check_secret_false_for_wrong_secret() { $this->assertFalse( $this->client->check_secret( 'wrongsecret' ) ); } + public function test_requires_secret_true_for_private_client() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $this->assertTrue( $client->requires_secret() ); + } + + public function test_requires_secret_false_for_public_client() { + $client = $this->create_client( [ 'type' => 'public' ] ); + $this->assertFalse( $client->requires_secret() ); + } + + public function test_requires_secret_false_for_other_type() { + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_requires_secret_false_without_a_stored_type() { + delete_post_meta( $this->client->get_post_id(), Client::TYPE_KEY ); + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_requires_secret_can_be_filtered() { + add_filter( 'oauth2.client.requires_secret', '__return_true' ); + $requires = $this->client->requires_secret(); + remove_filter( 'oauth2.client.requires_secret', '__return_true' ); + + $this->assertTrue( $requires ); + } + public function test_update_changes_name() { $updated = $this->client->update( [ 'name' => 'Updated Name', From 12bfb41758b5746d8fbb57f02ad9972cafd6c4a3 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Thu, 17 Sep 2026 15:00:39 +0100 Subject: [PATCH 3/3] Authenticate confidential clients on the authorization_code grant The grant resolved client_id and code, then went straight to validating the code and issuing a token. A client_secret sent in the body was accepted as a request parameter and never read. Anyone holding a leaked or intercepted code could redeem it without the secret, which is what RFC 6749 section 4.1.3 requires the server to prevent for a confidential client. Check the secret when the client requires one, before the code is looked up, so a failed attempt cannot consume or probe a code. Public clients still exchange codes with no secret, which section 4.1.3 also allows, so this only bites clients an admin explicitly marked Private. A Private client whose stored secret is empty now always fails. That is a broken registration rather than a supported configuration, and failing it closed beats hash_equals( '', '' ) returning true for an empty submission. Reuse the error the client_credentials grant already returns rather than adding a third failure shape: one invalid_client code, 401, and no hint about which half of the credentials was wrong. Both paths now also send WWW-Authenticate when the client authenticated with the Authorization header, per section 5.2. WP_Error cannot carry a header, so those responses go through rest_convert_error_to_response() to get one. Finally, read the Basic header for a secret even when client_id arrived in the body, provided the header names the same client. Before, the header was only consulted when client_id was absent, so a client splitting its credentials across the two would have been rejected for sending no secret at all. Co-Authored-By: Claude Opus 5 --- inc/endpoints/class-token.php | 101 +++++++++++++--- tests/test-token-endpoint.php | 211 ++++++++++++++++++++++++++++++++++ 2 files changed, 293 insertions(+), 19 deletions(-) diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index b853c75..647f068 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -11,6 +11,7 @@ use WP_Http; use WP\OAuth2; use WP_REST_Request; +use WP_REST_Response; /** * Token endpoint handler. */ @@ -74,16 +75,19 @@ public function exchange_token( WP_REST_Request $request ) { // RFC 6749 section 2.3.1: a client may authenticate with HTTP Basic // instead of body parameters. Body parameters take precedence. - if ( $request->get_param( 'client_id' ) === null || $request->get_param( 'client_id' ) === '' ) { - $basic = $this->get_basic_auth_credentials( $request ); - if ( is_wp_error( $basic ) ) { - return $basic; - } - if ( null !== $basic ) { + $basic = $this->get_basic_auth_credentials( $request ); + if ( is_wp_error( $basic ) ) { + return $basic; + } + if ( null !== $basic ) { + if ( $this->is_param_empty( $request, 'client_id' ) ) { $request->set_param( 'client_id', $basic[0] ); - if ( $request->get_param( 'client_secret' ) === null || $request->get_param( 'client_secret' ) === '' ) { - $request->set_param( 'client_secret', $basic[1] ); - } + } + + // Only accept the header secret for the client it names, so a body + // client_id can still be paired with a Basic secret. + if ( $this->is_param_empty( $request, 'client_secret' ) && $basic[0] === $request->get_param( 'client_id' ) ) { + $request->set_param( 'client_secret', $basic[1] ); } } @@ -93,7 +97,7 @@ public function exchange_token( WP_REST_Request $request ) { // shape matches what WP REST API would produce at the schema layer. $missing = []; foreach ( [ 'client_id', 'code' ] as $required_param ) { - if ( $request->get_param( $required_param ) === null || $request->get_param( $required_param ) === '' ) { + if ( $this->is_param_empty( $request, $required_param ) ) { $missing[] = $required_param; } } @@ -122,6 +126,15 @@ public function exchange_token( WP_REST_Request $request ) { ); } + // RFC 6749 section 4.1.3: the server must authenticate the client when + // the client is confidential. Public clients have no secret to check. + if ( $client->requires_secret() ) { + $client_secret = (string) $request->get_param( 'client_secret' ); + if ( '' === $client_secret || ! $client->check_secret( $client_secret ) ) { + return $this->client_authentication_failed( $request ); + } + } + $auth_code = $client->get_authorization_code( $request['code'] ); if ( is_wp_error( $auth_code ) ) { return $auth_code; @@ -182,11 +195,7 @@ private function handle_client_credentials( WP_REST_Request $request ) { $grant_ok = $client && $client->is_client_credentials_enabled(); if ( ! $creds_ok || ! $grant_ok ) { - return new WP_Error( - 'oauth2.endpoints.token.invalid_client', - __( 'Client authentication failed.', 'oauth2' ), - [ 'status' => WP_Http::UNAUTHORIZED ] - ); + return $this->client_authentication_failed( $request ); } $token = OAuth2\Tokens\Access_Token::create_for_client( $client ); @@ -228,6 +237,62 @@ private function extract_client_credentials( WP_REST_Request $request ) { ); } + /** + * Check whether a request parameter is missing or empty. + * + * @param WP_REST_Request $request Request object. + * @param string $param Parameter name. + * + * @return bool True if the parameter has no usable value. + */ + private function is_param_empty( WP_REST_Request $request, $param ) { + $value = $request->get_param( $param ); + + return null === $value || '' === $value; + } + + /** + * Build the response for a failed client authentication. + * + * The reason is never given: telling "unknown client" apart from "wrong + * secret" would confirm a valid client ID and secret pair. + * + * @param WP_REST_Request $request Request object. + * + * @return WP_Error|WP_REST_Response Error, or a response carrying a Basic challenge. + */ + private function client_authentication_failed( WP_REST_Request $request ) { + $error = new WP_Error( + 'oauth2.endpoints.token.invalid_client', + __( 'Client authentication failed.', 'oauth2' ), + [ 'status' => WP_Http::UNAUTHORIZED ] + ); + + if ( ! $this->has_basic_auth_header( $request ) ) { + return $error; + } + + // RFC 6749 section 5.2: a client that authenticated with the + // Authorization header must get a matching challenge back. + $response = rest_convert_error_to_response( $error ); + $response->header( 'WWW-Authenticate', 'Basic realm="OAuth2 token endpoint"' ); + + return $response; + } + + /** + * Check whether the request carries an HTTP Basic Authorization header. + * + * @param WP_REST_Request $request Request object. + * + * @return bool True if a Basic header is present. + */ + private function has_basic_auth_header( WP_REST_Request $request ) { + $auth_header = $request->get_header( 'authorization' ); + + return ! empty( $auth_header ) && stripos( $auth_header, 'Basic ' ) === 0; + } + /** * Read client credentials from an HTTP Basic Authorization header. * @@ -236,13 +301,11 @@ private function extract_client_credentials( WP_REST_Request $request ) { * header is malformed, or null if there is no Basic header. */ private function get_basic_auth_credentials( WP_REST_Request $request ) { - $auth_header = $request->get_header( 'authorization' ); - - if ( empty( $auth_header ) || stripos( $auth_header, 'Basic ' ) !== 0 ) { + if ( ! $this->has_basic_auth_header( $request ) ) { return null; } - $encoded = substr( $auth_header, 6 ); + $encoded = substr( $request->get_header( 'authorization' ), 6 ); $decoded = base64_decode( $encoded, true ); if ( false === $decoded ) { diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index d7c7180..7d00723 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\PersonalClient; use WP\OAuth2\Tokens\Authorization_Code; use WP_REST_Request; use WP_REST_Server; @@ -340,4 +341,214 @@ public function test_validate_grant_type_rejects_unknown() { $this->assertFalse( $handler->validate_grant_type( 'implicit' ) ); $this->assertFalse( $handler->validate_grant_type( '' ) ); } + + // ------------------------------------------------------------------------- + // Confidential client authentication + // ------------------------------------------------------------------------- + + /** + * Request an access token for a client, with an optional secret. + * + * @param Client $client Client to exchange a fresh code for. + * @param array $params Extra request parameters. + * @param string $basic Raw value for an Authorization header, if any. + * + * @return \WP_REST_Response Dispatched response. + */ + protected function request_token( Client $client, array $params = [], $basic = '' ) { + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'code', $code->get_code() ); + foreach ( $params as $key => $value ) { + $request->set_param( $key, $value ); + } + if ( '' !== $basic ) { + $request->add_header( 'Authorization', 'Basic ' . $basic ); + } + + return $this->server->dispatch( $request ); + } + + public function test_private_client_without_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 401, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.endpoints.token.invalid_client', $data['code'] ); + } + + public function test_private_client_with_correct_secret_is_accepted() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => $client->get_secret(), + ] + ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_private_client_with_wrong_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => 'wrong-secret', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_private_client_secret_via_basic_auth_header() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':' . $client->get_secret() ); + + $response = $this->request_token( $client, [], $encoded ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_private_client_basic_secret_pairs_with_body_client_id() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':' . $client->get_secret() ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ], $encoded ); + + $this->assertEquals( 200, $response->get_status() ); + } + + public function test_private_client_basic_secret_ignored_for_another_client() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $other = $this->create_client( [ 'type' => 'private' ], 'Other Client' ); + $encoded = base64_encode( $other->get_id() . ':' . $other->get_secret() ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ], $encoded ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_private_client_with_empty_stored_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + update_post_meta( $client->get_post_id(), Client::CLIENT_SECRET_KEY, '' ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => '', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_failed_client_authentication_keeps_the_code() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $this->server->dispatch( $request ); + + $stored = Authorization_Code::get_by_code( $client, $code->get_code() ); + $this->assertNotWPError( $stored ); + } + + public function test_failed_basic_authentication_sends_a_challenge() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':wrong-secret' ); + + $response = $this->request_token( $client, [], $encoded ); + + $this->assertEquals( 401, $response->get_status() ); + $headers = $response->get_headers(); + $this->assertArrayHasKey( 'WWW-Authenticate', $headers ); + $this->assertStringStartsWith( 'Basic realm=', $headers['WWW-Authenticate'] ); + } + + public function test_failed_body_authentication_sends_no_challenge() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => 'wrong-secret', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + $this->assertArrayNotHasKey( 'WWW-Authenticate', $response->get_headers() ); + } + + public function test_public_client_without_secret_is_accepted() { + $client = $this->create_client( [ 'type' => 'public' ] ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_client_without_a_stored_type_is_accepted() { + $client = $this->create_client(); + delete_post_meta( $client->get_post_id(), Client::TYPE_KEY ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_requires_secret_filter_can_force_authentication() { + $client = $this->create_client( [ 'type' => 'public' ] ); + add_filter( 'oauth2.client.requires_secret', '__return_true' ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + remove_filter( 'oauth2.client.requires_secret', '__return_true' ); + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_personal_client_cannot_use_the_authorization_code_grant() { + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', PersonalClient::ID ); + $request->set_param( 'code', 'anycode' ); + + $response = $this->server->dispatch( $request ); + + $this->assertNotEquals( 200, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.personalclient.no_auth_code', $data['code'] ); + } + + public function test_personal_client_cannot_use_the_client_credentials_grant() { + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'client_credentials' ); + $request->set_param( 'client_id', PersonalClient::ID ); + $request->set_param( 'client_secret', 'any-secret' ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 401, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.endpoints.token.invalid_client', $data['code'] ); + } }