From 58f4a9abbc3fb9fb7538d73611a4f7ab0b8c5c8c Mon Sep 17 00:00:00 2001 From: KodeStar Date: Sun, 18 Oct 2026 13:12:58 +0100 Subject: [PATCH 1/3] Accept SVG item icon uploads again 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 --- app/Http/Controllers/ItemController.php | 7 +- tests/Feature/ItemIconUploadTest.php | 94 +++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/ItemIconUploadTest.php diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index dceb86355..da01f05de 100644 --- a/app/Http/Controllers/ItemController.php +++ b/app/Http/Controllers/ItemController.php @@ -241,14 +241,17 @@ public static function storelogic(Request $request, $id = null): Item $validatedData = $request->validate([ 'title' => 'required|max:255', 'url' => 'required', - 'file' => 'image' + // SVG icons are accepted here because they are sanitized below. + 'file' => 'image:allow_svg' ]); if ($request->hasFile('file')) { $image = $request->file('file'); $extension = $image->getClientOriginalExtension(); - if ($extension === 'svg') { + // Validation and the stored file name both go by the detected + // type, so an SVG uploaded under another extension is sanitized too. + if ($extension === 'svg' || $image->guessExtension() === 'svg') { $sanitizer = new Sanitizer(); $sanitizedSvg = $sanitizer->sanitize(file_get_contents($image->getRealPath())); diff --git a/tests/Feature/ItemIconUploadTest.php b/tests/Feature/ItemIconUploadTest.php new file mode 100644 index 000000000..d10a561d4 --- /dev/null +++ b/tests/Feature/ItemIconUploadTest.php @@ -0,0 +1,94 @@ +' + . '' + . ''; + + private function itemPayload(UploadedFile $file): array + { + return [ + 'pinned' => 1, + 'appid' => 'null', + 'website' => null, + 'title' => 'Item A', + 'colour' => '#00f', + 'url' => 'http://10.0.1.1', + 'tags' => [0], + 'file' => $file, + ]; + } + + /** + * A real file on disk, so the MIME type is detected from its contents + * rather than taken from the client file name. + */ + private function upload(string $contents, string $clientName): UploadedFile + { + $path = tempnam(sys_get_temp_dir(), 'icon'); + file_put_contents($path, $contents); + + return new UploadedFile($path, $clientName, null, null, true); + } + + private function storedIcon(): string + { + $item = Item::where('title', 'Item A')->firstOrFail(); + Storage::disk('public')->assertExists($item->icon); + + return Storage::disk('public')->get($item->icon); + } + + public function test_svg_icon_upload_is_accepted_and_sanitized(): void + { + Storage::fake('public'); + $this->seed(); + + $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, 'icon.svg'))); + + $response->assertSessionHasNoErrors(); + $stored = $this->storedIcon(); + $this->assertStringContainsString('assertStringNotContainsString('assertStringNotContainsString('onload', $stored); + } + + public function test_svg_uploaded_under_another_extension_is_still_sanitized(): void + { + Storage::fake('public'); + $this->seed(); + + $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, 'icon.png'))); + + $response->assertSessionHasNoErrors(); + $stored = $this->storedIcon(); + $this->assertStringNotContainsString('assertStringNotContainsString('onload', $stored); + } + + public function test_non_image_upload_is_rejected(): void + { + Storage::fake('public'); + $this->seed(); + + $response = $this->post('/items', $this->itemPayload($this->upload('just some text', 'icon.txt'))); + + $response->assertSessionHasErrors('file'); + $this->assertNull(Item::where('title', 'Item A')->first()); + } +} From e3746aecb6f7dfe127fcb92be1189f0e70748020 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Mon, 19 Oct 2026 09:06:48 +0100 Subject: [PATCH 2/3] Gate SVG icon sanitizing on the detected type only 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. --- app/Http/Controllers/ItemController.php | 5 +-- tests/Feature/ItemIconUploadTest.php | 60 +++++++++++++------------ 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index da01f05de..209c2bf76 100644 --- a/app/Http/Controllers/ItemController.php +++ b/app/Http/Controllers/ItemController.php @@ -247,11 +247,10 @@ public static function storelogic(Request $request, $id = null): Item if ($request->hasFile('file')) { $image = $request->file('file'); - $extension = $image->getClientOriginalExtension(); // Validation and the stored file name both go by the detected - // type, so an SVG uploaded under another extension is sanitized too. - if ($extension === 'svg' || $image->guessExtension() === 'svg') { + // type, not the client file name, so the sanitizer does too. + if ($image->guessExtension() === 'svg') { $sanitizer = new Sanitizer(); $sanitizedSvg = $sanitizer->sanitize(file_get_contents($image->getRealPath())); diff --git a/tests/Feature/ItemIconUploadTest.php b/tests/Feature/ItemIconUploadTest.php index d10a561d4..9fcdf30c3 100644 --- a/tests/Feature/ItemIconUploadTest.php +++ b/tests/Feature/ItemIconUploadTest.php @@ -6,6 +6,7 @@ use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Storage; +use PHPUnit\Framework\Attributes\DataProvider; use Tests\TestCase; /** @@ -20,6 +21,24 @@ class ItemIconUploadTest extends TestCase . '' . ''; + /** @var string[] */ + private array $tempFiles = []; + + protected function setUp(): void + { + parent::setUp(); + + Storage::fake('public'); + $this->seed(); + } + + protected function tearDown(): void + { + array_map('unlink', array_filter($this->tempFiles, 'file_exists')); + + parent::tearDown(); + } + private function itemPayload(UploadedFile $file): array { return [ @@ -40,52 +59,37 @@ private function itemPayload(UploadedFile $file): array */ private function upload(string $contents, string $clientName): UploadedFile { - $path = tempnam(sys_get_temp_dir(), 'icon'); + $path = $this->tempFiles[] = tempnam(sys_get_temp_dir(), 'icon'); file_put_contents($path, $contents); return new UploadedFile($path, $clientName, null, null, true); } - private function storedIcon(): string + public static function svgClientNames(): array { - $item = Item::where('title', 'Item A')->firstOrFail(); - Storage::disk('public')->assertExists($item->icon); - - return Storage::disk('public')->get($item->icon); + return [ + 'svg extension' => ['icon.svg'], + // Fails if the sanitizer goes by the client file name. + 'png extension' => ['icon.png'], + ]; } - public function test_svg_icon_upload_is_accepted_and_sanitized(): void + #[DataProvider('svgClientNames')] + public function test_svg_icon_upload_is_accepted_and_sanitized(string $clientName): void { - Storage::fake('public'); - $this->seed(); - - $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, 'icon.svg'))); + $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, $clientName))); $response->assertSessionHasNoErrors(); - $stored = $this->storedIcon(); + $item = Item::where('title', 'Item A')->firstOrFail(); + Storage::disk('public')->assertExists($item->icon); + $stored = Storage::disk('public')->get($item->icon); $this->assertStringContainsString('assertStringNotContainsString('assertStringNotContainsString('onload', $stored); } - public function test_svg_uploaded_under_another_extension_is_still_sanitized(): void - { - Storage::fake('public'); - $this->seed(); - - $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, 'icon.png'))); - - $response->assertSessionHasNoErrors(); - $stored = $this->storedIcon(); - $this->assertStringNotContainsString('assertStringNotContainsString('onload', $stored); - } - public function test_non_image_upload_is_rejected(): void { - Storage::fake('public'); - $this->seed(); - $response = $this->post('/items', $this->itemPayload($this->upload('just some text', 'icon.txt'))); $response->assertSessionHasErrors('file'); From 4b34cd2062c2510f9e5b4d60e880dae7554492ba Mon Sep 17 00:00:00 2001 From: KodeStar Date: Mon, 19 Oct 2026 09:09:59 +0100 Subject: [PATCH 3/3] Reject unparseable SVG icons instead of storing them empty 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. --- app/Http/Controllers/ItemController.php | 44 ++++++++++++++++--------- tests/Feature/ItemIconUploadTest.php | 10 ++++++ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index 209c2bf76..50c7a07ac 100644 --- a/app/Http/Controllers/ItemController.php +++ b/app/Http/Controllers/ItemController.php @@ -231,6 +231,29 @@ public function edit(int $id): View return view('items.edit', $data); } + /** + * Strip scripts and other active content from an SVG icon. + * + * @throws ValidationException + */ + private static function sanitizeSvg(string $svg): string + { + $sanitized = (new Sanitizer())->sanitize($svg); + + // The sanitizer returns false when the SVG is not well-formed XML; + // storing that would leave an empty, extensionless icon. + if ($sanitized === false) { + throw ValidationException::withMessages(['file' => 'SVG could not be parsed and cannot be uploaded.']); + } + + // Verify that the sanitization removed malicious content + if (stripos($sanitized, ' 'SVG contains malicious content and cannot be uploaded.']); + } + + return $sanitized; + } + /** * @param null $id * @throws ValidationException @@ -251,16 +274,11 @@ public static function storelogic(Request $request, $id = null): Item // Validation and the stored file name both go by the detected // type, not the client file name, so the sanitizer does too. if ($image->guessExtension() === 'svg') { - $sanitizer = new Sanitizer(); - $sanitizedSvg = $sanitizer->sanitize(file_get_contents($image->getRealPath())); - - // Verify that the sanitization removed malicious content - if (strpos($sanitizedSvg, '