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

[Enhancement] [zos_copy] [zos_job_submit] [zos_script] Add option to disable autoescape for Jinja templates #1810

Open
wants to merge 46 commits into
base: dev
Choose a base branch
from

Conversation

rexemin
Copy link
Collaborator

@rexemin rexemin commented Nov 20, 2024

SUMMARY

Fixes #1198.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • module_utils/template.py
  • zos_copy
  • zos_job_submit
  • zos_script
ADDITIONAL INFORMATION

This adds a new option autoescape to all the modules that currently support templates. Its default value is True just to err on the side of caution, but it allows users to disable autoescaping whenever they need to.

@rexemin rexemin marked this pull request as ready for review November 22, 2024 19:29
@rexemin
Copy link
Collaborator Author

rexemin commented Nov 23, 2024

Results for the pipeline

Screenshot 2024-11-22 at 18 13 48
Screenshot 2024-11-22 at 18 14 00

Copy link
Collaborator

@ddimatos ddimatos left a comment

Choose a reason for hiding this comment

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

General questions:

  1. How does this impact users who have been piping values to safe?
  2. We probably want to update the changelog fragment so it also includes a behavior change so the one releasing can properly highlight this. This could be a known issues type fragment.
//{{template_values | safe }}

if autoescape:
self.templating_env = jinja2.Environment(autoescape=True, **environment_args)
else:
self.templating_env = jinja2.Environment(autoescape=jinja2.select_autoescape(), **environment_args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question, the branched logic here, else, is relying on jinja2.select_autoescape() which will return false as it is today, would it be safer to be explicit since this could change in the future where maybe it would be true thus if/else would return true should the behavior change.

Also note the documentation:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explicit in what way? I could change the function so it specifies the kinds of files that will commonly be templated, but just putting false in autoescape generates an error in Bandit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rexemin Is that the case if you use the following?

Suggested change
self.templating_env = jinja2.Environment(autoescape=jinja2.select_autoescape(), **environment_args)
self.templating_env = jinja2.Environment(autoescape=autoescape, **environment_args)

Just set autoescape parameter value to whatever autoescape variable has?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is the case, it was the original code that caused the bandit error in the first place

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup, is gonna trigger that scan issue no matter what. This is ok for me as long as we have a test which tests autoescape False and True. That means, if this gets changed in the future, then will be caught in the test cases.

@rexemin
Copy link
Collaborator Author

rexemin commented Nov 27, 2024

This change should not affect users already using the safe filter. Tested without issues with a template that used it.

Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Just requesting a little addition to test

if autoescape:
self.templating_env = jinja2.Environment(autoescape=True, **environment_args)
else:
self.templating_env = jinja2.Environment(autoescape=jinja2.select_autoescape(), **environment_args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rexemin Is that the case if you use the following?

Suggested change
self.templating_env = jinja2.Environment(autoescape=jinja2.select_autoescape(), **environment_args)
self.templating_env = jinja2.Environment(autoescape=autoescape, **environment_args)

Just set autoescape parameter value to whatever autoescape variable has?

@@ -158,6 +158,18 @@
/*
//SYSUT2 DD SYSOUT=*
//
""",
"Autoescape": """//{{ pgm_name }} JOB (T043JM,JM00,1,0,0,0),{{ parameter }},CLASS=R,
// MSGCLASS=X,MSGLEVEL=1,NOTIFY=S0JM
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the issue related users complained both about & and ' could you add those as part of the tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see single quotes is added down there so just &

AndreMarcel99 and others added 12 commits December 4, 2024 11:20
…e_shell_call_cp_to_python_module

[Enabler][1652]migrate_shell_call_cp_to_python_module
Swapped old zoau v1.1.0 to use as zoau 1.3.4 ga build
… resource types (#1822)

* Added fix for zos_find

* Added tests for fix

* Updated changelog
* Add enhance error message

* Add fragment

* Update plugins/modules/zos_copy.py

Co-authored-by: Fernando Flores <[email protected]>

* Update 1821-Add_error_message.yml

* Update zos_copy.py

---------

Co-authored-by: Fernando Flores <[email protected]>
…_init_portability

[Enabler][1589]volume_init_test_portability
…C tool (#1829)

* Update minimum version of z/OS for the GA of 1.12

Signed-off-by: ddimatos <[email protected]>

* Update AAP link

Signed-off-by: ddimatos <[email protected]>

* Venv for 2.18

Signed-off-by: ddimatos <[email protected]>

* remove merge flags

Signed-off-by: ddimatos <[email protected]>

* Merged release into dev

* fixed test case

* Fixed mvs raw test

---------

Signed-off-by: ddimatos <[email protected]>
Co-authored-by: Fernando Flores <[email protected]>
…1827)

* Change flag used to run specific test

* Add more env vars needed to run tests

* Add verbose option to ac_test

* Add mark and stop options to ac_test

* Refactor pytest call

Also changes how verbosity is handled, as well as adds printing of each pytest command run.

* Update config to support new way of handling env vars in tests

* Add profile option to ac_lint

* Add support to run multiple files in ac_test

* Change boolean options to flags

Options `debug` (ac_test, test_concurrent) and `stop` (ac_test) can be now used simply as flags, without the need to add a `"true"` string after the option.

* Document and clean up arg parser

* Update more boolean options to be flags

* Refactor pytest call

* Add option 'volumes' to ac_test

* Add user option and tweak verbosity

* Add ac-test-required command

* Add changelog fragment
Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

lgtm

… API (#1831)

* Replace calls to cp for dcp

* Replace copy functions in copy.py

* Replace copy calls in modules

* Fix typo

* Fix return statement

* Add changelog fragment

* Fix sanity issues
@rexemin rexemin requested a review from ddimatos December 18, 2024 00:02
@fernandofloresg fernandofloresg force-pushed the dev branch 2 times, most recently from 2660c02 to f5376bf Compare December 18, 2024 17:08
@fernandofloresg
Copy link
Collaborator

Please check why some fragments are being added, maybe the branch has diverted a bit from dev

@rexemin
Copy link
Collaborator Author

rexemin commented Jan 2, 2025

Please check why some fragments are being added, maybe the branch has diverted a bit from dev

Done, it looks like it just needed to merge recent changes in dev.

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.

[Enhancement] Disable jinja2 autoescape feature for some modules that use Ansible template
5 participants