Allow relative paths to external extension files for some providers, error out for others.

This change will enable a fix for issue 57289, which requires a different base directory for extensions.

BUG=70111
TEST=ExtensionServiceTest.ExternalPrefProvider

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

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

8 files changed, 134 insertions, 27 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 06290c7..790071b9 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -408,10 +408,10 @@ bool ExtensionService::UninstallExtensionHelper(
 }
 
 ExtensionService::ExtensionService(Profile* profile,
-                                     const CommandLine* command_line,
-                                     const FilePath& install_directory,
-                                     ExtensionPrefs* extension_prefs,
-                                     bool autoupdate_enabled)
+                                   const CommandLine* command_line,
+                                   const FilePath& install_directory,
+                                   ExtensionPrefs* extension_prefs,
+                                   bool autoupdate_enabled)
     : profile_(profile),
       extension_prefs_(extension_prefs),
       install_directory_(install_directory),
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 06851bb3..0a0e520 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -209,14 +209,20 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
 class MockProviderVisitor
     : public ExternalExtensionProviderInterface::VisitorInterface {
  public:
-  MockProviderVisitor() : ids_found_(0) {
+
+  // The provider will return |fake_base_path| from
+  // GetBaseCrxFilePath().  User can test the behavior with
+  // and without an empty path using this parameter.
+  explicit MockProviderVisitor(FilePath fake_base_path)
+      : ids_found_(0),
+        fake_base_path_(fake_base_path) {
   }
 
   int Visit(const std::string& json_data) {
     // Give the test json file to the provider for parsing.
     provider_.reset(new ExternalExtensionProviderImpl(
         this,
-        new ExternalTestingExtensionLoader(json_data),
+        new ExternalTestingExtensionLoader(json_data, fake_base_path_),
         Extension::EXTERNAL_PREF,
         Extension::EXTERNAL_PREF_DOWNLOAD));
 
@@ -254,6 +260,10 @@ class MockProviderVisitor
     EXPECT_TRUE(prefs_->GetDictionary(id, &pref))
        << "Got back ID (" << id.c_str() << ") we weren't expecting";
 
+    EXPECT_TRUE(path.IsAbsolute());
+    if (!fake_base_path_.empty())
+      EXPECT_TRUE(fake_base_path_.IsParent(path));
+
     if (pref) {
       EXPECT_TRUE(provider_->HasExtension(id));
 
@@ -309,7 +319,7 @@ class MockProviderVisitor
 
  private:
   int ids_found_;
-
+  FilePath fake_base_path_;
   scoped_ptr<ExternalExtensionProviderImpl> provider_;
   scoped_ptr<DictionaryValue> prefs_;
 
@@ -3004,9 +3014,16 @@ TEST_F(ExtensionServiceTest, ExternalUninstall) {
 
 TEST_F(ExtensionServiceTest, ExternalPrefProvider) {
   InitializeEmptyExtensionService();
+
+  // Test some valid extension records.
+  // Set a base path to avoid erroring out on relative paths.
+  // Paths starting with // are absolute on every platform we support.
+  FilePath base_path(FILE_PATH_LITERAL("//base/path"));
+  ASSERT_TRUE(base_path.IsAbsolute());
+  MockProviderVisitor visitor(base_path);
   std::string json_data =
       "{"
-      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
+      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
       "    \"external_crx\": \"RandomExtension.crx\","
       "    \"external_version\": \"1.0\""
       "  },"
@@ -3018,11 +3035,10 @@ TEST_F(ExtensionServiceTest, ExternalPrefProvider) {
       "    \"external_update_url\": \"http:\\\\foo.com/update\""
       "  }"
       "}";
-
-  MockProviderVisitor visitor;
+  EXPECT_EQ(3, visitor.Visit(json_data));
 
   // Simulate an external_extensions.json file that contains seven invalid
-  // extensions:
+  // records:
   // - One that is missing the 'external_crx' key.
   // - One that is missing the 'external_version' key.
   // - One that is specifying .. in the path.
@@ -3068,6 +3084,35 @@ TEST_F(ExtensionServiceTest, ExternalPrefProvider) {
       "  }"
       "}";
   EXPECT_EQ(1, visitor.Visit(json_data));
+
+  // Check that if a base path is not provided, use of a relative
+  // path fails.
+  FilePath empty;
+  MockProviderVisitor visitor_no_relative_paths(empty);
+
+  // Use absolute paths.  Expect success.
+  json_data =
+      "{"
+      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
+      "    \"external_crx\": \"//RandomExtension1.crx\","
+      "    \"external_version\": \"3.0\""
+      "  },"
+      "  \"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\": {"
+      "    \"external_crx\": \"//path/to/RandomExtension2.crx\","
+      "    \"external_version\": \"3.0\""
+      "  }"
+      "}";
+  EXPECT_EQ(2, visitor_no_relative_paths.Visit(json_data));
+
+  // Use a relative path.  Expect that it will error out.
+  json_data =
+      "{"
+      "  \"cccccccccccccccccccccccccccccccc\": {"
+      "    \"external_crx\": \"RandomExtension2.crx\","
+      "    \"external_version\": \"3.0\""
+      "  }"
+      "}";
+  EXPECT_EQ(0, visitor_no_relative_paths.Visit(json_data));
 }
 
 // Test loading good extensions from the profile directory.
diff --git a/chrome/browser/extensions/external_extension_loader.cc b/chrome/browser/extensions/external_extension_loader.cc
index 79fc787..48bda43 100644
--- a/chrome/browser/extensions/external_extension_loader.cc
+++ b/chrome/browser/extensions/external_extension_loader.cc
@@ -15,6 +15,14 @@ void ExternalExtensionLoader::Init(
   owner_ = owner;
 }
 
+const FilePath ExternalExtensionLoader::GetBaseCrxFilePath() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // By default, relative paths are not supported.
+  // Subclasses that wish to support them should override this method.
+  return FilePath();
+}
+
 void ExternalExtensionLoader::OwnerShutdown() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   owner_ = NULL;
diff --git a/chrome/browser/extensions/external_extension_loader.h b/chrome/browser/extensions/external_extension_loader.h
index 594fb9e..f4864cb 100644
--- a/chrome/browser/extensions/external_extension_loader.h
+++ b/chrome/browser/extensions/external_extension_loader.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_LOADER_H_
 #pragma once
 
+#include "base/file_path.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 
@@ -41,6 +42,13 @@ class ExternalExtensionLoader
   // in prefs_ and then call LoadFinished.
   virtual void StartLoading() = 0;
 
+  // Some external providers allow relative file paths to local CRX files.
+  // Subclasses that want this behavior should override this method to
+  // return the absolute path from which relative paths should be resolved.
+  // By default, return an empty path, which indicates that relative paths
+  // are not allowed.
+  virtual const FilePath GetBaseCrxFilePath();
+
  protected:
   virtual ~ExternalExtensionLoader() {}
 
diff --git a/chrome/browser/extensions/external_extension_provider_impl.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
index b6a8b46..6954aba 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/extensions/external_policy_extension_loader.h"
 #include "chrome/browser/extensions/external_pref_extension_loader.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_paths.h"
 
 #if defined(OS_WIN)
 #include "chrome/browser/extensions/external_registry_extension_loader_win.h"
@@ -120,12 +121,16 @@ void ExternalExtensionProviderImpl::SetPrefs(DictionaryValue* prefs) {
         continue;
       }
 
-      // If the path is relative, make it absolute.
+      // If the path is relative, and the provider has a base path,
+      // build the absolute path to the crx file.
       FilePath path(external_crx);
       if (!path.IsAbsolute()) {
-        // Try path as relative path from external extension dir.
-        FilePath base_path;
-        PathService::Get(app::DIR_EXTERNAL_EXTENSIONS, &base_path);
+        FilePath base_path = loader_->GetBaseCrxFilePath();
+        if (base_path.empty()) {
+          LOG(WARNING) << "File path " << external_crx.c_str()
+                       << " is relative.  An absolute path is required.";
+          continue;
+        }
         path = base_path.Append(external_crx);
       }
 
@@ -222,7 +227,7 @@ void ExternalExtensionProviderImpl::CreateExternalProviders(
       linked_ptr<ExternalExtensionProviderInterface>(
           new ExternalExtensionProviderImpl(
               service,
-              new ExternalPrefExtensionLoader,
+              new ExternalPrefExtensionLoader(app::DIR_EXTERNAL_EXTENSIONS),
               Extension::EXTERNAL_PREF,
               Extension::EXTERNAL_PREF_DOWNLOAD)));
 #if defined(OS_WIN)
diff --git a/chrome/browser/extensions/external_pref_extension_loader.cc b/chrome/browser/extensions/external_pref_extension_loader.cc
index 791005c..fc84b1e 100644
--- a/chrome/browser/extensions/external_pref_extension_loader.cc
+++ b/chrome/browser/extensions/external_pref_extension_loader.cc
@@ -32,10 +32,22 @@ DictionaryValue* ExtractPrefs(ValueSerializer* serializer) {
 
 }  // namespace
 
-ExternalPrefExtensionLoader::ExternalPrefExtensionLoader() {
+ExternalPrefExtensionLoader::ExternalPrefExtensionLoader(int base_path_key)
+    : base_path_key_(base_path_key) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
+const FilePath ExternalPrefExtensionLoader::GetBaseCrxFilePath() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // LoadOnFileThread() should set |external_file_path_| to a non-empty
+  // path.  This function should not be called until after LoadOnFileThread()
+  // is complete.
+  CHECK(!base_path_.empty());
+
+  return base_path_;
+}
+
 void ExternalPrefExtensionLoader::StartLoading() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(
@@ -47,11 +59,13 @@ void ExternalPrefExtensionLoader::StartLoading() {
 
 void ExternalPrefExtensionLoader::LoadOnFileThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  CHECK(PathService::Get(base_path_key_, &base_path_));
+
   FilePath json_file;
-  PathService::Get(app::DIR_EXTERNAL_EXTENSIONS, &json_file);
-  json_file = json_file.Append(FILE_PATH_LITERAL("external_extensions.json"));
-  scoped_ptr<DictionaryValue> prefs;
+  json_file = base_path_.Append(FILE_PATH_LITERAL("external_extensions.json"));
 
+  scoped_ptr<DictionaryValue> prefs;
   if (file_util::PathExists(json_file)) {
     JSONFileValueSerializer serializer(json_file);
     prefs.reset(ExtractPrefs(&serializer));
@@ -68,7 +82,9 @@ void ExternalPrefExtensionLoader::LoadOnFileThread() {
 }
 
 ExternalTestingExtensionLoader::ExternalTestingExtensionLoader(
-    const std::string& json_data) {
+    const std::string& json_data,
+    const FilePath& fake_base_path)
+    : fake_base_path_(fake_base_path) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   JSONStringValueSerializer serializer(json_data);
   testing_prefs_.reset(ExtractPrefs(&serializer));
@@ -79,3 +95,7 @@ void ExternalTestingExtensionLoader::StartLoading() {
   prefs_.reset(testing_prefs_->DeepCopy());
   LoadFinished();
 }
+
+const FilePath ExternalTestingExtensionLoader::GetBaseCrxFilePath() {
+  return fake_base_path_;
+}
diff --git a/chrome/browser/extensions/external_pref_extension_loader.h b/chrome/browser/extensions/external_pref_extension_loader.h
index 9224b3b..c6852d3 100644
--- a/chrome/browser/extensions/external_pref_extension_loader.h
+++ b/chrome/browser/extensions/external_pref_extension_loader.h
@@ -19,7 +19,12 @@
 // thread and they are expecting public method calls from the UI thread.
 class ExternalPrefExtensionLoader : public ExternalExtensionLoader {
  public:
-  ExternalPrefExtensionLoader();
+  // |base_path_key| is the directory containing the external_extensions.json
+  // file.  Relative file paths to extension files are resolved relative
+  // to this path.
+  explicit ExternalPrefExtensionLoader(int base_path_key);
+
+  virtual const FilePath GetBaseCrxFilePath();
 
  protected:
   virtual void StartLoading();
@@ -31,6 +36,9 @@ class ExternalPrefExtensionLoader : public ExternalExtensionLoader {
 
   void LoadOnFileThread();
 
+  int base_path_key_;
+  FilePath base_path_;
+
   DISALLOW_COPY_AND_ASSIGN(ExternalPrefExtensionLoader);
 };
 
@@ -38,7 +46,11 @@ class ExternalPrefExtensionLoader : public ExternalExtensionLoader {
 // from json data specified in a string.
 class ExternalTestingExtensionLoader : public ExternalExtensionLoader {
  public:
-  explicit ExternalTestingExtensionLoader(const std::string& json_data);
+  ExternalTestingExtensionLoader(
+      const std::string& json_data,
+      const FilePath& fake_base_path);
+
+  virtual const FilePath GetBaseCrxFilePath();
 
  protected:
   virtual void StartLoading();
@@ -48,6 +60,7 @@ class ExternalTestingExtensionLoader : public ExternalExtensionLoader {
 
   virtual ~ExternalTestingExtensionLoader() {}
 
+  FilePath fake_base_path_;
   scoped_ptr<DictionaryValue> testing_prefs_;
 
   DISALLOW_COPY_AND_ASSIGN(ExternalTestingExtensionLoader);
diff --git a/chrome/browser/extensions/external_registry_extension_loader_win.cc b/chrome/browser/extensions/external_registry_extension_loader_win.cc
index b3037c8..c78a41b 100644
--- a/chrome/browser/extensions/external_registry_extension_loader_win.cc
+++ b/chrome/browser/extensions/external_registry_extension_loader_win.cc
@@ -49,10 +49,18 @@ void ExternalRegistryExtensionLoader::LoadOnFileThread() {
     std::wstring key_path = ASCIIToWide(kRegistryExtensions);
     key_path.append(L"\\");
     key_path.append(iterator.Name());
-    if (key.Open(kRegRoot, key_path.c_str(), KEY_READ) == ERROR_SUCCESS) {
-      std::wstring extension_path;
-      if (key.ReadValue(kRegistryExtensionPath, &extension_path)
+    if (key.Open(kRegRoot, key_path.c_str(), KEY_READ)  == ERROR_SUCCESS) {
+      std::wstring extension_path_str;
+      if (key.ReadValue(kRegistryExtensionPath, &extension_path_str)
           == ERROR_SUCCESS) {
+        FilePath extension_path(extension_path_str);
+        if (!extension_path.IsAbsolute()) {
+          LOG(ERROR) << "Path " << extension_path_str
+                     << " needs to be absolute in key "
+                     << key_path;
+          ++iterator;
+          continue;
+        }
         std::wstring extension_version;
         if (key.ReadValue(kRegistryExtensionVersion, &extension_version)
             == ERROR_SUCCESS) {
@@ -81,7 +89,7 @@ void ExternalRegistryExtensionLoader::LoadOnFileThread() {
               WideToASCII(extension_version));
           prefs->SetString(
               id + "." + ExternalExtensionProviderImpl::kExternalCrx,
-              extension_path);
+              extension_path_str);
         } else {
           // TODO(erikkay): find a way to get this into about:extensions
           LOG(ERROR) << "Missing value " << kRegistryExtensionVersion