diff --git a/.cspell.json b/.cspell.json index 5244804..3e94a6f 100644 --- a/.cspell.json +++ b/.cspell.json @@ -42,6 +42,7 @@ "gnomovision", "hookspec", "icanon", + "itok", "initialisation", "initialised", "jangregor", diff --git a/filefield_paths.module b/filefield_paths.module index 79d23c0..aeec238 100644 --- a/filefield_paths.module +++ b/filefield_paths.module @@ -26,6 +26,7 @@ use Drupal\filefield_paths\Hook\FieldWidgetSingleElementForm; use Drupal\filefield_paths\Hook\File; use Drupal\filefield_paths\Hook\FileFieldPathsFieldSettingsLegacy; use Drupal\filefield_paths\Hook\FileFieldPathsProcessFileLegacy; +use Drupal\filefield_paths\Hook\FileUrlHooks; use Drupal\filefield_paths\Hook\LocalTaskAlter; use Drupal\filefield_paths\Hook\Tokens; use Drupal\filefield_paths\MoveFileProcessorInterface; @@ -212,6 +213,12 @@ function filefield_paths_file_presave(FileInterface $file): void {// phpcs:ignor \Drupal::service(File::class)->filePresave($file); } +// @phpstan-ignore-next-line +#[LegacyHook] +function filefield_paths_file_url_alter(string &$uri): void {// phpcs:ignore Drupal.Commenting.FunctionComment.Missing, Squiz.WhiteSpace.FunctionSpacing.Before + \Drupal::service(FileUrlHooks::class)->fileUrlAlter($uri); +} + // @phpstan-ignore-next-line #[LegacyHook] function filefield_paths_local_tasks_alter(array &$local_tasks): void {// phpcs:ignore Drupal.Commenting.FunctionComment.Missing, Squiz.WhiteSpace.FunctionSpacing.Before diff --git a/filefield_paths.routing.yml b/filefield_paths.routing.yml index 7eaa9de..d05c81e 100644 --- a/filefield_paths.routing.yml +++ b/filefield_paths.routing.yml @@ -5,3 +5,14 @@ filefield_paths.admin_settings: _title: 'File (Field) Paths settings' requirements: _permission: 'administer site configuration' + +filefield_paths.image_style_temporary: + path: '/filefield_paths/image-style/{image_style}/temporary' + defaults: + _controller: '\Drupal\image\Controller\ImageStyleDownloadController::deliver' + scheme: 'temporary' + required_derivative_scheme: 'temporary' + requirements: + _ffp_temp_image_style: 'TRUE' + options: + no_cache: TRUE diff --git a/filefield_paths.services.yml b/filefield_paths.services.yml index fcac198..0a3ed62 100644 --- a/filefield_paths.services.yml +++ b/filefield_paths.services.yml @@ -20,6 +20,12 @@ services: class: Drupal\filefield_paths\PathProcessor autowire: true + filefield_paths.access_checker.image_style_temporary: + class: Drupal\filefield_paths\Access\ImageStyleTemporaryAccessCheck + arguments: ['@config.factory'] + tags: + - { name: access_check, applies_to: _ffp_temp_image_style } + # Legacy hook support Drupal\filefield_paths\Hook\EntityWithFileField: class: Drupal\filefield_paths\Hook\EntityWithFileField @@ -45,3 +51,6 @@ services: Drupal\filefield_paths\Hook\Tokens: class: Drupal\filefield_paths\Hook\Tokens autowire: true + Drupal\filefield_paths\Hook\FileUrlHooks: + class: Drupal\filefield_paths\Hook\FileUrlHooks + autowire: true diff --git a/src/Access/ImageStyleTemporaryAccessCheck.php b/src/Access/ImageStyleTemporaryAccessCheck.php new file mode 100644 index 0000000..1ae57fd --- /dev/null +++ b/src/Access/ImageStyleTemporaryAccessCheck.php @@ -0,0 +1,62 @@ +setCacheContexts(['url.query_args:file']) + ->setCacheTags(['config:filefield_paths.settings']); + + $file = (string) ($request->query->get('file') ?? ''); + if ($file === '') { + return AccessResult::forbidden()->addCacheableDependency($cacheability); + } + + $temp_location = $this->configFactory + ->get('filefield_paths.settings') + ->get('temp_location') ?? ''; + + if (StreamWrapperManager::getScheme($temp_location) !== 'temporary') { + return AccessResult::forbidden()->addCacheableDependency($cacheability); + } + + $subdir = StreamWrapperManager::getTarget($temp_location); + if (!is_string($subdir) || $subdir === '') { + return AccessResult::forbidden()->addCacheableDependency($cacheability); + } + + $normalized_file = ltrim(str_replace('\\', '/', $file), '/'); + if (preg_match('~(^|/)\.\.(/|$)~', $normalized_file)) { + return AccessResult::forbidden()->addCacheableDependency($cacheability); + } + + $allowed = str_starts_with($normalized_file, $subdir . '/'); + return AccessResult::allowedIf($allowed)->addCacheableDependency($cacheability); + } + +} diff --git a/src/Hook/FileUrlHooks.php b/src/Hook/FileUrlHooks.php new file mode 100644 index 0000000..7a5f70a --- /dev/null +++ b/src/Hook/FileUrlHooks.php @@ -0,0 +1,48 @@ +configFactory + ->get('filefield_paths.settings') + ->get('temp_location') ?? ''; + + if (StreamWrapperManager::getScheme($temp_location) !== 'temporary') { + return; + } + + if (preg_match('#^temporary://styles/([^/]+)/temporary/(.+)$#', $uri, $m)) { + $uri = Url::fromRoute( + 'filefield_paths.image_style_temporary', + ['image_style' => $m[1]], + ['query' => ['file' => $m[2]], 'absolute' => TRUE], + )->toString(); + } + } + +} diff --git a/tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php b/tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php new file mode 100644 index 0000000..d0dfd89 --- /dev/null +++ b/tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php @@ -0,0 +1,149 @@ +getEditable('filefield_paths.settings') + ->set('temp_location', 'temporary://filefield_paths') + ->save(); + + // Create a simple image style. + $this->style = ImageStyle::create(['name' => 'ffp_test', 'label' => 'FFP test']); + $this->style->save(); + + // Copy a core test image into the FFP temp location. + $temp_location = 'temporary://filefield_paths'; + \Drupal::service('file_system')->prepareDirectory( + $temp_location, + FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS, + ); + $source = \Drupal::root() . '/core/tests/fixtures/files/image-1.png'; + $this->imageUri = \Drupal::service('file_system') + ->copy($source, $temp_location . '/image-1.png', FileExists::Replace); + } + + /** + * Tests that derivative URLs are rewritten to the FFP route. + */ + public function testUrlIsRewritten(): void { + $url = $this->style->buildUrl($this->imageUri); + $this->assertStringContainsString('/filefield_paths/image-style/ffp_test/temporary', $url); + $this->assertStringContainsString('file=filefield_paths/', $url); + $this->assertStringContainsString('itok=', $url); + } + + /** + * Tests that image style derivatives are served for temporary:// files. + */ + public function testDerivativeIsServed(): void { + $url = $this->style->buildUrl($this->imageUri); + + $this->drupalGet($url); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->responseHeaderContains('Content-Type', 'image/'); + } + + /** + * Tests that a missing or invalid itok returns 404. + */ + public function testMissingTokenReturns404(): void { + $url = $this->style->buildUrl($this->imageUri); + + // Strip the itok parameter. + $url_no_token = preg_replace('/[?&]itok=[^&]+/', '', $url); + + $this->drupalGet($url_no_token); + $this->assertSession()->statusCodeEquals(404); + } + + /** + * Tests that ?file= outside the FFP subdirectory returns 403. + */ + public function testFileOutsideSubdirReturns403(): void { + $url = $this->style->buildUrl($this->imageUri); + + // Replace the FFP subdirectory prefix with a different path. + $url_outside = preg_replace('#(\?|&)file=filefield_paths/#', '$1file=other_module/', $url); + + $this->drupalGet($url_outside); + $this->assertSession()->statusCodeEquals(403); + } + + /** + * Tests that accessing the route without a file parameter returns 403. + */ + public function testEmptyFileParamReturns403(): void { + $this->drupalGet('/filefield_paths/image-style/ffp_test/temporary'); + $this->assertSession()->statusCodeEquals(403); + } + + /** + * Tests that a path traversal attempt in ?file= returns 403. + */ + public function testPathTraversalReturns403(): void { + $url = $this->style->buildUrl($this->imageUri); + $url_traversal = preg_replace('#(\?|&)file=filefield_paths/#', '$1file=filefield_paths/../', $url); + $this->drupalGet($url_traversal); + $this->assertSession()->statusCodeEquals(403); + } + + /** + * Tests that non-temporary temp_location does not trigger URL rewriting. + * + * When temp_location uses private://, the alter hook must not intercept + * derivative URLs, leaving them on the standard private delivery route. + */ + public function testPrivateTempLocationUnaffected(): void { + \Drupal::configFactory() + ->getEditable('filefield_paths.settings') + ->set('temp_location', 'private://filefield_paths') + ->save(); + + $url = $this->style->buildUrl($this->imageUri); + $this->assertStringNotContainsString('/filefield_paths/image-style/', $url); + } + +} diff --git a/tests/src/Kernel/FileUrlHooksTest.php b/tests/src/Kernel/FileUrlHooksTest.php new file mode 100644 index 0000000..cc2b7bf --- /dev/null +++ b/tests/src/Kernel/FileUrlHooksTest.php @@ -0,0 +1,89 @@ + + */ + protected static $modules = [ + 'system', + 'user', + 'file', + 'filefield_paths', + ]; + + /** + * The hook implementation under test. + */ + protected FileUrlHooks $fileUrlHooks; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + $this->fileUrlHooks = $this->container->get(FileUrlHooks::class); + } + + /** + * Sets the FFP temp location config. + */ + protected function setTempLocation(string $temp_location): void { + $this->config('filefield_paths.settings')->set('temp_location', $temp_location)->save(); + } + + /** + * Non-temporary temp_location leaves the URI untouched. + */ + public function testNonTemporarySchemeIsUnaffected(): void { + $this->setTempLocation('private://filefield_paths'); + $uri = 'temporary://styles/thumbnail/temporary/filefield_paths/image.png'; + $this->fileUrlHooks->fileUrlAlter($uri); + $this->assertSame('temporary://styles/thumbnail/temporary/filefield_paths/image.png', $uri); + } + + /** + * A URI that does not match the temporary style pattern is unaffected. + */ + public function testNonMatchingUriIsUnaffected(): void { + $this->setTempLocation('temporary://filefield_paths'); + $uri = 'temporary://filefield_paths/image.png'; + $this->fileUrlHooks->fileUrlAlter($uri); + $this->assertSame('temporary://filefield_paths/image.png', $uri); + } + + /** + * A matching temporary style derivative URI is rewritten to the FFP route. + */ + public function testMatchingUriIsRewritten(): void { + $this->setTempLocation('temporary://filefield_paths'); + $uri = 'temporary://styles/thumbnail/temporary/filefield_paths/image.png'; + $this->fileUrlHooks->fileUrlAlter($uri); + $this->assertStringContainsString('/filefield_paths/image-style/thumbnail/temporary', $uri); + $this->assertStringContainsString('file=', $uri); + $this->assertStringContainsString('filefield_paths', $uri); + } + +} diff --git a/tests/src/Kernel/ImageStyleTemporaryAccessCheckTest.php b/tests/src/Kernel/ImageStyleTemporaryAccessCheckTest.php new file mode 100644 index 0000000..be2f8f1 --- /dev/null +++ b/tests/src/Kernel/ImageStyleTemporaryAccessCheckTest.php @@ -0,0 +1,122 @@ + + */ + protected static $modules = [ + 'system', + 'user', + 'file', + 'filefield_paths', + ]; + + /** + * The access checker under test. + */ + protected ImageStyleTemporaryAccessCheck $accessCheck; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + $this->accessCheck = $this->container->get('filefield_paths.access_checker.image_style_temporary'); + } + + /** + * Sets the FFP temp location config. + */ + protected function setTempLocation(string $temp_location): void { + $this->config('filefield_paths.settings')->set('temp_location', $temp_location)->save(); + } + + /** + * An empty ?file= parameter is forbidden. + */ + public function testEmptyFileIsForbidden(): void { + $this->setTempLocation('temporary://filefield_paths'); + $result = $this->accessCheck->access(Request::create('/')); + $this->assertTrue($result->isForbidden()); + if (!$result instanceof CacheableDependencyInterface) { + $this->fail('Expected a cacheable access result.'); + } + $this->assertSame(['url.query_args:file'], $result->getCacheContexts()); + $this->assertSame(['config:filefield_paths.settings'], $result->getCacheTags()); + } + + /** + * A non-temporary temp_location forbids access regardless of ?file=. + */ + public function testNonTemporarySchemeIsForbidden(): void { + $this->setTempLocation('private://filefield_paths'); + $result = $this->accessCheck->access(Request::create('/', 'GET', ['file' => 'filefield_paths/image.png'])); + $this->assertTrue($result->isForbidden()); + } + + /** + * An empty subdirectory in temp_location forbids access. + */ + public function testEmptySubdirIsForbidden(): void { + $this->setTempLocation('temporary://'); + $result = $this->accessCheck->access(Request::create('/', 'GET', ['file' => 'anything.png'])); + $this->assertTrue($result->isForbidden()); + } + + /** + * A path traversal sequence in ?file= is forbidden. + */ + public function testPathTraversalIsForbidden(): void { + $this->setTempLocation('temporary://filefield_paths'); + $result = $this->accessCheck->access(Request::create('/', 'GET', ['file' => 'filefield_paths/../etc/passwd'])); + $this->assertTrue($result->isForbidden()); + } + + /** + * A file outside the configured subdirectory is not allowed. + * + * AllowedIf(FALSE) returns a neutral result rather than an explicit forbid, + * so this asserts isAllowed() rather than isForbidden() (unlike the other + * negative cases above, which return AccessResult::forbidden() directly). + */ + public function testFileOutsideSubdirIsNotAllowed(): void { + $this->setTempLocation('temporary://filefield_paths'); + $result = $this->accessCheck->access(Request::create('/', 'GET', ['file' => 'other_module/image.png'])); + $this->assertFalse($result->isAllowed()); + } + + /** + * A file inside the configured subdirectory is allowed. + */ + public function testFileInsideSubdirIsAllowed(): void { + $this->setTempLocation('temporary://filefield_paths'); + $result = $this->accessCheck->access(Request::create('/', 'GET', ['file' => 'filefield_paths/image.png'])); + $this->assertTrue($result->isAllowed()); + } + +}