fix(#3121826): add on-demand image style delivery for temporary:// staged files - #5
fix(#3121826): add on-demand image style delivery for temporary:// staged files#5Decipher wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds temporary image-style delivery for files under the configured temporary staging directory. The change adds routing, access validation, URL rewriting, inline download handling, and kernel and functional tests. ChangesTemporary Image Style Delivery
Estimated code review effort: 3 (Moderate) | ~30 minutes Mergeability Score: 🟠 High · up to The new temporary image-style delivery path can misroute unrelated temporary derivatives and may allow path traversal to bypass the intended directory boundary, potentially exposing files that should not be downloadable. The PR is not merge-ready until the route scoping and traversal checks are corrected. Sequence Diagram(s)sequenceDiagram
participant Drupal
participant FileUrlHooks
participant Router
participant AccessChecker as ImageStyleTemporaryAccessCheck
participant Controller as ImageStyleDownloadController
Drupal->>FileUrlHooks: Alter temporary image-style URI
FileUrlHooks->>Router: Build temporary delivery URL
Router->>AccessChecker: Validate file query parameter
AccessChecker-->>Router: Allow or forbid request
Router->>Controller: Deliver derivative with temporary scheme
Controller-->>Drupal: Return image response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a8f056d to
4da2b52
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php (1)
100-119: ⚡ Quick winAdd a regression test for
../infilequery param.Current negative-path tests are good, but a traversal-shaped
filevalue (for examplefilefield_paths/../...) should be explicitly asserted as 403 to lock the boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php` around lines 100 - 119, Add a new test method in the FileFieldPathsImageStyleTemporaryTest class that specifically tests path traversal attempts using `../` in the file query parameter. The test should follow the same pattern as testFileOutsideSubdirReturns403 and testEmptyFileParamReturns403, constructing a URL where the file parameter contains a traversal sequence like filefield_paths/../, then call drupalGet with that URL, and assert the response status code is 403 to ensure path traversal attempts are properly blocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Access/ImageStyleTemporaryAccessCheck.php`:
- Around line 35-45: The access check in ImageStyleTemporaryAccessCheck needs to
enforce both the temporary scheme and prevent path traversal. First, validate
that the temp_location configuration value starts with the temporary:// scheme
before using it for the prefix check. Additionally, add a check to reject file
paths containing traversal segments like ../ to prevent out-of-scope access
patterns. Both validations should return AccessResult::forbidden() if they fail,
ensuring the str_starts_with check on line 44 cannot be bypassed through path
traversal or non-temporary scheme values.
- Around line 31-48: The ImageStyleTemporaryAccessCheck method has multiple
early return statements that create AccessResult objects without cache metadata.
The early returns at lines 32 and 41 (when file is empty or subdir is invalid)
return AccessResult::forbidden() without any cacheability information, while the
final return statement includes addCacheContexts and addCacheTags calls. Add the
same cache contexts ['url.query_args:file'] and cache tags
['config:filefield_paths.settings'] to all AccessResult returns in the method.
This ensures that all forbidden access decisions are properly cached and
invalidated when the file parameter or configuration changes, preventing stale
cached results.
---
Nitpick comments:
In `@tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php`:
- Around line 100-119: Add a new test method in the
FileFieldPathsImageStyleTemporaryTest class that specifically tests path
traversal attempts using `../` in the file query parameter. The test should
follow the same pattern as testFileOutsideSubdirReturns403 and
testEmptyFileParamReturns403, constructing a URL where the file parameter
contains a traversal sequence like filefield_paths/../, then call drupalGet with
that URL, and assert the response status code is 403 to ensure path traversal
attempts are properly blocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2328c92f-e5a5-43ba-b3e3-c13f1abaf323
📒 Files selected for processing (6)
.cspell.jsonfilefield_paths.routing.ymlfilefield_paths.services.ymlsrc/Access/ImageStyleTemporaryAccessCheck.phpsrc/Hook/FileUrlHooks.phptests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Access/ImageStyleTemporaryAccessCheck.php (1)
48-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
$subdirto match$normalized_filehandling.The
$filepath is normalized on line 53 (leading slashes trimmed), but$subdiris used raw fromgetTarget(). If the configuredtemp_locationhas a trailing slash (e.g.,temporary://filefield_paths/),getTarget()returnsfilefield_paths/, causing the line 58 check to compare againstfilefield_paths//—which always fails, incorrectly denying legitimate requests.Suggested fix
- $subdir = StreamWrapperManager::getTarget($temp_location); + $subdir = trim((string) StreamWrapperManager::getTarget($temp_location), '/'); if (!is_string($subdir) || $subdir === '') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Access/ImageStyleTemporaryAccessCheck.php` around lines 48 - 58, The $subdir variable obtained from StreamWrapperManager::getTarget() is not normalized like $normalized_file is on line 53. When the configured temp_location has a trailing slash, getTarget() returns a path with a trailing slash (e.g., "filefield_paths/"), which causes the str_starts_with check on line 58 to create a double slash ("filefield_paths//"), always failing the comparison. Normalize $subdir by removing trailing slashes using rtrim($subdir, '/') immediately after retrieving it from getTarget(), ensuring consistent path formatting for the subsequent comparison with $normalized_file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Access/ImageStyleTemporaryAccessCheck.php`:
- Around line 48-58: The $subdir variable obtained from
StreamWrapperManager::getTarget() is not normalized like $normalized_file is on
line 53. When the configured temp_location has a trailing slash, getTarget()
returns a path with a trailing slash (e.g., "filefield_paths/"), which causes
the str_starts_with check on line 58 to create a double slash
("filefield_paths//"), always failing the comparison. Normalize $subdir by
removing trailing slashes using rtrim($subdir, '/') immediately after retrieving
it from getTarget(), ensuring consistent path formatting for the subsequent
comparison with $normalized_file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9370a0ed-6eee-4380-aa53-70bf944e1381
📒 Files selected for processing (3)
filefield_paths.modulesrc/Access/ImageStyleTemporaryAccessCheck.phptests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/Functional/FileFieldPathsImageStyleTemporaryTest.php
ea168e8 to
01b6b90
Compare
Missed during the base-branch Rector fix (test: convert PHPUnit docblock annotations to attributes) since this file only exists on this branch.
Drupal 11.4 removed the token-valid shortcut in ImageStyleDownloadController::deliver() that previously treated temporary:// as a public scheme ($is_public = $scheme !== 'private' when the itok was valid). The controller now always invokes hook_file_download() for non-public schemes. Without an explicit grant, derivative requests for files staged in temporary://filefield_paths receive a 403 from AccessDeniedHttpException, breaking image thumbnails on the node edit form. Add hook_file_download() to FileUrlHooks that grants access to source images inside the configured FFP temp subdirectory. The hook mirrors core's ImageHooks::filePublish() pattern for private:// derivatives: it returns Content-Type/Content-Length from the source image so the controller treats the file as accessible. Security: the itok derivative token is validated by the controller before this hook fires, and the route access checker (ImageStyleTemporaryAccessCheck) already enforces the subdir boundary. The hook only confirms the source URI lives within that subdir.
4ccc6a5 to
5ac9473
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Hook/FileUrlHooks.php`:
- Around line 31-45: Update the URI rewriting logic near the temporary-location
check to read the configured staging subdirectory and only rewrite when the
captured path `$m[2]` starts with `$subdir . '/'`; leave unrelated temporary
URIs unchanged. Add a regression test covering
`temporary://styles/.../temporary/other_module/image.png` and verify it is not
converted to the File (Field) Paths route.
- Around line 89-95: Update the containment validation in the file download hook
around StreamWrapperManager::getTarget() to normalize path separators and reject
any .. path segments in the URI before applying the staging-subdirectory prefix
check. Ensure traversal targets return NULL and cannot receive download headers,
while preserving valid in-subdirectory access; add a kernel test covering
temporary://filefield_paths/../other_module/image.png.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6769eecb-0f03-4330-81f7-1733972a4fc3
📒 Files selected for processing (5)
filefield_paths.modulesrc/Hook/FileUrlHooks.phptests/src/Functional/FileFieldPathsImageStyleTemporaryTest.phptests/src/Kernel/FileUrlHooksTest.phptests/src/Kernel/ImageStyleTemporaryAccessCheckTest.php
| $temp_location = $this->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(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict URL rewriting to the configured staging subdirectory.
Line 39 rewrites every temporary://styles/.../temporary/... URI. It also rewrites derivatives for files outside the configured File (Field) Paths directory. The access checker then rejects those requests instead of leaving unrelated temporary files on their normal delivery path.
Read the configured target and require $m[2] to start with $subdir . '/' before creating the route. Add a regression test for temporary://styles/.../temporary/other_module/image.png.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Hook/FileUrlHooks.php` around lines 31 - 45, Update the URI rewriting
logic near the temporary-location check to read the configured staging
subdirectory and only rewrite when the captured path `$m[2]` starts with
`$subdir . '/'`; leave unrelated temporary URIs unchanged. Add a regression test
covering `temporary://styles/.../temporary/other_module/image.png` and verify it
is not converted to the File (Field) Paths route.
| $subdir = StreamWrapperManager::getTarget($temp_location); | ||
| $target = StreamWrapperManager::getTarget($uri); | ||
| if (!is_string($subdir) || $subdir === '' | ||
| || !is_string($target) | ||
| || !str_starts_with($target, $subdir . '/') | ||
| ) { | ||
| return NULL; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject traversal segments before granting download access.
temporary://filefield_paths/../other_module/image.png has a target that starts with filefield_paths/. This method returns an inline header although the resolved file is outside the staging subdirectory. The route access checker does not protect other callers of hook_file_download().
Normalize separators and reject .. path segments before the prefix check. Add a kernel test for this URI.
Proposed containment check
$subdir = StreamWrapperManager::getTarget($temp_location);
$target = StreamWrapperManager::getTarget($uri);
- if (!is_string($subdir) || $subdir === ''
- || !is_string($target)
- || !str_starts_with($target, $subdir . '/')
- ) {
+ if (!is_string($subdir) || !is_string($target)) {
+ return NULL;
+ }
+
+ $subdir = trim(str_replace('\\', '/', $subdir), '/');
+ $target = ltrim(str_replace('\\', '/', $target), '/');
+ if ($subdir === ''
+ || preg_match('~(^|/)\.\.(/|$)~', $target)
+ || !str_starts_with($target, $subdir . '/')) {
return NULL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $subdir = StreamWrapperManager::getTarget($temp_location); | |
| $target = StreamWrapperManager::getTarget($uri); | |
| if (!is_string($subdir) || $subdir === '' | |
| || !is_string($target) | |
| || !str_starts_with($target, $subdir . '/') | |
| ) { | |
| return NULL; | |
| $subdir = StreamWrapperManager::getTarget($temp_location); | |
| $target = StreamWrapperManager::getTarget($uri); | |
| if (!is_string($subdir) || !is_string($target)) { | |
| return NULL; | |
| } | |
| $subdir = trim(str_replace('\\', '/', $subdir), '/'); | |
| $target = ltrim(str_replace('\\', '/', $target), '/'); | |
| if ($subdir === '' | |
| || preg_match('~(^|/)\.\.(/|$)~', $target) | |
| || !str_starts_with($target, $subdir . '/')) { | |
| return NULL; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Hook/FileUrlHooks.php` around lines 89 - 95, Update the containment
validation in the file download hook around StreamWrapperManager::getTarget() to
normalize path separators and reject any .. path segments in the URI before
applying the staging-subdirectory prefix check. Ensure traversal targets return
NULL and cannot receive download headers, while preserving valid in-subdirectory
access; add a kernel test covering
temporary://filefield_paths/../other_module/image.png.
The #[Hook('file_download')] attribute on FileUrlHooks::fileDownload()
only fires on Drupal 11. Drupal 10 requires a procedural wrapper marked
with #[LegacyHook] that delegates to the service, matching the existing
pattern used for hook_file_url_alter() and every other hook in the
module.
Without this wrapper, hook_file_download() is never registered on D10,
so ImageStyleDownloadController::deliver() finds no module granting
access for temporary:// source files and throws
AccessDeniedHttpException (403).
1 participant
Summary
Fixes broken image thumbnails on the node edit form when File (Field) Paths
stages uploaded files in
temporary://. Image style derivatives are nowgenerated and served on demand via a dedicated delivery route, so the default
temporary://filefield_pathstemp location works without switching toprivate://.Closes #3121826
Background
When an image field uses File (Field) Paths, uploaded files are staged in a
temporary location (
temporary://filefield_pathsby default) and moved totheir final path only when the entity is saved. Drupal core's image style
delivery controller does not handle
temporary://URIs, so derivative URLsreturn 404 — causing the "broken thumbnail" symptom reported in the issue.
The workaround was to switch
temp_locationtoprivate://filefield_paths,but this requires the private filesystem to be configured and adds access
overhead that isn't needed in most cases.
How it works
hook_file_url_alter()— intercepts derivative URIs matchingtemporary://styles/{style}/temporary/{file}and rewrites them to adedicated route.
/system/files/styles/{image_style}/temporary) —delegates to core's
ImageStyleDownloadController::deliver()withscheme: temporary, so derivative generation anditokvalidation workexactly as they do for public/private files.
_ffp_temp_image_style) — restricts the route tofiles within the configured FFP temp subdirectory, preventing arbitrary
temporary://file access. Whentemp_locationuses a non-temporaryscheme (e.g.
private://), the checker returns neutral so the normalprivate file delivery path applies.
Changes
src/Hook/FileUrlHooks.phphook_file_url_alter()— rewrites temporary derivative URIssrc/Access/ImageStyleTemporaryAccessCheck.phpfilefield_paths.routing.ymlfilefield_paths.services.ymltests/src/Functional/FileFieldPathsImageStyleTemporaryTest.phpTest plan
DRUPAL_VERSION=10 make lintpassesDRUPAL_VERSION=10 make testpassestemporary://filefield_paths— thumbnails appear on node edit formSummary by CodeRabbit
New Features
Security
Tests