When an http page includes an https iframe that includes another http iframe then the state in the location bar and the control center don't agree. http://people.mozilla.org/~tvyas/mixedgrandiframe.html

The normal state seems fine: http://i.imgur.com/CSgvP8j.png http://i.imgur.com/KZl7Bdt.png We show that the connection is not secure but also notify the user that we blocked parts of the page. There is a button to unblock active content as well. Not when the user decides to unblock it looks like this: http://i.imgur.com/4AN1aNd.png http://i.imgur.com/iM6SwC3.png The location bar shows the globe for a non-secure connection, the control center however shows the "disabled mixed active content blocking" icon. Tanvi, what's the desired state here? What would we do if there's passive content loaded in the https iframe?

(In reply to Tim Taubert [:ttaubert] from comment #1) > Not when the user decides to unblock it looks like this: *Now when...

I agree that the normal state looks fine. After disable, it would be nice if we matched the globe in the location bar, but not a big deal if we don't. We can wait until 1182551 lands and then handle this in Firefox 43 (or potentially uplift). The mixed passive iframe is a bigger deal, because it shows a lock on an http page (http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html). Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1182551 takes care of this. I just need to add a test. Note that the changes in bug 1182551 may affect the behavior here and could potentially fix this bug too. We could test to check if that is the case.

The platform changes in bug 1182551 don't fix this bug and it is a bigger problem then I imagined before. In the passive case, the control center and url bar don't agree either and you see this: https://people.mozilla.org/~tvyas/lock_icon_on_http_page_panel.png https://people.mozilla.org/~tvyas/lock_icon_on_http_page_subpanel.png Putting the big lock on an http page is bad! We should fix this for Firefox 42.

(In reply to Tanvi Vyas [:tanvi] from comment #4) > The platform changes in bug 1182551 don't fix this bug and it is a bigger > problem then I imagined before. In the passive case, the control center and > url bar don't agree either and you see this: > https://people.mozilla.org/~tvyas/lock_icon_on_http_page_panel.png > https://people.mozilla.org/~tvyas/lock_icon_on_http_page_subpanel.png > > Putting the big lock on an http page is bad! We should fix this for Firefox > 42. Added to the agenda for Monday team meeting to discuss upgrading the priority level of the bug and which iteration development can happen.

[Tracking Requested - why for this release]: See Comment 4

We're going to prioritize this one for 43 and plan on requesting uplift.

Regarding Comment 4 and MCB in an https frame on an http page: It looks like the reason the globe is (correctly) showing up in the URL bar is that we don't set any MCB state for it inside refreshIdentityBlock, but in refreshIdentityPopup we are adding the passive-loaded state to show the "Parts of this page are not secure (such as images)" text. I think we need to add the MCB state attribute on identity-popup-securityView-body at all times to show the description text, but only add it to identity-popup if STATE_IS_BROKEN so that the globe shows up in this case. We also need some test coverage for these cases.

(In reply to Brian Grinstead [:bgrins] from comment #8) > I think we need to add the MCB state attribute on > identity-popup-securityView-body at all times to show the description text, > but only add it to identity-popup if STATE_IS_BROKEN so that the globe shows > up in this case. We also need some test coverage for these cases. I was mistaken about this - identity-popup-securityView-body is the subpanel element. We may need to actually store 'isbroken' as an attribute on identity-popup so we know if we can switch the icon.

(In reply to Brian Grinstead [:bgrins] from comment #8) > We also need some test coverage for these cases. You may be able to extend the this test to check the panel icon as well as the url bar icon: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_mixedContentFramesOnHttp.js

Actually, you could probably just build it into this method, that way every test that uses it will check both the url bar and the control center: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#966

(In reply to Tanvi Vyas [:tanvi] from comment #11) > Actually, you could probably just build it into this method, that way every > test that uses it will check both the url bar and the control center: > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/ > general/head.js#966 Sorry so many comments. I was wrong here. This is only checking the webprogress listener flags. Not actually looking at what the UI is showing.

(In reply to Tanvi Vyas [:tanvi] from comment #12) > (In reply to Tanvi Vyas [:tanvi] from comment #11) > > Actually, you could probably just build it into this method, that way every > > test that uses it will check both the url bar and the control center: > > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/ > > general/head.js#966 > > Sorry so many comments. I was wrong here. This is only checking the > webprogress listener flags. Not actually looking at what the UI is showing. I was thinking of modifying the assertMixedContentBlockingState method to add some checks for things like computed background images / classes, and then also call that method from browser_mixedContentFramesOnHttp.js. I think the MCB coverage has enough callers (including all variations of mixed content being present / active / disabled) that it would cover most states of these icons.

Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi

Tanvi, sorry if the test changes are hard to follow. I couldn't find anywhere else that we were directly testing this stuff so I had to come up with my best approximation of the right states for both the identity box and the CC based on STATE_IDENTITY_EV_TOPLEVEL, STATE_IS_SECURE, STATE_IS_BROKEN, along with the expected MCB state. It's a little tricky to get right because refreshIdentityBlock and refreshIdentityPopup have different flows of logic. I considered adding a new test function to just test identity and CC icons / state, but they are so tied in with MCB that I thought it was best to put it there. Also assertMixedContentBlockingState is already being called from many tests and in many different security environments, so it should have fairly good coverage already. Let me know if you think we should adjust the approach here. The fix itself is to not change to the lock icon if STATE_IS_BROKEN does not exist and MCB is happening. I believe this is accurately finding the problem case (http page with MCB happening in a subframe).

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN https://reviewboard.mozilla.org/r/16425/#review14749 ::: browser/base/content/test/general/head.js:792 (Diff revision 1) > + if (!stateEV && !stateSecure && !stateBroken) { Why not just check for the insecure state? Or check for state_is_insecure in addition? Also, we check for stateEV here but not in line 863. I believe stateEV is included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it also has STATE_IS_SECURE. worth doublechecking). So you might not need stateEV at all. ::: browser/base/content/test/general/head.js:798 (Diff revision 1) > + ok(!classList.contains("mixedActiveContent"), "No MCB icon on HTTP page"); I wanted to confirm that this is only testing the actual icon. There are cases where we would have mixed active content blocked on an http page (http top level, https iframe with http active content - http://people.mozilla.org/~tvyas/mixedgrandiframe.html). We offer the disable button in the control center and we'd like to keep that functionality. The if/else clause on lines 792/803, make it look like a bunch of these tests are skipped for this scenario (state_is_insecure && state_blocked_mixed_active_content). ::: browser/base/content/test/general/head.js:870 (Diff revision 1) > + is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-not-secure.svg\")", Are you sure this is the right icon for stateBroken and activeBlocked? I think you want the grey lock with the yellow triangle in this case. And also in the else if (passiveLoaded) case.

(In reply to Tanvi Vyas [:tanvi] from comment #16) > Comment on attachment 8649544 [details] > MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for > http pages that have https frames with MCB;r=tanvi > > https://reviewboard.mozilla.org/r/16425/#review14749 > > ::: browser/base/content/test/general/head.js:792 > (Diff revision 1) > > + if (!stateEV && !stateSecure && !stateBroken) { > > Why not just check for the insecure state? Or check for state_is_insecure > in addition? > > Also, we check for stateEV here but not in line 863. I believe stateEV is > included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it > also has STATE_IS_SECURE. worth doublechecking). So you might not need > stateEV at all. > > ::: browser/base/content/test/general/head.js:798 > (Diff revision 1) > > + ok(!classList.contains("mixedActiveContent"), "No MCB icon on HTTP page"); > > I wanted to confirm that this is only testing the actual icon. There are > cases where we would have mixed active content blocked on an http page (http > top level, https iframe with http active content - > http://people.mozilla.org/~tvyas/mixedgrandiframe.html). We offer the > disable button in the control center and we'd like to keep that > functionality. Yes, this is just testing the class on the identity box (the element in the URL bar). These classes just control that icon so you will see the globe in the URL bar in the http page regardless of MCB state. The CC will still show the warning and subview controls, and with this patch applied you will also see the globe as the icon in the CC.

(In reply to Tanvi Vyas [:tanvi] from comment #16) > Comment on attachment 8649544 [details] > MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for > http pages that have https frames with MCB;r=tanvi > > https://reviewboard.mozilla.org/r/16425/#review14749 > > ::: browser/base/content/test/general/head.js:792 > (Diff revision 1) > > + if (!stateEV && !stateSecure && !stateBroken) { > > Why not just check for the insecure state? Or check for state_is_insecure > in addition? > > Also, we check for stateEV here but not in line 863. I believe stateEV is > included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it > also has STATE_IS_SECURE. worth doublechecking). So you might not need > stateEV at all. You are right, I added stateInsecure and removed stateEV

(In reply to Tanvi Vyas [:tanvi] from comment #16) > ::: browser/base/content/test/general/head.js:870 > (Diff revision 1) > > + is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-not-secure.svg\")", > > Are you sure this is the right icon for stateBroken and activeBlocked? I > think you want the grey lock with the yellow triangle in this case. And > also in the else if (passiveLoaded) case. This is for the CC, so the yellow triangle isn't part of the icon. I don't think activeBlocked has any special state for the CC icon: * isSecure && activeBlocked shows the green lock: https://people.mozilla.org/~tvyas/mixedcontent.html * isBroken && activeBlocked shows the grey lock, but that's because of ciphers="weak" [0]. For example: https://people.mozilla.org/~tvyas/mixedboth.html. And I think this has uncovered some missing test coverage, because even if I change this assertion to ok(false) it doesn't ever fail in test coverage. Is there somewhere in the tests that should be catching this case -> (isBroken && activeBlocked)? [0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7055-7061

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi

Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi

OK, after discussion I've updated the patch. I took browser_mixedcontent_securityflags.js, converted it to add_task format, and added a case where {activeBlocked: true, passiveLoaded: true} by checking on the page before setting the security.mixed_content.block_display_content pref. I also incorporated changes from the first round of reviews, but this does get complicated indeed so let me know if you see anything else that looks out of place.

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN https://reviewboard.mozilla.org/r/16425/#review14829 ::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:5 (Diff revision 2) > -// on it while the "block mixed content" settings are _on_. > +// and makes sure that mixed content flags have been set correctly. The original text here is not great, so I'm going to attempt to give a better and more elaborate explanation that we can put in the comment instead: The test loads a web page with mixed active and mixed display content and makes sure that the mixed content flags on the docshell are set correctly. * Using default about:config prefs (mixed active blocked, mixed display loaded) we load the page and check the flags. * We change the about:config prefs (mixed active blocked, mixed display blocked), reload the page, and check the flags again. * We override protection so all mixed content can load and check the flags again. ::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:13 (Diff revision 2) > +const PREF_DISPLAY = "security.mixed_content.block_display_content"; You should explicity set what you want the prefs to be set to in this test, as one day the defaults might change (or they might change for a specific build) and we don't want to update every test at that point. ::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:35 (Diff revision 2) > + // Turn "block mixed content" settings on and reload the page Change to "Turn on mixed active and mixed display blocking and reload the page."

https://reviewboard.mozilla.org/r/16425/#review14831 ::: browser/base/content/test/general/head.js (Diff revisions 1 - 2) > - if (activeBlocked || activeLoaded || passiveLoaded) { Why remove this activeBlocked || activeLoaded || passiveLoaded case? ::: browser/base/content/test/general/head.js:895 (Diff revisions 1 - 2) > - } else { > + } else { You could put the is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-degraded.svg\")","CC using degraded icon"); and is(securityContentBG,...) tests here anyway.

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN https://reviewboard.mozilla.org/r/16559/#review14833 Ship It!

Clearing my needinfo because Brian and I discussed this over irc and he has new patches.

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi

(In reply to Tanvi Vyas [:tanvi] from comment #24) > https://reviewboard.mozilla.org/r/16425/#review14831 > > ::: browser/base/content/test/general/head.js > (Diff revisions 1 - 2) > > - if (activeBlocked || activeLoaded || passiveLoaded) { > > Why remove this activeBlocked || activeLoaded || passiveLoaded case? > > ::: browser/base/content/test/general/head.js:895 > (Diff revisions 1 - 2) > > - } else { > > + } else { > > You could put the is(securityViewBG, > "url(\"chrome://browser/skin/controlcenter/conn-degraded.svg\")","CC using > degraded icon"); and is(securityContentBG,...) tests here anyway. Oops, missed these two points before repushing. Will add those two parts next

(In reply to Tanvi Vyas [:tanvi] from comment #24) > https://reviewboard.mozilla.org/r/16425/#review14831 > > ::: browser/base/content/test/general/head.js > (Diff revisions 1 - 2) > > - if (activeBlocked || activeLoaded || passiveLoaded) { > > Why remove this activeBlocked || activeLoaded || passiveLoaded case? Double checked, and if condition has been removed, but it's just been changed to an assertion above: is (classList.contains("mixedContent"), activeBlocked || activeLoaded || passiveLoaded, "identityBox has expected class for mixed content"); This is actually a better check since it will fail on the case where none of those are true but the classList still contains mixedContent

Both patches look good aside from Comment 29. You probably need a browser peer to review too. (In reply to Brian Grinstead [:bgrins] from comment #30) > (In reply to Tanvi Vyas [:tanvi] from comment #24) > > https://reviewboard.mozilla.org/r/16425/#review14831 > > > > ::: browser/base/content/test/general/head.js > > (Diff revisions 1 - 2) > > > - if (activeBlocked || activeLoaded || passiveLoaded) { > > > > Why remove this activeBlocked || activeLoaded || passiveLoaded case? > > Double checked, and if condition has been removed, but it's just been > changed to an assertion above: > > is (classList.contains("mixedContent"), activeBlocked || activeLoaded || > passiveLoaded, > "identityBox has expected class for mixed content"); > > This is actually a better check since it will fail on the case where none of > those are true but the classList still contains mixedContent Will the classList contain mixedContent if mixed passive content is blocked? I don't think it will (since I believe the frontend ignores that case for now), so you should be okay here.

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16559/#review14947 Ship It!

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN https://reviewboard.mozilla.org/r/16425/#review14953 ::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:18 (Diff revision 4) > - SpecialPowers.pushPrefEnv({"set": [["security.mixed_content.block_active_content", true], > - ["security.mixed_content.block_display_content", true]]}, blockMixedContentTest); > + > +registerCleanupFunction(function() { pushPrefEnv is actually preferred nowadays since it does cleanup for you. You could wrap it in a promise if you want and put it in a setup_… task.

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN https://reviewboard.mozilla.org/r/16559/#review14955 Ship It!

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35) > browser/base/content/test/general/browser_mixedcontent_securityflags.js:18 > (Diff revision 4) > > - SpecialPowers.pushPrefEnv({"set": [["security.mixed_content.block_active_content", true], > > - ["security.mixed_content.block_display_content", true]]}, blockMixedContentTest); > > + > > +registerCleanupFunction(function() { > > pushPrefEnv is actually preferred nowadays since it does cleanup for you. > You could wrap it in a promise if you want and put it in a setup_… task. I spent some time looking into this, and it seems that flushPrefEnv isn't called in between the b-c tests. So if I run: ./mach mochitest -f browser browser/base/content/test/general/ --start-at browser_mixedcontent_securityflags.js --end-at browser_no_mcb_on_http_site.js And then log this in browser_no_mcb_on_http_site.js: console.log("Second test", Services.prefs.getBoolPref("security.mixed_content.block_display_content"), Services.prefs.getBoolPref("security.mixed_content.block_active_content")); The prefs haven't been reset! I believe this could be a bug in the existing 20 something usages of pushPrefEnv in browser/: https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser%2F+pushprefenv&redirect=true&case=false&limit=60&offset=0 and we need to file a bug for the test harness to call flushPrefEnv() in between tests. I'd also like if pushPrefEnv would return a Promise by default. In any case, I'll file those bugs but am just going to push it without that function for now since this patch needs to be uplifted and I don't want to track that work also.

https://hg.mozilla.org/integration/fx-team/rev/c61cc794b84c https://hg.mozilla.org/integration/fx-team/rev/c9f61ebf2af5

(In reply to Brian Grinstead [:bgrins] from comment #37) > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #35) > > pushPrefEnv is actually preferred nowadays since it does cleanup for you. > > You could wrap it in a promise if you want and put it in a setup_… task. > > I spent some time looking into this, and it seems that flushPrefEnv isn't > called in between the b-c tests. Filed Bug 1197266

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35) > pushPrefEnv is actually preferred nowadays since it does cleanup for you. > You could wrap it in a promise if you want and put it in a setup_… task. Filed Bug 1197310 for the promise return value

https://hg.mozilla.org/mozilla-central/rev/c61cc794b84c https://hg.mozilla.org/mozilla-central/rev/c9f61ebf2af5

The globe in the location bar matches now with the control center window on: http://people.mozilla.org/~tvyas/mixedgrandiframe.html http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html Verified fixed FF 43.0a1 (2015-08-24) Win 7, Ubuntu 14.04, OS X 10.10.

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN Approval Request Comment [Feature/regressing bug #]: 1175702 [User impact if declined]: The control center shows a lock icon for an http page that includes an iframe with mixed content [Describe test coverage new/current, TreeHerder]: Enhanced existing test coverage for all mixed content browser chrome tests, including a regression test for this case [Risks and why]: Code changes are very small (1 line of JS and a few CSS selector updates in the Control Center) so the risk is mostly limited to changing which icons are visible in the Control Center [String/UUID change made/needed]:

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN Approval Request Comment [Feature/regressing bug #]: 1175702 [User impact if declined]: The control center is adding a weak ciphers attribute when there is passive mixed content but no weak ciphers. This doesn't change the visible icon because the CSS was working around this. But if we need to do future uplifts for 42 campaign it'd be best to have 42 in the same state as 43 for these attributes. [Describe test coverage new/current, TreeHerder]: assertMixedContentBlockingState handles this case, but weak ciphers don't yet have any b-c tests [Risks and why]: Small risk, simple JS and CSS change. The attribute change is limited to the visible icons / messages in the control center [String/UUID change made/needed]:

Comment on attachment 8649544 [details] MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN Patch has been in Nightly for a week. Seems safe for uplift to Aurora42.

Comment on attachment 8650178 [details] MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN Aurora42+

https://hg.mozilla.org/releases/mozilla-aurora/rev/d84ae094ebaf https://hg.mozilla.org/releases/mozilla-aurora/rev/62156e0a139a

(In reply to Paul Silaghi, QA [:pauly] from comment #42) > The globe in the location bar matches now with the control center window on: > http://people.mozilla.org/~tvyas/mixedgrandiframe.html > http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child. > html > Verified fixed FF 43.0a1 (2015-08-24) Win 7, Ubuntu 14.04, OS X 10.10. Verified fixed 42.0a2 (2015-09-02) Win 7