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

RESOLVED FIXED in Firefox 43

Status

Testing
web-platform-tests
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: noemi, Assigned: bholley)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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
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);

Updated

3 years ago
See Also: → bug 1187951
Depends on: 1187951
Bobby said he'd take a look here.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 3

3 years ago
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?
Flags: needinfo?(bkelly)
(Assignee)

Updated

3 years ago
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 4

3 years ago
(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.
Flags: needinfo?(bkelly)
(Assignee)

Comment 5

3 years ago
Created attachment 8662724 [details] [diff] [review]
Check CSP before completing channel interception. v1

This seems to do the trick.
Attachment #8662724 - Flags: review?(bkelly)
(Assignee)

Comment 7

3 years ago
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.
Attachment #8662724 - Flags: review?(mozilla)
(Assignee)

Comment 8

3 years ago
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 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.
Attachment #8662724 - Flags: review?(mozilla) → feedback+

Comment 10

3 years ago
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
Attachment #8662724 - Flags: review?(bkelly) → review+

Comment 11

3 years ago
(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().
See Also: → bug 1048048

Comment 12

3 years ago
(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

3 years ago
(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

3 years ago
(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

3 years ago
(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...)
(Assignee)

Comment 16

3 years ago
(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

3 years ago
(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

3 years ago
(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

3 years ago
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.
Attachment #8662724 - Flags: review-

Comment 20

3 years ago
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.
(Assignee)

Comment 21

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

Comment 22

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8662724 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8663180 [details] [diff] [review]
Check CSP before completing channel interception. v2
(Assignee)

Comment 24

3 years ago
Created attachment 8663181 [details] [diff] [review]
Part 1 - Expose GetUnfilteredUrl on InternalResponse. v1
(Assignee)

Comment 25

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8663180 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8663208 [details] [diff] [review]
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.
Attachment #8663208 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8663181 - Attachment description: Expose GetUnfilteredUrl on InternalResponse. v1 → Part 1 - Expose GetUnfilteredUrl on InternalResponse. v1
Attachment #8663181 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8663208 - Attachment is obsolete: true
Attachment #8663208 - Flags: review?(ehsan)
(Assignee)

Comment 28

3 years ago
Created attachment 8663210 [details] [diff] [review]
Part 2 - Check CSP before completing channel interception. v4

Looks like nsIInterceptedChannel::GetChannel can also return null sometimes.
Attachment #8663210 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8663181 - Flags: review?(ehsan) → review+

Comment 30

2 years ago
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.
Attachment #8663210 - Flags: review?(ehsan) → review+
(Assignee)

Comment 31

2 years ago
(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 33

2 years ago
https://hg.mozilla.org/mozilla-central/rev/fb9cf8892905
https://hg.mozilla.org/mozilla-central/rev/2ce0f31385e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Reporter)

Comment 34

2 years ago
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
You need to log in before you can comment on or make changes to this bug.