Executed HTTPS upgrade before notifying the start of the provisional load.

This makes so that the fetch URL sent to the browser process for
navigations with the start provisional load notification is already the
final, potentially HTTPS upgraded one. This caused some problems with
upcoming changes where the URL stored in the NavigationHandle differed
from the actual URL being navigated to. A new test is also added to confirm
the upgrade is executed as expected.

BUG=618659
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2109633002
Cr-Commit-Position: refs/heads/master@{#404567}

content/browser/frame_host/navigation_handle_impl_browsertest.cc

@@ -21,9 +21,12 @@ namespace content {
 
 namespace {
 
+// Gathers data from the NavigationHandle assigned to navigations that start
+// with the expected URL.
 class NavigationHandleObserver : public WebContentsObserver {
  public:
-  NavigationHandleObserver(WebContents* web_contents, const GURL& expected_url)
+  NavigationHandleObserver(WebContents* web_contents,
+                           const GURL& expected_start_url)
       : WebContentsObserver(web_contents),
         handle_(nullptr),
         has_committed_(false),
@@ -36,10 +39,10 @@ class NavigationHandleObserver : public WebContentsObserver {
         was_redirected_(false),
         frame_tree_node_id_(-1),
         page_transition_(ui::PAGE_TRANSITION_LINK),
-        expected_url_(expected_url) {}
+        expected_start_url_(expected_start_url) {}
 
   void DidStartNavigation(NavigationHandle* navigation_handle) override {
-    if (handle_ || navigation_handle->GetURL() != expected_url_)
+    if (handle_ || navigation_handle->GetURL() != expected_start_url_)
       return;
 
     handle_ = navigation_handle;
@@ -115,12 +118,13 @@ class NavigationHandleObserver : public WebContentsObserver {
   bool was_redirected_;
   int frame_tree_node_id_;
   ui::PageTransition page_transition_;
-  GURL expected_url_;
+  GURL expected_start_url_;
   GURL last_committed_url_;
 };
 
 // A test NavigationThrottle that will return pre-determined checks and run
-// callbacks when the various NavigationThrottle methods are called.
+// callbacks when the various NavigationThrottle methods are called. It is
+// not instantiated directly but through a TestNavigationThrottleInstaller.
 class TestNavigationThrottle : public NavigationThrottle {
  public:
   TestNavigationThrottle(
@@ -169,8 +173,10 @@ class TestNavigationThrottle : public NavigationThrottle {
   base::Closure did_call_will_process_;
 };
 
-// Install a TestNavigationThrottle on all requests and allows waiting for
-// various NavigationThrottle related events.
+// Install a TestNavigationThrottle on all following requests and allows waiting
+// for various NavigationThrottle related events. Waiting works only for the
+// immediately next navigation. New instances are needed to wait for further
+// navigations.
 class TestNavigationThrottleInstaller : public WebContentsObserver {
  public:
   TestNavigationThrottleInstaller(
@@ -270,6 +276,22 @@ class TestNavigationThrottleInstaller : public WebContentsObserver {
   scoped_refptr<MessageLoopRunner> will_process_loop_runner_;
 };
 
+// Records all navigation start URLs from the WebContents.
+class NavigationStartUrlRecorder : public WebContentsObserver {
+ public:
+  NavigationStartUrlRecorder(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  void DidStartNavigation(NavigationHandle* navigation_handle) override {
+    urls_.push_back(navigation_handle->GetURL());
+  }
+
+  const std::vector<GURL>& urls() const { return urls_; }
+
+ private:
+  std::vector<GURL> urls_;
+};
+
 }  // namespace
 
 class NavigationHandleImplBrowserTest : public ContentBrowserTest {
@@ -632,4 +654,67 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, ThrottleDefer) {
             GURL(embedded_test_server()->GetURL("bar.com", "/title2.html")));
 }
 
+// Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
+// URL from the very beginning of the navigation.
+class NavigationHandleImplHttpsUpgradeBrowserTest
+    : public NavigationHandleImplBrowserTest {
+ public:
+  void CheckHttpsUpgradedIframeNavigation(const GURL& start_url,
+                                          const GURL& iframe_secure_url) {
+    ASSERT_TRUE(start_url.SchemeIs(url::kHttpScheme));
+    ASSERT_TRUE(iframe_secure_url.SchemeIs(url::kHttpsScheme));
+
+    NavigationStartUrlRecorder url_recorder(shell()->web_contents());
+    TestNavigationThrottleInstaller installer(
+        shell()->web_contents(), NavigationThrottle::PROCEED,
+        NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+    TestNavigationManager navigation_manager(shell()->web_contents(),
+                                             iframe_secure_url);
+
+    // Load the page and wait for the frame load with the expected URL.
+    // Note: if the test times out while waiting then a navigation to
+    // iframe_secure_url never happened and the expected upgrade may not be
+    // working.
+    shell()->LoadURL(start_url);
+    navigation_manager.WaitForWillStartRequest();
+
+    // The main frame should have finished navigating while the iframe should
+    // have just started.
+    EXPECT_EQ(2, installer.will_start_called());
+    EXPECT_EQ(0, installer.will_redirect_called());
+    EXPECT_EQ(1, installer.will_process_called());
+
+    // Check the correct start URLs have been registered.
+    EXPECT_EQ(iframe_secure_url, url_recorder.urls().back());
+    EXPECT_EQ(start_url, url_recorder.urls().front());
+    EXPECT_EQ(2ul, url_recorder.urls().size());
+  }
+};
+
+// Tests that the start URL is HTTPS upgraded for a same site navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplHttpsUpgradeBrowserTest,
+                       StartUrlIsHttpsUpgradedSameSite) {
+  GURL start_url(
+      embedded_test_server()->GetURL("/https_upgrade_same_site.html"));
+
+  // Builds the expected upgraded same site URL.
+  GURL::Replacements replace_scheme;
+  replace_scheme.SetSchemeStr("https");
+  GURL cross_site_iframe_secure_url = embedded_test_server()
+                                          ->GetURL("/title1.html")
+                                          .ReplaceComponents(replace_scheme);
+
+  CheckHttpsUpgradedIframeNavigation(start_url, cross_site_iframe_secure_url);
+}
+
+// Tests that the start URL is HTTPS upgraded for a cross site navigation.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplHttpsUpgradeBrowserTest,
+                       StartUrlIsHttpsUpgradedCrossSite) {
+  GURL start_url(
+      embedded_test_server()->GetURL("/https_upgrade_cross_site.html"));
+  GURL cross_site_iframe_secure_url("https://other.com/title1.html");
+
+  CheckHttpsUpgradedIframeNavigation(start_url, cross_site_iframe_secure_url);
+}
+
 }  // namespace content

content/test/data/https_upgrade_cross_site.html

+<!DOCTYPE html>
+<html>
+<title>Cross Site HTTPS upgrade</title>
+
+<meta http-equiv="Content-Security-Policy" content="upgrade-insecure-requests">
+
+<body>
+  <iframe src="http://other.com/title1.html"></iframe>
+</body>
+</html>

content/test/data/https_upgrade_same_site.html

+<!DOCTYPE html>
+<html>
+<title>Same Site HTTPS upgrade</title>
+
+<meta http-equiv="Content-Security-Policy" content="upgrade-insecure-requests">
+
+<body>
+  <iframe src="/title1.html"></iframe>
+</body>
+</html>

testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter

@@ -2,6 +2,7 @@
 -IFrameZoomBrowserTest.RedirectToPageWithSubframeZoomsCorrectly
 -IFrameZoomBrowserTest.SubframesDontZoomIndependently
 -NavigationControllerBrowserTest.BackTwiceToIframeWithContent
+-NavigationHandleImplHttpsUpgradeBrowserTest.*
 -RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
 -RequestDataResourceDispatcherHostBrowserTest.*
 -ServiceWorker*

third_party/WebKit/Source/core/fetch/FetchContext.cpp

@@ -109,7 +109,7 @@ void FetchContext::addConsoleMessage(const String&) const
 {
 }
 
-void FetchContext::upgradeInsecureRequest(FetchRequest&)
+void FetchContext::upgradeInsecureRequest(ResourceRequest&)
 {
 }
 

third_party/WebKit/Source/core/fetch/FetchContext.h

@@ -104,7 +104,7 @@ public:
     virtual void sendImagePing(const KURL&);
     virtual void addConsoleMessage(const String&) const;
     virtual SecurityOrigin* getSecurityOrigin() const { return nullptr; }
-    virtual void upgradeInsecureRequest(FetchRequest&);
+    virtual void upgradeInsecureRequest(ResourceRequest&);
     virtual void addClientHintsIfNecessary(FetchRequest&);
     virtual void addCSPHeaderIfNecessary(Resource::Type, FetchRequest&);
 

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

@@ -410,7 +410,8 @@ Resource* ResourceFetcher::requestResource(FetchRequest& request, const Resource
 {
     ASSERT(request.options().synchronousPolicy == RequestAsynchronously || factory.type() == Resource::Raw || factory.type() == Resource::XSLStyleSheet);
 
-    context().upgradeInsecureRequest(request);
+    if (request.resourceRequest().httpHeaderField("Upgrade-Insecure-Requests") != AtomicString("1"))
+        context().upgradeInsecureRequest(request.mutableResourceRequest());
     context().addClientHintsIfNecessary(request);
     context().addCSPHeaderIfNecessary(factory.type(), request);
 

third_party/WebKit/Source/core/loader/DocumentLoader.cpp

@@ -624,6 +624,11 @@ bool DocumentLoader::maybeLoadEmpty()
     return true;
 }
 
+void DocumentLoader::upgradeInsecureRequest()
+{
+    fetcher()->context().upgradeInsecureRequest(m_request);
+}
+
 void DocumentLoader::startLoadingMainResource()
 {
     timing().markNavigationStart();

third_party/WebKit/Source/core/loader/DocumentLoader.h

@@ -113,6 +113,8 @@ public:
     NavigationType getNavigationType() const { return m_navigationType; }
     void setNavigationType(NavigationType navigationType) { m_navigationType = navigationType; }
 
+    void upgradeInsecureRequest();
+
     void startLoadingMainResource();
 
     void acceptDataFromThreadedReceiver(const char* data, int dataLength, int encodedDataLength);

third_party/WebKit/Source/core/loader/FrameFetchContext.cpp

@@ -676,14 +676,20 @@ SecurityOrigin* FrameFetchContext::getSecurityOrigin() const
     return m_document ? m_document->getSecurityOrigin() : nullptr;
 }
 
-void FrameFetchContext::upgradeInsecureRequest(FetchRequest& fetchRequest)
+void FrameFetchContext::upgradeInsecureRequest(ResourceRequest& resourceRequest)
 {
-    KURL url = fetchRequest.resourceRequest().url();
-
     // Tack an 'Upgrade-Insecure-Requests' header to outgoing navigational requests, as described in
     // https://w3c.github.io/webappsec/specs/upgrade/#feature-detect
-    if (fetchRequest.resourceRequest().frameType() != WebURLRequest::FrameTypeNone)
-        fetchRequest.mutableResourceRequest().addHTTPHeaderField("Upgrade-Insecure-Requests", "1");
+    if (resourceRequest.frameType() != WebURLRequest::FrameTypeNone) {
+
+        // Early return if the request has already been upgraded.
+        if (resourceRequest.httpHeaderField("Upgrade-Insecure-Requests") == AtomicString("1"))
+            return;
+
+        resourceRequest.addHTTPHeaderField("Upgrade-Insecure-Requests", "1");
+    }
+
+    KURL url = resourceRequest.url();
 
     // If we don't yet have an |m_document| (because we're loading an iframe, for instance), check the FrameLoader's policy.
     WebInsecureRequestPolicy relevantPolicy = m_document ? m_document->getInsecureRequestPolicy() : frame()->loader().getInsecureRequestPolicy();
@@ -695,17 +701,15 @@ void FrameFetchContext::upgradeInsecureRequest(FetchRequest& fetchRequest)
         // 1. Are for subresources (including nested frames).
         // 2. Are form submissions.
         // 3. Whose hosts are contained in the document's InsecureNavigationSet.
-        const ResourceRequest& request = fetchRequest.resourceRequest();
-        if (request.frameType() == WebURLRequest::FrameTypeNone
-            || request.frameType() == WebURLRequest::FrameTypeNested
-            || request.requestContext() == WebURLRequest::RequestContextForm
-            || (!url.host().isNull() && relevantNavigationSet->contains(url.host().impl()->hash())))
-        {
+        if (resourceRequest.frameType() == WebURLRequest::FrameTypeNone
+            || resourceRequest.frameType() == WebURLRequest::FrameTypeNested
+            || resourceRequest.requestContext() == WebURLRequest::RequestContextForm
+            || (!url.host().isNull() && relevantNavigationSet->contains(url.host().impl()->hash()))) {
             UseCounter::count(m_document, UseCounter::UpgradeInsecureRequestsUpgradedRequest);
             url.setProtocol("https");
             if (url.port() == 80)
                 url.setPort(443);
-            fetchRequest.mutableResourceRequest().setURL(url);
+            resourceRequest.setURL(url);
         }
     }
 }

third_party/WebKit/Source/core/loader/FrameFetchContext.h

@@ -99,7 +99,7 @@ public:
     void sendImagePing(const KURL&) override;
     void addConsoleMessage(const String&) const override;
     SecurityOrigin* getSecurityOrigin() const override;
-    void upgradeInsecureRequest(FetchRequest&) override;
+    void upgradeInsecureRequest(ResourceRequest&) override;
     void addClientHintsIfNecessary(FetchRequest&) override;
     void addCSPHeaderIfNecessary(Resource::Type, FetchRequest&) override;
 

third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp

@@ -186,7 +186,7 @@ protected:
         fetchRequest.mutableResourceRequest().setRequestContext(requestContext);
         fetchRequest.mutableResourceRequest().setFrameType(frameType);
 
-        fetchContext->upgradeInsecureRequest(fetchRequest);
+        fetchContext->upgradeInsecureRequest(fetchRequest.mutableResourceRequest());
 
         EXPECT_STREQ(expectedURL.getString().utf8().data(), fetchRequest.resourceRequest().url().getString().utf8().data());
         EXPECT_EQ(expectedURL.protocol(), fetchRequest.resourceRequest().url().protocol());
@@ -204,10 +204,16 @@ protected:
         fetchRequest.mutableResourceRequest().setRequestContext(WebURLRequest::RequestContextScript);
         fetchRequest.mutableResourceRequest().setFrameType(frameType);
 
-        fetchContext->upgradeInsecureRequest(fetchRequest);
+        fetchContext->upgradeInsecureRequest(fetchRequest.mutableResourceRequest());
 
         EXPECT_STREQ(shouldPrefer ? "1" : "",
             fetchRequest.resourceRequest().httpHeaderField(HTTPNames::Upgrade_Insecure_Requests).utf8().data());
+
+        // Calling upgradeInsecureRequest more than once shouldn't affect the header.
+        if (shouldPrefer) {
+            fetchContext->upgradeInsecureRequest(fetchRequest.mutableResourceRequest());
+            EXPECT_STREQ("1", fetchRequest.resourceRequest().httpHeaderField(HTTPNames::Upgrade_Insecure_Requests).utf8().data());
+        }
     }
 
     RefPtr<SecurityOrigin> exampleOrigin;

third_party/WebKit/Source/core/loader/FrameLoader.cpp

@@ -1451,6 +1451,7 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest, FrameLoadType ty
     if (m_provisionalDocumentLoader->isClientRedirect())
         m_provisionalDocumentLoader->appendRedirect(m_frame->document()->url());
     m_provisionalDocumentLoader->appendRedirect(m_provisionalDocumentLoader->request().url());
+    m_provisionalDocumentLoader->upgradeInsecureRequest();
     double triggeringEventTime = frameLoadRequest.triggeringEvent() ? frameLoadRequest.triggeringEvent()->platformTimeStamp() : 0;
     client()->dispatchDidStartProvisionalLoad(triggeringEventTime);
     ASSERT(m_provisionalDocumentLoader);