From 3c35c6ace569d47f6e70f04e8df3fe83054bb4a9 Mon Sep 17 00:00:00 2001 From: Sourav Nandy Date: Mon, 24 Aug 2026 04:14:41 +0530 Subject: [PATCH] Fix npm dependency-confusion bug in MCP server default command python/src/graphkeeper/mcp_server.py defaulted to running `npx graphkeeper` (missing the `-cli` suffix) when no local-dev override was set. The published npm package for this project is `graphkeeper-cli`; the bare name `graphkeeper` resolves to a completely unrelated, unaudited third-party package by a different maintainer. Anyone following the documented MCP setup without GRAPHKEEPER_CLI_JS set would have silently installed and executed that unrelated package instead of this project's CLI. - python/src/graphkeeper/mcp_server.py: default command and docstrings now reference `graphkeeper-cli`, with an explicit warning against dropping the `-cli` suffix. - README.md: corrected the MCP section's prose reference to the underlying npm package name. - python/tests/test_mcp_server.py: new regression tests asserting the default command is `[npx, graphkeeper-cli]` and never contains the bare `graphkeeper` name. Also fixes two more issues found in a fresh audit pass: - python/src/graphkeeper/query.py: `normalize_file_arg` had two Windows-specific bugs versus the TypeScript original (src/query.ts): (1) `os.path.isabs` returns False for POSIX-style root-relative paths like /repo/src/a.py on Windows, unlike Node's `path.isAbsolute`, silently skipping relativization; (2) `os.path.relpath` raises ValueError when the file arg and repo path are on different drives, unlike `path.relative`, which crashed the query. Added `_is_absolute` to match Node's semantics and a try/except around `relpath` that falls back to leaving the path unchanged, matching the TS behavior. Added regression tests in test_query.py. - .github/workflows/codeql.yml: SHA-pinned actions/checkout and the github/codeql-action init/autobuild/analyze steps, which were on mutable version tags. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01QyJoCc6YtjnErv6HKa6NPu --- .github/workflows/codeql.yml | 8 +-- README.md | 5 +- python/src/graphkeeper/mcp_server.py | 25 +++++++-- python/src/graphkeeper/query.py | 41 +++++++++++++- python/tests/test_mcp_server.py | 84 ++++++++++++++++++++++++++++ python/tests/test_query.py | 38 ++++++++++++- 6 files changed, 185 insertions(+), 16 deletions(-) create mode 100644 python/tests/test_mcp_server.py diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 0b5ddcb..8aa8d30 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -24,17 +24,17 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Initialize CodeQL - uses: github/codeql-action/init@v3 + uses: github/codeql-action/init@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8 with: languages: ${{ matrix.language }} - name: Autobuild - uses: github/codeql-action/autobuild@v3 + uses: github/codeql-action/autobuild@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8 - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v3 + uses: github/codeql-action/analyze@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8 with: category: "/language:${{ matrix.language }}" diff --git a/README.md b/README.md index 5239e6d..a061304 100644 --- a/README.md +++ b/README.md @@ -283,8 +283,9 @@ Add it to your MCP client's config (for Claude Desktop, `claude_desktop_config.j } ``` -The server exposes one tool, `run`, that shells out to the published `graphkeeper` npm binary with -the given subcommand and arguments plus `--json`, and returns the parsed JSON result: +The server exposes one tool, `run`, that shells out to the published `graphkeeper-cli` npm package +(via `npx graphkeeper-cli`) with the given subcommand and arguments plus `--json`, and returns the +parsed JSON result: ``` run(["build", "."]) diff --git a/python/src/graphkeeper/mcp_server.py b/python/src/graphkeeper/mcp_server.py index 569eb86..1fee506 100644 --- a/python/src/graphkeeper/mcp_server.py +++ b/python/src/graphkeeper/mcp_server.py @@ -6,13 +6,20 @@ underlying CLI with --json appended, parses the JSON stdout, and returns it as the tool result. -Deliberately shells out to the Node/TypeScript CLI (`npx graphkeeper`) +Deliberately shells out to the Node/TypeScript CLI (`npx graphkeeper-cli`) rather than calling into this same Python package's own native `graphkeeper.cli` module. Per the template, the MCP wrapper's implementation language is independent of the CLI's: this keeps one wrapper shape reusable across the whole portfolio regardless of whether a given repo's CLI is Node -or Python, and it exercises the actual published `graphkeeper` npm binary -end to end rather than a parallel code path. +or Python, and it exercises the actual published `graphkeeper-cli` npm +package end to end rather than a parallel code path. + +NOTE ON THE PACKAGE NAME: the published npm package is `graphkeeper-cli` +(https://www.npmjs.com/package/graphkeeper-cli), not `graphkeeper`. A +completely unrelated, unaudited third-party package is published under the +bare name `graphkeeper` by a different maintainer -- `npx graphkeeper` +would silently install and run that package instead of this project's CLI +(a dependency-confusion trap). Always use the `-cli` suffix here. stdout is reserved for MCP's JSON-RPC framing, so anything this module logs goes to stderr. @@ -27,17 +34,23 @@ from mcp.server import MCPServer -# Local-test override: point at a built dist/cli.js instead of `npx graphkeeper`. +# Local-test override: point at a built dist/cli.js instead of `npx graphkeeper-cli`. # The npm package may not be globally linked on a dev machine, so testing # this wrapper against a repo checkout needs a direct `node ` command. -# Production default (no env var set) is `npx graphkeeper`. +# Production default (no env var set) is `npx graphkeeper-cli`. +# +# IMPORTANT: the published npm package name is `graphkeeper-cli`, NOT +# `graphkeeper`. `graphkeeper` (no suffix) is an unrelated third-party +# package from a different publisher -- using the bare name here would be a +# dependency-confusion bug, silently running someone else's code instead of +# this project's CLI. Do not drop the `-cli` suffix. _LOCAL_CLI_JS = os.environ.get("GRAPHKEEPER_CLI_JS") def _base_command() -> list[str]: if _LOCAL_CLI_JS: return ["node", _LOCAL_CLI_JS] - return ["npx", "graphkeeper"] + return ["npx", "graphkeeper-cli"] _TOOL_DESCRIPTION = """Run the GraphKeeper CLI to mine a local git repo's commit history for file-level co-change patterns and query the resulting knowledge graph. Call this when an agent needs to know which files tend to change together in a codebase (e.g. before editing a file, to see what else usually needs to change alongside it) or which functions call/are called by a given symbol. diff --git a/python/src/graphkeeper/query.py b/python/src/graphkeeper/query.py index d6a2e96..e71cfdd 100644 --- a/python/src/graphkeeper/query.py +++ b/python/src/graphkeeper/query.py @@ -13,13 +13,48 @@ from .types import CallsQueryResult, CoChangeQueryResult, CoChangeResultRow, GraphKeeperStore, GraphifyNode +def _is_absolute(path_str: str) -> bool: + """Cross-platform absolute-path check matching the semantics of Node's + `path.isAbsolute`, which the TypeScript original (src/query.ts) relies + on. + + On Windows, `os.path.isabs` disagrees with Node here for *root-relative* + paths that have no drive letter (e.g. "/repo/src/a.py" or + "\\repo\\src\\a.py"): Node's `path.isAbsolute` treats a leading path + separator as absolute (resolved against the current drive), but + `os.path.isabs` returns False for it, silently skipping relativization + for exactly the POSIX-style absolute paths an MCP client is likely to + pass. On POSIX this matches `os.path.isabs` exactly (leading "/"). + """ + if not path_str: + return False + if os.name == "nt": + first = path_str[0] + if first in ("\\", "/"): + return True + if len(path_str) > 2 and path_str[1] == ":" and path_str[2] in ("\\", "/"): + return True + return False + return path_str.startswith("/") + + def normalize_file_arg(store: GraphKeeperStore, file: str) -> str: """Normalizes a user-supplied file argument to the repo-relative form GraphKeeper stores paths in.""" normalized = file - if os.path.isabs(normalized): - rel = os.path.relpath(normalized, store.repo_path) - if not rel.startswith(".."): + if _is_absolute(normalized): + try: + rel = os.path.relpath(normalized, store.repo_path) + except ValueError: + # Windows-only: os.path.relpath raises when `normalized` and + # `store.repo_path` are on different drives (e.g. repo on C:, + # file arg on D:) -- there is no relative path between them. + # The TypeScript original (path.relative) doesn't throw in this + # case; it just returns the absolute target path unchanged, + # which also fails the `not rel.startswith("..")` check below. + # Mirror that by leaving `normalized` as-is instead of crashing. + rel = None + if rel is not None and not rel.startswith(".."): normalized = rel normalized = re.sub(r"^\./", "", normalized) return normalized.replace(os.sep, "/") diff --git a/python/tests/test_mcp_server.py b/python/tests/test_mcp_server.py new file mode 100644 index 0000000..e61c828 --- /dev/null +++ b/python/tests/test_mcp_server.py @@ -0,0 +1,84 @@ +"""Regression tests guarding against the dependency-confusion bug where the +MCP wrapper shelled out to `npx graphkeeper` (no `-cli` suffix) instead of +the actually-published `npx graphkeeper-cli`. `graphkeeper` (bare name) is +an unrelated third-party npm package from a different publisher; running it +instead of this project's CLI would silently execute unaudited code. + +These tests only exercise this module's own `_base_command()` logic, not +the MCP SDK itself, so they stub `mcp.server.MCPServer` when it isn't +importable rather than skipping outright -- this project's pyproject.toml +requires `mcp[cli]>=2.0.0`, and some local/shared environments may still +have an older `mcp` pinned for unrelated projects that predates that name, +which shouldn't stop this dependency-confusion regression test from running. +""" +from __future__ import annotations + +import importlib + +import pytest + +pytest.importorskip("mcp", reason="optional `mcp` extra not installed") + + +def _ensure_mcpserver_importable() -> None: + try: + from mcp.server import MCPServer # noqa: F401 + + return + except ImportError: + pass + + import mcp.server as real_server_module + + class _StubMCPServer: + """Minimal stand-in with just enough surface for mcp_server.py's + module-level `mcp = MCPServer(name=...)` and `@mcp.tool(...)` to + execute without error.""" + + def __init__(self, name: str) -> None: + self.name = name + + def tool(self, description: str = ""): + def decorator(func): + return func + + return decorator + + real_server_module.MCPServer = _StubMCPServer + + +_ensure_mcpserver_importable() + + +@pytest.fixture() +def mcp_server_module(monkeypatch): + monkeypatch.delenv("GRAPHKEEPER_CLI_JS", raising=False) + import graphkeeper.mcp_server as module + + return importlib.reload(module) + + +class TestBaseCommand: + def test_default_command_uses_published_cli_package_name(self, mcp_server_module): + # The published npm package is `graphkeeper-cli`, not `graphkeeper`. + # `npx graphkeeper` (missing the `-cli` suffix) resolves to a + # completely different, unrelated npm package -- this must never + # be the default. + assert mcp_server_module._base_command() == ["npx", "graphkeeper-cli"] + + def test_default_command_never_omits_cli_suffix(self, mcp_server_module): + command = mcp_server_module._base_command() + assert "graphkeeper" not in command, ( + "the bare `graphkeeper` npm package is an unrelated third-party " + "package; the CLI must always be invoked as `graphkeeper-cli`" + ) + + def test_local_override_still_takes_precedence(self, mcp_server_module, monkeypatch, tmp_path): + cli_js = tmp_path / "cli.js" + monkeypatch.setenv("GRAPHKEEPER_CLI_JS", str(cli_js)) + module = importlib.reload(mcp_server_module) + try: + assert module._base_command() == ["node", str(cli_js)] + finally: + monkeypatch.delenv("GRAPHKEEPER_CLI_JS", raising=False) + importlib.reload(module) diff --git a/python/tests/test_query.py b/python/tests/test_query.py index e9900d8..36fd022 100644 --- a/python/tests/test_query.py +++ b/python/tests/test_query.py @@ -1,10 +1,12 @@ """Ported from test/query.test.ts.""" from __future__ import annotations +import os from datetime import datetime, timezone from typing import Optional +from unittest.mock import patch -from graphkeeper.query import find_graphify_node, normalize_file_arg, query_calls, query_co_change +from graphkeeper.query import _is_absolute, find_graphify_node, normalize_file_arg, query_calls, query_co_change from graphkeeper.types import CoChangeEdge, GraphifyEnrichment, GraphKeeperStore @@ -38,6 +40,40 @@ def test_relativizes_absolute_path_under_repo_path(self): def test_leaves_absolute_path_outside_repo_path_unchanged(self): assert normalize_file_arg(_make_store(repo_path="/repo"), "/other/src/a.py") == "/other/src/a.py" + def test_does_not_crash_when_relpath_cannot_compute_a_relative_path(self): + # Windows-only in practice: os.path.relpath raises ValueError when + # the two paths are on different drives (e.g. repo on C:, file arg + # on D:), since there is no relative path between them. The + # TypeScript original (path.relative) doesn't throw in that case -- + # it just returns the absolute target path unchanged. Force + # `_is_absolute` and mock os.path.relpath so this regression test + # exercises that branch on every platform, not just Windows, and + # use forward slashes throughout so the os.sep normalization at the + # end of the function is a no-op regardless of host OS. + with patch("graphkeeper.query._is_absolute", return_value=True), patch.object( + os.path, "relpath", side_effect=ValueError("path is on mount 'D:', start on mount 'C:'") + ): + result = normalize_file_arg(_make_store(repo_path="C:/repo"), "D:/other/file.py") + assert result == "D:/other/file.py" + + +class TestIsAbsolute: + """`_is_absolute` exists because `os.path.isabs` disagrees with Node's + `path.isAbsolute` (which the TS original uses) on Windows for + root-relative paths with no drive letter -- e.g. `os.path.isabs` + returns False for "/repo/src/a.py" on Windows, so the un-fixed Python + port silently skipped relativizing exactly this kind of path.""" + + def test_posix_style_root_relative_path_is_absolute(self): + assert _is_absolute("/repo/src/a.py") is True + + def test_relative_path_is_not_absolute(self): + assert _is_absolute("src/a.py") is False + assert _is_absolute("./src/a.py") is False + + def test_empty_string_is_not_absolute(self): + assert _is_absolute("") is False + class TestQueryCoChange: def test_finds_co_change_partners_ranked_by_count(self):