Bugzilla

Assignee

Comment 2

•

5 years ago

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)

Mike Taylor [:miketaylr]

Updated

•

5 years ago

URL: https://cesiumjs.org

Comment 3

•

5 years ago

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)

Comment 4

•

5 years ago

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

status-firefox68: --- → affected

status-firefox69: --- → affected

tracking-firefox68: ? → +

Updated

•

5 years ago

status-firefox67: --- → unaffected

status-firefox-esr60: --- → unaffected

Assignee

Comment 5

•

5 years ago

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)

Comment 6

•

5 years ago

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)

Assignee

Comment 7

•

5 years ago

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.

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 8

•

5 years ago

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)

Comment 9

•

5 years ago

(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)

Ryan VanderMeulen [:RyanVM]

Comment 10

•

5 years ago

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)

https://hg.mozilla.org/mozilla-central/rev/96a0d9fa805a

Assignee

Comment 11

•

5 years ago

Attached file Bug 1559795 - drop check for header length under 128 chars in nsContentUtils::IsCORSSafelistedRequestHeader for webcompat; r?ckerschb, r?hsivonen — Details

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

Pulsebot

Comment 12

•

5 years ago

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

Oana Pop-Rus

Comment 13

•

5 years ago

bugherder

Status: UNCONFIRMED → RESOLVED

Closed: 5 years ago

status-firefox69: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 69

BugBot [:suhaib / :marco/ :calixte]

Updated

•

5 years ago

Assignee: nobody → twisniewski

Comment 14

•

5 years ago

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

status-firefox-esr68: --- → affected

Flags: needinfo?(twisniewski)

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 15

•

5 years ago

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)

Comment 16

•

5 years ago

(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)

Comment 17

•

5 years ago

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).

Omar Shehata

Reporter

Comment 18

•

5 years ago

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.

Assignee

Comment 19

•

5 years ago

•

Edited

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)

Assignee

Comment 20

•

5 years ago

@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)

Comment 21

•

5 years ago

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)

Daniel Veditz [:dveditz]

Comment 22

•

5 years ago

(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

tracking-firefox-esr68: --- → 68+

Flags: needinfo?(dveditz)

Comment 23

•

5 years ago

(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.

Comment 24

•

5 years ago

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.

Comment 25

•

5 years ago

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

Flags: needinfo?(twisniewski)

Updated

•

5 years ago

Component: Untriaged → DOM: Core & HTML

Product: Firefox → Core

Target Milestone: Firefox 69 → mozilla69

Comment 26

•

5 years ago

(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.

Assignee

Comment 27

•

5 years ago

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)

Comment 28

•

5 years ago

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)

Daniel Cicas [:dcicas], Release QA

Assignee

Comment 29

•

5 years ago

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)

Kohei Yoshino [:kohei]

Updated

•

5 years ago

Keywords: site-compat

Updated

•

5 years ago

Flags: qe-verify+