diff --git a/app/Http/Controllers/ItemController.php b/app/Http/Controllers/ItemController.php index dceb86355..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 @@ -241,24 +264,21 @@ 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') { - $sanitizer = new Sanitizer(); - $sanitizedSvg = $sanitizer->sanitize(file_get_contents($image->getRealPath())); - - // Verify that the sanitization removed malicious content - if (strpos($sanitizedSvg, '' + . ''; + + /** @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 [ + '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 = $this->tempFiles[] = tempnam(sys_get_temp_dir(), 'icon'); + file_put_contents($path, $contents); + + return new UploadedFile($path, $clientName, null, null, true); + } + + public static function svgClientNames(): array + { + return [ + 'svg extension' => ['icon.svg'], + // Fails if the sanitizer goes by the client file name. + 'png extension' => ['icon.png'], + ]; + } + + #[DataProvider('svgClientNames')] + public function test_svg_icon_upload_is_accepted_and_sanitized(string $clientName): void + { + $response = $this->post('/items', $this->itemPayload($this->upload(self::SVG, $clientName))); + + $response->assertSessionHasNoErrors(); + $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_malformed_svg_upload_is_rejected(): void + { + $malformed = ' '; + + $response = $this->post('/items', $this->itemPayload($this->upload($malformed, 'icon.svg'))); + + $response->assertSessionHasErrors('file'); + $this->assertNull(Item::where('title', 'Item A')->first()); + } + + public function test_non_image_upload_is_rejected(): void + { + $response = $this->post('/items', $this->itemPayload($this->upload('just some text', 'icon.txt'))); + + $response->assertSessionHasErrors('file'); + $this->assertNull(Item::where('title', 'Item A')->first()); + } +}