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

Autoescaping disabled by default is a dangerous default #528

Closed
delroth opened this issue Jan 6, 2016 · 4 comments
Closed

Autoescaping disabled by default is a dangerous default #528

delroth opened this issue Jan 6, 2016 · 4 comments
Labels

Comments

@delroth
Copy link

@delroth delroth commented Jan 6, 2016

I discovered today through an externally reported XSS that Jinja doesn't turn on autoescaping by default. This is a dangerous default, made even more dangerous by the fact that many people are introduced to Jinja through their previous knowledge of Django templates, which do have autoescaping enabled by default since 2007.

Looking through the internet, it looks like this fact is not very well known. In fact, I even found books (Python 3 Object Oriented Programming, chapter 12, cc @buchuki) that recommend using jinja2 for websites and leave autoescaping off. A random sample of Github projects also show that most people don't add autoescape=True or the autoescape extension to their project.

While I understand it would probably be a pain to migrate to a safer default at that point, the documentation should at least make it obvious that autoescaping is off by default. Maybe add autoescape=True to all examples that create a jinja2.Environment object and/or add a section very early on in the documentation.

@ThiefMaster

This comment has been minimized.

Copy link
Member

@ThiefMaster ThiefMaster commented Jan 6, 2016

Jinja is not only for HTML but a general-purpose template engine. I think most people use Jinja in Flask, which enables autoescape by default for HTML templates. If you integrate a template engine in your own framework, you are supposed to read the documentation on how to use it safely.

That said, I think being very clear in the docs that you should probably use autoescape when generating HTML is a good suggestion.

@delroth

This comment has been minimized.

Copy link
Author

@delroth delroth commented Jan 6, 2016

I would assume most major frameworks get this right (more eyes, less bugs, etc.) but there are also a lot of smaller projects integrating Jinja manually that do make this mistake.

And to be frank, Jinja's documentation is really poor on that subject. Here is what you first see when you open Jinja's documentation:

Jinja2 is a modern and designer-friendly templating language for Python, modelled after Django’s templates. It is fast, widely used and secure with the optional sandboxed template execution environment:

"modeled after Django's templates" and "secure" could lead to think it has Django's templates security features such as autoescaping.

Then, an example, which is HTML. And just after that, in the list of features:

powerful automatic HTML escaping system for XSS prevention

Which doesn't mention it's optional and not enabled by default.

The "Basic API usage" docs page doesn't show autoescaping enabled but it doesn't show HTML either, so I guess that's fine. But then when you go to the API "Basics" page:

from jinja2 import Environment, PackageLoader
env = Environment(loader=PackageLoader('yourapplication', 'templates'))
template = env.get_template('mytemplate.html')
print template.render(the='variables', go='here')

The first example is vulnerable to trivial XSSes. People are right to be confused.

@ThiefMaster

This comment has been minimized.

Copy link
Member

@ThiefMaster ThiefMaster commented Jan 6, 2016

Sounds like a good candidate for a pull request :)

@jeffwidman jeffwidman added the docs label Apr 13, 2016
@mitsuhiko

This comment has been minimized.

Copy link
Member

@mitsuhiko mitsuhiko commented Jan 6, 2017

We can't turn this off now without breaking everybody's code. I am however happy to update the docs accordingly. I will close this as a wontfix but feel free to open a PR for documentation updates.

@mitsuhiko mitsuhiko closed this Jan 6, 2017
simonw added a commit to simonw/datasette that referenced this issue Nov 16, 2017
We had XSS holes! Since we don't do cookies or authentication
they shouldn't cause any actual harm, but still really not good.

pallets/jinja#528
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.