Remove extension_features::kBypassCorbOnlyForExtensionsAllowlist.

We no longer need to support running with CORB disabled for
*all* extensions:

1. The BypassCorbOnlyForExtensionsAllowlist feature has shipped in M73.

2. Distributing allowlisted extensions via field trial param does not
   apply to M77+ (according to the BypassCorbExtensionsAllowlist.gcl
   config).

After this CL it is no longer possible to force a separate
URLLoaderFactory (with disabled CORB) for *all* content scripts by
launching Chrome with
--disable-features=BypassCorbOnlyForExtensionsAllowlist

After this CL it is still possible to avoid separate content script
URLLoaderFactories by launching Chrome with
--force-empty-corb-allowlist

Bug: 846346
Change-Id: Ia1264b318dd8c389bf8cb58041dd503a4dca5b6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774889
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692251}

chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc

@@ -5,14 +5,12 @@
 #include "chrome/browser/extensions/extension_action_runner.h"
 
 #include "base/bind.h"
-#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/json/json_reader.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/apps/launch_service/launch_service.h"
@@ -33,12 +31,11 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "extensions/browser/browsertest_util.h"
-#include "extensions/common/extension_features.h"
+#include "extensions/browser/url_loader_factory_manager.h"
 #include "extensions/test/test_extension_dir.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "services/network/cross_origin_read_blocking.h"
-#include "services/network/public/cpp/features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -344,13 +341,9 @@ class CrossOriginReadBlockingExtensionTest : public ExtensionBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(CrossOriginReadBlockingExtensionTest);
 };
 
-struct AllowlistingParam {
-  AllowlistingParam(bool feature_enabled, bool test_extension_allowlisted)
-      : feature_enabled(feature_enabled),
-        test_extension_allowlisted(test_extension_allowlisted) {}
-
-  bool feature_enabled;
-  bool test_extension_allowlisted;
+enum class AllowlistingParam {
+  kAllowlisted,
+  kNotAllowlisted,
 };
 
 class CrossOriginReadBlockingExtensionAllowlistingTest
@@ -361,9 +354,8 @@ class CrossOriginReadBlockingExtensionAllowlistingTest
 
   CrossOriginReadBlockingExtensionAllowlistingTest() {}
 
-  bool IsAllowlistFeatureEnabled() { return GetParam().feature_enabled; }
   bool IsExtensionAllowlisted() {
-    return GetParam().test_extension_allowlisted;
+    return GetParam() == AllowlistingParam::kAllowlisted;
   }
 
   const Extension* InstallExtension(
@@ -371,30 +363,22 @@ class CrossOriginReadBlockingExtensionAllowlistingTest
     const Extension* extension = Base::InstallExtension(
         resource_to_fetch_from_declarative_content_script);
 
-    if (IsAllowlistFeatureEnabled()) {
-      if (IsExtensionAllowlisted()) {
-        scoped_feature_list_.InitAndEnableFeatureWithParameters(
-            extensions_features::kBypassCorbOnlyForExtensionsAllowlist,
-            {{extensions_features::kBypassCorbAllowlistParamName,
-              extension->hashed_id().value()}});
-      } else {
-        scoped_feature_list_.InitAndEnableFeature(
-            extensions_features::kBypassCorbOnlyForExtensionsAllowlist);
-      }
+    if (IsExtensionAllowlisted()) {
+      URLLoaderFactoryManager::AddExtensionToAllowlistForTesting(*extension);
     } else {
-      scoped_feature_list_.InitAndDisableFeature(
-          extensions_features::kBypassCorbOnlyForExtensionsAllowlist);
+      URLLoaderFactoryManager::RemoveExtensionFromAllowlistForTesting(
+          *extension);
     }
 
     return extension;
   }
 
   bool AreContentScriptFetchesExpectedToBeBlocked() {
-    return IsAllowlistFeatureEnabled() && !IsExtensionAllowlisted();
+    return !IsExtensionAllowlisted();
   }
 
   bool IsCorbExpectedToBeTurnedOffAltogether() {
-    return (IsExtensionAllowlisted() || !IsAllowlistFeatureEnabled());
+    return IsExtensionAllowlisted();
   }
 
   void VerifyFetchFromContentScriptWasBlocked(
@@ -454,8 +438,6 @@ class CrossOriginReadBlockingExtensionAllowlistingTest
   }
 
  private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-
   DISALLOW_COPY_AND_ASSIGN(CrossOriginReadBlockingExtensionAllowlistingTest);
 };
 
@@ -1225,14 +1207,11 @@ IN_PROC_BROWSER_TEST_P(CrossOriginReadBlockingExtensionAllowlistingTest,
               ::testing::Not(::testing::HasSubstr("Origin: chrome-extension")));
 }
 
-INSTANTIATE_TEST_SUITE_P(AllowlistingDisabled,
-                         CrossOriginReadBlockingExtensionAllowlistingTest,
-                         ::testing::Values(AllowlistingParam(false, false)));
 INSTANTIATE_TEST_SUITE_P(Allowlisted,
                          CrossOriginReadBlockingExtensionAllowlistingTest,
-                         ::testing::Values(AllowlistingParam(true, true)));
+                         ::testing::Values(AllowlistingParam::kAllowlisted));
 INSTANTIATE_TEST_SUITE_P(NotAllowlisted,
                          CrossOriginReadBlockingExtensionAllowlistingTest,
-                         ::testing::Values(AllowlistingParam(true, false)));
+                         ::testing::Values(AllowlistingParam::kNotAllowlisted));
 
 }  // namespace extensions

extensions/browser/url_loader_factory_manager.cc

@@ -252,43 +252,20 @@ std::vector<std::string> CreateExtensionAllowlist() {
     return allowlist;
   }
 
-  // Make sure kHardcodedPartOfAllowlist will fit, but also leave some room
-  // for field trial params.
-  allowlist.reserve(base::size(kHardcodedPartOfCorbAllowlist) + 10);
-
   // Append extensions from the hardcoded allowlist.
+  allowlist.reserve(base::size(kHardcodedPartOfCorbAllowlist));
   for (const char* hash : kHardcodedPartOfCorbAllowlist) {
     DCHECK(IsValidHashedExtensionId(hash));  // It also validates the length.
     allowlist.push_back(std::string(hash, kHashedExtensionIdLength));
   }
 
-  // Append extensions from the field trial param.
-  std::string field_trial_arg = base::GetFieldTrialParamValueByFeature(
-      extensions_features::kBypassCorbOnlyForExtensionsAllowlist,
-      extensions_features::kBypassCorbAllowlistParamName);
-  field_trial_arg = base::ToUpperASCII(field_trial_arg);
-  std::vector<std::string> field_trial_allowlist = base::SplitString(
-      field_trial_arg, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
-  base::EraseIf(field_trial_allowlist, [](const std::string& hash) {
-    // Filter out invalid data from |field_trial_allowlist|.
-    if (IsValidHashedExtensionId(hash))
-      return false;  // Don't remove.
-
-    LOG(ERROR) << "Invalid extension hash: " << hash;
-    return true;  // Remove.
-  });
-  std::move(field_trial_allowlist.begin(), field_trial_allowlist.end(),
-            std::back_inserter(allowlist));
-
   return allowlist;
 }
 
 // Returns a set of HashedExtensionId of extensions that depend on relaxed CORB
 // behavior in their content scripts.
-const base::flat_set<std::string>& GetExtensionsAllowlist() {
-  DCHECK(base::FeatureList::IsEnabled(
-      extensions_features::kBypassCorbOnlyForExtensionsAllowlist));
-  static const base::NoDestructor<base::flat_set<std::string>> s_allowlist([] {
+base::flat_set<std::string>& GetExtensionsAllowlist() {
+  static base::NoDestructor<base::flat_set<std::string>> s_allowlist([] {
     base::flat_set<std::string> result(CreateExtensionAllowlist());
     result.shrink_to_fit();
     return result;
@@ -305,10 +282,6 @@ bool DoContentScriptsDependOnRelaxedCorb(const Extension& extension) {
   // Content scripts in the current version of extensions might depend on
   // relaxed CORB.
   if (extension.manifest_version() <= 2) {
-    if (!base::FeatureList::IsEnabled(
-            extensions_features::kBypassCorbOnlyForExtensionsAllowlist))
-      return true;
-
     const std::string& hash = extension.hashed_id().value();
     DCHECK(IsValidHashedExtensionId(hash));
     return base::Contains(GetExtensionsAllowlist(), hash);
@@ -589,4 +562,16 @@ network::mojom::URLLoaderFactoryPtrInfo URLLoaderFactoryManager::CreateFactory(
                                 *extension);
 }
 
+// static
+void URLLoaderFactoryManager::AddExtensionToAllowlistForTesting(
+    const Extension& extension) {
+  GetExtensionsAllowlist().insert(extension.hashed_id().value());
+}
+
+// static
+void URLLoaderFactoryManager::RemoveExtensionFromAllowlistForTesting(
+    const Extension& extension) {
+  GetExtensionsAllowlist().erase(extension.hashed_id().value());
+}
+
 }  // namespace extensions

extensions/browser/url_loader_factory_manager.h

@@ -69,6 +69,10 @@ class URLLoaderFactoryManager {
           header_client,
       const url::Origin& initiator_origin);
 
+  static void AddExtensionToAllowlistForTesting(const Extension& extension);
+  static void RemoveExtensionFromAllowlistForTesting(
+      const Extension& extension);
+
  private:
   // If |extension|'s manifest declares that it may inject JavaScript content
   // script into the |navigating_frame| / |navigation_target|, then

extensions/common/extension_features.cc

@@ -6,14 +6,6 @@
 
 namespace extensions_features {
 
-// Enables enforcement of Cross-Origin Read Blocking (CORB) for most extension
-// content scripts, except ones that are on an allowlist.  See also
-// https://crbug.com/846346 and DoContentScriptsDependOnRelaxedCorb function in
-// extensions/browser/url_loader_factory_manager.cc.
-const base::Feature kBypassCorbOnlyForExtensionsAllowlist{
-    "BypassCorbOnlyForExtensionsAllowlist", base::FEATURE_ENABLED_BY_DEFAULT};
-const char kBypassCorbAllowlistParamName[] = "BypassCorbExtensionsAllowlist";
-
 // Enables new extension updater service.
 const base::Feature kNewExtensionUpdaterService{
     "NewExtensionUpdaterService", base::FEATURE_DISABLED_BY_DEFAULT};

extensions/common/extension_features.h

@@ -9,8 +9,6 @@
 
 namespace extensions_features {
 
-extern const base::Feature kBypassCorbOnlyForExtensionsAllowlist;
-extern const char kBypassCorbAllowlistParamName[];
 extern const base::Feature kNewExtensionUpdaterService;
 extern const base::Feature kForceWebRequestProxyForTest;