Merge pull request #607 from dliebner/main

Fix auto-adding escaped closing tags
apostrophecms · Feb 2, 2023 · d5fbdc6 · d5fbdc6
2 parents f12a665 + 6ea35ad
commit d5fbdc6
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 7 deletions.
diff --git 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
@@ -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 = '';

diff --git 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');
+  });
 });