From f6d85e3dfef57f54931a3f3b427f8b1d93deef4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Wed, 31 May 2023 18:56:33 +0200 Subject: [PATCH] Fix sanitization issues when rendering Blade, add regression tests --- README.md | 3 +- .../components/extensions/twitter.blade.php | 8 +- resources/views/components/meta.blade.php | 2 +- src/SEOManager.php | 15 +++- tests/Pest/ExtensionTest.php | 2 +- tests/Pest/SanitizationTest.php | 84 +++++++++++++++++++ 6 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 tests/Pest/SanitizationTest.php diff --git a/README.md b/README.md index 2816341..71a7b1a 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,7 @@ url(string $url) title(string $title) description(string $description) image(string $url) +type(string $type) locale(string $locale) twitterCreator(string $username) @@ -266,7 +267,7 @@ The `previewify()` method also returns a signed URL to the image, which lets you > **Note** > The `previewify:` prefix will be automatically prepended to all provided data keys. - + ## Examples ### Service Provider diff --git a/resources/views/components/extensions/twitter.blade.php b/resources/views/components/extensions/twitter.blade.php index 1728818..661ddb4 100644 --- a/resources/views/components/extensions/twitter.blade.php +++ b/resources/views/components/extensions/twitter.blade.php @@ -1,6 +1,6 @@ -@if(seo('twitter.creator')) @endif -@if(seo('twitter.site')) @endif -@if(seo('twitter.title')) @endif -@if(seo('twitter.description')) @endif +@if(seo('twitter.creator')) @endif +@if(seo('twitter.site')) @endif +@if(seo('twitter.title')) @endif +@if(seo('twitter.description')) @endif @if(seo('twitter.image')) @endif diff --git a/resources/views/components/meta.blade.php b/resources/views/components/meta.blade.php index 5a6b493..ed5d44f 100644 --- a/resources/views/components/meta.blade.php +++ b/resources/views/components/meta.blade.php @@ -22,7 +22,7 @@ @endif -@if(seo('site')) @endif +@if(seo('site')) @endif @if(seo('locale')) @endif diff --git a/src/SEOManager.php b/src/SEOManager.php index b8d1422..9e5b9ad 100644 --- a/src/SEOManager.php +++ b/src/SEOManager.php @@ -262,6 +262,8 @@ class SEOManager /** Add a meta tag. */ public function tag(string $property, string $content): static { + $content = e($content); + $this->rawTag("meta.{$property}", ""); return $this; @@ -326,19 +328,26 @@ class SEOManager return $this->get($key); } - /** Render blade directive. */ + /** + * Render blade directive. + * + * This is the only method whose output (returned values) is wrapped in e() + * as these values are used in the meta.blade.php file via @seo calls. + */ public function render(...$args): array|string|null { // Flipp and Previewify support more arguments if (in_array($args[0], ['flipp', 'previewify'], true)) { $method = array_shift($args); + // The `flipp` and `previewify` methods return image URLs + // so we don't sanitize the returned value with e() here return $this->{$method}(...$args); } // Two arguments indicate that we're setting a value, e.g. `@seo('title', 'foo') if (count($args) === 2) { - return $this->set($args[0], $args[1]); + return e($this->set($args[0], $args[1])); } // An array means we don't return anything, e.g. `@seo(['title' => 'foo']) @@ -355,7 +364,7 @@ class SEOManager } // A single value means we fetch a value, e.g. `@seo('title') - return $this->get($args[0]); + return e($this->get($args[0])); } /** Handle magic get. */ diff --git a/tests/Pest/ExtensionTest.php b/tests/Pest/ExtensionTest.php index b664ba7..caefdba 100644 --- a/tests/Pest/ExtensionTest.php +++ b/tests/Pest/ExtensionTest.php @@ -56,7 +56,7 @@ test('twitter falls back to the default values', function () { expect(seo('twitter.description'))->toBe('bar'); expect(seo('description'))->toBe('baz'); - expect(meta())->toContain(''); + expect(meta())->toContain(''); }); test('extensions are automatically enabled when values for them are set', function () { diff --git a/tests/Pest/SanitizationTest.php b/tests/Pest/SanitizationTest.php new file mode 100644 index 0000000..b1daa82 --- /dev/null +++ b/tests/Pest/SanitizationTest.php @@ -0,0 +1,84 @@ + " . \' .'; + + seo()->{$method}($unsanitizedContent); + + $meta = meta(); + + $sanitizedContent = e($unsanitizedContent); + + // These assertions are equivalent, but included for clarity + expect($meta)->not()->toContain('content="Testing string " with several \' XSS characters " . \' ."'); + expect($meta)->not()->toContain("content=\"{$unsanitizedContent}\""); + + expect($meta)->toContain(""); + expect($meta)->toContain(""); +})->with([ + ['site', 'og:site_name'], + ['url', 'og:url'], + ['image', 'og:image'], + ['type', 'og:type'], + ['locale', 'og:locale'], +]); + +// The Twitter integration is tested separately as it uses `meta name=""` instead of `meta property=""` +test('the twitter extension properly sanitizes input', function (string $method, $property) { + $unsanitizedContent = 'Testing string " with several \' XSS characters " . \' .'; + + seo()->{$method}($unsanitizedContent); + + $meta = meta(); + + $sanitizedContent = e($unsanitizedContent); + + // These assertions are equivalent, but included for clarity + expect($meta)->not()->toContain('content="Testing string " with several \' XSS characters " . \' ."'); + expect($meta)->not()->toContain("content=\"{$unsanitizedContent}\""); + + expect($meta)->toContain(""); + expect($meta)->toContain(""); +})->with([ + ['twitterCreator', 'twitter:creator'], + ['twitterSite', 'twitter:site'], + ['twitterTitle', 'twitter:title'], + ['twitterDescription', 'twitter:description'], + ['twitterImage', 'twitter:image'], +]); + +// This method is tested separately as it adds an extra () tag +test('the title method properly sanitizes both tags', function () { + $unsanitizedContent = 'Testing string " with several \' XSS characters " . \' .'; + + seo()->title($unsanitizedContent); + + $meta = meta(); + + $sanitizedContent = e($unsanitizedContent); + + // These assertions are equivalent, but included for clarity + expect($meta)->not()->toContain('meta property="og:title" content="Testing string " with several \' XSS characters " . \' ."'); + expect($meta)->not()->toContain('Testing string " with several \' XSS characters " . \' ."'); + expect($meta)->not()->toContain("meta property=\"og:title\" content=\"{$unsanitizedContent}\""); + expect($meta)->not()->toContain("{$unsanitizedContent}"); + + expect($meta)->toContain("{$sanitizedContent}"); + expect($meta)->toContain("Testing string " with several ' XSS characters </title> " . ' ."); + expect($meta)->toContain(""); + expect($meta)->toContain(""); +}); + +test('seo blade directive calls are sanitized', function () { + seo(['image' => $string = 'Testing string " with several \' XSS characters " . \' .']); + + $escaped = e($string); + + // Using @seo() to get a value + expect(blade('')) + ->toBe("") + ->not()->toBe(' " . \' ."'); + + // Using @seo() to set a value + expect(blade("@seo('description', 'abc \' def &')"))->toBe('abc ' def &'); +});