Validate uploaded file attachments are images#128
Open
n3crosis wants to merge 4 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens TinyEditor’s Livewire temporary-upload persistence path by validating that files referenced by <img data-id="..."> are actually images before they are saved as file attachments.
Changes:
- Added a MIME-type guard requiring uploaded attachments to start with
image/. - Added a content-based guard using
getimagesize()to ensure the file is a readable image before persisting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $attachment = $this->getUploadedFileAttachment($fileKey); | ||
|
|
||
| if ($attachment) { | ||
| if (! str_starts_with($attachment->getMimeType(), 'image/')) { |
Comment on lines
+126
to
+129
| if (! getimagesize($attachment->getRealPath())) { | ||
| continue; | ||
| } | ||
|
|
Author
|
Quick reproduction note for verification:
Impact note: the missing validation definitely allows unintended non-image file persistence / exposure. A |
…tion - Normalize getMimeType() to '' before str_starts_with() to avoid TypeError when MIME type cannot be determined - Guard getRealPath() result as a non-empty string before reading file contents, skipping attachments whose temp file is missing - Replace getimagesize($path) with getimagesizefromstring($contents) to avoid PHP warnings on invalid/unreadable files and remove any dependency on a real filesystem path at validation time
Accidentally overwrote these in the previous commit; revert them back to the values from bae4f48 (the base of this branch) while keeping the getMimeType/getRealPath/getimagesizefromstring security fixes.
Comment on lines
+130
to
+137
| // getRealPath() returns false for stream wrappers or missing | ||
| // temp files. Read the file contents instead and validate the | ||
| // image data with getimagesizefromstring(), which is also free | ||
| // of the PHP warnings that getimagesize() emits on bad input. | ||
| $realPath = $attachment->getRealPath(); | ||
| if (! is_string($realPath) || $realPath === '') { | ||
| continue; | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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
This adds validation before persisting TinyEditor uploaded attachments from Livewire temporary storage.
TinyEditor::setUp()currently walks<img>tags in the submitted HTML, resolves thedata-idback to a temporary upload, and then callssaveUploadedFileAttachment()directly. That means a crafted request can reference a non-image temporary upload through an<img>tag and have it persisted as an editor attachment.This patch adds two guards before saving the attachment:
image/getimagesize()to succeed on the uploaded fileWhy this matters
Without these checks, the code path trusts that any temporary upload referenced by an
<img>element is actually an image. In practice, that allows non-image files to move from temporary upload storage into the configured attachment storage if the request payload is manipulated.Validation
php -l src/TinyEditor.phpThis patch is based on a local security fix already in production use.