Closed Bug 1559795 Opened 4 months ago Closed 4 months ago

Cross-Origin Request Blocked despite CORS header set to allow wildcard

Categories

(Core :: DOM: Core & HTML, defect)

68 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 68+ wontfix
firefox67 --- unaffected
firefox68 + wontfix
firefox69 --- fixed

People

(Reporter: omar.sameh.shehata, Assigned: twisniewski)

References

(Regression, )

Details

(Keywords: regression, site-compat)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

Go to:

https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=3D%20Tiles%20Point%20Cloud.html

Using Firefox 68.0b10 (64-bit)

Actual results:

Assets in our mapping application fail to load from assets.cesium.com. Console error says:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://assets.cesium.com/1/layer.json. (Reason: missing token ‘accept’ in CORS header ‘Access-Control-Allow-Headers’ from CORS preflight channel).

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://assets.cesium.com/1/layer.json. (Reason: CORS request did not succeed).

Expected results:

The point cloud model and satellite imagery assets should load with no errors. This works correctly in 67.0.2 (64-bit) as well as Chrome/Edge.

The options request has the following response header:

access-control-allow-headers: *

Which I would think should allow this request?

[Tracking Requested - why for this release]: Preemptive tracking request, dunno the impact right now.

37:15.99 INFO: Last good revision: ec0b9bfa65646a96e0b3a7da3eb8dd9ce81ef085
37:15.99 INFO: First bad revision: aaedb3306c2e4444ff3b89fff1ea701fcd9ec6b8
37:15.99 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ec0b9bfa65646a96e0b3a7da3eb8dd9ce81ef085&tochange=aaedb3306c2e4444ff3b89fff1ea701fcd9ec6b8

Tom, thoughts? Do we need to back it out and update the spec, or is this something we feel strongly about in terms of interop with other engines?

Flags: needinfo?(twisniewski)
Keywords: regression
Regressed by: 1495880

Anne, based on what Mike mentioned above, it seem to me like we need to support wildcards before this patch is web-compatible (bug 1309358).

Does that seem reasonable to you as well? If so, we should probably just back this out for now.

Flags: needinfo?(twisniewski) → needinfo?(annevk)

Wouldn't bug 1483815 also cause this (affected Fx 65)? Anyway, reverting that or the patch Mike identified is a regression in security. I guess we can do it, but it's not great. If we do it we should increase the priority for wildcards and relanding these patches.

Flags: needinfo?(annevk)

Tentatively tracking, though we're getting close to release for 68 so if we have to do anything it would have to be soon.

After testing a bit, CesiumJS is failing because of the XHR-specific check for header values over 128 characters in length, so it's just my patch that would be affecting it, Anne (and just that single bit of the patch.. Cesium seems fine with the other stricter checks, at least it loads and seems to work for me).

Given that I don't think we've seen any other breakage related to these patches yet, perhaps we ought to just temporarily disable that one XHR-specific check for now and see what happens? Or would you recommend that we change both InternalHeaders::IsSimpleHeader and nsContentUtils::IsCORSSafelistedRequestHeader so they perform their old checks for now (at least until wildcards support is added?)

Flags: needinfo?(annevk)

I chatted with Thomas and he'll take ownership of the decision as he has the more relevant insights. He'll also communicate the follow-up work. Thanks!

Flags: needinfo?(annevk) → needinfo?(twisniewski)

Just so others know, I propose just removing this one check in the code for now: https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#6886-6888

That seems to make Cesium work fine for me. I have a patch ready that seems to be fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e2ba7a22e15dc9ee037ade52b804def424762ef

But I'm on PTO today, and it hasn't been reviewed yet, so I highly doubt it can make today's the soft freeze.

Chris, do you have thoughts on my suggested approach here? I'd like to land the patch, and seek uplift if we feel it's worthwhile. But if you feel it's better to roll back more of the related change than my suggestion above, or that we should try to get bug 1309358 fixed instead (and potentially live with any breakage in the meantime), let's figure that out.

Flags: needinfo?(twisniewski) → needinfo?(ckerschb)

(In reply to Thomas Wisniewski [:twisniewski] from comment #8)

Chris, do you have thoughts on my suggested approach here? I'd like to land the patch, and seek uplift if we feel it's worthwhile. But if you feel it's better to roll back more of the related change than my suggestion above, or that we should try to get bug 1309358 fixed instead (and potentially live with any breakage in the meantime), let's figure that out.

Just chatted with Thomas and we both think it's worth fixing the problem. Thomas will prepare a patch and I and Henri will review to make sure we are not missing anything since we are late in the game here. The patch should be fairly small and I think the risk is kept at a minimum, see preliminary patch here:
https://hg.mozilla.org/try/rev/0bc44e85203a41bbdc8c37a42641fbecbb38ca8e

Ritu, we are in the soft code freeze, but I guess we can still make it, right?
Can you please help us set the right flags?

Flags: needinfo?(ckerschb) → needinfo?(rkothari)

Feel free to land this patch now for 69. For 68, we're already in RC week and it may be too late there.

Flags: needinfo?(rkothari)

drop check for header length under 128 chars in nsContentUtils::IsCORSSafelistedRequestHeader for webcompat

Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96a0d9fa805a
drop check for header length under 128 chars in nsContentUtils::IsCORSSafelistedRequestHeader for webcompat; r=ckerschb,hsivonen
Status: UNCONFIRMED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Assignee: nobody → twisniewski

Should we consider fixing this in a 68 dot release or can it ride the trains to 69?

Flags: needinfo?(twisniewski)

Yes. If we feel Cesium is used enough to justify it, then I would push to at least have the patch in the ESR.

Flags: needinfo?(twisniewski)

(In reply to Thomas Wisniewski [:twisniewski] from comment #15)

Yes. If we feel Cesium is used enough to justify it, then I would push to at least have the patch in the ESR.

Thomas, can you lay out the pros and cons of having the patch in ESR to help folks make a decision?

Dan, I think it's too late for 68 itself, but we should consider it in a dot release - but what do you suggest regarding ESR? Do we push the same dot releases to ESR? If so, then I think this is what we should do.

Flags: needinfo?(twisniewski)
Flags: needinfo?(dveditz)

This looks like a significant regression to me. I understand the security desire to check/limit header values lengths for Simple CORS GET requests, but shouldn't the fallback here be to trigger an OPTIONS request and if the header is listed allow it no matter the length? That's what Chrome does.

Even if we switch Cesium to use the Authorization header, which triggers the OPTIONS request up-front, Firefox still fails because I assume there is now a hard limit on header lengths? My understanding of the spec is that it was updated to limit Simple CORS request headers, not all CORS requests. Servers should still be able to specifically allow long headers through.

Assuming my understanding of the issue is correct, I believe this is far more damaging than currently realized. Any client passing around JWT tokens in an Authorization or other header will be bit by this (because JWTs in general tend to be well over 128 characters).

Thanks for looking into this so quickly! So just to clarify, would 68 be coming out without this fix? How long would it take before the dot release with the fix comes out?

Just to add some context from our side at Cesium, this would be a pretty fatal regression for a most of our users on Firefox. The Cesium ion platform serves about 10 million requests per day on average, and a lot of sites/businesses rely on it. Some of the biggest ones are (1) https://ayvri.com/ which visualizes outdoor sports, wildlife tracking, drone planning, (2) https://nationalmap.gov.au/ the national map of Australia, and (3) Red Bull (https://www.redbullxalps.com/video/full-race-recap-red-bull-x-alps-2019.html) which had a live tracker of this race a few weeks ago but is over now. And there's hundreds of other smaller ones.

For the ESR folks: without this patch, Firefox ESR will not function with CesiumJS (and potentially other sites we have not hit upon yet). That is the risk: web compatibility. As noted in other comments here, multiple sites rely on CesiumJS, so it's a significant web-compat issue.

What the patch does is relax a header-length check that was added to the CORS spec for extra security, but CesiumJS relies on these longer headers. Ideally we would want to fix bug 1309358 instead to resolve this, but this is the simplest fix for now.

The risk for taking the patch is of course that we will not have the extra security this extra spec-check adds, for one more ESR cycle (previous versions of Firefox did not have it).

In my opinion the best case scenario would be for the ESR to eventually accept the fix in bug 1309358 (and the reversion of this patch), in a subsequent security dot release down the line. But this is the least risky fix for web-compat that I am aware of right now.

Flags: needinfo?(twisniewski)

@Omar, we'll have to wait to see whether the ESR folks accept this patch, and then we can begin to guess on a timeline.

@anne, @Chris, do you have thoughts to share with Matthew in comment 17? (I'm not really an expert on the progress of the CORS spec).

Flags: needinfo?(ckerschb)
Flags: needinfo?(annevk)

I'm not sure I follow. My understanding was that we got a wildcard from the server if we created a preflight for the Accept header due to imposing a length limit on its value as per CORS-safelisted request-headers. We don't support wildcarding yet, so the proposal was that we temporarily revert the length limit check on that header (and the 3 like it) so it does not result in a preflight and we do not end up with a wildcard which we do not support. We'll then prioritize implementing wildcard support and imposing the length limit checks as a follow-up for a future release.

The Authorization header cannot be wildcarded and always needs to be listed explicitly, so it should not be affected by anything we do here and should work fine in release of Firefox (past, present, and future). (It also does not have a length limit imposed on it.)

Hope this helps.

Flags: needinfo?(ckerschb)
Flags: needinfo?(annevk)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)

Dan, I think it's too late for 68 itself, but we should consider it in a dot release - but what do you suggest regarding ESR? Do we push the same dot releases to ESR?

Yes: typically anything important enough to cause a point-release for an ESR's base version are also shipped in the ESR itself

Flags: needinfo?(dveditz)

(In reply to Anne (:annevk) from comment #21)

Hope this helps.

Thanks. That does help and I don't know how I missed that Authorization can't be wildcarded. I'm going to continue to see if there are changes we can make on the Cesium server end to address this issue without relying on wildcarding at all.

Matthew, you certainly could and it'd be appreciated. The OPTIONS request contains an Access-Control-Request-Headers header. Its value is a comma-separated list of header names. If you echo this list in an Access-Control-Allow-Headers header value in the response (rather than using *), it should work in Firefox, regardless of release.

Please request uplift to mozilla-release and mozilla-esr68 if you think we should take this in a dot release.

Flags: needinfo?(twisniewski)
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Target Milestone: Firefox 69 → mozilla69

(In reply to Anne (:annevk) from comment #24)

Matthew, you certainly could and it'd be appreciated. The OPTIONS request contains an Access-Control-Request-Headers header. Its value is a comma-separated list of header names. If you echo this list in an Access-Control-Allow-Headers header value in the response (rather than using *), it should work in Firefox, regardless of release.

Since FF 68 came out yesterday, we removed the wildcard Access-Control-Request-Headers and replaced it with an explicit list in order to avoid outages for our users. I'd still like to see wildcard supported, but it looks like that's already on the schedule.

Thanks.

Thanks, Matthew! Are there known instances of CesiumJS that could still be broken after your fix? (I'd like to know whether it's worth uplifting this patch at this stage, as at least the links given Omar in comment #18 seem to work for me on the 68 release).

Flags: needinfo?(matt.amato)

There are no known issues at this time. The fix we put in works with both 67 and 68 (and all non FF browsers).

Flags: needinfo?(matt.amato)

Alright, thanks again. In light of that, I think it's worth waiting to see if there is any other fallout before taking the extra steps to uplift, as it may prove more worthwhile to uplift the wildcard patch instead (and that bug seems to be actively being worked-on right now).

Flags: needinfo?(twisniewski)
Flags: qe-verify+

Just for the record, we've since reverted this quick fix, as wildcard support has landed.

Depends on: 1564175

Sounds like this bug doesn't need to land on Fx68/ESR68 at this point. We'll make a separate decision about bug 1309358, though.

Clearing QE+ flag due to the changes from bug 1564175 and comment 30.

Also performed a check with the current builds 69.0b8 on the available OSs and the mentioned issue appears to not reproduce.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.