Don't reject all .. symlinks i

Fixes #1122

Author: spoorcc

CHANGELOG.rst

@@ -10,6 +10,7 @@ Release 0.14.0 (unreleased)
 * Allow ``dfetch freeze`` to accept project names to freeze only specific projects (#1063)
 * Edit manifest in-place when freezing inside a git or SVN superproject, preserving comments and layout (#1063)
 * Add new ``remove`` command to remove projects from manifest and disk (#26)
+* Fix "unsafe symlink target" error for archives containing relative ``..`` symlinks (#1122)
 
 Release 0.13.0 (released 2026-03-30)
 ====================================

dfetch/vcs/archive.py

@@ -44,6 +44,7 @@
 
 from dfetch.log import get_logger
 from dfetch.util.util import (
+    check_no_path_traversal,
     copy_directory_contents,
     copy_src_subset,
     prune_files_by_pattern,
@@ -325,6 +326,8 @@ def extract(
             else:
                 copy_directory_contents(extract_root, dest_dir)
 
+            ArchiveLocalRepo._check_symlinks_in_dest(dest_dir)
+
             if ignore:
                 prune_files_by_pattern(dest_dir, ignore)
 
@@ -384,20 +387,38 @@ def check_zip_members(zf: zipfile.ZipFile) -> list[zipfile.ZipInfo]:
 
     @staticmethod
     def _is_unsafe_symlink_target(target: str) -> bool:
-        r"""Return *True* when *target* is an unsafe symlink destination.
+        r"""Return *True* when *target* is an absolute symlink destination.
 
-        A target is unsafe if it is absolute or contains ``..`` components
-        under either POSIX or Windows path semantics, so that backslash-based
-        traversals like ``..\\..\\evil`` are caught on any host OS.
+        Absolute targets (POSIX ``/`` or Windows drive/UNC anchors) are
+        always unsafe regardless of context.  Relative targets that contain
+        ``..`` components are NOT rejected here; they are validated after
+        extraction by :meth:`_check_symlinks_in_dest` using the manifest
+        root as the safety boundary.
         """
         posix = pathlib.PurePosixPath(target)
         win = pathlib.PureWindowsPath(target)
-        return (
-            posix.is_absolute()
-            or bool(win.anchor)
-            or any(part == ".." for part in posix.parts)
-            or any(part == ".." for part in win.parts)
-        )
+        return posix.is_absolute() or bool(win.anchor)
+
+    @staticmethod
+    def _check_symlinks_in_dest(dest_dir: str) -> None:
+        """Raise *RuntimeError* if any symlink in *dest_dir* escapes the manifest root.
+
+        Walks *dest_dir* without following symlinks and calls
+        :func:`~dfetch.util.util.check_no_path_traversal` for every symlink
+        found.  Because :func:`os.path.realpath` follows the link to its
+        target, this catches any symlink—including deeply nested ones—whose
+        resolved destination lies outside the current working directory
+        (the manifest root, as set by :func:`~dfetch.util.util.in_directory`).
+
+        Raises:
+            RuntimeError: When a symlink resolves to a path outside the manifest root.
+        """
+        manifest_root = os.getcwd()
+        for root, dirs, files in os.walk(dest_dir, followlinks=False):
+            for name in dirs + files:
+                path = os.path.join(root, name)
+                if os.path.islink(path):
+                    check_no_path_traversal(path, manifest_root)
 
     @staticmethod
     def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None:

features/fetch-archive.feature

@@ -224,3 +224,33 @@ Feature: Fetching dependencies from an archive (tar/zip)
             | path           |
             | MyProject/LibA |
             | MyProject/LibB |
+
+    Scenario: Archive with internal relative symlink using .. is fetched safely
+        Given an archive "SomeProject.tar.gz" with the files
+            | path             |
+            | README.md        |
+            | other/target.mk  |
+        And the archive "SomeProject.tar.gz" contains a symlink "sub/dir/link.mk" pointing to "../../other/target.mk"
+        And the manifest 'dfetch.yaml' in MyProject
+            """
+            manifest:
+              version: '0.0'
+              projects:
+                - name: SomeProject
+                  url: some-remote-server/SomeProject.tar.gz
+                  vcs: archive
+            """
+        When I run "dfetch update" in MyProject
+        Then 'MyProject' looks like:
+            """
+            MyProject/
+                SomeProject/
+                    .dfetch_data.yaml
+                    README.md
+                    other/
+                        target.mk
+                    sub/
+                        dir/
+                            link.mk
+                dfetch.yaml
+            """

features/steps/archive_steps.py

@@ -70,6 +70,28 @@ def create_zip(archive_path: str, name: str, files: list[dict]) -> None:
             zf.writestr(member_path, content)
 
 
+def add_symlink_to_tar_gz(
+    archive_path: str, name: str, symlink_path: str, symlink_target: str
+) -> None:
+    """Append a symlink member to an existing .tar.gz archive.
+
+    Rewrites the archive because gzip does not support append mode in Python's tarfile.
+    """
+    existing: list[tuple[tarfile.TarInfo, bytes | None]] = []
+    with tarfile.open(archive_path, "r:gz") as tar:
+        for member in tar.getmembers():
+            fobj = tar.extractfile(member)
+            existing.append((member, fobj.read() if fobj else None))
+
+    with tarfile.open(archive_path, "w:gz") as tar:
+        for member, data in existing:
+            tar.addfile(member, io.BytesIO(data) if data is not None else None)
+        info = tarfile.TarInfo(name=f"{name}/{symlink_path}")
+        info.type = tarfile.SYMTYPE
+        info.linkname = symlink_target
+        tar.addfile(info)
+
+
 def _archive_url(context, filename: str) -> str:
     """Build the archive URL in the same format used by apply_manifest_substitutions.
 
@@ -139,3 +161,15 @@ def step_impl(context, name, license_id):
         info.size = len(content_bytes)
         tar.addfile(info, io.BytesIO(content_bytes))
     context.archive_url = _archive_url(context, f"{name}.tar.gz")
+
+
+@given(
+    'the archive "{name}.tar.gz" contains a symlink "{symlink_path}" pointing to "{symlink_target}"'
+)
+def step_impl(context, name, symlink_path, symlink_target):
+    server_path = context.remotes_dir_path
+    archive_path = os.path.join(server_path, f"{name}.tar.gz")
+    add_symlink_to_tar_gz(archive_path, name, symlink_path, symlink_target)
+    context.archive_sha256 = _file_digest(archive_path, hashlib.sha256)
+    context.archive_sha384 = _file_digest(archive_path, hashlib.sha384)
+    context.archive_sha512 = _file_digest(archive_path, hashlib.sha512)

tests/test_archive.py

@@ -156,17 +156,15 @@ def test_check_zip_members_symlink_absolute_target():
 
 
 def test_check_zip_members_symlink_dotdot_target():
-    """ZIP symlink pointing outside extraction dir via .. must be rejected."""
+    """ZIP symlink with .. is allowed at pre-extraction time (post-copy check enforces boundary)."""
     zf = _make_zip_with_symlink("link", "../../etc/shadow")
-    with pytest.raises(RuntimeError, match="symlink"):
-        _check_zip_members(zf)
+    _check_zip_members(zf)  # must NOT raise
 
 
 def test_check_zip_members_symlink_windows_dotdot_target():
-    """ZIP symlink with Windows-style backslash traversal must be rejected."""
+    """ZIP symlink with Windows-style backslash .. is allowed at pre-extraction time."""
     zf = _make_zip_with_symlink("link", "..\\..\\evil")
-    with pytest.raises(RuntimeError, match="symlink"):
-        _check_zip_members(zf)
+    _check_zip_members(zf)  # must NOT raise
 
 
 def test_check_zip_members_symlink_safe_relative():
@@ -269,18 +267,17 @@ def test_check_tar_member_type_absolute_symlink():
 
 
 def test_check_tar_member_type_dotdot_symlink():
+    """Relative .. symlinks are allowed at pre-extraction time (post-copy check enforces boundary)."""
     tf = _make_tar_with_member(lambda t: _add_symlink(t, "link", "../../etc/passwd"))
     member = tf.getmembers()[0]
-    with pytest.raises(RuntimeError, match="unsafe target"):
-        _check_tar_member_type(member)
+    _check_tar_member_type(member)  # must NOT raise
 
 
 def test_check_tar_member_type_windows_dotdot_symlink():
-    """TAR symlink with Windows-style backslash traversal must be rejected."""
+    """Windows-style .. symlinks are allowed at pre-extraction time."""
     tf = _make_tar_with_member(lambda t: _add_symlink(t, "link", "..\\..\\evil"))
     member = tf.getmembers()[0]
-    with pytest.raises(RuntimeError, match="unsafe target"):
-        _check_tar_member_type(member)
+    _check_tar_member_type(member)  # must NOT raise
 
 
 # ---------------------------------------------------------------------------
@@ -346,6 +343,49 @@ def test_check_tar_members_rejects_device_file():
         _check_tar_members(tf)
 
 
+# ---------------------------------------------------------------------------
+# ArchiveLocalRepo._check_symlinks_in_dest
+# ---------------------------------------------------------------------------
+
+
+def test_check_symlinks_in_dest_safe_internal_dotdot(tmp_path, monkeypatch):
+    """A symlink with .. that stays within dest_dir must be accepted."""
+    monkeypatch.chdir(tmp_path)
+    dest = tmp_path / "pkg"
+    dest.mkdir()
+    (dest / "other").mkdir()
+    (dest / "other" / "target.mk").write_text("content")
+    sub = dest / "sub" / "dir"
+    sub.mkdir(parents=True)
+    (sub / "link.mk").symlink_to("../../other/target.mk")
+
+    ArchiveLocalRepo._check_symlinks_in_dest(str(dest))  # must NOT raise
+
+
+def test_check_symlinks_in_dest_escaping_symlink(tmp_path, monkeypatch):
+    """A symlink that resolves outside the manifest root must be rejected."""
+    monkeypatch.chdir(tmp_path)
+    dest = tmp_path / "pkg"
+    dest.mkdir()
+    (dest / "evil.txt").symlink_to("../../../etc/passwd")
+
+    with pytest.raises(RuntimeError):
+        ArchiveLocalRepo._check_symlinks_in_dest(str(dest))
+
+
+def test_check_symlinks_in_dest_sibling_within_manifest(tmp_path, monkeypatch):
+    """A symlink pointing to a sibling project (still within manifest root) is accepted."""
+    monkeypatch.chdir(tmp_path)
+    sibling = tmp_path / "sibling"
+    sibling.mkdir()
+    (sibling / "file.txt").write_text("content")
+    dest = tmp_path / "pkg"
+    dest.mkdir()
+    (dest / "link.txt").symlink_to("../sibling/file.txt")
+
+    ArchiveLocalRepo._check_symlinks_in_dest(str(dest))  # must NOT raise
+
+
 # ---------------------------------------------------------------------------
 # ArchiveRemote.is_accessible
 # ---------------------------------------------------------------------------