From 5e8d189f2eedced8f82adb26d67010363bd58f66 Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Sat, 25 Jul 2026 21:03:29 +0300 Subject: [PATCH] fix(mcp): harden local file and token access - Resolve configured root paths through symlinks before file operations - Compare authentication tokens through fixed-length digests --- CHANGELOG.md | 1 + packages/mcp/src/auth.ts | 9 +- packages/mcp/src/tool/output.ts | 240 ++++++++++++++++++- packages/mcp/src/tool/registration.ts | 40 +++- tests/engine/mcp/auth.test.ts | 23 ++ tests/engine/mcp/path-scoping.test.ts | 329 ++++++++++++++++++++++++-- tests/engine/mcp/server.test.ts | 5 +- 7 files changed, 597 insertions(+), 50 deletions(-) create mode 100644 tests/engine/mcp/auth.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c76d48c46..bf30f0241 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ ### Fixed +- Keep MCP file access inside its configured root even when paths contain symlinks, and harden local authentication token checks. - Keep desktop text visible across the scene and overlay canvases, refresh it after local fonts load, and preserve rendering when a requested italic face is unavailable (#395). - Honor node-scoped variable modes in `.fig` files so light and dark component examples keep their intended colors. - Preserve nested instance text, visibility, paint, geometry, and clipping across repeated children and component swaps in `.fig` files. diff --git a/packages/mcp/src/auth.ts b/packages/mcp/src/auth.ts index 9b3840531..6098536c4 100644 --- a/packages/mcp/src/auth.ts +++ b/packages/mcp/src/auth.ts @@ -1,3 +1,5 @@ +import { createHash, timingSafeEqual } from 'node:crypto' + export function bearerToken(header: string | undefined | null): string | null { return header?.startsWith('Bearer ') ? header.slice('Bearer '.length) : null } @@ -10,5 +12,10 @@ export function mcpRequestToken( } export function isAuthorized(provided: string | null, expected: string | null): boolean { - return expected === null || provided === expected + if (expected === null) return true + if (provided === null) return false + + const providedDigest = createHash('sha256').update(provided, 'utf8').digest() + const expectedDigest = createHash('sha256').update(expected, 'utf8').digest() + return timingSafeEqual(providedDigest, expectedDigest) } diff --git a/packages/mcp/src/tool/output.ts b/packages/mcp/src/tool/output.ts index 4bde6cb79..cbaf77fd1 100644 --- a/packages/mcp/src/tool/output.ts +++ b/packages/mcp/src/tool/output.ts @@ -1,16 +1,216 @@ -import { mkdir, writeFile } from 'node:fs/promises' -import { dirname, resolve, sep as osSep } from 'node:path' +import { lstat, mkdir, readlink, realpath, writeFile } from 'node:fs/promises' +import { dirname, basename, isAbsolute, join, parse, resolve, sep as osSep } from 'node:path' import { ok } from '#mcp/result' import type { MCPResult } from '#mcp/result' -export function resolveSafePath(filePath: string, root: string): string { - const resolved = resolve(filePath) - const sep = root.endsWith('/') || root.endsWith('\\') ? '' : osSep - if (!resolved.startsWith(root + sep) && resolved !== root) { +/** Returns true for filesystem errors that indicate a missing or unresolvable path (ENOENT, ENOTDIR, ELOOP). */ +function isMissingPathError(e: unknown): boolean { + const code = + typeof e === 'object' && e !== null && 'code' in e ? (e as { code?: unknown }).code : undefined + return code === 'ENOENT' || code === 'ENOTDIR' || code === 'ELOOP' +} + +/** + * Resolve a file path and verify it stays within the allowed root directory. + * Uses fs.realpath to resolve symlinks for the security check, preventing + * traversal attacks where a symlink inside root points outside root. + * Returns the resolved (normalized) path for display and file operations. + */ + +/** Walk up from `p` until we find a path that realpath() can resolve. */ +async function resolveRealAncestor( + p: string +): Promise<{ realAncestor: string; remainder: string }> { + let current = resolve(p) + let remainder = '' + let iterations = 0 + do { + try { + const real = await realpath(current) + return { realAncestor: real, remainder } + } catch (e) { + if (!isMissingPathError(e)) throw e + const parent = dirname(current) + if (parent === current) return { realAncestor: current, remainder } + remainder = osSep + basename(current) + remainder + current = parent + } + iterations++ + } while (iterations < 64) // depth limit to prevent infinite loops + // Fail closed: if we exceeded the depth limit, the path may contain + // unresolved symlink components (e.g., circular symlinks). Returning a + // partially-resolved path would allow it to pass containment checks + // even though the fully-resolved path could be outside root. + throw new Error(`Path resolution depth limit exceeded (possible circular symlinks): ${p}`) +} + +/** + * Walks the non-existent path segments (remainder) from the real ancestor and + * rejects any that are symlinks. A symlink in a non-existent path segment + * could point outside the allowed root — when the file is eventually written, + * the OS follows the symlink chain and the write lands outside root. + */ +async function assertNoSymlinksInRemainder(realAncestor: string, remainder: string): Promise { + if (!remainder) return + const segments = remainder.split(osSep).filter(Boolean) + let current = realAncestor + for (const seg of segments) { + current = join(current, seg) + try { + const stat = await lstat(current) + if (stat.isSymbolicLink()) { + throw new Error(`Path is outside the allowed root: symlink at ${current}`) + } + } catch (e) { + if (e instanceof Error && e.message.includes('outside the allowed root')) throw e + if (!isMissingPathError(e)) throw e + // lstat failed — the component doesn't exist at all, which is expected + // since we're creating a new file. No symlink to worry about. + } + } +} + +/** Throw if `rootPath` is trivially broad (e.g. "/" or "C:\"). */ +function assertNarrowRoot(rootPath: string, original: string): void { + const normalized = resolve(rootPath) + const parsedRoot = parse(normalized).root + if (normalized === '/' || normalized === osSep || normalized === parsedRoot) { + throw new Error( + `Root path is too broad: "${original}" (resolved to "${normalized}"). ` + + 'Specify a narrower OPENPENCIL_MCP_ROOT directory.' + ) + } +} + +/** Resolve `p` to its realpath-validated canonical form, handling non-existent paths. */ +async function resolveRealPath(p: string): Promise { + try { + return await realpath(p) + } catch (e) { + if (!isMissingPathError(e)) throw e + const parentDir = dirname(p) + const baseName = basename(p) + try { + const realParent = await realpath(parentDir) + return join(realParent, baseName) + } catch (e) { + if (!isMissingPathError(e)) throw e + const { realAncestor, remainder } = await resolveRealAncestor(parentDir) + await assertNoSymlinksInRemainder(realAncestor, remainder) + return join(realAncestor, remainder.slice(osSep.length), baseName) + } + } +} + +export interface SafePathResult { + /** The user-provided normalized path (for display/error messages). */ + resolved: string + /** The canonical realpath-validated path (for filesystem operations). */ + realPath: string +} + +const MAX_SYMLINK_DEPTH = 16 + +/** + * Resolve a dangling symlink's target. When realpath fails on a symlink, + * read the link target and recursively validate it is inside root. + * Returns the canonical realPath of the target, or undefined if `resolved` + * is not a symlink. + */ +async function resolveDanglingSymlink( + resolved: string, + root: string, + realRoot: string, + symlinkDepth: number +): Promise { + try { + const stat = await lstat(resolved) + if (!stat.isSymbolicLink()) return undefined + // Dangling symlink — realpath already failed, which means the + // target doesn't exist. Read the symlink target and validate it + // is inside root. Absolute-path targets pointing outside root are + // rejected. Relative targets are resolved relative to the symlink + // directory and checked against root. + const linkTarget = await readlink(resolved) + const linkDir = dirname(resolved) + const resolvedTarget = isAbsolute(linkTarget) ? linkTarget : resolve(linkDir, linkTarget) + // Recursively validate the target (handles nested symlinks). + // Pass the already-canonical realRoot so the recursive call skips + // redundant assertNarrowRoot + realpath(root) work. + const targetResult = await resolveSafePathInternal( + resolvedTarget, + root, + symlinkDepth + 1, + realRoot + ) + return targetResult.realPath + } catch (e) { + if (e instanceof Error) { + // Re-throw security, root-scope, and depth-limit errors — these must + // not be swallowed. They indicate genuine validation failures, not + // the expected "path doesn't exist" case. + if ( + e.message.includes('outside the allowed root') || + e.message.includes('Root path is too broad') || + e.message.includes('depth limit exceeded') + ) + throw e + } + if (!isMissingPathError(e)) throw e + // lstat failed (file doesn't exist at all) — not a symlink + return undefined + } +} + +async function resolveSafePathInternal( + filePath: string, + root: string, + symlinkDepth: number, + realRoot?: string +): Promise { + if (symlinkDepth >= MAX_SYMLINK_DEPTH) { + throw new Error( + `Symlink resolution depth limit exceeded (possible circular symlinks): ${filePath}` + ) + } + + const normalizedRoot = resolve(root) + const resolved = isAbsolute(filePath) ? resolve(filePath) : resolve(normalizedRoot, filePath) + + if (!realRoot) { + assertNarrowRoot(root, root) + + try { + realRoot = await realpath(root) + } catch (e) { + if (!isMissingPathError(e)) throw e + const { realAncestor, remainder } = await resolveRealAncestor(root) + realRoot = remainder ? join(realAncestor, remainder.slice(osSep.length)) : realAncestor + } + + assertNarrowRoot(realRoot, root) + } + + const realSep = realRoot.endsWith('/') || realRoot.endsWith('\\') ? '' : osSep + + let realPath: string + try { + realPath = await realpath(resolved) + } catch (e) { + if (!isMissingPathError(e)) throw e + const symlinkRealPath = await resolveDanglingSymlink(resolved, root, realRoot, symlinkDepth) + realPath = symlinkRealPath ?? (await resolveRealPath(resolved)) + } + + if (!realPath.startsWith(realRoot + realSep) && realPath !== realRoot) { throw new Error(`Path is outside the allowed root: ${root}`) } - return resolved + return { resolved, realPath } +} + +export async function resolveSafePath(filePath: string, root: string): Promise { + return resolveSafePathInternal(filePath, root, 0) } export async function writeToolOutput( @@ -19,18 +219,32 @@ export async function writeToolOutput( filePath: string, root: string ): Promise { - const resolved = resolveSafePath(filePath, root) - await mkdir(dirname(resolved), { recursive: true }) + const { resolved, realPath } = await resolveSafePath(filePath, root) + // Use the canonical realPath for filesystem operations to prevent TOCTOU: + // an attacker could swap a directory component with a symlink between + // resolveSafePath's validation and the writeFile call. Writing to the + // realpath-validated path ensures the write lands inside root. + const parentDir = dirname(realPath) + await mkdir(parentDir, { recursive: true }) + // TOCTOU mitigation: re-verify the parent directory still resolves inside + // the root after mkdir. An attacker who replaces an ancestor directory + // with a symlink between resolveSafePath and the write would be detected + // here — realpath follows the ancestor symlink and resolves outside root. + await resolveSafePath(parentDir, root) if (toolName === 'export_svg' && typeof result.svg === 'string') { - await writeFile(resolved, result.svg, 'utf8') + await writeFile(realPath, result.svg, 'utf8') + await resolveSafePath(realPath, root) return ok({ written: resolved, byteLength: Buffer.byteLength(result.svg, 'utf8') }) } if (toolName === 'export_image' && typeof result.base64 === 'string') { - await writeFile(resolved, Buffer.from(result.base64, 'base64')) - return ok({ written: resolved, byteLength: result.byteLength ?? null }) + const buffer = Buffer.from(result.base64, 'base64') + await writeFile(realPath, buffer) + await resolveSafePath(realPath, root) + return ok({ written: resolved, byteLength: buffer.length }) } if (toolName === 'get_jsx' && typeof result.jsx === 'string') { - await writeFile(resolved, result.jsx, 'utf8') + await writeFile(realPath, result.jsx, 'utf8') + await resolveSafePath(realPath, root) return ok({ written: resolved, byteLength: Buffer.byteLength(result.jsx, 'utf8') }) } return null diff --git a/packages/mcp/src/tool/registration.ts b/packages/mcp/src/tool/registration.ts index 0c7f3091b..1e77b53b5 100644 --- a/packages/mcp/src/tool/registration.ts +++ b/packages/mcp/src/tool/registration.ts @@ -130,7 +130,11 @@ export function registerTools(mcpServer: McpServer, options: RegisterToolsOption : 'Save the current document to disk. Uses the existing file path if available, otherwise prompts for a location.', inputSchema: resolvedRoot ? z.object({ - path: z.string().describe('Optional absolute path for the .fig file').optional(), + path: z + .string() + .min(1) + .describe('Path for the .fig file, absolute or relative to the MCP root') + .optional(), ...automationTargetSchema }) : z.object({ ...automationTargetSchema }) @@ -138,14 +142,19 @@ export function registerTools(mcpServer: McpServer, options: RegisterToolsOption async (args: { path?: string; document_id?: string; page_id?: string }) => { try { const safePath = - args.path && resolvedRoot ? resolveSafePath(args.path, resolvedRoot) : undefined + args.path !== undefined && resolvedRoot + ? await resolveSafePath(args.path, resolvedRoot) + : undefined const { target } = splitAutomationTarget(args) - const result = await sendRpc({ command: 'save_file', args: { ...target, path: safePath } }) + const result = await sendRpc({ + command: 'save_file', + args: { ...target, path: safePath?.realPath } + }) const res = result as { ok?: boolean; result?: unknown; target?: unknown; error?: string } if (res.ok === false) return fail(new Error(res.error)) return ok({ saved: true, - ...(safePath ? { path: safePath } : {}), + ...(safePath ? { path: safePath.resolved } : {}), ...(res.target ? { target: res.target } : {}) }) } catch (e) { @@ -160,15 +169,21 @@ export function registerTools(mcpServer: McpServer, options: RegisterToolsOption { description: `Open a .fig or .pen file from disk into a new tab. Path must be inside ${resolvedRoot}.`, inputSchema: z.object({ - path: z.string().describe('Absolute path to the design file'), + path: z + .string() + .min(1) + .describe('Path to the design file, absolute or relative to the MCP root'), ...automationTargetSchema }) }, async (args: { path: string; document_id?: string; page_id?: string }) => { try { - const safe = resolveSafePath(args.path, resolvedRoot) + const safe = await resolveSafePath(args.path, resolvedRoot) const { target } = splitAutomationTarget(args) - const result = await sendRpc({ command: 'open_file', args: { ...target, path: safe } }) + const result = await sendRpc({ + command: 'open_file', + args: { ...target, path: safe.realPath } + }) const res = result as { ok?: boolean; result?: unknown; target?: unknown; error?: string } if (res.ok === false) return fail(new Error(res.error)) return ok({ opened: true, ...(res.target ? { target: res.target } : {}) }) @@ -183,17 +198,22 @@ export function registerTools(mcpServer: McpServer, options: RegisterToolsOption { description: `Create a new empty document. Optionally set a save path inside ${resolvedRoot}.`, inputSchema: z.object({ - path: z.string().describe('Optional absolute path for the new file').optional(), + path: z + .string() + .min(1) + .describe('Path for the new file, absolute or relative to the MCP root') + .optional(), ...automationTargetSchema }) }, async (args: { path?: string; document_id?: string; page_id?: string }) => { try { - const safePath = args.path ? resolveSafePath(args.path, resolvedRoot) : undefined + const safePath = + args.path !== undefined ? await resolveSafePath(args.path, resolvedRoot) : undefined const { target } = splitAutomationTarget(args) const result = await sendRpc({ command: 'new_document', - args: { ...target, path: safePath } + args: { ...target, path: safePath?.realPath } }) const res = result as { ok?: boolean; result?: unknown; target?: unknown; error?: string } if (res.ok === false) return fail(new Error(res.error)) diff --git a/tests/engine/mcp/auth.test.ts b/tests/engine/mcp/auth.test.ts new file mode 100644 index 000000000..fd23be313 --- /dev/null +++ b/tests/engine/mcp/auth.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, test } from 'bun:test' + +import { bearerToken, isAuthorized, mcpRequestToken } from '#mcp/auth' + +describe('MCP authentication', () => { + test('accepts exact tokens and rejects mismatches', () => { + expect(isAuthorized('secret-token', 'secret-token')).toBe(true) + expect(isAuthorized('secret-token-x', 'secret-token')).toBe(false) + expect(isAuthorized(null, 'secret-token')).toBe(false) + }) + + test('allows requests when authentication is disabled', () => { + expect(isAuthorized(null, null)).toBe(true) + expect(isAuthorized('unused', null)).toBe(true) + }) + + test('extracts bearer and fallback MCP tokens', () => { + expect(bearerToken('Bearer secret-token')).toBe('secret-token') + expect(bearerToken('Basic secret-token')).toBeNull() + expect(mcpRequestToken('Bearer bearer-token', 'header-token')).toBe('bearer-token') + expect(mcpRequestToken(undefined, 'header-token')).toBe('header-token') + }) +}) diff --git a/tests/engine/mcp/path-scoping.test.ts b/tests/engine/mcp/path-scoping.test.ts index c6b6343ea..836f97307 100644 --- a/tests/engine/mcp/path-scoping.test.ts +++ b/tests/engine/mcp/path-scoping.test.ts @@ -1,48 +1,327 @@ import { describe, test, expect } from 'bun:test' -import { resolve, sep } from 'node:path' +import { randomUUID } from 'node:crypto' +import { mkdir, realpath, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' -// Test the resolveSafePath logic directly (extracted for testability) -function resolveSafePath(filePath: string, root: string): string { - const resolved = resolve(filePath) - const normalizedSep = root.endsWith('/') || root.endsWith('\\') ? '' : sep - if (!resolved.startsWith(root + normalizedSep) && resolved !== root) { - throw new Error(`Path is outside the allowed root: ${root}`) - } - return resolved -} +import { resolveSafePath } from '#mcp/tool/output' + +const isUnix = process.platform !== 'win32' +const TEST_ID = randomUUID().slice(0, 8) describe('MCP path scoping', () => { - const root = resolve('/tmp/mcp-test-root') + const root = resolve(tmpdir(), 'mcp-' + TEST_ID + '-test-root') - test('allows path inside root', () => { - expect(resolveSafePath(`${root}/design.fig`, root)).toBe(`${root}/design.fig`) + test('allows path inside root', async () => { + try { + await mkdir(root, { recursive: true }) + const result = await resolveSafePath(`${root}/design.fig`, root) + expect(result.resolved).toBe(resolve(`${root}/design.fig`)) + // realPath is the canonical form (realpath-resolved), which may differ + // from resolve() on macOS where /var -> /private/var + const canonicalRoot = await realpath(root) + expect(result.realPath).toBe(join(canonicalRoot, 'design.fig')) + } finally { + await rm(root, { recursive: true, force: true }) + } }) - test('allows nested path inside root', () => { - expect(resolveSafePath(`${root}/sub/dir/file.fig`, root)).toBe(`${root}/sub/dir/file.fig`) + test('resolves relative path inside root', async () => { + const result = await resolveSafePath('design.fig', root) + expect(result.resolved).toBe(resolve(`${root}/design.fig`)) }) - test('allows root itself', () => { - expect(resolveSafePath(root, root)).toBe(root) + test('resolves nested relative path inside root', async () => { + const result = await resolveSafePath('sub/dir/file.fig', root) + expect(result.resolved).toBe(resolve(`${root}/sub/dir/file.fig`)) }) - test('rejects path outside root', () => { - expect(() => resolveSafePath('/etc/passwd', root)).toThrow('outside the allowed root') + test('allows nested path inside root', async () => { + const result = await resolveSafePath(`${root}/sub/dir/file.fig`, root) + expect(result.resolved).toBe(resolve(`${root}/sub/dir/file.fig`)) }) - test('rejects path traversal', () => { - expect(() => resolveSafePath(`${root}/../../../etc/passwd`, root)).toThrow( + test('allows root itself', async () => { + const result = await resolveSafePath(root, root) + expect(result.resolved).toBe(root) + }) + + test('rejects path outside root', async () => { + const outsideRoot = resolve(tmpdir(), 'mcp-' + TEST_ID + '-outside-root') + await expect(resolveSafePath(`${outsideRoot}/passwd`, root)).rejects.toThrow( 'outside the allowed root' ) }) - test('rejects sibling directory', () => { - expect(() => resolveSafePath(`${root}/../other-root/file.fig`, root)).toThrow( + test('rejects path traversal', async () => { + await expect(resolveSafePath(`${root}/../../../etc/passwd`, root)).rejects.toThrow( 'outside the allowed root' ) }) - test('rejects root prefix trick (root-evil)', () => { - expect(() => resolveSafePath(`${root}-evil/file.fig`, root)).toThrow('outside the allowed root') + test('rejects sibling directory', async () => { + await expect(resolveSafePath(`${root}/../other-root/file.fig`, root)).rejects.toThrow( + 'outside the allowed root' + ) + }) + + test('rejects root prefix trick (root-evil)', async () => { + await expect(resolveSafePath(`${root}-evil/file.fig`, root)).rejects.toThrow( + 'outside the allowed root' + ) + }) + + test.skipIf(!isUnix)('rejects non-dangling symlink pointing outside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-test') + const linkPath = `${testDir}/escape.fig` + const outsideTarget = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-outside-target') + try { + await mkdir(testDir, { recursive: true }) + // Create the target so the symlink is NOT dangling — this exercises + // the realpath → outside-root branch rather than the dangling path. + await mkdir(outsideTarget, { recursive: true }) + await symlink(outsideTarget, linkPath) + await expect(resolveSafePath(linkPath, testDir)).rejects.toThrow('outside the allowed root') + } finally { + await rm(testDir, { recursive: true, force: true }) + await rm(outsideTarget, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('allows dangling symlink pointing inside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-dangling-inside-test') + const linkPath = `${testDir}/dangling.fig` + const insideTarget = `${testDir}/nonexistent-target.fig` + try { + await mkdir(testDir, { recursive: true }) + // Dangling symlink: target doesn't exist, but points inside root. + // This is a legitimate use case — the target will be created by the + // write operation. The target path is validated to be inside root. + await symlink(insideTarget, linkPath) + const result = await resolveSafePath(linkPath, testDir) + expect(result.resolved).toBe(resolve(linkPath)) + // realPath should resolve to the target's canonical parent + filename + const canonicalParent = await realpath(testDir) + expect(result.realPath).toBe(join(canonicalParent, 'nonexistent-target.fig')) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('allows symlink pointing inside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-safe-test') + const targetDir = `${testDir}/targets` + const targetFile = `${targetDir}/real.fig` + const linkPath = `${testDir}/link.fig` + try { + await mkdir(targetDir, { recursive: true }) + await writeFile(targetFile, 'test') + await symlink(targetFile, linkPath) + // Returns both resolved (for display) and realPath (canonical, for writes). + // The resolved path is the user-provided normalized path for usability. + const result = await resolveSafePath(linkPath, testDir) + expect(result.resolved).toBe(resolve(linkPath)) + // realPath must be the canonical target (not the symlink itself) + const canonicalTarget = await realpath(targetFile) + expect(result.realPath).toBe(canonicalTarget) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('rejects dangling symlink pointing outside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-dangling-test') + const linkPath = `${testDir}/dangling.fig` + const outsideTarget = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-dangling-outside') + try { + await mkdir(testDir, { recursive: true }) + // Dangling symlink: target doesn't exist, points outside root + await symlink(outsideTarget, linkPath) + await expect(resolveSafePath(linkPath, testDir)).rejects.toThrow('outside the allowed root') + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test('allows nonexistent file inside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-nonexistent-test') + const filePath = `${testDir}/new.fig` + try { + await mkdir(testDir, { recursive: true }) + // File doesn't exist yet (common for save_file / export operations) + const result = await resolveSafePath(filePath, testDir) + expect(result.resolved).toBe(resolve(filePath)) + // realPath uses the canonical parent directory + filename + const canonicalParent = await realpath(testDir) + expect(result.realPath).toBe(join(canonicalParent, 'new.fig')) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test('rejects trivially broad root "/"', async () => { + await expect(resolveSafePath('/some/path', '/')).rejects.toThrow('Root path is too broad') + }) + + test.skipIf(process.platform !== 'win32')( + 'rejects drive root as broad root on Windows', + async () => { + // On Windows, path.parse('C:\\').root === 'C:\\', which resolveSafePath + // should reject. This test runs only on Windows where drive roots exist. + const { parse, resolve: winResolve } = await import('node:path') + const driveRoot = parse(winResolve('C:\\')).root + await expect(resolveSafePath('C:\\some\\path', driveRoot)).rejects.toThrow( + 'Root path is too broad' + ) + } + ) + + test('rejects path exceeding resolution depth limit', async () => { + // resolveRealAncestor has a 64-iteration depth cap. When the entire + // ancestor chain doesn't exist (e.g., a deep non-existent root path), + // the function must fail closed rather than returning a partially-resolved + // path that could bypass containment checks. + const deepSegments = Array.from({ length: 70 }, () => 'sub').join('/') + const deepRoot = resolve(tmpdir(), deepSegments) + await expect(resolveSafePath(`${deepRoot}/file.fig`, deepRoot)).rejects.toThrow( + 'depth limit exceeded' + ) + }) + + test('rejects deep file path exceeding resolution depth limit with existing root', async () => { + // When the root exists but the file path has >64 non-existent ancestor + // segments, resolveRealAncestor is called for the parent directory and + // must fail closed. This exercises the second call site (line 142 in + // output.ts: resolveRealAncestor(parentDir)). + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-deep-path-test') + try { + await mkdir(testDir, { recursive: true }) + // 70 non-existent subdirectories under an existing root + const deepFile = Array.from({ length: 70 }, () => 'sub').join('/') + '/file.fig' + await expect(resolveSafePath(deepFile, testDir)).rejects.toThrow('depth limit exceeded') + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('rejects symlinked root that resolves to /', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-root-test') + const rootLink = `${testDir}/root-link` + try { + await mkdir(testDir, { recursive: true }) + await symlink('/', rootLink) + await expect(resolveSafePath(`${rootLink}/file.fig`, rootLink)).rejects.toThrow( + 'Root path is too broad' + ) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('rejects circular symlinks', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-circular-test') + const linkA = `${testDir}/a.fig` + const linkB = `${testDir}/b.fig` + try { + await mkdir(testDir, { recursive: true }) + // Circular symlink chain: a -> b -> a + await symlink(linkB, linkA) + await symlink(linkA, linkB) + await expect(resolveSafePath(linkA, testDir)).rejects.toThrow('depth limit exceeded') + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('allows absolute symlink target inside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-abs-inside-test') + const targetDir = `${testDir}/targets` + const targetFile = `${targetDir}/real.fig` + const linkPath = `${testDir}/link.fig` + try { + await mkdir(targetDir, { recursive: true }) + await writeFile(targetFile, 'test') + // Absolute symlink target pointing inside root + await symlink(targetFile, linkPath) + const result = await resolveSafePath(linkPath, testDir) + expect(result.resolved).toBe(resolve(linkPath)) + const canonicalTarget = await realpath(targetFile) + expect(result.realPath).toBe(canonicalTarget) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('rejects absolute symlink target outside root', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-abs-outside-test') + const outsideDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-abs-outside-target') + const outsideFile = `${outsideDir}/secret.fig` + const linkPath = `${testDir}/link.fig` + try { + await mkdir(testDir, { recursive: true }) + await mkdir(outsideDir, { recursive: true }) + await writeFile(outsideFile, 'secret') + // Absolute symlink target pointing outside root + await symlink(outsideFile, linkPath) + await expect(resolveSafePath(linkPath, testDir)).rejects.toThrow('outside the allowed root') + } finally { + await rm(testDir, { recursive: true, force: true }) + await rm(outsideDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)( + 'allows relative symlink with traversal that stays inside root', + async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-rel-traversal-test') + const subDir = `${testDir}/sub` + const targetFile = `${testDir}/target.fig` + const linkPath = `${subDir}/link.fig` + try { + await mkdir(subDir, { recursive: true }) + await writeFile(targetFile, 'test') + // Relative symlink: sub/link.fig -> ../target.fig (resolves inside root) + await symlink('../target.fig', linkPath) + const result = await resolveSafePath(linkPath, testDir) + expect(result.resolved).toBe(resolve(linkPath)) + const canonicalTarget = await realpath(targetFile) + expect(result.realPath).toBe(canonicalTarget) + } finally { + await rm(testDir, { recursive: true, force: true }) + } + } + ) + + test.skipIf(!isUnix)('rejects self-referencing symlink', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-self-test') + const linkPath = `${testDir}/self.fig` + try { + await mkdir(testDir, { recursive: true }) + // Self-referencing symlink: link.fig -> link.fig + await symlink(linkPath, linkPath) + await expect(resolveSafePath(linkPath, testDir)).rejects.toThrow('depth limit exceeded') + } finally { + await rm(testDir, { recursive: true, force: true }) + } + }) + + test.skipIf(!isUnix)('allows symlink chain inside root (non-dangling)', async () => { + const testDir = resolve(tmpdir(), 'mcp-' + TEST_ID + '-symlink-chain-test') + const targetFile = `${testDir}/real.fig` + const link1 = `${testDir}/link1.fig` + const link2 = `${testDir}/link2.fig` + try { + await mkdir(testDir, { recursive: true }) + await writeFile(targetFile, 'test') + // Chain: link2 -> link1 -> real.fig + await symlink(targetFile, link1) + await symlink(link1, link2) + const result = await resolveSafePath(link2, testDir) + expect(result.resolved).toBe(resolve(link2)) + const canonicalTarget = await realpath(targetFile) + expect(result.realPath).toBe(canonicalTarget) + } finally { + await rm(testDir, { recursive: true, force: true }) + } }) }) diff --git a/tests/engine/mcp/server.test.ts b/tests/engine/mcp/server.test.ts index 814464109..82c73de96 100644 --- a/tests/engine/mcp/server.test.ts +++ b/tests/engine/mcp/server.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test, beforeEach, afterEach } from 'bun:test' +import { mkdir, realpath } from 'node:fs/promises' import type { AddressInfo } from 'node:net' import { tmpdir } from 'node:os' import path from 'node:path' @@ -483,6 +484,7 @@ describe('MCP server with mcpRoot', () => { await client.connect(transport) const savePath = path.join(TEST_MCP_ROOT, 'unicode', 'пример.fig') + await mkdir(path.dirname(savePath), { recursive: true }) const result = await client.callTool({ name: 'save_file', arguments: { path: savePath } @@ -490,7 +492,8 @@ describe('MCP server with mcpRoot', () => { expect(result.isError).not.toBe(true) const request = browser.requests.find((item) => item.command === 'save_file') - expect(request?.args).toEqual({ path: savePath }) + const canonicalPath = path.join(await realpath(path.dirname(savePath)), path.basename(savePath)) + expect(request?.args).toEqual({ path: canonicalPath }) await client.close() browser.close()