Skip to content

Commit

Permalink
XML/HTML entities in attributes will no longer be preserved when rend…
Browse files Browse the repository at this point in the history
…ering (#353)

This applies the previous commit's XSS fix to all custom attributes
  • Loading branch information
colinodell committed Mar 21, 2019
1 parent f1453b9 commit 6f16c6e
Show file tree
Hide file tree
Showing 14 changed files with 17 additions and 13 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,10 @@ Updates should follow the [Keep a CHANGELOG](http://keepachangelog.com/) princip

## [Unreleased][unreleased]

### Changed

- XML/HTML entities in attributes will no longer be preserved when rendering (#353)

### Fixed

- Fix XSS vulnerability caused by improper preservation of entities when rendering (#353)
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/BlockQuoteRenderer.php
Expand Up @@ -37,7 +37,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

$filling = $htmlRenderer->renderBlocks($block->children());
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/FencedCodeRenderer.php
Expand Up @@ -37,7 +37,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

$infoWords = $block->getInfoWords();
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/HeadingRenderer.php
Expand Up @@ -39,7 +39,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement($tag, $attrs, $htmlRenderer->renderInlines($block->children()));
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/IndentedCodeRenderer.php
Expand Up @@ -37,7 +37,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement(
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/ListBlockRenderer.php
Expand Up @@ -41,7 +41,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

if ($listData->start !== null && $listData->start !== 1) {
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/ListItemRenderer.php
Expand Up @@ -45,7 +45,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

$li = new HtmlElement('li', $attrs, $contents);
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/ParagraphRenderer.php
Expand Up @@ -41,7 +41,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement('p', $attrs, $htmlRenderer->renderInlines($block->children()));
Expand Down
2 changes: 1 addition & 1 deletion src/Block/Renderer/ThematicBreakRenderer.php
Expand Up @@ -37,7 +37,7 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende

$attrs = [];
foreach ($block->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement('hr', $attrs, '', true);
Expand Down
2 changes: 1 addition & 1 deletion src/Inline/Renderer/CodeRenderer.php
Expand Up @@ -36,7 +36,7 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen

$attrs = [];
foreach ($inline->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement('code', $attrs, Xml::escape($inline->getContent()));
Expand Down
2 changes: 1 addition & 1 deletion src/Inline/Renderer/EmphasisRenderer.php
Expand Up @@ -36,7 +36,7 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen

$attrs = [];
foreach ($inline->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement('em', $attrs, $htmlRenderer->renderInlines($inline->children()));
Expand Down
2 changes: 1 addition & 1 deletion src/Inline/Renderer/ImageRenderer.php
Expand Up @@ -44,7 +44,7 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen

$attrs = [];
foreach ($inline->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

$forbidUnsafeLinks = $this->config->getConfig('safe') || !$this->config->getConfig('allow_unsafe_links');
Expand Down
2 changes: 1 addition & 1 deletion src/Inline/Renderer/LinkRenderer.php
Expand Up @@ -44,7 +44,7 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen

$attrs = [];
foreach ($inline->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

$forbidUnsafeLinks = $this->config->getConfig('safe') || !$this->config->getConfig('allow_unsafe_links');
Expand Down
2 changes: 1 addition & 1 deletion src/Inline/Renderer/StrongRenderer.php
Expand Up @@ -36,7 +36,7 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen

$attrs = [];
foreach ($inline->getData('attributes', []) as $key => $value) {
$attrs[$key] = Xml::escape($value, true);
$attrs[$key] = Xml::escape($value);
}

return new HtmlElement('strong', $attrs, $htmlRenderer->renderInlines($inline->children()));
Expand Down

2 comments on commit 6f16c6e

@glensc
Copy link
Contributor

@glensc glensc commented on 6f16c6e Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it's weird that you escape when adding attributes. as I think the data should be escaped depending on context, in this case when the attribute is rendered as html.

for example "raw to javascript" would have different escape rules.

this is probably internal optimization as the only escape needed is HTML (XML)? i.e context is "raw to html (xml)"

@colinodell
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should really be doing this inside the HtmlElement instead of relying on its callers to pre-escape things. I'll create an issue for this.

Please sign in to comment.