1195173 - Use channel->asyncOpen2 layout/style/Loader.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_loader.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_test_updates.patch

The stylesheet test was failing because we now moved the security checks to asyncOpen2(). I think that test wasn't very robust and the better alternative is to use onloader() and onerror() events which are available for link tags [1].

I also updated the test for scripts (not necessary for this bug to work though) to rely on onerror. I don't have a better alternative for object though, I think that one has to remain as it is.

Should work right? Unless there was a reason this test didn't rely on onerror/onloader for scripts and stylesheets in the first place :-)

[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77e80cf1f8f4

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686434 [details] [diff] [review]
bug_1195173_asyncopen2_layout_style_loader.patch

>-                                        nsIScriptSecurityManager::ALLOW_CHROME);

I don't see SEC_ALLOW_CHROME on the new codepaths.  Shouldn't it be there?

r=me with that fixed, I think.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_loader.patch

> I don't see SEC_ALLOW_CHROME on the new codepaths.  Shouldn't it be there?

Indeed, that should be there. That caused lots of tests to fail actually.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: failing_tests.diff

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_unit_test_updates.patch

Boris, seems like there is still one test failing, which is:
> parser/xml/test/unit/test_sanitizer.js

The test calls ParserUtils.sanitize() which then creates a document of 'about:blank' but never sets a loadgroup on it. Hence the assertion in Loader.cpp gets triggered. (See [1] for a detailed list of failing tests and also the stacktrace underneath).

I don't think we can query a loadgroup for those cases. One possible solution would be to opt-out of the assertion if the URI is 'about:blank'. I am not sure if that is the right fix though. Happy to incorporate a better solution. 

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8686847

 0:00.64 PROCESS_OUTPUT: Thread-1 (pid:5600) "Assertion failure: false, at /home/ckerschb/moz/mc/dom/base/nsDocument.cpp:1481"
 0:03.89 PROCESS_OUTPUT: Thread-1 (pid:5600) "#01: nsDocument (/home/ckerschb/moz/mc/dom/base/nsDocument.cpp:1481)"
 0:06.15 PROCESS_OUTPUT: Thread-1 (pid:5600) "#02: nsHTMLDocument (/home/ckerschb/moz/mc/dom/html/nsHTMLDocument.cpp:174)"
 0:06.15 PROCESS_OUTPUT: Thread-1 (pid:5600) "#03: NS_NewHTMLDocument(nsIDocument**, bool) (/home/ckerschb/moz/mc/dom/html/nsHTMLDocument.cpp:154)"
 0:07.93 PROCESS_OUTPUT: Thread-1 (pid:5600) "#04: NS_NewDOMDocument(nsIDOMDocument**, nsAString_internal const&, nsAString_internal const&, nsIDOMDocumentType*, nsIURI*, nsIURI*, nsIPrincipal*, bool, nsIGlobalObject*, DocumentFlavor) (/home/ckerschb/moz/mc/dom/xml/XMLDocument.cpp:88)"
 0:07.96 PROCESS_OUTPUT: Thread-1 (pid:5600) "#05: nsParserUtils::Sanitize(nsAString_internal const&, unsigned int, nsAString_internal&) (/home/ckerschb/moz/mc/parser/html/nsParserUtils.cpp:81)"

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8686436 [details] [diff] [review]
bug_1195173_asyncopen2_layout_style_test_updates.patch

Nice changes!  Thanks Christoph!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686848 [details] [diff] [review]
bug_1195173_unit_test_updates.patch

Why is this not a problem on tip right now?  As far as I can tell, the assert is there...

Is it because we never reach that code because the current security check comes way before this part and denies the load?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Speaking of which, since we now do the security check _after_ creating the stylesheet, not before, what is the behavior, with this patch, of something like this:

  <link rel="stylesheet" href="file:///home/whoever/test.css">
  <script>
    onload = function() {
      alert(document.styleSheets[0].cssRules);
    }
  </script>

Do we at least throw an exception on the .cssRules access?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_tests.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> Speaking of which, since we now do the security check _after_ creating the
> stylesheet, not before, what is the behavior, with this patch, of something
> like this:
> 
>   <link rel="stylesheet" href="file:///home/whoever/test.css">
>   <script>
>     onload = function() {
>       alert(document.styleSheets[0].cssRules);
>     }
>   </script>
> 
> Do we at least throw an exception on the .cssRules access?

Yes, exception is thrown in that case. Providing a testcase, up to you if we want that to land. Maybe not a bad idea after all.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Is it because we never reach that code because the current security check
> comes way before this part and denies the load?

That is absolutely correct. The security check happens way earlier at the moment so the load gets blocked before we reach the assertion.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8687321 [details] [diff] [review]
bug_1195173_asyncopen2_tests.patch

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686848 [details] [diff] [review]
bug_1195173_unit_test_updates.patch

I think we should just bail out with an error if there is no loadgroup instead.  This assertion really makes no sense if we're doing security checks after this point, and I'd rather not rely on particular URIs here.  But being asked to do a load for a document with no loadgroup indicates that something weird is up and we should just not waste time discovering what it is and bail out instead.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_unit_test_updates.patch

(In reply to Boris Zbarsky [:bz] from comment #14)
> Comment on attachment 8686848 [details] [diff] [review]
> bug_1195173_unit_test_updates.patch
> 
> I think we should just bail out with an error if there is no loadgroup
> instead.  This assertion really makes no sense if we're doing security
> checks after this point, and I'd rather not rely on particular URIs here. 
> But being asked to do a load for a document with no loadgroup indicates that
> something weird is up and we should just not waste time discovering what it
> is and bail out instead.

Agreed - I can fold that patch into the main patch once r+ed - thanks for your help!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8687509 [details] [diff] [review]
bug_1195173_unit_test_updates.patch

No, you need to do three things before returning if you want to bail out:

1)  ifdef debug, reset mSyncCallback to false.  You can avoid this part if you
    just reorder this block with the #ifdef block above it.
2)  LOG_ERROR what's going on.
3)  Call SheetComplete(aLoadData, NS_ERROR_UNEXPECTED).  Otherwise you leak
    aLoadData.

See, for example, how we handle NS_NewChannel failure in this same method.

r=me with the above fixed.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_loader.patch

Thanks Boris; just incorporated Boris' suggestions and qfolded the patches together. Carrying over r+.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Kris, within this bug we are converting stylesheet loads to use AsyncOpen2(). Doing so I encountered that test [0] (see also complete TRY push [1]) is failing. I traced down the root cause of the problem and it seems that without my patches we are entering this else-branch [2] whereas with my patches applied we are entering the if-branch here [3].

When converting stylesheet loads to use asyncOpen2() we are setting a securityFlag with style/Loader.cpp. So from now on aLoadInfo->GetSecurityMode() will always return true here [3]. This test [0] installs an app and injects a css into a page which according to the comment within ExtensionProtocolHandler.cpp should always be loaded in sync.

Here are my 2 questions:
1) Based on what decision did you decide to use this if condition [3]. It seems it's only working right now because we haven't converted all callsites to use asyncOpen2() and set a securityFlag different than SEC_NORMAL. Can we use a different if-condition so that stylesheet loads for extension content scripts are styled loaded in sync?

2) Can we use ->Open2() instead of ->Open() in the else-branch, because ->Open() will be deprecated soon. If we need to bypass security checks for ->Open2() we can definitely figure something. E.g. call loadinfo->SetInitialSecurityChecksDone() before calling Open2() on it.

[0] http://mxr.mozilla.org/mozilla-central/source/dom/apps/tests/test_app_addons.html?force=1#153
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=56dadff1b0af
[2] http://hg.mozilla.org/mozilla-central/annotate/7104d650a97d/netwerk/protocol/res/ExtensionProtocolHandler.cpp#l130
[3] http://hg.mozilla.org/mozilla-central/annotate/7104d650a97d/netwerk/protocol/res/ExtensionProtocolHandler.cpp#l107

Comment 19

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)
> 1) Based on what decision did you decide to use this if condition [3]. It
> seems it's only working right now because we haven't converted all callsites
> to use asyncOpen2() and set a securityFlag different than SEC_NORMAL. Can we
> use a different if-condition so that stylesheet loads for extension content
> scripts are styled loaded in sync?

The async case should only be absolutely necessary when the security mode is
SEC_REQUIRE_CORS_DATA_INHERITS. I chose the if condition mainly to minimize
the cases where we wind up in the sync branch, since the only time the
channels really need to be sync now is when we load stylesheets using
loadSheetUsingURIString.

At this point, we should probably just check for SEC_REQUIRE_CORS_DATA_INHERITS
to minimize the chances of ending up in async mode when we need to be sync.
That unfortunately means we'll wind up with a sync channel in a bunch of cases
where we don't need one, but I don't have a better solution short of
implementing a new channel type, or making loadSheetUsingURIString async.

> 2) Can we use ->Open2() instead of ->Open() in the else-branch, because
> ->Open() will be deprecated soon. If we need to bypass security checks for
> ->Open2() we can definitely figure something. E.g. call
> loadinfo->SetInitialSecurityChecksDone() before calling Open2() on it.

We should be able to use Open2(), yes. I went with Open() because we were only
hitting that branch when there were no security checks. If we change that, I
don't think there should be any problems with Open2().

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] from comment #19)
> We should be able to use Open2(), yes. I went with Open() because we were
> only
> hitting that branch when there were no security checks. If we change that, I
> don't think there should be any problems with Open2().

Thanks for the speedy response, I will update those changes within Bug 1242611 and flag you for review.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_extensionprotocolhandler_open2.patch

Kris, it seems the changes for the ExtensionProtocolHandler.cpp have to land together with the patches in that bug because we have assertions that open2 is not called with the right securiy flags. Hence I will mark Bug 1242611 as INVALID.

Anyway, as discussed in previous comments, we should call asyncOpen2() if we have to enforce CORS othwerwise we should fall back to calling open2(), but also here we should be careful in case a channel might not have a loadInfo we still have to call Open() instead of Open2(). Once we deprecate Open() completely I'll clean those bits up. We have similar if-else branches in other parts of the code to make sure we don't crash in case we have no loadinfo on a channel.

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8711789 [details] [diff] [review]
bug_1195173_extensionprotocolhandler_open2.patch

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

Looks good. Thanks.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp
@@ +105,5 @@
>  
>    nsCOMPtr<nsIInputStream> inputStream;
> +  if (aLoadInfo &&
> +      aLoadInfo->GetSecurityMode() == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
> +    // if the channel needs to enforce CORS, we need to open the channel async

Please capitalize and add period.

@@ +134,5 @@
>      nsCOMPtr<nsIInputStream> sourceStream;
> +    if (aLoadInfo && aLoadInfo->GetEnforceSecurity()) {
> +      rv = (*result)->Open2(getter_AddRefs(sourceStream));
> +    }
> +    else {

Please move the else to the same line as the }

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_csp_changes.patch

Boris, in an earlier patch within this bug we removed CheckLoadAllowed() from layout/style/Loader.cpp, but since we now have support for Meta CSP we have to bring back a CSP check to make sure that CSP is consulted in case a page uses doc_write(meta CSP) which would deny a preloade style from being applied to the page. To be precise, the preload should succeed, but the actual load (reuse) should be blocked by CSP. (We have test_docwrite_meta.html to make sure we are blocking such a load).

I think it makes to have the CSP check within LoadStyleLink(), but alternatively we could add it to
> bool KeyEquals(const URIPrincipalReferrerPolicyAndCORSModeHashKey* aKey)
where we check whether things changed between the preload and the actual load. What do you think?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_test_updates.patch

It seems that test 'nine' is slightly wrong because a cross origin request can not be anonymous and require credentials at the same time, see also the *.sjs [1]. I think removing |class="reverse"| is the right fix for the test.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/test/bug732209-css.sjs#10

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8711792 [details] [diff] [review]
bug_1195173_csp_changes.patch

Doing this in LoadStyleLink would mean that we would NOT do it for @import, right?  But I'd think we do in fact want to do it for @import.

Which means we need to do it in some other centralized place.  The cache key might be ok, if the check is fast enough.  If it's not fast, we'll have to figure out some way to make it fast.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, and the other option, of course, is to bring back CheckLoadAllowed(), which was called from all the relevant spots.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #25)
> Comment on attachment 8711792 [details] [diff] [review]
> bug_1195173_csp_changes.patch
> 
> Doing this in LoadStyleLink would mean that we would NOT do it for @import,
> right?  But I'd think we do in fact want to do it for @import.

Is @import also preloaded? If that is the case, then yes, otherwise asyncOpen2() would block the load because CSP would be consultet there.

> Which means we need to do it in some other centralized place.  The cache key
> might be ok, if the check is fast enough.  If it's not fast, we'll have to
> figure out some way to make it fast.

How would we measure fast here?


In general I would rather avoid bringing back CheckLoadAllowed because we would do securityChecks twice for all stylesheet loads, which is what we should try to avoid in my opinion.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8711795 [details] [diff] [review]
bug_1195173_asyncopen2_layout_style_test_updates.patch

So the way this test is organized is like this:

1-3) Are loading same-origin, so the sheet loads.
4)   Loads cross-origin without a "crossorigin" attribute, so the sheet
     loads.
5,6) Load cross-origin with a "crossorigin" attribute, but without telling the
     server to send the correct headers to allow the load, so the sheet does NOT
     load.
7-9) Tell the server to send the right headers to only allow anonymous
     cross-origin loads when CORS is used.  CORS is not used for 7, used in
     anonymous mode for 8, used in credentials mode for 9.  So 7 and 8 should
     load but 9 should NOT load.
10-12) Same as 7-9, but telling the server to send the right headers to allow
       both anonymous and with-credentials loads.  All the sheets should load.

I'm not sure what "because a cross origin request can not be anonymous and require credentials at the same time" has to do with anything.  In item 9 of the test the browser tries to do a load involving credentials, but the server is instructed to tell the browser that it only allows anonymous loads.  This is a situation that can certainly arise and needs to be tested, and should cause the sheet to not load, of course.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Is @import also preloaded?

It can be, effectively, if the same URL appears both in a <link> and in an @import.  Please do add a test for this situation as needed.

> How would we measure fast here?

Good question.  Clearly hashing a URI is considered fast enough.  Parsing an arbitrarily long CSP as part of the hashtable lookup probably not.  But I assume that's not really going to happen here, right?

What sort of work _would_ happen during this key comparison?  I guess the main bad edge cases here are "any change to CSP from the <meta> tag disables preloads" and "we have some sort of complicated CSP-diffing algorithm that takes a long time".  I'm sure we can find something in between those, though.

> In general I would rather avoid bringing back CheckLoadAllowed because we would do securityChecks twice for
> all stylesheet loads, which is what we should try to avoid in my opinion.

Then you should include CSP state as cache key and make sure it's not too slow.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #29)
> > Is @import also preloaded?
> It can be, effectively, if the same URL appears both in a <link> and in an
> @import.  Please do add a test for this situation as needed.

Yes, that is indeed true.

> > How would we measure fast here?
> 
> Good question.  Clearly hashing a URI is considered fast enough.  Parsing an
> arbitrarily long CSP as part of the hashtable lookup probably not.  But I
> assume that's not really going to happen here, right?
> 
> What sort of work _would_ happen during this key comparison?  I guess the
> main bad edge cases here are "any change to CSP from the <meta> tag disables
> preloads" and "we have some sort of complicated CSP-diffing algorithm that
> takes a long time".  I'm sure we can find something in between those, though.

Currently the principal holds a preloadCSP and an actual CSP which gets applied once we hit ::bindToTree(). Saying that, within the KeyEquals() function we would only do a 'csp->ShouldLoad()' which should be sufficently fast. Also, it only happens if a page has a csp. To be precise, no parsing of CSP would happen inside KeyEquals.

Should we go with that approach? That would be my choice, but would like to hear your opinion. The other reason why I like that solution is, because KeyEquals is called from several places which already have test coverage, so we could be somehow sure we are not missing any corner case. Agreed?

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Boris, If I incorporate the csp check into KeyEquals() [1] then this assertion gets triggered [2], so proably after all maybe we should bring back CheckLoadAllowed but only perform a CSP check there if it's not a preload. All other securitychecks are checked by asyncOpen, what would do you say?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.h#86
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1860

Comment 32

[Boris Zbarsky [:bzbarsky]]

> then this assertion gets triggered [2]

Why?  How did you introduce the check?  Is it triggering for preloads that are not finding themselves in the hashtable because they're now checking with the "wrong" CSP?

> maybe we should bring back CheckLoadAllowed but only perform a CSP check there if it's not a preload

Here's a question.  How are we handling this setup for script preloads?

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_csp_changes.patch

(In reply to Boris Zbarsky [:bz] from comment #32)
> > then this assertion gets triggered [2]
> 
> Why?  How did you introduce the check?  Is it triggering for preloads that
> are not finding themselves in the hashtable because they're now checking
> with the "wrong" CSP?

Here is the patch. There is no csp for preloads (at least not for the testcase test_docwrite_meta.html) where we use doc.write(meta csp) to write a csp into the document). So for preloads we don't use a CSP (if we would have one, then it would be stored in 'preloadCSP' and not in the actual 'csp' which we query using ->GetCSP().) Hence I suppose that check in KeyEquals should be what we want.

> > maybe we should bring back CheckLoadAllowed but only perform a CSP check there if it's not a preload
> 
> Here's a question.  How are we handling this setup for script preloads?

Which setup? Currently in the code?

In the current codebase we have a preloadFlag to CheckLoadAllowed(). Within checkLoadAllowed we either call contentPolicies using TYPE_STYLE_PRELOAD or TYPE_STYLE. Later within nsCSPSerivce::ShouldLoad() we query the preload CSP based on the contentPolicyType. So nsCSPService queries and consults the preloadCSP in case of a preload and in addition the actual CSP [1] (because CSP might also be deliverd through a header at the same time as a meta csp).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#151

Comment 34

[Boris Zbarsky [:bzbarsky]]

> Here is the patch. 

OK, right.  That's not going to work, precisely because preloads won't find themselves in the hashtable.

So one option would be to store the CSP in use at the time of the load in the hashtable key or something, but this would then lead to precisely the "any change to CSP from the <meta> tag disables preloads" failure mode.

> Which setup?

What do we do for CSP for <script> elements and their preloads?

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #34)
> What do we do for CSP for <script> elements and their preloads?

For scripts we are actually calling CheckContentPolicy before we reuse the preload. The more I think about the more I think you are right and we should bring back some sort of CheckLoadAllowed. Maybe we should also name it CheckContentPolicy and only call contentPolicies, basically the same we do for scripts [1]. 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#529

Comment 36

[Boris Zbarsky [:bzbarsky]]

That seems reasonable, if the only thing we need it for is CSP checks.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_csp_changes.patch

(In reply to Boris Zbarsky [:bz] from comment #36)
> That seems reasonable, if the only thing we need it for is CSP checks.

As agreed, let's introduce a method called CheckContentPolicy which is called in all the places we used to call CheckLoadAllowed(). To be consistent with nsScriptLoader we can call all contentPolicies, but in fact we could also eliminate some overhead and just call csp->ShouldLoad directly. I think we should do what we have in this patch, but wanted to bring it up again to make sure we agree on that. Thanks for all your help!

[Still have to look into test/test_bug732209.html again - will do that tomorrow though]

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_cors_updates.patch

(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8711795 [details] [diff] [review]
> bug_1195173_asyncopen2_layout_style_test_updates.patch
> 
> So the way this test is organized is like this ...

Thanks for the detailed description of the test. In fact, it was my mistake, I rebased the patch locally and commented out those relevant bits because the patch got bitrotted and in the meantime SEC_REQUIRE_CORS_WITH_CREDENTIALS was replaced with SEC_COOKIES_INCLUDE.

The test 'layout/style/test/test_bug732209.html' is correct. Awesome that we have that test. The update in this patch fixes the problem. Once r+ I'll qfold those bits into the main patch withing this bug.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712018 [details] [diff] [review]
bug_1195173_csp_changes.patch

r=me

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712227 [details] [diff] [review]
bug_1195173_cors_updates.patch

r=me

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_layout_style_loader.patch

Rebased all the patches, carrying over r+ from bz and maglione.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_extensionprotocolhandler_open2.patch

Just rebased, carrying over r+.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_csp_changes.patch

Just rebased, carrying over r+.

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_asyncopen2_tests.patch

Just rebased, carrying over r+.

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a75bb8d083e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b51d7bae8e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d026d6f185ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b7c3c6e875

Comment 46

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1001182a8a2

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_cors_updates.patch

Jonas, Boris, theses patches got backed out because browser_styleinspector_csslogic-content-stylesheets.js was timing out and also because fetch-request-resources.https.html was failing.

After looking at the CORS flags we use for the styleloader again while debugging, I ultimately think we should use the same securityFlags (in particular CORS flags) as the scriptLoader does [1].

Which further makes me think that the failing test needs to be updated, because for scripts the test looks like:
> script_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');
so the stylesheet test should look the same, or am I missing something?


[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#293

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8713350 [details] [diff] [review]
bug_1195173_cors_updates.patch

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

Sorry, I don't know the specs here well enough to review.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +129,5 @@
>          script_test(f, REMOTE_URL, 'use-credentials', 'cors', 'include');
>  
>          css_test(f, LOCAL_URL, 'anonymous', 'cors', 'same-origin');
>          css_test(f, LOCAL_URL, 'use-credentials', 'cors', 'include');
> +        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');

How come this test was passing before?

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #48)
> > +        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');
> 
> How come this test was passing before?

Probably asyncOpen2() is fixing a problem related to Bug 1206124 : Fetch does not handle "same-origin" credentials in CORS mode properly

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8713350 [details] [diff] [review]
bug_1195173_cors_updates.patch

>+      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL

This looks wrong to me.  This will mean that a page can't access the CSSOM of a stylesheet loaded from data:, right?  Do we not have tests for that, or does it not happen for some reason as a result of this patch?  Testcase:

  <link rel="stylesheet" href="data:text/css, foo {}"></link>
  <script>onload = () => { alert(document.styleSheets[0].cssRules[0].cssText); }</script>

This alerts the serialization of the rule for me on tip.  Does it do that with the patch?

For the scriptloader, I think our behavior is broken-ish, but looks like it's been that way for a while...  Testcase:

  <script>
    window.onerror = function(a, b, c, d, e, f) {
      console.log(a, b, c, d, e, f);
    }
  </script>
  <script src="data:application/javascript,foo("></script>

The console.log() call has most of the info missing, but that's been the case for a while, and matches other browsers... Not sure what the spec says should happen there.

>+        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');

I can't tell what that last argument means, so I have no idea what this change means in practice..  I would think that in crossorigin="anonymous" mode for a cross-origin URL we would not send cookies, right?  I assume this is actually testing something different (e.g. some sort of state on some Fetch API object) instead?  If so, this change looks reasonable. But it doesn't require us to change any data: behavior.

Comment 51

[Boris Zbarsky [:bzbarsky]]

And fwiw, being told which part of the test failed, and in what way, would have been quite useful for evaluating this patch....

Comment 52

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #51)
> And fwiw, being told which part of the test failed, and in what way, would
> have been quite useful for evaluating this patch....

I agree and I am sorry about that. Tested only locally before setting the review flags, should have run it through TRY. I'll get all those answers before asking for review again.

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8713350 [details] [diff] [review]
bug_1195173_cors_updates.patch

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +129,5 @@
>          script_test(f, REMOTE_URL, 'use-credentials', 'cors', 'include');
>  
>          css_test(f, LOCAL_URL, 'anonymous', 'cors', 'same-origin');
>          css_test(f, LOCAL_URL, 'use-credentials', 'cors', 'include');
> +        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');

Ehsan, you touched that test recently, can you tell us what the correct behavior here, should be? Is it possible that the test is wrong and needs to be updated?

Comment 54

[(no longer active)]

Comment on attachment 8713350 [details] [diff] [review]
bug_1195173_cors_updates.patch

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +129,5 @@
>          script_test(f, REMOTE_URL, 'use-credentials', 'cors', 'include');
>  
>          css_test(f, LOCAL_URL, 'anonymous', 'cors', 'same-origin');
>          css_test(f, LOCAL_URL, 'use-credentials', 'cors', 'include');
> +        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');

I've already answered this in bug 1195172 comment 15.

Comment 55

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195173_cors_updates.patch

(In reply to Boris Zbarsky [:bz] from comment #50)
> Comment on attachment 8713350 [details] [diff] [review]
> bug_1195173_cors_updates.patch
> 
> >+      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> 
> This looks wrong to me.  This will mean that a page can't access the CSSOM
> of a stylesheet loaded from data:, right?  Do we not have tests for that, or
> does it not happen for some reason as a result of this patch?  Testcase:
> 
>   <link rel="stylesheet" href="data:text/css, foo {}"></link>
>   <script>onload = () => {
> alert(document.styleSheets[0].cssRules[0].cssText); }</script>
> 
> This alerts the serialization of the rule for me on tip.  Does it do that
> with the patch?

You are absolutely right, if we use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL I get a security error, but if we use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, then I see the alert from your provided testcase above.

(In reply to :Ehsan Akhgari from comment #54)
> > +        css_test(f, REMOTE_URL, 'anonymous', 'cors', 'same-origin');
> 
> I've already answered this in bug 1195172 comment 15.

So, I assume you are fine with updating the test even though there was a longer discussion following comment 15 within bug 1195172? TRY seems fine with the change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edb9ce3dd76e

Comment 56

[(no longer active)]

Comment on attachment 8714569 [details] [diff] [review]
bug_1195173_cors_updates.patch

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

Yep, the test change looks good!

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8714569 [details] [diff] [review]
bug_1195173_cors_updates.patch

r=me

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d0744f63c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a1dca9e27a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae3c9886156
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c27b21c934

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86d0744f63c6
https://hg.mozilla.org/mozilla-central/rev/a1a1dca9e27a
https://hg.mozilla.org/mozilla-central/rev/4ae3c9886156
https://hg.mozilla.org/mozilla-central/rev/94c27b21c934

Comment 60

[Boris Zbarsky [:bzbarsky]]

By the way, _did_ we have a testcase for CSSOM access to a data: stylesheet per comment 50?  If not, did you add one?

Comment 61

[Jorg K (CEST = GMT+2)]

Looks like this busted Thunderbird e-mail composition.

TB loads stylesheets like this:
https://dxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#279
Loading chrome://editor/content/EditorContent.css
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2424
Loading chrome://messenger/skin/messageQuotes.css
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4933
Loading chrome://messenger/content/composerOverlay.css
They all fail now.

What can we do about that?

Comment 62

[Boris Zbarsky [:bzbarsky]]

That's a question for Christoph, but behavior of addOverrideStyleSheet should NOT have changed.  If it did, that's a bug in this patch that needs to be fixed.  Please file a bug blocking this one, request tracking for 47?

Christoph, this is calling into the Loader::LoadSheetSync API with aUseSystemPrincipal set to true.  This used to call into InternalLoadNonDocumentSheet with aOriginPrincipal set to null.  That would mean no check would happen in CheckLoadAllowed.

The new CheckContentPolicy call, however goes ahead and does the call to NS_CheckContentLoadPolicy.  No idea what that's going to do, for a start.  After that we end up in LoadSheet, and that seems to use the system principal if !aLoadData->mLoaderPrincipal so might be OK; my money is on the content policy check there...

And of course we need to add actual tests for addOverrideStyleSheet, since clearly we have none.

Comment 63

[Jorg K (CEST = GMT+2)]

I raised the bug you wanted (bug 1245681), but I'd prefer to back this bug out until it's fixed properly. BTW, I rolled my local tree back to before the landing, and addOverrideStyleSheet() behaves normally again.

Comment 64

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #60)
> By the way, _did_ we have a testcase for CSSOM access to a data: stylesheet
> per comment 50?  If not, did you add one?

Boris, if we use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL instead of SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS then all of the tests listed underneath are failing (when running mochitests within layout/style/test/). We do have a particular test for the case you brought up in comment 50, which is test_load_events_on_stylesheets.html. So we should be good on that end, unless I am misinterpreting that test. In that case, please let me know.


The following tests failed:
335702 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug221428.html | Should be able to access data: <link> stylesheet 
    @layout/style/test/test_bug221428.html:39:1
335703 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug221428.html | Should be able to access data: @import stylesheet 
    @layout/style/test/test_bug221428.html:58:1
335704 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug405818.html | uncaught exception - SecurityError: The operation is insecure. at http://mochi.test:8888/tests/layout/style/test/test_bug405818.html:45
    simpletestOnerror@SimpleTest/SimpleTest.js:1525:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1512:1
335705 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug412901.html | undefined assertion name - got "0.5px", expected "1px"
    SimpleTest.is@SimpleTest/SimpleTest.js:267:5
    @layout/style/test/test_bug412901.html:28:1
335706 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug412901.html | undefined assertion name - got "5.5px", expected "5px"
    SimpleTest.is@SimpleTest/SimpleTest.js:267:5
    @layout/style/test/test_bug412901.html:31:1
335707 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_load_events_on_stylesheets.html | uncaught exception - SecurityError: The operation is insecure. at http://mochi.test:8888/tests/layout/style/test/test_load_events_on_stylesheets.html:68
    simpletestOnerror@SimpleTest/SimpleTest.js:1525:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1512:1
335708 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_load_events_on_stylesheets.html | Load event for sheet should have fired - got +0, expected 4
    SimpleTest.is@SimpleTest/SimpleTest.js:267:5
    window.onmessage@layout/style/test/test_load_events_on_stylesheets.html:52:3
    EventHandlerNonNull*@layout/style/test/test_load_events_on_stylesheets.html:48:1
335709 INFO TEST-UNEXPECTED-ERROR | layout/style/test/test_load_events_on_stylesheets.html | called finish() multiple times
TEST-INFO 
335710 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_media_queries.html | uncaught exception - SecurityError: The operation is insecure. at http://mochi.test:8888/tests/layout/style/test/test_media_queries.html:166
    simpletestOnerror@SimpleTest/SimpleTest.js:1525:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1512:1
335711 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | uncaught exception - SecurityError: The operation is insecure. at http://mochi.test:8888/tests/layout/style/test/test_selectors.html:223
    simpletestOnerror@SimpleTest/SimpleTest.js:1525:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1512:1
335712 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_cloning.html | uncaught exception - SecurityError: The operation is insecure. at http://mochi.test:8888/tests/layout/style/test/test_value_cloning.html:123
    simpletestOnerror@SimpleTest/SimpleTest.js:1525:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1512:1

Comment 65

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #62)
> That's a question for Christoph, but behavior of addOverrideStyleSheet
> should NOT have changed.  If it did, that's a bug in this patch that needs
> to be fixed.  Please file a bug blocking this one, request tracking for 47?
> 
> Christoph, this is calling into the Loader::LoadSheetSync API with
> aUseSystemPrincipal set to true.  This used to call into
> InternalLoadNonDocumentSheet with aOriginPrincipal set to null.  That would
> mean no check would happen in CheckLoadAllowed.
> 
> The new CheckContentPolicy call, however goes ahead and does the call to
> NS_CheckContentLoadPolicy.  No idea what that's going to do, for a start. 
> After that we end up in LoadSheet, and that seems to use the system
> principal if !aLoadData->mLoaderPrincipal so might be OK; my money is on the
> content policy check there...
> 
> And of course we need to add actual tests for addOverrideStyleSheet, since
> clearly we have none.

Let's fix that problem over in Bug 1245681. We should also add any kind of discussion within Bug 1245681. I'll have a look now and add a testcase.

Comment 66

[Boris Zbarsky [:bzbarsky]]

> So we should be good on that end

Yeah, looks like it.  That test is not explicitly testing this, but it tests enough things with a data: sheet that the result is good enough.