Only security check Access-Control-Allow-Origin if using CORS

RESOLVED WORKSFORME

Status

()

Core
Networking
RESOLVED WORKSFORME
5 years ago
a year ago

People

(Reporter: jduell, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

We've got some sites (just a handful, but some visible ones--see bug 845789, bug 847080) that are breaking due to their sending multiple Access-Control-Allow-Origin headers (which is illegal for CORS) during regular HTTP replies.

Anne (CORS spec editor) suggests we should only enforce this restraint for actual CORS requests  (bug 845789 comment 11).  

I could go either way here--there seem to be very few sites out there that are breaking, and they are likely to be fixed server-side by the time we roll out a client-side fix here.  But I can't think of any security issue with allowing multiple ACAO headers in non-CORS requests, so there's no logical reason not to fix this.

A fix here could either move the check to XHR, or we could add a flag that turns on/off the existing check in the HTTP channel.
(Reporter)

Comment 1

5 years ago
Jonas, do you have an opinion on whether this needs to be fixed, and if so, who might work on the XHR code to fix it?
Flags: needinfo?(jonas)
I don't have a strong opinion one way or another
Flags: needinfo?(jonas)
The HTTP parser should fold the multiple header field values into a single value, and the CORS code that checks that header should reject it due to syntax violation.

That means that if the receiver of the CORS response isn't the CORS enforcement code, then the response should be accepted.

And, generally all headers should work this way, except Set-Cookie (because you can't fold them), and perhaps except for headers that are processed directly by nsHttpChannel like Content-Length.

Comment 4

5 years ago
Brian, that logic does not work for what we do with Location.
(In reply to Anne (:annevk) from comment #4)
> Brian, that logic does not work for what we do with Location.

Why not? Location is one of the "headers that are processed directly by nsHttpChannel."

Comment 6

5 years ago
Jason, note also that the way CORS is written today, the request does not fail either if Access-Control-Allow-Credentials is malformed in some way (by e.g. occurring multiple times) if that header needs to be checked.
(Reporter)

Comment 7

5 years ago
So we're backing out bug 814117 (it broke a lot of sites that for no good reason send multiple Access-Control-Allow-Origin headers even when they're not replying to a CORS request).

That means for now multiple Access-Control-Allow-Origin headers will be folded into a comma-separated list.  IIRC that means they should be rejected at the CORS level anyway (since we only support one origin, not a list).  i.e. I assume bsmith's suggestion from comment 3 will work.  If not, and for some reason we need to detect if multiple headers were used (instead of one header with multiple values) we can redo the patch from 814117 with a flag that turns it on only for CORS requests.
Summary: Only security check Access-Control-Allow-Origin if using CORS? → Only security check Access-Control-Allow-Origin if using CORS
(Reporter)

Comment 8

4 years ago
Anne: do we still have something to do here (or in the CORS code) to make us spec-compliant?
Flags: needinfo?(annevk)

Comment 9

4 years ago
The current approach sounds okay. Per how we want to expose HTTP headers to JavaScript I don't think we want to fold multiple headers into one. Each header line should be its own entry. But that can be a separate bug I guess.
Flags: needinfo?(annevk)
Whiteboard: [necko-backlog]

Comment 10

a year ago
Closing this per my own comment to reduce the noise in the number of CORS issues outstanding.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.