From fcfc372511944dbddbef6ae8e172352a91c15fae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Sun, 21 Jun 2026 22:34:17 +0200 Subject: [PATCH] fix(installer): validate app archive before deleting installed app on update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installer::updateApp() removed the currently-installed app directory (`rmdirr($basedir)`) before copying the new version from the extracted archive, and — unlike installApp() — never checked that the archive actually contained a directory named after the app id. An invalid tarball (top-level directory not matching the app id) therefore deleted the installed app and then tried to copy from a non-existent source, leaving the app broken and surfacing a misleading error instead of a clear message (issue #34669). Mirror installApp()'s guard in updateApp(): compute the expected app directory inside the extract dir and, BEFORE the destructive removal of the installed app, verify it exists. If not, clean up the extract dir and throw the same translated "Archive does not contain a directory named %s" error installApp() uses. The installed app is now left intact on invalid input, and the success path for valid archives is unchanged. Adds a regression test (with an invalid-archive fixture) asserting the clear error is thrown and the previously-installed app is not deleted. Fixes #34669 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- lib/private/Installer.php | 19 +++++++++--- tests/data/testapp_invalid.zip | Bin 0 -> 622 bytes tests/lib/InstallerTest.php | 51 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 tests/data/testapp_invalid.zip diff --git a/lib/private/Installer.php b/lib/private/Installer.php index cc97e1b0edce..cd16cb35e7d4 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -214,6 +214,8 @@ public static function isInstalled($app) { * "\OC::$server->getAppConfig()->getValue($appid, 'installed_version')" */ public static function updateApp($info= [], $isShipped=false) { + $l = \OC::$server->getL10N('lib'); + list($extractDir, $path) = self::downloadApp($info); $info = self::checkAppsIntegrity($info, $extractDir, $path, $isShipped); @@ -230,16 +232,25 @@ public static function updateApp($info= [], $isShipped=false) { $basedir = $currentDir; } - if (\is_dir($basedir)) { - OC_Helper::rmdirr($basedir); - } - $appInExtractDir = $extractDir; if (\substr($extractDir, -1) !== '/') { $appInExtractDir .= '/'; } $appInExtractDir .= $info['id']; + + // Verify the archive actually contains the app directory before + // removing the currently installed app, otherwise an invalid archive + // would leave the user without the previously installed app. + if (!\is_dir($appInExtractDir)) { + OC_Helper::rmdirr($extractDir); + throw new \Exception($l->t("Archive does not contain a directory named %s", $info['id'])); + } + + if (\is_dir($basedir)) { + OC_Helper::rmdirr($basedir); + } + OC_Helper::copyr($appInExtractDir, $basedir); OC_Helper::rmdirr($extractDir); diff --git a/tests/data/testapp_invalid.zip b/tests/data/testapp_invalid.zip new file mode 100644 index 0000000000000000000000000000000000000000..c4ef9813e9bf9bc0f5de89198568f1ef1c09f438 GIT binary patch literal 622 zcmWIWW@h1H00F)&S7X2oC;<{JFUrqL&r8fr)epd_P6$O^VnIP>URpjL^^#cC1NG?@ zWE6m`5dkXT06WM0*qOauK;Bs(mcptHWS3q=ZjNu?M!rJ^JTB>OE5S{aubM&-~Opv#!p)ySE%)m(Lz@;N8nW*#`A9Q#Nrj zZplzQr_;ip=-YUdYo5l<6N;y&7k%{pzj8|2wkJ9dzna~13|DWA{CwJ-WiPkm#CzKA zhlF2fwZ3!S`2O62)q$JVzWK`(;LXS+$Ba8jf!+WD0fxVhAR0MassertNotEquals($oldVersionNumber, $newVersionNumber); } + /** + * Tests that updating with an archive whose top-level directory is not + * named after the app id fails with a clear error message and, crucially, + * does not destroy the already-installed app. + */ + public function testUpdateWithInvalidArchiveKeepsInstalledApp() { + // Install the valid app first + $pathOfOldTestApp = __DIR__ . '/../data/testapp.zip'; + $oldTmp = \OC::$server->getTempManager()->getTemporaryFile('.zip'); + \OC_Helper::copyr($pathOfOldTestApp, $oldTmp); + $oldData = [ + 'path' => $oldTmp, + 'source' => 'path', + 'appdata' => [ + 'id' => 'testapp', + 'level' => 100, + ] + ]; + Installer::installApp($oldData); + $this->assertTrue(Installer::isInstalled(self::$appid)); + $appPath = \OC_App::getAppPath(self::$appid); + $this->assertNotFalse($appPath); + $this->assertTrue(\is_dir($appPath)); + + // Attempt to update with an invalid archive: its top-level directory is + // "wrongname" although info.xml declares the id "testapp". + $pathOfInvalidTestApp = __DIR__ . '/../data/testapp_invalid.zip'; + $invalidTmp = \OC::$server->getTempManager()->getTemporaryFile('.zip'); + \OC_Helper::copyr($pathOfInvalidTestApp, $invalidTmp); + $invalidData = [ + 'path' => $invalidTmp, + 'source' => 'path', + 'appdata' => [ + 'id' => 'testapp', + 'level' => 100, + ] + ]; + + $thrown = false; + try { + Installer::updateApp($invalidData); + } catch (\Exception $e) { + $thrown = true; + $this->assertStringStartsWith('Archive does not contain a directory named', $e->getMessage()); + } + $this->assertTrue($thrown, 'updateApp() should throw for an invalid archive'); + + // The previously installed app must still be present and untouched. + $this->assertTrue(\is_dir($appPath), 'The installed app directory must not be deleted on invalid update'); + } + /** * Tests that update is installed into writable app dir if the original app dir is not writable */