WSODs protection returns incorrect content type for JSON Requests

[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.

[spacedmonkey]

Related: #34999

[ocean90]

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

[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.

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.

[rmccue]

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.

[spacedmonkey]

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.

[rmccue]

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.

[spacedmonkey]

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.

[spacedmonkey]

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

[spacedmonkey]

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.

[pento]

Adding to 5.1 milestone for consideration.

[flixos90]

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]

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.

[flixos90]

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.

[flixos90]

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.

[flixos90]

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

[spacedmonkey]

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]

This patch returns support for jsonp

[spacedmonkey]

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

[spacedmonkey]

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

[spacedmonkey]

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.

[spacedmonkey]

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.

[spacedmonkey]

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

[flixos90]

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​.

[spacedmonkey]

@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.

[birgire]

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.

[spacedmonkey]

@flixos90 I have create two new tickets and uploaded patches there.

1. #46026 Related to an xml handler for wp_die.
2. #46025 Related to the JSONP bug with the json handler.

I have renamed this ticket to relate to just the JSON handler. The patch 45933.6.diff should be handled here, as it is bug created by this ticket.

[flixos90]

@spacedmonkey Sounds good. Some small issues with 45933.6.diff​:

• The code is rather complicated to follow. If the preparing function already handles $message and $args, it should also handle $title, which also relies on the same process and that preparation is currently hard-coded only in the default wp-die-handler.
• If you pass a WP_Error without status set in $data, the code will currently break, overriding the original $data and thus effectively deleting $data['status'].
• The preparing function must never override $args because it would also erase additional arguments. It should ensure that things like back_link, link_url and link_text are maintained as well.
• The function lacks docs.
• Generally, the function's behave would be much more obvious by doing the following:

  function _wp_die_process_input( $message, $title, $args ) {
      // all the logic here
  
      return array( $message, $title, $args );
  }
  

This way you could write list( $message, $title, $args ) = _wp_die_process_input( $message, $title, $args ), done. Straightforward and no more custom processing required by the individual handlers.

[spacedmonkey]

Thanks for the feedback @flixos90

I have changed some of the stuff in your request but not all.

• There is a lack of docs as the function is still being written, docs are a final stage when design is locked down, not in the patch stage.
• With _wp_die_process_input the idea is create a formatted array, that can be used by _json_wp_die_handler. This format is defined ​here and looks something like this.

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

Going forward, I would like xml handler (see #46026) to be in this same structure but just formatted as xml. So

<error>
    <code>rest_no_route</code>
    <message>No route was found matching the URL and request method</message>
    <data>
       <status>404</status>
    </data>
</error>

Keeping this structure is important, as it means that, if we copy the existing structure from the rest api, then this error message should work with existing code (in gutenberg) that handles REST API errors. It is important to have a defined error state that doesn't change it's format, as monitor tools and error handling will depend on this exact formatting.

I know the format of the data isn't perfect, I don't love the data array for status and title, but it makes it possible for WP_Error to pass a title as well as status, which is an improvement.

[flixos90]

I agree it makes sense to output the JSON data in accordance with how the REST API does it. However, note that wp_is_json_request() is in no way actually related to the REST API.

More importantly though, let's separate preparing the data passed to the function from actually outputting them. The individual handlers are responsible for the output - _wp_die_process_input() should, like its name already states, be responsible for processing the input, not processing it for output. That function is part of the "wp_die() API" (if it can be called that), so its behavior should be in line with the other related functions, which mean consisting of $message, $title, and $args. $args should consist of the existing "response", "back_link", "link_url", "link_text", plus the new "code". It makes sense to just pass through the three wp_die() parameters like that for easily understanding that this function simply prepares arguments.

The individual handlers can then use this data in any way they like, for example the JSON handler would need to output the following:

list( $message, $title, $args ) = _wp_die_process_input( $message, $title, $args );

...

echo wp_json_encode(
    array(
        'code'    => $args['code'],
        'message' => $message,
        'data'    => array(
            'status' => $args['response'],
        ),
    )
);

[spacedmonkey]

In 45933.10.diff I made _wp_die_process_input much more generic. Now it just processes the values are returns an error.

The only thing that is different from the above idea, is I changed the order of the params of the process function and added an array of errors, as it was required.

This is feeling really close now. Feel happy we could merge this soon after a little testing.

[flixos90]

45933.2.diff​ is a modification of 45933.10.diff​ that makes the following changes:

• Make _wp_die_process_input() more predictable and aligned with other functions: You pass in $message, $title, and $args, and you get back $message, $title, and $args.
• Move all input data processing into the new function so that the handlers really only need to worry about the output. For example, the wp_parse_args() call and default values for arguments like text_direction are now handled there as well (note that _ajax_wp_die_handler() still contains its own call to ensure the "irregular" default of a 200 response code). The handler itself is then free to choose which of that information to render.
• Ensure that additional errors each contain their code, message, and data rather than only their message.
• Ensure that JSON output is formatted coherently with REST API errors, with a data key including status instead of status in the root.

This way it is ensured that the function calling wp_die() does not need to know what kind of request is currently being made. Regardless of how/which arguments are passed, they will just work.

[schlessera]

A few observations regarding the latest patch:

• Line 3375: $args['code'] = 500; should be $args['response'] = 500;
• Doing empty() checks on strings means that it is not possible to override a string like $args['title'] to be an empty string. Just as well, doing empty() checks on integers means that it is not possible to override values like $args['code'] to be 0. isset() or array_key_exists() checks would be more explicit and still allow for empty strings and the value 0 as a valid input.
• $args['response'] seems to have both 0 (line 3327) as well as 500 (line 3375) as default values. If it should be 0, then some of the calls will be wrong, as setting of return code 500 is removed in some places by the patch (line 3024, line 3218, line 3257).
• 'code' should be an integer but defaults to an empty string (line 3328).
• 'text_direction' could default to 'ltr' to simplify the code (line 3332).

[flixos90]

Replying to schlessera:
Thanks for the review!

• Doing empty() checks on strings means that it is not possible to override a string like $args['title'] to be an empty string. Just as well, doing empty() checks on integers means that it is not possible to override values like $args['code'] to be 0. isset() or array_key_exists() checks would be more explicit and still allow for empty strings and the value 0 as a valid input.

That is intentional and needed for backward-compatibility. Existing code ensures already that the response code and title are never empty.

• $args['response'] seems to have both 0 (line 3327) as well as 500 (line 3375) as default values. If it should be 0, then some of the calls will be wrong, as setting of return code 500 is removed in some places by the patch (line 3024, line 3218, line 3257).

The value is set to 0 specifically to allow overriding it. For example the WP_Error should only override the value if it wasn't manually set to something already. Since setting the value to 0 is not supported, we can rely on this. If the default value initially was 500, it would be more complicated to determine whether that 500 is the default or was actually passed.

• 'text_direction' could default to 'ltr' to simplify the code (line 3332).

Same here. The logic that sets a default (via is_rtl() check) should only kick in if no value has passed (as in the value is empty). If 'ltr' was the default, this would be indistinguishable.

[flixos90]

45933.12.diff​ addresses the bugs pointed out by @schlessera. It also documents the new $code argument you can pass to wp_die() (currently only used by the JSON handler).

[flixos90]

In 44666:

Bootstrap/Load: Support WP_Error and $args passed to wp_die() consistently in all handlers.

Prior to this change, each wp_die() handler had their own logic for how to parse arguments, causing inconsistencies and even breakage because the arguments possible to pass to wp_die() depended on the request context. Passing a WP_Error as $message for example used to be only support by the default handler, but not the AJAX and XML-RPC handlers.

With the fatal error protection, plus the new wp_die() handlers related to that, improving this support and compatibility has become more significant. Therefore this changeset introduces a private _wp_die_process_input() function that handles all function parameters consistently.

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

[flixos90]

In 44673:

Bootstrap/Load: Fix bug causing AJAX functions to return a 500 when passing a null response to wp_die().

This bug was introduced in [44497].

Props ocean90.
See #45933.

[spacedmonkey]

@flixos90 On last thing for _wp_die_process_input I would check if $args['response'] is an int. Maybe something like this

 if ( empty( $args['response'] ) || !is_int( $args['response'] ) ) {
    $args['response'] = 500;
 }

[spacedmonkey]

Related: #46054

[dd32]

@flixos90 [44666] has introduced a fatal, looks like the $error variable is supposed to be something else

E_ERROR: /wp-includes/functions.php:3377
Uncaught Error: Call to a member function get_error_data() on null in /wp-includes/functions.php:3377

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/functions.php?marks=3377#L3355

[flixos90]

In 44690:

Bootstrap/Load: Fix fatal error when passing a WP_Error to wp_die().

This was introduced in [44466]. Also, this changeset adds tests for _wp_die_process_input() so that this never happens again.

Props dd32.
See #45933.