Closed Bug 1045809 Opened 10 years ago Closed 10 years ago

Doorhanger action to re-enable mixed content blocking for a given page

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: geko1702+bugzilla, Assigned: geko1702+bugzilla)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking
11.11 KB, patch
mmc
: review+
Details | Diff | Splinter Review 
It seems there's no way to re-enable mixed content protection for a given page without closing the tab. Reloading with LOAD_FLAGS_NONE doesn't help since mMixedContentChannel in nsDocShell is not cleared. (set when LOAD_FLAGS_ALLOW_MIXED_CONTENT)
The idea is to call SetMixedContentChannel with nullptr (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5783) when reloading with LOAD_FLAGS_DISALLOW_MIXED_CONTENT. Unfortunately there is no room in http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl#77 for a new flag in the first 16 bits and higher bits cannot be used when reloading a document.

P.S.: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9957
Discussed that with Tanvi and I think she actually found a way so you don't have to introduce a new flag. Have a look at this code:
https://bugzilla.mozilla.org/attachment.cgi?id=812349&action=diff#a/mobile/android/chrome/content/browser.js_sec2

In JavaScript, you just would have to set the mixedcontentChannel to null:

> // Set mixedContentChannel to null to re-enable mixed content blocking.
> 1330	            let docShell = browser.webNavigation.QueryInterface(Ci.nsIDocShell);
> 1331	            docShell.mixedContentChannel = null;
See bugs https://bugzilla.mozilla.org/show_bug.cgi?id=921023#c8 and https://bugzilla.mozilla.org/show_bug.cgi?id=902586.  The reason we don't currently have a re-enable protection on desktop is because I don't know where to put the UI for it.  I was waiting for a site identity box revamp instead of trying to stick it in the current site identity box, but I'm not sure when and if that is going to happen.
Depends on: 1043797
Status: NEW → ASSIGNED
Assignee: nobody → gkontaxis
Summary: Add nsIWebNavigation.LOAD_FLAGS_DISALLOW_MIXED_CONTENT and corresponding logic to re-enable mixed content blocking for a given docshell → Doorhanger action to re-enable mixed content blocking for a given page
Attachment #8471822 - Attachment is obsolete: true
Depends on: 1043803
Comment on attachment 8473423 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8473423 [details] [diff] [review]:
-----------------------------------------------------------------

This tiny patch gives us the option of re-enabling mixed content blocking for a page. This is a new feature. (old mixed content doorhanger did not have it so that's why it's not in the other patches that implement the new doorhanger)
Attachment #8473423 - Flags: review?(bmcbride)
Attachment #8473423 - Flags: review?(bmcbride)
Attachment #8473423 - Attachment is obsolete: true
Attachment #8475331 - Flags: review?(adw)
Comment on attachment 8475331 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Spreading out reviews, Jared can get on this.
Attachment #8475331 - Flags: review?(adw) → review?(jaws)
(In reply to gkontaxis from comment #9)
> Created attachment 8476800 [details] [diff] [review]
> mixedContentChannel is set to null and page is reloaded to re-engage mixed
> content blocking

nits
Comment on attachment 8476800 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8476800 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +725,5 @@
>  <!ENTITY mixedContentBlocked2.options "Options">
>  <!ENTITY mixedContentBlocked2.unblock.label "Disable protection for now">
>  <!ENTITY mixedContentBlocked2.unblock.accesskey "D">
> +<!ENTITY mixedContentBlocked2.block.label "Enable protection">
> +<!ENTITY mixedContentBlocked2.block.accesskey "B">

"B" is already used for "Keep Blocking". We should use "E" for the accesskey.
Attachment #8476800 - Flags: review+
Attachment #8475331 - Attachment is obsolete: true
Attachment #8475331 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> "B" is already used for "Keep Blocking". We should use "E" for the accesskey.

Well, these two don't show up at the same time, but it would be better to use "E" anyways.
Attachment #8476800 - Attachment is obsolete: true
(In reply to gkontaxis from comment #13)
> Created attachment 8476981 [details] [diff] [review]
> mixedContentChannel is set to null and page is reloaded to re-engage mixed
> content blocking

We now have a mochitest.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > "B" is already used for "Keep Blocking". We should use "E" for the accesskey.
> 
> Well, these two don't show up at the same time, but it would be better to
> use "E" anyways.

I think there's no 'keep blocking' any more. Anywhere.
Blocks: 1043801
Attachment #8476981 - Attachment is obsolete: true
The icon in the location bar is missing from the tryserver build for Windows,
http://screencast.com/t/lniYAslYx

Since the dropdown only has one choice, why is there a dropdown? The dropdown says "Options" but there is only one option, either "Enable protection" or "Disable protection".

This should be sorted out before landing.
Note, you'll need to get the tests reviewed before you land them.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Since the dropdown only has one choice, why is there a dropdown? The
> dropdown says "Options" but there is only one option, either "Enable
> protection" or "Disable protection".
> 
> This should be sorted out before landing.

That's a question for Philipp, who did the UX work and posted the mockup in bug 1029193.
Flags: needinfo?(philipp)
Comment on attachment 8477007 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8477007 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for writing tests, these are great to have.

::: browser/base/content/test/general/browser_bug1045809.js
@@ +11,5 @@
> +}
> +
> +function test1() {
> +  gTestBrowser.removeEventListener("load", test1, true);
> +  gTestBrowser.addEventListener("load", test2, true);

Please rewrite this test to use add_task() and promises. You can see how add_task and promises are used by looking through other tests and head.js.

@@ +16,5 @@
> +
> +  var notification = PopupNotifications.getNotification("bad-content", gTestBrowser);
> +  ok(notification, "OK: Mixed Content Doorhanger did appear in Test1");
> +  notification.reshow();
> +  ok(PopupNotifications.panel.firstChild._IsMixedContentBlocked(), "OK: Mixed Content is being blocked in Test1");

Is there any better way to find out of mixed content is blocked? If not, it doesn't seem like this method should have an underscore prefix as that would usually mean it is private and not called from outside of the binding/class.

@@ +19,5 @@
> +  notification.reshow();
> +  ok(PopupNotifications.panel.firstChild._IsMixedContentBlocked(), "OK: Mixed Content is being blocked in Test1");
> +
> +  var x = content.document.getElementsByTagName('iframe')[0].contentDocument.getElementById('mixedContentContainer');
> +  ok(x == null, "OK: Mixed Content is NOT to be found in Test1");

Here, and elsewhere, should use is(x, null, "... message ...");
Doing so will improve the reporting when it fails. With ok(), when it fails it won't report back what `x` was, just that the comparison evaluated to false.
Attachment #8477007 - Flags: review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> The icon in the location bar is missing from the tryserver build for Windows,
> http://screencast.com/t/lniYAslYx
> 
> Since the dropdown only has one choice, why is there a dropdown? The
> dropdown says "Options" but there is only one option, either "Enable
> protection" or "Disable protection".
> 
> This should be sorted out before landing.

I tried to match the mockup we have. Perhaps rethinking the presence of the drop-down could be a follow-up bug?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Note, you'll need to get the tests reviewed before you land them.

This bug has a single test bundled in the patch (latest one I uploaded). Could you review that as well?
(In reply to gkontaxis from comment #23)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> > Note, you'll need to get the tests reviewed before you land them.
> 
> This bug has a single test bundled in the patch (latest one I uploaded).
> Could you review that as well?

Yeah, I reviewed it already. See comment #21 :)
Attachment #8477007 - Attachment is obsolete: true
Comment on attachment 8477101 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8477101 [details] [diff] [review]:
-----------------------------------------------------------------

Addressed review comments (rewritten mochitest). I would like to be able to test the internal state of the doorhanger so IsMixedContentBlocked() is necessary. Renaming it would conflict with a bunch of other patches in review so I'll fix it as soon as everything else is ready to land.
Attachment #8477101 - Flags: review?(jaws)
Comment on attachment 8477101 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8477101 [details] [diff] [review]:
-----------------------------------------------------------------

The icon is still missing when the protection is disabled. That doesn't appear to be the fault of this patch, but did you track down where/why that is happening?
Attachment #8477101 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> Comment on attachment 8477101 [details] [diff] [review]
> mixedContentChannel is set to null and page is reloaded to re-engage mixed
> content blocking
> 
> Review of attachment 8477101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The icon is still missing when the protection is disabled. That doesn't
> appear to be the fault of this patch, but did you track down where/why that
> is happening?

Yes. Icon was missing from windows/jar.mn for skin/classic/aero themes. Fixed now in bug 1043803 which is the one that introduces the icon. Thanks!
Attachment #8477101 - Attachment is obsolete: true
Comment on attachment 8477652 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8477652 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over jaws's review in comment 27.
Attachment #8477652 - Flags: review+
Attachment #8477652 - Attachment is obsolete: true
Comment on attachment 8477810 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

Review of attachment 8477810 [details] [diff] [review]:
-----------------------------------------------------------------

comment 27
Attachment #8477810 - Flags: review+
Comment on attachment 8477810 [details] [diff] [review]
mixedContentChannel is set to null and page is reloaded to re-engage mixed content blocking

>+          let docShell = gBrowser.webNavigation.QueryInterface(Ci.nsIDocShell);
>+          docShell.mixedContentChannel = null;
gBrowser.webNavigation is actually gBrowser.docShell.QueryInterface(Ci.nsIWebNavigation)
Thanks Neil, I'll send you a review on a followup bug.
https://hg.mozilla.org/mozilla-central/rev/4e6d8f30c2da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #36)
> Thanks Neil, I'll send you a review on a followup bug.

https://bugzilla.mozilla.org/show_bug.cgi?id=1058086
(In reply to Drew Willcoxon :adw from comment #20)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> > Since the dropdown only has one choice, why is there a dropdown? The
> > dropdown says "Options" but there is only one option, either "Enable
> > protection" or "Disable protection".
> > 
> > This should be sorted out before landing.
> 
> That's a question for Philipp, who did the UX work and posted the mockup in
> bug 1029193.

Hey Jared!

Three reasons:
1) It shouldn't be super easy to disable this stuff.
2) The strings can get quite long in some languages. Hiding it in a dropdown gives us space to be more descriptive here.
2) There might be more options in the future
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: