CSP: 'connect-src' should not cause exceptions.

CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879, 694525
R=tyoshino@chromium.org,foolip@chromium.org

Review-Url: https://codereview.chromium.org/2456013002
Cr-Commit-Position: refs/heads/master@{#458384}
Committed: https://chromium.googlesource.com/chromium/src/+/8a4e051abc8f7a36687b5f45a214e9489b6a66b7

[Mike West, 2016-10-27 13:41:06]

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

[commit-bot: I haz the power, 2016-10-27 13:41: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-10-27 13:43:17]

foolip@: Can you take a look at the (completely rewritten) tests?

tyoshino@: Can you look at Beacon, XHR, WebSocket, and EventSource changes?

[foolip, 2016-10-27 13:59:51]

tests lgtm % nits, but I looked less carefully when the structure looked similar
to previous tests.

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html:11:
return;
This together with step_func_done might be wrong, should the test pass if it
returns early here? And what is this get.txt that I don't see elsewhere in this
test?

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html:13:
assert_equals(xhr.readyState, XMLHttpRequest.DONE);
There's no xhr around.

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html:18:
async_test(t => {
No t.done(), how does this test end?

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html:15:
t.done();
With t.done() in two places, are there two valid ways for the test to pass?
Seems like two errors events would do it, but that seems wrong. Maybe
promise_test with Promise.all([foo, bar]) if you want to wait for two things to
happen.

Or, if the order of events is well defined, just listen for one and then add the
second event listener.

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html:10:
ws.onopen = t.unreached_func("Open should not fire.");
Maybe lowercase open

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html:30:
}, "WebSocket should fire onerror.");
Maybe "error event"

[tyoshino (SeeGerritForStatus), 2016-10-27 14:16:35]

lgtm

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp (left):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp:335: // once
webkit.org/b/104520 is solved.
is it fine to remove this? We still have the same comment for the code in
FrameFetchContext.cpp (which is, I think, the one mentioned in the CL
description). Remove that too if it's been fixed or wontfixed?

[Mike West, 2016-10-27 14:18:24]

Thanks, foolip!

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html:11:
return;
On 2016/10/27 at 13:59:50, foolip wrote:
> This together with step_func_done might be wrong, should the test pass if it
returns early here?

Yup. That was dumb.

> And what is this get.txt that I don't see elsewhere in this test

This too.

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-beacon-blocked.html:13:
assert_equals(xhr.readyState, XMLHttpRequest.DONE);
On 2016/10/27 at 13:59:50, foolip wrote:
> There's no xhr around.

Indeed!

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html:15:
t.done();
On 2016/10/27 at 13:59:50, foolip wrote:
> With t.done() in two places, are there two valid ways for the test to pass?
Seems like two errors events would do it, but that seems wrong. Maybe
promise_test with Promise.all([foo, bar]) if you want to wait for two things to
happen.

I'll ping you about this.

> Or, if the order of events is well defined, just listen for one and then add
the second event listener.

It isn't. At least, I don't think it's important that every browser fires one
and then the other.

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html
(right):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html:10:
ws.onopen = t.unreached_func("Open should not fire.");
On 2016/10/27 at 13:59:50, foolip wrote:
> Maybe lowercase open

Sure!

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html:30:
}, "WebSocket should fire onerror.");
On 2016/10/27 at 13:59:50, foolip wrote:
> Maybe "error event"

Sure!

[Mike West, 2016-10-27 14:20:25]

Thanks, tyoshino!

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp (left):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp:335: // once
webkit.org/b/104520 is solved.
On 2016/10/27 at 14:16:35, tyoshino wrote:
> is it fine to remove this? We still have the same comment for the code in
FrameFetchContext.cpp (which is, I think, the one mentioned in the CL
description). Remove that too if it's been fixed or wontfixed?

Since it seems like the extensions team has entirely punted on the idea of
making the isolated world's policy apply to the isolated world's actions until
v3, which doesn't seem likely to happen, I'd call this WONTFIX.

[tyoshino (SeeGerritForStatus), 2016-10-27 14:22:13]

lgtm

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp (left):

https://codereview.chromium.org/2456013002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp:335: // once
webkit.org/b/104520 is solved.
On 2016/10/27 14:20:25, Mike West wrote:
> On 2016/10/27 at 14:16:35, tyoshino wrote:
> > is it fine to remove this? We still have the same comment for the code in
> FrameFetchContext.cpp (which is, I think, the one mentioned in the CL
> description). Remove that too if it's been fixed or wontfixed?
> 
> Since it seems like the extensions team has entirely punted on the idea of
> making the isolated world's policy apply to the isolated world's actions until
> v3, which doesn't seem likely to happen, I'd call this WONTFIX.

OK

[Mike West, 2016-10-27 14:31:47]

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

[commit-bot: I haz the power, 2016-10-27 14:32: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-10-27 15:50:31]

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

[commit-bot: I haz the power, 2016-10-27 15:50:32]

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

[Mike West, 2016-10-27 15:56:59]

Looks like I missed some existing tests. I'll fix them up tomorrow to not rely
on XHR throwing.

Thanks to you both!

[Mike West, 2016-10-28 06:54:22]

Description was changed from

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

BUG=651879
R=tyoshino@chromium.org,foolip@chromium.org
==========

to

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879
R=tyoshino@chromium.org,foolip@chromium.org
==========

[Mike West, 2017-03-17 14:09:10]

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

[commit-bot: I haz the power, 2017-03-17 14:09:24]

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 15:57:14]

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

[commit-bot: I haz the power, 2017-03-17 15:57:15]

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

[Mike West, 2017-03-21 09:32:26]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-03-21 09:32:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2456013002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-21 09:32:42]

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

[Mike West, 2017-03-21 09:33:18]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2017-03-21 09:35:46]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-03-21 09:35:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2456013002/#ps80001
(title: "One more test.")

[commit-bot: I haz the power, 2017-03-21 09:36:02]

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

[Mike West, 2017-03-21 09:45:39]

Description was changed from

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879
R=tyoshino@chromium.org,foolip@chromium.org
==========

to

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879,694525
R=tyoshino@chromium.org,foolip@chromium.org
==========

[commit-bot: I haz the power, 2017-03-21 11:29:30]

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

[commit-bot: I haz the power, 2017-03-21 11:29:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2017-03-21 11:34:40]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2017-03-21 11:34:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2456013002/#ps100001
(title: "Ugh.")

[commit-bot: I haz the power, 2017-03-21 11:34:55]

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-21 12:57:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490096080073310,
"parent_rev": "e7744503e5d45e5404373b447e1499c981174eb9", "commit_rev":
"8a4e051abc8f7a36687b5f45a214e9489b6a66b7"}

[commit-bot: I haz the power, 2017-03-21 12:58:54]

Description was changed from

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879,694525
R=tyoshino@chromium.org,foolip@chromium.org
==========

to

==========
CSP: 'connect-src' should not cause exceptions.

We changed the spec quite some time ago to deal with 'connect-src'
violations in Fetch rather than in each API individually. This means
that we should stop throwing exceptions in 'XHR::open', 'EventSource',
'WebSocket', and 'sendBeacon'.

Closes w3c/webappsec-csp#120.

BUG=651879,694525
R=tyoshino@chromium.org,foolip@chromium.org

Review-Url: https://codereview.chromium.org/2456013002
Cr-Commit-Position: refs/heads/master@{#458384}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a4e051abc8f7a36687b5f45a214...
==========

[commit-bot: I haz the power, 2017-03-21 12:58:56]

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

[jeffcarp, 2017-03-22 18:25:10]

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

[jeffcarp, 2017-03-22 18:25:11]

https://codereview.chromium.org/2456013002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/inside-worker/dedicated-inheritance.html
(right):

https://codereview.chromium.org/2456013002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/inside-worker/dedicated-inheritance.html:17:
/*
Was this left over for debugging? This is what's conflicting and keeping the CL
from being exported.

[jeffcarp, 2017-03-22 21:07:46]

On 2017/03/22 at 18:25:11, jeffcarp wrote:
>
https://codereview.chromium.org/2456013002/diff/100001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/inside-worker/dedicated-inheritance.html
(right):
> 
>
https://codereview.chromium.org/2456013002/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/inside-worker/dedicated-inheritance.html:17:
/*
> Was this left over for debugging? This is what's conflicting and keeping the
CL from being exported.

This CL wasn't being exported since it didn't apply cleanly onto WPT
origin/master. I created a manual export PR here:
https://github.com/w3c/web-platform-tests/pull/5205