Loading woff fonts triggers bogus mixed content warnings in different tab's console (Stylo)
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
People
(Reporter: d.huigens, Assigned: emilio)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: bug 1426207 has attached testcases)
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
jfkthame
:
review+
jcristau
:
approval-mozilla-beta-
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
46.57 KB,
image/png
|
Details | |
95.72 KB,
image/png
|
Details |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171123161455 Steps to reproduce: 1. In about:config, create an int, dom.ipc.multiOptOut, and set it to 1 2. Open http://www.mediamarkt.nl 3. Open https://www.airbornos.com/ in a different tab and open the console Actual results: A bunch of mixed content warnings show up (all of them show up twice because there's an iframe on https://www.airbornos.com/), e.g.: Blocked loading mixed active content “http://www.mediamarkt.nl/static/fonts/MMTextProWebTT-Bold.woff” Even after you close the first tab and refresh the second tab, or if you open the second url in a private window, they keep showing up. Expected results: The mixed content warnings shouldn't show up, since airbornos.com doesn't load those woff files. Furthermore, if the woff files are actually being attempted to load, and they would load if you opened https://www.mediamarkt.nl (note the https) instead, the paranoid in me thinks this is the kind of thing that could be combined with a woff vulnerability to exploit a third-party domain (but I haven't investigated so this is pure speculation). Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5 (Bug 1330412)
Updated•7 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Too late for 58.
Comment 5•6 years ago
|
||
Is this a bug in the mixed content blocker or just a bug in the devtools console attributing error messages to the wrong tab?
Comment 6•6 years ago
|
||
The mixed content is triggered in this stack: > nsMixedContentBlocker::ShouldLoad() > nsContentPolicy::CheckPolicy() > nsContentPolicy::ShouldLoad() > NS_CheckContentLoadPolicy() > mozilla::dom::FontFaceSet::IsFontLoadAllowed() > mozilla::dom::FontFaceSet::UserFontSet::IsFontLoadAllowed() > gfxUserFontSet::UserFontCache::UpdateAllowedFontSets() > mozilla::ServoStyleSet::PreTraverseSync() So this is from bug 1376964, but I suppose bug 1384741 is meant to fix it. (Also it seems we are maintaining a table containing item for each [font loaded in the past] x [open document]... which doesn't sound quite memory efficient...)
Comment 7•6 years ago
|
||
OK, so bug 1384741 only buffers the CSP check result, it doesn't do anything about mixed content blocker :/
Comment 8•6 years ago
|
||
This doesn't just affect the console. If you have mixed content blocking set to false, when this error happens, pages that are secure appear as insecure.
Comment 10•6 years ago
|
||
Hi Brian, See my last comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1427416#c25 it not only is .woff it also is .ttf related Tx, Pieter
Comment 11•6 years ago
|
||
This seems like a pretty serious hindrance to our "https everywhere" especially given comment 8.
Comment 12•6 years ago
|
||
Yea, I was deploying an *EV cert* to our site yesterday and am *demoing it* showing the certificate info telling that the TLS has been deployed perfectly now without any issues. How displeased was I when clicking on the lock icon - I got "Firefox has blocked parts of this page that are not secure" notification. Thanksalot! "Yea, I suspect it is a bug in my browser... at least we shouldn't be loading any web fonts from online newspapers". 58.0.2 on Ubuntu 17.10.
Comment 13•6 years ago
|
||
Kindly asking Jet to figure out if he can bump the priority of this bug for two reasons: 1) It breaks the lock icon for secure pages, which discourages HTTPS adoption 2) We have 4 duplicates and are likely to get more, now that this is in release
Assignee | ||
Comment 15•6 years ago
|
||
Ok, I think I got something that should work.
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=472c08bef6b188a0b864352850d6bed7a8a5d3f1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review236150 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/thebes/gfxUserFontSet.cpp:621 (Diff revision 2) > - LoadPlatformFont(buffer, bufferLength)) { > + LoadPlatformFont(buffer, bufferLength)) { > - SetLoadState(STATUS_LOADED); > + SetLoadState(STATUS_LOADED); > - Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > + Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > - currSrc.mSourceType + 1); > + currSrc.mSourceType + 1); > - return; > + return; > - } else { > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review236152 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/thebes/gfxUserFontSet.cpp:621 (Diff revision 1) > - LoadPlatformFont(buffer, bufferLength)) { > + LoadPlatformFont(buffer, bufferLength)) { > - SetLoadState(STATUS_LOADED); > + SetLoadState(STATUS_LOADED); > - Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > + Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > - currSrc.mSourceType + 1); > + currSrc.mSourceType + 1); > - return; > + return; > - } else { > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review236350 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/thebes/gfxUserFontSet.cpp:619 (Diff revision 3) > - LoadPlatformFont(buffer, bufferLength)) { > + LoadPlatformFont(buffer, bufferLength)) { > - SetLoadState(STATUS_LOADED); > + SetLoadState(STATUS_LOADED); > - Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > + Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > - currSrc.mSourceType + 1); > + currSrc.mSourceType + 1); > - return; > + return; > - } else { > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review236352 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/thebes/gfxUserFontSet.cpp:619 (Diff revision 4) > - LoadPlatformFont(buffer, bufferLength)) { > + LoadPlatformFont(buffer, bufferLength)) { > - SetLoadState(STATUS_LOADED); > + SetLoadState(STATUS_LOADED); > - Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > + Telemetry::Accumulate(Telemetry::WEBFONT_SRCTYPE, > - currSrc.mSourceType + 1); > + currSrc.mSourceType + 1); > - return; > + return; > - } else { > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review236906 r=me with the comment below on the nsDocument bits. If you meant for me to take a look at other parts as well, please let me know! ::: dom/base/nsDocument.cpp:2401 (Diff revision 4) > SetPrincipal(principal); > } > } > } > > + if (mFontFaceSet) { Why not just do this inside SetPrincipal instead of here? Are there cases where SetPrincipal happens and we do _not_ want to do this?
Assignee | ||
Comment 33•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32) > Comment on attachment 8961834 [details] > Bug 1420680: Rework how the loadability of font-faces is computed. > > https://reviewboard.mozilla.org/r/230654/#review236906 > > r=me with the comment below on the nsDocument bits. If you meant for me to > take a look at other parts as well, please let me know! I assume that was meant to be an r+? :) > ::: dom/base/nsDocument.cpp:2401 > (Diff revision 4) > > SetPrincipal(principal); > > } > > } > > } > > > > + if (mFontFaceSet) { > > Why not just do this inside SetPrincipal instead of here? Are there cases > where SetPrincipal happens and we do _not_ want to do this? Well, SetPrincipal gets called with a bunch of different things like null for an intermediate state. This looked light the right place to add this given it's also the only caller of RefreshCompartmentPrincipal. Is that reasonable? I don't think you can have a font face set before the other callers, but I can indeed just change it to SetPrincipal w/ a null-check. Let me know if you prefer that.
Updated•6 years ago
|
Comment 34•6 years ago
|
||
> I assume that was meant to be an r+? :) Yes, will fix. > Well, SetPrincipal gets called with a bunch of different things like null for an intermediate state. Sure. We'd update any time it's set non-null. > given it's also the only caller of RefreshCompartmentPrincipal That's fair. Can we add asserts at some or all of the other callers that !mFontFaceSet?
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review238254
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8961835 [details] Bug 1420680: Remove the mechanism to buffer CSP violations. https://reviewboard.mozilla.org/r/230656/#review238256
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. https://reviewboard.mozilla.org/r/230654/#review238734 LGTM, thanks for doing this. I think the end result seems easier to understand than the old code (in addition to fixing the current bug). :) ::: layout/style/FontFaceSet.cpp:1367 (Diff revision 4) > + if (ServoStyleSet::IsInServoTraversal()) { > + bool* entry = mAllowedFontLoads.GetValue(&aSrc); > + MOZ_DIAGNOSTIC_ASSERT(entry, "Missed an update?"); > + return entry ? *entry : false; > } > Maybe assert here that we're on the main thread (if we didn't take the above early-return)... I think that should be true, shouldn't it?
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8961848 [details] Bug 1420680: Remove the user font cache generation, which is also unused now. https://reviewboard.mozilla.org/r/230668/#review238736
Comment 39•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/161ecc173b01 Rework how the loadability of font-faces is computed. r=jfkthame,bz https://hg.mozilla.org/integration/mozilla-inbound/rev/f0414698eff1 Remove the mechanism to buffer CSP violations. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a19d50f51c Remove the user font cache generation, which is also unused now. r=jfkthame
Comment 40•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/161ecc173b01 https://hg.mozilla.org/mozilla-central/rev/f0414698eff1 https://hg.mozilla.org/mozilla-central/rev/b9a19d50f51c
Comment 41•6 years ago
|
||
> firefox60 --- affected
> firefox61 --- fixed
Given we get various reports and dupes. Do you want to request an uplift, Emilio?
Assignee | ||
Comment 42•6 years ago
|
||
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. It's non-trivial, but maybe worth doing, yeah... Approval Request Comment [Feature/Bug causing the regression]: bug 1376964 [User impact if declined]: bogus security messages in the console. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: see comment 0 [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: somewhat, but not too much I'd say. [Why is the change risky/not risky?]: It's a somewhat substantive change, but this code path is executed fairly often, and the code is way simpler with this patch than without. Also, this bug has got no regressions reported while it's been on Nightly, while this bug has got a fair amount of dupes reported. [String changes made/needed]: none Freddy, want to add something to the approval request? You may have more context than I.
Comment 44•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #42) > Freddy, want to add something to the approval request? You may have more > context than I. Nah. Duplicates should already be flagged properly.
Comment 45•6 years ago
|
||
Comment on attachment 8961834 [details] Bug 1420680: Rework how the loadability of font-faces is computed. Feels like this can ride to 61, not a new issue and it's getting late for 60.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 46•6 years ago
|
||
I've tried to reproduce the issue on Ubuntu 16.04 x64 and to verify it on the latest Nightly 62.0a1 and on Firefox 61.0b5 following the steps from comment 0 but I didn't succeed. Every time I accessed "http://www.mediamarkt.nl" I've been redirected to "https://www.mediamarkt.nl" and after I opened https://www.airbornos.com/ in a different tab and open the console the only messages displayed are: Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, index.html) main.js:87:2 Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, main.css) main.js:87:2 Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, index.css) main.js:87:2 Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, main.js) main.js:87:2 Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, index.js) main.js:87:2 Airborn is still the same version as last time you opened it. (response_unchanged, fc692adbbf965fca9e362a403fc82dda0be54f0d, lang.json) main.js:87:2 Could you please tell me to what am I doing wrong in order to reproduce and verify this issue? Thanks.
Comment 47•6 years ago
|
||
Looks like the http website here (www.mediamarkt.nl) has been updated to use https by default. With that, you would no longer be able to load font from http from that site, and thus would not get mixed content warning, so probably this bug is no longer reproducible from the original STR. You can probably try looking for other http websites using web font for reproducing this. I don't have any site in my mind immediately.
Comment 48•6 years ago
|
||
I tried again to reproduce and verify this issue using https://web.archive.org and accessed (https://web.archive.org/web/20180315183546/http://www.mediamarkt.nl) and then I followed the steps from the description. I'm not sure if this solution is a good one, I'll attach here the results from the browser console comparing a nightly build from 2018-03-15 and a build from 2018-05-21. Xidorn Quan could you please take a look at the screenshots attached?
Comment 49•6 years ago
|
||
Comment 50•6 years ago
|
||
That's not going to work, because this issue is not very related to what content is served (so archive.org is not really going to help), but about what protocol is the web font served. The website was served from http, and now https-only. archive.org is served from https, so it should basically have the same behavior as https-version of the site. I just had a look at my recent browsing history... and http://www.passiontree.com.au/ seems to be a website which contains web font served from http. You can probably use this website instead of www.mediamarkt.nl to check the behavior change.
Comment 51•6 years ago
|
||
Build ID: 20180517141400 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 Verified as fixed on Firefox Nightly 62.0a1 and on Beta 61.0b6 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Comment 52•6 years ago
|
||
Hello This does not seem as resolved. I have been having this issue for a while now. The mentioned fonts/file path below has AFAIK nothing to do with my website. I have no such fonts directory on my local MAMP installation and certainly not a MAMP directory on the webserver. Here is the console message on latest Firefox version. Blocked loading mixed active content “http://localhost/MAMP/fonts/Ubuntu.woff” On Firefox Developer Edition, also latest, the following console message shows: Loading mixed (insecure) display content “http://tags.bluekai.com/site/39588?&id=9393086f-9be8-4913-bd1a-c5020845a62f” on a secure page Again, AFAIK, my website has nothing to do with 'bluekai'. The website is www.soeezauto.ma, by the way. Thanks Bernard
Comment 53•6 years ago
|
||
> On Firefox Developer Edition, also latest
Is this Firefox 61 developer edition? What exact revision (from about:buildconfig) are you using?
Comment 54•6 years ago
|
||
Is this what you need? Firefox https://hg.mozilla.org/releases/mozilla-release/rev/03d4f76300bedeffd47c726ce7fee0221873da11 FDE https://hg.mozilla.org/releases/mozilla-beta/rev/13924a524f1cbf616195af08388fbe24132e0fdd
Comment 55•6 years ago
|
||
(In reply to ba.adeux from comment #54) > Firefox > https://hg.mozilla.org/releases/mozilla-release/rev/ > 03d4f76300bedeffd47c726ce7fee0221873da11 > > FDE > https://hg.mozilla.org/releases/mozilla-beta/rev/ > 13924a524f1cbf616195af08388fbe24132e0fdd It looks like you are testing Firefox 60.0.1 Release channel and Firefox Developer Edition 61.0b10. This bug should be fixed in the current Developer Edition, so you should see this bug in Firefox 60 Release channel and NOT see the bug in the 61 Developer Edition.
Comment 56•6 years ago
|
||
Hi Chris, I have both browsers updating automatically. So I take what is out there without any filters. I am a bit confused about the releases. Are you saying that both ('regular' Firefox and FDE ) should be clear of this bug on their next releases? When? Thanks Bernard
Comment 57•6 years ago
|
||
> Is this what you need? Yes. So as comment 55 says, the build from "mozilla-release/rev/03d4f76300bedeffd47c726ce7fee0221873da11" predates this fix. The build from "mozilla-beta/rev/13924a524f1cbf616195af08388fbe24132e0fdd" should have this fix. It's not clear from comment 52 which of the messages are appearing in the latter build. It it just the "http://tags.bluekai.com" message?
Comment 58•6 years ago
|
||
The above release of the Developer Edition is showing Loading mixed (insecure) display content “http://tags.bluekai.com/site/39588?&id=9393086f-9be8-4913-bd1a-c5020845a62f” on a secure page So, it would seem that the Ubuntu.woff thing has disappeared on the latest FDE, but we now get the bluekai.com thing.
Comment 59•6 years ago
|
||
OK. I expect the bluekai.com load is coming from one of the ads on the page, for what it's worth...
Comment 63•5 years ago
|
||
This is hitting ESR users. Is there a simple version of this fix we can put on ESR?
Assignee | ||
Comment 64•5 years ago
|
||
I don't think trivially, no... We could try to apply the fix and if it applies cleanly it may be worth uplifting this somehow, the patch has been around for long enough that it should be safe enough. Just let me know.
Comment 65•5 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #63) > This is hitting ESR users. Is there a simple version of this fix we can put > on ESR? Setting esr64=affected flag
Comment 66•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #65)
(In reply to Mike Kaply [:mkaply] from comment #63)
This is hitting ESR users. Is there a simple version of this fix we can put
on ESR?Setting esr64=affected flag
Hello,
Do you know when it will be fixed on the ESR version ?
Will it be fixed in ESR 60.5.0 ?
Thank you very much for your answer
Comment 67•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #64)
I don't think trivially, no... We could try to apply the fix and if it
applies cleanly it may be worth uplifting this somehow, the patch has been
around for long enough that it should be safe enough. Just let me know.
Emilio, does this bug actually break web content or does it just cause spurious console warnings?
I'm just wondering whether we really need to uplift the fix to ESR or not. ESR will update from 60 to 68 in July 2019.
Comment 68•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #67)
Emilio, does this bug actually break web content or does it just cause spurious console warnings?
The latter. It doesn't break anything which shouldn't be broken.
Comment 69•5 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+11 from comment #68)
The latter. It doesn't break anything which shouldn't be broken.
Thanks. In that case, I don't see any urgency to uplift a fix to ESR for a bug that is not a security or webcompat issue.
Updated•5 years ago
|
Reporter | ||
Updated•2 years ago
|
Description
•