From e5d3a64fd391dada790251bf906d9e55755aea60 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 | 97 ++++++++++++++------------- 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 75baff228d6..40119536044 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -31,6 +31,7 @@ use OCP\Files\InvalidPathException; 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; @@ -327,51 +328,7 @@ class File extends Node implements IFile { } } - // 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'); - } - } - - $fileInfoUpdate = [ - 'upload_time' => time() - ]; - - // 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'); - } - - $this->fileView->putFileInfo($this->path, $fileInfoUpdate); - - if ($view) { - $this->emitPostHooks($exists); - } - - $this->refreshInfo(); - - $checksumHeader = $this->request->getHeader('oc-checksum'); - if ($checksumHeader) { - $checksum = trim($checksumHeader); - $this->setChecksum($checksum); - } elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') { - $this->setChecksum(''); - } + $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); } @@ -379,6 +336,56 @@ class File extends Node implements IFile { return '"' . $this->info->getEtag() . '"'; } + 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'); + } + } + + // 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'); + } + + // 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->fileView->putFileInfo($this->path, $fileInfoUpdate); + $this->refreshInfo(); + + if ($view) { + $this->emitPostHooks($exists); + } + + // 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) { $partFileInStorage = Server::get(IConfig::class)->getSystemValue('part_file_in_storage', true); if ($partFileInStorage) { From fa9cd65ba4cce72603580fcf5ed31e7c86dd04c8 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 40119536044..769d94957c4 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -338,8 +338,9 @@ class File extends Node implements IFile { 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 = [ @@ -374,16 +375,17 @@ class File extends Node implements IFile { $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 d137a91f6bb09709a0abe152ea842d1f2ed619b6 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 8c5b04486dd..c8d7dc7f7bc 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -776,7 +776,13 @@ class FileTest extends TestCase { } /** - * 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/'); @@ -810,8 +816,8 @@ class FileTest extends TestCase { ->addMethods(['writeCallback', 'postWriteCallback']) ->getMock(); - // 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( @@ -820,6 +826,10 @@ class FileTest extends TestCase { $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( @@ -850,8 +860,8 @@ class FileTest extends TestCase { // 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 510510b73d7ec87c0b7d40d80fb924b109fd40ca 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 c8d7dc7f7bc..2711f75c0aa 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -230,7 +230,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -794,7 +795,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -1025,7 +1027,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); From ae53b140dc10844f459b3149a4b809ece4c73add 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 213ee4ceec6..10c145407be 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -368,7 +368,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { * @inheritdoc */ public function getChecksum() { - return $this->data['checksum']; + return return $this->data['checksum'] ?? ''; } public function getExtension(): string { From dff4dcdd35012247b450a02c335cb669e786e5e2 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 10c145407be..f049a146853 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -368,7 +368,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { * @inheritdoc */ public function getChecksum() { - return return $this->data['checksum'] ?? ''; + return $this->data['checksum'] ?? ''; } public function getExtension(): string { From ab88d4f83fc98fdf404f450c2d99143e19ea473c 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 2711f75c0aa..64d0a856002 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -231,7 +231,7 @@ class FileTest extends TestCase { [ 'permissions' => Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FILE, - 'checksum' => '', + 'checksum' => '', ], null );