Implement upgrade-insecure-requests in browser for frame requests

content/browser/frame_host/navigation_request.cc

Index: content/browser/frame_host/navigation_request.cc
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index c74c6031fab6ffaaf33cb9384de74038b571f597..1cb627fad9e296500f99e2eac7d91a9659052940 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -945,8 +945,40 @@ NavigationRequest::CheckContentSecurityPolicyFrameSrc(bool is_redirect) {
   SourceLocation source_location;
   if (common_params_.source_location)
     source_location = common_params_.source_location.value();
+
+  // CSP checking happens in three phases, per steps 3-5 of
+  // https://fetch.spec.whatwg.org/#main-fetch:
+  //
+  // (1) Check report-only policies and trigger reports for any violations.
+  // (2) Upgrade the request to HTTPS if necessary.
+  // (3) Check enforced policies (triggering reports for any violations of those
+  //     policies) and block the request if necessary.
+  //
+  // This sequence of events allows site owners to learn about (via step 1) any
+  // requests that are upgraded in step 2.
+
+  bool allowed = parent->IsAllowedByCsp(
+      CSPDirective::FrameSrc, common_params_.url, is_redirect, source_location,
+      CSPContext::CHECK_REPORT_ONLY_CSP);
+  // Checking report-only CSP should never return false because no requests are

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

nit: empty line before the comment.

[estark, 2017/06/06 19:16:14]

Done.

+  // blocked by report-only policies.
+  DCHECK(allowed);
+
+  // TODO(mkwst,estark): upgrade-insecure-requests does not work when following
+  // redirects. Trying to uprade the new URL on redirect here is fruitless: the
+  // redirect URL cannot be changed at this point. upgrade-insecure-requests
+  // needs to move to the net stack to resolve this. https://crbug.com/615885
+  if (!is_redirect) {
+    GURL new_url;
+    if (parent->ShouldModifyRequestUrlForCsp(
+            common_params_.url, true /* is subresource */, &new_url)) {

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

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:16:14]

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

+      common_params_.url = new_url;
+    }
+  }
+
   if (parent->IsAllowedByCsp(CSPDirective::FrameSrc, common_params_.url,
-                             is_redirect, source_location)) {
+                             is_redirect, source_location,
+                             CSPContext::CHECK_ENFORCED_CSP)) {
     return CONTENT_SECURITY_POLICY_CHECK_PASSED;
   }