fix(security): 6 browse bugs found via sqry semantic code graph

bin/gstack-telemetry-sync

@@ -122,6 +122,11 @@ case "$HTTP_CODE" in
     # Advance by SENT count (not inserted count) because we can't map inserted back to
     # source lines. If inserted==0, something is systemically wrong — don't advance.
     INSERTED="$(grep -o '"inserted":[0-9]*' "$RESP_FILE" 2>/dev/null | grep -o '[0-9]*' || echo "0")"
+    # Check for upsert errors (installation tracking failures) — log but don't block cursor advance
+    UPSERT_ERRORS="$(grep -o '"upsertErrors"' "$RESP_FILE" 2>/dev/null || true)"
+    if [ -n "$UPSERT_ERRORS" ]; then
+      echo "[gstack-telemetry-sync] Warning: installation upsert errors in response" >&2
+    fi
     if [ "${INSERTED:-0}" -gt 0 ] 2>/dev/null; then
       NEW_CURSOR=$(( CURSOR + COUNT ))
       echo "$NEW_CURSOR" > "$CURSOR_FILE" 2>/dev/null || true

browse/src/browser-manager.ts

@@ -694,7 +694,14 @@ export class BrowserManager {
       this.wirePageEvents(page);
 
       if (saved.url) {
-        await page.goto(saved.url, { waitUntil: 'domcontentloaded', timeout: 15000 }).catch(() => {});
+        // Validate URL before navigation — state files loaded from disk could
+        // contain file://, chrome://, or cloud metadata URLs
+        try {
+          await validateNavigationUrl(saved.url);
+          await page.goto(saved.url, { waitUntil: 'domcontentloaded', timeout: 15000 }).catch(() => {});
+        } catch {
+          // Blocked URL — skip navigation for this page
+        }
       }
 
       if (saved.storage) {

browse/src/meta-commands.ts

@@ -15,11 +15,31 @@ 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));
+  // Resolve symlinks to prevent symlink-based escapes (e.g., /tmp/link → /etc/crontab)
+  let realPath: string;
+  try {
+    realPath = fs.realpathSync(resolved);
+  } catch (err: any) {
+    if (err.code === 'ENOENT') {
+      // File doesn't exist yet (output path) — resolve directory part for symlinks
+      try {
+        const dir = fs.realpathSync(path.dirname(resolved));
+        realPath = path.join(dir, path.basename(resolved));
+      } catch {
+        realPath = resolved;
+      }
+    } else {
+      throw new Error(`Cannot resolve real path: ${filePath} (${err.code})`);
+    }
+  }
+  const isSafe = SAFE_DIRECTORIES.some(dir => isPathWithin(realPath, dir));
   if (!isSafe) {
     throw new Error(`Path must be within: ${SAFE_DIRECTORIES.join(', ')}`);
   }

browse/src/read-commands.ts

@@ -13,6 +13,10 @@ import * as path from 'path';
 import { TEMP_DIR, isPathWithin } from './platform';
 import { inspectElement, formatInspectorResult, getModificationHistory } from './cdp-inspector';
 
+// Redaction patterns for sensitive cookie/storage values — exported for test coverage
+export const SENSITIVE_COOKIE_NAME = /(^|[_.-])(token|secret|key|password|credential|auth|jwt|session|csrf|sid)($|[_.-])|api.?key/i;
+export const SENSITIVE_COOKIE_VALUE = /^(eyJ|sk-|sk_live_|sk_test_|pk_live_|pk_test_|rk_live_|sk-ant-|ghp_|gho_|github_pat_|xox[bpsa]-|AKIA[A-Z0-9]{16}|AIza|SG\.|Bearer\s|sbp_)/;
+
 /** Detect await keyword, ignoring comments. Accepted risk: await in string literals triggers wrapping (harmless). */
 function hasAwait(code: string): boolean {
   const stripped = code.replace(/\/\/.*$/gm, '').replace(/\/\*[\s\S]*?\*\//g, '');
@@ -300,7 +304,14 @@ export async function handleReadCommand(
 
     case 'cookies': {
       const cookies = await page.context().cookies();
-      return JSON.stringify(cookies, null, 2);
+      // Redact cookie values that look like secrets (consistent with storage redaction)
+      const redacted = cookies.map(c => {
+        if (SENSITIVE_COOKIE_NAME.test(c.name) || SENSITIVE_COOKIE_VALUE.test(c.value)) {
+          return { ...c, value: `[REDACTED — ${c.value.length} chars]` };
+        }
+        return c;
+      });
+      return JSON.stringify(redacted, null, 2);
     }
 
     case 'storage': {

browse/src/server.ts

@@ -519,11 +519,19 @@ function spawnClaude(userMessage: string, extensionUrl?: string | null, forTabId
   // Agent status transitions happen when we receive agent_done/agent_error events.
 }
 
-function killAgent(): void {
+function killAgent(targetTabId?: number | null): void {
   if (agentProcess) {
     try { agentProcess.kill('SIGTERM'); } catch {}
     setTimeout(() => { try { agentProcess?.kill('SIGKILL'); } catch {} }, 3000);
   }
+  // Signal the sidebar-agent worker to cancel via a per-tab cancel file.
+  // Using per-tab files prevents race conditions where one agent's cancel
+  // signal is consumed by a different tab's agent in concurrent mode.
+  // When targetTabId is provided, only that tab's agent is cancelled.
+  const cancelDir = path.join(process.env.HOME || '/tmp', '.gstack');
+  const tabId = targetTabId ?? agentTabId ?? 0;
+  const cancelFile = path.join(cancelDir, `sidebar-agent-cancel-${tabId}`);
+  try { fs.writeFileSync(cancelFile, Date.now().toString()); } catch {}
   agentProcess = null;
   agentStartTime = null;
   currentMessage = null;
@@ -1198,7 +1206,8 @@ async function start() {
         if (!validateAuth(req)) {
           return new Response(JSON.stringify({ error: 'Unauthorized' }), { status: 401, headers: { 'Content-Type': 'application/json' } });
         }
-        killAgent();
+        const killBody = await req.json().catch(() => ({}));
+        killAgent(killBody.tabId ?? null);
         addChatEntry({ ts: new Date().toISOString(), role: 'agent', type: 'agent_error', error: 'Killed by user' });
         // Process next in queue
         if (messageQueue.length > 0) {
@@ -1213,7 +1222,8 @@ async function start() {
         if (!validateAuth(req)) {
           return new Response(JSON.stringify({ error: 'Unauthorized' }), { status: 401, headers: { 'Content-Type': 'application/json' } });
         }
-        killAgent();
+        const stopBody = await req.json().catch(() => ({}));
+        killAgent(stopBody.tabId ?? null);
         addChatEntry({ ts: new Date().toISOString(), role: 'agent', type: 'agent_error', error: 'Stopped by user' });
         return new Response(JSON.stringify({ ok: true, queuedMessages: messageQueue.length }), {
           status: 200, headers: { 'Content-Type': 'application/json' },

browse/src/sidebar-agent.ts

@@ -19,10 +19,16 @@ const SERVER_URL = `http://127.0.0.1:${SERVER_PORT}`;
 const POLL_MS = 200;  // 200ms poll — keeps time-to-first-token low
 const B = process.env.BROWSE_BIN || path.resolve(__dirname, '../../.claude/skills/gstack/browse/dist/browse');
 
+const CANCEL_DIR = path.join(process.env.HOME || '/tmp', '.gstack');
+function cancelFileForTab(tabId: number): string {
+  return path.join(CANCEL_DIR, `sidebar-agent-cancel-${tabId}`);
+}
+
 let lastLine = 0;
 let authToken: string | null = null;
 // Per-tab processing — each tab can run its own agent concurrently
 const processingTabs = new Set<number>();
+let activeProc: ReturnType<typeof spawn> | null = null;
 
 // ─── File drop relay ──────────────────────────────────────────
 
@@ -236,6 +242,10 @@ async function askClaude(queueEntry: any): Promise<void> {
     let effectiveCwd = cwd || process.cwd();
     try { fs.accessSync(effectiveCwd); } catch { effectiveCwd = process.cwd(); }
 
+    // Clear any stale cancel signal for this tab before starting
+    const cancelFile = cancelFileForTab(tid);
+    try { fs.unlinkSync(cancelFile); } catch {}
+
     const proc = spawn('claude', claudeArgs, {
       stdio: ['pipe', 'pipe', 'pipe'],
       cwd: effectiveCwd,
@@ -247,9 +257,23 @@ async function askClaude(queueEntry: any): Promise<void> {
         BROWSE_TAB: String(tid),
       },
     });
+    activeProc = proc;
 
     proc.stdin.end();
 
+    // Poll for per-tab cancel signal from server's killAgent()
+    const cancelCheck = setInterval(() => {
+      try {
+        if (fs.existsSync(cancelFile)) {
+          console.log(`[sidebar-agent] Cancel signal received for tab ${tid} — killing claude subprocess`);
+          try { proc.kill('SIGTERM'); } catch {}
+          setTimeout(() => { try { proc.kill('SIGKILL'); } catch {} }, 3000);
+          fs.unlinkSync(cancelFile);
+          clearInterval(cancelCheck);
+        }
+      } catch {}
+    }, 500);
+
     let buffer = '';
 
     proc.stdout.on('data', (data: Buffer) => {
@@ -268,6 +292,8 @@ async function askClaude(queueEntry: any): Promise<void> {
     });
 
     proc.on('close', (code) => {
+      clearInterval(cancelCheck);
+      activeProc = null;
       if (buffer.trim()) {
         try { handleStreamEvent(JSON.parse(buffer), tid); } catch {}
       }
@@ -282,6 +308,8 @@ async function askClaude(queueEntry: any): Promise<void> {
     });
 
     proc.on('error', (err) => {
+      clearInterval(cancelCheck);
+      activeProc = null;
       const errorMsg = stderrBuffer.trim()
         ? `${err.message}\nstderr: ${stderrBuffer.trim().slice(-500)}`
         : err.message;

browse/src/url-validation.ts

@@ -3,13 +3,32 @@
  * Localhost and private IPs are allowed (primary use case: QA testing local dev servers).
  */
 
-const BLOCKED_METADATA_HOSTS = new Set([
+export const BLOCKED_METADATA_HOSTS = new Set([
   '169.254.169.254',  // AWS/GCP/Azure instance metadata
-  'fd00::',           // IPv6 unique local (metadata in some cloud setups)
   'metadata.google.internal', // GCP metadata
   'metadata.azure.internal',  // Azure IMDS
 ]);
 
+/**
+ * IPv6 prefixes to block (CIDR-style). Any address starting with these
+ * hex prefixes is rejected. Covers the full ULA range (fc00::/7 = fc00:: and fd00::).
+ */
+const BLOCKED_IPV6_PREFIXES = ['fc', 'fd'];
+
+/**
+ * Check if an IPv6 address falls within a blocked prefix range.
+ * Handles the full ULA range (fc00::/7) — not just the exact literal fd00::.
+ * Only matches actual IPv6 addresses (must contain ':'), not hostnames
+ * like fd.example.com or fcustomer.com.
+ */
+function isBlockedIpv6(addr: string): boolean {
+  const normalized = addr.toLowerCase().replace(/^\[|\]$/g, '');
+  // Must contain a colon to be an IPv6 address — avoids false positives on
+  // hostnames like fd.example.com or fcustomer.com
+  if (!normalized.includes(':')) return false;
+  return BLOCKED_IPV6_PREFIXES.some(prefix => normalized.startsWith(prefix));
+}
+
 /**
  * Normalize hostname for blocklist comparison:
  * - Strip trailing dot (DNS fully-qualified notation)
@@ -35,7 +54,7 @@ function isMetadataIp(hostname: string): boolean {
   try {
     const probe = new URL(`http://${hostname}`);
     const normalized = probe.hostname;
-    if (BLOCKED_METADATA_HOSTS.has(normalized)) return true;
+    if (BLOCKED_METADATA_HOSTS.has(normalized) || isBlockedIpv6(normalized)) return true;
     // Also check after stripping trailing dot
     if (normalized.endsWith('.') && BLOCKED_METADATA_HOSTS.has(normalized.slice(0, -1))) return true;
   } catch {
@@ -51,9 +70,13 @@ function isMetadataIp(hostname: string): boolean {
 async function resolvesToBlockedIp(hostname: string): Promise<boolean> {
   try {
     const dns = await import('node:dns');
-    const { resolve4 } = dns.promises;
-    const addresses = await resolve4(hostname);
-    return addresses.some(addr => BLOCKED_METADATA_HOSTS.has(addr));
+    const { resolve4, resolve6 } = dns.promises;
+    // Check both IPv4 (A) and IPv6 (AAAA) records — an attacker can use either
+    const [v4Addrs, v6Addrs] = await Promise.all([
+      resolve4(hostname).catch(() => [] as string[]),
+      resolve6(hostname).catch(() => [] as string[]),
+    ]);
+    return [...v4Addrs, ...v6Addrs].some(addr => BLOCKED_METADATA_HOSTS.has(addr) || isBlockedIpv6(addr));
   } catch {
     // DNS resolution failed — not a rebinding risk
     return false;
@@ -76,7 +99,7 @@ export async function validateNavigationUrl(url: string): Promise<void> {
 
   const hostname = normalizeHostname(parsed.hostname.toLowerCase());
 
-  if (BLOCKED_METADATA_HOSTS.has(hostname) || isMetadataIp(hostname)) {
+  if (BLOCKED_METADATA_HOSTS.has(hostname) || isMetadataIp(hostname) || isBlockedIpv6(hostname)) {
     throw new Error(
       `Blocked: ${parsed.hostname} is a cloud metadata endpoint. Access is denied for security.`
     );

browse/src/write-commands.ts

@@ -370,19 +370,30 @@ export async function handleWriteCommand(
       const [selector, ...filePaths] = args;
       if (!selector || filePaths.length === 0) throw new Error('Usage: browse upload <selector> <file1> [file2...]');
 
-      // Validate all files exist before upload
+      // Security: resolve and validate all file paths within safe directories before upload.
+      // Use resolved paths for the actual operation to eliminate TOCTOU symlink races.
+      const safeDirs = [TEMP_DIR, process.cwd()];
+      const resolvedPaths: string[] = [];
       for (const fp of filePaths) {
         if (!fs.existsSync(fp)) throw new Error(`File not found: ${fp}`);
+        const realFp = fs.realpathSync(path.resolve(fp));
+        const isSafe = safeDirs.some(dir => {
+          try { return isPathWithin(realFp, fs.realpathSync(dir)); } catch { return isPathWithin(realFp, dir); }
+        });
+        if (!isSafe) {
+          throw new Error(`Upload path must be within: ${safeDirs.join(', ')}`);
+        }
+        resolvedPaths.push(realFp);
       }
 
       const resolved = await bm.resolveRef(selector);
       if ('locator' in resolved) {
-        await resolved.locator.setInputFiles(filePaths);
+        await resolved.locator.setInputFiles(resolvedPaths);
       } else {
-        await target.locator(resolved.selector).setInputFiles(filePaths);
+        await target.locator(resolved.selector).setInputFiles(resolvedPaths);
       }
 
-      const fileInfo = filePaths.map(fp => {
+      const fileInfo = resolvedPaths.map(fp => {
         const stat = fs.statSync(fp);
         return `${path.basename(fp)} (${stat.size}B)`;
       }).join(', ');
@@ -407,19 +418,17 @@ export async function handleWriteCommand(
     case 'cookie-import': {
       const filePath = args[0];
       if (!filePath) throw new Error('Usage: browse cookie-import <json-file>');
-      // 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(', ')}`);
-        }
-      }
-      if (path.normalize(filePath).includes('..')) {
-        throw new Error('Path traversal sequences (..) are not allowed');
-      }
+      // Path validation — resolve symlinks to prevent reading arbitrary files
       if (!fs.existsSync(filePath)) throw new Error(`File not found: ${filePath}`);
-      const raw = fs.readFileSync(filePath, 'utf-8');
+      const realFilePath = fs.realpathSync(path.resolve(filePath));
+      const safeDirs = [TEMP_DIR, process.cwd()];
+      const isSafe = safeDirs.some(dir => {
+        try { return isPathWithin(realFilePath, fs.realpathSync(dir)); } catch { return isPathWithin(realFilePath, dir); }
+      });
+      if (!isSafe) {
+        throw new Error(`Path must be within: ${safeDirs.join(', ')}`);
+      }
+      const raw = fs.readFileSync(realFilePath, 'utf-8');
       let cookies: any[];
       try { cookies = JSON.parse(raw); } catch { throw new Error(`Invalid JSON in ${filePath}`); }
       if (!Array.isArray(cookies)) throw new Error('Cookie file must contain a JSON array');