nextcloud / nextcloud/server

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

Open
#62,853 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

1. to develop 33-feedback bug feature: dav high hotspot: file transfer performance
Dominant language
PHP
Stars
36.9k
Forks
5.2k
Avg merge
2d 3h
Merged PRs (30d)
713

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in apps/dav/lib/Connector/Sabre/File.php around the needsPartFile guard, then inspect lib/private/Files/Storage/Common.php and the Local storage behavior. Run the provided Nextcloud bootstrap reproduction for existing and new files; done means both cases retain part-file staging while the fallback remains available when the containing directory cannot create files.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.