diff --git a/src/mcp_server_git/applications/server_application.py b/src/mcp_server_git/applications/server_application.py index fc46c8f0..e123f6c4 100644 --- a/src/mcp_server_git/applications/server_application.py +++ b/src/mcp_server_git/applications/server_application.py @@ -12,6 +12,7 @@ import asyncio import logging +import os import signal import sys from collections.abc import AsyncIterator @@ -1500,6 +1501,63 @@ class MCPResponse: logger.info("MCP tools registered successfully") + def _looks_like_file_path(self, value: str) -> bool: + """ + Check if a string looks like a file path rather than a GitHub identifier. + + Args: + value: String to check + + Returns: + True if the value appears to be a file path, False otherwise + """ + # Check for path separators (forward or back slash) + if "/" in value or "\\" in value: + return True + + # Check if it's an absolute path + # Note: This is a conservative check. While it might flag edge cases like + # "C:" on Windows, GitHub itself doesn't allow colons in repository names, + # so this provides an extra layer of safety. + if os.path.isabs(value): + return True + + return False + + def _validate_github_params(self, repo_owner: str, repo_name: str) -> None: + """ + Validate GitHub API parameters to ensure they're not file paths. + + GitHub API operations use repo_owner and repo_name (e.g., "MementoRC", "mcp-git"), + NOT local file system paths. This validation prevents confusion between: + - Local git operations: use repository paths (e.g., "/home/user/repo") + - GitHub API operations: use owner/name pairs (e.g., "owner", "repo") + + Args: + repo_owner: GitHub repository owner/organization name + repo_name: GitHub repository name + + Raises: + ValueError: If parameters look like file paths instead of GitHub identifiers + """ + # Check if repo_owner looks like a file path + if self._looks_like_file_path(repo_owner): + raise ValueError( + f"Invalid repo_owner: '{repo_owner}' appears to be a file path. " + f"GitHub API operations require repository owner/name parameters, not local paths. " + f"Use 'repo_owner' (e.g., 'MementoRC') and 'repo_name' (e.g., 'mcp-git'), " + f"not the bound '--repository' path." + ) + + # Check if repo_name looks like a file path + if self._looks_like_file_path(repo_name): + raise ValueError( + f"Invalid repo_name: '{repo_name}' appears to be a file path. " + f"GitHub API operations require repository owner/name parameters, not local paths. " + f"Use 'repo_owner' (e.g., 'MementoRC') and 'repo_name' (e.g., 'mcp-git'), " + f"not the bound '--repository' path." + ) + async def _execute_tool_operation(self, name: str, arguments: dict): """Execute the actual tool logic without middleware.""" # COMPREHENSIVE INTEGRATED LOGGING @@ -1537,7 +1595,8 @@ async def _execute_tool_operation(self, name: str, arguments: dict): ) from ..utils.git_import import Repo - # Get repository path from arguments + # Get repository path from arguments for LOCAL git operations ONLY + # NOTE: This path is used ONLY for git operations, NOT for GitHub API operations default_repo_path: str = ( str(self.config.repository_path) if self.config.repository_path else "." ) @@ -1670,9 +1729,15 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitTools.REMOTE_GET_URL: result = git_remote_get_url(repo, arguments["name"]) # GitHub Tools (don't require a Repo object) + # NOTE: GitHub API operations use repo_owner/repo_name, NOT local file paths elif name == GitHubTools.CREATE_ISSUE: from ..github.api import github_create_issue + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_create_issue( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1685,6 +1750,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.LIST_ISSUES: from ..github.api import github_list_issues + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_list_issues( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1703,6 +1773,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.UPDATE_ISSUE: from ..github.api import github_update_issue + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_update_issue( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1717,6 +1792,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_PR_CHECKS: from ..github.api import github_get_pr_checks + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_pr_checks( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1727,6 +1807,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_PR_DETAILS: from ..github.api import github_get_pr_details + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_pr_details( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1737,6 +1822,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.LIST_PULL_REQUESTS: from ..github.api import github_list_pull_requests + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_list_pull_requests( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1751,6 +1841,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_PR_STATUS: from ..github.api import github_get_pr_status + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_pr_status( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1759,6 +1854,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_PR_FILES: from ..github.api import github_get_pr_files + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_pr_files( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1770,6 +1870,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.EDIT_PR_DESCRIPTION: from ..github.api import github_edit_pr_description + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_edit_pr_description( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1779,6 +1884,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_WORKFLOW_RUN: from ..github.api import github_get_workflow_run + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_workflow_run( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1788,6 +1898,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.LIST_WORKFLOW_RUNS: from ..github.api import github_list_workflow_runs + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_list_workflow_runs( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1811,6 +1926,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): if "repo_owner" not in arguments or "repo_name" not in arguments: result = "Error: repo_owner and repo_name are required arguments" else: + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_await_workflow_completion( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1821,6 +1941,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.CREATE_PR: from ..github.api import github_create_pr + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_create_pr( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1833,6 +1958,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.MERGE_PR: from ..github.api import github_merge_pr + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_merge_pr( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1844,6 +1974,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.ADD_PR_COMMENT: from ..github.api import github_add_pr_comment + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_add_pr_comment( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1853,6 +1988,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.CLOSE_PR: from ..github.api import github_close_pr + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_close_pr( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1861,6 +2001,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.REOPEN_PR: from ..github.api import github_reopen_pr + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_reopen_pr( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1869,6 +2014,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.UPDATE_PR: from ..github.api import github_update_pr + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_update_pr( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], @@ -1881,6 +2031,11 @@ async def _execute_tool_operation(self, name: str, arguments: dict): elif name == GitHubTools.GET_FAILING_JOBS: from ..github.api import github_get_failing_jobs + # Validate that repo_owner/repo_name are not file paths + self._validate_github_params( + arguments["repo_owner"], arguments["repo_name"] + ) + result = await github_get_failing_jobs( repo_owner=arguments["repo_owner"], repo_name=arguments["repo_name"], diff --git a/tests/unit/applications/test_github_validation.py b/tests/unit/applications/test_github_validation.py new file mode 100644 index 00000000..bff9d3e6 --- /dev/null +++ b/tests/unit/applications/test_github_validation.py @@ -0,0 +1,125 @@ +""" +Unit tests for GitHub parameter validation in server application. + +This module tests the validation that prevents GitHub operations from +incorrectly using local repository paths instead of repo_owner/repo_name. +""" + +import pytest + +from mcp_server_git.applications.server_application import GitServerApplication +from mcp_server_git.frameworks.server_configuration import GitServerConfig + + +class TestGitHubParameterValidation: + """Test validation of GitHub API parameters.""" + + def setup_method(self): + """Set up test fixtures.""" + config = GitServerConfig() + self.app = GitServerApplication(config) + + def test_validate_github_params_valid_simple(self): + """Test validation accepts simple valid repository identifiers.""" + # Should not raise any exception + self.app._validate_github_params("MementoRC", "mcp-git") + self.app._validate_github_params("facebook", "react") + self.app._validate_github_params("microsoft", "vscode") + + def test_validate_github_params_valid_with_hyphens(self): + """Test validation accepts identifiers with hyphens.""" + # Should not raise any exception + self.app._validate_github_params("my-org", "my-repo") + self.app._validate_github_params("some-company", "some-project-name") + + def test_validate_github_params_valid_with_underscores(self): + """Test validation accepts identifiers with underscores.""" + # Should not raise any exception + self.app._validate_github_params("my_org", "my_repo") + self.app._validate_github_params("some_company", "some_project_name") + + def test_validate_github_params_valid_with_dots(self): + """Test validation accepts identifiers with dots.""" + # Should not raise any exception + self.app._validate_github_params("my.org", "my.repo") + self.app._validate_github_params("company.com", "project.name") + + def test_validate_github_params_reject_unix_path_owner(self): + """Test validation rejects Unix-style path in repo_owner.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params( + "/home/user/repos/project", "mcp-git" + ) + + def test_validate_github_params_reject_unix_path_name(self): + """Test validation rejects Unix-style path in repo_name.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params( + "MementoRC", "/home/user/repos/mcp-git" + ) + + def test_validate_github_params_reject_windows_path_owner(self): + """Test validation rejects Windows-style path in repo_owner.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params( + "C:\\Users\\user\\repos\\project", "mcp-git" + ) + + def test_validate_github_params_reject_windows_path_name(self): + """Test validation rejects Windows-style path in repo_name.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params( + "MementoRC", "C:\\Users\\user\\repos\\mcp-git" + ) + + def test_validate_github_params_reject_relative_path_owner(self): + """Test validation rejects relative path in repo_owner.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("../repos/project", "mcp-git") + + def test_validate_github_params_reject_relative_path_name(self): + """Test validation rejects relative path in repo_name.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("MementoRC", "./mcp-git") + + def test_validate_github_params_reject_owner_with_slash(self): + """Test validation rejects repo_owner containing forward slash.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("owner/something", "repo") + + def test_validate_github_params_reject_name_with_slash(self): + """Test validation rejects repo_name containing forward slash.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("owner", "repo/something") + + def test_validate_github_params_reject_owner_with_backslash(self): + """Test validation rejects repo_owner containing backslash.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("owner\\something", "repo") + + def test_validate_github_params_reject_name_with_backslash(self): + """Test validation rejects repo_name containing backslash.""" + with pytest.raises(ValueError, match="appears to be a file path"): + self.app._validate_github_params("owner", "repo\\something") + + def test_validate_github_params_error_message_clarity(self): + """Test that error messages clearly explain the issue.""" + with pytest.raises(ValueError) as exc_info: + self.app._validate_github_params( + "/home/memento/ClaudeCode/Servers/hexagonal-architecture/development", + "mcp-git" + ) + + error_msg = str(exc_info.value) + # Check that error message contains helpful information + assert "appears to be a file path" in error_msg + assert "GitHub API operations" in error_msg + assert "repo_owner" in error_msg or "repo_name" in error_msg + assert "'--repository'" in error_msg or "bound" in error_msg + + def test_validate_github_params_preserves_case(self): + """Test validation preserves case of identifiers.""" + # Should not raise and should preserve case + self.app._validate_github_params("MementoRC", "mcp-git") + self.app._validate_github_params("OWNER", "REPO") + self.app._validate_github_params("MiXeDCaSe", "RePoNaMe")