Replace bleach with nh3 #3696
Replace bleach with nh3 #3696
Conversation
|
Nice work getting a start on this. Ammonia say they are 20 times faster then Bleach, that should shave of a number of milli seconds for nearly every request. Would be good to upstream the changes if they are generally useful. I see you added code to pick up any Other instances might have set their own values but the release notes and the docs can inform them of the need to update the settings. I prefer to be explicit, remove every mention of BLEACH from the code so there is no confusion about what we are using. The settings are complex and confusing as it is :-). |
|
Thanks for taking a look! I'm looking forward to seeing the stats once it's implemented, should be significant.
I did this only really with the upstream in mind, I wasn't totally sure if other adopters would've wanted somewhat of a drop-in solution but realistically I think you're totally right in the fact that it only adds complexity and confusion so I'll strip that out. Hoping to iron out some unit tests I have locally on my fork then going to be putting up a PR. Should be cool! |
|
All unit tests (both in Hypha & in django-nh3) are passing and functionality looks good! Once/if this gets merged upstream, the only thing left is to swap out the custom github link in |
|
Upstream is all merged, should be good to go! |
|
@wes-otf Excellent work on the upstream PR! I noticed a contribution from Torchbox/Wagtail people as well so they are looking at using nh3 as well. This functionality is core to Hypha security since we filter on output, not on input. (It is a long term goal to change this.) Therefor I do not want to rush in to deploying this, especially since the django-nh3 package is so fresh. What do you think? |
|
@frjo I totally agree. Especially with the long term deprecation plan for bleach I don't see any reason to jump into this without some exhaustive user testing. Totally fine waiting to merge this in the new year. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@frjo What are you thinking on the timeline for this? Once we get everything all set with the public stuff we could let the user testing group give this a go and ensure nothing weird changed. |
hypha/settings/django.py
Outdated
| @@ -57,7 +57,8 @@ | |||
| "django_filters", | |||
| "django_select2", | |||
| "addressfield", | |||
| "django_bleach", | |||
| # "django_bleach", | |||
There was a problem hiding this comment.
I think this line can be deleted.
|
@wes-otf There are some public pages that gets re-added by mistake in this PR. My concern regarding timeline is that the django-nh3 package is very new. On the other hand it is a simple wrapper and this PR touch a lot of files and will likely get a lot of merge conflicts if we have it hanging around. I suggest we make v5.4.0 with all the public go out first. Then we work to get this in to the next release. |
|
Good catch! That was the result of a sloppy merge on my part. That timeline sounds good though - the continuous merge conflicts were also my worry so I think that'll work nice. |
|
@wes-otf Can you rebase this? Then we can put it on test and get it in before more rebases are needed. |
frjo
added
the
Status: Needs dev testing 🧑💻
Fixes #3693 Aims to fully replace bleach with nh3 due to bleach deprecation. Currently, [django-nh3](https://github.com/marksweb/django-nh3) is in it's infancy, but seems like it could be an almost drop in replacement for [django-bleach](https://github.com/marksweb/django-bleach), for I [forked it](https://github.com/wes-otf/django-nh3) and made some small additions that would allow it to work for our purposes and be smoothly migrated. Initial smoke testing in Hypha seems to work exactly as bleach did but needs more extensive testing. Ideally I would smooth out some edges of my fork and put in a PR to django-nh3. Let me know any thoughts/questions!
Fixes #3693 Aims to fully replace bleach with nh3 due to bleach deprecation. Currently, [django-nh3](https://github.com/marksweb/django-nh3) is in it's infancy, but seems like it could be an almost drop in replacement for [django-bleach](https://github.com/marksweb/django-bleach), for I [forked it](https://github.com/wes-otf/django-nh3) and made some small additions that would allow it to work for our purposes and be smoothly migrated. Initial smoke testing in Hypha seems to work exactly as bleach did but needs more extensive testing. Ideally I would smooth out some edges of my fork and put in a PR to django-nh3. Let me know any thoughts/questions!
Fixes HyphaApp#3693 Aims to fully replace bleach with nh3 due to bleach deprecation. Currently, [django-nh3](https://github.com/marksweb/django-nh3) is in it's infancy, but seems like it could be an almost drop in replacement for [django-bleach](https://github.com/marksweb/django-bleach), for I [forked it](https://github.com/wes-otf/django-nh3) and made some small additions that would allow it to work for our purposes and be smoothly migrated. Initial smoke testing in Hypha seems to work exactly as bleach did but needs more extensive testing. Ideally I would smooth out some edges of my fork and put in a PR to django-nh3. Let me know any thoughts/questions!
frjo
removed
Status: Needs testing
Fixes #3693
Aims to fully replace bleach with nh3 due to bleach deprecation. Currently, django-nh3 is in it's infancy, but seems like it could be an almost drop in replacement for django-bleach, for I forked it and made some small additions that would allow it to work for our purposes and be smoothly migrated.
Initial smoke testing in Hypha seems to work exactly as bleach did but needs more extensive testing. Ideally I would smooth out some edges of my fork and put in a PR to django-nh3. Let me know any thoughts/questions!