diff --git a/src/filesystem/__tests__/path-validation.test.ts b/src/filesystem/__tests__/path-validation.test.ts index 81ad247ee2..23c49f69ac 100644 --- a/src/filesystem/__tests__/path-validation.test.ts +++ b/src/filesystem/__tests__/path-validation.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import * as path from 'path'; import * as fs from 'fs/promises'; import * as os from 'os'; -import { isPathWithinAllowedDirectories } from '../path-validation.js'; +import { isPathWithin, isPathWithinAllowedDirectories } from '../path-validation.js'; /** * Check if the current environment supports symlink creation @@ -998,3 +998,93 @@ describe('Path Validation', () => { }); }); }); + +describe('isPathWithin - pure containment helper', () => { + // Platform-appropriate fixture root so these assertions run everywhere + const root = path.sep === '\\' ? 'C:\\allowed-root' : '/allowed-root'; + + it('allows exact root match and subdirectories', () => { + expect(isPathWithin([root], root)).toBe(true); + expect(isPathWithin([root], path.join(root, 'sub'))).toBe(true); + expect(isPathWithin([root], path.join(root, 'sub', 'file.txt'))).toBe(true); + }); + + it('denies traversal escapes judged by resolved location (any platform)', () => { + expect(isPathWithin([root], path.join(root, '..', 'escape'))).toBe(false); + expect(isPathWithin([root], path.join(root, 'a', '..', '..', 'escape'))).toBe(false); + // Deep traversal that lands back inside must still be allowed + expect(isPathWithin([root], path.join(root, 'a', 'b', '..', 'c', 'file.txt'))).toBe(true); + }); + + it('denies sibling directories sharing a name prefix (segment boundary)', () => { + expect(isPathWithin([root], root + '2')).toBe(false); + expect(isPathWithin([root], root + '-backup')).toBe(false); + }); + + it('rejects invalid inputs without throwing', () => { + expect(isPathWithin([], '/x')).toBe(false); + expect(isPathWithin(null as any, '/x')).toBe(false); + expect(isPathWithin(['/x'], '')).toBe(false); + expect(isPathWithin(['/x'], null as any)).toBe(false); + expect(isPathWithin(['/home/user/project'], '/home/user/project\x00/etc/passwd')).toBe(false); + expect(isPathWithin(['/home/user/project\x00'], '/home/user/project')).toBe(false); + }); + + it('handles file-system roots as allowed roots', () => { + if (path.sep === '/') { + expect(isPathWithin(['/'], '/etc/passwd')).toBe(true); + } else { + // Drive root: same drive allowed, different drive denied + expect(isPathWithin(['C:\\'], 'C:\\anything\\here')).toBe(true); + expect(isPathWithin(['C:\\'], 'D:\\other')).toBe(false); + } + }); +}); + +describe('Case-insensitive containment on Windows (issue #470)', () => { + const isWin = process.platform === 'win32'; + + it('allows drive-letter case mismatches on Windows', () => { + if (!isWin) return; + const allowed = ['c:\\source']; + expect(isPathWithin(allowed, 'C:\\source\\file.md')).toBe(true); + expect(isPathWithin(allowed, 'c:\\source\\file.md')).toBe(true); + + // Same decision through the legacy wrapper used by validate_path in lib.ts + expect(isPathWithinAllowedDirectories('C:\\source\\file.md', ['c:\\source'])).toBe(true); + expect(isPathWithinAllowedDirectories('c:\\source', ['C:\\source'])).toBe(true); + }); + + it('allows inner-component case mismatches on Windows', () => { + if (!isWin) return; + const allowed = ['C:\\Users\\ADITY\\Documents']; + expect(isPathWithin(allowed, 'C:\\users\\adity\\documents\\notes\\file.md')).toBe(true); + expect(isPathWithin(allowed, 'c:\\Users\\Adity\\Documents')).toBe(true); + expect(isPathWithin(allowed, 'C:\\USERS\\adity\\DOCUMENTS\\a\\b\\c.txt')).toBe(true); + }); + + it('still rejects case-folded siblings and other drives on Windows', () => { + if (!isWin) return; + const allowed = ['c:\\source']; + expect(isPathWithin(allowed, 'C:\\source2\\file.md')).toBe(false); + expect(isPathWithin(allowed, 'C:\\source-backup\\file.md')).toBe(false); + expect(isPathWithin(allowed, 'D:\\source\\file.md')).toBe(false); + }); + + it('denies traversal escapes even with case folding on Windows', () => { + if (!isWin) return; + const allowed = ['c:\\source']; + expect(isPathWithin(allowed, 'C:\\SOURCE\\..\\..\\Windows\\system32')).toBe(false); + expect(isPathWithin(allowed, 'c:\\source\\sub\\..\\..\\elsewhere\\secret.txt')).toBe(false); + }); + + it('keeps POSIX comparison byte-exact (case-sensitive)', () => { + if (isWin) return; + const allowed = ['/home/user/project']; + expect(isPathWithin(allowed, '/Home/user/project/file.md')).toBe(false); + expect(isPathWithin(allowed, '/home/user/PROJECT/file.md')).toBe(false); + expect(isPathWithin(allowed, '/HOME/USER/PROJECT')).toBe(false); + expect(isPathWithin(allowed, '/home/user/project/file.md')).toBe(true); + expect(isPathWithinAllowedDirectories('/Home/user/project', ['/home/user/project'])).toBe(false); + }); +}); diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 234605bb13..cf592bc05d 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -7,7 +7,7 @@ import { type Root, } from "@modelcontextprotocol/sdk/types.js"; import fs from "fs/promises"; -import { createReadStream } from "fs"; +import { createReadStream, realpathSync } from "fs"; import path from "path"; import { pathToFileURL } from "url"; import { z } from "zod"; @@ -38,33 +38,41 @@ if (args.length === 0) { console.error("At least one directory must be provided by EITHER method for the server to operate."); } -// Store allowed directories in normalized and resolved form +/** + * Resolves an allowed directory to its on-disk canonical form. + * realpathSync.native collapses symlinks and — on Windows — drive-letter and + * inner-component casing plus 8.3 short names (GitHub issue #470), so the + * containment checks in validate_path compare consistently-cased roots. + * Returns null when the path cannot be resolved (e.g. it does not exist yet). + */ +function canonicalizeAllowedDirectory(dir: string): string | null { + try { + return normalizePath(realpathSync.native(dir)); + } catch { + return null; + } +} + +// Store allowed directories in normalized and resolved form. +// Each argv entry is path.resolve()d exactly once at startup, then passed +// through native realpath canonicalization above. // We store BOTH the original path AND the resolved path to handle symlinks correctly // This fixes the macOS /tmp -> /private/tmp symlink issue where users specify /tmp // but the resolved path is /private/tmp -let allowedDirectories = (await Promise.all( - args.map(async (dir) => { - const expanded = expandHome(dir); - const absolute = path.resolve(expanded); - const normalizedOriginal = normalizePath(absolute); - try { - // Security: Resolve symlinks in allowed directories during startup - // This ensures we know the real paths and can validate against them later - const resolved = await fs.realpath(absolute); - const normalizedResolved = normalizePath(resolved); - // Return both original and resolved paths if they differ - // This allows matching against either /tmp or /private/tmp on macOS - if (normalizedOriginal !== normalizedResolved) { - return [normalizedOriginal, normalizedResolved]; - } - return [normalizedResolved]; - } catch (error) { - // If we can't resolve (doesn't exist), use the normalized absolute path - // This allows configuring allowed dirs that will be created later - return [normalizedOriginal]; - } - }) -)).flat(); +let allowedDirectories = args.flatMap((dir) => { + const expanded = expandHome(dir); + const absolute = path.resolve(expanded); + const normalizedOriginal = normalizePath(absolute); + const canonical = canonicalizeAllowedDirectory(absolute); + // Return both original and resolved paths if they differ + // This allows matching against either /tmp or /private/tmp on macOS + if (canonical && canonical !== normalizedOriginal) { + return [normalizedOriginal, canonical]; + } + // If we can't resolve (doesn't exist), use the normalized absolute path + // This allows configuring allowed dirs that will be created later + return [canonical ?? normalizedOriginal]; +}); // Filter to only accessible directories, warn about inaccessible ones const accessibleDirectories: string[] = []; diff --git a/src/filesystem/path-validation.ts b/src/filesystem/path-validation.ts index 972e9c49d0..4315b4f9cc 100644 --- a/src/filesystem/path-validation.ts +++ b/src/filesystem/path-validation.ts @@ -1,86 +1,108 @@ import path from 'path'; /** - * Checks if an absolute path is within any of the allowed directories. - * - * @param absolutePath - The absolute path to check (will be normalized) - * @param allowedDirectories - Array of absolute allowed directory paths (will be normalized) - * @returns true if the path is within an allowed directory, false otherwise - * @throws Error if given relative paths after normalization + * Whether path containment should be compared case-insensitively. + * + * Windows file systems (NTFS/FAT) are case-insensitive but case-preserving, + * so the same directory may legitimately be reached as `c:\source`, + * `C:\source`, or `C:\Source` (GitHub issue #470). Only fold case when + * running on win32; POSIX file systems are case-sensitive and must keep + * byte-exact comparisons. */ -export function isPathWithinAllowedDirectories(absolutePath: string, allowedDirectories: string[]): boolean { - // Type validation - if (typeof absolutePath !== 'string' || !Array.isArray(allowedDirectories)) { - return false; - } +const IS_WINDOWS = process.platform === 'win32'; + +/** + * Folds a resolved absolute path into its comparable form. + * On Windows both sides of the comparison are lowercased so drive-letter and + * inner-component casing differences never cause false access denials. + * On POSIX the path is returned untouched. + */ +function comparablePath(absolutePath: string): string { + return IS_WINDOWS ? absolutePath.toLowerCase() : absolutePath; +} - // Reject empty inputs - if (!absolutePath || allowedDirectories.length === 0) { +/** + * Pure containment check: is `targetPath` equal to, or nested under, any of + * `allowedRoots`? + * + * Both sides are resolved with path.resolve() before comparing, so relative + * input, redundant separators, and `.`/`..` segments cannot smuggle a path + * across a boundary — a traversal escape is judged by its final resolved + * location. The root prefix must match up to a segment boundary, so + * `/allowed` never matches `/allowed2`. + * + * The comparison is case-insensitive when `process.platform === 'win32'` + * and byte-exact on every other platform. + * + * @param allowedRoots - Allowed directory roots (absolute recommended; resolved internally) + * @param targetPath - Path to test against the allowed roots + * @returns true if targetPath is within an allowed root, false otherwise + */ +export function isPathWithin(allowedRoots: unknown[], targetPath: unknown): boolean { + // Type validation + if (!Array.isArray(allowedRoots) || typeof targetPath !== 'string' || !targetPath) { return false; } // Reject null bytes (forbidden in paths) - if (absolutePath.includes('\x00')) { + if (targetPath.includes('\x00')) { return false; } - // Normalize the input path - let normalizedPath: string; + // Normalize the target once; resolve() also collapses `.`/`..` segments so + // traversal attempts are judged on where they actually land. + let resolvedTarget: string; try { - normalizedPath = path.resolve(path.normalize(absolutePath)); + resolvedTarget = path.resolve(path.normalize(targetPath)); } catch { return false; } + const foldedTarget = comparablePath(resolvedTarget); - // Verify it's absolute after normalization - if (!path.isAbsolute(normalizedPath)) { - throw new Error('Path must be absolute after normalization'); - } - - // Check against each allowed directory - return allowedDirectories.some(dir => { - if (typeof dir !== 'string' || !dir) { - return false; - } - - // Reject null bytes in allowed dirs - if (dir.includes('\x00')) { + return allowedRoots.some(root => { + if (typeof root !== 'string' || !root || root.includes('\x00')) { return false; } - // Normalize the allowed directory - let normalizedDir: string; + let resolvedRoot: string; try { - normalizedDir = path.resolve(path.normalize(dir)); + resolvedRoot = path.resolve(path.normalize(root)); } catch { return false; } + const foldedRoot = comparablePath(resolvedRoot); - // Verify allowed directory is absolute after normalization - if (!path.isAbsolute(normalizedDir)) { - throw new Error('Allowed directories must be absolute paths after normalization'); - } - - // Check if normalizedPath is within normalizedDir - // Path is inside if it's the same or a subdirectory - if (normalizedPath === normalizedDir) { + // The target may be the allowed root itself + if (foldedTarget === foldedRoot) { return true; } - - // Special case for root directory to avoid double slash - // On Windows, we need to check if both paths are on the same drive - if (normalizedDir === path.sep) { - return normalizedPath.startsWith(path.sep); - } - - // On Windows, also check for drive root (e.g., "C:\") - if (path.sep === '\\' && normalizedDir.match(/^[A-Za-z]:\\?$/)) { - // Ensure both paths are on the same drive - const dirDrive = normalizedDir.charAt(0).toLowerCase(); - const pathDrive = normalizedPath.charAt(0).toLowerCase(); - return pathDrive === dirDrive && normalizedPath.startsWith(normalizedDir.replace(/\\?$/, '\\')); - } - - return normalizedPath.startsWith(normalizedDir + path.sep); + + // Require a segment-boundary prefix. Resolved roots only keep their + // trailing separator at a file-system root (`/` or `C:\`), so this one + // rule covers plain directories, POSIX root, and drive roots alike — + // including the different-drive rejection on Windows. + const rootPrefix = foldedRoot.endsWith(path.sep) ? foldedRoot : foldedRoot + path.sep; + return foldedTarget.startsWith(rootPrefix); }); } + +/** + * Checks if an absolute path is within any of the allowed directories. + * + * Backward-compatible wrapper around {@link isPathWithin} that preserves the + * historic argument order (target path first). All containment decisions — + * including the Windows case-insensitive behavior for issue #470 — live in + * {@link isPathWithin}. + * + * @param absolutePath - The absolute path to check (will be normalized) + * @param allowedDirectories - Array of absolute allowed directory paths (will be normalized) + * @returns true if the path is within an allowed directory, false otherwise + */ +export function isPathWithinAllowedDirectories(absolutePath: string, allowedDirectories: string[]): boolean { + // Type validation + if (typeof absolutePath !== 'string' || !Array.isArray(allowedDirectories)) { + return false; + } + + return isPathWithin(allowedDirectories, absolutePath); +}