Extensions: file handling clean up.

- remove various invalid uses of ASCII functions
- properly escape resource requests
- clean up file path handling

Some work remains to be done on the last bullet point but this is enough to fix the bug.

BUG=30509
TEST=see bug

Review URL: http://codereview.chromium.org/501046

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34923 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 51 insertions, 56 deletions

diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 4ed91b58..9097086 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -72,9 +72,12 @@ static bool IsAPIPermission(const std::string& str) {
 
 }  // namespace
 
-const char Extension::kManifestFilename[] = "manifest.json";
-const char Extension::kLocaleFolder[] = "_locales";
-const char Extension::kMessagesFilename[] = "messages.json";
+const FilePath::CharType Extension::kManifestFilename[] =
+    FILE_PATH_LITERAL("manifest.json");
+const FilePath::CharType Extension::kLocaleFolder[] =
+    FILE_PATH_LITERAL("_locales");
+const FilePath::CharType Extension::kMessagesFilename[] =
+    FILE_PATH_LITERAL("messages.json");
 
 // A list of all the keys allowed by themes.
 static const wchar_t* kValidThemeKeys[] = {
@@ -301,15 +304,14 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
     for (size_t script_index = 0; script_index < js->GetSize();
          ++script_index) {
       Value* value;
-      std::wstring relative;
+      std::string relative;
       if (!js->Get(script_index, &value) || !value->GetAsString(&relative)) {
         *error = ExtensionErrorUtils::FormatErrorMessage(errors::kInvalidJs,
             IntToString(definition_index), IntToString(script_index));
         return false;
       }
-      // TODO(georged): Make GetResourceURL accept wstring too
-      GURL url = GetResourceURL(WideToUTF8(relative));
-      ExtensionResource resource = GetResource(WideToUTF8(relative));
+      GURL url = GetResourceURL(relative);
+      ExtensionResource resource = GetResource(relative);
       result->js_scripts().push_back(UserScript::File(
           resource.extension_root(), resource.relative_path(), url));
     }
@@ -319,15 +321,14 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
     for (size_t script_index = 0; script_index < css->GetSize();
          ++script_index) {
       Value* value;
-      std::wstring relative;
+      std::string relative;
       if (!css->Get(script_index, &value) || !value->GetAsString(&relative)) {
         *error = ExtensionErrorUtils::FormatErrorMessage(errors::kInvalidCss,
             IntToString(definition_index), IntToString(script_index));
         return false;
       }
-      // TODO(georged): Make GetResourceURL accept wstring too
-      GURL url = GetResourceURL(WideToUTF8(relative));
-      ExtensionResource resource = GetResource(WideToUTF8(relative));
+      GURL url = GetResourceURL(relative);
+      ExtensionResource resource = GetResource(relative);
       result->css_scripts().push_back(UserScript::File(
           resource.extension_root(), resource.relative_path(), url));
     }
@@ -489,21 +490,6 @@ bool Extension::ContainsNonThemeKeys(const DictionaryValue& source) {
   return false;
 }
 
-// static
-ExtensionResource Extension::GetResource(const FilePath& extension_path,
-                                         const std::string& relative_path) {
-  // IsAbsolutePath gets confused on Unix if relative path starts with /.
-  // Lets remove leading / if there is one.
-  size_t start_pos = relative_path.find_first_not_of("/");
-  if (start_pos == std::string::npos)
-    return ExtensionResource();
-  std::string trimmed_path = relative_path.substr(start_pos);
-
-  FilePath relative_resource_path;
-  return ExtensionResource(extension_path,
-                           relative_resource_path.AppendASCII(trimmed_path));
-}
-
 Extension::Extension(const FilePath& path)
     : converted_from_user_script_(false), is_theme_(false),
       background_page_ready_(false) {
@@ -525,6 +511,19 @@ Extension::Extension(const FilePath& path)
 #endif
 }
 
+ExtensionResource Extension::GetResource(const std::string& relative_path) {
+#if defined(OS_POSIX)
+  FilePath relative_file_path(relative_path);
+#elif defined(OS_WIN)
+  FilePath relative_file_path(UTF8ToWide(relative_path));
+#endif
+  return ExtensionResource(path(), relative_file_path);
+}
+
+ExtensionResource Extension::GetResource(const FilePath& relative_file_path) {
+  return ExtensionResource(path(), relative_file_path);
+}
+
 // TODO(rafaelw): Move ParsePEMKeyBytes, ProducePEM & FormatPEMForOutput to a
 // util class in base:
 // http://code.google.com/p/chromium/issues/detail?id=13572
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 52e3363..9906aea 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -100,13 +100,13 @@ class Extension {
   };
 
   // The name of the manifest inside an extension.
-  static const char kManifestFilename[];
+  static const FilePath::CharType kManifestFilename[];
 
   // The name of locale folder inside an extension.
-  static const char kLocaleFolder[];
+  static const FilePath::CharType kLocaleFolder[];
 
   // The name of the messages file inside an extension.
-  static const char kMessagesFilename[];
+  static const FilePath::CharType kMessagesFilename[];
 
 #if defined(OS_WIN)
   static const char* kExtensionRegistryPath;
@@ -144,15 +144,12 @@ class Extension {
     return GetResourceURL(url(), relative_path);
   }
 
-  // Returns an extension resource object. The |extension_path| argument should
-  // be the path() from an Extension object.
-  // The |relative_path| can be untrusted user input.
-  // NOTE: Static so that it can be used from multiple threads.
-  static ExtensionResource GetResource(const FilePath& extension_path,
-                                       const std::string& relative_path);
-  ExtensionResource GetResource(const std::string& relative_path) {
-    return GetResource(path(), relative_path);
-  }
+  // Returns an extension resource object. |relative_path| should be UTF8
+  // encoded.
+  ExtensionResource GetResource(const std::string& relative_path);
+
+  // As above, but with |relative_path| following the file system's encoding.
+  ExtensionResource GetResource(const FilePath& relative_path);
 
   // |input| is expected to be the text of an rsa public or private key. It
   // tolerates the presence or absence of bracking header/footer like this:
diff --git a/chrome/common/extensions/extension_l10n_util.cc b/chrome/common/extensions/extension_l10n_util.cc
index f315d92..f3431d7 100644
--- a/chrome/common/extensions/extension_l10n_util.cc
+++ b/chrome/common/extensions/extension_l10n_util.cc
@@ -153,7 +153,7 @@ bool AddLocale(const std::set<std::string>& chrome_locales,
   }
   // Check if messages file is actually present (but don't check content).
   if (file_util::PathExists(
-         locale_folder.AppendASCII(Extension::kMessagesFilename))) {
+      locale_folder.Append(Extension::kMessagesFilename))) {
     valid_locales->insert(locale_name);
   } else {
     *error = StringPrintf("Catalog file is missing for locale %s.",
@@ -249,7 +249,7 @@ static DictionaryValue* LoadMessageFile(const FilePath& locale_path,
                                         std::string* error) {
   std::string extension_locale = locale;
   FilePath file = locale_path.AppendASCII(extension_locale)
-      .AppendASCII(Extension::kMessagesFilename);
+      .Append(Extension::kMessagesFilename);
   JSONFileValueSerializer messages_serializer(file);
   Value *dictionary = messages_serializer.Deserialize(error);
   if (!dictionary && error->empty()) {
diff --git a/chrome/common/extensions/extension_l10n_util_unittest.cc b/chrome/common/extensions/extension_l10n_util_unittest.cc
index 4da8b38..93b16d3 100644
--- a/chrome/common/extensions/extension_l10n_util_unittest.cc
+++ b/chrome/common/extensions/extension_l10n_util_unittest.cc
@@ -26,7 +26,7 @@ TEST(ExtensionL10nUtil, GetValidLocalesEmptyLocaleFolder) {
   ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());
 
-  FilePath src_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath src_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(src_path));
 
   std::string error;
@@ -42,7 +42,7 @@ TEST(ExtensionL10nUtil, GetValidLocalesWithValidLocaleNoMessagesFile) {
   ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());
 
-  FilePath src_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath src_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(src_path));
   ASSERT_TRUE(file_util::CreateDirectory(src_path.AppendASCII("sr")));
 
@@ -63,7 +63,7 @@ TEST(ExtensionL10nUtil, GetValidLocalesWithValidLocalesAndMessagesFile) {
       .AppendASCII("Extensions")
       .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj")
       .AppendASCII("1.0.0.0")
-      .AppendASCII(Extension::kLocaleFolder);
+      .Append(Extension::kLocaleFolder);
 
   std::string error;
   std::set<std::string> locales;
@@ -84,7 +84,7 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsValidFallback) {
       .AppendASCII("Extensions")
       .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj")
       .AppendASCII("1.0.0.0")
-      .AppendASCII(Extension::kLocaleFolder);
+      .Append(Extension::kLocaleFolder);
 
   std::string error;
   std::set<std::string> locales;
@@ -105,7 +105,7 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsMissingFiles) {
   ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());
 
-  FilePath src_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath src_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(src_path));
 
   std::set<std::string> valid_locales;
@@ -124,7 +124,7 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsBadJSONFormat) {
   ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());
 
-  FilePath src_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath src_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(src_path));
 
   FilePath locale = src_path.AppendASCII("sr");
@@ -132,7 +132,7 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsBadJSONFormat) {
 
   std::string data = "{ \"name\":";
   ASSERT_TRUE(
-      file_util::WriteFile(locale.AppendASCII(Extension::kMessagesFilename),
+      file_util::WriteFile(locale.Append(Extension::kMessagesFilename),
                            data.c_str(), data.length()));
 
   std::set<std::string> valid_locales;
@@ -151,7 +151,7 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsDuplicateKeys) {
   ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());
 
-  FilePath src_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath src_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(src_path));
 
   FilePath locale_1 = src_path.AppendASCII("en");
@@ -161,14 +161,14 @@ TEST(ExtensionL10nUtil, LoadMessageCatalogsDuplicateKeys) {
     "{ \"name\": { \"message\": \"something\" }, "
     "\"name\": { \"message\": \"something else\" } }";
   ASSERT_TRUE(
-      file_util::WriteFile(locale_1.AppendASCII(Extension::kMessagesFilename),
+      file_util::WriteFile(locale_1.Append(Extension::kMessagesFilename),
                            data.c_str(), data.length()));
 
   FilePath locale_2 = src_path.AppendASCII("sr");
   ASSERT_TRUE(file_util::CreateDirectory(locale_2));
 
   ASSERT_TRUE(
-      file_util::WriteFile(locale_2.AppendASCII(Extension::kMessagesFilename),
+      file_util::WriteFile(locale_2.Append(Extension::kMessagesFilename),
                            data.c_str(), data.length()));
 
   std::set<std::string> valid_locales;
diff --git a/chrome/common/extensions/extension_resource.cc b/chrome/common/extensions/extension_resource.cc
index aeb6231..575b3a3 100644
--- a/chrome/common/extensions/extension_resource.cc
+++ b/chrome/common/extensions/extension_resource.cc
@@ -9,7 +9,6 @@
 #include "base/string_util.h"
 #include "chrome/common/extensions/extension_l10n_util.h"
 #include "googleurl/src/gurl.h"
-#include "net/base/net_util.h"
 
 ExtensionResource::ExtensionResource() {
 }
diff --git a/chrome/common/extensions/extension_resource_unittest.cc b/chrome/common/extensions/extension_resource_unittest.cc
index 7493107..258227f 100644
--- a/chrome/common/extensions/extension_resource_unittest.cc
+++ b/chrome/common/extensions/extension_resource_unittest.cc
@@ -52,7 +52,7 @@ TEST(ExtensionResourceTest, CreateWithAllResourcesOnDisk) {
   ASSERT_TRUE(file_util::WriteFile(root_resource, data.c_str(), data.length()));
 
   // Create l10n resources (for current locale and its parents).
-  FilePath l10n_path = temp.path().AppendASCII(Extension::kLocaleFolder);
+  FilePath l10n_path = temp.path().Append(Extension::kLocaleFolder);
   ASSERT_TRUE(file_util::CreateDirectory(l10n_path));
 
   std::vector<std::string> locales;
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index 894d5be..db992eb 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -42,7 +42,7 @@ TEST(ExtensionTest, DISABLED_InitFromValueInvalid) {
       .AppendASCII("Extensions")
       .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj")
       .AppendASCII("1.0.0.0")
-      .AppendASCII(Extension::kManifestFilename);
+      .Append(Extension::kManifestFilename);
 
   JSONFileValueSerializer serializer(extensions_path);
   scoped_ptr<DictionaryValue> valid_value(
diff --git a/chrome/common/extensions/extension_unpacker.cc b/chrome/common/extensions/extension_unpacker.cc
index e5c7758..425105f 100644
--- a/chrome/common/extensions/extension_unpacker.cc
+++ b/chrome/common/extensions/extension_unpacker.cc
@@ -88,7 +88,7 @@ static bool PathContainsParentDirectory(const FilePath& path) {
 
 DictionaryValue* ExtensionUnpacker::ReadManifest() {
   FilePath manifest_path =
-      temp_install_dir_.AppendASCII(Extension::kManifestFilename);
+      temp_install_dir_.Append(Extension::kManifestFilename);
   if (!file_util::PathExists(manifest_path)) {
     SetError(errors::kInvalidManifest);
     return NULL;
@@ -113,7 +113,7 @@ DictionaryValue* ExtensionUnpacker::ReadManifest() {
 bool ExtensionUnpacker::ReadAllMessageCatalogs(
     const std::string& default_locale) {
   FilePath locales_path =
-    temp_install_dir_.AppendASCII(Extension::kLocaleFolder);
+    temp_install_dir_.Append(Extension::kLocaleFolder);
 
   // Treat all folders under _locales as valid locales.
   file_util::FileEnumerator locales(locales_path,
@@ -134,7 +134,7 @@ bool ExtensionUnpacker::ReadAllMessageCatalogs(
       continue;
 
     FilePath messages_path =
-      locale_path.AppendASCII(Extension::kMessagesFilename);
+      locale_path.Append(Extension::kMessagesFilename);
 
     if (!ReadMessageCatalog(messages_path))
       return false;