Closed
Bug 803225
Opened 13 years ago
Closed 13 years ago
WebSockets and about:blank content triggers mixed-active-content blocking
Categories
(Core Graveyard :: Security: UI, defect)
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.
Reporter | ||
Comment 1•13 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Summary: Unable to "Reconnect" in IRCCloud, clicking on "Reconnect" link does nothing → WebSockets and about:blank content triggers mixed-active-content blocking
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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?
Reporter | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
Push to try: https://tbpl.mozilla.org/?tree=Try&rev=44f97ab27aeb
Reporter | ||
Comment 7•13 years ago
|
||
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?
Reporter | ||
Comment 8•13 years ago
|
||
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-
Assignee | ||
Comment 10•13 years ago
|
||
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;
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #673481 -
Attachment is obsolete: true
Well, jar can be safe or unsafe, I think.
(Though, I don't know whether http and https is supported with it.)
Assignee | ||
Comment 13•13 years ago
|
||
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
Assignee | ||
Comment 14•13 years ago
|
||
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)
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 20•13 years ago
|
||
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)
Assignee | ||
Comment 21•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
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)
Assignee | ||
Comment 23•13 years ago
|
||
Push to try: https://tbpl.mozilla.org/?tree=Try&rev=739b3b326556
Assignee | ||
Comment 24•13 years ago
|
||
(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?
Assignee | ||
Comment 25•13 years ago
|
||
(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://
Looks like jar:https://people.mozilla.org/~opettay/jartest.jar!/jartest.html doesn't work either.
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+
Assignee | ||
Comment 31•13 years ago
|
||
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)
Assignee | ||
Comment 32•13 years ago
|
||
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 | ||
Comment 33•13 years ago
|
||
Push to try: https://tbpl.mozilla.org/?tree=Try&rev=18b990023b34
Assignee | ||
Updated•13 years ago
|
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+
Assignee | ||
Comment 36•13 years ago
|
||
Rebased addressing Olli's comment 35. Carrying over r+.
Attachment #679419 -
Attachment is obsolete: true
Attachment #680145 -
Flags: review+
Assignee | ||
Comment 37•13 years ago
|
||
(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
Assignee | ||
Comment 38•13 years ago
|
||
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)
![]() |
||
Comment 39•13 years ago
|
||
Er... do you really want review from me on the obsolete patch? Or on something else?
Assignee | ||
Comment 40•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #679419 -
Flags: review?(bzbarsky)
![]() |
||
Comment 41•13 years ago
|
||
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+
Assignee | ||
Comment 42•13 years ago
|
||
(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+
Assignee | ||
Comment 43•13 years ago
|
||
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 44•13 years ago
|
||
Comment on attachment 681289 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp
Looks good.
Attachment #681289 -
Flags: review+
![]() |
||
Comment 45•13 years ago
|
||
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+
Assignee | ||
Comment 46•13 years ago
|
||
(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+
Assignee | ||
Updated•13 years ago
|
Attachment #681734 -
Flags: review+
![]() |
||
Comment 47•13 years ago
|
||
> 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.
Assignee | ||
Comment 48•13 years ago
|
||
(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+
![]() |
||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•13 years ago
|
Attachment #682117 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #681734 -
Attachment is obsolete: true
![]() |
||
Comment 49•13 years ago
|
||
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?
Assignee | ||
Comment 50•13 years ago
|
||
(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.
Assignee | ||
Comment 51•13 years ago
|
||
(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.
Assignee | ||
Comment 52•13 years ago
|
||
(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+
Assignee | ||
Comment 53•13 years ago
|
||
Fixing a typo.
Attachment #682174 -
Attachment is obsolete: true
Attachment #682218 -
Flags: review+
Comment 54•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df27137cb7d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdf8bbcd02f
Flags: in-testsuite+
Keywords: checkin-needed
Comment 55•13 years ago
|
||
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.
Comment 56•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 57•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e53d2e1e53
https://hg.mozilla.org/integration/mozilla-inbound/rev/158f2345bf97
Keywords: checkin-needed
Updated•13 years ago
|
Target Milestone: --- → mozilla19
Comment 58•13 years ago
|
||
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: 13 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•