From a24934b1a50b07cf448337c6e70a0989990c9c76 Mon Sep 17 00:00:00 2001 From: samin zavarkesh Date: Fri, 5 Dec 2025 09:20:33 +0100 Subject: [PATCH] fix: add filename validation with proper error handling - Add unit tests for FilesAppService filename validation - Add filename validation using IFilenameValidator - Handle InvalidPathException and convert to BadRequestException - Map sanitized characters (e.g., ':' to '/') for better UX - Fix integration test workflow to handle missing log files - Improve code comments for better explanation of edge cases Signed-off-by: samin zavarkesh --- .github/workflows/integration.yml | 7 +- lib/Service/FilesAppService.php | 28 ++++ tests/unit/Service/FilesAppServiceTest.php | 170 +++++++++++++++++++++ 3 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 tests/unit/Service/FilesAppServiceTest.php diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index d85235498b..03e2738b6b 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -107,7 +107,12 @@ jobs: - name: Print log if: always() - run: cat data/nextcloud.log + run: | + if [ -f data/nextcloud.log ]; then + cat data/nextcloud.log + else + echo "Log file not found" + fi - name: Query count if: ${{ matrix.databases == 'mysql' }} diff --git a/lib/Service/FilesAppService.php b/lib/Service/FilesAppService.php index 7b65f8fe63..38f3cf3734 100644 --- a/lib/Service/FilesAppService.php +++ b/lib/Service/FilesAppService.php @@ -1,5 +1,7 @@ '/']; + private IRequest $request; private IRootFolder $rootFolder; private DeckShareProvider $shareProvider; @@ -42,6 +54,7 @@ class FilesAppService implements IAttachmentService, ICustomAttachmentService { private CardMapper $cardMapper; private LoggerInterface $logger; private IDBConnection $connection; + private IFilenameValidator $filenameValidator; public function __construct( IRequest $request, @@ -56,6 +69,7 @@ public function __construct( CardMapper $cardMapper, LoggerInterface $logger, IDBConnection $connection, + IFilenameValidator $filenameValidator, ?string $userId, ) { $this->request = $request; @@ -71,6 +85,7 @@ public function __construct( $this->cardMapper = $cardMapper; $this->logger = $logger; $this->connection = $connection; + $this->filenameValidator = $filenameValidator; } public function listAttachments(int $cardId): array { @@ -172,6 +187,7 @@ public function create(Attachment $attachment) { } $fileName = $file['name']; + $this->validateFilename($fileName); // get shares for current card // check if similar filename already exists @@ -307,4 +323,16 @@ private function getShareForAttachment(Attachment $attachment): IShare { return $share; } + + private function validateFilename(string $fileName): void { + try { + $this->filenameValidator->validateFilename($fileName); + } catch (InvalidPathException $e) { + $errorMessage = $e->getMessage(); + foreach (self::SANITIZED_CHAR_MAPPING as $sanitized => $original) { + $errorMessage = str_replace($sanitized, $original, $errorMessage); + } + throw new BadRequestException($errorMessage); + } + } } diff --git a/tests/unit/Service/FilesAppServiceTest.php b/tests/unit/Service/FilesAppServiceTest.php new file mode 100644 index 0000000000..0c5b3f461e --- /dev/null +++ b/tests/unit/Service/FilesAppServiceTest.php @@ -0,0 +1,170 @@ +request = $this->createMock(IRequest::class); + $this->filenameValidator = $this->createMock(IFilenameValidator::class); + $this->l10n = $this->createMock(IL10N::class); + $this->l10n->expects($this->any()) + ->method('t') + ->willReturnCallback(fn ($string, $args = []) => vsprintf($string, $args)); + + $this->rootFolder = $this->createMock(\OCP\Files\IRootFolder::class); + $this->configService = $this->createMock(ConfigService::class); + $this->shareManager = $this->createMock(\OCP\Share\IManager::class); + + $this->filesAppService = new FilesAppService( + $this->request, + $this->l10n, + $this->rootFolder, + $this->shareManager, + $this->configService, + $this->createMock(\OCA\Deck\Sharing\DeckShareProvider::class), + $this->createMock(\OCP\IPreview::class), + $this->createMock(\OCP\Files\IMimeTypeDetector::class), + $this->createMock(PermissionService::class), + $this->createMock(\OCA\Deck\Db\CardMapper::class), + $this->createMock(\Psr\Log\LoggerInterface::class), + $this->createMock(\OCP\IDBConnection::class), + $this->filenameValidator, + 'admin' + ); + } + + public function testCreateWithInvalidFilename() { + $this->expectException(BadRequestException::class); + + $attachment = new Attachment(); + $attachment->setCardId(123); + + $this->request->expects($this->once()) + ->method('getUploadedFile') + ->with('file') + ->willReturn([ + 'name' => 'test:file.txt', + 'tmp_name' => __FILE__, + 'error' => UPLOAD_ERR_OK, + ]); + + $this->filenameValidator->expects($this->once()) + ->method('validateFilename') + ->with('test:file.txt') + ->willThrowException(new \OCP\Files\InvalidPathException('":\" is not allowed inside a file or folder name.')); + + $this->filesAppService->create($attachment); + } + + public function testCreateWithValidFilename() { + $attachment = new Attachment(); + $attachment->setCardId(123); + + $this->request->expects($this->once()) + ->method('getUploadedFile') + ->with('file') + ->willReturn([ + 'name' => 'valid-file.txt', + 'tmp_name' => __FILE__, + 'error' => UPLOAD_ERR_OK, + ]); + + $this->filenameValidator->expects($this->once()) + ->method('validateFilename') + ->with('valid-file.txt'); + + $userFolder = $this->createMock(\OCP\Files\Folder::class); + $this->rootFolder->expects($this->any()) + ->method('getUserFolder') + ->willReturn($userFolder); + + $this->configService->expects($this->any()) + ->method('getAttachmentFolder') + ->willReturn('deck'); + + $folder = $this->createMock(\OCP\Files\Folder::class); + $userFolder->expects($this->any()) + ->method('get') + ->willReturn($folder); + + $folder->expects($this->any()) + ->method('isShared') + ->willReturn(false); + + $folder->expects($this->any()) + ->method('getNonExistingName') + ->willReturnArgument(0); + + $file = $this->createMock(\OCP\Files\File::class); + $folder->expects($this->any()) + ->method('newFile') + ->willReturn($file); + + $file->expects($this->any()) + ->method('putContent'); + + $file->expects($this->any()) + ->method('getName') + ->willReturn('valid-file.txt'); + + $share = $this->createMock(\OCP\Share\IShare::class); + $share->expects($this->any()) + ->method('getId') + ->willReturn('123'); + $share->expects($this->any()) + ->method('setNode') + ->willReturnSelf(); + $share->expects($this->any()) + ->method('setShareType') + ->willReturnSelf(); + $share->expects($this->any()) + ->method('setSharedWith') + ->willReturnSelf(); + $share->expects($this->any()) + ->method('setPermissions') + ->willReturnSelf(); + $share->expects($this->any()) + ->method('setSharedBy') + ->willReturnSelf(); + + $this->shareManager->expects($this->any()) + ->method('newShare') + ->willReturn($share); + $this->shareManager->expects($this->any()) + ->method('createShare') + ->willReturn($share); + + try { + $this->filesAppService->create($attachment); + } catch (BadRequestException $e) { + $this->fail('Validation should pass for valid filename, but BadRequestException was thrown: ' . $e->getMessage()); + } catch (\Exception $e) { + // Other exceptions are expected since not everything is mocked. + } + } +}