Extensions: Move handling of sandbox page CSP to CSPHandler.

This CL moves handling of the "sandbox.content_security_policy" to the
CSPHandler from SandboxedPageHandler. This should have no behavior change. This
is in preparation of introducing the "content_security_policy.sandbox" manifest
key.

BUG=914224

Change-Id: I27e839a09211d7fccc0104e6195d6c123cca8ef2
Reviewed-on: https://chromium-review.googlesource.com/c/1391877
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619817}

extensions/common/manifest.h

@@ -181,6 +181,9 @@ class Manifest {
 
   // These access the wrapped manifest value, returning false when the property
   // does not exist or if the manifest type can't access it.
+  // TODO(karandeepb): These methods should be changed to use base::StringPiece.
+  // Better, we should pass a list of path components instead of a unified
+  // |path| to do away with our usage of deprecated base::Value methods.
   bool HasKey(const std::string& key) const;
   bool HasPath(const std::string& path) const;
   bool Get(const std::string& path, const base::Value** out_value) const;

extensions/common/manifest_constants.cc

@@ -598,8 +598,6 @@ const char kInvalidSandboxedPagesList[] =
     "Invalid value for 'sandbox.pages'.";
 const char kInvalidSandboxedPage[] =
     "Invalid value for 'sandbox.pages[*]'.";
-const char kInvalidSandboxedPagesCSP[] =
-    "Invalid value for 'sandbox.content_security_policy'.";
 const char kInvalidSearchEngineMissingKeys[] =
     "Missing mandatory parameters for "
     "'chrome_settings_overrides.search_provider'.";

extensions/common/manifest_constants.h

@@ -425,7 +425,6 @@ extern const char kInvalidRequirements[];
 extern const char kInvalidRunAt[];
 extern const char kInvalidSandboxedPagesList[];
 extern const char kInvalidSandboxedPage[];
-extern const char kInvalidSandboxedPagesCSP[];
 extern const char kInvalidSearchEngineMissingKeys[];
 extern const char kInvalidSearchEngineURL[];
 extern const char kInvalidShortName[];

extensions/common/manifest_handlers/csp_info.cc

@@ -31,6 +31,10 @@ const char kDefaultContentSecurityPolicy[] =
     "script-src 'self' blob: filesystem: chrome-extension-resource:; "
     "object-src 'self' blob: filesystem:;";
 
+const char kDefaultSandboxedPageContentSecurityPolicy[] =
+    "sandbox allow-scripts allow-forms allow-popups allow-modals; "
+    "script-src 'self' 'unsafe-inline' 'unsafe-eval'; child-src 'self';";
+
 const char kExtensionPagesKey[] = "extension_pages";
 const char kExtensionPagesPath[] = "content_security_policy.extension_pages";
 
@@ -84,6 +88,14 @@ base::string16 GetInvalidManifestKeyError(base::StringPiece key) {
   return ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidManifestKey, key);
 }
 
+// Returns null if the manifest type can't access the path. Else returns the
+// corresponding Value.
+const base::Value* GetManifestPath(const Extension* extension,
+                                   const char* path) {
+  const base::Value* value = nullptr;
+  return extension->manifest()->Get(path, &value) ? value : nullptr;
+}
+
 }  // namespace
 
 CSPInfo::CSPInfo(const std::string& security_policy)
@@ -101,13 +113,22 @@ const std::string& CSPInfo::GetContentSecurityPolicy(
   return csp_info ? csp_info->content_security_policy : base::EmptyString();
 }
 
+// static
+const std::string& CSPInfo::GetSandboxContentSecurityPolicy(
+    const Extension* extension) {
+  CSPInfo* csp_info = static_cast<CSPInfo*>(
+      extension->GetManifestData(keys::kContentSecurityPolicy));
+  return csp_info ? csp_info->sandbox_content_security_policy
+                  : base::EmptyString();
+}
+
 // static
 const std::string& CSPInfo::GetResourceContentSecurityPolicy(
     const Extension* extension,
     const std::string& relative_path) {
-  return SandboxedPageInfo::IsSandboxedPage(extension, relative_path) ?
-      SandboxedPageInfo::GetContentSecurityPolicy(extension) :
-      GetContentSecurityPolicy(extension);
+  return SandboxedPageInfo::IsSandboxedPage(extension, relative_path)
+             ? GetSandboxContentSecurityPolicy(extension)
+             : GetContentSecurityPolicy(extension);
 }
 
 CSPHandler::CSPHandler() = default;
@@ -124,9 +145,7 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
   // "content_security_policy" : {
   //     "extension_pages" : ""
   //  }
-  const base::Value* csp = nullptr;
-  bool result = extension->manifest()->Get(key, &csp);
-  DCHECK_EQ(result, !!csp);
+  const base::Value* csp = GetManifestPath(extension, key);
 
   // TODO(crbug.com/914224): Remove the channel check once the support for the
   // dictionary key is launched to other channels.
@@ -136,7 +155,9 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
   if (csp_dictionary_supported && csp && csp->is_dict())
     return ParseCSPDictionary(extension, error, *csp);
 
-  return ParseExtensionPagesCSP(extension, error, key, csp);
+  return ParseExtensionPagesCSP(extension, error, key, csp) &&
+         ParseSandboxCSP(extension, error, keys::kSandboxedPagesCSP,
+                         GetManifestPath(extension, keys::kSandboxedPagesCSP));
 }
 
 bool CSPHandler::ParseCSPDictionary(Extension* extension,
@@ -150,7 +171,7 @@ bool CSPHandler::ParseCSPDictionary(Extension* extension,
 bool CSPHandler::ParseExtensionPagesCSP(
     Extension* extension,
     base::string16* error,
-    const std::string& manifest_key,
+    base::StringPiece manifest_key,
     const base::Value* content_security_policy) {
   if (!content_security_policy)
     return SetDefaultExtensionPagesCSP(extension);
@@ -181,6 +202,36 @@ bool CSPHandler::ParseExtensionPagesCSP(
   return true;
 }
 
+bool CSPHandler::ParseSandboxCSP(Extension* extension,
+                                 base::string16* error,
+                                 base::StringPiece manifest_key,
+                                 const base::Value* sandbox_csp) {
+  if (!sandbox_csp) {
+    SetSandboxCSP(extension, kDefaultSandboxedPageContentSecurityPolicy);
+    return true;
+  }
+
+  if (!sandbox_csp->is_string()) {
+    *error = GetInvalidManifestKeyError(manifest_key);
+    return false;
+  }
+
+  const std::string& sandbox_csp_str = sandbox_csp->GetString();
+  if (!csp_validator::ContentSecurityPolicyIsLegal(sandbox_csp_str) ||
+      !csp_validator::ContentSecurityPolicyIsSandboxed(sandbox_csp_str,
+                                                       extension->GetType())) {
+    *error = GetInvalidManifestKeyError(manifest_key);
+    return false;
+  }
+
+  std::vector<InstallWarning> warnings;
+  std::string effective_sandbox_csp =
+      csp_validator::GetEffectiveSandoxedPageCSP(sandbox_csp_str, &warnings);
+  SetSandboxCSP(extension, std::move(effective_sandbox_csp));
+  extension->AddInstallWarnings(std::move(warnings));
+  return true;
+}
+
 bool CSPHandler::SetDefaultExtensionPagesCSP(Extension* extension) {
   // TODO(abarth): Should we continue to let extensions override the
   //               default Content-Security-Policy?
@@ -199,6 +250,17 @@ bool CSPHandler::SetDefaultExtensionPagesCSP(Extension* extension) {
   return true;
 }
 
+void CSPHandler::SetSandboxCSP(Extension* extension, std::string sandbox_csp) {
+  CHECK(csp_validator::ContentSecurityPolicyIsSandboxed(sandbox_csp,
+                                                        extension->GetType()));
+
+  // By now we must have parsed the extension page CSP.
+  CSPInfo* csp_info = static_cast<CSPInfo*>(
+      extension->GetManifestData(keys::kContentSecurityPolicy));
+  DCHECK(csp_info);
+  csp_info->sandbox_content_security_policy = std::move(sandbox_csp);
+}
+
 bool CSPHandler::AlwaysParseForType(Manifest::Type type) const {
   return type == Manifest::TYPE_PLATFORM_APP ||
          type == Manifest::TYPE_EXTENSION ||
@@ -207,7 +269,8 @@ bool CSPHandler::AlwaysParseForType(Manifest::Type type) const {
 
 base::span<const char* const> CSPHandler::Keys() const {
   static constexpr const char* kKeys[] = {
-      keys::kContentSecurityPolicy, keys::kPlatformAppContentSecurityPolicy};
+      keys::kContentSecurityPolicy, keys::kPlatformAppContentSecurityPolicy,
+      keys::kSandboxedPagesCSP};
   return kKeys;
 }
 

extensions/common/manifest_handlers/csp_info.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/strings/string_piece_forward.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handler.h"
 
@@ -23,9 +24,18 @@ struct CSPInfo : public Extension::ManifestData {
   // vulnerabilities.
   std::string content_security_policy;
 
+  // Content Security Policy that should be used to enforce the sandbox used
+  // by sandboxed pages (guaranteed to have the "sandbox" directive without the
+  // "allow-same-origin" token).
+  std::string sandbox_content_security_policy;
+
   static const std::string& GetContentSecurityPolicy(
       const Extension* extension);
 
+  // Returns the extension's Content Security Policy for the sandboxed pages.
+  static const std::string& GetSandboxContentSecurityPolicy(
+      const Extension* extension);
+
   // Returns the Content Security Policy that the specified resource should be
   // served with.
   static const std::string& GetResourceContentSecurityPolicy(
@@ -33,7 +43,8 @@ struct CSPInfo : public Extension::ManifestData {
       const std::string& relative_path);
 };
 
-// Parses "content_security_policy" and "app.content_security_policy" keys.
+// Parses "content_security_policy", "app.content_security_policy" and
+// "sandbox.content_security_policy" manifest keys.
 class CSPHandler : public ManifestHandler {
  public:
   CSPHandler();
@@ -52,12 +63,22 @@ class CSPHandler : public ManifestHandler {
   // pages.
   bool ParseExtensionPagesCSP(Extension* extension,
                               base::string16* error,
-                              const std::string& manifest_key,
+                              base::StringPiece manifest_key,
                               const base::Value* content_security_policy);
 
+  // Parses the content security policy specified in the manifest for sandboxed
+  // pages. This should be called after ParseExtensionPagesCSP.
+  bool ParseSandboxCSP(Extension* extension,
+                       base::string16* error,
+                       base::StringPiece manifest_key,
+                       const base::Value* sandbox_csp);
+
   // Sets the default CSP value for the extension.
   bool SetDefaultExtensionPagesCSP(Extension* extension);
 
+  // Helper to set the sandbox content security policy manifest data.
+  void SetSandboxCSP(Extension* extension, std::string sandbox_csp);
+
   base::span<const char* const> Keys() const override;
 
   DISALLOW_COPY_AND_ASSIGN(CSPHandler);

extensions/common/manifest_handlers/csp_info_unittest.cc

@@ -47,6 +47,14 @@ TEST_F(CSPInfoUnitTest, SandboxedPages) {
   scoped_refptr<Extension> extension5(
       LoadAndExpectSuccess("sandboxed_pages_valid_5.json"));
 
+  // Sandboxed pages specified for a platform app with a custom CSP.
+  scoped_refptr<Extension> extension6(
+      LoadAndExpectSuccess("sandboxed_pages_valid_6.json"));
+
+  // Sandboxed pages specified for a platform app with no custom CSP.
+  scoped_refptr<Extension> extension7(
+      LoadAndExpectSuccess("sandboxed_pages_valid_7.json"));
+
   const char kSandboxedCSP[] =
       "sandbox allow-scripts allow-forms allow-popups allow-modals; "
       "script-src 'self' 'unsafe-inline' 'unsafe-eval'; child-src 'self';";
@@ -72,17 +80,21 @@ TEST_F(CSPInfoUnitTest, SandboxedPages) {
                                extension5.get(), "/path/test.ext"));
   EXPECT_EQ(kDefaultCSP, CSPInfo::GetResourceContentSecurityPolicy(
                              extension5.get(), "/test"));
+  EXPECT_EQ(kCustomSandboxedCSP, CSPInfo::GetResourceContentSecurityPolicy(
+                                     extension6.get(), "/test"));
+  EXPECT_EQ(kSandboxedCSP, CSPInfo::GetResourceContentSecurityPolicy(
+                               extension7.get(), "/test"));
 
   Testcase testcases[] = {
       Testcase("sandboxed_pages_invalid_1.json",
                errors::kInvalidSandboxedPagesList),
       Testcase("sandboxed_pages_invalid_2.json", errors::kInvalidSandboxedPage),
       Testcase("sandboxed_pages_invalid_3.json",
-               errors::kInvalidSandboxedPagesCSP),
+               GetInvalidManifestKeyError(keys::kSandboxedPagesCSP)),
       Testcase("sandboxed_pages_invalid_4.json",
-               errors::kInvalidSandboxedPagesCSP),
+               GetInvalidManifestKeyError(keys::kSandboxedPagesCSP)),
       Testcase("sandboxed_pages_invalid_5.json",
-               errors::kInvalidSandboxedPagesCSP)};
+               GetInvalidManifestKeyError(keys::kSandboxedPagesCSP))};
   RunTestcases(testcases, base::size(testcases), EXPECT_TYPE_ERROR);
 }
 

extensions/common/manifest_handlers/sandboxed_page_info.cc

@@ -12,7 +12,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
-#include "extensions/common/csp_validator.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/url_pattern.h"
@@ -24,10 +23,6 @@ namespace errors = manifest_errors;
 
 namespace {
 
-const char kDefaultSandboxedPageContentSecurityPolicy[] =
-    "sandbox allow-scripts allow-forms allow-popups allow-modals; "
-    "script-src 'self' 'unsafe-inline' 'unsafe-eval'; child-src 'self';";
-
 static base::LazyInstance<SandboxedPageInfo>::DestructorAtExit
     g_empty_sandboxed_info = LAZY_INSTANCE_INITIALIZER;
 
@@ -45,11 +40,6 @@ SandboxedPageInfo::SandboxedPageInfo() {
 SandboxedPageInfo::~SandboxedPageInfo() {
 }
 
-const std::string& SandboxedPageInfo::GetContentSecurityPolicy(
-    const Extension* extension) {
-  return GetSandboxedPageInfo(extension).content_security_policy;
-}
-
 const URLPatternSet& SandboxedPageInfo::GetPages(const Extension* extension) {
   return GetSandboxedPageInfo(extension).pages;
 }
@@ -94,33 +84,6 @@ bool SandboxedPageHandler::Parse(Extension* extension, base::string16* error) {
     sandboxed_info->pages.AddPattern(pattern);
   }
 
-  if (extension->manifest()->HasPath(keys::kSandboxedPagesCSP)) {
-    std::string content_security_policy;
-    if (!extension->manifest()->GetString(keys::kSandboxedPagesCSP,
-                                          &content_security_policy)) {
-      *error = base::ASCIIToUTF16(errors::kInvalidSandboxedPagesCSP);
-      return false;
-    }
-
-    if (!csp_validator::ContentSecurityPolicyIsLegal(content_security_policy) ||
-        !csp_validator::ContentSecurityPolicyIsSandboxed(
-            content_security_policy, extension->GetType())) {
-      *error = base::ASCIIToUTF16(errors::kInvalidSandboxedPagesCSP);
-      return false;
-    }
-
-    std::vector<InstallWarning> warnings;
-    sandboxed_info->content_security_policy =
-        csp_validator::GetEffectiveSandoxedPageCSP(content_security_policy,
-                                                   &warnings);
-    extension->AddInstallWarnings(std::move(warnings));
-  } else {
-    sandboxed_info->content_security_policy =
-        kDefaultSandboxedPageContentSecurityPolicy;
-  }
-  CHECK(csp_validator::ContentSecurityPolicyIsSandboxed(
-      sandboxed_info->content_security_policy, extension->GetType()));
-
   extension->SetManifestData(keys::kSandboxedPages, std::move(sandboxed_info));
   return true;
 }

extensions/common/manifest_handlers/sandboxed_page_info.h

@@ -18,10 +18,6 @@ struct SandboxedPageInfo : public Extension::ManifestData {
   SandboxedPageInfo();
   ~SandboxedPageInfo() override;
 
-  // Returns the extension's Content Security Policy for the sandboxed pages.
-  static const std::string& GetContentSecurityPolicy(
-      const Extension* extension);
-
   // Returns the extension's sandboxed pages.
   static const URLPatternSet& GetPages(const Extension* extension);
 
@@ -32,13 +28,10 @@ struct SandboxedPageInfo : public Extension::ManifestData {
   // Optional list of extension pages that are sandboxed (served from a unique
   // origin with a different Content Security Policy).
   URLPatternSet pages;
-
-  // Content Security Policy that should be used to enforce the sandbox used
-  // by sandboxed pages (guaranteed to have the "sandbox" directive without the
-  // "allow-same-origin" token).
-  std::string content_security_policy;
 };
 
+// Responsible for parsing the "sandbox.pages" manifest key.
+// "sandbox.content_security_policy" is parsed by CSPHandler.
 class SandboxedPageHandler : public ManifestHandler {
  public:
   SandboxedPageHandler();

extensions/test/data/manifest_tests/sandboxed_pages_valid_6.json

+{
+  "name": "test",
+  "version": "0.1",
+  "manifest_version": 2,
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  },
+  "sandbox": {
+    "pages": ["test"],
+    "content_security_policy": "sandbox; script-src https://www.google.com"
+  }
+}

extensions/test/data/manifest_tests/sandboxed_pages_valid_7.json

+{
+  "name": "test",
+  "version": "0.1",
+  "manifest_version": 2,
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  },
+  "sandbox": {
+    "pages": ["test"]
+  }
+}