Split CSP into pre- and post-upgrade checks

Split CSP into pre- and post-upgrade checks

When populating a resource request, before upgrading an insecure request
if appropriate, we now check report-only CSP headers, to ensure that
CSP report-only violations are reported before any modifications of the
request. After modifying the request, we check the enforced CSP headers to
ensure that the request is still allowed.

This is as described in the upgrade-insecure-requests spec:
https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades

Patch stolen from mkwst@

BUG=625156
TEST=added web platform test upgrade-insecure-requests-reporting.https.html

Review-Url: https://codereview.chromium.org/2790693002
Cr-Commit-Position: refs/heads/master@{#465741}
Committed: https://chromium.googlesource.com/chromium/src/+/a98bc6ea35dbcc498eaa7a84b67e91622d8eb4d0

[estark, 2017-03-30 18:40:32]

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

[commit-bot: I haz the power, 2017-03-30 18:41:00]

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-30 18:44:39]

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

[commit-bot: I haz the power, 2017-03-30 18:44:40]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2017-03-30 18:54:02]

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

[commit-bot: I haz the power, 2017-03-30 18:54: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-30 20:23:34]

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

[commit-bot: I haz the power, 2017-03-30 20:23:35]

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_...)

[estark, 2017-04-07 21:01:44]

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

[commit-bot: I haz the power, 2017-04-07 21:03:26]

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-04-07 21:07:43]

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

[commit-bot: I haz the power, 2017-04-07 21:07:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[estark, 2017-04-07 21:26:10]

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

[commit-bot: I haz the power, 2017-04-07 21:27:05]

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-04-07 21:40:14]

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

[commit-bot: I haz the power, 2017-04-07 21:40:15]

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

[estark, 2017-04-07 21:46:22]

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

[commit-bot: I haz the power, 2017-04-07 21:46:53]

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-04-07 22:17:31]

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

[commit-bot: I haz the power, 2017-04-07 22:17:32]

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

[estark, 2017-04-08 01:19:35]

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

[commit-bot: I haz the power, 2017-04-08 01:19:56]

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-04-08 02:28:33]

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

[commit-bot: I haz the power, 2017-04-08 02:28:34]

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_...)

[estark, 2017-04-11 17:53:04]

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

[commit-bot: I haz the power, 2017-04-11 17:53:36]

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-04-11 19:10:18]

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

[commit-bot: I haz the power, 2017-04-11 19:10:19]

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_...)

[estark, 2017-04-11 22:01:46]

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

[commit-bot: I haz the power, 2017-04-11 22:02:47]

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-04-11 23:40:38]

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

[commit-bot: I haz the power, 2017-04-11 23:40:39]

Dry run: This issue passed the CQ dry run.

[estark, 2017-04-12 00:44:01]

Description was changed from

==========
[WIP] Split CSP into pre- and post-upgrade checks

Patch stolen from mkwst@

BUG=625156
==========

to

==========
Split CSP into pre- and post-upgrade checks

When populating a resource request, before upgrading an insecure request
if appropriate, we now check report-only CSP headers, to ensure that
CSP report-only violations are reported before any modifications of the
request. After modifying the request, we check the enforced CSP headers to
ensure that the request is still allowed.

This is as described in the upgrade-insecure-requests spec:
https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades

Patch stolen from mkwst@

BUG=625156
TEST=added web platform test upgrade-insecure-requests-reporting.https.html
==========

[estark, 2017-04-12 00:44:40]

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

[commit-bot: I haz the power, 2017-04-12 00:45:13]

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

[estark, 2017-04-12 01:30:10]

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

[commit-bot: I haz the power, 2017-04-12 01:30:48]

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

[estark, 2017-04-12 01:35:18]

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

[estark, 2017-04-12 01:35:19]

mkwst, PTAL at this patch that is mostly stolen from you? :) This is so that
requests upgraded by upgrade-insecure-requests still get CSP reports sent for
them.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html
(right):

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:16:
var loaded = false;
surely there is some nicer way to write a test with two step_funcs where the
test is done when both steps have completed...?

[commit-bot: I haz the power, 2017-04-12 03:30:42]

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

[commit-bot: I haz the power, 2017-04-12 03:30:43]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-04-12 13:31:36]

The code and overall approach looks good, but I'd like to see a few more tests.
In particular, it would be good to verify the behavior for redirects, and for
frames (as the latter's enforcement has moved up the stack to the browser side).
Depending on the complexity there, it might make sense to punt on dealing with
redirected frames in a first pass if it doesn't work out of the box.

(Thank you for picking this up... I lost track of it entirely. :( )

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html
(right):

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:16:
var loaded = false;
On 2017/04/12 at 01:35:19, estark wrote:
> surely there is some nicer way to write a test with two step_funcs where the
test is done when both steps have completed...?

You'd think so. But you have to build it yourself. This, of course, is absurd.
:(

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:27:
assert_equals(i.naturalWidth, 168, "Width.");
Best to drop these comparisons. Andy found that WPT sometimes fails to deliver
images, which makes for flaky tests. :( The `onload` check is probably enough.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:34:
}, "Upgraded image is reported");
Can you add an iframe check as well? Some of that code has moved up to
//content, so let's verify that we're checking it early enough in blink to catch
the upgrade.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-blocked-uri-cross-origin-expected.txt
(right):

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-blocked-uri-cross-origin-expected.txt:1:
CONSOLE ERROR: line 3: [Report Only] Refused to load the image
'http://localhost:8080/security/resources/abe.png' because it violates the
following Content Security Policy directive: "img-src 'none'".
Why did these change? Just timing?

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

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php:2:
header("Content-Security-Policy-Report-Only: img-src http://allowed.test:8000");
Hrm. How did this ever work?

[estark, 2017-04-13 01:35:17]

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

[commit-bot: I haz the power, 2017-04-13 01:36:56]

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

[estark, 2017-04-13 01:42:18]

Thanks Mike! Your comments triggered me to discover an exciting series of
riddles wrapped in mysteries inside enigmas. See inline for the most part, but I
also did a little reshuffling where I changed the header type arguments to the
Allow*FromSource methods on ContentSecurityPolicy. I realized that there were a
number of one-off callsites (for example, [1]) that I had unwittingly changed to
check only enforced headers (since that was the default HeaderType argument)
whereas before they were checking report-only and enforced headers. (A little
disturbingly, no tests failed...) So I changed the header type argument to
default to checking both report-only and enforced headers, and then only in the
callsites we care about (FrameFetchContext and FrameLoader) do I limit to
report-only or enforced.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html
(right):

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:16:
var loaded = false;
On 2017/04/12 13:31:36, Mike West wrote:
> On 2017/04/12 at 01:35:19, estark wrote:
> > surely there is some nicer way to write a test with two step_funcs where the
> test is done when both steps have completed...?
> 
> You'd think so. But you have to build it yourself. This, of course, is absurd.
> :(

Acknowledged.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:27:
assert_equals(i.naturalWidth, 168, "Width.");
On 2017/04/12 13:31:36, Mike West wrote:
> Best to drop these comparisons. Andy found that WPT sometimes fails to deliver
> images, which makes for flaky tests. :( The `onload` check is probably enough.

Done.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html:34:
}, "Upgraded image is reported");
On 2017/04/12 13:31:36, Mike West wrote:
> Can you add an iframe check as well? Some of that code has moved up to
> //content, so let's verify that we're checking it early enough in blink to
catch
> the upgrade.

I have learned many exciting things:

- I added an iframe test, which revealed that this check needed to be duplicated
in a second place inside Blink, in FrameLoader.

- AFAICT the upgrade always happens in Blink, either in FrameLoader or
FrameFetchContext, but in PlzNavigate, the frame-src CSP gets checked in the
browser process, so this whole approach doesn't work for PlzNavigate. One could
imagine checking report-only headers in Blink, then doing the upgrade in Blink,
then checking enforced headers in //content... but that seems extremely weird to
me and I have no idea what it might break. (It already seems weird that the
upgrade is in Blink while the CSP checking is in //content in the browser
process.) So I left a TODO for the PlzNavigate case for now since I have no idea
what to do about it.

- I added a general (not uir related) wpt to test that both enforced and
report-only policies are checked on frames that redirect, however...

- I then tried writing a test for when a frame redirects from https to http but
things are broken x2. First, UIR doesn't work for redirects
(https://crbug.com/615885), so we can't really test the whole scenario (i.e.
that a frame that redirects from https to http is both reported and upgraded).
So then I thought maybe we should just test that a frame redirects from https to
http is reported, even if we can't test that it's upgraded, but that's broken
because it looks like mixed content blocking and CSP reporting don't play
nicely: https://crbug.com/711105. Soooo, long story short, I think we should
punt for now on testing that iframes that redirect to a mixed URL are reported,
but I did add a test that an img which redirects to http gets reported.

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-blocked-uri-cross-origin-expected.txt
(right):

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-blocked-uri-cross-origin-expected.txt:1:
CONSOLE ERROR: line 3: [Report Only] Refused to load the image
'http://localhost:8080/security/resources/abe.png' because it violates the
following Content Security Policy directive: "img-src 'none'".
On 2017/04/12 13:31:36, Mike West wrote:
> Why did these change? Just timing?

DARN I assumed you knew and I wouldn't have to dig into it because it was in the
patch I stole from you :P

Ok but more seriously, I've spent a while looking into this and something very
strange is going on. I'll need a while more to figure it out, so I'm mailing the
updated CL so you can review the rest of it in the meantime.

When I load up this test in stable Chrome, the line number is reported as 4 when
I first load the test in the tab, and then 3 on subsequent loads of the test in
the same tab. O.o Whereas with this patch it seems to consistently report as 3
(which does seem to be the correct line number). So I'm inclined to say this is
an improvement, albeit one I don't understand at all.

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

https://codereview.chromium.org/2790693002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/report-original-url.php:2:
header("Content-Security-Policy-Report-Only: img-src http://allowed.test:8000");
On 2017/04/12 13:31:36, Mike West wrote:
> Hrm. How did this ever work?

Hmm I don't know why I made this change, it was bogus.

Now I see that this test was hanging indefinitely (both in stable and my local
build with this CL) because the TODOs on lines 32 and 40 appear to have been
done at some point. So the test was just waiting around for blockedURIs that
were never reported, because the correct pre-redirect URLs were being reported
instead. I don't, however, understand why it wasn't timing out... but I've at
least fixed it now so that it checks for the proper pre-redirect URLs and
confirmed that it passes with that change.

[commit-bot: I haz the power, 2017-04-13 01:43:30]

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

[commit-bot: I haz the power, 2017-04-13 01:43:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2017-04-13 02:31:46]

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

[estark, 2017-04-13 02:31:53]

https://codereview.chromium.org/2790693002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/frame-src/frame-src-redirect.html
(right):

https://codereview.chromium.org/2790693002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/frame-src/frame-src-redirect.html:10:
if (e.originalPolicy == policy && (new URL(e.blockedURI)).origin ==
blocked_origin)
I'm only checking origin here rather than the full URL because Chrome has some
detailed rules about when it strips URLs to origin (for example, it allows
reports blockedURI as the origin only on frame-src violations) and I'm not sure
if other browsers follow suit.

Normally I'd leave a comment explaining in the test, but not sure if such a
Chrome-specific comment makes sense in wpt...

[commit-bot: I haz the power, 2017-04-13 02:33:16]

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-04-13 03:33:13]

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

[commit-bot: I haz the power, 2017-04-13 03:33:14]

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

[estark, 2017-04-13 04:01:08]

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

[commit-bot: I haz the power, 2017-04-13 04:01:34]

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-04-13 05:59:11]

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

[commit-bot: I haz the power, 2017-04-13 05:59:13]

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

[Mike West, 2017-04-14 16:12:10]

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

[Mike West, 2017-04-14 16:12:12]

I'm OOO through Tuesday; I skimmed this, and it looks reasonable, but I'm
supposed to be playing in a sandbox right now, and didn't put enough effort in.
Perhaps Eric would be interested in reviewing in my absence? If not, Tuesday. :P

[elawrence, 2017-04-14 16:39:44]

On 2017/04/14 16:12:12, Mike West (OOO until 18th) wrote:
> I'm OOO through Tuesday; I skimmed this, and it looks reasonable, but I'm
> supposed to be playing in a sandbox right now, and didn't put enough effort
in.
> Perhaps Eric would be interested in reviewing in my absence? If not, Tuesday.
:P

I am, alas, out today as well, and my quick skim mostly yielded the realization
that I'm not yet qualified to review this.

[Mike West, 2017-04-19 08:31:24]

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

[commit-bot: I haz the power, 2017-04-19 08:31:44]

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-04-19 08:36:11]

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

[commit-bot: I haz the power, 2017-04-19 08:36:12]

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

[Mike West, 2017-04-19 08:42:13]

LGTM.

Thank you for digging into this, Emily. The plznavigate bit is a mess, indeed.
And it's my fault that we're not upgrading redirects, as I've apparently let
that CL lapse again. Ugh.

Let's land this, as it's better than status quo, and worry about cleaning up
navigation once UIR works correctly, and PlzNavigate actually launches.

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:963: //
impact of this backwards-incompatible change.
Nit: Can you add a `// TODO(mkwst): We reverted this.`? :)

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1505: // the browser
process too. See also https://crbug.com/692595
Ugh. Yes. This is a mess.

Given infinite time, we should stop doing one set of checks in the browser, and
another in Blink. Given the world we live in, let's punt it.

[estark, 2017-04-19 16:20:14]

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

[commit-bot: I haz the power, 2017-04-19 16:20:52]

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

[estark, 2017-04-19 16:23:44]

The CQ bit was checked by estark@chromium.org

[estark, 2017-04-19 16:23:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2790693002/#ps340001
(title: "add mkwst TODO")

[estark, 2017-04-19 16:23:54]

Thanks, Mike!

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2790693002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:963: //
impact of this backwards-incompatible change.
On 2017/04/19 08:42:13, Mike West wrote:
> Nit: Can you add a `// TODO(mkwst): We reverted this.`? :)

Done.

[commit-bot: I haz the power, 2017-04-19 16:24:21]

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-04-19 18:40:29]

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

[commit-bot: I haz the power, 2017-04-19 18:40:31]

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_...)

[estark, 2017-04-19 20:00:10]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2017-04-19 20:00:48]

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-04-19 20:53:01]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492632010970840,
"parent_rev": "baa379d2816ac03540dc343fa20d6f0ba9791475", "commit_rev":
"a98bc6ea35dbcc498eaa7a84b67e91622d8eb4d0"}

[commit-bot: I haz the power, 2017-04-19 20:53:14]

Description was changed from

==========
Split CSP into pre- and post-upgrade checks

When populating a resource request, before upgrading an insecure request
if appropriate, we now check report-only CSP headers, to ensure that
CSP report-only violations are reported before any modifications of the
request. After modifying the request, we check the enforced CSP headers to
ensure that the request is still allowed.

This is as described in the upgrade-insecure-requests spec:
https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades

Patch stolen from mkwst@

BUG=625156
TEST=added web platform test upgrade-insecure-requests-reporting.https.html
==========

to

==========
Split CSP into pre- and post-upgrade checks

When populating a resource request, before upgrading an insecure request
if appropriate, we now check report-only CSP headers, to ensure that
CSP report-only violations are reported before any modifications of the
request. After modifying the request, we check the enforced CSP headers to
ensure that the request is still allowed.

This is as described in the upgrade-insecure-requests spec:
https://w3c.github.io/webappsec-upgrade-insecure-requests/#reporting-upgrades

Patch stolen from mkwst@

BUG=625156
TEST=added web platform test upgrade-insecure-requests-reporting.https.html

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

[commit-bot: I haz the power, 2017-04-19 20:53:16]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/a98bc6ea35dbcc498eaa7a84b67e...