From dfe94ed03d823d5e540de1dbf70905f2c1394d08 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 10:58:19 +0000 Subject: [PATCH 01/11] Return a 403 from authorize when the app is not yet approved Unapproved clients are saved as drafts, and get_by_id() only finds published posts. An unapproved app therefore got the same "Client ID is invalid" 400 as a typo, which gave the developer no hint that approval was the missing step. Add Client::get_unapproved_by_id(), matching draft and pending posts only, never trash. Both lookups now share one private query helper, and get_by_id() behaves as before. Base::handle_authorisation() uses the new lookup to return a distinct unapproved_client error with a 403 status. Unknown client IDs still get invalid_client_id. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/class-client.php | 27 ++++++++++++++++++++++++++- inc/types/class-base.php | 10 ++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/inc/class-client.php b/inc/class-client.php index 093a305..d24291a 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -315,9 +315,34 @@ public function issue_token( WP_User $user, $meta = [] ) { * @return static|null Token if ID is found, null otherwise. */ public static function get_by_id( $id ) { + return static::get_by_slug_and_status( $id, [ 'publish' ] ); + } + + /** + * Get an unapproved client by ID. + * + * Only draft and pending clients match, trashed clients never do. + * + * @param string $id Client ID. + * + * @return static|null Client if an unapproved client is found, null otherwise. + */ + public static function get_unapproved_by_id( $id ) { + return static::get_by_slug_and_status( $id, [ 'draft', 'pending' ] ); + } + + /** + * Get a client by ID, limited to the given post statuses. + * + * @param string $id Client ID. + * @param string[] $statuses Post statuses to match. + * + * @return static|null Client if found, null otherwise. + */ + private static function get_by_slug_and_status( $id, array $statuses ) { $args = [ 'post_type' => static::POST_TYPE, - 'post_status' => 'publish', + 'post_status' => $statuses, 'posts_per_page' => 1, 'no_found_rows' => true, diff --git a/inc/types/class-base.php b/inc/types/class-base.php index a35cf13..fe20186 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -48,6 +48,16 @@ public function handle_authorisation() { $state = isset( $_GET['state'] ) && is_string( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized $client = Client::get_by_id( $client_id ); + if ( empty( $client ) && Client::get_unapproved_by_id( $client_id ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', + __( 'This application is not currently approved. Ask the site administrator to approve it.', 'oauth2' ), + [ + 'status' => WP_Http::FORBIDDEN, + 'client_id' => $client_id, + ] + ); + } if ( empty( $client ) ) { return new WP_Error( 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', From d4cc4772d04040a6f3e928606a57ed9756335f6b Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 10:58:19 +0000 Subject: [PATCH 02/11] Render authorize errors on the login screen Every error from the authorize endpoint went to wp_die(), which sends HTTP 500 and a bare message, and was marked as a TODO. The error status (such as 400 or 403) was ignored. Render the error with login_header() and login_footer() instead, using the status from the error data and defaulting to 400. This code runs inside wp-login.php's login_form_* hook, so the login helpers are loaded. A link back to the site home gives the user somewhere to go. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/endpoints/class-authorization.php | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/inc/endpoints/class-authorization.php b/inc/endpoints/class-authorization.php index ee031df..8c16587 100644 --- a/inc/endpoints/class-authorization.php +++ b/inc/endpoints/class-authorization.php @@ -51,9 +51,29 @@ public function handle_request() { } if ( is_wp_error( $result ) ) { - // TODO: Handle it. - wp_die( esc_html( $result->get_error_message() ) ); + $this->render_error( $result ); } exit; } + + /** + * Render an authorization error on the login screen, then exit. + * + * @param WP_Error $error Error to display. The `status` error data sets the HTTP status. + */ + protected function render_error( WP_Error $error ) { + $data = $error->get_error_data(); + $status = is_array( $data ) && ! empty( $data['status'] ) ? (int) $data['status'] : 400; + + status_header( $status ); + login_header( __( 'Authorization error', 'oauth2' ), '', $error ); + printf( + '

%s

', + esc_url( home_url( '/' ) ), + /* translators: %s: site name */ + esc_html( sprintf( __( 'Go to %s', 'oauth2' ), get_bloginfo( 'name', 'display' ) ) ) + ); + login_footer(); + exit; + } } From 40238ee1f03f4a1e78233352097a272da33a88bb Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 10:58:51 +0000 Subject: [PATCH 03/11] Show approval status in the applications list Admins could only tell an app was unapproved from the draft state label next to its name. Add an Approved column that prints Yes or No based on whether the post is published. Extract the approve URL into get_approve_url() so the list table and the upcoming edit screen build the same nonce-protected link. Also escape the edit link in column_name(), which was printed raw. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/admin/class-listtable.php | 27 ++++++++++++++++----------- inc/admin/namespace.php | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/inc/admin/class-listtable.php b/inc/admin/class-listtable.php index 83331b9..41b1a71 100644 --- a/inc/admin/class-listtable.php +++ b/inc/admin/class-listtable.php @@ -49,6 +49,7 @@ public function get_columns() { $c = [ 'cb' => '', 'name' => __( 'Name', 'oauth2' ), + 'approved' => __( 'Approved', 'oauth2' ), 'description' => __( 'Description', 'oauth2' ), ]; @@ -104,16 +105,7 @@ protected function column_name( $item ) { $post_type_object = get_post_type_object( $item->post_type ); if ( current_user_can( $post_type_object->cap->publish_posts ) && 'publish' !== $item->post_status ) { - $publish_link = add_query_arg( - [ - 'page' => 'rest-oauth2-apps', - 'action' => 'approve', - 'id' => $item->ID, - ], - admin_url( 'users.php' ) - ); - - $publish_link = wp_nonce_url( $publish_link, 'rest-oauth2-approve:' . $item->ID ); + $publish_link = get_approve_url( $item ); $actions['app-approve'] = sprintf( '%s', esc_url( $publish_link ), @@ -128,7 +120,7 @@ protected function column_name( $item ) { _post_states( $item ); $title = sprintf( '%s%s', - $edit_link, + esc_url( $edit_link ), $title, ob_get_clean() ); @@ -136,6 +128,19 @@ protected function column_name( $item ) { return $title . ' ' . $action_html; } + /** + * Print whether the client is approved. + * + * @param \WP_Post $item Post object. + */ + public function column_approved( $item ) { + if ( 'publish' === $item->post_status ) { + esc_html_e( 'Yes', 'oauth2' ); + } else { + esc_html_e( 'No', 'oauth2' ); + } + } + /** * @param \WP_Post $item Post object. * @return string Content of the column. diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index ea5192b..3fe8b2d 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -9,6 +9,7 @@ use WP\OAuth2\Client; use WP_Error; +use WP_Post; const BASE_SLUG = 'rest-oauth2-apps'; @@ -46,6 +47,26 @@ function get_url( $params = [] ) { return add_query_arg( urlencode_deep( $params ), $url ); } +/** + * Get the URL that approves a client. + * + * @param WP_Post|int $client_post Client post or post ID. + * @param array|string $params Extra query parameters, such as `return=edit`. + * + * @return string Approve URL including the nonce. + */ +function get_approve_url( $client_post, $params = [] ) { + $id = $client_post instanceof WP_Post ? $client_post->ID : absint( $client_post ); + $url = get_url( + [ + 'action' => 'approve', + 'id' => $id, + ] + wp_parse_args( $params ) + ); + + return wp_nonce_url( $url, 'rest-oauth2-approve:' . $id ); +} + /** * Get the current page action. * From b7bff8cab163ffb8186473c772ec142ccfccea81 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 10:59:06 +0000 Subject: [PATCH 04/11] Show approval status and an Approve button on the edit screen An unapproved app looked the same as an approved one on its edit screen, and approving meant going back to the list. Print a success or warning notice above the form for existing clients. Admins who can publish also get an Approve button. Other users are told to ask an administrator. The button adds return=edit to the approve URL. handle_approve() then sends the user back to the edit screen with an "Application approved." message instead of the list. The list's approve action is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/admin/namespace.php | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index 3fe8b2d..431f03b 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -294,6 +294,38 @@ function handle_edit_submit( ?Client $consumer = null ) { exit; } +/** + * Output the approval status notice for a client, with an approve button where allowed. + * + * @param WP_Post|int $client_post Client post or post ID. + */ +function render_approval_status( $client_post ) { + $client_post = get_post( $client_post ); + if ( ! $client_post ) { + return; + } + + $is_approved = 'publish' === $client_post->post_status; + if ( $is_approved ) { + echo '

' . esc_html__( 'This application is approved.', 'oauth2' ) . '

'; + return; + } + + $post_type_object = get_post_type_object( Client::POST_TYPE ); + ?> +
+

+

+ cap->publish_posts ) ) : ?> + + + + +

+
+ + get_post_id() ) ); + } + ?> +
@@ -620,6 +662,19 @@ function handle_approve() { wp_die( $did_approve ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped } + if ( isset( $_GET['return'] ) && 'edit' === $_GET['return'] ) { + wp_safe_redirect( + get_url( + [ + 'action' => 'edit', + 'id' => $id, + 'did_action' => 'approve', + ] + ) + ); + exit; + } + wp_safe_redirect( get_url( 'approved=1' ) ); exit; } From e08e5fa3d6ad2802499c6ff8b89df9d095f19486 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 11:00:09 +0000 Subject: [PATCH 05/11] Test unapproved client lookup, authorize error and approval UI Cover Client::get_unapproved_by_id() for draft, published and trashed clients, the 403 unapproved_client error versus the 400 invalid_client_id error from the authorize step, and the Approved column and approval status notice in the admin. Add a create_unapproved_client() helper to the shared test case. create_client() now builds on it and approves the result. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/class-test-case.php | 18 ++++++- tests/test-admin.php | 88 +++++++++++++++++++++++++++++++++ tests/test-authorize-errors.php | 44 +++++++++++++++++ tests/test-client.php | 19 +++++++ 4 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 tests/test-authorize-errors.php diff --git a/tests/class-test-case.php b/tests/class-test-case.php index 9035d85..f10a749 100644 --- a/tests/class-test-case.php +++ b/tests/class-test-case.php @@ -16,14 +16,14 @@ abstract class Test_Case extends WP_UnitTestCase { /** - * Create and approve a test client. + * Create a test client that is not approved. * * @param array $meta_overrides Optional overrides for the client meta. * @param string $name Client name. * * @return Client */ - protected function create_client( array $meta_overrides = [], $name = 'Test Client' ) { + protected function create_unapproved_client( array $meta_overrides = [], $name = 'Test Client' ) { $data = [ 'name' => $name, 'description' => 'Test client description.', @@ -39,6 +39,20 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie $client = Client::create( $data ); $this->assertInstanceOf( Client::class, $client, 'create_client helper: Client::create() must return a Client instance.' ); + + return $client; + } + + /** + * Create and approve a test client. + * + * @param array $meta_overrides Optional overrides for the client meta. + * @param string $name Client name. + * + * @return Client + */ + protected function create_client( array $meta_overrides = [], $name = 'Test Client' ) { + $client = $this->create_unapproved_client( $meta_overrides, $name ); $client->approve(); return $client; diff --git a/tests/test-admin.php b/tests/test-admin.php index 0e085bb..cc57192 100644 --- a/tests/test-admin.php +++ b/tests/test-admin.php @@ -9,6 +9,8 @@ require_once __DIR__ . '/class-test-case.php'; +use WP\OAuth2\Admin\ListTable; +use function WP\OAuth2\Admin\render_approval_status; use function WP\OAuth2\Admin\validate_parameters; /** @@ -77,4 +79,90 @@ public function test_validate_parameters_rejects_negative_ttl() { $this->assertWPError( $result ); $this->assertEquals( 'rest_oauth2_invalid_ttl', $result->get_error_code() ); } + + /** + * Get a list table instance. + * + * @return ListTable + */ + protected function get_list_table() { + set_current_screen( 'users_page_rest-oauth2-apps' ); + require_once ABSPATH . 'wp-admin/includes/class-wp-list-table.php'; + require_once dirname( __DIR__ ) . '/inc/admin/class-listtable.php'; + + return new ListTable( [ 'screen' => 'users_page_rest-oauth2-apps' ] ); + } + + /** + * Capture the output of the approved column for a client. + * + * @param \WP\OAuth2\Client $client Client to print the column for. + * + * @return string Column output. + */ + protected function get_approved_column( $client ) { + ob_start(); + $this->get_list_table()->column_approved( get_post( $client->get_post_id() ) ); + + return ob_get_clean(); + } + + /** + * Capture the output of the approval status for a client. + * + * @param \WP\OAuth2\Client $client Client to print the status for. + * + * @return string Notice markup. + */ + protected function get_approval_status( $client ) { + ob_start(); + render_approval_status( get_post( $client->get_post_id() ) ); + + return ob_get_clean(); + } + + public function test_list_table_has_approved_column() { + $this->assertArrayHasKey( 'approved', $this->get_list_table()->get_columns() ); + } + + public function test_approved_column_prints_yes_for_published_client() { + $this->assertSame( 'Yes', $this->get_approved_column( $this->create_client() ) ); + } + + public function test_approved_column_prints_no_for_unapproved_client() { + $this->assertSame( 'No', $this->get_approved_column( $this->create_unapproved_client() ) ); + } + + public function test_approval_status_shows_warning_and_button_for_admin() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $output = $this->get_approval_status( $this->create_unapproved_client() ); + + $this->assertStringContainsString( 'notice-warning', $output ); + $this->assertStringContainsString( 'This application is not approved yet.', $output ); + $this->assertStringContainsString( 'button button-primary', $output ); + $this->assertStringContainsString( 'action=approve', $output ); + $this->assertStringContainsString( 'return=edit', $output ); + $this->assertStringNotContainsString( 'Ask an administrator', $output ); + } + + public function test_approval_status_asks_for_administrator_without_capability() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'subscriber' ] ) ); + + $output = $this->get_approval_status( $this->create_unapproved_client() ); + + $this->assertStringContainsString( 'notice-warning', $output ); + $this->assertStringContainsString( 'Ask an administrator to approve it.', $output ); + $this->assertStringNotContainsString( 'button-primary', $output ); + } + + public function test_approval_status_shows_success_for_published_client() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $output = $this->get_approval_status( $this->create_client() ); + + $this->assertStringContainsString( 'notice-success', $output ); + $this->assertStringContainsString( 'This application is approved.', $output ); + $this->assertStringNotContainsString( 'button-primary', $output ); + } } diff --git a/tests/test-authorize-errors.php b/tests/test-authorize-errors.php new file mode 100644 index 0000000..bd7bfb4 --- /dev/null +++ b/tests/test-authorize-errors.php @@ -0,0 +1,44 @@ +create_unapproved_client(); + $_GET = [ 'client_id' => $client->get_id() ]; + + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertWPError( $result ); + $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', $result->get_error_code() ); + $this->assertSame( 403, $result->get_error_data()['status'] ); + } + + public function test_unknown_client_still_returns_invalid_client_id() { + $_GET = [ 'client_id' => 'nonexistent-client-id' ]; + + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertWPError( $result ); + $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', $result->get_error_code() ); + $this->assertSame( 400, $result->get_error_data()['status'] ); + } +} diff --git a/tests/test-client.php b/tests/test-client.php index fb3618c..353cabe 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -97,6 +97,25 @@ public function test_get_by_id_returns_null_for_unapproved() { $this->assertNull( $result ); } + public function test_get_unapproved_by_id_finds_draft_client() { + $draft_client = $this->create_unapproved_client(); + + $found = Client::get_unapproved_by_id( $draft_client->get_id() ); + $this->assertInstanceOf( Client::class, $found ); + $this->assertEquals( $draft_client->get_id(), $found->get_id() ); + } + + public function test_get_unapproved_by_id_returns_null_for_published_client() { + $this->assertNull( Client::get_unapproved_by_id( $this->client->get_id() ) ); + } + + public function test_get_unapproved_by_id_returns_null_for_trashed_client() { + $draft_client = $this->create_unapproved_client(); + wp_trash_post( $draft_client->get_post_id() ); + + $this->assertNull( Client::get_unapproved_by_id( $draft_client->get_id() ) ); + } + public function test_get_by_post_id_finds_client() { $found = Client::get_by_post_id( $this->client->get_post_id() ); $this->assertInstanceOf( Client::class, $found ); From e7963d20bd4015c7e2e5d09fc89338fc83f9acca Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 11:07:13 +0000 Subject: [PATCH 06/11] Only approve and show Approve for draft or pending clients The Approve button and handler accepted any status, so a trashed app could be approved and published again. handle_approve() also fataled on a missing post and never checked the post type. Add Client::UNAPPROVED_STATUSES as the one definition of "unapproved", used by get_unapproved_by_id(), render_approval_status() and handle_approve(). The status notice now prints nothing for trash, private, future and auto-draft posts. handle_approve() dies with a 400 unless the post exists, is a client and is draft or pending. The edit screen also rejects posts of another type. Drop the "Application approved." message and the did_action param after approving from the edit screen. The status notice changing to "This application is approved." is the feedback, so the message showed the same news twice. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/admin/namespace.php | 23 +++++++++++++---------- inc/class-client.php | 3 ++- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index 431f03b..bbcabff 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -305,12 +305,15 @@ function render_approval_status( $client_post ) { return; } - $is_approved = 'publish' === $client_post->post_status; - if ( $is_approved ) { + if ( 'publish' === $client_post->post_status ) { echo '

' . esc_html__( 'This application is approved.', 'oauth2' ) . '

'; return; } + if ( ! in_array( $client_post->post_status, Client::UNAPPROVED_STATUSES, true ) ) { + return; + } + $post_type_object = get_post_type_object( Client::POST_TYPE ); ?>
@@ -341,7 +344,7 @@ function render_edit_page() { if ( ! empty( $_REQUEST['id'] ) ) { $id = absint( $_REQUEST['id'] ); $consumer = Client::get_by_post_id( $id ); - if ( is_wp_error( $consumer ) || empty( $consumer ) ) { + if ( is_wp_error( $consumer ) || empty( $consumer ) || get_post_type( $id ) !== Client::POST_TYPE ) { wp_die( esc_html__( 'Invalid client ID.', 'oauth2' ) ); } @@ -382,10 +385,6 @@ function render_edit_page() { $messages[] = esc_html__( 'Regenerated secret.', 'oauth2' ); break; - case 'approve': - $messages[] = esc_html__( 'Application approved.', 'oauth2' ); - break; - default: $messages[] = esc_html__( 'Successfully created application.', 'oauth2' ); break; @@ -652,6 +651,11 @@ function handle_approve() { ); } + $post = get_post( $id ); + if ( ! $post || Client::POST_TYPE !== $post->post_type || ! in_array( $post->post_status, Client::UNAPPROVED_STATUSES, true ) ) { + wp_die( esc_html__( 'This application cannot be approved.', 'oauth2' ), '', [ 'response' => 400 ] ); + } + $client = Client::get_by_post_id( $id ); if ( is_wp_error( $client ) ) { wp_die( $client ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped @@ -666,9 +670,8 @@ function handle_approve() { wp_safe_redirect( get_url( [ - 'action' => 'edit', - 'id' => $id, - 'did_action' => 'approve', + 'action' => 'edit', + 'id' => $id, ] ) ); diff --git a/inc/class-client.php b/inc/class-client.php index d24291a..cee1dbc 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -16,6 +16,7 @@ class Client implements ClientInterface { const POST_TYPE = 'oauth2_client'; + const UNAPPROVED_STATUSES = [ 'draft', 'pending' ]; const CLIENT_SECRET_KEY = '_oauth2_client_secret'; const TYPE_KEY = '_oauth2_client_type'; const REDIRECT_URI_KEY = '_oauth2_redirect_uri'; @@ -328,7 +329,7 @@ public static function get_by_id( $id ) { * @return static|null Client if an unapproved client is found, null otherwise. */ public static function get_unapproved_by_id( $id ) { - return static::get_by_slug_and_status( $id, [ 'draft', 'pending' ] ); + return static::get_by_slug_and_status( $id, static::UNAPPROVED_STATUSES ); } /** From 6bc8a519ae5564c3634c59d512396aac82a8663a Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 11:07:22 +0000 Subject: [PATCH 07/11] Escape authorize error messages and drop the duplicate home link login_header() prints error messages as HTML. Messages such as "Client ID %s is invalid." include request input, so pass login_header() a copy of the error with every message run through esc_html(). login_footer() already prints a back-to-site link, so remove the second one. Move the status lookup into get_error_status() so it can be tested without the exit in render_error(). Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/endpoints/class-authorization.php | 31 ++++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/inc/endpoints/class-authorization.php b/inc/endpoints/class-authorization.php index 8c16587..98c4e44 100644 --- a/inc/endpoints/class-authorization.php +++ b/inc/endpoints/class-authorization.php @@ -56,23 +56,34 @@ public function handle_request() { exit; } + /** + * Get the HTTP status for an error. + * + * @param WP_Error $error Error to read the `status` error data from. + * @return int Status code, 400 if the error does not set one. + */ + public function get_error_status( WP_Error $error ) { + $data = $error->get_error_data(); + + return is_array( $data ) && ! empty( $data['status'] ) ? (int) $data['status'] : 400; + } + /** * Render an authorization error on the login screen, then exit. * * @param WP_Error $error Error to display. The `status` error data sets the HTTP status. */ protected function render_error( WP_Error $error ) { - $data = $error->get_error_data(); - $status = is_array( $data ) && ! empty( $data['status'] ) ? (int) $data['status'] : 400; + // login_header() prints error messages as HTML, so escape them first. + $escaped = new WP_Error(); + foreach ( $error->get_error_codes() as $code ) { + foreach ( $error->get_error_messages( $code ) as $message ) { + $escaped->add( $code, esc_html( $message ) ); + } + } - status_header( $status ); - login_header( __( 'Authorization error', 'oauth2' ), '', $error ); - printf( - '

%s

', - esc_url( home_url( '/' ) ), - /* translators: %s: site name */ - esc_html( sprintf( __( 'Go to %s', 'oauth2' ), get_bloginfo( 'name', 'display' ) ) ) - ); + status_header( $this->get_error_status( $error ) ); + login_header( __( 'Authorization error', 'oauth2' ), '', $escaped ); login_footer(); exit; } From 58be1a6a884c5323c565e1467a5a4670e3edac3b Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 11:07:33 +0000 Subject: [PATCH 08/11] Fix trashed-client test and cover multisite and error status wp_trash_post() renames the slug to "__trashed", so the trashed-client lookup test passed because the slug no longer matched, not because trash was excluded. Set the status with wp_update_post() so the slug stays. Grant super admin in the admin tests when running on multisite, where edit_users needs it. Add tests that the approval status prints nothing for a trashed client, and for the error status lookup (403 for unapproved_client, 400 by default). Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test-admin.php | 24 +++++++++++++++++++++++- tests/test-authorize-errors.php | 14 ++++++++++++++ tests/test-client.php | 9 ++++++++- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/tests/test-admin.php b/tests/test-admin.php index cc57192..d8f6f33 100644 --- a/tests/test-admin.php +++ b/tests/test-admin.php @@ -134,7 +134,11 @@ public function test_approved_column_prints_no_for_unapproved_client() { } public function test_approval_status_shows_warning_and_button_for_admin() { - wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); $output = $this->get_approval_status( $this->create_unapproved_client() ); @@ -165,4 +169,22 @@ public function test_approval_status_shows_success_for_published_client() { $this->assertStringContainsString( 'This application is approved.', $output ); $this->assertStringNotContainsString( 'button-primary', $output ); } + + public function test_approval_status_prints_nothing_for_trashed_client() { + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); + + $client = $this->create_unapproved_client(); + wp_update_post( + [ + 'ID' => $client->get_post_id(), + 'post_status' => 'trash', + ] + ); + + $this->assertSame( '', trim( $this->get_approval_status( $client ) ) ); + } } diff --git a/tests/test-authorize-errors.php b/tests/test-authorize-errors.php index bd7bfb4..03d4188 100644 --- a/tests/test-authorize-errors.php +++ b/tests/test-authorize-errors.php @@ -9,7 +9,9 @@ require_once __DIR__ . '/class-test-case.php'; +use WP\OAuth2\Endpoints\Authorization; use WP\OAuth2\Types\Authorization_Code; +use WP_Error; /** * Test cases for authorize errors. @@ -41,4 +43,16 @@ public function test_unknown_client_still_returns_invalid_client_id() { $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', $result->get_error_code() ); $this->assertSame( 400, $result->get_error_data()['status'] ); } + + public function test_error_status_is_forbidden_for_unapproved_client() { + $client = $this->create_unapproved_client(); + $_GET = [ 'client_id' => $client->get_id() ]; + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertSame( 403, ( new Authorization() )->get_error_status( $result ) ); + } + + public function test_error_status_defaults_to_bad_request() { + $this->assertSame( 400, ( new Authorization() )->get_error_status( new WP_Error( 'some_code', 'Message.' ) ) ); + } } diff --git a/tests/test-client.php b/tests/test-client.php index 353cabe..e139a68 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -111,7 +111,14 @@ public function test_get_unapproved_by_id_returns_null_for_published_client() { public function test_get_unapproved_by_id_returns_null_for_trashed_client() { $draft_client = $this->create_unapproved_client(); - wp_trash_post( $draft_client->get_post_id() ); + wp_update_post( + [ + 'ID' => $draft_client->get_post_id(), + 'post_status' => 'trash', + ] + ); + + $this->assertSame( 'trash', get_post_status( $draft_client->get_post_id() ) ); $this->assertNull( Client::get_unapproved_by_id( $draft_client->get_id() ) ); } From 69d1ef596d0e2de55ce0da4d30862a5b8b5aafa4 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 11:11:30 +0000 Subject: [PATCH 09/11] Limit the list Approve action to draft and pending clients The row action showed for every status except publish, so trashed clients got an Approve link that handle_approve() now refuses. Use Client::UNAPPROVED_STATUSES, as the edit screen and the handler do. Add a test that handle_approve() dies and leaves a trashed client in the trash. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/admin/class-listtable.php | 2 +- tests/test-admin.php | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/inc/admin/class-listtable.php b/inc/admin/class-listtable.php index 41b1a71..ae25fec 100644 --- a/inc/admin/class-listtable.php +++ b/inc/admin/class-listtable.php @@ -104,7 +104,7 @@ protected function column_name( $item ) { ]; $post_type_object = get_post_type_object( $item->post_type ); - if ( current_user_can( $post_type_object->cap->publish_posts ) && 'publish' !== $item->post_status ) { + if ( current_user_can( $post_type_object->cap->publish_posts ) && in_array( $item->post_status, Client::UNAPPROVED_STATUSES, true ) ) { $publish_link = get_approve_url( $item ); $actions['app-approve'] = sprintf( '%s', diff --git a/tests/test-admin.php b/tests/test-admin.php index d8f6f33..f80444f 100644 --- a/tests/test-admin.php +++ b/tests/test-admin.php @@ -10,6 +10,7 @@ require_once __DIR__ . '/class-test-case.php'; use WP\OAuth2\Admin\ListTable; +use function WP\OAuth2\Admin\handle_approve; use function WP\OAuth2\Admin\render_approval_status; use function WP\OAuth2\Admin\validate_parameters; @@ -18,6 +19,12 @@ */ class Test_Admin extends Test_Case { + public function tear_down() { + $_GET = []; + $_REQUEST = []; + parent::tear_down(); + } + /** * Build a valid set of submitted client parameters. * @@ -187,4 +194,33 @@ public function test_approval_status_prints_nothing_for_trashed_client() { $this->assertSame( '', trim( $this->get_approval_status( $client ) ) ); } + + public function test_handle_approve_refuses_trashed_client() { + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); + + $client = $this->create_unapproved_client(); + $id = $client->get_post_id(); + wp_update_post( + [ + 'ID' => $id, + 'post_status' => 'trash', + ] + ); + + $nonce = wp_create_nonce( 'rest-oauth2-approve:' . $id ); + $_GET['id'] = $id; + $_GET['_wpnonce'] = $nonce; + $_REQUEST['_wpnonce'] = $nonce; + + $this->expectException( \WPDieException::class ); + try { + handle_approve(); + } finally { + $this->assertSame( 'trash', get_post_status( $id ) ); + } + } } From ec5aabce2e34e9acaca51458d54ff0d1a7ad3504 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 12:28:04 +0000 Subject: [PATCH 10/11] Let get_by_id() optionally include unapproved clients A separate get_unapproved_by_id() duplicated the lookup for one caller. Give get_by_id() an $include_unapproved flag instead. The default still matches published clients only, so the token endpoint and other callers are unchanged. With the flag set it also matches draft and pending clients, and trashed clients never match. Add Client::is_approved(). Base::handle_authorisation() now looks the client up once with the flag set, returns invalid_client_id if there is none, and unapproved_client (403) if the client is not approved. The private status helper is folded into get_by_id() since nothing else needs it. approve() now refreshes the cached post, so is_approved() is true on the same instance straight after approving. Co-Authored-By: Claude Opus 5.5 (1M context) --- inc/class-client.php | 51 ++++++++++++++++++---------------------- inc/types/class-base.php | 21 +++++++++-------- 2 files changed, 34 insertions(+), 38 deletions(-) diff --git a/inc/class-client.php b/inc/class-client.php index cee1dbc..5ecd57f 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -62,6 +62,15 @@ public function get_post_id() { return $this->post->ID; } + /** + * Check whether the client is approved. + * + * @return bool True if the client is published. + */ + public function is_approved() { + return 'publish' === $this->post->post_status; + } + /** * Get the client's name. * @@ -311,36 +320,17 @@ public function issue_token( WP_User $user, $meta = [] ) { /** * Get a client by ID. * - * @param string $id Client ID. + * @param string $id Client ID. + * @param bool $include_unapproved Whether to also match draft and pending clients. Trashed clients never match. * - * @return static|null Token if ID is found, null otherwise. + * @return static|null Client if ID is found, null otherwise. */ - public static function get_by_id( $id ) { - return static::get_by_slug_and_status( $id, [ 'publish' ] ); - } - - /** - * Get an unapproved client by ID. - * - * Only draft and pending clients match, trashed clients never do. - * - * @param string $id Client ID. - * - * @return static|null Client if an unapproved client is found, null otherwise. - */ - public static function get_unapproved_by_id( $id ) { - return static::get_by_slug_and_status( $id, static::UNAPPROVED_STATUSES ); - } + public static function get_by_id( $id, $include_unapproved = false ) { + $statuses = [ 'publish' ]; + if ( $include_unapproved ) { + $statuses = array_merge( $statuses, static::UNAPPROVED_STATUSES ); + } - /** - * Get a client by ID, limited to the given post statuses. - * - * @param string $id Client ID. - * @param string[] $statuses Post statuses to match. - * - * @return static|null Client if found, null otherwise. - */ - private static function get_by_slug_and_status( $id, array $statuses ) { $args = [ 'post_type' => static::POST_TYPE, 'post_status' => $statuses, @@ -481,8 +471,13 @@ public function approve() { 'post_status' => 'publish', ]; $result = wp_update_post( wp_slash( $data ), true ); + if ( is_wp_error( $result ) ) { + return $result; + } - return is_wp_error( $result ) ? $result : true; + $this->post = get_post( $this->get_post_id() ); + + return true; } /** diff --git a/inc/types/class-base.php b/inc/types/class-base.php index fe20186..a217303 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -47,24 +47,25 @@ public function handle_authorisation() { // State is opaque to the server and must be returned to the client unchanged. $state = isset( $_GET['state'] ) && is_string( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - $client = Client::get_by_id( $client_id ); - if ( empty( $client ) && Client::get_unapproved_by_id( $client_id ) ) { + $client = Client::get_by_id( $client_id, true ); + if ( empty( $client ) ) { return new WP_Error( - 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', - __( 'This application is not currently approved. Ask the site administrator to approve it.', 'oauth2' ), + 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', + /* translators: %s: client ID */ + sprintf( __( 'Client ID %s is invalid.', 'oauth2' ), $client_id ), [ - 'status' => WP_Http::FORBIDDEN, + 'status' => WP_Http::BAD_REQUEST, 'client_id' => $client_id, ] ); } - if ( empty( $client ) ) { + + if ( ! $client->is_approved() ) { return new WP_Error( - 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', - /* translators: %s: client ID */ - sprintf( __( 'Client ID %s is invalid.', 'oauth2' ), $client_id ), + 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', + __( 'This application is not currently approved. Ask the site administrator to approve it.', 'oauth2' ), [ - 'status' => WP_Http::BAD_REQUEST, + 'status' => WP_Http::FORBIDDEN, 'client_id' => $client_id, ] ); From 0a7de1e1398d0f6b919ccb91e21266aed408ce19 Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Mon, 26 Oct 2026 12:28:04 +0000 Subject: [PATCH 11/11] Use a create_client() flag instead of a separate unapproved helper create_client() keeps its name and gains an $approve argument that defaults to true. Tests that need a draft client pass false, so the extra create_unapproved_client() helper goes away. Replace the get_unapproved_by_id() tests with get_by_id( $id, true ) tests for draft and trashed clients, and add is_approved() tests. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/class-test-case.php | 23 ++++++----------------- tests/test-admin.php | 10 +++++----- tests/test-authorize-errors.php | 4 ++-- tests/test-client.php | 23 +++++++++++++---------- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/tests/class-test-case.php b/tests/class-test-case.php index f10a749..4b9e372 100644 --- a/tests/class-test-case.php +++ b/tests/class-test-case.php @@ -16,14 +16,15 @@ abstract class Test_Case extends WP_UnitTestCase { /** - * Create a test client that is not approved. + * Create and optionally approve a test client. * * @param array $meta_overrides Optional overrides for the client meta. * @param string $name Client name. + * @param bool $approve Whether to approve the client. * * @return Client */ - protected function create_unapproved_client( array $meta_overrides = [], $name = 'Test Client' ) { + protected function create_client( array $meta_overrides = [], $name = 'Test Client', $approve = true ) { $data = [ 'name' => $name, 'description' => 'Test client description.', @@ -39,21 +40,9 @@ protected function create_unapproved_client( array $meta_overrides = [], $name = $client = Client::create( $data ); $this->assertInstanceOf( Client::class, $client, 'create_client helper: Client::create() must return a Client instance.' ); - - return $client; - } - - /** - * Create and approve a test client. - * - * @param array $meta_overrides Optional overrides for the client meta. - * @param string $name Client name. - * - * @return Client - */ - protected function create_client( array $meta_overrides = [], $name = 'Test Client' ) { - $client = $this->create_unapproved_client( $meta_overrides, $name ); - $client->approve(); + if ( $approve ) { + $client->approve(); + } return $client; } diff --git a/tests/test-admin.php b/tests/test-admin.php index f80444f..f259654 100644 --- a/tests/test-admin.php +++ b/tests/test-admin.php @@ -137,7 +137,7 @@ public function test_approved_column_prints_yes_for_published_client() { } public function test_approved_column_prints_no_for_unapproved_client() { - $this->assertSame( 'No', $this->get_approved_column( $this->create_unapproved_client() ) ); + $this->assertSame( 'No', $this->get_approved_column( $this->create_client( [], 'Test Client', false ) ) ); } public function test_approval_status_shows_warning_and_button_for_admin() { @@ -147,7 +147,7 @@ public function test_approval_status_shows_warning_and_button_for_admin() { } wp_set_current_user( $user_id ); - $output = $this->get_approval_status( $this->create_unapproved_client() ); + $output = $this->get_approval_status( $this->create_client( [], 'Test Client', false ) ); $this->assertStringContainsString( 'notice-warning', $output ); $this->assertStringContainsString( 'This application is not approved yet.', $output ); @@ -160,7 +160,7 @@ public function test_approval_status_shows_warning_and_button_for_admin() { public function test_approval_status_asks_for_administrator_without_capability() { wp_set_current_user( self::factory()->user->create( [ 'role' => 'subscriber' ] ) ); - $output = $this->get_approval_status( $this->create_unapproved_client() ); + $output = $this->get_approval_status( $this->create_client( [], 'Test Client', false ) ); $this->assertStringContainsString( 'notice-warning', $output ); $this->assertStringContainsString( 'Ask an administrator to approve it.', $output ); @@ -184,7 +184,7 @@ public function test_approval_status_prints_nothing_for_trashed_client() { } wp_set_current_user( $user_id ); - $client = $this->create_unapproved_client(); + $client = $this->create_client( [], 'Test Client', false ); wp_update_post( [ 'ID' => $client->get_post_id(), @@ -202,7 +202,7 @@ public function test_handle_approve_refuses_trashed_client() { } wp_set_current_user( $user_id ); - $client = $this->create_unapproved_client(); + $client = $this->create_client( [], 'Test Client', false ); $id = $client->get_post_id(); wp_update_post( [ diff --git a/tests/test-authorize-errors.php b/tests/test-authorize-errors.php index 03d4188..bd205da 100644 --- a/tests/test-authorize-errors.php +++ b/tests/test-authorize-errors.php @@ -24,7 +24,7 @@ public function tear_down() { } public function test_unapproved_client_returns_forbidden_error() { - $client = $this->create_unapproved_client(); + $client = $this->create_client( [], 'Test Client', false ); $_GET = [ 'client_id' => $client->get_id() ]; $result = ( new Authorization_Code() )->handle_authorisation(); @@ -45,7 +45,7 @@ public function test_unknown_client_still_returns_invalid_client_id() { } public function test_error_status_is_forbidden_for_unapproved_client() { - $client = $this->create_unapproved_client(); + $client = $this->create_client( [], 'Test Client', false ); $_GET = [ 'client_id' => $client->get_id() ]; $result = ( new Authorization_Code() )->handle_authorisation(); diff --git a/tests/test-client.php b/tests/test-client.php index e139a68..db37e2a 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -97,20 +97,16 @@ public function test_get_by_id_returns_null_for_unapproved() { $this->assertNull( $result ); } - public function test_get_unapproved_by_id_finds_draft_client() { - $draft_client = $this->create_unapproved_client(); + public function test_get_by_id_with_unapproved_finds_draft_client() { + $draft_client = $this->create_client( [], 'Test Client', false ); - $found = Client::get_unapproved_by_id( $draft_client->get_id() ); + $found = Client::get_by_id( $draft_client->get_id(), true ); $this->assertInstanceOf( Client::class, $found ); $this->assertEquals( $draft_client->get_id(), $found->get_id() ); } - public function test_get_unapproved_by_id_returns_null_for_published_client() { - $this->assertNull( Client::get_unapproved_by_id( $this->client->get_id() ) ); - } - - public function test_get_unapproved_by_id_returns_null_for_trashed_client() { - $draft_client = $this->create_unapproved_client(); + public function test_get_by_id_with_unapproved_returns_null_for_trashed_client() { + $draft_client = $this->create_client( [], 'Test Client', false ); wp_update_post( [ 'ID' => $draft_client->get_post_id(), @@ -119,8 +115,15 @@ public function test_get_unapproved_by_id_returns_null_for_trashed_client() { ); $this->assertSame( 'trash', get_post_status( $draft_client->get_post_id() ) ); + $this->assertNull( Client::get_by_id( $draft_client->get_id(), true ) ); + } + + public function test_is_approved_is_true_for_published_client() { + $this->assertTrue( $this->client->is_approved() ); + } - $this->assertNull( Client::get_unapproved_by_id( $draft_client->get_id() ) ); + public function test_is_approved_is_false_for_draft_client() { + $this->assertFalse( $this->create_client( [], 'Test Client', false )->is_approved() ); } public function test_get_by_post_id_finds_client() {