Fix XSS caused by disabled autoescaping in the default DRF Browsable API view templates by zyv · Pull Request #6330 · 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

Conversation6 Commits3 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 }})

zyv

@rpkilby

Thanks for adding the test case @zyv.

@zyv zyv mentioned this pull request

Dec 1, 2018

@rpkilby

Hi @zyv. I need to dig in to this more fully, but I'm not 100% that the test demonstrates the correct behavior. Basically, this test ensures that urlize_quoted_links escapes by default, ignoring the autoescape context, which conflicts with the intent of #6191. However, I'm inclined to agree w/ #6191 that urlize_quoted_links should obey the autoescape tag/context, and I think that the issues lie with the base template instead (i.e., the base template may need more selective auto escaping).

I think what we need instead are tests for the browsable API templates. By default, the browsable API should escape links once. And from there, we would then need to fix the templates and how/when it autoescapes.

Again, I'm not 100% on what the correct answer here is - I'd need to look at the base template and urlize_quoted_links more thoroughly.

@zyv

Hi @rpkilby, you are right that my test doesn't demonstrate the correct expected behaviour of urlize_quoted_links, my goal was to prove the point that PR #6191 introduced an XSS into the default DRF Browsable API templates, and I was hoping that the commit will get reverted and a new release will ensue quickly to fix this vulnerability, if a proper solution cannot be found on short notice due to limited maintainer capacity.

Anyways, I've now finally managed to find some time to look into it, and have fixed the test, as well as (hopefully) found a solution to the problem. I would appreciate if you could have a look at my new commits. A more detailed explanation of what/why they do follows:

I believe that the initial author of urlize_quoted_links made a mistake by not marking the resulting string as safe and faced the double quoting problem if autoescaping was enabled. To work around this issue, autoescaping was disabled in the templates. The "fix" in PR #6191 tipped the delicate balance of two mistakes compensating each other by making the function conform to Django API, which lead to HTML not being escaped at all in the default DRF templates.

I have changed the function to correctly mark final string as safe and also got rid of the crazy escaping logic, so that hopefully it is now clearer what the function does (and what it does not).

@zyv zyv changed the titleAdd test that verifies that HTML is correctly escaped in Browsable API views Fix XSS caused by disabled autoescaping in the default DRF Browsable API view templates

Dec 14, 2018

@rpkilby

👌 nice.

I'll take a look at this over the weekend. I really appreciate the effort you put into this.

@carltongibson

Thanks for the work @zyv! I will be preparing 3.9.1 next week. A fix here will be part of that. Your efforts are greatly appreciated.

@tomchristie

Looks correct to me, yup.

openstack-gerrit pushed a commit to recordsansible/ara-server that referenced this pull request

Jan 17, 2019

@apollo13

martey added a commit to pydata/conf_site that referenced this pull request

May 25, 2019

@martey

This was referenced

Dec 20, 2019

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

@zyv

…API view templates (encode#6330)