Skip to content

Improve attribute removal in Html::clean() #264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LukeTowers merged 2 commits into from
Oct 22, 2026
Merged

Improve attribute removal in Html::clean() #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3ad4b7
Remove unsafe attributes individually in Html::clean()
LukeTowers Oct 22, 2026
3eb82c3
Keep scanning attributes past a stray equals sign
LukeTowers Oct 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions src/Html/HtmlBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,18 @@ protected static function getReadableLength($str)
return strlen(preg_replace('/\s+/', ' ', $str));
}

/**
* 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>]*+))?)';

/**
* Attribute names that are never safe to keep.
*/
protected const UNSAFE_ATTRIBUTE_NAME_PATTERN = '#^(?:on\w*|xmlns(?::[\w\-]+)?|style)$#i';

/**
* Cleans HTML to prevent most XSS attacks.
* @param string $html HTML
Expand All @@ -498,18 +510,20 @@ 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. 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]),
$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);
$html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*v[\x00-\x20]*b[\x00-\x20]*s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:#iUu', '$1=$2novbscript...', $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: <span style="width: expression(alert('Ping!'));"></span>
$html = preg_replace('#(<[^>]+[\x00-\x20\"\'\/])style[^>]*>#iUu', "$1>", $html);

// Remove namespaced elements (we do not need them)
$html = preg_replace('#</*\w+:\w[^>]*>#i', "", $html);

Expand All @@ -520,4 +534,19 @@ public static function clean($html)

return $html;
}

/**
* 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)
{
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
);
}
}
87 changes: 87 additions & 0 deletions tests/Html/HtmlBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,91 @@ public function testClean()
$result = with(new HtmlBuilder)->clean('<a href=" &#14;; javascript: alert(\'Ping!\');">Test</a>');
$this->assertEquals('<a href="nojavascript... alert(\'Ping!\');">Test</a>', $result);
}

public function unsafeAttributeProvider()
{
return [
'unquoted value' => ['<img src=x onerror=alert(1)>', 'onerror'],
'double quoted value' => ['<img src="x" onerror="alert(1)">', 'onerror'],
'greater than in an earlier value' => ['<img alt=">" src="x" onerror="alert(1)">', 'onerror'],
'greater than, unquoted handler' => ['<img src=">" onerror=alert(1)>', 'onerror'],
'greater than, single quotes' => ["<img alt='>' src='x' onerror='alert(1)'>", 'onerror'],
'greater than, no separator' => ['<img src=">"onerror=alert(1)>', 'onerror'],
'no separator' => ['<img src="x"onerror=alert(1)>', 'onerror'],
'slash separator' => ['<img src="x"/onerror=alert(1)>', 'onerror'],
'quote inside an unquoted value' => ['<img alt=a"b onerror=alert(1)>', 'onerror'],
'unterminated tag' => ['<img src=x onerror=alert(1)//', 'onerror'],
'unterminated tag, greater than' => ['<img alt=">" src=x onerror=alert(1)', 'onerror'],
'upper case' => ['<IMG SRC=">" ONERROR=ALERT(1)>', 'onerror'],
'newline separator' => ["<img alt=\">\" src=x\nonerror=alert(1)>", 'onerror'],
'spaces around the equals sign' => ['<img alt=">" src=x onerror = alert(1)>', 'onerror'],
'greater than in a later value' => ['<img src=x title=">" onerror=alert(1)>', 'onerror'],
'two handlers' => ['<img alt=">" src=x onload=alert(1) onerror=alert(2)>', 'onerror'],
'handler before other attributes' => ['<img alt=">" onerror=alert(1) src=x>', 'onerror'],
'nested quotes' => ['<img alt=\'">\' src=x onerror=alert(1)>', 'onerror'],
'svg element' => ['<svg id=">" onload=alert(1)></svg>', 'onload'],
'details element' => ['<details alt=">" ontoggle=alert(1) open></details>', 'ontoggle'],
'xmlns declaration' => ['<img alt=">" xmlns="x" src=x>', 'xmlns'],
'namespaced xmlns declaration' => ['<img alt=">" xmlns:xlink="x" src=x>', 'xmlns'],
'inline style' => ['<span alt=">" style="width: expression(alert(1))"></span>', 'style'],
'equals sign before a name' => ['<img =foo onerror=alert(1)>', 'onerror'],
'bare equals sign' => ['<img = onerror=alert(1)>', 'onerror'],
'slash then equals sign' => ['<img /=foo onerror=alert(1)>', 'onerror'],
'repeated equals signs' => ['<img ==foo onerror=alert(1)>', 'onerror'],
];
}

/**
* @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<?php echo 'hi';\n```\n"],
'inline code' => ['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 <b>bold</b> and <em>italic</em> text'],
'greater than in an attribute' => ['<img alt="a > b" src="x.png">'],
'greater than in a title' => ['<a href="/x" title="a > b">x</a>'],
'path beginning with on' => ['<a href="https://example.com/one=1">x</a>'],
'attribute text inside a value' => ['<img alt="x onerror=alert(1)" src="y.png">'],
'data attributes' => ['<div data-one="1" data-two="2">x</div>'],
'framework bound attributes' => ['<button v-on:click="go" x-on:click="go" @click="go">x</button>'],
'several attributes' => ['<img src="a.png" alt="a" title="t" width="10">'],
'self closing tags' => ['<br /><hr />'],
'html entities' => ['a &amp; b &lt; c'],
];
}

/**
* @dataProvider safeMarkupProvider
*/
public function testCleanPreservesSafeMarkup($html)
{
$this->assertEquals($html, with(new HtmlBuilder)->clean($html));
}

public function testCleanKeepsTheAttributesSurroundingARemovedOne()
{
$result = with(new HtmlBuilder)->clean('<img alt=">" onerror=alert(1) src="y.png" title="t">');

$this->assertStringNotContainsString('onerror', $result);
$this->assertStringContainsString('alt=">"', $result);
$this->assertStringContainsString('src="y.png"', $result);
$this->assertStringContainsString('title="t"', $result);
}
}
Loading