[Permissions-Policy] Use different error message prefix for Permissions-Policy header

Previously parse of 'Permissions-Policy' HTTP header still reports
parsing error with 'Error with Feature Policy: '.

This CL lets parse of 'Permissions-Policy' HTTP header use a separate
logger that has prefix 'Error with Permissions Policy: '.

Note:
Previously merge of feature policy and permissions policy was implicit,
i.e. done by concatenating the two together and relying on the parsing
rule that first occurrence of feature wins.
This CL makes the merge explicit in |FeaturePolicyParser::ParseHeader|.

Change-Id: I25f877899aa709705fb96b6a6cfe003dedd217d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2514961Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Charlie Hu <chenleihu@google.com>
Cr-Commit-Position: refs/heads/master@{#826127}

third_party/blink/renderer/core/execution_context/security_context_init.cc

@@ -148,7 +148,12 @@ void SecurityContextInit::ApplyFeaturePolicy(
   PolicyParserMessageBuffer feature_policy_logger(
       "Error with Feature-Policy header: ");
   PolicyParserMessageBuffer report_only_feature_policy_logger(
-      "Error with Report-Only-Feature-Policy header: ");
+      "Error with Feature-Policy-Report-Only header: ");
+
+  PolicyParserMessageBuffer permissions_policy_logger(
+      "Error with Permissions-Policy header: ");
+  PolicyParserMessageBuffer report_only_permissions_policy_logger(
+      "Error with Permissions-Policy-Report-Only header: ");
 
   WTF::StringBuilder policy_builder;
   policy_builder.Append(response.HttpHeaderField(http_names::kFeaturePolicy));
@@ -161,26 +166,28 @@ void SecurityContextInit::ApplyFeaturePolicy(
   feature_policy_header_ = FeaturePolicyParser::ParseHeader(
       feature_policy_header, permissions_policy_header,
       execution_context_->GetSecurityOrigin(), feature_policy_logger,
-      execution_context_);
+      permissions_policy_logger, execution_context_);
 
   ParsedFeaturePolicy report_only_feature_policy_header =
       FeaturePolicyParser::ParseHeader(
           response.HttpHeaderField(http_names::kFeaturePolicyReportOnly),
           report_only_permissions_policy_header,
           execution_context_->GetSecurityOrigin(),
-          report_only_feature_policy_logger, execution_context_);
+          report_only_feature_policy_logger,
+          report_only_permissions_policy_logger, execution_context_);
 
   if (!report_only_feature_policy_header.empty()) {
     UseCounter::Count(execution_context_,
                       WebFeature::kFeaturePolicyReportOnlyHeader);
   }
 
-  for (const auto& message : feature_policy_logger.GetMessages()) {
-    execution_context_->AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
-        mojom::blink::ConsoleMessageSource::kSecurity, message.level,
-        message.content));
-  }
-  for (const auto& message : report_only_feature_policy_logger.GetMessages()) {
+  auto messages = Vector<PolicyParserMessageBuffer::Message>();
+  messages.AppendVector(feature_policy_logger.GetMessages());
+  messages.AppendVector(report_only_feature_policy_logger.GetMessages());
+  messages.AppendVector(permissions_policy_logger.GetMessages());
+  messages.AppendVector(report_only_permissions_policy_logger.GetMessages());
+
+  for (const auto& message : messages) {
     execution_context_->AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
         mojom::blink::ConsoleMessageSource::kSecurity, message.level,
         message.content));

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

@@ -20,7 +20,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   // TODO(csharrison): Be smarter about parsing this origin for performance.
   scoped_refptr<const blink::SecurityOrigin> origin =
       blink::SecurityOrigin::CreateFromString("https://example.com/");
-  blink::FeaturePolicyParser::ParseHeader(WTF::String(data, size),
-                                          g_empty_string, origin.get(), logger);
+  blink::FeaturePolicyParser::ParseHeader(
+      WTF::String(data, size), g_empty_string, origin.get(), logger, logger);
   return 0;
 }

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

@@ -56,11 +56,13 @@ class CORE_EXPORT FeaturePolicyParser {
   // ExecutionContext is used to determine if any origin trials affect the
   // parsing. Example of a feature policy string:
   //     "vibrate a.com b.com; fullscreen 'none'; payment 'self', payment *".
-  static ParsedFeaturePolicy ParseHeader(const String& feature_policy_header,
-                                         const String& permission_policy_header,
-                                         scoped_refptr<const SecurityOrigin>,
-                                         PolicyParserMessageBuffer& logger,
-                                         ExecutionContext* = nullptr);
+  static ParsedFeaturePolicy ParseHeader(
+      const String& feature_policy_header,
+      const String& permission_policy_header,
+      scoped_refptr<const SecurityOrigin>,
+      PolicyParserMessageBuffer& feature_policy_logger,
+      PolicyParserMessageBuffer& permissions_policy_logger,
+      ExecutionContext* = nullptr);
 
   // Converts a container policy string into a vector of allowlists, given self
   // and src origins provided, one for each feature specified. Unrecognized

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

@@ -106,7 +106,7 @@ class FeaturePolicyParserTest : public ::testing::Test {
       PolicyParserMessageBuffer& logger,
       ExecutionContext* context = nullptr) {
     return FeaturePolicyParser::ParseHeader(
-        feature_policy_header, g_empty_string, origin, logger, context);
+        feature_policy_header, g_empty_string, origin, logger, logger, context);
   }
 };
 
@@ -193,6 +193,15 @@ class FeaturePolicyParserParsingTest
     }
   }
 
+  void CheckConsoleMessage(
+      const Vector<PolicyParserMessageBuffer::Message>& actual,
+      const std::vector<String> expected) {
+    ASSERT_EQ(actual.size(), expected.size());
+    for (size_t i = 0; i < actual.size(); ++i) {
+      EXPECT_EQ(actual[i].content, expected[i]);
+    }
+  }
+
  public:
   static const FeaturePolicyParserTestCase kCases[];
 };
@@ -550,16 +559,64 @@ TEST_P(FeaturePolicyParserParsingTest, PermissionsPolicyParsedCorrectly) {
 }
 
 TEST_F(FeaturePolicyParserParsingTest,
-       FeaturePolicyHeaderPermissionsPolicyHeaderCoExist) {
+       FeaturePolicyDuplicatedFeatureDeclaration) {
   PolicyParserMessageBuffer logger;
-  // When there is conflict take the value from permission policy.
-  // Non-conflicting entries will be merged.
+
+  // For Feature-Policy header, if there are multiple declaration for same
+  // feature, the allowlist value from *FIRST* declaration will be taken.
+  CheckParsedPolicy(FeaturePolicyParser::ParseHeader(
+                        "geolocation 'none', geolocation 'self'", "",
+                        origin_a_.get(), logger, logger, nullptr /* context */),
+                    {
+                        {
+                            // allowlist value 'none' is expected.
+                            mojom::blink::FeaturePolicyFeature::kGeolocation,
+                            /* matches_all_origins */ false,
+                            /* matches_opaque_src */ false,
+                            {},
+                        },
+                    });
+
+  EXPECT_TRUE(logger.GetMessages().IsEmpty());
+}
+
+TEST_F(FeaturePolicyParserParsingTest,
+       PermissionsPolicyDuplicatedFeatureDeclaration) {
+  PolicyParserMessageBuffer logger;
+
+  // For Permissions-Policy header, if there are multiple declaration for same
+  // feature, the allowlist value from *LAST* declaration will be taken.
+  CheckParsedPolicy(FeaturePolicyParser::ParseHeader(
+                        "", "geolocation=(), geolocation=self", origin_a_.get(),
+                        logger, logger, nullptr /* context */),
+                    {
+                        {
+                            // allowlist value 'self' is expected.
+                            mojom::blink::FeaturePolicyFeature::kGeolocation,
+                            /* matches_all_origins */ false,
+                            /* matches_opaque_src */ false,
+                            {ORIGIN_A},
+                        },
+                    });
+
+  EXPECT_TRUE(logger.GetMessages().IsEmpty());
+}
+
+TEST_F(FeaturePolicyParserParsingTest,
+       FeaturePolicyHeaderPermissionsPolicyHeaderCoExistConflictEntry) {
+  PolicyParserMessageBuffer logger;
+
+  // When there is conflict take the value from permission policy,
+  // non-conflicting entries will be merged.
   CheckParsedPolicy(FeaturePolicyParser::ParseHeader(
                         "geolocation 'none', fullscreen 'self'",
                         "geolocation=self, payment=*", origin_a_.get(), logger,
-                        nullptr /* context */),
+                        logger, nullptr /* context */),
                     {
                         {
+                            // With geolocation appearing in both headers,
+                            // the value should be taken from permissions policy
+                            // header, which is 'self' here.
                             mojom::blink::FeaturePolicyFeature::kGeolocation,
                             /* matches_all_origins */ false,
                             /* matches_opaque_src */ false,
@@ -580,6 +637,49 @@ TEST_F(FeaturePolicyParserParsingTest,
                     });
 }
 
+TEST_F(FeaturePolicyParserParsingTest,
+       FeaturePolicyHeaderPermissionsPolicyHeaderCoExistSeparateLogger) {
+  PolicyParserMessageBuffer feature_policy_logger("Feature Policy: ");
+  PolicyParserMessageBuffer permissions_policy_logger("Permissions Policy: ");
+
+  // 'geolocation' in permissions policy has a invalid allowlist item, which
+  // results in an empty allowlist, which is equivalent to 'none' in feature
+  // policy syntax.
+  CheckParsedPolicy(
+      FeaturePolicyParser::ParseHeader(
+          "worse-feature 'none', geolocation 'self'" /* feature_policy_header */
+          ,
+          "bad-feature=*, geolocation=\"data://bad-origin\"" /* permissions_policy_header
+                                                              */
+          ,
+          origin_a_.get(), feature_policy_logger, permissions_policy_logger,
+          nullptr /* context */
+          ),
+      {
+          {
+              mojom::blink::FeaturePolicyFeature::kGeolocation,
+              /* matches_all_origins */ false,
+              /* matches_opaque_src */ false,
+              {},
+          },
+      });
+
+  CheckConsoleMessage(
+      feature_policy_logger.GetMessages(),
+      {
+          "Feature Policy: Unrecognized feature: 'worse-feature'.",
+          "Feature Policy: Feature geolocation has been specified in both "
+          "Feature-Policy and Permissions-Policy header. Value defined in "
+          "Permissions-Policy header will be used.",
+      });
+  CheckConsoleMessage(
+      permissions_policy_logger.GetMessages(),
+      {
+          "Permissions Policy: Unrecognized feature: 'bad-feature'.",
+          "Permissions Policy: Unrecognized origin: 'data://bad-origin'.",
+      });
+}
+
 TEST_F(FeaturePolicyParserTest, ParseValidPolicy) {
   for (const char* policy_string : kValidPolicies) {
     PolicyParserMessageBuffer logger;

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

@@ -21,6 +21,6 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   scoped_refptr<const blink::SecurityOrigin> origin =
       blink::SecurityOrigin::CreateFromString("https://example.com/");
   blink::FeaturePolicyParser::ParseHeader(
-      g_empty_string, WTF::String(data, size), origin.get(), logger);
+      g_empty_string, WTF::String(data, size), origin.get(), logger, logger);
   return 0;
 }

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

@@ -35,7 +35,7 @@ class PolicyTest : public testing::Test {
         "fullscreen *; payment 'self'; midi 'none'; camera 'self' "
         "https://example.com https://example.net",
         /* permissions_policy_header */ g_empty_string, origin.get(),
-        dummy_logger_);
+        dummy_logger_, dummy_logger_);
     feature_policy->SetHeaderPolicy(header);
 
     auto& security_context =