Implement upgrade-insecure-requests in browser for frame requests

Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evaluate report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

There are other cases for which we might need to do something similar which
are not handled by this CL: namely form submissions and same-host main-frame
navigations.

Also note that I'm not attempting to apply upgrade-insecure-requests when
following redirects. UIR in general does not work when following redirects, and
that's a much larger issue outside the scope of this CL.
(https://crbug.com/615885)

This is a follow-up to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2910573002
Cr-Commit-Position: refs/heads/master@{#478478}
Committed: https://chromium.googlesource.com/chromium/src/+/4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee

[estark, 2017-05-26 00:12:45]

Description was changed from

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process, and properly splits up CSP
checks per spec: (1) evalute report-only CSPs, (2) upgrade request if needed,
(3) evaluate enforced CSPs.

A similar change may need to be made for form submissions as a follow-up.

This is a follow-up CL to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
==========

to

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process, and properly splits up CSP
checks per spec: (1) evalute report-only CSPs, (2) upgrade request if needed,
(3) evaluate enforced CSPs.

A similar change may need to be made for form submissions as a follow-up.

This is a follow-up CL to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[estark, 2017-05-26 00:12:59]

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

[commit-bot: I haz the power, 2017-05-26 00:13:54]

Dry run: CQ is trying da patch.

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

[estark, 2017-05-26 00:14:04]

Description was changed from

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process, and properly splits up CSP
checks per spec: (1) evalute report-only CSPs, (2) upgrade request if needed,
(3) evaluate enforced CSPs.

A similar change may need to be made for form submissions as a follow-up.

This is a follow-up CL to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evalute report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

A similar change may need to be made for form submissions as a follow-up.

This is a follow-up CL to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-26 00:18:20]

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

[commit-bot: I haz the power, 2017-05-26 00:18:21]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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...)
  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-05-26 00:45:19]

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

[commit-bot: I haz the power, 2017-05-26 00:46: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-05-26 00:52:07]

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

[commit-bot: I haz the power, 2017-05-26 00:52:19]

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-05-26 01:06:09]

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

[commit-bot: I haz the power, 2017-05-26 01:06:10]

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

[estark, 2017-05-26 01:26:34]

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

[commit-bot: I haz the power, 2017-05-26 01:27:15]

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-05-26 02:07:17]

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

[commit-bot: I haz the power, 2017-05-26 02:07:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[estark, 2017-05-26 02:21:38]

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

[commit-bot: I haz the power, 2017-05-26 02:22:33]

Dry run: CQ is trying da patch.

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

[estark, 2017-05-26 04:27:20]

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

[commit-bot: I haz the power, 2017-05-26 04:27:52]

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-05-26 07:17:31]

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

[commit-bot: I haz the power, 2017-05-26 07:17:32]

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-05-26 18:06:53]

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

[commit-bot: I haz the power, 2017-05-26 18:07:31]

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-05-26 19:57:03]

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

[commit-bot: I haz the power, 2017-05-26 19:57:04]

Dry run: This issue passed the CQ dry run.

[estark, 2017-05-27 00:32:36]

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

[commit-bot: I haz the power, 2017-05-27 00:32:46]

Dry run: CQ is trying da patch.

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

[estark, 2017-05-27 00:34:52]

Description was changed from

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evalute report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

A similar change may need to be made for form submissions as a follow-up.

This is a follow-up CL to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evaluate report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

There are other cases for which we might need to do something similar which
are not handled by this CL: namely form submissions and same-host main-frame
navigations.

Also note that I'm not attempting to apply upgrade-insecure-requests when
following redirects. UIR in general does not work when following redirects, and
that's a much larger issue outside the scope of this CL.
(https://crbug.com/615885)

This is a follow-up to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-27 00:42:46]

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

[commit-bot: I haz the power, 2017-05-27 00:42:47]

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

[estark, 2017-05-27 01:56:24]

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

[commit-bot: I haz the power, 2017-05-27 01:56: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-05-27 03:42:40]

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

[commit-bot: I haz the power, 2017-05-27 03:42:41]

Dry run: This issue passed the CQ dry run.

[estark, 2017-05-29 20:56:51]

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

[estark, 2017-05-29 20:56:53]

This CL has some unit tests, but its main test is a web-platform-test that I
added previously:
content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html

That test used to time out with --enable-browser-side-navigation but now passes.
Not sure if there is somewhere I should mark that this test now passes with
PlzNavigate?

[Mike West, 2017-05-30 07:36:23]

On 2017/05/29 at 20:56:53, estark wrote:
> This CL has some unit tests, but its main test is a web-platform-test that I
added previously:
content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html

You are the best. This approach seems reasonable to me. //third_party/WebKit
LGTM % nits, and the rest looks fine too.

> That test used to time out with --enable-browser-side-navigation but now
passes. Not sure if there is somewhere I should mark that this test now passes
with PlzNavigate?

You can change the test expectations in
`//third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation`.

[Mike West, 2017-05-30 07:36:31]

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1402:
blink::WebString("upgrade-insecure-requests"),
Can you add this directive to the method's comment in the .h file?

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

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1600: settings &&
settings->GetBrowserSideNavigationEnabled()) {
Tiny nit: You could skip checking `settings` here if you wrapped the assignment
in an if. Like `if (Settings* settings = frame_->GetSettings())`

[estark, 2017-06-01 04:17:47]

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

[commit-bot: I haz the power, 2017-06-01 04:18:04]

Dry run: CQ is trying da patch.

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

[estark, 2017-06-01 04:18:29]

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

[estark, 2017-06-01 04:18:30]

Thanks Mike! Nasko, this is #2/2 for upgrade-insecure-requests reporting with
PlzNavigate (follow-up to https://codereview.chromium.org/2909513002/)

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:1402:
blink::WebString("upgrade-insecure-requests"),
On 2017/05/30 07:36:30, Mike West wrote:
> Can you add this directive to the method's comment in the .h file?

Done.

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

https://codereview.chromium.org/2910573002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1600: settings &&
settings->GetBrowserSideNavigationEnabled()) {
On 2017/05/30 07:36:30, Mike West wrote:
> Tiny nit: You could skip checking `settings` here if you wrapped the
assignment
> in an if. Like `if (Settings* settings = frame_->GetSettings())`

I'll skip this one if you don't mind -- my weakling brain finds such things hard
to read. Happy to do it if you feel strongly though :)

[commit-bot: I haz the power, 2017-06-01 05:21:15]

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

[commit-bot: I haz the power, 2017-06-01 05:21:16]

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

[nasko, 2017-06-01 05:26:53]

nasko@chromium.org changed reviewers:
+ clamy@chromium.org

[nasko, 2017-06-01 05:26:54]

Adding clamy@ here too, as this is a follow up to the other CL.

[estark, 2017-06-01 05:53:41]

Yeah -- I mentioned in the other cl, upgrade-insecure-requests in general
doesn't work on redirects now. We'll have to fix that in some other way,
but I think for now it's good to report on requests that we do upgrade.

On May 31, 2017 10:26 PM, "nasko@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Adding clamy@ here too, as this is a follow up to the other CL.
>
> https://codereview.chromium.org/2910573002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[estark, 2017-06-01 05:53:41]

Yeah -- I mentioned in the other cl, upgrade-insecure-requests in general
doesn't work on redirects now. We'll have to fix that in some other way,
but I think for now it's good to report on requests that we do upgrade.

On May 31, 2017 10:26 PM, "nasko@chromium.org via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Adding clamy@ here too, as this is a follow up to the other CL.
>
> https://codereview.chromium.org/2910573002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2017-06-05 21:31:49]

Reviewed only content/browser/frame_host/. Let me know if my review is needed
elsewhere.

Overall looks good, just one clarification question and some notes.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
File content/browser/frame_host/form_submission_throttle.cc (right):

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/form_submission_throttle.cc:65: // CSP to match how
frame-src works. https://crbug.com/713388
Alternatively, we can see if it makes sense to have
NavigationThrottle/NavigationHandle gain functionality to allow updating of the
URL for such cases. It is easier if we have all the checks in throttles code
instead of having some in NavigaitonRequest and some in throttles. However, this
should have no impact on this CL, just thinking aloud about the comment.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:963: // Checking report-only
CSP should never return false because no requests are
nit: empty line before the comment.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:974: common_params_.url, true
/* is subresource */, &new_url)) {
We will have to be careful here with the changes coming from davidben@ and the
work we need to do in PlzNavigate to not update common_params.url until all
throttles have returned success. 
This needs to be the URL that the request is being redirected to, correct?

Are subframes that navigate considered subresource?

[estark, 2017-06-06 19:10:39]

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

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

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-06-06 19:14:08]

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

[commit-bot: I haz the power, 2017-06-06 19:14:09]

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-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-06-06 19:16:15]

I added a test and ran into an additional exciting issue that we need to update
|request_params_.original_url| or else the renderer ends up thinking it has
navigated to the pre-upgrade URL
(https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=400).
See rambling comment inline.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
File content/browser/frame_host/form_submission_throttle.cc (right):

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/form_submission_throttle.cc:65: // CSP to match how
frame-src works. https://crbug.com/713388
On 2017/06/05 21:31:49, nasko (slow) wrote:
> Alternatively, we can see if it makes sense to have
> NavigationThrottle/NavigationHandle gain functionality to allow updating of
the
> URL for such cases. It is easier if we have all the checks in throttles code
> instead of having some in NavigaitonRequest and some in throttles. However,
this
> should have no impact on this CL, just thinking aloud about the comment.

Acknowledged. I agree it could be nice to allow the throttle to do this logic,
since it's kind of weird to have this one check done here and other similar
checks in throttles. However, one challenge with that approach is that we'd have
to be careful about the order that throttles run in, because the throttle that
does the CSP upgrade would have to run, and be allowed to update the URL, before
the mixed content throttle.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:963: // Checking report-only
CSP should never return false because no requests are
On 2017/06/05 21:31:49, nasko (slow) wrote:
> nit: empty line before the comment.

Done.

https://codereview.chromium.org/2910573002/diff/190001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:974: common_params_.url, true
/* is subresource */, &new_url)) {
On 2017/06/05 21:31:49, nasko (slow) wrote:
> We will have to be careful here with the changes coming from davidben@ and the
> work we need to do in PlzNavigate to not update common_params.url until all
> throttles have returned success. 
> This needs to be the URL that the request is being redirected to, correct?

Yes. As discussed in person, there are some open questions around how UIR should
work for redirects, and especially UIR+reporting. I think UIR will probably be
moving to the net stack for redirects, similar to referrer policy, but how we
will report upgrades that happen in the net stack, I have no idea!

> 
> Are subframes that navigate considered subresource?

Yes, according to
https://w3c.github.io/webappsec-upgrade-insecure-requests/#upgrade-request Step
2.2, navigation requests for nested frames should be upgraded if the parent has
upgrade-insecure-requests set.

I added a test for this to upgrade-insecure-requests-reporting.https.html.
However, in the course of doing so, I realized that (as discussed in person
yesterday) merely updating common_params.url doesn't update the URL that the
renderer sees for the navigation. So the renderer ended up thinking that the
request was from the http:// URL. 

To resolve this I ended up updating the original_url as well... how do you feel
about that? (It fixes the issue but feels weird to change something that's
called "original", and I'm still waiting for tests to run so I don't even know
yet if it breaks anything.)

[estark, 2017-06-06 21:01:55]

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

[commit-bot: I haz the power, 2017-06-06 21:02:21]

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-06-06 23:25:45]

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

[commit-bot: I haz the power, 2017-06-06 23:25:47]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-06-08 14:51:19]

On 2017/06/06 19:16:15, estark (slow thru Jun8) wrote:
> 
> I added a test for this to upgrade-insecure-requests-reporting.https.html.
> However, in the course of doing so, I realized that (as discussed in person
> yesterday) merely updating common_params.url doesn't update the URL that the
> renderer sees for the navigation. So the renderer ended up thinking that the
> request was from the http:// URL. 
> 
> To resolve this I ended up updating the original_url as well... how do you
feel
> about that? (It fixes the issue but feels weird to change something that's
> called "original", and I'm still waiting for tests to run so I don't even know
> yet if it breaks anything.)

I think it makes sense, as UIR does not function as a redirect, but a rewrite of
the URL. The original_url is used when we have encountered redirects, so I think
it is the right thing to update it for UIR.

Camille, would you please double check my reasoning is correct and it is the
right thing to do?

[clamy, 2017-06-09 14:43:20]

On 2017/06/08 14:51:19, nasko (slow) wrote:
> On 2017/06/06 19:16:15, estark (slow thru Jun8) wrote:
> > 
> > I added a test for this to upgrade-insecure-requests-reporting.https.html.
> > However, in the course of doing so, I realized that (as discussed in person
> > yesterday) merely updating common_params.url doesn't update the URL that the
> > renderer sees for the navigation. So the renderer ended up thinking that the
> > request was from the http:// URL. 
> > 
> > To resolve this I ended up updating the original_url as well... how do you
> feel
> > about that? (It fixes the issue but feels weird to change something that's
> > called "original", and I'm still waiting for tests to run so I don't even
know
> > yet if it breaks anything.)
> 
> I think it makes sense, as UIR does not function as a redirect, but a rewrite
of
> the URL. The original_url is used when we have encountered redirects, so I
think
> it is the right thing to update it for UIR.
> 
> Camille, would you please double check my reasoning is correct and it is the
> right thing to do?

That seems correct to me.

[nasko, 2017-06-09 22:25:41]

content/browser/frame_host/ LGTM

[estark, 2017-06-09 22:55:31]

The CQ bit was checked by estark@chromium.org

[estark, 2017-06-09 22:55:31]

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/2910573002/#ps230001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-09 22:55:56]

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-06-10 00:46:10]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1497048931313490,
"parent_rev": "bb0267315083d4fbc1fbd9f308370e5573e79c44", "commit_rev":
"4bb7f5d6743f3ae5a69f1624c6e3547ba4f234ee"}

[commit-bot: I haz the power, 2017-06-10 00:46:22]

Description was changed from

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evaluate report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

There are other cases for which we might need to do something similar which
are not handled by this CL: namely form submissions and same-host main-frame
navigations.

Also note that I'm not attempting to apply upgrade-insecure-requests when
following redirects. UIR in general does not work when following redirects, and
that's a much larger issue outside the scope of this CL.
(https://crbug.com/615885)

This is a follow-up to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement upgrade-insecure-requests in browser for frame requests

In PlzNavigate, frame requests currently have most of their CSP checks done in
the browser process. But upgrade-insecure-requests was still applied in Blink,
meaning that upgraded frame requests couldn't be properly reported.

This CL moves upgrading into the browser process for frame requests, and
properly splits up CSP checks per spec: (1) evaluate report-only CSPs,
(2) upgrade request if needed, (3) evaluate enforced CSPs.

There are other cases for which we might need to do something similar which
are not handled by this CL: namely form submissions and same-host main-frame
navigations.

Also note that I'm not attempting to apply upgrade-insecure-requests when
following redirects. UIR in general does not work when following redirects, and
that's a much larger issue outside the scope of this CL.
(https://crbug.com/615885)

This is a follow-up to https://codereview.chromium.org/2909513002/, and is
the browser-process version of https://codereview.chromium.org/2790693002.

BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2910573002
Cr-Commit-Position: refs/heads/master@{#478478}
Committed:
https://chromium.googlesource.com/chromium/src/+/4bb7f5d6743f3ae5a69f1624c6e3...
==========

[commit-bot: I haz the power, 2017-06-10 00:46:24]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/4bb7f5d6743f3ae5a69f1624c6e3...