From 93d53c168770a6032bc72481ff75e897f9bd8801 Mon Sep 17 00:00:00 2001 From: MateuszKolankowski Date: Mon, 25 May 2026 14:45:12 +0200 Subject: [PATCH 1/2] Add filename validation in DownloadController and corresponding tests --- .../Controller/Content/DownloadController.php | 4 + .../Content/DownloadControllerTest.php | 142 ++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100644 tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php diff --git a/src/lib/MVC/Symfony/Controller/Content/DownloadController.php b/src/lib/MVC/Symfony/Controller/Content/DownloadController.php index 64f7debd0d..045aa8a5d4 100644 --- a/src/lib/MVC/Symfony/Controller/Content/DownloadController.php +++ b/src/lib/MVC/Symfony/Controller/Content/DownloadController.php @@ -116,6 +116,10 @@ public function downloadBinaryFileAction(int $contentId, string $fieldIdentifier ); } + if ($field->value->fileName !== $filename) { + throw new NotFoundException('File', $filename); + } + $response = new BinaryStreamResponse($this->ioService->loadBinaryFile($field->value->id), $this->ioService); $response->setContentDisposition( ResponseHeaderBag::DISPOSITION_ATTACHMENT, diff --git a/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php new file mode 100644 index 0000000000..1c02064194 --- /dev/null +++ b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php @@ -0,0 +1,142 @@ +contentService = $this->createMock(ContentService::class); + $this->ioService = $this->createMock(IOServiceInterface::class); + $this->translationHelper = $this->createMock(TranslationHelper::class); + } + + public function testDownloadBinaryFileActionReturnsBinaryResponseWhenFilenameMatches(): void + { + $content = $this->createContent(); + $field = $content->getField('file', 'eng-GB'); + self::assertInstanceOf(Field::class, $field); + + $request = new Request(['inLanguage' => 'eng-GB']); + $binaryFile = new BinaryFile([ + 'id' => 'binary-file-id', + 'mtime' => new DateTime(), + 'size' => 123, + 'uri' => 'binary-file-uri', + ]); + + $this->contentService + ->expects($this->once()) + ->method('loadContent') + ->with(42) + ->willReturn($content); + $this->translationHelper + ->expects($this->once()) + ->method('getTranslatedField') + ->with($content, 'file', 'eng-GB') + ->willReturn($field); + $this->ioService + ->expects($this->once()) + ->method('loadBinaryFile') + ->with('binary-file-id') + ->willReturn($binaryFile); + + $response = $this->createController()->downloadBinaryFileAction(42, 'file', 'Test-file.pdf', $request); + + self::assertStringContainsString('attachment;', (string) $response->headers->get('Content-Disposition')); + self::assertStringContainsString('Test-file.pdf', (string) $response->headers->get('Content-Disposition')); + } + + public function testDownloadBinaryFileActionReturnsNotFoundWhenFilenameDoesNotMatch(): void + { + $this->expectException(NotFoundException::class); + + $content = $this->createContent(); + $field = $content->getField('file', 'eng-GB'); + self::assertInstanceOf(Field::class, $field); + + $request = new Request(['inLanguage' => 'eng-GB']); + + $this->contentService + ->expects($this->once()) + ->method('loadContent') + ->with(42) + ->willReturn($content); + $this->translationHelper + ->expects($this->once()) + ->method('getTranslatedField') + ->with($content, 'file', 'eng-GB') + ->willReturn($field); + $this->ioService + ->expects($this->never()) + ->method('loadBinaryFile'); + + $this->createController()->downloadBinaryFileAction(42, 'file', 'SomeRandomText.txt', $request); + } + + private function createController(): DownloadController + { + return new DownloadController( + $this->contentService, + $this->ioService, + $this->translationHelper + ); + } + + private function createContent(): Content + { + return new Content([ + 'internalFields' => [ + new Field([ + 'fieldDefIdentifier' => 'file', + 'languageCode' => 'eng-GB', + 'value' => new BinaryFileValue([ + 'id' => 'binary-file-id', + 'fileName' => 'Test-file.pdf', + ]), + ]), + ], + 'versionInfo' => new VersionInfo([ + 'contentInfo' => new ContentInfo([ + 'id' => 42, + 'mainLanguageCode' => 'eng-GB', + 'name' => 'Test content', + 'status' => ContentInfo::STATUS_PUBLISHED, + ]), + ]), + ]); + } +} From 51d7f2c13b2884ce42752f78e16526f387fa8abc Mon Sep 17 00:00:00 2001 From: MateuszKolankowski Date: Fri, 29 May 2026 17:53:58 +0200 Subject: [PATCH 2/2] Add test for URL-encoded filenames in DownloadController request flow --- phpstan-baseline-7.4.neon | 6 - phpstan-baseline-doctrine-persistence-v3.neon | 6 - phpstan-baseline-lte-8.1.neon | 6 + phpstan-baseline.neon | 12 -- .../DownloadControllerRequestFlowTest.php | 196 ++++++++++++++++++ .../MVC/Symfony/InternalRoutingTestKernel.php | 34 +++ .../Content/DownloadControllerTest.php | 8 +- 7 files changed, 241 insertions(+), 27 deletions(-) create mode 100644 tests/integration/Core/MVC/Symfony/Controller/Content/DownloadControllerRequestFlowTest.php create mode 100644 tests/integration/Core/MVC/Symfony/InternalRoutingTestKernel.php diff --git a/phpstan-baseline-7.4.neon b/phpstan-baseline-7.4.neon index d91c9ee853..b31d35a1cd 100644 --- a/phpstan-baseline-7.4.neon +++ b/phpstan-baseline-7.4.neon @@ -186,12 +186,6 @@ parameters: count: 1 path: src/lib/MVC/Symfony/Matcher/ContentBased/UrlAlias.php - - - message: '#^Method Ibexa\\Core\\Persistence\\Legacy\\Content\\FieldValue\\Converter\\DateAndTimeConverter\:\:getDateIntervalFromXML\(\) should return DateInterval but returns DateInterval\|false\.$#' - identifier: return.type - count: 1 - path: src/lib/Persistence/Legacy/Content/FieldValue/Converter/DateAndTimeConverter.php - - message: '#^Cannot access property \$ownerDocument on DOMElement\|false\.$#' identifier: property.nonObject diff --git a/phpstan-baseline-doctrine-persistence-v3.neon b/phpstan-baseline-doctrine-persistence-v3.neon index 35aaa35c0d..54b9d6b911 100644 --- a/phpstan-baseline-doctrine-persistence-v3.neon +++ b/phpstan-baseline-doctrine-persistence-v3.neon @@ -1,11 +1,5 @@ parameters: ignoreErrors: - - - message: '#^Call to function method_exists\(\) with Doctrine\\ORM\\EntityManagerInterface and ''isUninitializedObje…'' will always evaluate to true\.$#' - identifier: function.alreadyNarrowedType - count: 1 - path: src/lib/Persistence/Doctrine/SiteAccessAwareEntityManager.php - - message: '#^Method Doctrine\\Persistence\\ObjectManager\:\:clear\(\) invoked with 1 parameter, 0 required\.$#' identifier: arguments.count diff --git a/phpstan-baseline-lte-8.1.neon b/phpstan-baseline-lte-8.1.neon index dd1038f24c..3e37a0dd4b 100644 --- a/phpstan-baseline-lte-8.1.neon +++ b/phpstan-baseline-lte-8.1.neon @@ -1,5 +1,11 @@ parameters: ignoreErrors: + - + message: '#^Method Ibexa\\Core\\Persistence\\Legacy\\Content\\FieldValue\\Converter\\DateAndTimeConverter\:\:getDateIntervalFromXML\(\) should return DateInterval but returns DateInterval\|false\.$#' + identifier: return.type + count: 1 + path: src/lib/Persistence/Legacy/Content/FieldValue/Converter/DateAndTimeConverter.php + - message: "#^Class SensitiveParameterValue not found\\.$#" count: 1 diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 9bee1e5f4b..49b7609da5 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -66760,12 +66760,6 @@ parameters: count: 1 path: tests/lib/Persistence/Legacy/User/Role/LimitationConverterTest.php - - - message: '#^Instanceof between Ibexa\\Contracts\\Core\\Persistence\\User\\Policy and Ibexa\\Contracts\\Core\\Persistence\\User\\Policy will always evaluate to true\.$#' - identifier: instanceof.alwaysTrue - count: 1 - path: tests/lib/Persistence/Legacy/User/UserHandlerTest.php - - message: '#^Method Ibexa\\Tests\\Core\\Persistence\\Legacy\\User\\UserHandlerTest\:\:createRole\(\) has no return type specified\.$#' identifier: missingType.return @@ -67084,12 +67078,6 @@ parameters: count: 5 path: tests/lib/Persistence/Legacy/User/UserHandlerTest.php - - - message: '#^Right side of && is always true\.$#' - identifier: booleanAnd.rightAlwaysTrue - count: 1 - path: tests/lib/Persistence/Legacy/User/UserHandlerTest.php - - message: '#^Cannot call method fetchAll\(\) on Doctrine\\DBAL\\ForwardCompatibility\\Result\|int\|string\.$#' identifier: method.nonObject diff --git a/tests/integration/Core/MVC/Symfony/Controller/Content/DownloadControllerRequestFlowTest.php b/tests/integration/Core/MVC/Symfony/Controller/Content/DownloadControllerRequestFlowTest.php new file mode 100644 index 0000000000..4394ab4836 --- /dev/null +++ b/tests/integration/Core/MVC/Symfony/Controller/Content/DownloadControllerRequestFlowTest.php @@ -0,0 +1,196 @@ +contentService = $this->createMock(ContentService::class); + $this->ioService = $this->createMock(IOServiceInterface::class); + $this->translationHelper = $this->createMock(TranslationHelper::class); + } + + public function testDownloadsFileWithUrlEncodedFilename(): void + { + $routes = $this->createRouteCollection(); + $context = new RequestContext(); + $url = (new UrlGenerator($routes, $context))->generate( + 'ibexa.content.download', + [ + 'contentId' => 42, + 'fieldIdentifier' => 'file', + 'filename' => self::FILENAME, + 'inLanguage' => 'eng-GB', + ] + ); + + self::assertStringContainsString('Q1%20report%20%231%20+%20100%25.jpg', $url); + + $content = $this->createContent(); + $field = $content->getField('file', 'eng-GB'); + self::assertInstanceOf(Field::class, $field); + + $binaryFile = new BinaryFile([ + 'id' => 'binary-file-id', + 'mtime' => new DateTime(), + 'size' => 123, + 'uri' => 'binary-file-uri', + ]); + + $this->contentService + ->expects($this->once()) + ->method('loadContent') + ->with(42) + ->willReturn($content); + $this->translationHelper + ->expects($this->once()) + ->method('getTranslatedField') + ->with($content, 'file', 'eng-GB') + ->willReturn($field); + $this->ioService + ->expects($this->once()) + ->method('loadBinaryFile') + ->with('binary-file-id') + ->willReturn($binaryFile); + + $this->configureDownloadController($routes); + + $response = $this->createHttpKernel($routes, $context)->handle( + Request::create($url), + HttpKernelInterface::MAIN_REQUEST, + false + ); + + self::assertSame(Response::HTTP_OK, $response->getStatusCode()); + self::assertInstanceOf(BinaryStreamResponse::class, $response); + } + + private function configureDownloadController(RouteCollection $routes): void + { + $route = $routes->get('ibexa.content.download'); + self::assertNotNull($route); + $route->setDefault('_controller', [$this->createController(), 'downloadBinaryFileAction']); + } + + private function createController(): DownloadController + { + return new DownloadController( + $this->contentService, + $this->ioService, + $this->translationHelper + ); + } + + private function createHttpKernel(RouteCollection $routes, RequestContext $context): HttpKernel + { + $requestStack = new RequestStack(); + $dispatcher = new EventDispatcher(); + $dispatcher->addSubscriber(new RouterListener( + new UrlMatcher($routes, $context), + $requestStack, + $context + )); + + $controllerResolver = self::getContainer()->get('controller_resolver'); + self::assertInstanceOf(ControllerResolverInterface::class, $controllerResolver); + + return new HttpKernel($dispatcher, $controllerResolver, $requestStack, new ArgumentResolver()); + } + + private function createRouteCollection(): RouteCollection + { + $routes = new RouteCollection(); + $routes->add('ibexa.content.download', new Route( + '/content/download/{contentId}/{fieldIdentifier}/{filename}', + [], + ['contentId' => '\d+'] + )); + + return $routes; + } + + private function createContent(): Content + { + return new Content([ + 'internalFields' => [ + new Field([ + 'fieldDefIdentifier' => 'file', + 'languageCode' => 'eng-GB', + 'value' => new BinaryFileValue([ + 'id' => 'binary-file-id', + 'fileName' => self::FILENAME, + ]), + ]), + ], + 'versionInfo' => new VersionInfo([ + 'contentInfo' => new ContentInfo([ + 'id' => 42, + 'mainLanguageCode' => 'eng-GB', + 'name' => 'Test content', + 'status' => ContentInfo::STATUS_PUBLISHED, + ]), + ]), + ]); + } +} diff --git a/tests/integration/Core/MVC/Symfony/InternalRoutingTestKernel.php b/tests/integration/Core/MVC/Symfony/InternalRoutingTestKernel.php new file mode 100644 index 0000000000..bf4a4c28fb --- /dev/null +++ b/tests/integration/Core/MVC/Symfony/InternalRoutingTestKernel.php @@ -0,0 +1,34 @@ +load(static function (ContainerBuilder $container): void { + self::loadRouting($container); + }); + } + + private static function loadRouting(ContainerBuilder $container): void + { + $container->loadFromExtension('framework', [ + 'router' => [ + 'resource' => '@IbexaCoreBundle/Resources/config/routing/internal.yml', + ], + ]); + } +} diff --git a/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php index 1c02064194..e2bd59a1fe 100644 --- a/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php +++ b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php @@ -4,6 +4,8 @@ * @copyright Copyright (C) Ibexa AS. All rights reserved. * @license For full copyright and license information view LICENSE file distributed with this source code. */ +declare(strict_types=1); + namespace Ibexa\Tests\Core\MVC\Symfony\Controller\Controller\Content; use DateTime; @@ -26,13 +28,13 @@ */ final class DownloadControllerTest extends TestCase { - /** @var \Ibexa\Contracts\Core\Repository\ContentService|\PHPUnit\Framework\MockObject\MockObject */ + /** @var \Ibexa\Contracts\Core\Repository\ContentService&\PHPUnit\Framework\MockObject\MockObject */ private $contentService; - /** @var \Ibexa\Core\IO\IOServiceInterface|\PHPUnit\Framework\MockObject\MockObject */ + /** @var \Ibexa\Core\IO\IOServiceInterface&\PHPUnit\Framework\MockObject\MockObject */ private $ioService; - /** @var \Ibexa\Core\Helper\TranslationHelper|\PHPUnit\Framework\MockObject\MockObject */ + /** @var \Ibexa\Core\Helper\TranslationHelper&\PHPUnit\Framework\MockObject\MockObject */ private $translationHelper; protected function setUp(): void