Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Conversation

kedarchandrayan
Copy link
Contributor

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)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • 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:

@boutell
Copy link
Member

boutell commented Oct 11, 2022

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.

@boutell boutell closed this Oct 11, 2022
@kedarchandrayan
Copy link
Contributor Author

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 boutell reopened this Oct 11, 2022
@boutell
Copy link
Member

boutell commented Oct 11, 2022

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

@kedarchandrayan
Copy link
Contributor Author

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

@boutell boutell merged commit ba3a2f6 into apostrophecms:main Oct 24, 2022
@boutell
Copy link
Member

boutell commented Oct 24, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{allowedTags:null} allows <script>
2 participants