Repository: apostrophecms/sanitize-html (js)

Issue:
Escaping input with unclosed, invalid tags returns it with unneeded escaped closing tags appended to end
## 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



Pull request:
Fix auto-adding escaped closing tags
<!-- Please don't delete this template -->

<!-- PULL REQUEST TEMPLATE -->
<!-- (Update "[ ]" to "[x]" to check a box) -->

## Summary

Closes #464 
Adds `tagAllowed()` helper function.

## What are the specific steps to test this change?

Existing tests may need to be reviewed. I don't do a lot of PRs, sorry.

## What kind of change does this PR introduce?
*(Check at least one)*

- [x] Bug fix
- [ ] New feature
- [x] Refactor
- [ ] Documentation
- [ ] Build-related changes
- [ ] Other

## Make sure the PR fulfills these requirements:

- [x] It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
- [ ] The changelog is updated
- [ ] Related documentation has been updated
- [ ] Related tests have been updated

If adding a new feature without an already open issue, it's best to open a **feature request issue** first and wait for approval before working on it.

**Other information:**


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2e49d2e..2f1f8b4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## UNRELEASED (2023-01-31)
+
+- Fix auto-adding escaped closing tags. In other words, do not add implied closing tags to disallowed tags when `disallowedTagMode` is set to any variant of `escape` -- just escape the disallowed tags that are present. This fixes [issue #464](https://github.com/apostrophecms/sanitize-html/issues/464). Thanks to [Daniel Liebner](https://github.com/dliebner)
+- Add `tagAllowed()` helper function which takes a tag name and checks it against `options.allowedTags` and returns `true` if the tag is allowed and `false` if it is not.
+
 ## 2.9.0 (2023-01-27)
 
 - Add option parseStyleAttributes to skip style parsing. This fixes [issue #547](https://github.com/apostrophecms/sanitize-html/issues/547). Thanks to [Bert Verhelst](https://github.com/bertyhell).
diff --git a/index.js b/index.js
index c02907c..4363267 100644
--- a/index.js
+++ b/index.js
@@ -117,12 +117,13 @@ function sanitizeHtml(html, options, _recursing) {
   options = Object.assign({}, sanitizeHtml.defaults, options);
   options.parser = Object.assign({}, htmlParserDefaults, options.parser);
 
+  const tagAllowed = function (name) {
+    return options.allowedTags === false || (options.allowedTags || []).indexOf(name) > -1;
+  };
+
   // vulnerableTags
   vulnerableTags.forEach(function (tag) {
-    if (
-      options.allowedTags !== false && (options.allowedTags || []).indexOf(tag) > -1 &&
-      !options.allowVulnerableTags
-    ) {
+    if (tagAllowed(tag) && !options.allowVulnerableTags) {
       console.warn(`\n\n⚠️ Your \`allowedTags\` option includes, \`${tag}\`, which is inherently\nvulnerable to XSS attacks. Please remove it from \`allowedTags\`.\nOr, to disable this warning, add the \`allowVulnerableTags\` option\nand ensure you are accounting for this risk.\n\n`);
     }
   });
@@ -254,7 +255,7 @@ function sanitizeHtml(html, options, _recursing) {
         }
       }
 
-      if ((options.allowedTags !== false && (options.allowedTags || []).indexOf(name) === -1) || (options.disallowedTagsMode === 'recursiveEscape' && !isEmptyObject(skipMap)) || (options.nestingLimit != null && depth >= options.nestingLimit)) {
+      if (!tagAllowed(name) || (options.disallowedTagsMode === 'recursiveEscape' && !isEmptyObject(skipMap)) || (options.nestingLimit != null && depth >= options.nestingLimit)) {
         skip = true;
         skipMap[depth] = true;
         if (options.disallowedTagsMode === 'discard') {
@@ -513,7 +514,7 @@ function sanitizeHtml(html, options, _recursing) {
         frame.text += text;
       }
     },
-    onclosetag: function(name) {
+    onclosetag: function(name, isImplied) {
 
       if (skipText) {
         skipTextDepth--;
@@ -563,8 +564,12 @@ function sanitizeHtml(html, options, _recursing) {
       frame.updateParentNodeMediaChildren();
       frame.updateParentNodeText();
 
-      if (options.selfClosing.indexOf(name) !== -1) {
+      if (
         // Already output />
+        options.selfClosing.indexOf(name) !== -1 ||
+        // Escaped tag, closing tag is implied
+        (isImplied && !tagAllowed(name) && [ 'escape', 'recursiveEscape' ].indexOf(options.disallowedTagsMode) >= 0)
+      ) {
         if (skip) {
           result = tempResult;
           tempResult = '';


Test patch:
diff --git a/test/test.js b/test/test.js
index 88ecbea..92e91aa 100644
--- a/test/test.js
+++ b/test/test.js
@@ -1561,4 +1561,19 @@ describe('sanitizeHtml', function() {
       }), '<script src="//example.com/script.js"></script>'
     );
   });
+  it('should not automatically attach close tag for escaped tags in escape mode', function() {
+    assert.equal(sanitizeHtml('<test>Hello', {
+      disallowedTagsMode: 'escape'
+    }), '&lt;test&gt;Hello');
+  });
+  it('should not automatically attach close tag for escaped tags in recursiveEscape mode', function() {
+    assert.equal(sanitizeHtml('<test><test><test><test><test>Hello', {
+      disallowedTagsMode: 'recursiveEscape'
+    }), '&lt;test&gt;&lt;test&gt;&lt;test&gt;&lt;test&gt;&lt;test&gt;Hello');
+  });
+  it('should discard unclosed disallowed tags', function() {
+    assert.equal(sanitizeHtml('<test>Hello', {
+      disallowedTagsMode: 'discard'
+    }), 'Hello');
+  });
 });