From 8b531298265f3a673296963db1c4fdd66b45785e Mon Sep 17 00:00:00 2001 From: "aa@chromium.org" Date: Tue, 6 Mar 2012 23:17:30 +0000 Subject: Better error messages when using unsupported manifest features. BUG=112620 Review URL: https://chromiumcodereview.appspot.com/9609007 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125247 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/common/extensions/extension_constants.cc | 2 +- chrome/common/extensions/feature.cc | 56 +++++++++++++++----- chrome/common/extensions/feature.h | 32 +++++++++--- chrome/common/extensions/feature_unittest.cc | 69 ++++++++++++++----------- chrome/common/extensions/manifest.cc | 38 +++++++------- chrome/common/extensions/manifest.h | 2 +- chrome/common/extensions/manifest_unittest.cc | 10 +++- 7 files changed, 135 insertions(+), 74 deletions(-) diff --git a/chrome/common/extensions/extension_constants.cc b/chrome/common/extensions/extension_constants.cc index 868dc3c..a77ee67 100644 --- a/chrome/common/extensions/extension_constants.cc +++ b/chrome/common/extensions/extension_constants.cc @@ -175,7 +175,7 @@ const char kExperimentalFlagRequired[] = "default. You can enable 'Experimental Extension APIs' " "by visiting chrome://flags."; const char kFeatureNotAllowed[] = - "Feature '*' is not allowed in this type of manifest."; + "Feature '*' is not accessible. *"; const char kInvalidAllFrames[] = "Invalid value for 'content_scripts[*].all_frames'."; const char kInvalidBackground[] = diff --git a/chrome/common/extensions/feature.cc b/chrome/common/extensions/feature.cc index aad4d01..8ee35d90 100644 --- a/chrome/common/extensions/feature.cc +++ b/chrome/common/extensions/feature.cc @@ -7,6 +7,7 @@ #include #include "base/lazy_instance.h" +#include "base/stringprintf.h" namespace { @@ -151,40 +152,67 @@ Feature::Location Feature::ConvertLocation(Extension::Location location) { return UNSPECIFIED_LOCATION; } -bool Feature::IsAvailable(const std::string& extension_id, - Extension::Type type, - Location location, - Context context, - Platform platform, - int manifest_version) { +std::string Feature::GetErrorMessage(Feature::Availability result) { + switch (result) { + case IS_AVAILABLE: + return ""; + case NOT_FOUND_IN_WHITELIST: + return "Not allowed for specified extension ID."; + case INVALID_TYPE: + return "Not allowed for specified package type (theme, app, etc.)."; + case INVALID_CONTEXT: + return "Not allowed for specified context type content script, extension " + "page, web page, etc.)."; + case INVALID_LOCATION: + return "Not allowed for specified install location."; + case INVALID_PLATFORM: + return "Not allowed for specified platform."; + case INVALID_MIN_MANIFEST_VERSION: + return base::StringPrintf("Requires manifest version of at least %d.", + min_manifest_version_); + case INVALID_MAX_MANIFEST_VERSION: + return base::StringPrintf("Requires manifest version of %d or lower.", + max_manifest_version_); + default: + CHECK(false); + return ""; + } +} + +Feature::Availability Feature::IsAvailable(const std::string& extension_id, + Extension::Type type, + Location location, + Context context, + Platform platform, + int manifest_version) { if (!whitelist_.empty() && whitelist_.find(extension_id) == whitelist_.end()) { - return false; + return NOT_FOUND_IN_WHITELIST; } if (!extension_types_.empty() && extension_types_.find(type) == extension_types_.end()) { - return false; + return INVALID_TYPE; } if (!contexts_.empty() && contexts_.find(context) == contexts_.end()) { - return false; + return INVALID_CONTEXT; } if (location_ != UNSPECIFIED_LOCATION && location_ != location) - return false; + return INVALID_LOCATION; if (platform_ != UNSPECIFIED_PLATFORM && platform_ != platform) - return false; + return INVALID_PLATFORM; if (min_manifest_version_ != 0 && manifest_version < min_manifest_version_) - return false; + return INVALID_MIN_MANIFEST_VERSION; if (max_manifest_version_ != 0 && manifest_version > max_manifest_version_) - return false; + return INVALID_MAX_MANIFEST_VERSION; - return true; + return IS_AVAILABLE; } } // namespace diff --git a/chrome/common/extensions/feature.h b/chrome/common/extensions/feature.h index 0a95689..61a96a5 100644 --- a/chrome/common/extensions/feature.h +++ b/chrome/common/extensions/feature.h @@ -41,6 +41,19 @@ class Feature { CHROMEOS_PLATFORM }; + // Whether a feature is available in a given situation or not, and if not, + // why not. + enum Availability { + IS_AVAILABLE, + NOT_FOUND_IN_WHITELIST, + INVALID_TYPE, + INVALID_CONTEXT, + INVALID_LOCATION, + INVALID_PLATFORM, + INVALID_MIN_MANIFEST_VERSION, + INVALID_MAX_MANIFEST_VERSION + }; + Feature(); ~Feature(); @@ -75,13 +88,13 @@ class Feature { // Returns true if the feature is available to the specified extension. Use // this overload for features that are not associated with a specific context. - bool IsAvailable(const Extension* extension) { + Availability IsAvailable(const Extension* extension) { return IsAvailable(extension, UNSPECIFIED_CONTEXT); } // Returns true if the feature is available to the specified extension, in the // specified context type. - bool IsAvailable(const Extension* extension, Context context) { + Availability IsAvailable(const Extension* extension, Context context) { return IsAvailable(extension->id(), extension->GetType(), ConvertLocation(extension->location()), context, GetCurrentPlatform(), @@ -91,8 +104,9 @@ class Feature { // Returns true if the feature is available to extensions with the specified // properties. Use this overload for features that are not associated with a // specific context, and when a full Extension object is not available. - bool IsAvailable(const std::string& extension_id, Extension::Type type, - Location location, int manifest_version) { + Availability IsAvailable(const std::string& extension_id, + Extension::Type type, Location location, + int manifest_version) { return IsAvailable(extension_id, type, location, UNSPECIFIED_CONTEXT, GetCurrentPlatform(), manifest_version); } @@ -100,9 +114,13 @@ class Feature { // Returns true if the feature is available to extensions with the specified // properties, in the specified context type, and on the specified platform. // This overload is mainly used for testing. - bool IsAvailable(const std::string& extension_id, Extension::Type type, - Location location, Context context, - Platform platform, int manifest_version); + Availability IsAvailable(const std::string& extension_id, + Extension::Type type, Location location, + Context context, Platform platform, + int manifest_version); + + // Returns an error message for an Availability code. + std::string GetErrorMessage(Availability result); private: // For clarify and consistency, we handle the default value of each of these diff --git a/chrome/common/extensions/feature_unittest.cc b/chrome/common/extensions/feature_unittest.cc index 0ebaaa9..c4d6fae 100644 --- a/chrome/common/extensions/feature_unittest.cc +++ b/chrome/common/extensions/feature_unittest.cc @@ -25,19 +25,26 @@ struct IsAvailableTestData { TEST(ExtensionFeatureTest, IsAvailableNullCase) { const IsAvailableTestData tests[] = { { "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, true }, + Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "random-extension", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, true }, + Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "", Extension::TYPE_PACKAGED_APP, Feature::UNSPECIFIED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, true }, + Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "", Extension::TYPE_UNKNOWN, Feature::PRIVILEGED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, true }, + Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_CONTEXT, - Feature::COMPONENT_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, true }, + Feature::COMPONENT_LOCATION, Feature::UNSPECIFIED_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::CHROMEOS_PLATFORM, -1, true }, + Feature::UNSPECIFIED_LOCATION, Feature::CHROMEOS_PLATFORM, -1, + Feature::IS_AVAILABLE }, { "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_CONTEXT, - Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, 25, true } + Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_PLATFORM, 25, + Feature::IS_AVAILABLE } }; Feature feature; @@ -55,22 +62,22 @@ TEST(ExtensionFeatureTest, Whitelist) { feature.whitelist()->insert("foo"); feature.whitelist()->insert("bar"); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "foo", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "bar", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::NOT_FOUND_IN_WHITELIST, feature.IsAvailable( "baz", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::NOT_FOUND_IN_WHITELIST, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); feature.extension_types()->insert(Extension::TYPE_PACKAGED_APP); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::NOT_FOUND_IN_WHITELIST, feature.IsAvailable( "baz", Extension::TYPE_PACKAGED_APP, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); } @@ -80,17 +87,17 @@ TEST(ExtensionFeatureTest, PackageType) { feature.extension_types()->insert(Extension::TYPE_EXTENSION); feature.extension_types()->insert(Extension::TYPE_PACKAGED_APP); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_EXTENSION, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_PACKAGED_APP, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_TYPE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_TYPE, feature.IsAvailable( "", Extension::TYPE_THEME, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); } @@ -100,17 +107,17 @@ TEST(ExtensionFeatureTest, Context) { feature.contexts()->insert(Feature::PRIVILEGED_CONTEXT); feature.contexts()->insert(Feature::CONTENT_SCRIPT_CONTEXT); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::PRIVILEGED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::CONTENT_SCRIPT_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_CONTEXT, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNPRIVILEGED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_CONTEXT, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); } @@ -118,10 +125,10 @@ TEST(ExtensionFeatureTest, Context) { TEST(ExtensionFeatureTest, Location) { Feature feature; feature.set_location(Feature::COMPONENT_LOCATION); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::COMPONENT_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_LOCATION, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); } @@ -129,10 +136,10 @@ TEST(ExtensionFeatureTest, Location) { TEST(ExtensionFeatureTest, Platform) { Feature feature; feature.set_platform(Feature::CHROMEOS_PLATFORM); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::CHROMEOS_PLATFORM, -1)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_PLATFORM, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, -1)); } @@ -141,29 +148,29 @@ TEST(ExtensionFeatureTest, Version) { Feature feature; feature.set_min_manifest_version(5); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_MIN_MANIFEST_VERSION, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 0)); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_MIN_MANIFEST_VERSION, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 4)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 5)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 10)); feature.set_max_manifest_version(8); - EXPECT_FALSE(feature.IsAvailable( + EXPECT_EQ(Feature::INVALID_MAX_MANIFEST_VERSION, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 10)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 8)); - EXPECT_TRUE(feature.IsAvailable( + EXPECT_EQ(Feature::IS_AVAILABLE, feature.IsAvailable( "", Extension::TYPE_UNKNOWN, Feature::UNSPECIFIED_LOCATION, Feature::UNSPECIFIED_CONTEXT, Feature::UNSPECIFIED_PLATFORM, 7)); } diff --git a/chrome/common/extensions/manifest.cc b/chrome/common/extensions/manifest.cc index d462bb8..0cf2ef2 100644 --- a/chrome/common/extensions/manifest.cc +++ b/chrome/common/extensions/manifest.cc @@ -28,18 +28,24 @@ Manifest::~Manifest() { bool Manifest::ValidateManifest(string16* error) const { for (DictionaryValue::key_iterator key = value_->begin_keys(); key != value_->end_keys(); ++key) { - bool was_known = false; - if (!CanAccessKey(*key, &was_known)) { + scoped_ptr feature = + ManifestFeatureProvider::GetDefaultInstance()->GetFeature(*key); + if (!feature.get()) { // When validating the extension manifests, we ignore keys that are not // recognized for forward compatibility. - if (!was_known) { - // TODO(aa): Consider having an error here in the case of strict error - // checking to let developers know when they screw up. - continue; - } + // TODO(aa): Consider having an error here in the case of strict error + // checking to let developers know when they screw up. + continue; + } + Feature::Availability result = feature->IsAvailable( + extension_id_, GetType(), Feature::ConvertLocation(location_), + GetManifestVersion()); + if (result != Feature::IS_AVAILABLE) { *error = ExtensionErrorUtils::FormatErrorMessageUTF16( - errors::kFeatureNotAllowed, *key); + errors::kFeatureNotAllowed, + *key, + feature->GetErrorMessage(result)); return false; } } @@ -48,7 +54,7 @@ bool Manifest::ValidateManifest(string16* error) const { } bool Manifest::HasKey(const std::string& key) const { - return CanAccessKey(key, NULL) && value_->HasKey(key); + return CanAccessKey(key) && value_->HasKey(key); } bool Manifest::Get( @@ -140,22 +146,18 @@ bool Manifest::IsHostedApp() const { bool Manifest::CanAccessPath(const std::string& path) const { std::vector components; base::SplitString(path, '.', &components); - return CanAccessKey(components[0], NULL); + return CanAccessKey(components[0]); } -bool Manifest::CanAccessKey(const std::string& key, bool *was_known) const { +bool Manifest::CanAccessKey(const std::string& key) const { scoped_ptr feature = ManifestFeatureProvider::GetDefaultInstance()->GetFeature(key); if (!feature.get()) return false; - if (was_known) - *was_known = true; - - return feature->IsAvailable(extension_id_, - GetType(), - Feature::ConvertLocation(location_), - GetManifestVersion()); + return Feature::IS_AVAILABLE == feature->IsAvailable( + extension_id_, GetType(), Feature::ConvertLocation(location_), + GetManifestVersion()); } } // namespace extensions diff --git a/chrome/common/extensions/manifest.h b/chrome/common/extensions/manifest.h index 9645589..43e2caa 100644 --- a/chrome/common/extensions/manifest.h +++ b/chrome/common/extensions/manifest.h @@ -82,7 +82,7 @@ class Manifest { private: // Returns true if the extension can specify the given |path|. bool CanAccessPath(const std::string& path) const; - bool CanAccessKey(const std::string& key, bool* was_known) const; + bool CanAccessKey(const std::string& key) const; // A persistent, globally unique ID. An extension's ID is used in things // like directory structures and URLs, and is expected to not change across diff --git a/chrome/common/extensions/manifest_unittest.cc b/chrome/common/extensions/manifest_unittest.cc index 7ca6ac5..0d45982 100644 --- a/chrome/common/extensions/manifest_unittest.cc +++ b/chrome/common/extensions/manifest_unittest.cc @@ -72,8 +72,14 @@ TEST_F(ManifestTest, Extension) { // Validate should also stop working. error.clear(); EXPECT_FALSE(manifest->ValidateManifest(&error)); - EXPECT_EQ(ExtensionErrorUtils::FormatErrorMessageUTF16( - errors::kFeatureNotAllowed, "background_page"), error); + { + Feature feature; + feature.set_max_manifest_version(1); + EXPECT_EQ(ExtensionErrorUtils::FormatErrorMessageUTF16( + errors::kFeatureNotAllowed, + "background_page", + feature.GetErrorMessage(Feature::INVALID_MAX_MANIFEST_VERSION)), error); + } // Test DeepCopy and Equals. scoped_ptr manifest2(manifest->DeepCopy()); -- cgit v1.1