Add error/warning message output for DocumentPolicyParser

Previously when document policy is parsed, parsing errors are silent,
which can cause difficulties in debugging. This CL adds parsing
error output for DocumentPolicyParser.

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

third_party/blink/renderer/build/scripts/templates/policy_helper.cc.tmpl

@@ -12,7 +12,7 @@
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
-
+  
 namespace {
 
 {% for feature in feature_policy_features %}

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

@@ -274,9 +274,17 @@ void SecurityContextInit::InitializeDocumentPolicy(
   // we have origin trial information available.
   document_policy_ = FilterByOriginTrial(initializer.GetDocumentPolicy(), this);
 
+  // Handle Report-Only-Document-Policy HTTP header.
+  // Console messages generated from logger are discarded, because currently
+  // there is no way to output them to console.
+  // Calling |Document::AddConsoleMessage| in
+  // |SecurityContextInit::ApplyPendingDataToDocument| will have no effect,
+  // because when the function is called, the document is not fully initialized
+  // yet (|document_| field in current frame is not yet initialized yet).
+  PolicyParserMessageBuffer logger("%s", /* discard_message */ true);
   base::Optional<DocumentPolicy::ParsedDocumentPolicy>
       report_only_parsed_policy = DocumentPolicyParser::Parse(
-          initializer.ReportOnlyDocumentPolicyHeader());
+          initializer.ReportOnlyDocumentPolicyHeader(), logger);
   if (report_only_parsed_policy) {
     report_only_document_policy_ =
         FilterByOriginTrial(*report_only_parsed_policy, this);

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

@@ -13,6 +13,8 @@
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   static blink::BlinkFuzzerTestSupport test_support =
       blink::BlinkFuzzerTestSupport();
-  blink::DocumentPolicyParser::Parse(WTF::String(data, size));
+
+  blink::PolicyParserMessageBuffer logger;
+  blink::DocumentPolicyParser::Parse(WTF::String(data, size), logger);
   return 0;
 }

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

@@ -9,9 +9,9 @@
 #include "third_party/blink/public/common/feature_policy/document_policy_features.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/feature_policy/policy_helper.h"
-#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 
 namespace blink {
+
 class CORE_EXPORT DocumentPolicyParser {
   STATIC_ONLY(DocumentPolicyParser);
 
@@ -19,14 +19,16 @@ class CORE_EXPORT DocumentPolicyParser {
   // Parse document policy header and 'policy' attribute on iframe to
   // DocumentPolicy::FeatureState.
   static base::Optional<DocumentPolicy::ParsedDocumentPolicy> Parse(
-      const String& policy_string);
+      const String& policy_string,
+      PolicyParserMessageBuffer&);
 
   // Internal parsing method for testing.
   static base::Optional<DocumentPolicy::ParsedDocumentPolicy> ParseInternal(
       const String& policy_string,
       const DocumentPolicyNameFeatureMap& name_feature_map,
       const DocumentPolicyFeatureInfoMap& feature_info_map,
-      const DocumentPolicyFeatureSet& available_features);
+      const DocumentPolicyFeatureSet& available_features,
+      PolicyParserMessageBuffer&);
 };
 }  // namespace blink
 

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

@@ -6,14 +6,57 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_FEATURE_POLICY_POLICY_HELPER_H_
 
 #include "third_party/blink/public/common/feature_policy/feature_policy.h"
+#include "third_party/blink/public/mojom/devtools/console_message.mojom-blink.h"
 #include "third_party/blink/public/mojom/feature_policy/document_policy_feature.mojom-blink-forward.h"
 #include "third_party/blink/public/mojom/feature_policy/feature_policy.mojom-blink-forward.h"
 #include "third_party/blink/public/mojom/feature_policy/feature_policy_feature.mojom-blink-forward.h"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
 #include "third_party/blink/renderer/platform/wtf/linked_hash_set.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+#include "third_party/blink/renderer/platform/wtf/vector.h"
 
 namespace blink {
+class PolicyParserMessageBuffer {
+ public:
+  struct Message {
+    mojom::blink::ConsoleMessageLevel level;
+    String content;
+
+    Message(mojom::blink::ConsoleMessageLevel level, const String& content)
+        : level(level), content(content) {}
+  };
+
+  PolicyParserMessageBuffer() = default;
+  explicit PolicyParserMessageBuffer(const String& prefix,
+                                     bool discard_message = false)
+      : prefix_(prefix), discard_message_(discard_message) {}
+
+  ~PolicyParserMessageBuffer() = default;
+
+  void Warn(const String& message) {
+    if (!discard_message_) {
+      message_buffer_.emplace_back(mojom::blink::ConsoleMessageLevel::kWarning,
+                                   prefix_ + message);
+    }
+  }
+
+  void Error(const String& message) {
+    if (!discard_message_) {
+      message_buffer_.emplace_back(mojom::blink::ConsoleMessageLevel::kError,
+                                   prefix_ + message);
+    }
+  }
+
+  const Vector<Message>& GetMessages() { return message_buffer_; }
+
+ private:
+  String prefix_;
+  Vector<Message> message_buffer_;
+  // If a dummy message buffer is desired, i.e. messages are not needed for
+  // the caller, this flag can be set to true and the message buffer will
+  // discard any incoming messages.
+  bool discard_message_ = false;
+};
 
 using FeatureNameMap = HashMap<String, mojom::blink::FeaturePolicyFeature>;
 

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

@@ -308,8 +308,6 @@ void HTMLIFrameElement::ParseAttribute(
   }
 }
 
-// TODO(crbug.com/993790): Emit error message 'endpoint cannot be specified
-// on iframe attribute.'.
 DocumentPolicy::FeatureState HTMLIFrameElement::ConstructRequiredPolicy()
     const {
   if (!RuntimeEnabledFeatures::DocumentPolicyEnabled(&GetDocument()))
@@ -321,19 +319,32 @@ DocumentPolicy::FeatureState HTMLIFrameElement::ConstructRequiredPolicy()
         mojom::blink::WebFeature::kDocumentPolicyIframePolicyAttribute);
   }
 
-  DocumentPolicy::FeatureState new_required_policy =
-      DocumentPolicyParser::Parse(required_policy_)
-          .value_or(DocumentPolicy::ParsedDocumentPolicy{})
-          .feature_state;
+  PolicyParserMessageBuffer logger;
+  DocumentPolicy::ParsedDocumentPolicy new_required_policy =
+      DocumentPolicyParser::Parse(required_policy_, logger)
+          .value_or(DocumentPolicy::ParsedDocumentPolicy{});
 
-  for (const auto& policy_entry : new_required_policy) {
+  for (const auto& message : logger.GetMessages()) {
+    GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
+        mojom::blink::ConsoleMessageSource::kOther, message.level,
+        message.content));
+  }
+
+  if (!new_required_policy.endpoint_map.empty()) {
+    GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
+        mojom::blink::ConsoleMessageSource::kOther,
+        mojom::blink::ConsoleMessageLevel::kWarning,
+        "Iframe policy attribute cannot specify reporting endpoint."));
+  }
+
+  for (const auto& policy_entry : new_required_policy.feature_state) {
     mojom::blink::DocumentPolicyFeature feature = policy_entry.first;
     if (!GetDocument().DocumentPolicyFeatureObserved(feature)) {
       UMA_HISTOGRAM_ENUMERATION(
           "Blink.UseCounter.DocumentPolicy.PolicyAttribute", feature);
     }
   }
-  return new_required_policy;
+  return new_required_policy.feature_state;
 }
 
 ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy(

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

@@ -10,6 +10,8 @@
 #include "third_party/blink/renderer/core/dom/document_init.h"
 #include "third_party/blink/renderer/core/feature_policy/feature_policy_parser.h"
 #include "third_party/blink/renderer/core/testing/dummy_page_holder.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
+#include "third_party/blink/renderer/core/testing/sim/sim_test.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_utf8_adaptor.h"
 
@@ -361,4 +363,26 @@ TEST_F(HTMLIFrameElementTest, ConstructContainerPolicyWithAllowAttributes) {
             container_policy[2].feature);
 }
 
+using HTMLIFrameElementSimTest = SimTest;
+
+TEST_F(HTMLIFrameElementSimTest, PolicyAttributeParsingError) {
+  blink::ScopedDocumentPolicyForTest sdp(true);
+  SimRequest main_resource("https://example.com", "text/html");
+  LoadURL("https://example.com");
+  main_resource.Complete(R"(
+    <iframe policy="bad-feature-name"></iframe>
+  )");
+
+  // Note: Parsing of policy attribute string, i.e. call to
+  // HTMLFrameOwnerElement::UpdateRequiredPolicy(), happens twice in above
+  // situation:
+  // - HTMLFrameOwnerElement::LoadOrRedirectSubframe()
+  // - HTMLIFrameElement::ParseAttribute()
+  EXPECT_EQ(ConsoleMessages().size(), 2u);
+  for (const auto& message : ConsoleMessages()) {
+    EXPECT_TRUE(
+        message.StartsWith("Unrecognized document policy feature name"));
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/core/loader/document_loader.cc

@@ -186,21 +186,6 @@ DocumentLoader::DocumentLoader(
 
   document_policy_ = CreateDocumentPolicy();
 
-  // If the document is blocked by document policy, there won't be content
-  // in the sub-frametree, thus no need to initialize required_policy for
-  // subtree.
-  if (!was_blocked_by_document_policy_) {
-    // Require-Document-Policy header only affects subtree of current document,
-    // but not the current document.
-    const DocumentPolicy::FeatureState header_required_policy =
-        DocumentPolicyParser::Parse(
-            response_.HttpHeaderField(http_names::kRequireDocumentPolicy))
-            .value_or(DocumentPolicy::ParsedDocumentPolicy{})
-            .feature_state;
-    frame_->SetRequiredDocumentPolicy(DocumentPolicy::MergeFeatureState(
-        frame_policy_.required_document_policy, header_required_policy));
-  }
-
   WebNavigationTimings& timings = params_->navigation_timings;
   if (!timings.input_start.is_null())
     document_load_timing_.SetInputStart(timings.input_start);
@@ -841,22 +826,53 @@ DocumentPolicy::ParsedDocumentPolicy DocumentLoader::CreateDocumentPolicy() {
       url_.ProtocolIs("blob") || url_.ProtocolIs("filesystem"))
     return {frame_policy_.required_document_policy, {} /* endpoint_map */};
 
-  // Assume Document policy feature is enabled so we can check the
-  // Required- headers. Will re-validate when we install the new Document.
-  const auto parsed_policy =
+  PolicyParserMessageBuffer header_logger("Document-Policy HTTP header: ");
+  PolicyParserMessageBuffer require_header_logger(
+      "Require-Document-Policy HTTP header: ");
+
+  // Filtering out features that are disabled by origin trial is done
+  // in SecurityContextInit when origin trial context is available.
+  auto parsed_policy =
       DocumentPolicyParser::Parse(
-          response_.HttpHeaderField(http_names::kDocumentPolicy))
+          response_.HttpHeaderField(http_names::kDocumentPolicy), header_logger)
           .value_or(DocumentPolicy::ParsedDocumentPolicy{});
 
+  // |parsed_policy| can have policies that are disabled by origin trial,
+  // but |frame_policy_.required_document_policy| cannot.
+  // It is safe to call |IsPolicyCompatible| as long as required policy is
+  // checked against origin trial.
   if (!DocumentPolicy::IsPolicyCompatible(
           frame_policy_.required_document_policy,
           parsed_policy.feature_state)) {
     was_blocked_by_document_policy_ = true;
     // When header policy is less strict than required policy, use required
     // policy to initialize document policy for the document.
-    return {frame_policy_.required_document_policy, {} /* endpoint_map */};
+    parsed_policy = {frame_policy_.required_document_policy,
+                     {} /* endpoint_map */};
+  }
+
+  // Initialize required document policy for subtree.
+  //
+  // If the document is blocked by document policy, there won't be content
+  // in the sub-frametree, thus no need to initialize required_policy for
+  // subtree.
+  if (!was_blocked_by_document_policy_) {
+    // Require-Document-Policy header only affects subtree of current document,
+    // but not the current document.
+    const DocumentPolicy::FeatureState header_required_policy =
+        DocumentPolicyParser::Parse(
+            response_.HttpHeaderField(http_names::kRequireDocumentPolicy),
+            require_header_logger)
+            .value_or(DocumentPolicy::ParsedDocumentPolicy{})
+            .feature_state;
+    frame_->SetRequiredDocumentPolicy(DocumentPolicy::MergeFeatureState(
+        frame_policy_.required_document_policy, header_required_policy));
   }
 
+  document_policy_parsing_messages_.AppendVector(header_logger.GetMessages());
+  document_policy_parsing_messages_.AppendVector(
+      require_header_logger.GetMessages());
+
   return parsed_policy;
 }
 
@@ -1413,6 +1429,13 @@ void DocumentLoader::DidInstallNewDocument(Document* document) {
   // header 'Require-Document-Policy'.
   if (!frame_policy_.required_document_policy.empty())
     UseCounter::Count(*document, WebFeature::kRequiredDocumentPolicy);
+
+  for (const auto& message : document_policy_parsing_messages_) {
+    document->AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
+        mojom::blink::ConsoleMessageSource::kOther, message.level,
+        message.content));
+  }
+  document_policy_parsing_messages_.clear();
 }
 
 void DocumentLoader::WillCommitNavigation() {

third_party/blink/renderer/core/loader/document_loader.h

@@ -50,6 +50,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/source_location.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/weak_identifier_map.h"
+#include "third_party/blink/renderer/core/feature_policy/policy_helper.h"
 #include "third_party/blink/renderer/core/frame/dactyloscoper.h"
 #include "third_party/blink/renderer/core/frame/frame_types.h"
 #include "third_party/blink/renderer/core/frame/use_counter_helper.h"
@@ -70,7 +71,6 @@
 #include "third_party/blink/renderer/platform/weborigin/referrer.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 #include "third_party/blink/renderer/platform/wtf/shared_buffer.h"
-
 namespace base {
 class TickClock;
 }
@@ -496,6 +496,7 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
 
   DocumentPolicy::ParsedDocumentPolicy document_policy_;
   bool was_blocked_by_document_policy_;
+  Vector<PolicyParserMessageBuffer::Message> document_policy_parsing_messages_;
 
   const Member<ContentSecurityPolicy> content_security_policy_;
   const bool was_blocked_by_csp_;

third_party/blink/renderer/core/loader/document_loader_test.cc

@@ -317,6 +317,33 @@ TEST_F(DocumentLoaderSimTest, DocumentPolicyNoEffectWhenFlagNotSet) {
       PolicyValue(1.0)));
 }
 
+TEST_F(DocumentLoaderSimTest, ReportDocumentPolicyHeaderParsingError) {
+  blink::ScopedDocumentPolicyForTest sdp(true);
+  SimRequest::Params params;
+  params.response_http_headers = {{"Document-Policy", "bad-feature-name"}};
+  SimRequest main_resource("https://example.com", "text/html", params);
+  LoadURL("https://example.com");
+  main_resource.Finish();
+
+  EXPECT_EQ(ConsoleMessages().size(), 1u);
+  EXPECT_TRUE(
+      ConsoleMessages().front().StartsWith("Document-Policy HTTP header:"));
+}
+
+TEST_F(DocumentLoaderSimTest, ReportRequireDocumentPolicyHeaderParsingError) {
+  blink::ScopedDocumentPolicyForTest sdp(true);
+  SimRequest::Params params;
+  params.response_http_headers = {
+      {"Require-Document-Policy", "bad-feature-name"}};
+  SimRequest main_resource("https://example.com", "text/html", params);
+  LoadURL("https://example.com");
+  main_resource.Finish();
+
+  EXPECT_EQ(ConsoleMessages().size(), 1u);
+  EXPECT_TRUE(ConsoleMessages().front().StartsWith(
+      "Require-Document-Policy HTTP header:"));
+}
+
 TEST_F(DocumentLoaderSimTest, ReportErrorWhenDocumentPolicyIncompatible) {
   blink::ScopedDocumentPolicyForTest sdp(true);
   SimRequest::Params params;