Provide client hints only on secure contexts.

Update the http-equiv and preload scanner code path as well. The client
hints are provided only on secure contexts if
ClientHintsPersistentEnabled() feature is enabled.

Bug: 782381
Change-Id: I28d75b6bfd14f36accdba8f87488ae687df049d7
Reviewed-on: https://chromium-review.googlesource.com/887924
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533109}

third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp

@@ -789,9 +789,10 @@ void TokenPreloadScanner::ScanCommon(const Token& token,
                                                  "accept-ch")) {
             const typename Token::Attribute* content_attribute =
                 token.GetAttributeItem(contentAttr);
-            if (content_attribute)
+            if (content_attribute) {
               client_hints_preferences_.UpdateFromAcceptClientHintsHeader(
-                  content_attribute->Value(), nullptr);
+                  content_attribute->Value(), document_url_, nullptr);
+            }
           }
           return;
         }

third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp

@@ -20,6 +20,7 @@
 #include "public/platform/WebClientHintsType.h"
 #include "public/platform/WebURLLoaderMockFactory.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/WebKit/public/platform/WebRuntimeFeatures.h"
 
 namespace blink {
 
@@ -208,9 +209,12 @@ class HTMLPreloadScannerTest : public PageTestBase {
   void RunSetUp(
       ViewportState viewport_state,
       PreloadState preload_state = kPreloadEnabled,
-      ReferrerPolicy document_referrer_policy = kReferrerPolicyDefault) {
+      ReferrerPolicy document_referrer_policy = kReferrerPolicyDefault,
+      bool use_secure_document_url = false) {
     HTMLParserOptions options(&GetDocument());
-    KURL document_url("http://whatever.test/");
+    KURL document_url = KURL("http://whatever.test/");
+    if (use_secure_document_url)
+      document_url = KURL("https://whatever.test/");
     GetDocument().SetURL(document_url);
     GetDocument().SetSecurityOrigin(SecurityOrigin::Create(document_url));
     GetDocument().GetSettings()->SetViewportEnabled(viewport_state ==
@@ -594,11 +598,58 @@ TEST_F(HTMLPreloadScannerTest, testMetaAcceptCH) {
   };
 
   for (const auto& test_case : test_cases) {
-    RunSetUp(kViewportDisabled);
+    RunSetUp(kViewportDisabled, kPreloadEnabled, kReferrerPolicyDefault,
+             true /* use_secure_document_url */);
     Test(test_case);
   }
 }
 
+TEST_F(HTMLPreloadScannerTest, testMetaAcceptCHInsecureDocument) {
+  ClientHintsPreferences all;
+  all.SetShouldSendForTesting(mojom::WebClientHintsType::kDpr);
+  all.SetShouldSendForTesting(mojom::WebClientHintsType::kResourceWidth);
+  all.SetShouldSendForTesting(mojom::WebClientHintsType::kViewportWidth);
+
+  const PreloadScannerTestCase expect_no_client_hint = {
+      "http://example.test",
+      "<meta http-equiv='accept-ch' content='  viewport-width  ,width, "
+      "wutever, dpr \t'><img sizes='90vw' srcset='bla.gif 320w, blabla.gif "
+      "640w'>",
+      "blabla.gif",
+      "http://example.test/",
+      Resource::kImage,
+      450};
+
+  const PreloadScannerTestCase expect_client_hint = {
+      "http://example.test",
+      "<meta http-equiv='accept-ch' content='  viewport-width  ,width, "
+      "wutever, dpr \t'><img sizes='90vw' srcset='bla.gif 320w, blabla.gif "
+      "640w'>",
+      "blabla.gif",
+      "http://example.test/",
+      Resource::kImage,
+      450,
+      all};
+
+  // For an insecure document, client hint should not be attached.
+  WebRuntimeFeatures::EnableClientHintsPersistent(true);
+  RunSetUp(kViewportDisabled, kPreloadEnabled, kReferrerPolicyDefault,
+           false /* use_secure_document_url */);
+  Test(expect_no_client_hint);
+
+  // For a secure document, client hint should be attached.
+  RunSetUp(kViewportDisabled, kPreloadEnabled, kReferrerPolicyDefault,
+           true /* use_secure_document_url */);
+  Test(expect_client_hint);
+
+  // For an insecure document, client hint should be attached if the persistent
+  // client hints are not enabled.
+  WebRuntimeFeatures::EnableClientHintsPersistent(false);
+  RunSetUp(kViewportDisabled, kPreloadEnabled, kReferrerPolicyDefault,
+           false /* use_secure_document_url */);
+  Test(expect_client_hint);
+}
+
 TEST_F(HTMLPreloadScannerTest, testPreconnect) {
   HTMLPreconnectTestCase test_cases[] = {
       {"http://example.test", "<link rel=preconnect href=http://example2.test>",

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

@@ -178,12 +178,6 @@ mojom::FetchCacheMode DetermineFrameCacheMode(Frame* frame,
                             ResourceType::kIsNotMainResource, load_type);
 }
 
-bool IsClientHintsAllowed(const KURL& url) {
-  return (url.ProtocolIs("http") || url.ProtocolIs("https")) &&
-         (SecurityOrigin::IsSecure(url) ||
-          SecurityOrigin::Create(url)->IsLocalhost());
-}
-
 }  // namespace
 
 struct FrameFetchContext::FrozenState final
@@ -528,14 +522,11 @@ void FrameFetchContext::DispatchDidReceiveResponse(
                               ->Loader()
                               .GetProvisionalDocumentLoader()) {
     FrameClientHintsPreferencesContext hints_context(GetFrame());
-    if (!blink::RuntimeEnabledFeatures::ClientHintsPersistentEnabled() ||
-        IsClientHintsAllowed(response.Url())) {
-      // If the persistent client hint feature is enabled, then client hints
-      // should be allowed only on secure URLs.
-      document_loader_->GetClientHintsPreferences()
-          .UpdateFromAcceptClientHintsHeader(
-              response.HttpHeaderField(HTTPNames::Accept_CH), &hints_context);
-    }
+    document_loader_->GetClientHintsPreferences()
+        .UpdateFromAcceptClientHintsHeader(
+            response.HttpHeaderField(HTTPNames::Accept_CH), response.Url(),
+            &hints_context);
+
     // When response is received with a provisional docloader, the resource
     // haven't committed yet, and we cannot load resources, only preconnect.
     resource_loading_policy = LinkLoader::kDoNotLoadResources;
@@ -851,7 +842,7 @@ void FrameFetchContext::AddClientHintsIfNecessary(
   if (blink::RuntimeEnabledFeatures::ClientHintsPersistentEnabled()) {
     // If the feature is enabled, then client hints are allowed only on secure
     // URLs.
-    if (!IsClientHintsAllowed(request.Url()))
+    if (!ClientHintsPreferences::IsClientHintsAllowed(request.Url()))
       return;
 
     // Check if |url| is allowed to run JavaScript. If not, client hints are not
@@ -1184,9 +1175,6 @@ bool FrameFetchContext::ShouldSendClientHint(
 
 void FrameFetchContext::ParseAndPersistClientHints(
     const ResourceResponse& response) {
-  if (!IsClientHintsAllowed(response.Url()))
-    return;
-
   ClientHintsPreferences hints_preferences;
   WebEnabledClientHints enabled_client_hints;
   TimeDelta persist_duration;

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

@@ -95,7 +95,7 @@ void HttpEquiv::ProcessHttpEquivAcceptCH(Document& document,
   UseCounter::Count(document, WebFeature::kClientHintsMetaAcceptCH);
   FrameClientHintsPreferencesContext hints_context(document.GetFrame());
   document.GetClientHintsPreferences().UpdateFromAcceptClientHintsHeader(
-      content, &hints_context);
+      content, document.Url(), &hints_context);
 }
 
 void HttpEquiv::ProcessHttpEquivDefaultStyle(Document& document,

third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.cpp

@@ -52,10 +52,18 @@ void ClientHintsPreferences::UpdateFrom(
 
 void ClientHintsPreferences::UpdateFromAcceptClientHintsHeader(
     const String& header_value,
+    const KURL& url,
     Context* context) {
   if (header_value.IsEmpty())
     return;
 
+  // If the persistent client hint feature is enabled, then client hints
+  // should be allowed only on secure URLs.
+  if (blink::RuntimeEnabledFeatures::ClientHintsPersistentEnabled() &&
+      !IsClientHintsAllowed(url)) {
+    return;
+  }
+
   WebEnabledClientHints new_enabled_types;
 
   ParseAcceptChHeader(header_value, new_enabled_types);
@@ -101,6 +109,8 @@ void ClientHintsPreferences::UpdatePersistentHintsFromHeaders(
   }
 
   const KURL url = response.Url();
+  if (!IsClientHintsAllowed(url))
+    return;
 
   bool conversion_ok = false;
   int64_t persist_duration_seconds =
@@ -115,4 +125,11 @@ void ClientHintsPreferences::UpdatePersistentHintsFromHeaders(
   ParseAcceptChHeader(accept_ch_header_value, enabled_hints);
 }
 
+// static
+bool ClientHintsPreferences::IsClientHintsAllowed(const KURL& url) {
+  return (url.ProtocolIs("http") || url.ProtocolIs("https")) &&
+         (SecurityOrigin::IsSecure(url) ||
+          SecurityOrigin::Create(url)->IsLocalhost());
+}
+
 }  // namespace blink

third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferences.h

@@ -13,6 +13,7 @@
 
 namespace blink {
 
+class KURL;
 class ResourceResponse;
 
 // TODO (tbansal): Remove PLATFORM_EXPORT, and pass WebClientHintsType
@@ -35,8 +36,12 @@ class PLATFORM_EXPORT ClientHintsPreferences {
   void UpdateFrom(const ClientHintsPreferences&);
 
   // Parses the client hints headers, and populates |this| with the client hint
-  // preferences. |context| may be null.
-  void UpdateFromAcceptClientHintsHeader(const String& header_value, Context*);
+  // preferences. |url| is the URL of the resource whose response included the
+  // |header_value|. |context| may be null. If client hints are not allowed for
+  // |url|, then |this| would not be updated.
+  void UpdateFromAcceptClientHintsHeader(const String& header_value,
+                                         const KURL&,
+                                         Context*);
 
   bool ShouldSend(mojom::WebClientHintsType type) const {
     return enabled_hints_.IsEnabled(type);
@@ -59,6 +64,10 @@ class PLATFORM_EXPORT ClientHintsPreferences {
       WebEnabledClientHints& enabled_hints,
       TimeDelta* persist_duration);
 
+  // Returns true if client hints are allowed for the provided KURL. Client
+  // hints are allowed only on HTTP URLs that belong to secure contexts.
+  static bool IsClientHintsAllowed(const KURL&);
+
  private:
   WebEnabledClientHints enabled_hints_;
 };

third_party/WebKit/Source/platform/loader/fetch/ClientHintsPreferencesTest.cpp

@@ -15,7 +15,7 @@ namespace blink {
 
 class ClientHintsPreferencesTest : public ::testing::Test {};
 
-TEST_F(ClientHintsPreferencesTest, Basic) {
+TEST_F(ClientHintsPreferencesTest, BasicSecure) {
   struct TestCase {
     const char* header_value;
     bool expectation_resource_width;
@@ -32,7 +32,8 @@ TEST_F(ClientHintsPreferencesTest, Basic) {
 
   for (const auto& test_case : cases) {
     ClientHintsPreferences preferences;
-    preferences.UpdateFromAcceptClientHintsHeader(test_case.header_value,
+    const KURL kurl(String::FromUTF8("https://www.google.com/"));
+    preferences.UpdateFromAcceptClientHintsHeader(test_case.header_value, kurl,
                                                   nullptr);
     EXPECT_EQ(
         test_case.expectation_resource_width,
@@ -45,7 +46,7 @@ TEST_F(ClientHintsPreferencesTest, Basic) {
 
     // Calling UpdateFromAcceptClientHintsHeader with empty header should have
     // no impact on client hint preferences.
-    preferences.UpdateFromAcceptClientHintsHeader("", nullptr);
+    preferences.UpdateFromAcceptClientHintsHeader("", kurl, nullptr);
     EXPECT_EQ(
         test_case.expectation_resource_width,
         preferences.ShouldSend(mojom::WebClientHintsType::kResourceWidth));
@@ -57,7 +58,7 @@ TEST_F(ClientHintsPreferencesTest, Basic) {
 
     // Calling UpdateFromAcceptClientHintsHeader with an invalid header should
     // have no impact on client hint preferences.
-    preferences.UpdateFromAcceptClientHintsHeader("foobar", nullptr);
+    preferences.UpdateFromAcceptClientHintsHeader("foobar", kurl, nullptr);
     EXPECT_EQ(
         test_case.expectation_resource_width,
         preferences.ShouldSend(mojom::WebClientHintsType::kResourceWidth));
@@ -69,6 +70,18 @@ TEST_F(ClientHintsPreferencesTest, Basic) {
   }
 }
 
+TEST_F(ClientHintsPreferencesTest, Insecure) {
+  for (const auto& use_secure_url : {false, true}) {
+    ClientHintsPreferences preferences;
+    const KURL kurl = use_secure_url
+                          ? KURL(String::FromUTF8("https://www.google.com/"))
+                          : KURL(String::FromUTF8("http://www.google.com/"));
+    preferences.UpdateFromAcceptClientHintsHeader("dpr", kurl, nullptr);
+    EXPECT_EQ(use_secure_url,
+              preferences.ShouldSend(mojom::WebClientHintsType::kDpr));
+  }
+}
+
 TEST_F(ClientHintsPreferencesTest, PersistentHints) {
   struct TestCase {
     bool enable_persistent_runtime_feature;