Make WordPress Core

Opened 7 years ago

Last modified 20 months ago

#43428 new enhancement

Improve CORS headers sent to REST Api requests

Reported by: andreiigna's profile andrei.igna Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: rest-api Cc:

Description

Currently some CORS headers are not sent correctly for REST API requests. This doesn't break anything yet, but can be improved for better performance.

The problematic headers are these:

  • Access-Control-Allow-Headers and Access-Control-Allow-Methods need to be sent just to preflight requests, meaning just on requests with OPTIONS method. Now they are sent back on any REST API request, which is unnecessary
  • Access-Control-Allow-Methods doesn't need to list the OPTIONS method. Preflight requests are made to check what's allowed, and OPTIONS requests are allowed by default. Current setup says something like 'I allow myself', as the preflight request is already a OPTIONS request

There's more detailed info here https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS, but the relevant part is this (it doesn't have an anchor for direct link):

(Note: as described below, the actual POST request does not include the Access-Control-Request-* headers; they are needed only for the OPTIONS request.)

The proposed changes are:

  1. Move header Access-Control-Allow-Headers from serve_request https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/class-wp-rest-server.php#L239 to rest_send_cors_headers https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api.php#L536 making it easier to control the CORS headers from only one place, with one hook
  2. Remove OPTIONS keyword from Access-Control-Allow-Methods
  3. Send Access-Control-Allow-Headers and Access-Control-Allow-Methods only on request with OPTIONS method (preflight), as it's the place where they are necessary
  4. While sending the Allow headers, include Access-Control-Max-Age: 600 for caching the preflight requests, thus improving the performance for next requests (less requests between browser & server when requesting same data). Timing can be changed
  5. Optional - Include Content-Disposition in Access-Control-Allow-Headers, enabling direct file upload (requested in other tickets as well)

Why?
To follow recommended specs for CORS and make the REST API work as expected with web apps

All proposed changes were tested in the last few months on a fleet of ~500 websites with different web clients for managing WordPress data. Everything works fine

Change History (6)

#1 @andrei.igna
7 years ago

It always bugs me when things are not done properly and others take that as a reference..

Last edited 7 years ago by andrei.igna (previous) (diff)

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


7 years ago

#3 @schlessera
7 years ago

@andrei.igna Regarding your first point, I agree that this seems to be what the spec is stating.

However, regarding your second point, it looks like OPTIONS is also included in the examples that the spec shows.

#4 @andrei.igna
7 years ago

Yes, there seems to be a small mistake on MDN docs about this. Probably will be updated shortly there too

OPTIONS can be added in Allow response header to let the client know this method is available, but is unnecessary in Access-Control-Allow-Headers

Access-Control-Allow-Headers is a response header found only in OPTIONS requests, so it can't say that it allows itself after the request has been served

#5 @pento
6 years ago

  • Version trunk deleted

#6 @spacedmonkey
20 months ago

  • Focuses performance removed
Note: See TracTickets for help on using tickets.