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

Autoescape temlpates by default #178

Closed
wants to merge 2 commits into from
Closed

Conversation

gcmurphy
Copy link
Contributor

If a custom override for autoescape is not present the default behaviour
will be to escape templates with extensions html, htm, xhtml, xml, and
jinja2.

Closes #177

If a custom override for autoescape is not present the default behaviour
will be to escape templates with extensions html, htm, xhtml, xml, and
jinja2.
@codecov
Copy link

codecov bot commented Jan 12, 2018

Codecov Report

Merging #178 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #178   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          97     99    +2     
  Branches       13     15    +2     
=====================================
+ Hits           97     99    +2
Impacted Files Coverage Δ
aiohttp_jinja2/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64501fc...0d0edae. Read the comment docs.

@@ -34,6 +36,12 @@ def setup(app, *args, app_key=APP_KEY, context_processors=(),
return env


def aiohttp_jinja2_autoescape(template):
if template is None:
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line is not test covered

@@ -24,6 +24,8 @@ def setup(app, *args, app_key=APP_KEY, context_processors=(),
env.globals.update(GLOBAL_HELPERS)
if filters is not None:
env.filters.update(filters)
if 'autoescape' not in kwargs:
env.autoescape = aiohttp_jinja2_autoescape
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not autoescape everything by default?
Why do you choose based on template template suffix?

@@ -24,6 +24,8 @@ def setup(app, *args, app_key=APP_KEY, context_processors=(),
env.globals.update(GLOBAL_HELPERS)
if filters is not None:
env.filters.update(filters)
if 'autoescape' not in kwargs:
env.autoescape = lambda _: True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env.autoescape = True would be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel like it would've been easier to fix this yourself...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You are asking for new feature.
  2. I personally don't need it but the feature looks interesting, I'm ready for finding a time for review.
  3. You are providing a PR -- awesome. The PR is not perfect. It requires a couple iterations before merging. You give up after first step.

Do you need the feature? If yes -- please finish the PR. The project is driven by OSS enthusiasts in spare time, personally I busy on other (and more important for me) projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear I'm not an aiohttp user but noticed this when I was trying out the framework and wanted to make sure developers were aware of it. I do not consider this a feature, but more of a security vulnerability and nobody else seems to know they will need to enable autoescape when using this project

Everybody is busy and in this case both our time has been wasted with this back and forth. I will try again but please consider how much effort you put in typing the above versus actually fixing this issue.

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