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

escaping user provided variables in Jinja templates #13

Closed
wants to merge 1 commit into from

Conversation

@daghan
Copy link

daghan commented Dec 31, 2019

Hi there,

We have a free program analysis tool for Python based web projects, called Bento. While we were scanning GitHub projects for issues, your project triggered a warning for unescaped Jinja templates.

You are passing the host parameter to your jinja template in views.py:63. lis_person_name_full comes from request.form.get('lis_person_name_full'). This line may be susceptible to XSS attacks. I went ahead and html-escaped the lis_person_name_full variable in launch.htm.j2 file using the {{value|e}} pattern in Jinja. (https://jinja.palletsprojects.com/en/2.10.x/templates/#working-with-manual-escaping).

Note that if your template file extensions ended with .html, .htm, .xml, or .xhtml, they would have been automatically html escaped. For more, take a look here: https://checks.bento.dev/en/latest/flake8-flask/unescaped-file-extension/

Bento flagged a few other issues including the usage of "bare except" but I didn't mess with those to keep this PR simple. If you are curious, feel free download and give Bento a try (https://bento.dev)

@Thetwam Thetwam self-requested a review Jan 2, 2020
@Thetwam

This comment has been minimized.

Copy link
Member

Thetwam commented Jan 2, 2020

Hey @daghan, thanks for your PR. I wasn't aware that .htm.j2 files were treated differently than .htm or .html files, so thank you for bringing it to our attention!

Instead of the change you suggested, would renaming the files to use a .html (or .xml, etc.) extension remove the vulnerability?

@daghan

This comment has been minimized.

Copy link
Author

daghan commented Jan 2, 2020

Absolutely. I wasn't sure which one was the more minimalistic solution so I went with the manual escaping option. But yes, sticking to the auto-escaped file extensions would definitely solve this issue.

@Thetwam

This comment has been minimized.

Copy link
Member

Thetwam commented Jan 2, 2020

Awesome, thank you @daghan. Closing this PR in favor of #14.

@Thetwam Thetwam closed this Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.