diff --git a/browse/src/path-security.ts b/browse/src/path-security.ts index bc02ccde0..ff224d524 100644 --- a/browse/src/path-security.ts +++ b/browse/src/path-security.ts @@ -31,48 +31,41 @@ 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). */ +/** + * Validate a file path for writing (screenshot, pdf, download, scrape, archive). + * + * Walks the path one component at a time the way the kernel will, resolving + * every existing component (symlinks included) before applying the next, so + * `link/../x` follows `link` first. A component that does not exist yet must + * already sit inside a safe directory; a dangling symlink is rejected because + * a write would create its target. Both the caller's spelling and its lexical + * normalization are checked, since callers write with either. + */ export function validateOutputPath(filePath: string): void { - const resolved = path.resolve(filePath); - - // If the target already exists and is a symlink, resolve through it. - // Without this, a symlink at /tmp/evil.png → /etc/crontab passes the - // parent-directory check (parent is /tmp, which is safe) but the actual - // write follows the symlink to /etc/crontab. - try { - const stat = fs.lstatSync(resolved); - if (stat.isSymbolicLink()) { - const realTarget = fs.realpathSync(resolved); - const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realTarget, dir)); - if (!isSafe) { - throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); + const reject = () => { throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`); }; + for (const spelling of new Set([filePath, path.resolve(filePath)])) { + const root = path.parse(spelling).root; + let resolved = path.resolve(root || '.'); + for (const component of spelling.slice(root.length).split(process.platform === 'win32' ? /[\\/]+/ : /\/+/)) { + if (!component || component === '.') continue; + if (component === '..') { + resolved = path.dirname(resolved); + continue; + } + resolved = path.join(resolved, component); + try { + fs.lstatSync(resolved); + } catch (err: any) { + if (err?.code === 'ENOENT' && SAFE_DIRECTORIES.some(dir => isPathWithin(resolved, dir))) continue; + reject(); + } + try { + resolved = fs.realpathSync.native(resolved); + } catch { + reject(); } - return; // symlink target verified, no need to check parent } - } catch (e: any) { - // ENOENT = file doesn't exist yet, fall through to parent-dir check - if (e.code !== 'ENOENT') throw e; - } - - // For new files (no existing symlink), verify the parent directory. - // 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(', ')}`); + if (!SAFE_DIRECTORIES.some(dir => isPathWithin(resolved, dir))) reject(); } } diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index b1178cd6d..b771deb88 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -1309,12 +1309,17 @@ export async function handleWriteCommand( const selectorIdx = args.indexOf('--selector'); const selector = selectorIdx >= 0 ? args[selectorIdx + 1] : undefined; const dirIdx = args.indexOf('--dir'); - const dir = dirIdx >= 0 ? args[dirIdx + 1] : path.join(TEMP_DIR, `browse-scrape-${Date.now()}`); + const requestedDir = dirIdx >= 0 ? args[dirIdx + 1] : path.join(TEMP_DIR, `browse-scrape-${Date.now()}`); const limitIdx = args.indexOf('--limit'); const limit = Math.min(limitIdx >= 0 ? parseInt(args[limitIdx + 1], 10) || 50 : 50, 200); - validateOutputPath(dir); + validateOutputPath(requestedDir); + const dir = path.resolve(requestedDir); fs.mkdirSync(dir, { recursive: true }); + // Each file is validated too: a symlink planted in an existing --dir + // would otherwise redirect the write outside the safe directories. + const manifestPath = path.join(dir, 'manifest.json'); + validateOutputPath(manifestPath); const { extractMedia } = await import('./media-extract'); const target = bm.getActiveFrameOrPage(); @@ -1373,6 +1378,7 @@ export async function handleWriteCommand( const ext = mimeToExt(ct.split(';')[0].trim()); const filename = `${type}-${String(i + 1).padStart(3, '0')}${ext}`; const filePath = path.join(dir, filename); + validateOutputPath(filePath); const body = Buffer.from(await response.body()); try { fs.writeFileSync(filePath, body); @@ -1393,7 +1399,7 @@ export async function handleWriteCommand( } // Write manifest - fs.writeFileSync(path.join(dir, 'manifest.json'), JSON.stringify(manifest, null, 2)); + fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); return `Scraped ${toDownload.length} items to ${dir}/\n${lines.join('\n')}\n\nSummary: ${manifest.succeeded} succeeded, ${manifest.failed} failed, ${Math.round(manifest.total_size / 1024)}KB total`; } diff --git a/browse/test/output-path-symlinks.test.ts b/browse/test/output-path-symlinks.test.ts new file mode 100644 index 000000000..f3be495d9 --- /dev/null +++ b/browse/test/output-path-symlinks.test.ts @@ -0,0 +1,76 @@ +/** + * Output-destination validation (salvaged from #2971): validateOutputPath must + * judge the path the kernel will actually write, component by component. + * The fixture lives under /var/tmp, outside every safe directory, and the + * validator runs in a child whose cwd (a safe directory) is the fixture's + * allowed dir; nothing is ever written to HOME. + */ +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +const VALIDATOR = path.resolve(import.meta.dir, '../src/path-security.ts'); +const POSIX = process.platform !== 'win32' && fs.existsSync('/var/tmp'); +let root = ''; +let allowed = ''; +let outside = ''; + +beforeAll(() => { + if (!POSIX) return; + root = fs.realpathSync(fs.mkdtempSync('/var/tmp/gstack-output-path-')); + allowed = path.join(root, 'allowed'); + outside = path.join(root, 'outside'); + fs.mkdirSync(path.join(allowed, 'nested'), { recursive: true }); + fs.mkdirSync(path.join(outside, 'nested'), { recursive: true }); + fs.writeFileSync(path.join(allowed, 'internal.txt'), 'x'); + fs.writeFileSync(path.join(outside, 'existing.txt'), 'x'); + const links: Array<[string, string]> = [ + ['internal-link.txt', path.join(allowed, 'internal.txt')], + ['internal-parent', path.join(allowed, 'nested')], + ['outward-live.txt', path.join(outside, 'existing.txt')], + ['outward-dangling.txt', path.join(outside, 'missing.txt')], + ['outward-parent', outside], + ['outward-nested', path.join(outside, 'nested')], + ['dangling-parent', path.join(outside, 'missing-dir')], + ['loop', path.join(allowed, 'loop')], + ]; + for (const [name, target] of links) fs.symlinkSync(target, path.join(allowed, name)); +}); + +afterAll(() => { + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +function verdicts(files: string[]): Record { + const script = ` + import { validateOutputPath } from ${JSON.stringify(VALIDATOR)}; + const out = {}; + for (const f of JSON.parse(process.argv[1])) { + try { validateOutputPath(f); out[f] = null; } catch (e) { out[f] = e.message; } + } + console.log(JSON.stringify(out)); + `; + const r = Bun.spawnSync([process.execPath, '-e', script, JSON.stringify(files)], { + cwd: allowed, + env: { ...process.env, TMPDIR: path.join(allowed, 'nested') }, + stdout: 'pipe', + stderr: 'pipe', + timeout: 15_000, + }); + expect(r.exitCode, r.stderr.toString()).toBe(0); + return JSON.parse(r.stdout.toString()); +} + +describe.skipIf(!POSIX)('validateOutputPath follows the physical path', () => { + test('ordinary, new, and in-root symlinked destinations are allowed', () => { + const files = ['ordinary.txt', 'new-dir/deeper/new.txt', 'internal-link.txt', 'internal-parent/x.txt', 'nested/../x.txt']; + expect(verdicts(files.map(f => path.join(allowed, f)))).toEqual(Object.fromEntries(files.map(f => [path.join(allowed, f), null]))); + }); + + test('destinations that a write would land outside are rejected', () => { + const files = ['outward-live.txt', 'outward-dangling.txt', 'outward-parent/new.txt', 'outward-nested/../escaped.txt', 'dangling-parent/new.txt', 'loop', 'loop/new.txt']; + const result = verdicts(files.map(f => `${allowed}/${f}`)); + for (const f of files) expect(result[`${allowed}/${f}`]).toContain('Path must be within'); + expect(fs.existsSync(path.join(outside, 'missing.txt'))).toBe(false); + }); +});