CORS: remove a flag to disable ABNF based A-C-A-* header checks

Now the ABNF based checks are fully enabled on the stable.
This is the clean-up change to remove a feature flag.

Bug: 1114019
Change-Id: Id8fde776ed7ae5e6fa0d4d1afc55e20695bfabf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2387461
Auto-Submit: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806618}

services/network/public/cpp/cors/preflight_result.cc

@@ -4,7 +4,6 @@
 
 #include "services/network/public/cpp/cors/preflight_result.h"
 
-#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ -15,7 +14,6 @@
 #include "net/http/http_request_headers.h"
 #include "net/http/http_util.h"
 #include "services/network/public/cpp/cors/cors.h"
-#include "services/network/public/cpp/features.h"
 
 namespace network {
 
@@ -61,12 +59,8 @@ bool ParseAccessControlMaxAge(const base::Optional<std::string>& max_age,
 }
 
 // Parses |string| as a Access-Control-Allow-* header value, storing the result
-// in |set|.
-//
-// If the |kStrictAccessControlAllowListCheck| feature is enabled,
-// this function returns false when |string| does not satisfy the syntax
-// here: https://fetch.spec.whatwg.org/#http-new-header-syntax.
-// The function always succeeds if the feature is disabled.
+// in |set|. This function returns false when |string| does not satisfy the
+// syntax here: https://fetch.spec.whatwg.org/#http-new-header-syntax.
 bool ParseAccessControlAllowList(const base::Optional<std::string>& string,
                                  base::flat_set<std::string>* set,
                                  bool insert_in_lower_case) {
@@ -75,13 +69,10 @@ bool ParseAccessControlAllowList(const base::Optional<std::string>& string,
   if (!string)
     return true;
 
-  const bool enable_strict_check = base::FeatureList::IsEnabled(
-      features::kStrictAccessControlAllowListCheck);
-
   net::HttpUtil::ValuesIterator it(string->begin(), string->end(), ',', true);
   while (it.GetNext()) {
     base::StringPiece value = it.value_piece();
-    if (enable_strict_check && !net::HttpUtil::IsToken(value)) {
+    if (!net::HttpUtil::IsToken(value)) {
       set->clear();
       return false;
     }

services/network/public/cpp/cors/preflight_result_unittest.cc

@@ -4,12 +4,9 @@
 
 #include "services/network/public/cpp/cors/preflight_result.h"
 
-#include "base/feature_list.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/time/time.h"
 #include "net/http/http_request_headers.h"
-#include "services/network/public/cpp/features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace network {
@@ -291,11 +288,7 @@ const ParseAccessListTestCase kParseMethodsCases[] = {
     {"", {}, kNoError},
     {", GET, POST, ,", {"GET", "POST"}, kNoError}};
 
-TEST_F(PreflightResultTest, ParseAllowControlAllowHeadersStrict) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      features::kStrictAccessControlAllowListCheck);
-
+TEST_F(PreflightResultTest, ParseAllowControlAllowHeaders) {
   for (const auto& test : kParseHeadersCases) {
     base::Optional<mojom::CorsError> error;
     std::unique_ptr<PreflightResult> result = PreflightResult::Create(
@@ -314,32 +307,7 @@ TEST_F(PreflightResultTest, ParseAllowControlAllowHeadersStrict) {
   }
 }
 
-TEST_F(PreflightResultTest, ParseAllowControlAllowHeadersLax) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(
-      features::kStrictAccessControlAllowListCheck);
-
-  for (const auto& test : kParseHeadersCases) {
-    base::Optional<mojom::CorsError> error;
-    std::unique_ptr<PreflightResult> result = PreflightResult::Create(
-        mojom::CredentialsMode::kOmit, /*allow_methods_header=*/base::nullopt,
-        test.input, /*max_age_header=*/base::nullopt, &error);
-    EXPECT_EQ(error, kNoError);
-
-    for (const auto& request_header : test.values_to_be_accepted) {
-      net::HttpRequestHeaders headers;
-      headers.AddHeadersFromString(request_header);
-      EXPECT_EQ(base::nullopt,
-                result->EnsureAllowedCrossOriginHeaders(headers, false));
-    }
-  }
-}
-
-TEST_F(PreflightResultTest, ParseAllowControlAllowMethodsStrict) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeature(
-      features::kStrictAccessControlAllowListCheck);
-
+TEST_F(PreflightResultTest, ParseAllowControlAllowMethods) {
   for (const auto& test : kParseMethodsCases) {
     base::Optional<mojom::CorsError> error;
     std::unique_ptr<PreflightResult> result =
@@ -357,26 +325,6 @@ TEST_F(PreflightResultTest, ParseAllowControlAllowMethodsStrict) {
   }
 }
 
-TEST_F(PreflightResultTest, ParseAllowControlAllowMethodsLax) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(
-      features::kStrictAccessControlAllowListCheck);
-
-  for (const auto& test : kParseMethodsCases) {
-    base::Optional<mojom::CorsError> error;
-    std::unique_ptr<PreflightResult> result =
-        PreflightResult::Create(mojom::CredentialsMode::kOmit, test.input,
-                                /*allow_headers_header=*/base::nullopt,
-                                /*max_age_header=*/base::nullopt, &error);
-    EXPECT_EQ(error, kNoError);
-
-    for (const auto& request_method : test.values_to_be_accepted) {
-      EXPECT_EQ(base::nullopt,
-                result->EnsureAllowedCrossOriginMethod(request_method));
-    }
-  }
-}
-
 }  // namespace
 
 }  // namespace cors

services/network/public/cpp/features.cc

@@ -188,13 +188,6 @@ const base::Feature kRequestInitiatorSiteLockEnfocement = {
     base::FEATURE_ENABLED_BY_DEFAULT};
 #endif
 
-// The preflight parser should reject Access-Control-Allow-* headers which do
-// not conform to ABNF. But if the strict check is applied directly, some
-// existing sites might fail to load. The feature flag controls whether a strict
-// check will be used or not.
-const base::Feature kStrictAccessControlAllowListCheck = {
-    "StrictAccessControlAllowListCheck", base::FEATURE_ENABLED_BY_DEFAULT};
-
 // When the CertVerifierService is enabled, certificate verification will not be
 // performed in the network service, but will instead be brokered to a separate
 // cert verification service potentially running in a different process.

services/network/public/cpp/features.h

@@ -74,8 +74,6 @@ extern const char kCorbAllowlistAlsoAppliesToOorCorsParamName[];
 COMPONENT_EXPORT(NETWORK_CPP)
 extern const base::Feature kRequestInitiatorSiteLockEnfocement;
 COMPONENT_EXPORT(NETWORK_CPP)
-extern const base::Feature kStrictAccessControlAllowListCheck;
-COMPONENT_EXPORT(NETWORK_CPP)
 extern const base::Feature kCertVerifierService;
 
 COMPONENT_EXPORT(NETWORK_CPP)