Skip to content

SECURITY_REVIEW.md

Latest commit

 

History

History
218 lines (164 loc) · 10.6 KB

SECURITY_REVIEW.md

File metadata and controls

218 lines (164 loc) · 10.6 KB
Raw

Security & Quality Review — gitpulse-mcp

Date: 2026-05-22 Reviewer: Autonomous application-security engineer Scope: Full source (src/**), tests, CI, dependencies, docs. Method: Manual code audit + dynamic exploitation against a probe git repo + npm audit. Tool stack: Node.js + TypeScript MCP stdio server that shells out to the local git binary.

Severity legend: Critical / High / Medium / Low / Info.

Summary of findings

# Severity Title File
1 Critical Git argument/option injection via ref → arbitrary file write & read-only bypass src/tools/recentChanges.ts:54
2 High Git argument/option injection via ref in git blame → arbitrary file read & write src/tools/blameSummary.ts:84, src/tools/whoTouchedLines.ts:55
3 Medium changesSince ref injection only blocked incidentally by ^{commit} suffix; no explicit ref validation src/tools/changesSince.ts:36-46, src/git/repo.ts:43-53
4 Medium No centralized defense-in-depth against option injection in the exec layer src/git/exec.ts:40
5 Medium Dev-dependency CVEs (vitest → vite/esbuild path-traversal + dev-server SSRF) package.json (devDeps)
6 Low Unbounded process fan-out in currentLineCounts (Promise.all over up to ~300 git show spawns) src/tools/hotspots.ts:151
7 Low normalizeRenamePath uses while (regex.test) { replace } (O(n²) rescans) on git-controlled path strings src/git/parse.ts:155-166
8 Low Error results echo raw git stderr (absolute paths / config) back to the MCP client src/lib/format.ts:23, src/git/exec.ts:67
9 Info No upper bound / validation on free-text string params (since, author, query) beyond git's own parsing multiple tools
10 Info README/FAQ claims "safe from command injection" — true for shell injection but was false for argument injection (finding #1/#2) README.md:220-221

Counts: 1 Critical, 1 High, 3 Medium, 3 Low, 2 Info (10 total).

Detailed findings

1. [Critical] Git argument/option injection via ref → arbitrary file write + read-only bypass

File: src/tools/recentChanges.ts:54 (if (args.ref) gitArgs.push(args.ref);)

Impact: recentChanges pushes the untrusted ref parameter onto the git log argument array as a bare positional, before any -- separator and with no validation that it does not begin with -. execFile correctly prevents shell injection, but it does not prevent git option injection: a value that looks like a git flag is still interpreted by git as a flag.

A caller can pass ref = "--output=/abs/path/PWNED.txt". git log --output=<file> writes its output to an arbitrary filesystem path. This:

  • Breaks the project's headline "read-only by design" guarantee (the tool now writes files).
  • Is an arbitrary file write primitive (content controllable by also choosing the pretty-format/other injected flags), usable to clobber configs, drop files, etc., anywhere the server process can write.

Proof (run against a real repo):

git log --pretty=format:x --output=/tmp/PWNED.txt -n1   # exit 0
ls -la /tmp/PWNED.txt                                    # file created

Fix: Validate every ref-position argument with a strict allowlist that rejects leading - (and other dangerous shapes), and add git's --end-of-options sentinel plus a -- guard so git can never interpret user values as options. Implemented a shared validateRev() in src/git/repo.ts and applied it in every tool that takes a ref; also reordered args so flags precede --end-of-options.

2. [High] Git argument/option injection via ref in git blame → arbitrary file read + write

Files: src/tools/blameSummary.ts:84, src/tools/whoTouchedLines.ts:55 (if (args.ref) blameArgs.push(args.ref);)

Impact: Same class as #1 but via git blame, which exposes options that read arbitrary files. ref = "--contents=/etc/passwd" (or any absolute path) makes git blame --contents=<file> -- <path> attribute and echo lines sourced from an arbitrary external file, an information-disclosure / path-traversal primitive. Other blame options (e.g. --output) are also reachable, enabling file writes. Rated High rather than Critical because the leaked content is filtered through blame's porcelain output, but it still reads files outside the repo.

Proof:

git blame --line-porcelain --contents=/tmp/secret.txt -- a.txt
# output attributes lines to "External file (--contents)" and includes its content

Fix: Same validateRev() + --end-of-options hardening applied to both blame tools.

3. [Medium] changesSince relies on incidental ^{commit} protection

Files: src/tools/changesSince.ts:36-46, src/git/repo.ts:43-53

Impact: changesSince validates ref/to through refExists(), which runs git rev-parse --verify --quiet "<ref>^{commit}". Because the ^{commit} suffix is appended, most option-injection tokens become invalid and are rejected — but this is incidental, not a deliberate control, and the constructed range "<base>..<target>" is later passed to git log/git diff as a bare positional. A future refactor (or a ref crafted to survive the suffix) could reintroduce injection. There is no explicit "must not start with -" check.

Fix: Added explicit validateRev() on ref/to before any git call, and added --end-of-options before the range in the log/diff invocations.

4. [Medium] No defense-in-depth in the central exec layer

File: src/git/exec.ts:40

Impact: All injection protection lived in the individual tools. A single missed call site (as in #1/#2) defeats it. There was no last line of defense in the one place every git call funnels through.

Fix: git() now rejects, by default, any leading-dash positional argument that is not a recognized safe flag, unless the caller explicitly opts in via a known-safe flag list. This is belt-and-suspenders: even if a tool forgets to validate, the exec layer refuses to pass an unexpected --prefixed token. (See assertNoArgInjection in exec.ts.)

5. [Medium] Dev-dependency CVEs (vitest → vite/esbuild)

File: package.json (devDependencies)

Impact: npm audit reports 5 moderate advisories, all transitive under vitest@2:

  • esbuild ≤0.24.2 — dev server lets any website read responses (GHSA-67mh-4wv8-2f99).
  • vite ≤6.4.1 — path traversal in optimized-deps .map handling (GHSA-4w7w-66w2-5vf9).
  • @vitest/mocker, vite-node, vitest pulled along.

These are dev/test-only and never shipped (the published package contains only dist/, README, LICENSE, CHANGELOG), so end-user exposure is nil. Still resolved to keep CI clean and the supply chain healthy.

Fix: Upgrade vitest to ^4 (major) which pulls fixed vite/esbuild. Verified the suite still passes and npm audit reports 0 vulnerabilities.

6. [Low] Unbounded process fan-out in currentLineCounts

File: src/tools/hotspots.ts:151

Impact: Promise.all(paths.map(... git show ...)) spawns one git show child process per candidate path concurrently. candidates is bounded to max(top*3, top) with top ≤ 100, so up to ~300 simultaneous git processes — a mild resource-exhaustion / thundering-herd risk on large requests.

Fix: Bounded the concurrency with a small worker pool (default 16) so memory and process count stay bounded regardless of top.

7. [Low] O(n²) rescans in normalizeRenamePath

File: src/git/parse.ts:155-166

Impact: while (brace.test(p)) { p = p.replace(brace, …) } re-tests from the start of the string each iteration; on adversarial git path strings this is quadratic. Input is git-controlled (rename notation in numstat) and bounded by maxBuffer, so it is not a practical DoS, but it is fragile. Not classic exponential ReDoS (the lazy [^}]*? classes cannot catastrophically backtrack).

Fix: Rewrote to a single global-regex replace pass (linear) instead of the test/replace loop. Behavior preserved (covered by existing parse.test.ts cases, plus a new nested-brace regression test).

8. [Low] Error results echo raw git stderr to the client

Files: src/lib/format.ts:23, src/git/exec.ts:67

Impact: GitError includes raw git stderr, which fail() returns to the MCP client. stderr can contain absolute filesystem paths and local config hints. For a local developer tool this is low-impact (the client already chose the repo), but it is mild information disclosure.

Resolution: Accepted as-is for now — surfacing git's own diagnostic is the expected, useful behavior for a local developer tool, and the client already has filesystem context. Documented here as an intentional, low-risk choice. (No secrets beyond what the operator already controls are exposed.)

9. [Info] No length/shape bounds on free-text string params

Files: recentChanges.ts, hotspots.ts, ownership.ts, searchCommits.ts

Impact: since, author, query are forwarded to git via the safe --flag=value single-argument form (verified not injectable), and numeric params are already int-bounded by zod. Free-text values are bounded only by git's own parsing and the per-call timeout. No practical vulnerability; noted for completeness.

Resolution: No change required. The =value argument form is injection-safe (confirmed dynamically) and timeouts cap runtime.

10. [Info] README injection-safety claim was incomplete

File: README.md:220-221

Impact: The FAQ stated the tool is "safe from command injection" because it uses execFile with no shell. That is true for shell injection but, prior to this review, false for git argument/option injection (findings #1/#2).

Fix: With #1–#4 fixed the claim is now accurate; clarified the wording to note that arguments are also validated against option injection, not only shell injection.

Verification performed (post-fix)

  • npm ci — clean install.
  • npm run build — TypeScript compiles with no errors (strict, noUncheckedIndexedAccess).
  • npm run lint — eslint clean.
  • npm test — full vitest suite green, including new regression tests for findings #1, #2, #3, #7.
  • npm audit — 0 vulnerabilities after the vitest upgrade.
  • npm run smoke — end-to-end MCP stdio smoke test green (lists 8 tools, calls all).
  • Re-ran the exploit payloads from #1/#2 against the patched tools and confirmed they are now rejected with a clean isError result (no file written, no external file read).