836459 - Mixed Active Content Icon doesn't display for a page with both mixed active and display content

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Example: https://people.mozilla.com/~tvyas/mixedcontent4.html

Launch nightly.  Set the pref to disable mixed active content.  Go to https://people.mozilla.com/~tvyas/mixedcontent4.html.  Mixed display content loads, Mixed script does not.  Click the shield and disable protection.  The mixed display and script load, but instead of the triangle icon you see the globe.

This is a regression to bug 822367.  When updating the State with the nsIWebProgressListener flags when a mixed display load is caught here, I do not check if the page has mixed script:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#84

Patch to come shortly.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v1

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v2

Fixing typo.

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=d13cb4368359

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: PSM - Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v1

Also need to update psm code.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 708296 [details] [diff] [review]
PSM - Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v1

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

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +300,5 @@
> +  } else if (docShell->GetHasMixedActiveContentLoaded()) {
> +      *aState = STATE_IS_BROKEN | nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT;
> +  } else if (docShell->GetHasMixedDisplayContentLoaded()) {
> +      *aState = STATE_IS_BROKEN | nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT;
> +  }

Wrap lines <= 80 characeters

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: PSM - Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v2

(In reply to Brian Smith (:bsmith) from comment #4)
> 
> Wrap lines <= 80 characeters

Done.

Carrying over the r+.  Thanks Brian!

Here is an updated push to try: https://tbpl.mozilla.org/?tree=Try&rev=ec8f6126c421

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 708284 [details] [diff] [review]
Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v2

Overlong lines.
eventSink->OnSecurityChange(mContext, (nsIWebProgressListener::STATE_IS_BROKEN |
                                       nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT |
                                       nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT));
should work.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Set both nsWebProgressListener flags for pages that are both mixed script and mixed display v3

Made lines shorter.  Carrying over r+ from smaug.  Thanks!

~Tanvi

Comment 8

[Tanvi Vyas[:tanvi]]

There are a bunch more lines to make shorter (<=80 characters) in nsMixedContentBlocker.cpp.  Will file a followup for that.

Comment 9

[Tanvi Vyas[:tanvi]]

Pushed: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/0cd43265df73
http://hg.mozilla.org/integration/mozilla-inbound/rev/c6a959580748

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6a959580748
https://hg.mozilla.org/mozilla-central/rev/0cd43265df73

Comment 11

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20130220 Firefox/21.0
Mozilla/5.0 (Windows NT 6.2; rv:22.0) Gecko/20130220 Firefox/22.0

Verified on Windows 8, Mac 10.7, Ubuntu 12.10 on latest Aurora and Nightly.

https://people.mozilla.com/~tvyas/mixedcontent4.html

Looks good:
-both prefs enabled =>> neither script or image is loaded, lock icon
-both prefs disabled =>> both script and image are loaded, globe icon
-active content blocking enabled and display content disabled =>> image displayed, doorhanger displayed, globe icon
-mixed content blocking enabled and active content disabled =>> image not loaded, script loaded, globe icon

When disabling protection, the triangle icon is displayed.

Comment 12

[Virgil Dicu [:virgil] [QA]]

One adjacent question, though: should mixed display content be loaded as well when overriding the active script block?

1. Enable both mixed content blocks in about:config
2. Load https://people.mozilla.com/~tvyas/mixedcontent4.html
3. Click the shield Icon and choose disable protection on this page.

Both image and script are loaded. However, on a simple html page with mixed display content, the user doesn't have the possibility to override the block through the doorhanger.

Comment 13

[Tanvi Vyas[:tanvi]]

Thanks Virgil for qa'ing the mixed content bugs!

(In reply to Virgil Dicu [:virgil] [QA] from comment #12)
> One adjacent question, though: should mixed display content be loaded as
> well when overriding the active script block?
> 
Yes, that is intended.  Here is the rationale...

We don't intend to turn the preferences to block mixed display content on anytime in the near and forseeable future.  But we provide this option to advanced users who want to disable http mixed display loads on https pages.  If a user does decide to block these loads, we don't have a way for them to unblock on a per page basis (for pages that have mixed display content, but no mixed active content).

If the user also has the preference to block mixed active content (which presumambly they would; if they are concerned about display they are probably concerned about active content), they will have an option to "disable protection" and unblock on a per page basis.  When they decide to unblock, both mixed active and mixed display content will be unblocked.  If they are already unblocking mixed active content, they are already at risk and adding the less-risky mixed display content is not their biggest problem.

I should probably include this information in the to-be-written documentation / blog post.

> 1. Enable both mixed content blocks in about:config
> 2. Load https://people.mozilla.com/~tvyas/mixedcontent4.html
> 3. Click the shield Icon and choose disable protection on this page.
> 
> Both image and script are loaded. However, on a simple html page with mixed
> display content, the user doesn't have the possibility to override the block
> through the doorhanger.

Answered above.

Thanks!