From 4f2feaeae24d91abfaedbeb842a36f9e81eec072 Mon Sep 17 00:00:00 2001 From: Martin Vlach Date: Wed, 14 Oct 2026 12:46:33 +0000 Subject: [PATCH] fix(CrawlService): Validate actual response body size in archiveFile Previously, archiveFile checked the Content-Length response header against MAX_BODY_LENGTH, defaulting to 0 when the header was missing (such as with chunked transfer-encoding) or trusting a spoofed low Content-Length. This allowed oversized payloads to bypass the limit and be written to disk. Verify both the Content-Length header and the actual response body length against MAX_BODY_LENGTH before writing the file. Reported-by: BS.ehlas.cz Assisted-by: Antigravity:gemini-3.8-flash --- lib/Service/CrawlService.php | 47 +++++----- tests/CrawlServiceTest.php | 161 +++++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 21 deletions(-) create mode 100644 tests/CrawlServiceTest.php diff --git a/lib/Service/CrawlService.php b/lib/Service/CrawlService.php index 419724396..688690eff 100644 --- a/lib/Service/CrawlService.php +++ b/lib/Service/CrawlService.php @@ -129,28 +129,33 @@ private function archiveFile(Bookmark $bookmark, IResponse $resp): void { if ($contentType !== null && !str_contains($contentType, 'text/html') && $bookmark->getArchivedFile() === null) { $contentLengthHeader = $resp->getHeader('Content-Length'); - $contentLength = $contentLengthHeader !== '' ? (int)$contentLengthHeader : 0; + if ($contentLengthHeader !== '' && (int)$contentLengthHeader >= self::MAX_BODY_LENGTH) { + return; + } - if ($contentLength < self::MAX_BODY_LENGTH) { - try { - $userFolder = $this->rootFolder->getUserFolder($bookmark->getUserId()); - $folderPath = $this->getArchivePath($bookmark, $userFolder); - $name = $bookmark->slugify('title'); - $extension = $this->mimey->getExtension($contentType) ?? 'txt'; - - $i = 0; - do { - $path = $folderPath . '/' . $name . ($i > 0 ? '_' . $i : '') . '.' . $extension; - $i++; - } while ($userFolder->nodeExists($path)); - - $file = $userFolder->newFile($path); - $file->putContent($resp->getBody()); - $bookmark->setArchivedFile($file->getId()); - $this->bookmarkMapper->update($bookmark); - } catch (NotPermittedException|NoUserException|GenericFileException|LockedException|UrlParseError|InvalidPathException|NotFoundException $e) { - $this->logger->debug(get_class($e) . ' ' . $e->getMessage() . "\r\n" . $e->getTraceAsString()); - } + $body = $resp->getBody(); + if (strlen($body) >= self::MAX_BODY_LENGTH) { + return; + } + + try { + $userFolder = $this->rootFolder->getUserFolder($bookmark->getUserId()); + $folderPath = $this->getArchivePath($bookmark, $userFolder); + $name = $bookmark->slugify('title'); + $extension = $this->mimey->getExtension($contentType) ?? 'txt'; + + $i = 0; + do { + $path = $folderPath . '/' . $name . ($i > 0 ? '_' . $i : '') . '.' . $extension; + $i++; + } while ($userFolder->nodeExists($path)); + + $file = $userFolder->newFile($path); + $file->putContent($body); + $bookmark->setArchivedFile($file->getId()); + $this->bookmarkMapper->update($bookmark); + } catch (NotPermittedException|NoUserException|GenericFileException|LockedException|UrlParseError|InvalidPathException|NotFoundException $e) { + $this->logger->debug(get_class($e) . ' ' . $e->getMessage() . "\r\n" . $e->getTraceAsString()); } } } diff --git a/tests/CrawlServiceTest.php b/tests/CrawlServiceTest.php new file mode 100644 index 000000000..9f4d6a499 --- /dev/null +++ b/tests/CrawlServiceTest.php @@ -0,0 +1,161 @@ +bookmarkMapper = $this->createMock(BookmarkMapper::class); + $this->bookmarkPreviewer = $this->createMock(BookmarkPreviewer::class); + $this->faviconPreviewer = $this->createMock(FaviconPreviewer::class); + $this->client = $this->createMock(IClient::class); + $this->clientService = $this->createMock(IClientService::class); + $this->clientService->method('newClient')->willReturn($this->client); + $this->config = $this->createMock(IConfig::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->l10n = $this->createMock(IL10N::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->userSettings = $this->createMock(UserSettingsService::class); + + $this->userFolder = $this->createMock(Folder::class); + $this->file = $this->createMock(File::class); + + $this->rootFolder->method('getUserFolder')->with('testuser')->willReturn($this->userFolder); + $this->userSettings->method('get')->with('archive.enabled')->willReturn('true'); + $this->config->method('getUserValue')->willReturn('Bookmarks'); + + $this->crawlService = new CrawlService( + $this->bookmarkMapper, + $this->bookmarkPreviewer, + $this->faviconPreviewer, + $this->clientService, + $this->config, + $this->rootFolder, + $this->l10n, + $this->logger, + $this->userSettings + ); + } + + public function testArchiveFileRejectsMissingContentLengthWhenBodyOversized(): void { + $bookmark = new Bookmark(); + $bookmark->setUrl('https://example.com/oversized.bin'); + $bookmark->setUserId('testuser'); + $bookmark->setTitle('Oversized File'); + + $oversizedBody = str_repeat('A', CrawlService::MAX_BODY_LENGTH + 1024); + + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getHeader')->willReturnCallback(function ($header) { + if ($header === 'Content-Type') { + return 'application/octet-stream'; + } + if ($header === 'Content-Length') { + return ''; // Missing Content-Length header (chunked) + } + return ''; + }); + $response->method('getBody')->willReturn($oversizedBody); + + $this->client->method('get')->willReturn($response); + + // An oversized body MUST NOT be written to user storage + $this->userFolder->expects($this->never())->method('newFile'); + + $this->crawlService->crawl($bookmark); + } + + public function testArchiveFileRejectsSpoofedContentLengthWhenBodyOversized(): void { + $bookmark = new Bookmark(); + $bookmark->setUrl('https://example.com/spoofed.bin'); + $bookmark->setUserId('testuser'); + $bookmark->setTitle('Spoofed Size File'); + + $oversizedBody = str_repeat('A', CrawlService::MAX_BODY_LENGTH + 1024); + + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getHeader')->willReturnCallback(function ($header) { + if ($header === 'Content-Type') { + return 'application/octet-stream'; + } + if ($header === 'Content-Length') { + return '10'; // Falsified small Content-Length header + } + return ''; + }); + $response->method('getBody')->willReturn($oversizedBody); + + $this->client->method('get')->willReturn($response); + + // An oversized body MUST NOT be written to user storage + $this->userFolder->expects($this->never())->method('newFile'); + + $this->crawlService->crawl($bookmark); + } + + public function testArchiveFileSavesValidBodyWithinLimit(): void { + $bookmark = new Bookmark(); + $bookmark->setUrl('https://example.com/valid.bin'); + $bookmark->setUserId('testuser'); + $bookmark->setTitle('Valid File'); + + $validBody = 'small file payload'; + + $response = $this->createMock(IResponse::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getHeader')->willReturnCallback(function ($header) { + if ($header === 'Content-Type') { + return 'application/octet-stream'; + } + if ($header === 'Content-Length') { + return (string)strlen('small file payload'); + } + return ''; + }); + $response->method('getBody')->willReturn($validBody); + + $this->client->method('get')->willReturn($response); + + $this->userFolder->method('nodeExists')->willReturn(false); + $this->userFolder->expects($this->once())->method('newFile')->willReturn($this->file); + $this->file->expects($this->once())->method('putContent')->with($validBody); + $this->file->method('getId')->willReturn(12345); + + $this->crawlService->crawl($bookmark); + + $this->assertEquals(12345, $bookmark->getArchivedFile()); + } +}