Extensions: Make extension_id mandatory for constructing Manifest.

The various manifest feature availability checks in the Manfiest class also depend on the specified extension ID. The extension ID is set using the Manifest::SetExtensionId function. This makes it possible for clients to use the class incorrectly since any feature availability checks made before an extension ID is set might be incorrect as they won't incorporate the correct extension ID value. Fix this by taking in the Extension ID through the Manifest constructor. BUG=1167276 Change-Id: Iade390c963e593ceb3895406dfab18e83f99da42 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2633152 Auto-Submit: Karan Bhatia <karandeepb@chromium.org> Reviewed-by: Sergey Poromov <poromov@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#848017}
marcosholgado · Jan 28, 2021 · 65980c0 · 65980c0
1 parent 984aac4
commit 65980c0
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 100 deletions.
diff --git a/chrome/browser/ash/app_mode/kiosk_app_data.cc b/chrome/browser/ash/app_mode/kiosk_app_data.cc
@@ -218,7 +218,7 @@ class KioskAppData::WebstoreDataParser
       const SkBitmap& icon,
       std::unique_ptr<base::DictionaryValue> parsed_manifest) override {
     extensions::Manifest manifest(extensions::Manifest::INVALID_LOCATION,
-                                  std::move(parsed_manifest));
+                                  std::move(parsed_manifest), id);
 
     if (!IsValidKioskAppManifest(manifest)) {
       ReportFailure();

diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
@@ -252,9 +252,8 @@ WebstoreInstaller::Approval::CreateWithNoInstallPrompt(
   std::unique_ptr<Approval> result(new Approval());
   result->extension_id = extension_id;
   result->profile = profile;
-  result->manifest = std::unique_ptr<Manifest>(new Manifest(
-      Manifest::INVALID_LOCATION,
-      std::unique_ptr<base::DictionaryValue>(parsed_manifest->DeepCopy())));
+  result->manifest = std::make_unique<Manifest>(
+      Manifest::INVALID_LOCATION, std::move(parsed_manifest), extension_id);
   result->skip_install_dialog = true;
   result->manifest_check_level = strict_manifest_check ?
     MANIFEST_CHECK_LEVEL_STRICT : MANIFEST_CHECK_LEVEL_LOOSE;

diff --git a/chrome/common/extensions/manifest_unittest.cc b/chrome/common/extensions/manifest_unittest.cc
@@ -12,6 +12,7 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "components/crx_file/id_util.h"
 #include "extensions/common/api/shared_module.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/features/feature.h"
@@ -56,21 +57,23 @@ class ManifestUnitTest : public testing::Test {
       manifest_value->Set(key, std::move(value));
     else
       manifest_value->Remove(key, nullptr);
-    manifest->reset(
-        new Manifest(Manifest::INTERNAL, std::move(manifest_value)));
+    ExtensionId extension_id = manifest->get()->extension_id();
+    *manifest = std::make_unique<Manifest>(
+        Manifest::INTERNAL, std::move(manifest_value), extension_id);
   }
 
   // Helper function that replaces the manifest held by |manifest| with a copy
   // and uses the |for_login_screen| during creation to determine its type.
   void MutateManifestForLoginScreen(std::unique_ptr<Manifest>* manifest,
                                     bool for_login_screen) {
     auto manifest_value = manifest->get()->value()->CreateDeepCopy();
+    ExtensionId extension_id = manifest->get()->extension_id();
     if (for_login_screen) {
       *manifest = Manifest::CreateManifestForLoginScreen(
-          Manifest::EXTERNAL_POLICY, std::move(manifest_value));
+          Manifest::EXTERNAL_POLICY, std::move(manifest_value), extension_id);
     } else {
-      *manifest = std::make_unique<Manifest>(Manifest::INTERNAL,
-                                             std::move(manifest_value));
+      *manifest = std::make_unique<Manifest>(
+          Manifest::INTERNAL, std::move(manifest_value), extension_id);
     }
   }
 
@@ -88,7 +91,8 @@ TEST_F(ManifestUnitTest, Extension) {
   manifest_value->SetString("unknown_key", "foo");
 
   std::unique_ptr<Manifest> manifest(
-      new Manifest(Manifest::INTERNAL, std::move(manifest_value)));
+      new Manifest(Manifest::INTERNAL, std::move(manifest_value),
+                   crx_file::id_util::GenerateId("extid")));
   std::string error;
   std::vector<InstallWarning> warnings;
   EXPECT_TRUE(manifest->ValidateManifest(&error, &warnings));
@@ -108,7 +112,8 @@ TEST_F(ManifestUnitTest, Extension) {
 
   // Test EqualsForTesting.
   auto manifest2 = std::make_unique<Manifest>(
-      Manifest::INTERNAL, manifest->value()->CreateDeepCopy());
+      Manifest::INTERNAL, manifest->value()->CreateDeepCopy(),
+      crx_file::id_util::GenerateId("extid"));
   EXPECT_TRUE(manifest->EqualsForTesting(*manifest2));
   EXPECT_TRUE(manifest2->EqualsForTesting(*manifest));
   MutateManifest(&manifest, "foo", std::make_unique<base::Value>("blah"));
@@ -122,7 +127,8 @@ TEST_F(ManifestUnitTest, ExtensionTypes) {
   value->SetString(keys::kVersion, "1");
 
   std::unique_ptr<Manifest> manifest(
-      new Manifest(Manifest::INTERNAL, std::move(value)));
+      new Manifest(Manifest::INTERNAL, std::move(value),
+                   crx_file::id_util::GenerateId("extid")));
   std::string error;
   std::vector<InstallWarning> warnings;
   EXPECT_TRUE(manifest->ValidateManifest(&error, &warnings));
@@ -183,7 +189,8 @@ TEST_F(ManifestUnitTest, RestrictedKeys) {
   value->SetString(keys::kVersion, "1");
 
   std::unique_ptr<Manifest> manifest(
-      new Manifest(Manifest::INTERNAL, std::move(value)));
+      new Manifest(Manifest::INTERNAL, std::move(value),
+                   crx_file::id_util::GenerateId("extid")));
   std::string error;
   std::vector<InstallWarning> warnings;
   EXPECT_TRUE(manifest->ValidateManifest(&error, &warnings));

diff --git a/extensions/browser/zipfile_installer.cc b/extensions/browser/zipfile_installer.cc
@@ -160,7 +160,8 @@ void ZipFileInstaller::ManifestParsed(
     return;
   }
 
-  Manifest manifest(Manifest::INTERNAL, std::move(manifest_dictionary));
+  Manifest::Type manifest_type =
+      Manifest::GetTypeFromManifestValue(*manifest_dictionary);
 
   unzip::UnzipFilterCallback filter = base::BindRepeating(
       [](bool is_theme, const base::FilePath& file_path) -> bool {
@@ -169,7 +170,7 @@ void ZipFileInstaller::ManifestParsed(
         return ZipFileInstaller::ShouldExtractFile(is_theme, file_path) &&
                !ZipFileInstaller::IsManifestFile(file_path);
       },
-      manifest.is_theme());
+      manifest_type == Manifest::TYPE_THEME);
 
   // TODO(crbug.com/645263): This silently ignores blocked file types.
   //                         Add install warnings.

diff --git a/extensions/common/extension.cc b/extensions/common/extension.cc
@@ -141,6 +141,41 @@ bool IsManifestSupported(int manifest_version,
   return true;
 }
 
+// Computes the |extension_id| from the given parameters. On success, returns
+// true. On failure, populates |error| and returns false.
+bool ComputeExtensionID(const base::DictionaryValue& manifest,
+                        const base::FilePath& path,
+                        int creation_flags,
+                        base::string16* error,
+                        ExtensionId* extension_id) {
+  if (manifest.HasKey(keys::kPublicKey)) {
+    std::string public_key;
+    std::string public_key_bytes;
+    if (!manifest.GetString(keys::kPublicKey, &public_key) ||
+        !Extension::ParsePEMKeyBytes(public_key, &public_key_bytes)) {
+      *error = base::ASCIIToUTF16(errors::kInvalidKey);
+      return false;
+    }
+    *extension_id = crx_file::id_util::GenerateId(public_key_bytes);
+    return true;
+  }
+
+  if (creation_flags & Extension::REQUIRE_KEY) {
+    *error = base::ASCIIToUTF16(errors::kInvalidKey);
+    return false;
+  }
+
+  // If there is a path, we generate the ID from it. This is useful for
+  // development mode, because it keeps the ID stable across restarts and
+  // reloading the extension.
+  *extension_id = crx_file::id_util::GenerateIdForPath(path);
+  if (extension_id->empty()) {
+    NOTREACHED() << "Could not create ID from path.";
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 const int Extension::kInitFromValueFlagBits = 15;
@@ -189,17 +224,21 @@ scoped_refptr<Extension> Extension::Create(const base::FilePath& path,
   DCHECK(utf8_error);
   base::string16 error;
 
+  ExtensionId extension_id;
+  if (!explicit_id.empty()) {
+    extension_id = explicit_id;
+  } else if (!ComputeExtensionID(value, path, flags, &error, &extension_id)) {
+    *utf8_error = base::UTF16ToUTF8(error);
+    return nullptr;
+  }
+
   std::unique_ptr<extensions::Manifest> manifest;
   if (flags & FOR_LOGIN_SCREEN) {
-    manifest = Manifest::CreateManifestForLoginScreen(location,
-                                                      value.CreateDeepCopy());
+    manifest = Manifest::CreateManifestForLoginScreen(
+        location, value.CreateDeepCopy(), std::move(extension_id));
   } else {
-    manifest = std::make_unique<Manifest>(location, value.CreateDeepCopy());
-  }
-
-  if (!InitExtensionID(manifest.get(), path, explicit_id, flags, &error)) {
-    *utf8_error = base::UTF16ToUTF8(error);
-    return nullptr;
+    manifest = std::make_unique<Manifest>(location, value.CreateDeepCopy(),
+                                          std::move(extension_id));
   }
 
   std::vector<InstallWarning> install_warnings;
@@ -496,47 +535,6 @@ void Extension::AddWebExtentPattern(const URLPattern& pattern) {
   extent_.AddPattern(pattern);
 }
 
-// static
-bool Extension::InitExtensionID(extensions::Manifest* manifest,
-                                const base::FilePath& path,
-                                const std::string& explicit_id,
-                                int creation_flags,
-                                base::string16* error) {
-  if (!explicit_id.empty()) {
-    manifest->SetExtensionId(explicit_id);
-    return true;
-  }
-
-  if (manifest->HasKey(keys::kPublicKey)) {
-    std::string public_key;
-    std::string public_key_bytes;
-    if (!manifest->GetString(keys::kPublicKey, &public_key) ||
-        !ParsePEMKeyBytes(public_key, &public_key_bytes)) {
-      *error = base::ASCIIToUTF16(errors::kInvalidKey);
-      return false;
-    }
-    std::string extension_id = crx_file::id_util::GenerateId(public_key_bytes);
-    manifest->SetExtensionId(extension_id);
-    return true;
-  }
-
-  if (creation_flags & REQUIRE_KEY) {
-    *error = base::ASCIIToUTF16(errors::kInvalidKey);
-    return false;
-  } else {
-    // If there is a path, we generate the ID from it. This is useful for
-    // development mode, because it keeps the ID stable across restarts and
-    // reloading the extension.
-    std::string extension_id = crx_file::id_util::GenerateIdForPath(path);
-    if (extension_id.empty()) {
-      NOTREACHED() << "Could not create ID from path.";
-      return false;
-    }
-    manifest->SetExtensionId(extension_id);
-    return true;
-  }
-}
-
 Extension::Extension(const base::FilePath& path,
                      std::unique_ptr<extensions::Manifest> manifest)
     : manifest_version_(0),

diff --git a/extensions/common/extension.h b/extensions/common/extension.h
@@ -346,14 +346,6 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
  private:
   friend class base::RefCountedThreadSafe<Extension>;
 
-  // Chooses the extension ID for an extension based on a variety of criteria.
-  // The chosen ID will be set in |manifest|.
-  static bool InitExtensionID(extensions::Manifest* manifest,
-                              const base::FilePath& path,
-                              const ExtensionId& explicit_id,
-                              int creation_flags,
-                              base::string16* error);
-
   Extension(const base::FilePath& path,
             std::unique_ptr<extensions::Manifest> manifest);
   virtual ~Extension();

diff --git a/extensions/common/manifest.cc b/extensions/common/manifest.cc
@@ -6,14 +6,14 @@
 
 #include <utility>
 
+#include "base/check.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/notreached.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/crx_file/id_util.h"
 #include "extensions/common/api/shared_module.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/features/feature.h"
@@ -161,30 +161,32 @@ bool Manifest::ShouldAlwaysLoadExtension(Manifest::Location location,
 // static
 std::unique_ptr<Manifest> Manifest::CreateManifestForLoginScreen(
     Location location,
-    std::unique_ptr<base::DictionaryValue> value) {
+    std::unique_ptr<base::DictionaryValue> value,
+    ExtensionId extension_id) {
   CHECK(IsPolicyLocation(location));
   // Use base::WrapUnique + new because the constructor is private.
-  return base::WrapUnique(new Manifest(location, std::move(value), true));
+  return base::WrapUnique(
+      new Manifest(location, std::move(value), std::move(extension_id), true));
 }
 
 Manifest::Manifest(Location location,
-                   std::unique_ptr<base::DictionaryValue> value)
-    : Manifest(location, std::move(value), false) {}
+                   std::unique_ptr<base::DictionaryValue> value,
+                   ExtensionId extension_id)
+    : Manifest(location, std::move(value), std::move(extension_id), false) {}
 
 Manifest::Manifest(Location location,
                    std::unique_ptr<base::DictionaryValue> value,
+                   ExtensionId extension_id,
                    bool for_login_screen)
-    : location_(location),
+    : extension_id_(std::move(extension_id)),
+      hashed_id_(HashedExtensionId(extension_id_)),
+      location_(location),
       value_(std::move(value)),
-      type_(GetTypeFromManifestValue(*value_, for_login_screen)) {}
-
-Manifest::~Manifest() {
+      type_(GetTypeFromManifestValue(*value_, for_login_screen)) {
+  DCHECK(!extension_id_.empty());
 }
 
-void Manifest::SetExtensionId(const ExtensionId& id) {
-  extension_id_ = id;
-  hashed_id_ = HashedExtensionId(id);
-}
+Manifest::~Manifest() = default;
 
 bool Manifest::ValidateManifest(
     std::string* error,
@@ -235,7 +237,7 @@ bool Manifest::HasKey(const std::string& key) const {
 }
 
 bool Manifest::HasPath(const std::string& path) const {
-  base::Value* ignored = NULL;
+  const base::Value* ignored = nullptr;
   return CanAccessPath(path) && value_->Get(path, &ignored);
 }
 

diff --git a/extensions/common/manifest.h b/extensions/common/manifest.h
@@ -146,13 +146,14 @@ class Manifest {
   // (like platform apps) may be installed in the same login screen profile.
   static std::unique_ptr<Manifest> CreateManifestForLoginScreen(
       Location location,
-      std::unique_ptr<base::DictionaryValue> value);
+      std::unique_ptr<base::DictionaryValue> value,
+      ExtensionId extension_id);
 
-  Manifest(Location location, std::unique_ptr<base::DictionaryValue> value);
+  Manifest(Location location,
+           std::unique_ptr<base::DictionaryValue> value,
+           ExtensionId extension_id);
   virtual ~Manifest();
 
-  void SetExtensionId(const ExtensionId& id);
-
   const ExtensionId& extension_id() const { return extension_id_; }
   const HashedExtensionId& hashed_id() const { return hashed_id_; }
 
@@ -226,6 +227,7 @@ class Manifest {
  private:
   Manifest(Location location,
            std::unique_ptr<base::DictionaryValue> value,
+           ExtensionId extension_id,
            bool for_login_screen);
   // Returns true if the extension can specify the given |path|.
   bool CanAccessPath(const std::string& path) const;
@@ -236,19 +238,19 @@ class Manifest {
   // like directory structures and URLs, and is expected to not change across
   // versions. It is generated as a SHA-256 hash of the extension's public
   // key, or as a hash of the path in the case of unpacked extensions.
-  std::string extension_id_;
+  const std::string extension_id_;
 
   // The hex-encoding of the SHA1 of the extension id; used to determine feature
   // availability.
-  HashedExtensionId hashed_id_;
+  const HashedExtensionId hashed_id_;
 
   // The location the extension was loaded from.
-  Location location_;
+  const Location location_;
 
   // The underlying dictionary representation of the manifest.
-  std::unique_ptr<base::DictionaryValue> value_;
+  const std::unique_ptr<const base::DictionaryValue> value_;
 
-  Type type_;
+  const Type type_;
 
   DISALLOW_COPY_AND_ASSIGN(Manifest);
 };