Update to invalid app tarball fails with wrong error message

[PVince81]

Steps

1. Install an app, for example activity-2.4.1 from tarball: occ market:install -l activity-2.4.1.tar.gz
2. Go to Github releases and download the wrong tarball, not the release one but the github sources: https://github.com/owncloud/activity/archive/v2.4.2.tar.gz
3. occ market:upgrade -l activity-2.4.2.tar.gz <= the wrong tarball

Expected result

Upgrade fails with proper error message.

Actual result

Original app is deleted but the new app isn't there.
This basically deletes the app...

Version

stable10 / 10.1.0

Note: the "wrong" tarball doesn't have "activity" as root path but "activity-2.4.2".
We should extend the update code to verify that the tarball contains an "activity" folder just like the install process does.

[PVince81]

Ref: the update happens here: https://github.com/owncloud/core/blob/v10.1.0/lib/private/Installer.php#L222

The folder gets extracted properly but later on it doesn't get copied over since it has the wrong name.

But at this point the old version was already detected through rmdirr.

[ownclouders]

GitMate.io thinks possibly related issues are #21366 (Update), #20661 (Update), #18826 (update fail), #20593 (Update failed), and #11017 (Update fails).

[jnweiger]

also, when using a normal occ wapper:

• when using an occ wapper, a relative path to the tar ball may or may not work.
• the path to the tar ball must be fully readable by the www-data user. Tar ball in root's home does not work, even when running as root.

[DeepDiver1975]

Classification: confirmed-bug (low severity, as labelled p4-low / junior job).

Confirmed on current master (lib/private/Installer.php). @PVince81's 2019 diagnosis is still accurate — here is the located cause.

Root cause

Installer::checkAppsIntegrity() tolerates a tarball whose top-level folder name does not match the app id: when appinfo/info.xml isn't at the archive root it searches one level of subdirectories and locally re-points $extractDir to that subfolder (Installer.php L316–328). But that adjusted $extractDir is not returned — only $info is.

installApp() handles the consequence correctly: after building the source path it verifies it exists before copying, and only an empty freshly-created dir is at risk:

$extractDir .= '/' . $info['id'];
if (\!\file_exists($extractDir)) {
    OC_Helper::rmdirr($basedir);
    throw new \Exception($l->t("Archive does not contain a directory named %s", $info['id']));
}
OC_Helper::copyr($extractDir, $basedir);

(L119–124)

updateApp() does not have this guard (L216–246):

if (\is_dir($basedir)) {
    OC_Helper::rmdirr($basedir);          // existing app is deleted here
}
$appInExtractDir = $extractDir;
if (\substr($extractDir, -1) \!== '/') {
    $appInExtractDir .= '/';
}
$appInExtractDir .= $info['id'];          // builds .../<extractDir>/<appid>
OC_Helper::copyr($appInExtractDir, $basedir);   // no existence check on source

So for an invalid tarball (top folder name ≠ app id) updateApp first rmdirrs the currently-installed app, then tries to copyr from a path that does not exist. The user sees a confusing copy/filesystem error instead of the clear "Archive does not contain a directory named <appid>" message that installApp produces — and the previously-working app has already been removed.

Suggested fix (junior-friendly)

Mirror the installApp guard in updateApp: after computing $appInExtractDir, throw the same clear "Archive does not contain a directory named %s" error before the rmdirr($basedir) of the existing app, so a bad tarball is rejected without destroying the installed version. (Validating the source path up front also avoids the data-loss side effect.)

🤖 Automated issue analysis by Claude Code review agent.

[DeepDiver1975]

🤖 Follow-up: opened a fix — #41643. It mirrors installApp()'s archive-directory guard in updateApp(), but runs the check before removing the installed app — so an invalid tarball now yields the correct "Archive does not contain a directory named …" error instead of deleting the installed app and failing with a misleading message. Includes a regression test + fixture.