Reland restrict extension features based on the extension type.

chrome/common/extensions/manifest_value.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Aaron Boodman, 2011/11/23 01:45:25]

Shame to pull out this nice, encapsulated class and not have any unit tests for
it.

[jstritar, 2011/11/28 23:09:56]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/extensions/manifest_value.h"
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "base/string_split.h"
+#include "base/values.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/extensions/extension_error_utils.h"
+
+namespace errors = extension_manifest_errors;
+namespace keys = extension_manifest_keys;
+
+struct ManifestValue::FeatureRestriction {
+  const char* key;
+  const int restriction;
+};
+
+const ManifestValue::FeatureRestriction
+    ManifestValue::kFeatureRestrictions[] = {
+  // Base keys that all manifests can specify.
+  { keys::kName,                    kTypeAll },
+  { keys::kVersion,                 kTypeAll },
+  { keys::kManifestVersion,         kTypeAll },
+  { keys::kDescription,             kTypeAll },
+  { keys::kIcons,                   kTypeAll },
+  { keys::kCurrentLocale,           kTypeAll },
+  { keys::kDefaultLocale,           kTypeAll },
+  { keys::kSignature,               kTypeAll },
+  { keys::kUpdateURL,               kTypeAll },
+  { keys::kPublicKey,               kTypeAll },
+
+  // Type specific.
+  { keys::kApp,                     kTypeAllApps },
+  { keys::kTheme,                   kTypeTheme },
+  { keys::kPlatformApp,             kTypePlatformApp },
+
+  // Extensions only.
+  { keys::kBrowserAction,           kTypeExtension },
+  { keys::kPageAction,              kTypeExtension },
+  { keys::kPageActions,             kTypeExtension },
+  { keys::kChromeURLOverrides,      kTypeExtension },
+
+  // Hosted and packaged apps.
+  { keys::kPermissions,             kTypeAllButThemes },
+  { keys::kOptionalPermissions,     kTypeAllButThemes },
+  { keys::kOptionsPage,             kTypeAllButThemes },
+  { keys::kBackground,              kTypeAllButThemes },
+  { keys::kOfflineEnabled,          kTypeAllButThemes },
+  { keys::kMinimumChromeVersion,    kTypeAllButThemes },
+  { keys::kRequirements,            kTypeAllButThemes },
+  { keys::kConvertedFromUserScript, kTypeAllButThemes },
+
+  // Extensions and packaged apps.
+  { keys::kContentScripts,          kTypeExtension | kTypePackagedApp },
+  { keys::kOmnibox,                 kTypeExtension | kTypePackagedApp },
+  { keys::kDevToolsPage,            kTypeExtension | kTypePackagedApp },
+  { keys::kSidebar,                 kTypeExtension | kTypePackagedApp },
+  { keys::kHomepageURL,             kTypeExtension | kTypePackagedApp },
+
+  // Extensions, packaged apps and platform apps.
+  { keys::kContentSecurityPolicy,   kTypeAllApps | kTypeExtension },
+  { keys::kFileBrowserHandlers,     kTypeAllApps | kTypeExtension },
+  { keys::kIncognito,               kTypeAllApps | kTypeExtension },
+  { keys::kNaClModules,             kTypeAllApps | kTypeExtension },
+  { keys::kPlugins,                 kTypeAllApps | kTypeExtension },
+  { keys::kInputComponents,         kTypeAllApps | kTypeExtension },
+  { keys::kTtsEngine,               kTypeAllApps | kTypeExtension },
+  { keys::kIntents,                 kTypeAllApps | kTypeExtension },
+};
+
+ManifestValue::ManifestValue(DictionaryValue* value)
+    : value_(value) {
+  // Read the feature restriction data.
+  for (size_t i = 0; i < arraysize(kFeatureRestrictions); ++i) {
+    restrictions_[kFeatureRestrictions[i].key] =

[Aaron Boodman, 2011/11/23 01:45:25]

Sucks to have a separate copy of this for every instance of the object.

Instead, use base::LazyInstance to have one static instance. You also don't need
the big array that you translate into a map: You can just have like

namespace {

typedef ... RestrictionMap;

struct Restrictions {
  Restrictions() {
    map[foo] = bar;
    map[monkey] = balls;
    ...
  }

  RestrictionMap map;
};

base::LazyInstance<Restrictions> g_restrictions;

}

Then, in your code, use g_restrictions.Get().

[jstritar, 2011/11/28 23:09:56]

Done.. LazyInstance is sweet!

+        kFeatureRestrictions[i].restriction;
+  }
+
+  // Determine what type of extension the manifest represents.
+  if (IsTheme())

[Aaron Boodman, 2011/11/23 01:45:25]

Consider omitting the type_ member and instead having a GetType() method that
does the work to figure out the type and then have Is*() helpers that just
delegate to GetType().

Otherwise you are essentially duplicating information - the type is represented
in both type_ and in the dictionary.

[jstritar, 2011/11/28 23:09:56]

Done.

+    type_ = kTypeTheme;
+  else if (IsPlatformApp())
+    type_ = kTypePlatformApp;
+  else if (IsHostedApp())
+    type_ = kTypeHostedApp;
+  else if (IsPackagedApp())
+    type_ = kTypePackagedApp;
+  else
+    type_ = kTypeExtension;
+}
+
+ManifestValue::~ManifestValue() {}
+
+bool ManifestValue::ValidateManifest(std::string* error) const {
+  for (DictionaryValue::key_iterator key = value_->begin_keys();
+       key != value_->end_keys(); ++key) {
+    // When validating the extension manifests, we ignore keys that are not
+    // recognized for compatibility.

[Aaron Boodman, 2011/11/23 01:45:25]

For clarity: s/for compatibility/for forward compatibility/

[jstritar, 2011/11/28 23:09:56]

Done.

+    if (!IsKnownKey(*key))

[Aaron Boodman, 2011/11/23 01:45:25]

Add:

TODO(aa): Consider having an error here in the case of strict error checking to
let developers know when they screw up.

[jstritar, 2011/11/28 23:09:56]

Done.

+      continue;
+
+    if (!CanAccessKey(*key)) {

[Aaron Boodman, 2011/11/23 01:45:25]

I think that there will probably be legacy data here, too, right? So we'll need
the same strategy as from the other place.

[jstritar, 2011/11/28 23:09:56]

I think we're okay here because we parsed hosted app manifests with a whitelist
before (See the old Extension::EnsureNotHybridApp), and we also had the
ContainsNonThemeKeys and HasMultipleUISurfaces checks.

+      *error = ExtensionErrorUtils::FormatErrorMessage(
+          errors::kFeatureNotAllowed, *key);
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool ManifestValue::HasKey(const std::string& key) const {
+  // Return false instead of CHECKing, since we're just testing for existence.
+  return CanAccessKey(key) && value_->HasKey(key);
+}
+
+bool ManifestValue::Get(
+    const std::string& path, Value** out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->Get(path, out_value);
+}
+
+bool ManifestValue::GetBoolean(
+    const std::string& path, bool* out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetBoolean(path, out_value);
+}
+
+bool ManifestValue::GetInteger(
+    const std::string& path, int* out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetInteger(path, out_value);
+}
+
+bool ManifestValue::GetString(
+    const std::string& path, std::string* out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetString(path, out_value);
+}
+
+bool ManifestValue::GetString(
+    const std::string& path, string16* out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetString(path, out_value);
+}
+
+bool ManifestValue::GetDictionary(
+    const std::string& path, DictionaryValue** out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetDictionary(path, out_value);
+}
+
+bool ManifestValue::GetList(
+    const std::string& path, ListValue** out_value) const {
+  CHECK(CanAccessPath(path)) << path;
+  return value_->GetList(path, out_value);
+}
+
+ManifestValue* ManifestValue::DeepCopy() const {
+  return new ManifestValue(value_->DeepCopy());
+}
+
+bool ManifestValue::Equals(const ManifestValue* other) const {
+  return other && value_->Equals(other->value());
+}
+
+bool ManifestValue::IsTheme() const {
+  DictionaryValue* dict_value = NULL;
+  return value_->HasKey(keys::kTheme) &&
+      value_->GetDictionary(keys::kTheme, &dict_value);
+}
+
+bool ManifestValue::IsPlatformApp() const {
+  return value_->HasKey(keys::kPlatformApp);
+}
+
+bool ManifestValue::IsPackagedApp() const {
+  return value_->HasKey(keys::kApp) && !value_->HasKey(keys::kWebURLs);
+}
+
+bool ManifestValue::IsHostedApp() const {
+  return value_->HasKey(keys::kApp) && value_->HasKey(keys::kWebURLs);
+}
+
+bool ManifestValue::IsKnownKey(const std::string& key) const {
+  RestrictionMap::const_iterator i = restrictions_.find(key);
+  return i != restrictions_.end();
+}
+
+bool ManifestValue::CanAccessKey(const std::string& key) const {
+  RestrictionMap::const_iterator i = restrictions_.find(key);
+  return (i != restrictions_.end() && (type_ & i->second) != 0);
+}
+
+bool ManifestValue::CanAccessPath(const std::string& path) const {
+  std::vector<std::string> components;
+  base::SplitString(path, '.', &components);
+  return CanAccessKey(components[0]);
+}