Not setting hasMixedDisplayContentLoaded and hasMixedDisplayContentBlocked flag in nsMixedContentBlocker.cpp

RESOLVED FIXED in mozilla26

Status

()

Core
Security
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: tanvi, Assigned: ialagenchev)

Tracking

(Blocks: 1 bug)

21 Branch
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 710445 [details] [diff] [review]
Missed setting hasMixedDisplayContentBlocked

We are missing a call to SetHasMixedDisplayContentBlocked in nsMixedContentBlocker.cpp.  Missing this shouldn't really cause any issues today, since we aren't doing much with this flag and it doesn't have a UI impact today.  But, it is supposed to be set when mixed display content is blocked, so here is a patch that does so.
Attachment #710445 - Flags: review?(bugs)
Comment on attachment 710445 [details] [diff] [review]
Missed setting hasMixedDisplayContentBlocked

Can we have a test for this (so that we catch possible regressions)-
Attachment #710445 - Flags: review?(bugs) → review+
(Reporter)

Updated

5 years ago
Duplicate of this bug: 838403
(Reporter)

Comment 3

5 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b5262e61b9af

I need to add a test.
(In reply to Tanvi Vyas [:tanvi] from comment #0)
> We are missing a call to SetHasMixedDisplayContentBlocked in
> nsMixedContentBlocker.cpp.  Missing this shouldn't really cause any issues
> today, since we aren't doing much with this flag and it doesn't have a UI
> impact today.  But, it is supposed to be set when mixed display content is
> blocked, so here is a patch that does so.

This flag is now used in nsSecureBrowserUIImpl to help calculate the UI state. The easiest way to test this may be to have a test if an <img src=x.png>, where the server returns a 404 response for x.png. In that case, I nsMixedContentBlocker will rightly consider the page  to have mixed content, bug the old nsISecureBrowserUIImpl calculations will, I think, wrongly consider the page to be mixed-content free.
(Reporter)

Updated

4 years ago
Assignee: tanvi → alagenchev
Summary: Not setting hasMixedActiveContentLoaded flag in nsMixedContentBlocker.cpp → Not setting hasMixedDisplayContentLoaded flag in nsMixedContentBlocker.cpp
(Reporter)

Comment 5

4 years ago
We forget to set the hasMixedDispalyContentBlocked the following case:

* User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true.
* User visits a page with mixed display content
* The mixed display content load is rejected by nsMixedContetnBlocker.cpp:462, but we never set the hasMixedDisplayContentBlocked flag.

We need a rootDoc->SetHasMixedDisplayContentBlocked(true); at line 465.  The patch does this (but might not apply cleanly because of change to the file).

You can test this by checking the value of hasMixedDisplayContentBlocked, which is exposed in this idl:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#510


Also, this bug has an implication I didn't realize before.  We are under-reporting telemetry for pages with mixed display content because we forgot to set this flag.  However, the percentage of under-reporting is VERY low since it's only for users who gone to about:config and set the pref to block mixed display.  If I had to guess, I would say this would be on the order of 100 users.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#1452
(Reporter)

Comment 6

4 years ago
While looking at this more carefully, I realized we also forget to set hasMixedDisplayContentLoaded() in the following case:

* User has explicitly enabled mixed display content blocking in about:config by setting security.mixed_content.block_display_content to true.
* User visits a page with BOTH mixed active & mixed display content.  The user clicks the shield and "disables protection".
* The mixed display content load is hence accepted by nsMixedContetnBlocker.cpp:456, but we never set the hasMixedDisplayContentLoaded flag.


Fix:
We need a rootDoc->SetHasMixedDisplayContentLoaded(true); at line 459.

Implications: As in the previous case, there are no UI implications to missing this flag.  Thi is because this is a case where there is both mixed display and mixed active content.  Hence, the UI would look the same as if there were just mixed active content anyway (yellow triangle).

Again, there are telemetry implications where we are under-reporting.  But this situation is even more rare than in the previous comment, so I"m not expecting any spikes in the mixed display content telemetry.

Test:
To test this, we can check the value of hasMixedDisplayContentLoaded after the user "disables protection on this page".  This is exposed in this idl:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#503
Summary: Not setting hasMixedDisplayContentLoaded flag in nsMixedContentBlocker.cpp → Not setting hasMixedDisplayContentLoaded and hasMixedDisplayContentBlocked flag in nsMixedContentBlocker.cpp
(Assignee)

Comment 7

4 years ago
Created attachment 795748 [details] [diff] [review]
838396.patch

I am uploading a WIP patch, since I am running into two issues that I can't seem to figure out on my own. The first one is that these checks are failing for some reason after clicking on the doorhanger to allow mixed content:
+  is(browser.docShell.hasMixedDisplayContentLoaded, true, "hasMixedDisplayContentLoaded flag has not been set");
+  is(browser.docShell.hasMixedActiveContentLoaded, true, "hasMixedActiveContentLoaded flag has not been set");

I stepped through the code in nsMixedContentBlocker and the flags are getting set correctly. The checks before clicking on the doorhanger work fine too. 

The second issue is that when putting the tests in browser/devtools/webconsole/test and running them from there, they run fine. When I try to put them in browser/base/content/test then nothing really happens. I've added both files to the Makefile.in and did mach build.

Can I please get some quick pointers about what's going on here? I asked about the first issue on #developers without success.
Thanks a lot!
Attachment #795748 - Flags: feedback?(bzbarsky)
So this happens:

>+  notification.secondaryActions[0].callback();

and that starts a new (async) load, right?

But the flags are checked immediately, before that load completes.  Is that the basic problem?

I have no idea what's going on with the test dir issue.
Attachment #795748 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 9

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> So this happens:
> 
> >+  notification.secondaryActions[0].callback();
> 
> and that starts a new (async) load, right?
> 
> But the flags are checked immediately, before that load completes.  Is that
> the basic problem?
Oh yes! I knew it was something simple I was missing. 

> I have no idea what's going on with the test dir issue.
I'll try to ask around on #developers again.

Thanks bz!
(Assignee)

Comment 10

4 years ago
Created attachment 796334 [details] [diff] [review]
838396.patch
Attachment #795748 - Attachment is obsolete: true
Attachment #796334 - Flags: review?(bzbarsky)
Comment on attachment 796334 [details] [diff] [review]
838396.patch

r=me
Attachment #796334 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

4 years ago
Comment on attachment 796334 [details] [diff] [review]
838396.patch

Tanvi asked me to request a review from smaug too, since he's familiar with the MCB code.
Attachment #796334 - Flags: review?(bugs)
Attachment #796334 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

4 years ago
Created attachment 797331 [details] [diff] [review]
838396.patch
Attachment #710445 - Attachment is obsolete: true
Attachment #796334 - Attachment is obsolete: true
Attachment #797331 - Flags: review+
(Assignee)

Comment 14

4 years ago
Created attachment 797332 [details] [diff] [review]
838396.patch
Attachment #797331 - Attachment is obsolete: true
Attachment #797332 - Flags: review+
(Assignee)

Comment 15

4 years ago
tbpl: https://tbpl.mozilla.org/?tree=Try&rev=4bea3029c17b
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e615c2a302
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e3e615c2a302
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.