Disable client hints on insecure contexts.

This only affects the client hints that are requested by
origins using "Accept-CH" header.

Before this change, origins can request client hints
using the main frame response. Chrome would then attach
the requested client hints on either HTTP or
HTTPS subresources. With this change, the client hints
would be attached on HTTPS subresources only.

This is guarded behind a WebRuntimeFeature which will
be enabled after external communication on blink-dev.

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/852863.

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

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

@@ -528,11 +528,14 @@ 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);
-
+    }
     // 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;
@@ -857,17 +860,22 @@ void FrameFetchContext::AddClientHintsIfNecessary(
     const FetchParameters::ResourceWidth& resource_width,
     ResourceRequest& request) {
   WebEnabledClientHints enabled_hints;
-  // Check if |url| is allowed to run JavaScript. If not, client hints are not
-  // attached to the requests that initiate on the render side.
-  if (blink::RuntimeEnabledFeatures::ClientHintsPersistentEnabled() &&
-      IsClientHintsAllowed(request.Url()) && GetContentSettingsClient() &&
-      AllowScriptFromSource(request.Url())) {
-    // TODO(tbansal): crbug.com/735518 This code path is not executed for main
-    // frame navigations when browser side navigation is enabled. For main
-    // frame requests with browser side navigation enabled, the client hints
-    // should be attached by the browser process.
-    GetContentSettingsClient()->GetAllowedClientHintsFromSource(request.Url(),
-                                                                &enabled_hints);
+  if (blink::RuntimeEnabledFeatures::ClientHintsPersistentEnabled()) {
+    // If the feature is enabled, then client hints are allowed only on secure
+    // URLs.
+    if (!IsClientHintsAllowed(request.Url()))
+      return;
+
+    // Check if |url| is allowed to run JavaScript. If not, client hints are not
+    // attached to the requests that initiate on the render side.
+    if (GetContentSettingsClient() && AllowScriptFromSource(request.Url())) {
+      // TODO(tbansal): crbug.com/735518 This code path is not executed for main
+      // frame navigations when browser side navigation is enabled. For main
+      // frame requests with browser side navigation enabled, the client hints
+      // should be attached by the browser process.
+      GetContentSettingsClient()->GetAllowedClientHintsFromSource(
+          request.Url(), &enabled_hints);
+    }
   }
 
   if (ShouldSendClientHint(mojom::WebClientHintsType::kDeviceMemory,

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

@@ -63,6 +63,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/common/device_memory/approximated_device_memory.h"
+#include "third_party/WebKit/public/platform/WebRuntimeFeatures.h"
 
 namespace blink {
 
@@ -559,15 +560,36 @@ TEST_F(FrameFetchContextHintsTest, MonitorDeviceMemorySecureTransport) {
 // Verify that client hints are not attched when the resources do not belong to
 // a secure context.
 TEST_F(FrameFetchContextHintsTest, MonitorDeviceMemoryHintsInsecureContext) {
+  WebRuntimeFeatures::EnableClientHintsPersistent(false);
   ExpectHeader("http://www.example.com/1.gif", "Device-Memory", false, "");
-  ClientHintsPreferences preferences;
-  preferences.SetShouldSendForTesting(mojom::WebClientHintsType::kDeviceMemory);
-  document->GetClientHintsPreferences().UpdateFrom(preferences);
-  ApproximatedDeviceMemory::SetPhysicalMemoryMBForTesting(4096);
-  ExpectHeader("http://www.example.com/1.gif", "Device-Memory", true, "4");
-  ExpectHeader("http://www.example.com/1.gif", "DPR", false, "");
-  ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
-  ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+
+  {
+    ClientHintsPreferences preferences;
+    preferences.SetShouldSendForTesting(
+        mojom::WebClientHintsType::kDeviceMemory);
+    document->GetClientHintsPreferences().UpdateFrom(preferences);
+    ApproximatedDeviceMemory::SetPhysicalMemoryMBForTesting(4096);
+    ExpectHeader("http://www.example.com/1.gif", "Device-Memory", true, "4");
+    ExpectHeader("http://www.example.com/1.gif", "DPR", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+  }
+
+  {
+    // Verify that client hints are not attched when the resources do not belong
+    // to a secure context and the persistent client hint features is enabled.
+    WebRuntimeFeatures::EnableClientHintsPersistent(true);
+    ExpectHeader("http://www.example.com/1.gif", "Device-Memory", false, "");
+    ClientHintsPreferences preferences;
+    preferences.SetShouldSendForTesting(
+        mojom::WebClientHintsType::kDeviceMemory);
+    document->GetClientHintsPreferences().UpdateFrom(preferences);
+    ApproximatedDeviceMemory::SetPhysicalMemoryMBForTesting(4096);
+    ExpectHeader("http://www.example.com/1.gif", "Device-Memory", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "DPR", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+  }
 }
 
 // Verify that client hints are attched when the resources belong to a local
@@ -615,15 +637,28 @@ TEST_F(FrameFetchContextHintsTest, MonitorDPRHints) {
 }
 
 TEST_F(FrameFetchContextHintsTest, MonitorDPRHintsInsecureTransport) {
+  WebRuntimeFeatures::EnableClientHintsPersistent(false);
   ExpectHeader("http://www.example.com/1.gif", "DPR", false, "");
-  ClientHintsPreferences preferences;
-  preferences.SetShouldSendForTesting(mojom::WebClientHintsType::kDpr);
-  document->GetClientHintsPreferences().UpdateFrom(preferences);
-  ExpectHeader("http://www.example.com/1.gif", "DPR", true, "1");
-  dummy_page_holder->GetPage().SetDeviceScaleFactorDeprecated(2.5);
-  ExpectHeader("http://www.example.com/1.gif", "DPR", true, "2.5");
-  ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
-  ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+
+  {
+    ClientHintsPreferences preferences;
+    preferences.SetShouldSendForTesting(mojom::WebClientHintsType::kDpr);
+    document->GetClientHintsPreferences().UpdateFrom(preferences);
+    ExpectHeader("http://www.example.com/1.gif", "DPR", true, "1");
+    dummy_page_holder->GetPage().SetDeviceScaleFactorDeprecated(2.5);
+    ExpectHeader("http://www.example.com/1.gif", "DPR", true, "2.5");
+    ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+  }
+
+  {
+    WebRuntimeFeatures::EnableClientHintsPersistent(true);
+    ExpectHeader("http://www.example.com/1.gif", "DPR", false, "");
+    dummy_page_holder->GetPage().SetDeviceScaleFactorDeprecated(2.5);
+    ExpectHeader("http://www.example.com/1.gif", "DPR", false, "  ");
+    ExpectHeader("http://www.example.com/1.gif", "Width", false, "");
+    ExpectHeader("http://www.example.com/1.gif", "Viewport-Width", false, "");
+  }
 }
 
 TEST_F(FrameFetchContextHintsTest, MonitorResourceWidthHints) {