WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 15 months ago

#45933 closed defect (bug)

WSODs protection returns incorrect content type for JSON Requests — at Version 27

Reported by: spacedmonkey Owned by: flixos90
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Site Health Keywords: servehappy has-patch
Focuses: multisite Cc:

Description (last modified by spacedmonkey)

While testing WSODs protection ( #44458 ), I noticed that on REST API calls that wrong content type is returned. The REST API expects valid json to be returned. Other invalid rest requests, return an error state in the following format.

{
    code: "rest_no_route",
    message: "No route was found matching the URL and request method",
    data: {
       status: 404
    }
}

This issue is that wp_die, has go handles for, admin ajax, xmlrpc and html responses. But doesn't check for feed or rest api rests.

Change History (35)

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


21 months ago

@spacedmonkey
21 months ago

#4 @ocean90
21 months ago

Instead of wp_doing_rest() you could use wp_is_json_request() from #44534. See also #42061.

#5 follow-up: @spacedmonkey
21 months ago

  • Keywords has-patch added; needs-patch removed

In 45933.diff I had a first pass at handling REST API requests. However, it proved much much harder than I thought to handle REST requests than I thought. It is extremely hard to workout the request is meant to be REST API. This patch add is_doing_rest, that tries it's best to guess if the current request.

One of the biggest problems I had when testing this, was the fact that rest_api_loaded calls die instead of exit means that, the shutdown handler were never called.

I am looping @rmccue because we need a little history here.

#6 in reply to: ↑ 5 @rmccue
21 months ago

Replying to spacedmonkey:

In 45933.diff I had a first pass at handling REST API requests. However, it proved much much harder than I thought to handle REST requests than I thought. It is extremely hard to workout the request is meant to be REST API. This patch add is_doing_rest, that tries it's best to guess if the current request.

The REST_REQUEST constant is built to answer the question of "is the current top-level request a REST API request?".

The main thing is that knowing whether the current request is a REST request or not is mostly irrelevant, because the REST API is re-entrant. Specifically, REST requests can be dispatched and handled on page loads outside of REST API URLs.

If I do rest_do_request() inside my header template, is the whole request a REST request? Is the request a REST request while rest_do_request() is running? Is the answer different inside embedded requests fired in the server?

Generally speaking, we want people to avoid asking "is the parent code of the current code running in a REST API request?" because this introduces disparity between how WordPress operates for every other request and how it operates for the API. This means you'll inevitably end up with code that doesn't fit with an API-first mentality. Avoiding a way to detect this helps to shape behaviour to avoid this.

One of the biggest problems I had when testing this, was the fact that rest_api_loaded calls die instead of exit means that, the shutdown handler were never called.

die is an alias of exit in PHP, they both fire shutdown hooks, and they both generate a EXIT opcode, so they are equivalent in every way. It's just a stylistic choice as to which to use, not a behavioural one. I'm not sure how you tested this to see disparity between the two.

#7 follow-up: @spacedmonkey
21 months ago

Thanks @rmccue for getting back to me on this one.

The issue here is, that the REST_REQUEST is setup until parse_request filter, this is far late to help for this use case. Take the following use case as example.

  1. User requests posts from rest api - http://build.wordpress-develop.test/wp-json/wp/v2/posts
  2. Plugin activated by users, is hooked in at plugins_loaded
  3. Plugin has fatal error.
  4. Users is returned error message from shutdown controller.

As plugins_loaded is before parse_request, REST_REQUEST isn't setup and we can't work out if request was meant to be a REST API request and a html error is displayed. doing_rest should only be true if the url requested wp-json/wp/v2/posts. So the logic of it should be more like this.

function wp_doing_rest(){
     global $wp;
     if ( isset( $wp ) && $wp->query_vars['rest_route'] ) {
         return true;
     }
     return false. 
}

But this isn't possible as this $wp->add_query_var( 'rest_route' ); isn't called until init.

Not sure how I saw different behaviours on die / exit change, maybe that wasn't the reason why I saw a different and something else was at play here. I don't know.

#8 in reply to: ↑ 7 @rmccue
21 months ago

Replying to spacedmonkey:

The issue here is, that the REST_REQUEST is setup until parse_request filter, this is far late to help for this use case.

WordPress doesn't know that it's a REST API request until WP's routing is done. There's no real way around this; it's simply too early before parse_request to tell what the request is.

In the error handler, you could try detecting the Accept header, and external well-formed requests can send Accept: application/json. Otherwise, I don't think there's really a solution.

#9 @spacedmonkey
21 months ago

Thanks for your feedback @rmccue and @ocean90 . I have modified the patch accordingly.

Allowing changes are in 45933.1.diff.

  1. wp_die uses wp_is_json_request.
  2. _json_wp_die_handler introduced.
  3. _json_wp_die_handler handles WP_Error being passed.
  4. wp_is_json_request now defects jsonp requests (is another bug I found while looking at this).
  5. display_error_template method, now uses WP_Error where possible.

I want to get this right with json responses, before looking at XML / feeds.

#10 @spacedmonkey
21 months ago

Also worth noting you need to apply the patch on #45958 as well, as they related bugs.

This ticket was mentioned in Slack in #core-php by schlessera. View the logs.


21 months ago

#12 @spacedmonkey
21 months ago

Patch 45933.3.diff is feedback from Slack conversion.

cc @schlessera for code review

Let's get this merged now and I will write the same patch for xml return.

#13 @pento
21 months ago

  • Milestone changed from Awaiting Review to 5.1

Adding to 5.1 milestone for consideration.

#14 @flixos90
21 months ago

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

#15 @flixos90
21 months ago

While in #34999 the decision was not to support the REST API specifically in wp_die(), I think this use-case shows that there is a need to support specifically requested content types.

The patch is looking good, we need to double-ensure though the output works in all formats - we should avoid relying on specific HTML markup here as much as possible.

@flixos90
21 months ago

#16 @flixos90
21 months ago

45933.4.diff simplifies the previous patches a bit while following the same approach. Most importantly it includes these changes:

  • Get rid of supporting WP_Error objects in the JSON wp_die() handler, and thus simplify its code significantly. Only the default handler supports them, so it is not actually safe passing WP_Error objects, and in our case does not provide any benefits.
  • Instead of tying in with the back_link argument of wp_die(), rely on new arguments link_url and link_text. Our use-case does not actually use a "back link", so using that naming would be confusing. Furthermore, with new arguments there is no risk whatsoever about backward-compatibility breaks.
  • Simplify the logic in WP_Shutdown_Handler a bit, with a new display_default_error_template() making it easy to override individual pieces. Since wp_die() is in wp-includes/functions.php, a new check is in place, preventing a potential fatal error early in the loading process. Filters to modify the wp_die() parameters are introduced with wp_php_error_message and wp_php_error_args.

#17 @flixos90
21 months ago

In 44624:

Bootstrap/Load: Fix workaround to display admin link in PHP error template by introducing $link_url and $link_text arguments to wp_die().

This changeset removes the hack that was used before to display more complex HTML markup than a simple message in the default PHP error template via wp_die(). By removing HTML markup from the arguments passed to wp_die() it furthermore paves the way for supporting other content types than the default.

The message and arguments can be modified with new wp_php_error_message and wp_php_error_args filters respectively.

Furthermore this changeset fixes a few issues of functions not existing which could potentially have caused fatal errors when executed early in the WordPress bootstrap process.

Props flixos90, spacedmonkey.
See #45933, #44458.

#18 @flixos90
21 months ago

In 44625:

Bootstrap/Load: Add support for JSON requests to wp_die().

In addition to AJAX and XML-RPC requests, wp_die() now handles JSON requests correctly, returning information in the expected content type.

Props spacedmonkey.
See #45933, #44458.

#19 @flixos90
21 months ago

  • Keywords needs-patch added; has-patch removed

@spacedmonkey Now that these changes are committed, feel free to proceed with a wp_die() handler for RSS feeds. Thanks for tackling this!

#20 @spacedmonkey
21 months ago

There patch as merge has a number issues. These include,

  • No handling of WP_Error. This may generate fatal errors, so I am going to action this next.
  • No support for jsonp, which the REST API has.
  • No XML handling.

I am unhappy with the state of the current commit, so I am going to work on this and get us in a better place. But the fatal errors will need to fixed before 5.1 release or a revert will be needed.

@spacedmonkey
21 months ago

This patch returns support for jsonp

@spacedmonkey
21 months ago

In this patch, a new function called _wp_die_process_input add support for WP_Error and formats the response.

@spacedmonkey
21 months ago

In this patch I had done a first pass at xml / feed wp_die handler.

#21 @spacedmonkey
21 months ago

  • Keywords needs-testing added

I have broken these 3 issues into 3 different diff files, so parts can be merged without holding other parts.

  1. 45933.5.diff simply add jsonp support to response. This stop invalid json being returned for those using the REST API.
  2. 45933.6.diff add a new helper function that process values inputted into wp_die and gets values of WP_error and returns formatted array. This means patch makes xmlrpc, json, and admin-ajax all allow wp_error object to be passed to them.
  3. 45933.7.diff Is a first pass at xml wp die handler. It is very similar to json handler but with some special code to output as xml. I couldn't find a risk free way of using simplexmlelement class, so I wrote a little helper function called _wp_wrap_xml. It would be best if 45933.7.diff goes in, that 45933.6.diff goes in as well, as they are designed to work together.

Looping in @flixos90 and @schlessera for code review.

#22 @spacedmonkey
21 months ago

Forgot to mention in 45933.6.diff had a couple of other changes.

  1. All call nocache_headers();, as no error state should be cached and brings inline with default.
  2. All the wp_die handlers respect status passed in the WP_Error object.

#23 @spacedmonkey
21 months ago

In 45933.8.diff updates 45933.7.diff to check if is_feed or is_trackback return true.

#24 @flixos90
21 months ago

Thanks for following up on this @spacedmonkey!

Since those are three different patches (for a good reason), they should preferably be on individual tickets, to make the conversation and review easier. I think 45933.7.diff / 45933.8.diff respectively should remain here as they tackle the original problem this ticket points out (we still need to add RSS feed support, which those patches are intended for).

I suggest creating separate tickets for both 45933.5.diff and 45933.6.diff.

Last edited 21 months ago by flixos90 (previous) (diff)

#25 @spacedmonkey
21 months ago

@flixos90 I agree, we can break the JSONP into another ticket as this is a different bug. However, the other two pieces should say on this ticket. The WP_Error handling fixes an issue created the last commit and the fix should stay original ticket for histical reasons. The WP_error processing and xml handler are so heavy coupled it took me 30 minutes to separate them when making the patch. It is going to confuse the matter by breaking this up into two ticket. But anyone feels strongly about the matter, they can create the tickets.

#26 @birgire
21 months ago

I had a quick look at wp_is_xml_request() in 45933.8.diff and wonder about this part:

if ( isset( $_SERVER['CONTENT_TYPE'] ) && 'application/json' === $_SERVER['CONTENT_TYPE'] ) {
    if ( isset( $_SERVER['CONTENT_TYPE'] ) && in_array( $_SERVER['CONTENT_TYPE'], $accepted, true ) ) {
        return true;
    }
}

Here isset( $_SERVER['CONTENT_TYPE'] ) is checked twice and 'application/json' is not part of $accepted:

$accepted = array(
    'application/rss+xml',
    'application/rss+xml',
    'text/xml',
    'application/atom+xml',
    'application/rdf+xml',
);

where the 'application/rss+xml' is duplicated in $accepted.

#27 @spacedmonkey
21 months ago

  • Description modified (diff)
  • Summary changed from WSODs protection returns incorrect content type to WSODs protection returns incorrect content type for JSON Requests
Note: See TracTickets for help on using tickets.