Problem: autoescape not getting passed to urlize_quoted_links filter by dkliban · Pull Request #6191 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Solution: set needs_autoescape=True when registering the filter
Without this patch, the disabling autoescape in the template does not work.
refs #6201
Hi @dkliban. Can you please add a failing test case (with your example from #6201) to go with this change? Thanks.
Solution: set needs_autoescape=True when registering the filter
Without this patch, the disabling autoescape in the template does not work.
@carltongibson I've added a test. It fails when I remove my patch and passes with it. Can you think of a better name for the test?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dkliban. Confirmed on my end that the test does fail without the fix.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Super. Thanks!
@dkliban unfortunately, after your fix the browsable API became unusable if the response contains HTML, because it is now unescaped, as autoescape is off in base.html, but if autoescape is turned on in the template, then the content is escaped twice - first in the function, and then outside.
class TestViewSet(GenericViewSet):
def list(self, request):
return Response({"a": "<h1>badmarker</h1> - https://www.google.com"})
is now rendered like this (wrong)
{
"a": "<h1>badmarker</h1> - <a href="https://www.google.com" rel="nofollow">https://www.google.com</a>"
}
instead of this (right)
{
"a": "<h1>badmarker</h1> - <a href="https://www.google.com" rel="nofollow">https://www.google.com</a>"
}
I don't quite understand from your test, what was your motivation to change this function to respect autoescape behaviour? As the other test shows, before your change it was working correctly with browsable API...
/cc @carltongibson
@zyv can you open a PR adding your test case? It may be we need to revert this, but let’s have a look at it.
@dkliban thanks for the feedback. This fix may only highlight another issue. We'll have a closer look at it with a failing test case.
@dkliban thank you for the explanation! I currently believe that you were misled by the parameter name and it was never supposed to adhere to the API in the first place, it's just that the parameter name is confusing, but that's only my working theory. I will hopefully have a look into your case as time permits.
@carltongibson thanks for chiming in, I have opened a new PR with a test that demonstrates the issue.
We ran into the issue @zyv described above (HTML getting rendered in the browseable API) and have to downgrade to 3.8.2. Perhaps this change should get reverted for now, until a proper fix for the OP is found.
I'm a bit worried that there haven't been any visible progress on #6330 so far, given that it's a gaping XSS hole for anyone using Browsable API and returning user-entered data as a part of the response.
I second the opinion of @sloria, that maybe if a proper fix has proved to be challenging, it would be great to merge #6330, revert #6191 and make a 3.9.1 bugfix release... Thanks!
Thanks for your concern @zyv. The issue is just time to examine it properly rather than it looking particularly difficult. Reverting is one option, but we need to make sure that’s the correct option before just rushing on. And that needs capacity, which can take a while in an open source project.
You’re welcome to take it on yourself if you like.
Or, if you’re in a desperate rush, you can momentarily fork and revert, and then deploy your fork, using Pip’s excellent VCS support.
Failing either of those, we’ll get to it.
Ach - sorry for the long response time. I had started writing something out but then got distracted.
I've replied to the PR with some thoughts.
I think I've found a correct solution to the problem in #6330 and I would appreciate your feedback @sloria @dkliban .
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request
Solution: set needs_autoescape=True when registering the filter
Without this patch, the disabling autoescape in the template does not work.