From a4a43acae2915b8f1af66822f439cf111c3c3c4e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 9 Nov 2026 23:48:46 -0500 Subject: [PATCH 1/2] fix(git): address GHSA-6hqx-gqrx-c5r2 and GHSA-q74p-4wrc-q7w6 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/git/README.md | 17 +- src/git/src/mcp_server_git/server.py | 255 +++++++++++-- src/git/tests/test_merge.py | 33 +- src/git/tests/test_protocol.py | 152 +++++++- src/git/tests/test_repository_config.py | 455 ++++++++++++++++++++++++ 5 files changed, 863 insertions(+), 49 deletions(-) create mode 100644 src/git/tests/test_repository_config.py diff --git a/src/git/README.md b/src/git/README.md index 9bb7688503..54499dd378 100644 --- a/src/git/README.md +++ b/src/git/README.md @@ -173,7 +173,22 @@ python -m mcp_server_git `--repository` (`-r`) accepts any path inside a Git working tree. At startup the server walks up to the enclosing repository root, the way `git rev-parse --show-toplevel` does, so `--repository .` works when a shared config is launched from any subdirectory of the repository. A path inside the repository therefore allows tool calls anywhere in that repository, not only under the subdirectory given. A path that does not exist, or is not inside a Git repository, is logged as a one-line error at startup. -A tool call's `repo_path` must be inside that repository, and so must the directories Git then uses for it: a repository nested inside whose `.git` file, `commondir` or `core.worktree` points outside is refused. A linked worktree can still be the `--repository`, though its Git directory lives under the main repository's `.git`. +A tool call's `repo_path` must be inside that repository, and so must the directories Git then uses for it: a repository nested inside whose `.git` file, `commondir` or `core.worktree` points outside is refused. A linked worktree can still be the `--repository`, though its Git directory lives under the main repository's `.git`; only the worktree itself may use that directory, so a repository nested in the worktree whose `.git` names it, or the main repository's `.git`, is refused. With `--repository` set, the server also refuses a repository whose Git directory: + +- names alternate object stores (`objects/info/alternates` or `objects/info/http-alternates` with any entry), wherever they are. A clone made with `--reference` or `--shared` therefore cannot be used under `--repository`; `git repack -a -d` and then deleting `objects/info/alternates` makes it self-contained. +- holds a symbolic link (at `objects`, `refs`, `index`, `HEAD` or anywhere else in it) that resolves outside both the allowed repository and that Git directory. A `hooks` directory linked elsewhere is allowed, since no hook runs. + +### A repository's own programs + +A repository's configuration and hooks travel with it (an unpacked archive, a shared folder, a vendored tree), so the server never runs a program they name, with or without `--repository`. Every Git command it runs overrides, through `GIT_CONFIG_COUNT`, the settings that would start one: + +- `core.fsmonitor` is `false`, and `core.hooksPath` points at the null device, so no hook runs; `git_commit` also skips the hooks GitPython would run itself (`pre-commit`, `commit-msg`, `post-commit`). +- Diffs pass `--no-ext-diff` and `--no-textconv`, so `diff.external`, `diff..command` and `diff..textconv` are ignored. +- Each filter driver (`filter..clean`, `smudge`, `process`) and merge driver (`merge..driver`) that the repository's own configuration defines is emptied: files are staged and checked out unfiltered, and a file a merge driver would have merged is a conflict, which `git_merge` aborts. Drivers from your global or system configuration, such as Git LFS's, keep working. +- `commit.gpgSign` and `merge.verifySignatures` are `false`, `submodule.recurse` and `merge.autoStash` are `false`, and `core.pager` is `cat`. +- `git_status`, `git_diff_unstaged` and `git_diff` pass `--ignore-submodules=dirty`, because looking for changes inside a submodule runs Git under the submodule's own configuration. A submodule whose checked-out commit moved is still reported; changes inside one are not, so run the tool on the submodule's path to see them. + +This needs Git 2.31 or later; with an older Git every tool call fails with an error saying so, rather than run unprotected. ### Commit identity diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index f12a60ce55..d93e96e5e4 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -1,9 +1,10 @@ # The git MCP server: tools that read and change a Git repository through # GitPython, and prompts that embed a repository's changes in a message for the # model (#5195). Repository confinement (`--repository`, `validate_repo_path` on -# the path given, `validate_opened_repo` on the directories git then uses) and -# the flag-injection guards live beside the handlers because every tool call and -# prompt must pass them before any git command runs. +# the path given, `validate_opened_repo` on the directories git then uses), the +# overrides that keep a repository's own configuration from running programs +# (`harden_repo`), and the flag-injection guards live beside the handlers +# because every tool call and prompt must pass them before any git command runs. # # Built on the MCP Python SDK's high-level `MCPServer` (#5174), the API these # reference servers are meant to teach: each tool is a typed function, and the @@ -19,6 +20,7 @@ # logging), so nothing differs between them. import logging +import os import re import threading from collections.abc import Callable, Iterator @@ -107,18 +109,135 @@ def _git_text(repo: git.Repo, command: str, *args: str) -> str: return output.decode("utf-8", errors="replace") +# Configuration that names a program git would run, overridden for every git +# command the server runs in a repository it opens (`harden_repo`). A +# repository's own `.git/config` is not trusted: it travels with the +# repository (an unpacked archive, a shared folder, a vendored tree), so +# honoring it would let the repository run code as the server's user on a +# read-only call such as git_status. The overrides go in through +# GIT_CONFIG_COUNT (git 2.31+), which outranks every config file and is +# inherited by the git processes git itself starts, submodules included. +# +# - `core.fsmonitor` is run on every index refresh (status, diff, add). +# - `core.hooksPath` pointing at the null device leaves no hook to find, so +# no hook runs (post-checkout, pre-merge-commit, reference-transaction, ...). +# GitPython's `index.commit` runs hooks itself; git_commit skips them. +# - `core.pager` is never started without a terminal; pinned all the same. +# - `commit.gpgSign` and `merge.verifySignatures` would run `gpg.program` on +# a merge commit or a merge. +# - `submodule.recurse` would carry checkout and merge into submodules, whose +# own configuration this override cannot neutralize (see `_LISTED_KEYS`). +# - `merge.autoStash` keeps git_merge from stashing behind the tool's back. +# +# External diff programs and textconv filters are turned off per command +# (`_DIFF_FLAGS`), and filter and merge drivers per repository (`_LISTED_KEYS`). +HARDENED_CONFIG: tuple[tuple[str, str], ...] = ( + ("core.fsmonitor", "false"), + ("core.hooksPath", os.devnull), + ("core.pager", "cat"), + ("commit.gpgSign", "false"), + ("merge.verifySignatures", "false"), + ("submodule.recurse", "false"), + ("status.submoduleSummary", "false"), + ("merge.autoStash", "false"), +) + +# A filter driver (`filter..clean`/`smudge`/`process`, applied by +# `.gitattributes` on add, checkout and status) or merge driver +# (`merge..driver`) is a command too, under a name the repository +# chooses, so each one the repository's own configuration defines is +# overridden with an empty command, which git skips (a filter) or reports as +# a conflict (a merge driver). Drivers from the user's global or system +# configuration (git-lfs, say) are the user's own and keep working. +# `harden_repo` lists these keys, and `core.hooksPath`, whose command-scope +# value shows that git applied the overrides. +_LISTED_KEYS = ( + r"^(core\.hookspath|filter\..+\.(clean|smudge|process)|merge\..+\.driver)$" +) + +# Scopes whose configuration the user wrote rather than the repository. +_TRUSTED_SCOPES = frozenset({"system", "global", "command"}) + +# For every diff whose output is the patch: `--no-ext-diff` turns off +# `diff.external` and `diff..command`, `--no-textconv` the +# `diff..textconv` filters git diff applies by default. +_DIFF_FLAGS = ("--no-ext-diff", "--no-textconv") + +# A command that compares the working tree looks inside each submodule by +# running `git status` there, under the submodule's own configuration, whose +# filter drivers `harden_repo` never sees. `dirty` still reports a submodule +# whose checked-out commit moved, without that look inside. The flag outranks +# `submodule..ignore`, which the repository could set to `none`. +_NO_SUBMODULE_STATUS = "--ignore-submodules=dirty" + + +def _config_env(entries: list[tuple[str, str]]) -> dict[str, str]: + """GIT_CONFIG_COUNT variables for `entries`, after any the user set.""" + base = int(os.environ.get("GIT_CONFIG_COUNT", "0")) + env = {"GIT_CONFIG_COUNT": str(base + len(entries))} + for i, (key, value) in enumerate(entries, start=base): + env[f"GIT_CONFIG_KEY_{i}"] = key + env[f"GIT_CONFIG_VALUE_{i}"] = value + return env + + +def harden_repo(repo: git.Repo) -> None: + """Keep every git command run for `repo` from running a program it names. + + Sets `HARDENED_CONFIG` on the repository's git, then reads which filter + and merge drivers the repository's own configuration (its `.git/config`, + a worktree's `config.worktree`, and files they include) defines, and + overrides each with an empty command. The same read checks that git + applied the overrides: git older than 2.31 ignores GIT_CONFIG_COUNT, and + is refused rather than run unprotected. + """ + entries = list(HARDENED_CONFIG) + repo.git.update_environment(**_config_env(entries)) + try: + listed = repo.git.config("--show-scope", "-z", "--get-regexp", _LISTED_KEYS) + except git.GitCommandError as e: + # Exit 1 is "nothing matched", which a git that applied the + # overrides never reports, since core.hooksPath always matches. + if e.status != 1: + raise + listed = "" + applied = False + drivers: set[str] = set() + # -z prints `\0\n\0` for each entry. + fields = listed.split("\0") + for scope, entry in zip(fields[0::2], fields[1::2]): + key, _, value = entry.partition("\n") + if key == "core.hookspath": + applied = applied or (scope == "command" and value == os.devnull) + elif scope not in _TRUSTED_SCOPES: + drivers.add(key) + if not applied: + raise git.GitError( + "Git 2.31 or later is required: this git ignores the configuration " + "overrides that keep a repository's own configuration from running " + "programs" + ) + if drivers: + entries += [(key, "") for key in sorted(drivers)] + repo.git.update_environment(**_config_env(entries)) + + def git_status(repo: git.Repo) -> str: - return _git_text(repo, "status") + return _git_text(repo, "status", _NO_SUBMODULE_STATUS) def git_diff_unstaged( repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES ) -> str: - return _git_text(repo, "diff", f"--unified={context_lines}") + return _git_text( + repo, "diff", *_DIFF_FLAGS, _NO_SUBMODULE_STATUS, f"--unified={context_lines}" + ) def git_diff_staged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES) -> str: - return _git_text(repo, "diff", f"--unified={context_lines}", "--cached") + return _git_text( + repo, "diff", *_DIFF_FLAGS, f"--unified={context_lines}", "--cached" + ) def _validate_diff_target(repo: git.Repo, target: str) -> None: @@ -161,7 +280,14 @@ def git_diff( raise FlagInjectionError(f"Invalid target: '{target}' - cannot start with '-'") # Validates target is a real revision or range, throws BadName if not _validate_diff_target(repo, target) - return _git_text(repo, "diff", f"--unified={context_lines}", target) + return _git_text( + repo, + "diff", + *_DIFF_FLAGS, + _NO_SUBMODULE_STATUS, + f"--unified={context_lines}", + target, + ) def _has_staged_changes(repo: git.Repo) -> bool: @@ -196,12 +322,14 @@ def git_commit(repo: git.Repo, message: str) -> str: parents = [repo.head.commit] + [ repo.commit(sha) for sha in merge_head.read_text().split() ] - commit = repo.index.commit(message, parent_commits=parents) + commit = repo.index.commit(message, parent_commits=parents, skip_hooks=True) for name in ("MERGE_HEAD", "MERGE_MSG", "MERGE_MODE", "AUTO_MERGE"): (git_dir / name).unlink(missing_ok=True) autostash_note = _apply_merge_autostash(repo) else: - commit = repo.index.commit(message) + # skip_hooks: GitPython runs the repository's hooks itself, ignoring + # the core.hooksPath override (`harden_repo`). + commit = repo.index.commit(message, skip_hooks=True) autostash_note = "" return f"Changes committed successfully with hash {commit.hexsha}{autostash_note}" @@ -410,7 +538,9 @@ def _merge_failure(repo: git.Repo, error: git.GitCommandError, what: str) -> str return f"{what} failed. {unchanged} Git reported:\n{_git_stderr(error)}" # Through _git_text, so a non-ASCII name prints as written, not as # core.quotepath's octal escapes. - conflicts = _git_text(repo, "diff", "--name-only", "--diff-filter=U").splitlines() + conflicts = _git_text( + repo, "diff", _NO_SUBMODULE_STATUS, "--name-only", "--diff-filter=U" + ).splitlines() repo.git.merge("--abort") if conflicts: return ( @@ -449,7 +579,9 @@ def git_merge( raise ValueError( "A merge is already in progress; conclude it with git_commit first" ) - if repo.is_dirty(): + # Not `repo.is_dirty()`, whose `git diff` looks inside submodules + # (_NO_SUBMODULE_STATUS); a merge never touches a submodule's working tree. + if repo.git.status("--porcelain", "--untracked-files=no", _NO_SUBMODULE_STATUS): raise ValueError( f"Cannot merge into '{target}': the working tree has uncommitted " "changes. Commit or discard them first, so a failed merge can be " @@ -589,13 +721,19 @@ def validate_opened_repo( follow a `.git` file (`gitdir:`), a `commondir` file or `core.worktree` to a directory anywhere else, so the working tree, git directory and common directory git actually uses are checked too. The working tree must lie - inside the allowed repository. The two git directories must lie inside it - or be the allowed repository's own: a linked worktree's git directory - lives under the main repository's `.git`, outside the worktree it serves. + inside the allowed repository. The two git directories must lie inside it, + or be the allowed repository's own as a pair, opened from the allowed + repository itself: a linked worktree's git directory lives under the main + repository's `.git`, outside the worktree it serves, and only that + worktree may use it. A repository nested inside a linked worktree whose + `.git` names the main repository's git directory is refused. Both git's answer (`rev-parse`, which the tools' git commands follow) and GitPython's (which some tools read directly) are checked: GitPython does not apply `core.worktree`, so the two can differ. + + Git also reaches past those directories from inside them, so + `_check_git_dir_contents` holds what they contain to the same boundary. """ if allowed_repository is None: return @@ -621,21 +759,79 @@ def outside(used: Path) -> ValueError: if not working_tree.resolve().is_relative_to(allowed): raise outside(working_tree) + git_dirs = [Path(git_dir), Path(repo.git_dir)] + common_dirs = [Path(common_dir), Path(repo.common_dir)] escaped = [ - p - for p in map(Path, (git_dir, common_dir, repo.git_dir, repo.common_dir)) - if not p.resolve().is_relative_to(allowed) + p for p in git_dirs + common_dirs if not p.resolve().is_relative_to(allowed) ] - if not escaped: - return - with git.Repo(allowed) as allowed_repo: - own = { - Path(allowed_repo.git_dir).resolve(), - Path(allowed_repo.common_dir).resolve(), - } - for p in escaped: - if p.resolve() not in own: - raise outside(p) + if escaped: + with git.Repo(allowed) as allowed_repo: + own_git_dir = Path(allowed_repo.git_dir).resolve() + own_common_dir = Path(allowed_repo.common_dir).resolve() + is_own = ( + bool(working_trees) + and all(Path(w).resolve() == allowed for w in working_trees) + and all(p.resolve() == own_git_dir for p in git_dirs) + and all(p.resolve() == own_common_dir for p in common_dirs) + ) + if not is_own: + raise outside(escaped[0]) + + for directory in {p.resolve() for p in git_dirs + common_dirs}: + _check_git_dir_contents(repo_path, directory, allowed, outside) + + +# Files in a git directory that name more object stores for git to read: an +# alternate's objects are read as the repository's own, from wherever it is. +_ALTERNATES = ("alternates", "http-alternates") + + +def _check_git_dir_contents( + repo_path: Path, + directory: Path, + allowed: Path, + outside: Callable[[Path], ValueError], +) -> None: + """Refuse what in a git directory leads git outside the allowed repository. + + - An alternate object store (`objects/info/alternates`, and + `http-alternates`), of any kind: under `--repository`, a repository + reads only its own objects. + - A symbolic link that resolves outside both the allowed repository and + `directory` itself: a link at `objects`, `refs`, `index`, `HEAD` or + anything else in a git directory is followed by git, for reading and + for writing (a linked `index` takes git_add's writes elsewhere). + + `hooks` is not walked: no hook runs (`HARDENED_CONFIG`), so a hooks + directory linked to a shared one elsewhere is harmless. + """ + for name in _ALTERNATES: + alternates = directory / "objects" / "info" / name + if alternates.is_file(): + lines = alternates.read_text(encoding="utf-8", errors="replace") + if any( + line.strip() and not line.startswith("#") for line in lines.split("\n") + ): + raise ValueError( + f"Repository path '{repo_path}' uses '{alternates}': " + "alternate object stores are not allowed with --repository" + ) + pending = [directory] + while pending: + current = pending.pop() + with os.scandir(current) as entries: + for entry in entries: + if current == directory and entry.name == "hooks": + continue + if entry.is_symlink(): + target = Path(entry.path).resolve() + if not ( + target.is_relative_to(allowed) + or target.is_relative_to(directory) + ): + raise outside(target) + elif entry.is_dir(): + pending.append(Path(entry.path)) def git_branch( @@ -932,7 +1128,7 @@ def summarize_changes_prompt(repo: git.Repo, target: str | None = None) -> str: parts.append(f"Unstaged changes:\n{_fenced(unstaged)}") # The porcelain status is empty only on a clean tree; an untracked file # alone leaves both diffs empty but is still a change to summarize. - clean = not repo.git.status("--porcelain").strip() + clean = not repo.git.status("--porcelain", _NO_SUBMODULE_STATUS).strip() changes = "" if clean else "\n\n".join(parts) if not changes: return f"There are no changes {scope} to summarize. Tell the user so." @@ -994,6 +1190,9 @@ def open_repo(repo_path: str) -> git.Repo: validate_repo_path(path, repository) repo = git.Repo(path) try: + # First, so that no git command run for the repository, the + # confinement checks' included, runs a program it names. + harden_repo(repo) validate_opened_repo(repo, path, repository) except Exception: repo.close() diff --git a/src/git/tests/test_merge.py b/src/git/tests/test_merge.py index 042a57f491..4853b4e1bc 100644 --- a/src/git/tests/test_merge.py +++ b/src/git/tests/test_merge.py @@ -8,8 +8,6 @@ and the repository afterwards. """ -import stat -import sys from pathlib import Path from typing import Any @@ -17,6 +15,8 @@ import pytest from conftest import connect, text_result, tool_error, wire +from mcp_server_git.server import _merge_failure + def root_of(repo: git.Repo) -> Path: assert repo.working_dir is not None @@ -203,28 +203,23 @@ async def test_fetch_head_merges_only_the_commit_it_resolves_to(repo: git.Repo): assert not (root_of(repo) / "second.txt").exists() -@pytest.mark.skipif( - sys.platform == "win32", reason="git for Windows runs hooks through sh" -) -async def test_failed_merge_commit_is_aborted(repo: git.Repo): - # A merge with no conflicts can still fail to commit; here a - # pre-merge-commit hook refuses it, which leaves MERGE_HEAD behind. +def test_failed_merge_commit_is_aborted(repo: git.Repo): + # A merge with no conflicts can still fail to commit (a ref lock held by + # another process, say), which leaves MERGE_HEAD behind. No hook can make + # it fail here, since the server runs none, so the merge is stopped before + # its commit with --no-commit and the failure is handed to _merge_failure. diverge(repo, conflicting=False) head = repo.head.commit.hexsha - hook = Path(repo.git_dir) / "hooks" / "pre-merge-commit" - hook.parent.mkdir(exist_ok=True) - hook.write_text("#!/bin/sh\necho 'merge refused by hook' >&2\nexit 1\n") - hook.chmod(hook.stat().st_mode | stat.S_IEXEC) + repo.git.merge("--no-ff", "--no-commit", "feature") + assert (Path(repo.git_dir) / "MERGE_HEAD").exists() + error = git.GitCommandError(["git", "merge"], 128, "fatal: merge refused") - result = await merge(repo, "feature", fast_forward_only=False) + message = _merge_failure(repo, error, "Merge of 'feature' into 'main'") - assert result["isError"] is True - text = result["content"][0]["text"] - assert text.startswith( - "Error executing tool git_merge: Merge of 'feature' into 'main' failed. " - "The merge was aborted and the repository is unchanged. Git reported:\n" + assert message == ( + "Merge of 'feature' into 'main' failed. The merge was aborted and the " + "repository is unchanged. Git reported:\nfatal: merge refused" ) - assert "merge refused by hook" in text assert_untouched(repo, head) diff --git a/src/git/tests/test_protocol.py b/src/git/tests/test_protocol.py index e3aa0171ad..b5da78d55b 100644 --- a/src/git/tests/test_protocol.py +++ b/src/git/tests/test_protocol.py @@ -24,7 +24,7 @@ import anyio import git import pytest -from conftest import connect, make_repo, text_result, tool_error, wire +from conftest import connect, make_repo, rmtree, text_result, tool_error, wire from mcp.client.session import ClientRequestContext from mcp.shared.exceptions import MCPError from mcp.shared.memory import create_client_server_memory_streams @@ -1403,6 +1403,156 @@ async def test_restricted_server_serves_a_linked_worktree(tmp_path: Path): main.close() +@pytest.mark.parametrize("name", ["alternates", "http-alternates"]) +async def test_restricted_server_rejects_alternate_object_stores( + tmp_path: Path, repo: git.Repo, name: str +): + # `objects/info/alternates` names more object stores git reads as the + # repository's own: a nested repository naming an outside one would show + # that repository's commits and files by id. + root = root_of(repo) + other = make_repo(tmp_path / "other") + secret = other.head.commit.hexsha + nested = make_repo(root / "nested") + alternates = Path(nested.git_dir) / "objects" / "info" / name + alternates.parent.mkdir(parents=True, exist_ok=True) + alternates.write_text(f"# a comment\n\n{Path(other.git_dir) / 'objects'}\n") + nested.close() + try: + shown = await call( + root, "git_show", {"repo_path": str(root / "nested"), "revision": secret} + ) + assert shown["isError"] is True + text = shown["content"][0]["text"] + assert f"uses '{alternates}'" in text + assert "alternate object stores are not allowed with --repository" in text + assert "line 1" not in text + finally: + other.close() + + +async def test_restricted_server_accepts_empty_alternates(repo: git.Repo): + # An alternates file holding only comments and blank lines names nothing. + root = root_of(repo) + alternates = Path(repo.git_dir) / "objects" / "info" / "alternates" + alternates.parent.mkdir(parents=True, exist_ok=True) + alternates.write_text("# no alternates\n\n") + result = await call(root, "git_status", {"repo_path": str(root)}) + assert result["isError"] is False + + +@pytest.mark.parametrize("entry", ["objects", "refs", "index", "packed-refs"]) +async def test_restricted_server_rejects_git_dir_symlink_pointing_outside( + tmp_path: Path, repo: git.Repo, entry: str +): + # A symbolic link inside a nested repository's `.git` is followed by git, + # for reading (objects, refs) and for writing (a linked index takes + # git_add's writes to the repository it names). + root = root_of(repo) + other = make_repo(tmp_path / "other") + other.git.pack_refs("--all") + nested = make_repo(root / "nested") + nested_git = Path(nested.git_dir) + nested.close() + link = nested_git / entry + if link.is_dir(): + rmtree(link) + else: + link.unlink(missing_ok=True) + link.symlink_to( + Path(other.git_dir) / entry, + target_is_directory=link.name in ("objects", "refs"), + ) + index_before = (Path(other.git_dir) / "index").read_bytes() + (root / "nested" / "new.txt").write_text("new\n") + try: + for tool, arguments in ( + ("git_show", {"revision": other.head.commit.hexsha}), + ("git_branch", {"branch_type": "local"}), + ("git_add", {"files": ["new.txt"]}), + ): + result = await call( + root, tool, {"repo_path": str(root / "nested"), **arguments} + ) + assert uses_outside( + result, (Path(other.git_dir) / entry).resolve(), root + ), (tool, result) + assert (Path(other.git_dir) / "index").read_bytes() == index_before + finally: + other.close() + + +async def test_restricted_server_accepts_git_dir_symlinks_that_stay_inside( + tmp_path: Path, repo: git.Repo +): + # A link within the git directory, or to elsewhere in the allowed + # repository, leads nowhere new; nor does a hooks directory linked to a + # shared one outside, since no hook runs. + root = root_of(repo) + git_dir = Path(repo.git_dir) + (git_dir / "head-link").symlink_to(git_dir / "HEAD") + (git_dir / "readme-link").symlink_to(root / "test.txt") + shared_hooks = tmp_path / "shared-hooks" + shared_hooks.mkdir() + hooks = git_dir / "hooks" + if hooks.exists(): + rmtree(hooks) + hooks.symlink_to(shared_hooks, target_is_directory=True) + result = await call(root, "git_status", {"repo_path": str(root)}) + assert result["isError"] is False + + +async def test_restricted_server_rejects_nested_gitdir_naming_main_of_worktree( + tmp_path: Path, +): + # With a linked worktree as the allowed repository, the main repository's + # git directory is the worktree's common directory, outside it. A `.git` + # file nested in the worktree naming that directory must not reach the + # main repository's index, HEAD or branches. + main = make_repo(tmp_path / "main") + worktree = tmp_path / "worktree" + main.git.worktree("add", str(worktree)) + (tmp_path / "main" / "test.txt").write_text("staged in main\n") + main.git.add("test.txt") + staged_before = main.git.diff("--cached", "--name-only") + main_head = main.head.commit.hexsha + nested = worktree / "nested" + nested.mkdir() + (nested / ".git").write_text(f"gitdir: {main.git_dir}\n") + try: + for tool, arguments in ( + ("git_reset", {}), + ("git_log", {}), + ("git_commit", {"message": "escaped"}), + ): + result = await call(worktree, tool, {"repo_path": str(nested), **arguments}) + assert uses_outside(result, Path(main.git_dir), worktree), (tool, result) + assert main.git.diff("--cached", "--name-only") == staged_before + assert main.head.commit.hexsha == main_head + finally: + main.close() + + +async def test_restricted_server_rejects_nested_gitdir_naming_worktree_git_dir( + tmp_path: Path, +): + # The linked worktree's own git directory, named from a repository nested + # in it, is the allowed repository's, but not opened from the allowed + # repository: refused too. + main = make_repo(tmp_path / "main") + worktree = tmp_path / "worktree" + main.git.worktree("add", str(worktree)) + worktree_git_dir = Path(main.git_dir) / "worktrees" / "worktree" + nested = worktree / "nested" + nested.mkdir() + (nested / ".git").write_text(f"gitdir: {worktree_git_dir}\n") + try: + result = await call(worktree, "git_status", {"repo_path": str(nested)}) + assert uses_outside(result, worktree_git_dir, worktree), result + finally: + main.close() + + async def test_restricted_server_accepts_nested_bare_repository(repo: git.Repo): # A bare repository has no working tree; its git directory is inside. root = root_of(repo) diff --git a/src/git/tests/test_repository_config.py b/src/git/tests/test_repository_config.py new file mode 100644 index 0000000000..3495594707 --- /dev/null +++ b/src/git/tests/test_repository_config.py @@ -0,0 +1,455 @@ +"""A repository's own configuration never makes the server run a program. + +Git runs programs that configuration names: `core.fsmonitor` on an index +refresh, `diff.external` and textconv filters on a diff, filter and merge +drivers that `.gitattributes` selects, hooks, `gpg.program`. A repository's +`.git/config` and hooks travel with it (an unpacked archive, a shared folder), +so the server overrides all of them (`harden_repo`, `HARDENED_CONFIG`) and +none may run, on a read-only tool or any other. + +Each test plants a program that records its own run in a canary file, drives +the real server through a client (`conftest.connect`), and asserts the canary +was never written. The payloads are POSIX shell scripts, so those tests skip +on Windows; the tests of `harden_repo` itself run everywhere. +""" + +import os +import shlex +import stat +import sys +from pathlib import Path +from typing import Any +from unittest import mock + +import git +import pytest +from conftest import connect, make_repo, wire + +from mcp_server_git import server +from mcp_server_git.server import HARDENED_CONFIG, harden_repo + +posix_only = pytest.mark.skipif( + sys.platform == "win32", reason="the payloads are POSIX shell scripts" +) + + +def root_of(repo: git.Repo) -> Path: + assert repo.working_dir is not None + return Path(repo.working_dir) + + +class Payload: + """A program that appends its first argument to a canary file. + + As a filter it passes its input through, and as a textconv it prints the + file it is given, so a run that is not stopped still behaves like the + real thing. + """ + + def __init__(self, directory: Path) -> None: + directory.mkdir() + self.canary = directory / "canary" + self.script = directory / "payload.sh" + canary = shlex.quote(str(self.canary)) + self.script.write_text( + "#!/bin/sh\n" + f'echo "$1" >> {canary}\n' + 'case "$1" in\n' + " filter) exec cat ;;\n" + ' textconv) exec cat "$2" ;;\n' + "esac\n" + "exit 0\n" + ) + self.script.chmod(self.script.stat().st_mode | stat.S_IEXEC) + + def command(self, tag: str) -> str: + """A shell command line that runs the payload with `tag`.""" + return f"{shlex.quote(str(self.script))} {tag}" + + def install_hooks(self, repo: git.Repo, *names: str) -> None: + hooks = Path(repo.git_dir) / "hooks" + hooks.mkdir(exist_ok=True) + for name in names: + hook = hooks / name + hook.write_text(f"#!/bin/sh\n{self.command(name)}\n") + hook.chmod(hook.stat().st_mode | stat.S_IEXEC) + + def runs(self) -> list[str]: + if not self.canary.exists(): + return [] + return self.canary.read_text().split() + + +@pytest.fixture +def payload(tmp_path: Path) -> Payload: + return Payload(tmp_path / "payload") + + +async def call( + name: str, arguments: dict[str, Any], repository: Path | None = None +) -> dict[str, Any]: + async with connect(repository) as session: + return wire(await session.call_tool(name, arguments)) + + +def set_config(repo: git.Repo, **values: str) -> None: + """Set `section.key` (or `section.sub.key`, as `section__sub__key`) values.""" + with repo.config_writer() as config: + for name, value in values.items(): + parts = name.split("__") + section = parts[0] if len(parts) == 2 else f'{parts[0]} "{parts[1]}"' + config.set_value(section, parts[-1], value) + + +def commit_file(repo: git.Repo, name: str, content: str) -> None: + (root_of(repo) / name).write_text(content, newline="\n") + repo.git.add(name) + repo.git.commit("-m", f"write {name}") + + +# -------------------------------------------------------------------------- +# core.fsmonitor +# -------------------------------------------------------------------------- + + +@posix_only +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("git_status", {}), + ("git_diff_unstaged", {}), + ("git_diff_staged", {}), + ("git_diff", {"target": "HEAD"}), + ("git_add", {"files": ["test.txt"]}), + ("git_checkout", {"branch_name": "HEAD~0"}), + ("git_commit", {"message": "change"}), + ], +) +async def test_fsmonitor_never_runs( + repo: git.Repo, payload: Payload, tool: str, arguments: dict[str, Any] +): + (root_of(repo) / "test.txt").write_text("changed\n") + repo.git.add("test.txt") + (root_of(repo) / "test.txt").write_text("changed again\n") + set_config(repo, core__fsmonitor=payload.command("fsmonitor")) + + result = await call(tool, {"repo_path": str(root_of(repo)), **arguments}) + + assert result["isError"] is False, result + assert payload.runs() == [] + + +@posix_only +async def test_fsmonitor_never_runs_under_repository(repo: git.Repo, payload: Payload): + # The same holds for a repository nested inside the allowed one. + nested = make_repo(root_of(repo) / "vendor" / "nested") + set_config(nested, core__fsmonitor=payload.command("fsmonitor")) + nested.close() + + result = await call( + "git_status", + {"repo_path": str(root_of(repo) / "vendor" / "nested")}, + root_of(repo), + ) + + assert result["isError"] is False, result + assert payload.runs() == [] + + +# -------------------------------------------------------------------------- +# External diff programs and textconv filters +# -------------------------------------------------------------------------- + + +@posix_only +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("git_diff_unstaged", {}), + ("git_diff_staged", {}), + ("git_diff", {"target": "HEAD"}), + ], +) +async def test_external_diff_never_runs( + repo: git.Repo, payload: Payload, tool: str, arguments: dict[str, Any] +): + set_config( + repo, + diff__external=payload.command("external"), + diff__evil__command=payload.command("command"), + ) + (root_of(repo) / ".gitattributes").write_text("*.txt diff=evil\n") + (root_of(repo) / "test.txt").write_text("changed\n") + (root_of(repo) / "staged.txt").write_text("staged\n") + repo.git.add("staged.txt") + + result = await call(tool, {"repo_path": str(root_of(repo)), **arguments}) + + assert result["isError"] is False, result + assert ( + "+changed" in result["content"][0]["text"] + or "+staged" in (result["content"][0]["text"]) + ) + assert payload.runs() == [] + + +@posix_only +async def test_textconv_never_runs(repo: git.Repo, payload: Payload): + set_config(repo, diff__evil__textconv=payload.command("textconv")) + commit_file(repo, ".gitattributes", "*.txt diff=evil\n") + (root_of(repo) / "test.txt").write_text("changed\n") + + async with connect(None) as session: + for tool in ("git_diff_unstaged", "git_diff"): + arguments = {"repo_path": str(root_of(repo))} + if tool == "git_diff": + arguments["target"] = "HEAD" + result = wire(await session.call_tool(tool, arguments)) + assert "+changed" in result["content"][0]["text"], result + + assert payload.runs() == [] + + +# -------------------------------------------------------------------------- +# Hooks +# -------------------------------------------------------------------------- + +ALL_HOOKS = ( + "pre-commit", + "prepare-commit-msg", + "commit-msg", + "post-commit", + "post-checkout", + "pre-merge-commit", + "post-merge", + "reference-transaction", + "post-index-change", +) + + +@posix_only +async def test_no_hook_runs(repo: git.Repo, payload: Payload): + # git_commit (GitPython runs pre-commit, commit-msg and post-commit + # itself), git_checkout (post-checkout), git_merge (pre-merge-commit, + # post-merge), and every ref update and index write. + repo.git.checkout("-b", "feature") + commit_file(repo, "feature.txt", "feature\n") + repo.git.checkout("main") + commit_file(repo, "main.txt", "main\n") + payload.install_hooks(repo, *ALL_HOOKS) + path = str(root_of(repo)) + + async with connect(None) as session: + calls: list[tuple[str, dict[str, Any]]] = [ + ("git_merge", {"source_branch": "feature", "fast_forward_only": False}), + ("git_checkout", {"branch_name": "feature"}), + ("git_checkout", {"branch_name": "main"}), + ("git_add", {"files": ["new.txt"]}), + ("git_commit", {"message": "new"}), + ("git_reset", {}), + ("git_create_branch", {"branch_name": "other"}), + ] + (root_of(repo) / "new.txt").write_text("new\n") + for tool, arguments in calls: + result = wire( + await session.call_tool(tool, {"repo_path": path, **arguments}) + ) + assert result["isError"] is False, (tool, result) + + assert payload.runs() == [] + + +# -------------------------------------------------------------------------- +# Filter and merge drivers +# -------------------------------------------------------------------------- + + +@posix_only +@pytest.mark.parametrize("kind", ["clean", "smudge", "process"]) +async def test_filter_driver_never_runs(repo: git.Repo, payload: Payload, kind: str): + commit_file(repo, ".gitattributes", "*.txt filter=evil\n") + repo.git.checkout("-b", "other") + commit_file(repo, "other.txt", "other\n") + repo.git.checkout("main") + set_config(repo, **{f"filter__evil__{kind}": payload.command("filter")}) + (root_of(repo) / "new.txt").write_text("new\n") + path = str(root_of(repo)) + + async with connect(None) as session: + for tool, arguments in ( + ("git_status", {}), + ("git_diff_unstaged", {}), + ("git_add", {"files": ["new.txt"]}), + ("git_checkout", {"branch_name": "other"}), + ): + result = wire( + await session.call_tool(tool, {"repo_path": path, **arguments}) + ) + assert result["isError"] is False, (tool, result) + + assert payload.runs() == [] + assert (root_of(repo) / "other.txt").read_text() == "other\n" + + +@posix_only +async def test_merge_driver_never_runs(repo: git.Repo, payload: Payload): + # With the driver disabled, the conflicting change is a plain conflict, + # which git_merge aborts. + commit_file(repo, ".gitattributes", "*.txt merge=evil\n") + repo.git.checkout("-b", "feature") + commit_file(repo, "test.txt", "feature\n") + repo.git.checkout("main") + commit_file(repo, "test.txt", "main\n") + set_config(repo, merge__evil__driver=payload.command("merge")) + + result = await call( + "git_merge", + { + "repo_path": str(root_of(repo)), + "source_branch": "feature", + "fast_forward_only": False, + }, + ) + + assert result["isError"] is True + assert "stopped on conflicts in: test.txt" in result["content"][0]["text"] + assert payload.runs() == [] + + +@posix_only +async def test_signing_program_never_runs(repo: git.Repo, payload: Payload): + gpg = payload.script.parent / "gpg.sh" + gpg.write_text(f"#!/bin/sh\n{payload.command('gpg')}\nexit 1\n") + gpg.chmod(gpg.stat().st_mode | stat.S_IEXEC) + repo.git.checkout("-b", "feature") + commit_file(repo, "feature.txt", "feature\n") + repo.git.checkout("main") + commit_file(repo, "main.txt", "main\n") + set_config( + repo, + commit__gpgSign="true", + merge__verifySignatures="true", + user__signingKey="ABCDEF", + gpg__program=str(gpg), + ) + + result = await call( + "git_merge", + { + "repo_path": str(root_of(repo)), + "source_branch": "feature", + "fast_forward_only": False, + }, + ) + + assert result["isError"] is False, result + assert payload.runs() == [] + + +@posix_only +async def test_submodule_configuration_is_not_reached( + tmp_path: Path, repo: git.Repo, payload: Payload +): + # Status and diff look inside a submodule by running git there, under the + # submodule's own configuration, which the server's overrides of driver + # names cannot see; they no longer look inside. + source = make_repo(tmp_path / "source") + commit_file(source, ".gitattributes", "*.txt filter=evil\n") + source.close() + repo.git(c="protocol.file.allow=always").submodule( + "add", str(tmp_path / "source"), "sm" + ) + repo.git.commit("-m", "add submodule") + submodule = git.Repo(root_of(repo) / "sm") + set_config(submodule, filter__evil__clean=payload.command("filter")) + submodule.close() + # Same content, new mtime: git must read the file again, through the filter. + os.utime(root_of(repo) / "sm" / "test.txt", (1_000_000_000, 1_000_000_000)) + path = str(root_of(repo)) + + async with connect(None) as session: + for tool in ("git_status", "git_diff_unstaged"): + result = wire(await session.call_tool(tool, {"repo_path": path})) + assert result["isError"] is False, (tool, result) + + assert payload.runs() == [] + + +# -------------------------------------------------------------------------- +# harden_repo +# -------------------------------------------------------------------------- + + +def config_value(repo: git.Repo, key: str) -> str: + return repo.git.config("--get", key) + + +def test_harden_repo_overrides_the_repository_configuration(repo: git.Repo): + set_config(repo, core__fsmonitor="true", core__hooksPath="hooks") + harden_repo(repo) + for key, value in HARDENED_CONFIG: + assert config_value(repo, key) == value + + +def test_harden_repo_keeps_the_users_own_drivers( + tmp_path: Path, repo: git.Repo, monkeypatch: pytest.MonkeyPatch +): + # A driver from the user's global configuration (git-lfs, say) keeps + # working; the repository's own drivers are emptied, and a repository + # that redefines the user's driver gets that one emptied too. + global_config = tmp_path / "global-config" + global_config.write_text( + '[filter "lfs"]\n\tclean = lfs-clean\n[filter "mine"]\n\tsmudge = my-smudge\n' + ) + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(global_config)) + set_config( + repo, + filter__lfs__clean="evil", + filter__Evil__process="evil", + merge__evil__driver="evil", + ) + + harden_repo(repo) + + assert config_value(repo, "filter.mine.smudge") == "my-smudge" + assert config_value(repo, "filter.lfs.clean") == "" + assert config_value(repo, "filter.Evil.process") == "" + assert config_value(repo, "merge.evil.driver") == "" + + +def test_harden_repo_keeps_the_users_command_line_configuration( + repo: git.Repo, monkeypatch: pytest.MonkeyPatch +): + # Configuration the user passes the server through GIT_CONFIG_COUNT stays + # in force; the overrides are numbered after it. + monkeypatch.setenv("GIT_CONFIG_COUNT", "1") + monkeypatch.setenv("GIT_CONFIG_KEY_0", "user.name") + monkeypatch.setenv("GIT_CONFIG_VALUE_0", "From Environment") + harden_repo(repo) + assert config_value(repo, "user.name") == "From Environment" + assert config_value(repo, "core.fsmonitor") == "false" + + +def test_harden_repo_refuses_a_git_that_ignores_the_overrides( + tmp_path: Path, repo: git.Repo, monkeypatch: pytest.MonkeyPatch +): + # Git before 2.31 ignores GIT_CONFIG_COUNT; simulated by setting nothing. + # With no configuration but the repository's, nothing matches (exit 1). + (tmp_path / "empty-config").write_text("") + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / "empty-config")) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + with ( + mock.patch.object(server, "_config_env", return_value={}), + pytest.raises(git.GitError, match="Git 2.31 or later is required"), + ): + harden_repo(repo) + + +def test_harden_repo_reports_a_failing_git(repo: git.Repo): + # Any failure but "nothing matched" (exit 1) is git's own, reported as is. + failure = git.GitCommandError(["git", "config"], 128, "fatal: bad config") + with ( + mock.patch.object(git.Git, "config", side_effect=failure, create=True), + pytest.raises(git.GitCommandError, match="bad config"), + ): + harden_repo(repo) From 759cbbc03c59ce5717a77bf4518ee86761f55128 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 11 Nov 2026 00:03:36 -0500 Subject: [PATCH 2/2] fix(git): address review findings for GHSA-6hqx-gqrx-c5r2 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/git/README.md | 4 +-- src/git/src/mcp_server_git/server.py | 29 ++++++++++++++++---- src/git/tests/test_protocol.py | 26 ++++++++++++++++++ src/git/tests/test_repository_config.py | 36 +++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 7 deletions(-) diff --git a/src/git/README.md b/src/git/README.md index 54499dd378..fab7aa5527 100644 --- a/src/git/README.md +++ b/src/git/README.md @@ -176,7 +176,7 @@ python -m mcp_server_git A tool call's `repo_path` must be inside that repository, and so must the directories Git then uses for it: a repository nested inside whose `.git` file, `commondir` or `core.worktree` points outside is refused. A linked worktree can still be the `--repository`, though its Git directory lives under the main repository's `.git`; only the worktree itself may use that directory, so a repository nested in the worktree whose `.git` names it, or the main repository's `.git`, is refused. With `--repository` set, the server also refuses a repository whose Git directory: - names alternate object stores (`objects/info/alternates` or `objects/info/http-alternates` with any entry), wherever they are. A clone made with `--reference` or `--shared` therefore cannot be used under `--repository`; `git repack -a -d` and then deleting `objects/info/alternates` makes it self-contained. -- holds a symbolic link (at `objects`, `refs`, `index`, `HEAD` or anywhere else in it) that resolves outside both the allowed repository and that Git directory. A `hooks` directory linked elsewhere is allowed, since no hook runs. +- holds a symbolic link (at `objects`, `refs`, `index`, `HEAD` or anywhere else in it) that resolves outside both the allowed repository and that Git directory. A link that stays inside is followed, and what it leads to is checked the same way. A `hooks` directory linked elsewhere is allowed, since no hook runs. ### A repository's own programs @@ -185,7 +185,7 @@ A repository's configuration and hooks travel with it (an unpacked archive, a sh - `core.fsmonitor` is `false`, and `core.hooksPath` points at the null device, so no hook runs; `git_commit` also skips the hooks GitPython would run itself (`pre-commit`, `commit-msg`, `post-commit`). - Diffs pass `--no-ext-diff` and `--no-textconv`, so `diff.external`, `diff..command` and `diff..textconv` are ignored. - Each filter driver (`filter..clean`, `smudge`, `process`) and merge driver (`merge..driver`) that the repository's own configuration defines is emptied: files are staged and checked out unfiltered, and a file a merge driver would have merged is a conflict, which `git_merge` aborts. Drivers from your global or system configuration, such as Git LFS's, keep working. -- `commit.gpgSign` and `merge.verifySignatures` are `false`, `submodule.recurse` and `merge.autoStash` are `false`, and `core.pager` is `cat`. +- `commit.gpgSign` and `merge.verifySignatures` are `false`, and `git_merge` passes `--no-gpg-sign` and `--no-verify-signatures`. Each `branch..mergeOptions` the repository's own configuration sets is emptied, since Git reads it as command-line options. `submodule.recurse` and `merge.autoStash` are `false`, and `core.pager` is `cat`. - `git_status`, `git_diff_unstaged` and `git_diff` pass `--ignore-submodules=dirty`, because looking for changes inside a submodule runs Git under the submodule's own configuration. A submodule whose checked-out commit moved is still reported; changes inside one are not, so run the tool on the submodule's path to see them. This needs Git 2.31 or later; with an older Git every tool call fails with an error saying so, rather than run unprotected. diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index d93e96e5e4..cd1d874ece 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -147,12 +147,16 @@ def _git_text(repo: git.Repo, command: str, *args: str) -> str: # (`merge..driver`) is a command too, under a name the repository # chooses, so each one the repository's own configuration defines is # overridden with an empty command, which git skips (a filter) or reports as -# a conflict (a merge driver). Drivers from the user's global or system -# configuration (git-lfs, say) are the user's own and keep working. +# a conflict (a merge driver). `branch..mergeOptions` is emptied the +# same way: git merge reads it as command-line options, which outrank the +# overrides above (`--gpg-sign` there would run `gpg.program` whatever +# `commit.gpgSign` says). Drivers and options from the user's global or +# system configuration (git-lfs, say) are the user's own and keep working. # `harden_repo` lists these keys, and `core.hooksPath`, whose command-scope # value shows that git applied the overrides. _LISTED_KEYS = ( - r"^(core\.hookspath|filter\..+\.(clean|smudge|process)|merge\..+\.driver)$" + r"^(core\.hookspath|filter\..+\.(clean|smudge|process)|merge\..+\.driver" + r"|branch\..+\.mergeoptions)$" ) # Scopes whose configuration the user wrote rather than the repository. @@ -603,7 +607,9 @@ def git_merge( # config says (merge.ff, branch..mergeOptions): --ff/--ff-only # choose fast-forwarding, --commit and --no-squash make a merge that # succeeds a completed one that moves HEAD, and --no-edit keeps git from - # opening an editor for the message. --no-overwrite-ignore refuses to + # opening an editor for the message. --no-gpg-sign and + # --no-verify-signatures keep `gpg.program` from running whatever asks + # for it (see `_LISTED_KEYS`). --no-overwrite-ignore refuses to # replace an ignored local file (a `.env`, say) that the source tracks: # `is_dirty()` cannot see one, and git would overwrite it unrecoverably. # @@ -619,6 +625,8 @@ def git_merge( "--commit", "--no-squash", "--no-edit", + "--no-gpg-sign", + "--no-verify-signatures", "--no-overwrite-ignore", "-m", f"Merge '{source_branch}' into '{target}'", @@ -802,6 +810,10 @@ def _check_git_dir_contents( anything else in a git directory is followed by git, for reading and for writing (a linked `index` takes git_add's writes elsewhere). + A link that is allowed and names a directory is walked too, since git + follows it into that directory and on into any link inside it; each + directory is walked once, so a cycle of links ends. + `hooks` is not walked: no hook runs (`HARDENED_CONFIG`), so a hooks directory linked to a shared one elsewhere is harmless. """ @@ -817,6 +829,7 @@ def _check_git_dir_contents( "alternate object stores are not allowed with --repository" ) pending = [directory] + visited = {directory} while pending: current = pending.pop() with os.scandir(current) as entries: @@ -830,8 +843,14 @@ def _check_git_dir_contents( or target.is_relative_to(directory) ): raise outside(target) + if target.is_dir() and target not in visited: + visited.add(target) + pending.append(target) elif entry.is_dir(): - pending.append(Path(entry.path)) + real = Path(entry.path).resolve() + if real not in visited: + visited.add(real) + pending.append(real) def git_branch( diff --git a/src/git/tests/test_protocol.py b/src/git/tests/test_protocol.py index b5da78d55b..ffdaf91276 100644 --- a/src/git/tests/test_protocol.py +++ b/src/git/tests/test_protocol.py @@ -1482,6 +1482,32 @@ async def test_restricted_server_rejects_git_dir_symlink_pointing_outside( other.close() +async def test_restricted_server_follows_an_allowed_git_dir_symlink( + tmp_path: Path, repo: git.Repo +): + # A link that stays inside the allowed repository is followed by git, so + # what it leads to is checked too: here `.git/objects` names an object + # store in the working tree holding a link outside. + root = root_of(repo) + other = make_repo(tmp_path / "other") + nested = make_repo(root / "nested") + nested_git = Path(nested.git_dir) + nested.close() + store = root / "store" + shutil.copytree(nested_git / "objects", store, symlinks=True) + rmtree(nested_git / "objects") + (nested_git / "objects").symlink_to(store, target_is_directory=True) + escape = Path(other.git_dir) / "objects" + (store / "elsewhere").symlink_to(escape, target_is_directory=True) + # A cycle of allowed links ends rather than being walked forever. + (store / "loop").symlink_to(store, target_is_directory=True) + try: + result = await call(root, "git_status", {"repo_path": str(root / "nested")}) + assert uses_outside(result, escape.resolve(), root), result + finally: + other.close() + + async def test_restricted_server_accepts_git_dir_symlinks_that_stay_inside( tmp_path: Path, repo: git.Repo ): diff --git a/src/git/tests/test_repository_config.py b/src/git/tests/test_repository_config.py index 3495594707..c5ae1eec8d 100644 --- a/src/git/tests/test_repository_config.py +++ b/src/git/tests/test_repository_config.py @@ -346,6 +346,40 @@ async def test_signing_program_never_runs(repo: git.Repo, payload: Payload): assert payload.runs() == [] +@posix_only +async def test_merge_options_never_run_the_signing_program( + repo: git.Repo, payload: Payload +): + # `branch..mergeOptions` reaches git merge as command-line options, + # which outrank `commit.gpgSign=false`: `--gpg-sign` there would sign the + # merge commit with the repository's `gpg.program`. + gpg = payload.script.parent / "gpg.sh" + gpg.write_text(f"#!/bin/sh\n{payload.command('gpg')}\nexit 1\n") + gpg.chmod(gpg.stat().st_mode | stat.S_IEXEC) + repo.git.checkout("-b", "feature") + commit_file(repo, "feature.txt", "feature\n") + repo.git.checkout("main") + commit_file(repo, "main.txt", "main\n") + set_config( + repo, + branch__main__mergeOptions="--gpg-sign --verify-signatures", + user__signingKey="ABCDEF", + gpg__program=str(gpg), + ) + + result = await call( + "git_merge", + { + "repo_path": str(root_of(repo)), + "source_branch": "feature", + "fast_forward_only": False, + }, + ) + + assert result["isError"] is False, result + assert payload.runs() == [] + + @posix_only async def test_submodule_configuration_is_not_reached( tmp_path: Path, repo: git.Repo, payload: Payload @@ -407,6 +441,7 @@ def test_harden_repo_keeps_the_users_own_drivers( filter__lfs__clean="evil", filter__Evil__process="evil", merge__evil__driver="evil", + branch__main__mergeOptions="--gpg-sign", ) harden_repo(repo) @@ -415,6 +450,7 @@ def test_harden_repo_keeps_the_users_own_drivers( assert config_value(repo, "filter.lfs.clean") == "" assert config_value(repo, "filter.Evil.process") == "" assert config_value(repo, "merge.evil.driver") == "" + assert config_value(repo, "branch.main.mergeOptions") == "" def test_harden_repo_keeps_the_users_command_line_configuration(