String#unescapeHTML() decodes entities after stripping tags, reintroducing markup

[knogineer]

String#unescapeHTML() calls stripTags() first and then decodes entities. Because the decode runs after the strip, encoded markup that survives stripping (since it is not a real tag at that point) gets turned back into live markup. Any code that assumes the output is tag-free will be wrong.

Current implementation (around line 439 of src/prototype/lang/string.js):

function unescapeHTML() {
  return this.stripTags().replace(/&lt;/g,'<').replace(/&gt;/g,'>').replace(/&amp;/g,'&');
}

Reproduction:

'<img src=x onerror=alert(1)>'.unescapeHTML();
// stripTags() leaves the entity text alone (there is no real tag yet),
// then the decode step produces a live tag:
// => '<img src=x onerror=alert(1)>'

If a developer relies on unescapeHTML() to produce safe, tag-free text before inserting it into the page, the decode step reintroduces executable markup, which is a path to XSS.

Suggested fix: decode entities first and then strip, or use a single normalization pass that does not leave decoded markup behind. It would also help to document that the result is not safe to insert into the DOM as HTML.

Refs: CWE-79, CWE-116, OWASP ASVS V5.3.3.

[knogineer]

Adding some cross-references after going through the existing open issues, so this is not read in isolation:

• This sits in the same area as CVE-2020-27511, which already covers String#stripTags / String#unescapeHTML (see also CVE-2020-27511 #355 and OWASP dependency-check (Retire.js) flags every fork of 1.7.3 for CVE-2020-27511, even patched ones — request a tagged 1.7.4 / version bump #368). I am not claiming a new CVE here.
• unescapeHTML() is casesensetive #251 covers a different aspect of the same method (entity case sensitivity, < vs <).

The specific point I am raising is narrower and, as far as I can tell, not captured by either of those: the order of operations. unescapeHTML() strips tags first and then decodes entities, so encoded markup that is not yet a tag at strip time gets decoded back into live markup afterwards. Example:

'<img src=x onerror=alert(1)>'.unescapeHTML();
// stripTags() leaves the entity text alone (no real tag yet),
// then the decode step yields a live tag:
// => '<img src=x onerror=alert(1)>'

So anything that treats the output of unescapeHTML() as tag-free before inserting it into the DOM can end up reintroducing executable markup. Decoding first and then stripping (or a single normalization pass) would avoid this. Either way it would help to document that the result is not safe to insert as HTML.