Jinja sandbox refinement (#85385)

* DTFIX recategorize

* fix ansible-console generated task dict

* use non-deprecated task shape

* switch Jinja from ImmutableSandboxedEnvironment to SandboxedEnvironment

* Restore ability to call collection mutation methods.
* Restore ability to directly call integer bitwise operator methods.
* Adjust tests.

Co-authored-by: Matt Clay <matt@mystile.com>

---------

Co-authored-by: Matt Clay <matt@mystile.com>
(cherry picked from commit 11f5563895)

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__``.

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

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

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 | {

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,

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

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

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]'"

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

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."""