Block 'javascript:' navigation in the correct document.

Block 'javascript:' navigation in the correct document.

While working through [1], I noticed that Blink blocks 'javascript:' URL
navigation in the _target_ document as opposed to in the _navigating_
document. This is particularly incorrect for `<iframe>`.

This patch corrects the targeting. As a drive-by, this patch also ensures
that we fire a `SecurityPolicyViolationEvent` for all CSP events. Previously,
we fired only _unique_ CSP events. This optimization is meant to reduce
traffic to a server (as completely duplicate reports are ~useless), but
inadvertently blocked DOM events as well. The change to
`ContentSecurityPolicy::reportViolation` fixes that.

[1]: https://github.com/w3c/webappsec-csp/issues/127

R=jochen@chromium.org

Committed: https://crrev.com/cab8b9f289ebe22d5e5ad79f6150cf729ef5079c
Cr-Commit-Position: refs/heads/master@{#431539}

[Mike West, 2016-11-09 15:24:10]

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

[commit-bot: I haz the power, 2016-11-09 15:24:23]

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-09 17:39:10]

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

[commit-bot: I haz the power, 2016-11-09 17:39:11]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-10 07:29:57]

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

[Mike West, 2016-11-10 07:29:58]

foolip@: WDYT?

[foolip, 2016-11-10 09:25:01]

I didn't totally understand what's going on, so something more in touch with CSP
should perhaps take a look, but one question and nit about the test.

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html
(right):

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:14:
assert_equals(element.contentDocument.body.innerText, "");
What's this checking? That it's still the about:blank document?

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:15:
element.parentNode.removeChild(element);
element.remove()

[Mike West, 2016-11-10 12:35:37]

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

[commit-bot: I haz the power, 2016-11-10 12:35: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-10 12:44:52]

I improved the tests. :)

Debugging those tests uncovered two issues:

1. `<iframe src="javascript:">` never created a document if the navigation was
blocked. This is technically against spec, which suggests that we should create
an initial document and _then_ do the navigation. Blink combines those steps.
The change in `HTMLFrameElementBase::openURL` should bring us into compliance
now, which is tested by the first test in the file.

2. We only fired _unique_ CSP events. This optimization is meant to reduce
traffic to a server (as completely duplicate reports are ~useless), but
inadvertently blocked DOM events as well. The change to
`ContentSecurityPolicy::reportViolation` fixes that.

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html
(right):

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:14:
assert_equals(element.contentDocument.body.innerText, "");
On 2016/11/10 at 09:25:01, foolip wrote:
> What's this checking? That it's still the about:blank document?

We navigate the document to `javascript:'Fail.'` (see line 25, for instance). If
that navigation succeeds, then `'Fail.'` will be executed, resolved as a string,
and we'll end up navigating to a synthesized response that contains that string
(see
https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-docume...
(yes, this is bonkers and I'd love to burn it to the ground. Yay web.)).

Examining the text content of the document seems like a good way of validating
that that process hasn't happened.

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:15:
element.parentNode.removeChild(element);
On 2016/11/10 at 09:25:01, foolip wrote:
> element.remove()

Oh. Huh. :)

[foolip, 2016-11-11 09:39:40]

test lgtm % nits

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html
(right):

https://codereview.chromium.org/2490943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:14:
assert_equals(element.contentDocument.body.innerText, "");
On 2016/11/10 12:44:52, Mike West wrote:
> On 2016/11/10 at 09:25:01, foolip wrote:
> > What's this checking? That it's still the about:blank document?
> 
> We navigate the document to `javascript:'Fail.'` (see line 25, for instance).
If
> that navigation succeeds, then `'Fail.'` will be executed, resolved as a
string,
> and we'll end up navigating to a synthesized response that contains that
string
> (see
>
https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-docume...
> (yes, this is bonkers and I'd love to burn it to the ground. Yay web.)).
> 
> Examining the text content of the document seems like a good way of validating
> that that process hasn't happened.

OK, thanks for documenting.

https://codereview.chromium.org/2490943002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html
(right):

https://codereview.chromium.org/2490943002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:8:
<iframe src="http://clients1.google.com/generate_204"></iframe>
Should this really be here? None of the tests seem to poke at it.

https://codereview.chromium.org/2490943002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:32:
iframe.setAttribute("src", "javascript:'Fail.'");
iframe.src?

https://codereview.chromium.org/2490943002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/navigation/to-javascript-url.html:39:
i.id = "explicit-src";
Are the IDs used anywhere?

[foolip, 2016-11-11 09:42:40]

ok, code also lgtm, if there's no other reviewer in sight :)

[Mike West, 2016-11-11 09:59:36]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-11-11 09:59:37]

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

[commit-bot: I haz the power, 2016-11-11 09:59:51]

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

[Mike West, 2016-11-11 10:00:30]

Thanks, foolip@. :)

[Mike West, 2016-11-11 10:01:35]

Description was changed from

==========
Block 'javascript:' navigation in the correct document.

While working through [1], I noticed that Blink blocks 'javascript:' URL
navigation in the _target_ document as opposed to in the _navigating_
document. This is particularly incorrect for `<iframe>`.

This patch corrects the targeting.

[1]: https://github.com/w3c/webappsec-csp/issues/127

R=jochen@chromium.org
==========

to

==========
Block 'javascript:' navigation in the correct document.

While working through [1], I noticed that Blink blocks 'javascript:' URL
navigation in the _target_ document as opposed to in the _navigating_
document. This is particularly incorrect for `<iframe>`.

This patch corrects the targeting. As a drive-by, this patch also ensures
that we fire a `SecurityPolicyViolationEvent` for all CSP events. Previously,
we fired only _unique_ CSP events. This optimization is meant to reduce
traffic to a server (as completely duplicate reports are ~useless), but
inadvertently blocked DOM events as well. The change to
`ContentSecurityPolicy::reportViolation` fixes that.

[1]: https://github.com/w3c/webappsec-csp/issues/127

R=jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-11-11 11:15:21]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Block 'javascript:' navigation in the correct document.

While working through [1], I noticed that Blink blocks 'javascript:' URL
navigation in the _target_ document as opposed to in the _navigating_
document. This is particularly incorrect for `<iframe>`.

This patch corrects the targeting. As a drive-by, this patch also ensures
that we fire a `SecurityPolicyViolationEvent` for all CSP events. Previously,
we fired only _unique_ CSP events. This optimization is meant to reduce
traffic to a server (as completely duplicate reports are ~useless), but
inadvertently blocked DOM events as well. The change to
`ContentSecurityPolicy::reportViolation` fixes that.

[1]: https://github.com/w3c/webappsec-csp/issues/127

R=jochen@chromium.org
==========

to

==========
Block 'javascript:' navigation in the correct document.

While working through [1], I noticed that Blink blocks 'javascript:' URL
navigation in the _target_ document as opposed to in the _navigating_
document. This is particularly incorrect for `<iframe>`.

This patch corrects the targeting. As a drive-by, this patch also ensures
that we fire a `SecurityPolicyViolationEvent` for all CSP events. Previously,
we fired only _unique_ CSP events. This optimization is meant to reduce
traffic to a server (as completely duplicate reports are ~useless), but
inadvertently blocked DOM events as well. The change to
`ContentSecurityPolicy::reportViolation` fixes that.

[1]: https://github.com/w3c/webappsec-csp/issues/127

R=jochen@chromium.org

Committed: https://crrev.com/cab8b9f289ebe22d5e5ad79f6150cf729ef5079c
Cr-Commit-Position: refs/heads/master@{#431539}
==========

[commit-bot: I haz the power, 2016-11-11 11:18:17]

Patchset 4 (id:??) landed as
https://crrev.com/cab8b9f289ebe22d5e5ad79f6150cf729ef5079c
Cr-Commit-Position: refs/heads/master@{#431539}