From b3ad4b73eda17b4e428d87639d16ee22544b6183 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Thu, 22 Oct 2026 12:30:52 -0600 Subject: [PATCH 1/2] Remove unsafe attributes individually in Html::clean() The attribute strip matched from the start of a tag to the first ">", so a tag whose attribute value contained one was only partly processed, and everything from the matched attribute to the end of the tag was discarded along with it. Walk a tag's attributes as complete units instead, treating a quote as opening a value only where a value may legally begin, and remove just the attribute that is not safe to keep. The style strip is folded into the same pass, since it was a second copy of the same pattern. --- src/Html/HtmlBuilder.php | 53 ++++++++++++++++++++-- tests/Html/HtmlBuilderTest.php | 83 ++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 5 deletions(-) diff --git a/src/Html/HtmlBuilder.php b/src/Html/HtmlBuilder.php index 662adf8e..73191d2b 100644 --- a/src/Html/HtmlBuilder.php +++ b/src/Html/HtmlBuilder.php @@ -482,6 +482,26 @@ protected static function getReadableLength($str) return strlen(preg_replace('/\s+/', ' ', $str)); } + /** + * Matches a single HTML attribute within the attribute section of a tag. + * + * Captures the separator before the attribute name (1), the name (2) and the value with its + * leading equals sign, if any (3). A value is only treated as quoted where a value may + * legally begin, so a quote mark anywhere else is an ordinary character and a quoted value + * is consumed whole - including any ">" it contains. + * + * Applied repeatedly from the start of the attribute section, this walks the attributes of a + * tag in order: the possessive quantifiers mean each match consumes one complete attribute + * and the scan cannot drift into the middle of a value. + */ + protected const ATTRIBUTE_PATTERN = '([\x00-\x20\/]*+)([^\x00-\x20=\/>]++)((?:[\x00-\x20]*+=[\x00-\x20]*+(?:"[^"]*+"|\'[^\']*+\'|[^\x00-\x20>]*+))?)'; + + /** + * Matches the name of an attribute that is never safe to keep: an event handler, a namespace + * declaration, or an inline style. + */ + protected const UNSAFE_ATTRIBUTE_NAME_PATTERN = '#^(?:on\w*|xmlns(?::[\w\-]+)?|style)$#i'; + /** * Cleans HTML to prevent most XSS attacks. * @param string $html HTML @@ -498,8 +518,14 @@ public static function clean($html) $html = preg_replace('#(&\#x*)([0-9A-F]+);*#iu', "$1$2;", $html); $html = html_entity_decode($html, ENT_COMPAT|ENT_HTML5, 'UTF-8'); - // Remove any attribute starting with "on" or xmlns - $html = preg_replace('#(<[^>]+[\x00-\x20\"\'\/])(on|xmlns)[^>]*>#iUu', "$1>", $html); + // Remove event handler, xmlns and style attributes from every tag. Inline styles are + // included because of expression() support in legacy versions of Internet Explorer: + // + $html = preg_replace_callback( + '#(<[a-z][^\x00-\x20\/>]*+)((?:' . self::ATTRIBUTE_PATTERN . ')*+)#iu', + fn (array $tag): string => $tag[1] . static::stripUnsafeAttributes($tag[2]), + $html + ); // Remove javascript: and vbscript: protocols $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*j[\x00-\x20]*a[\x00-\x20]*v[\x00-\x20]*a[\x00-\x20]*s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:#iUu', '$1=$2nojavascript...', $html); @@ -507,9 +533,6 @@ public static function clean($html) $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*-moz-binding[\x00-\x20]*:#Uu', '$1=$2nomozbinding...', $html); $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*data[\x00-\x20]*:#Uu', '$1=$2nodata...', $html); - // Only works in IE: - $html = preg_replace('#(<[^>]+[\x00-\x20\"\'\/])style[^>]*>#iUu', "$1>", $html); - // Remove namespaced elements (we do not need them) $html = preg_replace('#]*>#i', "", $html); @@ -520,4 +543,24 @@ public static function clean($html) return $html; } + + /** + * Removes the unsafe attributes from the attribute section of a single HTML tag. + * + * The separator before each removed attribute is kept, so that the attributes either side of + * it do not run together. + * + * @param string $attributes The attribute section of a tag, i.e. everything after its name. + * @return string + */ + protected static function stripUnsafeAttributes($attributes) + { + return preg_replace_callback( + '#' . self::ATTRIBUTE_PATTERN . '#iu', + fn (array $attribute): string => preg_match(self::UNSAFE_ATTRIBUTE_NAME_PATTERN, $attribute[2]) + ? $attribute[1] + : $attribute[0], + $attributes + ); + } } diff --git a/tests/Html/HtmlBuilderTest.php b/tests/Html/HtmlBuilderTest.php index 1c4ffb54..a521538c 100644 --- a/tests/Html/HtmlBuilderTest.php +++ b/tests/Html/HtmlBuilderTest.php @@ -80,4 +80,87 @@ public function testClean() $result = with(new HtmlBuilder)->clean('Test'); $this->assertEquals('Test', $result); } + + public function unsafeAttributeProvider() + { + return [ + 'unquoted value' => ['', 'onerror'], + 'double quoted value' => ['', 'onerror'], + 'greater than in an earlier value' => ['>', 'onerror'], + 'greater than, unquoted handler' => ['', 'onerror'], + 'greater than, single quotes' => [">", 'onerror'], + 'greater than, no separator' => ['', 'onerror'], + 'no separator' => ['', 'onerror'], + 'slash separator' => ['', 'onerror'], + 'quote inside an unquoted value' => ['a"b', 'onerror'], + 'unterminated tag' => [' ['> ['', 'onerror'], + 'newline separator' => ["\"\" src=x\nonerror=alert(1)>", 'onerror'], + 'spaces around the equals sign' => ['>', 'onerror'], + 'greater than in a later value' => ['', 'onerror'], + 'two handlers' => ['>', 'onerror'], + 'handler before other attributes' => ['>', 'onerror'], + 'nested quotes' => ['\'"\' src=x onerror=alert(1)>', 'onerror'], + 'svg element' => ['', 'onload'], + 'details element' => ['
', 'ontoggle'], + 'xmlns declaration' => ['>', 'xmlns'], + 'namespaced xmlns declaration' => ['>', 'xmlns'], + 'inline style' => ['', 'style'], + ]; + } + + /** + * @dataProvider unsafeAttributeProvider + */ + public function testCleanRemovesUnsafeAttributes($html, $attribute) + { + $result = with(new HtmlBuilder)->clean($html); + + $this->assertStringNotContainsStringIgnoringCase($attribute, $result); + } + + public function safeMarkupProvider() + { + return [ + 'heading' => ["# Hello\n"], + 'emphasis' => ["**bold** and _italic_\n"], + 'link with an ampersand' => ["[x](https://example.com/a?b=1&c=2)\n"], + 'table' => ["| a | b |\n|---|---|\n| 1 | 2 |\n"], + 'list' => ["- one\n- two\n"], + 'block quote' => ["> quoted text\n"], + 'code fence' => ["```php\n ['use `onerror=alert(1)` carefully'], + 'prose mentioning an attribute' => ['Set onboarding=true in the config.'], + 'prose mentioning a handler' => ['The onclick= attribute is removed.'], + 'inline html' => ['some bold and italic text'], + 'greater than in an attribute' => ['a > b'], + 'greater than in a title' => ['x'], + 'path beginning with on' => ['x'], + 'attribute text inside a value' => ['x onerror=alert(1)'], + 'data attributes' => ['
x
'], + 'framework bound attributes' => [''], + 'several attributes' => ['a'], + 'self closing tags' => ['
'], + 'html entities' => ['a & b < c'], + ]; + } + + /** + * @dataProvider safeMarkupProvider + */ + public function testCleanPreservesSafeMarkup($html) + { + $this->assertEquals($html, with(new HtmlBuilder)->clean($html)); + } + + public function testCleanKeepsTheAttributesSurroundingARemovedOne() + { + $result = with(new HtmlBuilder)->clean('>'); + + $this->assertStringNotContainsString('onerror', $result); + $this->assertStringContainsString('alt=">"', $result); + $this->assertStringContainsString('src="y.png"', $result); + $this->assertStringContainsString('title="t"', $result); + } } From 3eb82c3fcf7c5320c065009361cf587e534f56e3 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Thu, 22 Oct 2026 12:39:14 -0600 Subject: [PATCH 2/2] Keep scanning attributes past a stray equals sign An equals sign could neither separate two attributes nor begin a name, so a tag carrying one where a name was expected ended the attribute scan early and left the attributes after it unexamined. Treat it as a separator, which makes the scan total: from anywhere in a tag it either consumes an attribute or reaches the end of the tag. Also trims the comments on the two patterns. --- src/Html/HtmlBuilder.php | 32 +++++++++----------------------- tests/Html/HtmlBuilderTest.php | 4 ++++ 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/Html/HtmlBuilder.php b/src/Html/HtmlBuilder.php index 73191d2b..36bee077 100644 --- a/src/Html/HtmlBuilder.php +++ b/src/Html/HtmlBuilder.php @@ -483,22 +483,14 @@ protected static function getReadableLength($str) } /** - * Matches a single HTML attribute within the attribute section of a tag. - * - * Captures the separator before the attribute name (1), the name (2) and the value with its - * leading equals sign, if any (3). A value is only treated as quoted where a value may - * legally begin, so a quote mark anywhere else is an ordinary character and a quoted value - * is consumed whole - including any ">" it contains. - * - * Applied repeatedly from the start of the attribute section, this walks the attributes of a - * tag in order: the possessive quantifiers mean each match consumes one complete attribute - * and the scan cannot drift into the middle of a value. + * Matches one HTML attribute: the separators before its name (1), the name (2), and the value + * with its equals sign, if any (3). A quoted value is consumed whole, including any ">" inside + * it. The separators include "=" so that a stray one cannot strand the rest of the scan. */ - protected const ATTRIBUTE_PATTERN = '([\x00-\x20\/]*+)([^\x00-\x20=\/>]++)((?:[\x00-\x20]*+=[\x00-\x20]*+(?:"[^"]*+"|\'[^\']*+\'|[^\x00-\x20>]*+))?)'; + protected const ATTRIBUTE_PATTERN = '([\x00-\x20\/=]*+)([^\x00-\x20=\/>]++)((?:[\x00-\x20]*+=[\x00-\x20]*+(?:"[^"]*+"|\'[^\']*+\'|[^\x00-\x20>]*+))?)'; /** - * Matches the name of an attribute that is never safe to keep: an event handler, a namespace - * declaration, or an inline style. + * Attribute names that are never safe to keep. */ protected const UNSAFE_ATTRIBUTE_NAME_PATTERN = '#^(?:on\w*|xmlns(?::[\w\-]+)?|style)$#i'; @@ -518,9 +510,8 @@ public static function clean($html) $html = preg_replace('#(&\#x*)([0-9A-F]+);*#iu', "$1$2;", $html); $html = html_entity_decode($html, ENT_COMPAT|ENT_HTML5, 'UTF-8'); - // Remove event handler, xmlns and style attributes from every tag. Inline styles are - // included because of expression() support in legacy versions of Internet Explorer: - // + // Remove event handler, xmlns and style attributes from every tag. Styles are + // included for IE's expression() support. $html = preg_replace_callback( '#(<[a-z][^\x00-\x20\/>]*+)((?:' . self::ATTRIBUTE_PATTERN . ')*+)#iu', fn (array $tag): string => $tag[1] . static::stripUnsafeAttributes($tag[2]), @@ -545,13 +536,8 @@ public static function clean($html) } /** - * Removes the unsafe attributes from the attribute section of a single HTML tag. - * - * The separator before each removed attribute is kept, so that the attributes either side of - * it do not run together. - * - * @param string $attributes The attribute section of a tag, i.e. everything after its name. - * @return string + * Removes unsafe attributes from a tag's attribute section, keeping each separator so the + * attributes either side of a removed one do not run together. */ protected static function stripUnsafeAttributes($attributes) { diff --git a/tests/Html/HtmlBuilderTest.php b/tests/Html/HtmlBuilderTest.php index a521538c..5ea4b572 100644 --- a/tests/Html/HtmlBuilderTest.php +++ b/tests/Html/HtmlBuilderTest.php @@ -107,6 +107,10 @@ public function unsafeAttributeProvider() 'xmlns declaration' => ['>', 'xmlns'], 'namespaced xmlns declaration' => ['>', 'xmlns'], 'inline style' => ['', 'style'], + 'equals sign before a name' => ['', 'onerror'], + 'bare equals sign' => ['', 'onerror'], + 'slash then equals sign' => ['', 'onerror'], + 'repeated equals signs' => ['', 'onerror'], ]; }