Closed Bug 902156 Opened 6 years ago Closed 6 years ago

Persist "disable protection" option for Mixed Content Blocker

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox24 + verified
firefox25 + verified
firefox26 + verified
relnote-firefox --- +

People

(Reporter: tanvi, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: ux-efficiency, Whiteboard: [parity-chrome])

Attachments

(3 files, 9 obsolete files)

3.78 KB, patch
smaug
: review+
tanvi
: feedback+
Details | Diff | Splinter Review
12.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.87 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Right now, when a user "Disables Protection on This Page" and loads Mixed Active Content, the decision is only remembered for a single page load.  There have been requests to persist the decision for longer than this.

Per-session seems too long, especially if the user uses session restore to restore their tabs/session.  In that case, the decision would persist longer than one browsing session.

What we could do is persist the decision on a specific tab while the user is browsing a specific domain.  As soon as they leave the domain, we'd forget about the decision.  But this way, if a user is navigating through a site that has a lot of mixed active content, they can disable protection once, and complete there work on that site without "disabling protection" on every page load.

This may be a little tricky.  Right now, we use a load flag to determine when protection has been disabled.  And we store information in the channel.  On a subsequent page load, the information on the channel is cleared.  Instead of clearing the channel, we need to check if the domain has changed.  If it has, then we can clear the channel, and if it hasn't, then we don't clear the channel.

This could pose a problem if a user changes their mind and wants to re-enable protection.  If a user wants to re-enable protection right now, they just need to refresh the page.  If change to the above proposed behavior, we have to decide to a) add UI so that the user can re-enable protection and clear the mixed content info on the channel OR b) not provide a way for the user to re-enable protection on that tab (the user would have to open a new tab or navigate to a different domain and then back to the one they want to re-enable protection on).

Thoughts on this proposal are welcome.  If someone wants to chip in and contribute code to this bug, that is also welcome!
(In reply to Tanvi Vyas [:tanvi] from comment #0)

Thanks for filing, I keep meaning to file it.

> 
> Per-session seems too long, especially if the user uses session restore to
> restore their tabs/session.  In that case, the decision would persist longer
> than one browsing session.

For Click-to-Play, we have something we call "enable short term" (not the user-facing name), wherein we allow the plugin to run on the entire site until the user either closes the tab or he doesn't interact with the site for 60 minutes. We could take a similar approach for Mixed Content.

> 
> What we could do is persist the decision on a specific tab while the user is
> browsing a specific domain.  As soon as they leave the domain, we'd forget
> about the decision.  But this way, if a user is navigating through a site
> that has a lot of mixed active content, they can disable protection once,
> and complete there work on that site without "disabling protection" on every
> page load.

This might also be a good heuristic.
> 
> 
> This could pose a problem if a user changes their mind and wants to
> re-enable protection.  If a user wants to re-enable protection right now,
> they just need to refresh the page.  If change to the above proposed
> behavior, we have to decide to a) add UI so that the user can re-enable
> protection and clear the mixed content info on the channel OR b) not provide
> a way for the user to re-enable protection on that tab (the user would have
> to open a new tab or navigate to a different domain and then back to the one
> they want to re-enable protection on).

Yeah, we deal with this with CtP also, and there are some notable edge cases. For example: If you have the same page open on two tabs, and you enable mixed content on one, how/when will the second tab respond?

In general though, the UI work would be fairly straightforward. You would just need to be able to toggle between the two different states. This would also mean a persistent icon :p But I think the Firefox team is slowly warming up to that idea. It's certainly how Mixed Content is designed.

I think the user concern is legitimate enough that we should try to implement this though.
BTW, persisting for the session is covered by bug 860712.

I think we should at least do what Chrome does. They seem to persist for the tab+domain combo.
Keywords: ux-efficiency
See Also: → 860712
Whiteboard: [parity-chrome]
(In reply to Matthew N. [:MattN] from comment #2)
> BTW, persisting for the session is covered by bug 860712.
> 
> I think we should at least do what Chrome does. They seem to persist for the
> tab+domain combo.

Yeah, this is fairly new.  When I tested some months ago, Chrome had no persistence.  I like the way they persist now though.  I don't really want to persist for an entire browsing session this those sometimes go on for ages.

After I finish working on bug https://bugzilla.mozilla.org/show_bug.cgi?id=902350, I will start working on this.
If the channel and the docshell persist while a user navigates a particular domain, then this might be a simple fix.  We have to add another condition to the if/else block here, that does not reset the channel to null if the domain has not changed from the previous navigation:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9536


pseudocode:

if (mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) {
        rv = SetMixedContentChannel(channel);
        NS_ENSURE_SUCCESS(rv, rv);
} else if (channel != mMixedContentChannel){
       rv = SetMixedContentChannel(nullptr);
       NS_ENSURE_SUCCESS(rv, rv);
}
Assignee: nobody → mozilla
Tracking since this would probably benefit from uplift to reduce the likelihood that users think Firefox is the problem and not in fact the persistence of an override of blocking.
Bhavanna - just want to give you a heads up on this for 24, if we can get a low risk fix this is definitely something worth pushing into Beta in order to address some of the concerns in https://blog.mozilla.org/tanvi/2013/04/10/mixed-content-blocking-enabled-in-firefox-23/ comments as well as to ensure that the ESR 24 base gets the parity of domain persistence.  Tanvi - there should be another email to the ESR mailing list explaining to them that they should do advance testing of intranet sites over the 2 release cycles where 17 & 24 are both present, before we flip to only supporting FF24 for security releases.
This patch remembers the users decision once the user hits the 'disable protection' button for browsing sub pages originating from the same domain.
We check if the host has not changed between the earlier channel and the mixedContentChannel still active in the docShell. However, we still have to set a new mixed content channel, because the channel itself changed, even though the host remains the same.

Note, this patch does not apply for the same page loaded in a new tab! In such a case, the user would have to actively hit the 'disable proection' button again.
Attachment #790927 - Flags: review?(tanvi)
Comment on attachment 790927 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

feedback of course, not review :-)
Attachment #790927 - Flags: review?(tanvi) → feedback?(tanvi)
Comment on attachment 790927 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

># HG changeset patch
># User Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
># Date 1376598018 25200
># Node ID 58ab3bdcc088ec94b107f093a1e6265a06e06ca5
># Parent  aada0f74faf9af9a5bc0f54d270020c2d7668969
>Bug 902156 - Persist 'disable protection' option for Mixed Content Blocker
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -419,16 +419,29 @@ static bool
> IsSameHost(nsIURI *uri1, nsIURI *uri2)
> {
>   nsAutoCString host1, host2;
>   uri1->GetAsciiHost(host1);
>   uri2->GetAsciiHost(host2);
>   return host1.Equals(host2);
> }
> 
>+// Check to see if two Channels are the same
>+static bool
>+IsSameChannel(nsIChannel *channel1, nsIChannel *channel2)
>+{
>+  if (!channel1 || !channel2)
>+    return false;
>+
>+  nsCOMPtr<nsIURI> uri1, uri2;
>+  channel1->GetURI(getter_AddRefs(uri1));
>+  channel2->GetURI(getter_AddRefs(uri2));
>+  return IsSameHost(uri1, uri2);
>+}
>+
We probably have to call this function something else; because we aren't checking if it is same channel but if the channels have the same host.  Maybe "ChannelsHaveSameHost".  That is also not very good; if you don't come up with something better, I'm sure smaug can help us name this function.  Or he may tell us to just put this code directly into DoURILoad itself.

Also, don't you need to add the function prototype to nsDocShell.h?


...

>+    if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
>+        (IsSameChannel(channel, mMixedContentChannel))) {
>           rv = SetMixedContentChannel(channel);
>           NS_ENSURE_SUCCESS(rv, rv);
>     } else {
>           rv = SetMixedContentChannel(nullptr);
>           NS_ENSURE_SUCCESS(rv, rv);
>     }
> 
>     //hack
Remove the line hack.

Also, there is one case I didn't tell you about.  When you do a document.open, you end up with a new channel, and have to set the mixedContentChannel again.  See the code here: http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1626.  I'm not sure if this code needs to be touched for this persistence bug to work.  

Assume a page has mixed content.  The user disables protection.  Mixed Script loads.  Then the page does a document.open.  Mixed content is still allowed on the document.opened page.  Here is an example that shows that working today - https://people.mozilla.com/~tvyas/mixeddocument.html.  Now assume that the user clicks on a link in the document.open'ed page that navigates it to the same domain.  Do we persist the disable protection decision (without making any changes to nsHTMLDocument.cpp)?  Probably not.  Do we want to bother persisting the decision in this case and update nsHTMLDocument.cpp in the same way we updated nsDocShell.cpp?

If we do want to persist the decision, should we do it in this bug, or make a new bug for the document.open edge case?
(In reply to Tanvi Vyas [:tanvi] from comment #9)
> >+// Check to see if two Channels are the same
> >+static bool
> >+IsSameChannel(nsIChannel *channel1, nsIChannel *channel2)
> >+{
> >+  if (!channel1 || !channel2)
> >+    return false;
> >+
> >+  nsCOMPtr<nsIURI> uri1, uri2;
> >+  channel1->GetURI(getter_AddRefs(uri1));
> >+  channel2->GetURI(getter_AddRefs(uri2));
> >+  return IsSameHost(uri1, uri2);
> >+}
> >+
> We probably have to call this function something else; because we aren't
> checking if it is same channel but if the channels have the same host. 
> Maybe "ChannelsHaveSameHost".  That is also not very good; if you don't come
> up with something better, I'm sure smaug can help us name this function.  Or

Of course we can change the function name, what about 'IsSameHostOnChannel', because we already use the existing 'IsSameHost' in the functions body.

> he may tell us to just put this code directly into DoURILoad itself.

I would like to keep it in a separate function, because if the first condition in

if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
    (IsSameChannel(channel, mMixedContentChannel))) {

evaluates to true, then we don't even have to perform the 'more expensive' tasks of extracting the host.
 
> Also, don't you need to add the function prototype to nsDocShell.h?

No, it's a static function outside the classes scope and only accessible in this cpp file.

> 
> >+    if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
> >+        (IsSameChannel(channel, mMixedContentChannel))) {
> >           rv = SetMixedContentChannel(channel);
> >           NS_ENSURE_SUCCESS(rv, rv);
> >     } else {
> >           rv = SetMixedContentChannel(nullptr);
> >           NS_ENSURE_SUCCESS(rv, rv);
> >     }
> > 
> >     //hack
> Remove the line hack.

I haven't even introduced that :-)

> 
> Also, there is one case I didn't tell you about.  When you do a
> document.open, you end up with a new channel, and have to set the
> mixedContentChannel again.  See the code here:
> http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/
> nsHTMLDocument.cpp#1626.  I'm not sure if this code needs to be touched for
> this persistence bug to work.  
> 
> Assume a page has mixed content.  The user disables protection.  Mixed
> Script loads.  Then the page does a document.open.  Mixed content is still
> allowed on the document.opened page.  Here is an example that shows that
> working today - https://people.mozilla.com/~tvyas/mixeddocument.html.  Now
> assume that the user clicks on a link in the document.open'ed page that
> navigates it to the same domain.  Do we persist the disable protection
> decision (without making any changes to nsHTMLDocument.cpp)?  Probably not. 
> Do we want to bother persisting the decision in this case and update
> nsHTMLDocument.cpp in the same way we updated nsDocShell.cpp?
> 
> If we do want to persist the decision, should we do it in this bug, or make
> a new bug for the document.open edge case?

I have checked for all appearances of SetMixedContentChannel(). Turns out that
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1631
only sets the mixed content channel if
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5397
returns true for the variable allowMixedContent, which is not the case if a mixedcontentChannel is not already set yet. Therefore that edge case is not a problem and is not affecting this patch!
Comment on attachment 790927 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

(In reply to Christoph Kerschbaumer from comment #10)
> (In reply to Tanvi Vyas [:tanvi] from comment #9)
> > >+// Check to see if two Channels are the same
> > >+static bool
> > >+IsSameChannel(nsIChannel *channel1, nsIChannel *channel2)
> > >+{
> > >+  if (!channel1 || !channel2)
> > >+    return false;
> > >+
> > >+  nsCOMPtr<nsIURI> uri1, uri2;
> > >+  channel1->GetURI(getter_AddRefs(uri1));
> > >+  channel2->GetURI(getter_AddRefs(uri2));
> > >+  return IsSameHost(uri1, uri2);
> > >+}
> > >+
> > We probably have to call this function something else; because we aren't
> > checking if it is same channel but if the channels have the same host. 
> > Maybe "ChannelsHaveSameHost".  That is also not very good; if you don't come
> > up with something better, I'm sure smaug can help us name this function.  Or
> 
> Of course we can change the function name, what about 'IsSameHostOnChannel',
> because we already use the existing 'IsSameHost' in the functions body.
> 
Sounds good!


  
> > >+    if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
> > >+        (IsSameChannel(channel, mMixedContentChannel))) {
> > >           rv = SetMixedContentChannel(channel);
> > >           NS_ENSURE_SUCCESS(rv, rv);
> > >     } else {
> > >           rv = SetMixedContentChannel(nullptr);
> > >           NS_ENSURE_SUCCESS(rv, rv);
> > >     }
> > > 
> > >     //hack
> > Remove the line hack.
> 
> I haven't even introduced that :-)
> 
Ah yes, I missed that.

> > Also, there is one case I didn't tell you about.  When you do a
> > document.open, you end up with a new channel, and have to set the
> > mixedContentChannel again.  See the code here:
> > http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/
> > nsHTMLDocument.cpp#1626.  I'm not sure if this code needs to be touched for
> > this persistence bug to work.  
> > 
> > Assume a page has mixed content.  The user disables protection.  Mixed
> > Script loads.  Then the page does a document.open.  Mixed content is still
> > allowed on the document.opened page.  Here is an example that shows that
> > working today - https://people.mozilla.com/~tvyas/mixeddocument.html.  Now
> > assume that the user clicks on a link in the document.open'ed page that
> > navigates it to the same domain.  Do we persist the disable protection
> > decision (without making any changes to nsHTMLDocument.cpp)?  Probably not. 
> > Do we want to bother persisting the decision in this case and update
> > nsHTMLDocument.cpp in the same way we updated nsDocShell.cpp?
> > 
> > If we do want to persist the decision, should we do it in this bug, or make
> > a new bug for the document.open edge case?
> 
> I have checked for all appearances of SetMixedContentChannel(). Turns out
> that
> http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/
> nsHTMLDocument.cpp#1631
> only sets the mixed content channel if
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#5397
> returns true for the variable allowMixedContent, which is not the case if a
> mixedcontentChannel is not already set yet. Therefore that edge case is not
> a problem and is not affecting this patch!

Yes, you are right. Great!  Glad we don't have to handle any edge cases.  feedback + from me.  Change the function name and then r? to smaug.
Attachment #790927 - Flags: feedback?(tanvi) → feedback+
updated the function name :-)
Attachment #790927 - Attachment is obsolete: true
Attachment #790963 - Flags: review?(bugs)
Comment on attachment 790963 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

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

::: docshell/base/nsDocShell.cpp
@@ +419,4 @@
>  IsSameHost(nsIURI *uri1, nsIURI *uri2)
>  {
>    nsAutoCString host1, host2;
>    uri1->GetAsciiHost(host1);

This will fail if GetURI fails below.

Maybe it's more consistent to do NS_ENSURE_ARG(uri1) here and below, unless GetURI never fails.

@@ +9545,5 @@
>              NS_ENSURE_SUCCESS(rv, rv);
>          }
>      }
>  
> +    /*

Thank you for the expanded comment :) Minor nit: formatting for 80 chars would be ok.

@@ +9557,5 @@
> +     * same webpage.
> +     */
> +
> +    if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
> +        (IsSameHostOnChannel(channel, mMixedContentChannel))) {

too many parens! this doesn't actually require any

if (mLoadType == LOAD... || IsSameHostOnChannel()) {
Component: Security → Document Navigation
Product: Firefox → Core
Comment on attachment 790963 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

This needs tests.
Attachment #790963 - Flags: review?(tanvi)
Attachment #790963 - Flags: review?(bugs)
Attachment #790963 - Flags: review+
Please address the review comments from Monica in comment 13.

For GetURI, you could do something similar to what you did for the channel:
if (!uri1 || !uri2)
  return false;
Blocks: 906190
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #13)
> >  IsSameHost(nsIURI *uri1, nsIURI *uri2)
> >  {
> >    nsAutoCString host1, host2;
> >    uri1->GetAsciiHost(host1);
> 
> This will fail if GetURI fails below.
> 
> Maybe it's more consistent to do NS_ENSURE_ARG(uri1) here and below, unless
> GetURI never fails.

Addressed!

> Thank you for the expanded comment :) Minor nit: formatting for 80 chars
> would be ok.

80 chars it is :-)

> > +    if ((mLoadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) ||
> > +        (IsSameHostOnChannel(channel, mMixedContentChannel))) {
> 
> too many parens! this doesn't actually require any
> 
> if (mLoadType == LOAD... || IsSameHostOnChannel()) {

In my opinion it's always better to have a parent too much than you get behavior you can not completely predict!
In this case however, you are right, I updated it!
Incorporated Monica's comments from Comment 13.
Attachment #790963 - Attachment is obsolete: true
Attachment #790963 - Flags: review?(tanvi)
Hey Tanvi,

please look really close and make sure that I do the right thing!
Attachment #791628 - Flags: feedback?(tanvi)
Comment on attachment 791628 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_update_test_822367.patch

This looks good.
Attachment #791628 - Flags: feedback?(tanvi) → feedback+
Comment on attachment 791625 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

Looks good.  Thanks Christoph!  r+ from me and we can carry over the r+ from smaug.
Attachment #791625 - Flags: review+
Comment on attachment 791625 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

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

::: docshell/base/nsDocShell.cpp
@@ +437,5 @@
> +
> +  if (!uri1 || !uri2)
> +    return false;
> +
> +  return IsSameHost(uri1, uri2);

This should be a same-origin (i.e. a same-principal) check, not a "same host" check, right? By default, security checks are done by comparing principals. If "same host" is really the intent, then please explain in a comment why a same-origin check isn't being done.

While you are correcting this, please add a comment "Do not use this for anything other than <a ping>" to IsSameHost.
Attachment #791625 - Flags: feedback-
Also, see bug 326155 to understand the history of why "<a ping>" is doing a same-host check instead of a same-origin check.
Thanks for pointing that out Brian, but we intentionally use the sameHostCheck because if a users decides to 'Disable Protection for this page' we remember his decision regardless of scheme and port.
I added a comment to make it explicit.

Carrying over r+ from smaug!
Attachment #791625 - Attachment is obsolete: true
Attachment #792279 - Flags: review+
Comment on attachment 792279 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

Sorry Brian, I didn't want to overrule your feedback decision!
Attachment #792279 - Flags: feedback?(brian)
Comment on attachment 792279 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

Sorry Brian, I didn't want to overrule your feedback decision!
Attachment #792279 - Flags: review+ → feedback?(brian)
(In reply to Christoph Kerschbaumer from comment #24)
> Created attachment 792279 [details] [diff] [review]
> bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch
> 
> Thanks for pointing that out Brian, but we intentionally use the
> sameHostCheck because if a users decides to 'Disable Protection for this
> page' we remember his decision regardless of scheme and port.

Thanks for adding the comment. But, the comment doesn't explain why this special exception to our normal same-origin checking is important. Why treat this specially in this part of this feature? Do we have a motivating example? I would expect that app://example.org and https://example.org:4343 must have no effect on each other; that's why app:// has the semantics that it has.

In the absence of a strong reason to do otherwise, I would much rather we do same-principal checks. If nothing else, it will help us in the future if/when we extend mixed content blocker to the browser chrome and/or about: and/or app:.
Brian, I'm not sure if we should be checking same host or same-origin in this case.

Consider this example.  A user visits a homepage of a shopping site over HTTPS.  There is mixed active content on the page, and the user disables protection.  The user starts browsing the catalog for items.  The catalog items are served over HTTP.  The user looks at reviews for items and they are also served over HTTP.  Then the user is ready to finalize their purchases.  They visit their shopping cart which is served over HTTPS.  The shopping cart has mixed active content on it.  The user continues through the HTTPS purchase flow which also has mixed active content.

In this example, if we do a same host check, the user disables protection once.  If we do same origin check, then the user has to disable protection twice, or even more than that if they go from their shopping cart back to the catalog to purchase more goods.

Can you think of an example where doing a same host check would cause a user experience that is unexpected?  What kind of security issue might we have?

Alternatively, we could do a same-origin check and make an exception if the difference in protocol is http/https and the difference in port is 80/443.
Thanks for the explanation Tanvi. Basically, if we navigate https://example.org/ -> http://example.org/ -> https://example.org/, and if the user disabled protection on the first visit to https://example.org/, then the second visit to https://example.org/ to should also have the protection disabled.

I do think that the logic you described makes sense. This patch doesn't implement that logic, but rather different (though similar) logic. And, also, that logic doesn't handle other important cases where the navigation goes through a domain with a different hostname, which is common with things like PayPal and similar.

Do we know of any examples where https://example.org/ -> http://example.org/ -> https://example.org/ is something we have to worry about? Can we say YAGNI? It seems like if we have to worry about that case then we almost definitely have to worry about https://example.org/ -> https://login.example.org/ -> https://example.org/ (same eTLD + 1) and https://example.org/ -> https://paypal.com/ -> https://example.org (different eTLD) too.
(In reply to Tanvi Vyas [:tanvi] from comment #28)
> Can you think of an example where doing a same host check would cause a user
> experience that is unexpected?  What kind of security issue might we have?

https://example.org -> app://example.org comes to mind. Also, I do not think it makes sense to allow https://example.org -> https://example.org:4343 but disallow https://example.org -> https://login.example.org. (I also disagree with this being done for <a ping> but since <a ping> is disabled in Firefox anyway, I don't bother arguing about it.)

> Alternatively, we could do a same-origin check and make an exception if the
> difference in protocol is http/https and the difference in port is 80/443.

The main problem I have with the current patch is this sequence:

1. user navigates to https://example.org
2. user disables protection
3. user navigates to app://example.org or https://example.org:4343. Now protection is disabled for these *different* origins.
4. user navigates to https://example.org again. Protection is disabled as it should be.

The problem is #3. protection shouldn't be disabled for app://example.org or https://example.org:4343 just because the user disabled it for a different origin https://example.org.
I'm not comfortable extending this to subdomains.  And we shouldn't extend this to any port or any scheme for the reasons Brian mentioned in comment 30.

That means that we could extend this to where we treat http://example.org:80 as equivalent to https://example.com:443.  But do we really need to?  I couldn't find any live examples where this would be helpful.  If you are navigating through a sites HTTPS and HTTP links, you are likely going to switch to different subdomains for thigns like purchases and logins.  Since we aren't comfrotable persisting the decision for subdomains and we can't find an example where persisting the decision just across http/https and 80/443 is helpful, let's just do a same origin check.

If we find need to do more than this from user feedback / reports, etc, then we can create a new bug to extend the persistence.

Christoph, can you change the logic to check the principal and re-r? to me and smaug.  Thanks!
Comment on attachment 791626 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_test_cases.patch

Christoph, please make the following changes and then r? to smaug.  Thanks!


>Tests for Bug 902156 - Persist 'disable protection' option for Mixed Content Blocker
>
>diff --git a/browser/base/content/test/browser_bug902156.js b/browser/base/content/test/browser_bug902156.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_bug902156.js
>@@ -0,0 +1,213 @@
>+/*
>+ * Description of the Tests for
>+ *  - Bug 902156: Persist "disable protection" option for Mixed Content Blocker
>+ *
>+ * 1. Navigate to the same domain via document.location
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Reload the page using document.location
Can we use another file in the same-origin instead of using the same file?  This way, we show that it persists for different html files within the same-origin.  You don't have to create a new file.  You can use file_bug902156_2.html in test 1 and file_bug902156_1.html in test 2.
>+ *    - Doorhanger should not appear anymore!
>+ * 
>+ * 2. Navigate to the same domain via simulateclick for a link on the page
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Reload the page simulating a click
Same as above.
>+ *    - Doorhanger should not appear anymore!
>+ *
>+ * 3. Navigate to a differnet domain and show the content is still blocked
>+ *    - Load a different html page which has mixed content
>+ *    - Doorhanger to disable protection should appear again because
>+ *      we navigated away from html page where we disabled the protection.
>+ *
>+ * Note, for all tests we set gHttpTestRoot to use 'https'.
>+ */


>+function test1C() {
>+  gTestBrowser.removeEventListener("load", test1B, true);
>+  var actual = content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 1");
>+
>+  // The Script loaded after we disabled the page, now we are going to reload the
>+  // page and see if our decision is persistent
>+  gTestBrowser.addEventListener("load", test1D, true);
>+
>+  var url = gHttpTestRoot1 + "file_bug902156_1.html";
Change to file_bug902156_2.html

>+  gTestBrowser.contentWindow.location = url;
>+}
>+
>+function test1D() {
>+  gTestBrowser.removeEventListener("load", test1D, true);
>+
>+  // The Doorhanger should not appear, because our decision of disabling the
>+  // mixed content blocker is persistent.
>+  var notification = PopupNotifications.getNotification("mixed-content-blocked", gTestBrowser);
>+  ok(!notification, "OK: Mixed Content Doorhanger did not appear again in Test1!");
>+
Can you add a check to innerHTML for "Mixed Script Blocker disabled" to ensure the mixed script loaded?

>+  // move on to Test 2
>+  test2();
>+  //cleanUpAfterTests();
Remove commented line.


>+
>+function test2D() {
>+  gTestBrowser.removeEventListener("load", test2D, true);
>+
>+  // The Doorhanger should not appear, because our decision of disabling the
>+  // mixed content blocker is persistent.
>+  var notification = PopupNotifications.getNotification("mixed-content-blocked", gTestBrowser);
>+  ok(!notification, "OK: Mixed Content Doorhanger did not appear again in Test2!");
>+
Same as above.  add a test to check that innerHTML is "Mixed Script Blocker disabled" to ensure that mixed script loaded.



>+function test() {
>+  // Perfomring async calls, e.g. 'onload', we have to wait till all of them finished
spelling - Performing

>+  waitForExplicitFinish();
>+
>+  // Store original preferences so we can restore settings after testing
>+  origBlockDisplay = Services.prefs.getBoolPref(PREF_DISPLAY);
>+  origBlockActive = Services.prefs.getBoolPref(PREF_ACTIVE);
>+
>+  Services.prefs.setBoolPref(PREF_DISPLAY, true);
We don't actually need to set the mixed display pref in this test because we don't use any mixed display content.  You can remove it from here and from above.

>+  Services.prefs.setBoolPref(PREF_ACTIVE, true);

>diff --git a/browser/base/content/test/file_bug902156_2.html b/browser/base/content/test/file_bug902156_2.html
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/file_bug902156_2.html

>+<body>
>+  <div id="mctestdiv">Mixed Content Blocker enabled</div>
>+  <a href="https://test2.example.com/browser/browser/base/content/test/file_bug902156_2.html"
Change to file_bug902156_1.html

>+     id="mctestlink" target="_top">Go to http site</a>
>+  <script src="http://test2.example.com/browser/browser/base/content/test/file_bug902156.js" ></script>
>+</body>
>+</html>
Attachment #791626 - Flags: feedback?(tanvi) → feedback+
Followed Brians suggestion and changed the patch from using sameHost to sameOrigin!
Attachment #792279 - Attachment is obsolete: true
Attachment #792279 - Flags: feedback?(brian)
Attachment #792995 - Flags: feedback?(tanvi)
Attachment #792995 - Flags: feedback?(brian)
Incorporated Tanvis suggestions!
Attachment #791626 - Attachment is obsolete: true
Attachment #792996 - Flags: review?(bugs)
Comment on attachment 792996 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_test_cases.patch

># HG changeset patch
># User Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
># Date 1376716502 25200
># Node ID 76461b31bcac833ab9b4ab0af7cc43db14041d4e
># Parent  f469d51c818006244fad5b66270662ab982662bc
>Tests for Bug 902156 - Persist 'disable protection' option for Mixed Content Blocker
>
>diff --git a/browser/base/content/test/browser_bug902156.js b/browser/base/content/test/browser_bug902156.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_bug902156.js

>+const PREF_ACTIVE = "security.mixed_content.block_active_content";
>+
>+// We alternate for even and odd test cases to simulate different hosts
>+const gHttpTestRoot1 = "https://test1.example.com/browser/browser/base/content/test/";
>+const gHttpTestRoot2 = "https://test2.example.com/browser/browser/base/content/test/";
>+
>+var origBlockDisplay;
Remove - unused variable

>+var origBlockActive;
Attachment #792996 - Flags: feedback+
Comment on attachment 792995 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

>Bug 902156 - Persist 'disable protection' option for Mixed Content Blocker
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -9547,19 +9547,42 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>+
>+        nsCOMPtr<nsIURI> cURI, mccURI;
>+        channel->GetURI(getter_AddRefs(cURI));
>+        mMixedContentChannel->GetURI(getter_AddRefs(mccURI));
>+        if (NS_SUCCEEDED(secMan->CheckSameOriginURI(cURI, mccURI, false))) {
Should the third parameter to CheckSameOriginURI be true or false for error reporting?

>+          rv = SetMixedContentChannel(channel);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+        }
>+        else {
>           rv = SetMixedContentChannel(nullptr);
>           NS_ENSURE_SUCCESS(rv, rv);
>+        }
Attachment #792995 - Flags: review?(bugs)
Attachment #792995 - Flags: feedback?(tanvi)
Attachment #792995 - Flags: feedback+
Comment on attachment 792995 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

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

::: docshell/base/nsDocShell.cpp
@@ +9572,5 @@
> +        mMixedContentChannel->GetURI(getter_AddRefs(mccURI));
> +        if (NS_SUCCEEDED(secMan->CheckSameOriginURI(cURI, mccURI, false))) {
> +          rv = SetMixedContentChannel(channel);
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }

It is better to use nsContentUtils::CheckSameOrigin(nsIChannel *aOldChannel, nsIChannel *aNewChannel) if possible.
Attachment #792995 - Flags: feedback?(brian) → feedback-
Using checkSameOrigin instead of CheckSameOriginURI! Thanks Brian for pointing that out!
Attachment #792995 - Attachment is obsolete: true
Attachment #792995 - Flags: review?(bugs)
Attachment #793200 - Flags: review?(bugs)
Took out unused variable! Thanks Tanvi for catching that!
Attachment #792996 - Attachment is obsolete: true
Attachment #792996 - Flags: review?(bugs)
Attachment #793202 - Flags: review?(bugs)
Comment on attachment 793200 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

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

::: docshell/base/nsDocShell.cpp
@@ +9559,5 @@
> +       * mMixedContentChannel is still null.
> +       * Later, if the new channel passes a same orign check, we remember the
> +       * users decision by calling SetMixedContentChannel using the new channel.
> +       * This way, the user does not have to click the disable protection button
> +       * over and over for browsing the same webpage.

s/webpage/site/.
Attachment #793200 - Flags: feedback+
everything for you brian :-)
Attachment #793200 - Attachment is obsolete: true
Attachment #793200 - Flags: review?(bugs)
Attachment #793211 - Flags: review?(bugs)
Comment on attachment 793211 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch


>+    } else if (mMixedContentChannel) {
>+      /*
>+       * If the user "Disables Protection on This Page", we call
>+       * SetMixedContentChannel for the first time, otherwise
>+       * mMixedContentChannel is still null.
>+       * Later, if the new channel passes a same orign check, we remember the
>+       * users decision by calling SetMixedContentChannel using the new channel.
>+       * This way, the user does not have to click the disable protection button
>+       * over and over for browsing the same site.
>+       */
>+      rv = nsContentUtils::CheckSameOrigin(channel, mMixedContentChannel);
Isn't mMixedContentChannel the old channel and channel the new one?

>+      if (NS_SUCCEEDED(rv)) {
>+        rv = SetMixedContentChannel(channel);
>+        NS_ENSURE_SUCCESS(rv, rv);
Hmm, we shouldn't return error in this case but just clear the channel or something


>+      }
>+      else {
} else {



But could this be 

rv = nsContentUtils::CheckSameOrigin(mMixedContentChannel, channel);
if (NS_FAILED(rv) || NS_FAILED(SetMixedContentChannel(channel))) {
  SetMixedContentChannel(nullptr);
}
Attachment #793211 - Flags: review?(bugs) → review+
Comment on attachment 793202 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_test_cases.patch

Make sure to run these tests several times on tryserver before pushing to
trunk.
Attachment #793202 - Flags: review?(bugs) → review+
Comment on attachment 791628 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_update_test_822367.patch

Looks like we didn't r? this one to Olli.  Olli - this is a very simple change.  We are just alternating between domains for a previous test case, since adding persistence would otherwise make this test case fail.
Attachment #791628 - Flags: review?(bugs)
Comment on attachment 791628 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_update_test_822367.patch

rs+

(but I thought this was part of some patch I looked at)
Attachment #791628 - Flags: review?(bugs) → review+
Updated patch following smaugs suggestions and also carried over the r+
Attachment #793211 - Attachment is obsolete: true
Attachment #793707 - Flags: review+
And here is a previous try run (but with an older version of the patches): https://tbpl.mozilla.org/?tree=Try&rev=99392161086e

Christoph is going to send this to try a couple more times (per smaug).  

Bhavana - I wanted to land today to get it into tomorrow's beta build but I think it's better if we wait and make sure we have results from all the try runs.  If the results are good in the morning (or really late tonight), we can check-in to inbound/aurora/beta but I'm not sure that will be in time for the beat build.
(In reply to Tanvi Vyas [:tanvi] from comment #49)
> Two new try pushes for just the browser-chrome tests:

Note that you can re-trigger test runs on an existing build rather than doing a new build with a new try push. http://grab.by/pA1q
(In reply to Matthew N. [:MattN] from comment #50)
> (In reply to Tanvi Vyas [:tanvi] from comment #49)
> > Two new try pushes for just the browser-chrome tests:
> 
> Note that you can re-trigger test runs on an existing build rather than
> doing a new build with a new try push. http://grab.by/pA1q

Yes you are right!  I forgot about this.  Cancelling the extra try pushes and just retriggering on the original one: https://tbpl.mozilla.org/?tree=Try&rev=56762655bdb4
Comment on attachment 793707 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834836 to turn on the Mixed Content Blocker
User impact if declined: Negative user feedback and security warning fatigue
Testing completed (on m-c, etc.): Added mochitest that tests the persistence of the user's "disable protection" decision.
Risk to taking this patch (and alternatives if risky): Patch includes a change to nsDocShell.cpp that is invoked only after a user has disabled protection on a page.
String or IDL/UUID changes made by this patch: None.
Attachment #793707 - Flags: approval-mozilla-beta?
Attachment #793707 - Flags: approval-mozilla-aurora?
Comment on attachment 793202 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_test_cases.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834836 to turn on the Mixed Content Blocker
User impact if declined: Negative user feedback and security warning fatigue
Testing completed (on m-c, etc.): Added mochitest that tests the persistence of the user's "disable protection" decision.
Risk to taking this patch (and alternatives if risky): Adds browser mochitest.
String or IDL/UUID changes made by this patch: None.
Attachment #793202 - Flags: approval-mozilla-beta?
Attachment #793202 - Flags: approval-mozilla-aurora?
Comment on attachment 791628 [details] [diff] [review]
bug_902156_persist_disable_protection_option_for_mixed_content_blocker_update_test_822367.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834836 to turn on the Mixed Content Blocker
User impact if declined: Negative user feedback and security warning fatigue
Testing completed (on m-c, etc.): Added mochitest that tests the persistence of the user's "disable protection" decision.
Risk to taking this patch (and alternatives if risky): Updates an existing browser mochitest for mixed content blocker.
String or IDL/UUID changes made by this patch: None.
Attachment #791628 - Flags: approval-mozilla-beta?
Attachment #791628 - Flags: approval-mozilla-aurora?
(In reply to Tanvi Vyas [:tanvi] from comment #53)
> Comment on attachment 793707 [details] [diff] [review]
> bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 834836 to turn on the Mixed
> Content Blocker
> User impact if declined: Negative user feedback and security warning fatigue
> Testing completed (on m-c, etc.): Added mochitest that tests the persistence
> of the user's "disable protection" decision.
> Risk to taking this patch (and alternatives if risky): Patch includes a
> change to nsDocShell.cpp that is invoked only after a user has disabled
> protection on a page.

Can we please get some Quantification of the risk here ? Also what is the risk of new regressions given this new code path? I see there a lot of tests which is great :) but should we get any more manual testing here to mitigate the risk if needed ?
> String or IDL/UUID changes made by this patch: None.
(In reply to bhavana bajaj [:bajaj] from comment #56)
> (In reply to Tanvi Vyas [:tanvi] from comment #53)
> > Comment on attachment 793707 [details] [diff] [review]
> > bug_902156_persist_disable_protection_option_for_mixed_content_blocker.patch
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): bug 834836 to turn on the Mixed
> > Content Blocker
> > User impact if declined: Negative user feedback and security warning fatigue
> > Testing completed (on m-c, etc.): Added mochitest that tests the persistence
> > of the user's "disable protection" decision.
> > Risk to taking this patch (and alternatives if risky): Patch includes a
> > change to nsDocShell.cpp that is invoked only after a user has disabled
> > protection on a page.
> 
> Can we please get some Quantification of the risk here ? Also what is the
> risk of new regressions given this new code path? I see there a lot of tests
> which is great :) but should we get any more manual testing here to mitigate
> the risk if needed ?
> > String or IDL/UUID changes made by this patch: None.

Also a quick question on the bake-time..given this is not baked at all on central/aurora, I would like input on if it is ok to take into beta in that state? I am fine with the option as long as we are confident on the patch and are sure this will not lead into any serious/critical user-facing regression keeping in mind the gain that we would get by getting this into our beta users sooner.
(In reply to bhavana bajaj [:bajaj] from comment #57)
> Also a quick question on the bake-time..given this is not baked at all on
> central/aurora, I would like input on if it is ok to take into beta in that
> state? I am fine with the option as long as we are confident on the patch
> and are sure this will not lead into any serious/critical user-facing
> regression keeping in mind the gain that we would get by getting this into
> our beta users sooner.

I think it is a good idea to let this bake for a week on mozilla-central (and maybe mozilla-aurora) before landing it to mozilla-beta.
(In reply to bhavana bajaj [:bajaj] from comment #56)
> (In reply to Tanvi Vyas [:tanvi] from comment #53)
> > Risk to taking this patch (and alternatives if risky): Patch includes a
> > change to nsDocShell.cpp that is invoked only after a user has disabled
> > protection on a page.
> 
> Can we please get some Quantification of the risk here ? Also what is the
> risk of new regressions given this new code path? I see there a lot of tests
> which is great :) but should we get any more manual testing here to mitigate
> the risk if needed ?

Not sure how to quantify the risk, but I can discribe it further...

The new code will only get called if a mixed content channel exists.  A mixed content channel exists if the user has previously "disabled protection" on a page.  The new code checks the hosts associated with the current channel and the mixed content channel.  If they match, we continue to disable protection.  If they don't, the mixed content channel is set to null and we don't disable protection.

If this check fails in some way we could have one of two outcomes:
1) The persistence doesn't work and the user has to disable protection again
2) We end up disabling protection on a page we shouldn't have.

QA could do some basic manual testing on websites with mixed content blocked and see if they are behaving as intended.  If we want to do this, Christoph and I can give them some pointers on how to test.
As far as bake time, I think we should let it bake on mozilla-central for a day or two.  On Saturday, we could land on aurora.  Then we can land on Beta sometime next week.  Maybe Tuesday so that it has a day in beta before Thursday mornings beta build.  Bhavana, how does that sound?
(In reply to Tanvi Vyas [:tanvi] from comment #60)
> As far as bake time, I think we should let it bake on mozilla-central for a
> day or two.  On Saturday, we could land on aurora. 

I'll approve the aurora landing tomorrow for Friday/sat landing.

> Then we can land on Beta
> sometime next week.  Maybe Tuesday so that it has a day in beta before
> Thursday mornings beta build.  Bhavana, how does that sound?

Our next beta(Fx24 Beta 6) goes to build Monday so lets make sure its in there by then.
(In reply to Tanvi Vyas [:tanvi] from comment #59)
> (In reply to bhavana bajaj [:bajaj] from comment #56)
> > (In reply to Tanvi Vyas [:tanvi] from comment #53)
> > > Risk to taking this patch (and alternatives if risky): Patch includes a
> > > change to nsDocShell.cpp that is invoked only after a user has disabled
> > > protection on a page.
> > 
> > Can we please get some Quantification of the risk here ? Also what is the
> > risk of new regressions given this new code path? I see there a lot of tests
> > which is great :) but should we get any more manual testing here to mitigate
> > the risk if needed ?
> 
> Not sure how to quantify the risk, but I can discribe it further...
> 
> The new code will only get called if a mixed content channel exists.  A
> mixed content channel exists if the user has previously "disabled
> protection" on a page.  The new code checks the hosts associated with the
> current channel and the mixed content channel.  If they match, we continue
> to disable protection.  If they don't, the mixed content channel is set to
> null and we don't disable protection.
> 
> If this check fails in some way we could have one of two outcomes:
> 1) The persistence doesn't work and the user has to disable protection again
> 2) We end up disabling protection on a page we shouldn't have.
> 
> QA could do some basic manual testing on websites with mixed content blocked
> and see if they are behaving as intended.  If we want to do this, Christoph
> and I can give them some pointers on how to test.
I spoke to Tanvi more about this offline and the worst possible instance would be that the user may end up in the same state as Fx23 or how Fx22 behaved by default.But this can be mitigated with testing.

Hence I am cc'ing Mihai(who has helped in WebRTC testing) to have some special testplan around this Bug and request to reach out to Tanvi and Christoph to some real world testing scenarios. Tanvi ,I think it will really be helpful to go through the user comments on blogs or other feedback we've received and make sure they are in QA'a testplan.
QA Contact: mihai.morar
Keywords: verifyme
(In reply to bhavana bajaj [:bajaj] from comment #63)
> Hence I am cc'ing Mihai(who has helped in WebRTC testing) to have some
> special testplan around this Bug and request to reach out to Tanvi and
> Christoph to some real world testing scenarios. Tanvi ,I think it will
> really be helpful to go through the user comments on blogs or other feedback
> we've received and make sure they are in QA'a testplan.

Christoph, can you provide Mihai with some test cases he can use to test the feature and some details on the expected results?
Mostly, I was using Tanvis page for testing:
https://people.mozilla.com/~tvyas/AllMixedTests.html

For example:
test document open : https://people.mozilla.com/~tvyas/mixeddocument.html

Step 1 - open page
Step 2 - (click) Disable Protection for this page (see alert pop up)
Step 3 - (click) new Document (see second alert) [Expected: Doorhanger should not appear again]
Step 4 - (click) this link (see alert 9) [Expected: Doorhanger should not appear again]
(In reply to bhavana bajaj [:bajaj] from comment #63)
> (In reply to Tanvi Vyas [:tanvi] from comment #59)
> > (In reply to bhavana bajaj [:bajaj] from comment #56)
> > > (In reply to Tanvi Vyas [:tanvi] from comment #53)
> > > > Risk to taking this patch (and alternatives if risky): Patch includes a
> > > > change to nsDocShell.cpp that is invoked only after a user has disabled
> > > > protection on a page.
> > > 
> > > Can we please get some Quantification of the risk here ? Also what is the
> > > risk of new regressions given this new code path? I see there a lot of tests
> > > which is great :) but should we get any more manual testing here to mitigate
> > > the risk if needed ?
> > 
> > Not sure how to quantify the risk, but I can discribe it further...
> > 
> > The new code will only get called if a mixed content channel exists.  A
> > mixed content channel exists if the user has previously "disabled
> > protection" on a page.  The new code checks the hosts associated with the
> > current channel and the mixed content channel.  If they match, we continue
> > to disable protection.  If they don't, the mixed content channel is set to
> > null and we don't disable protection.
> > 
> > If this check fails in some way we could have one of two outcomes:
> > 1) The persistence doesn't work and the user has to disable protection again
> > 2) We end up disabling protection on a page we shouldn't have.
> > 
> > QA could do some basic manual testing on websites with mixed content blocked
> > and see if they are behaving as intended.  If we want to do this, Christoph
> > and I can give them some pointers on how to test.
> I spoke to Tanvi more about this offline and the worst possible instance
> would be that the user may end up in the same state as Fx23 or how Fx22
> behaved by default.But this can be mitigated with testing.
> 
> Hence I am cc'ing Mihai(who has helped in WebRTC testing) to have some
err, sorry Mihai , meant MCB testing (>//<)
> special testplan around this Bug and request to reach out to Tanvi and
> Christoph to some real world testing scenarios. Tanvi ,I think it will
> really be helpful to go through the user comments on blogs or other feedback
> we've received and make sure they are in QA'a testplan.
Attachment #791628 - Flags: approval-mozilla-beta?
Attachment #791628 - Flags: approval-mozilla-beta+
Attachment #791628 - Flags: approval-mozilla-aurora?
Attachment #791628 - Flags: approval-mozilla-aurora+
Attachment #793202 - Flags: approval-mozilla-beta?
Attachment #793202 - Flags: approval-mozilla-beta+
Attachment #793202 - Flags: approval-mozilla-aurora?
Attachment #793202 - Flags: approval-mozilla-aurora+
Attachment #793707 - Flags: approval-mozilla-beta?
Attachment #793707 - Flags: approval-mozilla-beta+
Attachment #793707 - Flags: approval-mozilla-aurora?
Attachment #793707 - Flags: approval-mozilla-aurora+
Mihai, here is how you can test that the patch in this bug works:

Go to https://people.mozilla.com/~tvyas/mixedcontent.html.  Disable protection.  Click the link to navigate to another page.  You will get to https://people.mozilla.com/~tvyas/mixedcontent2.html and you should see some alerts.  You should NOT see the shield.  Click the link on that page that will take you to https://people.mozilla.com/~tvyas/mixedcontent3.html.  Again, you will see some alerts.  You should NOT see the shield.  Click the link on that page and that will take you to https://espn.go.com.  You SHOULD see the shield again because you have switched domains and the decision to disable protection should not have persisted across domains.  Click the shield and disable protection on the page so that it renders correctly.

Now, in the same type, type in https://people.mozilla.com/~tvyas/mixedcontent4.html.  You SHOULD see the shield again since you have navigated away from the domain (you went to espn.go.com) and then came back to it.

Do you also want to test that without this patch, the above does not work?  Without this patch, you will see the shield on every single page you visit as you go through these steps.  When you disable protection on mixedcontent.html and then click the link on mixedcontent2.html, you would have to disable protection again because without this patch there is no persistence.

Does this make sense?
verified with STR's from comment 68 in Fx24beta10, and builds from 20130913 on Nightly and Aurora.
Release note could read something like "Firefox's mixed content blocker now allows users to disable protection permanently for a specific website." Feel free to suggest better language if you've got it. Thanks.
Asa, "permanently" isn't what I observed testing this, nor what I think is expected, per comment 68.  This allows you to disable protection for as long as you stay in the same domain in the current tab.
(In reply to bhavana bajaj [:bajaj] from comment #63)
 to

> Hence I am cc'ing Mihai(who has helped in WebRTC testing) to have some
> special testplan around this Bug and request to reach out to Tanvi and
> Christoph to some real world testing scenarios. 

Oh.. sorry for missing this bug, I was in PTO for a while and I simply hadn't seen this since now. Thanks Tracy for verifying this, I can make additional investigation if this is required and Tany's STR from Comment 68 are not enought for testing this.
The steps to verify in comment 68 + the mochitests should suffice.  Thanks Mihai and Tracy!

Asa, Tracy is right that we do not persist the decision permanently.  It is only while you are navigating a specific domain on a specific tab.  Once you go to another domain or close the tab, the MCB will block again.
(In reply to Tanvi Vyas [:tanvi] from comment #73)
> The steps to verify in comment 68 + the mochitests should suffice.  Thanks
> Mihai and Tracy!

Guys please needinfo? me for any future testing or investigations, so I will not miss other important bugs as this one. I'm receiving tons of email daily from bugzilla being QA responsible on Security and Addons Manager components and it's hard to keep an eye on all of them. If would be easier for me to see the needinfo flag. Thanks and sorry again for missing this issue.
Removing "verifyme" based on Tracy's verification in Comment 69
Keywords: verifyme
Depends on: 921023
Hello, is there any news on this? :)
I think it's still an issue on Nightly 44, but I'm not sure if it's happening because something I missconfigured or an actual issue.

¡Saludos!
(In reply to Mauricio Navarro Miranda from comment #76)
> Hello, is there any news on this? :)
> I think it's still an issue on Nightly 44, but I'm not sure if it's
> happening because something I missconfigured or an actual issue.
> 
> ¡Saludos!

This bug report has been closed for over two years. Please work with our support team at support.mozilla.org to confirm if you've misconfigured something. Assuming you haven't and you're experiencing a Firefox bug, please file a new bug report.

Thank you.
You need to log in before you can comment on or make changes to this bug.