diff --git a/src/gui/updater/ocupdater.cpp b/src/gui/updater/ocupdater.cpp index 27507a7c24e85..07d7460cb987d 100644 --- a/src/gui/updater/ocupdater.cpp +++ b/src/gui/updater/ocupdater.cpp @@ -26,7 +26,6 @@ const auto updateAvailableC = QStringLiteral("Updater/updateAvailable"); const auto updateTargetVersionC = QStringLiteral("Updater/updateTargetVersion"); const auto updateTargetVersionStringC = QStringLiteral("Updater/updateTargetVersionString"); const auto autoUpdateAttemptedC = QStringLiteral("Updater/autoUpdateAttempted"); -const auto msiLogFileNameC = QStringLiteral("msi.log"); } UpdaterScheduler::UpdaterScheduler(QObject *parent) @@ -223,7 +222,7 @@ void OCUpdater::slotStartInstaller() return QDir::toNativeSeparators(path); }; - QString msiLogFile = cfg.configPath() + msiLogFileNameC; + const auto msiLogFile = cfg.msiLogFilePath(); QString command = QStringLiteral("&{msiexec /i '%1' /L*V '%2'| Out-Null ; &'%3'}") .arg(preparePathForPowershell(updateFile)) .arg(preparePathForPowershell(msiLogFile)) @@ -305,30 +304,11 @@ void NSISUpdater::slotWriteFile() void NSISUpdater::wipeUpdateData() { - ConfigFile cfg; - QSettings settings(cfg.configFile(), QSettings::IniFormat); - QString updateFileName = settings.value(updateAvailableC).toString(); - if (!updateFileName.isEmpty()) { - if (QFile::remove(updateFileName)) { - qCInfo(lcUpdater) << "Removed updater file:" << updateFileName; - } else { - qCWarning(lcUpdater) << "Failed to remove updater file:" << updateFileName; - } - } - // Also try to remove the msi log file (created when running msiexec) - const auto msiLogFileName = QString{cfg.configPath() + msiLogFileNameC}; - if (QFile::exists(msiLogFileName)) { - if (QFile::remove(msiLogFileName)) { - qCInfo(lcUpdater) << "Removed msi log file:" << msiLogFileName; - } else { - qCWarning(lcUpdater) << "Failed to remove msi log file:" << msiLogFileName; - } - } - - settings.remove(updateAvailableC); - settings.remove(updateTargetVersionC); - settings.remove(updateTargetVersionStringC); - settings.remove(autoUpdateAttemptedC); + // Deliberately delegated: ConfigFile::cleanUpdaterConfiguration() is also + // reached from Application::configVersionMigration() on the first start of + // a newly installed version, and both paths must remove the installer, not + // just the keys that point at it. + ConfigFile().cleanUpdaterConfiguration(); } void NSISUpdater::slotDownloadFinished() diff --git a/src/libsync/configfile.cpp b/src/libsync/configfile.cpp index 920ff4aa9ef20..27cdbe6f6fd7f 100644 --- a/src/libsync/configfile.cpp +++ b/src/libsync/configfile.cpp @@ -434,10 +434,38 @@ QString ConfigFile::excludeFileFromSystem() return fi.absoluteFilePath(); } +namespace { +void removeUpdaterArtifact(const QString &path) +{ + if (path.isEmpty() || !QFile::exists(path)) { + return; + } + + if (QFile::remove(path)) { + qCInfo(lcConfigFile) << "Removed leftover updater file:" << path; + } else { + qCWarning(lcConfigFile) << "Failed to remove leftover updater file:" << path; + } +} +} + +QString ConfigFile::msiLogFilePath() const +{ + return configPath() + QStringLiteral("msi.log"); +} + void OCC::ConfigFile::cleanUpdaterConfiguration() { QSettings settings(configFile(), QSettings::IniFormat); settings.beginGroup("Updater"); + + // The config is the only record of where the downloaded installer lives. + // Delete it before dropping the keys, or every version change orphans + // another installer in the config folder with nothing left to find it by. + // See https://github.com/nextcloud/desktop/issues/7009 + removeUpdaterArtifact(settings.value("updateAvailable").toString()); + removeUpdaterArtifact(msiLogFilePath()); + settings.remove("autoUpdateAttempted"); settings.remove("updateTargetVersion"); settings.remove("updateTargetVersionString"); diff --git a/src/libsync/configfile.h b/src/libsync/configfile.h index 552273b2ef387..d6d44fafbea9d 100644 --- a/src/libsync/configfile.h +++ b/src/libsync/configfile.h @@ -42,6 +42,8 @@ class OWNCLOUDSYNC_EXPORT ConfigFile static QString excludeFileFromSystem(); // doesn't access config dir void cleanUpdaterConfiguration(); + /** Path of the log msiexec writes while installing an update (Windows). */ + [[nodiscard]] QString msiLogFilePath() const; void cleanupGlobalNetworkConfiguration(); /** diff --git a/test/testupdater.cpp b/test/testupdater.cpp index bf8968114675d..d91212328e81b 100644 --- a/test/testupdater.cpp +++ b/test/testupdater.cpp @@ -51,6 +51,53 @@ private Q_SLOTS: QVERIFY(currVersion < highVersion); } + // Reproduces #7009: on the first start of a newly installed version the + // leftover installer must be gone. Application::configVersionMigration() + // (src/gui/application.cpp:161-164) drops the Updater/* keys before + // main() calls handleStartup() (src/gui/main.cpp:104 vs 142), so the + // cleanup in NSISUpdater::handleStartup() never sees the artifact. + void testLeftoverInstallerIsRemovedAfterVersionChange() + { + QTemporaryDir tempDir; + QVERIFY(tempDir.isValid()); + QVERIFY(ConfigFile::setConfDir(tempDir.path())); + + ConfigFile cfg; + const auto writeFile = [](const QString &path, const QByteArray &contents) { + QFile f(path); + return f.open(QIODevice::WriteOnly) && f.write(contents) == contents.size(); + }; + + // a previous run downloaded an installer and msiexec left its log behind + const auto installer = FileSystem::joinPath(cfg.configPath(), "Nextcloud-1.0.0-x64.msi"_L1); + const auto msiLog = FileSystem::joinPath(cfg.configPath(), "msi.log"_L1); + QVERIFY(writeFile(installer, "this would be the installer"_ba)); + QVERIFY(writeFile(msiLog, "this would be the msiexec log"_ba)); + + { + QSettings settings(cfg.configFile(), QSettings::IniFormat); + settings.setValue("Updater/updateAvailable"_L1, installer); + settings.setValue("Updater/updateTargetVersion"_L1, "1.0.0"_L1); + settings.setValue("Updater/updateTargetVersionString"_L1, "1.0.0"_L1); + settings.setValue("Updater/autoUpdateAttempted"_L1, true); + settings.sync(); + } + + // the update was installed: the running binary is newer than the target + QVERIFY(Updater::Helper::currentVersionToInt() > Updater::Helper::stringVersionToInt("1.0.0"_L1)); + + // what Application::configVersionMigration() does on that very first + // start, before the updater gets a chance to look at its own state + cfg.setClientVersionString("1.0.0"_L1); + cfg.cleanUpdaterConfiguration(); + + NSISUpdater updater(QUrl("http://localhost:1/updateinfo.xml"_L1)); + updater.handleStartup(); + + QVERIFY(!QFileInfo::exists(installer)); + QVERIFY(!QFileInfo::exists(msiLog)); + } + #ifdef HAVE_QHTTPSERVER void testUpdaterDownloadRedirect() {