Skip to content

Commit 9780610

Browse files
author
Abhishek Jakhotiya
committed
Added tests for different kind of expressions possible in echo statement.
We are not trying to fix all the unsafe output. We try to escape output that we can be sure of. We leave complex expressions as it is. Developer can for now fix them manually. As of now it should cover 70% of cases. Escaping xss in URL will be handled as a separate rule.
1 parent e8232c6 commit 9780610

8 files changed

+155
-8
lines changed

‎Magento2/Rector/Src/AddHtmlEscaperToOutput.php

Lines changed: 103 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616

1717
class AddHtmlEscaperToOutput extends AbstractRector
1818
{
19+
20+
private $_phpFunctionsToIgnore = ['\count','\strip_tags'];
21+
1922
/**
2023
* @inheritDoc
2124
*/
@@ -29,20 +32,37 @@ public function getNodeTypes(): array
2932
*/
3033
public function refactor(Node $node): ?Node
3134
{
32-
// if the echo already has escapeHtml called on $block or escaper, don't do anthing
3335

3436
$echoContent = $node->exprs[0];
3537

36-
if ($echoContent instanceof Node\Expr\Variable || $echoContent instanceof Node\Expr\FuncCall) {
37-
$node->exprs[0] = new Node\Expr\MethodCall(
38-
new Node\Expr\Variable('escaper'),
39-
'escapeHtml',
40-
[new Node\Arg($echoContent)]
41-
);
38+
if ($echoContent instanceof Node\Expr\Ternary) {
39+
40+
if (!$this->hasNoEscapeAttribute($echoContent->if) && $this->canEscapeOutput($echoContent->if)) {
41+
$node->exprs[0]->if = new Node\Expr\MethodCall(
42+
new Node\Expr\Variable('escaper'),
43+
'escapeHtml',
44+
[new Node\Arg($echoContent->if)]
45+
);
46+
47+
}
48+
49+
if (!$this->hasNoEscapeAttribute($echoContent->else) && $this->canEscapeOutput($echoContent->else)) {
50+
$node->exprs[0]->else = new Node\Expr\MethodCall(
51+
new Node\Expr\Variable('escaper'),
52+
'escapeHtml',
53+
[new Node\Arg($echoContent->else)]
54+
);
55+
56+
}
4257
return $node;
58+
4359
}
4460

45-
if ($echoContent instanceof Node\Expr\MethodCall && $echoContent->name != 'escapeHtml') {
61+
if ($this->hasNoEscapeAttribute($echoContent)) {
62+
return null;
63+
}
64+
65+
if ($this->canEscapeOutput($echoContent)) {
4666

4767
$node->exprs[0] = new Node\Expr\MethodCall(
4868
new Node\Expr\Variable('escaper'),
@@ -55,6 +75,81 @@ public function refactor(Node $node): ?Node
5575
return null;
5676
}
5777

78+
private function canEscapeOutput(Node $echoContent):bool
79+
{
80+
if ($echoContent instanceof Node\Expr\Variable) {
81+
return true;
82+
}
83+
84+
if ($echoContent instanceof Node\Expr\FuncCall
85+
&& !$this->willFunctionReturnSafeOutput($echoContent)) {
86+
87+
// if string passed to __() contains html don't do anthing
88+
if ($echoContent->name == '__' && $this->stringContainsHtml($echoContent->args[0]->value->value)) {
89+
return false;
90+
}
91+
92+
return true;
93+
}
94+
95+
// if the echo already has escapeHtml called on $block or escaper, don't do anthing
96+
if ($echoContent instanceof Node\Expr\MethodCall &&
97+
!$this->methodReturnsValidHtmlOrUrl($echoContent)
98+
) {
99+
100+
// if the method is part of secureRenderer don't do anything
101+
if ($echoContent->var->name === 'secureRenderer') {
102+
return false;
103+
}
104+
105+
return true;
106+
107+
}
108+
109+
return false;
110+
}
111+
112+
private function stringContainsHtml(string $str)
113+
{
114+
return strlen($str) !== strlen(strip_tags($str));
115+
}
116+
117+
/**
118+
* If the developer has marked the output as noEscape by using the @noEscape
119+
* we want to leave that code as it is
120+
*/
121+
private function hasNoEscapeAttribute(Node $echoContent):bool
122+
{
123+
124+
$comments = $echoContent->getComments();
125+
foreach ($comments as $comment) {
126+
if (stripos($comment->getText(), '@noEscape') !== false) {
127+
return true;
128+
}
129+
}
130+
return false;
131+
}
132+
133+
/**
134+
* If method contains the keyword HTML we assume developer intends to output html
135+
*/
136+
private function methodReturnsValidHtmlOrUrl(Node\Expr\MethodCall $echoContent):bool
137+
{
138+
return stripos($echoContent->name->name, 'Html') !== false
139+
|| stripos($echoContent->name->name, 'Url') !== false;
140+
}
141+
142+
/**
143+
* Some php function return safe output. They need not be escaped.
144+
* count, strip_tags are example
145+
*/
146+
private function willFunctionReturnSafeOutput(Node\Expr\FuncCall $funcNode):bool
147+
{
148+
//@TODO did not handle things like $callback();
149+
$funcName = $funcNode->name->toCodeString();
150+
return in_array($funcName, $this->_phpFunctionsToIgnore);
151+
}
152+
58153
/**
59154
* @inheritDoc
60155
*/

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/complex-phtml-code-mix.php.inc

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
$items = $block->getAllItems();
3+
?>
4+
<div class="test">
5+
<?php if (count($items) > 0): ?>
6+
<div class="counter"><?= __('Total <span>%1</span> items found', count($items)) ?></div>
7+
<?php foreach ($items as $item): ?>
8+
<a href="<?= $block->getActionUrl($item); ?>"><?= $item->getName(); ?></a>
9+
<?php endforeach; ?>
10+
<?php else: ?>
11+
<?= __('No items found.'); ?>
12+
<?php endif; ?>
13+
</div>
14+
-----
15+
<?php
16+
$items = $block->getAllItems();
17+
?>
18+
<div class="test">
19+
<?php if (count($items) > 0): ?>
20+
<div class="counter"><?= __('Total <span>%1</span> items found', count($items)) ?></div>
21+
<?php foreach ($items as $item): ?>
22+
<a href="<?= $block->getActionUrl($item); ?>"><?= $escaper->escapeHtml($item->getName()); ?></a>
23+
<?php endforeach; ?>
24+
<?php else: ?>
25+
<?= $escaper->escapeHtml(__('No items found.')); ?>
26+
<?php endif; ?>
27+
</div>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/message-string-contains-html.php.inc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
<div class="counter"><?= __('Total <span>%1</span> items found',count($items))?></div>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/method-that-return-html.php.inc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<?= $block->getChildHtml(); ?>
2+
<?= $block->getToolBarHtml();?>
3+
<?= $block->getChildBlock('toolbar')->setIsBottom(true)->toHtml() ?>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-escaped-output-using-secure-renderer.php.inc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?= $secureRenderer->renderStyleAsTag(
2+
$position,
3+
'product-item-info_' . $_product->getId() . ' div.actions-primary'
4+
) ?>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-output-of-count-function.php.inc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<?= count($items); ?>
2+
<?= (int)$items ?>
3+
<?= (bool)$items ?>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-with-no-escape-annotation.php.inc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<?= /* @noEscape */ $block->getProductPrice($_product) ?>
2+
<?= /* @noEscape */ $price ?>
3+
<?= /* @noEscape */ trim($price) ?>

‎Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/ternary-condition.php.inc

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
3+
echo $block->canShowInfo() ? $escaper->escapeHtml($block->getInfo()) : __('Nothing to show');
4+
5+
?>
6+
-----
7+
<?php
8+
9+
echo $block->canShowInfo() ? $escaper->escapeHtml($block->getInfo()) : $escaper->escapeHtml(__('Nothing to show'));
10+
11+
?>

0 commit comments

Comments
 (0)