dav: part file staging is skipped on every overwrite — isCreatable() is a directory-only test
Nobody has claimed this yet.
- 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()returnstrue) forbidden_filename_extensions: unset (default['.filepart'])part_file_in_storage: unset (defaulttrue) — setting it tofalsedoes not help, as the relocated part file is still a fileocc integrity:check-core: clean, no local modifications
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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