Exempt Chrome Apps <webview> from HTTPS-First Mode

This fixes a crash caused by the HttpsOnlyModeTabHelper not being
created for Chrome Apps <webview> contexts, and makes things a bit more
robust by creating the TabHelper if it does not yet exist when the
HttpsOnlyModeNavigationThrottle is created. This also exempts Chrome
Apps <webview> contexts from HTTPS-First Mode, since they fall outside
the domain of the feature as currently implemented (and because Apps
have superpowers that could bypass this and other features already).
Also includes new WebViewHttpsFirstModeTest regression tests for this
crash for both HTTP and HTTPS page loads in <webview>.

Bug: 1233889
Change-Id: I6a2d5fb5325e5ae390cdb88f43e234cc9430c0d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3059961
Commit-Queue: Chris Thompson <cthomp@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#907334}

chrome/browser/apps/guest_view/web_view_browsertest.cc

@@ -21,6 +21,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_restrictions.h"
@@ -50,7 +51,9 @@
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/recently_audible_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
@@ -61,6 +64,7 @@
 #include "components/guest_view/browser/guest_view_manager_factory.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
 #include "components/no_state_prefetch/browser/no_state_prefetch_link_manager.h"
+#include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/browser/db/fake_database_manager.h"
 #include "components/security_interstitials/content/security_interstitial_tab_helper.h"
 #include "components/version_info/channel.h"
@@ -2183,6 +2187,85 @@ IN_PROC_BROWSER_TEST_F(WebViewSafeBrowsingTest,
   TestHelper("testLoadAbortSafeBrowsing", "web_view/shim", NO_TEST_SERVER);
 }
 
+class WebViewHttpsFirstModeTest : public WebViewTest {
+ public:
+  WebViewHttpsFirstModeTest() = default;
+  ~WebViewHttpsFirstModeTest() override = default;
+
+  void SetUp() override {
+    feature_list_.InitAndEnableFeature(features::kHttpsOnlyMode);
+    WebViewTest::SetUp();
+  }
+
+  void LoadUrlInGuest(const GURL& guest_url) {
+    // Create the guest.
+    auto* embedder_web_contents = GetFirstAppWindowWebContents();
+    ExtensionTestMessageListener guest_added("GuestAddedToDom", false);
+    EXPECT_TRUE(content::ExecuteScript(embedder_web_contents,
+                                       base::StringPrintf("createGuest();\n")));
+    ASSERT_TRUE(guest_added.WaitUntilSatisfied());
+
+    // Now load the guest.
+    content::TestNavigationObserver observer(guest_url);
+    ExtensionTestMessageListener guest_loaded("GuestLoaded", false);
+    std::string command =
+        base::StringPrintf("loadGuestUrl('%s');\n", guest_url.spec().c_str());
+    EXPECT_TRUE(content::ExecuteScript(embedder_web_contents, command));
+    ASSERT_TRUE(guest_loaded.WaitUntilSatisfied());
+
+    // Wait for guest navigation to complete.
+    auto* guest_web_contents =
+        GetGuestViewManager()->WaitForSingleGuestCreated();
+    ASSERT_TRUE(
+        guest_web_contents->GetMainFrame()->GetProcess()->IsForGuestsOnly());
+    observer.WatchExistingWebContents();
+    observer.WaitForNavigationFinished();
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+// Tests that loading an HTTPS page in a guest <webview> with HTTPS-First Mode
+// enabled doesn't crash and doesn't trigger the interstitial.
+// Regression test for crbug.com/1233889
+IN_PROC_BROWSER_TEST_F(WebViewHttpsFirstModeTest, GuestLoadsHttpsWithoutError) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled,
+                                               true);
+
+  net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_server.ServeFilesFromSourceDirectory(GetChromeTestDataDir());
+  ASSERT_TRUE(https_server.Start());
+  GURL guest_url = https_server.GetURL("/simple.html");
+  LoadAndLaunchPlatformApp("web_view/interstitial_teardown", "EmbedderLoaded");
+  LoadUrlInGuest(guest_url);
+
+  // Page should load without any interstitial (and no crashing).
+  auto* embedder_web_contents = GetFirstAppWindowWebContents();
+  auto* guest_web_contents = GetGuestViewManager()->WaitForSingleGuestCreated();
+  EXPECT_FALSE(IsShowingInterstitial(guest_web_contents));
+  EXPECT_FALSE(IsShowingInterstitial(embedder_web_contents));
+}
+
+// Tests that loading an HTTP page in a guest <webview> with HTTPS-First Mode
+// enabled doesn't crash and doesn't trigger the interstitial.
+// Regression test for crbug.com/1233889
+IN_PROC_BROWSER_TEST_F(WebViewHttpsFirstModeTest, GuestLoadsHttpWithoutError) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled,
+                                               true);
+
+  ASSERT_TRUE(StartEmbeddedTestServer());
+  GURL guest_url = embedded_test_server()->GetURL("/simple.html");
+  LoadAndLaunchPlatformApp("web_view/interstitial_teardown", "EmbedderLoaded");
+  LoadUrlInGuest(guest_url);
+
+  // Page should load without any interstitial (and no crashing).
+  auto* embedder_web_contents = GetFirstAppWindowWebContents();
+  auto* guest_web_contents = GetGuestViewManager()->WaitForSingleGuestCreated();
+  EXPECT_FALSE(IsShowingInterstitial(guest_web_contents));
+  EXPECT_FALSE(IsShowingInterstitial(embedder_web_contents));
+}
+
 IN_PROC_BROWSER_TEST_F(WebViewTest, ShimSrcAttribute) {
   ASSERT_TRUE(RunExtensionTest("platform_apps/web_view/src_attribute",
                                {.launch_as_platform_app = true}))

chrome/browser/ssl/https_only_mode_navigation_throttle.cc

@@ -44,6 +44,13 @@ HttpsOnlyModeNavigationThrottle::MaybeCreateThrottleFor(
     return nullptr;
   }
 
+  // Ensure that the HttpsOnlyModeTabHelper has been created (this does nothing
+  // if it has already been created for the WebContents). There are cases where
+  // the tab helper won't get created by the initialization in
+  // chrome/browser/ui/tab_helpers.cc but the criteria for adding the throttle
+  // are still met (see crbug.com/1233889 for one example).
+  HttpsOnlyModeTabHelper::CreateForWebContents(handle->GetWebContents());
+
   return std::make_unique<HttpsOnlyModeNavigationThrottle>(
       handle, std::move(blocking_page_factory));
 }

chrome/browser/ssl/https_only_mode_upgrade_interceptor.cc

@@ -12,12 +12,17 @@
 #include "components/prefs/pref_service.h"
 #include "components/security_interstitials/content/stateful_ssl_host_state_delegate.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/buildflags/buildflags.h"
 #include "net/base/url_util.h"
 #include "third_party/blink/public/mojom/loader/resource_load_info.mojom.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 #include "url/url_util.h"
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "components/guest_view/browser/guest_view_base.h"
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+
 namespace {
 
 // Used to handle upgrading/fallback for tests using EmbeddedTestServer which
@@ -56,13 +61,14 @@ void HttpsOnlyModeUpgradeInterceptor::MaybeCreateLoader(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // If there isn't a BrowserContext/Profile for this, then just allow it.
-  if (!browser_context) {
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+  if (!profile) {
     std::move(callback).Run({});
     return;
   }
 
   // Don't upgrade if the HTTPS-Only Mode setting isn't enabled.
-  auto* prefs = Profile::FromBrowserContext(browser_context)->GetPrefs();
+  auto* prefs = profile->GetPrefs();
   if (!prefs || !prefs->GetBoolean(prefs::kHttpsOnlyModeEnabled)) {
     std::move(callback).Run({});
     return;
@@ -75,12 +81,16 @@ void HttpsOnlyModeUpgradeInterceptor::MaybeCreateLoader(
     std::move(callback).Run({});
     return;
   }
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  if (!profile) {
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  // If this is a GuestView (e.g., Chrome Apps <webview>) then HTTPS-First Mode
+  // should not apply. See crbug.com/1233889 for more details.
+  if (guest_view::GuestViewBase::IsGuest(web_contents)) {
     std::move(callback).Run({});
     return;
   }
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+
   // TODO(crbug.com/1228188) There might be a race condition where we
   // get here before there is a tab. If the issue was caused by web
   // contents, profile or tab helper being null, try to determine the

chrome/test/data/extensions/platform_apps/web_view/interstitial_teardown/embedder.js

@@ -27,6 +27,15 @@ window.loadGuest = function(port) {
   chrome.test.sendMessage('GuestLoaded');
 };
 
+window.loadGuestUrl = function(url) {
+  window.console.log('embedder.loadGuest: ' + url);
+  webview.setAttribute('src', url);
+  webview.style.position = 'fixed';
+  webview.style.left = '0px';
+  webview.style.top = '0px';
+  chrome.test.sendMessage('GuestLoaded');
+};
+
 window.onload = function() {
   chrome.test.sendMessage('EmbedderLoaded');
 };