Closed Bug 1420680 Opened 7 years ago Closed 6 years ago

Loading woff fonts triggers bogus mixed content warnings in different tab's console (Stylo)

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

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)

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)
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
See Also: → 1426207
Summary: Loading woff fonts triggers bogus mixed content warnings in different tab → Loading woff fonts triggers bogus mixed content warnings in different tab (Stylo)
Whiteboard: bug 1426207 has attached testcases
Summary: Loading woff fonts triggers bogus mixed content warnings in different tab (Stylo) → Loading woff fonts triggers bogus mixed content warnings in different tab's console (Stylo)
Is this a bug in the mixed content blocker or just a bug in the devtools console attributing error messages to the wrong tab?
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...)
Blocks: 1376964
OK, so bug 1384741 only buffers the CSP check result, it doesn't do anything about mixed content blocker :/
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.
Depends on: 1440561
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
This seems like a pretty serious hindrance to our "https everywhere" especially given comment 8.
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.
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
Flags: needinfo?(bugs)
I told Xidorn I'd take a look at this.
Flags: needinfo?(emilio)
Ok, I think I got something that should work.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Flags: needinfo?(bugs)
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 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 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 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 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?
Attachment #8961834 - Flags: review?(bzbarsky) → review-
(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.
Flags: needinfo?(bzbarsky)
> 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?
Flags: needinfo?(bzbarsky) → needinfo?(emilio)
Comment on attachment 8961834 [details]
Bug 1420680: Rework how the loadability of font-faces is computed.

https://reviewboard.mozilla.org/r/230654/#review238254
Attachment #8961834 - Flags: review- → review+
Comment on attachment 8961835 [details]
Bug 1420680: Remove the mechanism to buffer CSP violations.

https://reviewboard.mozilla.org/r/230656/#review238256
Attachment #8961835 - Flags: review?(bzbarsky) → 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?
Attachment #8961834 - Flags: review?(jfkthame) → 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
Attachment #8961848 - Flags: review?(jfkthame) → review+
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
> firefox60 	--- 	affected
> firefox61 	--- 	fixed

Given we get various reports and dupes. Do you want to request an uplift, Emilio?
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.
Flags: needinfo?(emilio) → needinfo?(fbraun)
Attachment #8961834 - Flags: approval-mozilla-beta?
(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.
Flags: needinfo?(fbraun)
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.
Attachment #8961834 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: qe-verify+
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(d.huigens)
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.
Flags: needinfo?(xidorn+moz)
Attached image 2018-05-21.png
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?
Attached image 2018-03-15.png
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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
> On Firefox Developer Edition, also latest

Is this Firefox 61 developer edition?  What exact revision (from about:buildconfig) are you using?
Flags: needinfo?(ba.adeux)
(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.
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
> 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?
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.
OK.  I expect the bluekai.com load is coming from one of the ads on the page, for what it's worth...
This is hitting ESR users. Is there a simple version of this fix we can put on ESR?
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.
(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

(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

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(d.huigens)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: