Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

Open
wants to merge 26 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
88952a5
Add REST API health check for Optimization Detective
b1ink0 Dec 19, 2024
4bff9fe
Improve error message of site health check
b1ink0 Dec 19, 2024
56d769a
Add scheduled health check for Optimization Detective REST API
b1ink0 Dec 19, 2024
ba911e1
Save site health check status for Optimization Detective REST API
b1ink0 Dec 19, 2024
12a229c
Add test for when REST API is available
b1ink0 Dec 19, 2024
a0f460d
Add tests for REST API unauthorized error handling
b1ink0 Dec 19, 2024
c29cb7c
Add test for REST API forbidden error handling
b1ink0 Dec 19, 2024
8d8ae3e
Refactor to move site-health directory to plugin root directory
b1ink0 Dec 20, 2024
422a5bf
Clear site health check data during plugin uninstallation
b1ink0 Dec 20, 2024
0bc74f7
Move REST API availability check to appropriate function
b1ink0 Dec 20, 2024
36042fd
Add error message and error code to option
b1ink0 Dec 20, 2024
ad21df8
Refactor to store status code instead of string, Add fallback else co…
b1ink0 Dec 20, 2024
0bba468
Refactor to only use update_option once
b1ink0 Dec 20, 2024
e1b9004
Add activation hook to initialize option value
b1ink0 Dec 23, 2024
dbede65
Move scheduling to activation hook
b1ink0 Dec 23, 2024
2b2ca3e
Change health check scheduling from hourly to weekly
b1ink0 Dec 23, 2024
e7c1001
Run REST API health check on plugin activation, Display notice if che…
b1ink0 Dec 24, 2024
7e4e057
Move activation logic to separate function
b1ink0 Dec 24, 2024
a2baa65
Refactor activation logic to directly call REST API health check func…
b1ink0 Dec 24, 2024
e1ec10c
Merge branch 'trunk' into add/site-health-check-for-od-rest-api
b1ink0 Dec 24, 2024
48c83a5
Improve site health status messages for clarity and consistency
b1ink0 Dec 25, 2024
8895d35
Refactor site health checks files by combining them into single file
b1ink0 Jan 8, 2025
2fbd530
Move added action and filters for site health checks to plugins's hoo…
b1ink0 Jan 8, 2025
5949bc9
Merge branch 'trunk' into add/site-health-check-for-od-rest-api
b1ink0 Jan 8, 2025
1aa82b4
Merge branch 'trunk' into add/site-health-check-for-od-rest-api
b1ink0 Jan 8, 2025
98fff4d
Merge branch 'trunk' into add/site-health-check-for-od-rest-api
westonruter Jan 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions plugins/optimization-detective/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -127,5 +127,8 @@ class_alias( OD_URL_Metric_Group_Collection::class, 'OD_URL_Metrics_Group_Collec

// Add hooks for the above requires.
require_once __DIR__ . '/hooks.php';

// Load site health checks.
require_once __DIR__ . '/site-health/load.php';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted below, I think the directory can be removed in favor of just a simpler site-health.php for now:

Suggested change
require_once __DIR__ . '/site-health/load.php';
require_once __DIR__ . '/site-health.php';

}
);
7 changes: 6 additions & 1 deletion plugins/optimization-detective/optimization.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,12 @@ static function ( string $output, ?int $phase ): string {
* @access private
*/
function od_maybe_add_template_output_buffer_filter(): void {
if ( ! od_can_optimize_response() || isset( $_GET['optimization_detective_disabled'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$od_rest_api_info = get_option( 'od_rest_api_info' );
if (
! od_can_optimize_response() ||
isset( $_GET['optimization_detective_disabled'] ) || // phpcs:ignore WordPress.Security.NonceVerification.Recommended
( isset( $od_rest_api_info['available'] ) && ! (bool) $od_rest_api_info['available'] )
) {
return;
}
$callback = 'od_optimize_template_output_buffer';
Expand Down
15 changes: 15 additions & 0 deletions plugins/optimization-detective/site-health/load.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file can be removed in favor of moving the /site-health/rest-api/helper.php file below up to /site-health.php.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php
/**
* Site Health checks loader.
*
* @package optimization-detective
* @since n.e.x.t
*/

if ( ! defined( 'ABSPATH' ) ) {
exit; // Exit if accessed directly.
}

// REST API site health check.
require_once __DIR__ . '/rest-api/helper.php';
require_once __DIR__ . '/rest-api/hooks.php';
122 changes: 122 additions & 0 deletions plugins/optimization-detective/site-health/rest-api/helper.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is only one Site Health test, what about eliminating the load.php file and moving this file to /site-health.php?

Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<?php
/**
* Helper functions for the Optimization Detective REST API health check.
*
* @package optimization-detective
* @since n.e.x.t
*/

if ( ! defined( 'ABSPATH' ) ) {
exit; // Exit if accessed directly.
}

/**
* Tests availability of the Optimization Detective REST API endpoint.
*
* @since n.e.x.t
*
* @return array{label: string, status: string, badge: array{label: string, color: string}, description: string, actions: string, test: string} Result.
*/
function od_optimization_detective_rest_api_test(): array {
$result = array(
'label' => __( 'Your site has functional Optimization Detective REST API endpoint', 'optimization-detective' ),
'status' => 'good',
'badge' => array(
'label' => __( 'Optimization Detective', 'optimization-detective' ),
'color' => 'blue',
),
'description' => sprintf(
'<p>%s</p>',
__( 'Optimization Detective can send and store URL metrics via REST API endpoint', 'optimization-detective' )
),
'actions' => '',
'test' => 'optimization_detective_rest_api',
);

$rest_url = get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE );
$response = wp_remote_post(
$rest_url,
array(
'headers' => array( 'Content-Type' => 'application/json' ),
'sslverify' => false,
)
);

if ( is_wp_error( $response ) ) {
$result['status'] = 'recommended';
$result['label'] = __( 'Your site encountered error accessing Optimization Detective REST API endpoint', 'optimization-detective' );
$result['description'] = sprintf(
'<p>%s</p>',
esc_html__( 'The Optimization Detective endpoint could not be reached. This might mean the REST API is disabled or blocked.', 'optimization-detective' )
);
$info = array(
'error_message' => $response->get_error_message(),
'error_code' => $response->get_error_code(),
'available' => false,
);
} else {
$status_code = wp_remote_retrieve_response_code( $response );
$data = json_decode( wp_remote_retrieve_body( $response ), true );
$expected_params = array( 'slug', 'current_etag', 'hmac', 'url', 'viewport', 'elements' );
$info = array(
'status_code' => $status_code,
'available' => false,
);

if (
400 === $status_code
&& isset( $data['data']['params'] )
&& is_array( $data['data']['params'] )
&& count( $expected_params ) === count( array_intersect( $data['data']['params'], $expected_params ) )
) {
// The REST API endpoint is available.
$info['available'] = true;
} elseif ( 401 === $status_code ) {
$result['status'] = 'recommended';
$result['label'] = __( 'Your site encountered unauthorized error for Optimization Detective REST API endpoint', 'optimization-detective' );
$result['description'] = sprintf(
'<p>%s</p>',
esc_html__( 'The REST API endpoint requires authentication. Ensure proper credentials are provided.', 'optimization-detective' )
);
} elseif ( 403 === $status_code ) {
$result['status'] = 'recommended';
$result['label'] = __( 'Your site encountered forbidden error for Optimization Detective REST API endpoint', 'optimization-detective' );
$result['description'] = sprintf(
'<p>%s</p>',
esc_html__( 'The REST API endpoint is blocked check server or security settings.', 'optimization-detective' )
);
} else {
$result['status'] = 'recommended';
$result['label'] = __( 'Your site encountered error accessing Optimization Detective REST API endpoint', 'optimization-detective' );
$result['description'] = sprintf(
'<p>%s</p>',
esc_html__( 'The Optimization Detective endpoint could not be reached. This might mean the REST API is disabled or blocked.', 'optimization-detective' )
);
}
}

update_option( 'od_rest_api_info', $info );
return $result;
}

/**
* Periodically runs the Optimization Detective REST API health check.
*
* @since n.e.x.t
*/
function od_schedule_rest_api_health_check(): void {
if ( ! (bool) wp_next_scheduled( 'od_rest_api_health_check_event' ) ) {
wp_schedule_event( time(), 'hourly', 'od_rest_api_health_check_event' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hourly is too much. Maybe weekly would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I currently have it set to run weekly, but that might be too infrequent. If a configuration change disables the REST API, it could take an entire week for user to detect the issue. I believe running it daily would be a better choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think weekly is fine. It's not likely that a user would be changing the availability of the REST API. If we check at the moment that a plugin is activated, and then check weekly thereafter, then this should be good. Note that Site Health's own checks run on a weekly basis via the wp_site_health_scheduled_check action.

}
}
add_action( 'wp', 'od_schedule_rest_api_health_check' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a best practice? Should it rather go in admin_init to avoid frontend writes? I'm not sure what others do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think scheduling on plugin activation hook will be better than admin_init.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the plugin activation hook doesn't work when network-activating a plugin in multisite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In looking at WP_Site_Health, it goes ahead and schedules an event even for frontend requests, since it calls its maybe_create_scheduled_event method in the constructor. And the instance is loaded in wp-settings.php. Nevertheless, since a database write is involved, it is preferable if event scheduling happens via an admin request and not unauthenticated frontend requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at this comment regarding which hook should be used.


/**
* Hook for the scheduled REST API health check.
*
* @since n.e.x.t
*/
function od_run_scheduled_rest_api_health_check(): void {
od_optimization_detective_rest_api_test();
}
add_action( 'od_rest_api_health_check_event', 'od_run_scheduled_rest_api_health_check' );
29 changes: 29 additions & 0 deletions plugins/optimization-detective/site-health/rest-api/hooks.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php
/**
* Hook callbacks used for the Optimization Detective REST API health check.
*
* @package optimization-detective
* @since n.e.x.t
*/

if ( ! defined( 'ABSPATH' ) ) {
exit; // Exit if accessed directly.
}

/**
* Adds the Optimization Detective REST API check to site health tests.
*
* @since n.e.x.t
*
* @param array{direct: array<string, array{label: string, test: string}>} $tests Site Health Tests.
* @return array{direct: array<string, array{label: string, test: string}>} Amended tests.
*/
function od_optimization_detective_add_rest_api_test( array $tests ): array {
$tests['direct']['optimization_detective_rest_api'] = array(
'label' => __( 'Optimization Detective REST API Endpoint Availability', 'optimization-detective' ),
'test' => 'od_optimization_detective_rest_api_test',
);

return $tests;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be moved to a /site-health.php file.

add_filter( 'site_status_tests', 'od_optimization_detective_add_rest_api_test' );
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
<?php
/**
* Tests for Optimization Detective REST API site health check.
*
* @package optimization-detective
*/

class Test_OD_REST_API_Site_Health_Check extends WP_UnitTestCase {

/**
* Holds mocked response headers for different test scenarios.
*
* @var array<string, array<string, mixed>>
*/
protected $mocked_responses = array();

/**
* Setup each test.
*/
public function setUp(): void {
parent::setUp();

// Clear any filters or mocks.
remove_all_filters( 'pre_http_request' );

// Add the filter to mock HTTP requests.
add_filter( 'pre_http_request', array( $this, 'mock_http_requests' ), 10, 3 );
}

/**
* Test that the site health check is `good` when the REST API is available.
*/
public function test_rest_api_available(): void {
$this->mocked_responses = array(
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response(
400,
'Bad Request',
array(
'data' => array(
'params' => array( 'slug', 'current_etag', 'hmac', 'url', 'viewport', 'elements' ),
),
)
),
);

$result = od_optimization_detective_rest_api_test();
$od_rest_api_info = get_option( 'od_rest_api_info', array() );

$this->assertSame( 'good', $result['status'] );
$this->assertSame( 400, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' );
$this->assertTrue( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : false );
}

/**
* Test behavior when REST API returns an unauthorized error.
*/
public function test_rest_api_unauthorized(): void {
$this->mocked_responses = array(
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response(
401,
'Unauthorized'
),
);

$result = od_optimization_detective_rest_api_test();
$od_rest_api_info = get_option( 'od_rest_api_info', array() );

$this->assertSame( 'recommended', $result['status'] );
$this->assertSame( 401, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' );
$this->assertFalse( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : true );
}

/**
* Test behavior when REST API returns an forbidden error.
*/
public function test_rest_api_forbidden(): void {
$this->mocked_responses = array(
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response(
403,
'Forbidden'
),
);

$result = od_optimization_detective_rest_api_test();
$od_rest_api_info = get_option( 'od_rest_api_info', array() );

$this->assertSame( 'recommended', $result['status'] );
$this->assertSame( 403, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' );
$this->assertFalse( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : true );
}

/**
* Mock HTTP requests for assets to simulate different responses.
*
* @param bool $response A preemptive return value of an HTTP request. Default false.
* @param array<string, mixed> $args Request arguments.
* @param string $url The request URL.
* @return array<string, mixed> Mocked response.
*/
public function mock_http_requests( bool $response, array $args, string $url ): array {
if ( isset( $this->mocked_responses[ $url ] ) ) {
return $this->mocked_responses[ $url ];
}

// If no specific mock set, default to a generic success response.
return array(
'response' => array(
'code' => 200,
'message' => 'OK',
),
);
}

/**
* Build a mock response.
*
* @param int $status_code HTTP status code.
* @param string $message HTTP status message.
* @param array<string, mixed> $body Response body.
* @return array<string, mixed> Mocked response.
*/
protected function build_mock_response( int $status_code, string $message, array $body = array() ): array {
return array(
'response' => array(
'code' => $status_code,
'message' => $message,
),
'body' => wp_json_encode( $body ),
);
}
}
4 changes: 4 additions & 0 deletions plugins/optimization-detective/uninstall.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
// Delete all URL Metrics posts for the current site.
OD_URL_Metrics_Post_Type::delete_all_posts();
wp_unschedule_hook( OD_URL_Metrics_Post_Type::GC_CRON_EVENT_NAME );

// Clear out site health check data.
delete_option( 'od_rest_api_info' );
wp_unschedule_hook( 'od_rest_api_health_check_event' );
};

$od_delete_site_data();
Expand Down
Loading