From 9e5d5b8e4b5dd6fa4fc84cad5eb291568024aad4 Mon Sep 17 00:00:00 2001 From: Jenn Newton <153535757+jenn-newton@users.noreply.github.com> Date: Wed, 17 Dec 2025 11:01:19 -0500 Subject: [PATCH] Merge commit from fork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add validation to reject arguments starting with '-' and verify arguments resolve to valid git refs via rev_parse before passing to git CLI commands. This prevents flag-like values from being interpreted as command-line options (e.g., --output=/path/to/file). CWE-88: Improper Neutralization of Argument Delimiters in a Command 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude --- src/git/src/mcp_server_git/server.py | 10 +++ src/git/tests/test_server.py | 114 ++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 1968ded2..fb334b54 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -116,6 +116,11 @@ def git_diff_staged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES) return repo.git.diff(f"--unified={context_lines}", "--cached") def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_LINES) -> str: + # Defense in depth: reject targets starting with '-' to prevent flag injection, + # even if a malicious ref with that name exists (e.g. via filesystem manipulation) + if target.startswith("-"): + raise git.exc.BadName(f"Invalid target: '{target}' - cannot start with '-'") + repo.rev_parse(target) # Validates target is a real git ref, throws BadName if not return repo.git.diff(f"--unified={context_lines}", target) def git_commit(repo: git.Repo, message: str) -> str: @@ -179,6 +184,11 @@ def git_create_branch(repo: git.Repo, branch_name: str, base_branch: str | None return f"Created branch '{branch_name}' from '{base.name}'" def git_checkout(repo: git.Repo, branch_name: str) -> str: + # Defense in depth: reject branch names starting with '-' to prevent flag injection, + # even if a malicious ref with that name exists (e.g. via filesystem manipulation) + if branch_name.startswith("-"): + raise git.exc.BadName(f"Invalid branch name: '{branch_name}' - cannot start with '-'") + repo.rev_parse(branch_name) # Validates branch_name is a real git ref, throws BadName if not repo.git.checkout(branch_name) return f"Switched to branch '{branch_name}'" diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index c852d1f7..921e4a7b 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -39,7 +39,7 @@ def test_git_checkout_existing_branch(test_repository): def test_git_checkout_nonexistent_branch(test_repository): - with pytest.raises(git.GitCommandError): + with pytest.raises(git.exc.BadName): git_checkout(test_repository, "nonexistent-branch") def test_git_branch_local(test_repository): @@ -248,3 +248,115 @@ def test_git_show_initial_commit(test_repository): assert "Commit:" in result assert "initial commit" in result assert "test.txt" in result + + +# Tests for argument injection protection + +def test_git_diff_rejects_flag_injection(test_repository): + """git_diff should reject flags that could be used for argument injection.""" + with pytest.raises(git.exc.BadName): + git_diff(test_repository, "--output=/tmp/evil") + + with pytest.raises(git.exc.BadName): + git_diff(test_repository, "--help") + + with pytest.raises(git.exc.BadName): + git_diff(test_repository, "-p") + + +def test_git_checkout_rejects_flag_injection(test_repository): + """git_checkout should reject flags that could be used for argument injection.""" + with pytest.raises(git.exc.BadName): + git_checkout(test_repository, "--help") + + with pytest.raises(git.exc.BadName): + git_checkout(test_repository, "--orphan=evil") + + with pytest.raises(git.exc.BadName): + git_checkout(test_repository, "-f") + + +def test_git_diff_allows_valid_refs(test_repository): + """git_diff should work normally with valid git refs.""" + # Get the default branch name + default_branch = test_repository.active_branch.name + + # Create a branch with a commit for diffing + test_repository.git.checkout("-b", "valid-diff-branch") + file_path = Path(test_repository.working_dir) / "test.txt" + file_path.write_text("valid diff content") + test_repository.index.add(["test.txt"]) + test_repository.index.commit("valid diff commit") + + # Test with branch name + result = git_diff(test_repository, default_branch) + assert "test.txt" in result + + # Test with HEAD~1 + result = git_diff(test_repository, "HEAD~1") + assert "test.txt" in result + + # Test with commit hash + commit_sha = test_repository.head.commit.hexsha + result = git_diff(test_repository, commit_sha) + assert result is not None + + +def test_git_checkout_allows_valid_branches(test_repository): + """git_checkout should work normally with valid branch names.""" + # Get the default branch name + default_branch = test_repository.active_branch.name + + # Create a branch to checkout + test_repository.git.branch("valid-checkout-branch") + + result = git_checkout(test_repository, "valid-checkout-branch") + assert "Switched to branch 'valid-checkout-branch'" in result + assert test_repository.active_branch.name == "valid-checkout-branch" + + # Checkout back to default branch + result = git_checkout(test_repository, default_branch) + assert "Switched to branch" in result + assert test_repository.active_branch.name == default_branch + + +def test_git_diff_rejects_malicious_refs(test_repository): + """git_diff should reject refs starting with '-' even if they exist. + + This tests defense in depth against an attacker who creates malicious + refs via filesystem manipulation (e.g. using mcp-filesystem to write + to .git/refs/heads/--output=...). + """ + import os + + # Manually create a malicious ref by writing directly to .git/refs + sha = test_repository.head.commit.hexsha + refs_dir = Path(test_repository.git_dir) / "refs" / "heads" + malicious_ref_path = refs_dir / "--output=evil.txt" + malicious_ref_path.write_text(sha) + + # Even though the ref exists, it should be rejected + with pytest.raises(git.exc.BadName): + git_diff(test_repository, "--output=evil.txt") + + # Verify no file was created (the attack was blocked) + assert not os.path.exists("evil.txt") + + # Cleanup + malicious_ref_path.unlink() + + +def test_git_checkout_rejects_malicious_refs(test_repository): + """git_checkout should reject refs starting with '-' even if they exist.""" + # Manually create a malicious ref + sha = test_repository.head.commit.hexsha + refs_dir = Path(test_repository.git_dir) / "refs" / "heads" + malicious_ref_path = refs_dir / "--orphan=evil" + malicious_ref_path.write_text(sha) + + # Even though the ref exists, it should be rejected + with pytest.raises(git.exc.BadName): + git_checkout(test_repository, "--orphan=evil") + + # Cleanup + malicious_ref_path.unlink()