Merge commit from fork

Fix XSS in AttributesExtension

Author: colinodell

CHANGELOG.md

@@ -6,6 +6,15 @@ Updates should follow the [Keep a CHANGELOG](https://keepachangelog.com/) princi
 
 ## [Unreleased][unreleased]
 
+This is a **security release** to address a potential cross-site scripting (XSS) vulnerability when using the `AttributesExtension` with untrusted user input.
+
+### Added
+- Added `attributes/allow` config option to specify which attributes users are allowed to set on elements (default allows virtually all attributes)
+
+### Changed
+- The `AttributesExtension` blocks all attributes starting with `on` unless explicitly allowed via the `attributes/allow` config option
+- The `allow_unsafe_links` option is now respected by the `AttributesExtension` when users specify `href` and `src` attributes
+
 ## [2.6.2] - 2025-04-18
 
 ### Fixed

docs/2.7/extensions/attributes.md

@@ -9,6 +9,8 @@ redirect_from: /extensions/attributes/
 
 The `AttributesExtension` allows HTML attributes to be added from within the document.
 
+**Security warning:** Allowing untrusted users to inject arbitrary HTML attributes could lead to XSS vulnerabilities, styling issues, or other problems. Consider [disabling unsafe links](/2.7/security/#unsafe-links), [configuring allowed attributes](#configuration), and/or [using additional filtering](/2.7/security/#additional-filtering).
+
 ## Attribute Syntax
 
 The basic syntax was inspired by [Kramdown](http://kramdown.gettalong.org/syntax.html#attribute-list-definitions)'s Attribute Lists feature.
@@ -75,8 +77,12 @@ use League\CommonMark\Extension\Attributes\AttributesExtension;
 use League\CommonMark\Extension\CommonMark\CommonMarkCoreExtension;
 use League\CommonMark\MarkdownConverter;
 
-// Define your configuration, if needed
-$config = [];
+// Example custom configuration
+$config = [
+    'attributes' => [
+        'allow' => ['id', 'class', 'align'],
+    ],
+];
 
 // Configure the Environment with all the CommonMark parsers/renderers
 $environment = new Environment($config);
@@ -87,5 +93,15 @@ $environment->addExtension(new AttributesExtension());
 
 // Instantiate the converter engine and start converting some Markdown!
 $converter = new MarkdownConverter($environment);
-echo $converter->convert('# Hello World!');
+echo $converter->convert('# Hello World! {.article-title}');
 ```
+
+## Configuration
+
+As of version 2.7.0, this extension can be configured by providing a `attributes` array with nested configuration options.
+
+### `allow`
+
+An array of allowed attributes. An empty array `[]` (default) allows virtually all attributes.
+
+**Note:** Attributes starting with `on` (e.g. `onclick` or `onerror`) are capable of executing JavaScript code and are therefore **never allowed by default**. You must explicitly add them to the `allow` list if you want to use them.

src/Extension/Attributes/AttributesExtension.php

@@ -19,14 +19,26 @@
 use League\CommonMark\Extension\Attributes\Event\AttributesListener;
 use League\CommonMark\Extension\Attributes\Parser\AttributesBlockStartParser;
 use League\CommonMark\Extension\Attributes\Parser\AttributesInlineParser;
-use League\CommonMark\Extension\ExtensionInterface;
+use League\CommonMark\Extension\ConfigurableExtensionInterface;
+use League\Config\ConfigurationBuilderInterface;
+use Nette\Schema\Expect;
 
-final class AttributesExtension implements ExtensionInterface
+final class AttributesExtension implements ConfigurableExtensionInterface
 {
+    public function configureSchema(ConfigurationBuilderInterface $builder): void
+    {
+        $builder->addSchema('attributes', Expect::structure([
+            'allow' => Expect::arrayOf('string')->default([]),
+        ]));
+    }
+
     public function register(EnvironmentBuilderInterface $environment): void
     {
+        $allowList        = $environment->getConfiguration()->get('attributes.allow');
+        $allowUnsafeLinks = $environment->getConfiguration()->get('allow_unsafe_links');
+
         $environment->addBlockStartParser(new AttributesBlockStartParser());
         $environment->addInlineParser(new AttributesInlineParser());
-        $environment->addEventListener(DocumentParsedEvent::class, [new AttributesListener(), 'processDocument']);
+        $environment->addEventListener(DocumentParsedEvent::class, [new AttributesListener($allowList, $allowUnsafeLinks), 'processDocument']);
     }
 }

src/Extension/Attributes/Event/AttributesListener.php

@@ -29,6 +29,19 @@ final class AttributesListener
     private const DIRECTION_PREFIX = 'prefix';
     private const DIRECTION_SUFFIX = 'suffix';
 
+    /** @var list<string> */
+    private array $allowList;
+    private bool $allowUnsafeLinks;
+
+    /**
+     * @param list<string> $allowList
+     */
+    public function __construct(array $allowList = [], bool $allowUnsafeLinks = true)
+    {
+        $this->allowList        = $allowList;
+        $this->allowUnsafeLinks = $allowUnsafeLinks;
+    }
+
     public function processDocument(DocumentParsedEvent $event): void
     {
         foreach ($event->getDocument()->iterator() as $node) {
@@ -50,7 +63,7 @@ public function processDocument(DocumentParsedEvent $event): void
                     $attributes = AttributesHelper::mergeAttributes($node->getAttributes(), $target);
                 }
 
-                $target->data->set('attributes', $attributes);
+                $target->data->set('attributes', AttributesHelper::filterAttributes($attributes, $this->allowList, $this->allowUnsafeLinks));
             }
 
             $node->detach();

src/Extension/Attributes/Util/AttributesHelper.php

@@ -139,4 +139,42 @@ public static function mergeAttributes($attributes1, $attributes2): array
 
         return $attributes;
     }
+
+    /**
+     * @param array<string, mixed> $attributes
+     * @param list<string>         $allowList
+     *
+     * @return array<string, mixed>
+     */
+    public static function filterAttributes(array $attributes, array $allowList, bool $allowUnsafeLinks): array
+    {
+        $allowList = \array_fill_keys($allowList, true);
+
+        foreach ($attributes as $name => $value) {
+            $attrNameLower = \strtolower($name);
+
+            // Remove any unsafe links
+            if (! $allowUnsafeLinks && ($attrNameLower === 'href' || $attrNameLower === 'src') && \is_string($value) && RegexHelper::isLinkPotentiallyUnsafe($value)) {
+                unset($attributes[$name]);
+                continue;
+            }
+
+            // No allowlist?
+            if ($allowList === []) {
+                // Just remove JS event handlers
+                if (\str_starts_with($attrNameLower, 'on')) {
+                    unset($attributes[$name]);
+                }
+
+                continue;
+            }
+
+            // Remove any attributes not in that allowlist (case-sensitive)
+            if (! isset($allowList[$name])) {
+                unset($attributes[$name]);
+            }
+        }
+
+        return $attributes;
+    }
 }

tests/functional/Extension/Attributes/data/allowlist.html

@@ -0,0 +1,3 @@
+<h2 id="header1">Header with attributes</h2>
+<p class="text">some text</p>
+<p><img align="left" src="/assets/image.jpg" alt="image" /></p>

tests/functional/Extension/Attributes/data/allowlist.md

@@ -0,0 +1,12 @@
+---
+attributes:
+    allow: [id, class, align]
+---
+
+Header with attributes {#header1}
+---------------------------------
+
+{class="text" hello="world"}
+some text
+
+![image](/assets/image.jpg){align=left width=100px}

tests/functional/Extension/Attributes/data/js_event_attributes.html

@@ -0,0 +1 @@
+<p><img class="blocked" src="" alt="this extension blocks js event attributes" /></p>

tests/functional/Extension/Attributes/data/js_event_attributes.md

@@ -0,0 +1 @@
+![this extension blocks js event attributes](){onerror=alert(1) class=blocked}

tests/functional/Extension/Attributes/data/unsafe_links_allowed.html

@@ -0,0 +1 @@
+<p><a href="javascript:alert(1)">click me</a></p>

tests/functional/Extension/Attributes/data/unsafe_links_allowed.md

@@ -0,0 +1,5 @@
+---
+allow_unsafe_links: true
+---
+
+[click me](javascript:alert(1)){href="javascript:alert(1)"}

tests/functional/Extension/Attributes/data/unsafe_links_blocked.html

@@ -0,0 +1 @@
+<p><a>click me</a></p>

tests/functional/Extension/Attributes/data/unsafe_links_blocked.md

@@ -0,0 +1,5 @@
+---
+allow_unsafe_links: false
+---
+
+[click me](javascript:alert(1)){href="javascript:alert(1)"}

tests/unit/Extension/Attributes/Util/AttributesHelperTest.php

@@ -194,4 +194,70 @@ public static function dataForTestMergeAttributes(): iterable
             ['id' => 'block', 'class' => 'inline block'],
         ];
     }
+
+    /**
+     * @dataProvider dataForTestFilterAttributes
+     *
+     * @param array<string, mixed> $attributes
+     * @param list<string>         $allowList
+     * @param array<string, mixed> $expected
+     */
+    public function testFilterAttributes(array $attributes, array $allowList, bool $allowUnsafeLinks, array $expected): void
+    {
+        $this->assertEquals($expected, AttributesHelper::filterAttributes($attributes, $allowList, $allowUnsafeLinks));
+    }
+
+    /**
+     * @return iterable<array<mixed>>
+     */
+    public static function dataForTestFilterAttributes(): iterable
+    {
+        // No allow list; unsafe links disallowed (default behavior)
+        yield [
+            ['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
+            [],
+            false,
+            ['id' => 'foo', 'class' => 'bar'],
+        ];
+
+        // No allow list; unsafe links allowed
+        yield [
+            ['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
+            [],
+            true,
+            ['id' => 'foo', 'class' => 'bar', 'href' => 'javascript:alert("XSS")'],
+        ];
+
+        // Allow list; unsafe links disallowed
+        yield [
+            ['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
+            ['id', 'onclick', 'href'],
+            false,
+            ['id' => 'foo', 'onclick' => 'alert("XSS")'],
+        ];
+
+        // Allow list; unsafe links allowed
+        yield [
+            ['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
+            ['id', 'onclick', 'href'],
+            true,
+            ['id' => 'foo', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
+        ];
+
+        // Allow list blocks all
+        yield [
+            ['id' => 'foo', 'class' => '<script>alert("XSS")</script>'],
+            ['style'],
+            false,
+            [],
+        ];
+
+        // Can't use weird casing to bypass allowlist or 'on*' restriction
+        yield [
+            ['ID' => 'foo', 'oNcLiCk' => 'alert("XSS")'],
+            ['id', 'class'],
+            false,
+            [],
+        ];
+    }
 }