Revert "Stop unnecessarily disabling CORB for extensions."

This reverts commit fd7db5d756, because
it has accidentally introduced a breaking change - before this CL
Chrome Extensions with appropriate host permissions were able to
read the body of cross-origin, no-cors responses.

Original change's description:
> Stop unnecessarily disabling CORB for extensions.
>
> After r897555 CORB only applies to `no-cors` requests.  This means that
> there is no need anymore to disable CORB when extensions need relaxed
> CORS.  Keeping CORB enabled will tighten what capabilities are exposed
> to renderers hosting extensions with no HTTP permissions (such
> extensions have no CORS exceptions enabled via
> SetCorsOriginAccessListForExtension and after this CL will be also
> unable to bypass CORB - this reduces what such a compromised renderer
> can do).

Bug: 1252173, 1016904
Change-Id: I794830cfb4590ee39c475b4145174cecf3c356ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3177465
Reviewed-by: K. Moon <kmoon@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#924498}

chrome/browser/extensions/corb_and_cors_extension_browsertest.cc

@@ -17,7 +17,6 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
-#include "chrome/browser/extensions/api/permissions/permissions_api.h"
 #include "chrome/browser/extensions/api/tabs/tabs_api.h"
 #include "chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
@@ -260,7 +259,8 @@ class CorbAndCorsExtensionBrowserTest : public CorbAndCorsExtensionTestBase {
                               "has been blocked by CORS policy")));
   }
 
-  void VerifyFetchWasBlockedByCorb(const base::HistogramTester& histograms) {
+  void VerifyFetchFromContentScriptWasBlockedByCorb(
+      const base::HistogramTester& histograms) {
     // Make sure that histograms logged in other processes (e.g. in
     // NetworkService process) get synced.
     metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
@@ -271,8 +271,9 @@ class CorbAndCorsExtensionBrowserTest : public CorbAndCorsExtensionTestBase {
                                  CORBAction::kBlockedWithoutSniffing, 1);
   }
 
-  void VerifyFetchWasAllowedByCorb(const base::HistogramTester& histograms,
-                                   bool expecting_sniffing = false) {
+  void VerifyFetchFromContentScriptWasAllowedByCorb(
+      const base::HistogramTester& histograms,
+      bool expecting_sniffing = false) {
     // Make sure that histograms logged in other processes (e.g. in
     // NetworkService process) get synced.
     metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
@@ -319,7 +320,8 @@ class CorbAndCorsExtensionBrowserTest : public CorbAndCorsExtensionTestBase {
       const std::string& actual_fetch_result,
       const std::string& expected_fetch_result_prefix) {
     // Verify that CORB allowed the response.
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
+    VerifyFetchFromContentScriptWasAllowedByCorb(
+        histograms, false /* expecting_sniffing */);
 
     // Verify that the response body was blocked by CORS.
     EXPECT_EQ(kCorsErrorWhenFetching, actual_fetch_result);
@@ -330,15 +332,6 @@ class CorbAndCorsExtensionBrowserTest : public CorbAndCorsExtensionTestBase {
     return browser()->tab_strip_model()->GetActiveWebContents();
   }
 
-  const Extension* InstallExtensionWithManifest(base::StringPiece manifest) {
-    dir_.WriteManifest(manifest);
-    dir_.WriteFile(FILE_PATH_LITERAL("background_script.js"), "");
-    dir_.WriteFile(FILE_PATH_LITERAL("page.html"), "");
-
-    extension_ = LoadExtension(dir_.UnpackedPath());
-    return extension_;
-  }
-
   const Extension* InstallExtensionWithPermissionToAllUrls(
       bool enable_file_access = false) {
     const char kManifestTemplate[] = R"(
@@ -725,11 +718,10 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
     content::ExecuteScriptAsync(active_web_contents(), kFetchInitiatingScript);
     std::string fetch_result = PopString(&queue);
 
-    // Verify that the fetch was blocked by CORS.  (CORB only applies to
-    // `no-cors` requests.)
     EXPECT_EQ(kCorsErrorWhenFetching, fetch_result);
+    VerifyFetchFromContentScriptWasAllowedByCorb(
+        histograms, false /* expecting_sniffing */);
     VerifyFetchWasBlockedByCors(console_observer);
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
   }
 }
 
@@ -781,11 +773,11 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
     std::string fetch_result =
         FetchViaContentScript(cross_site_resource, active_web_contents());
 
-    // Verify that the fetch was blocked by CORS.  (CORB only applies to
-    // `no-cors` requests.)
+    // Verify that the fetch was blocked by CORS.
     EXPECT_EQ(kCorsErrorWhenFetching, fetch_result);
+    VerifyFetchFromContentScriptWasAllowedByCorb(
+        histograms, false /* expecting_sniffing */);
     VerifyFetchWasBlockedByCors(console_observer);
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
   }
 }
 
@@ -839,7 +831,8 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
 
     // Verify that the fetch was blocked by CORS.
     EXPECT_EQ(kCorsErrorWhenFetching, fetch_result);
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
+    VerifyFetchFromContentScriptWasAllowedByCorb(
+        histograms, false /* expecting_sniffing */);
     VerifyFetchWasBlockedByCors(console_observer);
   }
 }
@@ -1065,7 +1058,7 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
   // Verify that the fetch succeeded (because of the server's
   // Access-Control-Allow-Origin response header).
   EXPECT_EQ("cors-ok.txt - body\n", fetch_result);
-  VerifyFetchWasAllowedByCorb(histograms);
+  VerifyFetchFromContentScriptWasAllowedByCorb(histograms);
 }
 
 // Test that verifies that CORS blocks non-CORB-eligible fetches for targets
@@ -1100,7 +1093,8 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
 
   // Verify that the fetch was allowed by CORB (because CORB doesn't apply to
   // CORS requests).
-  VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
+  VerifyFetchFromContentScriptWasAllowedByCorb(histograms,
+                                               false /* expecting_sniffing */);
 }
 
 // Tests that same-origin fetches (same-origin relative to the webpage the
@@ -1129,7 +1123,8 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
 
   // Verify that no blocking occurred.
   EXPECT_THAT(fetch_result, ::testing::StartsWith("nosniff.xml - body"));
-  VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
+  VerifyFetchFromContentScriptWasAllowedByCorb(histograms,
+                                               false /* expecting_sniffing */);
 }
 
 // Test that responses that would have been allowed by CORB anyway are not
@@ -1345,7 +1340,7 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
                                            "" /* expected_response_body */);
 }
 
-// Test that requests from an extension background page use relaxed CORB
+// Test that requests from an extension background page use relaxed CORS
 // processing.
 IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
                        FromBackgroundPage_NoSniffXml) {
@@ -1357,18 +1352,12 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
   EXPECT_FALSE(IncognitoInfo::IsSplitMode(extension()));
 
   // Performs a cross-origin fetch from the background page.
-  {
-    base::HistogramTester histograms;
-    GURL cross_site_resource(
-        embedded_test_server()->GetURL("cross-site.com", "/nosniff.xml"));
-    std::string fetch_result = FetchViaBackgroundPage(cross_site_resource);
+  GURL cross_site_resource(
+      embedded_test_server()->GetURL("cross-site.com", "/nosniff.xml"));
+  std::string fetch_result = FetchViaBackgroundPage(cross_site_resource);
 
-    // Verify that CORS did *not* block the response.
-    EXPECT_EQ("nosniff.xml - body\n", fetch_result);
-
-    // CORB only applies to `no-cors` requests.
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
-  }
+  // Verify that no blocking occurred.
+  EXPECT_EQ("nosniff.xml - body\n", fetch_result);
 }
 
 // Test that requests from an extension background page use relaxed CORB
@@ -1439,148 +1428,6 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
   }
 }
 
-// Test that CORB+CORS are enforced for extensions with no permissions to
-// http/https origins.
-IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
-                       ExtensionWithNoHttpPermissions) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  // "permission" entry in the manifest below mimics the PDF extension which
-  // has no permissions to http/https origins (and therefore doesn't care
-  // about relaxed CORB and/or CORS).
-  const char kManifest[] = R"(
-      {
-        "name": "CrossOriginReadBlockingTest - Extension",
-        "version": "1.0",
-        "manifest_version": 2,
-        "permissions": [ "contentSettings" ],
-        "background": {"scripts": ["background_script.js"]}
-      } )";
-  ASSERT_TRUE(InstallExtensionWithManifest(kManifest));
-
-  // Perform a cross-origin CORS fetch from the background page.
-  {
-    base::HistogramTester histograms;
-    GURL cross_site_resource1(
-        embedded_test_server()->GetURL("cross-site.com", "/nosniff.empty"));
-    std::string fetch_result = FetchViaBackgroundPage(cross_site_resource1);
-
-    // Verify that the fetch was blocked by CORS.
-    //
-    // This behavior verification is a bit important, but here it mostly tests
-    // the test setup, rather than the behavior this test was created for.
-    EXPECT_EQ(kCorsErrorWhenFetching, fetch_result);
-
-    // CORB only applies to `no-cors` requests.
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
-  }
-
-  // Perform a cross-origin `no-cors` request from the background page.
-  {
-    // Use a slightly different URL to avoid having to think what effect the
-    // network cache might have on test results.
-    GURL cross_site_resource2(
-        embedded_test_server()->GetURL("cross-site.com", "/nosniff.xml"));
-
-    base::Value request_init(base::Value::Type::DICTIONARY);
-    request_init.SetStringPath("method", "GET");
-    request_init.SetStringPath("mode", "no-cors");
-
-    content::WebContents* background_web_contents =
-        ProcessManager::Get(browser()->profile())
-            ->GetBackgroundHostForExtension(extension()->id())
-            ->host_contents();
-
-    base::HistogramTester histograms;
-    content::DOMMessageQueue queue;
-    content::ExecuteScriptAsync(
-        background_web_contents,
-        CreateFetchScript(cross_site_resource2, std::move(request_init)));
-    std::string fetch_result = PopString(&queue);
-
-    // Verify that the fetch was blocked by CORB.
-    //
-    // This is the main verification in the test.  This verifies that the
-    // extension background page uses a URLLoaderFactory created with
-    // URLLoaderFactoryParams::is_corb_enabled set to the default, secure value
-    // of `true`.
-    EXPECT_EQ("", fetch_result);  // `no-cors` = empty, opaque response.
-    VerifyFetchWasBlockedByCorb(histograms);
-  }
-}
-
-// Test that CORB+CORS are enforced for extensions with optional permissions to
-// http/https origins.
-IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
-                       ExtensionWithOptionalHttpPermissions) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  // There are only *optional* HTTP permissions declared in the manifest below -
-  // they will be granted later, at runtime (but not at the extension
-  // installation time).
-  const char kManifest[] = R"(
-      {
-        "name": "CrossOriginReadBlockingTest - Extension",
-        "version": "1.0",
-        "manifest_version": 2,
-        "optional_permissions": [ "<all_urls>", "http://example.com/" ],
-        "background": {"scripts": ["background_script.js"]}
-      } )";
-  ASSERT_TRUE(InstallExtensionWithManifest(kManifest));
-
-  // Navigate a tab to the extension origin.
-  GURL extension_resource = GetExtensionResource("page.html");
-  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), extension_resource));
-  content::RenderFrameHost* test_frame =
-      browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
-  ASSERT_EQ(GetExtensionOrigin(), test_frame->GetLastCommittedOrigin());
-
-  // Perform a cross-origin fetch from an extension frame and verify that it got
-  // blocked by CORS.
-  GURL cross_site_url(
-      embedded_test_server()->GetURL("cross-site.com", "/nosniff.xml"));
-  url::Origin cross_site_origin = url::Origin::Create(cross_site_url);
-  {
-    base::HistogramTester histograms;
-    std::string fetch_result = FetchViaFrame(cross_site_url, test_frame);
-
-    // Verify that the fetch was blocked by CORS.  (CORB only applies to
-    // `no-cors` requests.)
-    EXPECT_EQ(kCorsErrorWhenFetching, fetch_result);
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
-  }
-
-  // Grant the optional permissions to the extension.
-  {
-    PermissionsRequestFunction::SetAutoConfirmForTests(true);
-    const char kPermissionGrantingScriptTemplate[] = R"(
-        chrome.permissions.request(
-            { origins: [$1] },
-            granted => { domAutomationController.send(granted); });
-    )";
-    bool has_permission_been_granted = false;
-    std::string script = content::JsReplace(kPermissionGrantingScriptTemplate,
-                                            cross_site_origin.GetURL());
-    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-        test_frame, script, &has_permission_been_granted));
-    ASSERT_TRUE(has_permission_been_granted);
-    PermissionsRequestFunction::SetAutoConfirmForTests(false);
-  }
-
-  // Performs a cross-origin fetch from the background page and verify that it
-  // didn't get blocked by CORS.
-  {
-    base::HistogramTester histograms;
-    std::string fetch_result = FetchViaFrame(cross_site_url, test_frame);
-
-    // Verify that CORS did *not* block the response.
-    EXPECT_EQ("nosniff.xml - body\n", fetch_result);
-
-    // CORB only applies to `no-cors` requests.
-    VerifyFetchWasAllowedByCorb(histograms, false /* expecting_sniffing */);
-  }
-}
-
 // Test that requests from an extension page hosted in a foreground tab use
 // relaxed CORB processing.
 IN_PROC_BROWSER_TEST_F(CorbAndCorsExtensionBrowserTest,
@@ -2078,12 +1925,12 @@ IN_PROC_BROWSER_TEST_F(CorbAndCorsAppBrowserTest, WebViewContentScript) {
       } )";
   TestExtensionDir dir;
   dir.WriteManifest(kManifest);
-  const char kBackgroundScript[] = R"(
+  const char kBackgroungScript[] = R"(
       chrome.app.runtime.onLaunched.addListener(function() {
         chrome.app.window.create('page.html', {}, function () {});
       });
   )";
-  dir.WriteFile(FILE_PATH_LITERAL("background_script.js"), kBackgroundScript);
+  dir.WriteFile(FILE_PATH_LITERAL("background_script.js"), kBackgroungScript);
   const char kPage[] = R"(
       <div id="webview-tag-container"></div>
   )";

extensions/browser/url_loader_factory_manager.cc

@@ -35,34 +35,70 @@ enum class FactoryUser {
   kExtensionProcess,
 };
 
-bool DoExtensionPermissionsCoverHttpOrHttpsOrigins(const Extension& extension) {
-  // TODO(lukasza): https://crbug.com/1016904: Return false if the |extension|'s
-  // permissions do not actually cover http or https origins.  For now we
-  // conservatively return true so that *all* extensions get relaxed CORS
-  // treatment.
-  return true;
-}
-
-bool DoContentScriptsDependOnRelaxedCors(const Extension& extension) {
-  // Content scripts injected by Chrome Apps (e.g. into <webview> tag) may need
-  // to run with relaxed CORS.
+bool DoContentScriptsDependOnRelaxedCorbOrCors(const Extension& extension) {
+  // Content scripts injected by Chrome Apps (e.g. into <webview> tag) need to
+  // run with relaxed CORB.
   //
   // TODO(https://crbug.com/1152550): Remove this exception once Chrome Platform
   // Apps are gone.
   if (extension.is_platform_app())
-    return DoExtensionPermissionsCoverHttpOrHttpsOrigins(extension);
+    return true;
 
-  // Content scripts are not granted an ability to relax CORS.
+  // Content scripts are not granted an ability to relax CORB and/or CORS.
   return false;
 }
 
+bool DoExtensionPermissionsCoverHttpOrHttpsOrigins(const Extension& extension) {
+  // TODO(lukasza): https://crbug.com/1016904: Return false if the |extension|'s
+  // permissions do not actually cover http or https origins.  For now we
+  // conservatively return true so that *all* extensions get relaxed CORS/CORB
+  // treatment.
+  return true;
+}
+
+// Returns whether the default URLLoaderFactoryParams::is_corb_enabled should be
+// overridden and changed to false.
+bool ShouldDisableCorb(const Extension& extension, FactoryUser factory_user) {
+  if (!DoExtensionPermissionsCoverHttpOrHttpsOrigins(extension))
+    return false;
+
+  switch (factory_user) {
+    case FactoryUser::kContentScript:
+      return DoContentScriptsDependOnRelaxedCorbOrCors(extension);
+    case FactoryUser::kExtensionProcess:
+      return true;
+  }
+}
+
+// Returns whether URLLoaderFactoryParams::ignore_isolated_world_origin should
+// be overridden and changed to false.
+bool ShouldInspectIsolatedWorldOrigin(const Extension& extension,
+                                      FactoryUser factory_user) {
+  if (!DoExtensionPermissionsCoverHttpOrHttpsOrigins(extension))
+    return false;
+
+  switch (factory_user) {
+    case FactoryUser::kContentScript:
+      return DoContentScriptsDependOnRelaxedCorbOrCors(extension);
+    case FactoryUser::kExtensionProcess:
+      return false;
+  }
+}
+
+bool ShouldCreateSeparateFactoryForContentScripts(const Extension& extension) {
+  return ShouldDisableCorb(extension, FactoryUser::kContentScript) ||
+         ShouldInspectIsolatedWorldOrigin(extension,
+                                          FactoryUser::kContentScript);
+}
+
 void OverrideFactoryParams(const Extension& extension,
                            FactoryUser factory_user,
                            network::mojom::URLLoaderFactoryParams* params) {
-  if (factory_user == FactoryUser::kContentScript &&
-      DoContentScriptsDependOnRelaxedCors(extension)) {
+  if (ShouldDisableCorb(extension, factory_user))
+    params->is_corb_enabled = false;
+
+  if (ShouldInspectIsolatedWorldOrigin(extension, factory_user))
     params->ignore_isolated_world_origin = false;
-  }
 
   // TODO(lukasza): Do not override |unsafe_non_webby_initiator| unless
   // DoExtensionPermissionsCoverHttpOrHttpsOrigins(extension).
@@ -93,8 +129,7 @@ void URLLoaderFactoryManager::WillInjectContentScriptsWhenNavigationCommits(
 
   std::vector<url::Origin> initiators_requiring_separate_factory;
   for (const Extension* extension : extensions) {
-    // Do nothing if a separate URLLoaderFactory is not needed.
-    if (!DoContentScriptsDependOnRelaxedCors(*extension))
+    if (!ShouldCreateSeparateFactoryForContentScripts(*extension))
       continue;
 
     initiators_requiring_separate_factory.push_back(extension->origin());
@@ -117,8 +152,7 @@ void URLLoaderFactoryManager::WillProgrammaticallyInjectContentScript(
     base::PassKey<ContentScriptTracker> pass_key,
     content::RenderFrameHost* frame,
     const Extension& extension) {
-  // Do nothing if a separate URLLoaderFactory is not needed.
-  if (!DoContentScriptsDependOnRelaxedCors(extension))
+  if (!ShouldCreateSeparateFactoryForContentScripts(extension))
     return;
 
   // When WillExecuteCode runs, the frame already received the initial
@@ -145,7 +179,7 @@ void URLLoaderFactoryManager::OverrideURLLoaderFactoryParams(
 
   // Opaque origins normally don't inherit security properties of their
   // precursor origins, but here opaque origins (e.g. think data: URIs) created
-  // by an extension should inherit CORS treatment of the extension.
+  // by an extension should inherit CORS/CORB treatment of the extension.
   url::SchemeHostPort precursor_origin =
       origin.GetTupleOrPrecursorTupleIfOpaque();
 

extensions/browser/url_loader_factory_manager.h

@@ -27,7 +27,7 @@ namespace extensions {
 class ContentScriptTracker;
 
 // This class manages URLLoaderFactory objects that handle network requests that
-// require extension-specific permissions (related to relaxed CORS).
+// require extension-specific permissions (related to relaxed CORB and CORS).
 //
 // See also https://crbug.com/846346 for motivation for having separate
 // URLLoaderFactory objects for content scripts.
@@ -88,10 +88,9 @@ class URLLoaderFactoryManager {
   //                                 |           |             |
   // URLLoaderFactoryParams:         |           |             |
   // - request_initiator_origin_lock |    web    |  extension  |     web
-  // - overridden properties?:       |           |             |
-  //    is_corb_enabled              |    always secure-default = true/enabled
-  //    ignore_isolated_world_origin | sec-default=true/ignore | if needed (app)
-  //    unsafe_non_webby_initiator   |   false   |    true     |    false
+  // - overridden properties?        |    no     |     yes     |  if needed
+  //    - is_corb_enabled            | secure-   |  ext-based  | ext-based for
+  //    - ..._access_patterns        |  -default |             | platform apps
   static void OverrideURLLoaderFactoryParams(
       content::BrowserContext* browser_context,
       const url::Origin& origin,

pdf/ppapi_migration/url_loader.cc

@@ -345,7 +345,6 @@ void PepperUrlLoader::Open(const UrlRequest& request, ResultCallback callback) {
   pp::URLRequestInfo pp_request(plugin_instance_);
   pp_request.SetURL(request.url);
   pp_request.SetMethod(request.method);
-  pp_request.SetCustomReferrerURL(request.url);
 
   if (request.ignore_redirects)
     pp_request.SetFollowRedirects(false);