Drop Origin & Accept from Access-Control-Allow-Headers value

[sideshowbarker]

This change drops the Origin and Accept header names from the recommended value for the CORS Access-Control-Allow-Headers header. Per the CORS protocol, it’s not necessary or useful to include them.

Details:

Per-spec at https://fetch.spec.whatwg.org/#forbidden-header-name, Origin is a “forbidden header name” set by the browser and that frontend JavaScript code is never allowed to set.

So the value of Access-Control-Allow-Headers isn’t relevant to Origin or in general to other headers set by the browser itself — the browser never ever consults the Access-Control-Allow-Headers value to confirm that it’s OK for the request to include an Origin header.

And per-spec at https://fetch.spec.whatwg.org/#cors-safelisted-request-header, Accept is a “CORS-safelisted request-header”, which means that browsers allow requests to contain the Accept header regardless of whether the Access-Control-Allow-Headers value contains "Accept".＊

So it’s unnecessary for the Access-Control-Allow-Headers to explicitly include Accept. Browsers will not perform a CORS preflight for requests containing an Accept request header.

Related: matrix-org/synapse#10114

Signed-off-by: Michael[tm] Smith <[email protected]>

---

＊ There is actually one edge case in which browsers won’t allow an Accept header in a request: If the value contains a “CORS-unsafe request-header byte” https://fetch.spec.whatwg.org/#cors-unsafe-request-header-byte. But in that case, the Accept header is anyway malformed and is therefore not going to have its intended effect. So that edge case doesn’t merit including Accept in the Access-Control-Allow-Headers value.

[richvdh]

sounds plausible to me.

For the record, this was added in #1365, based on Synapse's implementation. The presence of Accept and Origin there dates way back to the first git commit of Synapse.

However, I'm going to leave this for a review from someone who knows more about CORS headers than me.

We should also ask ourselves whether this requires an MSC, but given it's just a recommendation, my inclination is to say not.

@sideshowbarker: do you know if this change has been implemented in any server implementations? I'd be happy to see a PR for Synapse.

[sideshowbarker]

@sideshowbarker: do you know if this change has been implemented in any server implementations?

It’s not been — I started just with raising it here.

I'd be happy to see a PR for Synapse.

OK, raised matrix-org/synapse#10114

[turt2live]

I don't think this sort of change needs an MSC by the way: if it becomes a problem then we can MSC it, but for the time being it's a clarification-style change.

[turt2live]

Thanks!