Honor ACL header from 1p origin in iframes navigation response

Currently, Chromium disregards Accept-CH-Lifetime header on iframe
navigation responses. This CL changes Chromium behavior to
accept ACL header on iframe navigation responses provided the origin
of the response matches the origin of the main frame navigation.

This behavior is compliant with the updated client hints spec.

WPT tests will come in a subsequent CL.

Change-Id: I5603cad33f77d8e413b402b8a6c184497d87aec6
Bug: 856700
Reviewed-on: https://chromium-review.googlesource.com/1117859
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571438}

chrome/browser/client_hints/client_hints_browsertest.cc

@@ -132,6 +132,7 @@ class ClientHintsBrowserTest : public InProcessBrowserTest {
   ClientHintsBrowserTest()
       : http_server_(net::EmbeddedTestServer::TYPE_HTTP),
         https_server_(net::EmbeddedTestServer::TYPE_HTTPS),
+        https_cross_origin_server_(net::EmbeddedTestServer::TYPE_HTTPS),
         expect_client_hints_on_main_frame_(false),
         expect_client_hints_on_subresources_(false),
         count_client_hints_headers_seen_(0),
@@ -139,16 +140,27 @@ class ClientHintsBrowserTest : public InProcessBrowserTest {
     http_server_.ServeFilesFromSourceDirectory("chrome/test/data/client_hints");
     https_server_.ServeFilesFromSourceDirectory(
         "chrome/test/data/client_hints");
+    https_cross_origin_server_.ServeFilesFromSourceDirectory(
+        "chrome/test/data/client_hints");
 
     http_server_.RegisterRequestMonitor(
-        base::Bind(&ClientHintsBrowserTest::MonitorResourceRequest,
-                   base::Unretained(this)));
+        base::BindRepeating(&ClientHintsBrowserTest::MonitorResourceRequest,
+                            base::Unretained(this)));
     https_server_.RegisterRequestMonitor(
-        base::Bind(&ClientHintsBrowserTest::MonitorResourceRequest,
-                   base::Unretained(this)));
+        base::BindRepeating(&ClientHintsBrowserTest::MonitorResourceRequest,
+                            base::Unretained(this)));
+    https_cross_origin_server_.RegisterRequestMonitor(
+        base::BindRepeating(&ClientHintsBrowserTest::MonitorResourceRequest,
+                            base::Unretained(this)));
+    https_server_.RegisterRequestHandler(base::BindRepeating(
+        &ClientHintsBrowserTest::RequestHandlerToFetchCrossOriginIframe,
+        base::Unretained(this)));
 
     EXPECT_TRUE(http_server_.Start());
     EXPECT_TRUE(https_server_.Start());
+    EXPECT_TRUE(https_cross_origin_server_.Start());
+
+    EXPECT_NE(https_server_.base_url(), https_cross_origin_server_.base_url());
 
     accept_ch_with_lifetime_http_local_url_ =
         http_server_.GetURL("/accept_ch_with_lifetime.html");
@@ -314,7 +326,46 @@ class ClientHintsBrowserTest : public InProcessBrowserTest {
 
   base::test::ScopedFeatureList scoped_feature_list_;
 
+  std::string intercept_iframe_resource_;
+
  private:
+  // Intercepts only the main frame requests that contain
+  // |intercept_iframe_resource_| in the resource path. The intercepted requests
+  // are served an HTML file that fetches an iframe from a cross-origin HTTPS
+  // server.
+  std::unique_ptr<net::test_server::HttpResponse>
+  RequestHandlerToFetchCrossOriginIframe(
+      const net::test_server::HttpRequest& request) {
+    if (intercept_iframe_resource_.empty())
+      return nullptr;
+
+    // Check if it's a main frame request.
+    if (request.relative_url.find(".html") == std::string::npos)
+      return nullptr;
+
+    if (request.relative_url.find(intercept_iframe_resource_) ==
+        std::string::npos) {
+      return nullptr;
+    }
+
+    const std::string iframe_url =
+        https_cross_origin_server_.GetURL("/accept_ch_with_lifetime.html")
+            .spec();
+
+    std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
+        new net::test_server::BasicHttpResponse());
+    http_response->set_code(net::HTTP_OK);
+    http_response->set_content_type("text/html");
+    http_response->set_content(
+        "<html>"
+        "<link rel='icon' href='data:;base64,='><head></head>"
+        "Empty file which uses link-rel to disable favicon fetches. "
+        "<iframe src='" +
+        iframe_url + "'></iframe></html>");
+
+    return std::move(http_response);
+  }
+
   // Called by |https_server_|.
   void MonitorResourceRequest(const net::test_server::HttpRequest& request) {
     bool is_main_frame_navigation =
@@ -461,6 +512,7 @@ class ClientHintsBrowserTest : public InProcessBrowserTest {
 
   net::EmbeddedTestServer http_server_;
   net::EmbeddedTestServer https_server_;
+  net::EmbeddedTestServer https_cross_origin_server_;
   GURL accept_ch_with_lifetime_http_local_url_;
   GURL accept_ch_with_lifetime_url_;
   GURL accept_ch_without_lifetime_url_;
@@ -653,11 +705,11 @@ IN_PROC_BROWSER_TEST_F(ClientHintsBrowserTest,
 }
 
 // Loads a HTTPS webpage that does not request persisting of client hints.
-// An iframe loaded by the webpage requests persistence of client hints.
-// Verify that the request from the iframe is not honored, and client hints
-// preference is not persisted.
+// A same-origin iframe loaded by the webpage requests persistence of client
+// hints. Verify that the request from the iframe is honored, and client hints
+// preference is persisted.
 IN_PROC_BROWSER_TEST_F(ClientHintsBrowserTest,
-                       DisregardPersistenceRequestIframe) {
+                       PersistenceRequestIframe_SameOrigin) {
   base::HistogramTester histogram_tester;
   ContentSettingsForOneType host_settings;
 
@@ -669,14 +721,47 @@ IN_PROC_BROWSER_TEST_F(ClientHintsBrowserTest,
   ui_test_utils::NavigateToURL(browser(),
                                accept_ch_without_lifetime_with_iframe_url());
 
-  histogram_tester.ExpectTotalCount("ClientHints.UpdateEventCount", 0);
+  histogram_tester.ExpectTotalCount("ClientHints.UpdateEventCount", 1);
 
   content::FetchHistogramsFromChildProcesses();
   SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
 
   // accept_ch_without_lifetime_with_iframe_url() loads
   // accept_ch_with_lifetime() in an iframe. The request to persist client
-  // hints from accept_ch_with_lifetime() should be disregarded.
+  // hints from accept_ch_with_lifetime() should be persisted.
+  histogram_tester.ExpectTotalCount("ClientHints.UpdateSize", 1);
+  histogram_tester.ExpectUniqueSample("ClientHints.PersistDuration",
+                                      3600 * 1000, 1);
+}
+
+// Loads a HTTPS webpage that does not request persisting of client hints.
+// An iframe loaded by the webpage from an cross origin server requests
+// persistence of client hints.
+// Verify that the request from the cross origin iframe is not honored, and
+// client hints preference is not persisted.
+IN_PROC_BROWSER_TEST_F(ClientHintsBrowserTest,
+                       DisregardPersistenceRequestIframe_CrossOrigin) {
+  intercept_iframe_resource_ =
+      accept_ch_without_lifetime_with_iframe_url().path();
+  base::HistogramTester histogram_tester;
+  ContentSettingsForOneType host_settings;
+
+  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
+      ->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_CLIENT_HINTS, std::string(),
+                              &host_settings);
+  EXPECT_EQ(0u, host_settings.size());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               accept_ch_without_lifetime_with_iframe_url());
+
+  histogram_tester.ExpectTotalCount("ClientHints.UpdateEventCount", 0);
+
+  content::FetchHistogramsFromChildProcesses();
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+
+  // accept_ch_without_lifetime_with_iframe_url() loads
+  // accept_ch_with_lifetime() in a cross origin iframe. The request to persist
+  // client hints from accept_ch_with_lifetime() should be disregarded.
   histogram_tester.ExpectTotalCount("ClientHints.UpdateSize", 0);
   histogram_tester.ExpectTotalCount("ClientHints.PersistDuration", 0);
 }

third_party/blink/renderer/core/loader/frame_fetch_context.cc

@@ -590,12 +590,12 @@ void FrameFetchContext::DispatchDidReceiveResponse(
   if (frame_url == NullURL())
     frame_url = document_loader_->Url();
 
-  // Check if |response| belongs to a resource in the main frame, and if belongs
-  // to the same origin as frame top request. Also, the accept-ch-lifetime
-  // header is honored only on the navigation responses.
-  if (SecurityOrigin::AreSameSchemeHostPort(response.Url(), frame_url) &&
-      GetFrame()->IsMainFrame() &&
-      (resource->GetType() == Resource::kMainResource)) {
+  // The accept-ch-lifetime header is honored only on the navigation responses.
+  // Further, the navigation response should be from a top level frame (i.e.,
+  // main frame) or the origin of the response should match the origin of the
+  // top level frame.
+  if ((resource->GetType() == Resource::kMainResource) &&
+      (IsMainFrame() || IsFirstPartyOrigin(response.Url()))) {
     ParseAndPersistClientHints(response);
   }
 
@@ -900,8 +900,6 @@ void FrameFetchContext::AddClientHintsIfNecessary(
     ResourceRequest& request) {
   WebEnabledClientHints enabled_hints;
 
-  bool is_1p_origin = false;
-
   // If the feature is enabled, then client hints are allowed only on secure
   // URLs.
   if (!ClientHintsPreferences::IsClientHintsAllowed(request.Url()))
@@ -912,16 +910,7 @@ void FrameFetchContext::AddClientHintsIfNecessary(
   if (!AllowScriptFromSourceWithoutNotifying(request.Url()))
     return;
 
-  if (IsDetached())
-    return;
-
-  is_1p_origin =
-      GetFrame()
-          ->Tree()
-          .Top()
-          .GetSecurityContext()
-          ->GetSecurityOrigin()
-          ->IsSameSchemeHostPort(SecurityOrigin::Create(request.Url()).get());
+  bool is_1p_origin = IsFirstPartyOrigin(request.Url());
 
   if (!base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) &&
       !is_1p_origin) {
@@ -1075,6 +1064,18 @@ bool FrameFetchContext::AllowScriptFromSourceWithoutNotifying(
   return true;
 }
 
+bool FrameFetchContext::IsFirstPartyOrigin(const KURL& url) const {
+  if (IsDetached())
+    return false;
+
+  return GetFrame()
+      ->Tree()
+      .Top()
+      .GetSecurityContext()
+      ->GetSecurityOrigin()
+      ->IsSameSchemeHostPort(SecurityOrigin::Create(url).get());
+}
+
 bool FrameFetchContext::ShouldBlockRequestByInspector(const KURL& url) const {
   if (IsDetached())
     return false;

third_party/blink/renderer/core/loader/frame_fetch_context.h

@@ -261,6 +261,10 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
   // JavaScript was blocked from being executed.
   bool AllowScriptFromSourceWithoutNotifying(const KURL&) const;
 
+  // Returns true if the origin of |url| is same as the origin of the top level
+  // frame's main resource.
+  bool IsFirstPartyOrigin(const KURL& url) const;
+
   Member<DocumentLoader> document_loader_;
   Member<Document> document_;