Enable checking for Content Security Policies before loading an external worklet script.

Enable checking for Content Security Policies before loading an external worklet script.

BUG=567358

[Gleb Lanbin, 2016-07-12 20:16:38]

Patchset #2 (id:20001) has been deleted

[Gleb Lanbin, 2016-07-12 20:16:47]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-07-12 20:17:16]

glebl@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[Gleb Lanbin, 2016-07-18 16:26:34]

Ian, just a friendly reminder that I'm waiting for your review.

[ikilpatrick, 2016-07-20 17:56:16]

ikilpatrick@chromium.org changed reviewers:
+ mkwst@chromium.org, nhiroki@chromium.org, yhirano@chromium.org

[ikilpatrick, 2016-07-20 17:56:17]

lgtm but wait for mkwst.

+mkwst

+nhiroki & +yhirano for sanity check as well.

mkwst/nhiroki/yhirano (this is un-related to this patch) I was just quickly
reading through the network loading code, should WorkerScriptLoader use
something similar to core/fetch/ScriptResource.h internally?
Or replace WorkerScriptLoader with ScriptResource?

https://codereview.chromium.org/2146633002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/2146633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:30: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NetworkError, "The script at '" + scriptURL.elidedString()
+ "' failed to load."));
A better message is probably something like "Access denied due to document CSP",
otherwise this looks good.

[yhirano, 2016-07-21 05:09:48]

> mkwst/nhiroki/yhirano (this is un-related to this patch) I was just quickly
> reading through the network loading code, should WorkerScriptLoader use
> something similar to core/fetch/ScriptResource.h internally?
> Or replace WorkerScriptLoader with ScriptResource?
> 

blink::Resource and subclasses can work only on the main thread so I don't think
it's usable with WorkerScriptLoader.
Sharing the logic is generally a good thing, but I'm not sure which part you are
talking about. Can you tell me?

[yhirano, 2016-07-21 05:16:56]

https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked.html
(right):

https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked.html:15:
}).catch(function(error) {
This catches the assert_unreached exception. I would suggest

return paintWorklet.import(...).then(
  () => assert_unreached('import should fail'),
  error => assert_equals(error.name, 'NetworkError', ...));

[Mike West, 2016-07-21 07:26:48]

https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):

https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/worklet/Worklet.cpp:30: return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NetworkError, "Access to '" + scriptURL.elidedString() + "'
is denied by document's Content Security Policies."));
Hrm. This seems inconsistent with the way we handle most other requests. It
doesn't handle redirect responses, for instance.

We currently do most of the CSP checks in `FrameFetchContext::canRequest`
(called from `ResourceFetcher`). Does this loader not route through that
mechanism? It appears that we're checking for worker requests in
`ContentSecurityPolicy::allowRequest`, for instance.

[Gleb Lanbin, 2016-07-26 00:51:28]

On 2016/07/21 07:26:48, Mike West (OOO until 25th) wrote:
>
https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):
> 
>
https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/worklet/Worklet.cpp:30: return
> ScriptPromise::rejectWithDOMException(scriptState,
> DOMException::create(NetworkError, "Access to '" + scriptURL.elidedString() +
"'
> is denied by document's Content Security Policies."));
> Hrm. This seems inconsistent with the way we handle most other requests. It
> doesn't handle redirect responses, for instance.
> 
> We currently do most of the CSP checks in `FrameFetchContext::canRequest`
> (called from `ResourceFetcher`). Does this loader not route through that
> mechanism? It appears that we're checking for worker requests in
> `ContentSecurityPolicy::allowRequest`, for instance.

I tried to change Worklet code to use ScriptResource instead of the current
approach(WorkerScriptLoader).
It worked fine.
Here is the patch http://crrev.com/2178223002. Could you please take a look?

If it looks acceptable to you then I will just abandon this patch in favor the
one with ScriptResource.

[Gleb Lanbin, 2016-07-27 23:23:41]

On 2016/07/26 00:51:28, glebl wrote:
> On 2016/07/21 07:26:48, Mike West (OOO until 25th) wrote:
> >
>
https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/worklet/Worklet.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2146633002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/worklet/Worklet.cpp:30: return
> > ScriptPromise::rejectWithDOMException(scriptState,
> > DOMException::create(NetworkError, "Access to '" + scriptURL.elidedString()
+
> "'
> > is denied by document's Content Security Policies."));
> > Hrm. This seems inconsistent with the way we handle most other requests. It
> > doesn't handle redirect responses, for instance.
> > 
> > We currently do most of the CSP checks in `FrameFetchContext::canRequest`
> > (called from `ResourceFetcher`). Does this loader not route through that
> > mechanism? It appears that we're checking for worker requests in
> > `ContentSecurityPolicy::allowRequest`, for instance.
> 
> I tried to change Worklet code to use ScriptResource instead of the current
> approach(WorkerScriptLoader).
> It worked fine.
> Here is the patch http://crrev.com/2178223002. Could you please take a look?
> 
> If it looks acceptable to you then I will just abandon this patch in favor the
> one with ScriptResource.

This patch is abandoned in favor of http://crrev.com/2178223002 which is based
on Mike's recommendation to use ScriptResource.

[Gleb Lanbin, 2016-07-27 23:23:51]

Description was changed from

==========
Enable checking for Content Security Policies before loading an external worklet
script.

BUG=567358
==========

to

==========
Enable checking for Content Security Policies before loading an external worklet
script.

BUG=567358
==========

[Gleb Lanbin, 2016-07-27 23:23:51]

glebl@chromium.org changed reviewers:
- ikilpatrick@chromium.org, mkwst@chromium.org, nhiroki@chromium.org,
yhirano@chromium.org