From 529c42c63bdadced6ea51dc0d20fc8710f7a3cd5 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 14 Sep 2026 12:45:00 -0700 Subject: [PATCH 1/4] fix(mcp): keep upload modal, skip short secrets and bad headers Do not redact secret values shorter than 4 characters. Ignore route header lines that have no name. Clear the file chooser only after setFiles succeeds so a failed upload can be retried. Signed-off-by: Sebastien Tardif --- .../src/tools/backend/context.ts | 2 +- .../src/tools/backend/files.ts | 2 +- .../src/tools/backend/route.ts | 6 ++- tests/mcp/files.spec.ts | 37 +++++++++++++++++++ tests/mcp/route.spec.ts | 36 ++++++++++++++++++ tests/mcp/secrets.spec.ts | 24 ++++++++++++ 6 files changed, 103 insertions(+), 4 deletions(-) diff --git a/packages/playwright-core/src/tools/backend/context.ts b/packages/playwright-core/src/tools/backend/context.ts index 05b22548a3059..0e706a306e50a 100644 --- a/packages/playwright-core/src/tools/backend/context.ts +++ b/packages/playwright-core/src/tools/backend/context.ts @@ -408,7 +408,7 @@ export class Context { redactSecrets(text: string): string { for (const [secretName, secretValue] of Object.entries(this.config.secrets ?? {})) { - if (!secretValue) + if (!secretValue || secretValue.length < 4) continue; text = text.replaceAll(secretValue, `${secretName}`); } diff --git a/packages/playwright-core/src/tools/backend/files.ts b/packages/playwright-core/src/tools/backend/files.ts index 44e1f5ee6badd..f1ce5dec76bad 100644 --- a/packages/playwright-core/src/tools/backend/files.ts +++ b/packages/playwright-core/src/tools/backend/files.ts @@ -43,11 +43,11 @@ export const uploadFile = defineTabTool({ response.addCode(`await fileChooser.setFiles(${JSON.stringify(paths)})`); - tab.clearModalState(modalState); await tab.waitForCompletion(async () => { if (paths) await modalState.fileChooser.setFiles(paths); }); + tab.clearModalState(modalState); }, clearsModalState: 'fileChooser', diff --git a/packages/playwright-core/src/tools/backend/route.ts b/packages/playwright-core/src/tools/backend/route.ts index 71a3ae014f93d..e59142e2e2336 100644 --- a/packages/playwright-core/src/tools/backend/route.ts +++ b/packages/playwright-core/src/tools/backend/route.ts @@ -40,9 +40,11 @@ const route = defineTool({ }, handle: async (context, params, response) => { - const addHeaders = params.headers ? Object.fromEntries(params.headers.map(h => { + const addHeaders = params.headers ? Object.fromEntries(params.headers.flatMap(h => { const colonIndex = h.indexOf(':'); - return [h.substring(0, colonIndex).trim(), h.substring(colonIndex + 1).trim()]; + if (colonIndex <= 0) + return []; + return [[h.substring(0, colonIndex).trim(), h.substring(colonIndex + 1).trim()]]; })) : undefined; const removeHeaders = params.removeHeaders ? params.removeHeaders.split(',').map(h => h.trim()) : undefined; diff --git a/tests/mcp/files.spec.ts b/tests/mcp/files.spec.ts index 583923a56a04f..8cc7ddaf9028e 100644 --- a/tests/mcp/files.spec.ts +++ b/tests/mcp/files.spec.ts @@ -103,6 +103,43 @@ test('browser_file_upload', async ({ client, server }, testInfo) => { } }); +test('browser_file_upload keeps chooser when setFiles fails', async ({ client, server }, testInfo) => { + server.setContent('/', ``, 'text/html'); + + await client.callTool({ + name: 'browser_navigate', + arguments: { url: server.PREFIX }, + }); + + await client.callTool({ + name: 'browser_click', + arguments: { + element: 'Textbox', + target: 'e2', + }, + }); + + const missing = testInfo.outputPath('missing.txt'); + const failed = await client.callTool({ + name: 'browser_file_upload', + arguments: { paths: [missing] }, + }); + expect(failed).toHaveResponse({ + isError: true, + modalState: expect.stringContaining(`[File chooser]`), + }); + + const filePath = testInfo.outputPath('retry.txt'); + await fs.writeFile(filePath, 'retry'); + const retried = await client.callTool({ + name: 'browser_file_upload', + arguments: { paths: [filePath] }, + }); + expect(retried).toHaveResponse({ + modalState: undefined, + }); +}); + test('clicking on download link emits download', async ({ startClient, server }, testInfo) => { const { client } = await startClient({ config: { outputDir: testInfo.outputPath('output') }, diff --git a/tests/mcp/route.spec.ts b/tests/mcp/route.spec.ts index 6c1b5267a38f4..76465d07fda57 100644 --- a/tests/mcp/route.spec.ts +++ b/tests/mcp/route.spec.ts @@ -136,6 +136,42 @@ test('browser_route modifies request headers', async ({ client, server }) => { expect(receivedHeaders['x-custom-header']).toBe('test-value'); }); +test('browser_route ignores header lines without a colon', async ({ client, server }) => { + let receivedHeaders: Record = {}; + server.setRoute('/api/check', (req, res) => { + receivedHeaders = req.headers as Record; + res.writeHead(200); + res.end('ok'); + }); + + server.setContent('/', ` + + `, 'text/html'); + + await client.callTool({ + name: 'browser_navigate', + arguments: { url: server.PREFIX }, + }); + + await client.callTool({ + name: 'browser_route', + arguments: { + pattern: '**/api/check', + headers: ['NotAHeader', 'X-Custom-Header: test-value'], + }, + }); + + const requestPromise = server.waitForRequest('/api/check'); + await client.callTool({ + name: 'browser_click', + arguments: { element: 'Fetch button', target: 'e2' }, + }); + + await requestPromise; + expect(receivedHeaders['x-custom-header']).toBe('test-value'); + expect(receivedHeaders['']).toBeUndefined(); +}); + test('browser_route_list shows active routes', async ({ client, server }) => { await client.callTool({ name: 'browser_navigate', diff --git a/tests/mcp/secrets.spec.ts b/tests/mcp/secrets.spec.ts index 266c7f34ea376..973d1f340f08d 100644 --- a/tests/mcp/secrets.spec.ts +++ b/tests/mcp/secrets.spec.ts @@ -164,6 +164,30 @@ await page.getByRole('textbox', { name: 'Password' }).fill(process.env['X-PASSWO }); }); +test('short secret values are not redacted', async ({ startClient, server }) => { + const secretsFile = test.info().outputPath('secrets.env'); + await fs.promises.writeFile(secretsFile, 'X-TINY=e'); + + const { client } = await startClient({ + args: ['--secrets', secretsFile], + }); + + server.setContent('/', ` + + +

hello

+ + `, 'text/html'); + + const response = await client.callTool({ + name: 'browser_navigate', + arguments: { url: server.PREFIX }, + }); + + expect(JSON.stringify(response)).not.toContain('X-TINY'); + expect(JSON.stringify(response)).toContain('hello'); +}); + test('empty secret value is ignored', async ({ startClient, server }) => { const secretsFile = test.info().outputPath('secrets.env'); await fs.promises.writeFile(secretsFile, 'EMPTY_SECRET=\nX-PASSWORD=password123'); From d6572182cd513c1d694cae475508f4429cab9c76 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 14 Sep 2026 20:28:21 -0700 Subject: [PATCH 2/4] fix(mcp): run setFiles before waitForCompletion waitForCompletion returns immediately when a fileChooser modal is already listed, so wrapping setFiles in it skipped the upload. Call setFiles first, keep the chooser on failure, then settle. Signed-off-by: Sebastien Tardif --- packages/playwright-core/src/tools/backend/files.ts | 11 ++++++++--- tests/mcp/secrets.spec.ts | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/playwright-core/src/tools/backend/files.ts b/packages/playwright-core/src/tools/backend/files.ts index f1ce5dec76bad..c35716f632cf1 100644 --- a/packages/playwright-core/src/tools/backend/files.ts +++ b/packages/playwright-core/src/tools/backend/files.ts @@ -43,11 +43,16 @@ export const uploadFile = defineTabTool({ response.addCode(`await fileChooser.setFiles(${JSON.stringify(paths)})`); - await tab.waitForCompletion(async () => { - if (paths) + if (paths) { + try { await modalState.fileChooser.setFiles(paths); - }); + } catch (e) { + response.addError(e instanceof Error ? e.message : String(e)); + return; + } + } tab.clearModalState(modalState); + await tab.waitForCompletion(async () => {}); }, clearsModalState: 'fileChooser', diff --git a/tests/mcp/secrets.spec.ts b/tests/mcp/secrets.spec.ts index 973d1f340f08d..ac90a86ddb2a7 100644 --- a/tests/mcp/secrets.spec.ts +++ b/tests/mcp/secrets.spec.ts @@ -184,8 +184,10 @@ test('short secret values are not redacted', async ({ startClient, server }) => arguments: { url: server.PREFIX }, }); + expect(response).toHaveResponse({ + snapshot: expect.stringContaining('hello'), + }); expect(JSON.stringify(response)).not.toContain('X-TINY'); - expect(JSON.stringify(response)).toContain('hello'); }); test('empty secret value is ignored', async ({ startClient, server }) => { From b69aeeb7571385a77216cf88f549828c93008475 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Tue, 15 Sep 2026 16:54:43 -0700 Subject: [PATCH 3/4] fix(mcp): error on route headers without a colon Return an argument error for values that are not Name: Value. Signed-off-by: Sebastien Tardif --- .../src/tools/backend/route.ts | 18 +++++++---- tests/mcp/route.spec.ts | 30 ++++--------------- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/packages/playwright-core/src/tools/backend/route.ts b/packages/playwright-core/src/tools/backend/route.ts index e59142e2e2336..9c2e293c0d13c 100644 --- a/packages/playwright-core/src/tools/backend/route.ts +++ b/packages/playwright-core/src/tools/backend/route.ts @@ -40,12 +40,18 @@ const route = defineTool({ }, handle: async (context, params, response) => { - const addHeaders = params.headers ? Object.fromEntries(params.headers.flatMap(h => { - const colonIndex = h.indexOf(':'); - if (colonIndex <= 0) - return []; - return [[h.substring(0, colonIndex).trim(), h.substring(colonIndex + 1).trim()]]; - })) : undefined; + let addHeaders: Record | undefined; + if (params.headers) { + addHeaders = {}; + for (const h of params.headers) { + const colonIndex = h.indexOf(':'); + if (colonIndex <= 0) { + response.addError(`Invalid header "${h}": expected "Name: Value"`); + return; + } + addHeaders[h.substring(0, colonIndex).trim()] = h.substring(colonIndex + 1).trim(); + } + } const removeHeaders = params.removeHeaders ? params.removeHeaders.split(',').map(h => h.trim()) : undefined; const handler = async (route: playwright.Route) => { diff --git a/tests/mcp/route.spec.ts b/tests/mcp/route.spec.ts index 76465d07fda57..0f627d30868b7 100644 --- a/tests/mcp/route.spec.ts +++ b/tests/mcp/route.spec.ts @@ -136,40 +136,22 @@ test('browser_route modifies request headers', async ({ client, server }) => { expect(receivedHeaders['x-custom-header']).toBe('test-value'); }); -test('browser_route ignores header lines without a colon', async ({ client, server }) => { - let receivedHeaders: Record = {}; - server.setRoute('/api/check', (req, res) => { - receivedHeaders = req.headers as Record; - res.writeHead(200); - res.end('ok'); - }); - - server.setContent('/', ` - - `, 'text/html'); - +test('browser_route errors on header lines without a colon', async ({ client, server }) => { await client.callTool({ name: 'browser_navigate', - arguments: { url: server.PREFIX }, + arguments: { url: server.EMPTY_PAGE }, }); - await client.callTool({ + expect(await client.callTool({ name: 'browser_route', arguments: { pattern: '**/api/check', headers: ['NotAHeader', 'X-Custom-Header: test-value'], }, + })).toHaveResponse({ + isError: true, + error: expect.stringContaining('Invalid header "NotAHeader"'), }); - - const requestPromise = server.waitForRequest('/api/check'); - await client.callTool({ - name: 'browser_click', - arguments: { element: 'Fetch button', target: 'e2' }, - }); - - await requestPromise; - expect(receivedHeaders['x-custom-header']).toBe('test-value'); - expect(receivedHeaders['']).toBeUndefined(); }); test('browser_route_list shows active routes', async ({ client, server }) => { From 36971b0027ae613530d4d058955bb9958a502020 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Thu, 17 Sep 2026 12:38:29 -0700 Subject: [PATCH 4/4] fix(mcp): wrap setFiles in waitForCompletion Clear the file chooser first so waitForCompletion can run, then setFiles inside it so WebKit upload requests and change-handler dialogs are observed. Restore the chooser if setFiles fails. Redact all non-empty secret values. Do not skip short ones. Signed-off-by: Sebastien Tardif --- .../src/tools/backend/context.ts | 2 +- .../src/tools/backend/files.ts | 19 +++++++------- tests/mcp/secrets.spec.ts | 26 ------------------- 3 files changed, 11 insertions(+), 36 deletions(-) diff --git a/packages/playwright-core/src/tools/backend/context.ts b/packages/playwright-core/src/tools/backend/context.ts index 0e706a306e50a..05b22548a3059 100644 --- a/packages/playwright-core/src/tools/backend/context.ts +++ b/packages/playwright-core/src/tools/backend/context.ts @@ -408,7 +408,7 @@ export class Context { redactSecrets(text: string): string { for (const [secretName, secretValue] of Object.entries(this.config.secrets ?? {})) { - if (!secretValue || secretValue.length < 4) + if (!secretValue) continue; text = text.replaceAll(secretValue, `${secretName}`); } diff --git a/packages/playwright-core/src/tools/backend/files.ts b/packages/playwright-core/src/tools/backend/files.ts index c35716f632cf1..86160f2e3cbc1 100644 --- a/packages/playwright-core/src/tools/backend/files.ts +++ b/packages/playwright-core/src/tools/backend/files.ts @@ -43,16 +43,17 @@ export const uploadFile = defineTabTool({ response.addCode(`await fileChooser.setFiles(${JSON.stringify(paths)})`); - if (paths) { - try { - await modalState.fileChooser.setFiles(paths); - } catch (e) { - response.addError(e instanceof Error ? e.message : String(e)); - return; - } - } tab.clearModalState(modalState); - await tab.waitForCompletion(async () => {}); + try { + await tab.waitForCompletion(async () => { + if (paths) + await modalState.fileChooser.setFiles(paths); + }); + } catch (e) { + tab.setModalState(modalState); + response.addError(e instanceof Error ? e.message : String(e)); + return; + } }, clearsModalState: 'fileChooser', diff --git a/tests/mcp/secrets.spec.ts b/tests/mcp/secrets.spec.ts index ac90a86ddb2a7..266c7f34ea376 100644 --- a/tests/mcp/secrets.spec.ts +++ b/tests/mcp/secrets.spec.ts @@ -164,32 +164,6 @@ await page.getByRole('textbox', { name: 'Password' }).fill(process.env['X-PASSWO }); }); -test('short secret values are not redacted', async ({ startClient, server }) => { - const secretsFile = test.info().outputPath('secrets.env'); - await fs.promises.writeFile(secretsFile, 'X-TINY=e'); - - const { client } = await startClient({ - args: ['--secrets', secretsFile], - }); - - server.setContent('/', ` - - -

hello

- - `, 'text/html'); - - const response = await client.callTool({ - name: 'browser_navigate', - arguments: { url: server.PREFIX }, - }); - - expect(response).toHaveResponse({ - snapshot: expect.stringContaining('hello'), - }); - expect(JSON.stringify(response)).not.toContain('X-TINY'); -}); - test('empty secret value is ignored', async ({ startClient, server }) => { const secretsFile = test.info().outputPath('secrets.env'); await fs.promises.writeFile(secretsFile, 'EMPTY_SECRET=\nX-PASSWORD=password123');