feat: add path validation for commit_files MCP tool (#796)

Add validatePathWithinRepo helper to ensure file paths resolve within the repository root directory. This hardens the commit_files tool by validating paths before file operations.

Changes:
- Add src/mcp/path-validation.ts with async path validation using realpath
- Update commit_files to validate all paths before reading files
- Prevent symlink-based path escapes by resolving real paths
- Add comprehensive test coverage including symlink attack scenarios

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>

src/mcp/github-file-ops-server.ts

@@ -4,11 +4,12 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
 import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
 import { z } from "zod";
 import { readFile, stat } from "fs/promises";
-import { join } from "path";
+import { resolve } from "path";
 import { constants } from "fs";
 import fetch from "node-fetch";
 import { GITHUB_API_URL } from "../github/api/config";
 import { retryWithBackoff } from "../utils/retry";
+import { validatePathWithinRepo } from "./path-validation";
 
 type GitHubRef = {
   object: {
@@ -213,12 +214,18 @@ server.tool(
         throw new Error("GITHUB_TOKEN environment variable is required");
       }
 
-      const processedFiles = files.map((filePath) => {
-        if (filePath.startsWith("/")) {
-          return filePath.slice(1);
-        }
-        return filePath;
-      });
+      // Validate all paths are within repository root and get full/relative paths
+      const resolvedRepoDir = resolve(REPO_DIR);
+      const validatedFiles = await Promise.all(
+        files.map(async (filePath) => {
+          const fullPath = await validatePathWithinRepo(filePath, REPO_DIR);
+          // Calculate the relative path for the git tree entry
+          // Use the original filePath (normalized) for the git path, not the symlink-resolved path
+          const normalizedPath = resolve(resolvedRepoDir, filePath);
+          const relativePath = normalizedPath.slice(resolvedRepoDir.length + 1);
+          return { fullPath, relativePath };
+        }),
+      );
 
       // 1. Get the branch reference (create if doesn't exist)
       const baseSha = await getOrCreateBranchRef(
@@ -247,18 +254,14 @@ server.tool(
 
       // 3. Create tree entries for all files
       const treeEntries = await Promise.all(
-        processedFiles.map(async (filePath) => {
-          const fullPath = filePath.startsWith("/")
-            ? filePath
-            : join(REPO_DIR, filePath);
-
+        validatedFiles.map(async ({ fullPath, relativePath }) => {
           // Get the proper file mode based on file permissions
           const fileMode = await getFileMode(fullPath);
 
           // Check if file is binary (images, etc.)
           const isBinaryFile =
             /\.(png|jpg|jpeg|gif|webp|ico|pdf|zip|tar|gz|exe|bin|woff|woff2|ttf|eot)$/i.test(
-              filePath,
+              relativePath,
             );
 
           if (isBinaryFile) {
@@ -284,7 +287,7 @@ server.tool(
             if (!blobResponse.ok) {
               const errorText = await blobResponse.text();
               throw new Error(
-                `Failed to create blob for ${filePath}: ${blobResponse.status} - ${errorText}`,
+                `Failed to create blob for ${relativePath}: ${blobResponse.status} - ${errorText}`,
               );
             }
 
@@ -292,7 +295,7 @@ server.tool(
 
             // Return tree entry with blob SHA
             return {
-              path: filePath,
+              path: relativePath,
               mode: fileMode,
               type: "blob",
               sha: blobData.sha,
@@ -301,7 +304,7 @@ server.tool(
             // For text files, include content directly in tree
             const content = await readFile(fullPath, "utf-8");
             return {
-              path: filePath,
+              path: relativePath,
               mode: fileMode,
               type: "blob",
               content: content,
@@ -421,7 +424,9 @@ server.tool(
           author: newCommitData.author.name,
           date: newCommitData.author.date,
         },
-        files: processedFiles.map((path) => ({ path })),
+        files: validatedFiles.map(({ relativePath }) => ({
+          path: relativePath,
+        })),
         tree: {
           sha: treeData.sha,
         },

src/mcp/path-validation.ts

@@ -0,0 +1,64 @@
+import { realpath } from "fs/promises";
+import { resolve, sep } from "path";
+
+/**
+ * Validates that a file path resolves within the repository root.
+ * Prevents path traversal attacks via "../" sequences and symlinks.
+ * @param filePath - The file path to validate (can be relative or absolute)
+ * @param repoRoot - The repository root directory
+ * @returns The resolved absolute path (with symlinks resolved) if valid
+ * @throws Error if the path resolves outside the repository root
+ */
+export async function validatePathWithinRepo(
+  filePath: string,
+  repoRoot: string,
+): Promise<string> {
+  // First resolve the path string (handles .. and . segments)
+  const initialPath = resolve(repoRoot, filePath);
+
+  // Resolve symlinks to get the real path
+  // This prevents symlink attacks where a link inside the repo points outside
+  let resolvedRoot: string;
+  let resolvedPath: string;
+
+  try {
+    resolvedRoot = await realpath(repoRoot);
+  } catch {
+    throw new Error(`Repository root '${repoRoot}' does not exist`);
+  }
+
+  try {
+    resolvedPath = await realpath(initialPath);
+  } catch {
+    // File doesn't exist yet - fall back to checking the parent directory
+    // This handles the case where we're creating a new file
+    const parentDir = resolve(initialPath, "..");
+    try {
+      const resolvedParent = await realpath(parentDir);
+      if (
+        resolvedParent !== resolvedRoot &&
+        !resolvedParent.startsWith(resolvedRoot + sep)
+      ) {
+        throw new Error(
+          `Path '${filePath}' resolves outside the repository root`,
+        );
+      }
+      // Parent is valid, return the initial path since file doesn't exist yet
+      return initialPath;
+    } catch {
+      throw new Error(
+        `Path '${filePath}' resolves outside the repository root`,
+      );
+    }
+  }
+
+  // Path must be within repo root (or be the root itself)
+  if (
+    resolvedPath !== resolvedRoot &&
+    !resolvedPath.startsWith(resolvedRoot + sep)
+  ) {
+    throw new Error(`Path '${filePath}' resolves outside the repository root`);
+  }
+
+  return resolvedPath;
+}

test/github-file-ops-path-validation.test.ts

@@ -0,0 +1,214 @@
+import { describe, expect, it, beforeAll, afterAll } from "bun:test";
+import { validatePathWithinRepo } from "../src/mcp/path-validation";
+import { resolve } from "path";
+import { mkdir, writeFile, symlink, rm, realpath } from "fs/promises";
+import { tmpdir } from "os";
+
+describe("validatePathWithinRepo", () => {
+  // Use a real temp directory for tests that need filesystem access
+  let testDir: string;
+  let repoRoot: string;
+  let outsideDir: string;
+  // Real paths after symlink resolution (e.g., /tmp -> /private/tmp on macOS)
+  let realRepoRoot: string;
+
+  beforeAll(async () => {
+    // Create test directory structure
+    testDir = resolve(tmpdir(), `path-validation-test-${Date.now()}`);
+    repoRoot = resolve(testDir, "repo");
+    outsideDir = resolve(testDir, "outside");
+
+    await mkdir(repoRoot, { recursive: true });
+    await mkdir(resolve(repoRoot, "src"), { recursive: true });
+    await mkdir(outsideDir, { recursive: true });
+
+    // Create test files
+    await writeFile(resolve(repoRoot, "file.txt"), "inside repo");
+    await writeFile(resolve(repoRoot, "src", "main.js"), "console.log('hi')");
+    await writeFile(resolve(outsideDir, "secret.txt"), "sensitive data");
+
+    // Get real paths after symlink resolution
+    realRepoRoot = await realpath(repoRoot);
+  });
+
+  afterAll(async () => {
+    // Cleanup
+    await rm(testDir, { recursive: true, force: true });
+  });
+
+  describe("valid paths", () => {
+    it("should accept simple relative paths", async () => {
+      const result = await validatePathWithinRepo("file.txt", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "file.txt"));
+    });
+
+    it("should accept nested relative paths", async () => {
+      const result = await validatePathWithinRepo("src/main.js", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "src/main.js"));
+    });
+
+    it("should accept paths with single dot segments", async () => {
+      const result = await validatePathWithinRepo("./src/main.js", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "src/main.js"));
+    });
+
+    it("should accept paths that use .. but resolve inside repo", async () => {
+      // src/../file.txt resolves to file.txt which is still inside repo
+      const result = await validatePathWithinRepo("src/../file.txt", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "file.txt"));
+    });
+
+    it("should accept absolute paths within the repo root", async () => {
+      const absolutePath = resolve(repoRoot, "file.txt");
+      const result = await validatePathWithinRepo(absolutePath, repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "file.txt"));
+    });
+
+    it("should accept the repo root itself", async () => {
+      const result = await validatePathWithinRepo(".", repoRoot);
+      expect(result).toBe(realRepoRoot);
+    });
+
+    it("should handle new files (non-existent) in valid directories", async () => {
+      const result = await validatePathWithinRepo("src/newfile.js", repoRoot);
+      // For non-existent files, we validate the parent but return the initial path
+      // (can't realpath a file that doesn't exist yet)
+      expect(result).toBe(resolve(repoRoot, "src/newfile.js"));
+    });
+  });
+
+  describe("path traversal attacks", () => {
+    it("should reject simple parent directory traversal", async () => {
+      await expect(
+        validatePathWithinRepo("../outside/secret.txt", repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+
+    it("should reject deeply nested parent directory traversal", async () => {
+      await expect(
+        validatePathWithinRepo("../../../etc/passwd", repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+
+    it("should reject traversal hidden within path", async () => {
+      await expect(
+        validatePathWithinRepo("src/../../outside/secret.txt", repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+
+    it("should reject traversal at the end of path", async () => {
+      await expect(
+        validatePathWithinRepo("src/../..", repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+
+    it("should reject absolute paths outside the repo root", async () => {
+      await expect(
+        validatePathWithinRepo("/etc/passwd", repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+
+    it("should reject absolute paths to sibling directories", async () => {
+      await expect(
+        validatePathWithinRepo(resolve(outsideDir, "secret.txt"), repoRoot),
+      ).rejects.toThrow(/resolves outside the repository root/);
+    });
+  });
+
+  describe("symlink attacks", () => {
+    it("should reject symlinks pointing outside the repo", async () => {
+      // Create a symlink inside the repo that points to a file outside
+      const symlinkPath = resolve(repoRoot, "evil-link");
+      await symlink(resolve(outsideDir, "secret.txt"), symlinkPath);
+
+      try {
+        // The symlink path looks like it's inside the repo, but points outside
+        await expect(
+          validatePathWithinRepo("evil-link", repoRoot),
+        ).rejects.toThrow(/resolves outside the repository root/);
+      } finally {
+        await rm(symlinkPath, { force: true });
+      }
+    });
+
+    it("should reject symlinks to parent directories", async () => {
+      // Create a symlink to the parent directory
+      const symlinkPath = resolve(repoRoot, "parent-link");
+      await symlink(testDir, symlinkPath);
+
+      try {
+        await expect(
+          validatePathWithinRepo("parent-link/outside/secret.txt", repoRoot),
+        ).rejects.toThrow(/resolves outside the repository root/);
+      } finally {
+        await rm(symlinkPath, { force: true });
+      }
+    });
+
+    it("should accept symlinks that resolve within the repo", async () => {
+      // Create a symlink inside the repo that points to another file inside
+      const symlinkPath = resolve(repoRoot, "good-link");
+      await symlink(resolve(repoRoot, "file.txt"), symlinkPath);
+
+      try {
+        const result = await validatePathWithinRepo("good-link", repoRoot);
+        // Should resolve to the actual file location
+        expect(result).toBe(resolve(realRepoRoot, "file.txt"));
+      } finally {
+        await rm(symlinkPath, { force: true });
+      }
+    });
+
+    it("should reject directory symlinks that escape the repo", async () => {
+      // Create a symlink to outside directory
+      const symlinkPath = resolve(repoRoot, "escape-dir");
+      await symlink(outsideDir, symlinkPath);
+
+      try {
+        await expect(
+          validatePathWithinRepo("escape-dir/secret.txt", repoRoot),
+        ).rejects.toThrow(/resolves outside the repository root/);
+      } finally {
+        await rm(symlinkPath, { force: true });
+      }
+    });
+  });
+
+  describe("edge cases", () => {
+    it("should handle empty path (current directory)", async () => {
+      const result = await validatePathWithinRepo("", repoRoot);
+      expect(result).toBe(realRepoRoot);
+    });
+
+    it("should handle paths with multiple consecutive slashes", async () => {
+      const result = await validatePathWithinRepo("src//main.js", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "src/main.js"));
+    });
+
+    it("should handle paths with trailing slashes", async () => {
+      const result = await validatePathWithinRepo("src/", repoRoot);
+      expect(result).toBe(resolve(realRepoRoot, "src"));
+    });
+
+    it("should reject prefix attack (repo root as prefix but not parent)", async () => {
+      // Create a sibling directory with repo name as prefix
+      const evilDir = repoRoot + "-evil";
+      await mkdir(evilDir, { recursive: true });
+      await writeFile(resolve(evilDir, "file.txt"), "evil");
+
+      try {
+        await expect(
+          validatePathWithinRepo(resolve(evilDir, "file.txt"), repoRoot),
+        ).rejects.toThrow(/resolves outside the repository root/);
+      } finally {
+        await rm(evilDir, { recursive: true, force: true });
+      }
+    });
+
+    it("should throw error for non-existent repo root", async () => {
+      await expect(
+        validatePathWithinRepo("file.txt", "/nonexistent/repo"),
+      ).rejects.toThrow(/does not exist/);
+    });
+  });
+});