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

b1ink0 · 2024-12-19T17:59:46Z

Summary

Relevant technical choices

Adds a site health check to verify the availability and status of the /optimization-detective/v1/url-metrics:store REST API endpoint. The process will short-circuit if the endpoint is inaccessible.
Based on the status, a global admin notice on plugin activation and meta row notice on subsequent admin pages will be shown.

Scenarios:

When the health check passes
When the REST API endpoint returns a forbidden error
When the REST API endpoint returns an unauthorized error
When other errors are encountered

westonruter

Should there be a plugin activation hook added as well which does add_option() for the new option and then also kicks off (or schedules) a REST API check? Ideally there would be a warning shown immediately after activating the plugin (e.g. on the plugins list table screen) whether the REST API is working so that the user doesn't have to discover it later via Site Health.

westonruter · 2024-12-19T18:26:02Z

plugins/optimization-detective/includes/site-health/load.php

Maybe put site-health in the root directory instead of inside includes since there are no other directories in there?

I thought in future if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing then it would be better to add that feature in includes/admin. But we can just refactor things later when we need it so moving site-health to root makes sence.

Yeah, let's put it in the root for now since all other directories are there.

if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing

Aside: I did put together a rough utility plugin for this: https://github.com/westonruter/od-admin-ui

westonruter · 2024-12-19T18:27:41Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+			'<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(


This is the first PR that adds an option to to Optimization Detective. We'll need to make sure that the relevant delete_option() calls get added to the plugin's uninstall.php.

westonruter · 2024-12-19T18:29:24Z

plugins/optimization-detective/optimization.php

+	// Disable detection if the REST API is disabled.
+	$od_rest_api_info = get_option( 'od_rest_api_info' );
+	if ( is_array( $od_rest_api_info ) && isset( $od_rest_api_info['available'] ) ) {
+		$needs_detection = (bool) $od_rest_api_info['available'];
+	}
+


Actually, this check wouldn't make sense here. It should rather be done in od_maybe_add_template_output_buffer_filter() to short-circuit if the REST API it is not available.

westonruter · 2024-12-19T18:32:01Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		update_option(
+			'od_rest_api_info',
+			array(
+				'status'    => 'error',


Should the $error->get_message() and maybe $error->get_code() be stored here?

westonruter · 2024-12-19T18:34:40Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		update_option(
+			'od_rest_api_info',
+			array(
+				'status'    => 'forbidden',


Instead of storing the string, what about storing the $status_code instead?

westonruter · 2024-12-19T18:35:07Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+				'available' => false,
+			)
+		);
+	}


The else condition should be added as an error result as well. Here especially the $status_code could be used.

westonruter · 2024-12-19T18:38:03Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		&& count( $expected_params ) === count( array_intersect( $data['data']['params'], $expected_params ) )
+	) {
+		// The REST API endpoint is available.
+		update_option(


Instead of having update_option() appearing in multiple places, each condition could populate an $info variable which is then sent into update_option() once at the end of the function.

…ndition

westonruter · 2024-12-20T21:13:28Z

plugins/optimization-detective/site-health/rest-api/helper.php

+		wp_schedule_event( time(), 'hourly', 'od_rest_api_health_check_event' );
+	}
+}
+add_action( 'wp', 'od_schedule_rest_api_health_check' );


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

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

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

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.

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

westonruter · 2024-12-20T21:13:42Z

plugins/optimization-detective/site-health/rest-api/helper.php

+ */
+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' );


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

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.

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.

…ck fails

…tion

… add/site-health-check-for-od-rest-api

…tests

…dmins

…ove formatting of error message

westonruter · 2025-01-16T01:17:20Z

I realized that the Site Health test was getting buried among other recommended tests so I've bumped it to critical. I've also included the raw response so the user can have additional context:

westonruter · 2025-01-16T01:24:19Z

Example I've seen with Nginx blocking the REST API shows up as this:

…or example

westonruter

Looking forward to getting this out there!

westonruter · 2025-01-16T15:16:51Z

plugins/optimization-detective/site-health.php

+		add_action(
+			'admin_notices',
+			static function (): void {
+				od_maybe_render_rest_api_health_check_admin_notice( false );


Ah, the rest case should actually run the admin_notices action to increase code coverage here.

Done in 6fee919

westonruter · 2025-01-16T15:17:19Z

plugins/optimization-detective/site-health.php

+		||
+		is_wp_error( $response )
+	) {
+		return $response;


Looks like I neglected to include a test for the cached state.

See 6fee919

westonruter · 2025-01-16T15:18:38Z

plugins/optimization-detective/site-health.php

+	$response       = od_get_rest_api_health_check_response( false );
+	$result         = od_compose_site_health_result( $response );
+	$is_unavailable = 'good' !== $result['status'];
+	update_option( 'od_rest_api_unavailable', $is_unavailable ? '1' : '0' );


This intentionally makes the option autoloaded. Maybe we should pass the parameter to make this explicit.

See 8926f10

felixarntz

I think there are still a few things in here that need to be addressed, given the major additional changes made.

plugins/optimization-detective/site-health.php

felixarntz · 2025-01-17T17:45:18Z

plugins/optimization-detective/site-health.php

+	$response = wp_remote_post(
+		$rest_url,
+		array(
+			'headers'   => array( 'Content-Type' => 'application/json' ),
+			'sslverify' => false,
+		)
+	);
+
+	// This transient will be used when showing the admin notice with the plugin on the plugins screen.
+	// The 1-day expiration allows for fresher content than the weekly check initiated by Site Health.
+	set_transient( $transient_key, $response, DAY_IN_SECONDS );


I'm a bit concerned about trying to store a potential WP_Error object in a transient. Can we parse that into a raw data array instead to make this safer?

Why is it concerning to store a WP_Error in a transient? The object gets serialized.

Hmm, is there prior art for that in WordPress? I suppose it could work, but have you tested this, both using the DB for transients as well as a persistent object cache?

Looking at the WP_Error class, for instance I would expect the $additional_data property to be missing, since it's not public. Granted, that data may not be important for what we're doing here, but it shows the fragility of relying on serialization.

As noted above, the test_od_get_rest_api_health_check_response test confirms that transients return unserialized WP_Error instances.

Taking a step back though, we're probably arguing too much about this detail. I think there is a risk, but feel free to include it. If someone reports this later as a problem, we can still address it.

Take a look at fetch_feed() such as used in the RSS widget. It stores an array that contains objects in it.

FWIW, I've been aware of storing class instances in transients and options since before I can remember. What's the difference with arrays? Both require serialization.

I was googling for other examples, and I found a reference to Site Kit storing objects in transients: https://wpengine.com/resources/guide-to-transients-in-wordpress/

I can't find the current reference to that code in GitHub though.

Here's an example in the Transients documentation about storing a WP_Query instance in a transient: https://developer.wordpress.org/apis/transients/#complete-example

Here's another example where an object is stored in a transient in wp_version_check(): https://github.com/WordPress/wordpress-develop/blob/2f654881e494424634d5821d1ef37c06edb8923a/src/wp-includes/update.php#L36-L73

Objects are also stored in object cache in WP_Term_Query: https://github.com/WordPress/wordpress-develop/blob/2f654881e494424634d5821d1ef37c06edb8923a/src/wp-includes/class-wp-term-query.php#L879-L902

felixarntz · 2025-01-17T17:49:17Z

plugins/optimization-detective/site-health.php

+ * @param string $plugin_file Plugin file.
+ */
+function od_render_rest_api_health_check_admin_notice_in_plugin_row( string $plugin_file ): void {
+	if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?


I think we could use plugin_basename( __FILE__ ) to address the TODO here?

Suggested change

if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?

if ( plugin_basename( __FILE__ ) !== $plugin_file ) {

Looks like that doesn't work, at least not in the wp-env test environment. Here plugin_basename( __FILE__ ) is returning performance/plugins/optimization-detective/site-health.php.

Note that Embed Optimizer has similar code running at the after_plugin_row_meta filter:

performance/plugins/embed-optimizer/hooks.php

Lines 345 to 349 in 96bcb2d

function embed_optimizer_print_row_meta_install_notice( string $plugin_file ): void {

$od_plugin_slug = 'optimization-detective';

$od_plugin_file = "{$od_plugin_slug}/load.php";

$od_plugin_name = 'Optimization Detective';

if ( 'embed-optimizer/load.php' === $plugin_file && ! is_plugin_active( $od_plugin_file ) ) {

That should only be a concern in a "weird" development setup like wp-env though, for regular sites the plugin basename will be optimization-detective/load.php.

I wonder how WordPress computes the value for $plugin_file if it's not matching plugin_basename( $plugin_root_dir_file ). 🤔

Anyway, what we could potentially do for now is create a function like optimization_detective_get_basename() that calls plugin_basename( __FILE__ ) and uses it unless there is more than one / character in it, in which case it's a "weird setup" situation and we can fall back to the hard-coded one.

I guess we could do this:

Suggested change

if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?

if ( basename( __DIR__ ) !== dirname( $plugin_file ) ) {

Maybe even slightly safer as:

Suggested change

if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?

if ( basename( __DIR__ ) . '/load.php' !== $plugin_file ) {

Granted, the Embed Optimizer logic really depends on the installed plugin slugs actually being the same ones as on the directory. Since this is just the addition of a notice which will be showing up in Site Health anyway, maybe we should just accept that there's a slight chance a notice won't be shown given it would very unlikely for anything but the official slug to be used:

Suggested change

if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?

if ( 'optimization-detective/load.php' !== $plugin_file ) {

I agree it's not critical, but I also think the hard-coding is not ideal. So maybe let's just keep the TODO in place for now and think about this separately later.

felixarntz · 2025-01-17T17:51:02Z

plugins/optimization-detective/storage/rest-api.php

 */
 function od_register_endpoint(): void {

 	// The slug and cache_purge_post_id args are further validated via the validate_callback for the 'hmac' parameter,
 	// they are provided as input with the 'url' argument to create the HMAC by the server.
+	// The following args are referenced in od_compose_site_health_result().


See my comment above on why that's not a good idea.

Reverted in ddc6164

Co-authored-by: felixarntz <[email protected]>

felixarntz

@westonruter This looks good, only a few minor points now. My only blocking concern still is #1762 (comment) (I don't think we can rely on serialization for sites with a persistent object cache).

plugins/optimization-detective/site-health.php

felixarntz · 2025-01-17T22:15:52Z

plugins/optimization-detective/site-health.php

+ * @param string $plugin_file Plugin file.
+ */
+function od_render_rest_api_health_check_admin_notice_in_plugin_row( string $plugin_file ): void {
+	if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?


I agree it's not critical, but I also think the hard-coding is not ideal. So maybe let's just keep the TODO in place for now and think about this separately later.

Co-authored-by: Felix Arntz <[email protected]>

b1ink0 added 7 commits December 19, 2024 18:34

Add REST API health check for Optimization Detective

88952a5

Improve error message of site health check

4bff9fe

Add scheduled health check for Optimization Detective REST API

56d769a

Save site health check status for Optimization Detective REST API

ba911e1

Add test for when REST API is available

12a229c

Add tests for REST API unauthorized error handling

a0f460d

Add test for REST API forbidden error handling

c29cb7c

b1ink0 changed the title ~~Add/site health check for od rest api~~ Optimization Detective can be blocked from working due to sites disabling the REST API Dec 19, 2024

westonruter reviewed Dec 19, 2024

View reviewed changes

westonruter added this to the optimization-detective n.e.x.t milestone Dec 19, 2024

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Dec 19, 2024

b1ink0 added 6 commits December 20, 2024 18:04

Refactor to move site-health directory to plugin root directory

8d8ae3e

Clear site health check data during plugin uninstallation

422a5bf

Move REST API availability check to appropriate function

0bc74f7

Add error message and error code to option

36042fd

Refactor to store status code instead of string, Add fallback else co…

ad21df8

…ndition

Refactor to only use update_option once

0bba468

b1ink0 changed the title ~~Optimization Detective can be blocked from working due to sites disabling the REST API~~ Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible Dec 20, 2024

westonruter reviewed Dec 20, 2024

View reviewed changes

b1ink0 added 7 commits December 23, 2024 19:07

Add activation hook to initialize option value

e1b9004

Move scheduling to activation hook

dbede65

Change health check scheduling from hourly to weekly

2b2ca3e

Run REST API health check on plugin activation, Display notice if che…

e7c1001

…ck fails

Move activation logic to separate function

7e4e057

Refactor activation logic to directly call REST API health check func…

a2baa65

…tion

Merge branch 'trunk' into add/site-health-check-for-od-rest-api

e1ec10c

b1ink0 marked this pull request as ready for review December 24, 2024 14:12

b1ink0 requested a review from felixarntz as a code owner December 24, 2024 14:12

westonruter added 7 commits January 15, 2025 14:57

Merge branch 'trunk' of https://github.com/WordPress/performance into…

19ff8e4

… add/site-health-check-for-od-rest-api

Update check in od_maybe_add_template_output_buffer_filter() and add …

546217f

…tests

Add test coverage for Site Health logic

3d5e8a8

Account for Site Health not being available in multisite to regular a…

c3b682f

…dmins

Add test coverage for HTTP request returning WP_Error

c085ff0

Fix absent site health test after failed function rename

50a1898

Further account for multisite and Site Health

c7d31a7

westonruter requested a review from felixarntz January 16, 2025 00:48

westonruter added 2 commits January 15, 2025 16:55

Make Optimization Detective REST API access a critical error and impr…

85da162

…ove formatting of error message

Increase specificity of what the expected error response looks like

0772ebe

Show REST API error message in blockquote if available; add Nginx err…

dbff1f7

…or example

westonruter approved these changes Jan 16, 2025

View reviewed changes

westonruter reviewed Jan 16, 2025

View reviewed changes

westonruter mentioned this pull request Jan 16, 2025

Use of Cache-Control: no-store, no-cache, max-age=0 should be flagged by Site Health for unauthenticated frontend responses #1692

Open

b1ink0 and others added 3 commits January 16, 2025 22:32

Add test to make sure hooks are added

0693086

Explicitly autoload od_rest_api_unavailable option

8926f10

Add explicit test for od_get_rest_api_health_check_response

6fee919

felixarntz reviewed Jan 17, 2025

View reviewed changes

westonruter and others added 3 commits January 17, 2025 09:59

Remove checking for specific missing required params

ddc6164

Co-authored-by: felixarntz <[email protected]>

Remove overkill type checking for transient response

b4736a4

Co-authored-by: felixarntz <[email protected]>

Change Site Health test from critical back to recommended

02953c7

Co-authored-by: felixarntz <[email protected]>

westonruter force-pushed the add/site-health-check-for-od-rest-api branch from 93a51d8 to 02953c7 Compare January 17, 2025 18:41

westonruter requested a review from felixarntz January 17, 2025 18:47

felixarntz reviewed Jan 17, 2025

View reviewed changes

Use definite article instead of possessive

14e9471

Co-authored-by: Felix Arntz <[email protected]>

westonruter requested a review from felixarntz January 18, 2025 01:11

	if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?
	if ( plugin_basename( __FILE__ ) !== $plugin_file ) {

	function embed_optimizer_print_row_meta_install_notice( string $plugin_file ): void {
	$od_plugin_slug = 'optimization-detective';
	$od_plugin_file = "{$od_plugin_slug}/load.php";
	$od_plugin_name = 'Optimization Detective';
	if ( 'embed-optimizer/load.php' === $plugin_file && ! is_plugin_active( $od_plugin_file ) ) {

	if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?
	if ( basename( __DIR__ ) !== dirname( $plugin_file ) ) {

	if ( 'optimization-detective/load.php' !== $plugin_file ) { // TODO: What if a different plugin slug is used?
	if ( basename( __DIR__ ) . '/load.php' !== $plugin_file ) {

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

Are you sure you want to change the base?

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

Conversation

b1ink0 commented Dec 19, 2024 • edited Loading

Summary

Relevant technical choices

Scenarios:

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jan 16, 2025 • edited Loading

westonruter commented Jan 16, 2025

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1ink0 commented Dec 19, 2024 •

edited

Loading

westonruter commented Jan 16, 2025 •

edited

Loading