diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index b491680b205..d79e41582de 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -553,7 +553,15 @@ private function createFileChunked($data) { try { $this->fileView->lockFile($targetPath, ILockingProvider::LOCK_SHARED); - $this->emitPreHooks($exists, $targetPath); + $run = $this->emitPreHooks($exists, $targetPath); + if ($run === false) { + // a hook (e.g. the blacklist check on the decoded target + // name) vetoed the write - abort before assembling the + // chunks into the final file + $this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED); + $chunk_handler->cleanup(); + throw new Forbidden('Writing the file was not allowed'); + } $this->fileView->changeLock($targetPath, ILockingProvider::LOCK_EXCLUSIVE); /** @var \OC\Files\Storage\Storage $targetStorage */ list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath); diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 29c9d923201..737e9ccfcd4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -679,6 +679,46 @@ public function testChunkedPut() { $this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1')); } + /** + * A chunked upload whose assembled target name is blacklisted (e.g. + * `.htaccess`) must be rejected, just like the non-chunked path. The + * chunk names themselves (`.htaccess-chunking-...`) are not blacklisted, + * so the guard has to fire on the decoded target name. + */ + public function testChunkedPutToBlacklistedNameIsForbidden() { + // wire the blacklist guard exactly as lib/kernel.php does at boot + // (setUp() clears all hooks, so it is not registered by default) + \OC_Hook::connect( + 'OC_Filesystem', + 'write', + 'OC\Files\Filesystem', + 'isForbiddenFileOrDir_Hook' + ); + + $_SERVER['HTTP_OC_CHUNKED'] = true; + + $this->assertNull($this->doPut('/.htaccess-chunking-12345-2-0')); + + $thrown = false; + try { + $this->doPut('/.htaccess-chunking-12345-2-1'); + } catch (\Sabre\DAV\Exception\Forbidden $e) { + $thrown = true; + } + $this->assertTrue($thrown, 'Uploading a blacklisted .htaccess via chunking must be forbidden'); + + // The blacklisted target must never reach the storage backend - check + // the physical storage directly, because the file cache scanner skips + // blacklisted names and would hide a file that is actually on disk. + $view = Filesystem::getView(); + list($storage, $internalPath) = $view->resolvePath('/.htaccess'); + $this->assertFalse( + $storage->file_exists($internalPath), + 'Blacklisted .htaccess must not be written to storage via chunked upload' + ); + $this->assertEmpty($this->listPartFiles(), 'No stray part files'); + } + /** * Test that putting a file triggers create hooks */ diff --git a/changelog/unreleased/41762 b/changelog/unreleased/41762 new file mode 100644 index 00000000000..7f845ce5c70 --- /dev/null +++ b/changelog/unreleased/41762 @@ -0,0 +1,9 @@ +Change: Honour the write hook veto on legacy chunked WebDAV uploads + +The legacy WebDAV chunked upload path assembled the final file without +respecting the pre-write hook result, so the filename blacklist that applies +to ordinary uploads was not enforced for chunked uploads. The chunked +assembly now aborts when a write hook vetoes the file, matching the +non-chunked upload path. + +https://github.com/owncloud/core/pull/41762