Skip to content

fix(dav): finalize upload metadata before post-write hooks while preserving shared hook locks #60453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshtrichards wants to merge 4 commits into
base: master
Choose a base branch
from
+71 −49
Open

fix(dav): finalize upload metadata before post-write hooks while preserving shared hook locks #60453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7a850c
fix(dav): finalize upload metadata before downgrading lock
joshtrichards May 15, 2026
3ab1bd4
fix(dav): preserves the old hook-accessibility contract while still i…
joshtrichards May 17, 2026
2d52412
test(dav): add clarifying comments to testPutLocking test
joshtrichards May 17, 2026
b08338d
test(dav): make sure FileInfo is constructed with checksum
joshtrichards May 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 50 additions & 41 deletions apps/dav/lib/Connector/Sabre/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use OCP\Files\LockNotAcquiredException;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\Files\Storage\IStorage;
use OCP\Files\Storage\IWriteStreamStorage;
use OCP\Files\StorageNotAvailableException;
use OCP\IConfig;
Expand Down Expand Up @@ -334,56 +335,64 @@ public function put($data) {
}
}

// since we skipped the view we need to scan and emit the hooks ourselves
$storage->getUpdater()->update($internalPath);

try {
$this->changeLock(ILockingProvider::LOCK_SHARED);
} catch (LockedException $e) {
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
}

// allow sync clients to send the mtime along in a header
$mtimeHeader = $this->request->getHeader('x-oc-mtime');
if ($mtimeHeader !== '') {
$mtime = $this->sanitizeMtime($mtimeHeader);
if ($this->fileView->touch($this->path, $mtime)) {
$this->header('X-OC-MTime: accepted');
}
}
$this->finalizeUpload($storage, $internalPath, $exists, $view);
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($this->l10n->t('Failed to check file size: %1$s', [$e->getMessage()]), 0, $e);
}

$fileInfoUpdate = [
'upload_time' => time()
];
return '"' . $this->info->getEtag() . '"';
}

// allow sync clients to send the creation time along in a header
$ctimeHeader = $this->request->getHeader('x-oc-ctime');
if ($ctimeHeader) {
$ctime = $this->sanitizeMtime($ctimeHeader);
$fileInfoUpdate['creation_time'] = $ctime;
$this->header('X-OC-CTime: accepted');
private function finalizeUpload(IStorage $storage, string $internalPath, bool $exists, ?View $view): void {
// Since we skipped the view for the final publish step, finalize the file
// state explicitly here: update cache/bookkeeping, persist metadata, then
// downgrade to a shared lock before emitting post-write hooks so listeners
// can still access the file.
$storage->getUpdater()->update($internalPath);

$fileInfoUpdate = [
'upload_time' => time(),
];

// allow sync clients to send the mtime along in a header
$mtimeHeader = $this->request->getHeader('x-oc-mtime');
if ($mtimeHeader !== '') {
$mtime = $this->sanitizeMtime($mtimeHeader);
if ($this->fileView->touch($this->path, $mtime)) {
$this->header('X-OC-MTime: accepted');
}
}

$this->fileView->putFileInfo($this->path, $fileInfoUpdate);
// allow sync clients to send the creation time along in a header
$ctimeHeader = $this->request->getHeader('x-oc-ctime');
if ($ctimeHeader !== '') {
$ctime = $this->sanitizeMtime($ctimeHeader);
$fileInfoUpdate['creation_time'] = $ctime;
$this->header('X-OC-CTime: accepted');
}

if ($view) {
$this->emitPostHooks($exists);
}
// Persist checksum before post hooks so observers see fully finalized metadata.
$checksumHeader = $this->request->getHeader('oc-checksum');
if ($checksumHeader) {
$fileInfoUpdate['checksum'] = trim($checksumHeader);
} elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') {
$fileInfoUpdate['checksum'] = '';
}

$this->refreshInfo();
$this->fileView->putFileInfo($this->path, $fileInfoUpdate);
$this->refreshInfo();

$checksumHeader = $this->request->getHeader('oc-checksum');
if ($checksumHeader) {
$checksum = trim($checksumHeader);
$this->setChecksum($checksum);
} elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') {
$this->setChecksum('');
}
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($this->l10n->t('Failed to check file size: %1$s', [$e->getMessage()]), 0, $e);
// Downgrade to shared lock before post hooks so legacy hook consumers can
// still access the file during post_write.
try {
$this->changeLock(ILockingProvider::LOCK_SHARED);
} catch (LockedException $e) {
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
}

return '"' . $this->info->getEtag() . '"';
if ($view) {
$this->emitPostHooks($exists);
}
}

private function getPartFileBasePath($path) {
Expand Down
29 changes: 21 additions & 8 deletions apps/dav/tests/unit/Connector/Sabre/FileTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ private function doPut(string $path, ?string $viewRoot = null, ?Request $request
null,
[
'permissions' => Constants::PERMISSION_ALL,
'type' => FileInfo::TYPE_FOLDER,
'type' => FileInfo::TYPE_FILE,
'checksum' => '',
],
null
);
Expand Down Expand Up @@ -800,7 +801,13 @@ protected function assertHookCall($callData, $signal, $hookPath) {
}

/**
* Test whether locks are set before and after the operation
* Test that PUT keeps hook-time lock semantics compatible:
* - pre-write hooks run while the file is shared-locked
* - post-write hooks also run while the file is shared-locked
*
* Post-write hooks are expected to observe a fully finalized file state,
* but should still be able to access the file without exclusive-lock
* contention.
*/
public function testPutLocking(): void {
$view = new View('/' . $this->user . '/files/');
Expand All @@ -812,7 +819,8 @@ public function testPutLocking(): void {
null,
[
'permissions' => Constants::PERMISSION_ALL,
'type' => FileInfo::TYPE_FOLDER,
'type' => FileInfo::TYPE_FILE,
'checksum' => '',
],
null
);
Expand All @@ -832,8 +840,8 @@ public function testPutLocking(): void {
$wasLockedPost = false;
$eventHandler = $this->createMock(EventHandlerMock::class);

// both pre and post hooks might need access to the file,
// so only shared lock is acceptable
// Pre-write hooks should run under a shared lock so observers can safely
// inspect the target while the write is in progress.
$eventHandler->expects($this->once())
->method('writeCallback')
->willReturnCallback(
Expand All @@ -842,6 +850,10 @@ function () use ($view, $path, &$wasLockedPre): void {
$wasLockedPre = $wasLockedPre && !$this->isFileLocked($view, $path, ILockingProvider::LOCK_EXCLUSIVE);
}
);

// Post-write hooks should also run under a shared lock. They are expected to
// see fully finalized metadata/state, but still be able to access the file
// during the callback.
$eventHandler->expects($this->once())
->method('postWriteCallback')
->willReturnCallback(
Expand Down Expand Up @@ -872,8 +884,8 @@ function () use ($view, $path, &$wasLockedPost): void {
// afterMethod unlocks
$view->unlockFile($path, ILockingProvider::LOCK_SHARED);

$this->assertTrue($wasLockedPre, 'File was locked during pre-hooks');
$this->assertTrue($wasLockedPost, 'File was locked during post-hooks');
$this->assertTrue($wasLockedPre, 'File was shared-locked during pre-hooks');
$this->assertTrue($wasLockedPost, 'File was shared-locked during post-hooks');

$this->assertFalse(
$this->isFileLocked($view, $path, ILockingProvider::LOCK_SHARED),
Expand Down Expand Up @@ -1037,7 +1049,8 @@ public function testPutLockExpired(): void {
null,
[
'permissions' => Constants::PERMISSION_ALL,
'type' => FileInfo::TYPE_FOLDER,
'type' => FileInfo::TYPE_FILE,
'checksum' => '',
],
null
);
Expand Down
Loading