WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 12 days ago

#43691 assigned defect (bug)

class-wp-rest-server sends response body regardless of actual response code or result type

Reported by: matthias.thiel Owned by: TimothyBlynJacobs
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: needs-patch needs-unit-tests
Focuses: rest-api Cc:

Description

The WP Rest Server always echoes the result of wp_json_encode unless there is an error.

See line 410 of class-wp-rest-server.php

echo $result;

A use case where this leads to an undesired behaviour is when using the HTTP Status Code 204 which implies no response body being sent. Returning a rest response with a status code 204 will send a body at the moment.

There are two problems with this behaviour:

  1. A response status 204 should not send any body content
  2. A response data of null (new \WP_REST_Response( null, 204 )) will be sent as the (string) null

Most clients are pretty forgiving and will ignore the request body but MacOS Safari will not complete the request in this situation and will try to parse the null response as JSON. I found the behaviour is also server configuration specific as some configurations will strip the response body automatically and some will not.

Since there is no consistency in treating a 204 response by servers and browsers, I would suggest to stick to the spec (https://tools.ietf.org/search/rfc2616#section-4.3) and not return a response at all.

I can see two ways to achieve this:

  1. Consult the response status to decide whether or not to return a response body, which is spec compliant
  2. Consult the response data and treat null by not sending a response body, which honours the semantic of null.

Attachments (2)

43691.diff (846 bytes) - added by andizer 4 weeks ago.
43691-2.diff (740 bytes) - added by andizer 4 weeks ago.
Remove the 410, 451 types which actually can have a body.

Download all attachments as: .zip

Change History (8)

#1 @pento
6 months ago

  • Version trunk deleted

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


4 weeks ago

#3 @TimothyBlynJacobs
4 weeks ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned
  • Version set to 4.7

There is a related Gutenberg ticket, https://github.com/WordPress/gutenberg/pull/11208

Handling null is probably the safest way to go. It is hard to see a reason why someone would expect the string "null" to appear in the response body when using the null type in PHP and as such is least likely to cause a BC break.

We could also consider adding a _doing_it_wrong if a response body is sent with a 204 response code.

#4 @TimothyBlynJacobs
4 weeks ago

  • Version changed from 4.7 to 4.4

@andizer
4 weeks ago

@andizer
4 weeks ago

Remove the 410, 451 types which actually can have a body.

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


3 weeks ago

#6 @TimothyBlynJacobs
12 days ago

  • Keywords needs-unit-tests added

Thanks for the patch @andizer!

After discussing in #core-restapi, I think it makes sense to drop the _doing_it_wrong if people are sending null without using the 204 response code and instead return null there as well.

Do you want to work on unit tests for this?

Note: See TracTickets for help on using tickets.