<a ping="..."> should be covered by connect-src CSP directive.

<a ping="..."> should be covered by connect-src CSP directive.

BUG=663048
Committed: https://crrev.com/3678dd47cb4ccb61fa4281dfdcc5b92adc6c21de
Cr-Commit-Position: refs/heads/master@{#430629}

[Łukasz Anforowicz, 2016-11-07 21:23:31]

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

[commit-bot: I haz the power, 2016-11-07 21:23:58]

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

[Łukasz Anforowicz, 2016-11-07 21:25:48]

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

[Łukasz Anforowicz, 2016-11-07 21:25:49]

mkwst@, can you take a look please?  It turns out that before this CL, <a
ping="..."> was not covered by connect-src (i.e. before product changes from
this CL, dummy.txt would be fetched while executing the layout test;  "fetched"
= ad-hoc printf debugging would show that URLRequest::Start would execute for
this URI).

[commit-bot: I haz the power, 2016-11-07 23:08:39]

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

[commit-bot: I haz the power, 2016-11-07 23:08:40]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-08 14:24:59]

https://codereview.chromium.org/2483903003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/PingLoader.cpp (right):

https://codereview.chromium.org/2483903003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/PingLoader.cpp:423: }
This does too much, as it also ends up applying CSP to certain kinds of image
loads (`PingLoader::loadImage`) and to CSP violation reports
(`PingLoader::sendViolationReport`). I'd suggest moving the check either to
`PingLoader::sendLinkAuditPing`, or out of `PingLoader` entirely, into
`HTMLAnchorElement::sendPings`. I'm happy either way.

[Mike West, 2016-11-08 14:25:12]

Thanks for taking a look! One comment inline.

[Łukasz Anforowicz, 2016-11-08 14:36:02]

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

[commit-bot: I haz the power, 2016-11-08 14:36:18]

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

[Łukasz Anforowicz, 2016-11-08 14:38:59]

Thanks for the review.

https://codereview.chromium.org/2483903003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/PingLoader.cpp (right):

https://codereview.chromium.org/2483903003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/PingLoader.cpp:423: }
On 2016/11/08 14:24:59, Mike West wrote:
> This does too much, as it also ends up applying CSP to certain kinds of image
> loads (`PingLoader::loadImage`) and to CSP violation reports
> (`PingLoader::sendViolationReport`). I'd suggest moving the check either to
> `PingLoader::sendLinkAuditPing`, or out of `PingLoader` entirely, into
> `HTMLAnchorElement::sendPings`. I'm happy either way.

Thanks for catching this.  I should have checked what other kinds of "pings"
there are... :-/

Done.

https://codereview.chromium.org/2483903003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2483903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:832: case
WebURLRequest::RequestContextPing:
Handling RequestContextPing here is probably wrong (and so - I've reverted this
in the latest patchset):

1) It is not needed for the new test to pass.

2) You've pointed out that there are more types of pings (in addition to the
anchor ping that we want to block here).

[commit-bot: I haz the power, 2016-11-08 14:49:34]

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

[commit-bot: I haz the power, 2016-11-08 14:49:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mike West, 2016-11-08 15:42:07]

LGTM, thanks.

[Łukasz Anforowicz, 2016-11-08 16:03:35]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-11-08 16:04:00]

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-08 16:24:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-08 16:34:38]

Description was changed from

==========
<a ping="..."> should be covered by connect-src CSP directive.

BUG=663048
==========

to

==========
<a ping="..."> should be covered by connect-src CSP directive.

BUG=663048

Committed: https://crrev.com/3678dd47cb4ccb61fa4281dfdcc5b92adc6c21de
Cr-Commit-Position: refs/heads/master@{#430629}
==========

[commit-bot: I haz the power, 2016-11-08 16:34:40]

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