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

Problem: autoescape not getting passed to urlize_quoted_links filter #6191

Merged
merged 1 commit into from Oct 10, 2018

Conversation

dkliban
Copy link
Contributor

@dkliban dkliban commented Sep 17, 2018

Solution: set needs_autoescape=True when registering the filter

Without this patch, the disabling autoescape in the template does not work.

refs #6201

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Oct 2, 2018

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.
@dkliban dkliban force-pushed the fix_urlize_quoted_links branch from aaeea76 to 3205d8e Compare Oct 2, 2018
@dkliban
Copy link
Contributor Author

@dkliban dkliban commented Oct 2, 2018

@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?

@encode encode deleted a comment from codecov-io Oct 3, 2018
@rpkilby rpkilby added this to the 3.9 Release milestone Oct 3, 2018
rpkilby
rpkilby approved these changes Oct 3, 2018
Copy link
Member

@rpkilby rpkilby left a comment

Thanks @dkliban. Confirmed on my end that the test does fail without the fix.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Yep. Super. Thanks!

@carltongibson carltongibson merged commit dd19a44 into encode:master Oct 10, 2018
1 check passed
@zyv
Copy link
Contributor

@zyv zyv commented Nov 21, 2018

@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)

{
    &quot;a&quot;: &quot;&lt;h1&gt;badmarker&lt;/h1&gt; - <a href="https://www.google.com" rel="nofollow">https://www.google.com</a>&quot;
}

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

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Nov 21, 2018

@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
Copy link
Contributor Author

@dkliban dkliban commented Nov 21, 2018

@zyv I added the patch because the filter was not adhering to the API. In my code[0] the filter is chained with other filters. The filter that processed the string after 'urlize_quoted_urls' was receiving data that was escaped when autoescape was supposed to be disabled.

[0] https://github.com/pulp/pulp/blob/master/pulpcore/pulpcore/app/templates/rest_framework/api.html#L176

@xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Nov 21, 2018

@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.

@zyv
Copy link
Contributor

@zyv zyv commented Nov 21, 2018

@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.

@sloria
Copy link
Contributor

@sloria sloria commented Nov 29, 2018

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.

@zyv
Copy link
Contributor

@zyv zyv commented Dec 1, 2018

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!

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Dec 2, 2018

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.

@zyv
Copy link
Contributor

@zyv zyv commented Dec 10, 2018

Alright, since we've already started migration to 3.9.x, and the only thing holding us back is this XSS issue, I've decided to follow @carltongibson suggestion and made a custom unofficial DRF 3.9.0.post1 release with this commit reverted.

If anyone is interested, the pre-built wheel that we'll be using is available at https://moneymeets.github.io/django/djangorestframework/ and the corresponding source at
https://github.com/moneymeets/django-rest-framework/tree/moneymeets/3.9.0.post1 , but, of course, keep in mind that I'm just a random guy on the Internet, this is no official DRF release and if something goes awry you keep the pieces...

@rpkilby
Copy link
Member

@rpkilby rpkilby commented Dec 10, 2018

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.

@zyv
Copy link
Contributor

@zyv zyv commented Dec 14, 2018

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 issue Nov 17, 2020
…ncode#6191)

Solution: set needs_autoescape=True when registering the filter

Without this patch, the disabling autoescape in the template does not work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants