Closed Bug 803225 Opened 8 years ago Closed 7 years ago

WebSockets and about:blank content triggers mixed-active-content blocking

Categories

(Core Graveyard :: Security: UI, defect)

18 Branch
x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: jaws, Assigned: tanvi)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 14 obsolete files)

7.88 KB, patch
tanvi
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
14.89 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
For the past few weeks I have been unable to "Reconnect" on the IRCCloud website. I bisected on Nightlies today and narrowed it down to this pushlog, http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f0587ce1774&tochange=0d3b17a88d5f

I suspect that bug 761227 is to blame here. It seems to be a combination with something in my profile and this patch.

I can help test a patch for this bug. I'd also be happy to hook up a debugger here if someone can tell me what to look for.
Actually, it turns out that I had the security.mixed_content.block_active_content pref set in about:config.

Maybe this bug is INVALID then, since the UI for showing that the content has been blocked hasn't landed yet.

I'll let Tanvi or Brandon decide here.
Blocks: 62178
No longer blocks: 761227
Component: DOM: Workers → Security: UI
Summary: Unable to "Reconnect" in IRCCloud, clicking on "Reconnect" link does nothing → WebSockets and about:blank content triggers mixed-active-content blocking
This is because the Mixed Blocker Content patch in FF 18 (the one in bug 62178) decides if a resource is secure by checking if its an https resource.  In fact, other types of resources are secure as well, and should also be allowed (ex: wss).  I've made the following change locally to nsMixedContentBlocker.cpp and it was going to be part of bug 782654:

-  // Check the parent scheme. If it is not an HTTPS page then mixed content
-  // restrictions do not apply.
-  bool parentIsHttps;
-  if (NS_FAILED(aRequestingLocation->SchemeIs("https", &parentIsHttps)) ||
-      !parentIsHttps) {
+  // Get the scheme of the sub-document resource to be requested. If it is
+  // an HTTPS load then mixed content doesn't apply.
+  bool isSchemeSafe;
+  //Note - Open to suggestions on the proper way of formatting this
+  if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) || isSchemeSafe ) {

I think we should separate this out from that patches in bug 782654 and uplift it to FF 18, since without it pages with websockets are unusable with the security.mixed_content.block_active_content pref set to true.
Blocks: 782654
No longer blocks: 62178
Here is the attachment that contains the code mentioned in comment 2:
https://bug782654.bugzilla.mozilla.org/attachment.cgi?id=671665

There was some feedback that we shouldn't use the actual strings "https", "about".  I'm open to suggestions on a better way to check these.  Create a separate function that goes through an array of acceptable protocols?
(In reply to Tanvi Vyas from comment #2)
> +  if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe))
> +       NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) ||

Note that the schemes should not include the trailing colon.

See http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#904 for an example of how this can be done using an array of these strings.
Jared, I believe this will fix the websockets issue you are having on irccloud.  Please confirm.  If this gets through review, I think we should uplift to 18.  Without this, all websites with websockets will not work currently when a user disables mixed active content.
Attachment #673481 - Flags: review?(bugs)
Comment on attachment 673481 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

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

::: content/base/src/nsMixedContentBlocker.cpp
@@ +134,5 @@
> +     "https",
> +     "about",
> +     "data",
> +     "javascript",
> +     "wss",

Can we add "blob" here too, and alphabetize this list?
I can confirm that the try build fixes the irccloud issue I was seeing before.
Comment on attachment 673481 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

What about nested urls, like jar:

This needs tests.
Attachment #673481 - Flags: review?(bugs) → review-
I am updating this to the following:
  static const char *const schemeWhiteList[] = {
     "https",
     "about",
     "blob",
     "chrome",
     "data",
     "jar",
     "javascript",
     "mailto",
     "resource",
     "wss",
     nullptr
  };

Note that https is at the top.  This is the most frequent case so I'd rather than run through https before about, blob, chrome, and data for performance reasons.  Yes that means it is not alphabetized, but does it really have to be?  I will post a patch shortly (testing locally first).

My next step will be to create tests for these cases, before updating the patch anymore.

I know it is not ideal to have a whitelist and it would be ideal to instead use URI flags.  I haven't found existing flags that would cover all the cases though.  Right now I have pseudo-code for some cases that looks something like this:

if(   !NS_URIChainHasFlags(aContentLocation,
       nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeIsSafe)  
   || !NS_URIChainHasFlags(aContentLocation,
       nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT, &schemeIsSafe) )
   return NS_ERROR_FAILURE;

if(isSchemeSafe)
  return NS_OK;
Well, jar can be safe or unsafe, I think.
(Though, I don't know whether http and https is supported with it.)
This version of the code has a bunch of comments and isn't ready to land.  Right now it includes whitelist of protocols allowed on https pages.  I've included commented code that would use protocol flags instead of a whitelist, that I will try out next.
Attachment #674056 - Attachment is obsolete: true
I've added four mochitests for four different protocols.  Two fall under the URI_IS_LOCAL_RESOURCE protocol flag.  One falls under URI_DOES_NOT_RETURN_DATA.  And the last one (about) doesn't fall into either of these yet, but should probably be in URI_IS_LOCAL_RESOURCE.

What additional tests should I write?  There are numerous protocols, and I'm not sure if we need a test for each one.

Thanks!
Attachment #675773 - Flags: review?(bugs)
Comment on attachment 675773 [details] [diff] [review]
Mochitests for data, about, mailto, and javascript protocols

I believe new patch is coming.
Attachment #675773 - Flags: review?(bugs)
Ready for review.  This is now using protocol flags instead of a whitelist of schemes.  This way, if additional schemes are added with one of the following flags, it will not be blocked by the mixed content blocker:
URI_IS_LOCAL_RESOURCE
URI_DOES_NOT_RETURN_DATA
URI_INHERITS_SECURITY_CONTEXT

I'm still working on finishing the tests.  Thanks!
Attachment #675772 - Attachment is obsolete: true
Attachment #677704 - Flags: review?(bugs)
Comment on attachment 677704 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

>+  /* Check Protocl Flags:
>+  * URI_INHERITS_SECURITY_CONTEXT - i.e:
>+  *  "javascript",
>+  * URI_IS_LOCAL_RESOURCE - i.e:
>+  *   "blob",
>+  *   "chrome",
>+  *   "data",
>+  *   "resource",
>+  *   "moz-icon",
>+  *   "moz-safe-about"
>+  *
>+  * URI_DOES_NOT_RETURN_DATA - i.e:
>+  *   "mailto"
>+  */
>+  bool isSchemeLocal = false;
>+  bool isSchemeExternal = false;
>+  bool isSchemeInherits = false;
>+  if (  NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &isSchemeLocal))  ||
>+        NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
So this would allow not only wss: loads but also ws: loads?


Coding style is
if (expression) {
  statement;
}
i.e. always use curly brackets and don't add space before nor after expression.

>+
>+  if(isSchemeLocal || isSchemeExternal || isSchemeInherits)
>+     return NS_OK;
>+
> NS_IMETHODIMP
> nsSafeAboutProtocolHandler::GetProtocolFlags(uint32_t *result)
> {
>-    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE;
>+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE | URI_IS_LOCAL_RESOURCE;
I don't quite understand this one.
Why certain about:* uris would have URI_IS_LOCAL_RESOURCE set but not all.
That is rather odd inconsistency.


I wonder... should we add a new protocol flag for this case. Or just whitelist protocols for now.
Attachment #677704 - Flags: review?(bugs) → review-
> >+  if (  NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &isSchemeLocal))  ||
> >+        NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
> So this would allow not only wss: loads but also ws: loads?
Er, I missed if (aContentType == nsIContentPolicy::TYPE_DOCUMENT || aContentType == nsIContentPolicy::TYPE_WEBSOCKET) {
which I don't understand.
Why is ws: load ok? (wss: sure is)
Ah, it is per websocket spec.
Could you replace the comment // Websockets code requires same scheme
with something like
// Creating unsecure websocket connections in a secure page is blocked already in websocket constructor.
I have updated the patch with the review comments with the coding style change for the if() statement and the comment change for allowing TYPE_WEBSOCKET.

For the about protocol, there are some about: resources that are allowed and some others (where the safe ones are distinguished by moz-about-safe).  More details on this can be found at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#82.  A resource isSafe if it has the about specific protocol flag nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT.  The change to nsAboutProtocolHandler adds the URI_IS_LOCAL_RESOURCE flag to nsSafeAboutProtocolHandler::GetProtocolFlags (not nsAboutProtocolHandler::GetProtocolFlags)
Attachment #677704 - Attachment is obsolete: true
Attachment #678926 - Flags: review?(bugs)
Attached patch Mochitests (obsolete) — Splinter Review
I've written 8 tests.

wss - allowed in nsMixedContentBlocker.cpp by checking for TYPE_WEBSOCKET

about (ex: about:blank)
unsafe about (ex: about:config) - Should not load since it doesn't have the URI_IS_LOCAL_RESOURCE flag

data, resource, and moz-icon - all have the URI_IS_LOCAL_RESOURCE flag

mailto - has the URI_DOES_NOT_RETURN_DATA flag

javascript - has the URI_INHERITS_SECURITY_CONTEXT flag.

Olli - I have one question about using a for loop for the about and javascript tests which can be done in a generic_iframe, or separating them out.  I have the separated out tests commented in the code and can remove them once I hear your preference.  Thanks!
Attachment #675773 - Attachment is obsolete: true
Attachment #678932 - Flags: review?(bugs)
Add additional secure schemes to nsMixedContentBlocker.cpp

I have updated the patch with the review comments with the coding style change for the if() statement and the comment change for allowing TYPE_WEBSOCKET.

For the about protocol, there are some about: resources that are allowed and some others (where the safe ones are distinguished by moz-about-safe).  More details on this can be found at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#82.  A resource isSafe if it has the about specific protocol flag nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT.  The change to nsAboutProtocolHandler adds the URI_IS_LOCAL_RESOURCE flag to nsSafeAboutProtocolHandler::GetProtocolFlags (not nsAboutProtocolHandler::GetProtocolFlags)
Attachment #678926 - Attachment is obsolete: true
Attachment #678926 - Flags: review?(bugs)
Attachment #678937 - Flags: review?(bugs)
(In reply to Tanvi Vyas from comment #23)
> Push to try: https://tbpl.mozilla.org/?tree=Try&rev=739b3b326556

Something is wrong with my websocket server, but I can't figure out what.  file_mixed_content_main_bug803225_websocket_wsh.py is almost identical to https://mxr.mozilla.org/mozilla-central/source/content/base/test/file_websocket_hello_wsh.py.  Is there another file that needs to be modified in addition to creating the websocket server file?
(In reply to Tanvi Vyas from comment #24)
> (In reply to Tanvi Vyas from comment #23)
> > Push to try: https://tbpl.mozilla.org/?tree=Try&rev=739b3b326556
> 
> Something is wrong with my websocket server, but I can't figure out what. 
> file_mixed_content_main_bug803225_websocket_wsh.py is almost identical to
> https://mxr.mozilla.org/mozilla-central/source/content/base/test/
> file_websocket_hello_wsh.py.  Is there another file that needs to be
> modified in addition to creating the websocket server file?

The websocket test passes locally, but not on try.
Comment on attachment 678937 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

>   // Get the scheme of the sub-document resource to be requested. If it is
>-  // an HTTPS load then mixed content doesn't apply.
>-  bool isHttps;
>+  // an HTTPS or secure load then mixed content doesn't apply  Note that we do
Missing . after apply.

>NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
URI_DOES_NOT_RETURN_DATA doesn't mean external, so could you change the variable name.

> NS_IMETHODIMP
> nsSafeAboutProtocolHandler::GetProtocolFlags(uint32_t *result)
> {
>-    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE;
>+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE | URI_IS_LOCAL_RESOURCE;
>     return NS_OK;
> }
This still feels like a misuse of URI_IS_LOCAL_RESOURCE to me.


Also, I think this patch is still missing jar: handling.
That requires dealing with nested urls.
Attachment #678937 - Flags: review?(bugs) → review-
Oh, hmm, is jar: handling not supported anymore.
At least jar:http:// gives security error, but what about jar:https://
So then the r- is really about use of URI_IS_LOCAL_RESOURCE.
Could we add a new flag to explicitly allow mixed content?
Comment on attachment 678932 [details] [diff] [review]
Mochitests

># HG changeset patch
># Parent 5b56e1a8686db262b9027eaec7d7f1a3645e0545
># User Tanvi Vyas <tvyas@mozilla.com>
>Bug 803225 - Test different protocols that are allowed in nsMixedContentBlocker.cpp
>
>diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -578,16 +578,20 @@ MOCHITEST_FILES_B = \
> 		test_XHR_anon.html \
> 		file_XHR_anon.sjs \
> 		test_XHR_system.html \
> 		test_XHR_parameters.html \
> 		test_ipc_messagemanager_blob.html \
> 		test_mixed_content_blocker.html \
> 		file_mixed_content_main.html \
> 		file_mixed_content_server.sjs \
>+		test_mixed_content_blocker_bug803225.html \
>+		file_mixed_content_main_bug803225.html \
>+		file_mixed_content_main_bug803225_websocket_wsh.py \
>+		bug803225_test_mailto.html \
> 		test_bug789856.html \
> 		$(NULL)
> 
> # OOP tests don't work on Windows (bug 763081) or native-fennec
> # (see Bug 774939)
> ifneq ($(OS_ARCH),WINNT)
> ifndef MOZ_ANDROID_OMTC
> MOCHITEST_FILES_B += \
>diff --git a/content/base/test/bug803225_test_mailto.html b/content/base/test/bug803225_test_mailto.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/bug803225_test_mailto.html
>@@ -0,0 +1,13 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+Tests for Mixed Content Blocker - Mailto Protocol Compose Page
>+https://bugzilla.mozilla.org/show_bug.cgi?id=803225
>+-->
>+<head> <meta charset="utf-8"> 
>+</head>
>+<body>
>+Hello
>+<script>window.close();</script>
>+</body>
>+</html>
>diff --git a/content/base/test/file_mixed_content_main_bug803225.html b/content/base/test/file_mixed_content_main_bug803225.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/file_mixed_content_main_bug803225.html
>@@ -0,0 +1,217 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+Tests for Mixed Content Blocker - Allowed Protocols
>+https://bugzilla.mozilla.org/show_bug.cgi?id=803225
>+-->
>+<head>
>+  <meta charset="utf-8">
>+  <title>Tests for Bug 62178</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+</head>
>+<body>
>+<div id="testContent"></div>
>+
>+<!-- Test additional schemes the Mixed Content Blocker should not block
>+     "about" protocol URIs that are URI_SAFE_FOR_UNTRUSTED_CONTENT (moz-safe-about; see nsAboutProtocolHandler::NewURI
>+     "data",
>+     "javascript",
>+     "mailto",
>+     "resource",
>+     "moz-icon",
>+     "wss"
>+-->
>+
>+<script>
>+
>+  //For tests that require setTimeout, set the maximum polling time to 100 x 100ms = 10 seconds.
>+  var MAX_COUNT = 100;
>+  var TIMEOUT_INTERVAL = 100;
>+
>+  var testContent = document.getElementById("testContent");
>+
>+  // Test 1 & 2: about and javascript protcols within an iframe
>+  var data = Array(2,2);
>+  var protocols = [
>+                    ["about", ""], //When no source is specified, the frame gets a source of about:blank
>+                    ["javascript", "javascript:document.open();document.write='<h1>SUCCESS</h1>';document.close();"],
>+                  ];
>+  for(var i=0; i < 2; i++)
s/2/protocols.length/
That would make the code easier to read I think


>+  /* smaug - do you prefer using the above for loop, or would you like individual tests instead?
Loop is ok

>+  // Test 1: about protocol within an iframe
>+  var about_frame = document.createElement("iframe");
>+  //When no source is specified, the frame gets a source of about:blank
>+  about_frame.src = "";
>+  about_frame.name="about_protocol";
>+  about_frame.onload = function() {
>+    parent.postMessage({"test": "about", "msg": "resource with about protocol loaded"}, "http://mochi.test:8888");
>+  }
>+  about_frame.onerror = function() {
>+    parent.postMessage({"test": "about", "msg": "resource with about protocol did not load"}, "http://mochi.test:8888");
>+  }
>+  testContent.appendChild(about_frame);
>+  */
>+  /*
>+  // Test 2: javascript protocol within an iframe
>+  var javascript_frame = document.createElement("iframe");
>+  javascript_frame.src = "javascript:document.open();document.write='<h1>SUCCESS</h1>';document.close();";
>+  //Do we need to open and close the document?  We could do this instead ->
>+  //javascript_frame.src = "javascript:document.write='<h1>SUCCESS</h1>';";
>+  javascript_frame.name="javascript_protocol";
>+  javascript_frame.onload = function() {
>+    parent.postMessage({"test": "javascript", "msg": "resource with javascript protocol loaded"}, "http://mochi.test:8888");
>+  }
>+  javascript_frame.onerror = function() {
>+    parent.postMessage({"test": "javascript", "msg": "resource with javascript protocol did not load"}, "http://mochi.test:8888");
>+  };
>+  testContent.appendChild(javascript_frame);
>+  */
Hmm, does the error event actually work in these cases? If we would
block javascript: or about: I don't think error event would be dispatched.
So I think onerror cases can be removed, probably everywhere.
checkTestsCompleted() should anyway check that all the tests were run successfully.


That thing changed, r=me

>+    //if the testsToRun are all completed, chnage the pref and run the tests again until we have cycled through all the prefs.
change
Attachment #678932 - Flags: review?(bugs) → review+
Fixed Olli's comments in comment 26.  Added a new URI Protocol Flag called URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT that can be used in case we want to force a scheme to be secure that does not fall into URI_DOES_NOT_RETURN_DATA, URI_IS_LOCAL_RESOURCE, or URI_INHERITS_SECURITY_CONTEXT (ex: https and moz-safe-about).

Resubmitting for review.
Attachment #678937 - Attachment is obsolete: true
Attachment #679419 - Flags: review?(bugs)
Attached patch Mochitests (obsolete) — Splinter Review
Fixed the review comments in comment 30, except for removing the onerror events...

checkTestsCompleted is dependent on file_mixed_content_main_bug803225.html to send a post-message.  The onerror event does fire, as you can see with the unsafe about test, where we try to load an iframe with src about:config.  The onerror event fires, and sends a message "resource with unsafe about protocol did not load" to the parent.  The parent then marks that test as completed by setting unsafe_about to true in testsToRun.  (Note that it also goes through a loop and runs this test 4 times, once for each setting combination.)

I tested that moz-icon, safe about, javascript, resource do fire the onerror when the resource does not load.  I did this by removing my patch to nsMixedContentBlocker and then running the tests.  Hence, I'm leaving in the onerror events.

I found a few other quirks while testing (don't need to append child for moz-icon, replaced name of data test with data_protocol, to avoid overloading "data".  I also found that mailto: URIs seem to have a content type of TYPE_DOCUMENT, and hence are allowed with or without the patch to nsMixedContentBlocker.  

Can I carry over the r+?
Attachment #678932 - Attachment is obsolete: true
Attachment #679498 - Flags: review?(bugs)
Assignee: nobody → tanvi
Comment on attachment 679498 [details] [diff] [review]
Mochitests

Make sure to run this in tryserver few time to ensure we don't
cause random orange.
Attachment #679498 - Flags: review?(bugs) → review+
Comment on attachment 679419 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp


>+
>+  if(isSchemeLocal || isSchemeNoReturnData || isSchemeInherits || isSchemeSecure)
>+     return NS_OK;

if (expr) {
  stmt;
}

Always use {} and 2 space indentation.

>+    /**
>+     * URI is secure to load in an https page and should not be blocked
>+     * by nsMixedContentBlocker
>+     */
>+    const unsigned long URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT = (1<<17);
>+
17 has been used already. Use 18.
nsIProtocolHandler and nsAboutProtocolHandler.cpp changes need Necko peer review.
Attachment #679419 - Flags: review?(bugs) → review+
Rebased addressing Olli's comment 35.  Carrying over r+.
Attachment #679419 - Attachment is obsolete: true
Attachment #680145 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 679498 [details] [diff] [review]
> Mochitests
> 
> Make sure to run this in tryserver few time to ensure we don't
> cause random orange.

New push to try - https://tbpl.mozilla.org/?tree=Try&rev=1f325dfdc693
Comment on attachment 679419 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

Per Olli, requesting Necko Peer Review for the new flag in nsIProtocolHandler.idl.

Thanks Olli for your reviews!
Attachment #679419 - Flags: review+ → review?(bzbarsky)
Er... do you really want review from me on the obsolete patch?  Or on something else?
Comment on attachment 680145 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

(In reply to Boris Zbarsky (:bz) from comment #39)
> Er... do you really want review from me on the obsolete patch?  Or on
> something else?

Oops!  Sorry about that; I marked the wrong patch.  This is the patch I'd like you to review, specifically for the change in nsIProtocolHandler.idl.  Thanks!
Attachment #680145 - Flags: review?(bzbarsky)
Attachment #679419 - Flags: review?(bzbarsky)
Comment on attachment 680145 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

s/unsecure/insecure/ ?

s/aRequestLocation\.\./aRequestLocation./

The comment about "avoid calculating" makes no sense to me.  The check for aContentLocation happens _after_ we already checked aRequestLocation!

s/Protcl/Protocol/

All your "i.e" should be "e.g."

Why are we leaving the "https" special-case?  Seems like it would be covered by the protocol flag checks, no?

You need a space after your "if".

The booleans on the stack there should probably be "schemeLocal" and so forth, without the extra "is".

r=me with those nits picked, though I'd like to see the updated patch.
Attachment #680145 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #41)
> Comment on attachment 680145 [details] [diff] [review]
> Add additional secure schemes to nsMixedContentBlocker.cpp
> 
> s/unsecure/insecure/ ?
>
Fixed

> s/aRequestLocation\.\./aRequestLocation./
>
Removed
 
> The comment about "avoid calculating" makes no sense to me.  The check for
> aContentLocation happens _after_ we already checked aRequestLocation!
> 
Sorry, that will happen in a separate patch, and I accidentally left in that comment.

> s/Protcl/Protocol/
> 
Fixed

> All your "i.e" should be "e.g."
>
Done
 
> Why are we leaving the "https" special-case?  Seems like it would be covered
> by the protocol flag checks, no?
> 
Yes, you are right.  We don't need a separate case for https since it is covered in URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT.  I took the https special case out and updated the comments.

> You need a space after your "if".
> 
Fixed in 3 places.

> The booleans on the stack there should probably be "schemeLocal" and so
> forth, without the extra "is".
> 
Done

> r=me with those nits picked, though I'd like to see the updated patch.
Updated patch attached.  Carrying over r+ from smaug.  BZ, let me know if you are okay with this patch and then we can land it :)
Attachment #680145 - Attachment is obsolete: true
Attachment #681289 - Flags: review+
Comment on attachment 679498 [details] [diff] [review]
Mochitests

Per comment 41 and 42, waiting for BZ's final approval.

Fresh try run - https://tbpl.mozilla.org/?tree=Try&rev=9719ade02d5e
Attachment #679498 - Flags: review?(bzbarsky)
Comment on attachment 681289 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

Looks good.
Attachment #681289 - Flags: review+
Comment on attachment 679498 [details] [diff] [review]
Mochitests

Why do you need to limit the polling more than the normal test harness timeout does?  Seems like that will just trigger random orange, no?

r=me with that removed or very clearly explained.
Attachment #679498 - Flags: review?(bzbarsky) → review+
Attached patch Mochitests (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #45)
> Comment on attachment 679498 [details] [diff] [review]
> Mochitests
> 
> Why do you need to limit the polling more than the normal test harness
> timeout does?  Seems like that will just trigger random orange, no?
> 
> r=me with that removed or very clearly explained.

There are 8 mochitests that are tested 4 times each (with 4 different setting combinations for blocking/allowing display and active content).  For situations where an error event is not an available option, we need to set a polling timeout to determine whether a resource loaded or not.  If we do not set a polling time limit, the test will get stuck on a specific resource that doesn't load and the remaining tests won't execute.

This is what we used in the mochitest for bug 62178 as well.  In bug 62178 it was necessary to poll since sometimes the failure of a resource to load would result in a Passing Test (since that resource wasn't supposed to load) and we would want to continue on to the next test.

For this bug, we are using polling for the mailto and data protocols, both which are expected to load.  If we do not poll and the data protocol fails to load, the test will timeout and the remaining 24+ tests won't execute.

The exact max time limit (10 seconds) was proposed by Olli.  You can also see more details on our discussion about polling in https://bugzilla.mozilla.org/show_bug.cgi?id=62178#c229 and comment 230. 

I have added a couple comments to the code in the places where we poll to indicate why polling is necessary.

Carrying over the r+'s.  Can we land this?
Attachment #679498 - Attachment is obsolete: true
Attachment #681734 - Flags: review+
Attachment #681734 - Flags: review+
> If we do not set a polling time limit, the test will get stuck on a specific resource
> that doesn't load and the remaining tests won't execute.

And then eventually the harness timeout will kick in and this test will be marked as failing and turn the tree orange.  Why is this a problem?  If we were expecting tests to pass on not loading it would be, but as you said, that's not the case here.

> If we do not poll and the data protocol fails to load, the test will timeout and the
> remaining 24+ tests won't execute.

Sure.  Why is that a problem?

Given your description above, I think you should take the polling out.  It'll just lead to random oranges, with no perceptable benefit I see.
Attached patch Mochitests (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #47)
> Given your description above, I think you should take the polling out. 
> It'll just lead to random oranges, with no perceptable benefit I see.

Okay, removed the polling.

I think this is ready to land.  Any volunteers to land it?
Attachment #682117 - Flags: review+
Attachment #682117 - Flags: review+
Attachment #681734 - Attachment is obsolete: true
Comment on attachment 682117 [details] [diff] [review]
Mochitests

So I'm also a bit confused about the dataProtocolStatus bit.  It's polling with a comment about a bug about onerror... but that bug is fixed.  Does that bit still need to poll at all?
(In reply to Tanvi Vyas from comment #48)
> Created attachment 682117 [details] [diff] [review]
> Mochitests
> 
> (In reply to Boris Zbarsky (:bz) from comment #47)
> > Given your description above, I think you should take the polling out. 
> > It'll just lead to random oranges, with no perceptable benefit I see.
> 
> Okay, removed the polling.
> 
Sorry, I meant that I removed the MAX_TIMEOUT for the polling.  So now it will poll until success or timeout.
(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 682117 [details] [diff] [review]
> Mochitests
> 
> So I'm also a bit confused about the dataProtocolStatus bit.  It's polling
> with a comment about a bug about onerror... but that bug is fixed.  Does
> that bit still need to poll at all?

I am looking into this.
Attached patch Mochitests (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 682117 [details] [diff] [review]
> Mochitests
> 
> So I'm also a bit confused about the dataProtocolStatus bit.  It's polling
> with a comment about a bug about onerror... but that bug is fixed.  Does
> that bit still need to poll at all?

You are right, since bug 789856 is now fixed, we don't need to poll for the data_protocol <script> and can now use the error event.  I've changed the data protocol tests accordingly.
Attachment #682117 - Attachment is obsolete: true
Attachment #682174 - Flags: review+
Attached patch MochitestsSplinter Review
Fixing a typo.
Attachment #682174 - Attachment is obsolete: true
Attachment #682218 - Flags: review+
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out for mochitest failures. I knew I should have run this through Try first...
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4a9a61a335

https://tbpl.mozilla.org/php/getParsedLog.php?id=17088443&tree=Mozilla-Inbound

40393 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40401 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40409 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40417 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load

Please run this through Try before requesting checkin again.
The patch that was pushed was not the most recent patch, it is for an obsoleted patch. Here is the try push for the most recent patch on this bug: https://tbpl.mozilla.org/?tree=Try&rev=47a49ff238f5
Keywords: checkin-needed
Target Milestone: --- → mozilla19
Blocks: 812649
https://hg.mozilla.org/mozilla-central/rev/87e53d2e1e53
https://hg.mozilla.org/mozilla-central/rev/158f2345bf97

Sorry about the mix-up yesterday. No clue how I managed to land the wrong mochitest patch.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 822366
Blocks: 822367
Blocks: 822371
Blocks: 822373
No longer blocks: 466391
Blocks: 840938
Depends on: 842146
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.