Fix installing roles containing symlinks (ansible#82911)

* Fix installing roles containing symlinks

Fix sanitizing tarfile symlinks relative to the link directory instead of the archive

For example:

role
├── handlers
│   └── utils.yml -> ../tasks/utils/suite.yml

The link ../tasks/utils/suite.yml will resolve to a path outside of the link's directory, but within the role

role/handlers/../tasks/utils/suite.yml

the resolved path relative to the role is tasks/utils/suite.yml, but if the symlink is set to that value, tarfile would extract it from role/handlers/tasks/utils/suite.yml

* Replace overly forgiving test case with tests for a symlink in a subdirectory of the archive and a symlink in the archive dir when these are not equivalent.

* Build test case from role files to make it easier to add test cases

Fixes ansible#82702
Fixes ansible#81965
Fixes ansible#82051

changelogs/fragments/ansible-galaxy-role-install-symlink.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ansible-galaxy role install - fix symlinks (https://github.com/ansible/ansible/issues/82702, https://github.com/ansible/ansible/issues/81965).

lib/ansible/galaxy/role.py

@@ -386,6 +386,8 @@ def install(self):
                         else:
                             os.makedirs(self.path)
 
+                        resolved_archive = unfrackpath(archive_parent_dir, follow=False)
+
                         # We strip off any higher-level directories for all of the files
                         # contained within the tar file here. The default is 'github_repo-target'.
                         # Gerrit instances, on the other hand, does not have a parent directory at all.
@@ -400,33 +402,29 @@ def install(self):
                                 if not (attr_value := getattr(member, attr, None)):
                                     continue
 
-                                if attr_value.startswith(os.sep) and not is_subpath(attr_value, archive_parent_dir):
-                                    err = f"Invalid {attr} for tarfile member: path {attr_value} is not a subpath of the role {archive_parent_dir}"
-                                    raise AnsibleError(err)
-
                                 if attr == 'linkname':
                                     # Symlinks are relative to the link
-                                    relative_to_archive_dir = os.path.dirname(getattr(member, 'name', ''))
-                                    archive_dir_path = os.path.join(archive_parent_dir, relative_to_archive_dir, attr_value)
+                                    relative_to = os.path.dirname(getattr(member, 'name', ''))
                                 else:
                                     # Normalize paths that start with the archive dir
                                     attr_value = attr_value.replace(archive_parent_dir, "", 1)
                                     attr_value = os.path.join(*attr_value.split(os.sep))  # remove leading os.sep
-                                    archive_dir_path = os.path.join(archive_parent_dir, attr_value)
+                                    relative_to = ''
 
-                                resolved_archive = unfrackpath(archive_parent_dir)
-                                resolved_path = unfrackpath(archive_dir_path)
-                                if not is_subpath(resolved_path, resolved_archive):
-                                    err = f"Invalid {attr} for tarfile member: path {resolved_path} is not a subpath of the role {resolved_archive}"
+                                full_path = os.path.join(resolved_archive, relative_to, attr_value)
+                                if not is_subpath(full_path, resolved_archive, real=True):
+                                    err = f"Invalid {attr} for tarfile member: path {full_path} is not a subpath of the role {resolved_archive}"
                                     raise AnsibleError(err)
 
-                                relative_path = os.path.join(*resolved_path.replace(resolved_archive, "", 1).split(os.sep)) or '.'
+                                relative_path_dir = os.path.join(resolved_archive, relative_to)
+                                relative_path = os.path.join(*full_path.replace(relative_path_dir, "", 1).split(os.sep))
                                 setattr(member, attr, relative_path)
 
                             if _check_working_data_filter():
                                 # deprecated: description='extract fallback without filter' python_version='3.11'
                                 role_tar_file.extract(member, to_native(self.path), filter='data')  # type: ignore[call-arg]
                             else:
+                                # Remove along with manual path filter once Python 3.12 is minimum supported version
                                 role_tar_file.extract(member, to_native(self.path))
 
                         # write out the install info file for later use

test/integration/targets/ansible-galaxy-role/files/safe-symlinks/defaults/main.yml

@@ -0,0 +1 @@
+common_vars/subdir/group0/main.yml

test/integration/targets/ansible-galaxy-role/files/safe-symlinks/handlers/utils.yml

@@ -0,0 +1 @@
+../tasks/utils/suite.yml

test/integration/targets/ansible-galaxy-role/tasks/valid-role-symlinks.yml

@@ -1,78 +1,38 @@
-- name: create test directories
-  file:
-    path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}'
-    state: directory
-  loop:
-    - source
-    - target
-    - roles
-
-- name: create subdir in the role content to test relative symlinks
-  file:
-    dest: '{{ remote_tmp_dir }}/dir-traversal/source/role_subdir'
-    state: directory
-
-- copy:
-    dest: '{{ remote_tmp_dir }}/dir-traversal/source/role_subdir/.keep'
-    content: ''
-
-- set_fact:
-    installed_roles: "{{ remote_tmp_dir | realpath }}/dir-traversal/roles"
-
-- name: build role with symlink to a directory in the role
-  script:
-    chdir: '{{ remote_tmp_dir }}/dir-traversal/source'
-    cmd: create-role-archive.py safe-link-dir.tar ./ role_subdir/..
-    executable: '{{ ansible_playbook_python }}'
-
-- name: install role successfully
-  command:
-    cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles safe-link-dir.tar'
-    chdir: '{{ remote_tmp_dir }}/dir-traversal/source'
-  register: galaxy_install_ok
-
-- name: check for the directory symlink in the role
-  stat:
-    path: "{{ installed_roles }}/safe-link-dir.tar/symlink"
-  register: symlink_in_role
-
-- assert:
-    that:
-      - symlink_in_role.stat.exists
-      - symlink_in_role.stat.lnk_source == installed_roles + '/safe-link-dir.tar'
-
-- name: remove tarfile for next test
-  file:
-    path: '{{ remote_tmp_dir }}/dir-traversal/source/safe-link-dir.tar'
-    state: absent
-
-- name: build role with safe relative symlink
-  script:
-    chdir: '{{ remote_tmp_dir }}/dir-traversal/source'
-    cmd: create-role-archive.py safe.tar ./ role_subdir/../context.txt
-    executable: '{{ ansible_playbook_python }}'
-
-- name: install role successfully
-  command:
-    cmd: 'ansible-galaxy role install --roles-path {{ remote_tmp_dir }}/dir-traversal/roles safe.tar'
-    chdir: '{{ remote_tmp_dir }}/dir-traversal/source'
-  register: galaxy_install_ok
-
-- name: check for symlink in role
-  stat:
-    path: "{{ installed_roles }}/safe.tar/symlink"
-  register: symlink_in_role
-
-- assert:
-    that:
-      - symlink_in_role.stat.exists
-      - symlink_in_role.stat.lnk_source == installed_roles + '/safe.tar/context.txt'
-
-- name: remove test directories
-  file:
-    path: '{{ remote_tmp_dir }}/dir-traversal/{{ item }}'
-    state: absent
-  loop:
-    - source
-    - target
-    - roles
+- delegate_to: localhost
+  block:
+  - name: Create archive
+    command: "tar -cf safe-symlinks.tar {{ role_path }}/files/safe-symlinks"
+    args:
+      chdir: "{{ remote_tmp_dir }}"
+
+  - name: Install role successfully
+    command: ansible-galaxy role install --roles-path '{{ remote_tmp_dir }}/roles' safe-symlinks.tar
+    args:
+      chdir: "{{ remote_tmp_dir }}"
+
+  - name: Validate each of the symlinks exists
+    stat:
+      path: "{{ remote_tmp_dir }}/roles/safe-symlinks.tar/{{ item }}"
+    loop:
+      - defaults/main.yml
+      - handlers/utils.yml
+    register: symlink_stat
+
+  - assert:
+      that:
+        - symlink_stat.results[0].stat.exists
+        - symlink_stat.results[0].stat.lnk_source == ((dest, 'roles/safe-symlinks.tar/defaults/common_vars/subdir/group0/main.yml') | path_join)
+        - symlink_stat.results[1].stat.exists
+        - symlink_stat.results[1].stat.lnk_source == ((dest, 'roles/safe-symlinks.tar/tasks/utils/suite.yml') | path_join)
+    vars:
+      dest: "{{ remote_tmp_dir | realpath }}"
+
+  always:
+  - name: Clean up
+    file:
+      path: "{{ item }}"
+      state: absent
+    delegate_to: localhost
+    loop:
+      - "{{ remote_tmp_dir }}/roles/"
+      - "{{ remote_tmp_dir }}/safe-symlinks.tar"