Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rector rule added to escape unsafe output in phtml files #472

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Jakhotiya
Copy link

Summary

Often a codebase is handed over to you which does not follow all magento standards and phpcs starts flagging them. One such example is unsafe output in template files. For a large number of files, its cumbersome to refactor these files.
Rector rule can do this quickly. This pull requests addresses that.

Checks done.

Added test cases for the rector rule.
Ran it on actual template files

Things not covered

This does not yet implement escapeJs and escapeUrl functions. Should be able to add it soon. This PR should also help people to get started with writing such rules.

…d 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
@Jakhotiya Jakhotiya changed the title Rector rule to add escape unsafe output in phtml files Rector rule added to escape unsafe output in phtml files Nov 15, 2023
@fredden
Copy link
Member

fredden commented Nov 16, 2023

I like the idea of auto-fixing this category, however I don't have confidence that this won't break things. Please can you add tests covering the following cases:

<?php
print $var;
print($var);
print_r($var);
printf('%s', $var);
?>
<?= $var; ?>
<input value="<?= $var ?>">
<input value="<?php echo $var; ?>">
<?= /* @noEscape */ $var ?>
<a href="<?= $escaper->escapeUrl($var) ?>">
    <?= count($var) ?>
</a>
<span>PHP constant: <?= PHP_VERSION ?></span>
<?= 'hard-coded string' ?>
<?= "hard-coded string" ?>
<?= "String with interpolated $var" ?>
<span>Cast to "safe" type: <?= (int) $var ?></span>
<?= $block->getChildHtml() ?>

I can think of some more, but these should give better coverage and highlight some things that are likely to break. See Magento2/Sniffs/Security/XssTemplateSniff.php for what's considered safe / unsafe within this standard.

@Jakhotiya
Copy link
Author

Thank you Dan for feedback. Let me add those tests. I see your concern. Espcially when we do something like

<?= $block->getChildHtml() ?>

OR

<?php
$productCard = $block->getLayout()->getBlock();
$productCard->setProduct($product)
?>
<?= $productCard->toHtml() ?>

Predominatly things will break if we escape valid HTML itself. This espcially the case when Renderer blocks are used.
And looking at vendor/magento/module-catalog/view/frontend/templates/product/list.phtml I don't see an easy way out.

A naive way is to ignore all methods with "Html" in them, Any suggestions on this @fredden ?

Abhishek Jakhotiya added 2 commits November 23, 2023 00:03
…ent.

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.
@Jakhotiya
Copy link
Author

Jakhotiya commented Nov 23, 2023

@fredden I've escaped code only when echo statements are used

  1. it's just a variable
  2. when method name does not contain "Url" or "Html"
  3. when @NoEscape is not used
  4. When __() contains string that does not contain html

Have tried to do it defensively. This does not intend to fix all escaping issues. Only where we are confident.

Risky case:

<?php

$list = '';

foreach($block->getItems() as $item){
    $list .= '<li>'.$item->getName().'</li>';
}
?>
<ul><?= $list; ?></ul>

I've added a failing test on my local but haven't fixed it.

The way I'm using this is by using dry-run to be safe

<?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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't $block->getActionUrl($item); be wrapped to escapeUrl? or escapeHtmlAttr (because it's part of attribute)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

3 participants