chore: update extensions url handling to match upstream (#40063)

- https://chromium-review.googlesource.com/c/chromium/src/+/4772028
- https://chromium-review.googlesource.com/c/chromium/src/+/4264656
- https://chromium-review.googlesource.com/c/chromium/src/+/4712150

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

shell/browser/extensions/api/tabs/tabs_api.cc

@@ -7,20 +7,27 @@
 #include <memory>
 #include <utility>
 
+#include "base/command_line.h"
 #include "base/strings/pattern.h"
+#include "base/types/expected_macros.h"
 #include "chrome/common/url_constants.h"
 #include "components/url_formatter/url_fixer.h"
 #include "content/public/browser/navigation_entry.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/common/error_utils.h"
+#include "extensions/common/extension_features.h"
+#include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/mojom/host_id.mojom.h"
 #include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/switches.h"
 #include "shell/browser/api/electron_api_web_contents.h"
 #include "shell/browser/native_window.h"
 #include "shell/browser/web_contents_zoom_controller.h"
 #include "shell/browser/window_list.h"
 #include "shell/common/extensions/api/tabs.h"
+#include "third_party/blink/public/common/chrome_debug_urls.h"
 #include "third_party/blink/public/common/page/page_zoom.h"
 #include "url/gurl.h"
 
@@ -468,17 +475,25 @@ bool IsKillURL(const GURL& url) {
     DCHECK(url.IsAboutBlank() || url.IsAboutSrcdoc());
 #endif
 
-  static const char* const kill_hosts[] = {
-      chrome::kChromeUICrashHost,         chrome::kChromeUIDelayedHangUIHost,
-      chrome::kChromeUIHangUIHost,        chrome::kChromeUIKillHost,
+  // Disallow common renderer debug URLs.
+  // Note: this would also disallow JavaScript URLs, but we already explicitly
+  // check for those before calling into here from PrepareURLForNavigation.
+  if (blink::IsRendererDebugURL(url)) {
+    return true;
+  }
+
+  if (!url.SchemeIs(content::kChromeUIScheme)) {
+    return false;
+  }
+
+  // Also disallow a few more hosts which are not covered by the check above.
+  static const char* const kKillHosts[] = {
+      chrome::kChromeUIDelayedHangUIHost, chrome::kChromeUIHangUIHost,
       chrome::kChromeUIQuitHost,          chrome::kChromeUIRestartHost,
       content::kChromeUIBrowserCrashHost, content::kChromeUIMemoryExhaustHost,
   };
 
-  if (!url.SchemeIs(content::kChromeUIScheme))
-    return false;
-
-  return base::Contains(kill_hosts, url.host_piece());
+  return base::Contains(kKillHosts, url.host_piece());
 }
 
 GURL ResolvePossiblyRelativeURL(const std::string& url_string,
@@ -489,10 +504,18 @@ GURL ResolvePossiblyRelativeURL(const std::string& url_string,
 
   return url;
 }
-bool PrepareURLForNavigation(const std::string& url_string,
-                             const Extension* extension,
-                             GURL* return_url,
-                             std::string* error) {
+
+bool AllowFileAccess(const ExtensionId& extension_id,
+                     content::BrowserContext* context) {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kDisableExtensionsFileAccessCheck) ||
+         ExtensionPrefs::Get(context)->AllowFileAccess(extension_id);
+}
+
+base::expected<GURL, std::string> PrepareURLForNavigation(
+    const std::string& url_string,
+    const Extension* extension,
+    content::BrowserContext* browser_context) {
   GURL url = ResolvePossiblyRelativeURL(url_string, extension);
 
   // Ideally, the URL would only be "fixed" for user input (e.g. for URLs
@@ -504,34 +527,62 @@ bool PrepareURLForNavigation(const std::string& url_string,
   // Reject invalid URLs.
   if (!url.is_valid()) {
     const char kInvalidUrlError[] = "Invalid url: \"*\".";
-    *error = ErrorUtils::FormatErrorMessage(kInvalidUrlError, url_string);
-    return false;
+    return base::unexpected(
+        ErrorUtils::FormatErrorMessage(kInvalidUrlError, url_string));
+  }
+
+  // Don't let the extension use JavaScript URLs in API triggered navigations.
+  if (url.SchemeIs(url::kJavaScriptScheme)) {
+    const char kJavaScriptUrlsNotAllowedInExtensionNavigations[] =
+        "JavaScript URLs are not allowed in API based extension navigations. "
+        "Use "
+        "chrome.scripting.executeScript instead.";
+    return base::unexpected(kJavaScriptUrlsNotAllowedInExtensionNavigations);
   }
 
   // Don't let the extension crash the browser or renderers.
   if (IsKillURL(url)) {
     const char kNoCrashBrowserError[] =
         "I'm sorry. I'm afraid I can't do that.";
-    *error = kNoCrashBrowserError;
-    return false;
+    return base::unexpected(kNoCrashBrowserError);
   }
 
   // Don't let the extension navigate directly to devtools scheme pages, unless
   // they have applicable permissions.
-  if (url.SchemeIs(content::kChromeDevToolsScheme) &&
-      !(extension->permissions_data()->HasAPIPermission(
-            extensions::mojom::APIPermissionID::kDevtools) ||
-        extension->permissions_data()->HasAPIPermission(
-            extensions::mojom::APIPermissionID::kDebugger))) {
-    const char kCannotNavigateToDevtools[] =
-        "Cannot navigate to a devtools:// page without either the devtools or "
-        "debugger permission.";
-    *error = kCannotNavigateToDevtools;
-    return false;
+  if (url.SchemeIs(content::kChromeDevToolsScheme)) {
+    bool has_permission =
+        extension && (extension->permissions_data()->HasAPIPermission(
+                          mojom::APIPermissionID::kDevtools) ||
+                      extension->permissions_data()->HasAPIPermission(
+                          mojom::APIPermissionID::kDebugger));
+    if (!has_permission) {
+      const char kCannotNavigateToDevtools[] =
+          "Cannot navigate to a devtools:// page without either the devtools "
+          "or "
+          "debugger permission.";
+      return base::unexpected(kCannotNavigateToDevtools);
+    }
   }
 
-  return_url->Swap(&url);
-  return true;
+  // Don't let the extension navigate directly to chrome-untrusted scheme pages.
+  if (url.SchemeIs(content::kChromeUIUntrustedScheme)) {
+    const char kCannotNavigateToChromeUntrusted[] =
+        "Cannot navigate to a chrome-untrusted:// page.";
+    return base::unexpected(kCannotNavigateToChromeUntrusted);
+  }
+
+  // Don't let the extension navigate directly to file scheme pages, unless
+  // they have file access.
+  if (url.SchemeIsFile() &&
+      !AllowFileAccess(extension->id(), browser_context) &&
+      base::FeatureList::IsEnabled(
+          extensions_features::kRestrictFileURLNavigation)) {
+    const char kFileUrlsNotAllowedInExtensionNavigations[] =
+        "Cannot navigate to a file URL without local file access.";
+    return base::unexpected(kFileUrlsNotAllowedInExtensionNavigations);
+  }
+
+  return url;
 }
 
 TabsUpdateFunction::TabsUpdateFunction() : web_contents_(nullptr) {}
@@ -566,22 +617,14 @@ ExtensionFunction::ResponseAction TabsUpdateFunction::Run() {
 bool TabsUpdateFunction::UpdateURL(const std::string& url_string,
                                    int tab_id,
                                    std::string* error) {
-  GURL url;
-  if (!PrepareURLForNavigation(url_string, extension(), &url, error)) {
-    return false;
-  }
-
-  const bool is_javascript_scheme = url.SchemeIs(url::kJavaScriptScheme);
-  // JavaScript URLs are forbidden in chrome.tabs.update().
-  if (is_javascript_scheme) {
-    const char kJavaScriptUrlsNotAllowedInTabsUpdate[] =
-        "JavaScript URLs are not allowed in chrome.tabs.update. Use "
-        "chrome.tabs.executeScript instead.";
-    *error = kJavaScriptUrlsNotAllowedInTabsUpdate;
+  auto url =
+      PrepareURLForNavigation(url_string, extension(), browser_context());
+  if (!url.has_value()) {
+    *error = std::move(url.error());
     return false;
   }
 
-  content::NavigationController::LoadURLParams load_params(url);
+  content::NavigationController::LoadURLParams load_params(*url);
 
   // Treat extension-initiated navigations as renderer-initiated so that the URL
   // does not show in the omnibox until it commits.  This avoids URL spoofs

spec/extensions-spec.ts

@@ -1039,33 +1039,77 @@ describe('chrome extensions', () => {
         });
       });
 
-      it('update', async () => {
-        await w.loadURL(url);
+      describe('update', () => {
+        it('can update muted status', async () => {
+          await w.loadURL(url);
 
-        const message = { method: 'update', args: [{ muted: true }] };
-        w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+          const message = { method: 'update', args: [{ muted: true }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
 
-        const [,, responseString] = await once(w.webContents, 'console-message');
-        const response = JSON.parse(responseString);
+          const [,, responseString] = await once(w.webContents, 'console-message');
+          const response = JSON.parse(responseString);
+
+          expect(response).to.have.property('mutedInfo').that.is.a('object');
+          const { mutedInfo } = response;
+          expect(mutedInfo).to.deep.eq({
+            muted: true,
+            reason: 'user'
+          });
+        });
+
+        it('fails when navigating to an invalid url', async () => {
+          await w.loadURL(url);
+
+          const message = { method: 'update', args: [{ url: 'chrome://crash' }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+
+          const [,, responseString] = await once(w.webContents, 'console-message');
+          const { error } = JSON.parse(responseString);
+          expect(error).to.eq('I\'m sorry. I\'m afraid I can\'t do that.');
+        });
+
+        it('fails when navigating to prohibited url', async () => {
+          await w.loadURL(url);
+
+          const message = { method: 'update', args: [{ url: 'chrome://crash' }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+
+          const [,, responseString] = await once(w.webContents, 'console-message');
+          const { error } = JSON.parse(responseString);
+          expect(error).to.eq('I\'m sorry. I\'m afraid I can\'t do that.');
+        });
+
+        it('fails when navigating to a devtools url without permission', async () => {
+          await w.loadURL(url);
 
-        expect(response).to.have.property('url').that.is.a('string');
-        expect(response).to.have.property('title').that.is.a('string');
-        expect(response).to.have.property('active').that.is.a('boolean');
-        expect(response).to.have.property('autoDiscardable').that.is.a('boolean');
-        expect(response).to.have.property('discarded').that.is.a('boolean');
-        expect(response).to.have.property('groupId').that.is.a('number');
-        expect(response).to.have.property('highlighted').that.is.a('boolean');
-        expect(response).to.have.property('id').that.is.a('number');
-        expect(response).to.have.property('incognito').that.is.a('boolean');
-        expect(response).to.have.property('index').that.is.a('number');
-        expect(response).to.have.property('pinned').that.is.a('boolean');
-        expect(response).to.have.property('selected').that.is.a('boolean');
-        expect(response).to.have.property('windowId').that.is.a('number');
-        expect(response).to.have.property('mutedInfo').that.is.a('object');
-        const { mutedInfo } = response;
-        expect(mutedInfo).to.deep.eq({
-          muted: true,
-          reason: 'user'
+          const message = { method: 'update', args: [{ url: 'devtools://blah' }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+
+          const [, , responseString] = await once(w.webContents, 'console-message');
+          const { error } = JSON.parse(responseString);
+          expect(error).to.eq('Cannot navigate to a devtools:// page without either the devtools or debugger permission.');
+        });
+
+        it('fails when navigating to a chrome-untrusted url', async () => {
+          await w.loadURL(url);
+
+          const message = { method: 'update', args: [{ url: 'chrome-untrusted://blah' }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+
+          const [, , responseString] = await once(w.webContents, 'console-message');
+          const { error } = JSON.parse(responseString);
+          expect(error).to.eq('Cannot navigate to a chrome-untrusted:// page.');
+        });
+
+        it('fails when navigating to a file url withotut file access', async () => {
+          await w.loadURL(url);
+
+          const message = { method: 'update', args: [{ url: 'file://blah' }] };
+          w.webContents.executeJavaScript(`window.postMessage('${JSON.stringify(message)}', '*')`);
+
+          const [, , responseString] = await once(w.webContents, 'console-message');
+          const { error } = JSON.parse(responseString);
+          expect(error).to.eq('Cannot navigate to a file URL without local file access.');
         });
       });
 

spec/fixtures/extensions/chrome-tabs/api-async/background.js

@@ -1,6 +1,6 @@
 /* global chrome */
 
-const handleRequest = (request, sender, sendResponse) => {
+const handleRequest = async (request, sender, sendResponse) => {
   const { method, args = [] } = request;
   const tabId = sender.tab.id;
 
@@ -53,7 +53,12 @@ const handleRequest = (request, sender, sendResponse) => {
 
     case 'update': {
       const [params] = args;
-      chrome.tabs.update(tabId, params).then(sendResponse);
+      try {
+        const response = await chrome.tabs.update(tabId, params);
+        sendResponse(response);
+      } catch (error) {
+        sendResponse({ error: error.message });
+      }
       break;
     }
   }