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"] diff --git a/AGENTS.md b/AGENTS.md index 6b2963cf4..cdffa4764 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,8 @@ Before starting work, read and follow [CONTRIBUTING.md](CONTRIBUTING.md), including the [Prevent agent impersonation](CONTRIBUTING.md#prevent-agent-impersonation) -section governing identification when communicating through a person's account. +section governing agent identification and separation of unaltered user statements +when communicating through a person's account. # Commit messages diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 76f276323..1a64a701d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,6 +27,13 @@ example in issue or PR descriptions and comments. AI assistance that does not re the person as the speaker, such as proofreading or wording polish, does not require identification. +Even when identifying themselves, agents must not assert on a person's behalf that +that person performed an action, such as reviewing or approving a PR. The person +must make any such statement themselves. If it is included alongside agent-authored +content, it must be supplied by the person and preserved verbatim in a clearly +labeled, separate user-authored section. Agents must not draft, paraphrase, or embed +such statements in their own narration. + Attributing AI assistance in commit metadata, for example with a `Co-authored-by` trailer, is welcome but not required. Code is reviewed the same way regardless of its origin. 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/cmd.py b/git/cmd.py index cb88e7c24..1d1ca909a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -15,44 +15,27 @@ import re import signal import subprocess -from subprocess import DEVNULL, PIPE, Popen import sys -from textwrap import dedent import threading +import time import warnings - -from git.compat import defenc, force_bytes, safe_decode -from git.exc import ( - CommandError, - GitCommandError, - GitCommandNotFound, - UnsafeOptionError, - UnsafeProtocolError, -) -from git.util import ( - cygpath, - expand_path, - is_cygwin_git, - patch_env, - remove_password_if_present, - stream_copy, -) +from subprocess import DEVNULL, PIPE, Popen +from textwrap import dedent # typing --------------------------------------------------------------------------- - from typing import ( + IO, + TYPE_CHECKING, Any, AnyStr, BinaryIO, Callable, Dict, - IO, Iterator, List, Mapping, Optional, Sequence, - TYPE_CHECKING, TextIO, Tuple, Union, @@ -60,12 +43,29 @@ overload, ) +from git.compat import defenc, force_bytes, safe_decode +from git.exc import ( + CommandError, + GitCommandError, + GitCommandNotFound, + UnsafeOptionError, + UnsafeProtocolError, +) +from git.util import ( + cygpath, + expand_path, + is_cygwin_git, + patch_env, + remove_password_if_present, + stream_copy, +) + if sys.version_info >= (3, 10): from typing import TypeAlias else: from typing_extensions import TypeAlias -from git.types import Literal, PathLike, TBD +from git.types import TBD, Literal, PathLike if TYPE_CHECKING: from git.diff import DiffIndex @@ -99,6 +99,45 @@ ## @{ +def _kill_process(pid: int) -> bool: + """Kill a POSIX process and its descendants, returning whether it was killed.""" + # Collect descendants before signalling, while their parent PIDs still identify them. + pids = [pid] + for parent_pid in pids: + try: + try: + p = Popen(["pgrep", "-P", str(parent_pid)], stdout=PIPE) + except FileNotFoundError: + # POSIX ps does not support selecting by parent PID. + ps_args = ["ps", "-ef"] if sys.platform == "cygwin" else ["ps", "-A", "-o", "pid=", "-o", "ppid="] + with Popen(ps_args, stdout=PIPE) as p: + if p.stdout is not None: + for line in p.stdout: + fields = line.split() + if sys.platform == "cygwin": + fields = fields[1:3] # ps -ef starts with UID, PID, PPID. + if len(fields) == 2 and all(field.isdigit() for field in fields): + if int(fields[1]) == parent_pid: + pids.append(int(fields[0])) + else: + with p: + if p.stdout is not None: + for line in p.stdout: + if line.strip().isdigit(): + pids.append(int(line)) + except OSError as ex: + _logger.info("Unable to enumerate child processes: %r", ex) + killed = False + if sys.platform != "win32": + for process_pid in pids: + try: + os.kill(process_pid, signal.SIGKILL) + killed = killed or process_pid == pid + except OSError: + pass + return killed + + def handle_process_output( process: Union["Git.AutoInterrupt", Popen], stdout_handler: Union[ @@ -165,7 +204,7 @@ def pump_stream( except Exception as ex: _logger.error(f"Pumping {name!r} of cmd({remove_password_if_present(cmdline)}) failed due to: {ex!r}") - if "I/O operation on closed file" not in str(ex): + if not (isinstance(ex, ValueError) and stream.closed): # Only reraise if the error was not due to the stream closing. raise CommandError([f"<{name}-pump>"] + remove_password_if_present(cmdline), ex) from ex finally: @@ -199,28 +238,26 @@ def pump_stream( t.start() threads.append(t) - # FIXME: Why join? Will block if stdin needs feeding... + # Wait for output handlers to finish before finalizing. + # If the child needs stdin, the caller must arrange to feed/close it + # before this call or concurrently; this function only drains output. + deadline = None if kill_after_timeout is None else time.monotonic() + kill_after_timeout for t in threads: - t.join(timeout=kill_after_timeout) + t.join(timeout=None if deadline is None else max(0, deadline - time.monotonic())) if t.is_alive(): if isinstance(process, Git.AutoInterrupt): + if sys.platform != "win32" and process.proc is not None and process.proc.poll() is None: + _kill_process(process.proc.pid) process._terminate() else: # Don't want to deal with the other case. raise RuntimeError( "Thread join() timed out in cmd.handle_process_output()." f" kill_after_timeout={kill_after_timeout} seconds" ) - if stderr_handler: - error_str: Union[str, bytes] = ( - f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds" - ) - if not decode_streams and isinstance(p_stderr, BinaryIO): - # Assume stderr_handler needs binary input. - error_str = cast(str, error_str) - error_str = error_str.encode() - # We ignore typing on the next line because mypy does not like the way - # we inferred that stderr takes str or bytes. - stderr_handler(error_str) # type: ignore[arg-type] + process._timeout_error = ( + f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds" + ) + break if finalizer: finalizer(process) @@ -326,7 +363,7 @@ class _AutoInterrupt: raise. """ - __slots__ = ("proc", "args", "status") + __slots__ = ("proc", "args", "status", "_timeout_error") # If this is non-zero it will override any status code during _terminate, used # to prevent race conditions in testing. @@ -336,6 +373,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: self.proc = proc self.args = args self.status: Union[int, None] = None + self._timeout_error: Optional[str] = None def _terminate(self) -> None: """Terminate the underlying process.""" @@ -344,37 +382,45 @@ def _terminate(self) -> None: proc = self.proc self.proc = None - if proc.stdin: - proc.stdin.close() - if proc.stdout: - proc.stdout.close() - if proc.stderr: - proc.stderr.close() - # Did the process finish already so we have a return code? try: - if proc.poll() is not None: - self.status = self._status_code_if_terminate or proc.poll() + if proc.stdin: + # A timed-out process may already have exited before input is flushed. + with contextlib.suppress(BrokenPipeError): + proc.stdin.close() + # Did the process finish already so we have a return code? + try: + if proc.poll() is not None: + self.status = self._status_code_if_terminate or proc.poll() + return + except OSError as ex: + _logger.info("Ignored error after process had died: %r", ex) + + # It can be that nothing really exists anymore... + if getattr(os, "kill", None) is None: return - except OSError as ex: - _logger.info("Ignored error after process had died: %r", ex) - # It can be that nothing really exists anymore... - if os is None or getattr(os, "kill", None) is None: - return + # Try to kill it. + try: + proc.terminate() + except (OSError, AttributeError) as ex: + # On interpreter shutdown (notably on Windows), parts of the stdlib used by + # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None), + # which can cause AttributeError during terminate(). In that case, we prefer + # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. + _logger.info("Ignored error while terminating process: %r", ex) + return + # END exception handling + finally: + if proc.stdout: + proc.stdout.close() + if proc.stderr: + proc.stderr.close() - # Try to kill it. try: - proc.terminate() - status = proc.wait() # Ensure the process goes away. - + status = proc.wait() self.status = self._status_code_if_terminate or status except (OSError, AttributeError) as ex: - # On interpreter shutdown (notably on Windows), parts of the stdlib used by - # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None), - # which can cause AttributeError during terminate(). In that case, we prefer - # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. - _logger.info("Ignored error while terminating process: %r", ex) - # END exception handling + _logger.info("Ignored error while waiting for terminated process: %r", ex) def __del__(self) -> None: self._terminate() @@ -393,9 +439,8 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: May deadlock if output or error pipes are used and not handled separately. :raise git.exc.GitCommandError: - If the return status is not 0. + If the return status is not 0 or output handling timed out. """ - stderr_b = force_bytes(data=stderr, encoding="utf-8") or b"" status: Union[int, None] if self.proc is not None: status = self.proc.wait() @@ -404,6 +449,11 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: status = self.status p_stderr = None + stderr_b = force_bytes(data=stderr, encoding="utf-8") or b"" + if self._timeout_error is not None: + stderr_b += (b"\n" if stderr_b else b"") + self._timeout_error.encode("utf-8") + status = status or 1 + def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes: if stream: try: @@ -1381,9 +1431,10 @@ def execute( 1. This feature is not supported at all on Windows. 2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not - installed. Effectiveness may vary on systems without these commands. - 3. Deeper descendants do not receive signals, though they may sometimes - terminate as a consequence of their parent processes being killed. + installed (``ps -ef`` on Cygwin). Effectiveness may vary on systems + without these commands. + 3. Descendants are enumerated before signalling. Processes that detach + or spawn after enumeration may not receive signals. 4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side effects on a repository. For example, stale locks in case of :manpage:`git-gc(1)` could render the repository incapable of accepting @@ -1537,43 +1588,9 @@ def execute( timeout = kill_after_timeout def kill_process(pid: int) -> None: - """Callback to kill a process. - - This callback implementation would be ineffective and unsafe on Windows. - """ - child_pids = [] - try: - p = Popen(["pgrep", "-P", str(pid)], stdout=PIPE) - except FileNotFoundError: - # POSIX ps does not support selecting by parent PID. - with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p: - if p.stdout is not None: - for line in p.stdout: - fields = line.split() - if len(fields) == 2 and all(field.isdigit() for field in fields): - if int(fields[1]) == pid: - child_pids.append(int(fields[0])) - else: - with p: - if p.stdout is not None: - for line in p.stdout: - if line.strip().isdigit(): - child_pids.append(int(line)) - try: - os.kill(pid, signal.SIGKILL) - for child_pid in child_pids: - try: - os.kill(child_pid, signal.SIGKILL) - except OSError: - pass - # Tell the main routine that the process was killed. + if _kill_process(pid): assert kill_check is not None kill_check.set() - except OSError: - # It is possible that the process gets completed in the duration - # after timeout happens and before we try to kill the process. - pass - return def make_timeout_error() -> Union[str, bytes]: err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.' @@ -1923,9 +1940,15 @@ def _prepare_ref(self, ref: object) -> bytes: else: refstr = ref - if not refstr.endswith("\n"): - refstr += "\n" - return refstr.encode(defenc) + # A line feed terminates a request, so one object name must be one line. An + # embedded one would queue a second request on the persistent command while + # only one response line is read back, leaving every later call one response + # behind, answered with the header of an object it did not ask for. + if refstr.endswith("\n"): + refstr = refstr[:-1] + if "\n" in refstr: + raise ValueError("Object name %r contains a line feed" % refstr) + return (refstr + "\n").encode(defenc) def _get_persistent_cmd(self, attr_name: str, cmd_name: str, *args: Any, **kwargs: Any) -> "Git.AutoInterrupt": cur_val = getattr(self, attr_name) diff --git a/git/index/base.py b/git/index/base.py index c9d99840d..17a722c21 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -525,17 +525,17 @@ def _write_path_to_stdin( the piped-in files are processed anyway and just in time. :note: - Newlines are essential here, git's behaviour is somewhat inconsistent on - this depending on the version, hence we try our best to deal with newlines - carefully. Usually the last newline will not be sent, instead we will close - stdin to break the pipe. + Paths are NUL-terminated, so the command has to run with ``-z``. A path can + contain a line feed, and with line-feed separation git would read such a + path as two paths and act on files that were never passed. git also unquotes + a line-feed separated path that begins with a double quote. """ fprogress(filepath, False, item) rval: Union[None, str] = None if proc.stdin is not None: try: - proc.stdin.write(("%s\n" % filepath).encode(defenc)) + proc.stdin.write(("%s\0" % filepath).encode(defenc)) except OSError as e: # Pipe broke, usually because some error happened. raise fmakeexc() from e @@ -728,6 +728,8 @@ def _preprocess_add_items( else: raise TypeError("Invalid Type: %r" % item) # END for each item + # Source paths must be safe to read, but their recorded names may be rewritten. + # Apply mode-dependent restrictions to the final entries in add(). for entry in entries: _validate_repo_path(entry.path) return paths, entries @@ -1026,7 +1028,7 @@ def handle_null_entries(self: "IndexFile") -> None: # FINALIZE # Add the new entries to this instance. for entry in entries_added: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) for entry in entries_added: self.entries[(entry.path, 0)] = IndexEntry.from_base(entry) @@ -1303,7 +1305,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 +1444,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 @@ -1450,6 +1452,7 @@ def handle_stderr(proc: "Popen[bytes]", iter_checked_out_files: Iterable[PathLik # initialization. self.entries # noqa: B018 + args.append("-z") args.append("--stdin") kwargs["as_process"] = True kwargs["istream"] = subprocess.PIPE diff --git a/git/index/fun.py b/git/index/fun.py index 929038f1f..2491f55ae 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -279,7 +279,7 @@ def write_cache( # Body for entry in entries: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) beginoffset = tell() write(entry.ctime_bytes) # ctime write(entry.mtime_bytes) # mtime @@ -394,7 +394,7 @@ def read_cache( if terminator != b"\0": raise ValueError("Unterminated index entry path") path = path_bytes.decode(defenc) - _validate_repo_path(path) + _validate_repo_path(path, mode) real_size = (tell() - beginoffset + 7) & ~7 padding_size = beginoffset + real_size - tell() @@ -462,7 +462,7 @@ def write_tree_from_cache( """ if si == 0: for entry in entries[sl]: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) tree_items: List["TreeCacheTup"] = [] ci = sl.start @@ -510,7 +510,7 @@ def write_tree_from_cache( def _tree_entry_to_baseindexentry(tree_entry: "TreeCacheTup", stage: int) -> BaseIndexEntry: - _validate_repo_path(tree_entry[2]) + _validate_repo_path(tree_entry[2], tree_entry[1]) return BaseIndexEntry((tree_entry[1], tree_entry[0], stage << CE_STAGESHIFT, tree_entry[2])) diff --git a/git/objects/commit.py b/git/objects/commit.py index 771f87976..690a296a5 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -662,6 +662,10 @@ def create_from_tree( :return: :class:`Commit` object representing the new commit. + :raise ValueError: + If the name or email of the author or committer contains ``<``, ``>`` or a + line feed, as these would change the identity headers of the commit. + :note: Additional information about the committer and author are taken from the environment or from the git configuration. See :manpage:`git-commit-tree(1)` @@ -786,6 +790,16 @@ def create_from_tree( # { Serializable Implementation def _serialize(self, stream: BytesIO) -> "Commit": + # An identity is written as "name date" on a single header line, so a + # line feed or an angle bracket inside a name or email moves those boundaries: + # it can add header lines, end the headers early, or present another email. + # Git drops these three characters when it writes an identity; refuse them + # here before anything is written. + for actor in (self.author, self.committer): + for value in (actor.name, actor.email): + if value and any(char in value for char in "<>\n"): + raise ValueError("Commit identity %r must not contain '<', '>' or a line feed" % value) + write = stream.write write(("tree %s\n" % self.tree).encode("ascii")) for p in self.parents: @@ -885,7 +899,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 +909,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/git/objects/fun.py b/git/objects/fun.py index ff1bc8483..41ddecafd 100644 --- a/git/objects/fun.py +++ b/git/objects/fun.py @@ -39,11 +39,11 @@ # --------------------------------------------------- -def _validate_tree_entry_name(name: str) -> None: +def _validate_tree_entry_name(name: str, mode: Union[int, None] = None) -> None: if "/" in name: raise ValueError("Tree entry names must not contain '/' characters") # A tree name is a component, not a rooted path; a colon cannot select a drive. - _validate_repo_path("tree/" + name) + _validate_repo_path("tree/" + name, mode) def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer"], Union[int, None]]) -> None: @@ -82,7 +82,7 @@ def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer name_bytes = name.encode(defenc) else: name_bytes = name # type: ignore[unreachable] # check runtime types - is always str? - _validate_tree_entry_name(safe_decode(name_bytes)) + _validate_tree_entry_name(safe_decode(name_bytes), mode) write(b"".join((mode_str, b" ", name_bytes, b"\0", binsha))) # END for each item @@ -112,7 +112,7 @@ def tree_entries_from_data(data: bytes) -> List[EntryTup]: if name_end < 0 or name_end + 21 > len(data): raise ValueError("Truncated tree entry") name = safe_decode(bytes(data[mode_end + 1 : name_end])) - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) offset = name_end + 21 out.append((bytes(data[name_end + 1 : offset]), mode, name)) return out 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/git/objects/tree.py b/git/objects/tree.py index d2a5859df..96e263530 100644 --- a/git/objects/tree.py +++ b/git/objects/tree.py @@ -110,7 +110,7 @@ def add(self, sha: bytes, mode: int, name: str, force: bool = False) -> "TreeMod :return: self """ - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) if (mode >> 12) not in Tree._map_id_to_type: raise ValueError("Invalid object type according to mode %o" % mode) 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/git/remote.py b/git/remote.py index 47708b093..9af8a6c8b 100644 --- a/git/remote.py +++ b/git/remote.py @@ -988,8 +988,9 @@ def stdout_handler(line: str) -> None: # even if there is an output). if not output: raise - elif stderr_text: - _logger.warning("Error lines received while fetching: %s", stderr_text) + elif stderr_text or proc._timeout_error: + if stderr_text: + _logger.warning("Error lines received while pushing: %s", stderr_text) output.error = e return output diff --git a/git/util.py b/git/util.py index 6f1f38401..61ddab861 100644 --- a/git/util.py +++ b/git/util.py @@ -29,63 +29,62 @@ if sys.platform == "win32": __all__.append("to_native_path_windows") -from abc import abstractmethod import contextlib -from functools import wraps import getpass import logging import ntpath import os import os.path as osp -from pathlib import Path import platform import re import shutil import stat import subprocess import time -from urllib.parse import urlsplit, urlunsplit import warnings - -# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of -# these be used indirectly through other GitPython modules, which avoids having to write -# gitdb all the time in their imports. They are not in __all__, at least currently, -# because they could be removed or changed at any time, and so should not be considered -# conceptually public to code outside GitPython. Linters of course do not like it. -from gitdb.util import ( - LazyMixin, # noqa: F401 - LockedFD, # noqa: F401 - bin_to_hex, # noqa: F401 - file_contents_ro, # noqa: F401 - file_contents_ro_filepath, # noqa: F401 - hex_to_bin, # noqa: F401 - make_sha, - to_bin_sha, # noqa: F401 - to_hex_sha, # noqa: F401 -) +from abc import abstractmethod +from functools import wraps +from pathlib import Path # typing --------------------------------------------------------- - from typing import ( + IO, + TYPE_CHECKING, Any, AnyStr, Callable, Dict, Generator, - IO, Iterator, List, Optional, Pattern, Sequence, Tuple, - TYPE_CHECKING, Type, TypeVar, Union, cast, overload, ) +from urllib.parse import urlsplit, urlunsplit + +# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of +# these be used indirectly through other GitPython modules, which avoids having to write +# gitdb all the time in their imports. They are not in __all__, at least currently, +# because they could be removed or changed at any time, and so should not be considered +# conceptually public to code outside GitPython. Linters of course do not like it. +from gitdb.util import ( + LazyMixin, # noqa: F401 + LockedFD, # noqa: F401 + bin_to_hex, # noqa: F401 + file_contents_ro, # noqa: F401 + file_contents_ro_filepath, # noqa: F401 + hex_to_bin, # noqa: F401 + make_sha, + to_bin_sha, # noqa: F401 + to_hex_sha, # noqa: F401 +) if TYPE_CHECKING: from git.cmd import Git @@ -94,9 +93,9 @@ from git.repo.base import Repo from git.types import ( + HSH_TD, Files_TD, Has_id_attribute, - HSH_TD, Literal, PathLike, Protocol, @@ -385,12 +384,27 @@ def _to_relative_path(root: PathLike, path: PathLike) -> str: "", "", "\u200c\u200d\u200e\u200f\u202a\u202b\u202c\u202d\u202e\u206a\u206b\u206c\u206d\u206e\u206f\ufeff" ) +# Match Git's is_ntfs_dotgitmodules in path.c on a lowercased, trimmed name. +# Besides gitmod~1..4, fallback aliases have exactly eight ASCII characters: +# a shrinking prefix of "gi7eba", "~", and digits with no leading zero. +# Explicit digit counts avoid accepting shorter/longer names or Unicode digits. +_NTFS_DOTGITMODULES_SHORT_NAME = re.compile( + r"(?:gitmod~[1-4]|gi7eba~[1-9]|gi7eb~[1-9][0-9]|gi7e~[1-9][0-9]{2}|" + r"gi7~[1-9][0-9]{3}|gi~[1-9][0-9]{4}|g~[1-9][0-9]{5}|~[1-9][0-9]{6})" +) + -def _validate_repo_path(path: PathLike) -> None: +def _validate_repo_path(path: PathLike, mode: Union[int, None] = None) -> None: """Reject unsafe tree/index paths without normalizing away their components. Protect Git metadata aliases on NTFS and HFS even when writing on another platform. Other POSIX filename characters, including newlines, remain valid. + + :param mode: + Mode of the index or tree entry the path belongs to, where one is known. + Git refuses a symbolic link that aliases ``.gitmodules``, since the + submodule configuration would then be read through the link, so that + name is only rejected once the mode says the entry is a link. """ name = os.fspath(path) if not name or "\0" in name or ntpath.splitdrive(name)[0] or name.startswith("/"): @@ -406,6 +420,13 @@ def _validate_repo_path(path: PathLike) -> None: hfs_name = part.translate(_HFS_IGNORABLES).lower() if ntfs_name in (".git", "git~1") or hfs_name == ".git": raise ValueError("Repository path aliases Git metadata: %r" % name) + if mode is not None and stat.S_ISLNK(mode): + if ( + ntfs_name == ".gitmodules" + or hfs_name == ".gitmodules" + or _NTFS_DOTGITMODULES_SHORT_NAME.fullmatch(ntfs_name) is not None + ): + raise ValueError("Symbolic link aliases the submodule configuration: %r" % name) def assure_directory_exists(path: PathLike, is_file: bool = False) -> bool: @@ -658,10 +679,11 @@ 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, "*****")) + # 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. @@ -1166,15 +1188,56 @@ 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. Checking for it first would allow another + # holder to create it between the check and the open. + try: + 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" % (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 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 = [ diff --git a/test/test_autointerrupt.py b/test/test_autointerrupt.py index 645ec402c..fbc55771b 100644 --- a/test/test_autointerrupt.py +++ b/test/test_autointerrupt.py @@ -1,3 +1,9 @@ +from subprocess import PIPE +import sys +import threading + +import pytest + from git.cmd import Git @@ -31,3 +37,36 @@ def test_autointerrupt_terminate_ignores_attributeerror(): # Ensure the reference is cleared to avoid repeated attempts. assert ai.proc is None + + +def test_autointerrupt_terminate_closes_buffered_stdin(): + ai = Git().hash_object("--stdin", as_process=True, istream=PIPE) + proc = ai.proc + proc.stdin.write(b"buffered input") + try: + ai._terminate() + assert proc.stdin.closed + assert proc.stdout.closed + assert proc.stderr.closed + finally: + proc.stdout.close() + proc.stderr.close() + + +@pytest.mark.skipif(sys.platform == "win32", reason="requires POSIX SIGTERM handling") +def test_autointerrupt_terminate_closes_pipes_before_waiting(): + script = ( + "import signal, sys; signal.signal(signal.SIGTERM, signal.SIG_IGN); " + "print('ready', flush=True); sys.stdin.read(); sys.stdout.buffer.write(b'x' * 1000000)" + ) + ai = Git().execute([sys.executable, "-c", script], as_process=True, istream=PIPE) + proc = ai.proc + assert proc.stdout.readline() == b"ready\n" + watchdog = threading.Timer(5, proc.kill) + watchdog.start() + try: + ai._terminate() + assert proc.returncode != -9, "cleanup waited for exit with undrained output pipes" + finally: + watchdog.cancel() + watchdog.join() diff --git a/test/test_commit.py b/test/test_commit.py index b5baf0e33..08ba4a634 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -424,6 +424,49 @@ def test_invalid_commit(self): self.assertEqual(cmt.author.name, "E.Azer Ko�o�o�oculu", cmt.author.name) self.assertEqual(cmt.author.email, "azer@kodfabrik.com", cmt.author.email) + @with_rw_directory + def test_identity_cannot_alter_headers(self, rw_dir): + """A name or email must not add header lines or present another identity.""" + rw_repo = Repo.init(osp.join(rw_dir, "test_identity_headers")) + path = osp.join(str(rw_repo.working_tree_dir), "hello.txt") + touch(path) + rw_repo.index.add([path]) + tree = rw_repo.index.write_tree() + service = Actor("Service", "service@example.com") + forged = "committer Forged 0 +0000" + + for name, email in ( + # A line feed ends the header line, so the remainder would become headers + # of its own, which Git reads before the committer written after them. + ("User 0 +0000\n" + forged, "user@example.com"), + ("User", "user@example.com> 0 +0000\n" + forged), + # Angle brackets delimit the email, so these would present another one. + ("Forged ", "user@example.com"), + ("User", "forged@example.com> ", "user@example.com"), + ("User", "10L20sH", 0, 0, 0, 0, 0, 0, 0o100644, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name + entry = struct.pack(">10L20sH", 0, 0, 0, 0, 0, 0, mode, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name entry += b"\0" * (8 - len(entry) % 8) data = b"DIRC" + struct.pack(">LL", 2, 1) + entry return data + sha1(data).digest() @@ -340,6 +340,68 @@ def test_index_reader_and_writer_reject_unsafe_paths(self, path): with pytest.raises(ValueError): write_cache([entry], BytesIO()) + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules.", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~4", + "gi7eba~1", + "gi7eba~9", + "GI7EB~10", + "GI7EB~99", + "GI7E~100", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "GI7EB~10. ", + "GI7E~100:$DATA", + "sub/~1000000", + "sub/.gitmodules", + ) + def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path): + """An entry that turns .gitmodules into a symbolic link is rejected, while the + same name stays valid for a regular file. The spellings are those + `git update-index --add --cacheinfo 120000,,` refuses on git 2.52.0.""" + with pytest.raises(ValueError): + read_cache(BytesIO(_raw_index(path, mode=0o120000))) + with pytest.raises(ValueError): + write_cache([IndexEntry((0o120000, b"a" * 20, 0, path))], BytesIO()) + + assert next(iter(read_cache(BytesIO(_raw_index(path)))[1])) == (path, 0) + stream = BytesIO() + write_cache([IndexEntry((0o100644, b"a" * 20, 0, path))], stream) + stream.seek(0) + assert next(iter(read_cache(stream)[1])) == (path, 0) + + @ddt.data("GI7EB~10", "GI7E~100", "GI7~1000", "GI~10000", "G~100000", "~1000000", "~9999999") + @with_rw_directory + def test_index_add_and_write_tree_reject_gitmodules_fallback_symlinks(self, rw_dir, path): + with Repo.init(rw_dir) as repo: + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + index = repo.index + for item in (Blob(repo, binsha, 0o120000, path), BaseIndexEntry((0o120000, binsha, 0, path))): + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], write=False) + assert not index.entries + + index.entries[(path, 0)] = IndexEntry((0o120000, binsha, 0, path)) + with pytest.raises(ValueError, match="submodule configuration"): + index.write_tree() + assert not Path(index.path).exists() + + index.add([BaseIndexEntry((0o100644, binsha, 0, path))]) + assert repo.index.entries[(path, 0)].mode == 0o100644 + assert index.write_tree()[path].mode == 0o100644 + def test_valid_unusual_index_names_round_trip(self): names = ["a b", "a\nb", "a\tb", "name:value", "dir/.gitignore", "café"] if os.name != "nt": @@ -381,20 +443,26 @@ def _cmp_tree_index(self, tree, index): @with_rw_repo("0.1.6") def test_index_lock_handling(self, rw_repo): - def add_bad_blob(): - rw_repo.index.add([Blob(rw_repo, b"f" * 20, "bad-permissions", "foo")]) - - try: - ## First, fail on purpose adding into index. - add_bad_blob() - except Exception as ex: - assert "required argument is not an integer" in str(ex) - - ## The second time should not fail due to stray lock file. - try: - add_bad_blob() - except Exception as ex: - assert "index.lock' could not be obtained" not in str(ex) + index = rw_repo.index + index_path = Path(index.path) + lock_path = Path(str(index_path) + ".lock") + before = index_path.read_bytes() + + def fail_serialize(stream, ignore_extension_data): + assert lock_path.exists() + stream.write(b"partial index") + raise OSError("simulated index write failure") + + with mock.patch.object(IndexFile, "_serialize", side_effect=fail_serialize): + for _ in range(2): + with pytest.raises(OSError, match="simulated index write failure"): + index.write() + assert not lock_path.exists() + assert index_path.read_bytes() == before + + index.add([Blob(rw_repo, b"f" * 20, 0o100644, "foo")]) + assert not lock_path.exists() + assert rw_repo.index.entries[("foo", 0)].mode == 0o100644 @with_rw_repo("0.1.6") def test_read_tree_methods_reject_index_output(self, rw_repo): @@ -1221,6 +1289,60 @@ def test_staging_rejects_unsafe_object_paths_even_without_writing(self, rw_dir, index.add([item], write=False, **kwargs) assert not index.entries + @ddt.data(*product(("path", "blob", "entry", "stored-blob", "stored-entry"), (False, True), (False, True))) + @ddt.unpack + @with_rw_directory + def test_staging_gitmodules_symlink_check_uses_rewritten_path(self, rw_dir, kind, unsafe_destination, write): + with Repo.init(rw_dir) as repo: + source, destination = ("safe-link", ".gitmodules") if unsafe_destination else (".gitmodules", "safe-link") + binsha = Blob.NULL_BIN_SHA + if kind.startswith("stored-"): + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + else: + try: + (Path(rw_dir) / source).symlink_to("target") + except OSError: + pytest.skip("Symlinks unavailable") + if kind == "path": + item = source + elif kind.endswith("blob"): + item = Blob(repo, binsha, 0o120000, source) + else: + item = BaseIndexEntry((0o120000, binsha, 0, source)) + index = repo.index + rewriter = mock.Mock(return_value=destination) + if unsafe_destination: + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], path_rewriter=rewriter, write=write) + assert not index.entries + assert not Path(index.path).exists() + else: + added = index.add([item], path_rewriter=rewriter, write=write) + assert [(entry.path, entry.mode) for entry in added] == [(destination, 0o120000)] + assert set(index.entries) == {(destination, 0)} + assert index.write_tree()[destination].mode == 0o120000 + if write: + assert repo.index.entries[(destination, 0)].mode == 0o120000 + rewriter.assert_called_once() + assert rewriter.call_args[0][0].path == source + + @ddt.data(*product(("blob", "entry"), ("../outside", ".git/config"))) + @ddt.unpack + @with_rw_directory + def test_staging_rewriter_cannot_sanitize_unsafe_source_paths(self, rw_dir, kind, path): + with Repo.init(rw_dir) as repo: + item = ( + Blob(repo, b"a" * 20, 0o120000, path) + if kind == "blob" + else BaseIndexEntry((0o120000, b"a" * 20, 0, path)) + ) + index = repo.index + rewriter = mock.Mock(return_value="safe-link") + with pytest.raises(ValueError): + index.add([item], path_rewriter=rewriter, write=False) + rewriter.assert_not_called() + assert not index.entries + @with_rw_directory def test_staging_root_preserves_symlinks_and_skips_git_metadata(self, rw_dir): tmp_path = Path(rw_dir) @@ -1731,6 +1853,53 @@ 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 + + @pytest.mark.skipif(os.name == "nt", reason="Line feeds and quotes are not valid Windows filenames") + def test_checkout_sends_each_path_as_one_record(self, tmp_path): + with Repo.init(tmp_path) as repo: + nested = tmp_path / "nested" + nested.mkdir() + (nested / "first\noutside").write_bytes(b"nested") + (tmp_path / "outside").write_bytes(b"committed") + (tmp_path / '"quoted"').write_bytes(b"quoted") + repo.index.add(["nested", "outside", '"quoted"']) + + (nested / "first\noutside").unlink() + (tmp_path / '"quoted"').unlink() + (tmp_path / "outside").write_bytes(b"local") + + checked_out = {"nested/first\noutside", '"quoted"'} + assert set(repo.index.checkout(["nested", '"quoted"'], force=True)) == checked_out + assert (nested / "first\noutside").read_bytes() == b"nested" + assert (tmp_path / '"quoted"').read_bytes() == b"quoted" + # Neither "nested/first" nor "outside" was requested. + assert (tmp_path / "outside").read_bytes() == b"local" + + class TestIndexUtils: @pytest.mark.parametrize("file_path_type", [str, Path]) def test_temporary_file_swap(self, tmp_path, file_path_type): diff --git a/test/test_remote.py b/test/test_remote.py index 0212ffab8..afa15fa2d 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -8,8 +8,11 @@ import os.path as osp from pathlib import Path import random +import socket import sys import tempfile +import threading +import time from unittest import mock, skipIf import pytest @@ -24,6 +27,7 @@ Remote, RemoteProgress, RemoteReference, + Repo, SymbolicReference, TagReference, ) @@ -1087,7 +1091,67 @@ def test_fetch_unsafe_branch_name(self, rw_repo, remote_repo): Head.delete(remote_repo, bad_branch_name) +@pytest.mark.skipif(sys.platform == "win32", reason="kill_after_timeout is not supported on Windows") +@pytest.mark.parametrize("protocol", ["git", "http", "https"]) +@pytest.mark.parametrize("operation", ["fetch", "pull", "push"]) +def test_timeout_stalled_remote(tmp_path, protocol, operation): + repo = Repo.init(tmp_path) + repo.git.symbolic_ref("HEAD", "refs/heads/main") + with repo.config_writer() as config: + config.set_value("user", "name", "Timeout Test") + config.set_value("user", "email", "timeout@example.com") + config.set_value("commit", "gpgSign", False) + repo.git.commit(allow_empty=True, message="initial commit") + + with socket.socket() as server: + server.bind(("127.0.0.1", 0)) + server.listen() + server.settimeout(10) + finished = threading.Event() + connected = threading.Event() + + def stall(): + with server.accept()[0]: + connected.set() + # Release the connection even if the timeout cleanup deadlocks. + finished.wait(5) + + thread = threading.Thread(target=stall) + thread.start() + remote = repo.create_remote("origin", f"{protocol}://127.0.0.1:{server.getsockname()[1]}/stalled.git") + started = time.monotonic() + try: + with pytest.raises(GitCommandError, match="process killed because it timed out"): + getattr(remote, operation)("main", kill_after_timeout=0.5) + assert connected.is_set(), "the command did not reach the stalled remote" + assert time.monotonic() - started < 3, "timeout cleanup blocked on a pipe reader" + finally: + finished.set() + thread.join(10) + repo.close() + assert not thread.is_alive() + + class TestTimeouts(TestBase): + @skipIf(sys.platform == "win32", "requires POSIX timeout signalling") + @with_rw_repo("HEAD", bare=False) + def test_timeout_partial_push(self, repo): + process = repo.git.execute( + [ + sys.executable, + "-c", + "import time; print('=\\trefs/heads/main:refs/heads/main\\t[up to date]', flush=True); time.sleep(30)", + ], + as_process=True, + universal_newlines=True, + ) + with mock.patch("git.remote._logger.warning") as warning: + result = repo.remote("origin")._get_push_info(process, None, kill_after_timeout=0.5) + warning.assert_not_called() + assert len(result) == 1 + with pytest.raises(GitCommandError, match="process killed because it timed out"): + result.raise_if_error() + @with_rw_repo("HEAD", bare=False) def test_timeout_funcs(self, repo): # Maintenance may outlive a timed-out fetch and race with fixture cleanup. 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", diff --git a/test/test_tree.py b/test/test_tree.py index 803f1f381..3a85405bd 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -138,6 +138,113 @@ def test_tree_names_are_checked_at_construction_and_serialization(self, name): with pytest.raises(ValueError): tree_to_stream([(b"a" * 20, 0o100644, name)], BytesIO().write) + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~2", + "gitmod~3", + "GITMOD~4", + "gi7eba~1", + "GI7EBA~9", + "GI7EB~10", + "GI7EB~11", + "GI7EB~99", + "GI7E~100", + "GI7E~101", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "Gi7Eb~42", + "gi7e~120", + "GITMOD~4 . ", + "GI7EB~10. ", + "GI7E~100:$DATA", + "~1000000 . :$DATA", + ) + def test_gitmodules_symlink_entries_are_rejected(self, name): + """A symbolic link named like the submodule configuration would make Git read + it from outside the repository, so such an entry is refused in both + directions. A regular file with the same name is the normal case.""" + symlink_mode = 0o120000 + cache = [] + with pytest.raises(ValueError): + TreeModifier(cache).add(b"a" * 20, symlink_mode, name) + assert not cache + with pytest.raises(ValueError): + tree_to_stream([(b"a" * 20, symlink_mode, name)], BytesIO().write) + raw = b"120000 " + name.encode() + b"\0" + b"a" * 20 + with pytest.raises(ValueError): + tree_entries_from_data(raw) + + TreeModifier(cache).add(b"a" * 20, 0o100644, name) + assert cache == [(b"a" * 20, 0o100644, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + assert tree_entries_from_data(data.getvalue()) == cache + + @ddt.data( + "gitmod~0", + "gitmod~5", + "gitmod~10", + "GI7EBA~", + "GI7EBA~0", + "GI7EBA~~1", + "GI7EBA~X", + "GI7EBA~10", + "Gx7EBA~1", + "GI7EBX~1", + "GI7EB~1", + "GI7EB~01", + "GI7EB~1X", + "GI7EB~100", + "GI7E~10", + "GI7E~010", + "GI7E~1000", + "GI7~100", + "GI7~0100", + "GI7~10000", + "GI~1000", + "GI~01000", + "GI~100000", + "G~10000", + "G~010000", + "G~1000000", + "~100000", + "~0100000", + "~10000000", + "GI7EBA~\u0661", + "GI7EB~1\uff10", + "GI7EB~10x", + "GI7EB~10.x", + " GI7EB~10", + "GI7EB~10\n", + "GI7EB~10\t", + "GI7EB~10x:$DATA", + ".gitmodules x", + ".gitmodules .x", + ".gitmodules,:$DATA", + ) + def test_gitmodules_short_name_near_misses_round_trip(self, name): + """Only exact aliases are forbidden, and only for symbolic links.""" + for mode in (0o100644, 0o120000): + cache = [] + TreeModifier(cache).add(b"a" * 20, mode, name) + assert cache == [(b"a" * 20, mode, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + raw = ("%o " % mode).encode() + name.encode() + b"\0" + b"a" * 20 + assert data.getvalue() == raw + assert tree_entries_from_data(raw) == cache + def test_traverse(self): root = self.rorepo.tree("0.1.6") num_recursive = 0 diff --git a/test/test_util.py b/test/test_util.py index c6e68cf51..ca8e006f3 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 @@ -22,6 +23,7 @@ from git.objects.util import ( altz_to_utctz_str, from_timestamp, + mode_str_to_int, parse_actor_and_date, parse_date, tzoffset, @@ -417,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. @@ -428,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) @@ -436,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) @@ -445,6 +451,67 @@ 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, 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() + 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: + 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() + + 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: my_file = os.path.join(tdir, "my-lock-file") @@ -760,3 +827,38 @@ 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_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"] + + @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 + assert mode_str_to_int(b"100644") == 0o100644 + assert mode_str_to_int("644") == 0o644 + assert mode_str_to_int(b"120000") == 0o120000