diff --git a/changelogs/fragments/template-sandbox.yml b/changelogs/fragments/template-sandbox.yml new file mode 100644 index 00000000000..c1336db7506 --- /dev/null +++ b/changelogs/fragments/template-sandbox.yml @@ -0,0 +1,5 @@ +minor_changes: + - templating - Switched from the Jinja immutable sandbox to the standard sandbox. + This restores the ability to use mutation methods such as ``list.append`` and ``dict.update``. + - templating - Relaxed the Jinja sandbox to allow specific bitwise operations which have no filter equivalent. + The allowed methods are ``__and__``, ``__lshift__``, ``__or__``, ``__rshift__``, ``__xor__``. diff --git a/lib/ansible/_internal/_json/__init__.py b/lib/ansible/_internal/_json/__init__.py index 069e51dac5d..21f56f68a79 100644 --- a/lib/ansible/_internal/_json/__init__.py +++ b/lib/ansible/_internal/_json/__init__.py @@ -144,16 +144,16 @@ class AnsibleVariableVisitor: value = self._template_engine.transform(value) value_type = type(value) - # DTFIX3: need to handle native copy for keys too + # DTFIX0: need to handle native copy for keys too if self.convert_to_native_values and isinstance(value, _datatag.AnsibleTaggedObject): value = value._native_copy() value_type = type(value) result: _T - # DTFIX3: the visitor is ignoring dict/mapping keys except for debugging and schema-aware checking, it should be doing type checks on keys + # DTFIX0: the visitor is ignoring dict/mapping keys except for debugging and schema-aware checking, it should be doing type checks on keys # keep in mind the allowed types for keys is a more restrictive set than for values (str and tagged str only, not EncryptedString) - # DTFIX5: some type lists being consulted (the ones from datatag) are probably too permissive, and perhaps should not be dynamic + # DTFIX0: some type lists being consulted (the ones from datatag) are probably too permissive, and perhaps should not be dynamic if (result := self._early_visit(value, value_type)) is not _sentinel: pass diff --git a/lib/ansible/_internal/_templating/_jinja_bits.py b/lib/ansible/_internal/_templating/_jinja_bits.py index 82aedf5b670..d7fdb1e48fc 100644 --- a/lib/ansible/_internal/_templating/_jinja_bits.py +++ b/lib/ansible/_internal/_templating/_jinja_bits.py @@ -20,7 +20,7 @@ from jinja2.lexer import TOKEN_VARIABLE_BEGIN, TOKEN_VARIABLE_END, TOKEN_STRING, from jinja2.nativetypes import NativeCodeGenerator from jinja2.nodes import Const, EvalContext from jinja2.runtime import Context, Macro -from jinja2.sandbox import ImmutableSandboxedEnvironment +from jinja2.sandbox import SandboxedEnvironment from jinja2.utils import missing, LRUCache from ansible.utils.display import Display @@ -518,7 +518,7 @@ def create_template_error(ex: Exception, variable: t.Any, is_expression: bool) - return exception_to_raise -# DTFIX3: implement CapturedExceptionMarker deferral support on call (and lookup), filter/test plugins, etc. +# DTFIX1: implement CapturedExceptionMarker deferral support on call (and lookup), filter/test plugins, etc. # also update the protomatter integration test once this is done (the test was written differently since this wasn't done yet) _BUILTIN_FILTER_ALIASES: dict[str, str] = {} @@ -535,7 +535,7 @@ _BUILTIN_FILTERS = filter_loader._wrap_funcs(defaults.DEFAULT_FILTERS, _BUILTIN_ _BUILTIN_TESTS = test_loader._wrap_funcs(t.cast(dict[str, t.Callable], defaults.DEFAULT_TESTS), _BUILTIN_TEST_ALIASES) -class AnsibleEnvironment(ImmutableSandboxedEnvironment): +class AnsibleEnvironment(SandboxedEnvironment): """ Our custom environment, which simply allows us to override the class-level values for the Template and Context classes used by jinja2 internally. @@ -546,6 +546,21 @@ class AnsibleEnvironment(ImmutableSandboxedEnvironment): code_generator_class = AnsibleCodeGenerator intercepted_binops = frozenset(('eq',)) + _allowed_unsafe_attributes: dict[str, type | tuple[type, ...]] = dict( + # Allow bitwise operations on int until bitwise filters are available. + # see: https://github.com/ansible/ansible/issues/85204 + __and__=int, + __lshift__=int, + __or__=int, + __rshift__=int, + __xor__=int, + ) + """ + Attributes which are considered unsafe by `is_safe_attribute`, which should be allowed when used on specific types. + The attributes allowed here are intended only for backward compatibility with existing use cases. + They should be exposed as filters in a future release and eventually deprecated. + """ + _lexer_cache = LRUCache(50) # DTFIX-FUTURE: bikeshed a name/mechanism to control template debugging @@ -609,6 +624,9 @@ class AnsibleEnvironment(ImmutableSandboxedEnvironment): if _TemplateConfig.sandbox_mode == _SandboxMode.ALLOW_UNSAFE_ATTRIBUTES: return True + if (type_or_tuple := self._allowed_unsafe_attributes.get(attr)) and isinstance(obj, type_or_tuple): + return True + return super().is_safe_attribute(obj, attr, value) @property diff --git a/lib/ansible/_internal/_templating/_utils.py b/lib/ansible/_internal/_templating/_utils.py index 7c6be307f31..5cd72201450 100644 --- a/lib/ansible/_internal/_templating/_utils.py +++ b/lib/ansible/_internal/_templating/_utils.py @@ -99,7 +99,7 @@ Omit = object.__new__(_OmitType) _datatag._untaggable_types.add(_OmitType) -# DTFIX5: review these type sets to ensure they're not overly permissive/dynamic +# DTFIX0: review these type sets to ensure they're not overly permissive/dynamic IGNORE_SCALAR_VAR_TYPES = {value for value in _datatag._ANSIBLE_ALLOWED_SCALAR_VAR_TYPES if not issubclass(value, str)} PASS_THROUGH_SCALAR_VAR_TYPES = _datatag._ANSIBLE_ALLOWED_SCALAR_VAR_TYPES | { diff --git a/lib/ansible/cli/console.py b/lib/ansible/cli/console.py index aff75bf6dc9..5a6f941664f 100755 --- a/lib/ansible/cli/console.py +++ b/lib/ansible/cli/console.py @@ -194,7 +194,7 @@ class ConsoleCLI(CLI, cmd.Cmd): result = None try: check_raw = module in C._ACTION_ALLOWS_RAW_ARGS - task = dict(action=dict(module=module, args=parse_kv(module_args, check_raw=check_raw)), timeout=self.task_timeout) + task = dict(action=module, args=parse_kv(module_args, check_raw=check_raw), timeout=self.task_timeout) play_ds = dict( name="Ansible Shell", hosts=self.cwd, diff --git a/lib/ansible/executor/module_common.py b/lib/ansible/executor/module_common.py index a18088bca61..150ed9acc79 100644 --- a/lib/ansible/executor/module_common.py +++ b/lib/ansible/executor/module_common.py @@ -954,7 +954,7 @@ class _BuiltModule: class _CachedModule: """Cached Python module created by AnsiballZ.""" - # DTFIX5: secure this (locked down pickle, don't use pickle, etc.) + # FIXME: switch this to use a locked down pickle config or don't use pickle- easy to mess up and reach objects that shouldn't be pickled zip_data: bytes metadata: ModuleMetadata diff --git a/test/integration/targets/callback-dispatch/callback_plugins/legacy_warning_display.py b/test/integration/targets/callback-dispatch/callback_plugins/legacy_warning_display.py index ed9dc00e9a9..b4a4d456160 100644 --- a/test/integration/targets/callback-dispatch/callback_plugins/legacy_warning_display.py +++ b/test/integration/targets/callback-dispatch/callback_plugins/legacy_warning_display.py @@ -10,7 +10,7 @@ from ansible.plugins.callback import CallbackBase class CallbackModule(CallbackBase): - # DTFIX5: validate VaultedValue redaction behavior + # DTFIX1: validate VaultedValue redaction behavior CALLBACK_NEEDS_ENABLED = True seen_tr = [] # track taskresult instances to ensure every call sees a unique instance diff --git a/test/integration/targets/protomatter/tasks/main.yml b/test/integration/targets/protomatter/tasks/main.yml index 4931deeeb2a..43b9837adea 100644 --- a/test/integration/targets/protomatter/tasks/main.yml +++ b/test/integration/targets/protomatter/tasks/main.yml @@ -50,7 +50,7 @@ assert: that: - "'[1, 2]' | ansible._protomatter.python_literal_eval == [1, 2]" - # DTFIX5: This test requires fixing plugin captured error handling first. + # DTFIX1: This test requires fixing plugin captured error handling first. # Once fixed, the error handling test below can be replaced by this assert. # - "'x[1, 2]' | ansible._protomatter.python_literal_eval | true_type == 'CapturedExceptionMarker'" - "'x[1, 2]' | ansible._protomatter.python_literal_eval(ignore_errors=True) == 'x[1, 2]'" diff --git a/test/units/_internal/templating/test_jinja_bits.py b/test/units/_internal/templating/test_jinja_bits.py index 715660d3159..ad32166da70 100644 --- a/test/units/_internal/templating/test_jinja_bits.py +++ b/test/units/_internal/templating/test_jinja_bits.py @@ -235,17 +235,21 @@ undefined_with_unsafe = AnsibleUndefinedVariable("is unsafe") ('on_dict["get"]', ok), # [] prefers getitem, matching dict key present (no attr lookup) ('on_dict.get("_native_copy")', ok), # . matches safe method on dict, should be callable to fetch a valid key ('on_dict["clear"]', ok), # [] prefers getitem, matching dict key present (no attr lookup) - ('on_dict.clear', ok), # . matches known-mutating method on _AnsibleTaggedDict, custom fallback to getitem with valid key - ('on_dict["setdefault"]', undefined_with_unsafe), # [] finds no matching dict key, getattr fallback matches known-mutating method, fails - ('on_dict.setdefault', undefined_with_unsafe), # . finds a known-mutating method, getitem fallback finds no matching dict key, fails ('on_dict["_non_method_or_attr"]', ok), # [] prefers getitem, sunder key ok ('on_dict._non_method_or_attr', ok), # . finds nothing, getattr fallback finds dict key, `_` prefix has no effect - ('on_list.sort', undefined_with_unsafe), # . matches known-mutating method on list, fails - ('on_list["sort"]', undefined_with_unsafe), # [] gets TypeError, getattr fallback matches known-mutating method on list, fails ('on_list._native_copy', undefined_with_unsafe), # . matches sunder-named method on list, fails ('on_list["_native_copy"]', undefined_with_unsafe), # [] gets TypeError, getattr fallback matches sunder-named method on list, fails ('on_list.0', 42), # . gets AttributeError, getitem fallback succeeds ('on_list[0]', 42), # [] prefers getitem, succeeds + # -- Jinja mutable method sandbox test cases follow; if sandbox is re-enabled, the correct behavior is defined by the commented value below + ('on_dict.clear | type_debug', 'builtin_function_or_method'), + # ('on_dict.clear', ok), # . matches known-mutating method on _AnsibleTaggedDict, custom fallback to getitem with valid key + ('on_dict["setdefault"] | type_debug', 'method'), + # ('on_dict.setdefault', undefined_with_unsafe), # . finds a known-mutating method, getitem fallback finds no matching dict key, fails + ('on_list.sort | type_debug', 'method'), + # ('on_list.sort', undefined_with_unsafe), # . matches known-mutating method on list, fails + ('on_list["sort"] | type_debug', 'method'), + # ('on_list["sort"]', undefined_with_unsafe), # [] gets TypeError, getattr fallback matches known-mutating method on list, fails )) def test_jinja_getattr(expr: str, expected: object) -> None: """Validate expected behavior from Jinja environment getattr/getitem methods, including Ansible-customized fallback behavior.""" @@ -401,3 +405,41 @@ def test_macro_marker_handling(template: str, variables: dict[str, object], expe res = TemplateEngine(variables=variables).template(TRUST.tag(template)) assert res == expected + + +@pytest.mark.parametrize("expression, result", ( + ("(0x20).__or__(0xf)", 47), + ("(0x20).__and__(0x29)", 32), + ("(0x20).__lshift__(1)", 64), + ("(0x20).__rshift__(1)", 16), + ("(0x20).__xor__(0x29)", 9), +)) +def test_bitwise_dunder_methods(expression: str, result: object) -> None: + """ + Verify a limited set of dunder methods are supported. + This feature may be deprecated and removed in the future after bitwise filters are implemented. + """ + assert TemplateEngine().evaluate_expression(TRUST.tag(expression)) == result + + +@pytest.mark.parametrize("expression", ( + "{1:2}.__delitem__(1)", + "[123].__len__()", +)) +def test_disallowed_dunder_methods(expression: str) -> None: + """Verify dunder methods are disallowed by the Jinja sandbox.""" + with pytest.raises(AnsibleUndefinedVariable, match="is unsafe"): + TemplateEngine().evaluate_expression(TRUST.tag(expression)) + + +@pytest.mark.parametrize("template, result", ( + ("{% set my_list = [] %}{% set _ = my_list.append(1) %}{{ my_list }}", [1]), + ("{% set my_list = [] %}{% set _ = my_list.extend([1, 2]) %}{{ my_list }}", [1, 2]), + ("{% set my_dict = {} %}{% set _ = my_dict.update(a=1) %}{{ my_dict }}", dict(a=1)), +)) +def test_mutation_methods(template: str, result: object) -> None: + """ + Verify mutation methods are allowed by the Jinja sandbox. + This feature may be deprecated and removed in a future release by using Jinja's ImmutableSandboxedEnvironment. + """ + assert TemplateEngine().template(TRUST.tag(template)) == result diff --git a/test/units/_internal/templating/test_templar.py b/test/units/_internal/templating/test_templar.py index 72c876e4256..ed1c5eb40bb 100644 --- a/test/units/_internal/templating/test_templar.py +++ b/test/units/_internal/templating/test_templar.py @@ -1061,9 +1061,7 @@ def test_jinja_const_template_finalized() -> None: @pytest.mark.parametrize("template,expected", ( - ("{% set x=[] %}{% set _=x.append(42) %}{{ x }}", [42]), - ("{{ (32).__or__(64) }}", 96), - ("{% set x={'foo': 42} %}{% set _=x.clear() %}{{ x }}", {}), + ("{{ (-1).__abs__() }}", 1), )) def test_unsafe_attr_access(template: str, expected: object) -> None: """Verify that unsafe attribute access fails by default and works when explicitly configured."""