From 6faa93247211988b552e5c3f64045036d22b6a0b Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:00:52 +0000 Subject: [PATCH 01/16] fix(security): harden validateOutputPath with realpathSync to block symlink traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both meta-commands.ts and write-commands.ts validateOutputPath previously used path.resolve which resolves paths logically without following symlinks. A symlink at /tmp/safe pointing to /etc would pass the safe-directory check. Switched to fs.realpathSync (matching the existing validateReadPath pattern in read-commands.ts): - Resolve the full real path for existing files - Resolve the parent directory for new files (ENOENT case) - Also resolve SAFE_DIRECTORIES themselves through realpathSync (handles macOS /tmp → /private/tmp) Added browse/test/security-audit-r2.test.ts with source-level checks and behavioral tests including symlink-to-/etc attack verification. --- browse/src/meta-commands.ts | 27 ++++- browse/src/write-commands.ts | 27 ++++- browse/test/security-audit-r2.test.ts | 146 ++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 browse/test/security-audit-r2.test.ts diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index e2060c214..0bb9717f8 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -15,11 +15,32 @@ import { resolveConfig } from './config'; import type { Frame } from 'playwright'; // Security: Path validation to prevent path traversal attacks -const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()]; +// 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); - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(resolved, dir)); + // Always resolve to absolute first (fixes relative path symlink bypass) + const absolute = path.resolve(filePath); + // Resolve symlinks — for new files, resolve the parent directory + let realPath: string; + try { + realPath = fs.realpathSync(absolute); + } 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(absolute)); + realPath = path.join(dir, path.basename(absolute)); + } catch { + realPath = absolute; + } + } 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(', ')}`); } diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 19283fef0..1f15767ef 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -14,11 +14,32 @@ import { TEMP_DIR, isPathWithin } from './platform'; import { modifyStyle, undoModification, resetModifications, getModificationHistory } from './cdp-inspector'; // Security: Path validation for screenshot output -const SAFE_DIRECTORIES = [TEMP_DIR, process.cwd()]; +// 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); - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(resolved, dir)); + // Always resolve to absolute first (fixes relative path symlink bypass) + const absolute = path.resolve(filePath); + // Resolve symlinks — for new files, resolve the parent directory + let realPath: string; + try { + realPath = fs.realpathSync(absolute); + } 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(absolute)); + realPath = path.join(dir, path.basename(absolute)); + } catch { + realPath = absolute; + } + } 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(', ')}`); } diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts new file mode 100644 index 000000000..b1c56a109 --- /dev/null +++ b/browse/test/security-audit-r2.test.ts @@ -0,0 +1,146 @@ +/** + * Security audit round-2 tests — static source checks + behavioral verification. + * + * These tests verify that security fixes are present at the source level and + * behave correctly at runtime. Source-level checks guard against regressions + * that could silently remove a fix without breaking compilation. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; + +// ─── Shared source reads (used across multiple test sections) ─────────────── +const META_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/meta-commands.ts'), 'utf-8'); +const WRITE_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/write-commands.ts'), 'utf-8'); +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'); + +// ─── Helper ───────────────────────────────────────────────────────────────── + +/** + * Extract the source text between two string markers. + */ +function sliceBetween(src: string, startMarker: string, endMarker: string): string { + const start = src.indexOf(startMarker); + if (start === -1) return ''; + const end = src.indexOf(endMarker, start + startMarker.length); + if (end === -1) return src.slice(start); + return src.slice(start, end + endMarker.length); +} + +/** + * Extract a function body by name — finds `function name(` or `export function name(` + * and returns the full balanced-brace block. + */ +function extractFunction(src: string, name: string): string { + const pattern = new RegExp(`(?:export\\s+)?function\\s+${name}\\s*\\(`); + const match = pattern.exec(src); + if (!match) return ''; + let depth = 0; + let inBody = false; + const start = match.index; + for (let i = start; i < src.length; i++) { + if (src[i] === '{') { depth++; inBody = true; } + else if (src[i] === '}') { depth--; } + if (inBody && depth === 0) return src.slice(start, i + 1); + } + return src.slice(start); +} + +// ─── Task 1: Harden validateOutputPath to use realpathSync ────────────────── + +describe('Task 1: validateOutputPath uses realpathSync', () => { + describe('source-level checks', () => { + it('meta-commands.ts validateOutputPath contains realpathSync', () => { + const fn = extractFunction(META_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', ';'); + 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'); + }); + }); + + describe('behavioral checks', () => { + let tmpDir: string; + let symlinkPath: string; + + beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-sec-test-')); + symlinkPath = path.join(tmpDir, 'evil-link'); + try { + fs.symlinkSync('/etc', symlinkPath); + } catch { + symlinkPath = ''; + } + }); + + afterAll(() => { + try { + if (symlinkPath) fs.unlinkSync(symlinkPath); + fs.rmdirSync(tmpDir); + } catch { + // best-effort cleanup + } + }); + + it('meta-commands validateOutputPath rejects path through /etc symlink', async () => { + if (!symlinkPath) { + console.warn('Skipping: symlink creation failed'); + return; + } + const mod = await import('../src/meta-commands.ts'); + const attackPath = path.join(symlinkPath, 'passwd'); + expect(() => mod.validateOutputPath(attackPath)).toThrow(); + }); + + it('realpathSync on symlink-to-/etc resolves to /etc (out of safe dirs)', () => { + if (!symlinkPath) { + console.warn('Skipping: symlink creation failed'); + return; + } + const resolvedLink = fs.realpathSync(symlinkPath); + expect(resolvedLink).toBe('/etc'); + const TEMP_DIR_VAL = process.platform === 'win32' ? os.tmpdir() : '/tmp'; + const safeDirs = [TEMP_DIR_VAL, process.cwd()].map(d => { + try { return fs.realpathSync(d); } catch { return d; } + }); + const passwdReal = path.join(resolvedLink, 'passwd'); + const isSafe = safeDirs.some(d => passwdReal === d || passwdReal.startsWith(d + path.sep)); + expect(isSafe).toBe(false); + }); + + it('meta-commands validateOutputPath accepts legitimate tmpdir paths', async () => { + const mod = await import('../src/meta-commands.ts'); + const legitimatePath = path.join(os.tmpdir(), 'gstack-screenshot.png'); + expect(() => mod.validateOutputPath(legitimatePath)).not.toThrow(); + }); + + it('meta-commands validateOutputPath accepts paths in cwd', async () => { + const mod = await import('../src/meta-commands.ts'); + const cwdPath = path.join(process.cwd(), 'output.png'); + expect(() => mod.validateOutputPath(cwdPath)).not.toThrow(); + }); + + it('meta-commands validateOutputPath rejects paths outside safe dirs', async () => { + const mod = await import('../src/meta-commands.ts'); + expect(() => mod.validateOutputPath('/home/user/secret.png')).toThrow(/Path must be within/); + expect(() => mod.validateOutputPath('/var/log/access.log')).toThrow(/Path must be within/); + }); + }); +}); From ddfecb617feb8a93d66c4571a576f755308cb108 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:02:27 +0000 Subject: [PATCH 02/16] fix(security): add CSS value validator at all injection points Block data exfiltration via url(), expression(), @import, javascript:, and data: patterns in write-commands style handler, cdp-inspector modifyStyle, extension injectCSS (also validates id format), and extension toggleClass (validates className format). Adds source-level regression tests for all four injection points. --- browse/src/cdp-inspector.ts | 6 +++ browse/src/write-commands.ts | 6 +++ browse/test/security-audit-r2.test.ts | 57 +++++++++++++++++++++++++++ extension/inspector.js | 9 +++++ 4 files changed, 78 insertions(+) diff --git a/browse/src/cdp-inspector.ts b/browse/src/cdp-inspector.ts index f8ed51762..19e99a13b 100644 --- a/browse/src/cdp-inspector.ts +++ b/browse/src/cdp-inspector.ts @@ -472,6 +472,12 @@ export async function modifyStyle( throw new Error(`Invalid CSS property name: ${property}. Only letters and hyphens allowed.`); } + // Validate CSS value — block data exfiltration patterns + const DANGEROUS_CSS = /url\s*\(|expression\s*\(|@import|javascript:|data:/i; + if (DANGEROUS_CSS.test(value)) { + throw new Error('CSS value rejected: contains potentially dangerous pattern.'); + } + let oldValue = ''; let source = 'inline'; let sourceLine = 0; diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 1f15767ef..97265aa5a 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -520,6 +520,12 @@ export async function handleWriteCommand( throw new Error(`Invalid CSS property name: ${property}. Only letters and hyphens allowed.`); } + // Validate CSS value — block data exfiltration patterns + const DANGEROUS_CSS = /url\s*\(|expression\s*\(|@import|javascript:|data:/i; + if (DANGEROUS_CSS.test(value)) { + throw new Error('CSS value rejected: contains potentially dangerous pattern.'); + } + const mod = await modifyStyle(page, selector, property, value); return `Style modified: ${selector} { ${property}: ${mod.oldValue || '(none)'} → ${value} } (${mod.method})`; } diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index b1c56a109..6b7f77541 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -49,6 +49,63 @@ function extractFunction(src: string, name: string): string { return src.slice(start); } +// ─── Shared source reads for CSS validator tests ──────────────────────────── +const CDP_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/cdp-inspector.ts'), 'utf-8'); +const EXTENSION_SRC = fs.readFileSync( + path.join(import.meta.dir, '../../extension/inspector.js'), + 'utf-8' +); + +// ─── Task 2: Shared CSS value validator ───────────────────────────────────── + +describe('Task 2: CSS value validator blocks dangerous patterns', () => { + describe('source-level checks', () => { + it('write-commands.ts style handler contains DANGEROUS_CSS url check', () => { + const styleBlock = sliceBetween(WRITE_SRC, "case 'style':", 'case \'cleanup\''); + expect(styleBlock).toMatch(/url\\s\*\\\(/); + }); + + it('write-commands.ts style handler blocks expression()', () => { + const styleBlock = sliceBetween(WRITE_SRC, "case 'style':", "case 'cleanup'"); + expect(styleBlock).toMatch(/expression\\s\*\\\(/); + }); + + it('write-commands.ts style handler blocks @import', () => { + const styleBlock = sliceBetween(WRITE_SRC, "case 'style':", "case 'cleanup'"); + expect(styleBlock).toContain('@import'); + }); + + it('cdp-inspector.ts modifyStyle contains DANGEROUS_CSS url check', () => { + const fn = extractFunction(CDP_SRC, 'modifyStyle'); + expect(fn).toBeTruthy(); + expect(fn).toMatch(/url\\s\*\\\(/); + }); + + it('cdp-inspector.ts modifyStyle blocks @import', () => { + const fn = extractFunction(CDP_SRC, 'modifyStyle'); + expect(fn).toContain('@import'); + }); + + it('extension injectCSS validates id format', () => { + const fn = extractFunction(EXTENSION_SRC, 'injectCSS'); + expect(fn).toBeTruthy(); + // Should contain a regex test for valid id characters + expect(fn).toMatch(/\^?\[a-zA-Z0-9_-\]/); + }); + + it('extension injectCSS blocks dangerous CSS patterns', () => { + const fn = extractFunction(EXTENSION_SRC, 'injectCSS'); + expect(fn).toMatch(/url\\s\*\\\(/); + }); + + it('extension toggleClass validates className format', () => { + const fn = extractFunction(EXTENSION_SRC, 'toggleClass'); + expect(fn).toBeTruthy(); + expect(fn).toMatch(/\^?\[a-zA-Z0-9_-\]/); + }); + }); +}); + // ─── Task 1: Harden validateOutputPath to use realpathSync ────────────────── describe('Task 1: validateOutputPath uses realpathSync', () => { diff --git a/extension/inspector.js b/extension/inspector.js index 01af66d91..db4cc697f 100644 --- a/extension/inspector.js +++ b/extension/inspector.js @@ -373,6 +373,9 @@ } function toggleClass(selector, className, action) { + if (!/^[a-zA-Z0-9_-]+$/.test(className)) { + return { error: 'Invalid class name' }; + } const el = findElement(selector); if (!el) return { error: 'Element not found' }; @@ -387,6 +390,12 @@ } function injectCSS(id, css) { + if (!/^[a-zA-Z0-9_-]+$/.test(id)) { + return { error: 'Invalid CSS injection id' }; + } + if (/url\s*\(|expression\s*\(|@import|javascript:|data:/i.test(css)) { + return { error: 'CSS contains blocked pattern (url, expression, @import)' }; + } const styleId = `gstack-inject-${id}`; let styleEl = document.getElementById(styleId); if (!styleEl) { From 5c23b88682dfa035deb3a646a029f3335305f5b7 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:03:45 +0000 Subject: [PATCH 03/16] fix(security): prevent shell injection in gstack-learnings-search Replace direct bash variable interpolation into JS string literals with environment variable passing. Values are now passed via GSTACK_FILTER_* env vars and read with process.env inside the bun -e block, eliminating the shell injection vector where single quotes in --query/--type/--slug could break out of JS string literals and execute arbitrary code. Adds browse/test/learnings-injection.test.ts with source-level and behavioral tests to prevent regression. --- bin/gstack-learnings-search | 12 ++++----- browse/test/learnings-injection.test.ts | 33 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 browse/test/learnings-injection.test.ts diff --git a/bin/gstack-learnings-search b/bin/gstack-learnings-search index 4ac187ec1..ead423748 100755 --- a/bin/gstack-learnings-search +++ b/bin/gstack-learnings-search @@ -43,13 +43,13 @@ if [ ${#FILES[@]} -eq 0 ]; then fi # Process all files through bun for JSON parsing, decay, dedup, filtering -cat "${FILES[@]}" 2>/dev/null | bun -e " +cat "${FILES[@]}" 2>/dev/null | GSTACK_FILTER_TYPE="$TYPE" GSTACK_FILTER_QUERY="$QUERY" GSTACK_FILTER_LIMIT="$LIMIT" GSTACK_FILTER_SLUG="$SLUG" GSTACK_FILTER_CROSS="$CROSS_PROJECT" bun -e " const lines = (await Bun.stdin.text()).trim().split('\n').filter(Boolean); const now = Date.now(); -const type = '${TYPE}'; -const query = '${QUERY}'.toLowerCase(); -const limit = ${LIMIT}; -const slug = '${SLUG}'; +const type = process.env.GSTACK_FILTER_TYPE || ''; +const query = (process.env.GSTACK_FILTER_QUERY || '').toLowerCase(); +const limit = parseInt(process.env.GSTACK_FILTER_LIMIT || '10', 10) || 10; +const slug = process.env.GSTACK_FILTER_SLUG || ''; const entries = []; for (const line of lines) { @@ -67,7 +67,7 @@ for (const line of lines) { // Determine if this is from the current project or cross-project // Cross-project entries are tagged for display - e._crossProject = !line.includes(slug) && '${CROSS_PROJECT}' === 'true'; + e._crossProject = !line.includes(slug) && (process.env.GSTACK_FILTER_CROSS || '') === 'true'; entries.push(e); } catch {} diff --git a/browse/test/learnings-injection.test.ts b/browse/test/learnings-injection.test.ts new file mode 100644 index 000000000..17dd33713 --- /dev/null +++ b/browse/test/learnings-injection.test.ts @@ -0,0 +1,33 @@ +import { describe, it, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; +import { spawnSync } from 'child_process'; + +const SCRIPT_PATH = path.join(import.meta.dir, '../../bin/gstack-learnings-search'); +const SCRIPT = fs.readFileSync(SCRIPT_PATH, 'utf-8'); +const BIN_DIR = path.join(import.meta.dir, '../../bin'); + +describe('gstack-learnings-search injection safety', () => { + it('must not interpolate variables into JS string literals', () => { + const jsBlock = SCRIPT.slice(SCRIPT.indexOf('bun -e')); + expect(jsBlock).not.toMatch(/const \w+ = '\$\{/); + expect(jsBlock).not.toMatch(/= \$\{[A-Z_]+\};/); + expect(jsBlock).not.toMatch(/'\$\{CROSS_PROJECT\}'/); + }); + + it('must use process.env for parameters', () => { + const jsBlock = SCRIPT.slice(SCRIPT.indexOf('bun -e')); + expect(jsBlock).toContain('process.env'); + }); +}); + +describe('gstack-learnings-search injection behavioral', () => { + it('handles single quotes in query safely', () => { + const result = spawnSync('bash', [ + path.join(BIN_DIR, 'gstack-learnings-search'), + '--query', "test'; process.exit(99); //", + '--limit', '1' + ], { encoding: 'utf-8', timeout: 5000, env: { ...process.env, HOME: '/tmp/nonexistent-gstack-test' } }); + expect(result.status).not.toBe(99); + }); +}); From 2557663a29437682b259e016b44ad99f95af2d66 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:10:14 +0000 Subject: [PATCH 04/16] fix(security): validate agent queue entries and restrict file permissions Add isValidQueueEntry() validator to sidebar-agent.ts that checks all 8 QueueEntry fields (prompt required, args array, cwd path traversal, typed optional fields). Invalid entries are skipped with console.warn. Queue directory and file permissions hardened to 0o700/0o600 in all three writers: server.ts, sidebar-agent.ts, and cli.ts. Tests added to security-audit-r2.test.ts. --- browse/src/cli.ts | 5 ++- browse/src/server.ts | 3 +- browse/src/sidebar-agent.ts | 50 ++++++++++++++++++++++++--- browse/test/security-audit-r2.test.ts | 48 +++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 7 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 6e0d42f9b..04de451d7 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -587,7 +587,10 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: } // Clear old agent queue const agentQueue = path.join(process.env.HOME || '/tmp', '.gstack', 'sidebar-agent-queue.jsonl'); - try { fs.writeFileSync(agentQueue, ''); } catch {} + try { + fs.mkdirSync(path.dirname(agentQueue), { recursive: true, mode: 0o700 }); + fs.writeFileSync(agentQueue, '', { mode: 0o600 }); + } catch {} // Resolve browse binary path the same way — execPath-relative let browseBin = path.resolve(__dirname, '..', 'dist', 'browse'); diff --git a/browse/src/server.ts b/browse/src/server.ts index 55b744aa2..a73f5590c 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -558,8 +558,9 @@ function spawnClaude(userMessage: string, extensionUrl?: string | null, forTabId tabId: agentTabId, }); try { - fs.mkdirSync(gstackDir, { recursive: true }); + fs.mkdirSync(gstackDir, { recursive: true, mode: 0o700 }); fs.appendFileSync(agentQueue, entry + '\n'); + try { fs.chmodSync(agentQueue, 0o600); } catch {} } catch (err: any) { addChatEntry({ ts: new Date().toISOString(), role: 'agent', type: 'agent_error', error: `Failed to queue: ${err.message}` }); agentStatus = 'idle'; diff --git a/browse/src/sidebar-agent.ts b/browse/src/sidebar-agent.ts index 61bbaa451..4719820a4 100644 --- a/browse/src/sidebar-agent.ts +++ b/browse/src/sidebar-agent.ts @@ -223,7 +223,7 @@ async function handleStreamEvent(event: any, tabId?: number): Promise { } } -async function askClaude(queueEntry: any): Promise { +async function askClaude(queueEntry: QueueEntry): Promise { const { prompt, args, stateFile, cwd, tabId } = queueEntry; const tid = tabId ?? 0; @@ -327,6 +327,41 @@ async function askClaude(queueEntry: any): Promise { }); } +// ─── Queue entry validation ───────────────────────────────────── + +interface QueueEntry { + prompt: string; + args?: string[]; + stateFile?: string; + cwd?: string; + tabId?: number | null; + message?: string | null; + pageUrl?: string | null; + sessionId?: string | null; + ts?: string; +} + +function isValidQueueEntry(e: unknown): e is QueueEntry { + if (typeof e !== 'object' || e === null) return false; + const obj = e as Record; + // Required + if (typeof obj.prompt !== 'string' || obj.prompt.length === 0) return false; + // Optional typed fields + if (obj.args !== undefined && (!Array.isArray(obj.args) || !obj.args.every(a => typeof a === 'string'))) return false; + if (obj.stateFile !== undefined && typeof obj.stateFile !== 'string') return false; + if (obj.cwd !== undefined) { + if (typeof obj.cwd !== 'string') return false; + if (obj.cwd.includes('..')) return false; + } + // tabId: optional number or null (writer emits null when no tab) + if (obj.tabId !== undefined && obj.tabId !== null && typeof obj.tabId !== 'number') return false; + if (obj.message !== undefined && obj.message !== null && typeof obj.message !== 'string') return false; + if (obj.pageUrl !== undefined && obj.pageUrl !== null && typeof obj.pageUrl !== 'string') return false; + // sessionId: optional string or null (writer emits sessionId: ... || null) + if (obj.sessionId !== undefined && obj.sessionId !== null && typeof obj.sessionId !== 'string') return false; + return true; +} + // ─── Poll loop ─────────────────────────────────────────────────── function countLines(): number { @@ -357,12 +392,16 @@ async function poll() { const line = readLine(lastLine); if (!line) continue; - let entry: any; - try { entry = JSON.parse(line); } catch (err: any) { + let parsed: unknown; + try { parsed = JSON.parse(line); } catch (err: any) { console.warn(`[sidebar-agent] Skipping malformed queue entry at line ${lastLine}:`, line.slice(0, 80), err.message); continue; } - if (!entry.message && !entry.prompt) continue; + if (!isValidQueueEntry(parsed)) { + console.warn(`[sidebar-agent] Skipping invalid queue entry at line ${lastLine}: failed schema validation`); + continue; + } + const entry = parsed; const tid = entry.tabId ?? 0; // Skip if this tab already has an agent running — server queues per-tab @@ -383,8 +422,9 @@ async function poll() { async function main() { const dir = path.dirname(QUEUE); - fs.mkdirSync(dir, { recursive: true }); + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); if (!fs.existsSync(QUEUE)) fs.writeFileSync(QUEUE, ''); + try { fs.chmodSync(QUEUE, 0o600); } catch {} lastLine = countLines(); await refreshToken(); diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 6b7f77541..a3ec2b4bc 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -49,6 +49,54 @@ function extractFunction(src: string, name: string): string { return src.slice(start); } +// ─── Task 4: Agent queue poisoning — full schema validation + permissions ─── + +describe('Agent queue security', () => { + it('server queue directory must use restricted permissions', () => { + const queueSection = SERVER_SRC.slice(SERVER_SRC.indexOf('agentQueue'), SERVER_SRC.indexOf('agentQueue') + 2000); + expect(queueSection).toMatch(/0o700/); + }); + + it('sidebar-agent queue directory must use restricted permissions', () => { + // The mkdirSync for the queue dir lives in main() — search the main() body + const mainStart = AGENT_SRC.indexOf('async function main'); + const queueSection = AGENT_SRC.slice(mainStart); + expect(queueSection).toMatch(/0o700/); + }); + + it('cli.ts queue file creation must use restricted permissions', () => { + const CLI_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/cli.ts'), 'utf-8'); + const queueSection = CLI_SRC.slice(CLI_SRC.indexOf('queue') || 0, CLI_SRC.indexOf('queue') + 2000); + expect(queueSection).toMatch(/0o700|0o600|mode/); + }); + + it('queue reader must have a validator function covering all fields', () => { + // Extract ONLY the validator function body by walking braces + const validatorStart = AGENT_SRC.indexOf('function isValidQueueEntry'); + expect(validatorStart).toBeGreaterThan(-1); + let depth = 0; + let bodyStart = AGENT_SRC.indexOf('{', validatorStart); + let bodyEnd = bodyStart; + for (let i = bodyStart; i < AGENT_SRC.length; i++) { + if (AGENT_SRC[i] === '{') depth++; + if (AGENT_SRC[i] === '}') depth--; + if (depth === 0) { bodyEnd = i + 1; break; } + } + const validatorBlock = AGENT_SRC.slice(validatorStart, bodyEnd); + + expect(validatorBlock).toMatch(/prompt.*string/); + expect(validatorBlock).toMatch(/Array\.isArray/); + expect(validatorBlock).toMatch(/\.\./); + expect(validatorBlock).toContain('stateFile'); + expect(validatorBlock).toContain('tabId'); + expect(validatorBlock).toMatch(/number/); + expect(validatorBlock).toContain('null'); + expect(validatorBlock).toContain('message'); + expect(validatorBlock).toContain('pageUrl'); + expect(validatorBlock).toContain('sessionId'); + }); +}); + // ─── Shared source reads for CSS validator tests ──────────────────────────── const CDP_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/cdp-inspector.ts'), 'utf-8'); const EXTENSION_SRC = fs.readFileSync( From cc1cd446721acef8be33889c051c0af0b50fe66d Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:19:19 +0000 Subject: [PATCH 05/16] fix(security): address round 3 code review findings - applyStyle in extension/inspector.js now validates CSS values with the same DANGEROUS_CSS pattern as injectCSS (blocks url(), expression(), @import, javascript:, data: exfiltration vectors) - snapshot.ts annotated screenshot path validation now uses realpathSync to resolve symlinks before checking against safe directories - cookie-import in write-commands.ts replaces inline path.resolve+isPathWithin with realpathSync-based validation consistent with validateOutputPath - isValidQueueEntry in sidebar-agent.ts now checks stateFile for '..' path traversal sequences in addition to type validation - Adds source-level regression tests for all four fixes in security-audit-r2.test.ts --- browse/src/sidebar-agent.ts | 5 +- browse/src/snapshot.ts | 31 +++++++-- browse/src/write-commands.ts | 29 +++++--- browse/test/security-audit-r2.test.ts | 97 +++++++++++++++++++++++++++ extension/inspector.js | 4 ++ 5 files changed, 151 insertions(+), 15 deletions(-) diff --git a/browse/src/sidebar-agent.ts b/browse/src/sidebar-agent.ts index 4719820a4..c7bd525db 100644 --- a/browse/src/sidebar-agent.ts +++ b/browse/src/sidebar-agent.ts @@ -348,7 +348,10 @@ function isValidQueueEntry(e: unknown): e is QueueEntry { if (typeof obj.prompt !== 'string' || obj.prompt.length === 0) return false; // Optional typed fields if (obj.args !== undefined && (!Array.isArray(obj.args) || !obj.args.every(a => typeof a === 'string'))) return false; - if (obj.stateFile !== undefined && typeof obj.stateFile !== 'string') return false; + if (obj.stateFile !== undefined) { + if (typeof obj.stateFile !== 'string') return false; + if (obj.stateFile.includes('..')) return false; + } if (obj.cwd !== undefined) { if (typeof obj.cwd !== 'string') return false; if (obj.cwd.includes('..')) return false; diff --git a/browse/src/snapshot.ts b/browse/src/snapshot.ts index 840cd6868..5e7e8c02e 100644 --- a/browse/src/snapshot.ts +++ b/browse/src/snapshot.ts @@ -313,11 +313,32 @@ export async function handleSnapshot( // ─── Annotated screenshot (-a) ──────────────────────────── if (opts.annotate) { const screenshotPath = opts.outputPath || `${TEMP_DIR}/browse-annotated.png`; - // Validate output path (consistent with screenshot/pdf/responsive) - const resolvedPath = require('path').resolve(screenshotPath); - const safeDirs = [TEMP_DIR, process.cwd()]; - if (!safeDirs.some((dir: string) => isPathWithin(resolvedPath, dir))) { - throw new Error(`Path must be within: ${safeDirs.join(', ')}`); + // Validate output path — resolve symlinks to prevent symlink traversal attacks + { + const nodePath = require('path') as typeof import('path'); + const nodeFs = require('fs') as typeof import('fs'); + const absolute = nodePath.resolve(screenshotPath); + const safeDirs = [TEMP_DIR, process.cwd()].map((d: string) => { + try { return nodeFs.realpathSync(d); } catch { return d; } + }); + let realPath: string; + try { + realPath = nodeFs.realpathSync(absolute); + } catch (err: any) { + if (err.code === 'ENOENT') { + try { + const dir = nodeFs.realpathSync(nodePath.dirname(absolute)); + realPath = nodePath.join(dir, nodePath.basename(absolute)); + } catch { + realPath = absolute; + } + } else { + throw new Error(`Cannot resolve real path: ${screenshotPath} (${err.code})`); + } + } + if (!safeDirs.some((dir: string) => isPathWithin(realPath, dir))) { + throw new Error(`Path must be within: ${safeDirs.join(', ')}`); + } } try { // Inject overlay divs at each ref's bounding box diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 97265aa5a..1a14a2160 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -428,16 +428,27 @@ export async function handleWriteCommand( case 'cookie-import': { const filePath = args[0]; if (!filePath) throw new Error('Usage: browse cookie-import '); - // Path validation — prevent reading arbitrary files - if (path.isAbsolute(filePath)) { - const safeDirs = [TEMP_DIR, process.cwd()]; - const resolved = path.resolve(filePath); - if (!safeDirs.some(dir => isPathWithin(resolved, dir))) { - throw new Error(`Path must be within: ${safeDirs.join(', ')}`); + // Path validation — resolve symlinks to prevent symlink traversal attacks (read path) + { + const absolute = path.resolve(filePath); + let realPath: string; + try { + realPath = fs.realpathSync(absolute); + } catch (err: any) { + if (err.code === 'ENOENT') { + try { + const dir = fs.realpathSync(path.dirname(absolute)); + realPath = path.join(dir, path.basename(absolute)); + } catch { + realPath = absolute; + } + } else { + throw new Error(`Cannot resolve real path: ${filePath} (${err.code})`); + } + } + if (!SAFE_DIRECTORIES.some(dir => isPathWithin(realPath, dir))) { + throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); } - } - if (path.normalize(filePath).includes('..')) { - throw new Error('Path traversal sequences (..) are not allowed'); } if (!fs.existsSync(filePath)) throw new Error(`File not found: ${filePath}`); const raw = fs.readFileSync(filePath, 'utf-8'); diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index a3ec2b4bc..d4fca72da 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -16,6 +16,7 @@ const META_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/meta-command const WRITE_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/write-commands.ts'), 'utf-8'); 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'); // ─── Helper ───────────────────────────────────────────────────────────────── @@ -249,3 +250,99 @@ describe('Task 1: validateOutputPath uses realpathSync', () => { }); }); }); + +// ─── Round-2 review findings: applyStyle CSS check ────────────────────────── + +describe('Round-2 finding 1: extension applyStyle blocks dangerous CSS values', () => { + const INSPECTOR_SRC = fs.readFileSync( + path.join(import.meta.dir, '../../extension/inspector.js'), + 'utf-8' + ); + + it('applyStyle function exists in inspector.js', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + expect(fn).toBeTruthy(); + }); + + it('applyStyle validates CSS value with url() block', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + // Source contains literal regex /url\s*\(/ — match the source-level escape sequence + expect(fn).toMatch(/url\\s\*\\\(/); + }); + + it('applyStyle blocks expression()', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + expect(fn).toMatch(/expression\\s\*\\\(/); + }); + + it('applyStyle blocks @import', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + expect(fn).toContain('@import'); + }); + + it('applyStyle blocks javascript: scheme', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + expect(fn).toContain('javascript:'); + }); + + it('applyStyle blocks data: scheme', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + expect(fn).toContain('data:'); + }); + + it('applyStyle value check appears before setProperty call', () => { + const fn = extractFunction(INSPECTOR_SRC, 'applyStyle'); + // Check that the CSS value guard (url\s*\() appears before setProperty + const valueCheckIdx = fn.search(/url\\s\*\\\(/); + const setPropIdx = fn.indexOf('setProperty'); + expect(valueCheckIdx).toBeGreaterThan(-1); + expect(setPropIdx).toBeGreaterThan(-1); + expect(valueCheckIdx).toBeLessThan(setPropIdx); + }); +}); + +// ─── Round-2 finding 2: snapshot.ts annotated path uses realpathSync ──────── + +describe('Round-2 finding 2: snapshot.ts annotated path uses realpathSync', () => { + it('snapshot.ts annotated screenshot section contains realpathSync', () => { + // Slice the annotated screenshot block from the source + const annotateStart = SNAPSHOT_SRC.indexOf('opts.annotate'); + expect(annotateStart).toBeGreaterThan(-1); + const annotateBlock = SNAPSHOT_SRC.slice(annotateStart, annotateStart + 2000); + expect(annotateBlock).toContain('realpathSync'); + }); + + it('snapshot.ts annotated path validation resolves safe dirs with realpathSync', () => { + const annotateStart = SNAPSHOT_SRC.indexOf('opts.annotate'); + const annotateBlock = SNAPSHOT_SRC.slice(annotateStart, annotateStart + 2000); + // safeDirs array must be built with .map() that calls realpathSync + // Pattern: [TEMP_DIR, process.cwd()].map(...realpathSync...) + expect(annotateBlock).toContain('[TEMP_DIR, process.cwd()].map'); + expect(annotateBlock).toContain('realpathSync'); + }); +}); + +// ─── Round-2 finding 3: stateFile path traversal check in isValidQueueEntry ─ + +describe('Round-2 finding 3: isValidQueueEntry checks stateFile for path traversal', () => { + it('isValidQueueEntry checks stateFile for .. traversal sequences', () => { + const fn = extractFunction(AGENT_SRC, 'isValidQueueEntry'); + expect(fn).toBeTruthy(); + // Must check stateFile for '..' — find the stateFile block and look for '..' string + const stateFileIdx = fn.indexOf('stateFile'); + expect(stateFileIdx).toBeGreaterThan(-1); + const stateFileBlock = fn.slice(stateFileIdx, stateFileIdx + 200); + // The block must contain a check for the two-dot traversal sequence + expect(stateFileBlock).toMatch(/'\.\.'|"\.\."|\.\./); + }); + + it('isValidQueueEntry stateFile block contains both type check and traversal check', () => { + const fn = extractFunction(AGENT_SRC, 'isValidQueueEntry'); + const stateFileIdx = fn.indexOf('stateFile'); + const stateBlock = fn.slice(stateFileIdx, stateFileIdx + 300); + // Must contain the type check + expect(stateBlock).toContain('typeof obj.stateFile'); + // Must contain the includes('..') call + expect(stateBlock).toMatch(/includes\s*\(\s*['"]\.\.['"]\s*\)/); + }); +}); diff --git a/extension/inspector.js b/extension/inspector.js index db4cc697f..df88b5a7d 100644 --- a/extension/inspector.js +++ b/extension/inspector.js @@ -355,6 +355,10 @@ function applyStyle(selector, property, value) { // Validate property name: alphanumeric + hyphens only if (!/^[a-zA-Z-]+$/.test(property)) return { error: 'Invalid property name' }; + // Validate CSS value: block exfiltration vectors (url(), expression(), @import, javascript:, data:) + if (/url\s*\(|expression\s*\(|@import|javascript:|data:/i.test(value)) { + return { error: 'CSS value contains blocked pattern' }; + } const el = findElement(selector); if (!el) return { error: 'Element not found' }; From dcdae09fe90786a432df851e78ebf86e3a1dadad Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:21:53 +0000 Subject: [PATCH 06/16] fix(security): remove currentUrl and currentMessage from unauthenticated /health --- browse/src/server.ts | 6 +- browse/test/security-audit-r2.test.ts | 89 +++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/browse/src/server.ts b/browse/src/server.ts index a73f5590c..eb1bf2779 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -282,6 +282,10 @@ function loadSession(): SidebarSession | null { try { const activeFile = path.join(SESSIONS_DIR, 'active.json'); const activeData = JSON.parse(fs.readFileSync(activeFile, 'utf-8')); + if (typeof activeData.id !== 'string' || !/^[a-zA-Z0-9_-]+$/.test(activeData.id)) { + console.warn('[browse] Invalid session ID in active.json — ignoring'); + return null; + } const sessionFile = path.join(SESSIONS_DIR, activeData.id, 'session.json'); const session = JSON.parse(fs.readFileSync(sessionFile, 'utf-8')) as SidebarSession; // Validate worktree still exists — crash may have left stale path @@ -1086,7 +1090,6 @@ async function start() { mode: browserManager.getConnectionMode(), uptime: Math.floor((Date.now() - startTime) / 1000), tabs: browserManager.getTabCount(), - currentUrl: browserManager.getCurrentUrl(), // Auth token for extension bootstrap. Safe: /health is localhost-only. // Previously served via .auth.json in extension dir, but that breaks // read-only .app bundles and codesigning. Extension reads token from here. @@ -1095,7 +1098,6 @@ async function start() { agent: { status: agentStatus, runningFor: agentStartTime ? Date.now() - agentStartTime : null, - currentMessage, queueLength: messageQueue.length, }, session: sidebarSession ? { id: sidebarSession.id, name: sidebarSession.name } : null, diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index d4fca72da..b402955b1 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -346,3 +346,92 @@ describe('Round-2 finding 3: isValidQueueEntry checks stateFile for path travers expect(stateBlock).toMatch(/includes\s*\(\s*['"]\.\.['"]\s*\)/); }); }); + +// ─── Task 5: /health endpoint must not expose sensitive fields ─────────────── + +describe('/health endpoint security', () => { + it('must not expose currentMessage', () => { + const block = sliceBetween(SERVER_SRC, "url.pathname === '/health'", "url.pathname === '/refs'"); + expect(block).not.toContain('currentMessage'); + }); + it('must not expose currentUrl', () => { + const block = sliceBetween(SERVER_SRC, "url.pathname === '/health'", "url.pathname === '/refs'"); + expect(block).not.toContain('currentUrl'); + }); +}); + +// ─── Task 6: frame --url ReDoS fix ────────────────────────────────────────── + +describe('frame --url ReDoS fix', () => { + it('frame --url section does not pass raw user input to new RegExp()', () => { + const block = sliceBetween(META_SRC, "target === '--url'", 'else {'); + expect(block).not.toMatch(/new RegExp\(args\[/); + }); + + it('frame --url section uses escapeRegExp before constructing RegExp', () => { + const block = sliceBetween(META_SRC, "target === '--url'", 'else {'); + expect(block).toContain('escapeRegExp'); + }); + + it('escapeRegExp neutralizes catastrophic patterns (behavioral)', async () => { + const mod = await import('../src/meta-commands.ts'); + const { escapeRegExp } = mod as any; + expect(typeof escapeRegExp).toBe('function'); + const evil = '(a+)+$'; + const escaped = escapeRegExp(evil); + const start = Date.now(); + new RegExp(escaped).test('aaaaaaaaaaaaaaaaaaaaaaaaaaa!'); + expect(Date.now() - start).toBeLessThan(100); + }); +}); + +// ─── Task 7: watch-mode guard in chain command ─────────────────────────────── + +describe('chain command watch-mode guard', () => { + it('chain loop contains isWatching() guard before write dispatch', () => { + const block = sliceBetween(META_SRC, 'for (const cmd of commands)', 'Wait for network to settle'); + expect(block).toContain('isWatching'); + }); + + it('chain loop BLOCKED message appears for write commands in watch mode', () => { + const block = sliceBetween(META_SRC, 'for (const cmd of commands)', 'Wait for network to settle'); + expect(block).toContain('BLOCKED: write commands disabled in watch mode'); + }); +}); + +// ─── Task 8: Cookie domain validation ─────────────────────────────────────── + +describe('cookie-import domain validation', () => { + it('cookie-import handler validates cookie domain against page domain', () => { + const block = sliceBetween(WRITE_SRC, "case 'cookie-import':", "case 'cookie-import-browser':"); + expect(block).toContain('cookieDomain'); + expect(block).toContain('defaultDomain'); + expect(block).toContain('does not match current page domain'); + }); + + it('cookie-import-browser handler validates --domain against page hostname', () => { + const block = sliceBetween(WRITE_SRC, "case 'cookie-import-browser':", "case 'style':"); + expect(block).toContain('normalizedDomain'); + expect(block).toContain('pageHostname'); + expect(block).toContain('does not match current page domain'); + }); +}); + +// ─── Task 9: loadSession ID validation ────────────────────────────────────── + +describe('loadSession session ID validation', () => { + it('loadSession validates session ID format before using it in a path', () => { + const fn = extractFunction(SERVER_SRC, 'loadSession'); + expect(fn).toBeTruthy(); + // Must contain the alphanumeric regex guard + expect(fn).toMatch(/\[a-zA-Z0-9_-\]/); + }); + + it('loadSession returns null on invalid session ID', () => { + const fn = extractFunction(SERVER_SRC, 'loadSession'); + const block = fn.slice(fn.indexOf('activeData.id')); + // Must warn and return null + expect(block).toContain('Invalid session ID'); + expect(block).toContain('return null'); + }); +}); From 4e8c64441581722f31b0da8b9e4d17bdcfc4dfd1 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:22:07 +0000 Subject: [PATCH 07/16] fix(security): escape user input in frame --url to prevent ReDoS --- browse/src/meta-commands.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 0bb9717f8..f8b3d3782 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -46,6 +46,11 @@ export function validateOutputPath(filePath: string): void { } } +/** 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[] = []; @@ -259,6 +264,10 @@ export async function handleMetaCommand( try { let result: string; if (WRITE_COMMANDS.has(name)) { + if (bm.isWatching()) { + results.push(`[${name}] BLOCKED: write commands disabled in watch mode`); + continue; + } result = await handleWriteCommand(name, cmdArgs, bm); lastWasWrite = true; } else if (READ_COMMANDS.has(name)) { @@ -556,7 +565,7 @@ export async function handleMetaCommand( frame = page.frame({ name: args[1] }); } else if (target === '--url') { if (!args[1]) throw new Error('Usage: frame --url '); - frame = page.frame({ url: new RegExp(args[1]) }); + frame = page.frame({ url: new RegExp(escapeRegExp(args[1])) }); } else { // CSS selector or @ref for the iframe element const resolved = await bm.resolveRef(target); From ed7f1691cf891e33dfab4fe6dd9dcc53b5ec6e62 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:22:24 +0000 Subject: [PATCH 08/16] fix(security): validate cookie domains in cookie-import and cookie-import-browser --- browse/src/write-commands.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 1a14a2160..603c41fed 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -462,7 +462,14 @@ export async function handleWriteCommand( for (const c of cookies) { if (!c.name || c.value === undefined) throw new Error('Each cookie must have "name" and "value" fields'); - if (!c.domain) c.domain = defaultDomain; + if (!c.domain) { + c.domain = defaultDomain; + } else { + const cookieDomain = c.domain.startsWith('.') ? c.domain.slice(1) : c.domain; + if (cookieDomain !== defaultDomain && !defaultDomain.endsWith('.' + cookieDomain)) { + throw new Error(`Cookie domain "${c.domain}" does not match current page domain "${defaultDomain}". Use the target site first.`); + } + } if (!c.path) c.path = '/'; } @@ -482,6 +489,12 @@ export async function handleWriteCommand( if (domainIdx !== -1 && domainIdx + 1 < args.length) { // Direct import mode — no UI const domain = args[domainIdx + 1]; + // Validate --domain against current page hostname to prevent cross-site cookie injection + const pageHostname = new URL(page.url()).hostname; + const normalizedDomain = domain.startsWith('.') ? domain.slice(1) : domain; + if (normalizedDomain !== pageHostname && !pageHostname.endsWith('.' + normalizedDomain)) { + throw new Error(`--domain "${domain}" does not match current page domain "${pageHostname}". Navigate to the target site first.`); + } const browser = browserArg || 'comet'; const result = await importCookies(browser, [domain], profile); if (result.cookies.length > 0) { From 5cf8cadb4dc84bb0bd876adf65a74f3d652a53fc Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:27:01 +0000 Subject: [PATCH 09/16] fix(security): validate each responsive screenshot path, not just prefix --- browse/src/meta-commands.ts | 7 ++++--- browse/test/security-audit-r2.test.ts | 30 +++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index f8b3d3782..6b00af7e3 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -221,9 +221,10 @@ export async function handleMetaCommand( for (const vp of viewports) { await page.setViewportSize({ width: vp.width, height: vp.height }); - const path = `${prefix}-${vp.name}.png`; - await page.screenshot({ path, fullPage: true }); - results.push(`${vp.name} (${vp.width}x${vp.height}): ${path}`); + const screenshotPath = `${prefix}-${vp.name}.png`; + validateOutputPath(screenshotPath); + await page.screenshot({ path: screenshotPath, fullPage: true }); + results.push(`${vp.name} (${vp.width}x${vp.height}): ${screenshotPath}`); } // Restore original viewport diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index b402955b1..43f86d66c 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -435,3 +435,33 @@ describe('loadSession session ID validation', () => { expect(block).toContain('return null'); }); }); + +// ─── Task 10: Responsive screenshot path validation ────────────────────────── + +describe('Task 10: responsive screenshot path validation', () => { + it('responsive loop contains validateOutputPath before page.screenshot()', () => { + // Extract the responsive case block + const block = sliceBetween(META_SRC, "case 'responsive':", 'Restore original viewport'); + expect(block).toBeTruthy(); + expect(block).toContain('validateOutputPath'); + }); + + it('responsive loop calls validateOutputPath on the per-viewport path, not just the prefix', () => { + const block = sliceBetween(META_SRC, 'for (const vp of viewports)', 'Restore original viewport'); + expect(block).toContain('validateOutputPath'); + }); + + it('validateOutputPath appears before page.screenshot() in the loop', () => { + const block = sliceBetween(META_SRC, 'for (const vp of viewports)', 'Restore original viewport'); + const validateIdx = block.indexOf('validateOutputPath'); + const screenshotIdx = block.indexOf('page.screenshot'); + expect(validateIdx).toBeGreaterThan(-1); + expect(screenshotIdx).toBeGreaterThan(-1); + expect(validateIdx).toBeLessThan(screenshotIdx); + }); + + it('results.push is present in the loop block (loop structure intact)', () => { + const block = sliceBetween(META_SRC, 'for (const vp of viewports)', 'Restore original viewport'); + expect(block).toContain('results.push'); + }); +}); From cec3d060b2008f7f2fbf95665ff3a9ca564afe19 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:27:50 +0000 Subject: [PATCH 10/16] fix(security): validate cookies and page URLs in state load --- browse/src/browser-manager.ts | 6 ++++ browse/src/meta-commands.ts | 14 +++++++- browse/test/security-audit-r2.test.ts | 50 +++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index ef476248e..9bdcf60b9 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -822,6 +822,12 @@ export class BrowserManager { this.wirePageEvents(page); if (saved.url) { + try { + await validateNavigationUrl(saved.url); + } catch (err: any) { + console.warn(`[browse] Skipping invalid URL in state file: ${saved.url} — ${err.message}`); + continue; + } await page.goto(saved.url, { waitUntil: 'domcontentloaded', timeout: 15000 }).catch(() => {}); } diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 6b00af7e3..686053d8d 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -526,6 +526,18 @@ export async function handleMetaCommand( if (!Array.isArray(data.cookies) || !Array.isArray(data.pages)) { throw new Error('Invalid state file: expected cookies and pages arrays'); } + // Validate and filter cookies — reject malformed or internal-network cookies + const validatedCookies = data.cookies.filter((c: any) => { + if (typeof c !== 'object' || !c) return false; + if (typeof c.name !== 'string' || typeof c.value !== 'string') return false; + if (typeof c.domain !== 'string' || !c.domain) return false; + const d = c.domain.startsWith('.') ? c.domain.slice(1) : c.domain; + if (d === 'localhost' || d.endsWith('.internal') || d === '169.254.169.254') return false; + return true; + }); + if (validatedCookies.length < data.cookies.length) { + console.warn(`[browse] Filtered ${data.cookies.length - validatedCookies.length} invalid cookies from state file`); + } // Warn on state files older than 7 days if (data.savedAt) { const ageMs = Date.now() - new Date(data.savedAt).getTime(); @@ -538,7 +550,7 @@ export async function handleMetaCommand( bm.setFrame(null); await bm.closeAllPages(); await bm.restoreState({ - cookies: data.cookies, + cookies: validatedCookies, pages: data.pages.map((p: any) => ({ ...p, storage: null })), }); return `State loaded: ${data.cookies.length} cookies, ${data.pages.length} pages`; diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 43f86d66c..0bde0426a 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -465,3 +465,53 @@ describe('Task 10: responsive screenshot path validation', () => { expect(block).toContain('results.push'); }); }); + +// ─── Task 11: State load — cookie + page URL validation ────────────────────── + +const BROWSER_MANAGER_SRC = fs.readFileSync(path.join(import.meta.dir, '../src/browser-manager.ts'), 'utf-8'); + +describe('Task 11: state load cookie validation', () => { + it('state load block filters cookies by domain and type', () => { + const block = sliceBetween(META_SRC, "action === 'load'", "throw new Error('Usage: state save|load"); + expect(block).toContain('cookie'); + expect(block).toContain('domain'); + expect(block).toContain('filter'); + }); + + it('state load block checks for localhost and .internal in cookie domains', () => { + const block = sliceBetween(META_SRC, "action === 'load'", "throw new Error('Usage: state save|load"); + expect(block).toContain('localhost'); + expect(block).toContain('.internal'); + }); + + it('state load block uses validatedCookies when calling restoreState', () => { + const block = sliceBetween(META_SRC, "action === 'load'", "throw new Error('Usage: state save|load"); + expect(block).toContain('validatedCookies'); + // Must pass validatedCookies to restoreState, not the raw data.cookies + const restoreIdx = block.indexOf('restoreState'); + const restoreBlock = block.slice(restoreIdx, restoreIdx + 200); + expect(restoreBlock).toContain('validatedCookies'); + }); + + it('browser-manager restoreState validates page URL before goto', () => { + // restoreState is a class method — use sliceBetween to extract the method body + const restoreFn = sliceBetween(BROWSER_MANAGER_SRC, 'async restoreState(', 'async recreateContext('); + expect(restoreFn).toBeTruthy(); + expect(restoreFn).toContain('validateNavigationUrl'); + }); + + it('browser-manager restoreState skips invalid URLs with a warning', () => { + const restoreFn = sliceBetween(BROWSER_MANAGER_SRC, 'async restoreState(', 'async recreateContext('); + expect(restoreFn).toContain('Skipping invalid URL'); + expect(restoreFn).toContain('continue'); + }); + + it('validateNavigationUrl call appears before page.goto in restoreState', () => { + const restoreFn = sliceBetween(BROWSER_MANAGER_SRC, 'async restoreState(', 'async recreateContext('); + const validateIdx = restoreFn.indexOf('validateNavigationUrl'); + const gotoIdx = restoreFn.indexOf('page.goto'); + expect(validateIdx).toBeGreaterThan(-1); + expect(gotoIdx).toBeGreaterThan(-1); + expect(validateIdx).toBeLessThan(gotoIdx); + }); +}); From 2733610476f9b17f89d83001b45c9da4ddb93349 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:28:23 +0000 Subject: [PATCH 11/16] fix(security): validate activeTabUrl before syncActiveTabByUrl --- browse/src/server.ts | 6 +++-- browse/test/security-audit-r2.test.ts | 35 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/browse/src/server.ts b/browse/src/server.ts index eb1bf2779..620866562 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1220,7 +1220,8 @@ async function start() { // Sync active tab from Chrome extension — detects manual tab switches const activeUrl = url.searchParams.get('activeUrl'); if (activeUrl) { - browserManager.syncActiveTabByUrl(activeUrl); + const sanitized = sanitizeExtensionUrl(activeUrl); + if (sanitized) browserManager.syncActiveTabByUrl(sanitized); } const tabs = await browserManager.getTabListWithTitles(); return new Response(JSON.stringify({ tabs }), { @@ -1293,7 +1294,8 @@ async function start() { // Sync active tab BEFORE reading the ID — the user may have switched // tabs manually and the server's activeTabId is stale. if (extensionUrl) { - browserManager.syncActiveTabByUrl(extensionUrl); + const sanitized = sanitizeExtensionUrl(extensionUrl); + if (sanitized) browserManager.syncActiveTabByUrl(sanitized); } const msgTabId = browserManager?.getActiveTabId?.() ?? 0; const ts = new Date().toISOString(); diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 0bde0426a..2e92838ab 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -515,3 +515,38 @@ describe('Task 11: state load cookie validation', () => { expect(validateIdx).toBeLessThan(gotoIdx); }); }); + +// ─── Task 12: Validate activeTabUrl before syncActiveTabByUrl ───────────────── + +describe('Task 12: activeTabUrl sanitized before syncActiveTabByUrl', () => { + it('sidebar-tabs route sanitizes activeUrl before syncActiveTabByUrl', () => { + const block = sliceBetween(SERVER_SRC, "url.pathname === '/sidebar-tabs'", "url.pathname === '/sidebar-tabs/switch'"); + expect(block).toContain('sanitizeExtensionUrl'); + expect(block).toContain('syncActiveTabByUrl'); + const sanitizeIdx = block.indexOf('sanitizeExtensionUrl'); + const syncIdx = block.indexOf('syncActiveTabByUrl'); + expect(sanitizeIdx).toBeLessThan(syncIdx); + }); + + it('sidebar-command route sanitizes extensionUrl before syncActiveTabByUrl', () => { + const block = sliceBetween(SERVER_SRC, "url.pathname === '/sidebar-command'", "url.pathname === '/sidebar-chat/clear'"); + expect(block).toContain('sanitizeExtensionUrl'); + expect(block).toContain('syncActiveTabByUrl'); + const sanitizeIdx = block.indexOf('sanitizeExtensionUrl'); + const syncIdx = block.indexOf('syncActiveTabByUrl'); + expect(sanitizeIdx).toBeLessThan(syncIdx); + }); + + it('direct unsanitized syncActiveTabByUrl calls are not present (all calls go through sanitize)', () => { + // Every syncActiveTabByUrl call should be preceded by sanitizeExtensionUrl in the nearby code + // We verify there are no direct browserManager.syncActiveTabByUrl(activeUrl) or + // browserManager.syncActiveTabByUrl(extensionUrl) patterns (without sanitize wrapper) + const block1 = sliceBetween(SERVER_SRC, "url.pathname === '/sidebar-tabs'", "url.pathname === '/sidebar-tabs/switch'"); + // Should NOT contain direct call with raw activeUrl + expect(block1).not.toMatch(/syncActiveTabByUrl\(activeUrl\)/); + + const block2 = sliceBetween(SERVER_SRC, "url.pathname === '/sidebar-command'", "url.pathname === '/sidebar-chat/clear'"); + // Should NOT contain direct call with raw extensionUrl + expect(block2).not.toMatch(/syncActiveTabByUrl\(extensionUrl\)/); + }); +}); From ff6b1b59f83a9ec05000fa21a448b448d862f5a1 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:28:45 +0000 Subject: [PATCH 12/16] fix(security): wrap inbox output as untrusted content --- browse/src/meta-commands.ts | 4 ++-- browse/test/security-audit-r2.test.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 686053d8d..2d1e14911 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -474,8 +474,8 @@ export async function handleMetaCommand( for (const msg of messages) { const ts = msg.timestamp ? `[${msg.timestamp}]` : '[unknown]'; - lines.push(`${ts} ${msg.url}`); - lines.push(` "${msg.userMessage}"`); + lines.push(`${ts} ${wrapUntrustedContent(msg.url, 'inbox:url')}`); + lines.push(` "${wrapUntrustedContent(msg.userMessage, 'inbox:message')}"`); lines.push(''); } diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 2e92838ab..86d5f9f23 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -550,3 +550,29 @@ describe('Task 12: activeTabUrl sanitized before syncActiveTabByUrl', () => { expect(block2).not.toMatch(/syncActiveTabByUrl\(extensionUrl\)/); }); }); + +// ─── Task 13: Inbox output wrapped as untrusted ────────────────────────────── + +describe('Task 13: inbox output wrapped as untrusted content', () => { + it('inbox handler wraps userMessage with wrapUntrustedContent', () => { + const block = sliceBetween(META_SRC, "case 'inbox':", "case 'state':"); + expect(block).toContain('wrapUntrustedContent'); + }); + + it('inbox handler applies wrapUntrustedContent to userMessage', () => { + const block = sliceBetween(META_SRC, "case 'inbox':", "case 'state':"); + // Should wrap userMessage + expect(block).toMatch(/wrapUntrustedContent.*userMessage|userMessage.*wrapUntrustedContent/); + }); + + it('inbox handler applies wrapUntrustedContent to url', () => { + const block = sliceBetween(META_SRC, "case 'inbox':", "case 'state':"); + // Should also wrap url + expect(block).toMatch(/wrapUntrustedContent.*msg\.url|msg\.url.*wrapUntrustedContent/); + }); + + it('wrapUntrustedContent calls appear in the message formatting loop', () => { + const block = sliceBetween(META_SRC, 'for (const msg of messages)', 'Handle --clear flag'); + expect(block).toContain('wrapUntrustedContent'); + }); +}); From 75161adf8da5fc45deec36390eeb8d0cee11da38 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:36:10 +0000 Subject: [PATCH 13/16] fix(security): replace DOM serialization round-trip with node cloning in tab switch --- browse/test/security-audit-r2.test.ts | 40 ++++++++++++++++++++++ extension/sidepanel.js | 49 ++++++++++++++++++++------- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 86d5f9f23..26b1001b0 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -576,3 +576,43 @@ describe('Task 13: inbox output wrapped as untrusted content', () => { expect(block).toContain('wrapUntrustedContent'); }); }); + +// ─── Task 14: DOM serialization round-trip replaced with DocumentFragment ───── + +const SIDEPANEL_SRC = fs.readFileSync(path.join(import.meta.dir, '../../extension/sidepanel.js'), 'utf-8'); + +describe('Task 14: switchChatTab uses DocumentFragment, not innerHTML round-trip', () => { + it('switchChatTab does NOT use innerHTML to restore chat (string-based re-parse removed)', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'switchChatTab'); + expect(fn).toBeTruthy(); + // Must NOT have the dangerous pattern of assigning chatDomByTab value back to innerHTML + expect(fn).not.toMatch(/chatMessages\.innerHTML\s*=\s*chatDomByTab/); + }); + + it('switchChatTab uses createDocumentFragment to save chat DOM', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'switchChatTab'); + expect(fn).toContain('createDocumentFragment'); + }); + + it('switchChatTab moves nodes via appendChild/firstChild (not innerHTML assignment)', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'switchChatTab'); + // Must use appendChild to restore nodes from fragment + expect(fn).toContain('chatMessages.appendChild'); + }); + + it('chatDomByTab comment documents that values are DocumentFragments, not strings', () => { + // Check module-level comment on chatDomByTab + const commentIdx = SIDEPANEL_SRC.indexOf('chatDomByTab'); + const commentLine = SIDEPANEL_SRC.slice(commentIdx, commentIdx + 120); + expect(commentLine).toMatch(/DocumentFragment|fragment/i); + }); + + it('welcome screen is built with DOM methods in the else branch (not innerHTML)', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'switchChatTab'); + // The else branch must use createElement, not innerHTML template literal + expect(fn).toContain('createElement'); + // The specific innerHTML template with chat-welcome must be gone + expect(fn).not.toMatch(/innerHTML\s*=\s*`[\s\S]*?chat-welcome/); + }); +}); + diff --git a/extension/sidepanel.js b/extension/sidepanel.js index bfd34e203..ba5d76a8c 100644 --- a/extension/sidepanel.js +++ b/extension/sidepanel.js @@ -20,7 +20,8 @@ let connState = 'disconnected'; // disconnected | connected | reconnecting | dea let lastOptimisticMsg = null; // track optimistically rendered user msg to avoid dupes let sidebarActiveTabId = null; // which browser tab's chat we're showing const chatLineCountByTab = {}; // tabId -> last seen chatLineCount -const chatDomByTab = {}; // tabId -> saved innerHTML +const chatDomByTab = {}; // tabId -> saved DocumentFragment (never serialized HTML) +let pollInProgress = false; // reentrancy guard — prevents concurrent/recursive pollChat calls let reconnectAttempts = 0; let reconnectTimer = null; const MAX_RECONNECT_ATTEMPTS = 30; // 30 * 2s = 60s before showing "dead" @@ -390,7 +391,9 @@ document.getElementById('stop-agent-btn').addEventListener('click', stopAgent); let initialLoadDone = false; async function pollChat() { - if (!serverUrl || !serverToken) return; + if (pollInProgress) return; + pollInProgress = true; + if (!serverUrl || !serverToken) { pollInProgress = false; return; } try { // Request chat for the currently displayed tab const tabParam = sidebarActiveTabId !== null ? `&tabId=${sidebarActiveTabId}` : ''; @@ -449,6 +452,8 @@ async function pollChat() { updateStopButton(data.agentStatus === 'processing'); } catch (err) { console.error('[gstack sidebar] Chat poll error:', err.message); + } finally { + pollInProgress = false; } } @@ -456,9 +461,14 @@ async function pollChat() { function switchChatTab(newTabId) { if (newTabId === sidebarActiveTabId) return; - // Save current tab's chat DOM + scroll position + // Save current tab's chat DOM into a DocumentFragment (avoids innerHTML + // serialization/re-parsing round-trip which is an XSS vector). if (sidebarActiveTabId !== null) { - chatDomByTab[sidebarActiveTabId] = chatMessages.innerHTML; + const frag = document.createDocumentFragment(); + while (chatMessages.firstChild) { + frag.appendChild(chatMessages.firstChild); + } + chatDomByTab[sidebarActiveTabId] = frag; chatLineCountByTab[sidebarActiveTabId] = chatLineCount; } @@ -468,7 +478,9 @@ function switchChatTab(newTabId) { // mid-message (the server may have switched tabs because the user's // Chrome tab changed, but we still want to show the optimistic UI). if (chatDomByTab[newTabId]) { - chatMessages.innerHTML = chatDomByTab[newTabId]; + // Move nodes back from the fragment — no HTML re-parsing, no XSS risk. + while (chatMessages.firstChild) chatMessages.removeChild(chatMessages.firstChild); + chatMessages.appendChild(chatDomByTab[newTabId]); chatLineCount = chatLineCountByTab[newTabId] || 0; // Reset agent state for restored tab agentContainer = null; @@ -480,12 +492,23 @@ function switchChatTab(newTabId) { chatLineCount = 0; // agentContainer/agentTextEl are already set from sendMessage() } else { - chatMessages.innerHTML = ` -
-
G
-

Send a message about this page.

-

Each tab has its own conversation.

-
`; + // Build welcome screen with DOM methods — never innerHTML for untrusted content. + chatMessages.textContent = ''; + const welcome = document.createElement('div'); + welcome.className = 'chat-welcome'; + welcome.id = 'chat-welcome'; + const icon = document.createElement('div'); + icon.className = 'chat-welcome-icon'; + icon.textContent = 'G'; + const p1 = document.createElement('p'); + p1.textContent = 'Send a message about this page.'; + const p2 = document.createElement('p'); + p2.className = 'muted'; + p2.textContent = 'Each tab has its own conversation.'; + welcome.appendChild(icon); + welcome.appendChild(p1); + welcome.appendChild(p2); + chatMessages.appendChild(welcome); chatLineCount = 0; // Reset agent state for fresh tab agentContainer = null; @@ -493,8 +516,8 @@ function switchChatTab(newTabId) { agentText = ''; } - // Immediately poll the new tab's chat - pollChat(); + // Defer poll to avoid direct mutual recursion with pollChat. + setTimeout(pollChat, 0); } function updateStopButton(agentRunning) { From 481b3f87ad3aef85794b763440817a6a5653b666 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:36:44 +0000 Subject: [PATCH 14/16] fix(security): break switchChatTab/pollChat mutual recursion with reentrancy guard --- browse/test/security-audit-r2.test.ts | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index 26b1001b0..f2bd830f5 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -616,3 +616,38 @@ describe('Task 14: switchChatTab uses DocumentFragment, not innerHTML round-trip }); }); +// ─── Task 15: pollChat/switchChatTab reentrancy guard ──────────────────────── + +describe('Task 15: pollChat reentrancy guard and deferred call in switchChatTab', () => { + it('pollInProgress guard variable is declared at module scope', () => { + // Must be declared before any function definitions (within first 2000 chars) + const moduleTop = SIDEPANEL_SRC.slice(0, 2000); + expect(moduleTop).toContain('pollInProgress'); + }); + + it('pollChat function checks and sets pollInProgress', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'pollChat'); + expect(fn).toBeTruthy(); + expect(fn).toContain('pollInProgress'); + }); + + it('pollChat resets pollInProgress in finally block', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'pollChat'); + // The finally block must contain the reset + const finallyIdx = fn.indexOf('finally'); + expect(finallyIdx).toBeGreaterThan(-1); + const finallyBlock = fn.slice(finallyIdx, finallyIdx + 60); + expect(finallyBlock).toContain('pollInProgress'); + }); + + it('switchChatTab calls pollChat via setTimeout (not directly)', () => { + const fn = extractFunction(SIDEPANEL_SRC, 'switchChatTab'); + // Must use setTimeout to defer pollChat — no direct call at the end + expect(fn).toMatch(/setTimeout\s*\(\s*pollChat/); + // Must NOT have a bare direct call `pollChat()` at the end (outside setTimeout) + // We check that there is no standalone `pollChat()` call (outside setTimeout wrapper) + const withoutSetTimeout = fn.replace(/setTimeout\s*\(\s*pollChat[^)]*\)/g, ''); + expect(withoutSetTimeout).not.toMatch(/\bpollChat\s*\(\s*\)/); + }); +}); + From e966c75f26cfdefca946fda5ce25a2b514bf3a61 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:36:49 +0000 Subject: [PATCH 15/16] fix(security): add SIGKILL escalation to sidebar-agent timeout handler --- browse/src/sidebar-agent.ts | 5 +++-- browse/test/security-audit-r2.test.ts | 28 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/browse/src/sidebar-agent.ts b/browse/src/sidebar-agent.ts index c7bd525db..795731826 100644 --- a/browse/src/sidebar-agent.ts +++ b/browse/src/sidebar-agent.ts @@ -313,9 +313,10 @@ async function askClaude(queueEntry: QueueEntry): Promise { // Timeout (default 300s / 5 min — multi-page tasks need time) const timeoutMs = parseInt(process.env.SIDEBAR_AGENT_TIMEOUT || '300000', 10); setTimeout(() => { - try { proc.kill(); } catch (killErr: any) { - console.warn(`[sidebar-agent] Tab ${tid}: Failed to kill timed-out process:`, killErr.message); + try { proc.kill('SIGTERM'); } catch (killErr: any) { + console.warn(`[sidebar-agent] Tab ${tid}: Failed to SIGTERM timed-out process:`, killErr.message); } + setTimeout(() => { try { proc.kill('SIGKILL'); } catch {} }, 3000); const timeoutMsg = stderrBuffer.trim() ? `Timed out after ${timeoutMs / 1000}s\nstderr: ${stderrBuffer.trim().slice(-500)}` : `Timed out after ${timeoutMs / 1000}s`; diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index f2bd830f5..d82e35d73 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -651,3 +651,31 @@ describe('Task 15: pollChat reentrancy guard and deferred call in switchChatTab' }); }); +// ─── Task 16: SIGKILL escalation in sidebar-agent timeout ──────────────────── + +describe('Task 16: sidebar-agent timeout handler uses SIGTERM→SIGKILL escalation', () => { + it('timeout block sends SIGTERM first', () => { + // Slice from "Timed out" / setTimeout block to processingTabs.delete + const timeoutStart = AGENT_SRC.indexOf("SIDEBAR_AGENT_TIMEOUT"); + expect(timeoutStart).toBeGreaterThan(-1); + const timeoutBlock = AGENT_SRC.slice(timeoutStart, timeoutStart + 600); + expect(timeoutBlock).toContain('SIGTERM'); + }); + + it('timeout block escalates to SIGKILL after delay', () => { + const timeoutStart = AGENT_SRC.indexOf("SIDEBAR_AGENT_TIMEOUT"); + const timeoutBlock = AGENT_SRC.slice(timeoutStart, timeoutStart + 600); + expect(timeoutBlock).toContain('SIGKILL'); + }); + + it('SIGTERM appears before SIGKILL in timeout block', () => { + const timeoutStart = AGENT_SRC.indexOf("SIDEBAR_AGENT_TIMEOUT"); + const timeoutBlock = AGENT_SRC.slice(timeoutStart, timeoutStart + 600); + const sigtermIdx = timeoutBlock.indexOf('SIGTERM'); + const sigkillIdx = timeoutBlock.indexOf('SIGKILL'); + expect(sigtermIdx).toBeGreaterThan(-1); + expect(sigkillIdx).toBeGreaterThan(-1); + expect(sigtermIdx).toBeLessThan(sigkillIdx); + }); +}); + From c902fe230fc700d541285ac4f8bf690fb2d5bdb5 Mon Sep 17 00:00:00 2001 From: "sqry-release-plz[bot]" <271695377+sqry-release-plz[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 02:36:56 +0000 Subject: [PATCH 16/16] fix(security): add bounds validation to viewport dimensions and wait timeouts --- browse/src/write-commands.ts | 12 +++++++--- browse/test/security-audit-r2.test.ts | 33 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 603c41fed..e0473a1e0 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -318,7 +318,9 @@ export async function handleWriteCommand( const selector = args[0]; if (!selector) throw new Error('Usage: browse wait '); if (selector === '--networkidle') { - const timeout = args[1] ? parseInt(args[1], 10) : 15000; + const MAX_WAIT_MS = 300_000; + const MIN_WAIT_MS = 1_000; + const timeout = Math.min(Math.max(args[1] ? parseInt(args[1], 10) || MIN_WAIT_MS : 15000, MIN_WAIT_MS), MAX_WAIT_MS); await page.waitForLoadState('networkidle', { timeout }); return 'Network idle'; } @@ -330,7 +332,9 @@ export async function handleWriteCommand( await page.waitForLoadState('domcontentloaded'); return 'DOM content loaded'; } - const timeout = args[1] ? parseInt(args[1], 10) : 15000; + const MAX_WAIT_MS = 300_000; + const MIN_WAIT_MS = 1_000; + const timeout = Math.min(Math.max(args[1] ? parseInt(args[1], 10) || MIN_WAIT_MS : 15000, MIN_WAIT_MS), MAX_WAIT_MS); const resolved = await bm.resolveRef(selector); if ('locator' in resolved) { await resolved.locator.waitFor({ state: 'visible', timeout }); @@ -343,7 +347,9 @@ export async function handleWriteCommand( case 'viewport': { const size = args[0]; if (!size || !size.includes('x')) throw new Error('Usage: browse viewport (e.g., 375x812)'); - const [w, h] = size.split('x').map(Number); + const [rawW, rawH] = size.split('x').map(Number); + const w = Math.min(Math.max(Math.round(rawW) || 1280, 1), 16384); + const h = Math.min(Math.max(Math.round(rawH) || 720, 1), 16384); await bm.setViewport(w, h); return `Viewport set to ${w}x${h}`; } diff --git a/browse/test/security-audit-r2.test.ts b/browse/test/security-audit-r2.test.ts index d82e35d73..a9efb423b 100644 --- a/browse/test/security-audit-r2.test.ts +++ b/browse/test/security-audit-r2.test.ts @@ -679,3 +679,36 @@ describe('Task 16: sidebar-agent timeout handler uses SIGTERM→SIGKILL escalati }); }); +// ─── Task 17: viewport and wait bounds clamping ────────────────────────────── + +describe('Task 17: viewport dimensions and wait timeouts are clamped', () => { + it('viewport case clamps width and height with Math.min/Math.max', () => { + const block = sliceBetween(WRITE_SRC, "case 'viewport':", "case 'cookie':"); + expect(block).toBeTruthy(); + expect(block).toMatch(/Math\.min|Math\.max/); + }); + + it('viewport case uses rawW/rawH before clamping (not direct destructure)', () => { + const block = sliceBetween(WRITE_SRC, "case 'viewport':", "case 'cookie':"); + expect(block).toContain('rawW'); + expect(block).toContain('rawH'); + }); + + it('wait case (networkidle branch) clamps timeout with MAX_WAIT_MS', () => { + const block = sliceBetween(WRITE_SRC, "case 'wait':", "case 'viewport':"); + expect(block).toBeTruthy(); + expect(block).toMatch(/MAX_WAIT_MS/); + }); + + it('wait case (element branch) also clamps timeout', () => { + const block = sliceBetween(WRITE_SRC, "case 'wait':", "case 'viewport':"); + // Both the networkidle and element branches declare MAX_WAIT_MS + const maxWaitCount = (block.match(/MAX_WAIT_MS/g) || []).length; + expect(maxWaitCount).toBeGreaterThanOrEqual(2); + }); + + it('wait case uses MIN_WAIT_MS as a floor', () => { + const block = sliceBetween(WRITE_SRC, "case 'wait':", "case 'viewport':"); + expect(block).toContain('MIN_WAIT_MS'); + }); +});