fix: use realpath for MCP roots validation (#2127)

src/McpContext.ts

@@ -5,6 +5,7 @@
  */
 
 import fs from 'node:fs/promises';
+import fsPromises from 'node:fs/promises';
 import os from 'node:os';
 import path from 'node:path';
 import {fileURLToPath, pathToFileURL} from 'node:url';
@@ -45,7 +46,11 @@ import type {
   GeolocationOptions,
   ExtensionServiceWorker,
 } from './types.js';
-import {ensureExtension, getTempFilePath} from './utils/files.js';
+import {
+  ensureExtension,
+  getTempFilePath,
+  resolveCanonicalPath,
+} from './utils/files.js';
 import {getNetworkMultiplierFromString} from './WaitForHelper.js';
 
 interface McpContextOptions {
@@ -175,7 +180,7 @@ export class McpContext implements Context {
     this.#roots = roots;
   }
 
-  validatePath(filePath?: string): void {
+  async validatePath(filePath?: string): Promise<void> {
     if (filePath === undefined) {
       return;
     }
@@ -183,19 +188,50 @@ export class McpContext implements Context {
     if (roots === undefined) {
       return;
     }
-    const absolutePath = path.resolve(filePath);
+
+    let canonicalPath: string;
+
+    try {
+      canonicalPath = await resolveCanonicalPath(filePath);
+    } catch (err) {
+      const errMsg = err instanceof Error ? err.message : String(err);
+      console.error(
+        `[MCP Context] Error resolving real path for ${filePath}: ${errMsg}`,
+      );
+      throw new Error(
+        `Access denied: Cannot resolve base path for ${filePath}.`,
+      );
+    }
+
+    let allowed = false;
     for (const root of roots) {
-      const rootPath = path.resolve(fileURLToPath(root.uri));
-      if (
-        absolutePath === rootPath ||
-        absolutePath.startsWith(rootPath + path.sep)
-      ) {
-        return;
+      try {
+        const rootPathUri = root.uri;
+        const rootPath = path.resolve(fileURLToPath(rootPathUri));
+        const canonicalRoot = await fsPromises.realpath(rootPath);
+
+        if (
+          canonicalPath === canonicalRoot ||
+          canonicalPath.startsWith(canonicalRoot + path.sep)
+        ) {
+          allowed = true;
+          break;
+        }
+      } catch (rootErr) {
+        const errMsg =
+          rootErr instanceof Error ? rootErr.message : String(rootErr);
+        console.warn(
+          `[MCP Context] Could not resolve configured root ${root.uri}: ${errMsg}`,
+        );
+        // Skip this root if it cannot be resolved.
       }
     }
-    throw new Error(
-      `Access denied: path ${filePath} is not within any of the workspace roots ${JSON.stringify(roots)}.`,
-    );
+
+    if (!allowed) {
+      throw new Error(
+        `Access denied: path ${filePath} (canonical: ${canonicalPath}) is not within any of the configured workspace roots.`,
+      );
+    }
   }
 
   resolveCdpRequestId(page: McpPage, cdpRequestId: string): number | undefined {
@@ -708,7 +744,7 @@ export class McpContext implements Context {
     filename: string,
   ): Promise<{filepath: string}> {
     const filepath = await getTempFilePath(filename);
-    this.validatePath(filepath);
+    await this.validatePath(filepath);
     try {
       await fs.writeFile(filepath, data);
     } catch (err) {
@@ -722,7 +758,7 @@ export class McpContext implements Context {
     clientProvidedFilePath: string,
     extension: SupportedExtensions,
   ): Promise<{filename: string}> {
-    this.validatePath(clientProvidedFilePath);
+    await this.validatePath(clientProvidedFilePath);
     try {
       const filePath = ensureExtension(
         path.resolve(clientProvidedFilePath),
@@ -794,7 +830,7 @@ export class McpContext implements Context {
   }
 
   async installExtension(extensionPath: string): Promise<string> {
-    this.validatePath(extensionPath);
+    await this.validatePath(extensionPath);
     const id = await this.browser.installExtension(extensionPath);
     return id;
   }
@@ -825,21 +861,21 @@ export class McpContext implements Context {
   async getHeapSnapshotAggregates(
     filePath: string,
   ): Promise<Record<string, AggregatedInfoWithId>> {
-    this.validatePath(filePath);
+    await this.validatePath(filePath);
     return await this.#heapSnapshotManager.getAggregates(filePath);
   }
 
   async getHeapSnapshotStats(
     filePath: string,
   ): Promise<DevTools.HeapSnapshotModel.HeapSnapshotModel.Statistics> {
-    this.validatePath(filePath);
+    await this.validatePath(filePath);
     return await this.#heapSnapshotManager.getStats(filePath);
   }
 
   async getHeapSnapshotStaticData(
     filePath: string,
   ): Promise<DevTools.HeapSnapshotModel.HeapSnapshotModel.StaticData | null> {
-    this.validatePath(filePath);
+    await this.validatePath(filePath);
     return await this.#heapSnapshotManager.getStaticData(filePath);
   }
 
@@ -847,7 +883,7 @@ export class McpContext implements Context {
     filePath: string,
     id: number,
   ): Promise<DevTools.HeapSnapshotModel.HeapSnapshotModel.ItemsRange> {
-    this.validatePath(filePath);
+    await this.validatePath(filePath);
     return await this.#heapSnapshotManager.getNodesById(filePath, id);
   }
 
@@ -855,6 +891,7 @@ export class McpContext implements Context {
     filePath: string,
     nodeId: number,
   ): Promise<DevTools.HeapSnapshotModel.HeapSnapshotModel.ItemsRange> {
+    await this.validatePath(filePath);
     return await this.#heapSnapshotManager.getRetainers(filePath, nodeId);
   }
 }

src/tools/ToolDefinition.ts

@@ -173,7 +173,7 @@ export type SupportedExtensions =
  * Only add methods used by tools/*.
  */
 export type Context = Readonly<{
-  validatePath(filePath?: string): void;
+  validatePath(filePath?: string): Promise<void>;
   isRunningPerformanceTrace(): boolean;
   setIsRunningPerformanceTrace(x: boolean): void;
   isCruxEnabled(): boolean;

src/tools/input.ts

@@ -463,7 +463,7 @@ export const uploadFile = definePageTool({
   blockedByDialog: true,
   handler: async (request, response, context) => {
     const {uid, filePath} = request.params;
-    context.validatePath(filePath);
+    await context.validatePath(filePath);
     const handle = (await request.page.getElementByUid(
       uid,
     )) as ElementHandle<HTMLInputElement>;

src/tools/lighthouse.ts

@@ -59,7 +59,7 @@ export const lighthouseAudit = definePageTool({
       outputDirPath,
     } = request.params;
 
-    context.validatePath(outputDirPath);
+    await context.validatePath(outputDirPath);
 
     const flags: Flags = {
       onlyCategories: categories,

src/tools/memory.ts

@@ -25,7 +25,7 @@ export const takeHeapSnapshot = definePageTool({
   blockedByDialog: true,
   handler: async (request, response, context) => {
     const page = request.page;
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
 
     await page.pptrPage.captureHeapSnapshot({
       path: ensureExtension(request.params.filePath, '.heapsnapshot'),
@@ -51,7 +51,7 @@ export const getHeapSnapshotSummary = defineTool({
   },
   blockedByDialog: false,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     const stats = await context.getHeapSnapshotStats(request.params.filePath);
     const staticData = await context.getHeapSnapshotStaticData(
       request.params.filePath,
@@ -83,7 +83,7 @@ export const getHeapSnapshotDetails = defineTool({
   },
   blockedByDialog: false,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     const aggregates = await context.getHeapSnapshotAggregates(
       request.params.filePath,
     );
@@ -112,7 +112,7 @@ export const getHeapSnapshotClassNodes = defineTool({
   },
   blockedByDialog: false,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     const nodes = await context.getHeapSnapshotNodesById(
       request.params.filePath,
       request.params.id,
@@ -142,7 +142,7 @@ export const getHeapSnapshotRetainers = defineTool({
     pageSize: zod.number().optional().describe('The page size for pagination.'),
   },
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
 
     const retainers = await context.getHeapSnapshotRetainers(
       request.params.filePath,

src/tools/network.ts

@@ -116,8 +116,8 @@ export const getNetworkRequest = definePageTool({
   },
   blockedByDialog: true,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.requestFilePath);
-    context.validatePath(request.params.responseFilePath);
+    await context.validatePath(request.params.requestFilePath);
+    await context.validatePath(request.params.responseFilePath);
     if (request.params.reqid) {
       response.attachNetworkRequest(request.params.reqid, {
         requestFilePath: request.params.requestFilePath,

src/tools/performance.ts

@@ -49,7 +49,7 @@ export const startTrace = definePageTool({
   },
   blockedByDialog: true,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     if (context.isRunningPerformanceTrace()) {
       response.appendResponseLine(
         'Error: a performance trace is already running. Use performance_stop_trace to stop it. Only one trace can be running at any given time.',
@@ -128,7 +128,7 @@ export const stopTrace = definePageTool({
   },
   blockedByDialog: true,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     if (!context.isRunningPerformanceTrace()) {
       return;
     }

src/tools/screencast.ts

@@ -40,7 +40,7 @@ export const startScreencast = definePageTool(args => ({
   },
   blockedByDialog: false,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     if (context.getScreenRecorder() !== null) {
       response.appendResponseLine(
         'Error: a screencast recording is already in progress. Use screencast_stop to stop it before starting a new one.',

src/tools/screenshot.ts

@@ -52,7 +52,7 @@ export const screenshot = definePageTool({
   },
   blockedByDialog: true,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     if (request.params.uid && request.params.fullPage) {
       throw new Error('Providing both "uid" and "fullPage" is not allowed.');
     }

src/tools/script.ts

@@ -81,7 +81,7 @@ Example with arguments: \`(el) => {
         filePath,
       } = request.params;
 
-      context.validatePath(filePath);
+      await context.validatePath(filePath);
 
       if (cliArgs?.categoryExtensions && serviceWorkerId) {
         if (uidArgs && uidArgs.length > 0) {

src/tools/snapshot.ts

@@ -35,7 +35,7 @@ in the DevTools Elements panel (if any).`,
   },
   blockedByDialog: true,
   handler: async (request, response, context) => {
-    context.validatePath(request.params.filePath);
+    await context.validatePath(request.params.filePath);
     response.includeSnapshot({
       verbose: request.params.verbose ?? false,
       filePath: request.params.filePath,

src/utils/files.ts

@@ -22,3 +22,51 @@ export function ensureExtension(
   const ext = path.extname(filepath);
   return filepath.slice(0, filepath.length - ext.length) + extension;
 }
+
+export async function resolveCanonicalPath(filePath: string): Promise<string> {
+  const absolutePath = path.resolve(filePath);
+  try {
+    // Get the true canonical path, resolving all symlinks.
+    return await fs.realpath(absolutePath);
+  } catch (err) {
+    if (
+      err &&
+      typeof err === 'object' &&
+      'code' in err &&
+      err.code === 'ENOENT'
+    ) {
+      // Find the nearest existing ancestor directory on the filesystem.
+      let current = absolutePath;
+      const missingSegments: string[] = [];
+      while (true) {
+        const parent = path.dirname(current);
+        if (parent === current) {
+          // Reached root directory but still couldn't resolve anything.
+          throw err;
+        }
+        try {
+          const canonicalParent = await fs.realpath(parent);
+          return path.join(
+            canonicalParent,
+            path.basename(current),
+            ...missingSegments,
+          );
+        } catch (parentErr) {
+          if (
+            parentErr &&
+            typeof parentErr === 'object' &&
+            'code' in parentErr &&
+            parentErr.code === 'ENOENT'
+          ) {
+            missingSegments.unshift(path.basename(current));
+            current = parent;
+          } else {
+            throw parentErr;
+          }
+        }
+      }
+    } else {
+      throw err;
+    }
+  }
+}

tests/McpContext.test.ts

@@ -5,6 +5,7 @@
  */
 
 import assert from 'node:assert';
+import fs from 'node:fs/promises';
 import os from 'node:os';
 import path from 'node:path';
 import {afterEach, describe, it} from 'node:test';
@@ -236,24 +237,51 @@ describe('McpContext', () => {
   it('validatePath allows paths within roots', async () => {
     await withMcpContext(async (_response, context) => {
       const workspacePath = path.resolve(os.homedir(), 'workspace-test');
-      const roots = [
-        {uri: pathToFileURL(workspacePath).href, name: 'workspace'},
-      ];
-      context.setRoots(roots);
-      // Valid path within root
-      context.validatePath(path.join(workspacePath, 'test.txt'));
-      context.validatePath(workspacePath);
+      await fs.mkdir(workspacePath, {recursive: true});
+      try {
+        const roots = [
+          {uri: pathToFileURL(workspacePath).href, name: 'workspace'},
+        ];
+        context.setRoots(roots);
+        // Valid path within root
+        await context.validatePath(path.join(workspacePath, 'test.txt'));
+        await context.validatePath(workspacePath);
 
-      // Invalid path outside root and outside temp dir
-      const outsidePath = path.resolve(os.homedir(), 'outside-test.txt');
-      assert.throws(() => context.validatePath(outsidePath), /Access denied/);
+        // Invalid path outside root and outside temp dir
+        const outsidePath = path.resolve(os.homedir(), 'outside-test.txt');
+        await assert.rejects(
+          context.validatePath(outsidePath),
+          /Access denied/,
+        );
+      } finally {
+        await fs.rm(workspacePath, {recursive: true, force: true});
+      }
+    });
+  });
+
+  it('validatePath allows non-existent nested paths within roots', async () => {
+    await withMcpContext(async (_response, context) => {
+      const workspacePath = path.resolve(os.homedir(), 'workspace-test-nested');
+      await fs.mkdir(workspacePath, {recursive: true});
+      try {
+        const roots = [
+          {uri: pathToFileURL(workspacePath).href, name: 'workspace'},
+        ];
+        context.setRoots(roots);
+        // Valid path within root with non-existent intermediate directories
+        await context.validatePath(
+          path.join(workspacePath, 'dir1', 'dir2', 'test.txt'),
+        );
+      } finally {
+        await fs.rm(workspacePath, {recursive: true, force: true});
+      }
     });
   });
 
   it('validatePath allows all paths if roots are undefined (legacy)', async () => {
     await withMcpContext(async (_response, context) => {
       context.setRoots(undefined);
-      context.validatePath(path.resolve(os.homedir(), 'anywhere.txt'));
+      await context.validatePath(path.resolve(os.homedir(), 'anywhere.txt'));
     });
   });
 
@@ -261,11 +289,11 @@ describe('McpContext', () => {
     await withMcpContext(async (_response, context) => {
       context.setRoots([]);
       // Should allow temp dir
-      context.validatePath(path.join(os.tmpdir(), 'test.txt'));
+      await context.validatePath(path.join(os.tmpdir(), 'test.txt'));
 
       // Should deny outside temp dir
-      assert.throws(
-        () => context.validatePath(path.resolve(os.homedir(), 'anywhere.txt')),
+      await assert.rejects(
+        context.validatePath(path.resolve(os.homedir(), 'anywhere.txt')),
         /Access denied/,
       );
     });

tests/roots.test.ts

@@ -5,6 +5,7 @@
  */
 
 import assert from 'node:assert';
+import fs from 'node:fs/promises';
 import os from 'node:os';
 import path from 'node:path';
 import {describe, it} from 'node:test';
@@ -18,7 +19,7 @@ describe('McpContext Roots', () => {
       context.setRoots([]);
       const tmpPath = path.join(os.tmpdir(), 'test-file.txt');
       // This should not throw
-      context.validatePath(tmpPath);
+      await context.validatePath(tmpPath);
     });
   });
 
@@ -26,24 +27,31 @@ describe('McpContext Roots', () => {
     await withMcpContext(async (_response, context) => {
       const otherRoot = path.resolve(
         os.tmpdir(),
-        '..',
         'other_workspace_root_for_test',
       );
-      context.setRoots([{uri: pathToFileURL(otherRoot).href, name: 'other'}]);
+      await fs.mkdir(otherRoot, {recursive: true});
+      try {
+        context.setRoots([{uri: pathToFileURL(otherRoot).href, name: 'other'}]);
 
-      const tmpPath = path.join(os.tmpdir(), 'test-file.txt');
-      // This should not throw.
-      context.validatePath(tmpPath);
+        const tmpPath = path.join(os.tmpdir(), 'test-file.txt');
+        // This should not throw.
+        await context.validatePath(tmpPath);
 
-      // Other root should also be allowed.
-      context.validatePath(path.join(otherRoot, 'file.txt'));
+        // Other root should also be allowed.
+        await context.validatePath(path.join(otherRoot, 'file.txt'));
 
-      // Outside should still be denied. Use a path that is definitely not a root or temp dir.
-      const outsidePath = path.resolve(
-        os.homedir(),
-        'a_very_unlikely_path_name_12345',
-      );
-      assert.throws(() => context.validatePath(outsidePath), /Access denied/);
+        // Outside should still be denied. Use a path that is definitely not a root or temp dir.
+        const outsidePath = path.resolve(
+          os.homedir(),
+          'a_very_unlikely_path_name_12345',
+        );
+        await assert.rejects(
+          context.validatePath(outsidePath),
+          /Access denied/,
+        );
+      } finally {
+        await fs.rm(otherRoot, {recursive: true, force: true});
+      }
     });
   });
 });

tests/utils/files.test.ts

@@ -5,9 +5,12 @@
  */
 
 import assert from 'node:assert';
+import fs from 'node:fs/promises';
+import os from 'node:os';
+import path from 'node:path';
 import {describe, it} from 'node:test';
 
-import {ensureExtension} from '../../src/utils/files.js';
+import {ensureExtension, resolveCanonicalPath} from '../../src/utils/files.js';
 
 describe('ensureExtension', () => {
   it('should add an extension to a filename without one', () => {
@@ -41,3 +44,85 @@ describe('ensureExtension', () => {
     assert.strictEqual(ensureExtension('file.tar.gz', '.zip'), 'file.tar.zip');
   });
 });
+
+describe('resolveCanonicalPath', () => {
+  it('should resolve an existing standard file path', async () => {
+    const tmpDir = await fs.mkdtemp(
+      path.join(os.tmpdir(), 'resolve-canonical-test-'),
+    );
+    try {
+      const filePath = path.join(tmpDir, 'test.txt');
+      await fs.writeFile(filePath, 'hello');
+
+      const resolved = await resolveCanonicalPath(filePath);
+      const canonicalTmpDir = await fs.realpath(tmpDir);
+      assert.strictEqual(resolved, path.join(canonicalTmpDir, 'test.txt'));
+    } finally {
+      await fs.rm(tmpDir, {recursive: true, force: true});
+    }
+  });
+
+  it('should resolve a non-existent file whose parent directory exists', async () => {
+    const tmpDir = await fs.mkdtemp(
+      path.join(os.tmpdir(), 'resolve-canonical-test-'),
+    );
+    try {
+      const filePath = path.join(tmpDir, 'non-existent.txt');
+
+      const resolved = await resolveCanonicalPath(filePath);
+      const canonicalTmpDir = await fs.realpath(tmpDir);
+      assert.strictEqual(
+        resolved,
+        path.join(canonicalTmpDir, 'non-existent.txt'),
+      );
+    } finally {
+      await fs.rm(tmpDir, {recursive: true, force: true});
+    }
+  });
+
+  it('should resolve a non-existent deeply nested file whose parent directories do not exist', async () => {
+    const tmpDir = await fs.mkdtemp(
+      path.join(os.tmpdir(), 'resolve-canonical-test-'),
+    );
+    try {
+      const filePath = path.join(
+        tmpDir,
+        'nested1',
+        'nested2',
+        'non-existent.txt',
+      );
+
+      const resolved = await resolveCanonicalPath(filePath);
+      const canonicalTmpDir = await fs.realpath(tmpDir);
+      assert.strictEqual(
+        resolved,
+        path.join(canonicalTmpDir, 'nested1', 'nested2', 'non-existent.txt'),
+      );
+    } finally {
+      await fs.rm(tmpDir, {recursive: true, force: true});
+    }
+  });
+
+  it('should resolve existing files with symlinks in path', async () => {
+    const tmpDir = await fs.mkdtemp(
+      path.join(os.tmpdir(), 'resolve-canonical-test-'),
+    );
+    try {
+      const targetDir = path.join(tmpDir, 'target');
+      await fs.mkdir(targetDir);
+      const targetFile = path.join(targetDir, 'file.txt');
+      await fs.writeFile(targetFile, 'hello');
+
+      const symlinkDir = path.join(tmpDir, 'symlink_dir');
+      await fs.symlink(targetDir, symlinkDir, 'dir');
+
+      const filePathWithSymlink = path.join(symlinkDir, 'file.txt');
+
+      const resolved = await resolveCanonicalPath(filePathWithSymlink);
+      const canonicalTargetDir = await fs.realpath(targetDir);
+      assert.strictEqual(resolved, path.join(canonicalTargetDir, 'file.txt'));
+    } finally {
+      await fs.rm(tmpDir, {recursive: true, force: true});
+    }
+  });
+});