WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 11 days ago

Last modified 10 days ago

#51547 closed defect (bug) (fixed)

Error checking in site health async cron tasks

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

Description

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' =>
      int(404)
    }

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 12 days ago.
51547.2.patch (4.2 KB) - added by Clorith 12 days ago.
51547.3.patch (4.2 KB) - added by Clorith 12 days ago.
51547.4.patch (4.1 KB) - added by Clorith 12 days ago.

Download all attachments as: .zip

Change History (17)

#1 @Clorith
2 weeks 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.

@Clorith
12 days ago

#2 @Clorith
12 days 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.


12 days ago

#4 @TimothyBlynJacobs
12 days ago

What are the set_site_status_test_cron_rest_token actions used for?

@Clorith
12 days ago

#5 @Clorith
12 days 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
12 days ago

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

@Clorith
12 days ago

#7 @Clorith
12 days ago

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

#8 @TimothyBlynJacobs
12 days 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.

@Clorith
12 days ago

#9 @Clorith
12 days 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
11 days ago

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

#11 @desrosj
11 days ago

  • Keywords needs-dev-note added

#12 @desrosj
11 days 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
10 days 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.

Note: See TracTickets for help on using tickets.