fix(backend): validate uploaded images with libmagic#3353
Merged
Conversation
Avatar, ROM artwork, and collection artwork uploads now sniff the file header with libmagic and reject anything that isn't PNG/JPEG/WebP/GIF, saving the file with an extension derived from the detected MIME rather than the user-supplied filename. Pairs with the raw asset endpoint, which decides inline vs attachment from the on-disk extension. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens backend asset handling by validating uploaded “image” files via libmagic and by ensuring the raw asset endpoint only serves trusted image types inline (everything else as an attachment) to mitigate stored XSS risks.
Changes:
- Added
validate_image_upload()+SAFE_IMAGE_MIME_TYPESto sniff uploads and derive a trusted extension. - Applied image sniffing to avatar, ROM artwork, and collection artwork upload flows (extension no longer taken from user filename).
- Updated
/raw/assets/...responses to serve only trusted image MIME types inline; others asapplication/octet-streamattachments, with tests updated accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/handler/filesystem/assets_handler.py | Adds shared libmagic-based image upload validation + safe MIME/extension mapping. |
| backend/endpoints/user.py | Uses validation helper for avatar uploads and enforces trusted extension. |
| backend/endpoints/roms/init.py | Uses validation helper for ROM artwork uploads. |
| backend/endpoints/collections.py | Uses validation helper for collection artwork uploads (add/update). |
| backend/endpoints/raw.py | Builds FileResponse with inline vs attachment behavior based on safe image MIME types. |
| backend/tests/endpoints/test_raw.py | Updates assertions to match new attachment behavior for non-image assets. |
| backend/tests/endpoints/test_identity.py | Adds avatar upload tests for rejecting non-images and accepting valid PNG with filename spoofing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Test Results (mariadb) 1 files 1 suites 4m 3s ⏱️ Results for commit e3aaa10. ♻️ This comment has been updated with latest results. |
Contributor
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Contributor
Test Results (postgresql) 1 files 1 suites 3m 27s ⏱️ Results for commit e3aaa10. ♻️ This comment has been updated with latest results. |
Adds rejection + acceptance tests for update_rom, add_collection, and update_collection artwork uploads, mirroring the existing avatar tests: non-image content returns 400, and a real PNG uploaded under a misleading filename like payload.html is stored with the trusted .png extension. Also fixes two `return HTTPException(...)` → `raise` in raw.py so the 404 path actually surfaces instead of silently returning the exception object. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
magic.Magic(mime=True) loads the magic database from disk on construction; instantiating it per request was adding pointless overhead to every avatar and artwork upload. Share a module-level instance guarded by a lock (the underlying magic_t handle is not thread-safe), and surface MagicException as a 400 so a sniffing failure fails closed instead of bubbling a 500. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
validate_image_upload()helper inassets_handler.pythat sniffs the leading bytes of an upload with libmagic and returns the trusted file extension, raisingHTTPException(400)if the file is not inSAFE_IMAGE_MIME_TYPES(PNG, JPEG, WebP, GIF).update_user), ROM artwork (update_rom), and collection artwork (add_collection/update_collection). Files are saved with the extension derived from the detected MIME type, not from the user-supplied filename.raw.pythat decides betweeninlineandattachmentcontent disposition based on the on-disk extension — this PR ensures that extension reflects the real file type, so a malicious filename likepayload.htmlcan no longer be served back as HTML.Previously the artwork endpoints relied on Pillow to fail closed on unrecognized formats; this is defense-in-depth and gives a clear 400 instead of a silent
(None, None)return.Test plan
test_update_user_rejects_non_image_avatarandtest_update_user_accepts_png_avatarstill pass.update_romwith a non-image file (e.g. HTML payload renamedcover.png) returns 400.add_collection/update_collectionwith a real PNG/JPEG/WebP/GIF still saves correctly and serves inline via/raw/assets/....romm_testMariaDB grant issue unrelated to this change).🤖 Generated with Claude Code