CSP violation reports should report the pre-redirect URL.

CSP violation reports should report the pre-redirect URL.

Before this patch, blocked cross-origin resource URLs are stripped down
to their origin before being reported to a policy's `report-uri` (same-origin
resources are reported in full). This doesn't match the specced behavior,
which suggests that we ought to be reporting the originally requested URL,
even if the blocked resource is the result of a redirect.

That is, given a policy which blocks `<img src='https://example.test/img.jpg'>`
directly, the report should contain `https://example.test/img.jpg`. If
that URL is allowed, but redirects to `https://example.test/other.jpg`,
which is blocked the report should still contain `https://example.test/img.jpg`
(see the note in https://w3c.github.io/webappsec-csp/#create-violation-for-request
for detail).

This patch gets us ~halfway there, by altering the behavior of
`stripURLForUseInReport` to take account of the redirect status of the blocked
resource. If it has been redirected, we'll keep the status quo stripping behavior.
If it hasn't been redirected, we'll report the entire URL.

A future patch will get redirects working entirely correctly, but given the
value of reporting for things like mixed content detection, I don't think it's
worth waiting for a full patch; there's enough value here over the
status quo to land it and merge it back a bit.

BUG=613960
Committed: https://crrev.com/7fdaebc90aef59e3501139c327db46d2655a1275
Cr-Commit-Position: refs/heads/master@{#396726}

[Mike West, 2016-05-23 10:15:57]

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

[Mike West, 2016-05-23 10:15:57]

Emily, Joel: if you have some time this week, I have ~2 CSP reporting patches
for y'all. I'm on vacation, and not actually in the office, so no rush on
reviewing. :)

This is the other one. :)

[Mike West, 2016-05-23 10:16:06]

Patchset #1 (id:1) has been deleted

[Mike West, 2016-05-25 19:12:09]

Ping. :)

[estark, 2016-05-25 22:58:24]

lgtm with a CL description nit: it took me a while to understand what the effect
of this patch was, because I forgot that cross-origin URLs were being sanitized
even when they were blocked without a redirect. For the benefit of slow people
like me, can you add something to the CL description like, "Specifically, as of
this patch, non-redirected blocked URLs are reported with the full URL, even if
it is cross-origin"?

[jww, 2016-05-26 00:14:00]

lgtm2, with a question on one of your tests. I also second estark@'s confusion
and comment.

https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php
(right):

https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php:44:
}, "Block after redirect, cross-origin = original URL in report");
Did you intend for these two descriptions to be what you *want* them to be,
rather than what they *are* right now?

[jww, 2016-05-26 00:14:38]

On 2016/05/26 00:14:00, jww wrote:
> lgtm2, with a question on one of your tests. I also second estark@'s confusion
> and comment.
> 
>
https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php
> (right):
> 
>
https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php:44:
> }, "Block after redirect, cross-origin = original URL in report");
> Did you intend for these two descriptions to be what you *want* them to be,
> rather than what they *are* right now?

er... lgtm 2

[Mike West, 2016-05-26 03:07:33]

Description was changed from

==========
CSP violation reports should report the pre-redirect URL.

Given a policy which blocks `<img src='https://example.test/img.jpg'>`
directly, the report should contain `https://example.test/img.jpg`. If
that URL is allowed, but redirects to `https://example.test/other.jpg`,
which is blocked the report should still contain `https://example.test/img.jpg`.

See the note in
https://w3c.github.io/webappsec-csp/#create-violation-for-request
for detail.

This patch gets us ~halfway there, reporting the original URL for direct
blockage, and maintaining the existing behavior for redirects. A future
patch will get redirects working correctly, but given the value of
reporting for things like mixed content detection, I don't think it's
worth waiting for a full patch; there's enough value here over the
status quo to land it and merge it back a bit.

BUG=613960
==========

to

==========
CSP violation reports should report the pre-redirect URL.

Before this patch, blocked cross-origin resource URLs are stripped down
to their origin before being reported to a policy's `report-uri` (same-origin
resources are reported in full). This doesn't match the specced behavior,
which suggests that we ought to be reporting the originally requested URL,
even if the blocked resource is the result of a redirect.

That is, given a policy which blocks `<img src='https://example.test/img.jpg'>`
directly, the report should contain `https://example.test/img.jpg`. If
that URL is allowed, but redirects to `https://example.test/other.jpg`,
which is blocked the report should still contain `https://example.test/img.jpg`
(see the note in
https://w3c.github.io/webappsec-csp/#create-violation-for-request
for detail).

This patch gets us ~halfway there, by altering the behavior of
`stripURLForUseInReport` to take account of the redirect status of the blocked
resource. If it has been redirected, we'll keep the status quo stripping
behavior.
If it hasn't been redirected, we'll report the entire URL.

A future patch will get redirects working entirely correctly, but given the
value of reporting for things like mixed content detection, I don't think it's
worth waiting for a full patch; there's enough value here over the
status quo to land it and merge it back a bit.

BUG=613960
==========

[Mike West, 2016-05-30 12:55:54]

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

[commit-bot: I haz the power, 2016-05-30 12:56:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002943002/60001

[commit-bot: I haz the power, 2016-05-30 13:46:47]

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

[commit-bot: I haz the power, 2016-05-30 13:46:48]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-05-30 14:42:56]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-05-30 14:42:56]

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

[commit-bot: I haz the power, 2016-05-30 14:43:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002943002/60001

[commit-bot: I haz the power, 2016-05-30 14:46:38]

Committed patchset #3 (id:60001)

[Mike West, 2016-05-30 14:46:40]

I've updated the tests, and fixed an issue with the (newly-landed) mixed content
checks as well (which required passing the redirect state through the
`reportMixedContent` bits). Thanks!

https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php
(right):

https://codereview.chromium.org/2002943002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php:44:
}, "Block after redirect, cross-origin = original URL in report");
On 2016/05/26 at 00:14:00, jww wrote:
> Did you intend for these two descriptions to be what you *want* them to be,
rather than what they *are* right now?

Yes! Though I realize now that I also intended to make these tests actually fail
and land an `-expectation.txt` file alongside. Thanks!

[commit-bot: I haz the power, 2016-05-30 14:47:58]

Description was changed from

==========
CSP violation reports should report the pre-redirect URL.

Before this patch, blocked cross-origin resource URLs are stripped down
to their origin before being reported to a policy's `report-uri` (same-origin
resources are reported in full). This doesn't match the specced behavior,
which suggests that we ought to be reporting the originally requested URL,
even if the blocked resource is the result of a redirect.

That is, given a policy which blocks `<img src='https://example.test/img.jpg'>`
directly, the report should contain `https://example.test/img.jpg`. If
that URL is allowed, but redirects to `https://example.test/other.jpg`,
which is blocked the report should still contain `https://example.test/img.jpg`
(see the note in
https://w3c.github.io/webappsec-csp/#create-violation-for-request
for detail).

This patch gets us ~halfway there, by altering the behavior of
`stripURLForUseInReport` to take account of the redirect status of the blocked
resource. If it has been redirected, we'll keep the status quo stripping
behavior.
If it hasn't been redirected, we'll report the entire URL.

A future patch will get redirects working entirely correctly, but given the
value of reporting for things like mixed content detection, I don't think it's
worth waiting for a full patch; there's enough value here over the
status quo to land it and merge it back a bit.

BUG=613960
==========

to

==========
CSP violation reports should report the pre-redirect URL.

Before this patch, blocked cross-origin resource URLs are stripped down
to their origin before being reported to a policy's `report-uri` (same-origin
resources are reported in full). This doesn't match the specced behavior,
which suggests that we ought to be reporting the originally requested URL,
even if the blocked resource is the result of a redirect.

That is, given a policy which blocks `<img src='https://example.test/img.jpg'>`
directly, the report should contain `https://example.test/img.jpg`. If
that URL is allowed, but redirects to `https://example.test/other.jpg`,
which is blocked the report should still contain `https://example.test/img.jpg`
(see the note in
https://w3c.github.io/webappsec-csp/#create-violation-for-request
for detail).

This patch gets us ~halfway there, by altering the behavior of
`stripURLForUseInReport` to take account of the redirect status of the blocked
resource. If it has been redirected, we'll keep the status quo stripping
behavior.
If it hasn't been redirected, we'll report the entire URL.

A future patch will get redirects working entirely correctly, but given the
value of reporting for things like mixed content detection, I don't think it's
worth waiting for a full patch; there's enough value here over the
status quo to land it and merge it back a bit.

BUG=613960

Committed: https://crrev.com/7fdaebc90aef59e3501139c327db46d2655a1275
Cr-Commit-Position: refs/heads/master@{#396726}
==========

[commit-bot: I haz the power, 2016-05-30 14:47:59]

Patchset 3 (id:??) landed as
https://crrev.com/7fdaebc90aef59e3501139c327db46d2655a1275
Cr-Commit-Position: refs/heads/master@{#396726}