Is false still allowed in allowedClasses attribute?

[KevinSJ]

I was upgrading our project dependencies to version with no vulnerabilities, one of the packages is this one. We have something in our codebase that can be simplified to the code below:

const sanitizeHtml = require('sanitize-html');

const ALLOWED_TAGS = [
    'li',
    'ul',
    'ol',
    'p',
    'h1',
    'h2',
    'h3',
    'h4',
    'h5',
    'h6',
    'img',
    'a',
    'div',
    'b',
    'table',
    'tbody',
    'tr',
    'th',
    'td'
];
const ALLOWED_SCHEMES = ['https', 'mailto'];

sanitizeHtml(`<ul class="should not sanitize"><li>Hello </li></ul>`, {
    allowedAttributes: { '*': ['*'] },
    allowedTags: ALLOWED_TAGS,
    allowedClasses: {
        ul: false
    },
    allowProtocolRelative: false,
    allowedSchemes: ALLOWED_SCHEMES
});

This code works in the 1.x version (1.27.5 in our case). However, once upgrade to 2.x, I got the following error.

This usage has not been documented, and I had to look through the code. My understanding is that ul:false in the allowedClasses attribute means class based filtering will not be done on this ul tag. In version 2.x, this usage can probably be replaced with ul: ['*']

This makes me wondering if ul:false is still valid in 2.x (or even 1.x)? If so, I can file a bug report as issue and create a PR. If not, the types package need to be updated to reflect this, I can create a separate PR there.

[boutell]

It looks like this feature was intentionally implemented, as the comment indicates, but never documented or covered by unit tests, with the unfortunate result you are seeing. That occurred around version 2.4.0.

A PR to add back support for this feature and add a unit test for it to prevent regressions would be welcome. This would involve checking that classes is not explicitly false before calling forEach on it.

I think it should be permitted only if the value is explicitly false.

[KevinSJ]

@boutell thanks for answering this. I'll take a look and create a PR to fix and add in some tests.