[ServiceWorker] CSP support for ServiceWorker environment.

[ServiceWorker] CSP support for ServiceWorker environment.

According to the CSP spec, we have to check the Content-Security-Policy HTTP response header of the ServiceWorker script.
https://w3c.github.io/webappsec/specs/content-security-policy/#processing-model-workers

For example:
When "Content-Security-Policy: script-src 'self'" is set, "importScripts('https://othersite/'); must fail.
When "Content-Security-Policy: connect-src 'self'" is set, "fetch('https://othersite/data'); must fail.

The changes in WebEmbeddedWorkerImpl.cpp introduce CSP check for ServiceWorker environment.
The changes in FetchManager.cpp introduce CSP check for fech API.

We need to set the CSP not only to the ServiceWorkerGlobalScope but also to the shadow page's document.
The CSP in the shadow page's document will be used while handling the redirect responses in DocumentThreadableLoader::isAllowedByContentSecurityPolicy()

BUG=432069
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185630

[horo, 2014-11-11 10:40:05]

Patchset #1 (id:1) has been deleted

[horo, 2014-11-11 10:40:12]

Patchset #1 (id:20001) has been deleted

[horo, 2014-11-11 11:06:30]

Patchset #1 (id:40001) has been deleted

[horo, 2014-11-11 11:11:43]

Patchset #1 (id:60001) has been deleted

[horo, 2014-11-11 11:13:08]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2014-11-11 11:13:09]

nhiroki@
Could you please review?

[nhiroki, 2014-11-12 07:43:14]

Looks good to me. I'm not familiar with CSP, so please ask an expert on CSP to
review this.

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php
(right):

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php:36:
} if ($directive == 'script') {
Can you add a line break before 'if' OR replace 'if' with 'else if'?

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php:67:
} if ($directive == 'connect') {
ditto.

https://codereview.chromium.org/714833002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:167: // "A network error."
This comment seems meaningless.

[horo, 2014-11-12 08:25:41]

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php
(right):

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php:36:
} if ($directive == 'script') {
On 2014/11/12 07:43:14, nhiroki wrote:
> Can you add a line break before 'if' OR replace 'if' with 'else if'?

Done.

https://codereview.chromium.org/714833002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/service-worker-csp-worker.php:67:
} if ($directive == 'connect') {
On 2014/11/12 07:43:14, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/714833002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:167: // "A network error."
On 2014/11/12 07:43:14, nhiroki wrote:
> This comment seems meaningless.

I want to keep this.
The comments are the copy from the spec.
I think they are useful to read and understand the codes.

[horo, 2014-11-12 08:26:23]

horo@chromium.org changed reviewers:
+ mkwst@chromium.org

[horo, 2014-11-12 08:26:24]

mkwst@
Could you please review this?

[Mike West, 2014-11-12 12:28:36]

Thank you for taking a look at this! One comment right off the top, and I'll
look in more detail soon.

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:431:
document->contentSecurityPolicy()->deprecatedHeader(),
I don't think this is correct; it differs from the flow described in
https://w3c.github.io/webappsec/specs/content-security-policy/#processing-mod....
In short, we should only inherit a policy from the creating document if we're
creating the Worker from a `blob:` or `filesystem:` (or `data:`, I guess) URL.

If we're loading an HTTPS URL, we should set the Worker's policy to whatever is
delivered via the HTTP header.

[horo, 2014-11-13 01:09:02]

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:431:
document->contentSecurityPolicy()->deprecatedHeader(),
On 2014/11/12 12:28:36, Mike West wrote:
> I don't think this is correct; it differs from the flow described in
>
https://w3c.github.io/webappsec/specs/content-security-policy/#processing-mod....
> In short, we should only inherit a policy from the creating document if we're
> creating the Worker from a `blob:` or `filesystem:` (or `data:`, I guess) URL.
> 
> If we're loading an HTTPS URL, we should set the Worker's policy to whatever
is
> delivered via the HTTP header.

This document is not the document who created (registered) the ServiceWorker.
It is the document of the shadow page which is created in
WebEmbeddedWorkerImpl::prepareShadowPageForLoader() for handling the resource
loading.
We create ContentSecurityPolicy from the HTTP headers of the script file in
WebEmbeddedWorkerImpl::Loader::didReceiveResponse().
And set the CSP to the document (at lint 422) and the ServiceWorkerGlobalScope.

We need to set CPS to the document because it will be used while handling the
redirect responses in
DocumentThreadableLoader::isAllowedByContentSecurityPolicy().

[Mike West, 2014-11-19 10:31:49]

Oh, man. I completely dropped this CL. I apologize for the delay, that wasn't at
all intentional. :(

If I ever don't respond within a day, please do ping me.

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:166: if
(!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
{
Why do we do the CSP check here, rather than in ResourceFetcher? It would be
nice to centralize the security checks we're applying to resource requests.

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:431:
document->contentSecurityPolicy()->deprecatedHeader(),
On 2014/11/13 01:09:02, horo wrote:
> On 2014/11/12 12:28:36, Mike West wrote:
> > I don't think this is correct; it differs from the flow described in
> >
>
https://w3c.github.io/webappsec/specs/content-security-policy/#processing-mod....
> > In short, we should only inherit a policy from the creating document if
we're
> > creating the Worker from a `blob:` or `filesystem:` (or `data:`, I guess)
URL.
> > 
> > If we're loading an HTTPS URL, we should set the Worker's policy to whatever
> is
> > delivered via the HTTP header.
> 
> This document is not the document who created (registered) the ServiceWorker.
> It is the document of the shadow page which is created in
> WebEmbeddedWorkerImpl::prepareShadowPageForLoader() for handling the resource
> loading.
> We create ContentSecurityPolicy from the HTTP headers of the script file in
> WebEmbeddedWorkerImpl::Loader::didReceiveResponse().
> And set the CSP to the document (at lint 422) and the
ServiceWorkerGlobalScope.
> 
> We need to set CPS to the document because it will be used while handling the
> redirect responses in
> DocumentThreadableLoader::isAllowedByContentSecurityPolicy().

Hrm. Ok, then I misunderstood the implementation here. You're basically creating
a dummy document in order to handle resource requests? That seems weird, but
it's certainly not something to change in this CL.

[horo, 2014-11-19 12:35:41]

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:166: if
(!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
{
The old comment was wrong.
We don't check CSP for fetch API currently.

The CSP check for "connect-src" is not handled in ResourceFetcher but in
XMLHttpRequest::open() and NavigatorBeacon::canSendBeacon() and
DOMWebSocket::connect().
So for fetch API it is better to check this in FetchManager::Loader.

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:431:
document->contentSecurityPolicy()->deprecatedHeader(),
> You're basically creating a dummy document in order to handle resource
requests?

Yes.
We create the dummy document which lives in the main thread and handles the
resource requests.

In the worker thread we use WorkerThreadableLoader to load the resources.
It communicates with the dummy document using
WorkerThreadableLoader::MainThreadBridge and WorkerLoaderProxy to go across the
threads.

[Mike West, 2014-11-19 12:40:45]

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:166: if
(!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
{
On 2014/11/19 12:35:41, horo wrote:
> The old comment was wrong.
> We don't check CSP for fetch API currently.
> 
> The CSP check for "connect-src" is not handled in ResourceFetcher but in
> XMLHttpRequest::open() and NavigatorBeacon::canSendBeacon() and
> DOMWebSocket::connect().
> So for fetch API it is better to check this in FetchManager::Loader.

We check in both XMLHTTPRequest and ResourceFetcher, don't we? ResourceFetcher
catches redirects, which aren't visible from `open()`.

[horo, 2014-11-19 12:57:35]

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:166: if
(!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
{
> We check in both XMLHTTPRequest and ResourceFetcher, don't we? ResourceFetcher
> catches redirects, which aren't visible from `open()`.

Are you saying "both XMLHTTPRequest and DocumentThreadableLoader"?
Yes.
DocumentThreadableLoader::isAllowedByContentSecurityPolicy() is called to check
the redirected URL.

[Mike West, 2014-11-19 13:40:36]

On 2014/11/19 12:57:35, horo wrote:
>
https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
> File Source/modules/serviceworkers/FetchManager.cpp (right):
> 
>
https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
> Source/modules/serviceworkers/FetchManager.cpp:166: if
> (!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
>
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
> {
> > We check in both XMLHTTPRequest and ResourceFetcher, don't we?
ResourceFetcher
> > catches redirects, which aren't visible from `open()`.
> 
> Are you saying "both XMLHTTPRequest and DocumentThreadableLoader"?
> Yes.
> DocumentThreadableLoader::isAllowedByContentSecurityPolicy() is called to
check
> the redirected URL.

I was thinking of ResourceFetcher::canRequest, but you're right, we're not
currently doing CSP checks there for XHR. Sorry for the confusion.

Regardless, would you mind adding a layout test to verify the redirect behavior?

[Mike West, 2014-11-19 13:41:00]

Assuming the layout test is added and passes, LGTM. Apologies again for the
delayed review.

[horo, 2014-11-19 14:33:37]

On 2014/11/19 13:40:36, Mike West wrote:
> On 2014/11/19 12:57:35, horo wrote:
> >
>
https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
> > File Source/modules/serviceworkers/FetchManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/714833002/diff/100001/Source/modules/servicew...
> > Source/modules/serviceworkers/FetchManager.cpp:166: if
> > (!ContentSecurityPolicy::shouldBypassMainWorld(m_executionContext) &&
> >
>
!m_executionContext->contentSecurityPolicy()->allowConnectToSource(m_request->url()))
> > {
> > > We check in both XMLHTTPRequest and ResourceFetcher, don't we?
> ResourceFetcher
> > > catches redirects, which aren't visible from `open()`.
> > 
> > Are you saying "both XMLHTTPRequest and DocumentThreadableLoader"?
> > Yes.
> > DocumentThreadableLoader::isAllowedByContentSecurityPolicy() is called to
> check
> > the redirected URL.
> 
> I was thinking of ResourceFetcher::canRequest, but you're right, we're not
> currently doing CSP checks there for XHR. Sorry for the confusion.
> 
> Regardless, would you mind adding a layout test to verify the redirect
behavior?

Currently we don't support redirect in fetch API.
So I will add LayoutTests when we support redirect. crbug.com/402389

[horo, 2014-11-19 21:57:32]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-11-19 21:58:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714833002/140001

[commit-bot: I haz the power, 2014-11-19 23:15:26]

Committed patchset #4 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185630