Skip to content

Commit 4320725

Browse files
colinodellcolinodell
committed
Fix XSS in AttributesExtension
1 parent d4b08b8 commit 4320725

14 files changed

+190
-7
lines changed

‎CHANGELOG.md

+9
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ Updates should follow the [Keep a CHANGELOG](https://keepachangelog.com/) princi
66

77
## [Unreleased][unreleased]
88

9+
This is a **security release** to address a potential cross-site scripting (XSS) vulnerability when using the `AttributesExtension` with untrusted user input.
10+
11+
### Added
12+
- Added `attributes/allow` config option to specify which attributes users are allowed to set on elements (default allows virtually all attributes)
13+
14+
### Changed
15+
- The `AttributesExtension` blocks all attributes starting with `on` unless explicitly allowed via the `attributes/allow` config option
16+
- The `allow_unsafe_links` option is now respected by the `AttributesExtension` when users specify `href` and `src` attributes
17+
918
## [2.6.2] - 2025-04-18
1019

1120
### Fixed

‎docs/2.7/extensions/attributes.md

+19-3
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ redirect_from: /extensions/attributes/
99

1010
The `AttributesExtension` allows HTML attributes to be added from within the document.
1111

12+
**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).
13+
1214
## Attribute Syntax
1315

1416
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;
7577
use League\CommonMark\Extension\CommonMark\CommonMarkCoreExtension;
7678
use League\CommonMark\MarkdownConverter;
7779

78-
// Define your configuration, if needed
79-
$config = [];
80+
// Example custom configuration
81+
$config = [
82+
'attributes' => [
83+
'allow' => ['id', 'class', 'align'],
84+
],
85+
];
8086

8187
// Configure the Environment with all the CommonMark parsers/renderers
8288
$environment = new Environment($config);
@@ -87,5 +93,15 @@ $environment->addExtension(new AttributesExtension());
8793

8894
// Instantiate the converter engine and start converting some Markdown!
8995
$converter = new MarkdownConverter($environment);
90-
echo $converter->convert('# Hello World!');
96+
echo $converter->convert('# Hello World! {.article-title}');
9197
```
98+
99+
## Configuration
100+
101+
As of version 2.7.0, this extension can be configured by providing a `attributes` array with nested configuration options.
102+
103+
### `allow`
104+
105+
An array of allowed attributes. An empty array `[]` (default) allows virtually all attributes.
106+
107+
**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

+15-3
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,26 @@
1919
use League\CommonMark\Extension\Attributes\Event\AttributesListener;
2020
use League\CommonMark\Extension\Attributes\Parser\AttributesBlockStartParser;
2121
use League\CommonMark\Extension\Attributes\Parser\AttributesInlineParser;
22-
use League\CommonMark\Extension\ExtensionInterface;
22+
use League\CommonMark\Extension\ConfigurableExtensionInterface;
23+
use League\Config\ConfigurationBuilderInterface;
24+
use Nette\Schema\Expect;
2325

24-
final class AttributesExtension implements ExtensionInterface
26+
final class AttributesExtension implements ConfigurableExtensionInterface
2527
{
28+
public function configureSchema(ConfigurationBuilderInterface $builder): void
29+
{
30+
$builder->addSchema('attributes', Expect::structure([
31+
'allow' => Expect::arrayOf('string')->default([]),
32+
]));
33+
}
34+
2635
public function register(EnvironmentBuilderInterface $environment): void
2736
{
37+
$allowList = $environment->getConfiguration()->get('attributes.allow');
38+
$allowUnsafeLinks = $environment->getConfiguration()->get('allow_unsafe_links');
39+
2840
$environment->addBlockStartParser(new AttributesBlockStartParser());
2941
$environment->addInlineParser(new AttributesInlineParser());
30-
$environment->addEventListener(DocumentParsedEvent::class, [new AttributesListener(), 'processDocument']);
42+
$environment->addEventListener(DocumentParsedEvent::class, [new AttributesListener($allowList, $allowUnsafeLinks), 'processDocument']);
3143
}
3244
}

‎src/Extension/Attributes/Event/AttributesListener.php

+14-1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,19 @@ final class AttributesListener
2929
private const DIRECTION_PREFIX = 'prefix';
3030
private const DIRECTION_SUFFIX = 'suffix';
3131

32+
/** @var list<string> */
33+
private array $allowList;
34+
private bool $allowUnsafeLinks;
35+
36+
/**
37+
* @param list<string> $allowList
38+
*/
39+
public function __construct(array $allowList = [], bool $allowUnsafeLinks = true)
40+
{
41+
$this->allowList = $allowList;
42+
$this->allowUnsafeLinks = $allowUnsafeLinks;
43+
}
44+
3245
public function processDocument(DocumentParsedEvent $event): void
3346
{
3447
foreach ($event->getDocument()->iterator() as $node) {
@@ -50,7 +63,7 @@ public function processDocument(DocumentParsedEvent $event): void
5063
$attributes = AttributesHelper::mergeAttributes($node->getAttributes(), $target);
5164
}
5265

53-
$target->data->set('attributes', $attributes);
66+
$target->data->set('attributes', AttributesHelper::filterAttributes($attributes, $this->allowList, $this->allowUnsafeLinks));
5467
}
5568

5669
$node->detach();

‎src/Extension/Attributes/Util/AttributesHelper.php

+38
Original file line numberDiff line numberDiff line change
@@ -139,4 +139,42 @@ public static function mergeAttributes($attributes1, $attributes2): array
139139

140140
return $attributes;
141141
}
142+
143+
/**
144+
* @param array<string, mixed> $attributes
145+
* @param list<string> $allowList
146+
*
147+
* @return array<string, mixed>
148+
*/
149+
public static function filterAttributes(array $attributes, array $allowList, bool $allowUnsafeLinks): array
150+
{
151+
$allowList = \array_fill_keys($allowList, true);
152+
153+
foreach ($attributes as $name => $value) {
154+
$attrNameLower = \strtolower($name);
155+
156+
// Remove any unsafe links
157+
if (! $allowUnsafeLinks && ($attrNameLower === 'href' || $attrNameLower === 'src') && \is_string($value) && RegexHelper::isLinkPotentiallyUnsafe($value)) {
158+
unset($attributes[$name]);
159+
continue;
160+
}
161+
162+
// No allowlist?
163+
if ($allowList === []) {
164+
// Just remove JS event handlers
165+
if (\str_starts_with($attrNameLower, 'on')) {
166+
unset($attributes[$name]);
167+
}
168+
169+
continue;
170+
}
171+
172+
// Remove any attributes not in that allowlist (case-sensitive)
173+
if (! isset($allowList[$name])) {
174+
unset($attributes[$name]);
175+
}
176+
}
177+
178+
return $attributes;
179+
}
142180
}

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

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

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

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
attributes:
3+
allow: [id, class, align]
4+
---
5+
6+
Header with attributes {#header1}
7+
---------------------------------
8+
9+
{class="text" hello="world"}
10+
some text
11+
12+
![image](/assets/image.jpg){align=left width=100px}

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

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

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

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

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

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

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

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
allow_unsafe_links: true
3+
---
4+
5+
[click me](javascript:alert(1)){href="javascript:alert(1)"}

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

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<p><a>click me</a></p>

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

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
allow_unsafe_links: false
3+
---
4+
5+
[click me](javascript:alert(1)){href="javascript:alert(1)"}

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

+66
Original file line numberDiff line numberDiff line change
@@ -194,4 +194,70 @@ public static function dataForTestMergeAttributes(): iterable
194194
['id' => 'block', 'class' => 'inline block'],
195195
];
196196
}
197+
198+
/**
199+
* @dataProvider dataForTestFilterAttributes
200+
*
201+
* @param array<string, mixed> $attributes
202+
* @param list<string> $allowList
203+
* @param array<string, mixed> $expected
204+
*/
205+
public function testFilterAttributes(array $attributes, array $allowList, bool $allowUnsafeLinks, array $expected): void
206+
{
207+
$this->assertEquals($expected, AttributesHelper::filterAttributes($attributes, $allowList, $allowUnsafeLinks));
208+
}
209+
210+
/**
211+
* @return iterable<array<mixed>>
212+
*/
213+
public static function dataForTestFilterAttributes(): iterable
214+
{
215+
// No allow list; unsafe links disallowed (default behavior)
216+
yield [
217+
['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
218+
[],
219+
false,
220+
['id' => 'foo', 'class' => 'bar'],
221+
];
222+
223+
// No allow list; unsafe links allowed
224+
yield [
225+
['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
226+
[],
227+
true,
228+
['id' => 'foo', 'class' => 'bar', 'href' => 'javascript:alert("XSS")'],
229+
];
230+
231+
// Allow list; unsafe links disallowed
232+
yield [
233+
['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
234+
['id', 'onclick', 'href'],
235+
false,
236+
['id' => 'foo', 'onclick' => 'alert("XSS")'],
237+
];
238+
239+
// Allow list; unsafe links allowed
240+
yield [
241+
['id' => 'foo', 'class' => 'bar', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
242+
['id', 'onclick', 'href'],
243+
true,
244+
['id' => 'foo', 'onclick' => 'alert("XSS")', 'href' => 'javascript:alert("XSS")'],
245+
];
246+
247+
// Allow list blocks all
248+
yield [
249+
['id' => 'foo', 'class' => '<script>alert("XSS")</script>'],
250+
['style'],
251+
false,
252+
[],
253+
];
254+
255+
// Can't use weird casing to bypass allowlist or 'on*' restriction
256+
yield [
257+
['ID' => 'foo', 'oNcLiCk' => 'alert("XSS")'],
258+
['id', 'class'],
259+
false,
260+
[],
261+
];
262+
}
197263
}

0 commit comments

Comments
 (0)