Call FixupURL from ExtensionTabUtil::PrepareURLForNavigation.

After this CL, chrome.tabs.update-initiated (and other
extension-API-initiated) navigations to about:newtab will navigate to
chrome://new-tab-page (similarly rewriting other URLs - e.g.
"about:version" => "chrome://version" or "localhost:1234" [wrong
scheme] to "http://localhost:1234").  This CL restores the old,
pre-r818969 behavior that some extensions depend on.

As pointed out in https://crbug.com/1145381#c4, exposing an ability to
manipulate URLs in an unexpected way may erode some of security benefits
that were achieved via an earlier r818969.  This additional security
risk seems acceptable because 1) this ability is only exposed to
extension origins and 2) the URL is mutated upfront, before a navigation
starts, 3) invalid URLs are rejected.

Fixed: 1145381
Change-Id: Ia1dd7d8a0f4fd373d04e1b1353419ac30f88eb06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519034Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827296}

chrome/browser/extensions/BUILD.gn

@@ -861,6 +861,7 @@ static_library("extensions") {
     "//components/unified_consent",
     "//components/update_client",
     "//components/update_client:common_impl",
+    "//components/url_formatter",
     "//components/url_matcher",
     "//components/user_prefs",
     "//components/vector_icons",

chrome/browser/extensions/api/tabs/tabs_test.cc

@@ -2370,16 +2370,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToAboutBlank) {
   content::WebContents* test_contents = nullptr;
   {
     content::WebContentsAddedObserver test_contents_observer;
-    content::TestNavigationObserver nav_observer(nullptr);
-    nav_observer.StartWatchingNewWebContents();
-    content::ExecuteScriptAsync(
-        extension_contents,
-        content::JsReplace("window.open($1, '_blank')", web_url));
-
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), web_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
     test_contents = test_contents_observer.GetWebContents();
-    nav_observer.WaitForNavigationFinished();
-    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
-    EXPECT_EQ(web_url, nav_observer.last_navigation_url());
   }
   EXPECT_EQ(web_origin,
             test_contents->GetMainFrame()->GetLastCommittedOrigin());
@@ -2417,6 +2411,66 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToAboutBlank) {
       test_frame->GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque());
 }
 
+// Tests updating a URL of a web tab to an about:newtab.  Verify that the new
+// frame is placed in the correct process, has the correct origin and that no
+// DCHECKs are hit anywhere.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToAboutNewTab) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
+  ASSERT_TRUE(extension);
+  GURL extension_url = extension->GetResourceURL("file.html");
+  url::Origin extension_origin = url::Origin::Create(extension_url);
+  GURL web_url = embedded_test_server()->GetURL("/title1.html");
+  url::Origin web_origin = url::Origin::Create(web_url);
+
+  // https://crbug.com/1145381: about:version is rewritten to chrome://version
+  // when entered in the omnibox or used in a bookmark.  Such rewriting is
+  // definitely undesirable for http-initiated navigations (see r818969), but
+  // it is less clear what should happen in extension-initiated navigations.
+  GURL about_newtab_url = GURL("about:newtab");
+  GURL chrome_newtab_url = GURL("chrome://new-tab-page/");
+
+  // Navigate a tab to an extension page.
+  ui_test_utils::NavigateToURL(browser(), extension_url);
+  content::WebContents* extension_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(extension_origin,
+            extension_contents->GetMainFrame()->GetLastCommittedOrigin());
+
+  // Create another tab and navigate it to a web page.
+  content::WebContents* test_contents = nullptr;
+  {
+    content::WebContentsAddedObserver test_contents_observer;
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), web_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+    test_contents = test_contents_observer.GetWebContents();
+  }
+
+  // Use |chrome.tabs.update| API to navigate |test_contents| to an about:newtab
+  // URL.
+  {
+    content::TestNavigationObserver nav_observer(test_contents, 1);
+    int test_tab_id = ExtensionTabUtil::GetTabId(test_contents);
+    content::ExecuteScriptAsync(
+        extension_contents,
+        content::JsReplace("chrome.tabs.update($1, { url: $2 })", test_tab_id,
+                           about_newtab_url));
+    nav_observer.WaitForNavigationFinished();
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+    EXPECT_EQ(chrome_newtab_url, nav_observer.last_navigation_url());
+  }
+
+  // Verify the origin and process of the about:newtab tab.
+  content::RenderFrameHost* test_frame = test_contents->GetMainFrame();
+  EXPECT_EQ(chrome_newtab_url, test_frame->GetLastCommittedURL());
+  EXPECT_EQ(url::Origin::Create(chrome_newtab_url),
+            test_frame->GetLastCommittedOrigin());
+  EXPECT_NE(extension_contents->GetMainFrame()->GetProcess(),
+            test_contents->GetMainFrame()->GetProcess());
+}
+
 // Tests updating a URL of a web tab to a non-web-accessible-resource of an
 // extension - such navigation should be allowed.
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToNonWAR) {
@@ -2441,16 +2495,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToNonWAR) {
   content::WebContents* test_contents = nullptr;
   {
     content::WebContentsAddedObserver test_contents_observer;
-    content::TestNavigationObserver nav_observer(nullptr);
-    nav_observer.StartWatchingNewWebContents();
-    content::ExecuteScriptAsync(
-        extension_contents,
-        content::JsReplace("window.open($1, '_blank')", web_url));
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), web_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
     test_contents = test_contents_observer.GetWebContents();
-    nav_observer.WaitForNavigationFinished();
-
-    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
-    EXPECT_EQ(web_url, nav_observer.last_navigation_url());
   }
   EXPECT_EQ(web_origin,
             test_contents->GetMainFrame()->GetLastCommittedOrigin());

chrome/browser/extensions/extension_tab_util.cc

@@ -768,6 +768,14 @@ GURL ExtensionTabUtil::ResolvePossiblyRelativeURL(const std::string& url_string,
 }
 
 bool ExtensionTabUtil::IsKillURL(const GURL& url) {
+#if DCHECK_IS_ON()
+  // Caller should ensure that |url| is already "fixed up" by
+  // url_formatter::FixupURL, which (among many other things) takes care
+  // of rewriting about:kill into chrome://kill/.
+  if (url.SchemeIs(url::kAboutScheme))
+    DCHECK(url.IsAboutBlank() || url.IsAboutSrcdoc());
+#endif
+
   static const char* const kill_hosts[] = {
       chrome::kChromeUICrashHost,         chrome::kChromeUIDelayedHangUIHost,
       chrome::kChromeUIHangUIHost,        chrome::kChromeUIKillHost,
@@ -775,19 +783,10 @@ bool ExtensionTabUtil::IsKillURL(const GURL& url) {
       content::kChromeUIBrowserCrashHost, content::kChromeUIMemoryExhaustHost,
   };
 
-  // Check a fixed-up URL, to normalize the scheme and parse hosts correctly.
-  GURL fixed_url =
-      url_formatter::FixupURL(url.possibly_invalid_spec(), std::string());
-  if (!fixed_url.SchemeIs(content::kChromeUIScheme))
+  if (!url.SchemeIs(content::kChromeUIScheme))
     return false;
 
-  base::StringPiece fixed_host = fixed_url.host_piece();
-  for (size_t i = 0; i < base::size(kill_hosts); ++i) {
-    if (fixed_host == kill_hosts[i])
-      return true;
-  }
-
-  return false;
+  return base::Contains(kill_hosts, url.host_piece());
 }
 
 bool ExtensionTabUtil::PrepareURLForNavigation(const std::string& url_string,
@@ -797,6 +796,13 @@ bool ExtensionTabUtil::PrepareURLForNavigation(const std::string& url_string,
   GURL url =
       ExtensionTabUtil::ResolvePossiblyRelativeURL(url_string, extension);
 
+  // Ideally, the URL would only be "fixed" for user input (e.g. for URLs
+  // entered into the Omnibox), but some extensions rely on the legacy behavior
+  // where all navigations were subject to the "fixing".  See also
+  // https://crbug.com/1145381.
+  url = url_formatter::FixupURL(url.spec(), "" /* = desired_tld */);
+
+  // Reject invalid URLs.
   if (!url.is_valid()) {
     *error = ErrorUtils::FormatErrorMessage(tabs_constants::kInvalidUrlError,
                                             url_string);

chrome/browser/extensions/extension_tab_util.h

@@ -206,6 +206,9 @@ class ExtensionTabUtil {
   // Returns true if navigating to |url| would kill a page or the browser
   // itself, whether by simulating a crash, browser quit, thread hang, or
   // equivalent. Extensions should be prevented from navigating to such URLs.
+  //
+  // The caller should ensure that |url| has already been "fixed up" by calling
+  // url_formatter::FixupURL.
   static bool IsKillURL(const GURL& url);
 
   // Resolves the URL and ensures the extension is allowed to navigate to it.