Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
BUG HTMLText whitelist considers text nodes
Minor improvement to #2853.
If a list of whitelisted elements are specified, text nodes no longer evade the whitelist
  • Loading branch information
Damian Mooyman committed Apr 28, 2014
1 parent ee487e2 commit 91034d1
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
29 changes: 23 additions & 6 deletions model/fieldtypes/HTMLText.php
Expand Up @@ -55,8 +55,9 @@ public function __construct($name = null, $options = array()) {
* - whitelist: If provided, a comma-separated list of elements that will be allowed to be stored
* (be careful on relying on this for XSS protection - some seemingly-safe elements allow
* attributes that can be exploited, for instance <img onload="exploiting_code();" src="..." />)
*
* @return void
* Text nodes outside of HTML tags are filtered out by default, but may be included by adding
* the text() directive. E.g. 'link,meta,text()' will allow only <link /> <meta /> and text at
* the root level.
*/
public function setOptions(array $options = array()) {
parent::setOptions($options);
Expand Down Expand Up @@ -184,20 +185,36 @@ public function forTemplate() {
}

public function prepValueForDB($value) {
return parent::prepValueForDB($this->whitelistContent($value));
}

/**
* Filter the given $value string through the whitelist filter
*
* @param string $value Input html content
* @return string Value with all non-whitelisted content stripped (if applicable)
*/
public function whitelistContent($value) {
if($this->whitelist) {
$dom = Injector::inst()->create('HTMLValue', $value);

$query = array();
foreach ($this->whitelist as $tag) $query[] = 'not(self::'.$tag.')';
$textFilter = ' | //body/text()';
foreach ($this->whitelist as $tag) {
if($tag === 'text()') {
$textFilter = ''; // Disable text filter if allowed
} else {
$query[] = 'not(self::'.$tag.')';
}
}

foreach($dom->query('//body//*['.implode(' and ', $query).']') as $el) {
foreach($dom->query('//body//*['.implode(' and ', $query).']'.$textFilter) as $el) {
if ($el->parentNode) $el->parentNode->removeChild($el);
}

$value = $dom->getContent();
}

return parent::prepValueForDB($value);
return $value;
}

/**
Expand Down
14 changes: 10 additions & 4 deletions tests/model/HTMLTextTest.php
Expand Up @@ -179,11 +179,17 @@ function testExists() {

function testWhitelist() {
$textObj = new HTMLText('Test', 'meta,link');

$this->assertEquals(
$textObj->prepValueForDB('<meta content="Keep"><link href="Also Keep">'),
$textObj->prepValueForDB('<meta content="Keep"><p>Remove</p><link href="Also Keep" />'),
'Removes any elements not in whitelist'
'<meta content="Keep"><link href="Also Keep">',
$textObj->whitelistContent('<meta content="Keep"><p>Remove</p><link href="Also Keep" />Remove Text'),
'Removes any elements not in whitelist excluding text elements'
);

$textObj = new HTMLText('Test', 'meta,link,text()');
$this->assertEquals(
'<meta content="Keep"><link href="Also Keep">Keep Text',
$textObj->whitelistContent('<meta content="Keep"><p>Remove</p><link href="Also Keep" />Keep Text'),
'Removes any elements not in whitelist including text elements'
);
}
}

0 comments on commit 91034d1

Please sign in to comment.