958967 - Change mixed content warning default from warn_viewing_mixed to warn_mixed_display_content and remove UI for old preference

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #904189 +++

(Quoting rsx11m from bug 904189 comment #2)
> This is confusing. There is the old option for mixed content in the center
> block, "SSL Warnings" - "Viewing a page with encrypted/unencrypted mix"
> generating a red notification bar with a strong message when triggered.
> 
> Then there are the new options in "Mixed Content" - "Warn me when encrypted
> pages contain insecure/other types of mixed content" which prompt a yellow
> notification bar with a less urgent message "does not prevent eavesdropping"
> or a gray bar that content has been blocked. This overrides the red bar of
> the old option.
> 
> Is this a conflict of different approaches or made this way by design?

(Quoting [e-mail] from bug 904189 comment #4)
> People may have turned on the old warning, so I didn't want to remove it.
> This just gives them the option of using the new warnings. I don't know
> whether the STATE_IS_BROKEN flag ensures that one of the
> STATE_LOADED_MIXED_*_CONTENT flags is also set.

Comment 1

[rsx11m]

Attachment: Proposed patch

It has been a while since bug 842191 introduced the new options for mixed-content blocking and the related warnings. While Firefox removed such options with bug 799009 they were retained for SeaMonkey and the old mixed-content warning remained enabled by default.

Given that there were no reports of the new warning system not working properly since it was introduced, and the confusing prefpane offering both options, I'd suggest to switch the default for passive mixed content warnings from the old to the new system while leaving the backend support intact for now.

The rationale for also removing the UI (beyond removing the cause for ambiguity on those options) is to force users who want to use the old system instead to enable it in the Config Editor, thus avoiding getting trapped with an explicitly UI-enabled old warning at the time the UI is removed.

Comment 2

[rsx11m]

Attachment: Screenshots now vs. proposed

Cleaned-up SSL preference pane reflecting the change in defaults.

Note that the main user-facing difference of the two warnings is that the old system uses a red notification bar with stronger wording whereas the new warning shows yellow only. Given the moderate security risk involved with mixed display content this somewhat lower warning level appears appropriate.

You can use https://getsatisfaction.com/mozilla_messaging for testing.

Comment 3

[[e-mail]]

(In reply to rsx11m)
> This is confusing. There is the old option for mixed content in the center
> block, "SSL Warnings" - "Viewing a page with encrypted/unencrypted mix"
> generating a red notification bar with a strong message when triggered.
> 
> Then there are the new options in "Mixed Content" - "Warn me when encrypted
> pages contain insecure/other types of mixed content" which prompt a yellow
> notification bar with a less urgent message "does not prevent eavesdropping"
> or a gray bar that content has been blocked. This overrides the red bar of
> the old option.
> 
> Is this a conflict of different approaches or made this way by design?

The new design allows you to reduce the severity of the mixed content. The old notification doesn't know how severe the content is, so it always displays a red background. If you disable all the mixed content then you should never see the old notification; if you allow all the mixed content and the new notifications then you will see that the new notification for insecure content is also red.

> (Quoting Neil from bug 904189 comment #4)
> > People may have turned on the old warning, so I didn't want to remove it.
Whoops, I forgot the old warning was on by default. We could:
1. Lower the level of the old warning
2. Turn the old pref off
3. Turn the mixed display content warning on
Or some or all of the above.

Unfortunately I am not 100% sure whether the new notifications completely replace the old ones.

Comment 4

[rsx11m]

(In reply to [e-mail] from comment #3)
> 1. Lower the level of the old warning
> 2. Turn the old pref off
> 3. Turn the mixed display content warning on
> Or some or all of the above.

The patch implements #2 and #3. As noted in the last paragraph of comment #1, removing the UI for the old preference should imply #2 of your list. 

> Unfortunately I am not 100% sure whether the new notifications completely
> replace the old ones.

It appears to me that they do. Thus far, I haven't seen a case where the "broken" icon appears but the notification doesn't (with active-content blocking being enabled as is the default). I don't know if anybody has run into such cases, but didn't see any reports on MZ either; thus, my guess is that it's at least marginal enough not to be an issue, if at all.

Comment 5

[rsx11m]

Looking at the classification of mixed content in nsMixedContentBlocker::ShouldLoad() [mozilla/content/base/src/nsMixedContentBlocker.cpp#194] the default for any mixed content is initially set to MixedContentTypes classification = eMixedScript for active content. Only later, it is reduced to classification = eMixedDisplay; for TYPE_IMAGE, TYPE_MEDIA, TYPE_OBJECT_SUBREQUEST, and TYPE_PING. Meaning, any unknown content not in the list would be classified as active content.

Thus, as long as nsIWebProgressListener::STATE_IS_BROKEN implies that the new function is invoked, it should be ensured that either STATE_LOADED_MIXED_ACTIVE_CONTENT or STATE_LOADED_MIXED_DISPLAY_CONTENT is triggered as well, but I couldn't figure out yet where this is called from. Possible other uses of STATE_IS_BROKEN may come from mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, which don't seem to relate to mixed content, though.

Comment 6

[[e-mail]]

(In reply to rsx11m from comment #5)
> Possible other uses of STATE_IS_BROKEN may come from
> mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, which don't
> seem to relate to mixed content, though.
This is actually the code that triggers the old mixed content warning. By this point the mixed content has already loaded. Hopefully this means that the mixed content blocker has already allowed it and set one of the new mixed states.

Comment 7

[rsx11m]

Switching "Don't load" along with the old warning on but leaving the new warnings off does /not/ trigger the "broken" icon or the old warning. Thus, indeed the mixed-content blocker code seems to run before the old code determines the "broken" state.

Comment 8

[rsx11m]

Attachment: Patch with description added (v2)

Looking at the prefpane again, the label for the old option actually added some nice "encrypted/unencrypted" context for the "Mixed Content" block which is now lost. Also, all other blocks have an introductory description.

Relative to attachment 8358967 [details] [diff] [review], this adds a description to the top of the "Mixed Content" group providing some context. It corresponds in style and length to the "SSL Warnings" group.

I'll leave the other patch up for now in case you think that this is overdoing it. As a compromise, a single-line description simply stating "Encrypted pages may contain unencrypted content." might be sufficient and take up less space.

Comment 9

[rsx11m]

Attachment: Updated screenshot (v2)

Comment 10

[Ian Neal]

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

I think I prefer the other patch, but other locales may run out of space for the description.

Comment 11

[rsx11m]

Thanks to bug 868495 the preference pane now expands if more space is needed (the current maximum being Advanced > Mouse Wheel) rather than cutting it off, thus to some extent this concern is covered.

Per my comment #8, shortening that description to just "Encrypted pages may contain unencrypted content." might sufficiently convey what these settings are about and should fit a single line.

Neil: any other suggestion, or just take as is with the matching descriptions?

Comment 12

[rsx11m]

Thanks Neil, so push of attachment 8359774 [details] [diff] [review] for comm-central please.

Comment 13

[Philip Chee]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/bffb1efac1e5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please remove checkin-needed when pushing.