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

Use of autoescape=True in Jinja template breaks Vagrant #3912

Closed
samiam opened this issue May 11, 2023 · 4 comments · Fixed by #3911
Closed

Use of autoescape=True in Jinja template breaks Vagrant #3912

samiam opened this issue May 11, 2023 · 4 comments · Fixed by #3911
Labels

Comments

@samiam
Copy link
Contributor

samiam commented May 11, 2023

Issue Type

  • Bug report

Molecule and Ansible details

molecule 5.0.1 using python 3.11
    ansible 2.14.5
    delegated 5.0.1 from molecule

Molecule installation method (one of):

  • pip

Ansible installation method (one of):

  • pip

Detail any linters or test runners used:

Desired Behavior

Jinja2 should generate correct Vagrant config files as it did in molecule <= v4.

Actual Behavior

This line:
https://github.com/ansible-community/molecule/blob/main/src/molecule/util.py#L178
from this PR introduced the regression.

Briefly, quoted strings that used to be output in the Vagrantfile, are now escaped.

# v4
c.vm.box_url = "https://some.url.com"

# v5
c.vm.box_url = &#34;https://some.url.com&#34;

molecule-vagrant no longer works.

Here's a line from the template that I'm using as an example. There's plenty more in there.

https://github.com/ansible-community/molecule-vagrant/blob/main/molecule_vagrant/modules/vagrant.py#L222

Here's my PR that adds a simple test of quoted strings in Jinja2 templates:
#3911

Summary

My ask is that autoescape be turned off when rendering templates.

@samiam samiam added the bug label May 11, 2023
@apatard
Copy link
Contributor

apatard commented May 11, 2023

I hoped I fixed everything in ansible-community/molecule-plugins@7eab71d. Looks like it's not the case. Can you open a bug on molecule-plugins with:

  • a test case
  • the parts of the template missing escaping ?

I'm not sure that disabling auto-escaping in molecule will happen since on general case it's a matter of security, so it's better to fix the template. If/When ansible-community/molecule-plugins#142 is merged, it may be considered to disable auto-escaping since it won't impact molecule.

@samiam
Copy link
Contributor Author

samiam commented May 11, 2023

I was afraid of this response. 😄

I'm not sure I buy the security argument since this is a testing harness and templates are used to generate files but I'm not going to die on that hill.

I can look at the template. I already started and did exactly what you did; add the safe filter. I can add them to the entire template wherever vars are used, but then, what's the point? We'd just be undoing the auto-escape feature.

Plus, there are vars that come from config options, like provider_raw_config_args that may or may not have double quotes, so you just have to assume there could be quotes and pass that to safe as well. eg.

 - "customize ['createmedium', 'disk', '--filename', 'example', '--size', '2048']"

Ref: ansible-community/molecule-plugins#60

Perhaps a middle ground is to create a new function render_template_unsafe() which won't pass the auto-escape flag, or the reverse - keep current behavior and create new function render_template_safe(). Or pass autoescape as an option to the current render_template.

Lastly, the Jinja docs seem to reference this very issue:
https://jinja.palletsprojects.com/en/3.0.x/templates/#working-with-automatic-escaping

If #142 is going to get merged soon and we can go back to old behavior, that may save a lot of headache for everyone.

@samiam
Copy link
Contributor Author

samiam commented May 15, 2023

I added a PR with the additional safe filters:
ansible-community/molecule-plugins#153.

Sorry I don't have a test with it.
I'm new to tox and my environment wasn't working.
I wish there was a little guidance on setting up the tests to run...
Anyway, I'll try to get one written when I can.
It will probably be a little janky; reading the Vagrantfile for certain config statements to ensure they are correct?

@samiam
Copy link
Contributor Author

samiam commented May 22, 2023

@apatard

Here's the issue I opened in molecule-plugins:
ansible-community/molecule-plugins#158

In the PR you'll find a fix and test to the autoescape issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants