From 892a51e875da16cda0711311fd6ec24dd89acfd2 Mon Sep 17 00:00:00 2001 From: Jason Varga Date: Mon, 5 Oct 2026 13:37:56 -0400 Subject: [PATCH] Tighten SVG sanitization Co-Authored-By: Claude Opus 5.5 --- composer.json | 2 +- src/Support/Svg.php | 2 +- tests/Assets/AssetTest.php | 47 ++++++++++++++++++++++++++++++++++++++ tests/Support/SvgTest.php | 22 ++++++++++++++++-- tests/Tags/SvgTagTest.php | 30 ++++++++++++++++++++---- 5 files changed, 94 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index 31009f2ce77..47085a50198 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,7 @@ "nesbot/carbon": "^3.0", "pragmarx/google2fa": "^8.0 || ^9.0", "rebing/graphql-laravel": "^9.15", - "rhukster/dom-sanitizer": "^1.0.10", + "rhukster/dom-sanitizer": "^1.0.19", "spatie/blink": "^1.3", "spatie/error-solutions": "^1.0 || ^2.0", "statamic/stringy": "^3.1.2", diff --git a/src/Support/Svg.php b/src/Support/Svg.php index 56e1d334ba9..49bdf547ea7 100644 --- a/src/Support/Svg.php +++ b/src/Support/Svg.php @@ -23,7 +23,7 @@ public static function sanitize(string $svg, ?DOMSanitizer $sanitizer = null): s $svg = static::sanitizeStyleTags($svg); return $sanitizer->sanitize($svg, [ - 'remove-xml-tags' => ! Str::startsWith($svg, ' true, ]); } diff --git a/tests/Assets/AssetTest.php b/tests/Assets/AssetTest.php index 9e2733134c1..c5fde7bc263 100644 --- a/tests/Assets/AssetTest.php +++ b/tests/Assets/AssetTest.php @@ -2176,6 +2176,53 @@ public static function unnormalizedSvgExtensionProvider() ]; } + #[Test] + #[DataProvider('xmlStylesheetSvgProvider')] + public function it_removes_xml_stylesheet_instructions_from_svgs_on_upload($svg) + { + Event::fake(); + + $asset = (new Asset)->container($this->container)->path('path/to/asset.svg')->syncOriginal(); + + Facades\AssetContainer::shouldReceive('findByHandle')->with('test_container')->andReturn($this->container); + + $asset->upload(UploadedFile::fake()->createWithContent('asset.svg', $svg)); + + $this->assertStringNotContainsString('xml-stylesheet', $asset->contents()); + } + + #[Test] + #[DataProvider('xmlStylesheetSvgProvider')] + public function it_removes_xml_stylesheet_instructions_from_svgs_on_reupload($svg) + { + Event::fake(); + + $asset = (new Asset)->container($this->container)->path('path/to/asset.svg')->syncOriginal(); + + Facades\AssetContainer::shouldReceive('findByHandle')->with('test_container')->andReturn($this->container); + + $asset->upload(UploadedFile::fake()->createWithContent('asset.svg', '')); + + Storage::fake('local')->put('path/to/replacement.svg', $svg); + + $asset->reupload(new ReplacementFile('path/to/replacement.svg')); + + $this->assertStringNotContainsString('xml-stylesheet', $asset->contents()); + } + + public static function xmlStylesheetSvgProvider() + { + // A processing instruction telling the browser to apply an inline xslt + // stylesheet, which would replace the image with arbitrary html. + $instruction = ''; + + return [ + 'on its own' => [$instruction.''], + 'after a declaration' => [''.$instruction.''], + 'on separate lines' => [''."\n".$instruction."\n".''], + ]; + } + public static function nonGlideableFileExtensionsProvider() { return [ diff --git a/tests/Support/SvgTest.php b/tests/Support/SvgTest.php index ab379a29795..bcec9573c25 100644 --- a/tests/Support/SvgTest.php +++ b/tests/Support/SvgTest.php @@ -127,13 +127,31 @@ public function it_passes_through_svg_without_style_tags() } #[Test] - public function it_preserves_xml_declaration() + public function it_removes_xml_declaration() { $svg = ''; $result = Svg::sanitize($svg); - $this->assertStringStartsWith('assertStringStartsWith('assertStringNotContainsString('xml-stylesheet', Svg::sanitize($svg)); + } + + public static function xmlStylesheetProvider() + { + $pi = ''; + + return [ + 'on its own' => [$pi.''], + 'after a declaration' => [''.$pi.''], + 'on separate lines' => [''."\n".$pi."\n".''], + ]; } #[Test] diff --git a/tests/Tags/SvgTagTest.php b/tests/Tags/SvgTagTest.php index 0033285061e..d91e503a2f8 100644 --- a/tests/Tags/SvgTagTest.php +++ b/tests/Tags/SvgTagTest.php @@ -3,6 +3,7 @@ namespace Tests\Tags; use Illuminate\Support\Facades\File; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use Statamic\Facades\Parse; use Statamic\Tags\Svg; @@ -72,15 +73,34 @@ public function sanitizing_doesnt_add_xml_tag() } #[Test] - public function sanitizing_doesnt_remove_an_xml_tag() + public function sanitizing_removes_an_xml_tag() { - // We want to make sure that we haven't configured it to remove it if we wanted it there to begin with. + // The declaration is meaningless once the svg is inlined into an html document, + // and keeping it meant trusting an attacker controlled "'; + File::put(resource_path('xmltag.svg'), ''); - File::put(resource_path('xmltag.svg'), $svg); + $this->assertEquals('', $this->tag('{{ svg src="xmltag" }}')); + } + + #[Test] + #[DataProvider('xmlStylesheetProvider')] + public function sanitizing_removes_xml_stylesheet_processing_instructions($svg) + { + File::put(resource_path('stylesheet.svg'), $svg); + + $this->assertStringNotContainsString('xml-stylesheet', $this->tag('{{ svg src="stylesheet" }}')); + } + + public static function xmlStylesheetProvider() + { + $pi = ''; - $this->assertEquals($svg, $this->tag('{{ svg src="xmltag" }}')); + return [ + 'on its own' => [$pi.''], + 'after a declaration' => [''.$pi.''], + 'on separate lines' => [''."\n".$pi."\n".''], + ]; } #[Test]