Closed Bug 1189668 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

(firefox42 affected, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: noemi, Assigned: bholley)

References

Details

Attachments

(2 files, 3 obsolete files)

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);
See Also: → 1187951
Depends on: 1187951
Bobby said he'd take a look here.
Flags: needinfo?(bobbyholley)
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: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Status: NEW → ASSIGNED
(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)
This seems to do the trick.
Attachment #8662724 - Flags: review?(bkelly)
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)
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 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+
(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: → 1048048
(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.
(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?
(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?
(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...)
(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).
(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.
(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 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-
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.
I'd think we have no choice but to call that a synthetic response from the SW, and blame the SW's origin.
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.
Attachment #8662724 - Attachment is obsolete: true
(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.
Attachment #8663180 - Attachment is obsolete: true
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)
Attachment #8663181 - Attachment description: Expose GetUnfilteredUrl on InternalResponse. v1 → Part 1 - Expose GetUnfilteredUrl on InternalResponse. v1
Attachment #8663181 - Flags: review?(ehsan)
Attachment #8663208 - Attachment is obsolete: true
Attachment #8663208 - Flags: review?(ehsan)
Looks like nsIInterceptedChannel::GetChannel can also return null sometimes.
Attachment #8663210 - Flags: review?(ehsan)
Attachment #8663181 - Flags: review?(ehsan) → review+
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/fb9cf8892905
https://hg.mozilla.org/mozilla-central/rev/2ce0f31385e1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.