Dont show SB interstitials for extension requests

Previously, if a safe browsing error triggered by an extension request
it would result in an invisible interstitial, and cause the extension
to hang. This removes the interstitial so the flagged request gets
cancelled, but the extension doesn't hang.

There was discussion in the bug about adding an API similar to the
one in WebView so the extension can react to the SB flagged request
and decide to bypass it or cancel it. This CL does not implement that,
just fixes the immediate bug.

Bug: 1036603
Change-Id: I0ae89bd9e8e09eb0956d3afab606d90738392c31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527534Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825991}

chrome/browser/safe_browsing/ui_manager.cc

@@ -36,6 +36,9 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/browser/process_manager.h"
+#endif
 #include "ipc/ipc_message.h"
 #include "url/gurl.h"
 
@@ -126,6 +129,27 @@ void SafeBrowsingUIManager::StartDisplayingBlockingPage(
     return;
   }
 
+// We don't show interstitials for extension triggered SB errors, since they
+// might not be visible, and cause the extension to hang. The request is just
+// cancelled instead.
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  extensions::ProcessManager* extension_manager =
+      extensions::ProcessManager::Get(web_contents->GetBrowserContext());
+  if (extension_manager) {
+    extensions::ExtensionHost* extension_host =
+        extension_manager->GetExtensionHostForRenderFrameHost(
+            web_contents->GetMainFrame());
+    if (extension_host) {
+      if (!resource.callback.is_null()) {
+        resource.callback_thread->PostTask(
+            FROM_HERE, base::BindOnce(resource.callback, false /* proceed */,
+                                      false /* showed_interstitial */));
+      }
+      return;
+    }
+  }
+#endif
+
   // With committed interstitials, if this is a main frame load, we need to
   // get the navigation URL and referrer URL from the navigation entry now,
   // since they are required for threat reporting, and the entry will be

chrome/browser/safe_browsing/ui_manager_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/run_loop.h"
+#include "base/values.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
 #include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
@@ -31,6 +32,13 @@
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/web_contents_tester.h"
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/browser/extension_host.h"
+#include "extensions/browser/process_manager.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/manifest_constants.h"
+#endif
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -45,36 +53,39 @@ static const char* kLandingURL = "https://www.landing.com";
 namespace safe_browsing {
 
 class SafeBrowsingCallbackWaiter {
-  public:
-   SafeBrowsingCallbackWaiter() {}
-
-   bool callback_called() const { return callback_called_; }
-   bool proceed() const { return proceed_; }
-
-   void OnBlockingPageDone(bool proceed, bool showed_interstitial) {
-     DCHECK_CURRENTLY_ON(BrowserThread::UI);
-     callback_called_ = true;
-     proceed_ = proceed;
-     loop_.Quit();
-   }
-
-   void OnBlockingPageDoneOnIO(bool proceed, bool showed_interstitial) {
-     DCHECK_CURRENTLY_ON(BrowserThread::IO);
-     content::GetUIThreadTaskRunner({})->PostTask(
-         FROM_HERE,
-         base::BindOnce(&SafeBrowsingCallbackWaiter::OnBlockingPageDone,
-                        base::Unretained(this), proceed, showed_interstitial));
-   }
-
-   void WaitForCallback() {
-     DCHECK_CURRENTLY_ON(BrowserThread::UI);
-     loop_.Run();
-   }
-
-  private:
-   bool callback_called_ = false;
-   bool proceed_ = false;
-   base::RunLoop loop_;
+ public:
+  SafeBrowsingCallbackWaiter() {}
+
+  bool callback_called() const { return callback_called_; }
+  bool proceed() const { return proceed_; }
+  bool showed_interstitial() const { return showed_interstitial_; }
+
+  void OnBlockingPageDone(bool proceed, bool showed_interstitial) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    callback_called_ = true;
+    proceed_ = proceed;
+    showed_interstitial_ = showed_interstitial;
+    loop_.Quit();
+  }
+
+  void OnBlockingPageDoneOnIO(bool proceed, bool showed_interstitial) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    content::GetUIThreadTaskRunner({})->PostTask(
+        FROM_HERE,
+        base::BindOnce(&SafeBrowsingCallbackWaiter::OnBlockingPageDone,
+                       base::Unretained(this), proceed, showed_interstitial));
+  }
+
+  void WaitForCallback() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    loop_.Run();
+  }
+
+ private:
+  bool callback_called_ = false;
+  bool proceed_ = false;
+  bool showed_interstitial_ = false;
+  base::RunLoop loop_;
 };
 
 class SafeBrowsingUIManagerTest : public ChromeRenderViewHostTestHarness {
@@ -550,4 +561,42 @@ TEST_F(SafeBrowsingUIManagerTest, ShowBlockPageNoCallback) {
   ui_manager()->DisplayBlockingPage(resource);
 }
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+TEST_F(SafeBrowsingUIManagerTest, NoInterstitialInExtensions) {
+  // Pretend the current web contents is in an extension.
+  base::DictionaryValue manifest;
+  manifest.SetString(extensions::manifest_keys::kName, "TestComponentApp");
+  manifest.SetString(extensions::manifest_keys::kVersion, "0.0.0.0");
+  manifest.SetString(extensions::manifest_keys::kApp, "true");
+  manifest.SetString(extensions::manifest_keys::kPlatformAppBackgroundPage,
+                     std::string());
+  std::string error;
+  scoped_refptr<extensions::Extension> app;
+  app = extensions::Extension::Create(
+      base::FilePath(), extensions::Manifest::COMPONENT, manifest, 0, &error);
+  extensions::ProcessManager* extension_manager =
+      extensions::ProcessManager::Get(web_contents()->GetBrowserContext());
+  extension_manager->CreateBackgroundHost(app.get(), GURL("background.html"));
+  extensions::ExtensionHost* host =
+      extension_manager->GetBackgroundHostForExtension(app->id());
+
+  security_interstitials::UnsafeResource resource =
+      MakeUnsafeResource(kBadURL, false /* is_subresource */);
+  resource.web_contents_getter = security_interstitials::GetWebContentsGetter(
+      host->host_contents()->GetMainFrame()->GetProcess()->GetID(),
+      host->host_contents()->GetMainFrame()->GetRoutingID());
+
+  SafeBrowsingCallbackWaiter waiter;
+  resource.callback =
+      base::BindRepeating(&SafeBrowsingCallbackWaiter::OnBlockingPageDone,
+                          base::Unretained(&waiter));
+  resource.callback_thread = content::GetUIThreadTaskRunner({});
+  SafeBrowsingUIManager::StartDisplayingBlockingPage(ui_manager(), resource);
+  waiter.WaitForCallback();
+  EXPECT_FALSE(waiter.proceed());
+  EXPECT_FALSE(waiter.showed_interstitial());
+  delete host;
+}
+#endif
+
 }  // namespace safe_browsing