Allow top-level navigation in extension pop-ups if it only triggers a download.

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

Index: chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index a8f61c9e8e30fa7545fcc3c368392fdcb7fdc905..9facdce27f2aecbd36a89d65d9bb2713417a07ba 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -24,10 +24,14 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/download_test_observer.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
@@ -38,6 +42,7 @@
 #include "extensions/test/result_catcher.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -105,7 +110,7 @@ class BrowserActionApiTest : public ExtensionApiTest {
     return browser_action_test_util_.get();
   }
 
-  bool OpenPopup(int index) {
+  WebContents* OpenPopup(int index) {
     ResultCatcher catcher;
     content::WindowedNotificationObserver popup_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
@@ -113,7 +118,13 @@ class BrowserActionApiTest : public ExtensionApiTest {
     GetBrowserActionsBar()->Press(index);
     popup_observer.Wait();
     EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
-    return GetBrowserActionsBar()->HasPopup();
+
+    if (!GetBrowserActionsBar()->HasPopup())
+      return nullptr;
+
+    const auto& source = static_cast<const content::Source<WebContents>&>(
+        popup_observer.source());
+    return source.ptr();
   }
 
   ExtensionAction* GetBrowserAction(const Extension& extension) {
@@ -794,5 +805,190 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, BrowserActionOpenPopupOnPopup) {
   EXPECT_TRUE(catcher.GetNextResult()) << message_;
 }
 
+class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
+ public:
+  const Extension& popup_extension() { return *popup_extension_; }
+  const Extension& other_extension() { return *other_extension_; }
+
+  void SetUpOnMainThread() override {
+    BrowserActionApiTest::SetUpOnMainThread();
+
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    // Load an extension with a pop-up.
+    ASSERT_TRUE(popup_extension_ = LoadExtension(test_data_dir_.AppendASCII(
+                    "browser_action/popup_with_form")));
+
+    // Load another extension (that we can try navigating to).
+    ASSERT_TRUE(other_extension_ = LoadExtension(test_data_dir_.AppendASCII(
+                    "browser_action/popup_with_iframe")));
+  }
+
+  enum ExpectedNavigationStatus {
+    EXPECTING_NAVIGATION_SUCCESS,
+    EXPECTING_NAVIGATION_FAILURE,
+  };
+
+  void TestPopupNavigationViaGet(
+      const GURL& target_url,
+      ExpectedNavigationStatus expected_navigation_status) {
+    std::string navigation_starting_script =
+        "window.location = '" + target_url.spec() + "';\n";
+    TestPopupNavigation(target_url, expected_navigation_status,
+                        navigation_starting_script);
+  }
+
+  void TestPopupNavigationViaPost(
+      const GURL& target_url,
+      ExpectedNavigationStatus expected_navigation_status) {
+    std::string navigation_starting_script =
+        "var form = document.getElementById('form');\n"
+        "form.action = '" + target_url.spec() + "';\n"
+        "form.submit();\n";
+    TestPopupNavigation(target_url, expected_navigation_status,
+                        navigation_starting_script);
+  }
+
+ private:
+  void TestPopupNavigation(const GURL& target_url,
+                           ExpectedNavigationStatus expected_navigation_status,
+                           std::string navigation_starting_script) {
+    // Were there any failures so far (e.g. in SetUpOnMainThread)?
+    ASSERT_FALSE(HasFailure());
+
+    // Simulate a click on the browser action to open the popup.
+    WebContents* popup = OpenPopup(0);
+    ASSERT_TRUE(popup);
+    GURL popup_url = popup_extension().GetResourceURL("popup.html");
+    EXPECT_EQ(popup_url, popup->GetLastCommittedURL());
+
+    // Note that the |setTimeout| call below is needed to make sure
+    // ExecuteScriptAndExtractBool returns *after* a scheduled navigation has
+    // already started.
+    std::string script_to_execute =
+        navigation_starting_script +
+        "setTimeout(\n"
+        "  function() { window.domAutomationController.send(true); },\n"
+        "  0);\n";
+
+    // Try to navigate the pop-up.
+    bool ignored_script_result = false;
+    content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);

[Devlin, 2016/09/27 15:57:28]

Is this used?

[Łukasz Anforowicz, 2016/09/27 17:00:50]

Ooops - thanks for catching this.  My mistake - we used to pass popup =
popup_destruction_watcher.web_contents() to the verification method - I stopped
doing this in the latest patchset... :-(  Fixed.

+    EXPECT_TRUE(ExecuteScriptAndExtractBool(popup, script_to_execute,
+                                            &ignored_script_result));
+
+    // Verify if the popup navigation succeeded or failed as expected.
+    if (!popup) {
+      // If navigation ends up in a tab, then the tab will be focused and
+      // therefore the popup will be closed, destroying associated WebContents -
+      // don't do any verification in this case.
+      ADD_FAILURE() << "Navigation should not close extension pop-up";
+    } else {
+      // If the extension popup is still opened, then wait until there is no
+      // load in progress, and verify whether the navigation succeeded or not.
+      WaitForLoadStop(popup);
+      if (expected_navigation_status == EXPECTING_NAVIGATION_SUCCESS) {
+        EXPECT_EQ(target_url, popup->GetLastCommittedURL())
+            << "Navigation to " << target_url
+            << " should succeed in an extension pop-up";
+      } else {
+        EXPECT_NE(target_url, popup->GetLastCommittedURL())
+            << "Navigation to " << target_url
+            << " should fail in an extension pop-up";
+        EXPECT_THAT(
+            popup->GetLastCommittedURL(),
+            ::testing::AnyOf(::testing::Eq(popup_url),
+                             ::testing::Eq(GURL("chrome-extension://invalid")),
+                             ::testing::Eq(GURL("about:blank"))));
+      }
+    }
+
+    // Make sure that the web navigation did not succeed somewhere outside of
+    // the extension popup (as it might if ExtensionViewHost::OpenURLFromTab
+    // forwards the navigation to Browser::OpenURL [which doesn't specify a
+    // source WebContents]).
+    TabStripModel* tabs = browser()->tab_strip_model();
+    for (int i = 0; i < tabs->count(); i++) {
+      WebContents* tab_contents = tabs->GetWebContentsAt(i);
+      WaitForLoadStop(tab_contents);
+      EXPECT_NE(target_url, tab_contents->GetLastCommittedURL())
+          << "Navigating an extension pop-up should not affect tabs.";
+    }
+
+    // Close the pop-up.
+    BrowserActionTestUtil* actions_bar = GetBrowserActionsBar();
+    EXPECT_TRUE(actions_bar->HidePopup());

[Devlin, 2016/09/27 15:57:28]

nit: Just inline actions_bar

[Łukasz Anforowicz, 2016/09/27 17:00:50]

Done.

+  }
+
+  const Extension* popup_extension_;
+  const Extension* other_extension_;
+};
+
+// Tests that an extension pop-up cannot be navigated to a web page.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, Webpage) {
+  GURL web_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  TestPopupNavigationViaGet(web_url, EXPECTING_NAVIGATION_FAILURE);
+  TestPopupNavigationViaPost(web_url, EXPECTING_NAVIGATION_FAILURE);
+}
+
+// Tests that an extension pop-up can be navigated to another page
+// in the same extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInSameExtension) {
+  GURL other_page_in_same_extension =
+      popup_extension().GetResourceURL("other_page.html");
+  TestPopupNavigationViaGet(other_page_in_same_extension,
+                            EXPECTING_NAVIGATION_SUCCESS);
+  TestPopupNavigationViaPost(other_page_in_same_extension,
+                             EXPECTING_NAVIGATION_SUCCESS);
+}
+
+// Tests that an extension pop-up cannot be navigated to a page
+// in another extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInOtherExtension) {
+  GURL other_extension_url = other_extension().GetResourceURL("other.html");
+  TestPopupNavigationViaGet(other_extension_url, EXPECTING_NAVIGATION_FAILURE);
+  TestPopupNavigationViaPost(other_extension_url, EXPECTING_NAVIGATION_FAILURE);
+}
+
+// Tests that navigating an extension pop-up to a http URI that returns
+// Content-Disposition: attachment; filename=...
+// works: No navigation, but download shelf visible + download goes through.
+//
+// Note - there is no "...ViaGet" flavour of this test, because we don't care
+// (yet) if GET succeeds with the download or not (it probably should succeed
+// for consistency with POST, but it always failed in M54 and before).  After
+// abandoing ShouldFork/OpenURL for all methods (not just for POST) [see comment
+// about https://crbug.com/646261 in ChromeContentRendererClient::ShouldFork]
+// GET should automagically start working for downloads.

[Devlin, 2016/09/27 15:57:28]

Should there be a TODO: Enable the test for Get once these are resolved?

[Łukasz Anforowicz, 2016/09/27 17:00:50]

Done.

+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaPost) {
+  content::DownloadTestObserverTerminal downloads_observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      1,  // == wait_count (only waiting for "download-test3.gif").
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+
+  // Navigate to a URL that replies with
+  // Content-Disposition: attachment; filename=...
+  // header.
+  GURL download_url(
+      embedded_test_server()->GetURL("foo.com", "/download-test3.gif"));
+  TestPopupNavigationViaPost(download_url, EXPECTING_NAVIGATION_FAILURE);
+
+  // Verify that "download-test3.gif got downloaded.
+  downloads_observer.WaitForFinished();
+  EXPECT_EQ(0u, downloads_observer.NumDangerousDownloadsSeen());
+  EXPECT_EQ(1u, downloads_observer.NumDownloadsSeenInState(
+                    content::DownloadItem::COMPLETE));
+
+  // The test verification below is applicable only to scenarios where the
+  // download shelf is supported - on ChromeOS, instead of the download shelf,
+  // there is a download notification in the right-bottom corner of the screen.
+#if !defined(OS_CHROMEOS)
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+#endif
+}
+
 }  // namespace
 }  // namespace extensions