Fix npm dependency-confusion bug in MCP server default command - #4
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QyJoCc6YtjnErv6HKa6NPu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
python/src/graphkeeper/mcp_server.pydefaulted to runningnpx graphkeeper(missing the-clisuffix) wheneverGRAPHKEEPER_CLI_JSwas unset. The project's actual published npm package isgraphkeeper-cli; the bare namegraphkeeperis a completely unrelated, unaudited third-party package (graphkeeper@0.4.1, published by a different maintainer,anusbutt). Anyone following the documented MCP setup without a local-dev override would have silently installed and run that unrelated package's code instead of this project's CLI. Fixed the default command, updated the surrounding docstrings/comments to warn against dropping the-clisuffix, corrected the one stale prose reference inREADME.md's MCP section, and addedpython/tests/test_mcp_server.pyto regression-test the exact command string.normalize_file_arg(python/src/graphkeeper/query.py). Compared against the TypeScript original (src/query.ts), the Python port had two Windows-only bugs: (1)os.path.isabsreturnsFalsefor POSIX-style root-relative paths like/repo/src/a.pyon Windows (unlike Node'spath.isAbsolute, which returnsTrue), so the port silently skipped relativizing exactly the kind of absolute path an MCP client is likely to pass; (2)os.path.relpathraisesValueErrorwhen the file argument and the repo path are on different drives (e.g. repo onC:, file onD:), unlikepath.relative, which just returns the path unchanged in that case -- this crashed the query instead of degrading gracefully. Added a_is_absolutehelper matching Node's semantics and wrappedrelpathto fall back to the TS behavior onValueError. Added regression tests inpython/tests/test_query.py..github/workflows/codeql.ymlreferencedactions/checkout@v4andgithub/codeql-action/{init,autobuild,analyze}@v3by mutable tag. SHA-pinned all four to match the pinning style already used inpublish-pypi.yml.Test plan
python -m pytest tests/(frompython/): 82 passed, 3 failed -- the 3 failures are pre-existing and unrelated to this change (OSError: [WinError 1314], Windows requires elevated privilege/Developer Mode to create symlinks viaos.symlink; these tests exercise symlink-based path-traversal safety and fail identically onmain).npm run typecheck: clean.npm test(vitest): 75 passed, 3 failed -- same pre-existing Windows symlink-privilege limitation on the TS side (EPERM: operation not permitted, symlink), not caused by this change (nosrc/*.tsfiles were touched).Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01QyJoCc6YtjnErv6HKa6NPu