Fix Escaper::escapeHtml dropping content after a stray "<" when allowed tags are passed (#37693) - #40976
Conversation
…ed tags are passed (magento#37693) The allowed-tags path sends input through DOMDocument::loadHTML(), where a stray less-than can trigger the error handler and leave the parsed DOM truncated. Escape less-than characters that are not valid tag starts before loadHTML while preserving real tags, closing tags, and comments. Fixes magento#37693
|
Hi @lbajsarowicz. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com. |
|
@magento run all tests |
|
@magento run all tests |
Description (*)
When
Escaper::escapeHtml()is called with a non-empty$allowedTagsarray, the input is routed throughDOMDocument::loadHTML(). That branch installs a customset_error_handlerthat converts any libxml parse warning into a thrown\InvalidArgumentException.If the input contains a stray
<that is not the start of a valid tag — e.g.Speed < 10m/s— libxml emitshtmlParseStartTag: invalid element name, the handler throws,loadHTML()aborts mid-parse leaving a truncated DOM (only the text before the stray<), the exception is caught and logged asCRITICAL, and the truncated content is returned. SoescapeHtml('Speed < 10m/s', ['b'])returnsSpeedand spamsexception.log.Without allowed tags the same input is handled safely by
htmlspecialchars(), so the bug is specific to the allowed-tags path.Fix: in
prepareUnescapedCharacters()(which already escapes&→&beforeloadHTML()), also escape<characters that are not the start of a valid tag — i.e. not immediately followed by an ASCII letter,/, or!— into<. Real/allowed tags (<b>,</b>), and comments (<!--) are untouched; only stray<become entities, whichloadHTML()parses cleanly as text. Replacement order (&first, then<) is preserved so the&inside a produced<is not double-escaped.Security impact
This change only adds escaping (stray
<→<) and never relaxes it. Sequences that begin a real tag are unchanged and still flow through the existing allowed/prohibited-tag filtering, so no tag that was previously stripped can now pass, and no new XSS vector is introduced. Net effect on the security posture is neutral-to-positive.Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
.phtml(or via a unit call) render:Speedand aCRITICAL InvalidArgumentException: DOMDocument::loadHTML(): htmlParseStartTag ...appears invar/log/exception.log.Speed < 10m/s(renders asSpeed < 10m/s), no exception logged.escapeHtml('<b>Bold</b> Speed < 10m/s', ['b'])→<b>Bold</b> Speed < 10m/s.Contribution checklist (*)