Repository navigation
Security: fix open advisories not covered by #455 and #456 - #457
Merged
Merged
Conversation
added 6 commits
October 13, 2026 11:44
…hunks The qquuid was used unchecked as a folder name and the combined file was written before its extension was validated.
RemoveUploadedFile used the type parameter unchecked in the file path and, like UploadFile, skipped the csrf check of the parent AjaxAction.
Backslashes passed the old check, and browsers treat "/\" like "//".
Media item titles and urls, page extra labels, location titles, search synonyms and multipleTextbox values were inserted without (complete) escaping. htmlEncode now also escapes quotes so it is safe in attributes.
Reviewer's GuideThis security-focused PR addresses multiple independent advisories by validating filesystem and tracking inputs, parameterizing locale queries, enforcing CSRF and directory restrictions on Pages uploads, limiting login redirects to local paths, escaping backend-rendered values, and disabling SVG uploads in CKFinder; it also adds focused MediaLibrary upload regression tests. Sequence diagram for secure chunked media uploadsequenceDiagram
participant Client
participant UploadHandler
participant Filesystem
Client->>UploadHandler: combineChunks(uploadDirectory, name)
UploadHandler->>UploadHandler: getUuid()
UploadHandler->>UploadHandler: checkFileExtension(name)
alt invalid UUID or extension
UploadHandler-->>Client: success false, preventRetry true
else valid upload
UploadHandler->>Filesystem: write combined file under validated UUID folder
UploadHandler-->>Client: upload result
end
Sequence diagram for CSRF-protected Pages file actionssequenceDiagram
participant Uploader
participant UploadFile
participant RemoveUploadedFile
participant AjaxAction
Uploader->>UploadFile: execute() with X-CSRF-Token
UploadFile->>AjaxAction: parent.execute()
AjaxAction-->>UploadFile: CSRF validated
UploadFile-->>Uploader: upload response
Uploader->>RemoveUploadedFile: execute() with file and type
RemoveUploadedFile->>AjaxAction: parent.execute()
AjaxAction-->>RemoveUploadedFile: CSRF validated
RemoveUploadedFile->>RemoveUploadedFile: validate type as UserTemplate
RemoveUploadedFile-->>Uploader: remove response
Sequence diagram for validated login redirectsequenceDiagram
participant User
participant Login
participant Filter
participant Browser
User->>Login: authenticate with redirect path
Login->>Filter: filter_var(queryString, FILTER_SANITIZE_URL)
Login->>Login: sanitizeQueryString(queryString, default)
alt one leading slash and no backslashes
Login->>Browser: redirect to local path
else invalid or external path
Login->>Browser: redirect to default local URL
end
Flow diagram for safe backend output renderingflowchart LR
Input[Untrusted backend value] --> Encode[utils.string.htmlEncode]
Encode --> Attribute[Escaped HTML attribute or text]
Attribute --> Render[Backend UI]
Encode --> Multiple[multipleTextbox values]
Encode --> Media[Media titles and URLs]
Encode --> Location[Location titles]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Backend/Modules/Pages/Ajax/UploadFile.php" line_range="38" />
<code_context>
+
public function execute(): void
{
+ parent::execute();
+
$request = $this->getRequest();
</code_context>
<issue_to_address>
**SVG upload remains enabled**
When a user with access to the Pages image uploader uploads an SVG and a same-origin visitor opens the stored file, `UploadFile::writeFile()` accepts `svg` in `ALLOWED_EXTENSIONS` and saves the uploaded bytes without sanitizing them, so an authenticated uploader can store an executable SVG under the publicly served Pages files path and expose same-origin visitors to script execution.
Reject SVG uploads in `UploadFile` or sanitize them before saving.
</issue_to_address>
### Comment 2
<location path="src/Backend/Modules/MediaLibrary/Js/MediaLibraryHelper.js" line_range="1547" />
<code_context>
if (mediaItem.type === 'image') {
- html += '<img src="' + mediaItem.preview_source + '" alt="' + mediaItem.title + '" title="' + mediaItem.title + '"/>'
+ html += '<img src="' + mediaItem.preview_source + '" alt="' + utils.string.htmlEncode(mediaItem.title) + '" title="' + utils.string.htmlEncode(mediaItem.title) + '"/>'
} else {
html += '<div class="icon"></div>'
</code_context>
<issue_to_address>
**Media preview URL enables XSS**
When an attacker able to upload media supplies a valid image with a filename containing attribute-breaking quotes, `preview_source` remains interpolated raw into each image’s `src` attribute. An attacker who uploads a valid image with a filename containing quotes and an event-handler attribute stores those characters in the media URL, so opening the media chooser parses attacker-controlled markup and runs the handler.
HTML-attribute-encode `preview_source` at each `src` interpolation, and ensure stored media URLs are safely encoded for their URL context.
Also at `src/Backend/Modules/MediaLibrary/Js/MediaLibraryHelper.js:1583`, `src/Backend/Modules/MediaLibrary/Js/MediaLibraryHelper.js:1611`.
</issue_to_address>
### Comment 3
<location path="src/Backend/Modules/MediaLibrary/Component/UploadHandler.php" line_range="63" />
<code_context>
}
+
+ try {
+ $uuid = $this->getUuid();
+ $this->checkFileExtension($name);
+ } catch (Exception $e) {
</code_context>
<issue_to_address>
**Retried completion creates empty upload**
When a duplicate or retried `done` request runs after another completion request has consumed the chunks, `combineChunks` opens the destination with `wb` before checking that the chunk files still exist, then reports success even if reopening chunks fails. If a completion request is retried after another request consumed the chunks, the retry creates an empty file and the upload endpoint can dispatch it as a media item.
Make chunk combination atomic and idempotent: claim the UUID once, verify all chunks and writes, and return the prior result or an error for subsequent completion requests.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: src/Backend/Modules/Pages/Ajax/UploadFile.php:38, src/Backend/Modules/MediaLibrary/Js/MediaLibraryHelper.js:1547, src/Backend/Modules/MediaLibrary/Component/UploadHandler.php:63
| 'name' => 'Files', // Single quotes not allowed. | ||
| 'directory' => 'files', | ||
| 'maxSize' => 0, | ||
| 'allowedExtensions' => '7z,aiff,asf,avi,bmp,csv,doc,docx,eps,fla,flv,gif,gz,gzip,jpeg,jpg,mid,mov,mp3,mp4,mpc,mpeg,mpg,ods,odt,pdf,png,ppt,pptx,pxd,qt,ram,rar,rm,rmi,rmvb,rtf,sdc,sitd,svg,swf,sxc,sxw,tar,tgz,tif,tiff,txt,vsd,wav,webp,wma,wmv,xls,xlsx,zip', |
Member
There was a problem hiding this comment.
Dat gaat opgelost zijn met #456
Dus zou dat hier niet weghalen/zo oplossen.
Member
Author
There was a problem hiding this comment.
Klopt, ik heb de CKFinder-wijziging uit deze PR en uit de upstream-PR gehaald. Let wel: #456 raakt CKFinder momenteel niet aan (enkel MediaLibrary en Pages UploadFile), dus CKFinder aanvaardt nog steeds ongesanitizede SVG's van elke ingelogde backend-gebruiker (GHSA-fjq6-2rhm-4g37). @absumo, kan je CKFinder mee opnemen in #456?
jonasdekeukelaere
force-pushed
the
security-advisories-fixes
branch
from
October 13, 2026 12:37
776c1a7 to
3032295
Compare
tijsverkoyen
approved these changes
Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for the open security advisories on forkcms/forkcms that are not covered by #455 (GenerateUrl reflection) and #456 (SVG sanitizing). One commit per issue, most severe first:
qquuidwas used unchecked as a folder name and the combined file was written before the extension check, which allowed writing (or, through the cleanup step, deleting) arbitrary paths. Not reported in any advisory. Regression test added.RemoveUploadedFile/UploadFile(GHSA-xv8g-294j-qxg8):typeis now limited toUserTemplate, and both actions now run the CSRF check ofAjaxAction(the uploader sends the header viacustomHeaders).getTranslations(GHSA-mq46-r6w7-882w): languages and types are bound as placeholders instead of concatenated./, no backslashes), validated afterFILTER_SANITIZE_URL.multipleTextboxvalues.utils.string.htmlEncodenow also escapes quotes.Testing
UploadHandlerTestfails without the fix and passes with it.parameters.yml), so CI needs to cover those.Summary by Sourcery
Harden file uploads, redirects, database queries, dynamic output, and tracking configuration against the remaining reported security vulnerabilities.
Bug Fixes:
Enhancements:
Tests: