From 8fb1e13313b67d7edcae2c7e0fc89cdbbce37cfe Mon Sep 17 00:00:00 2001 From: Abhishek Jakhotiya Date: Wed, 15 Nov 2023 19:55:35 +0530 Subject: [PATCH 1/4] This rule escapes output using ->escapeHtml output function. It should be run only in phtml files because right now it will do this for whichever php file you provide. That is untested. You can take look at associated tests for which scenarios are covered. escapeJs and escapeUrl are yet to be implemented --- .../Rector/Src/AddHtmlEscaperToOutput.php | 65 +++++++++++++++++++ .../AddHtmlEscaperToOutputTest.php | 36 ++++++++++ .../Fixture/already-escaped-ouput.php.inc | 11 ++++ .../Fixture/function-call.php.inc | 11 ++++ .../Fixture/simple-echo.php.inc | 11 ++++ .../config/configured_rule.php | 13 ++++ 6 files changed, 147 insertions(+) create mode 100644 Magento2/Rector/Src/AddHtmlEscaperToOutput.php create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/AddHtmlEscaperToOutputTest.php create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/already-escaped-ouput.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/function-call.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/simple-echo.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/config/configured_rule.php diff --git a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php new file mode 100644 index 00000000..a7ea23c8 --- /dev/null +++ b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php @@ -0,0 +1,65 @@ +exprs[0]; + + if($echoContent instanceof Node\Expr\Variable || $echoContent instanceof Node\Expr\FuncCall){ + $node->exprs[0] = new Node\Expr\MethodCall(new Node\Expr\Variable('escaper'),'escapeHtml',[new Node\Arg($echoContent)]); + return $node; + } + + if($echoContent instanceof Node\Expr\MethodCall && $echoContent->name != 'escapeHtml'){ + + $node->exprs[0] = new Node\Expr\MethodCall(new Node\Expr\Variable('escaper'),'escapeHtml',[new Node\Arg($echoContent)]); + return $node; + } + + return null; + } + + /** + * @inheritDoc + */ + public function getRuleDefinition(): RuleDefinition + { + return new RuleDefinition( + 'Add escaper methods like escapeHtml to html output', + [ + new CodeSample( + 'echo $productName', + 'echo $escaper->escapeHtml($productName)' + ), + ] + ); + } +} diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/AddHtmlEscaperToOutputTest.php b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/AddHtmlEscaperToOutputTest.php new file mode 100644 index 00000000..6719752d --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/AddHtmlEscaperToOutputTest.php @@ -0,0 +1,36 @@ +doTestFile($fileInfo); + } + + /** + * @return Iterator + */ + public function provideData(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/already-escaped-ouput.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/already-escaped-ouput.php.inc new file mode 100644 index 00000000..555eddf4 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/already-escaped-ouput.php.inc @@ -0,0 +1,11 @@ +escapeHtml($productName); + +?> +----- +escapeHtml($productName); + +?> diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/function-call.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/function-call.php.inc new file mode 100644 index 00000000..32107eb3 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/function-call.php.inc @@ -0,0 +1,11 @@ +getName(); +echo __('Something'); +?> +----- +escapeHtml($customer->getName()); +echo $escaper->escapeHtml(__('Something')); +?> diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/simple-echo.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/simple-echo.php.inc new file mode 100644 index 00000000..8bb9d442 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/simple-echo.php.inc @@ -0,0 +1,11 @@ + +----- +escapeHtml($productName); + +?> diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/config/configured_rule.php b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/config/configured_rule.php new file mode 100644 index 00000000..e8366789 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/config/configured_rule.php @@ -0,0 +1,13 @@ +rule(\Magento2\Rector\Src\AddHtmlEscaperToOutput::class); +}; From e8232c63b934709eaa30fa1c363e964f30432cb8 Mon Sep 17 00:00:00 2001 From: Abhishek Jakhotiya Date: Thu, 16 Nov 2023 06:50:57 +0530 Subject: [PATCH 2/4] Fixed phpcs errors and warnings because of which the merge checks were failing --- .../Rector/Src/AddHtmlEscaperToOutput.php | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php index a7ea23c8..bc3a8c55 100644 --- a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php +++ b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php @@ -29,20 +29,28 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - // if the echo already has escapeHtml called on $block or escaper, don't do anthing + // if the echo already has escapeHtml called on $block or escaper, don't do anthing - $echoContent = $node->exprs[0]; + $echoContent = $node->exprs[0]; - if($echoContent instanceof Node\Expr\Variable || $echoContent instanceof Node\Expr\FuncCall){ - $node->exprs[0] = new Node\Expr\MethodCall(new Node\Expr\Variable('escaper'),'escapeHtml',[new Node\Arg($echoContent)]); - return $node; - } + if ($echoContent instanceof Node\Expr\Variable || $echoContent instanceof Node\Expr\FuncCall) { + $node->exprs[0] = new Node\Expr\MethodCall( + new Node\Expr\Variable('escaper'), + 'escapeHtml', + [new Node\Arg($echoContent)] + ); + return $node; + } - if($echoContent instanceof Node\Expr\MethodCall && $echoContent->name != 'escapeHtml'){ + if ($echoContent instanceof Node\Expr\MethodCall && $echoContent->name != 'escapeHtml') { - $node->exprs[0] = new Node\Expr\MethodCall(new Node\Expr\Variable('escaper'),'escapeHtml',[new Node\Arg($echoContent)]); - return $node; - } + $node->exprs[0] = new Node\Expr\MethodCall( + new Node\Expr\Variable('escaper'), + 'escapeHtml', + [new Node\Arg($echoContent)] + ); + return $node; + } return null; } From 97806102f4eecc30e72b4e11f9a99d8604f03626 Mon Sep 17 00:00:00 2001 From: Abhishek Jakhotiya Date: Thu, 23 Nov 2023 00:03:17 +0530 Subject: [PATCH 3/4] 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. --- .../Rector/Src/AddHtmlEscaperToOutput.php | 111 ++++++++++++++++-- .../Fixture/complex-phtml-code-mix.php.inc | 27 +++++ .../message-string-contains-html.php.inc | 1 + .../Fixture/method-that-return-html.php.inc | 3 + ...caped-output-using-secure-renderer.php.inc | 4 + .../skip-output-of-count-function.php.inc | 3 + .../skip-with-no-escape-annotation.php.inc | 3 + .../Fixture/ternary-condition.php.inc | 11 ++ 8 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/complex-phtml-code-mix.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/message-string-contains-html.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/method-that-return-html.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-escaped-output-using-secure-renderer.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-output-of-count-function.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-with-no-escape-annotation.php.inc create mode 100644 Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/ternary-condition.php.inc diff --git a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php index bc3a8c55..f2a01ed2 100644 --- a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php +++ b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php @@ -16,6 +16,9 @@ class AddHtmlEscaperToOutput extends AbstractRector { + + private $_phpFunctionsToIgnore = ['\count','\strip_tags']; + /** * @inheritDoc */ @@ -29,20 +32,37 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - // if the echo already has escapeHtml called on $block or escaper, don't do anthing $echoContent = $node->exprs[0]; - if ($echoContent instanceof Node\Expr\Variable || $echoContent instanceof Node\Expr\FuncCall) { - $node->exprs[0] = new Node\Expr\MethodCall( - new Node\Expr\Variable('escaper'), - 'escapeHtml', - [new Node\Arg($echoContent)] - ); + if ($echoContent instanceof Node\Expr\Ternary) { + + if (!$this->hasNoEscapeAttribute($echoContent->if) && $this->canEscapeOutput($echoContent->if)) { + $node->exprs[0]->if = new Node\Expr\MethodCall( + new Node\Expr\Variable('escaper'), + 'escapeHtml', + [new Node\Arg($echoContent->if)] + ); + + } + + if (!$this->hasNoEscapeAttribute($echoContent->else) && $this->canEscapeOutput($echoContent->else)) { + $node->exprs[0]->else = new Node\Expr\MethodCall( + new Node\Expr\Variable('escaper'), + 'escapeHtml', + [new Node\Arg($echoContent->else)] + ); + + } return $node; + } - if ($echoContent instanceof Node\Expr\MethodCall && $echoContent->name != 'escapeHtml') { + if ($this->hasNoEscapeAttribute($echoContent)) { + return null; + } + + if ($this->canEscapeOutput($echoContent)) { $node->exprs[0] = new Node\Expr\MethodCall( new Node\Expr\Variable('escaper'), @@ -55,6 +75,81 @@ public function refactor(Node $node): ?Node return null; } + private function canEscapeOutput(Node $echoContent):bool + { + if ($echoContent instanceof Node\Expr\Variable) { + return true; + } + + if ($echoContent instanceof Node\Expr\FuncCall + && !$this->willFunctionReturnSafeOutput($echoContent)) { + + // if string passed to __() contains html don't do anthing + if ($echoContent->name == '__' && $this->stringContainsHtml($echoContent->args[0]->value->value)) { + return false; + } + + return true; + } + + // if the echo already has escapeHtml called on $block or escaper, don't do anthing + if ($echoContent instanceof Node\Expr\MethodCall && + !$this->methodReturnsValidHtmlOrUrl($echoContent) + ) { + + // if the method is part of secureRenderer don't do anything + if ($echoContent->var->name === 'secureRenderer') { + return false; + } + + return true; + + } + + return false; + } + + private function stringContainsHtml(string $str) + { + return strlen($str) !== strlen(strip_tags($str)); + } + + /** + * If the developer has marked the output as noEscape by using the @noEscape + * we want to leave that code as it is + */ + private function hasNoEscapeAttribute(Node $echoContent):bool + { + + $comments = $echoContent->getComments(); + foreach ($comments as $comment) { + if (stripos($comment->getText(), '@noEscape') !== false) { + return true; + } + } + return false; + } + + /** + * If method contains the keyword HTML we assume developer intends to output html + */ + private function methodReturnsValidHtmlOrUrl(Node\Expr\MethodCall $echoContent):bool + { + return stripos($echoContent->name->name, 'Html') !== false + || stripos($echoContent->name->name, 'Url') !== false; + } + + /** + * Some php function return safe output. They need not be escaped. + * count, strip_tags are example + */ + private function willFunctionReturnSafeOutput(Node\Expr\FuncCall $funcNode):bool + { + //@TODO did not handle things like $callback(); + $funcName = $funcNode->name->toCodeString(); + return in_array($funcName, $this->_phpFunctionsToIgnore); + } + /** * @inheritDoc */ diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/complex-phtml-code-mix.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/complex-phtml-code-mix.php.inc new file mode 100644 index 00000000..28dd2cc2 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/complex-phtml-code-mix.php.inc @@ -0,0 +1,27 @@ +getAllItems(); +?> +
+ 0): ?> +
%1 items found', count($items)) ?>
+ + getName(); ?> + + + + +
+----- +getAllItems(); +?> +
+ 0): ?> +
%1 items found', count($items)) ?>
+ + escapeHtml($item->getName()); ?> + + + escapeHtml(__('No items found.')); ?> + +
diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/message-string-contains-html.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/message-string-contains-html.php.inc new file mode 100644 index 00000000..9335a5e1 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/message-string-contains-html.php.inc @@ -0,0 +1 @@ +
%1 items found',count($items))?>
diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/method-that-return-html.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/method-that-return-html.php.inc new file mode 100644 index 00000000..973e025f --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/method-that-return-html.php.inc @@ -0,0 +1,3 @@ +getChildHtml(); ?> +getToolBarHtml();?> +getChildBlock('toolbar')->setIsBottom(true)->toHtml() ?> diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-escaped-output-using-secure-renderer.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-escaped-output-using-secure-renderer.php.inc new file mode 100644 index 00000000..45db5f32 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-escaped-output-using-secure-renderer.php.inc @@ -0,0 +1,4 @@ +renderStyleAsTag( + $position, + 'product-item-info_' . $_product->getId() . ' div.actions-primary' +) ?> diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-output-of-count-function.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-output-of-count-function.php.inc new file mode 100644 index 00000000..7dd0e4c6 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-output-of-count-function.php.inc @@ -0,0 +1,3 @@ + + + diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-with-no-escape-annotation.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-with-no-escape-annotation.php.inc new file mode 100644 index 00000000..2696f4c3 --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/skip-with-no-escape-annotation.php.inc @@ -0,0 +1,3 @@ +getProductPrice($_product) ?> + + diff --git a/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/ternary-condition.php.inc b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/ternary-condition.php.inc new file mode 100644 index 00000000..20d7451d --- /dev/null +++ b/Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/ternary-condition.php.inc @@ -0,0 +1,11 @@ +canShowInfo() ? $escaper->escapeHtml($block->getInfo()) : __('Nothing to show'); + +?> +----- +canShowInfo() ? $escaper->escapeHtml($block->getInfo()) : $escaper->escapeHtml(__('Nothing to show')); + +?> From 5ddc4805a9311f6cde253dae8a386134b71f6e5f Mon Sep 17 00:00:00 2001 From: Abhishek Jakhotiya Date: Thu, 23 Nov 2023 00:20:43 +0530 Subject: [PATCH 4/4] Fixed phpcs sniffs w.r.t to docblocks and spacing --- .../Rector/Src/AddHtmlEscaperToOutput.php | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php index f2a01ed2..d58aed72 100644 --- a/Magento2/Rector/Src/AddHtmlEscaperToOutput.php +++ b/Magento2/Rector/Src/AddHtmlEscaperToOutput.php @@ -8,7 +8,6 @@ namespace Magento2\Rector\Src; use PhpParser\Node; -use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\Echo_; use Rector\Core\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; @@ -17,6 +16,9 @@ class AddHtmlEscaperToOutput extends AbstractRector { + /** + * @var string[] + */ private $_phpFunctionsToIgnore = ['\count','\strip_tags']; /** @@ -75,6 +77,12 @@ public function refactor(Node $node): ?Node return null; } + /** + * We check if content of the echo should be escaped + * + * @param Node $echoContent + * @return bool + */ private function canEscapeOutput(Node $echoContent):bool { if ($echoContent instanceof Node\Expr\Variable) { @@ -109,14 +117,21 @@ private function canEscapeOutput(Node $echoContent):bool return false; } + /** + * We do not want to escape __() output if the inner content contains html + * + * @param string $str + * @return bool + */ private function stringContainsHtml(string $str) { return strlen($str) !== strlen(strip_tags($str)); } /** - * If the developer has marked the output as noEscape by using the @noEscape - * we want to leave that code as it is + * If the developer has marked the output as noEscape by using the @noEscape we want to leave that code as it is + * + * @param Node $echoContent */ private function hasNoEscapeAttribute(Node $echoContent):bool { @@ -132,6 +147,8 @@ private function hasNoEscapeAttribute(Node $echoContent):bool /** * If method contains the keyword HTML we assume developer intends to output html + * + * @param Node\Expr\MethodCall $echoContent */ private function methodReturnsValidHtmlOrUrl(Node\Expr\MethodCall $echoContent):bool { @@ -140,8 +157,9 @@ private function methodReturnsValidHtmlOrUrl(Node\Expr\MethodCall $echoContent): } /** - * Some php function return safe output. They need not be escaped. - * count, strip_tags are example + * Some php function return safe output. count, strip_tags need not be escaped. + * + * @param Node\Expr\FuncCall $funcNode */ private function willFunctionReturnSafeOutput(Node\Expr\FuncCall $funcNode):bool {