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

Status: RESOLVED WORKSFORME

(Core :: Networking, defect)

Description

[Jason Duell]

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.

Comment 1

[Jason Duell]

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?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't have a strong opinion one way or another

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Anne (:annevk)]

Brian, that logic does not work for what we do with Location.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Anne (:annevk)]

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.

Comment 7

[Jason Duell]

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.

Comment 8

[Jason Duell]

Anne: do we still have something to do here (or in the CORS code) to make us spec-compliant?

Comment 9

[Anne (:annevk)]

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.

Comment 10

[Anne (:annevk)]

Closing this per my own comment to reduce the noise in the number of CORS issues outstanding.