From 73f5d0b77de1699822d99de8e38fcad94accddd3 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 7 Apr 2026 18:56:46 -1000 Subject: [PATCH] refactor: extract path-security.ts shared module validateOutputPath, validateReadPath, and SAFE_DIRECTORIES were duplicated across write-commands.ts, meta-commands.ts, and read-commands.ts. Extract to a single shared module with re-exports for backward compatibility. Also adds validateTempPath() for the upcoming GET /file endpoint (TEMP_DIR only, not cwd, to prevent remote agents from reading project files). Co-Authored-By: Claude Opus 4.6 (1M context) --- browse/src/meta-commands.ts | 40 +--------- browse/src/path-security.ts | 103 ++++++++++++++++++++++++++ browse/src/read-commands.ts | 37 +-------- browse/src/write-commands.ts | 46 +----------- browse/test/security-audit-r2.test.ts | 26 +++---- 5 files changed, 126 insertions(+), 126 deletions(-) create mode 100644 browse/src/path-security.ts diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index e4abf696..1eaea71c 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -8,48 +8,16 @@ import { getCleanText } from './read-commands'; import { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS, PAGE_CONTENT_COMMANDS, wrapUntrustedContent } from './commands'; import { validateNavigationUrl } from './url-validation'; import { checkScope, type TokenInfo } from './token-registry'; +import { validateOutputPath, 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'; import * as fs from 'fs'; import * as path from 'path'; -import { TEMP_DIR, isPathWithin } from './platform'; +import { TEMP_DIR } from './platform'; import { resolveConfig } from './config'; import type { Frame } from 'playwright'; -// Security: Path validation to prevent path traversal attacks -// Resolve safe directories through realpathSync to handle symlinks (e.g., macOS /tmp → /private/tmp) -const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()].map(d => { - try { return fs.realpathSync(d); } catch { return d; } -}); - -export function validateOutputPath(filePath: string): void { - const resolved = path.resolve(filePath); - - // Resolve real path of the parent directory to catch symlinks. - // The file itself may not exist yet (e.g., screenshot output). - let dir = path.dirname(resolved); - let realDir: string; - try { - realDir = fs.realpathSync(dir); - } catch { - try { - realDir = fs.realpathSync(path.dirname(dir)); - } catch { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); - } - } - - const realResolved = path.join(realDir, path.basename(resolved)); - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realResolved, dir)); - if (!isSafe) { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); - } -} - -/** Escape special regex metacharacters in a user-supplied string to prevent ReDoS. */ -export function escapeRegExp(s: string): string { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - /** Tokenize a pipe segment respecting double-quoted strings. */ function tokenizePipeSegment(segment: string): string[] { const tokens: string[] = []; diff --git a/browse/src/path-security.ts b/browse/src/path-security.ts new file mode 100644 index 00000000..4b1961b0 --- /dev/null +++ b/browse/src/path-security.ts @@ -0,0 +1,103 @@ +/** + * Shared path validation — single source of truth for file path security. + * + * Previously duplicated across write-commands.ts, meta-commands.ts, and read-commands.ts. + * All file I/O commands (screenshot, pdf, download, scrape, archive, eval) must + * validate paths through these functions. + * + * validateOutputPath(path) — for writing files (screenshot, pdf, download, scrape, archive) + * validateReadPath(path) — for reading files (eval) + * validateTempPath(path) — for serving files to remote agents (GET /file, TEMP_DIR only) + * + * Security invariants: + * 1. All paths resolved to absolute before checking + * 2. Symlinks resolved to catch traversal via symlink inside safe dir + * 3. SAFE_DIRECTORIES = [TEMP_DIR, cwd] for local commands + * 4. TEMP_ONLY = [TEMP_DIR] for remote file serving (prevents project file exfil) + */ + +import * as fs from 'fs'; +import * as path from 'path'; +import { TEMP_DIR, isPathWithin } from './platform'; + +// Resolve safe directories through realpathSync to handle symlinks (e.g., macOS /tmp → /private/tmp) +export const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()].map(d => { + try { return fs.realpathSync(d); } catch { return d; } +}); + +const TEMP_ONLY = [TEMP_DIR].map(d => { + try { return fs.realpathSync(d); } catch { return d; } +}); + +/** Validate a file path for writing (screenshot, pdf, download, scrape, archive). */ +export function validateOutputPath(filePath: string): void { + const resolved = path.resolve(filePath); + + // Resolve real path of the parent directory to catch symlinks. + // The file itself may not exist yet (e.g., screenshot output). + // This also handles macOS /tmp → /private/tmp transparently. + let dir = path.dirname(resolved); + let realDir: string; + try { + realDir = fs.realpathSync(dir); + } catch { + try { + realDir = fs.realpathSync(path.dirname(dir)); + } catch { + throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); + } + } + + const realResolved = path.join(realDir, path.basename(resolved)); + const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realResolved, dir)); + if (!isSafe) { + throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); + } +} + +/** Validate a file path for reading (eval command). */ +export function validateReadPath(filePath: string): void { + const resolved = path.resolve(filePath); + let realPath: string; + try { + realPath = fs.realpathSync(resolved); + } catch (err: any) { + if (err.code === 'ENOENT') { + try { + const dir = fs.realpathSync(path.dirname(resolved)); + realPath = path.join(dir, path.basename(resolved)); + } catch { + realPath = resolved; + } + } else { + throw new Error(`Cannot resolve real path: ${filePath} (${err.code})`); + } + } + const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realPath, dir)); + if (!isSafe) { + throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); + } +} + +/** Validate a file path for remote serving (GET /file). TEMP_DIR only, not cwd. */ +export function validateTempPath(filePath: string): void { + const resolved = path.resolve(filePath); + let realPath: string; + try { + realPath = fs.realpathSync(resolved); + } catch (err: any) { + if (err.code === 'ENOENT') { + throw new Error('File not found'); + } + throw new Error(`Cannot resolve path: ${filePath}`); + } + const isSafe = TEMP_ONLY.some(dir => isPathWithin(realPath, dir)); + if (!isSafe) { + throw new Error(`Path must be within: ${TEMP_ONLY.join(', ')} (remote file serving is restricted to temp directory)`); + } +} + +/** Escape special regex metacharacters in a user-supplied string to prevent ReDoS. */ +export function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/browse/src/read-commands.ts b/browse/src/read-commands.ts index 03b327af..6091ed25 100644 --- a/browse/src/read-commands.ts +++ b/browse/src/read-commands.ts @@ -10,8 +10,11 @@ import { consoleBuffer, networkBuffer, dialogBuffer } from './buffers'; import type { Page, Frame } from 'playwright'; import * as fs from 'fs'; import * as path from 'path'; -import { TEMP_DIR, isPathWithin } from './platform'; +import { TEMP_DIR } from './platform'; import { inspectElement, formatInspectorResult, getModificationHistory } from './cdp-inspector'; +import { validateReadPath } from './path-security'; +// Re-export for backward compatibility (tests import from read-commands) +export { validateReadPath } from './path-security'; // Redaction patterns for sensitive cookie/storage values — exported for test coverage export const SENSITIVE_COOKIE_NAME = /(^|[_.-])(token|secret|key|password|credential|auth|jwt|session|csrf|sid)($|[_.-])|api.?key/i; @@ -41,38 +44,6 @@ function wrapForEvaluate(code: string): string { : `(async()=>(${trimmed}))()`; } -// Security: Path validation to prevent path traversal attacks -// Resolve safe directories through realpathSync to handle symlinks (e.g., macOS /tmp → /private/tmp) -const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()].map(d => { - try { return fs.realpathSync(d); } catch { return d; } -}); - -export function validateReadPath(filePath: string): void { - // Always resolve to absolute first (fixes relative path symlink bypass) - const resolved = path.resolve(filePath); - // Resolve symlinks — throw on non-ENOENT errors - let realPath: string; - try { - realPath = fs.realpathSync(resolved); - } catch (err: any) { - if (err.code === 'ENOENT') { - // File doesn't exist — resolve directory part for symlinks (e.g., /tmp → /private/tmp) - try { - const dir = fs.realpathSync(path.dirname(resolved)); - realPath = path.join(dir, path.basename(resolved)); - } catch { - realPath = resolved; - } - } else { - throw new Error(`Cannot resolve real path: ${filePath} (${err.code})`); - } - } - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realPath, dir)); - if (!isSafe) { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); - } -} - /** * Extract clean text from a page (strips script/style/noscript/svg). * Exported for DRY reuse in meta-commands (diff). diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 48550f1f..07899616 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -8,54 +8,12 @@ import type { BrowserManager } from './browser-manager'; import { findInstalledBrowsers, importCookies, listSupportedBrowserNames } from './cookie-import-browser'; import { validateNavigationUrl } from './url-validation'; +import { validateOutputPath } from './path-security'; import * as fs from 'fs'; import * as path from 'path'; -import { TEMP_DIR, isPathWithin } from './platform'; +import { TEMP_DIR } from './platform'; import { modifyStyle, undoModification, resetModifications, getModificationHistory } from './cdp-inspector'; -// Security: Path validation for screenshot output -// Resolve safe directories through realpathSync to handle symlinks (e.g., macOS /tmp -> /private/tmp) -const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()].map(d => { - try { return fs.realpathSync(d); } catch { return d; } -}); - -function validateOutputPath(filePath: string): void { - const resolved = path.resolve(filePath); - - // Basic containment check using lexical resolution only. - // This catches obvious traversal (../../../etc/passwd) but NOT symlinks. - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(resolved, dir)); - if (!isSafe) { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); - } - - // Symlink check: resolve the real path of the nearest existing ancestor - // directory and re-validate. This closes the symlink bypass where a - // symlink inside /tmp or cwd points outside the safe zone. - // - // We resolve the parent dir (not the file itself — it may not exist yet). - // If the parent doesn't exist either we fall back up the tree. - let dir = path.dirname(resolved); - let realDir: string; - try { - realDir = fs.realpathSync(dir); - } catch { - // Parent doesn't exist — check the grandparent, or skip if inaccessible - try { - realDir = fs.realpathSync(path.dirname(dir)); - } catch { - // Can't resolve — fail safe - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); - } - } - - const realResolved = path.join(realDir, path.basename(resolved)); - const isRealSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realResolved, dir)); - if (!isRealSafe) { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')} (symlink target blocked)`); - } -} - /** * Aggressive page cleanup selectors and heuristics. * Goal: make the page readable and clean while keeping it recognizable. diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index e1ff1d3d..985a53ed 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -17,6 +17,7 @@ const WRITE_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/write-comma const SERVER_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/server.ts'), 'utf-8'); const AGENT_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/sidebar-agent.ts'), 'utf-8'); const SNAPSHOT_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/snapshot.ts'), 'utf-8'); +const PATH_SECURITY_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/path-security.ts'), 'utf-8'); // ─── Helper ───────────────────────────────────────────────────────────────── @@ -159,26 +160,25 @@ describe('Task 2: CSS value validator blocks dangerous patterns', () => { describe('Task 1: validateOutputPath uses realpathSync', () => { describe('source-level checks', () => { - it('meta-commands.ts validateOutputPath contains realpathSync', () => { - const fn = extractFunction(META_SRC, 'validateOutputPath'); + it('path-security.ts validateOutputPath contains realpathSync', () => { + const fn = extractFunction(PATH_SECURITY_SRC, 'validateOutputPath'); expect(fn).toBeTruthy(); expect(fn).toContain('realpathSync'); }); - it('write-commands.ts validateOutputPath contains realpathSync', () => { - const fn = extractFunction(WRITE_SRC, 'validateOutputPath'); - expect(fn).toBeTruthy(); - expect(fn).toContain('realpathSync'); - }); - - it('meta-commands.ts SAFE_DIRECTORIES resolves with realpathSync', () => { - const safeBlock = sliceBetween(META_SRC, 'const SAFE_DIRECTORIES', ';'); + it('path-security.ts SAFE_DIRECTORIES resolves with realpathSync', () => { + const safeBlock = sliceBetween(PATH_SECURITY_SRC, 'const SAFE_DIRECTORIES', ';'); expect(safeBlock).toContain('realpathSync'); }); - it('write-commands.ts SAFE_DIRECTORIES resolves with realpathSync', () => { - const safeBlock = sliceBetween(WRITE_SRC, 'const SAFE_DIRECTORIES', ';'); - expect(safeBlock).toContain('realpathSync'); + it('meta-commands.ts re-exports validateOutputPath from path-security', () => { + expect(META_SRC).toContain("from './path-security'"); + expect(META_SRC).toContain('validateOutputPath'); + }); + + it('write-commands.ts imports validateOutputPath from path-security', () => { + expect(WRITE_SRC).toContain("from './path-security'"); + expect(WRITE_SRC).toContain('validateOutputPath'); }); });