From e960e28d190ef547f2f01e2bc70f120c993c3a2e Mon Sep 17 00:00:00 2001 From: treib Date: Fri, 11 Sep 2015 03:38:08 -0700 Subject: [PATCH] Only grant permissions to new extensions from sync if they have the expected version TBRing a trivial change to theme_syncable_service.cc TBR=pkotwicz@chromium.org BUG=529278 Review URL: https://codereview.chromium.org/1330163002 Cr-Commit-Position: refs/heads/master@{#348371} --- chrome/browser/apps/drive/drive_app_converter.cc | 2 +- chrome/browser/extensions/crx_installer.cc | 67 +++++++++++----------- chrome/browser/extensions/crx_installer.h | 41 ++++++++----- chrome/browser/extensions/extension_service.cc | 31 +++++----- .../extensions/extension_service_unittest.cc | 46 +++++++++++++-- .../browser/extensions/extension_sync_service.cc | 1 + .../extensions/pending_extension_manager.cc | 3 +- .../browser/extensions/pending_extension_manager.h | 6 +- chrome/browser/themes/theme_syncable_service.cc | 2 + 9 files changed, 125 insertions(+), 74 deletions(-) diff --git a/chrome/browser/apps/drive/drive_app_converter.cc b/chrome/browser/apps/drive/drive_app_converter.cc index e1df7c9e24b5..bbe79829339f 100644 --- a/chrome/browser/apps/drive/drive_app_converter.cc +++ b/chrome/browser/apps/drive/drive_app_converter.cc @@ -195,7 +195,7 @@ void DriveAppConverter::OnFinishCrxInstall(const std::string& extension_id, } extension_ = crx_installer_->extension(); - is_new_install_ = success && crx_installer_->current_version().empty(); + is_new_install_ = success && !crx_installer_->current_version().IsValid(); PostInstallCleanUp(); base::ResetAndReturn(&finished_callback_).Run(this, success); diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc index 295fe3472afa..c8cee2bda9dd 100644 --- a/chrome/browser/extensions/crx_installer.cc +++ b/chrome/browser/extensions/crx_installer.cc @@ -114,7 +114,7 @@ CrxInstaller::CrxInstaller(base::WeakPtr service_weak, hash_check_failed_(false), expected_manifest_check_level_( WebstoreInstaller::MANIFEST_CHECK_LEVEL_STRICT), - expected_version_strict_checking_(false), + fail_install_if_unexpected_version_(false), extensions_enabled_(service_weak->extensions_enabled()), delete_source_(false), create_app_shortcut_(false), @@ -153,10 +153,8 @@ CrxInstaller::CrxInstaller(base::WeakPtr service_weak, expected_manifest_.reset(approval->manifest->DeepCopy()); expected_id_ = approval->extension_id; } - if (approval->minimum_version.get()) { - expected_version_.reset(new Version(*approval->minimum_version)); - expected_version_strict_checking_ = false; - } + if (approval->minimum_version.get()) + minimum_version_ = base::Version(*approval->minimum_version); show_dialog_callback_ = approval->show_dialog_callback; set_is_ephemeral(approval->is_ephemeral); @@ -259,22 +257,20 @@ CrxInstallError CrxInstaller::AllowInstall(const Extension* extension) { base::ASCIIToUTF16(extension->id()))); } - if (expected_version_.get()) { - if (expected_version_strict_checking_) { - if (!expected_version_->Equals(*extension->version())) { - return CrxInstallError(l10n_util::GetStringFUTF16( - IDS_EXTENSION_INSTALL_UNEXPECTED_VERSION, - base::ASCIIToUTF16(expected_version_->GetString()), - base::ASCIIToUTF16(extension->version()->GetString()))); - } - } else { - if (extension->version()->CompareTo(*expected_version_) < 0) { - return CrxInstallError(l10n_util::GetStringFUTF16( - IDS_EXTENSION_INSTALL_UNEXPECTED_VERSION, - base::ASCIIToUTF16(expected_version_->GetString() + "+"), - base::ASCIIToUTF16(extension->version()->GetString()))); - } - } + if (minimum_version_.IsValid() && + extension->version()->CompareTo(minimum_version_) < 0) { + return CrxInstallError(l10n_util::GetStringFUTF16( + IDS_EXTENSION_INSTALL_UNEXPECTED_VERSION, + base::ASCIIToUTF16(minimum_version_.GetString() + "+"), + base::ASCIIToUTF16(extension->version()->GetString()))); + } + + if (expected_version_.IsValid() && fail_install_if_unexpected_version_ && + !expected_version_.Equals(*extension->version())) { + return CrxInstallError(l10n_util::GetStringFUTF16( + IDS_EXTENSION_INSTALL_UNEXPECTED_VERSION, + base::ASCIIToUTF16(expected_version_.GetString()), + base::ASCIIToUTF16(extension->version()->GetString()))); } // Make sure the manifests match if we want to bypass the prompt. @@ -480,7 +476,7 @@ void CrxInstaller::CheckInstall() { SharedModuleInfo::GetImports(extension()); std::vector::const_iterator i; for (i = imports.begin(); i != imports.end(); ++i) { - Version version_required(i->minimum_version); + base::Version version_required(i->minimum_version); const Extension* imported_module = service->GetExtensionById(i->extension_id, true); if (imported_module && @@ -617,8 +613,8 @@ void CrxInstaller::ConfirmInstall() { return; } - current_version_ = ExtensionPrefs::Get(service->profile()) - ->GetVersionString(extension()->id()); + current_version_ = base::Version(ExtensionPrefs::Get(service->profile()) + ->GetVersionString(extension()->id())); if (client_ && (!allow_silent_install_ || !approved_) && @@ -680,16 +676,14 @@ void CrxInstaller::InstallUIAbort(bool user_initiated) { void CrxInstaller::CompleteInstall() { DCHECK(installer_task_runner_->RunsTasksOnCurrentThread()); - if (!current_version_.empty()) { - Version current_version(current_version_); - if (current_version.CompareTo(*(extension()->version())) > 0) { - ReportFailureFromFileThread(CrxInstallError( - CrxInstallError::ERROR_DECLINED, - l10n_util::GetStringUTF16( - extension()->is_app() ? IDS_APP_CANT_DOWNGRADE_VERSION - : IDS_EXTENSION_CANT_DOWNGRADE_VERSION))); - return; - } + if (current_version_.IsValid() && + current_version_.CompareTo(*(extension()->version())) > 0) { + ReportFailureFromFileThread(CrxInstallError( + CrxInstallError::ERROR_DECLINED, + l10n_util::GetStringUTF16( + extension()->is_app() ? IDS_APP_CANT_DOWNGRADE_VERSION + : IDS_EXTENSION_CANT_DOWNGRADE_VERSION))); + return; } // See how long extension install paths are. This is important on @@ -812,7 +806,10 @@ void CrxInstaller::ReportSuccessFromUIThread() { // We update the extension's granted permissions if the user already // approved the install (client_ is non NULL), or we are allowed to install // this silently. - if ((client_ || allow_silent_install_) && grant_permissions_) { + if ((client_ || allow_silent_install_) && + grant_permissions_ && + (!expected_version_.IsValid() || + expected_version_.Equals(*extension()->version()))) { PermissionsUpdater perms_updater(profile()); perms_updater.InitializePermissions(extension()); perms_updater.GrantActivePermissions(extension()); diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h index 43b54f2f40ba..9c109e657a47 100644 --- a/chrome/browser/extensions/crx_installer.h +++ b/chrome/browser/extensions/crx_installer.h @@ -133,9 +133,14 @@ class CrxInstaller bool hash_check_failed() const { return hash_check_failed_; } void set_hash_check_failed(bool val) { hash_check_failed_ = val; } - void set_expected_version(const Version& val) { - expected_version_.reset(new Version(val)); - expected_version_strict_checking_ = true; + // Set the exact version the installed extension should have. If + // |fail_install_if_unexpected| is true, installation will fail if the actual + // version doesn't match. If it is false, the installation will still + // be performed, but the extension will not be granted any permissions. + void set_expected_version(const base::Version& val, + bool fail_install_if_unexpected) { + expected_version_ = val; + fail_install_if_unexpected_version_ = fail_install_if_unexpected; } bool delete_source() const { return delete_source_; } @@ -206,7 +211,9 @@ class CrxInstaller const Extension* extension() { return install_checker_.extension().get(); } - const std::string& current_version() const { return current_version_; } + // The currently installed version of the extension, for updates. Will be + // invalid if this isn't an update. + const base::Version& current_version() const { return current_version_; } private: friend class ::ExtensionServiceTest; @@ -322,16 +329,20 @@ class CrxInstaller // the |expected_manifest_|. WebstoreInstaller::ManifestCheckLevel expected_manifest_check_level_; - // If non-NULL, contains the expected version of the extension we're - // installing. Important for external sources, where claiming the wrong - // version could cause unnecessary unpacking of an extension at every - // restart. - scoped_ptr expected_version_; + // If valid, specifies the minimum version we'll install. Installation will + // fail if the actual version is smaller. + base::Version minimum_version_; - // If true, the actual version should be same with the |expected_version_|, - // Otherwise the actual version should be equal to or newer than - // the |expected_version_|. - bool expected_version_strict_checking_; + // If valid, contains the expected version of the extension we're installing. + // Important for external sources, where claiming the wrong version could + // cause unnecessary unpacking of an extension at every restart. + // See also |fail_install_if_unexpected_version_|! + base::Version expected_version_; + + // If true, installation will fail if the actual version doesn't match + // |expected_version_|. If false, the extension will still be installed, but + // not granted any permissions. + bool fail_install_if_unexpected_version_; // Whether manual extension installation is enabled. We can't just check this // before trying to install because themes are special-cased to always be @@ -354,9 +365,9 @@ class CrxInstaller // transformations like localization have taken place. scoped_ptr original_manifest_; - // If non-empty, contains the current version of the extension we're + // If valid, contains the current version of the extension we're // installing (for upgrades). - std::string current_version_; + base::Version current_version_; // The icon we will display in the installation UI, if any. scoped_ptr install_icon_; diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index e1606e1b4a7a..9ccb01e36ba1 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -518,26 +518,30 @@ bool ExtensionService::UpdateExtension(const extensions::CRXFileInfo& file, return false; } - scoped_refptr installer( - CrxInstaller::Create(this, scoped_ptr())); + scoped_refptr installer(CrxInstaller::CreateSilent(this)); installer->set_expected_id(id); installer->set_expected_hash(file.expected_hash); int creation_flags = Extension::NO_FLAGS; if (pending_extension_info) { installer->set_install_source(pending_extension_info->install_source()); installer->set_allow_silent_install(true); - if (pending_extension_info->remote_install()) - installer->set_grant_permissions(false); - creation_flags = pending_extension_info->creation_flags(); - if (pending_extension_info->mark_acknowledged()) - external_install_manager_->AcknowledgeExternalExtension(id); - // If the extension came in disabled due to a permission increase, then // don't grant it all the permissions. crbug.com/484214 - if (extensions::ExtensionPrefs::Get(profile_)->HasDisableReason( - id, Extension::DISABLE_PERMISSIONS_INCREASE)) { + bool has_permissions_increase = + extensions::ExtensionPrefs::Get(profile_)->HasDisableReason( + id, Extension::DISABLE_PERMISSIONS_INCREASE); + const base::Version& expected_version = pending_extension_info->version(); + if (has_permissions_increase || + pending_extension_info->remote_install() || + !expected_version.IsValid()) { installer->set_grant_permissions(false); + } else { + installer->set_expected_version(expected_version, + false /* fail_install_if_unexpected */); } + creation_flags = pending_extension_info->creation_flags(); + if (pending_extension_info->mark_acknowledged()) + external_install_manager_->AcknowledgeExternalExtension(id); } else if (extension) { installer->set_install_source(extension->location()); } @@ -1731,8 +1735,6 @@ void ExtensionService::OnExtensionInstalled( } else { // Requirement is supported now, remove the corresponding disable reason // instead. - extension_prefs_->RemoveDisableReason( - id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT); disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT; } @@ -1741,8 +1743,6 @@ void ExtensionService::OnExtensionInstalled( if (extensions::ExtensionManagementFactory::GetForBrowserContext(profile()) ->CheckMinimumVersion(extension, nullptr)) { // And remove the corresponding disable reason. - extension_prefs_->RemoveDisableReason( - id, Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY); disable_reasons &= ~Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY; } @@ -2163,7 +2163,8 @@ bool ExtensionService::OnExternalExtensionFileFound( scoped_refptr installer(CrxInstaller::CreateSilent(this)); installer->set_install_source(location); installer->set_expected_id(id); - installer->set_expected_version(*version); + installer->set_expected_version(*version, + true /* fail_install_if_unexpected */); installer->set_install_cause(extension_misc::INSTALL_CAUSE_EXTERNAL_FILE); installer->set_install_immediately(install_immediately); installer->set_creation_flags(creation_flags); diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 25afcad5b550..368f1af09ae6 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -2948,6 +2948,7 @@ TEST_F(ExtensionServiceTest, AddPendingExtensionFromSync) { service()->pending_extension_manager()->AddFromSync( kFakeId, kFakeUpdateURL, + base::Version(), &IsExtension, kFakeRemoteInstall, kFakeInstalledByCustodian)); @@ -2972,18 +2973,21 @@ TEST_F(ExtensionServiceTest, AddPendingExtensionFromSync) { namespace { const char kGoodId[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf"; const char kGoodUpdateURL[] = "http://good.update/url"; +const char kGoodVersion[] = "1"; const bool kGoodIsFromSync = true; const bool kGoodRemoteInstall = false; const bool kGoodInstalledByCustodian = false; } // namespace -// Test updating a pending extension. +// Test installing a pending extension (this goes through +// ExtensionService::UpdateExtension). TEST_F(ExtensionServiceTest, UpdatePendingExtension) { InitializeEmptyExtensionService(); EXPECT_TRUE( service()->pending_extension_manager()->AddFromSync( kGoodId, GURL(kGoodUpdateURL), + base::Version(kGoodVersion), &IsExtension, kGoodRemoteInstall, kGoodInstalledByCustodian)); @@ -2995,7 +2999,37 @@ TEST_F(ExtensionServiceTest, UpdatePendingExtension) { EXPECT_FALSE(service()->pending_extension_manager()->IsIdPending(kGoodId)); const Extension* extension = service()->GetExtensionById(kGoodId, true); - ASSERT_TRUE(extension); + EXPECT_TRUE(extension); +} + +TEST_F(ExtensionServiceTest, UpdatePendingExtensionWrongVersion) { + InitializeEmptyExtensionService(); + base::Version other_version("0.1"); + ASSERT_TRUE(other_version.IsValid()); + ASSERT_FALSE(other_version.Equals(base::Version(kGoodVersion))); + EXPECT_TRUE( + service()->pending_extension_manager()->AddFromSync( + kGoodId, + GURL(kGoodUpdateURL), + other_version, + &IsExtension, + kGoodRemoteInstall, + kGoodInstalledByCustodian)); + EXPECT_TRUE(service()->pending_extension_manager()->IsIdPending(kGoodId)); + + base::FilePath path = data_dir().AppendASCII("good.crx"); + // After installation, the extension should be disabled, because it's missing + // permissions. + UpdateExtension(kGoodId, path, DISABLED); + + EXPECT_TRUE( + ExtensionPrefs::Get(profile())->DidExtensionEscalatePermissions(kGoodId)); + + // It should still have been installed though. + EXPECT_FALSE(service()->pending_extension_manager()->IsIdPending(kGoodId)); + + const Extension* extension = service()->GetExtensionById(kGoodId, true); + EXPECT_TRUE(extension); } namespace { @@ -3011,7 +3045,7 @@ bool IsTheme(const Extension* extension) { TEST_F(ExtensionServiceTest, DISABLED_UpdatePendingTheme) { InitializeEmptyExtensionService(); EXPECT_TRUE(service()->pending_extension_manager()->AddFromSync( - theme_crx, GURL(), &IsTheme, false, false)); + theme_crx, GURL(), base::Version(), &IsTheme, false, false)); EXPECT_TRUE(service()->pending_extension_manager()->IsIdPending(theme_crx)); base::FilePath path = data_dir().AppendASCII("theme.crx"); @@ -3074,6 +3108,7 @@ TEST_F(ExtensionServiceTest, UpdatePendingExternalCrxWinsOverSync) { service()->pending_extension_manager()->AddFromSync( kGoodId, GURL(kGoodUpdateURL), + base::Version(), &IsExtension, kGoodRemoteInstall, kGoodInstalledByCustodian)); @@ -3105,6 +3140,7 @@ TEST_F(ExtensionServiceTest, UpdatePendingExternalCrxWinsOverSync) { service()->pending_extension_manager()->AddFromSync( kGoodId, GURL(kGoodUpdateURL), + base::Version(), &IsExtension, kGoodRemoteInstall, kGoodInstalledByCustodian)); @@ -3122,7 +3158,7 @@ TEST_F(ExtensionServiceTest, UpdatePendingExternalCrxWinsOverSync) { TEST_F(ExtensionServiceTest, UpdatePendingCrxThemeMismatch) { InitializeEmptyExtensionService(); EXPECT_TRUE(service()->pending_extension_manager()->AddFromSync( - theme_crx, GURL(), &IsExtension, false, false)); + theme_crx, GURL(), base::Version(), &IsExtension, false, false)); EXPECT_TRUE(service()->pending_extension_manager()->IsIdPending(theme_crx)); @@ -3147,6 +3183,7 @@ TEST_F(ExtensionServiceTest, UpdatePendingExtensionFailedShouldInstallTest) { service()->pending_extension_manager()->AddFromSync( kGoodId, GURL(kGoodUpdateURL), + base::Version(), &IsTheme, kGoodRemoteInstall, kGoodInstalledByCustodian)); @@ -7659,6 +7696,7 @@ class ExtensionSourcePriorityTest : public ExtensionServiceTest { return service()->pending_extension_manager()->AddFromSync( crx_id_, GURL(kGoodUpdateURL), + base::Version(), &IsExtension, kGoodRemoteInstall, kGoodInstalledByCustodian); diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc index 93f89ee6e712..ba875ac25fa4 100644 --- a/chrome/browser/extensions/extension_sync_service.cc +++ b/chrome/browser/extensions/extension_sync_service.cc @@ -392,6 +392,7 @@ void ExtensionSyncService::ApplySyncData( if (!extension_service()->pending_extension_manager()->AddFromSync( id, extension_sync_data.update_url(), + extension_sync_data.version(), extensions::sync_helper::IsSyncable, extension_sync_data.remote_install(), extension_sync_data.installed_by_custodian())) { diff --git a/chrome/browser/extensions/pending_extension_manager.cc b/chrome/browser/extensions/pending_extension_manager.cc index 7d0546210b9b..632ea4da55f7 100644 --- a/chrome/browser/extensions/pending_extension_manager.cc +++ b/chrome/browser/extensions/pending_extension_manager.cc @@ -89,6 +89,7 @@ bool PendingExtensionManager::HasPendingExtensionFromSync() const { bool PendingExtensionManager::AddFromSync( const std::string& id, const GURL& update_url, + const base::Version& version, PendingExtensionInfo::ShouldAllowInstallPredicate should_allow_install, bool remote_install, bool installed_by_custodian) { @@ -121,7 +122,7 @@ bool PendingExtensionManager::AddFromSync( return AddExtensionImpl(id, std::string(), update_url, - Version(), + version, should_allow_install, kIsFromSync, kSyncLocation, diff --git a/chrome/browser/extensions/pending_extension_manager.h b/chrome/browser/extensions/pending_extension_manager.h index f422d28c4cb4..f500a8e7b151 100644 --- a/chrome/browser/extensions/pending_extension_manager.h +++ b/chrome/browser/extensions/pending_extension_manager.h @@ -71,12 +71,12 @@ class PendingExtensionManager { // if the extension is pending from another source which overrides // sync installs (such as a policy extension) or if the extension // is already installed. - // - // TODO(akalin): Replace |install_silently| with a list of - // pre-enabled permissions. + // After installation, the extension will be granted permissions iff + // |version| is valid and matches the actual installed version. bool AddFromSync( const std::string& id, const GURL& update_url, + const base::Version& version, PendingExtensionInfo::ShouldAllowInstallPredicate should_allow_install, bool remote_install, bool installed_by_custodian); diff --git a/chrome/browser/themes/theme_syncable_service.cc b/chrome/browser/themes/theme_syncable_service.cc index 1e2ff50eaf70..cc169a755a60 100644 --- a/chrome/browser/themes/theme_syncable_service.cc +++ b/chrome/browser/themes/theme_syncable_service.cc @@ -5,6 +5,7 @@ #include "chrome/browser/themes/theme_syncable_service.h" #include "base/strings/stringprintf.h" +#include "base/version.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/themes/theme_service.h" @@ -235,6 +236,7 @@ void ThemeSyncableService::SetCurrentThemeFromThemeSpecifics( if (!extensions_service->pending_extension_manager()->AddFromSync( id, update_url, + base::Version(), &IsTheme, kRemoteInstall, kInstalledByCustodian)) { -- 2.11.4.GIT