Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#51547 closed defect (bug) (fixed)

Error checking in site health async cron tasks

Reported by: dd32's profile dd32 Owned by: desrosj's profile desrosj
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.4
Component: Site Health Keywords: has-patch has-dev-note
Focuses: Cc:


The following PHP Notice is generated by the Site Health cron tasks:

Notice: Undefined index: status in wp-admin/includes/class-wp-site-health.php on line 2594

Call Stack:
    0.7033   53046672   3. WP_Hook->do_action() wp-includes/plugin.php:544
    0.7033   53046672   4. WP_Hook->apply_filters() wp-includes/class-wp-hook.php:311
    0.7033   53047800   5. WP_Site_Health->wp_cron_scheduled_check() wp-includes/class-wp-hook.php:287

The $result value that it's processing at the time contains:

  array(3) {
    'code' =>
    string(13) "rest_no_route"
    'message' =>
    string(55) "No route was found matching the URL and request method."
    'data' =>
    array(1) {
      'status' =>

It appears that it happens when the wp-site-health/v1/tests/* REST API endpoints are called from cron.

There's two issues here:

  1. The code assumes that the only error conditional is a WP_Error from a HTTP failure. A wp-json REST API error is not a WP_Error, it's a standard array response.
  2. The Admin Ajax and REST API callbacks as run from cron are not adding any form of authentication, and so, will never work. The nonce attached would be for an anonymous user and as none of these endpoints are available unauthenticated do not pass.

I believe this is a followup to #47606 and it's prominence has only been noticed due to the changed in #48105 triggering these notices.

Attachments (4)

51547.patch (5.6 KB) - added by Clorith 4 years ago.
51547.2.patch (4.2 KB) - added by Clorith 4 years ago.
51547.3.patch (4.2 KB) - added by Clorith 4 years ago.
51547.4.patch (4.1 KB) - added by Clorith 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @Clorith
4 years ago

  • Milestone changed from Awaiting Review to 5.6

Good catch, not sure why I overlooked this. You are correct that it is because the rest endpoint isn't set in this scenario, but it would also fail because of the lack of auth.

Creating a cron-token and setting it as the nonce value would probably be needed here to bypass the need for an active authenticated user session, but shouldn't be a difficult thing to get added.

4 years ago

#2 @Clorith
4 years ago

  • Keywords has-patch added; needs-patch removed

As there's no internal way to authenticate against the REST endpoint, 51547.patch introduces a direct callback route that async tests can register if they want or can.

It also hardens the check which initially alerted us to the problems here by checking if the response is a WP error, and that it gets a non-error response code from the request as well.

This will need a dev note, but it goes along with the REST endpoint dev note in general.

This ticket was mentioned in Slack in #core by clorith. View the logs.

4 years ago

#4 @TimothyBlynJacobs
4 years ago

What are the set_site_status_test_cron_rest_token actions used for?

4 years ago

#5 @Clorith
4 years ago

51547.2.patch Removes the tokens which were left over from an authentication experiment.

It also renames the direct test argument to cover any kind of async test, it doesn't make sense to limit this to only REST endpoints, as admin-ajax may also be set up to require authentication.

#6 @TimothyBlynJacobs
4 years ago

I think async_direct_test should be documented as callable instead of mixed.

4 years ago

#7 @Clorith
4 years ago

Ahh, I'd never heard of callable there, consider it done!

#8 @TimothyBlynJacobs
4 years ago

isset( $test['async_direct_test'] ) && ! empty( $test['async_direct_test'] ) can be consolidated to just ! empty( $test['async_direct_test'] ). Besides that, code approach looks good to me. But I haven't tested manually triggering the cron job.

4 years ago

#9 @Clorith
4 years ago

Picking up all the new tricks this evening I am :) 51547.4.patch consolidates the suggestions above into a nice little package.

#10 @desrosj
4 years ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#11 @desrosj
4 years ago

  • Keywords needs-dev-note added

#12 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 49232:

Site Health: Improve the reliability of asynchronous tests.

This change adds additional logic to catch HTTP failures that do not return a WP_Error object (for example, a wp-json REST API error error).

This also fixes instances where REST API callbacks performed from cron do not work due to a lack of authentication by introducing a direct callback route that asynchronous tests can register.

Props dd32, clorith, timothyblynjacobs.
Fixes #51547.

#13 @SergeyBiryukov
4 years ago

In 49266:

Docs: Add a @since note about async_direct_test key to the site_status_tests filter.

Follow-up to [49232].

See #51547.

This ticket was mentioned in Slack in #core by daisyo. View the logs.

3 years ago

#15 @Clorith
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.