842191 - Implement notifications for mixed content blocker

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[[e-mail]]

Bug 62178 implemented a mechanism to report or block mixed content on secure sites that provides more detail than the previous set of notifications.

Comment 1

[[e-mail]]

Attachment: Proposed patch

This builds on the existing notifications added in bug 817441.

Comment 2

[Philip Chee]

> +++ b/suite/browser/browser-prefs.js
Could have a pref/link like this (in a followup bug):
+pref("browser.mixedcontent.warning.infoURL", "http://support.mozilla.org/1/%APP%/%VERSION%/%OS%/%LOCALE%/mixed-content/");
ref: https://hg.mozilla.org/mozilla-central/rev/a74d6901fd71#l1.12
We can probably copy and adapt content at Bug 822373 (Learn More pages for Mixed Content Blocker)

We already have a geolocation link pointing to somewhere on seamonkey-projects.org

> +            if (aState & nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT &&
> +                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
Shouldn't this be "security.warn_mixed_active_content" with a period between security and warn?

> +            } else if (aState & nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT &&
> +                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
Ditto

Comment 3

[[e-mail]]

(In reply to Philip Chee from comment #2)
> Could have a pref/link like this (in a followup bug):
> +pref("browser.mixedcontent.warning.infoURL",
> "http://support.mozilla.org/1/%APP%/%VERSION%/%OS%/%LOCALE%/mixed-content/");
> ref: https://hg.mozilla.org/mozilla-central/rev/a74d6901fd71#l1.12
> We can probably copy and adapt content at Bug 822373 (Learn More pages for
> Mixed Content Blocker)
Or we could just use Help instead, we need a followup bug for that anyway.

> > +            if (aState & nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT &&
> > +                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
> Shouldn't this be "security.warn_mixed_active_content" with a period between
> security and warn?
Oops, how did I miss that?

Comment 4

[[e-mail]]

I seem to have a number of spurious "var" statements too, I wonder how they got there (should be just priority = this.XXX).

Comment 5

[Tanvi Vyas[:tanvi]]

Hi Neil,

Thank you for your work on this bug!  Have you seen the new Mixed Content Blocker UI?  If you use Firefox 21 (Aurora) or 22 (Nightly) and go to about:config and set security.mixed_content.block_active_content to true you will see a shield icon next to the lock icon.  Clicking on that, you can disable protection and load the mixed content on a per page load basis.  This was implemented in bug 822371 (frontend) and 822367 (backend).

I'm still working on many other bugs, including a bugs that would show users and developers exactly what content is blocked (bugs 837351 and 800293).  The master bug for the mixed content blocker is bug 815321.

I will apply your patch and take a look at how it works.  Thanks!

Comment 6

[[e-mail]]

(In reply to Tanvi Vyas from comment #5)
> Have you seen the new Mixed Content Blocker UI?
No, I use SeaMonkey, not Firefox.

> If you use Firefox 21 (Aurora) or 22 (Nightly) and go to
> about:config and set security.mixed_content.block_active_content to true you
> will see a shield icon next to the lock icon.  Clicking on that, you can
> disable protection and load the mixed content on a per page load basis. 
SeaMonkey already had a basic notification for mixed content "encrypted page contains unencrypted information". This patch simply extends that to support the mixed content blocker functionality.

Comment 7

[Ian Neal]

Comment on attachment 714987 [details] [diff] [review]
Proposed patch

>+++ b/suite/common/bindings/notification.xml

>             var pref = "security.warn_leaving_secure";
>             var message = "EnterInsecureMessage";
>             var priority = this.PRIORITY_WARNING_LOW;

>+            if (aState & nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT &&
>+                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
The _ here has already been mentioned.
>+              pref = "security.warn_mixed_active_content";
>+              message = "MixedActiveContentMessage";
>+              priority = this.PRIORITY_CRITICAL_LOW;
>+            } else if (aState & nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT &&
>+                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
The _ here has already been mentioned.

>+                callback: this.reloadPage.bind(this, nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT | nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE)
Could this line be wrapped?

>+            } else if (aState & nsIWebProgressListener.STATE_LOADED_MIXED_DISPLAY_CONTENT &&
>+                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
The _ here has already been mentioned.

>+              pref = "security.warn_mixed_display_content";
>+              message = "MixedDisplayContentMessage";
>+              var priority = this.PRIORITY_WARNING_LOW;
priority is already set to this as default.

>+            } else if (aState & nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT &&
>+                this.prefs.getBoolPref("security_warn_mixed_active_content")) {
The _ here has already been mentioned.

>+              pref = "security.warn_blocked_display_content";
>+              message = "BlockedDisplayContentMessage";
>+              var priority = this.PRIORITY_INFO_LOW;
The unneeded var has already been mentioned.

I'd liked to test the new patch before I give an r=

Comment 8

[[e-mail]]

(In reply to Ian Neal from comment #7)
> >+              var priority = this.PRIORITY_WARNING_LOW;
> priority is already set to this as default.
Not true, it might have been changed by the basic security notification.

Comment 9

[[e-mail]]

Attachment: Fixed patch

While I was there I tweaked the mixed content notification slightly - now when you click Keep Blocking, it triggers the notification that would have happened, although it's not ideal, and maybe I should just use multiple notifications.

Comment 10

[Ian Neal]

Comment on attachment 717570 [details] [diff] [review]
Fixed patch

The items "Don't load insecure content on encrypted pages" and "Don't load other types of mixed content" do not seem to match "Set SeaMonkey to show a warning and ask permission before:" statement. Either they need rewording or moving. Is it easy to split warnings from permission asking?

I'm also a little confused by the indentation of the checkboxes in the preferences, usually when they are indented they have a dependency on those less indented and are enabled/disabled appropriately. This does not seem to be the case here.

Comment 11

[[e-mail]]

(In reply to Ian Neal from comment #10)
> The items "Don't load insecure content on encrypted pages" and "Don't load
> other types of mixed content" do not seem to match "Set SeaMonkey to show a
> warning and ask permission before:" statement. Either they need rewording or
> moving. Is it easy to split warnings from permission asking?
> 
> I'm also a little confused by the indentation of the checkboxes in the
> preferences, usually when they are indented they have a dependency on those
> less indented and are enabled/disabled appropriately. This does not seem to
> be the case here.

The "content was blocked" checkboxes only make sense if we're using the mixed content blocker (so you're right, they should be disabled if the blocker is disabled). They are indented under the "loading a page that supports encryption" checkboxes because they would replace that notification if they were triggered. Similarly the "content was loaded" notifications would replace the generic mixed content notification if they were enabled. However in theory they don't make sense if the mixed content blocker is enabled, so maybe I should rethink the code.

Comment 12

[[e-mail]]

Attachment: Revised patch

All the checkboxes now have equal value; the warning tells you that the mixed content was present and what happened to it (which is controlled by the other checkbox).

Comment 13

[Ian Neal]

>+<!ENTITY warn.mixedactivecontent            "Warn me when encrypted pages contain insecure content">
>+<!ENTITY warn.mixedactivecontent.accesskey  "W">
>+<!ENTITY block.activecontent                "Don't load insecure content on encrypted pages">
>+<!ENTITY block.activecontent.accesskey      "D">
>+<!ENTITY warn.mixeddisplaycontent           "Warn me when encrypted pages contain other types of mixed content">
>+<!ENTITY warn.mixeddisplaycontent.accesskey "c">
>+<!ENTITY block.displaycontent               "Don't load other types of mixed content on encrypted pages">
>+<!ENTITY block.displaycontent.accesskey     "m">
Do we want to warn when not loading? i.e. should it be a radio between warn, don't load and load without warning.

Comment 14

[[e-mail]]

(In reply to Ian Neal from comment #13)
> Do we want to warn when not loading? i.e. should it be a radio between warn,
> don't load and load without warning.

There are four options:
1. Load the mixed content (and display the current mixed content notification*)
2. Load the mixed content and warn about the type of mixed content that was loaded
3. Block the mixed content and warn about the type of mixed content that was blocked
4. Block the mixed content (and display the secure content notification*)
* as per existing preferences

Comment 15

[Ian Neal]

Comment on attachment 720340 [details] [diff] [review]
Revised patch

I think my head is full of mush now, r=me

Comment 16

[[e-mail]]

Pushed comm-central changeset cae9e77b3e18.