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

Stricter Escape Checks #490

Open
Morgy93 opened this issue Feb 11, 2025 · 2 comments
Open

Stricter Escape Checks #490

Morgy93 opened this issue Feb 11, 2025 · 2 comments
Labels
enhancement Improvements to existing rules Progress: ready for grooming

Comments

@Morgy93
Copy link
Member

Morgy93 commented Feb 11, 2025

Description

Currently, there is no validation to ensure that the correct escaping methods are used in the right context. This allows incorrect or inconsistent escaping, leading to potential security vulnerabilities.

For example, the following incorrect usages are currently not flagged:

<!-- Incorrect usage: escapeHtml() used inside an attribute -->
<div attr="<?= $escaper->escapeHtml('value') ?>"><?= $escaper->escapeHtmlAttr('text') ?></div>

<!-- Incorrect usage: escapeHtmlAttr() used for a URL -->
<a href="<?= $escaper->escapeHtmlAttr('https://example.com?param=value') ?>">Link</a>

<!-- Incorrect usage: escapeHtml() used inside JavaScript -->
<script>var msg = '<?= $escaper->escapeHtml("alert('XSS')") ?>';</script>

Correct Usage:

<!-- Proper escaping for HTML content and attributes -->
<div attr="<?= $escaper->escapeHtmlAttr('safe-value') ?>"><?= $escaper->escapeHtml('Safe Text') ?></div>

<!-- Proper escaping for URLs -->
<a href="<?= $escaper->escapeUrl('https://example.com?param=value') ?>">Link</a>

<!-- Proper escaping for JavaScript -->
<script>var msg = '<?= $escaper->escapeJs("alert('XSS')") ?>';</script>

Expected Behavior

  • The Magento Coding Standard should flag incorrect usage of escaping methods.
  • It should recommend the appropriate escaping function based on the context:
    • escapeHtml() → for content inside HTML tags.
    • escapeHtmlAttr() → for attribute values.
    • escapeUrl() → for URLs inside <a href="">, <form action="">, etc.
    • escapeJs() → for escaping JavaScript content inside <script> tags or inline JS handlers (onclick, onmouseover, etc.).
  • Developers should be alerted when incorrect escaping is used.

Benefits

  • Improves security by reducing the risk of XSS vulnerabilities caused by improper escaping.
  • Encourages best practices for secure and consistent code.
  • Enhances code quality by enforcing correct escaping usage.
@Morgy93 Morgy93 added the enhancement Improvements to existing rules label Feb 11, 2025
@magento-ct-gh-projects-svc magento-ct-gh-projects-svc moved this to Ready for Grooming in Backlog Feb 11, 2025
Copy link

m2-assistant bot commented Feb 11, 2025

Hi @Morgy93. Thank you for your report.
To speed up processing of this issue, make sure that you provided sufficient information.
Add a comment to assign the issue: @magento I am working on this


Join Magento Community Engineering Slack and ask your questions in #github channel.

@hostep
Copy link
Contributor

hostep commented Feb 12, 2025

Would love to see this!

Especially because Magento 2.3.x code is in a much better shape then 2.4.x code in this regards, because they forgot to forward-port magento/magento2@7caf492 from 2.3 to 2.4 branch many years ago (I've already reported it multiple times to their internal devs and security team, but they keep saying it's not important and they don't have time to fix it).

Having coding standards check better against correct usage would solve a lot of wrong usages each time a template is touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing rules Progress: ready for grooming
Projects
Status: Ready for Grooming
Development

No branches or pull requests

3 participants