From b3e4828476fdde362aca80c6c4859d2460abce76 Mon Sep 17 00:00:00 2001 From: Tomas Grasl Date: Tue, 25 Aug 2026 20:02:26 +0200 Subject: [PATCH 1/2] fix(windows): compare saveTo paths case-insensitively The saveTo boundary check compared the resolved path against its allowed root with a case-sensitive startsWith. Windows paths are case-insensitive, so `c:\Users\me\.firefox-devtools-mcp\out.json` was rejected as "outside the allowed location" while the identical path with a capital drive letter was accepted. Agents produce either spelling. Ignoring case on Windows only removes false rejections: the comparison now matches what the filesystem considers the same directory, so it cannot let through a path that previously escaped. --- src/utils/save-output.ts | 16 +++++++++++++- tests/utils/save-output.test.ts | 39 ++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/utils/save-output.ts b/src/utils/save-output.ts index c664c039..94df1021 100644 --- a/src/utils/save-output.ts +++ b/src/utils/save-output.ts @@ -29,6 +29,20 @@ async function isDirectory(path: string): Promise { } } +/** + * Whether `candidate` is `root` or sits inside it. Compared case-insensitively + * on Windows, where `c:\Users\...` and `C:\Users\...` are the same directory — + * matching case exactly would reject valid paths, not catch escapes. + */ +export function isWithinRoot(root: string, candidate: string): boolean { + const normalize = (path: string) => (process.platform === 'win32' ? path.toLowerCase() : path); + const normalizedRoot = normalize(root); + const normalizedCandidate = normalize(candidate); + return ( + normalizedCandidate === normalizedRoot || normalizedCandidate.startsWith(normalizedRoot + sep) + ); +} + /** * Reject saveTo paths that escape the allowed roots, unless the server was * started with --unrestricted-save-paths. Relative paths must stay within the @@ -41,7 +55,7 @@ async function assertAllowedPath(saveTo: string, resolvedPath: string): Promise< return; } const root = isAbsolute(saveTo) ? homeRoot() : process.cwd(); - if (resolvedPath !== root && !resolvedPath.startsWith(root + sep)) { + if (!isWithinRoot(root, resolvedPath)) { throw new Error( `saveTo "${saveTo}" resolves outside the allowed location (${resolvedPath}). Relative ` + `paths must stay within the current working directory and absolute paths within ` + diff --git a/tests/utils/save-output.test.ts b/tests/utils/save-output.test.ts index 0c3b88b0..7b4be1e6 100644 --- a/tests/utils/save-output.test.ts +++ b/tests/utils/save-output.test.ts @@ -18,7 +18,7 @@ vi.mock('node:os', async (importOriginal) => { const mockArgs = vi.hoisted(() => ({ unrestrictedSavePaths: false })); vi.mock('../../src/index.js', () => ({ args: mockArgs })); -import { saveOutput } from '../../src/utils/save-output.js'; +import { saveOutput, isWithinRoot } from '../../src/utils/save-output.js'; describe('saveOutput', () => { const tempDir = join(tmpdir(), `save-output-test-${process.pid}`); @@ -135,3 +135,40 @@ describe('saveOutput', () => { }); }); }); + +describe('isWithinRoot', () => { + const originalPlatform = process.platform; + const root = join('C:', 'Users', 'me', '.firefox-devtools-mcp'); + + const setPlatform = (platform: NodeJS.Platform) => + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + + afterEach(() => setPlatform(originalPlatform)); + + it('accepts the root itself and paths inside it', () => { + expect(isWithinRoot(root, root)).toBe(true); + expect(isWithinRoot(root, join(root, 'out.json'))).toBe(true); + expect(isWithinRoot(root, join(root, 'nested', 'out.json'))).toBe(true); + }); + + it('rejects paths outside the root, including prefix look-alikes', () => { + expect(isWithinRoot(root, join('C:', 'Users', 'me', 'secrets', 'out.json'))).toBe(false); + expect(isWithinRoot(root, `${root}-evil`)).toBe(false); + }); + + it('ignores case on Windows, where the filesystem does too', () => { + setPlatform('win32'); + expect(isWithinRoot(root, join(root.toLowerCase(), 'out.json'))).toBe(true); + expect(isWithinRoot(root, join(root.toUpperCase(), 'out.json'))).toBe(true); + }); + + it('still rejects an escape when case is ignored', () => { + setPlatform('win32'); + expect(isWithinRoot(root, join('c:', 'users', 'me', 'secrets', 'out.json'))).toBe(false); + }); + + it('matches case exactly off Windows', () => { + setPlatform('linux'); + expect(isWithinRoot(root, join(root.toLowerCase(), 'out.json'))).toBe(false); + }); +}); From 55fbccb836050d79191e3fab9c5be356c66a1863 Mon Sep 17 00:00:00 2001 From: Tomas Grasl Date: Tue, 25 Aug 2026 20:02:26 +0200 Subject: [PATCH 2/2] fix: create the parent directory for caller-supplied log paths Both log paths opened their file without ensuring the directory existed, while the auto-generated path right beside one of them already called mkdirSync. A --output-file whose directory was missing therefore threw ENOENT from deep inside connect(), and a --log-file in the same state silently disabled logging through a stream error. Windows makes this easy to hit, since paths like /tmp/foo.log do not exist there at all, but the gap is not platform-specific. saveOutput already creates parents for the paths it is given; this brings both log paths in line. --- src/firefox/core.ts | 6 +++++- src/utils/logger.ts | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/firefox/core.ts b/src/firefox/core.ts index ceb9d30c..57d1a56b 100644 --- a/src/firefox/core.ts +++ b/src/firefox/core.ts @@ -15,7 +15,7 @@ import { } from 'node:fs'; import { connect as netConnect } from 'node:net'; import { homedir } from 'node:os'; -import { join, delimiter } from 'node:path'; +import { dirname, join, delimiter } from 'node:path'; import type { FirefoxLaunchOptions } from './types.js'; import { log, logDebug } from '../utils/logger.js'; import { resolveProfilePath } from './profile.js'; @@ -363,6 +363,10 @@ export class FirefoxCore { } if (this.logFilePath) { + // Create the parent directory, as the generated-path branch above does. + // Without it a caller-supplied path whose directory is missing throws + // ENOENT from deep inside connect(). + mkdirSync(dirname(this.logFilePath), { recursive: true }); // Open file for appending, create if doesn't exist this.logFileFd = openSync(this.logFilePath, 'a'); serviceBuilder.setStdio(['ignore', this.logFileFd, this.logFileFd]); diff --git a/src/utils/logger.ts b/src/utils/logger.ts index a021445b..b071c6cc 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -1,4 +1,5 @@ import fs from 'node:fs'; +import path from 'node:path'; let logStream: fs.WriteStream | null = null; @@ -74,6 +75,9 @@ export function logError(message: string, error?: unknown): void { } export function setupLogFile(filePath: string): void { + // Create the parent directory first; otherwise a missing one only surfaces as + // a stream error and logging is silently disabled. + fs.mkdirSync(path.dirname(filePath), { recursive: true }); logStream = fs.createWriteStream(filePath, { flags: 'a' }); logStream.on('error', (error) => { console.error(`[firefox-devtools-mcp] Error writing to log file: ${error.message}`);