Q1: validate browser output paths the way the kernel resolves them (#2971 salvage)

validateOutputPath walks the destination one component at a time, resolving
each existing component before the next, so link/../x, a dangling symlink
and a dangling symlinked parent can no longer route a write outside the
safe directories, while a new multi-level directory inside them is allowed.
scrape validates its manifest and every downloaded file, not only --dir.
Ported from PR #2971 onto current code.

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();
   }
 }
 

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`;
     }

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<string, string | null> {
+  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);
+  });
+});