Skip to content

Commit a1e61f3

Browse files
davemacaulaydavemacaulay
committed
MC-10871: [Sec] XSS Injection in HTML Code Content Type
- Resolve issue with innerHTML usage - Resolve issue with HTML code executing in the browser by storing HTML as an encoded string then decoding it on the storefront
1 parent de61a63 commit a1e61f3

File tree

11 files changed

+210
-59
lines changed

11 files changed

+210
-59
lines changed

‎app/code/Magento/PageBuilder/Plugin/Filter/TemplatePlugin.php

Lines changed: 105 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ class TemplatePlugin
2424
*/
2525
private $logger;
2626

27+
/**
28+
* @var \DOMDocument
29+
*/
30+
private $domDocument;
31+
2732
/**
2833
* @param \Psr\Log\LoggerInterface $logger
2934
* @param \Magento\Framework\View\ConfigInterface $viewConfig
@@ -37,6 +42,8 @@ public function __construct(
3742
}
3843

3944
/**
45+
* After filter of template data apply transformations
46+
*
4047
* @param \Magento\Framework\Filter\Template $subject
4148
* @param string $result
4249
*
@@ -47,48 +54,113 @@ public function afterFilter(\Magento\Framework\Filter\Template $subject, string
4754
{
4855
// Validate if the filtered result requires background image processing
4956
if (strpos($result, self::DATA_BACKGROUND_IMAGE) !== false) {
50-
$domDocument = new \DOMDocument('1.0', 'UTF-8');
51-
set_error_handler(
52-
function ($errorNumber, $errorString) {
53-
throw new \Exception($errorString, $errorNumber);
54-
}
55-
);
56-
$string = mb_convert_encoding($result, 'HTML-ENTITIES', 'UTF-8');
57-
$wrapperElementId = uniqid();
58-
try {
59-
libxml_use_internal_errors(true);
60-
$domDocument->loadHTML(
61-
'<html><body id="' . $wrapperElementId . '">' . $string . '</body></html>'
62-
);
63-
libxml_clear_errors();
64-
} catch (\Exception $e) {
65-
restore_error_handler();
66-
$this->logger->critical($e);
67-
}
68-
restore_error_handler();
69-
$xpath = new \DOMXPath($domDocument);
57+
$document = $this->getDomDocument($result);
58+
$this->generateBackgroundImageStyles($document);
59+
}
7060

71-
$this->generateBackgroundImageStyles($xpath);
61+
// Process any HTML content types, they need to be decoded on the front-end
62+
if (strpos($result, 'data-role="html"') !== false) {
63+
$document = $this->getDomDocument($result);
64+
$this->decodeHtmlContentTypes($document);
65+
}
7266

67+
if (isset($document)) {
7368
// Match the contents of the body from our generated document
7469
preg_match(
75-
'/<body id="' . $wrapperElementId . '">(.+)<\/body><\/html>$/si',
76-
$domDocument->saveHTML(),
70+
'/<body>(.+)<\/body><\/html>$/si',
71+
$document->saveHTML(),
7772
$matches
7873
);
7974

80-
return !empty($matches) ? $matches[1] : $result;
75+
$result = !empty($matches) ? $matches[1] : $result;
8176
}
77+
8278
return $result;
8379
}
8480

81+
/**
82+
* Create a DOM document from a given string
83+
*
84+
* @param string $html
85+
*
86+
* @return \DOMDocument
87+
*/
88+
private function getDomDocument(string $html) : \DOMDocument
89+
{
90+
if (!$this->domDocument) {
91+
$this->domDocument = $this->createDomDocument($html);
92+
}
93+
94+
return $this->domDocument;
95+
}
96+
97+
/**
98+
* Create a DOMDocument from a string
99+
*
100+
* @param string $html
101+
*
102+
* @return \DOMDocument
103+
*/
104+
private function createDomDocument(string $html) : \DOMDocument
105+
{
106+
$domDocument = new \DOMDocument('1.0', 'UTF-8');
107+
set_error_handler(
108+
function ($errorNumber, $errorString) {
109+
throw new \Exception($errorString, $errorNumber);
110+
}
111+
);
112+
$string = mb_convert_encoding($html, 'HTML-ENTITIES', 'UTF-8');
113+
try {
114+
libxml_use_internal_errors(true);
115+
$domDocument->loadHTML(
116+
'<html><body>' . $string . '</body></html>'
117+
);
118+
libxml_clear_errors();
119+
} catch (\Exception $e) {
120+
restore_error_handler();
121+
$this->logger->critical($e);
122+
}
123+
restore_error_handler();
124+
125+
return $domDocument;
126+
}
127+
128+
/**
129+
* Decode the contents of any HTML content types for the store front
130+
*
131+
* @param \DOMDocument $document
132+
*/
133+
private function decodeHtmlContentTypes(\DOMDocument $document): void
134+
{
135+
$xpath = new \DOMXPath($document);
136+
$nodes = $xpath->query('//*[@data-role="html" and not(@data-decoded="true")]');
137+
foreach ($nodes as $node) {
138+
if (strlen(trim($node->nodeValue)) > 0) {
139+
/* @var \DOMElement $node */
140+
$fragment = $document->createDocumentFragment();
141+
$fragment->appendXML($node->nodeValue);
142+
// Store a decoded attribute on the element so we don't double decode
143+
$node->setAttribute("data-decoded", "true");
144+
$node->nodeValue = "";
145+
146+
// If the HTML code in the content type is invalid it may throw
147+
try {
148+
$node->appendChild($fragment);
149+
} catch (\Exception $e) {
150+
$this->logger->critical($e);
151+
}
152+
}
153+
}
154+
}
155+
85156
/**
86157
* Generate the CSS for any background images on the page
87158
*
88-
* @param \DOMXPath $xpath
159+
* @param \DOMDocument $document
89160
*/
90-
private function generateBackgroundImageStyles(\DOMXPath $xpath) : void
161+
private function generateBackgroundImageStyles(\DOMDocument $document) : void
91162
{
163+
$xpath = new \DOMXPath($document);
92164
$nodes = $xpath->query('//*[@' . self:: DATA_BACKGROUND_IMAGE . ']');
93165
foreach ($nodes as $node) {
94166
/* @var \DOMElement $node */
@@ -101,7 +173,7 @@ private function generateBackgroundImageStyles(\DOMXPath $xpath) : void
101173
'style',
102174
$this->generateCssFromImages($elementClass, $images)
103175
);
104-
$style->setAttribute("type", "text/css");
176+
$style->setAttribute('type', 'text/css');
105177
$node->parentNode->appendChild($style);
106178

107179
// Append our new class to the DOM element
@@ -148,14 +220,14 @@ private function generateCssFromImages(string $elementClass, array $images) : st
148220
*/
149221
private function cssFromArray(array $css) : string
150222
{
151-
$output = "";
223+
$output = '';
152224
foreach ($css as $selector => $body) {
153225
if (is_array($body)) {
154-
$output .= $selector . " {";
226+
$output .= $selector . ' {';
155227
$output .= $this->cssFromArray($body);
156-
$output .= "}";
228+
$output .= '}';
157229
} else {
158-
$output .= $selector . ': ' . $body . ";";
230+
$output .= $selector . ': ' . $body . ';';
159231
}
160232
}
161233
return $output;
@@ -173,9 +245,9 @@ private function getMobileMediaQuery() : ?string
173245
'breakpoints/mobile/conditions'
174246
);
175247
if ($breakpoints && count($breakpoints) > 0) {
176-
$mobileBreakpoint = "@media only screen ";
248+
$mobileBreakpoint = '@media only screen ';
177249
foreach ($breakpoints as $key => $value) {
178-
$mobileBreakpoint .= "and (" . $key . ": " . $value . ") ";
250+
$mobileBreakpoint .= 'and (' . $key . ': ' . $value . ') ';
179251
}
180252
return rtrim($mobileBreakpoint);
181253
}

‎app/code/Magento/PageBuilder/view/adminhtml/pagebuilder/content_type/html.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
<style name="padding" storage_key="margins_and_padding" reader="Magento_PageBuilder/js/property/paddings" converter="Magento_PageBuilder/js/converter/style/paddings"/>
3535
<attribute name="name" source="data-role"/>
3636
<attribute name="appearance" source="data-appearance"/>
37-
<html name="html"/>
37+
<html name="html" converter="Magento_PageBuilder/js/converter/html/encode"/>
3838
<css name="css_classes"/>
3939
</element>
4040
</elements>

‎app/code/Magento/PageBuilder/view/adminhtml/ui_component/pagebuilder_html_form.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
<dataScope>html</dataScope>
8989
<dataType>text</dataType>
9090
<placeholder translate="true">Enter HTML, CSS or JavaScript code</placeholder>
91+
<notice translate="true">HTML code must be valid, all CSS &amp; JavaScript must be wrapped in their HTML elements.</notice>
9192
</settings>
9293
</field>
9394
</fieldset>

‎app/code/Magento/PageBuilder/view/adminhtml/web/css/source/content-type/html/_default.less

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@
4444
}
4545

4646
.admin__font-console {
47-
font-family: @font-family__console;
4847
textarea {
48+
font-family: @font-family__console;
4949
font-size: 14px;
5050
height: 500px;
5151
line-height: 22px;

‎app/code/Magento/PageBuilder/view/adminhtml/web/js/converter/html/encode.js

Lines changed: 49 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎app/code/Magento/PageBuilder/view/adminhtml/web/js/master-format/validator.js

Lines changed: 5 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎app/code/Magento/PageBuilder/view/adminhtml/web/js/stage-builder.js

Lines changed: 3 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎app/code/Magento/PageBuilder/view/adminhtml/web/template/content-type/html/default/master.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,4 +5,4 @@
55
*/
66
-->
77

8-
<div attr="data.main.attributes" css="data.main.css" ko-style="data.main.style" html="data.main.html" class="bypass-html-filter"></div>
8+
<div attr="data.main.attributes" css="data.main.css" ko-style="data.main.style" text="data.main.html" class="bypass-html-filter"></div>

‎app/code/Magento/PageBuilder/view/adminhtml/web/ts/js/converter/html/encode.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/**
2+
* Copyright © Magento, Inc. All rights reserved.
3+
* See COPYING.txt for license details.
4+
*/
5+
6+
import {DataObject} from "../../data-store";
7+
import {get} from "../../utils/object";
8+
import ConverterInterface from "../converter-interface";
9+
10+
/**
11+
* @api
12+
*/
13+
export default class Encode implements ConverterInterface {
14+
/**
15+
* Convert value to internal format
16+
*
17+
* @param {string} value
18+
* @returns {string | object}
19+
*/
20+
public fromDom(value: string): string | object {
21+
// Convert the encoded string back to HTML without executing
22+
const html = new DOMParser().parseFromString(value, "text/html");
23+
return html.body.textContent;
24+
}
25+
26+
/**
27+
* Convert value to knockout format
28+
*
29+
* @param {string} name
30+
* @param {Object} data
31+
* @returns {string}
32+
*/
33+
public toDom(name: string, data: DataObject): string {
34+
return get(data, name);
35+
}
36+
}

‎app/code/Magento/PageBuilder/view/adminhtml/web/ts/js/master-format/validator.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,14 @@
66
import Config from "../config";
77

88
/**
9-
* Validate if content has page builder format
9+
* Validate if content has page builder format by checking for any data-role attributes
1010
*
1111
* @param {string} content
1212
* @returns {boolean}
1313
*/
14-
export default function Validate(content: string) {
15-
const stageDocument = document.createElement("div");
16-
17-
stageDocument.setAttribute(Config.getConfig("dataRoleAttributeName"), "stage");
18-
stageDocument.innerHTML = content;
14+
export default function validate(content: string) {
15+
const stageDocument = new DOMParser().parseFromString(content, "text/html");
1916
return !!stageDocument.querySelector(
20-
"[" + Config.getConfig("dataRoleAttributeName") + "=\"row\"]",
17+
`[${Config.getConfig("dataRoleAttributeName")}]`,
2118
);
2219
}

0 commit comments

Comments
 (0)