Autoupgrade redirects to HTTPS for mixed content

When the mixed content autoupgrade feature is enabled, content that
would have been autoupgraded but is already HTTPS does not get the
UpgradeIfInsecure flag set, which means if it is eventually redirected
to HTTP, that mixed content is not upgraded. This CL fixes that and
adds a regression test.

Bug: 1058077
Change-Id: I359d720ea5caf0833fabd810de7876e32f17b099
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2086258
Auto-Submit: Carlos IL <carlosil@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747318}

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

@@ -844,11 +844,16 @@ void MixedContentChecker::UpgradeInsecureRequest(
        mojom::blink::InsecureRequestPolicy::kUpgradeInsecureRequests) ==
       mojom::blink::InsecureRequestPolicy::kLeaveInsecureRequestsAlone) {
     mojom::RequestContextType context = resource_request.GetRequestContext();
-    if (context != mojom::RequestContextType::UNSPECIFIED &&
-        resource_request.Url().ProtocolIs("http") &&
-        MixedContentChecker::ShouldAutoupgrade(
+    if (context == mojom::RequestContextType::UNSPECIFIED ||
+        !MixedContentChecker::ShouldAutoupgrade(
             fetch_client_settings_object->GetHttpsState(), context,
             settings_client, fetch_client_settings_object->GlobalObjectUrl())) {
+      return;
+    }
+    // We set the upgrade if insecure flag regardless of whether we autoupgrade
+    // due to scheme not being http, so any redirects get upgraded.
+    resource_request.SetUpgradeIfInsecure(true);
+    if (resource_request.Url().ProtocolIs("http")) {
       if (execution_context_for_logging->IsDocument()) {
         Document* document = Document::From(execution_context_for_logging);
         document->AddConsoleMessage(

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

@@ -15,6 +15,7 @@
 #include "third_party/blink/renderer/core/frame/settings.h"
 #include "third_party/blink/renderer/core/loader/empty_clients.h"
 #include "third_party/blink/renderer/core/testing/dummy_page_holder.h"
+#include "third_party/blink/renderer/platform/loader/fetch/fetch_client_settings_object.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_response.h"
 #include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
@@ -214,4 +215,71 @@ TEST(MixedContentCheckerTest, DetectMixedFavicon) {
       ReportingDisposition::kSuppressReporting));
 }
 
+class TestFetchClientSettingsObject : public FetchClientSettingsObject {
+ public:
+  const KURL& GlobalObjectUrl() const override { return url; }
+  HttpsState GetHttpsState() const override { return HttpsState::kModern; }
+  mojom::blink::InsecureRequestPolicy GetInsecureRequestsPolicy()
+      const override {
+    return mojom::blink::InsecureRequestPolicy::kLeaveInsecureRequestsAlone;
+  }
+
+  // These are not used in test, but need to be implemented since they are pure
+  // virtual.
+  const KURL& BaseUrl() const override { return url; }
+  const SecurityOrigin* GetSecurityOrigin() const override { return nullptr; }
+  network::mojom::ReferrerPolicy GetReferrerPolicy() const override {
+    return network::mojom::ReferrerPolicy::kAlways;
+  }
+  const String GetOutgoingReferrer() const override { return ""; }
+  AllowedByNosniff::MimeTypeCheck MimeTypeCheckForClassicWorkerScript()
+      const override {
+    return AllowedByNosniff::MimeTypeCheck::kStrict;
+  }
+  network::mojom::IPAddressSpace GetAddressSpace() const override {
+    return network::mojom::IPAddressSpace::kLocal;
+  }
+  const InsecureNavigationsSet& GetUpgradeInsecureNavigationsSet()
+      const override {
+    return set;
+  }
+
+ private:
+  const KURL url = KURL("https://example.test");
+  const InsecureNavigationsSet set;
+};
+
+TEST(MixedContentCheckerTest,
+     NotAutoupgradedMixedContentHasUpgradeIfInsecureSet) {
+  ResourceRequest request;
+  request.SetUrl(KURL("https://example.test"));
+  request.SetRequestContext(mojom::RequestContextType::AUDIO);
+  TestFetchClientSettingsObject settings;
+  // Used to get a non-null document.
+  DummyPageHolder holder;
+
+  MixedContentChecker::UpgradeInsecureRequest(
+      request, &settings, holder.GetDocument().GetExecutionContext(),
+      mojom::RequestContextFrameType::kTopLevel, nullptr);
+
+  EXPECT_FALSE(request.IsAutomaticUpgrade());
+  EXPECT_TRUE(request.UpgradeIfInsecure());
+}
+
+TEST(MixedContentCheckerTest, AutoupgradedMixedContentHasUpgradeIfInsecureSet) {
+  ResourceRequest request;
+  request.SetUrl(KURL("http://example.test"));
+  request.SetRequestContext(mojom::RequestContextType::AUDIO);
+  TestFetchClientSettingsObject settings;
+  // Used to get a non-null document.
+  DummyPageHolder holder;
+
+  MixedContentChecker::UpgradeInsecureRequest(
+      request, &settings, holder.GetDocument().GetExecutionContext(),
+      mojom::RequestContextFrameType::kTopLevel, nullptr);
+
+  EXPECT_TRUE(request.IsAutomaticUpgrade());
+  EXPECT_TRUE(request.UpgradeIfInsecure());
+}
+
 }  // namespace blink