Repository navigation
Sanitize uploaded SVGs - #456
Merged
Merged
Conversation
Reviewer's GuideThe PR mitigates stored XSS from SVG uploads by introducing a DOM-based sanitizer, invoking it across the Pages and Media Library upload flows before persistent storage, cleaning up failed temporary uploads, and adding unit and integration regression tests. Sequence diagram for SVG upload sanitizationsequenceDiagram
participant Upload as Upload flow
participant Sanitizer as SvgSanitizer
participant DOM as DOMSanitizer
participant Storage as File storage
Upload->>Sanitizer: isSvgExtension(extension)
alt SVG upload
Upload->>Sanitizer: sanitize(content)
Sanitizer->>DOM: sanitize(content)
DOM-->>Sanitizer: sanitized SVG
Sanitizer-->>Upload: sanitized content
Upload->>Storage: write sanitized SVG
else Non-SVG upload
Upload->>Storage: write original file
end
Sequence diagram for failed Media Library SVG sanitizationsequenceDiagram
participant Upload as MediaItemUpload
participant Sanitizer as SvgSanitizer
participant Storage as Temporary storage
participant Client
Upload->>Storage: upload to temporary path
Upload->>Sanitizer: sanitizeFile(path)
alt Sanitization fails
Sanitizer-->>Upload: RuntimeException
Upload->>Storage: deleteFolder(uuid)
Upload-->>Client: sendResponseForResult(success=false)
else Sanitization succeeds
Sanitizer-->>Upload: sanitized file
Upload->>Storage: rename to final path
end
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 4 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="131-137" />
<code_context>
$fileName = $baseName . '.' . $extension;
+ // an svg is an active document, so strip the scripts and event handlers before it is stored
+ if (SvgSanitizer::isSvgExtension($extension)) {
+ try {
+ $content = (new SvgSanitizer())->sanitize($content);
+ } catch (RuntimeException $runtimeException) {
+ throw new Exception($runtimeException->getMessage());
+ }
+ }
+
</code_context>
<issue_to_address>
**CKFinder uploads retain active SVGs**
When an SVG is uploaded through CKFinder, `FileUpload::execute()` writes the uploaded stream without calling `SvgSanitizer`, leaving active SVG content available from the public files directory and allowing stored XSS.
Route CKFinder uploads through SVG sanitization before storing or publishing them.
Also at `src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php:103`, `src/ForkCMS/Utility/Svg/SvgSanitizer.php:15`.
</issue_to_address>
### Comment 2
<location path="src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php" line_range="103" />
<code_context>
+ // an svg is an active document, so strip the scripts and event handlers while the file still lives
+ // under its unguessable temporary name, it may never be reachable under its final name with the
+ // payload intact
+ $this->sanitizeSvg($tempUploadPath, $result);
+
// Move file to correct folder
</code_context>
<issue_to_address>
**Unsanitized SVGs are briefly reachable**
When a request fetches the temporary URL before sanitization completes, `UploadHandler` writes the original SVG beneath the public media root before `sanitizeSvg()` runs, and the request-supplied UUID makes the temporary path reachable; the server serves the active SVG and triggers stored XSS.
Keep temporary uploads outside the public root and publish them only after sanitization completes.
Also at `src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php:104`.
</issue_to_address>
### Comment 3
<location path="src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php" line_range="104" />
<code_context>
arguments:
- "%site.path_www%"
+ ForkCMS\Utility\Svg\SvgSanitizer:
+ public: true
</code_context>
<issue_to_address>
**Concurrent upload bypasses sanitization**
When two requests use the same upload UUID and filename, and one replaces the temporary file after the other sanitizes it but before it renames it, `sanitizeSvg()` sanitizes the shared temporary path, but another request can replace that file before `rename()` publishes it. The unsanitized replacement then becomes publicly accessible as an SVG.
Serialize finalization per upload UUID, or sanitize and atomically publish the exact file instance that was validated.
</issue_to_address>
### Comment 4
<location path="src/ForkCMS/Utility/Svg/SvgSanitizer.php" line_range="55" />
<code_context>
+ throw new RuntimeException('Could not read the SVG file "' . $path . '".');
+ }
+
+ if (@file_put_contents($path, $this->sanitize($content)) === false) {
+ throw new RuntimeException('Could not write the sanitized SVG file "' . $path . '".');
+ }
</code_context>
<issue_to_address>
**Truncated SVGs are accepted**
When the filesystem accepts only part of the sanitized content, `file_put_contents()` can return a positive short count, but `sanitizeFile()` rejects only `false`; it therefore accepts the truncated SVG and the upload proceeds with a corrupted asset.
Check that the number of bytes written equals the sanitized content length, and throw an exception otherwise.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and if the sanitizer fails to remove active SVG content, an uploaded file can remain available from the site origin and execute script when opened, exposing visitors to stored XSS. Reverting would stop future sanitization but would not remove or repair any malicious files already stored.
Blocking findings: src/Backend/Modules/Pages/Ajax/UploadFile.php:137, src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php:103, src/Backend/Modules/MediaLibrary/Ajax/MediaItemUpload.php:104, src/ForkCMS/Utility/Svg/SvgSanitizer.php:55
| "twig/inky-extra": "^3.23", | ||
| "symfony/uid": "^5.4", | ||
| "symfony/cache": "^5.4" | ||
| "symfony/cache": "^5.4", |
Member
There was a problem hiding this comment.
Deze zal dan ook geupdate moeten worden in symfony upgrade
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
3 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.
Summary by Sourcery
Protect uploaded SVGs from stored XSS by sanitizing active content before files are stored or served.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: