Skip to content

Commit ce3e40e

Browse files
svitjasvitja
committed
MAGETWO-72037: Bypass CSRF protection in CMS Block, Page and Email Template via XSS in Custom Variable
1 parent cd151d5 commit ce3e40e

File tree

3 files changed

+66
-43
lines changed

3 files changed

+66
-43
lines changed

‎app/code/Magento/Variable/Model/Variable.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,11 +153,11 @@ public function getVariablesOptionArray($withGroup = false)
153153
foreach ($collection->toOptionArray() as $variable) {
154154
$variables[] = [
155155
'value' => '{{customVar code=' . $variable['value'] . '}}',
156-
'label' => __('%1', $variable['label']),
156+
'label' => __('%1', $this->_escaper->escapeHtml($variable['label'])),
157157
];
158158
}
159159
if ($withGroup && $variables) {
160-
$variables = [['label' => __('Custom Variables'), 'value' => $variables]];
160+
$variables = ['label' => __('Custom Variables'), 'value' => $variables];
161161
}
162162
return $variables;
163163
}

‎app/code/Magento/Variable/Test/Unit/Model/VariableTest.php

Lines changed: 48 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,34 @@
99

1010
class VariableTest extends \PHPUnit\Framework\TestCase
1111
{
12-
/** @var \Magento\Variable\Model\Variable */
12+
/**
13+
* @var \Magento\Variable\Model\Variable
14+
*/
1315
private $model;
1416

15-
/** @var \PHPUnit_Framework_MockObject_MockObject */
17+
/**
18+
* @var \PHPUnit_Framework_MockObject_MockObject
19+
*/
1620
private $escaperMock;
1721

18-
/** @var \PHPUnit_Framework_MockObject_MockObject */
22+
/**
23+
* @var \PHPUnit_Framework_MockObject_MockObject
24+
*/
1925
private $resourceMock;
2026

21-
/** @var \Magento\Framework\Phrase */
27+
/**
28+
* @var \PHPUnit_Framework_MockObject_MockObject
29+
*/
30+
private $resourceCollection;
31+
32+
/**
33+
* @var \Magento\Framework\Phrase
34+
*/
2235
private $validationFailedPhrase;
2336

24-
/** @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager */
37+
/**
38+
* @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager
39+
*/
2540
private $objectManager;
2641

2742
protected function setUp()
@@ -33,11 +48,17 @@ protected function setUp()
3348
$this->resourceMock = $this->getMockBuilder(\Magento\Variable\Model\ResourceModel\Variable::class)
3449
->disableOriginalConstructor()
3550
->getMock();
51+
$this->resourceCollection = $this->getMockBuilder(
52+
\Magento\Variable\Model\ResourceModel\Variable\Collection::class
53+
)
54+
->disableOriginalConstructor()
55+
->getMock();
3656
$this->model = $this->objectManager->getObject(
3757
\Magento\Variable\Model\Variable::class,
3858
[
3959
'escaper' => $this->escaperMock,
40-
'resource' => $this->resourceMock
60+
'resource' => $this->resourceMock,
61+
'resourceCollection' => $this->resourceCollection,
4162
]
4263
);
4364
$this->validationFailedPhrase = __('Validation has failed.');
@@ -101,58 +122,44 @@ public function testValidate($variableArray, $objectId, $expectedResult)
101122
public function testGetVariablesOptionArrayNoGroup()
102123
{
103124
$origOptions = [
104-
['value' => 'VAL', 'label' => 'LBL']
125+
['value' => 'VAL', 'label' => 'LBL'],
105126
];
106127

107128
$transformedOptions = [
108-
['value' => '{{customVar code=VAL}}', 'label' => __('%1', 'LBL')]
129+
['value' => '{{customVar code=VAL}}', 'label' => __('%1', 'LBL')],
109130
];
110131

111-
$collectionMock = $this->getMockBuilder(\Magento\Variable\Model\ResourceModel\Variable\Collection::class)
112-
->disableOriginalConstructor()
113-
->getMock();
114-
$collectionMock->expects($this->any())
132+
$this->resourceCollection->expects($this->any())
115133
->method('toOptionArray')
116134
->willReturn($origOptions);
117-
$mockVariable = $this->getMockBuilder(\Magento\Variable\Model\Variable::class)
118-
->setMethods(['getCollection'])
119-
->setConstructorArgs($this->objectManager->getConstructArguments(\Magento\Variable\Model\Variable::class))
120-
->getMock();
121-
$mockVariable->expects($this->any())
122-
->method('getCollection')
123-
->willReturn($collectionMock);
124-
$this->assertEquals($transformedOptions, $mockVariable->getVariablesOptionArray());
135+
$this->escaperMock->expects($this->once())
136+
->method('escapeHtml')
137+
->with($origOptions[0]['label'])
138+
->willReturn($origOptions[0]['label']);
139+
$this->assertEquals($transformedOptions, $this->model->getVariablesOptionArray());
125140
}
126141

127142
public function testGetVariablesOptionArrayWithGroup()
128143
{
129144
$origOptions = [
130-
['value' => 'VAL', 'label' => 'LBL']
145+
['value' => 'VAL', 'label' => 'LBL'],
131146
];
132147

133148
$transformedOptions = [
134-
[
135-
'label' => __('Custom Variables'),
136-
'value' => [
137-
['value' => '{{customVar code=VAL}}', 'label' => __('%1', 'LBL')]
138-
]
139-
]
149+
'label' => __('Custom Variables'),
150+
'value' => [
151+
['value' => '{{customVar code=VAL}}', 'label' => __('%1', 'LBL')],
152+
],
140153
];
141154

142-
$collectionMock = $this->getMockBuilder(\Magento\Variable\Model\ResourceModel\Variable\Collection::class)
143-
->disableOriginalConstructor()
144-
->getMock();
145-
$collectionMock->expects($this->any())
155+
$this->resourceCollection->expects($this->any())
146156
->method('toOptionArray')
147157
->willReturn($origOptions);
148-
$mockVariable = $this->getMockBuilder(\Magento\Variable\Model\Variable::class)
149-
->setMethods(['getCollection'])
150-
->setConstructorArgs($this->objectManager->getConstructArguments(\Magento\Variable\Model\Variable::class))
151-
->getMock();
152-
$mockVariable->expects($this->any())
153-
->method('getCollection')
154-
->willReturn($collectionMock);
155-
$this->assertEquals($transformedOptions, $mockVariable->getVariablesOptionArray(true));
158+
$this->escaperMock->expects($this->atLeastOnce())
159+
->method('escapeHtml')
160+
->with($origOptions[0]['label'])
161+
->willReturn($origOptions[0]['label']);
162+
$this->assertEquals($transformedOptions, $this->model->getVariablesOptionArray(true));
156163
}
157164

158165
public function validateDataProvider()
@@ -163,15 +170,15 @@ public function validateDataProvider()
163170
return [
164171
'Empty Variable' => [[], null, true],
165172
'IDs match' => [$variable, 'matching_id', true],
166-
'IDs do not match' => [$variable, 'non_matching_id', __('Variable Code must be unique.')]
173+
'IDs do not match' => [$variable, 'non_matching_id', __('Variable Code must be unique.')],
167174
];
168175
}
169176

170177
public function validateMissingInfoDataProvider()
171178
{
172179
return [
173180
'Missing code' => ['', 'some-name'],
174-
'Missing name' => ['some-code', '']
181+
'Missing name' => ['some-code', ''],
175182
];
176183
}
177184
}

‎dev/tests/integration/testsuite/Magento/Variable/Model/VariableTest.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,20 @@ public function testCollection()
7777
$collection->addValuesToResult();
7878
$this->assertContains('variable_value', (string)$collection->getSelect());
7979
}
80+
81+
/**
82+
* Test to verify that returned by getVariablesOptionArray()
83+
* custom variable label is HTML escaped.
84+
*/
85+
public function testGetVariablesOptionArrayWithHtmlLabel()
86+
{
87+
$expectedLabel = '<b>HTML Name value</b>';
88+
$data = [
89+
'code' => 'html_name',
90+
'name' => '<b>HTML Name value</b>'
91+
];
92+
$this->_model->setData($data)->save();
93+
$actualLabel = current(current($this->_model->getVariablesOptionArray())['label']->getArguments());
94+
$this->assertEquals($expectedLabel, $actualLabel);
95+
}
8096
}

0 commit comments

Comments
 (0)