Rector rule added to escape unsafe output in phtml files

Magento2/Rector/Src/AddHtmlEscaperToOutput.php

@@ -0,0 +1,186 @@
+<?php
+/**
+ * Copyright 2021 Adobe
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+namespace Magento2\Rector\Src;
+
+use PhpParser\Node;
+use PhpParser\Node\Stmt\Echo_;
+use Rector\Core\Rector\AbstractRector;
+use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
+use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
+
+class AddHtmlEscaperToOutput extends AbstractRector
+{
+
+    /**
+     * @var string[]
+     */
+    private $_phpFunctionsToIgnore = ['\count','\strip_tags'];
+
+    /**
+     * @inheritDoc
+     */
+    public function getNodeTypes(): array
+    {
+        return [Echo_::class];
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function refactor(Node $node): ?Node
+    {
+
+        $echoContent  = $node->exprs[0];
+
+        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 ($this->hasNoEscapeAttribute($echoContent)) {
+            return null;
+        }
+
+        if ($this->canEscapeOutput($echoContent)) {
+
+            $node->exprs[0] = new Node\Expr\MethodCall(
+                new Node\Expr\Variable('escaper'),
+                'escapeHtml',
+                [new Node\Arg($echoContent)]
+            );
+            return $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) {
+            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;
+    }
+
+    /**
+     * 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
+     *
+     * @param Node $echoContent
+     */
+    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
+     *
+     * @param Node\Expr\MethodCall $echoContent
+     */
+    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. count, strip_tags need not be escaped.
+     *
+     * @param Node\Expr\FuncCall $funcNode
+     */
+    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
+     */
+    public function getRuleDefinition(): RuleDefinition
+    {
+        return new RuleDefinition(
+            'Add escaper methods like escapeHtml to html output',
+            [
+                new CodeSample(
+                    'echo $productName',
+                    'echo $escaper->escapeHtml($productName)'
+                ),
+            ]
+        );
+    }
+}

Magento2/Rector/Tests/AddHtmlEscaperToOutput/AddHtmlEscaperToOutputTest.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * Copyright 2021 Adobe
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+namespace Magento2\Rector\Tests\AddHtmlEscaperToOutput;
+
+use Iterator;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+use Symplify\SmartFileSystem\SmartFileInfo;
+
+class AddHtmlEscaperToOutputTest extends AbstractRectorTestCase
+{
+    /**
+     * @dataProvider provideData()
+     */
+    public function test(string $fileInfo): void
+    {
+        $this->doTestFile($fileInfo);
+    }
+
+    /**
+     * @return Iterator<SmartFileInfo>
+     */
+    public function provideData(): Iterator
+    {
+        return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}

Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/already-escaped-ouput.php.inc

@@ -0,0 +1,11 @@
+<?php
+
+echo $escaper->escapeHtml($productName);
+
+?>
+-----
+<?php
+
+echo $escaper->escapeHtml($productName);
+
+?>

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

@@ -0,0 +1,27 @@
+<?php
+$items = $block->getAllItems();
+?>
+<div class="test">
+    <?php if (count($items) > 0): ?>
+        <div class="counter"><?= __('Total <span>%1</span> items found', count($items)) ?></div>
+        <?php foreach ($items as $item): ?>
+            <a href="<?= $block->getActionUrl($item); ?>"><?= $item->getName(); ?></a>
+        <?php endforeach; ?>
+    <?php else: ?>
+        <?= __('No items found.'); ?>
+    <?php endif; ?>
+</div>
+-----
+<?php
+$items = $block->getAllItems();
+?>
+<div class="test">
+    <?php if (count($items) > 0): ?>
+        <div class="counter"><?= __('Total <span>%1</span> items found', count($items)) ?></div>
+        <?php foreach ($items as $item): ?>
+            <a href="<?= $block->getActionUrl($item); ?>"><?= $escaper->escapeHtml($item->getName()); ?></a>
+        <?php endforeach; ?>
+    <?php else: ?>
+        <?= $escaper->escapeHtml(__('No items found.')); ?>
+    <?php endif; ?>
+</div>

Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/function-call.php.inc

@@ -0,0 +1,11 @@
+<?php
+
+echo $customer->getName();
+echo __('Something');
+?>
+-----
+<?php
+
+echo $escaper->escapeHtml($customer->getName());
+echo $escaper->escapeHtml(__('Something'));
+?>

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

@@ -0,0 +1 @@
+<div class="counter"><?= __('Total <span>%1</span> items found',count($items))?></div>

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

@@ -0,0 +1,3 @@
+<?=  $block->getChildHtml(); ?>
+<?= $block->getToolBarHtml();?>
+<?= $block->getChildBlock('toolbar')->setIsBottom(true)->toHtml() ?>

Magento2/Rector/Tests/AddHtmlEscaperToOutput/Fixture/simple-echo.php.inc

@@ -0,0 +1,11 @@
+<?php
+
+echo $productName;
+
+?>
+-----
+<?php
+
+echo $escaper->escapeHtml($productName);
+
+?>

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

@@ -0,0 +1,4 @@
+<?=  $secureRenderer->renderStyleAsTag(
+    $position,
+    'product-item-info_' . $_product->getId() . ' div.actions-primary'
+) ?>

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

@@ -0,0 +1,3 @@
+<?= count($items); ?>
+<?= (int)$items ?>
+<?= (bool)$items ?>

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

@@ -0,0 +1,3 @@
+<?= /* @noEscape */ $block->getProductPrice($_product) ?>
+<?= /* @noEscape */ $price ?>
+<?= /* @noEscape */ trim($price) ?>

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

@@ -0,0 +1,11 @@
+<?php
+
+echo $block->canShowInfo() ?  $escaper->escapeHtml($block->getInfo()) : __('Nothing to show');
+
+?>
+-----
+<?php
+
+echo $block->canShowInfo() ?  $escaper->escapeHtml($block->getInfo()) : $escaper->escapeHtml(__('Nothing to show'));
+
+?>

Magento2/Rector/Tests/AddHtmlEscaperToOutput/config/configured_rule.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * Copyright 2022 Adobe
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+use Magento2\Rector\Src\AddArrayAccessInterfaceReturnTypes;
+use Rector\Config\RectorConfig;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rule(\Magento2\Rector\Src\AddHtmlEscaperToOutput::class);
+};