WIP: Move 'Upgrade-Insecure-Requests' to the browser process.

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 101 matching lines @@
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_job_factory.h"
 #include "storage/browser/blob/blob_data_handle.h"
 #include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/blob/blob_url_request_job_factory.h"
 #include "storage/browser/blob/shareable_file_reference.h"
 #include "storage/browser/fileapi/file_permission_policy.h"
 #include "storage/browser/fileapi/file_system_context.h"
+#include "third_party/WebKit/public/platform/WebInsecureRequestPolicy.h"
+#include "ui/base/page_transition_types.h"
 #include "url/url_constants.h"
 
 using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 using storage::ShareableFileReference;
 
 // ----------------------------------------------------------------------------
 
 namespace content {
@@ skipping 1330 matching lines @@
   // that was already fetched by the browser will not be transmitted to the
   // renderer.
   if (is_navigation_stream_request)
     new_request->set_method("GET");
   else
     new_request->set_method(request_data.method);
 
   new_request->set_first_party_for_cookies(
       request_data.first_party_for_cookies);
   new_request->set_initiator(request_data.request_initiator);
+  if (request_data.insecure_request_policy & blink::kUpgradeInsecureRequests) {
+    new_request->set_insecure_request_policy(
+        net::URLRequest::UPGRADE_ALL_INSECURE_REQUESTS);

[mmenke, 2016/06/09 18:56:55]

Does this have to be a member of the URLRequest?.  It seems like there are at
least two other options here:

1)  Make it a member of the lovely struct ResourceRequestInfoImpl (Which can be
grabbed from a URLRequest).
2)  URLRequest implements SupportsUserData, so could set it that way.

We could add it to URLRequest::GetHSTSRedirect, as you suggest.  My concerns
there are the scope of URLRequest's API (Which is constantly growing, and
eventually may grow to encompass the entire Chrome code base), in addition to
whether or not it belongs there.

On the other hand, set up here is more than a little obscure, since we have to
go through StoragePartition, which isn't great.

I think either of those options are better than the other browser-based options
(Modifying RedirectInfo, for the API/correctness reasons already mentioning, or
do what extensions do - which is much along the same lines as modifying
RedirectInfo, I believe - though they do it from the NetworkDelegate interface
instead).

[Mike West, 2016/06/10 09:55:16]

This seems like the best approach if you want to keep it out of URLRequest.
If we have to resort to SupportsUserData, I feel like we're just admitting that
we'd rather be lazy than design a reasonable solution. :) I mean, I _am_ lazy,
but I'd like not to admit it so publicly!
I think there's a case to be made for stuffing it into URLRequest, however, as
it's really triggering the same behavior as `URLRequest::GetHSTSRedirect`. It
would be nice to merge those together, and pulling HSTS out of URLRequest seems
like the wrong way to do it.

I agree that it's increasing the scope of URLRequest, but since it's not
actually adding a new feature, just giving the caller another way of triggering
existing behavior, it seems like a pretty minor expansion.
Right. If you hadn't explicitly suggested looking at `URLRequestInterceptor`, I
don't think I'd have found this mechanism. That's not fatal, of course, but it
is something to consider.
I defer to you on this; you have a much better picture of what you'd like the
API to be than I do.

+  }
 
   if (request_data.originated_from_service_worker) {
     new_request->SetUserData(URLRequestServiceWorkerData::kUserDataKey,
                              new URLRequestServiceWorkerData());
   }
 
   // If the request is a MAIN_FRAME request, the first-party URL gets updated on
   // redirects.
   if (request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME) {

[mmenke, 2016/06/14 18:49:31]

BUG:  Need to duplicate main frame request logic from BeginNavigationRequest
here.  PlzNavigate goes through the other path, but other main frame requests
still currently go through here, I believe.

     new_request->set_first_party_url_policy(
         net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
   }
 
   const Referrer referrer(request_data.referrer, request_data.referrer_policy);
   SetReferrerForRequest(new_request.get(), referrer);
 
   new_request->SetExtraRequestHeaders(headers);
 
   storage::BlobStorageContext* blob_context =
@@ skipping 723 matching lines @@
       url_buf, info.common_params.url.spec().c_str(), arraysize(url_buf));
   base::debug::Alias(url_buf);
   CHECK(ContainsKey(active_resource_contexts_, resource_context));
 
   const net::URLRequestContext* request_context =
       resource_context->GetRequestContext();
 
   int load_flags = info.begin_params.load_flags;
   load_flags |= net::LOAD_VERIFY_EV_CERT;
   if (info.is_main_frame)
     load_flags |= net::LOAD_MAIN_FRAME;

[mmenke, 2016/06/14 18:49:31]

Looks like we can we go through this for subframes, too?  I'm not up on all the
navigation logic, but if so, need to handle the subframe navigation case.

 
   // TODO(davidben): BuildLoadFlagsForRequest includes logic for
   // CanSendCookiesForOrigin and CanReadRawCookies. Is this needed here?
 
   // Sync loads should have maximum priority and should be the only
   // requests that have the ignore limits flag set.
   DCHECK(!(load_flags & net::LOAD_IGNORE_LIMITS));
 
   std::unique_ptr<net::URLRequest> new_request;
   new_request = request_context->CreateRequest(
       info.common_params.url, net::HIGHEST, nullptr);
 
   new_request->set_method(info.common_params.method);
   new_request->set_first_party_for_cookies(
       info.first_party_for_cookies);
   new_request->set_initiator(info.request_initiator);
+  if (info.should_upgrade_to_secure_transport) {
+    net::URLRequest::InsecureRequestPolicy policy =
+        net::URLRequest::UPGRADE_ALL_INSECURE_REQUESTS;
+    if (info.is_main_frame &&
+        info.common_params.transition !=
+            ui::PageTransition::PAGE_TRANSITION_FORM_SUBMIT) {
+      policy = net::URLRequest::UPGRADE_SAME_HOST_INSECURE_REQUESTS;
+    }
+    new_request->set_insecure_request_policy(policy);
+  }

[mmenke, 2016/06/14 18:49:31]

I'm not following how the "UPGRADE_SAME_HOST_INSECURE_REQUESTS" logic.  There's
mention of an "upgrade insecure navigations set", but I'm not following a word
of the description for how the "insecure navigations set" is created, and have
no idea how all that maps to "UPGRADE_SAME_HOST_INSECURE_REQUESTS".

   if (info.is_main_frame) {
     new_request->set_first_party_url_policy(
         net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
   }
 
   SetReferrerForRequest(new_request.get(), info.common_params.referrer);
 
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(info.begin_params.headers);
   new_request->SetExtraRequestHeaders(headers);
@@ skipping 442 matching lines @@
   ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(), child_id);
   response->head.security_info = SerializeSecurityInfo(ssl);
 }
 
 CertStore* ResourceDispatcherHostImpl::GetCertStore() {
   return cert_store_for_testing_ ? cert_store_for_testing_
                                  : CertStore::GetInstance();
 }
 
 }  // namespace content