Repository navigation
Accept SVG item icon uploads again - #1612
Merged
Merged
Conversation
Since the Laravel 11+ upgrade the bare "image" validation rule rejects SVG files, so uploading an SVG icon on the add/edit item form failed with "The file field must be an image." The form already sanitizes SVG uploads, so the rule now uses image:allow_svg. Validation and the stored file name both use the detected MIME type, while the sanitizer only ran when the client file name ended in .svg. With SVGs allowed, an SVG uploaded as icon.png would have been stored as .svg unsanitized, so sanitization now also runs when the detected type is SVG. Tag and user avatar uploads do not sanitize SVG and keep the stricter rule. Refs #1609
Validation and store() both name the file from guessExtension(), so the client extension check added nothing. Fold the two SVG upload tests into a data provider, move shared setup into setUp(), and clean up temp files.
Share one sanitizeSvg() helper between the upload and icon URL paths. It rejects SVGs the sanitizer cannot parse, and its script check now catches any case and attributes.
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 the second half of #1609: uploading an SVG icon on the add/edit item form fails with "The file field must be an image."
Why
Since the Laravel 11+ upgrade, the bare
imagevalidation rule excludes SVG unlessallow_svgis passed. The item form already runs uploaded SVGs throughenshrined/svg-sanitize, so it can accept them safely.What
ItemController::storelogicvalidatesfilewithimage:allow_svg..svg. Laravel validates and names the stored file from the detected type, so going by the file name would let an SVG uploaded asicon.pngpass validation and be stored as an unsanitized.svg.sanitizeSvg()helper. It rejects SVGs the sanitizer cannot parse, instead of storing an empty icon. Its script check is now case-insensitive and matches tags with attributes.Follow-up, not in this PR:
SettingsControllerandSetting::getInputstill decide whether to sanitize from the client file name.Tests
New
tests/Feature/ItemIconUploadTest.php:<script>andonloadstripped, whether it is named.svgor.png. The.pngcase fails if the sanitizer goes by the client file name.php artisan testis green (167 passed, 1 skipped). I also verified it manually against thelscr.io/linuxserver/heimdall:2.8.3image: the upload is rejected before the patch and saved after it.The "Icon could not be downloaded" half of #1609 did not reproduce, either locally or in the 2.8.3 container. It looks environmental, so I've asked the reporter for logs on the issue.