[Permissions Policy] Deprecate comma separator in allow attribute

This CL removes the comma separator support in allow attribute
parsing.

Bug: 1062400
Change-Id: Ib1c481b492903384a0413f221474ec7703e6f0df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600388
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842682}

third_party/blink/public/mojom/web_feature/web_feature.mojom

@@ -2679,7 +2679,7 @@ enum WebFeature {
   kV8PointerEvent_AltitudeAngle_AttributeGetter = 3352,
   kCrossBrowsingContextGroupMainFrameNulledNonEmptyNameAccessed = 3353,
   kPositionSticky = 3354,
-  kCommaSeparatorInAllowAttribute = 3355,
+  kOBSOLETE_CommaSeparatorInAllowAttribute = 3355,
   kMainFrameCSPViaHTTP = 3359,
   kMainFrameCSPViaMeta = 3360,
   kMainFrameCSPViaOriginPolicy = 3361,

third_party/blink/renderer/core/feature_policy/feature_policy_parser.cc

@@ -407,12 +407,19 @@ ParsingContext::FeaturePolicyNode ParsingContext::ParseFeaturePolicyToIR(
     return {};
   }
 
-  // RFC2616, section 4.2 specifies that headers appearing multiple times can be
-  // combined with a comma. Walk the header string, and parse each comma
-  // separated chunk as a separate header.
   Vector<String> policy_items;
-  // policy_items = [ policy *( "," [ policy ] ) ]
-  policy.Split(',', policy_items);
+
+  if (src_origin_) {
+    // Attribute parsing.
+    policy_items.push_back(policy);
+  } else {
+    // Header parsing.
+    // RFC2616, section 4.2 specifies that headers appearing multiple times can
+    // be combined with a comma. Walk the header string, and parse each comma
+    // separated chunk as a separate header.
+    // policy_items = [ policy *( "," [ policy ] ) ]
+    policy.Split(',', policy_items);
+  }
 
   if (policy_items.size() > 1) {
     UseCounter::Count(

third_party/blink/renderer/core/feature_policy/feature_policy_test.cc

@@ -34,7 +34,7 @@ namespace blink {
 
 namespace {
 
-const char* const kValidPolicies[] = {
+const char* const kValidHeaderPolicies[] = {
     "",      // An empty policy.
     " ",     // An empty policy.
     ";;",    // Empty policies.
@@ -44,8 +44,7 @@ const char* const kValidPolicies[] = {
     ",;,",   // Empty policies.
     "geolocation 'none'",
     "geolocation 'self'",
-    "geolocation 'src'",  // Only valid for iframe allow attribute.
-    "geolocation",        // Only valid for iframe allow attribute.
+    "geolocation",
     "geolocation; fullscreen; payment",
     "geolocation *",
     "geolocation " ORIGIN_A "",
@@ -58,10 +57,11 @@ const char* const kValidPolicies[] = {
     "fullscreen  " ORIGIN_A "; payment 'self'",
     "fullscreen " ORIGIN_A "; payment *, geolocation 'self'"};
 
-const char* const kInvalidPolicies[] = {
+const char* const kInvalidHeaderPolicies[] = {
     "badfeaturename",
     "badfeaturename 'self'",
     "1.0",
+    "geolocation 'src'",  // Only valid for iframe allow attribute.
     "geolocation data://badorigin",
     "geolocation https://bad;origin",
     "geolocation https:/bad,origin",
@@ -301,7 +301,7 @@ const FeaturePolicyParserTestCase FeaturePolicyParserParsingTest::kCases[] = {
         /* test_name */ "MultiplePoliciesIncludingBadFeatureName",
         /* feature_policy_string */
         "geolocation * " ORIGIN_B "; "
-        "fullscreen " ORIGIN_B " bad_feature_name " ORIGIN_C ","
+        "fullscreen " ORIGIN_B " bad_feature_name " ORIGIN_C ";"
         "payment 'self' badorigin",
         /* permissions_policy_string */
         "geolocation=(* \"" ORIGIN_B "\"),"
@@ -680,23 +680,50 @@ TEST_F(FeaturePolicyParserParsingTest,
       });
 }
 
-TEST_F(FeaturePolicyParserTest, ParseValidPolicy) {
-  for (const char* policy_string : kValidPolicies) {
+TEST_F(FeaturePolicyParserParsingTest, CommaSeparatorInAttribute) {
+  PolicyParserMessageBuffer logger;
+
+  CheckParsedPolicy(
+      FeaturePolicyParser::ParseAttribute(
+          "geolocation 'none', fullscreen 'self'",
+          /* self_origin */ origin_a_.get(),
+          /* src_origin */ origin_a_.get(), logger, /* context */ nullptr),
+      {
+          {
+              mojom::blink::FeaturePolicyFeature::kGeolocation,
+              /* matches_all_origins */ false,
+              /* matches_opaque_src */ false,
+              {ORIGIN_A},
+          },
+      });
+
+  EXPECT_EQ(logger.GetMessages().size(), 2u)
+      << "Parser should report parsing error.";
+
+  EXPECT_EQ(logger.GetMessages().front().content.Ascii(),
+            "Unrecognized origin: ''none','.")
+      << "\"'none',\" should be treated as an invalid allowlist item ";
+
+  EXPECT_EQ(logger.GetMessages().back().content.Ascii(),
+            "Unrecognized origin: 'fullscreen'.")
+      << "\"fullscreen\" should be treated as an invalid allowlist item";
+}
+
+TEST_F(FeaturePolicyParserTest, ParseValidHeaderPolicy) {
+  for (const char* policy_string : kValidHeaderPolicies) {
     PolicyParserMessageBuffer logger;
     FeaturePolicyParser::ParseFeaturePolicyForTest(
-        policy_string, origin_a_.get(), origin_b_.get(), logger,
-        test_feature_name_map);
+        policy_string, origin_a_.get(), nullptr, logger, test_feature_name_map);
     EXPECT_EQ(0UL, logger.GetMessages().size())
         << "Should parse " << policy_string;
   }
 }
 
-TEST_F(FeaturePolicyParserTest, ParseInvalidPolicy) {
-  for (const char* policy_string : kInvalidPolicies) {
+TEST_F(FeaturePolicyParserTest, ParseInvalidHeaderPolicy) {
+  for (const char* policy_string : kInvalidHeaderPolicies) {
     PolicyParserMessageBuffer logger;
     FeaturePolicyParser::ParseFeaturePolicyForTest(
-        policy_string, origin_a_.get(), origin_b_.get(), logger,
-        test_feature_name_map);
+        policy_string, origin_a_.get(), nullptr, logger, test_feature_name_map);
     EXPECT_LT(0UL, logger.GetMessages().size())
         << "Should fail to parse " << policy_string;
   }
@@ -891,26 +918,18 @@ class FeaturePolicyAllowlistHistogramTest
 
 const AllowlistHistogramData FeaturePolicyAllowlistHistogramTest::kCases[] = {
     {"Empty", "fullscreen", 1, {FeaturePolicyAllowlistType::kEmpty}},
-    {"Empty_MultipleDirectivesComma",
-     "fullscreen, geolocation, payment",
-     1,
-     {FeaturePolicyAllowlistType::kEmpty}},
     {"Empty_MultipleDirectivesSemicolon",
      "fullscreen; payment",
      1,
      {FeaturePolicyAllowlistType::kEmpty}},
     {"Star", "fullscreen *", 1, {FeaturePolicyAllowlistType::kStar}},
-    {"Star_MultipleDirectivesComma",
-     "fullscreen *, payment *",
-     1,
-     {FeaturePolicyAllowlistType::kStar}},
     {"Star_MultipleDirectivesSemicolon",
      "fullscreen *; payment *",
      1,
      {FeaturePolicyAllowlistType::kStar}},
     {"Self", "fullscreen 'self'", 1, {FeaturePolicyAllowlistType::kSelf}},
     {"Self_MultipleDirectives",
-     "fullscreen 'self', geolocation 'self', payment 'self'",
+     "fullscreen 'self'; geolocation 'self'; payment 'self'",
      1,
      {FeaturePolicyAllowlistType::kSelf}},
     {"None", "fullscreen 'none'", 1, {FeaturePolicyAllowlistType::kNone}},
@@ -922,10 +941,6 @@ const AllowlistHistogramData FeaturePolicyAllowlistHistogramTest::kCases[] = {
      "fullscreen " ORIGIN_A,
      1,
      {FeaturePolicyAllowlistType::kOrigins}},
-    {"Origins_MultipleDirectivesComma",
-     "fullscreen " ORIGIN_A ", payment " ORIGIN_A,
-     1,
-     {FeaturePolicyAllowlistType::kOrigins}},
     {"Origins_MultipleDirectivesSemicolon",
      "fullscreen " ORIGIN_A "; payment " ORIGIN_A " " ORIGIN_B,
      1,
@@ -935,7 +950,7 @@ const AllowlistHistogramData FeaturePolicyAllowlistHistogramTest::kCases[] = {
      1,
      {FeaturePolicyAllowlistType::kMixed}},
     {"Mixed_MultipleDirectives",
-     "fullscreen 'self' " ORIGIN_A ", payment 'none' " ORIGIN_A " " ORIGIN_B,
+     "fullscreen 'self' " ORIGIN_A "; payment 'none' " ORIGIN_A " " ORIGIN_B,
      1,
      {FeaturePolicyAllowlistType::kMixed}},
     {"KeywordsOnly",
@@ -954,11 +969,7 @@ const AllowlistHistogramData FeaturePolicyAllowlistHistogramTest::kCases[] = {
      "fullscreen *; geolocation 'none' " ORIGIN_A,
      2,
      {FeaturePolicyAllowlistType::kStar, FeaturePolicyAllowlistType::kMixed}},
-    {"MultipleDirectives_SeparateTypes_Comma",
-     "fullscreen *, geolocation 'none', payment " ORIGIN_A " " ORIGIN_B,
-     3,
-     {FeaturePolicyAllowlistType::kStar, FeaturePolicyAllowlistType::kNone,
-      FeaturePolicyAllowlistType::kOrigins}}};
+};
 
 INSTANTIATE_TEST_SUITE_P(
     All,

third_party/blink/renderer/core/frame/deprecation.cc

@@ -540,11 +540,6 @@ DeprecationInfo GetDeprecationInfo(WebFeature feature) {
                   "RTCConfiguration.encodedInsertableStreams", kM88,
                   "6321945865879552")};
 
-    case WebFeature::kCommaSeparatorInAllowAttribute:
-      return {"CommaSeparatorInAllowAttribute", kM89,
-              ReplacedWillBeRemoved("Comma separator in iframe allow attribute",
-                                    "semicolons", kM89, "5740835259809792")};
-
     case WebFeature::kRTCConstraintEnableRtpDataChannelsTrue:
       return {"RTP data channel", kM88,
               ReplacedWillBeRemoved("RTP data channels",

third_party/blink/renderer/core/html/html_iframe_element.cc

@@ -39,7 +39,6 @@
 #include "third_party/blink/renderer/core/feature_policy/iframe_policy.h"
 #include "third_party/blink/renderer/core/fetch/trust_token_issuance_authorization.h"
 #include "third_party/blink/renderer/core/frame/csp/content_security_policy.h"
-#include "third_party/blink/renderer/core/frame/deprecation.h"
 #include "third_party/blink/renderer/core/html/html_document.h"
 #include "third_party/blink/renderer/core/html/trust_token_attribute_parsing.h"
 #include "third_party/blink/renderer/core/html_names.h"
@@ -229,11 +228,6 @@ void HTMLIFrameElement::ParseAttribute(
         UseCounter::Count(GetDocument(),
                           WebFeature::kFeaturePolicyAllowAttribute);
       }
-      if (value.Contains(',')) {
-        Deprecation::CountDeprecation(
-            GetDocument().GetExecutionContext(),
-            WebFeature::kCommaSeparatorInAllowAttribute);
-      }
     }
   } else if (name == html_names::kDisallowdocumentaccessAttr &&
              RuntimeEnabledFeatures::DisallowDocumentAccessEnabled()) {

third_party/blink/renderer/core/html/html_iframe_element_test.cc

@@ -324,26 +324,4 @@ TEST_F(HTMLIFrameElementSimTest, AllowAttributeParsingError) {
       << ConsoleMessages().front();
 }
 
-TEST_F(HTMLIFrameElementSimTest, CommaSeparatorIsDeprecated) {
-  EXPECT_FALSE(
-      GetDocument().Loader()->GetUseCounterHelper().HasRecordedMeasurement(
-          WebFeature::kCommaSeparatorInAllowAttribute));
-  SimRequest main_resource("https://example.com", "text/html");
-  LoadURL("https://example.com");
-  main_resource.Complete(R"(
-    <iframe
-      allow="fullscreen, geolocation"></iframe>
-  )");
-
-  EXPECT_EQ(ConsoleMessages().size(), 1u)
-      << "Comma separator in allow attribute should log a deprecation message "
-         "to the console.";
-  EXPECT_TRUE(ConsoleMessages().front().Contains("5740835259809792"))
-      << "Console message should mention the chromestatus entry.";
-
-  EXPECT_TRUE(
-      GetDocument().Loader()->GetUseCounterHelper().HasRecordedMeasurement(
-          WebFeature::kCommaSeparatorInAllowAttribute));
-}
-
 }  // namespace blink

tools/metrics/histograms/enums.xml

@@ -30444,7 +30444,7 @@ Called by update_use_counter_feature_enum.py.-->
   <int value="3353"
       label="CrossBrowsingContextGroupMainFrameNulledNonEmptyNameAccessed"/>
   <int value="3354" label="PositionSticky"/>
-  <int value="3355" label="CommaSeparatorInAllowAttribute"/>
+  <int value="3355" label="OBSOLETE_CommaSeparatorInAllowAttribute"/>
   <int value="3359" label="MainFrameCSPViaHTTP"/>
   <int value="3360" label="MainFrameCSPViaMeta"/>
   <int value="3361" label="MainFrameCSPViaOriginPolicy"/>