apostrophecms / sanitize-html

Clean up user-submitted HTML, preserving whitelisted elements and whitelisted attributes on a per-element basis. Built on htmlparser2 for speed and tolerance
MIT License
3.68k stars 349 forks source link

For allowedTags, stop treating falsy values other than false, same as false. #577

Closed kedarchandrayan closed 1 year ago

kedarchandrayan commented 1 year ago

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)

Make sure the PR fulfills these requirements:

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:

boutell commented 1 year ago

This would be a bc break as it is not unreasonable to rely on the current behavior. Thus we'd have to force a major version change for it and I don't think it's really justified for this strictness. I would say that developers who have this concern should use the community-maintained TypeScript bindings to achieve the desired certainty.

kedarchandrayan commented 1 year ago

Hello @boutell, we discussed the same on the issue and after your go-ahead, the PR was created.

Actually, this strictness is needed as due to some bug in the user's code, undefined can come as a value passed to the allowedTags parameter, as discussed in the issue.

Developers who are already using the community-maintained TypeScript bindings will have no effect. It will effect only those who have accidentally sent null or undefined or 0 in the value of allowedTags. These usages were wrong, so avoiding those should not be a problem as suggested by you in the issue discussion.

IMO, a major version update is not needed as it is not breaking anything which was promised in the readme.

Please refer the issue #176 for the earlier discussion.

Thanks, Kedar Chandrayan

boutell commented 1 year ago

Good point, we did discuss, I will review more carefully.

kedarchandrayan commented 1 year ago

Hello @boutell, please let me know your comments and changes if needed.

boutell commented 1 year ago

Thanks!