From 485e1515073da8498382a2d53bce0587d35deaa2 Mon Sep 17 00:00:00 2001 From: Thayne Harbaugh Date: Fri, 4 May 2018 17:14:01 -0600 Subject: [PATCH 1/6] Support Jinja imports with relative paths. Issue #13889 Jinja imports will be interpreted as originating from the top of each of the directories in the searchpath when the template name does not begin with './' or '../'. When a template name begins with './' or '../' then the import will be relative to the importing file. Prior practices required the following construct: .. code-block:: yaml {% from tpldir ~ '/foo' import bar %} This patch supports a more "natural" construct: .. code-block:: yaml {% from './foo' import bar %} Comparatively when importing from a parent directory - prior practice: .. code-block:: yaml {% from tpldir ~ '/../foo' import bar %} Construct supported by this patch: .. code-block:: yaml {% from '../foo' import bar %} --- salt/utils/jinja.py | 56 +++++++++++++++---- tests/support/parser/__init__.py | 2 +- tests/unit/templates/files/rescape | 1 + .../templates/files/test/relative/rescape | 1 + .../unit/templates/files/test/relative/rhello | 2 + .../unit/templates/files/test/relative/rmacro | 4 ++ tests/unit/templates/test_jinja.py | 17 ++++++ 7 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 tests/unit/templates/files/rescape create mode 100644 tests/unit/templates/files/test/relative/rescape create mode 100644 tests/unit/templates/files/test/relative/rhello create mode 100644 tests/unit/templates/files/test/relative/rmacro diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 546f5b568d18..0bf5451f9f98 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -98,20 +98,52 @@ def check_cache(self, template): self.cached.append(template) def get_source(self, environment, template): - # checks for relative '..' paths - if '..' in template: - log.warning( - 'Discarded template path \'%s\', relative paths are ' - 'prohibited', template - ) - raise TemplateNotFound(template) + ''' + Salt-specific loader to find imported jinja files. + + Jinja imports will be interpreted as originating from the top + of each of the directories in the searchpath when the template + name does not begin with './' or '../'. When a template name + begins with './' or '../' then the import will be relative to + the importing file. + + ''' + # FIXME: somewhere do seprataor replacement: '\\' => '/' + _template = template + if template.split('/', 1)[0] in ('..', '.'): + is_relative = True + else: + is_relative = False + # checks for relative '..' paths that step-out of file_roots + if is_relative: + # Starts with a relative path indicator + + if not environment or 'tpldir' not in environment.globals: + log.warning( + 'Relative path "%s" cannot be resolved without an environment', + template + ) + print('Relative path "{0}" cannot be resolved without an environment'.format(template)) + raise TemplateNotFound + base_path = environment.globals['tpldir'] + _template = os.path.normpath('/'.join((base_path, _template))) + if _template.split('/', 1)[0] == '..': + log.warning( + 'Discarded template path "%s": attempts to' + ' ascend outside of file roots', template + ) + raise TemplateNotFound(template) - self.check_cache(template) + self.check_cache(_template) if environment and template: - tpldir = os.path.dirname(template).replace('\\', '/') + tpldir = os.path.dirname(_template).replace('\\', '/') + tplfile = _template + if is_relative: + tpldir = environment.globals.get('tpldir', tpldir) + tplfile = template tpldata = { - 'tplfile': template, + 'tplfile': tplfile, 'tpldir': '.' if tpldir == '' else tpldir, 'tpldot': tpldir.replace('/', '.'), } @@ -119,7 +151,7 @@ def get_source(self, environment, template): # pylint: disable=cell-var-from-loop for spath in self.searchpath: - filepath = os.path.join(spath, template) + filepath = os.path.join(spath, _template) try: with salt.utils.files.fopen(filepath, 'rb') as ifile: contents = ifile.read().decode(self.encoding) @@ -205,7 +237,7 @@ def skip_filter(data): ''' Suppress data output - .. code-balock:: yaml + .. code-block:: yaml {% my_string = "foo" %} diff --git a/tests/support/parser/__init__.py b/tests/support/parser/__init__.py index dd601241a2b0..b4bf117e2cf3 100644 --- a/tests/support/parser/__init__.py +++ b/tests/support/parser/__init__.py @@ -332,7 +332,7 @@ def parse_args(self, args=None, values=None): os.path.basename(fpath).startswith('test_'): self.options.name.append(fpath) continue - self.exit(status=1, msg='\'{}\' is not a valid test module'.format(fpath)) + self.exit(status=1, msg='\'{}\' is not a valid test module\n'.format(fpath)) print_header(u'', inline=True, width=self.options.output_columns) self.pre_execution_cleanup() diff --git a/tests/unit/templates/files/rescape b/tests/unit/templates/files/rescape new file mode 100644 index 000000000000..d77a94dcf005 --- /dev/null +++ b/tests/unit/templates/files/rescape @@ -0,0 +1 @@ +FAILURE diff --git a/tests/unit/templates/files/test/relative/rescape b/tests/unit/templates/files/test/relative/rescape new file mode 100644 index 000000000000..4c18d6dc4230 --- /dev/null +++ b/tests/unit/templates/files/test/relative/rescape @@ -0,0 +1 @@ +{% import '../../rescape' as xfail -%} diff --git a/tests/unit/templates/files/test/relative/rhello b/tests/unit/templates/files/test/relative/rhello new file mode 100644 index 000000000000..beab248763e1 --- /dev/null +++ b/tests/unit/templates/files/test/relative/rhello @@ -0,0 +1,2 @@ +{% from './rmacro' import rmacro with context -%} +{{ rmacro('Hey') ~ rmacro(a|default('a'), b|default('b')) }} diff --git a/tests/unit/templates/files/test/relative/rmacro b/tests/unit/templates/files/test/relative/rmacro new file mode 100644 index 000000000000..0827c4540fbf --- /dev/null +++ b/tests/unit/templates/files/test/relative/rmacro @@ -0,0 +1,4 @@ +{% from '../macro' import mymacro with context %} +{% macro rmacro(greeting, greetee='world') -%} +{{ mymacro(greeting, greetee) }} +{%- endmacro %} diff --git a/tests/unit/templates/test_jinja.py b/tests/unit/templates/test_jinja.py index b06f74010a6b..ba0fcd1f97ec 100644 --- a/tests/unit/templates/test_jinja.py +++ b/tests/unit/templates/test_jinja.py @@ -170,6 +170,23 @@ def test_import(self): self.assertEqual(fc.requests[0]['path'], 'salt://hello_import') self.assertEqual(fc.requests[1]['path'], 'salt://macro') + def test_relative_import(self): + ''' + You can import using relative paths + issue-13889 + ''' + fc, jinja = self.get_test_saltenv() + tmpl = jinja.get_template('relative/rhello') + result = tmpl.render() + self.assertEqual(result, 'Hey world !a b !') + assert len(fc.requests) == 3 + self.assertEqual(fc.requests[0]['path'], 'salt://relative/rhello') + self.assertEqual(fc.requests[1]['path'], 'salt://relative/rmacro') + self.assertEqual(fc.requests[2]['path'], 'salt://macro') + # This must fail when rendered: attempts to import from outside file root + template = jinja.get_template('relative/rescape') + self.assertRaises(exceptions.TemplateNotFound, template.render) + def test_include(self): ''' You can also include a template that imports and uses macros From 43ed9bcb684513076b0e8406a7ff06df9fb0c87a Mon Sep 17 00:00:00 2001 From: plastikos Date: Thu, 17 May 2018 23:04:26 -0600 Subject: [PATCH 2/6] Remove silly print() statement from utils/jinja.py --- salt/utils/jinja.py | 1 - 1 file changed, 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 0bf5451f9f98..8d5cd7a0ace6 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -123,7 +123,6 @@ def get_source(self, environment, template): 'Relative path "%s" cannot be resolved without an environment', template ) - print('Relative path "{0}" cannot be resolved without an environment'.format(template)) raise TemplateNotFound base_path = environment.globals['tpldir'] _template = os.path.normpath('/'.join((base_path, _template))) From bec2517e5401f771fe4fd1327713baafc8d73f27 Mon Sep 17 00:00:00 2001 From: plastikos Date: Thu, 17 May 2018 23:07:43 -0600 Subject: [PATCH 3/6] Code review fixes. Change message referencing "file roots" to "salt://" --- salt/utils/jinja.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 8d5cd7a0ace6..509c68adf0a1 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -129,7 +129,7 @@ def get_source(self, environment, template): if _template.split('/', 1)[0] == '..': log.warning( 'Discarded template path "%s": attempts to' - ' ascend outside of file roots', template + ' ascend outside of salt://', template ) raise TemplateNotFound(template) From 1be25616a72c433962a1dbf25ce5484f1c7d6e96 Mon Sep 17 00:00:00 2001 From: plastikos Date: Fri, 8 Jun 2018 02:02:07 -0600 Subject: [PATCH 4/6] Add Jinja include relative paths to Fluorine release notes --- doc/topics/releases/fluorine.rst | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/doc/topics/releases/fluorine.rst b/doc/topics/releases/fluorine.rst index ff95f7b05c9c..da84fa559c97 100644 --- a/doc/topics/releases/fluorine.rst +++ b/doc/topics/releases/fluorine.rst @@ -594,3 +594,29 @@ This release add an additional search using the ``groupattribute`` field as well. The original ``accountattributename`` search is done first then the ``groupattribute`` allowing for backward compatibility with previous Salt releases. + +Jinja Include Relative Paths +============================ + +When a jinja include template name begins with './' or +'../' then the import will be relative to the importing file. + +Prior practices required the following construct: + +.. code-block:: jinja + {% from tpldir ~ '/foo' import bar %} + +A more "natural" construct is now supported: + +.. code-block:: jinja + {% from './foo' import bar %} + +Comparatively when importing from a parent directory - prior practice: + +.. code-block:: jinja + {% from tpldir ~ '/../foo' import bar %} + +New style for including from a parent directory: + +.. code-block:: jinja + {% from '../foo' import bar %} From c1c8b366390214e1de1946219ac6055f30f136fc Mon Sep 17 00:00:00 2001 From: Nicole Thomas Date: Fri, 8 Jun 2018 09:38:58 -0400 Subject: [PATCH 5/6] RST Syntax Fix: code-blocks need a blank line --- doc/topics/releases/fluorine.rst | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/doc/topics/releases/fluorine.rst b/doc/topics/releases/fluorine.rst index da84fa559c97..05d57b0638aa 100644 --- a/doc/topics/releases/fluorine.rst +++ b/doc/topics/releases/fluorine.rst @@ -598,25 +598,29 @@ releases. Jinja Include Relative Paths ============================ -When a jinja include template name begins with './' or -'../' then the import will be relative to the importing file. +When a jinja include template name begins with ``./`` or +``../`` then the import will be relative to the importing file. Prior practices required the following construct: .. code-block:: jinja - {% from tpldir ~ '/foo' import bar %} + + {% from tpldir ~ '/foo' import bar %} A more "natural" construct is now supported: .. code-block:: jinja - {% from './foo' import bar %} + + {% from './foo' import bar %} Comparatively when importing from a parent directory - prior practice: .. code-block:: jinja - {% from tpldir ~ '/../foo' import bar %} + + {% from tpldir ~ '/../foo' import bar %} New style for including from a parent directory: .. code-block:: jinja - {% from '../foo' import bar %} + + {% from '../foo' import bar %} From a7e633ebe88d837e262f7e403e34717d4a7620db Mon Sep 17 00:00:00 2001 From: Nicole Thomas Date: Fri, 8 Jun 2018 09:42:32 -0400 Subject: [PATCH 6/6] Lint: Remove extra whitespace --- salt/utils/jinja.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 509c68adf0a1..0533e723967f 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -106,7 +106,7 @@ def get_source(self, environment, template): name does not begin with './' or '../'. When a template name begins with './' or '../' then the import will be relative to the importing file. - + ''' # FIXME: somewhere do seprataor replacement: '\\' => '/' _template = template