Skip to content

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