1189668 - "Harness status: Timeout" when running "fetch-csp.https.html" test

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[Noemí Freire (:noemi)]

Test run such as |./mach web-platform-tests _mozilla/service-workers/service-worker/fetch-csp.https.html|


Harness status: Timeout
Found 1 tests
1 Timeout
* Verify CSP control of fetch() in a Service Worker -> timed out
Traces: https://pastebin.mozilla.org/8841195

Comment 1

[Andrew Overholt [:overholt]]

Both of the tests failing here have a document containing a meta tag:

meta.setAttribute('content', 'img-src ' + host_info['HTTPS_ORIGIN'] +
                  '; script-src \'unsafe-inline\'');

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-csp-iframe.html?offset=100#28 adds an img element to a document with: 

img.src = host_info['HTTPS_REMOTE_ORIGIN'] + image_path;

which presumably shouldn't load here :)

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-csp-iframe.html?offset=300#55 fails if an appended img element loads with:

img.src = './dummy?mode=no-cors&url=' + encodeURIComponent(host_info['HTTPS_REMOTE_ORIGIN'] + image_path);

Comment 2

[Andrew Overholt [:overholt]]

Bobby said he'd take a look here.

Comment 3

[Bobby Holley (:bholley)]

From my naive understanding of the situation here, it would appear that the ServiceWorker implementation need to do a CSP check during RespondWith in order to determine that the origin of the Response passed is acceptable to return for the given Request. Does that seem right?

If that's the case, it seems like we need a way to get the content type to pass to NS_CheckContentLoadPolicy. Any idea if we can grab that off the nsIInterceptedChannel somehow?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) from comment #3)
> From my naive understanding of the situation here, it would appear that the
> ServiceWorker implementation need to do a CSP check during RespondWith in
> order to determine that the origin of the Response passed is acceptable to
> return for the given Request. Does that seem right?

This sounds reasonable.  I think it would roughly correspond to step 12 of Main Fetch:

  https://fetch.spec.whatwg.org/#main-fetch

It may not be obvious at first glance, but that takes place after the SW calls respondWith().

> If that's the case, it seems like we need a way to get the content type to
> pass to NS_CheckContentLoadPolicy. Any idea if we can grab that off the
> nsIInterceptedChannel somehow?

We have nsContentPolicyType on the InternalRequest here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.h?case=true&from=InternalRequest&offset=100#404

And we have access to the InternalRequest during RespondWith() here:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#358

So it seems we could do that if the other args to NS_CheckContentLoadPolicy() works.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Check CSP before completing channel interception. v1

This seems to do the trick.

Comment 6

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59cda3aeed32

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8662724 [details] [diff] [review]
Check CSP before completing channel interception. v1

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

Flagging Christoph for the CSP usage. Ben, feel free to bounce if someone else is a more appropriate reviewer for the SW bits.

Comment 8

[Bobby Holley (:bholley)]

Hm, looks like url can be the empty string sometimes, presumably when the response is programmatically generated? Seems to cause a handful of test failures in comment 6 - I'll have a look tomorrow morning.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8662724 [details] [diff] [review]
Check CSP before completing channel interception. v1

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

Looks good from the content policy side of things. Two questions though:
(1) do we really need to call all content policies, or just CSP? If we only need CSP, then probably we should just do:
    int16_t shouldLoad = nsIContentPolicy::ACCEPT;
    nsCOMPtr<nsIContentPolicy> cp = do_GetService(NS_CONTENTPOLICY_CONTRACTID);
    cp->ShouldLoad(...)

(2) Does that code need to be e10s proof? Or, let me phrase that differently: in e10s we can not serialize nsINode between child and parent and hence loadinfo->LoadingNode() might return null. In easier words, if ShoudLoad() is called in the parent process, then loadinfo->LoadingNode() returns null and might lead to unexpected behavior when calling shouldLoad. E.g. we have cases where mixed content blocker returns false, because it could not query the corresponding docshell.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +160,5 @@
> +    rv = mChannel->GetChannel(getter_AddRefs(underlyingChannel));
> +    NS_ENSURE_SUCCESS(rv, false);
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    rv = underlyingChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    NS_ENSURE_SUCCESS(rv, false);

you can simplify to:
nsCOMPtr<nsILoadInfo> loadInfo = underlyingChannel->GetLoadInfo();
NS_ENSURE_TRUE(loadInfo, false);

@@ +162,5 @@
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    rv = underlyingChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    int16_t decision;

please initalize to:
int16_t decision = nsIContentPolicy::ACCEPT;

@@ +163,5 @@
> +    rv = underlyingChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    int16_t decision;
> +    rv = NS_CheckContentLoadPolicy(loadInfo->GetContentPolicyType(), uri, loadInfo->LoadingPrincipal(),

Most likely it doesn't really matter for your use cases, but you are better off sending the internal content policy type to make it more future proof.
loadInfo->InternalContentPolicyType();

Before content policies are called we map the internal type to the external type but sometimes we also need to pass through the internal type, see:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#138

And jfyi, there are more patches in review right now that allow passing internal types to mixed content as well as CSP. Long story short: Just use the internal type here.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8662724 [details] [diff] [review]
Check CSP before completing channel interception. v1

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

r=me after figuring out a plan for nsDocShell::DisplayLoadError() for the returned error code. Thanks!

In regards to Christian's e10s question, the code in ServiceWorkerEvents.cpp always runs in the child process today.  I believe this will stay in the child in the future as well, but it might be useful to at least add a comment here.  In theory we could try to assert on XRE_IsContentProcess(), but I don't know a good way to make that conditional on e10s being enabled.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +114,5 @@
>    {
>      AssertIsOnMainThread();
>  
> +    if (!CSPPermitsResponse()) {
> +      mChannel->Cancel(NS_ERROR_CONTENT_BLOCKED);

I think we should have something in nsDocShell::DisplayLoadError() for whatever we return here.

If NS_ERROR_CONTENT_BLOCKED can get returned for navigations in non-service-worker cases, then adding a case for that error type makes sense.  That can be a follow-up bug.

Otherwise, should we instead return an error type here that will get a docshell neterror page for navigations?

The UX for failed navigations with an error code not in DisplayLoadError() is pretty bad.  We have worked to fix this for service worker interceptions otherwise here:

  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?case=true&from=nsDocShell.cpp&offset=0#4975

Comment 11

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Looks good from the content policy side of things. Two questions though:
> (1) do we really need to call all content policies, or just CSP? If we only
> need CSP, then probably we should just do:
>     int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>     nsCOMPtr<nsIContentPolicy> cp =
> do_GetService(NS_CONTENTPOLICY_CONTRACTID);
>     cp->ShouldLoad(...)

I think it's *wrong* to call all content policy implementations here, we only need to check CSP.

Also, if this lands before bug 1048048, I think we want to do something similar to https://bug1048048.bmoattachments.org/attachment.cgi?id=8644645 in this call site as well (this is a note to Christoph!)

> @@ +163,5 @@
> > +    rv = underlyingChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +
> > +    int16_t decision;
> > +    rv = NS_CheckContentLoadPolicy(loadInfo->GetContentPolicyType(), uri, loadInfo->LoadingPrincipal(),
> 
> Most likely it doesn't really matter for your use cases, but you are better
> off sending the internal content policy type to make it more future proof.
> loadInfo->InternalContentPolicyType();

Given the above, I think you need to keep passing the external type, but Christoph is right, you need to pass the internal type to NS_CheckContentLoadPolicy().

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) from comment #8)
> Hm, looks like url can be the empty string sometimes, presumably when the
> response is programmatically generated? Seems to cause a handful of test
> failures in comment 6 - I'll have a look tomorrow morning.

Yes.  Synthetic responses will have an empty string URL.  I'm not sure what this means for the security policy, though.  In other places we inherit the security settings from the Service Worker, though.  For example, the Response will have the security_info blob from the service worker.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> I think it's *wrong* to call all content policy implementations here, we
> only need to check CSP.

Are we sure we want to let sites use Service Workers to bypass ad blocking?

Comment 14

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #13)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> > I think it's *wrong* to call all content policy implementations here, we
> > only need to check CSP.
> 
> Are we sure we want to let sites use Service Workers to bypass ad blocking?

The initial load must have been subject to content policy checks, no?

Comment 15

[(no longer active)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #14)
> (In reply to Ben Kelly [:bkelly] from comment #13)
> > (In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> > > I think it's *wrong* to call all content policy implementations here, we
> > > only need to check CSP.
> > 
> > Are we sure we want to let sites use Service Workers to bypass ad blocking?
> 
> The initial load must have been subject to content policy checks, no?

(If they aren't, then you're right.  I am _hoping_ they are though...)

Comment 16

[Bobby Holley (:bholley)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> > The initial load must have been subject to content policy checks, no?
> 
> (If they aren't, then you're right.  I am _hoping_ they are though...)

Well, the content policy needs to know what the resource is being used for, right? And we don't know that until the final step of the SW interception (which is why we need to do CSP there too).

Comment 17

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #16)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> > > The initial load must have been subject to content policy checks, no?
> > 
> > (If they aren't, then you're right.  I am _hoping_ they are though...)
> 
> Well, the content policy needs to know what the resource is being used for,
> right? And we don't know that until the final step of the SW interception
> (which is why we need to do CSP there too).

Good point...  Now I am not sure of what I said before.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #14)
> > (In reply to Ben Kelly [:bkelly] from comment #13)
> > > (In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> > > > I think it's *wrong* to call all content policy implementations here, we
> > > > only need to check CSP.
> > > 
> > > Are we sure we want to let sites use Service Workers to bypass ad blocking?
> > 
> > The initial load must have been subject to content policy checks, no?
> 
> (If they aren't, then you're right.  I am _hoping_ they are though...)

You mean the fetch() in the service worker?  It doesn't know what DOM node its actually being loaded for, so I don't see how the check there could be equivalent to this.

Here is the check in FetchDriver:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?case=true&from=FetchDriver.cpp#115

Note, mDocument may be nullptr and FetchDriver always passes empty string for mime type.

Comment 19

[(no longer active)]

Comment on attachment 8662724 [details] [diff] [review]
Check CSP before completing channel interception. v1

OK, after looking at things a bit more, bkelly is right.  We do want to run all content policy checks here.  But you should still be passing in the internal content policy type, not the external one.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

What should we do in this case?

  var realResponse;
  e.respondWith(fetch(corsAdServer).then(function(response) {
    realResponse = response;
    return realResponse.text();
  }).then(function(text) {
    return new Response(text, realResponse);
  }));

This sends the remote server's body back to the requesting node, but we don't see the remote server's URL.

Comment 21

[Bobby Holley (:bholley)]

I'd think we have no choice but to call that a synthetic response from the SW, and blame the SW's origin.

Comment 22

[Bobby Holley (:bholley)]

So the other wrinkle here is that apparently we don't actually support <meta> CSP yet, which is what this test is using. I wonder if I can hack it up to serve with the appropriate headers for now.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Check CSP before completing channel interception. v2

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 1 - Expose GetUnfilteredUrl on InternalResponse. v1

Comment 25

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [PTO, back Sep 28, :bkelly] from comment #10)
> Comment on attachment 8662724 [details] [diff] [review]
> Check CSP before completing channel interception. v1
> 
> Review of attachment 8662724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me after figuring out a plan for nsDocShell::DisplayLoadError() for the
> returned error code. Thanks!
> 
> In regards to Christian's e10s question, the code in ServiceWorkerEvents.cpp
> always runs in the child process today.  I believe this will stay in the
> child in the future as well, but it might be useful to at least add a
> comment here.  In theory we could try to assert on XRE_IsContentProcess(),
> but I don't know a good way to make that conditional on e10s being enabled.
> 
> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +114,5 @@
> >    {
> >      AssertIsOnMainThread();
> >  
> > +    if (!CSPPermitsResponse()) {
> > +      mChannel->Cancel(NS_ERROR_CONTENT_BLOCKED);
> 
> I think we should have something in nsDocShell::DisplayLoadError() for
> whatever we return here.
> 
> If NS_ERROR_CONTENT_BLOCKED can get returned for navigations in
> non-service-worker cases, then adding a case for that error type makes
> sense.

Yes, see https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/docshell/base/nsDocShell.cpp#9521

> That can be a follow-up bug.

Filed bug 1206295.

Comment 26

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c91fa43a7b4

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 2 - Check CSP before completing channel interception. v3

Hm, looks like ServiceWorker::GetWorkerPrivate can return null. Hopefully this
works - I can't quite tell whether this URL has already been resolved through
redirects, but I'm pretty sure that's the case.

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 2 - Check CSP before completing channel interception. v4

Looks like nsIInterceptedChannel::GetChannel can also return null sometimes.

Comment 29

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=169d3da41080

Comment 30

[(no longer active)]

Comment on attachment 8663210 [details] [diff] [review]
Part 2 - Check CSP before completing channel interception. v4

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

Nice, thanks!

::: dom/workers/ServiceWorkerEvents.cpp
@@ +171,5 @@
> +    NS_ENSURE_SUCCESS(rv, false);
> +    NS_ENSURE_TRUE(underlyingChannel, false);
> +    nsCOMPtr<nsILoadInfo> loadInfo = underlyingChannel->GetLoadInfo();
> +
> +    int16_t decision = nsIContentPolicy::ACCEPT;

Please initialize this to reject by default.  If we have screw something up, it's better to be safe than sorry!  ;-)

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-csp-iframe.html.sub.headers
@@ +1,1 @@
> +Content-Security-Policy: img-src https://{{host}}:{{ports[https][0]}}

Looks like you forgot this part:

script-src 'unsafe-inline'

Please add this to maintain parity with the meta tag.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #30)
> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +171,5 @@
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +    NS_ENSURE_TRUE(underlyingChannel, false);
> > +    nsCOMPtr<nsILoadInfo> loadInfo = underlyingChannel->GetLoadInfo();
> > +
> > +    int16_t decision = nsIContentPolicy::ACCEPT;
> 
> Please initialize this to reject by default.  If we have screw something up,
> it's better to be safe than sorry!  ;-)

This was an explicit instruction in comment 9. I'm assuming Christoph had a reason for asking that.

> 
> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-csp-iframe.html.sub.headers
> @@ +1,1 @@
> > +Content-Security-Policy: img-src https://{{host}}:{{ports[https][0]}}
> 
> Looks like you forgot this part:
> 
> script-src 'unsafe-inline'

It was intentional - otherwise, the <script> tags fail to load. This doesn't affect the <meta> case, because it's added after the <script> tags.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9cf8892905
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce0f31385e1

Comment 33

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/fb9cf8892905
https://hg.mozilla.org/mozilla-central/rev/2ce0f31385e1

Comment 34

[Noemí Freire (:noemi)]

Hi,

just checked on m-c (fcef8ded8221 revision) and the test successfully runs. Thanks for fixing it!.

Summary

Harness status: OK

Found 1 tests
1 Pass
Details
Result	Test Name
Pass	Verify CSP control of fetch() in a Service Worker