diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 443acbd4..3521f05f 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -8,7 +8,7 @@ import { getCleanText } from './read-commands'; import { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS, PAGE_CONTENT_COMMANDS, wrapUntrustedContent, canonicalizeCommand } from './commands'; import { validateNavigationUrl } from './url-validation'; import { checkScope, type TokenInfo } from './token-registry'; -import { validateOutputPath, escapeRegExp } from './path-security'; +import { validateOutputPath, validateReadPath, SAFE_DIRECTORIES, escapeRegExp } from './path-security'; // Re-export for backward compatibility (tests import from meta-commands) export { validateOutputPath, escapeRegExp } from './path-security'; import * as Diff from 'diff'; @@ -134,6 +134,17 @@ function parsePdfArgs(args: string[]): ParsedPdfArgs { } function parsePdfFromFile(payloadPath: string): ParsedPdfArgs { + // Parity with load-html --from-file (browse/src/write-commands.ts) and + // the direct load-html path: every caller-supplied file path + // must pass validateReadPath so the safe-dirs policy can't be skirted + // by routing reads through the --from-file shortcut. + try { + validateReadPath(path.resolve(payloadPath)); + } catch { + throw new Error( + `pdf: --from-file ${payloadPath} must be under ${SAFE_DIRECTORIES.join(' or ')} (security policy). Copy the payload into the project tree or /tmp first.` + ); + } const raw = fs.readFileSync(payloadPath, 'utf8'); const json = JSON.parse(raw); const out: ParsedPdfArgs = { diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index f74f045e..73896ba3 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -188,6 +188,19 @@ export async function handleWriteCommand( if (args[i] === '--from-file') { const payloadPath = args[++i]; if (!payloadPath) throw new Error('load-html: --from-file requires a path'); + // Parity with the sibling `load-html ` path below (line 249): + // that branch runs every `file://` target through validateReadPath + // so the safe-dirs policy can't be side-stepped. Same policy must + // apply here — otherwise --from-file becomes a read-anywhere escape + // hatch for any caller that can pick the payload path (e.g., an + // MCP caller issuing load-html with an attacker-influenced path). + try { + validateReadPath(path.resolve(payloadPath)); + } catch { + throw new Error( + `load-html: --from-file ${payloadPath} must be under ${SAFE_DIRECTORIES.join(' or ')} (security policy). Copy the payload into the project tree or /tmp first.` + ); + } const raw = fs.readFileSync(payloadPath, 'utf8'); let json: any; try { json = JSON.parse(raw); } diff --git a/browse/test/from-file-path-validation.test.ts b/browse/test/from-file-path-validation.test.ts new file mode 100644 index 00000000..8128ae1d --- /dev/null +++ b/browse/test/from-file-path-validation.test.ts @@ -0,0 +1,68 @@ +/** + * Source-level guardrail for the --from-file shortcut flags. + * + * Context: both `load-html ` (write-commands.ts) and `pdf ` + * (meta-commands.ts) support a `--from-file ` shortcut that + * reads a JSON payload with the inline content (HTML body / PDF options). + * The DIRECT `load-html ` path runs every caller-supplied file path + * through `validateReadPath()` so reads are confined to SAFE_DIRECTORIES. + * The `--from-file` paths historically skipped this validation, opening a + * parity gap: an MCP caller that can pick the payload path could route + * reads through --from-file to bypass the safe-dirs policy. + * + * This test inspects the source to make sure both --from-file sites call + * validateReadPath before fs.readFileSync. Pattern mirrors + * postgres-engine.test.ts and pglite-search-timeout.test.ts. + */ + +import { describe, test, expect } from 'bun:test'; +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const ROOT = join(import.meta.dir, '..', 'src'); +const WRITE_SRC = readFileSync(join(ROOT, 'write-commands.ts'), 'utf-8'); +const META_SRC = readFileSync(join(ROOT, 'meta-commands.ts'), 'utf-8'); + +function stripComments(s: string): string { + return s.replace(/\/\*[\s\S]*?\*\//g, '').replace(/(^|\s)\/\/[^\n]*/g, '$1'); +} + +describe('--from-file path validation parity', () => { + test('load-html --from-file validates payload path before reading', () => { + const stripped = stripComments(WRITE_SRC); + // Grab the --from-file branch body. + const idx = stripped.indexOf("'--from-file'"); + expect(idx).toBeGreaterThan(-1); + const fromFileBranch = stripped.slice(idx, idx + 1200); + + // validateReadPath must appear BEFORE the readFileSync in the branch. + const vIdx = fromFileBranch.indexOf('validateReadPath'); + const rIdx = fromFileBranch.indexOf('readFileSync'); + expect(vIdx).toBeGreaterThan(-1); + expect(rIdx).toBeGreaterThan(-1); + expect(vIdx).toBeLessThan(rIdx); + }); + + test('pdf --from-file validates payload path before reading', () => { + const stripped = stripComments(META_SRC); + const idx = stripped.indexOf('function parsePdfFromFile'); + expect(idx).toBeGreaterThan(-1); + const fnBody = stripped.slice(idx, idx + 1200); + + const vIdx = fnBody.indexOf('validateReadPath'); + const rIdx = fnBody.indexOf('readFileSync'); + expect(vIdx).toBeGreaterThan(-1); + expect(rIdx).toBeGreaterThan(-1); + expect(vIdx).toBeLessThan(rIdx); + }); + + test('both sites reference SAFE_DIRECTORIES in the error message', () => { + // Error shape parity so ops teams / agents see a consistent message. + const write = stripComments(WRITE_SRC); + const meta = stripComments(META_SRC); + // load-html --from-file error + expect(write).toMatch(/load-html: --from-file [\s\S]{0,80}SAFE_DIRECTORIES/); + // pdf --from-file error + expect(meta).toMatch(/pdf: --from-file [\s\S]{0,80}SAFE_DIRECTORIES/); + }); +});