WordPress.org

Make WordPress Core

Opened 6 weeks ago

Closed 6 weeks ago

#40787 closed defect (bug) (fixed)

REST API: Attempts to output headers without confirming headers have not been sent

Reported by: kraftbj Owned by: jnylen0
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch dev-feedback
Focuses: rest-api Cc:

Description

The rest_handle_deprecated_function and rest_handle_deprecated_argument functions fire header() without confirming if headers have already been sent or not.

This can result in PHP warnings, which depending on the site's settings, will output into the REST API response, which breaks JSON.

Error: Warning: Cannot modify header information - headers already sent by (output started at /public_html//wp-includes/functions.php:3720) in /public_html/wp-includes/rest-api.php on line 492

Attachments (1)

40787.diff (782 bytes) - added by kraftbj 6 weeks ago.
Check if headers_sent is false before firing header

Download all attachments as: .zip

Change History (10)

@kraftbj
6 weeks ago

Check if headers_sent is false before firing header

#1 @kraftbj
6 weeks ago

  • Keywords has-patch added

In 40787.diff, I opted to check for headers_sent right before header() instead of earlier in the file in the event that the functions are expanded to do more than only the headers in the future.

#2 @jnylen0
6 weeks ago

  • Milestone changed from Awaiting Review to 4.8
  • Owner set to jnylen0
  • Status changed from new to assigned

Excellent catch, this has bitten me a large number of times and I'm not sure why I didn't think to patch it. I think we should get this in 4.8.

#3 @matt
6 weeks ago

Fine with this coming in if it's before beta 2.

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


6 weeks ago

#5 @kadamwhite
6 weeks ago

+1 from me, nice catch indeed. No strong opinion on wrapping each individual header() call vs making one check earlier in the file.

#6 @dd32
6 weeks ago

What scenario's would these headers be output, after the output has started in a rest context? Is it embedded responses? It's not clear how one would reproduce this other than outputting something before the rest api does.

#7 @jnylen0
6 weeks ago

  • Keywords dev-feedback added

There are various filters that run after the API sends its output. One example from WP.com used shutdown I think. This was pretty annoying to work around.

I would actually​ prefer to remove these headers entirely, because I don't think we should be exposing this information about the site. They were originally introduced during the migration from WP-API v1 to v2 (ref), and they've finished serving that purpose long ago.

Marking as dev-feedback to get a 2nd opinion on just ripping this code out.

#8 @rmccue
6 weeks ago

Makes sense to only send if ! headers_sent()

Deprecated information is still useful for reporting internally-deprecated things (it was intended to be a permanent change), but I'd say we should follow the regular trigger_error behaviour and only send when WP_DEBUG is on.

#9 @jnylen0
6 weeks ago

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

In 40782:

REST API: Do not set X-WP-Deprecated* headers as often.

Only set these headers if (1) WP_DEBUG is enabled and (2) headers have not already been sent.

Previously, this code could generate warnings by trying to set a header after response data has already been sent. This happens when code attached to the shutdown filter calls a deprecated function, for example.

Also, these headers are unlikely to be useful in the majority of cases; let's only send them if WP_DEBUG is enabled.

Props kraftbj, jnylen0, ocean90, rmccue.
Fixes #40787.

Note: See TracTickets for help on using tickets.