From 05fcee5bca61efc4112d36e4c549010e4a83c700 Mon Sep 17 00:00:00 2001 From: Jay Hemnani <193022578+jayhemnani9910@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:35:39 +0530 Subject: [PATCH] fix(filesystem): validate the request path as it will be used validatePath compared the request against the allowed directories after normalizePath, which trims surrounding whitespace and quotes, but then returned the untrimmed path for the file operation. Keep normalizePath's trimming for configured directories and check request paths with a format-only normalization, so the path that is checked is the path that is used. --- .../__tests__/sibling-paths.test.ts | 41 +++++++++++++++++++ src/filesystem/lib.ts | 12 +++--- src/filesystem/path-utils.ts | 10 ++++- 3 files changed, 56 insertions(+), 7 deletions(-) create mode 100644 src/filesystem/__tests__/sibling-paths.test.ts 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