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

replace ezyang/htmlpurifier with voku/anti-xss #3695

Closed

Conversation

dmytromikhieiev1985
Copy link
Contributor

@dmytromikhieiev1985 dmytromikhieiev1985 commented Aug 30, 2023

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

ezyang/htmlpurifier has LGPL licence. In this PR htmlpurifier replaced with package which has MIT licence

@dmytromikhieiev1985 dmytromikhieiev1985 force-pushed the replace-purifyer branch 2 times, most recently from 89ddedc to d18ef26 Compare August 30, 2023 12:34
@oleibman
Copy link
Collaborator

oleibman commented Aug 30, 2023

I don't have a problem with this change insofar as it doesn't break any tests. And I know you're not introducing this problem, but I really dislike testXssInComment. It tests to confirm that the string after being sanitized doesn't match the string going in, but that's a weak test. A poor sanitizer, for example, could just add a space after the first space it sees, and the test would pass. Could you rewrite the test so that it takes an expected result as a parameter and then tests that the html does contain the expected result? Also, looking at the github page for voku/anti-xss, it has at least 2 examples which aren't in our test set (example 2 hexadecimal html and example 7 iframe (except that unlike the example we don't want to allow iframe)); can you add tests for those, and any other examples that you don't see represented?

@oleibman
Copy link
Collaborator

Here's what I think XssVulnerabilityTest should be:
XssVulnerabilityTest.php.txt

@dmytromikhieiev1985
Copy link
Contributor Author

Added new PR due to a lot of conflicts #3724 @oleibman please check

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

Successfully merging this pull request may close these issues.

2 participants