From 62266dabde03645a2389972f26338a324463d1f3 Mon Sep 17 00:00:00 2001 From: Sasha Mitchell Date: Thu, 1 Oct 2026 17:15:39 +0700 Subject: [PATCH 01/11] objects: decode a byte mode before reading its digits mode_str_to_int(b"100644") returned 1830740. A byte is an int, so the digit 4 was read as 52. A text mode was already 0o100644. --- git/objects/util.py | 4 +++- test/test_util.py | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/git/objects/util.py b/git/objects/util.py index 64cdf71fd..1fa3b9df0 100644 --- a/git/objects/util.py +++ b/git/objects/util.py @@ -100,9 +100,11 @@ def mode_str_to_int(modestr: Union[bytes, str]) -> int: module regarding the rwx permissions for user, group and other, special flags and file system flags, such as whether it is a symlink. """ + if isinstance(modestr, bytes): + # A byte is an int. int(b"4"[0]) is 52, so b"100644" became 0o6767524. + modestr = modestr.decode("ascii") mode = 0 for iteration, char in enumerate(reversed(modestr[-6:])): - char = cast(Union[str, int], char) mode += int(char) << iteration * 3 # END for each char return mode diff --git a/test/test_util.py b/test/test_util.py index c6e68cf51..46417cc39 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -22,6 +22,7 @@ from git.objects.util import ( altz_to_utctz_str, from_timestamp, + mode_str_to_int, parse_actor_and_date, parse_date, tzoffset, @@ -760,3 +761,10 @@ def test_remove_password_from_command_line(self): redacted_cmd_6 = remove_password_if_present(cmd_6) assert authorization not in " ".join(redacted_cmd_6) assert "http.extraHeader=Authorization: *****" in redacted_cmd_6 + + +def test_mode_str_to_int_accepts_bytes(): + assert mode_str_to_int("100644") == 0o100644 + assert mode_str_to_int(b"100644") == 0o100644 + assert mode_str_to_int("644") == 0o644 + assert mode_str_to_int(b"120000") == 0o120000 From f24ed41dea663785f60cc4f7d5ece240a7d88174 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 13:37:36 +0000 Subject: [PATCH 02/11] build(deps): bump https://github.com/astral-sh/ruff-pre-commit Bumps the pre-commit group with 1 update: [https://github.com/astral-sh/ruff-pre-commit](https://github.com/astral-sh/ruff-pre-commit). Updates `https://github.com/astral-sh/ruff-pre-commit` from v0.16.5 to 0.16.9 - [Release notes](https://github.com/astral-sh/ruff-pre-commit/releases) - [Commits](https://github.com/astral-sh/ruff-pre-commit/compare/v0.16.5...v0.16.9) --- updated-dependencies: - dependency-name: https://github.com/astral-sh/ruff-pre-commit dependency-version: 0.16.9 dependency-type: direct:production dependency-group: pre-commit ... Signed-off-by: dependabot[bot] --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 45115f60c..9471680e8 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -2,7 +2,7 @@ exclude: ^(?:gitdb|smmap)/ repos: - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.16.5 + rev: v0.16.9 hooks: - id: ruff-check args: ["--fix"] From 97eadb888d2f99567e3113f4e455be4ca128f697 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 1 Oct 2026 19:17:13 +0200 Subject: [PATCH 03/11] fix(submodule): validate destinations before mutation Pretty much a rubber-stamp. It won't be out there long as the replacement with CLI + Gix is already on the way. Submodule checkout destinations could pass the containment check and be rejected by the index only after cloning had changed the filesystem. This addresses `GHSA-83vg-56qc-22m7` at the shared destination boundaries, including initialization and moves as well as creation. Reuse `_validate_repo_path` before checkout mutations to enforce portable NTFS/HFS metadata-alias checks and invalid-path rejection. Validate Windows filenames and submodule-name NULs before creating directories. Compare path components with `Repo.git_dir` and `Repo.common_dir` by filesystem identity, so separately named metadata directories and their aliases are protected too. Reject metadata destinations nested inside another submodule's Git directory before cloning, reuse, or renaming. Repeat the check after cloning and disable a clone that became nested. Preflight implicit metadata renames during moves, while preserving supported metadata symlinks and relocation of a submodule's own metadata directory. Git reference: `d38352cd43ab9745686d697872408bc3249a153f`, particularly `read-cache.c::verify_path_internal`, NTFS/HFS recognition, `compat/mingw.c::is_valid_win32_path`, and `submodule.c::validate_submodule_git_dir`. Related Git tests are in `t/t7450-bad-git-dotfiles.sh` and `t/t7406-submodule-update.sh`. Regression tests first demonstrated writes before rejection and acceptance of nested and separately named metadata destinations. Tests use harmless file content and compare portable aliases with native Git index validation. Coverage includes all 16 HFS ignored characters, Windows filename rules, relative and absolute paths, metadata reuse, and nesting during cloning. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6.0 Astra --- doc/source/changes.rst | 13 ++ git/objects/submodule/base.py | 91 ++++++++++++-- test/test_submodule.py | 225 ++++++++++++++++++++++++++++++++++ 3 files changed, 318 insertions(+), 11 deletions(-) diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 2f537c99d..8447228db 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,19 @@ Changelog ========= +3.2.1 +===== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-83vg-56qc-22m7 + +If you can, also try and provide feedback on the upcoming v4 branch +https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.2.1 + 3.2.0 ===== diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 15fe877e6..74fbd39fe 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -8,6 +8,7 @@ import ntpath import os import os.path as osp +import re import shlex import stat import sys @@ -47,6 +48,7 @@ IterableList, RemoteProgress, _to_relative_path, + _validate_repo_path, join_path_native, rmtree, to_native_path_linux, @@ -305,20 +307,55 @@ def _config_parser_constrained(self, read_only: bool) -> SectionConstraint: def _validated_name(cls, name: str) -> str: if ( not name + or "\0" in name or name.startswith(("/", "\\")) or ntpath.splitdrive(name)[0] or ".." in name.replace("\\", "/").split("/") ): raise ValueError("Invalid submodule name %r" % name) + cls._validate_windows_path(name) return name + @staticmethod + def _validate_windows_path(path: PathLike) -> None: + """Apply Git for Windows' filename checks before creating directories.""" + if sys.platform == "win32": + for component in ntpath.splitdrive(os.fspath(path))[1].replace("\\", "/").split("/"): + if component in (".", ".."): + continue + stem = component.split(".", 1)[0].rstrip(" ").upper() + if ( + component.endswith((" ", ".")) + or any(ord(char) < 32 or char in '<>:"|?*' for char in component) + or re.fullmatch(r"CON(?:IN\$|OUT\$)?|PRN|AUX|NUL|COM[1-9]|LPT[1-9]", stem) + ): + raise ValueError("Invalid submodule path on Windows: %r" % path) + @classmethod - def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> PathLike: + def _module_abspath( + cls, parent_repo: "Repo", path: PathLike, name: str, *, moving_from: Union[PathLike, None] = None + ) -> PathLike: + """Reject nested Git directories, allowing the source of a pending rename.""" + from git.repo.fun import is_git_dir + name = cls._validated_name(name) if cls._need_gitfile_submodules(parent_repo.git): + directory = osp.join(parent_repo.git_dir, "modules") + for component in to_native_path_linux(name).split("/")[:-1]: + directory = osp.join(directory, component) + if is_git_dir(directory) and ( + moving_from is None or Path(directory).resolve() != Path(moving_from).resolve() + ): + raise ValueError( + "Submodule metadata for %r is inside another Git directory: %r" % (name, directory) + ) return osp.join(parent_repo.git_dir, "modules", name) if parent_repo.working_tree_dir: - return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path)) + return cls._checked_abspath( + parent_repo.working_tree_dir, + cls._to_relative_path(parent_repo, path), + git_dirs=(parent_repo.git_dir, parent_repo.common_dir), + ) raise NotADirectoryError() @classmethod @@ -360,7 +397,9 @@ def _clone_repo( path = cls._to_relative_path(repo, path) if repo.working_tree_dir is None: raise NotADirectoryError("Submodules require a working tree") - module_checkout_path = cls._checked_abspath(repo.working_tree_dir, path) + module_checkout_path = cls._checked_abspath( + repo.working_tree_dir, path, git_dirs=(repo.git_dir, repo.common_dir) + ) module_abspath = cls._module_abspath(repo, path, name) if cls._need_gitfile_submodules(repo.git): if not allow_unsafe_options: @@ -402,6 +441,16 @@ def _clone_repo( **kwargs, ) if cls._need_gitfile_submodules(repo.git): + # A concurrent clone may have turned a leading directory into a repository. + try: + cls._module_abspath(repo, path, name) + except ValueError: + clone.close() + try: + os.remove(osp.join(clone.git_dir, "HEAD")) + except FileNotFoundError: + pass + raise cls._write_git_file_and_module_config(module_checkout_path, module_abspath) return clone @@ -411,8 +460,9 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike: """:return: A path guaranteed to be relative to the given parent repository :raise ValueError: - If path is not contained in the parent repository's working tree. + If path is outside the working tree or is unsafe as a submodule checkout. """ + cls._validate_windows_path(path) if parent_repo.working_tree_dir: path = _to_relative_path(parent_repo.working_tree_dir, path) else: @@ -422,6 +472,7 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike: if not path or path == ".": raise ValueError("Submodule checkout path must not be the repository root") + _validate_repo_path(path) return path @property @@ -433,22 +484,35 @@ def abspath(self) -> PathLike: def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike: """Check a checkout path already normalized by :meth:`_to_relative_path`.""" - return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink) + return self._checked_abspath( + self.repo.working_tree_dir, + relative_path, + allow_final_symlink, + git_dirs=(self.repo.git_dir, self.repo.common_dir), + ) @classmethod def _checked_abspath( - cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False + cls, + root: Union[PathLike, None], + relative_path: PathLike, + allow_final_symlink: bool = False, + *, + git_dirs: Sequence[PathLike] = (), ) -> str: - """Reject symlinks below a trusted root before accessing submodule paths.""" + """Reject symlinks and checkout aliases of Git directories below a trusted root.""" if root is None: raise NotADirectoryError("Submodules require a working tree") path = os.fspath(root) + metadata_dirs = set(git_dirs) components = to_native_path_linux(relative_path).split("/") for index, component in enumerate(components): path = os.fspath(join_path_native(path, component)) - if allow_final_symlink and index == len(components) - 1: - break + if metadata_dirs and osp.exists(path) and any(osp.samefile(path, directory) for directory in metadata_dirs): + raise ValueError("Submodule checkout path aliases Git metadata: %r" % relative_path) if osp.islink(path): + if allow_final_symlink and index == len(components) - 1: + break raise ValueError("Submodule path %r contains a symbolic link" % relative_path) return path @@ -1122,6 +1186,11 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # Validate the source before removing the destination. cur_path = self.abspath + module_abspath = self._module_abspath(self.repo, self.path, self.name) + if self.path == self.name: + self._module_abspath( + self.repo, module_checkout_path, os.fspath(module_checkout_path), moving_from=module_abspath + ) module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True) if osp.isfile(module_checkout_abspath): raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath) @@ -1160,7 +1229,6 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = renamed_module = True if osp.isfile(osp.join(module_checkout_abspath, ".git")): - module_abspath = self._module_abspath(self.repo, self.path, self.name) self._write_git_file_and_module_config(module_checkout_abspath, module_abspath) # END handle git file rewrite # END move physical module @@ -1522,8 +1590,8 @@ def rename(self, new_name: str) -> "Submodule": self._validated_name(self.name) self._validated_name(new_name) - destination_module_abspath = self._module_abspath(self.repo, self.path, new_name) mod = self.module() + destination_module_abspath = self._module_abspath(self.repo, self.path, new_name, moving_from=mod.git_dir) self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # .git/config @@ -1573,6 +1641,7 @@ def module(self) -> "Repo": """ self._validated_name(self.name) module_checkout_abspath = self.abspath + self._module_abspath(self.repo, self.path, self.name) try: repo = git.Repo(module_checkout_abspath) if repo != self.repo: diff --git a/test/test_submodule.py b/test/test_submodule.py index b1f4f5156..45b285c55 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -86,6 +86,7 @@ def test_submodule_update_preserves_literal_name(tmp_path, monkeypatch, caplog, def movable_submodule(tmp_path): """Create a committed local submodule whose logical name stays fixed when moved.""" with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: + source.git.symbolic_ref("HEAD", "refs/heads/master") (tmp_path / "source" / "file").write_text("content", encoding="utf-8") source.index.add(["file"]) source.index.commit("Create source") @@ -111,6 +112,207 @@ def _move_snapshot(submodule): ) +@pytest.mark.parametrize( + "path", + [ + ".git/child", + ".GiT/child", + "nested/.git/child", + "git~1/child", + "GIT~1 . /child", + ".git. /child", + ".git:stream/child", + ".git::$INDEX_ALLOCATION/child", + "nested\\.git\\child", + "C:relative", + "nul\0name", + ] + + [ + f".g{chr(codepoint)}it/child" + for codepoint in (*range(0x200C, 0x2010), *range(0x202A, 0x202F), *range(0x206A, 0x2070), 0xFEFF) + ], +) +@pytest.mark.parametrize("operation", ["add", "clone", "update", "move", "move-module", "move-config"]) +def test_submodule_rejects_unsafe_checkout_before_mutation(movable_submodule, tmp_path, path, operation): + sm = movable_submodule + root = Path(sm.repo.working_tree_dir) + before = _move_snapshot(sm) + paths = set(root.rglob("*")) + # Check the portable metadata aliases against Git's index validation as well. + if operation == "clone" and path not in ("C:relative", "nul\0name"): + with sm.repo.git.custom_environment(GIT_INDEX_FILE=str(tmp_path / "validation-index")): + with _patch_git_config("core.protectHFS", "true"), _patch_git_config("core.protectNTFS", "true"): + with pytest.raises(GitCommandError): + sm.repo.git.update_index("--add", "--cacheinfo", f"160000,{sm.hexsha},{path}") + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("clone attempted")): + with pytest.raises(ValueError): + if operation == "add": + sm.repo.create_submodule("new", path, sm.url) + elif operation == "clone": + Submodule._clone_repo(sm.repo, sm.url, path, "new") + elif operation == "update": + Submodule(sm.repo, sm.binsha, name="new", path=path, url=sm.url).update(init=True) + else: + sm.move(path, configuration=operation != "move-module", module=operation != "move-config") + assert _move_snapshot(sm) == before + assert set(root.rglob("*")) == paths + + +def test_add_rejects_metadata_checkout_without_writing_files(movable_submodule): + sm = movable_submodule + before = _move_snapshot(sm) + with pytest.raises(ValueError, match="Git metadata"): + sm.repo.create_submodule("new", ".git/new", sm.url) + assert not Path(sm.repo.git_dir, "new").exists() + assert not Path(sm.repo.git_dir, "modules/new").exists() + assert _move_snapshot(sm) == before + + +@pytest.mark.parametrize("absolute_path", [False, True]) +@pytest.mark.parametrize("metadata_name", ["metadata", "MeTaDaTa"]) +@pytest.mark.parametrize("operation", ["add", "clone", "update", "move"]) +def test_submodule_rejects_checkout_in_separate_metadata( + movable_submodule, tmp_path, absolute_path, metadata_name, operation +): + root = tmp_path / "separate" + with git.Repo.init(root, separate_git_dir=str(root / "metadata"), allow_unsafe_options=True) as parent: + if not (root / metadata_name).is_dir(): + pytest.skip("Requires a case-insensitive filesystem") + sm = parent.create_submodule("module", "module", movable_submodule.url) + parent.index.commit("Add submodule") + before = _move_snapshot(sm) + paths = set(root.rglob("*")) + path = root / metadata_name / "new" if absolute_path else f"{metadata_name}/new" + with pytest.raises(ValueError, match="Git metadata"): + if operation == "add": + parent.create_submodule("new", path, sm.url) + elif operation == "clone": + Submodule._clone_repo(parent, sm.url, path, "new") + elif operation == "update": + Submodule(parent, sm.binsha, name="new", path=path, url=sm.url).update(init=True) + else: + sm.move(path) + assert _move_snapshot(sm) == before + assert set(root.rglob("*")) == paths + + +@pytest.mark.parametrize("operation", ["add", "clone", "update", "rename", "move"]) +def test_submodule_rejects_nested_metadata_before_mutation(movable_submodule, operation): + sm = movable_submodule + other = sm.repo.create_submodule("other", "other", sm.url) + other.module().close() + root = Path(sm.repo.working_tree_dir) + before = _move_snapshot(sm), _move_snapshot(other) + paths = set(root.rglob("*")) + name = f"{sm.name}/child" + with pytest.raises(ValueError, match="inside.*Git directory"): + if operation == "add": + sm.repo.create_submodule(name, "new", sm.url) + elif operation == "clone": + Submodule._clone_repo(sm.repo, sm.url, "new", name) + elif operation == "update": + Submodule(sm.repo, sm.binsha, name=name, path="new", url=sm.url).update(init=True) + elif operation == "rename": + other.rename(name) + else: + other.move(name) + assert (_move_snapshot(sm), _move_snapshot(other)) == before + assert set(root.rglob("*")) == paths + + +@pytest.mark.parametrize("state", ["retained", "checked-out"]) +def test_update_rejects_existing_nested_metadata(movable_submodule, state): + sm = movable_submodule + name = f"{sm.name}/child" + checkout = Path(sm.repo.working_tree_dir, "new") + with git.Repo.clone_from( + sm.url, checkout, separate_git_dir=str(Path(sm.repo.git_dir, "modules", name)), allow_unsafe_options=True + ): + pass + if state == "retained": + shutil.rmtree(checkout) + before = _move_snapshot(sm) + paths = set(Path(sm.repo.working_tree_dir).rglob("*")) + with pytest.raises(ValueError, match="inside.*Git directory"): + Submodule(sm.repo, sm.binsha, name=name, path="new", url=sm.url).update(init=True, no_fetch=True) + assert _move_snapshot(sm) == before + assert set(Path(sm.repo.working_tree_dir).rglob("*")) == paths + + +@pytest.mark.parametrize( + "path", + [ + "CON", + "con.txt", + "CONIN$", + "conout$.txt", + "AUX .txt", + "PRN", + "NUL", + "COM9", + "LPT1", + "name:stream", + "space ", + "period.", + "line\nbreak", + "star*", + 'quote"', + "question?", + "angle<", + "angle>", + "pipe|", + ], +) +def test_submodule_rejects_windows_destination_names_before_mutation(movable_submodule, path): + sm = movable_submodule + before = _move_snapshot(sm) + paths = set(Path(sm.repo.working_tree_dir).rglob("*")) + with mock.patch("git.objects.submodule.base.sys", SimpleNamespace(platform="win32")): + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("clone attempted")): + with pytest.raises(ValueError): + sm.repo.create_submodule("new", f"nested/{path}", sm.url) + with pytest.raises(ValueError): + sm.repo.create_submodule(f"nested/{path}", "new", sm.url) + assert _move_snapshot(sm) == before + assert set(Path(sm.repo.working_tree_dir).rglob("*")) == paths + + +@pytest.mark.parametrize("path", ["nested/space ", "nested/period."]) +def test_windows_destination_validation_precedes_normalization(tmp_path, path): + parent = SimpleNamespace(working_tree_dir=str(tmp_path)) + # Windows' GetFullPathName removes trailing spaces and periods. + with mock.patch("git.objects.submodule.base._to_relative_path", return_value=path.rstrip(" .")): + with mock.patch("git.objects.submodule.base.sys", SimpleNamespace(platform="win32")): + with pytest.raises(ValueError, match="Invalid submodule path on Windows"): + Submodule._to_relative_path(parent, path) + + +def test_submodule_can_relocate_its_own_metadata(movable_submodule): + sm = movable_submodule + sm.rename(f"{sm.name}/child") + assert Path(sm.abspath, "file").read_text() == "content" + with sm.module() as module: + assert Path(module.git_dir) == Path(sm.repo.git_dir, "modules", sm.name) + + +def test_clone_disables_metadata_that_becomes_nested(movable_submodule, monkeypatch): + sm = movable_submodule + clone_from = git.Repo.clone_from + ancestor = Path(sm.repo.git_dir, "modules/new") + + def clone_and_create_ancestor(*args, **kwargs): + clone = clone_from(*args, **kwargs) + with git.Repo.init(ancestor, bare=True): + pass + return clone + + monkeypatch.setattr(git.Repo, "clone_from", clone_and_create_ancestor) + with pytest.raises(ValueError, match="inside.*Git directory"): + Submodule._clone_repo(sm.repo, sm.url, "new", "new/child") + assert not (ancestor / "child/HEAD").exists() + assert (ancestor / "HEAD").is_file() + + @pytest.mark.parametrize("target_kind", ["relative", "absolute", "internal", "dangling"]) @pytest.mark.parametrize("configuration,module", [(True, True), (False, True), (True, False)]) @pytest.mark.parametrize("absolute_path", [False, True]) @@ -167,6 +369,28 @@ def test_move_normal_destination(movable_submodule, absolute_path): assert _move_snapshot(submodule) == before +@pytest.mark.parametrize("metadata_dir", ["git_dir", "common_dir"]) +def test_move_rejects_leaf_symlink_to_metadata(movable_submodule, tmp_path, metadata_dir): + root = tmp_path / "worktree" + movable_submodule.repo.git.worktree("add", "--detach", str(root)) + with git.Repo(root) as parent: + assert not osp.samefile(parent.git_dir, parent.common_dir) + submodule = parent.submodules[0] + submodule.update(init=True) + target = Path(getattr(parent, metadata_dir)) + destination = root / "destination" + destination.symlink_to(target, target_is_directory=True) + before = _move_snapshot(submodule) + + with pytest.raises(ValueError, match="Git metadata"): + submodule.move("destination", module=False) + + assert _move_snapshot(submodule) == before + assert destination.is_symlink() + assert destination.samefile(target) + assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content" + + @pytest.mark.parametrize("kind", ["empty", "nonempty", "file", "dangling"]) def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind): """Preserve leaf-symlink replacement without modifying the external target. @@ -1410,6 +1634,7 @@ def test_update_rejects_parent_component_in_name(self, rwdir): invalid_names = ( "", + "nul\0name", "..", "../module", R"..\module", From b0ae041305ec342625b1398a673d5b8ada946fa5 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Fri, 2 Oct 2026 09:22:41 +0530 Subject: [PATCH 04/11] fix(util): create lock files in one exclusive step `LockFile._obtain_lock_or_raise()` tested for the lock with `osp.isfile()` and then created it with `open(lock_file, "w")`. Nothing keeps another holder out between the two calls, so several can pass the test and all of them set `_owns_lock`. Racing eight holders on one lock file leaves seven believing they own it, which removes the mutual exclusion that `GitConfigParser` in write mode and `RefLog.append_entry()` rely on. `osp.isfile()` also resolves symbolic links. A dangling symlink planted at `.lock` therefore reports no lock, and the following `open()` resolves it and creates the target, outside the repository. Create the lock with `os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)` instead. That is the single-step exclusive create `gitdb`'s `LockedFD.open()` and Git's own `lock_file()` already use, and `O_EXCL` fails with `EEXIST` on a symbolic link rather than resolving it, so both problems close together. The creation mode matches `LockedFD`; nothing reads a lock file's contents, and breaking a stale lock needs write permission on the containing directory rather than on the file. `FileExistsError` is translated back into the existing "did already exist" `OSError`, so `BlockingLockFile`'s retry loop and the existing `test_lock_file` and `test_blocking_lock_file` cases are unaffected. A directory at the lock path is still reported through the generic `OSError` branch. This covers acquiring the lock only; writing the locked file stays the caller's concern, as before. Adds `test_lock_file_does_not_follow_a_symlink` and `test_lock_file_is_obtained_by_a_single_holder`. Both fail on the previous code (`1 != 7`, and the symlink target gets created) and pass here. Validation: full `pytest` suite green on Python 3.11 on macOS, plus `ruff check`, `ruff format`, `mypy` and `basedpyright --warnings` clean. --- git/util.py | 15 +++++++++------ test/test_util.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/git/util.py b/git/util.py index 6f1f38401..ca942340d 100644 --- a/git/util.py +++ b/git/util.py @@ -1166,17 +1166,20 @@ def _obtain_lock_or_raise(self) -> None: if self._has_lock(): return lock_file = self._lock_file_path() - if osp.isfile(lock_file): + # Create the lock in one step, the way Git and gitdb's LockedFD do. Testing + # for the file first leaves a window in which another holder creates it and + # both proceed, and O_CREAT|O_EXCL additionally refuses to follow a symbolic + # link planted at the lock path instead of writing through it. + try: + fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError as e: raise OSError( "Lock for file %r did already exist, delete %r in case the lock is illegal" % (self._file_path, lock_file) - ) - - try: - with open(lock_file, mode="w"): - pass + ) from e except OSError as e: raise OSError(str(e)) from e + os.close(fd) self._owns_lock = True diff --git a/test/test_util.py b/test/test_util.py index 46417cc39..203e3a54f 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -12,6 +12,7 @@ import subprocess import sys import tempfile +import threading import time from unittest import SkipTest, mock @@ -446,6 +447,44 @@ def test_lock_file(self): lock_file._obtain_lock_or_raise() lock_file._release_lock() + @requires_symlinks + def test_lock_file_does_not_follow_a_symlink(self): + with tempfile.TemporaryDirectory() as tdir: + my_file = os.path.join(tdir, "my-lock-file") + outside = os.path.join(tdir, "outside-the-lock") + os.symlink(outside, my_file + ".lock") + + lock_file = LockFile(my_file) + self.assertRaises(IOError, lock_file._obtain_lock_or_raise) + assert not lock_file._has_lock() + assert not os.path.exists(outside) + + def test_lock_file_is_obtained_by_a_single_holder(self): + with tempfile.TemporaryDirectory() as tdir: + my_file = os.path.join(tdir, "my-lock-file") + racers = 8 + at_the_line = threading.Barrier(racers) + holders = [] + guard = threading.Lock() + + def obtain(): + lock_file = LockFile(my_file) + at_the_line.wait() + try: + lock_file._obtain_lock_or_raise() + except OSError: + return + with guard: + holders.append(lock_file) + + threads = [threading.Thread(target=obtain) for _ in range(racers)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(1, len(holders)) + def test_blocking_lock_file(self): with tempfile.TemporaryDirectory() as tdir: my_file = os.path.join(tdir, "my-lock-file") From 97a546899df8b41496c71c128b9b4154bacc5fe5 Mon Sep 17 00:00:00 2001 From: Byron Date: Fri, 2 Oct 2026 04:52:11 +0000 Subject: [PATCH 05/11] fix(util): acquire Windows locks without following symlinks Took brief look only. Hope this code soon won't be present anymore. Windows follows dangling symlinks even when `os.open` uses `O_CREAT | O_EXCL`, so acquiring a lock could create its symlink target and incorrectly report ownership. The `_winapi.CreateFile` workaround also uses the ANSI API on Python 3.8 through 3.10, causing failures in Unicode directories or creating locks under mangled filenames. Use `CreateFileW` with `CREATE_NEW` and `FILE_FLAG_OPEN_REPARSE_POINT` to create the lock atomically while rejecting existing links. Declare the `ctypes` argument and return types explicitly so Unicode paths and native handle sizes are preserved. Close the handle before recording ownership, propagate Windows errors, and reject embedded NULs before the native API can truncate a path. Preserve exclusive `os.open` creation on POSIX. Expand the lock tests to cover Unicode filenames and directories, including non-BMP characters, and verify that the requested lock path is actually created and removed. Check NUL rejection, preserve both existing and missing symlink targets, and explicitly release the concurrent test's acquired locks. Reproduced the CI failure in `test_clone_from_with_path_contains_unicode` on Windows/Python 3.8.10 before the fix. The affected utility, clone, and configuration modules pass on Python 3.8.10 and 3.13.14: 169 passed, 41 skipped, and 2 expected failures on each version. The new Unicode and NUL regressions also failed before their respective fixes. `ruff check`, `ruff format --check`, `mypy --python-version=3.13`, and `basedpyright --warnings` pass. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6 --- git/util.py | 50 +++++++++++++++++++++++++++++++++++++++++------ test/test_util.py | 37 ++++++++++++++++++++++++++++++----- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/git/util.py b/git/util.py index ca942340d..017a94ad3 100644 --- a/git/util.py +++ b/git/util.py @@ -1166,12 +1166,51 @@ def _obtain_lock_or_raise(self) -> None: if self._has_lock(): return lock_file = self._lock_file_path() - # Create the lock in one step, the way Git and gitdb's LockedFD do. Testing - # for the file first leaves a window in which another holder creates it and - # both proceed, and O_CREAT|O_EXCL additionally refuses to follow a symbolic - # link planted at the lock path instead of writing through it. + # Create the lock in one step. Checking for it first would allow another + # holder to create it between the check and the open. try: - fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + if sys.platform == "win32": + if "\0" in lock_file: + raise ValueError("embedded null character") + + import ctypes + from ctypes import wintypes + + # Unlike POSIX, Windows follows dangling symlinks even with O_EXCL. + # Open the reparse point itself so an existing link is rejected. + # Call the Unicode API directly: older _winapi.CreateFile wrappers + # use the ANSI API and can create a lock under the wrong filename. + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + create_file = kernel32.CreateFileW + create_file.argtypes = ( + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ) + create_file.restype = wintypes.HANDLE + close_handle = kernel32.CloseHandle + close_handle.argtypes = (wintypes.HANDLE,) + close_handle.restype = wintypes.BOOL + handle = create_file( + lock_file, + 0x40000000, # GENERIC_WRITE + 0, + None, + 1, # CREATE_NEW + 0x00200000, # FILE_FLAG_OPEN_REPARSE_POINT + None, + ) + if handle == wintypes.HANDLE(-1).value: + raise ctypes.WinError(ctypes.get_last_error()) + if not close_handle(handle): + raise ctypes.WinError(ctypes.get_last_error()) + else: + fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + os.close(fd) except FileExistsError as e: raise OSError( "Lock for file %r did already exist, delete %r in case the lock is illegal" @@ -1179,7 +1218,6 @@ def _obtain_lock_or_raise(self) -> None: ) from e except OSError as e: raise OSError(str(e)) from e - os.close(fd) self._owns_lock = True diff --git a/test/test_util.py b/test/test_util.py index 203e3a54f..eb520aac6 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -419,9 +419,11 @@ def test_it_should_dashify(self): self.assertEqual("this-is-my-argument", dashify("this_is_my_argument")) self.assertEqual("foo", dashify("foo")) - def test_lock_file(self): + @ddt.data("my-lock-file", "my-lock-file-\u0394", "\u0394/my-lock-file", "\U0001f680/my-lock-file") + def test_lock_file(self, filename): with tempfile.TemporaryDirectory() as tdir: - my_file = os.path.join(tdir, "my-lock-file") + my_file = os.path.join(tdir, filename) + os.makedirs(os.path.dirname(my_file), exist_ok=True) lock_file = LockFile(my_file) assert not lock_file._has_lock() # Release lock we don't have - fine. @@ -430,6 +432,7 @@ def test_lock_file(self): # Get lock. lock_file._obtain_lock_or_raise() assert lock_file._has_lock() + assert os.path.isfile(my_file + ".lock") # Concurrent access. other_lock_file = LockFile(my_file) @@ -438,6 +441,7 @@ def test_lock_file(self): lock_file._release_lock() assert not lock_file._has_lock() + assert not os.path.exists(my_file + ".lock") other_lock_file._obtain_lock_or_raise() self.assertRaises(IOError, lock_file._obtain_lock_or_raise) @@ -447,17 +451,36 @@ def test_lock_file(self): lock_file._obtain_lock_or_raise() lock_file._release_lock() + def test_lock_file_rejects_embedded_nul(self): + with tempfile.TemporaryDirectory() as tdir: + my_file = os.path.join(tdir, "my-lock-file") + lock_file = LockFile(my_file + "\0suffix") + self.assertRaises(ValueError, lock_file._obtain_lock_or_raise) + assert not lock_file._has_lock() + assert not os.path.exists(my_file) + + @ddt.data(False, True) @requires_symlinks - def test_lock_file_does_not_follow_a_symlink(self): + def test_lock_file_does_not_follow_a_symlink(self, target_exists): with tempfile.TemporaryDirectory() as tdir: my_file = os.path.join(tdir, "my-lock-file") outside = os.path.join(tdir, "outside-the-lock") + content = b"Do not modify the symlink target." + if target_exists: + with open(outside, "wb") as stream: + stream.write(content) os.symlink(outside, my_file + ".lock") lock_file = LockFile(my_file) self.assertRaises(IOError, lock_file._obtain_lock_or_raise) assert not lock_file._has_lock() - assert not os.path.exists(outside) + lock_file._release_lock() + assert os.path.islink(my_file + ".lock") + if target_exists: + with open(outside, "rb") as stream: + self.assertEqual(stream.read(), content) + else: + assert not os.path.exists(outside) def test_lock_file_is_obtained_by_a_single_holder(self): with tempfile.TemporaryDirectory() as tdir: @@ -483,7 +506,11 @@ def obtain(): for thread in threads: thread.join() - self.assertEqual(1, len(holders)) + try: + self.assertEqual(1, len(holders)) + finally: + for lock_file in holders: + lock_file._release_lock() def test_blocking_lock_file(self): with tempfile.TemporaryDirectory() as tdir: From 226654af2d560b2de87f83f9c86eb9a3312c9496 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Sat, 3 Oct 2026 00:27:34 +0530 Subject: [PATCH 06/11] fix(commit): accumulate gpgsig continuation lines in linear time `Commit._deserialize` collected a `gpgsig` header by appending each continuation line to a `bytes` object with `+=`. `bytes` are immutable, so every line copied the whole signature read so far, and a signature with *n* continuation lines cost O(n^2) to parse. Commit headers are fully controlled by whoever wrote the commit, and `_deserialize` runs on the first access to any commit attribute (`author`, `message`, `parents`, ...). Reading the commits of an untrusted repository is therefore enough to hit it. Measured on Python 3.11 with a `gpgsig` header of `" x\n"` lines: | object size | before | after | | ----------- | ------- | ------ | | 2.4 MB | 9.5 s | 0.08 s | | 4.8 MB | 116.8 s | | | 9.6 MB | 547.9 s | 0.32 s | Collect the lines in a list and join them once. This is linear and produces byte-identical `gpgsig` values, so the existing `test_gpgsig` round trip is unchanged. `test_gpgsig_deserialization_is_linear` deserializes a 1.8 MB signature under a 1 s CPU-time bound. It fails on the previous code and passes here. The full suite has no new failures; the 8 tests that fail locally (non-UTF-8 trailer encoding and NUL submodule names) fail identically without this change. `ruff`, `mypy` and `basedpyright --warnings` are clean. --- git/objects/commit.py | 6 +++--- test/test_commit.py | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/git/objects/commit.py b/git/objects/commit.py index 771f87976..307b8c59b 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -885,7 +885,7 @@ def _deserialize(self, stream: BytesIO) -> "Commit": if buf[0:10] == b"encoding ": self.encoding = buf[buf.find(b" ") + 1 :].decode(self.encoding, "ignore") elif buf[0:7] == b"gpgsig ": - sig = buf[buf.find(b" ") + 1 :] + b"\n" + sig_lines = [buf[buf.find(b" ") + 1 :] + b"\n"] is_next_header = False while True: sigbuf = readline() @@ -895,9 +895,9 @@ def _deserialize(self, stream: BytesIO) -> "Commit": buf = sigbuf.strip() is_next_header = True break - sig += sigbuf[1:] + sig_lines.append(sigbuf[1:]) # END read all signature - self.gpgsig = sig.rstrip(b"\n").decode(self.encoding, "ignore") + self.gpgsig = b"".join(sig_lines).rstrip(b"\n").decode(self.encoding, "ignore") if is_next_header: continue buf = readline().strip() diff --git a/test/test_commit.py b/test/test_commit.py index b5baf0e33..0a5bcdb7d 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -607,6 +607,28 @@ def test_commit_co_authors_bounds_malformed_trailer(self): # The malformed line yields nothing; the well-formed trailer still parses. assert result == [Actor("Real Name", "real@example.com")] + def test_gpgsig_deserialization_is_linear(self): + """A long gpgsig header must not make deserialization run in quadratic time.""" + num_lines = 600_000 + # Commit headers are fully attacker-controlled. Accumulating the signature with + # bytes concatenation copied it once per continuation line (O(n^2)). + data = ( + b"tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n" + b"author A 1700000000 +0000\n" + b"committer A 1700000000 +0000\n" + b"gpgsig -----BEGIN PGP SIGNATURE-----\n" + b" x\n" * num_lines + b" -----END PGP SIGNATURE-----\n" + b"\n" + b"message\n" + ) + cmt = copy.copy(self.rorepo.commit()) + start = time.process_time() + cmt._deserialize(BytesIO(data)) + elapsed = time.process_time() - start + # Leave ample CPU time for slow runners, but catch quadratic accumulation. + self.assertLess(elapsed, 1.0) + self.assertEqual(cmt.gpgsig.count("\n"), num_lines + 1) + self.assertEqual(cmt.message, "message\n") + @with_rw_directory def test_create_from_tree_with_trailers_dict(self, rw_dir): """Test that create_from_tree supports adding trailers via a dict.""" From 5fc5f3456a6816d4caa03fdb4a3b000d3170a290 Mon Sep 17 00:00:00 2001 From: Byron Date: Sat, 3 Oct 2026 04:31:59 +0200 Subject: [PATCH 07/11] review - remove test as it can totally fail on slow runners, and extending the time budget makes it hard to see if it's actually working. Let's trust what we see and look forward to a time when the in-python parsing is gone as well. --- test/test_commit.py | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/test/test_commit.py b/test/test_commit.py index 0a5bcdb7d..b5baf0e33 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -607,28 +607,6 @@ def test_commit_co_authors_bounds_malformed_trailer(self): # The malformed line yields nothing; the well-formed trailer still parses. assert result == [Actor("Real Name", "real@example.com")] - def test_gpgsig_deserialization_is_linear(self): - """A long gpgsig header must not make deserialization run in quadratic time.""" - num_lines = 600_000 - # Commit headers are fully attacker-controlled. Accumulating the signature with - # bytes concatenation copied it once per continuation line (O(n^2)). - data = ( - b"tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n" - b"author A 1700000000 +0000\n" - b"committer A 1700000000 +0000\n" - b"gpgsig -----BEGIN PGP SIGNATURE-----\n" + b" x\n" * num_lines + b" -----END PGP SIGNATURE-----\n" - b"\n" - b"message\n" - ) - cmt = copy.copy(self.rorepo.commit()) - start = time.process_time() - cmt._deserialize(BytesIO(data)) - elapsed = time.process_time() - start - # Leave ample CPU time for slow runners, but catch quadratic accumulation. - self.assertLess(elapsed, 1.0) - self.assertEqual(cmt.gpgsig.count("\n"), num_lines + 1) - self.assertEqual(cmt.message, "message\n") - @with_rw_directory def test_create_from_tree_with_trailers_dict(self, rw_dir): """Test that create_from_tree supports adding trailers via a dict.""" From 6795806a4d02781d7150fc71e27b7e778bb65b55 Mon Sep 17 00:00:00 2001 From: kokotatan Date: Sat, 3 Oct 2026 23:45:30 +0900 Subject: [PATCH 08/11] fix(index): accept a single PathLike in checkout `IndexFile.checkout()` documents accepting a single path, but only wraps strings before iterating. Passing one `pathlib.Path` or custom `os.PathLike` therefore raises `TypeError`, while a list containing the same object already works. Wrap single `os.PathLike` objects along with strings and widen the public annotation. Keep existing path normalization and iterable behavior. Add 36 real-repository cases covering path types, absolute and relative paths, files and directories, and single/list/iterator inputs. Eight single-PathLike cases failed before this fix. The focused tests pass, as do all pre-commit hooks, `mypy`, `basedpyright`, and strict Sphinx HTML. The full suite has 1,410 passing tests and 121 Windows symlink-privilege failures, all reproduced with identical node IDs on the pristine base. Assisted-by: OpenAI GPT-6 --- git/index/base.py | 4 ++-- test/test_index.py | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/git/index/base.py b/git/index/base.py index c9d99840d..4ecd0c5b1 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1303,7 +1303,7 @@ def _flush_stdin_and_wait(self, proc: "Popen[bytes]", ignore_stdout: bool = Fals @default_index def checkout( self, - paths: Union[None, Iterable[PathLike]] = None, + paths: Union[None, PathLike, Iterable[PathLike]] = None, force: bool = False, fprogress: Callable = lambda *args: None, allow_unsafe_options: bool = False, @@ -1442,7 +1442,7 @@ def handle_stderr(proc: "Popen[bytes]", iter_checked_out_files: Iterable[PathLik handle_stderr(proc, rval_iter) return rval_iter else: - if isinstance(paths, str): + if isinstance(paths, (str, os.PathLike)): paths = [paths] # Make sure we have our entries loaded before we start checkout_index, which diff --git a/test/test_index.py b/test/test_index.py index 8c33fe29d..30cb54565 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -1731,6 +1731,32 @@ def test_index_file_v3_with_git_command(self, tmp_dir): assert "A file2.txt" in status_lines +class TestIndexCheckout: + @pytest.mark.parametrize("path_type", [str, Path, PathLikeMock]) + @pytest.mark.parametrize("absolute", [False, True]) + @pytest.mark.parametrize("directory", [False, True]) + @pytest.mark.parametrize("container", ["single", "list", "iterator"]) + def test_checkout_pathlike(self, tmp_path, path_type, absolute, directory, container): + with Repo.init(tmp_path) as repo: + nested = tmp_path / "nested" + nested.mkdir() + files = {"nested/first": b"first", "nested/second": b"second", "outside": b"outside"} + for name, data in files.items(): + (tmp_path / name).write_bytes(data) + repo.index.add(list(files)) + + path_name = "nested" if directory else "nested/first" + path = path_type(str(tmp_path / path_name) if absolute else path_name) + paths = path if container == "single" else [path] if container == "list" else iter([path]) + expected = {"nested/first", "nested/second"} if directory else {"nested/first"} + for name in expected: + (tmp_path / name).unlink() + + assert set(repo.index.checkout(paths)) == expected + for name, data in files.items(): + assert (tmp_path / name).read_bytes() == data + + class TestIndexUtils: @pytest.mark.parametrize("file_path_type", [str, Path]) def test_temporary_file_swap(self, tmp_path, file_path_type): From 6fdfa315406170a0fbc4898123e0bfda67cb9070 Mon Sep 17 00:00:00 2001 From: Yunare Maia Date: Mon, 5 Oct 2026 02:14:59 +0000 Subject: [PATCH 09/11] fix(util): redact URL credentials without corrupting the host remove_password_if_present() redacted credentials by substituting them into url.netloc with str.replace(), which rewrites every occurrence anywhere in the netloc -- including the host. https://git@github.com/user/repo.git -> https://*****@*****hub.com/user/repo.git "git" is the most common Git username and a substring of "github.com", so this hits the common case. An empty username or password makes it worse: str.replace("", "*****") matches at every position, so https://:token@fakerepo.example.com/testrepo comes back shredded into a URL roughly ten times longer, which is what lands in GitCommandError messages when a fetch fails. Rebuild the netloc from its userinfo instead, so only the credentials are replaced. The existing test already covered the empty-password case but only asserted the password is absent, which any mangling satisfies. The function is the single redaction path used for logged command lines and for exception messages (git/cmd.py, git/exc.py, git/repo/base.py), so this fixes all of them at once. --- git/util.py | 12 ++++++++---- test/test_util.py | 13 +++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/git/util.py b/git/util.py index 017a94ad3..deebdc8d0 100644 --- a/git/util.py +++ b/git/util.py @@ -658,10 +658,14 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: if url.password is None and url.username is None: continue - if url.password is not None: - url = url._replace(netloc=url.netloc.replace(url.password, "*****")) - if url.username is not None: - url = url._replace(netloc=url.netloc.replace(url.username, "*****")) + # Redact the userinfo as a whole rather than substituting the + # username/password into the netloc: a substring replace also hits + # the host ("git" in "github.com") and an empty username or password + # matches at every position. + _, at, hostinfo = url.netloc.rpartition("@") + if at: + redacted = "*****:*****" if url.password is not None else "*****" + url = url._replace(netloc=f"{redacted}@{hostinfo}") new_cmdline[index] = urlunsplit(url) except ValueError: # This is not a valid URL. diff --git a/test/test_util.py b/test/test_util.py index eb520aac6..5b72cb432 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -828,6 +828,19 @@ def test_remove_password_from_command_line(self): assert authorization not in " ".join(redacted_cmd_6) assert "http.extraHeader=Authorization: *****" in redacted_cmd_6 + def test_remove_password_keeps_host_intact(self): + """Redaction must not touch the host, even when it contains the username.""" + redacted = remove_password_if_present(["git", "clone", "https://git@github.com/user/repo.git"]) + assert redacted == ["git", "clone", "https://*****@github.com/user/repo.git"] + + redacted = remove_password_if_present(["git", "clone", "ssh://git@github.com/u/r.git"]) + assert redacted == ["git", "clone", "ssh://*****@github.com/u/r.git"] + + def test_remove_empty_password_keeps_host_intact(self): + """An empty password must not expand into every position of the netloc.""" + redacted = remove_password_if_present(["git", "clone", "https://:@fakerepo.example.com/testrepo"]) + assert redacted == ["git", "clone", "https://*****:*****@fakerepo.example.com/testrepo"] + def test_mode_str_to_int_accepts_bytes(): assert mode_str_to_int("100644") == 0o100644 From 0f89e3621943a34c49d702061308ac90320780f5 Mon Sep 17 00:00:00 2001 From: Byron Date: Mon, 5 Oct 2026 04:42:18 +0200 Subject: [PATCH 10/11] review - add some edge cases to the tests. Assisted-by: GPT 6.1 Sol Co-authored-by: GPT 6.1 Sol --- git/util.py | 13 +++++-------- test/test_util.py | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/git/util.py b/git/util.py index deebdc8d0..75677b7d3 100644 --- a/git/util.py +++ b/git/util.py @@ -658,14 +658,11 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: if url.password is None and url.username is None: continue - # Redact the userinfo as a whole rather than substituting the - # username/password into the netloc: a substring replace also hits - # the host ("git" in "github.com") and an empty username or password - # matches at every position. - _, at, hostinfo = url.netloc.rpartition("@") - if at: - redacted = "*****:*****" if url.password is not None else "*****" - url = url._replace(netloc=f"{redacted}@{hostinfo}") + # Match urllib.parse's userinfo boundary. Keeping the raw hostinfo + # preserves hostname case, IPv6 brackets, and port formatting. + _, _, hostinfo = url.netloc.rpartition("@") + redacted = "*****:*****" if url.password is not None else "*****" + url = url._replace(netloc=f"{redacted}@{hostinfo}") new_cmdline[index] = urlunsplit(url) except ValueError: # This is not a valid URL. diff --git a/test/test_util.py b/test/test_util.py index 5b72cb432..ca8e006f3 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -841,6 +841,21 @@ def test_remove_empty_password_keeps_host_intact(self): redacted = remove_password_if_present(["git", "clone", "https://:@fakerepo.example.com/testrepo"]) assert redacted == ["git", "clone", "https://*****:*****@fakerepo.example.com/testrepo"] + @ddt.data( + ( + "https://user%40example.com:p%40ss@GitHub.COM:00443/repo@name?q=a@b#c@d", + "https://*****:*****@GitHub.COM:00443/repo@name?q=a@b#c@d", + ), + ("//user:pass@[2001:db8::1]:0080/repo", "//*****:*****@[2001:db8::1]:0080/repo"), + ("https://user:p@ss@example.com/repo", "https://*****:*****@example.com/repo"), + ("https://user:@example.com/repo", "https://*****:*****@example.com/repo"), + ("https://@example.com/repo", "https://*****@example.com/repo"), + ("https://example.com/repo@name?q=a@b#c@d", "https://example.com/repo@name?q=a@b#c@d"), + ) + @ddt.unpack + def test_remove_password_preserves_url_components(self, url, expected): + assert remove_password_if_present([url]) == [expected] + def test_mode_str_to_int_accepts_bytes(): assert mode_str_to_int("100644") == 0o100644 From af9ec3e96dd5e1fa08e117c809208a5eabf80f8e Mon Sep 17 00:00:00 2001 From: LE THANH LONG THANH TONG Date: Mon, 5 Oct 2026 15:11:12 +0200 Subject: [PATCH 11/11] change line for changelogs --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 4b8258fcb..ce7ed9cac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,6 +38,7 @@ dynamic = ["license", "version", "dependencies", "optional-dependencies"] [project.urls] "Source code" = "https://github.com/gitpython-developers/GitPython" "Documentation" = "https://gitpython.readthedocs.io/en/stable/" +"Changelog" = "https://github.com/gitpython-developers/GitPython/blob/main/doc/source/changes.rst" [dependency-groups] doc = [