From b7a850c3b764ac79c0afee0e5d14901cae414a5d Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 15 May 2026 10:03:30 -0400 Subject: [PATCH 1/7] fix(dav): finalize upload metadata before downgrading lock Signed-off-by: Josh --- apps/dav/lib/Connector/Sabre/File.php | 89 +++++++++++++++------------ 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 7f5e049c32912..ca6d56b7d53e3 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -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; @@ -334,56 +335,62 @@ 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, emit + // post-write hooks, and only then downgrade the lock. + $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); + if ($view) { + $this->emitPostHooks($exists); } - return '"' . $this->info->getEtag() . '"'; + // Keep the exclusive lock until all bookkeeping and metadata updates are complete. + try { + $this->changeLock(ILockingProvider::LOCK_SHARED); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } } private function getPartFileBasePath($path) { From 3ab1bd45491da09d7bf44a4016c4bb480adcc591 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 17 May 2026 10:44:58 -0400 Subject: [PATCH 2/7] fix(dav): preserves the old hook-accessibility contract while still improving consistency Signed-off-by: Josh --- apps/dav/lib/Connector/Sabre/File.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index ca6d56b7d53e3..40afe0f2f25b7 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -345,8 +345,9 @@ public function put($data) { 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, emit - // post-write hooks, and only then downgrade the lock. + // 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 = [ @@ -381,16 +382,17 @@ private function finalizeUpload(IStorage $storage, string $internalPath, bool $e $this->fileView->putFileInfo($this->path, $fileInfoUpdate); $this->refreshInfo(); - if ($view) { - $this->emitPostHooks($exists); - } - - // Keep the exclusive lock until all bookkeeping and metadata updates are complete. + // 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); } + + if ($view) { + $this->emitPostHooks($exists); + } } private function getPartFileBasePath($path) { From 2d524128c5dd29c225ab6b6da32f49512e3ce2eb Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 17 May 2026 10:49:00 -0400 Subject: [PATCH 3/7] test(dav): add clarifying comments to testPutLocking test Signed-off-by: Josh --- .../tests/unit/Connector/Sabre/FileTest.php | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 25b09db1c54b9..56d5c9fee59ea 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -800,7 +800,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/'); @@ -832,8 +838,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( @@ -842,6 +848,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( @@ -872,8 +882,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), From b08338d124391caa54424367aea1b3aefa517f26 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 17 May 2026 11:22:23 -0400 Subject: [PATCH 4/7] test(dav): make sure FileInfo is constructed with checksum At least where likely to be needed. Also fixed object type. Signed-off-by: Josh --- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 56d5c9fee59ea..3c41cb299743f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -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 ); @@ -818,7 +819,8 @@ public function testPutLocking(): void { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -1047,7 +1049,8 @@ public function testPutLockExpired(): void { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); From 92a28290a307769826b170dd9981792c413e3d70 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 18 May 2026 08:35:20 -0400 Subject: [PATCH 5/7] fix(FileInfo): harden getChecksum() - `checksum` is already optional/derived metadata in practice - callers already treat `null`l / `''` as "no checksum" Signed-off-by: Josh --- lib/private/Files/FileInfo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 65280b0f31580..c9cf441c7899c 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -382,7 +382,7 @@ public function addSubEntry($data, $entryPath) { */ #[\Override] public function getChecksum() { - return $this->data['checksum']; + return return $this->data['checksum'] ?? ''; } #[\Override] From fff2a542ba35e9b9cf83908844381aa3d122c145 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 18 May 2026 08:40:31 -0400 Subject: [PATCH 6/7] chore(FileInfo): fixup code typo Signed-off-by: Josh --- lib/private/Files/FileInfo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index c9cf441c7899c..d7b8545c5ab50 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -382,7 +382,7 @@ public function addSubEntry($data, $entryPath) { */ #[\Override] public function getChecksum() { - return return $this->data['checksum'] ?? ''; + return $this->data['checksum'] ?? ''; } #[\Override] From 106f53e53ce85f9cd7bee0e9bd19dc8af5c89729 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 18 May 2026 08:43:59 -0400 Subject: [PATCH 7/7] chore(FileTest): fixup formatting Signed-off-by: Josh --- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 3c41cb299743f..1431c6be3d311 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -239,7 +239,7 @@ private function doPut(string $path, ?string $viewRoot = null, ?Request $request [ 'permissions' => Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FILE, - 'checksum' => '', + 'checksum' => '', ], null );