CSP: Move 'worker-src' onto 'script-src'

CSP: Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmLBAAJ

BUG=662930, 694525
Review-Url: https://codereview.chromium.org/2533313002
Cr-Commit-Position: refs/heads/master@{#458026}
Committed: https://chromium.googlesource.com/chromium/src/+/d8cc69507968d87cea7eeefc39c0dae78f960879

[Mike West, 2016-11-29 15:01:23]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 15:01:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-11-29 15:44:40]

mkwst@chromium.org changed reviewers:
+ estark@chromium.org, jochen@chromium.org

[Mike West, 2016-11-29 15:44:41]

Jochen, Emily, would you mind reviewing this patch?

I'll send out an update to the intent to ship tomorrow, as this is a substantial
enough change to merit some discussion (though I think the mitigation in the
patch is pretty reasonable) but a sanity check in the meantime would be great.
:)

[commit-bot: I haz the power, 2016-11-29 16:09:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 16:09:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2016-11-29 17:05:14]

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:790: if
(!checkSource(workerSrc, url, redirectStatus) && !m_workerSrc) {
Bah. This should include `&& m_childSrc`. :(

[estark, 2016-11-29 22:01:17]

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worker-src/dedicated-fallback.html
(right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worker-src/dedicated-fallback.html:5:
<meta http-equiv="Content-Security-Policy" content="worker-src
http://127.0.0.1:8000 blob:; script-src 'unsafe-inline'
'http://example.test:8000'">
No quotes around the URL

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (left):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1142:
m_policy->experimentalFeaturesEnabled()) {
Was this an intentional change? Not clear why we don't need the experimental
flag here anymore

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:775: //
'defaut-src'
nit: wrapping is weird

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:783: // fails
(e.g. we'll block 'https://example.com/worker' given
nit: unclosed parenthesis *twitch*

But more importantly, I can't parse this comment and can't seem to figure out
exactly what's going on and why. Is the goal to prevent breakage by allowing
workers that are prohibited by script-src if they're allowed by child-src?

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:789: //
http://crbug.com/662930
nit: https

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:790: if
(!checkSource(workerSrc, url, redirectStatus) && !m_workerSrc) {
On 2016/11/29 17:05:14, Mike West (slow) wrote:
> Bah. This should include `&& m_childSrc`. :(

And shouldn't call operativeDirective on line 791?

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:420: 
Maybe add
script-src 'none'; child-src *
to test that it falls back to child-src when script-src blocks it?

[Mike West, 2016-11-30 09:03:53]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 09:04:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 10:22:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 10:22:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mike West, 2016-11-30 12:34:10]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 12:34:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2016-11-30 12:34:31]

Thanks, Emily!

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worker-src/dedicated-fallback.html
(right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worker-src/dedicated-fallback.html:5:
<meta http-equiv="Content-Security-Policy" content="worker-src
http://127.0.0.1:8000 blob:; script-src 'unsafe-inline'
'http://example.test:8000'">
On 2016/11/29 at 22:01:16, estark wrote:
> No quotes around the URL

And that explains why the test is failing... Good eye!

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (left):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1142:
m_policy->experimentalFeaturesEnabled()) {
On 2016/11/29 at 22:01:16, estark wrote:
> Was this an intentional change? Not clear why we don't need the experimental
flag here anymore

Yes. This is something I want to ship (hence going back to blink-dev with an
update to the intent to ship).

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:775: //
'defaut-src'
On 2016/11/29 at 22:01:16, estark wrote:
> nit: wrapping is weird

I blame clang format. :)

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:783: // fails
(e.g. we'll block 'https://example.com/worker' given
On 2016/11/29 at 22:01:16, estark wrote:
> nit: unclosed parenthesis *twitch*
> 
> But more importantly, I can't parse this comment and can't seem to figure out
exactly what's going on and why. Is the goal to prevent breakage by allowing
workers that are prohibited by script-src if they're allowed by child-src?

I've rewritten it in the hopes of being a little clearer about what I'm
planning.

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:789: //
http://crbug.com/662930
On 2016/11/29 at 22:01:16, estark wrote:
> nit: https

Arg!

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:790: if
(!checkSource(workerSrc, url, redirectStatus) && !m_workerSrc) {
On 2016/11/29 at 22:01:16, estark wrote:
> On 2016/11/29 17:05:14, Mike West (slow) wrote:
> > Bah. This should include `&& m_childSrc`. :(
> 
> And shouldn't call operativeDirective on line 791?

Hrm. Yeah, I think you're right.

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp (right):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp:420: 
On 2016/11/29 at 22:01:16, estark wrote:
> Maybe add
> script-src 'none'; child-src *
> to test that it falls back to child-src when script-src blocks it?

Added a new test for the fallback behavior. Thanks for the suggestion!

[commit-bot: I haz the power, 2016-11-30 13:53:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 13:53:18]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-30 15:30:04]

i'll stamp once Emily is happy and the intent is approved

[estark, 2016-12-01 05:16:56]

lgtm assuming 1.) the I2S gets approved, 2.) the discussion with Brian Smith
resolves in such a way that it still makes sense to do this, and 3.) you're okay
with my question inline about breaking workers that are currently allowed via
default-src.

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (left):

https://codereview.chromium.org/2533313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1142:
m_policy->experimentalFeaturesEnabled()) {
On 2016/11/30 12:34:30, Mike West (slow) wrote:
> On 2016/11/29 at 22:01:16, estark wrote:
> > Was this an intentional change? Not clear why we don't need the experimental
> flag here anymore
> 
> Yes. This is something I want to ship (hence going back to blink-dev with an
> update to the intent to ship).

Oh, sorry, missed that! Makes sense now

https://codereview.chromium.org/2533313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2533313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:779: // In CSP2,
workers are controlled via 'child-src'. CSP3 moves them to
Thanks, this rewording makes things much more clear.

https://codereview.chromium.org/2533313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:781: //
'child-src' that would have been blocked via 'script-src', we'll
There's still the possibility that this will break a site with a worker that was
allowed by default-src but blocked by script-src.

i.e. if a site with a policy of "script-src https://not-example.com; default-src
https://example.com" was loading a worker from https://example.com, that worker
will now be blocked. Are we okay with that?

[Mike West, 2017-03-17 09:13:30]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 09:13:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 09:28:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 09:28:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2017-03-17 09:31:20]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 09:31:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-03-17 10:49:58]

Description was changed from

==========
Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

BUG=662930
==========

to

==========
Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

BUG=662930,694525
==========

[Mike West, 2017-03-17 12:09:43]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 12:09:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-03-17 12:11:45]

I've rebased this onto ToT, and killed off our layout tests in favor of relying
on the tests I added to web-platform-tests a while back. Those are at
https://github.com/w3c/web-platform-tests/tree/master/content-security-policy...
if you'd like to skim through them to verify that we're not losing coverage.

Since it's been ~3 months, I'd be thrilled if y'all could take another look. :)

Of course, I'll wait for folks to chime in on the intent to ship thread before
landing this.

[Mike West, 2017-03-17 12:24:31]

Description was changed from

==========
Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

BUG=662930,694525
==========

to

==========
Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmL...

BUG=662930,694525
==========

[Mike West, 2017-03-17 12:25:12]

Description was changed from

==========
Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmL...

BUG=662930,694525
==========

to

==========
CSP: Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmL...

BUG=662930,694525
==========

[Mike West, 2017-03-17 12:26:54]

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

[Mike West, 2017-03-17 12:26:55]

+Andy

[commit-bot: I haz the power, 2017-03-17 14:10:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 14:10:33]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-03-20 07:06:42]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 07:06:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-03-20 07:11:51]

lgtm

[Mike West, 2017-03-20 08:42:38]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2017-03-20 08:42:40]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-03-20 08:42:41]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2533313002/#ps100001
(title: "WPT.")

[commit-bot: I haz the power, 2017-03-20 08:42:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 08:55:39]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489999360653320,
"parent_rev": "ba21ffd704aebcc8a5e5e1ab1f072e9b79b51fcb", "commit_rev":
"d8cc69507968d87cea7eeefc39c0dae78f960879"}

[commit-bot: I haz the power, 2017-03-20 08:56:23]

Description was changed from

==========
CSP: Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmL...

BUG=662930,694525
==========

to

==========
CSP: Move 'worker-src' onto 'script-src'

Based on the discussion in https://github.com/w3c/webappsec-csp/issues/146,
we're deprecating 'child-src' and moving 'worker-src' onto 'script-src'.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/npKDoKVOUAs/ogtlIFmL...

BUG=662930,694525

Review-Url: https://codereview.chromium.org/2533313002
Cr-Commit-Position: refs/heads/master@{#458026}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8cc69507968d87cea7eeefc39c0...
==========

[commit-bot: I haz the power, 2017-03-20 08:56:24]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d8cc69507968d87cea7eeefc39c0...

[foolip, 2017-06-16 08:00:45]

foolip@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2017-06-16 08:00:46]

https://codereview.chromium.org/2533313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2533313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Deprecation.cpp:435: M60,
"5922594955984896");
We're now at M61, can this be removed and merged back?