Escaping input with unclosed, invalid tags returns it with unneeded escaped closing tags appended to end

[lostfictions]

To Reproduce

From the Node REPL:

> const s = require('sanitize-html')
undefined
> s("here's a string with a <wacky> tag.", {disallowedTagsMode: "escape"})
"here's a string with a &lt;wacky&gt; tag.&lt;/wacky&gt;"

Expected behavior

The offending tag is escaped, but no addition markup is created. For example, given the input here's a string with a <wacky> tag, I would expect the output to be: here's a string with a <wacky> tag

Describe the bug

When sanitizing a string with disallowedTagsMode: "escape", extra, unneeded (escaped) closing tags are appended to the end of the string.

Details

Version of Node.js: 15.5.0
Server Operating System: Ubuntu 20.04

[boutell]

There's a pretty small number of tags that are known to not require a closure. If it's not one of those, sanitize-html will close it to ensure valid markup. However, you can expand this list via the "selfClosing" option, which is included in the documentation. Bear in mind that if you set this option you must set it to the complete list, not just your addition.

…

On Sat, Feb 13, 2021 at 7:34 AM s ***@***.***> wrote: To Reproduce From the Node REPL: > const s = require('sanitize-html') undefined > s("here's a string with a <wacky> tag.", {disallowedTagsMode: "escape"}) "here's a string with a &lt;wacky&gt; tag.&lt;/wacky&gt;" Expected behavior The offending tag is escaped, but no addition markup is created. For example, given the input here's a string with a <wacky> tag, I would expect the output to be: here's a string with a &lt;wacky&gt; tag Describe the bug When sanitizing a string with disallowedTagsMode: "escape", extra, unneeded (escaped) closing tags are appended to the end of the string. Details *Version of Node.js:* 15.5.0 *Server Operating System:* Ubuntu 20.04 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#464>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27JBSMZVZDIVJZW43LLS6ZWVJANCNFSM4XSDH5CA> .

-- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

[lostfictions]

Thanks a lot for the reply! In the specific case where we're performing escaping rather than outright removal of tags that aren't allowed, I still believe this behaviour is incorrect -- closing these tags isn't necessary, and in effect the result is "garbage" in the form of escaped closing tags being appended to the output and visible to the user. Here's another motivating example: imagine I have a fragment like this I want to sanitize by escaping:

Here are some <b>operators</b>: =equals<lessthan>greaterthan-minus+plus

Obviously this is badly or erroneously formatted input on the part of a user, but you might imagine situations like this might arise. Assuming I'm preserving tags like b, I'd hope for output like this:

Here are some <b>operators</b>: =equals&lt;lessthan&gt;greaterthan-minus+plus

That is, recognized tags are preserved, and unrecognized tags are simply escaped with no regard for whether they are balanced (since once they're escaped they're not really in the stack and have no semantic significance anymore anyway). Does that make sense?

[boutell]

Yes, I see your point.

…

On Sat, Feb 13, 2021, 5:48 PM s ***@***.***> wrote: Thanks a lot for the reply! In the specific case where we're performing escaping rather than outright removal, I still believe this behaviour is incorrect -- closing these tags isn't necessary. Here's another motivating example: imagine I have a fragment like this I want to sanitize by escaping: Here are some <b>operators</b>=equals<lessthan>greaterthan-minus+plus Obviously this is badly or erroneously formatted input on the part of a user, but you might imagine situations like this might arise. Assuming I'm preserving tags like b, I'd hope for output like this: Here are some <b>operators</b>=equals&lt;lessthan&gt;greaterthan-minus+plus That is, recognized tags are preserved, and unrecognized tags are simply escaped with no regard for whether they are balanced (since once they're escaped they're not really in the stack and have no semantic significance anymore anyway). Does that make sense? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#464 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27L7ZCB5OQSMWVZW7ETS636VFANCNFSM4XSDH5CA> .

[dev-boku]

@boutell Hello, I am curious about the current progress on this issue.

[abea]

I don't think there's anything to report right now, @potter7050. Our team continues to be focused on the next major version of ApostropheCMS.

I did look at this for a bit, though. It is complicated by the fact that htmlparser2, which sanitize-html depends on, automatically tries to close any opened tags (as long as they aren't self-closing). So our onCloseTag handler is given the non-existent tag (e.g., <wacky>) even though it's a non-standard tag with no closing tag.

A solution would need to either tell htmlparser2 to skip the closing tag (I don't think there's an option for this) or we track that the tag should be skipped and simply return at the beginning of that onCloseTag handler.

[dev-boku]

@abea I suggest this pr. please check this.
#469

[abea]

Thanks, @potter7050. We'll review it soon.

[sunnymsf]

Hi @abea

Any plans to work on this soon?

[abea]

@sunnymsf No active plans. I provided feedback on the PR #469 that could help someone move this forward. Based on my previous review, htmlparser2 might make this hard to resolve fully. We'd welcome a community contribution for this.

[sunnymsf]

@abea thanks for the update.

[Iwark]

I met the same problem, and here is my workaround:

sanitizeHtml(text, {disallowedTagsMode: 'escape'}).replace(/<\/.*?>/g, '')

It's ugly but works for my case.