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

Fix XSS caused by disabled autoescaping in the default DRF Browsable API view templates #6330

Merged
merged 3 commits into from Jan 16, 2019

Conversation

Projects
None yet
5 participants
@zyv
Copy link
Contributor

zyv commented Nov 21, 2018

As requested in #6191.

@xordoquy xordoquy added this to the 3.9.1 Release milestone Nov 21, 2018

@xordoquy xordoquy added the Bug label Nov 21, 2018

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Nov 21, 2018

Thanks for adding the test case @zyv.

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Dec 10, 2018

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 zyv force-pushed the moneymeets:moneymeets/html-escaping branch from 49ad94c to be68c52 Dec 14, 2018

@zyv

This comment has been minimized.

Copy link
Contributor Author

zyv commented Dec 14, 2018

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 title Add 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

This comment has been minimized.

Copy link
Member

rpkilby commented Dec 14, 2018

👌 nice.

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

@carltongibson

This comment has been minimized.

Copy link
Collaborator

carltongibson commented Dec 14, 2018

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

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 16, 2019

Looks correct to me, yup.

@tomchristie tomchristie merged commit 4bb9a3c into encode:master Jan 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

openstack-gerrit pushed a commit to openstack/ara-server that referenced this pull request Jan 17, 2019

Fixed XSS issue from DRF.
Details in encode/django-rest-framework#6330

Change-Id: Icea25569b92c3559029ae1c93712e746684187f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.