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

enable django autoescape for page-wrapper #187

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

bdemetris
Copy link
Contributor

autoescape is a good way to prevent xss attacks.

currently doing something like
curl "http://localhost:8000/%3E%3Cscript%3Ealert(document.domain)%3C/script%3E" would inject the trailing url params into the template, and the system would attempt to run the script.

this will provide somewhat of a stop gap, while not perfect, would prevent most of these xss attacks from succeeding by turning the trailing params into garbage.

see https://docs.djangoproject.com/en/2.0/ref/templates/language/#automatic-html-escaping

@grahamgilbert
Copy link
Member

How does this affect the widgets loading via JavaScript?

@bdemetris
Copy link
Contributor Author

im not an expert on the subject, but i dont think it would. my reasoning is that any javascript based widget is being called explicitly from the server (i.e there was no user input). autoescape is meant to protect against users submitting data (in this case a url) which django allows to be inserted into the template, and ran.

ive done some limited testing here, and the pages seem to load correctly for me. i also played around with removing the safe argument from various templates such as here: https://github.com/salopensource/sal/blob/master/datatableview/templates/datatableview/bootstrap_structure.html#L8

although i chose not to include removing the safe argument in the PR, i also didn't notice any adverse affects.

the safe argument can be used (according to the documentation) as a mechanism for explicitly trusting incoming params that you as a developer know to be ok, so if you put them there on purpose then it might be worth investigating each time the argument is used, and why it was used. this might be legacy code, or code that doesn't get used anymore, but either way thats the type of thing that would break by using autoescape.

@grahamgilbert grahamgilbert merged commit 74147c1 into salopensource:master Feb 1, 2018
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

2 participants