Repository: apostrophecms/sanitize-html (js)

Issue:
{allowedTags:null} allows <script>
```js
'<script>alert(1)</script>' ==
sanitizeHtml(
    '<script>alert(1)</script>',
    { allowedTags: null });

'<script>alert(1)</script>' ==
sanitizeHtml(
    '<script>alert(1)</script>',
    { allowedTags: undefined });
```

The [docs](https://github.com/punkave/sanitize-html#what-if-i-want-to-allow-all-tags-or-all-attributes) say

> ### "What if I want to allow all tags or all attributes?"
>
> Simple! instead of leaving allowedTags or allowedAttributes out of the options, set either one or both to false:
>
> ```js
> allowedTags: false,
> allowedAttributes: false
> ```

The internal check checks whether `allowedTags` is falsey, not false.

Treating `null` equivalently to false is problematic since `null` is
much more likely as an output from a function that otherwise
returns an array than `false`, so treating `null` and `undefined`
as equivalent to `false` is a corner-case with very serious security consequences.

For example,

```js
const MY_POLICY = {
  allowedTags: computeAllowedTags()
};

function computeAllowedTags() {
  if (complexCondition) {
    return INLINE_ELEMENTS;
  } else if (anotherComplexCondition) {
    return BLOCK_AND_INLINE_ELEMENTS;
  } else if (adNauseam) {
    return FORMATTING_ELEMENTS_AND_IMAGES;
  }
  // NOTE: Missing return at bottom implies return of undefined
}
```

Since the behavior for `undefined` and `null`, `0`, `NaN`, `""` and other falsey values is not documented, I recommend either

* changing the code that fils in blanks:
  ```js
  options = extend(sanitizeHtml.defaults, options);
  ```
  to first remove any properties with falsey, but non-false values.
* and/or change the falsey checks
  ```js
  if (options.allowedTags && options.allowedTags.indexOf(name) === -1) {
  ```
  to check for `false`:
  ```js
  if (options.allowedTags !== false
      && (options.allowedTags || []).indexOf(name) === -1) {
  ```
  and similarly for `allowedAttributes`.

Pull request description:
For allowedTags, stop treating falsy values other than false, same as false.
<!-- Please don't delete this template -->

<!-- PULL REQUEST TEMPLATE -->
<!-- (Update "[ ]" to "[x]" to check a box) -->

## Summary

Closes #176 

## What are the specific steps to test this change?
Run the following example snippet.
```
sanitizeHtml('<div><wiggly worms="ewww">hello</wiggly></div>', {
      allowedTags: undefined
    })
```

Before this change, we used to get the following output, ie. all tags allowed:
`'<div><wiggly worms="ewww">hello</wiggly></div>'`

After this change, we will get the following output, ie. no tag allowed:
`'hello'`

I have added new test cases to check the behaviour needed.

## What kind of change does this PR introduce?
*(Check at least one)*

- [x] Bug fix
- [ ] New feature
- [ ] 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
- [x] The changelog is updated
- [x] Related documentation has been updated
- [x] 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 9fae24b..5c5afc3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- If allowedTags is falsy but not exactly `false`, then do not assume that all tags are allowed. Rather, allow no tags in this case, to be on a safer side. This fixes [issue #176](https://github.com/apostrophecms/sanitize-html/issues/176).
+
 ## 2.7.2 (2022-09-15)
 
 - Closing tags must agree with opening tags. This fixes [issue #549](https://github.com/apostrophecms/sanitize-html/issues/549), in which closing tags not associated with any permitted opening tag could be passed through. No known exploit exists, but it's better not to permit this. Thanks to 
diff --git a/index.js b/index.js
index ee27a3c..ed152ad 100644
--- a/index.js
+++ b/index.js
@@ -117,7 +117,7 @@ function sanitizeHtml(html, options, _recursing) {
   // vulnerableTags
   vulnerableTags.forEach(function (tag) {
     if (
-      options.allowedTags && options.allowedTags.indexOf(tag) > -1 &&
+      options.allowedTags !== false && (options.allowedTags || []).indexOf(tag) > -1 &&
       !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`);
@@ -251,7 +251,7 @@ function sanitizeHtml(html, options, _recursing) {
         }
       }
 
-      if ((options.allowedTags && options.allowedTags.indexOf(name) === -1) || (options.disallowedTagsMode === 'recursiveEscape' && !isEmptyObject(skipMap)) || (options.nestingLimit != null && depth >= options.nestingLimit)) {
+      if ((options.allowedTags !== false && (options.allowedTags || []).indexOf(name) === -1) || (options.disallowedTagsMode === 'recursiveEscape' && !isEmptyObject(skipMap)) || (options.nestingLimit != null && depth >= options.nestingLimit)) {
         skip = true;
         skipMap[depth] = true;
         if (options.disallowedTagsMode === 'discard') {


Test patch:
diff --git a/test/test.js b/test/test.js
index 283b2b5..8d08c6a 100644
--- a/test/test.js
+++ b/test/test.js
@@ -40,6 +40,26 @@ describe('sanitizeHtml', function() {
       allowedAttributes: false
     }), '<div><wiggly worms="ewww">hello</wiggly></div>');
   });
+  it('should not pass through any markup if allowedTags is set to undefined (falsy but not exactly false)', function() {
+    assert.equal(sanitizeHtml('<div><wiggly worms="ewww">hello</wiggly></div>', {
+      allowedTags: undefined
+    }), 'hello');
+  });
+  it('should not pass through any markup if allowedTags is set to 0 (falsy but not exactly false)', function() {
+    assert.equal(sanitizeHtml('<div><wiggly worms="ewww">hello</wiggly></div>', {
+      allowedTags: 0
+    }), 'hello');
+  });
+  it('should not pass through any markup if allowedTags is set to null (falsy but not exactly false)', function() {
+    assert.equal(sanitizeHtml('<div><wiggly worms="ewww">hello</wiggly></div>', {
+      allowedTags: null
+    }), 'hello');
+  });
+  it('should not pass through any markup if allowedTags is set to empty string (falsy but not exactly false)', function() {
+    assert.equal(sanitizeHtml('<div><wiggly worms="ewww">hello</wiggly></div>', {
+      allowedTags: ''
+    }), 'hello');
+  });
   it('should respect text nodes at top level', function() {
     assert.equal(sanitizeHtml('Blah blah blah<p>Whee!</p>'), 'Blah blah blah<p>Whee!</p>');
   });