dav: part file staging is skipped on every overwrite — isCreatable() is a directory-only test

[skylabgit]

Summary

apps/dav/lib/Connector/Sabre/File.php disables .part staging for every WebDAV overwrite of an existing file, because it calls View::isCreatable() on a file path while Common::isCreatable() is implemented as a directory-only test. The result is that overwrites are streamed directly onto the final path, so an upload interrupted mid-stream truncates the live file.

New-file uploads are unaffected and still staged correctly.

The code

apps/dav/lib/Connector/Sabre/File.php (line 138 on 33.0.2, identical on master at time of writing):

if ($needsPartFile) {
    $transferId = \rand();
    $partFilePath = $this->getPartFileBasePath($this->path) . '.ocTransferId' . $transferId . '.part';

    if (!$view->isCreatable($partFilePath) && $view->isUpdatable($this->path)) {
        $needsPartFile = false;
    }
}

lib/private/Files/Storage/Common.php:

public function isCreatable(string $path): bool {
    if ($this->is_dir($path) && $this->isUpdatable($path)) {
        return true;
    }
    return false;
}

$partFilePath is a file that does not exist yet, so is_dir() is false and isCreatable($partFilePath) returns false unconditionally — for every file, on every storage that inherits Common::isCreatable() (including Local, which does not override it).

The guard therefore reduces to:

if ($view->isUpdatable($this->path)) {
    $needsPartFile = false;
}

upload	isUpdatable($this->path)	part file used?
new file (target absent)	false	yes — staged
overwrite (target exists)	true	no — streamed onto the live file

Steps to reproduce

Bare install, Local storage, single user, no external storage. Run under the Nextcloud bootstrap as the web user:

require_once '/var/www/nextcloud/lib/base.php';
\OC_User::setUserId('alice');
\OC_Util::setupFS('alice');
$view = \OC\Files\Filesystem::getView();   // rooted at /alice/files

// mirror getPartFileBasePath()
function partPath(string $p): string {
    $b = basename($p);
    return substr($p, 0, strlen($p) - strlen($b)) . hash('xxh128', $b) . '.ocTransferId999.part';
}

foreach (['/existing.txt' /* exists */, '/brand-new.txt' /* does not */] as $t) {
    $part = partPath($t);
    printf("%-16s isCreatable(part)=%s isUpdatable(target)=%s -> usesPartFile=%s\n",
        $t,
        var_export($view->isCreatable($part), true),
        var_export($view->isUpdatable($t), true),
        var_export(!(!$view->isCreatable($part) && $view->isUpdatable($t)), true));
}

Actual result

/existing.txt    isCreatable(part)=false isUpdatable(target)=true  -> usesPartFile=false
/brand-new.txt   isCreatable(part)=false isUpdatable(target)=false -> usesPartFile=true

Expected result

usesPartFile=true in both cases. The part file is genuinely writable — $view->file_put_contents($part, 'probe') succeeds (returns 5) for both cases on the same instance, so nothing about the storage prevents staging. Only the isCreatable() test does.

Suggested fix

Test the directory the part file will be created in, rather than the part file itself:

-if (!$view->isCreatable($partFilePath) && $view->isUpdatable($this->path)) {
+if (!$view->isCreatable(dirname($partFilePath)) && $view->isUpdatable($this->path)) {
     $needsPartFile = false;
 }

This preserves the intent of the guard — fall back to a direct write when the part file genuinely cannot be created, e.g. on an upload-only share — while restoring atomic staging in the normal case.

Impact and mitigation

files_versions snapshots the previous content via emitPreHooks() before the direct write, so a truncated overwrite is usually recoverable from version history. That limits the severity considerably, but it is a recovery path rather than the atomicity the part file is meant to provide, and it does not help where versioning is disabled or retention has expired.

Server configuration

• Nextcloud: 33.0.2.2 (also verified against master)
• Storage: Local (data directory on a CIFS mount; needsPartFile() returns true)
• forbidden_filename_extensions: unset (default ['.filepart'])
• part_file_in_storage: unset (default true) — setting it to false does not help, as the relocated part file is still a file
• occ integrity:check-core: clean, no local modifications