From 5da7ba548c7c2a4ef0bd92b436b6151fef19ca46 Mon Sep 17 00:00:00 2001 From: David Dworken Date: Wed, 7 Jan 2026 13:16:31 -0500 Subject: [PATCH] feat: add path validation for commit_files MCP tool (#796) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/mcp/github-file-ops-server.ts | 39 ++-- src/mcp/path-validation.ts | 64 ++++++ test/github-file-ops-path-validation.test.ts | 214 +++++++++++++++++++ 3 files changed, 300 insertions(+), 17 deletions(-) create mode 100644 src/mcp/path-validation.ts create mode 100644 test/github-file-ops-path-validation.test.ts diff --git a/src/mcp/github-file-ops-server.ts b/src/mcp/github-file-ops-server.ts index 9fcf00e..4d61621 100644 --- a/src/mcp/github-file-ops-server.ts +++ b/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, }, diff --git a/src/mcp/path-validation.ts b/src/mcp/path-validation.ts new file mode 100644 index 0000000..af15bf5 --- /dev/null +++ b/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 { + // 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; +} diff --git a/test/github-file-ops-path-validation.test.ts b/test/github-file-ops-path-validation.test.ts new file mode 100644 index 0000000..f2e991b --- /dev/null +++ b/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/); + }); + }); +});