From 618432748f3896a6ee727282a2e82caec8c46918 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 26 Mar 2026 23:35:15 +0000 Subject: [PATCH 1/7] Security fixes: ZIP symlink traversal, absolute dst paths, SVN rev injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three vulnerabilities found via white-hat audit and fixed with failing-first tests: 1. ZIP symlink path traversal (HIGH) - check_zip_members() only validated filenames but not Unix symlinks encoded in external_attr. A crafted ZIP with a symlink pointing to ../../etc/passwd would extract a real symlink, escaping the extraction directory. - Fix: add _check_zip_member_type() (mirrors _check_tar_member_type) that detects S_IFLNK in external_attr and validates the symlink target. - Tests: test_check_zip_members_symlink_{absolute,dotdot,safe_relative}_target 2. validate_destination() missing absolute path check (MEDIUM) - validate_destination() rejected ".." components but not absolute paths. Path("/etc/passwd").parts has no ".." so "/etc/passwd" passed silently. - Fix: add is_absolute() guard before the ".." check. - Tests: test_validate_destination_rejects_absolute_paths 3. SVN rev.split(" ") argument injection API (LOW) - SvnRepo.export() accepted a pre-formatted "–-revision N" string and split it on spaces, creating an API that enables option injection if any caller bypasses digit-only validation in svnsubproject.py. - Fix: accept a bare digit-only revision string, pass ["--revision", rev] directly; raise ValueError for non-digit input. - Tests: test_svn_vcs.py (new file) https://claude.ai/code/session_01CxKrTZekNRCoSD9PSMekQZ --- dfetch/manifest/manifest.py | 5 +++++ dfetch/project/svnsubproject.py | 5 ++--- dfetch/vcs/archive.py | 27 ++++++++++++++++++++++- dfetch/vcs/svn.py | 16 ++++++++++++-- tests/test_archive.py | 32 +++++++++++++++++++++++++++ tests/test_manifest.py | 23 ++++++++++++++++++++ tests/test_svn_vcs.py | 38 +++++++++++++++++++++++++++++++++ 7 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 tests/test_svn_vcs.py diff --git a/dfetch/manifest/manifest.py b/dfetch/manifest/manifest.py index 78bd0083..86335592 100644 --- a/dfetch/manifest/manifest.py +++ b/dfetch/manifest/manifest.py @@ -395,6 +395,11 @@ def validate_project_name(self, name: str) -> None: @staticmethod def validate_destination(dst: str) -> None: """Raise ValueError if *dst* is not a safe manifest destination path.""" + if Path(dst).is_absolute(): + raise ValueError( + f"Destination '{dst}' is an absolute path. " + "Paths must be relative to the manifest directory." + ) if any(part == ".." for part in Path(dst).parts): raise ValueError( f"Destination '{dst}' contains '..'. " diff --git a/dfetch/project/svnsubproject.py b/dfetch/project/svnsubproject.py index e4942e82..3a1cdfdf 100644 --- a/dfetch/project/svnsubproject.py +++ b/dfetch/project/svnsubproject.py @@ -116,7 +116,6 @@ def _remove_ignored_files(self) -> None: def _fetch_impl(self, version: Version) -> tuple[Version, list[Dependency]]: """Get the revision of the remote and place it at the local path.""" branch, branch_path, revision = self._determine_what_to_fetch(version) - rev_arg = f"--revision {revision}" if revision else "" complete_path = "/".join( filter(None, [self.remote, branch_path.strip(), self.source]) @@ -129,7 +128,7 @@ def _fetch_impl(self, version: Version) -> tuple[Version, list[Dependency]]: complete_path, file_pattern = self._parse_file_pattern(complete_path) - SvnRepo.export(complete_path, rev_arg, self.local_path) + SvnRepo.export(complete_path, revision, self.local_path) if file_pattern: for file in find_non_matching_files(self.local_path, (file_pattern,)): @@ -148,7 +147,7 @@ def _fetch_impl(self, version: Version) -> tuple[Version, list[Dependency]]: if os.path.isdir(self.local_path) else os.path.dirname(self.local_path) ) - SvnRepo.export(f"{root_branch_path}/{license_files[0]}", rev_arg, dest) + SvnRepo.export(f"{root_branch_path}/{license_files[0]}", revision, dest) if self.ignore: self._remove_ignored_files() diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index b8ce7216..5df3b3d8 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -28,6 +28,7 @@ import os import pathlib import shutil +import stat import sys import tarfile import tempfile @@ -353,7 +354,8 @@ def check_zip_members(zf: zipfile.ZipFile) -> list[zipfile.ZipInfo]: Raises: RuntimeError: When any member contains an absolute path, a ``..`` - component, or when the archive exceeds the size/count limits. + component, a symlink with an unsafe target, or when the archive + exceeds the size/count limits. """ members = zf.infolist() ArchiveLocalRepo._check_archive_limits( @@ -361,8 +363,31 @@ def check_zip_members(zf: zipfile.ZipFile) -> list[zipfile.ZipInfo]: ) for info in members: ArchiveLocalRepo._check_archive_member_path(info.filename) + ArchiveLocalRepo._check_zip_member_type(info, zf) return members + @staticmethod + def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None: + """Reject dangerous ZIP member types (mirrors :meth:`_check_tar_member_type`). + + Detects Unix symlinks encoded in the ``external_attr`` high word and + validates their targets with the same rules applied to TAR symlinks: + absolute targets and targets containing ``..`` are rejected. + + Raises: + RuntimeError: When *info* is a symlink with an unsafe target. + """ + unix_mode = info.external_attr >> 16 + if stat.S_ISLNK(unix_mode): + target = zf.read(info).decode(errors="replace") + if os.path.isabs(target) or any( + part == ".." for part in pathlib.PurePosixPath(target).parts + ): + raise RuntimeError( + f"Archive contains a symlink with an unsafe target: " + f"{info.filename!r} -> {target!r}" + ) + @staticmethod def _check_tar_member_type(member: tarfile.TarInfo) -> None: """Reject dangerous TAR member types that could harm the host system. diff --git a/dfetch/vcs/svn.py b/dfetch/vcs/svn.py index 89231a9d..2ddd1eaf 100644 --- a/dfetch/vcs/svn.py +++ b/dfetch/vcs/svn.py @@ -315,11 +315,23 @@ def untracked_files(path: str, ignore: Sequence[str]) -> list[str]: @staticmethod def export(url: str, rev: str = "", dst: str = ".") -> None: - """Export the given revision from url to destination.""" + """Export the given revision from url to destination. + + Args: + url: Repository URL to export from. + rev: Bare revision number (digits only) or empty string for HEAD. + Must not include flag names such as ``--revision``. + dst: Local destination path. + + Raises: + ValueError: If *rev* is non-empty and contains non-digit characters. + """ + if rev and not rev.isdigit(): + raise ValueError(f"SVN revision must be digits only, got: {rev!r}") run_on_cmdline( logger, ["svn", "export", "--non-interactive", "--force"] - + (rev.split(" ") if rev else []) + + (["--revision", rev] if rev else []) + [url, dst], ) diff --git a/tests/test_archive.py b/tests/test_archive.py index 47890001..24315f00 100644 --- a/tests/test_archive.py +++ b/tests/test_archive.py @@ -4,6 +4,7 @@ import io import os import pathlib +import stat as _stat import tarfile import tempfile import zipfile @@ -136,6 +137,37 @@ def test_check_zip_members_absolute(): _check_zip_members(zf) +def _make_zip_with_symlink(link_name: str, link_target: str) -> zipfile.ZipFile: + """Return an in-memory ZIP whose sole member is a Unix symlink.""" + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + info = zipfile.ZipInfo(link_name) + info.external_attr = (_stat.S_IFLNK | 0o777) << 16 + zf.writestr(info, link_target) # content == symlink target + buf.seek(0) + return zipfile.ZipFile(buf) + + +def test_check_zip_members_symlink_absolute_target(): + """ZIP symlink pointing to an absolute path must be rejected.""" + zf = _make_zip_with_symlink("link", "/etc/passwd") + with pytest.raises(RuntimeError, match="symlink"): + _check_zip_members(zf) + + +def test_check_zip_members_symlink_dotdot_target(): + """ZIP symlink pointing outside extraction dir via .. must be rejected.""" + zf = _make_zip_with_symlink("link", "../../etc/shadow") + with pytest.raises(RuntimeError, match="symlink"): + _check_zip_members(zf) + + +def test_check_zip_members_symlink_safe_relative(): + """ZIP symlink with a safe relative target must be accepted (mirrors TAR behaviour).""" + zf = _make_zip_with_symlink("link", "relative/safe/target") + _check_zip_members(zf) # must NOT raise + + # --------------------------------------------------------------------------- # _check_tar_members # --------------------------------------------------------------------------- diff --git a/tests/test_manifest.py b/tests/test_manifest.py index e323b176..330425d4 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -216,3 +216,26 @@ def test_get_manifest_location(name, manifest, project_name, result) -> None: manifest.find_name_in_manifest(project_name) else: assert manifest.find_name_in_manifest(project_name) == result + + +# --------------------------------------------------------------------------- +# validate_destination – security: absolute paths must be rejected +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "dst", + ["/etc/passwd", "/tmp/evil"], +) +def test_validate_destination_rejects_absolute_paths(dst) -> None: + with pytest.raises(ValueError, match="absolute"): + Manifest.validate_destination(dst) + + +def test_validate_destination_rejects_dotdot() -> None: + with pytest.raises(ValueError): + Manifest.validate_destination("sub/../../../etc/passwd") + + +def test_validate_destination_accepts_relative() -> None: + Manifest.validate_destination("external/mylib") # must NOT raise diff --git a/tests/test_svn_vcs.py b/tests/test_svn_vcs.py new file mode 100644 index 00000000..20cd0f7a --- /dev/null +++ b/tests/test_svn_vcs.py @@ -0,0 +1,38 @@ +"""Unit tests for SvnRepo.export() argument safety.""" + +# mypy: ignore-errors +# flake8: noqa + +import pytest +from unittest.mock import patch + +from dfetch.vcs.svn import SvnRepo + + +def test_export_with_revision_passes_correct_args(): + """export() must pass --revision and the digit as adjacent list elements.""" + with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: + SvnRepo.export("svn://example.com/repo", rev="12345", dst="/tmp/out") + cmd = mock_run.call_args[0][1] + idx = cmd.index("--revision") + assert cmd[idx + 1] == "12345" + + +def test_export_without_revision_omits_revision_args(): + """export() with no revision must omit --revision entirely.""" + with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: + SvnRepo.export("svn://example.com/repo", dst="/tmp/out") + assert "--revision" not in mock_run.call_args[0][1] + + +def test_export_rejects_non_digit_revision(): + """Space-containing or flag-like revision strings must be rejected. + + Before the fix, passing rev='--revision 12345' would be split on spaces and + inject '--revision' and '12345' as separate SVN arguments, allowing option + injection by any caller that bypasses the digit-only validation in + svnsubproject.py. + """ + with patch("dfetch.vcs.svn.run_on_cmdline"): + with pytest.raises((ValueError, TypeError)): + SvnRepo.export("svn://example.com/repo", rev="--revision 12345") From 9e32972fdafd6593575c4230d6fc04f343edda32 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 05:47:49 +0000 Subject: [PATCH 2/7] Security: harden validate_destination, bound ZIP symlink read, tighten SVN test - manifest.py: validate_destination() now rejects Windows-rooted paths by checking PurePosixPath.is_absolute(), PureWindowsPath.is_absolute(), and non-empty PureWindowsPath.drive in addition to the existing POSIX check. Added PurePosixPath/PureWindowsPath imports. Test covers C:/ and C:\ forms. - archive.py: _check_zip_member_type() now reads symlink target content via zf.open()+read(4096) instead of zf.read(), bounding memory allocation to 4 KB regardless of the stored member size. - test_svn_vcs.py: test_export_rejects_non_digit_revision narrowed to assert ValueError specifically (not ValueError|TypeError) and added mock_run.assert_not_called() to prove the subprocess is never reached. https://claude.ai/code/session_01CxKrTZekNRCoSD9PSMekQZ --- dfetch/manifest/manifest.py | 8 ++++++-- dfetch/vcs/archive.py | 3 ++- tests/test_manifest.py | 2 +- tests/test_svn_vcs.py | 9 +++++---- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/dfetch/manifest/manifest.py b/dfetch/manifest/manifest.py index 86335592..c5d565ad 100644 --- a/dfetch/manifest/manifest.py +++ b/dfetch/manifest/manifest.py @@ -24,7 +24,7 @@ import re from collections.abc import Sequence from dataclasses import dataclass -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import IO, Any import yaml @@ -395,7 +395,11 @@ def validate_project_name(self, name: str) -> None: @staticmethod def validate_destination(dst: str) -> None: """Raise ValueError if *dst* is not a safe manifest destination path.""" - if Path(dst).is_absolute(): + if ( + PurePosixPath(dst).is_absolute() + or PureWindowsPath(dst).is_absolute() + or bool(PureWindowsPath(dst).drive) + ): raise ValueError( f"Destination '{dst}' is an absolute path. " "Paths must be relative to the manifest directory." diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index 5df3b3d8..5ddbe59b 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -379,7 +379,8 @@ def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None: """ unix_mode = info.external_attr >> 16 if stat.S_ISLNK(unix_mode): - target = zf.read(info).decode(errors="replace") + with zf.open(info) as member_file: + target = member_file.read(4096).decode(errors="replace") if os.path.isabs(target) or any( part == ".." for part in pathlib.PurePosixPath(target).parts ): diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 330425d4..ee1a8708 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -225,7 +225,7 @@ def test_get_manifest_location(name, manifest, project_name, result) -> None: @pytest.mark.parametrize( "dst", - ["/etc/passwd", "/tmp/evil"], + ["/etc/passwd", "/tmp/evil", "C:/Windows/System32", "C:\\Windows\\System32"], ) def test_validate_destination_rejects_absolute_paths(dst) -> None: with pytest.raises(ValueError, match="absolute"): diff --git a/tests/test_svn_vcs.py b/tests/test_svn_vcs.py index 20cd0f7a..2c8f29f0 100644 --- a/tests/test_svn_vcs.py +++ b/tests/test_svn_vcs.py @@ -26,13 +26,14 @@ def test_export_without_revision_omits_revision_args(): def test_export_rejects_non_digit_revision(): - """Space-containing or flag-like revision strings must be rejected. + """Space-containing or flag-like revision strings must raise ValueError. Before the fix, passing rev='--revision 12345' would be split on spaces and inject '--revision' and '12345' as separate SVN arguments, allowing option injection by any caller that bypasses the digit-only validation in - svnsubproject.py. + svnsubproject.py. The subprocess must never be invoked for an invalid rev. """ - with patch("dfetch.vcs.svn.run_on_cmdline"): - with pytest.raises((ValueError, TypeError)): + with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: + with pytest.raises(ValueError): SvnRepo.export("svn://example.com/repo", rev="--revision 12345") + mock_run.assert_not_called() From 39a443f52db757f6efcf578e862014d920a3b6e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 05:55:26 +0000 Subject: [PATCH 3/7] Security: harden root-relative Windows paths, fail-closed ZIP symlink, exact SVN argv - manifest.py: replace PureWindowsPath(dst).drive with .anchor so root-relative Windows paths like \temp\evil (anchor="\", drive="") are rejected; .drive is empty for paths that have a root but no drive letter. - archive.py: read _MAX_SYMLINK_TARGET+1 (4097) bytes for ZIP symlink targets; if the extra byte is present the target exceeds the limit and is immediately rejected (fail-closed) instead of silently truncating a long target and potentially passing a dangerous suffix through the isabs/.. checks. - test_manifest.py: add "\\temp\\evil" (root-relative Windows path) to the parametrized absolute-path rejection cases. - test_svn_vcs.py: remove file-wide # mypy: ignore-errors / # flake8: noqa suppressions; replace adjacency checks with exact full argv equality assertions so the entire command shape and argument order are validated. https://claude.ai/code/session_01CxKrTZekNRCoSD9PSMekQZ --- dfetch/manifest/manifest.py | 2 +- dfetch/vcs/archive.py | 9 ++++++++- tests/test_manifest.py | 2 +- tests/test_svn_vcs.py | 20 +++++++++++--------- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/dfetch/manifest/manifest.py b/dfetch/manifest/manifest.py index c5d565ad..aff40963 100644 --- a/dfetch/manifest/manifest.py +++ b/dfetch/manifest/manifest.py @@ -398,7 +398,7 @@ def validate_destination(dst: str) -> None: if ( PurePosixPath(dst).is_absolute() or PureWindowsPath(dst).is_absolute() - or bool(PureWindowsPath(dst).drive) + or bool(PureWindowsPath(dst).anchor) ): raise ValueError( f"Destination '{dst}' is an absolute path. " diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index 5ddbe59b..e438385d 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -377,10 +377,17 @@ def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None: Raises: RuntimeError: When *info* is a symlink with an unsafe target. """ + _MAX_SYMLINK_TARGET = 4096 unix_mode = info.external_attr >> 16 if stat.S_ISLNK(unix_mode): with zf.open(info) as member_file: - target = member_file.read(4096).decode(errors="replace") + raw = member_file.read(_MAX_SYMLINK_TARGET + 1) + if len(raw) > _MAX_SYMLINK_TARGET: + raise RuntimeError( + f"Archive contains a symlink with an oversized target: " + f"{info.filename!r}" + ) + target = raw.decode(errors="replace") if os.path.isabs(target) or any( part == ".." for part in pathlib.PurePosixPath(target).parts ): diff --git a/tests/test_manifest.py b/tests/test_manifest.py index ee1a8708..8131ace4 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -225,7 +225,7 @@ def test_get_manifest_location(name, manifest, project_name, result) -> None: @pytest.mark.parametrize( "dst", - ["/etc/passwd", "/tmp/evil", "C:/Windows/System32", "C:\\Windows\\System32"], + ["/etc/passwd", "/tmp/evil", "C:/Windows/System32", "C:\\Windows\\System32", "\\temp\\evil"], ) def test_validate_destination_rejects_absolute_paths(dst) -> None: with pytest.raises(ValueError, match="absolute"): diff --git a/tests/test_svn_vcs.py b/tests/test_svn_vcs.py index 2c8f29f0..800d54a2 100644 --- a/tests/test_svn_vcs.py +++ b/tests/test_svn_vcs.py @@ -1,8 +1,5 @@ """Unit tests for SvnRepo.export() argument safety.""" -# mypy: ignore-errors -# flake8: noqa - import pytest from unittest.mock import patch @@ -10,19 +7,24 @@ def test_export_with_revision_passes_correct_args(): - """export() must pass --revision and the digit as adjacent list elements.""" + """export() must produce the exact expected SVN command.""" with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", rev="12345", dst="/tmp/out") - cmd = mock_run.call_args[0][1] - idx = cmd.index("--revision") - assert cmd[idx + 1] == "12345" + assert mock_run.call_args[0][1] == [ + "svn", "export", "--non-interactive", "--force", + "--revision", "12345", + "svn://example.com/repo", "/tmp/out", + ] def test_export_without_revision_omits_revision_args(): - """export() with no revision must omit --revision entirely.""" + """export() with no revision must produce the exact expected SVN command.""" with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", dst="/tmp/out") - assert "--revision" not in mock_run.call_args[0][1] + assert mock_run.call_args[0][1] == [ + "svn", "export", "--non-interactive", "--force", + "svn://example.com/repo", "/tmp/out", + ] def test_export_rejects_non_digit_revision(): From fe8eae2ece10d5c65c3db9ee177084d2f92e59c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 07:07:06 +0000 Subject: [PATCH 4/7] Security: fix Windows backslash traversal in symlink checks, rename constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - archive.py: extract _is_unsafe_symlink_target() helper that checks both PurePosixPath and PureWindowsPath so targets like "..\\..\\evil" are caught on any host OS; apply it to both _check_zip_member_type and _check_tar_member_type, removing the duplicated inline check. Rename _MAX_SYMLINK_TARGET → max_symlink_target to satisfy pylint C0103 (local variable vs. module-level constant naming convention). - test_archive.py: add Windows-backslash traversal cases for both ZIP (test_check_zip_members_symlink_windows_dotdot_target) and TAR (test_check_tar_member_type_windows_dotdot_symlink). https://claude.ai/code/session_01CxKrTZekNRCoSD9PSMekQZ --- dfetch/vcs/archive.py | 31 ++++++++++++++++++++++--------- tests/test_archive.py | 15 +++++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index e438385d..1d956b15 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -366,6 +366,23 @@ def check_zip_members(zf: zipfile.ZipFile) -> list[zipfile.ZipInfo]: ArchiveLocalRepo._check_zip_member_type(info, zf) return members + @staticmethod + def _is_unsafe_symlink_target(target: str) -> bool: + """Return *True* when *target* is an unsafe 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. + """ + posix = pathlib.PurePosixPath(target) + win = pathlib.PureWindowsPath(target) + return ( + posix.is_absolute() + or win.is_absolute() + or any(part == ".." for part in posix.parts) + or any(part == ".." for part in win.parts) + ) + @staticmethod def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None: """Reject dangerous ZIP member types (mirrors :meth:`_check_tar_member_type`). @@ -377,20 +394,18 @@ def _check_zip_member_type(info: zipfile.ZipInfo, zf: zipfile.ZipFile) -> None: Raises: RuntimeError: When *info* is a symlink with an unsafe target. """ - _MAX_SYMLINK_TARGET = 4096 + max_symlink_target = 4096 unix_mode = info.external_attr >> 16 if stat.S_ISLNK(unix_mode): with zf.open(info) as member_file: - raw = member_file.read(_MAX_SYMLINK_TARGET + 1) - if len(raw) > _MAX_SYMLINK_TARGET: + raw = member_file.read(max_symlink_target + 1) + if len(raw) > max_symlink_target: raise RuntimeError( f"Archive contains a symlink with an oversized target: " f"{info.filename!r}" ) target = raw.decode(errors="replace") - if os.path.isabs(target) or any( - part == ".." for part in pathlib.PurePosixPath(target).parts - ): + if ArchiveLocalRepo._is_unsafe_symlink_target(target): raise RuntimeError( f"Archive contains a symlink with an unsafe target: " f"{info.filename!r} -> {target!r}" @@ -422,9 +437,7 @@ def _check_tar_member_type(member: tarfile.TarInfo) -> None: """ if member.issym(): target = member.linkname - if os.path.isabs(target) or any( - part == ".." for part in pathlib.PurePosixPath(target).parts - ): + if ArchiveLocalRepo._is_unsafe_symlink_target(target): raise RuntimeError( f"Archive contains a symlink with an unsafe target: " f"{member.name!r} -> {target!r}" diff --git a/tests/test_archive.py b/tests/test_archive.py index 24315f00..a5185f83 100644 --- a/tests/test_archive.py +++ b/tests/test_archive.py @@ -162,6 +162,13 @@ def test_check_zip_members_symlink_dotdot_target(): _check_zip_members(zf) +def test_check_zip_members_symlink_windows_dotdot_target(): + """ZIP symlink with Windows-style backslash traversal must be rejected.""" + zf = _make_zip_with_symlink("link", "..\\..\\evil") + with pytest.raises(RuntimeError, match="symlink"): + _check_zip_members(zf) + + def test_check_zip_members_symlink_safe_relative(): """ZIP symlink with a safe relative target must be accepted (mirrors TAR behaviour).""" zf = _make_zip_with_symlink("link", "relative/safe/target") @@ -268,6 +275,14 @@ def test_check_tar_member_type_dotdot_symlink(): _check_tar_member_type(member) +def test_check_tar_member_type_windows_dotdot_symlink(): + """TAR symlink with Windows-style backslash traversal must be rejected.""" + 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 — hardlink validation # --------------------------------------------------------------------------- From 593d363029ea50dcefeb2789aeb1af9427e63793 Mon Sep 17 00:00:00 2001 From: Ben Date: Tue, 31 Mar 2026 09:21:15 +0200 Subject: [PATCH 5/7] Minor formatting changes --- dfetch/vcs/archive.py | 2 +- tests/test_manifest.py | 8 +++++++- tests/test_svn_vcs.py | 22 ++++++++++++++++------ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index 1d956b15..c0602d75 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -368,7 +368,7 @@ def check_zip_members(zf: zipfile.ZipFile) -> list[zipfile.ZipInfo]: @staticmethod def _is_unsafe_symlink_target(target: str) -> bool: - """Return *True* when *target* is an unsafe symlink destination. + r"""Return *True* when *target* is an unsafe symlink destination. A target is unsafe if it is absolute or contains ``..`` components under either POSIX or Windows path semantics, so that backslash-based diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 8131ace4..87d7ea57 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -225,7 +225,13 @@ def test_get_manifest_location(name, manifest, project_name, result) -> None: @pytest.mark.parametrize( "dst", - ["/etc/passwd", "/tmp/evil", "C:/Windows/System32", "C:\\Windows\\System32", "\\temp\\evil"], + [ + "/etc/passwd", + "/tmp/evil", + "C:/Windows/System32", + "C:\\Windows\\System32", + "\\temp\\evil", + ], ) def test_validate_destination_rejects_absolute_paths(dst) -> None: with pytest.raises(ValueError, match="absolute"): diff --git a/tests/test_svn_vcs.py b/tests/test_svn_vcs.py index 800d54a2..00068c65 100644 --- a/tests/test_svn_vcs.py +++ b/tests/test_svn_vcs.py @@ -1,8 +1,9 @@ """Unit tests for SvnRepo.export() argument safety.""" -import pytest from unittest.mock import patch +import pytest + from dfetch.vcs.svn import SvnRepo @@ -11,9 +12,14 @@ def test_export_with_revision_passes_correct_args(): with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", rev="12345", dst="/tmp/out") assert mock_run.call_args[0][1] == [ - "svn", "export", "--non-interactive", "--force", - "--revision", "12345", - "svn://example.com/repo", "/tmp/out", + "svn", + "export", + "--non-interactive", + "--force", + "--revision", + "12345", + "svn://example.com/repo", + "/tmp/out", ] @@ -22,8 +28,12 @@ def test_export_without_revision_omits_revision_args(): with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", dst="/tmp/out") assert mock_run.call_args[0][1] == [ - "svn", "export", "--non-interactive", "--force", - "svn://example.com/repo", "/tmp/out", + "svn", + "export", + "--non-interactive", + "--force", + "svn://example.com/repo", + "/tmp/out", ] From 6eabd615904e9127f6ce2f34702d694e7177b228 Mon Sep 17 00:00:00 2001 From: Ben Date: Tue, 31 Mar 2026 09:29:50 +0200 Subject: [PATCH 6/7] Reject rooted Windows targets, not just is_absolute() ones. --- dfetch/vcs/archive.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dfetch/vcs/archive.py b/dfetch/vcs/archive.py index c0602d75..d74c0c0d 100644 --- a/dfetch/vcs/archive.py +++ b/dfetch/vcs/archive.py @@ -378,7 +378,7 @@ def _is_unsafe_symlink_target(target: str) -> bool: win = pathlib.PureWindowsPath(target) return ( posix.is_absolute() - or win.is_absolute() + or bool(win.anchor) or any(part == ".." for part in posix.parts) or any(part == ".." for part in win.parts) ) From 96694a874241de03d17684c9ab01aaf9bb80c579 Mon Sep 17 00:00:00 2001 From: Ben Date: Tue, 31 Mar 2026 10:13:27 +0200 Subject: [PATCH 7/7] Harden tests --- tests/test_svn_vcs.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_svn_vcs.py b/tests/test_svn_vcs.py index 00068c65..313d27ec 100644 --- a/tests/test_svn_vcs.py +++ b/tests/test_svn_vcs.py @@ -11,6 +11,7 @@ def test_export_with_revision_passes_correct_args(): """export() must produce the exact expected SVN command.""" with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", rev="12345", dst="/tmp/out") + mock_run.assert_called_once() assert mock_run.call_args[0][1] == [ "svn", "export", @@ -27,6 +28,7 @@ def test_export_without_revision_omits_revision_args(): """export() with no revision must produce the exact expected SVN command.""" with patch("dfetch.vcs.svn.run_on_cmdline") as mock_run: SvnRepo.export("svn://example.com/repo", dst="/tmp/out") + mock_run.assert_called_once() assert mock_run.call_args[0][1] == [ "svn", "export",