Did you ever wonder, what @escapeNotVerified is doing all over the place in Magento templates? I've often seen it blindly copied from core code. Don't do this!
Let me explain:
First, there is a good overview by Magento architect Alex Paliarush @paliarush on StackExchange: Magento 2 @escapeNotVerified
Historically, when Magento introduced a code sniffer rule to detect unescaped output in templates, they automatically added the @escapeNotVerified annotation to all unescaped output because it could not be handled all at once. The code sniffer ignores those.
They serve as a TODO and it was intended to go through them over time and either add the missing escaping or replace the annotation with @noEscape instead.
@noEscape means that escaping is omitted on purpose, e.g. because it is actually HTML, or it's definitely safe
Side note: there's not much that is "definitely safe". Let's take a look:
- Numbers, from a method with return type, or explicitly casted, like
(int) $x: Yes ✔️ - Numbers, from store configuration or block arguments: No 🚫
- Hard coded strings without HTML: Yes ✔️
- Translated strings: No 🚫
- JSON (e.g. from json_encode()), inside
<script>element: Yes ✔️ - JSON, inside HTML attribute: No 🚫
JSON in scripts is the only case where escaping actually does harm; in other cases, it just has nothing to do. So if in doubt, rather escape any output.
But use the right method!
No, not escapeUrl() for <a href"">. This one should be used for untrusted (user provided) URLs, it removes things like "javascript:"
To learn more about the other template escaping methods, please refer to Magento 2: template security: which method to use? (StackExchange)
And if you want to output actual HTML, the guidelines say it should come from a method with "Html" suffix, e.g. getButtonHtml() - the code sniffer does not complain about missing escaping in that case.
Modified Core Templates
A debatable case is when core templates are overridden in a custom theme to make modifications. At this point you take ownership of the code, so ideally you should review the unescaped output and take the appropriate action. But sometimes the templates are large and the modifications small. Fixing the original code may seem like a waste of time and then you lose the ability to diff the own template against the original one and easily see the modifications. So it is a trade-off and I often pragmatically let these cases pass during code reviews.
State of Core Magento
Now, I was curious about the current state, since the statement is more than three years old now:
”In the future releases all occurrences of @escapeNotVerified will be verified and either marked with @noEscape or escaped with one of these methods:”
So I counted the occurrences in every release since 2.1.0 and plotted them over time. You'll notice that 2.2.0 and 2.1.9 in September 2017 were the last releases with significant reduce.
In 2.3.0 it actually went UP again, compared to 2.2.7, released on the same date
Why this? Did Magento lose interest in the rather boring, but important, verification task?
But there is good news: Magento works hard on a new code sniffer that will be used for core and extensions (magento/magento-coding-standard) and from what I've heard, new rules will only be applied to new code, so that something like the introduction of @escapeNotVerified will never happen again.
Now this is a chance: If we can remove the exception from the code sniffer rule, and only allow "noEscape", it will
- force anybody working on core templates to verify unverified output
- prevent further spreading of "escapeNotVerified" in marketplace extensions
Actually, the new code sniffer already warns about disallowed annotations for anything other than @noEscape (Source: XssTemplateSniff)
But Lena Orobei of the Magento Marketplace Extension Quality Program pointed out that in the core there are currently two tools that check missing escaping in templates. Not only the code sniffer, but also a static test, implemented in PHPUnit, XssOutputValidator, which still allows @escapeNotVerified.
The good thing about this additional tool is that it does not respect @codingStandardsIgnoreFile annotations, which are often found in templates. A potential disadvantage is that it may often not be part of the QA tooling for implementation projects or third party extensions.
The code sniffer rule may be dropped for that reason (see issue on GitHub), so as of January 2019 and Magento 2.3.0, all is still in motion.
Summary
Never use @escapeNotVerified, it was a temporary workaround for core code and there are efforts to remove it.
Instead, use the appropiate escaping methods for the context (e.g. HTML element or HTML attribute), or fall back to @noEscape if the output is supposed to be HTML and cannot be retrieved with a method that ends with "Html".