Merge pull request #528 from creative-commoners/pulls/1.11/cve-2022-3…

…8724 Add whitelist for image shortcode attributes.
silverstripe · Nov 20, 2022 · 97afd88 · 97afd88
2 parents 67076d1 + 6e5d354
commit 97afd88
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 3 deletions.
diff --git a/src/Shortcodes/ImageShortcodeProvider.php b/src/Shortcodes/ImageShortcodeProvider.php
@@ -20,6 +20,20 @@
  */
 class ImageShortcodeProvider extends FileShortcodeProvider implements ShortcodeHandler, Flushable
 {
+    /**
+     * A whitelist of attributes which are allowed in the resultant markup.
+     *
+     * @config
+     */
+    private static array $attribute_whitelist = [
+        'alt',
+        'class',
+        'height',
+        'loading',
+        'src',
+        'title',
+        'width',
+    ];
 
     /**
      * Gets the list of shortcodes provided by this handler
@@ -99,9 +113,10 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
             $attrs['alt'] = $record->Title;
         }
 
-        // Clean out any empty attributes (aside from alt)
-        $attrs = array_filter($attrs ?? [], function ($k, $v) {
-            return strlen(trim($v ?? '')) || $k === 'alt';
+        // Clean out any empty attributes (aside from alt) and anything not whitelisted
+        $whitelist = static::config()->get('attribute_whitelist');
+        $attrs = array_filter($attrs ?? [], function ($v, $k) use ($whitelist) {
+            return in_array($k, $whitelist) && (strlen(trim($v ?? '')) || $k === 'alt');
         }, ARRAY_FILTER_USE_BOTH);
 
         $markup = HTML::createTag('img', $attrs);

diff --git a/tests/php/Shortcodes/ImageShortcodeProviderTest.php b/tests/php/Shortcodes/ImageShortcodeProviderTest.php
@@ -261,4 +261,81 @@ public function testRegenerateShortcode()
         $this->assertSame($expected, $html);
         $this->assertTrue($assetStore->isGranted($parsedFileID));
     }
+
+    public function testEmptyAttributesAreRemoved()
+    {
+        $image = $this->objFromFixture(Image::class, 'imageWithTitle');
+        $parser = new ShortcodeParser();
+        $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']);
+
+        // Note that alt is allowed to be empty
+        $this->assertEquals(
+            sprintf(
+                '<img src="%s" alt="">',
+                $image->Link()
+            ),
+            $parser->parse(sprintf('[image id=%d alt="" class=""]', $image->ID))
+        );
+    }
+
+    public function testOnlyWhitelistedAttributesAllowed()
+    {
+        $image = $this->objFromFixture(Image::class, 'imageWithoutTitle');
+        $parser = new ShortcodeParser();
+        $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']);
+        $whitelist = ImageShortcodeProvider::config()->get('attribute_whitelist');
+
+        $attributes = '';
+        foreach ($whitelist as $attrName) {
+            if ($attrName === 'src') {
+                continue;
+            }
+            $attributes .= $attrName . '="' . $attrName . '" ';
+        }
+
+        $this->assertEquals(
+            sprintf(
+                '<img src="%s" %s>',
+                $image->Link(),
+                trim($attributes)
+            ),
+            $parser->parse(sprintf(
+                '[image id="%d" %s style="width:100px" data-some-value="my-data"]',
+                $image->ID,
+                trim($attributes)
+            ))
+        );
+    }
+
+    public function testWhiteIsConfigurable()
+    {
+        $image = $this->objFromFixture(Image::class, 'imageWithoutTitle');
+        $parser = new ShortcodeParser();
+        $parser->register('image', [ImageShortcodeProvider::class, 'handle_shortcode']);
+        $whitelist = ImageShortcodeProvider::config()->get('attribute_whitelist');
+
+        $attributes = '';
+        foreach ($whitelist as $attrName) {
+            if ($attrName === 'src') {
+                continue;
+            }
+            $attributes .= $attrName . '="' . $attrName . '" ';
+        }
+
+        // Allow new whitelisted attribute
+        Config::modify()->merge(ImageShortcodeProvider::class, 'attribute_whitelist', ['data-some-value']);
+
+        $this->assertEquals(
+            sprintf(
+                '<img src="%s" %s>',
+                $image->Link(),
+                trim($attributes) . ' data-some-value="my-data"'
+            ),
+            $parser->parse(sprintf(
+                '[image id="%d" %s style="width:100px" data-some-value="my-data"]',
+                $image->ID,
+                trim($attributes)
+            ))
+        );
+    }
 }