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

tojson not marking items as safe per docs #709

Closed
pleasantone opened this issue May 1, 2017 · 8 comments
Closed

tojson not marking items as safe per docs #709

pleasantone opened this issue May 1, 2017 · 8 comments

Comments

@pleasantone
Copy link

Expected Behavior

From the jinja docs:

"Note that this is available in templates through the |tojson filter which will also mark the result as safe. "

Actual Behavior

tojson doesn't seem to be marking the output as safe, so if we establish the jinja
environment with autoescape enabled, and then use tojson, we must add "|safe"
at the end.

Your Environment

  • Python version: 3.5.3
  • Jinja version: 2.9.6
@davidism
Copy link
Member

davidism commented May 1, 2017

This seems like an easy fix, if you'd like to submit a pull request.

@pleasantone
Copy link
Author

I think it just requires a Markup() around the results before returning, but I'm afraid to submit the request because I don't know all the comprehensive test cases you'd want to run it through, and I'd feel negligent submitting without fully testing. Sorry.

@davidism
Copy link
Member

davidism commented May 1, 2017

Don't worry, it's not that bad! Adding Markup around it, then making sure the existing test doesn't escape something since it should be safe now, should be fine.

@pleasantone
Copy link
Author

pleasantone commented May 1, 2017 via email

@ThiefMaster
Copy link
Member

This sounds like a backwards-incompatible change to me (just like it was in Flask, but at least Flask is pre-1.0).

If Jinja itself used to autoescape json data changing it means people need to add |forceescape whenever it's used outside script tags (e.g. in data attributes) or they'll be prone to XSS.

@pleasantone
Copy link
Author

The documentation says it's only safe inside of scripts. So either it's a bug that it's not surrounded by Markup() and you have to deal with people who were misusing it, or it's a feature, in which case, when you folks do eventually make autoescape the default (which is a very sane thing to do, and will squash more XSS bugs), you're going to have to deal with every place tojson is used, in a script, which is now going to need an explicit "|safe".

There's no perfect answer, but I'd lean towards following what the documentation always claimed it did, and save yourself the huge breakage you're going to experience any time someone runs with autoescape enabled (which is your stated future default).

@davidism
Copy link
Member

davidism commented May 1, 2017

It was also introduced very recently in 2.9, so it feels ok to fix this to match the documented behavior in 2.9-maintenance.

davidism added a commit that referenced this issue May 23, 2017
Make tojson always safe (fix #709)
@ayalash
Copy link
Contributor

ayalash commented May 28, 2017

@davidism Can you please close this issue? You merged my PR for fixing it (#718)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants