diff --git a/src/filesystem/__tests__/sibling-paths.test.ts b/src/filesystem/__tests__/sibling-paths.test.ts new file mode 100644 index 0000000000..45fe6a4925 --- /dev/null +++ b/src/filesystem/__tests__/sibling-paths.test.ts @@ -0,0 +1,41 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { setAllowedDirectories, validatePath } from '../lib.js'; + +// A request path must be checked exactly as it will be used: a sibling directory +// whose name is the allowed directory's name plus a quote or whitespace is outside it. +describe('validatePath with sibling names that differ by quotes or whitespace', () => { + let parentDir: string; + let allowedDir: string; + + beforeEach(async () => { + parentDir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-sibling-'))); + allowedDir = path.join(parentDir, 'allowed'); + await fs.mkdir(allowedDir); + setAllowedDirectories([allowedDir]); + }); + + afterEach(async () => { + setAllowedDirectories([]); + await fs.rm(parentDir, { recursive: true, force: true }); + }); + + it.each(["'", '"', ' ', '\t'])('rejects an existing sibling named allowed%j', async (suffix) => { + await fs.mkdir(allowedDir + suffix); + await fs.writeFile(path.join(allowedDir + suffix, 'secret.txt'), 'outside'); + await expect(validatePath(allowedDir + suffix)).rejects.toThrow('Access denied'); + await expect(validatePath(path.join(allowedDir + suffix, 'secret.txt'))).rejects.toThrow('Access denied'); + }); + + it('rejects a new file next to the allowed directory', async () => { + await expect(validatePath(allowedDir + '\t')).rejects.toThrow('Access denied'); + }); + + it('still allows a file inside the allowed directory whose name ends with a quote', async () => { + const file = path.join(allowedDir, "it's'"); + await fs.writeFile(file, 'inside'); + await expect(validatePath(file)).resolves.toBe(file); + }); +}); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 6cd2165b5a..5e84becb6a 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -5,7 +5,7 @@ import { randomBytes } from 'crypto'; import { StringDecoder } from 'string_decoder'; import { diffLines, createTwoFilesPatch } from 'diff'; import { minimatch } from 'minimatch'; -import { normalizePath, expandHome } from './path-utils.js'; +import { normalizePathFormat, expandHome } from './path-utils.js'; import { isPathWithinAllowedDirectories } from './path-validation.js'; // Global allowed directories - set by the main module @@ -83,7 +83,7 @@ function resolveRelativePathAgainstAllowedDirectories(relativePath: string): str // Try to resolve relative path against each allowed directory for (const allowedDir of allowedDirectories) { const candidate = path.resolve(allowedDir, relativePath); - const normalizedCandidate = normalizePath(candidate); + const normalizedCandidate = normalizePathFormat(candidate); // Check if the resulting path lies within any allowed directory if (isPathWithinAllowedDirectories(normalizedCandidate, allowedDirectories)) { @@ -100,7 +100,7 @@ function resolveRelativePathAgainstAllowedDirectories(relativePath: string): str async function resolveUnicodeEquivalentPath(absolutePath: string): Promise { const allowedDirectory = [...allowedDirectories] .sort((left, right) => right.length - left.length) - .find(directory => isPathWithinAllowedDirectories(normalizePath(absolutePath), [directory])); + .find(directory => isPathWithinAllowedDirectories(normalizePathFormat(absolutePath), [directory])); if (!allowedDirectory) { return absolutePath; @@ -129,7 +129,7 @@ async function resolveUnicodeEquivalentPath(absolutePath: string): Promise { ? path.resolve(expandedPath) : resolveRelativePathAgainstAllowedDirectories(expandedPath); - const normalizedRequested = normalizePath(absolute); + const normalizedRequested = normalizePathFormat(absolute); // Security: Check if path is within allowed directories before any file operations const isAllowed = isPathWithinAllowedDirectories(normalizedRequested, allowedDirectories); @@ -161,7 +161,7 @@ export async function validatePath(requestedPath: string): Promise { // This prevents attackers from creating symlinks that point outside allowed directories try { const realPath = await fs.realpath(absolute); - const normalizedReal = normalizePath(realPath); + const normalizedReal = normalizePathFormat(realPath); if (!isPathWithinAllowedDirectories(normalizedReal, allowedDirectories)) { throw new Error(`Access denied - symlink target outside allowed directories: ${realPath} not in ${allowedDirectories.join(', ')}`); } diff --git a/src/filesystem/path-utils.ts b/src/filesystem/path-utils.ts index 6ab5a5969b..440cf7173c 100644 --- a/src/filesystem/path-utils.ts +++ b/src/filesystem/path-utils.ts @@ -38,8 +38,16 @@ export function convertToWindowsPath(p: string): string { */ export function normalizePath(p: string): string { // Remove any surrounding quotes and whitespace - p = p.trim().replace(/^["']|["']$/g, ''); + return normalizePathFormat(p.trim().replace(/^["']|["']$/g, '')); +} +/** + * Like normalizePath, but keeps surrounding quotes and whitespace, which can be + * part of a real file name. Use it for paths that are checked and then used as-is. + * @param p The path to normalize + * @returns Normalized path + */ +export function normalizePathFormat(p: string): string { // Check if this is a Unix path that should not be converted // WSL paths (/mnt/) should ALWAYS be preserved as they work correctly in WSL with Node.js fs // Regular Unix paths should also be preserved