From e64524d5f851fdc6a42567eb75db4d591c8c8e03 Mon Sep 17 00:00:00 2001 From: Juan Claude Date: Wed, 11 Nov 2026 18:16:41 +0000 Subject: [PATCH 1/2] test(files): pin FileService allowed and refused writes before the split Assisted-by: ClaudeCode:claude-opus-5-5 --- .../Service/FileServiceGuardGoldenTest.php | 327 ++++++++++++++++++ 1 file changed, 327 insertions(+) create mode 100644 tests/Unit/Service/FileServiceGuardGoldenTest.php diff --git a/tests/Unit/Service/FileServiceGuardGoldenTest.php b/tests/Unit/Service/FileServiceGuardGoldenTest.php new file mode 100644 index 000000000..f126055d8 --- /dev/null +++ b/tests/Unit/Service/FileServiceGuardGoldenTest.php @@ -0,0 +1,327 @@ + + * @copyright 2026 Conduction b.v. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace Unit\Service; + +use OCA\LaunchPad\Db\AdminSetting; +use OCA\LaunchPad\Db\AdminSettingMapper; +use OCA\LaunchPad\Exception\FileAlreadyExistsException; +use OCA\LaunchPad\Exception\FileTypeNotAllowedException; +use OCA\LaunchPad\Exception\InvalidDirectoryException; +use OCA\LaunchPad\Exception\InvalidFilenameException; +use OCA\LaunchPad\Exception\StorageFailureException; +use OCA\LaunchPad\Service\FileService; +use OCP\Files\File; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; +use OCP\IURLGenerator; +use PHPUnit\Framework\TestCase; +use RuntimeException; + +class FileServiceGuardGoldenTest extends TestCase { + private IRootFolder $rootFolder; + private AdminSettingMapper $settingMapper; + private Folder $userFolder; + private FileService $service; + + protected function setUp(): void { + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->settingMapper = $this->createMock(AdminSettingMapper::class); + $this->userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder')->willReturn($this->userFolder); + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('linkToRouteAbsolute')->willReturn('https://nc/f'); + $this->service = new FileService($this->rootFolder, $urlGenerator, $this->settingMapper); + } + + private function storedExtensions(mixed $stored): void { + $this->settingMapper->method('getValue')->willReturn($stored); + } + + private function newFileIn(Folder $folder): File { + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(9); + $folder->method('nodeExists')->willReturn(false); + $folder->method('newFile')->willReturn($file); + + return $file; + } + + /** + * Filenames refused before storage is touched. + * + * @return array + */ + public static function refusedWrites(): array { + return [ + 'empty name' => ['', '/'], + 'too long' => [str_repeat('a', 252) . '.txt', '/'], + 'null byte' => ["a\0.txt", '/'], + 'dot dot' => ['a..txt', '/'], + 'slash' => ['a/b.txt', '/'], + 'backslash' => ['a\\b.txt', '/'], + 'pattern' => ['a*b.txt', '/'], + ]; + } + + /** + * @dataProvider refusedWrites + */ + public function testRefusedFilenameNeverReachesStorage(string $filename, string $dir): void { + $this->rootFolder->expects($this->never())->method('getUserFolder'); + $this->expectException(InvalidFilenameException::class); + + $this->service->createFile('alice', $filename, ['dir' => $dir]); + } + + /** + * Directories refused before storage is touched. + * + * @return array + */ + public static function refusedDirectories(): array { + return ['dot dot' => ['/a/../b'], 'null byte' => ["/a\0"]]; + } + + /** + * @dataProvider refusedDirectories + */ + public function testRefusedDirectoryNeverReachesStorage(string $dir): void { + $this->rootFolder->expects($this->never())->method('getUserFolder'); + $this->expectException(InvalidDirectoryException::class); + + $this->service->createFile('alice', 'ok.txt', ['dir' => $dir]); + } + + public function testFilenameIsCheckedBeforeDirectory(): void { + $this->expectException(InvalidFilenameException::class); + + $this->service->createFile('alice', 'a/b.txt', ['dir' => '/../']); + } + + public function testExtensionOffTheListIsRefused(): void { + $this->storedExtensions(['md']); + $this->rootFolder->expects($this->never())->method('getUserFolder'); + $this->expectException(FileTypeNotAllowedException::class); + + $this->service->createFile('alice', 'a.txt'); + } + + public function testAllowedLongestNameIsWritten(): void { + $this->storedExtensions(null); + $this->newFileIn($this->userFolder); + + $result = $this->service->createFile('alice', str_repeat('a', 251) . '.txt'); + + $this->assertSame(['status' => 'success', 'fileId' => 9, 'url' => 'https://nc/f'], $result); + } + + /** + * Directories that resolve to the user's own folder. + * + * @return array + */ + public static function rootDirectories(): array { + return ['slash' => ['/'], 'empty' => [''], 'slashes' => ['///']]; + } + + /** + * @dataProvider rootDirectories + */ + public function testRootDirectoryWritesIntoTheUserFolder(string $dir): void { + $this->storedExtensions(null); + $this->userFolder->expects($this->never())->method('get'); + $this->newFileIn($this->userFolder); + + $this->assertSame(9, $this->service->createFile('alice', 'a.txt', ['dir' => $dir])['fileId']); + } + + public function testExistingSubfolderIsUsed(): void { + $this->storedExtensions(null); + $sub = $this->createMock(Folder::class); + $this->userFolder->expects($this->once())->method('get')->with('/a/b')->willReturn($sub); + $this->newFileIn($sub); + + $this->assertSame(9, $this->service->createFile('alice', 'a.txt', ['dir' => 'a/b/'])['fileId']); + } + + /** + * Folder-resolution failures and their message. + * + * @return array + */ + public static function folderFailures(): array { + return [ + 'file in the way' => ['file', 'Failed to resolve target folder'], + 'lookup error' => ['error', 'Failed to resolve target folder'], + 'create error' => ['create', 'Failed to create target folder'], + ]; + } + + /** + * @dataProvider folderFailures + */ + public function testFolderFailureMessage(string $case, string $message): void { + $this->storedExtensions(null); + if ($case === 'file') { + $this->userFolder->method('get')->willReturn($this->createMock(File::class)); + } + + if ($case === 'error') { + $this->userFolder->method('get')->willThrowException(new RuntimeException('x')); + } + + if ($case === 'create') { + $this->userFolder->method('get')->willThrowException(new NotFoundException('x')); + $this->userFolder->method('newFolder')->willThrowException(new RuntimeException('x')); + } + + $this->expectException(StorageFailureException::class); + $this->expectExceptionMessage($message); + + $this->service->createFile('alice', 'a.txt', ['dir' => '/sub']); + } + + public function testMissingSubfolderIsCreated(): void { + $this->storedExtensions(null); + $sub = $this->createMock(Folder::class); + $this->userFolder->method('get')->willThrowException(new NotFoundException('x')); + $this->userFolder->expects($this->once())->method('newFolder')->with('/sub')->willReturn($sub); + $this->newFileIn($sub); + + $this->assertSame(9, $this->service->createFile('alice', 'a.txt', ['dir' => '/sub'])['fileId']); + } + + public function testRefuseModeAnswersAlreadyExistsUnwrapped(): void { + $this->storedExtensions(null); + $this->userFolder->method('nodeExists')->willReturn(true); + $this->userFolder->expects($this->never())->method('get'); + $this->expectException(FileAlreadyExistsException::class); + + $this->service->createFile('alice', 'a.txt', ['onExisting' => FileService::ON_EXISTING_REFUSE]); + } + + public function testReplaceModeOverwritesTheExistingFile(): void { + $this->storedExtensions(null); + $existing = $this->createMock(File::class); + $existing->method('getId')->willReturn(4); + $existing->expects($this->once())->method('putContent')->with('body'); + $this->userFolder->method('nodeExists')->willReturn(true); + $this->userFolder->method('get')->willReturn($existing); + $this->userFolder->expects($this->never())->method('newFile'); + + $this->assertSame(4, $this->service->createFile('alice', 'a.txt', ['content' => 'body'])['fileId']); + } + + /** + * Write failures, all answered with the same storage message. + * + * @return array + */ + public static function writeFailures(): array { + return ['folder in the way' => ['folder'], 'put fails' => ['put'], 'new fails' => ['new'], 'exists check fails' => ['exists']]; + } + + /** + * @dataProvider writeFailures + */ + public function testWriteFailureMessage(string $case): void { + $this->storedExtensions(null); + $this->userFolder->method('nodeExists')->willReturnCallback(static function () use ($case): bool { + if ($case === 'exists') { + throw new RuntimeException('x'); + } + + return $case !== 'new'; + }); + $existing = $this->createMock(File::class); + $existing->method('putContent')->willThrowException(new RuntimeException('x')); + $this->userFolder->method('get')->willReturn($case === 'folder' ? $this->createMock(Folder::class) : $existing); + $this->userFolder->method('newFile')->willThrowException(new RuntimeException('x')); + $this->expectException(StorageFailureException::class); + $this->expectExceptionMessage('Failed to write file'); + + $this->service->createFile('alice', 'a.txt'); + } + + public function testUserFolderFailureMessage(): void { + $this->storedExtensions(null); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->rootFolder->method('getUserFolder')->willThrowException(new RuntimeException('x')); + $service = new FileService($this->rootFolder, $this->createMock(IURLGenerator::class), $this->settingMapper); + $this->expectException(StorageFailureException::class); + $this->expectExceptionMessage('Failed to resolve user folder'); + + $service->createFile('alice', 'a.txt'); + } + + /** + * Stored allow-lists and what they read as. + * + * @return array}> + */ + public static function storedLists(): array { + $default = FileService::DEFAULT_ALLOWED_EXTENSIONS; + return [ + 'unset' => [null, $default], + 'not a list' => ['txt', $default], + 'empty' => [[], $default], + 'all invalid' => [[3, ' . ', 'p-d-f', ''], $default], + 'normalised' => [[' .TXT ', 'md', 'Md', 7, 'x y', 'pdf2', '..csv'], ['txt', 'md', 'pdf2', 'csv']], + ]; + } + + /** + * @dataProvider storedLists + */ + public function testStoredListReads(mixed $stored, array $expected): void { + $this->storedExtensions($stored); + + $this->assertSame($expected, $this->service->getAllowedExtensions()); + } + + /** + * Saved allow-lists and what is stored. + * + * @return array,1:list}> + */ + public static function savedLists(): array { + return [ + 'empty' => [[], FileService::DEFAULT_ALLOWED_EXTENSIONS], + 'all invalid' => [[null, '', '-'], FileService::DEFAULT_ALLOWED_EXTENSIONS], + 'normalised' => [['.PDF', 'pdf', ' odt '], ['pdf', 'odt']], + ]; + } + + /** + * @dataProvider savedLists + */ + public function testSavedListIsNormalisedAndStored(array $saved, array $expected): void { + $this->settingMapper->expects($this->once())->method('setSetting')->with( + AdminSetting::KEY_LINK_CREATE_FILE_EXTENSIONS, + $expected + ); + + $this->assertSame($expected, $this->service->setAllowedExtensions(extensions: $saved)); + } +}//end class From 0efe8318f6284ac3f5df18abfea1a8e5ba14e234 Mon Sep 17 00:00:00 2001 From: Juan Claude Date: Wed, 11 Nov 2026 18:17:51 +0000 Subject: [PATCH 2/2] refactor(files): split FileService validation, folder resolution and write into named steps Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/FileService.php | 240 ++++++++++++++++++++++++------------ 1 file changed, 162 insertions(+), 78 deletions(-) diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index 80cfc019f..9123f5fab 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -39,6 +39,7 @@ use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\IURLGenerator; use Throwable; @@ -221,24 +222,7 @@ public function getAllowedExtensions(): array { return self::DEFAULT_ALLOWED_EXTENSIONS; } - $normalised = []; - foreach ($stored as $value) { - if (is_string($value) === false) { - continue; - } - - $token = strtolower(string: ltrim(string: trim(string: $value), characters: '.')); - if ($token === '') { - continue; - } - - if (preg_match(pattern: '/^[a-z0-9]+$/', subject: $token) !== 1) { - continue; - } - - $normalised[$token] = $token; - } - + $normalised = self::normaliseExtensions(values: $stored); if (count($normalised) === 0) { return self::DEFAULT_ALLOWED_EXTENSIONS; } @@ -263,23 +247,7 @@ public function getAllowedExtensions(): array { * @spec openspec/specs/resource-uploads/spec.md */ public function setAllowedExtensions(array $extensions): array { - $normalised = []; - foreach ($extensions as $value) { - if (is_string($value) === false) { - continue; - } - - $token = strtolower(string: ltrim(string: trim(string: $value), characters: '.')); - if ($token === '') { - continue; - } - - if (preg_match(pattern: '/^[a-z0-9]+$/', subject: $token) !== 1) { - continue; - } - - $normalised[$token] = $token; - } + $normalised = self::normaliseExtensions(values: $extensions); $stored = self::DEFAULT_ALLOWED_EXTENSIONS; if (count($normalised) > 0) { @@ -294,6 +262,47 @@ public function setAllowedExtensions(array $extensions): array { return $stored; }//end setAllowedExtensions() + /** + * Normalise a list of extensions: lowercase, trimmed, leading dots + * stripped, alphanumeric only, de-duplicated, in first-seen order. + * + * @param array $values The candidate extensions. + * + * @return array The kept extensions, keyed by themselves. + */ + private static function normaliseExtensions(array $values): array { + $normalised = []; + foreach ($values as $value) { + $token = self::normaliseExtension(value: $value); + if ($token !== null) { + $normalised[$token] = $token; + } + } + + return $normalised; + }//end normaliseExtensions() + + /** + * Normalise one extension, null when it is not a string or not a + * non-empty alphanumeric token after normalisation. + * + * @param mixed $value The candidate extension. + * + * @return string|null The token, or null when dropped. + */ + private static function normaliseExtension(mixed $value): ?string { + if (is_string($value) === false) { + return null; + } + + $token = strtolower(string: ltrim(string: trim(string: $value), characters: '.')); + if ($token === '' || preg_match(pattern: '/^[a-z0-9]+$/', subject: $token) !== 1) { + return null; + } + + return $token; + }//end normaliseExtension() + /** * Reject filenames that fail any of the REQ-LBN-004 task 1.2 checks. * @@ -308,26 +317,32 @@ private function assertValidFilename(string $filename): void { throw new InvalidFilenameException(); } - if (str_contains(haystack: $filename, needle: "\0") === true) { - throw new InvalidFilenameException(); - } - - if (str_contains(haystack: $filename, needle: '..') === true) { + if (self::containsAny(haystack: $filename, needles: ["\0", '..', '/', '\\']) === true) { throw new InvalidFilenameException(); } - if (str_contains(haystack: $filename, needle: '/') === true) { + if (preg_match(pattern: self::FILENAME_PATTERN, subject: $filename) !== 1) { throw new InvalidFilenameException(); } + }//end assertValidFilename() - if (str_contains(haystack: $filename, needle: '\\') === true) { - throw new InvalidFilenameException(); + /** + * Whether a string contains any of the given substrings. + * + * @param string $haystack The string to search. + * @param array $needles The substrings to look for. + * + * @return bool True when at least one occurs. + */ + private static function containsAny(string $haystack, array $needles): bool { + foreach ($needles as $needle) { + if (str_contains(haystack: $haystack, needle: $needle) === true) { + return true; + } } - if (preg_match(pattern: self::FILENAME_PATTERN, subject: $filename) !== 1) { - throw new InvalidFilenameException(); - } - }//end assertValidFilename() + return false; + }//end containsAny() /** * Reject directories that fail any of the REQ-LBN-004 task 1.3 checks. @@ -392,25 +407,26 @@ private function resolveTargetFolder(Folder $userFolder, string $dir): Folder { return $userFolder; } - try { - $node = $userFolder->get(path: $normalised); - if ($node instanceof Folder) { - return $node; - } + return $this->folderAt(userFolder: $userFolder, path: $normalised); + }//end resolveTargetFolder() - // A file (not a folder) already occupies the path. - throw new StorageFailureException( - message: 'Failed to resolve target folder' - ); + /** + * The folder at a subdirectory path, created when it is missing. + * + * @param Folder $userFolder Owner's Files-area root. + * @param string $path The normalised subdirectory path (not `/`). + * + * @return Folder The resolved (or freshly created) folder. + * + * @throws StorageFailureException When the folder cannot be + * resolved or created. + */ + private function folderAt(Folder $userFolder, string $path): Folder { + try { + return $this->existingFolder(userFolder: $userFolder, path: $path); } catch (NotFoundException $e) { // Auto-create missing subdirectory chain. - try { - return $userFolder->newFolder(path: $normalised); - } catch (Throwable $createError) { - throw new StorageFailureException( - message: 'Failed to create target folder' - ); - } + return $this->createFolder(userFolder: $userFolder, path: $path); } catch (StorageFailureException $e) { throw $e; } catch (Throwable $e) { @@ -418,7 +434,50 @@ private function resolveTargetFolder(Folder $userFolder, string $dir): Folder { message: 'Failed to resolve target folder' ); }//end try - }//end resolveTargetFolder() + }//end folderAt() + + /** + * The folder at a path inside the user's Files area. + * + * @param Folder $userFolder Owner's Files-area root. + * @param string $path The normalised subdirectory path. + * + * @return Folder The folder. + * + * @throws NotFoundException When nothing exists at the path. + * @throws StorageFailureException When a file occupies the path. + */ + private function existingFolder(Folder $userFolder, string $path): Folder { + $node = $userFolder->get(path: $path); + if ($node instanceof Folder) { + return $node; + } + + // A file (not a folder) already occupies the path. + throw new StorageFailureException( + message: 'Failed to resolve target folder' + ); + }//end existingFolder() + + /** + * Create a missing subdirectory chain inside the user's Files area. + * + * @param Folder $userFolder Owner's Files-area root. + * @param string $path The normalised subdirectory path. + * + * @return Folder The created folder. + * + * @throws StorageFailureException When the folder cannot be created. + */ + private function createFolder(Folder $userFolder, string $path): Folder { + try { + return $userFolder->newFolder(path: $path); + } catch (Throwable $createError) { + throw new StorageFailureException( + message: 'Failed to create target folder' + ); + } + }//end createFolder() /** * Write the file with overwrite-on-exists semantics @@ -437,20 +496,8 @@ private function resolveTargetFolder(Folder $userFolder, string $dir): Folder { private function writeFile(Folder $folder, string $filename, string $content, string $onExisting): File { try { if ($folder->nodeExists(path: $filename) === true) { - if ($onExisting === self::ON_EXISTING_REFUSE) { - throw new FileAlreadyExistsException(); - } - - $existing = $folder->get(path: $filename); - if ($existing instanceof File) { - $existing->putContent(data: $content); - return $existing; - } - - // A folder with the same name already exists. - throw new StorageFailureException( - message: 'Failed to write file' - ); + self::assertMayReplace(onExisting: $onExisting); + return self::overwrite(existing: $folder->get(path: $filename), content: $content); } // Folder::newFile() returns a File node — the contract is @@ -466,4 +513,41 @@ private function writeFile(Folder $folder, string $filename, string $content, st ); }//end try }//end writeFile() + + /** + * Refuse to replace an existing file when the caller asked for that. + * + * @param string $onExisting ON_EXISTING_REPLACE or ON_EXISTING_REFUSE. + * + * @return void + * + * @throws FileAlreadyExistsException When $onExisting is ON_EXISTING_REFUSE. + */ + private static function assertMayReplace(string $onExisting): void { + if ($onExisting === self::ON_EXISTING_REFUSE) { + throw new FileAlreadyExistsException(); + } + }//end assertMayReplace() + + /** + * Replace the contents of the existing file node at the target name. + * + * @param Node $existing The node at the target name. + * @param string $content Bytes to write. + * + * @return File The overwritten file. + * + * @throws StorageFailureException When a folder occupies the name. + */ + private static function overwrite(Node $existing, string $content): File { + if ($existing instanceof File) { + $existing->putContent(data: $content); + return $existing; + } + + // A folder with the same name already exists. + throw new StorageFailureException( + message: 'Failed to write file' + ); + }//end overwrite() }//end class