feat: add path validation for commit_files MCP tool

[ddworken]

Summary

• Add validatePathWithinRepo helper to ensure file paths resolve within the repository root
• Hardens the commit_files tool by validating paths before file operations
• Uses realpath to resolve symlinks, preventing symlink-based path escapes
• Adds comprehensive test coverage (22 tests) including symlink attack scenarios

Changes

• src/mcp/path-validation.ts: New async path validation utility
• src/mcp/github-file-ops-server.ts: Updated to use path validation
• test/github-file-ops-path-validation.test.ts: Comprehensive test suite

Test plan

• All 22 path validation tests pass
• Typecheck passes (no new errors)
• Format check passes

🤖 Generated with Claude Code

[claude]

CRITICAL: Time-of-Check-Time-of-Use (TOCTOU) Race Condition

The validation occurs here, but the actual file read happens later in github-file-ops-server.ts (lines 269, 305). Between validation and use, an attacker could replace a legitimate file with a symlink to a sensitive location.

Attack scenario:

// 1. Validation passes for legitimate file
await validatePathWithinRepo("src/config.js", repoRoot); 
// 2. RACE WINDOW: Attacker replaces file
//    rm src/config.js && ln -s /etc/passwd src/config.js
// 3. File operation reads /etc/passwd
const content = await readFile(fullPath, "utf-8");

Recommendation: Use file descriptors with O_NOFOLLOW flag:

import { open, constants as fsConstants } from 'fs/promises';

// After validation, open without following symlinks
const fd = await open(fullPath, fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW);
const content = await fd.readFile('utf-8');
await fd.close();

CWE-367: Time-of-check Time-of-use Race Condition

[claude]

CRITICAL: Redundant realpath() Calls - Performance Issue

validatePathWithinRepo() calls realpath(REPO_DIR) for every file in this Promise.all loop. For a 50-file commit, this means 50 identical filesystem operations resolving the same directory.

Performance impact:

• O(N) redundant operations where N = number of files
• Each realpath involves kernel syscalls (~1-5ms each)
• For 50 files: ~50-250ms wasted

Recommendation: Resolve repo root once before the loop:

const resolvedRepoDir = resolve(REPO_DIR);
const resolvedRepoRoot = await realpath(REPO_DIR); // Once, outside the loop

const validatedFiles = await Promise.all(
  files.map(async (filePath) => {
    // Pass pre-resolved root to avoid redundant realpath calls
    const fullPath = await validatePathWithinRepo(filePath, REPO_DIR, resolvedRepoRoot);
    const normalizedPath = resolve(resolvedRepoDir, filePath);
    const relativePath = normalizedPath.slice(resolvedRepoDir.length + 1);
    return { fullPath, relativePath };
  }),
);

You'll need to update validatePathWithinRepo to accept an optional resolvedRepoRoot parameter.

[claude]

Missing Test Coverage: Chained Symlinks

The tests validate single-hop symlinks, but don't cover chained symlinks that eventually escape:

repo/link1 → repo/link2 → repo/link3 → /outside/secret.txt

While realpath() should resolve this, there's no explicit test validation.

Recommended test:

it("should reject chained symlinks that escape the repo", async () => {
  const link1 = resolve(repoRoot, "link1");
  const link2 = resolve(repoRoot, "link2");
  const link3 = resolve(repoRoot, "link3");
  
  await symlink(link2, link1);
  await symlink(link3, link2);
  await symlink(resolve(outsideDir, "secret.txt"), link3);
  
  try {
    await expect(
      validatePathWithinRepo("link1", repoRoot)
    ).rejects.toThrow(/resolves outside the repository root/);
  } finally {
    await rm(link1, { force: true });
    await rm(link2, { force: true });
    await rm(link3, { force: true });
  }
});

[claude]

Documentation Inaccuracy: Return Value Behavior

The JSDoc states symlinks are "resolved," but the implementation has two different behaviors:

• Existing files (line 30-31): Returns resolvedPath with symlinks fully resolved via realpath()
• Non-existent files (line 47): Returns initialPath with symlinks NOT resolved

Suggested fix:

* @returns The resolved absolute path if valid. For existing files, symlinks are 
*          fully resolved to their targets. For non-existent files, returns the 
*          normalized path without symlink resolution (since realpath cannot resolve 
*          paths that don't exist yet).

[claude]

Comprehensive Code Review - PR #796

I've completed a thorough review using specialized agents for security, performance, code quality, test coverage, and documentation. Here are the key findings:

---

🔴 Critical Issues

1. TOCTOU Race Condition (Security)

The path validation occurs separately from file operations, creating a race window where an attacker could replace a validated file with a symlink. See inline comment on path-validation.ts:28.

2. delete_files Tool Missing Path Validation (Security)

The delete_files tool (lines 482-496 in github-file-ops-server.ts) does NOT use validatePathWithinRepo(), making it vulnerable to:

• Path traversal attacks (../../../etc/passwd)
• Symlink-based escapes

Impact: Arbitrary file deletion outside repository boundaries.

Recommendation: Apply the same validatePathWithinRepo() validation to delete_files and add corresponding test coverage.

3. Performance: Redundant realpath() Calls

validatePathWithinRepo() calls realpath(REPO_DIR) for every file, resulting in N redundant filesystem operations for N-file commits. For 50 files, this wastes ~50-250ms. See inline comment on github-file-ops-server.ts:219-228.

---

🟡 High Priority Issues

4. Symlink File Mode Handling

getFileMode() at line 168 uses stat() which follows symlinks. After realpath() resolution, it will never detect symlinks. This means symlinks are automatically dereferenced, and Git will commit the target content rather than the symlink itself.

Recommendation: Use lstat() before path resolution, or explicitly reject symlinks for security.

5. Missing Test Coverage

• No tests for chained symlinks (link1 → link2 → link3 → outside)
• No tests for circular symlinks
• No tests for delete_files path validation
• No integration tests for end-to-end attack scenarios

See inline comment on test/github-file-ops-path-validation.test.ts:117-129 for chained symlink test recommendation.

---

📝 Documentation & Code Quality

6. JSDoc Inaccuracy

The return value documentation doesn't reflect that non-existent files return unresolved paths. See inline comment on path-validation.ts:9-10.

7. Complex Error Handling

Nested try-catch blocks (lines 30-53) swallow original error context, making debugging difficult. Consider extracting to separate helper functions and preserving error details.

---

✅ Positive Aspects

• Excellent test coverage for basic scenarios (22 tests)
• Correct use of realpath() for symlink resolution
• Good security approach with path separator checks
• Clear documentation and well-organized code structure

---

📋 Recommended Action Items

Must fix before merge:

1. Add path validation to delete_files tool
2. Optimize performance by caching realpath(REPO_DIR) result
3. Decide on symlink handling policy and implement consistently

Should fix:
4. Add test coverage for chained/circular symlinks
5. Fix JSDoc accuracy
6. Consider TOCTOU mitigation with O_NOFOLLOW flag

Nice to have:
7. Refactor nested error handling for clarity
8. Add integration tests for attack scenarios

---

Let me know if you'd like me to elaborate on any of these findings or help implement the recommended fixes!