Fix sanitization issues when rendering Blade, add regression tests

Author: stancl

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

resources/views/components/extensions/twitter.blade.php

@@ -1,6 +1,6 @@
 <meta name="twitter:card" content="summary_large_image">
-@if(seo('twitter.creator')) <meta name="twitter:creator" content="@seo('twitter.creator')"> @endif
-@if(seo('twitter.site')) <meta name="twitter:site" content="@seo('twitter.site')"> @endif
-@if(seo('twitter.title')) <meta name="twitter:title" content="@seo('twitter.title')"> @endif
-@if(seo('twitter.description')) <meta name="twitter:description" content="@seo('twitter.description')"> @endif
+@if(seo('twitter.creator')) <meta name="twitter:creator" content="@seo('twitter.creator')" /> @endif
+@if(seo('twitter.site')) <meta name="twitter:site" content="@seo('twitter.site')" /> @endif
+@if(seo('twitter.title')) <meta name="twitter:title" content="@seo('twitter.title')" /> @endif
+@if(seo('twitter.description')) <meta name="twitter:description" content="@seo('twitter.description')" /> @endif
 @if(seo('twitter.image')) <meta name="twitter:image" content="@seo('twitter.image')" /> @endif

resources/views/components/meta.blade.php

@@ -22,7 +22,7 @@
     <meta property="og:type" content="website" />
 @endif
 
-@if(seo('site')) <meta property="og:site_name" content="@seo('site')"> @endif
+@if(seo('site')) <meta property="og:site_name" content="@seo('site')" /> @endif
 
 @if(seo('locale')) <meta property="og:locale" content="@seo('locale')" /> @endif
 

src/SEOManager.php

@@ -262,6 +262,8 @@ public function rawTag(string $key, string $tag = null): static
     /** Add a meta tag. */
     public function tag(string $property, string $content): static
     {
+        $content = e($content);
+
         $this->rawTag("meta.{$property}", "<meta property=\"{$property}\" content=\"{$content}\" />");
 
         return $this;
@@ -326,19 +328,26 @@ public function __call(string $name, array $arguments): string|array|null|static
         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 @@ public function render(...$args): array|string|null
         }
 
         // 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. */

tests/Pest/ExtensionTest.php

@@ -56,7 +56,7 @@
     expect(seo('twitter.description'))->toBe('bar');
     expect(seo('description'))->toBe('baz');
 
-    expect(meta())->toContain('<meta name="twitter:title" content="foo">');
+    expect(meta())->toContain('<meta name="twitter:title" content="foo" />');
 });
 
 test('extensions are automatically enabled when values for them are set', function () {

tests/Pest/SanitizationTest.php

@@ -0,0 +1,84 @@
+<?php
+
+test('opengraph methods properly sanitize input', function (string $method, string $property) {
+    $unsanitizedContent = 'Testing string " with several \' XSS characters </title> " . \' .';
+
+    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 </title> " . \' ."');
+    expect($meta)->not()->toContain("content=\"{$unsanitizedContent}\"");
+
+    expect($meta)->toContain("<meta property=\"$property\" content=\"{$sanitizedContent}\" />");
+    expect($meta)->toContain("<meta property=\"$property\" content=\"Testing string &quot; with several &#039; XSS characters &lt;/title&gt; &quot; . &#039; .\" />");
+})->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 </title> " . \' .';
+
+    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 </title> " . \' ."');
+    expect($meta)->not()->toContain("content=\"{$unsanitizedContent}\"");
+
+    expect($meta)->toContain("<meta name=\"$property\" content=\"{$sanitizedContent}\" />");
+    expect($meta)->toContain("<meta name=\"$property\" content=\"Testing string &quot; with several &#039; XSS characters &lt;/title&gt; &quot; . &#039; .\" />");
+})->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 (<title>) tag
+test('the title method properly sanitizes both tags', function () {
+    $unsanitizedContent = 'Testing string " with several \' XSS characters </title> " . \' .';
+
+    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 </title> " . \' ."');
+    expect($meta)->not()->toContain('<title>Testing string " with several \' XSS characters </title> " . \' ."</title>');
+    expect($meta)->not()->toContain("meta property=\"og:title\" content=\"{$unsanitizedContent}\"");
+    expect($meta)->not()->toContain("<title>{$unsanitizedContent}</title>");
+
+    expect($meta)->toContain("<title>{$sanitizedContent}</title>");
+    expect($meta)->toContain("<title>Testing string &quot; with several &#039; XSS characters &lt;/title&gt; &quot; . &#039; .</title>");
+    expect($meta)->toContain("<meta property=\"og:title\" content=\"{$sanitizedContent}\" />");
+    expect($meta)->toContain("<meta property=\"og:title\" content=\"Testing string &quot; with several &#039; XSS characters &lt;/title&gt; &quot; . &#039; .\" />");
+});
+
+test('seo blade directive calls are sanitized', function () {
+    seo(['image' => $string = 'Testing string " with several \' XSS characters </title> " . \' .']);
+
+    $escaped = e($string);
+
+    // Using @seo() to get a value
+    expect(blade('<img src="@seo(\'image\')">'))
+        ->toBe("<img src=\"{$escaped}\">")
+        ->not()->toBe('<img src="Testing string " with several \' XSS characters </title> " . \' ."');
+
+    // Using @seo() to set a value
+    expect(blade("@seo('description', 'abc \' def &')"))->toBe('abc &#039; def &amp;');
+});