unsafe wrapping should only happen for with_ loops (#64401)

* unsafe wrapping should only happen for with_ lookups. Fixes #64379. Addresses #64169

* edit porting guide entry

* typo in changelog fragment

* typo

Co-Authored-By: Sandra McCann <samccann@redhat.com>

* punctuation

Co-Authored-By: Sandra McCann <samccann@redhat.com>

changelogs/fragments/64379-no-loop-unsafe.yml

@@ -0,0 +1,5 @@
+bugfixes:
+- loops - Do not indiscriminately mark loop items as unsafe, only apply unsafe
+  to ``with_`` style loops. The items from ``loop`` should not be explicitly
+  wrapped in unsafe. The underlying templating mechanism should dictate this.
+  (https://github.com/ansible/ansible/issues/64379)

docs/docsite/rst/porting_guides/porting_guide_2.9.rst

@@ -19,8 +19,19 @@ This document is part of a collection on porting. The complete list of porting g
 Playbook
 ========
 
+Inventory
+---------
+
  * ``hash_behaviour`` now affects inventory sources. If you have it set to ``merge``, the data you get from inventory might change and you will have to update playbooks accordingly. If you're using the default setting (``overwrite``), you will see no changes. Inventory was ignoring this setting.
 
+Loops
+-----
+
+Ansible 2.9 handles "unsafe" data more robustly, ensuring that data marked "unsafe" is not templated. In previous versions, Ansible recursively marked all data returned by the direct use of ``lookup()`` as "unsafe", but only marked structured data returned by indirect lookups using ``with_X`` style loops as "unsafe" if the returned elements were strings. Ansible 2.9 treats these two approaches consistently.
+
+As a result, if you use ``with_dict`` to return keys with templatable values, your templates may no longer work as expected in Ansible 2.9.
+
+To allow the old behavior, switch from using ``with_X`` to using ``loop`` with a filter as described at :ref:`migrating_to_loop`.
 
 Command Line
 ============

lib/ansible/executor/task_executor.py

@@ -242,7 +242,7 @@ class TaskExecutor:
                 setattr(mylookup, '_subdir', subdir + 's')
 
                 # run lookup
-                items = mylookup.run(terms=loop_terms, variables=self._job_vars, wantlist=True)
+                items = wrap_var(mylookup.run(terms=loop_terms, variables=self._job_vars, wantlist=True))
             else:
                 raise AnsibleError("Unexpected failure in finding the lookup named '%s' in the available lookup plugins" % self._task.loop_with)
 
@@ -264,11 +264,6 @@ class TaskExecutor:
             else:
                 del self._job_vars[k]
 
-        if items:
-            for idx, item in enumerate(items):
-                if item is not None and not isinstance(item, AnsibleUnsafe):
-                    items[idx] = wrap_var(item)
-
         return items
 
     def _run_loop(self, items):

lib/ansible/utils/unsafe_proxy.py

@@ -115,7 +115,7 @@ def _wrap_set(v):
 
 
 def wrap_var(v):
-    if isinstance(v, AnsibleUnsafe):
+    if v is None or isinstance(v, AnsibleUnsafe):
         return v
 
     if isinstance(v, Mapping):

lib/ansible/vars/manager.py

@@ -509,7 +509,7 @@ class VariableManager:
                 try:
                     loop_terms = listify_lookup_plugin_terms(terms=task.loop, templar=templar,
                                                              loader=self._loader, fail_on_undefined=True, convert_bare=False)
-                    items = lookup_loader.get(task.loop_with, loader=self._loader, templar=templar).run(terms=loop_terms, variables=vars_copy)
+                    items = wrap_var(lookup_loader.get(task.loop_with, loader=self._loader, templar=templar).run(terms=loop_terms, variables=vars_copy))
                 except AnsibleTemplateError:
                     # This task will be skipped later due to this, so we just setup
                     # a dummy array for the later code so it doesn't fail

test/integration/targets/loops/tasks/main.yml

@@ -359,3 +359,33 @@
 - assert:
     that:
       - loop_out['results'][1]['ansible_remote_tmp'] == '/tmp/test1'
+
+# https://github.com/ansible/ansible/issues/64169
+- include_vars: 64169.yml
+
+- set_fact: "{{ item.key }}={{ hostvars[inventory_hostname][item.value] }}"
+  with_dict:
+    foo: __foo
+
+- debug:
+    var: foo
+
+- assert:
+    that:
+      - foo[0] != 'foo1.0'
+      - foo[0] == unsafe_value
+  vars:
+    unsafe_value: !unsafe 'foo{{ version_64169 }}'
+
+- set_fact: "{{ item.key }}={{ hostvars[inventory_hostname][item.value] }}"
+  loop: "{{ dicty_dict|dict2items }}"
+  vars:
+    dicty_dict:
+      foo: __foo
+
+- debug:
+    var: foo
+
+- assert:
+    that:
+      - foo[0] == 'foo1.0'

test/integration/targets/loops/vars/64169.yml

@@ -0,0 +1,2 @@
+__foo:
+  - "foo{{ version_64169 }}"

test/integration/targets/loops/vars/main.yml

@@ -5,3 +5,4 @@ phrases:
 filenames:
   - 'data1.txt'
   - 'data2.txt'
+version_64169: '1.0'