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

make voku/anti-xss an optional package #1728

Closed
wants to merge 2 commits into from
Closed

make voku/anti-xss an optional package #1728

wants to merge 2 commits into from

Conversation

jasverix
Copy link
Contributor

@jasverix jasverix commented Nov 23, 2020

This package installs voku/portable-utf8 which "sanitizes" REQUEST_URI in globals, which in turn crashed our entire system.

Library packages should never 'do' anything, just be libraries.

This is:

- [ x ] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

Our system did break down some time ago after installing voku/portable-utf8 because it sanitizes global variable REQUEST_URI. After that, the package is completely banned from our code base. I don't know if you will accept making it optional (only used one place..), but I will give it a try.

This package installs voku/portable-utf8 which "sanitizes" REQUEST_URI in globals,
which in turn crashed our entire system. Library packages should never 'do' anything,
just be libraries.
@MarkBaker
Copy link
Member

MarkBaker commented Nov 24, 2020

Simply making this optional exposes the security vulnerability that it was intended to fix, which is not a good thing. Users don't install optional extras unless they need them; and security issues should never be an optional add-in

There is at least one practical alternative to voku/antixss, although it isn't quite as lightweight or easy to use, and that is ezyang/htmlpurifier, which is still active and also PHP8 ready... would you have any problems switching to that instead

@jasverix
Copy link
Contributor Author

Yes, I understand, and I just wanted to open discussion, because I did not think you would just remove it. But voku/portable-utf8 has a bootstrap.php file which does a little more than we can accept - libraries should in my world just add classes and not do things on each request.

I would not have any problems using htmlpurifier, that library is not adding any bootstrap file at least (and has much less dependencies). I can fix the PR to do that instead, and then you can consider. I am not using the HTML Writer myself.

@jasverix
Copy link
Contributor Author

Tried to implement this now, but you have unit-tests requiring that this string is not allowed:
<div class="comment">javascript:alert(1)</div>

That's not dangerous HTML, it's just strange plain text. As an attribute, though, it is more dangerous. But should we still not allow this text? Looks like anti-xss removed it completely, but HTMLPurifier leaves it (but the first test <script>alert('test')</script> is passing)

Not all unit-tests passes because the tests requires plain text that looks like javascript is removed as well.
@MarkBaker
Copy link
Member

I've updated the tests, and modified the code to use HTMLPurifier instead of anti-xss in a separate PR

@jasverix
Copy link
Contributor Author

Great! Thank you!

@jasverix jasverix closed this Dec 10, 2020
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