From a4bf3ebfbaec425080e9736b3bbccf93ddbbe26d Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 19 Sep 2025 13:31:33 -0700 Subject: [PATCH] fix(mcp): lax file path sanitization --- .../src/mcp/browser/browserContextFactory.ts | 4 ++-- packages/playwright/src/mcp/browser/config.ts | 20 +++++++++++++++---- .../playwright/src/mcp/browser/context.ts | 4 ++-- .../playwright/src/mcp/browser/sessionLog.ts | 2 +- packages/playwright/src/mcp/browser/tab.ts | 2 +- .../playwright/src/mcp/browser/tools/pdf.ts | 3 ++- .../src/mcp/browser/tools/screenshot.ts | 4 ++-- .../src/mcp/browser/tools/tracing.ts | 2 +- .../playwright/src/mcp/browser/tools/utils.ts | 5 +++++ 9 files changed, 32 insertions(+), 14 deletions(-) diff --git a/packages/playwright/src/mcp/browser/browserContextFactory.ts b/packages/playwright/src/mcp/browser/browserContextFactory.ts index 97cf9f12201af..4152d70605251 100644 --- a/packages/playwright/src/mcp/browser/browserContextFactory.ts +++ b/packages/playwright/src/mcp/browser/browserContextFactory.ts @@ -106,7 +106,7 @@ class IsolatedContextFactory extends BaseContextFactory { protected override async _doObtainBrowser(clientInfo: ClientInfo): Promise { await injectCdpPort(this.config.browser); const browserType = playwright[this.config.browser.browserName]; - const tracesDir = await outputFile(this.config, clientInfo, `traces`); + const tracesDir = await outputFile(this.config, clientInfo, `traces`, { origin: 'code' }); if (this.config.saveTrace) await startTraceServer(this.config, tracesDir); return browserType.launch({ @@ -173,7 +173,7 @@ class PersistentContextFactory implements BrowserContextFactory { await injectCdpPort(this.config.browser); testDebug('create browser context (persistent)'); const userDataDir = this.config.browser.userDataDir ?? await this._createUserDataDir(clientInfo); - const tracesDir = await outputFile(this.config, clientInfo, `traces`); + const tracesDir = await outputFile(this.config, clientInfo, `traces`, { origin: 'code' }); if (this.config.saveTrace) await startTraceServer(this.config, tracesDir); diff --git a/packages/playwright/src/mcp/browser/config.ts b/packages/playwright/src/mcp/browser/config.ts index b41d07f585452..0aa7124d99b35 100644 --- a/packages/playwright/src/mcp/browser/config.ts +++ b/packages/playwright/src/mcp/browser/config.ts @@ -271,15 +271,27 @@ async function loadConfig(configFile: string | undefined): Promise { } } -export async function outputFile(config: FullConfig, clientInfo: ClientInfo, name: string): Promise { +export async function outputFile(config: FullConfig, clientInfo: ClientInfo, fileName: string, options: { origin: 'code' | 'llm' | 'web' }): Promise { const rootPath = firstRootPath(clientInfo); const outputDir = config.outputDir ?? (rootPath ? path.join(rootPath, '.playwright-mcp') : undefined) ?? path.join(process.env.PW_TMPDIR_FOR_TEST ?? os.tmpdir(), 'playwright-mcp-output', String(clientInfo.timestamp)); - await fs.promises.mkdir(outputDir, { recursive: true }); - const fileName = sanitizeForFilePath(name); - return path.join(outputDir, fileName); + // Trust code. + if (options.origin === 'code') + return path.resolve(outputDir, fileName); + + // Trust llm to use valid characters in file names. + if (options.origin === 'llm') { + fileName = fileName.split('\\').join('/'); + const resolvedFile = path.resolve(outputDir, fileName); + if (!resolvedFile.startsWith(path.resolve(outputDir) + path.sep)) + throw new Error(`Resolved file path for ${fileName} is outside of the output directory`); + return resolvedFile; + } + + // Do not trust web, at all. + return path.join(outputDir, sanitizeForFilePath(fileName)); } function pickDefined(obj: T | undefined): Partial { diff --git a/packages/playwright/src/mcp/browser/context.ts b/packages/playwright/src/mcp/browser/context.ts index c353a87ee683b..f06c8155dd7db 100644 --- a/packages/playwright/src/mcp/browser/context.ts +++ b/packages/playwright/src/mcp/browser/context.ts @@ -113,8 +113,8 @@ export class Context { return url; } - async outputFile(name: string): Promise { - return outputFile(this.config, this._clientInfo, name); + async outputFile(fileName: string, options: { origin: 'code' | 'llm' | 'web' }): Promise { + return outputFile(this.config, this._clientInfo, fileName, options); } private _onPageCreated(page: playwright.Page) { diff --git a/packages/playwright/src/mcp/browser/sessionLog.ts b/packages/playwright/src/mcp/browser/sessionLog.ts index 4b39ea4c76388..ef236770951cf 100644 --- a/packages/playwright/src/mcp/browser/sessionLog.ts +++ b/packages/playwright/src/mcp/browser/sessionLog.ts @@ -53,7 +53,7 @@ export class SessionLog { } static async create(config: FullConfig, clientInfo: mcpServer.ClientInfo): Promise { - const sessionFolder = await outputFile(config, clientInfo, `session-${Date.now()}`); + const sessionFolder = await outputFile(config, clientInfo, `session-${Date.now()}`, { origin: 'code' }); await fs.promises.mkdir(sessionFolder, { recursive: true }); // eslint-disable-next-line no-console console.error(`Session: ${sessionFolder}`); diff --git a/packages/playwright/src/mcp/browser/tab.ts b/packages/playwright/src/mcp/browser/tab.ts index 8a5dd8eb77e22..1669d517676e2 100644 --- a/packages/playwright/src/mcp/browser/tab.ts +++ b/packages/playwright/src/mcp/browser/tab.ts @@ -117,7 +117,7 @@ export class Tab extends EventEmitter { const entry = { download, finished: false, - outputFile: await this.context.outputFile(download.suggestedFilename()) + outputFile: await this.context.outputFile(download.suggestedFilename(), { origin: 'web' }) }; this._downloads.push(entry); await download.saveAs(entry.outputFile); diff --git a/packages/playwright/src/mcp/browser/tools/pdf.ts b/packages/playwright/src/mcp/browser/tools/pdf.ts index b4c9b14ef46a8..91d142dcefcbc 100644 --- a/packages/playwright/src/mcp/browser/tools/pdf.ts +++ b/packages/playwright/src/mcp/browser/tools/pdf.ts @@ -17,6 +17,7 @@ import { z } from '../../sdk/bundle'; import { defineTabTool } from './tool'; import * as javascript from '../codegen'; +import { dateAsFileName } from './utils'; const pdfSchema = z.object({ filename: z.string().optional().describe('File name to save the pdf to. Defaults to `page-{timestamp}.pdf` if not specified.'), @@ -34,7 +35,7 @@ const pdf = defineTabTool({ }, handle: async (tab, params, response) => { - const fileName = await tab.context.outputFile(params.filename ?? `page-${new Date().toISOString()}.pdf`); + const fileName = await tab.context.outputFile(params.filename ?? `page-${dateAsFileName()}.pdf`, { origin: 'llm' }); response.addCode(`await page.pdf(${javascript.formatObject({ path: fileName })});`); response.addResult(`Saved page as ${fileName}`); await tab.page.pdf({ path: fileName }); diff --git a/packages/playwright/src/mcp/browser/tools/screenshot.ts b/packages/playwright/src/mcp/browser/tools/screenshot.ts index 4ee5cf81d91af..c3d0c383be4e1 100644 --- a/packages/playwright/src/mcp/browser/tools/screenshot.ts +++ b/packages/playwright/src/mcp/browser/tools/screenshot.ts @@ -17,7 +17,7 @@ import { z } from '../../sdk/bundle'; import { defineTabTool } from './tool'; import * as javascript from '../codegen'; -import { generateLocator } from './utils'; +import { generateLocator, dateAsFileName } from './utils'; import type * as playwright from 'playwright-core'; @@ -51,7 +51,7 @@ const screenshot = defineTabTool({ handle: async (tab, params, response) => { const fileType = params.type || 'png'; - const fileName = await tab.context.outputFile(params.filename ?? `page-${new Date().toISOString()}.${fileType}`); + const fileName = await tab.context.outputFile(params.filename ?? `page-${dateAsFileName()}.${fileType}`, { origin: 'llm' }); const options: playwright.PageScreenshotOptions = { type: fileType, quality: fileType === 'png' ? undefined : 90, diff --git a/packages/playwright/src/mcp/browser/tools/tracing.ts b/packages/playwright/src/mcp/browser/tools/tracing.ts index 828386c40d29c..613a9ae0dc47a 100644 --- a/packages/playwright/src/mcp/browser/tools/tracing.ts +++ b/packages/playwright/src/mcp/browser/tools/tracing.ts @@ -32,7 +32,7 @@ const tracingStart = defineTool({ handle: async (context, params, response) => { const browserContext = await context.ensureBrowserContext(); - const tracesDir = await context.outputFile(`traces`); + const tracesDir = await context.outputFile(`traces`, { origin: 'code' }); const name = 'trace-' + Date.now(); await (browserContext.tracing as Tracing).start({ name, diff --git a/packages/playwright/src/mcp/browser/tools/utils.ts b/packages/playwright/src/mcp/browser/tools/utils.ts index 2aedef1e507fd..8f5b3849247db 100644 --- a/packages/playwright/src/mcp/browser/tools/utils.ts +++ b/packages/playwright/src/mcp/browser/tools/utils.ts @@ -82,3 +82,8 @@ export async function generateLocator(locator: playwright.Locator): Promise(page: playwright.Page, callback: (page: playwright.Page) => Promise): Promise { return await (page as any)._wrapApiCall(() => callback(page), { internal: true }); } + +export function dateAsFileName(): string { + const date = new Date(); + return date.toISOString().replace(/[:.]/g, '-'); +}