CVE-2018-25045: Problem: autoescape not getting passed to urlize_quoted_links filter by dkliban · Pull Request #6191 · encode/django-rest-framework

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

Conversation 15 Commits 1 Checks 0 Files changed

Conversation

Copy link

Contributor

** 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

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.

Copy link

Contributor Author

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

Copy link

Member

** rpkilby left a comment**

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

Copy link

Contributor

** 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

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

Copy link

Contributor Author

** dkliban 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 mentioned this pull request

Nov 21, 2018

Copy link

Contributor

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

Copy link

Contributor

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

Copy link

Contributor

** 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!

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.

Copy link

Contributor

** zyv 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.

Copy link

Contributor

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