WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#40444 closed defect (bug) (fixed)

REST API responses are sent with an empty Last-Modified header.

Reported by: zinigor Owned by: jnylen0
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests needs-testing
Focuses: rest-api Cc:

Description

Earlier the same problem was fixed for places that use nocache_headers function in #23281, but REST API doesn't seem to use nocache_headers.

Steps to reproduce:

  • Use any plugin that utilizes REST API requests, for example, Jetpack. In connected mode Jetpack issues several REST API requests in the dashboard, for example.
  • Open the browser console and look at the response headers.
  • Observe the Last-Modified header that's either empty or has an epoch date depending on the configuration of your server.

This breaks REST API because browsers start caching responses.

Attachments (3)

40444.patch (587 bytes) - added by zinigor 3 months ago.
Replaces manual header output with the nocache_headers function.
40444.2.patch (2.8 KB) - added by zinigor 3 months ago.
New patch that uses send_header and adds a test.
40444.3.diff (2.8 KB) - added by jnylen0 2 months ago.
Rename function; clean up docs/comments; only send empty header if it was already sent before

Download all attachments as: .zip

Change History (15)

@zinigor
3 months ago

Replaces manual header output with the nocache_headers function.

#1 @zinigor
3 months ago

Huge props to @iv3rson76 for debugging and getting to the bottom of this!

#2 @swissspidy
3 months ago

  • Keywords has-patch needs-testing added
  • Version changed from trunk to 4.4

Introduced in [35773].

#3 @rmccue
3 months ago

The current behaviour of calling $this->send_header looks to be correct, as it goes via the REST API Server abstraction rather than just sending the headers. Internally, nocache_headers calls wp_get_nocache_headers() anyway.

However, nocache_headers() also clears the Last-Modified header, which we aren't doing. We can call header_remove or send an empty header per #23021. We should add a new remove_header method and call that (and if that method doesn't exist, call send_header( 'Last-Modified', '' ) for compatibility with existing WP_REST_Server subclasses).

@zinigor
3 months ago

New patch that uses send_header and adds a test.

#4 @zinigor
3 months ago

@rmccue thanks for taking a look! I have added a new patch that makes the flow go through the send_header method of the Server class. Do you think that solution is better?

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


3 months ago

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


2 months ago

@jnylen0
2 months ago

Rename function; clean up docs/comments; only send empty header if it was already sent before

#7 @jnylen0
2 months ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to 4.8
  • Owner set to jnylen0
  • Status changed from new to assigned

I've confirmed this issue on a couple of my test sites.

In 40444.3.diff:

  • In PHP 5.2, only send an empty header if we've already sent the header in question. Mirrors the existing logic from `nocache_headers`.
  • Rename the new function from clear_header to remove_header.
  • Clean up docblocks and comments.

nocache_headers calls @header_remove (note error suppression). I am not sure why it's called like that, but this patch does the same.

While it would be nice to get this in 4.8 for wider testing before release, the timing is pretty tight at this point. I'll milestone it for discussion.

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


2 months ago

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


2 months ago

#10 @zinigor
2 months ago

The new diff looks great, thank you for updating it!

#11 @jnylen0
2 months ago

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

In 40805:

REST API: Avoid sending blank Last-Modified headers with authenticated requests.

This commit adds a new WP_REST_Server#remove_header method and uses it to clear the Last-Modified header when the "no caching" headers are sent (by default for all authenticated REST API requests). This matches the behavior of the nocache_headers function used in other parts of WordPress.

Previously, the REST API would send an empty Last-Modified header in this situation. Under some server and browser configurations, this causes browsers to cache authenticated REST API requests, which is undesirable.

Props iv3rson76, zinigor, rmccue, jnylen0.
Fixes #40444.

#12 @jnylen0
2 months ago

Note, [40805] is the same as 40444.3.diff with the exception that header_remove is called directly without error suppression. This is because nocache_headers suppresses errors when calling header-related functions, but the pre-existing REST API code does not.

Note: See TracTickets for help on using tickets.