diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index 58d8178d3a..1d4ed9a0aa 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -132,6 +132,10 @@ def git_add(repo: git.Repo, files: list[str]) -> str: if files == ["."]: repo.git.add(".") else: + # Validate all file paths are within repository boundary + # to prevent path traversal attacks (e.g., '../../../.ssh/id_rsa') + for file_path in files: + validate_file_path(file_path, repo) repo.index.add(files) return "Files staged successfully" @@ -176,6 +180,13 @@ def git_log(repo: git.Repo, max_count: int = 10, start_timestamp: Optional[str] return log def git_create_branch(repo: git.Repo, branch_name: str, base_branch: str | None = None) -> str: + # Defense in depth: reject branch names starting with '-' to prevent + # creating refs that could be interpreted as flags by other git commands + if branch_name.startswith("-"): + raise ValueError(f"Invalid branch name: '{branch_name}' - cannot start with '-'") + if base_branch and base_branch.startswith("-"): + raise ValueError(f"Invalid base branch: '{base_branch}' - cannot start with '-'") + if base_branch: base = repo.references[base_branch] else: @@ -239,6 +250,32 @@ def validate_repo_path(repo_path: Path, allowed_repository: Path | None) -> None ) +def validate_file_path(file_path: str, repo: git.Repo) -> None: + """Validate that a file path is within the repository boundary. + + This prevents path traversal attacks where malicious paths like + '../../../.ssh/id_rsa' could be used to stage files outside the repo. + """ + repo_root = Path(repo.working_dir).resolve() + + # Resolve the file path relative to repo root + try: + if Path(file_path).is_absolute(): + resolved_file = Path(file_path).resolve() + else: + resolved_file = (repo_root / file_path).resolve() + except (OSError, RuntimeError): + raise ValueError(f"Invalid file path: {file_path}") + + # Check if resolved path is within repo boundary + try: + resolved_file.relative_to(repo_root) + except ValueError: + raise ValueError( + f"Path '{file_path}' is outside the repository" + ) + + def git_branch(repo: git.Repo, branch_type: str, contains: str | None = None, not_contains: str | None = None) -> str: match contains: case None: diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 054bf8c756..cf8de424d8 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -16,6 +16,7 @@ git_create_branch, git_show, validate_repo_path, + validate_file_path, ) import shutil @@ -423,3 +424,144 @@ def test_git_checkout_rejects_malicious_refs(test_repository): # Cleanup malicious_ref_path.unlink() + + +# Tests for git_add path traversal vulnerability (CVE fix) + +def test_git_add_rejects_path_traversal(test_repository): + """git_add should reject path traversal attempts using ../""" + with pytest.raises(ValueError) as exc_info: + git_add(test_repository, ["../../../etc/passwd"]) + assert "outside the repository" in str(exc_info.value) + + +def test_git_add_rejects_absolute_path_outside_repo(test_repository, tmp_path: Path): + """git_add should reject absolute paths outside the repository.""" + outside_file = tmp_path / "outside_file.txt" + outside_file.write_text("sensitive data") + + with pytest.raises(ValueError) as exc_info: + git_add(test_repository, [str(outside_file)]) + assert "outside the repository" in str(exc_info.value) + + +def test_git_add_rejects_kube_config_traversal(test_repository): + """git_add should reject attempts to stage ~/.kube/config via traversal.""" + with pytest.raises(ValueError) as exc_info: + git_add(test_repository, ["../../../.kube/config"]) + assert "outside the repository" in str(exc_info.value) + + +def test_git_add_rejects_ssh_key_traversal(test_repository): + """git_add should reject attempts to stage ~/.ssh/id_rsa via traversal.""" + with pytest.raises(ValueError) as exc_info: + git_add(test_repository, ["../../../.ssh/id_rsa"]) + assert "outside the repository" in str(exc_info.value) + + +def test_git_add_allows_valid_files_in_repo(test_repository): + """git_add should still work for valid files within the repository.""" + file_path = Path(test_repository.working_dir) / "valid_file.txt" + file_path.write_text("valid content") + + result = git_add(test_repository, ["valid_file.txt"]) + + assert result == "Files staged successfully" + staged_files = [item.a_path for item in test_repository.index.diff("HEAD")] + assert "valid_file.txt" in staged_files + + +def test_git_add_allows_subdirectory_files(test_repository): + """git_add should allow files in subdirectories within the repository.""" + subdir = Path(test_repository.working_dir) / "subdir" + subdir.mkdir() + file_path = subdir / "nested_file.txt" + file_path.write_text("nested content") + + result = git_add(test_repository, ["subdir/nested_file.txt"]) + + assert result == "Files staged successfully" + staged_files = [item.a_path for item in test_repository.index.diff("HEAD")] + assert "subdir/nested_file.txt" in staged_files + + +def test_validate_file_path_rejects_traversal(test_repository): + """validate_file_path should reject path traversal attempts.""" + with pytest.raises(ValueError) as exc_info: + validate_file_path("../../../etc/passwd", test_repository) + assert "outside the repository" in str(exc_info.value) + + +def test_validate_file_path_rejects_absolute_outside(test_repository, tmp_path: Path): + """validate_file_path should reject absolute paths outside repo.""" + outside = tmp_path / "outside.txt" + outside.write_text("outside") + + with pytest.raises(ValueError) as exc_info: + validate_file_path(str(outside), test_repository) + assert "outside the repository" in str(exc_info.value) + + +def test_validate_file_path_allows_valid_relative(test_repository): + """validate_file_path should allow valid relative paths in repo.""" + file_path = Path(test_repository.working_dir) / "valid.txt" + file_path.write_text("valid") + + # Should not raise + validate_file_path("valid.txt", test_repository) + + +def test_validate_file_path_allows_valid_absolute_in_repo(test_repository): + """validate_file_path should allow absolute paths within the repo.""" + file_path = Path(test_repository.working_dir) / "valid_abs.txt" + file_path.write_text("valid") + + # Should not raise + validate_file_path(str(file_path), test_repository) + + +# Tests for git_create_branch argument injection protection + +def test_git_create_branch_rejects_flag_injection(test_repository): + """git_create_branch should reject branch names starting with '-'.""" + with pytest.raises(ValueError) as exc_info: + git_create_branch(test_repository, "--help") + assert "cannot start with '-'" in str(exc_info.value) + + with pytest.raises(ValueError) as exc_info: + git_create_branch(test_repository, "--exec=calc.exe") + assert "cannot start with '-'" in str(exc_info.value) + + with pytest.raises(ValueError) as exc_info: + git_create_branch(test_repository, "-d") + assert "cannot start with '-'" in str(exc_info.value) + + +def test_git_create_branch_rejects_malicious_base_branch(test_repository): + """git_create_branch should reject base_branch names starting with '-'.""" + with pytest.raises(ValueError) as exc_info: + git_create_branch(test_repository, "new-branch", "--help") + assert "cannot start with '-'" in str(exc_info.value) + + with pytest.raises(ValueError) as exc_info: + git_create_branch(test_repository, "new-branch", "--exec=malicious") + assert "cannot start with '-'" in str(exc_info.value) + + +def test_git_create_branch_allows_valid_names(test_repository): + """git_create_branch should work with valid branch names.""" + result = git_create_branch(test_repository, "feature-branch") + assert "Created branch 'feature-branch'" in result + + branches = [ref.name for ref in test_repository.references] + assert "feature-branch" in branches + + +def test_git_create_branch_allows_valid_base_branch(test_repository): + """git_create_branch should work with valid base branch names.""" + # Get the default branch name + default_branch = test_repository.active_branch.name + + result = git_create_branch(test_repository, "derived-feature", default_branch) + assert "Created branch 'derived-feature'" in result + assert default_branch in result