fix(installer): validate app archive before deleting installed app on update (#34669)

[DeepDiver1975]

Summary

Fixes #34669 — updating an app with an invalid tarball produced a misleading error and could destroy the already-installed app.

Installer::updateApp() did this:

if (\is_dir($basedir)) {
    OC_Helper::rmdirr($basedir);   // delete the installed app FIRST
}
$appInExtractDir = $extractDir . '/' . $info['id'];
OC_Helper::copyr($appInExtractDir, $basedir);  // copy from a path that may not exist

Unlike installApp() — which validates that the extracted archive contains a directory named after the app id before copying — updateApp() had no such guard. So an archive whose top-level directory does not match the app id caused the installed app to be deleted and then copied from a non-existent source, leaving the app broken and surfacing a confusing error instead of a clear one.

Change

Mirror installApp()'s guard inside updateApp(), and crucially run it before the destructive removal:

• Compute the expected app directory inside the extract dir ($appInExtractDir) up front.
• If it does not exist, clean up the extract dir and throw the same translated message installApp() uses: "Archive does not contain a directory named %s".
• Only then proceed to remove the installed app and copy the new version.

Result:

• Invalid archive → clear, correct error message, and the installed app is left intact (no more data loss).
• Valid archive → behavior is identical to before.

Tests

Adds InstallerTest::testUpdateWithInvalidArchiveKeepsInstalledApp() plus a fixture tests/data/testapp_invalid.zip (top-level dir wrongname, info.xml id testapp). It installs the valid app, attempts an update with the invalid archive, and asserts (a) the "Archive does not contain a directory named …" exception is thrown and (b) the previously-installed app directory still exists.

Tagged with the existing InstallerTest fixtures/helpers. php -l clean; the full PHPUnit suite was not run in the preparation environment (needs full ownCloud bootstrap/DB).

Note: owncloud/core is in maintenance mode; this targets installations still on classic ownCloud 10.x. Issue labelled junior job / p4-low.

---

🤖 This PR was prepared by the Claude Code review agent from the analysis of #34669. Please review carefully before merging.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

🤖 Automated code review by Claude Code review agent

Overview

The change mirrors installApp()'s "archive contains the expected app directory" guard into updateApp(), and — the important part — places it before the destructive OC_Helper::rmdirr($basedir). An app archive whose top-level directory does not match the app id now throws a clear, translated "Archive does not contain a directory named %s" error and leaves the installed app intact, instead of deleting it and copying from a non-existent source. Net: 66/-4 across Installer.php, a new regression test, and a binary fixture.

Correctness

• Guard ordering is correct. The if (!\is_dir($appInExtractDir)) check is now emitted before if (\is_dir($basedir)) { rmdirr($basedir); }. The destructive removal can no longer run for an invalid archive. ✔
• No path mismatch. The guard tests the exact same $appInExtractDir variable that the subsequent OC_Helper::copyr($appInExtractDir, $basedir) consumes — guard and copy source are guaranteed identical. ✔
• $l is properly obtained. $l = \OC::$server->getL10N('lib'); is added at the top of updateApp(), consistent with installApp()/downloadApp()/checkAppsIntegrity() in the same file. ✔
• Valid-archive success path unchanged. For a well-formed archive $appInExtractDir exists, the new branch is skipped, and the original remove→copy→cleanup→OC_App::updateApp() flow runs exactly as before. ✔
• checkAppsIntegrity() does not interfere. It takes $extractDir by value (no &), so its internal $extractDir .= '/'.$folder (descending into wrongname/) is local; the $extractDir seen by updateApp() remains the temp-folder root. Thus $appInExtractDir = <root>/testapp correctly does NOT exist (only wrongname/ does) and the guard fires. ✔
• Minor improvement over installApp(): the new guard uses \is_dir(...) whereas installApp() uses \file_exists(...). is_dir is marginally stricter (rejects a same-named regular file) and is the right predicate here. Harmless divergence.

Tests

Traced testUpdateWithInvalidArchiveKeepsInstalledApp() end to end against the fixture (wrongname/appinfo/info.xml, <id>testapp</id>, <version>0.9</version>, identical to the valid testapp.zip apart from the top-level dir name):

1. installApp() succeeds (id resolved from info.xml = testapp); isInstalled() true; getAppPath('testapp') returns the on-disk path (resolved by filesystem presence, independent of enabled state). ✔
2. updateApp(invalid): checkAppsIntegrity() returns id=testapp cleanly — isAppCompatible passes (same content as the valid app used by the existing green tests) and the version-mismatch check is skipped because appdata carries no version. So no earlier/different exception is thrown that would defeat assertStringStartsWith('Archive does not contain a directory named', …). The guard's exception is the one observed. ✔
3. Post-failure assertTrue(\is_dir($appPath)) confirms the installed app directory survives the failed update — the regression assertion that matters. ✔

The test follows existing file conventions (getTemporaryFile('.zip'), OC_Helper::copyr, appdata id=testapp/level=100), matching testUpdateIntoWritableAppDir(). I expect it to pass in CI. The fixture is a valid 4-entry zip with the documented structure. ✔

Minor

• The $thrown flag + try/catch pattern works but @expectedException/expectException() is the more idiomatic PHPUnit form. Not blocking — the manual pattern is needed here to also assert post-state after the throw, so it is justified.
• Inline-comment and message wording deliberately match installApp(); good for consistency.

Verdict

approve — No blocking issues. The guard is correctly ordered before the destructive removal, uses the same path as the copy, and the regression test genuinely exercises and would pass. This is a clean, faithful backport of the installApp() safeguard and closes a real data-loss path in updateApp().