Unify CSP: Merge network/ and content/ CSPDirective

This correspond to step 1.c from the "Unify CSP" document:
https://docs.google.com/document/d/1v5mJnXJ5dSVXE_rgvJnNM9bzH0ni0YzdhPQ7GLqyhao

This merges:
 - content::CSPDirective (removed)
 - network::mojom::CSPDirective (kept)

Only the mojo one remains: network::mojom::CSPDirective;

Future follow-up will apply do the same for ContentSecurityPolicy.

The goal at the end is to have an unique mojo data structure for representing
Content-Security-Policy.

TBR=clamy@chromium.org

Bug: 1021462
Change-Id: Ic04f491992fadb6a9a97ba6ab4327b2d7b8ad1cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2027339Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736851}

content/browser/frame_host/form_submission_throttle_unittest.cc

@@ -15,11 +15,11 @@ namespace content {
 class FormSubmissionTest : public RenderViewHostImplTestHarness {
  public:
   void PreventFormSubmission() {
-    CSPDirective form_action_none(
+    auto form_action_none = network::mojom::CSPDirective::New(
         network::mojom::CSPDirectiveName::FormAction,
         network::mojom::CSPSourceList::New(
             std::vector<network::mojom::CSPSourcePtr>(), false, false, false));
-    std::vector<CSPDirective> directives;
+    std::vector<network::mojom::CSPDirectivePtr> directives;
     directives.push_back(std::move(form_action_none));
     ContentSecurityPolicy policy({}, std::move(directives), {}, false);
     main_test_rfh()->AddContentSecurityPolicy(std::move(policy));

content/common/BUILD.gn

@@ -82,8 +82,6 @@ source_set("common") {
     "content_security_policy/content_security_policy.h",
     "content_security_policy/csp_context.cc",
     "content_security_policy/csp_context.h",
-    "content_security_policy/csp_directive.cc",
-    "content_security_policy/csp_directive.h",
     "content_security_policy/csp_source.cc",
     "content_security_policy/csp_source.h",
     "content_security_policy/csp_source_list.cc",

content/common/content_security_policy/content_security_policy.cc

@@ -41,11 +41,11 @@ static CSPDirectiveName CSPFallback(CSPDirectiveName directive) {
 
 // Looks by name for a directive in a list of directives.
 // If it is not found, returns nullptr.
-static const CSPDirective* FindDirective(
+static const network::mojom::CSPDirectivePtr* FindDirective(
     CSPDirectiveName name,
-    const std::vector<CSPDirective>& directives) {
-  for (const CSPDirective& directive : directives) {
-    if (directive.name == name) {
+    const std::vector<network::mojom::CSPDirectivePtr>& directives) {
+  for (const network::mojom::CSPDirectivePtr& directive : directives) {
+    if (directive->name == name) {
       return &directive;
     }
   }
@@ -87,7 +87,7 @@ const char* ErrorMessage(CSPDirectiveName directive) {
 
 void ReportViolation(CSPContext* context,
                      const ContentSecurityPolicy& policy,
-                     const CSPDirective& directive,
+                     const network::mojom::CSPDirectivePtr& directive,
                      const CSPDirectiveName directive_name,
                      const GURL& url,
                      bool has_followed_redirect,
@@ -111,20 +111,20 @@ void ReportViolation(CSPContext* context,
 
   message << base::ReplaceStringPlaceholders(
       ErrorMessage(directive_name),
-      {ElideURLForReportViolation(blocked_url), directive.ToString()}, nullptr);
+      {ElideURLForReportViolation(blocked_url), ToString(directive)}, nullptr);
 
-  if (directive.name != directive_name) {
+  if (directive->name != directive_name) {
     message << " Note that '"
             << network::ContentSecurityPolicy::ToString(directive_name)
             << "' was not explicitly set, so '"
-            << network::ContentSecurityPolicy::ToString(directive.name)
+            << network::ContentSecurityPolicy::ToString(directive->name)
             << "' is used as a fallback.";
   }
 
   message << "\n";
 
   context->ReportContentSecurityPolicyViolation(CSPViolationParams(
-      network::ContentSecurityPolicy::ToString(directive.name),
+      network::ContentSecurityPolicy::ToString(directive->name),
       network::ContentSecurityPolicy::ToString(directive_name), message.str(),
       blocked_url, policy.report_endpoints, policy.use_reporting_api,
       policy.header.header_value, policy.header.type, has_followed_redirect,
@@ -133,13 +133,13 @@ void ReportViolation(CSPContext* context,
 
 bool AllowDirective(CSPContext* context,
                     const ContentSecurityPolicy& policy,
-                    const CSPDirective& directive,
+                    const network::mojom::CSPDirectivePtr& directive,
                     CSPDirectiveName directive_name,
                     const GURL& url,
                     bool has_followed_redirect,
                     bool is_response_check,
                     const SourceLocation& source_location) {
-  if (CheckCSPSourceList(directive.source_list, url, context,
+  if (CheckCSPSourceList(directive->source_list, url, context,
                          has_followed_redirect, is_response_check)) {
     return true;
   }
@@ -171,7 +171,7 @@ ContentSecurityPolicy::ContentSecurityPolicy() = default;
 
 ContentSecurityPolicy::ContentSecurityPolicy(
     const network::mojom::ContentSecurityPolicyHeader& header,
-    std::vector<CSPDirective> directives,
+    std::vector<network::mojom::CSPDirectivePtr> directives,
     const std::vector<std::string>& report_endpoints,
     bool use_reporting_api)
     : header(header),
@@ -179,16 +179,12 @@ ContentSecurityPolicy::ContentSecurityPolicy(
       report_endpoints(report_endpoints),
       use_reporting_api(use_reporting_api) {}
 
-// TODO(arthursonzogni): Add the |header| to the network ContentSecurityPolicy
-// struct.
 ContentSecurityPolicy::ContentSecurityPolicy(
     network::mojom::ContentSecurityPolicyPtr csp)
     : header(*(csp->header)),
+      directives(std::move(csp->directives)),
       report_endpoints(std::move(csp->report_endpoints)),
-      use_reporting_api(csp->use_reporting_api) {
-  for (auto& directive : csp->directives)
-    directives.emplace_back(std::move(directive));
-}
+      use_reporting_api(csp->use_reporting_api) {}
 
 ContentSecurityPolicy::ContentSecurityPolicy(ContentSecurityPolicy&& other) =
     default;
@@ -216,7 +212,7 @@ bool ContentSecurityPolicy::Allow(const ContentSecurityPolicy& policy,
 
   CSPDirectiveName current_directive_name = directive_name;
   do {
-    const CSPDirective* current_directive =
+    const network::mojom::CSPDirectivePtr* current_directive =
         FindDirective(current_directive_name, policy.directives);
     if (current_directive) {
       bool allowed = AllowDirective(context, policy, *current_directive,
@@ -230,36 +226,19 @@ bool ContentSecurityPolicy::Allow(const ContentSecurityPolicy& policy,
   return true;
 }
 
-std::string ContentSecurityPolicy::ToString() const {
-  std::stringstream text;
-  bool is_first_policy = true;
-  for (const CSPDirective& directive : directives) {
-    if (!is_first_policy)
-      text << "; ";
-    is_first_policy = false;
-    text << directive.ToString();
-  }
-
-  if (!report_endpoints.empty()) {
-    if (!is_first_policy)
-      text << "; ";
-    is_first_policy = false;
-    text << "report-uri";
-    for (const std::string& endpoint : report_endpoints)
-      text << " " << endpoint;
-  }
-
-  return text.str();
-}
-
 // static
 bool ContentSecurityPolicy::ShouldUpgradeInsecureRequest(
     const ContentSecurityPolicy& policy) {
-  for (const CSPDirective& directive : policy.directives) {
-    if (directive.name == CSPDirectiveName::UpgradeInsecureRequests)
+  for (auto& directive : policy.directives) {
+    if (directive->name == CSPDirectiveName::UpgradeInsecureRequests)
       return true;
   }
   return false;
 }
 
+std::string ToString(const network::mojom::CSPDirectivePtr& directive) {
+  return network::ContentSecurityPolicy::ToString(directive->name) + " " +
+         ToString(directive->source_list);
+}
+
 }  // namespace content

content/common/content_security_policy/content_security_policy.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "content/common/content_export.h"
-#include "content/common/content_security_policy/csp_directive.h"
 #include "services/network/public/mojom/content_security_policy.mojom.h"
 #include "url/gurl.h"
 
@@ -26,7 +25,7 @@ struct CONTENT_EXPORT ContentSecurityPolicy {
   ContentSecurityPolicy();
   ContentSecurityPolicy(
       const network::mojom::ContentSecurityPolicyHeader& header,
-      std::vector<CSPDirective> directives,
+      std::vector<network::mojom::CSPDirectivePtr> directives,
       const std::vector<std::string>& report_endpoints,
       bool use_reporting_api);
   explicit ContentSecurityPolicy(network::mojom::ContentSecurityPolicyPtr);
@@ -35,12 +34,10 @@ struct CONTENT_EXPORT ContentSecurityPolicy {
   ~ContentSecurityPolicy();
 
   network::mojom::ContentSecurityPolicyHeader header;
-  std::vector<CSPDirective> directives;
+  std::vector<network::mojom::CSPDirectivePtr> directives;
   std::vector<std::string> report_endpoints;
   bool use_reporting_api;
 
-  std::string ToString() const;
-
   // Return true when the |policy| allows a request to the |url| in relation to
   // the |directive| for a given |context|.
   // Note: Any policy violation are reported to the |context|.
@@ -58,5 +55,7 @@ struct CONTENT_EXPORT ContentSecurityPolicy {
   static bool ShouldUpgradeInsecureRequest(const ContentSecurityPolicy& policy);
 };
 
+std::string CONTENT_EXPORT ToString(const network::mojom::CSPDirectivePtr&);
+
 }  // namespace content
 #endif  // CONTENT_COMMON_CONTENT_SECURITY_POLICY_CONTENT_SECURITY_POLICY_H_

content/common/content_security_policy/content_security_policy_unittest.cc

@@ -44,10 +44,10 @@ ContentSecurityPolicy BuildPolicy(CSPDirectiveName directive_name,
                                   network::mojom::CSPSourcePtr source) {
   std::vector<network::mojom::CSPSourcePtr> sources;
   sources.push_back(std::move(source));
-  std::vector<CSPDirective> directives;
-  directives.emplace_back(
+  std::vector<network::mojom::CSPDirectivePtr> directives;
+  directives.push_back(network::mojom::CSPDirective::New(
       directive_name, network::mojom::CSPSourceList::New(std::move(sources),
-                                                         false, false, false));
+                                                         false, false, false)));
   return ContentSecurityPolicy({}, std::move(directives), {}, false);
 }
 
@@ -102,8 +102,9 @@ TEST(ContentSecurityPolicy, DirectiveFallback) {
 
   {
     CSPContextTest context;
-    std::vector<CSPDirective> directives;
-    directives.emplace_back(CSPDirectiveName::DefaultSrc, allow_host("a.com"));
+    std::vector<network::mojom::CSPDirectivePtr> directives;
+    directives.push_back(network::mojom::CSPDirective::New(
+        CSPDirectiveName::DefaultSrc, allow_host("a.com")));
     ContentSecurityPolicy policy({}, std::move(directives), {}, false);
     EXPECT_FALSE(ContentSecurityPolicy::Allow(
         policy, CSPDirectiveName::FrameSrc, GURL("http://b.com"), false, false,
@@ -121,8 +122,9 @@ TEST(ContentSecurityPolicy, DirectiveFallback) {
   }
   {
     CSPContextTest context;
-    std::vector<CSPDirective> directives;
-    directives.emplace_back(CSPDirectiveName::ChildSrc, allow_host("a.com"));
+    std::vector<network::mojom::CSPDirectivePtr> directives;
+    directives.push_back(network::mojom::CSPDirective::New(
+        CSPDirectiveName::ChildSrc, allow_host("a.com")));
     ContentSecurityPolicy policy({}, std::move(directives), {}, false);
     EXPECT_FALSE(ContentSecurityPolicy::Allow(
         policy, CSPDirectiveName::FrameSrc, GURL("http://b.com"), false, false,
@@ -140,9 +142,11 @@ TEST(ContentSecurityPolicy, DirectiveFallback) {
   }
   {
     CSPContextTest context;
-    std::vector<CSPDirective> directives;
-    directives.emplace_back(CSPDirectiveName::FrameSrc, allow_host("a.com"));
-    directives.emplace_back(CSPDirectiveName::ChildSrc, allow_host("b.com"));
+    std::vector<network::mojom::CSPDirectivePtr> directives;
+    directives.push_back(network::mojom::CSPDirective::New(
+        CSPDirectiveName::FrameSrc, allow_host("a.com")));
+    directives.push_back(network::mojom::CSPDirective::New(
+        CSPDirectiveName::ChildSrc, allow_host("b.com")));
     ContentSecurityPolicy policy({}, std::move(directives), {}, false);
     EXPECT_TRUE(ContentSecurityPolicy::Allow(
         policy, CSPDirectiveName::FrameSrc, GURL("http://a.com"), false, false,
@@ -246,8 +250,9 @@ TEST(ContentSecurityPolicy, ShouldUpgradeInsecureRequest) {
 
   EXPECT_FALSE(ContentSecurityPolicy::ShouldUpgradeInsecureRequest(policy));
 
-  policy.directives.emplace_back(CSPDirectiveName::UpgradeInsecureRequests,
-                                 network::mojom::CSPSourceList::New());
+  policy.directives.push_back(network::mojom::CSPDirective::New(
+      CSPDirectiveName::UpgradeInsecureRequests,
+      network::mojom::CSPSourceList::New()));
   EXPECT_TRUE(ContentSecurityPolicy::ShouldUpgradeInsecureRequest(policy));
 }
 
@@ -321,13 +326,13 @@ TEST(ContentSecurityPolicy, NavigateToChecks) {
   };
 
   for (auto& test : cases) {
-    std::vector<CSPDirective> directives;
-    directives.emplace_back(CSPDirectiveName::NavigateTo,
-                            std::move(test.navigate_to_list));
+    std::vector<network::mojom::CSPDirectivePtr> directives;
+    directives.push_back(network::mojom::CSPDirective::New(
+        CSPDirectiveName::NavigateTo, std::move(test.navigate_to_list)));
 
     if (test.form_action_list) {
-      directives.emplace_back(CSPDirectiveName::FormAction,
-                              std::move(test.form_action_list));
+      directives.push_back(network::mojom::CSPDirective::New(
+          CSPDirectiveName::FormAction, std::move(test.form_action_list)));
     }
 
     ContentSecurityPolicy policy({}, std::move(directives), {}, false);

content/common/content_security_policy/csp_context_unittest.cc

@@ -60,8 +60,8 @@ ContentSecurityPolicy BuildPolicy(CSPDirectiveName directive_name,
                                   network::mojom::CSPSourcePtr source) {
   std::vector<network::mojom::CSPSourcePtr> sources;
   sources.push_back(std::move(source));
-  std::vector<CSPDirective> directives;
-  directives.push_back(CSPDirective(
+  std::vector<network::mojom::CSPDirectivePtr> directives;
+  directives.push_back(network::mojom::CSPDirective::New(
       directive_name, network::mojom::CSPSourceList::New(std::move(sources),
                                                          false, false, false)));
   return ContentSecurityPolicy({}, std::move(directives), {}, false);
@@ -73,8 +73,8 @@ ContentSecurityPolicy BuildPolicy(CSPDirectiveName directive_name,
   std::vector<network::mojom::CSPSourcePtr> sources;
   sources.push_back(std::move(source_1));
   sources.push_back(std::move(source_2));
-  std::vector<CSPDirective> directives;
-  directives.push_back(CSPDirective(
+  std::vector<network::mojom::CSPDirectivePtr> directives;
+  directives.push_back(network::mojom::CSPDirective::New(
       directive_name, network::mojom::CSPSourceList::New(std::move(sources),
                                                          false, false, false)));
   return ContentSecurityPolicy({}, std::move(directives), {}, false);

content/common/content_security_policy/csp_directive.cc

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "content/common/content_security_policy/csp_directive.h"
-#include "content/common/content_security_policy/csp_source_list.h"
-#include "services/network/public/cpp/content_security_policy.h"
-
-namespace content {
-
-CSPDirective::CSPDirective() = default;
-
-CSPDirective::CSPDirective(network::mojom::CSPDirectiveName name,
-                           network::mojom::CSPSourceListPtr source_list)
-    : name(name), source_list(std::move(source_list)) {}
-
-CSPDirective::CSPDirective(network::mojom::CSPDirectivePtr directive)
-    : name(directive->name), source_list(std::move(directive->source_list)) {}
-CSPDirective::CSPDirective(CSPDirective&&) = default;
-CSPDirective::~CSPDirective() = default;
-
-std::string CSPDirective::ToString() const {
-  return network::ContentSecurityPolicy::ToString(name) + " " +
-         content::ToString(source_list);
-}
-
-}  // namespace content

content/common/content_security_policy/csp_directive.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CONTENT_COMMON_CONTENT_SECURITY_POLICY_CSP_DIRECTIVE_
-#define CONTENT_COMMON_CONTENT_SECURITY_POLICY_CSP_DIRECTIVE_
-
-#include <string>
-#include "content/common/content_export.h"
-#include "services/network/public/mojom/content_security_policy.mojom.h"
-
-namespace content {
-
-// CSPDirective contains a set of allowed sources for a given Content Security
-// Policy directive.
-//
-// For example, the Content Security Policy `default-src img.cdn.com
-// example.com` would produce a CSPDirective object whose 'name' is
-// 'DefaultSrc', and whose 'source_list' contains two CSPSourceExpressions
-// representing 'img.cdn.com' and 'example.com' respectively.
-//
-// https://w3c.github.io/webappsec-csp/#framework-directives
-struct CONTENT_EXPORT CSPDirective {
-  CSPDirective();
-  CSPDirective(network::mojom::CSPDirectiveName name,
-               network::mojom::CSPSourceListPtr source_list);
-  explicit CSPDirective(network::mojom::CSPDirectivePtr directive);
-  CSPDirective(const CSPDirective&) = delete;
-  CSPDirective(CSPDirective&&);
-  ~CSPDirective();
-
-  network::mojom::CSPDirectiveName name;
-  network::mojom::CSPSourceListPtr source_list;
-
-  std::string ToString() const;
-};
-
-}  // namespace content
-#endif  // CONTENT_COMMON_CONTENT_SECURITY_POLICY_CSP_DIRECTIVE_