Implement notifications for mixed content blocker

RESOLVED FIXED in seamonkey2.19

Status

SeaMonkey
Security
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.19
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 714987 [details] [diff] [review]
Proposed patch

This builds on the existing notifications added in bug 817441.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #714987 - Flags: review?(iann_bugzilla)

Comment 2

4 years ago
> +++ 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
(Assignee)

Comment 3

4 years ago
(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?
(Assignee)

Comment 4

4 years ago
I seem to have a number of spurious "var" statements too, I wonder how they got there (should be just priority = this.XXX).
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!
Blocks: 815321
(Assignee)

Comment 6

4 years ago
(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

4 years ago
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=
Attachment #714987 - Flags: review?(iann_bugzilla) → feedback+
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 717570 [details] [diff] [review]
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.
Attachment #714987 - Attachment is obsolete: true
Attachment #717570 - Flags: review?(iann_bugzilla)

Comment 10

4 years ago
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.
Attachment #717570 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 11

4 years ago
(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.
(Assignee)

Comment 12

4 years ago
Created attachment 720340 [details] [diff] [review]
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).
Attachment #717570 - Attachment is obsolete: true
Attachment #720340 - Flags: review?(iann_bugzilla)

Comment 13

4 years ago
>+<!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.
Flags: needinfo?(neil)
(Assignee)

Comment 14

4 years ago
(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
Flags: needinfo?(neil)

Comment 15

4 years ago
Comment on attachment 720340 [details] [diff] [review]
Revised patch

I think my head is full of mush now, r=me
Attachment #720340 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 16

4 years ago
Pushed comm-central changeset cae9e77b3e18.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → seamonkey2.19
(Assignee)

Updated

4 years ago
Blocks: 853268
(Assignee)

Updated

4 years ago
Blocks: 860970

Updated

4 years ago
Version: unspecified → Trunk

Updated

4 years ago
Depends on: 864369

Updated

4 years ago
Blocks: 904189

Updated

3 years ago
Blocks: 958967
You need to log in before you can comment on or make changes to this bug.