From 149fe74349f12bae5c1dfc6c2b3d8b0214553efe Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 13 May 2026 08:05:39 -0700 Subject: [PATCH] fix: harden auth-token validation, TDZ try/catch, lockfile path safety MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three security hardening fixes from /ship adversarial review: 1. AUTH_TOKEN unicode-whitespace bypass (server.ts:67-83). Old: `process.env.AUTH_TOKEN?.trim() || randomUUID()` only stripped ASCII whitespace. A misconfigured embedder shipping AUTH_TOKEN=$'' (BOM) or $'​' (zero-width space) would silently get a one-character bearer secret. New `sanitizeAuthToken()` strips all unicode whitespace via regex and requires >= 16 chars after stripping; anything shorter falls back to crypto.randomUUID(). Same sanitizer used by `resolveConfigFromEnv()` so the embedder path is hardened too. 2. security-classifier.ts checkTranscript safety net. `resolveClaudeCommand()` and `spawn()` can throw under transient conditions (PATH probe failure, posix_spawn ENOMEM). Old code let the throw propagate and rejected the Promise with a raw exception. Now wrapped in try/catch that calls finish() with a degraded signal, matching the graceful-degradation contract the layer already promises for missing-CLI / exit-nonzero / parse-error. 3. cleanSingletonLocks defensive guard tightened (config.ts). Old: basename === 'chromium-profile' OR userDataDir === $CHROMIUM_PROFILE. The second branch was env-controlled and the first was bypassable by passing a relative path that resolved to chromium-profile via CWD drift. New guard: refuses relative paths outright, resolves both sides via path.resolve(), and only accepts the env-match path when $CHROMIUM_PROFILE is itself absolute. Test updates: replace the old `.trim()` test with three new cases covering unicode-whitespace stripping, short-token rejection, and zero-width-only rejection (server-factory.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/config.ts | 27 ++++++++++++++++------- browse/src/security-classifier.ts | 27 +++++++++++++++++------ browse/src/server.ts | 23 ++++++++++++++++---- browse/test/server-factory.test.ts | 35 +++++++++++++++++++++++++++--- 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/browse/src/config.ts b/browse/src/config.ts index e8770ef45..fc4c97b95 100644 --- a/browse/src/config.ts +++ b/browse/src/config.ts @@ -186,24 +186,35 @@ export function resolveChromiumProfile(explicit?: string): string { * ProcessSingleton refuses to start when these exist from a prior crash * (SIGKILL, hard crash, etc.) since they point at a PID that no longer exists. * - * Defensive guard: refuses to operate unless the directory basename is - * 'chromium-profile' OR the path matches the explicit CHROMIUM_PROFILE env - * value. Prevents accidentally deleting lock files from an unrelated - * directory if profile resolution is misconfigured upstream. + * Defensive guard: refuses to operate unless ALL of these hold: + * 1. `userDataDir` is an absolute path (no CWD-relative footguns) + * 2. basename is exactly 'chromium-profile' OR the absolute path matches + * the absolute form of $CHROMIUM_PROFILE env value + * + * Prevents accidentally deleting lock files from an unrelated directory if + * profile resolution is misconfigured upstream (CWD drift, env injection). * * Caller MUST ensure external coordination has already guaranteed no live * peer is using this profile (gbd.lock for gbrowser; single-instance CLI * check for gstack). */ export function cleanSingletonLocks(userDataDir: string): void { - const basename = path.basename(userDataDir); + if (!path.isAbsolute(userDataDir)) { + console.warn(`[browse] cleanSingletonLocks: refusing relative path: ${userDataDir}`); + return; + } + const resolved = path.resolve(userDataDir); + const basename = path.basename(resolved); const explicitProfile = process.env.CHROMIUM_PROFILE; - const isSafe = basename === 'chromium-profile' || userDataDir === explicitProfile; + const explicitAbs = explicitProfile && path.isAbsolute(explicitProfile) + ? path.resolve(explicitProfile) + : null; + const isSafe = basename === 'chromium-profile' || (explicitAbs !== null && resolved === explicitAbs); if (!isSafe) { - console.warn(`[browse] cleanSingletonLocks: refusing to clean unrecognized profile dir: ${userDataDir}`); + console.warn(`[browse] cleanSingletonLocks: refusing to clean unrecognized profile dir: ${resolved}`); return; } for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) { - safeUnlinkQuiet(path.join(userDataDir, lockFile)); + safeUnlinkQuiet(path.join(resolved, lockFile)); } } diff --git a/browse/src/security-classifier.ts b/browse/src/security-classifier.ts index a74844cbc..68a41ba26 100644 --- a/browse/src/security-classifier.ts +++ b/browse/src/security-classifier.ts @@ -511,16 +511,29 @@ export async function checkTranscript(params: { resolve(signal); }; - const claude = resolveClaudeCommand(); + // Wrap resolveClaudeCommand + spawn in try/catch so any unexpected + // throw (PATH probe failure, transient FS error) degrades gracefully + // instead of rejecting the Promise with a raw exception. + let claude: ReturnType; + try { + claude = resolveClaudeCommand(); + } catch (err: any) { + return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: `resolve_error_${err?.message ?? 'unknown'}` } }); + } if (!claude) { return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: 'claude_cli_not_found' } }); } - const p = spawn(claude.command, [ - ...claude.argsPrefix, - '-p', prompt, - '--model', HAIKU_MODEL, - '--output-format', 'json', - ], { stdio: ['ignore', 'pipe', 'pipe'], cwd: os.tmpdir() }); + let p: ReturnType; + try { + p = spawn(claude.command, [ + ...claude.argsPrefix, + '-p', prompt, + '--model', HAIKU_MODEL, + '--output-format', 'json', + ], { stdio: ['ignore', 'pipe', 'pipe'], cwd: os.tmpdir() }); + } catch (err: any) { + return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: `spawn_throw_${err?.message ?? 'unknown'}` } }); + } p.stdout.on('data', (d: Buffer) => (stdout += d.toString())); p.on('exit', (code) => { diff --git a/browse/src/server.ts b/browse/src/server.ts index bf9c32e43..e8804d8bb 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -67,9 +67,21 @@ initAuditLog(config.auditLog); // ─── Auth ─────────────────────────────────────────────────────── // AUTH_TOKEN is injectable via process.env.AUTH_TOKEN so embedders // (gbrowser's gbd daemon spawn) can pre-allocate the token and hand it to -// the Bun child via env. Whitespace-only values fall back to randomUUID so -// the security boundary is never silently weakened by misconfiguration. -const AUTH_TOKEN = (process.env.AUTH_TOKEN?.trim()) || crypto.randomUUID(); +// the Bun child via env. +// +// Validation: require >= 16 chars after stripping ALL unicode whitespace +// (not just ASCII — .trim() misses U+200B / U+FEFF / U+00A0 / etc., which +// would otherwise let a misconfigured embedder ship a one-character BOM as +// the bearer secret). Reject tokens that are too short or contain only +// whitespace; fall back to randomUUID so the security boundary is never +// silently weakened by misconfiguration. +function sanitizeAuthToken(raw: string | undefined): string | null { + if (!raw) return null; + const stripped = raw.replace(/[\s ​-‍]/g, ''); + if (stripped.length < 16) return null; + return stripped; +} +const AUTH_TOKEN = sanitizeAuthToken(process.env.AUTH_TOKEN) || crypto.randomUUID(); initRegistry(AUTH_TOKEN); const BROWSE_PORT = parseInt(process.env.BROWSE_PORT || '0', 10); const IDLE_TIMEOUT_MS = parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10); // 30 min @@ -178,7 +190,10 @@ export function resolveConfigFromEnv(): Omit; } { return { - authToken: (process.env.AUTH_TOKEN?.trim()) || crypto.randomUUID(), + // Same sanitizer as the module-level AUTH_TOKEN: strips ALL unicode + // whitespace and rejects tokens shorter than 16 chars so a misconfigured + // embedder can't ship a BOM/zero-width as the bearer secret. + authToken: sanitizeAuthToken(process.env.AUTH_TOKEN) || crypto.randomUUID(), browsePort: parseInt(process.env.BROWSE_PORT || '0', 10), idleTimeoutMs: parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10), config: resolveConfig(), diff --git a/browse/test/server-factory.test.ts b/browse/test/server-factory.test.ts index 3d7232baf..f6f5429f9 100644 --- a/browse/test/server-factory.test.ts +++ b/browse/test/server-factory.test.ts @@ -53,12 +53,41 @@ describe('server.ts factory API surface', () => { } }); - test('AUTH_TOKEN value is trimmed', () => { + test('AUTH_TOKEN whitespace is stripped (including unicode whitespace)', () => { const orig = process.env.AUTH_TOKEN; - process.env.AUTH_TOKEN = ' padded-token '; + // 22 chars after stripping leading/trailing whitespace including BOM (U+FEFF) + // and zero-width space (U+200B), so passes the 16-char minimum. + process.env.AUTH_TOKEN = ' padded-token-abc123xyz ​'; try { const cfg = resolveConfigFromEnv(); - expect(cfg.authToken).toBe('padded-token'); + expect(cfg.authToken).toBe('padded-token-abc123xyz'); + } finally { + if (orig === undefined) delete process.env.AUTH_TOKEN; + else process.env.AUTH_TOKEN = orig; + } + }); + + test('AUTH_TOKEN shorter than 16 chars after stripping falls back to randomUUID', () => { + const orig = process.env.AUTH_TOKEN; + // Only 5 chars of content — too short for the 16-char minimum. + process.env.AUTH_TOKEN = 'short'; + try { + const cfg = resolveConfigFromEnv(); + // Must be a UUID, not the rejected short token. + expect(cfg.authToken).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/); + } finally { + if (orig === undefined) delete process.env.AUTH_TOKEN; + else process.env.AUTH_TOKEN = orig; + } + }); + + test('AUTH_TOKEN of only zero-width unicode whitespace falls back to randomUUID', () => { + const orig = process.env.AUTH_TOKEN; + // U+200B (ZWSP), U+FEFF (BOM), U+00A0 (NBSP) — would pass .trim() but not the unicode-aware strip. + process.env.AUTH_TOKEN = '​ ​'; + try { + const cfg = resolveConfigFromEnv(); + expect(cfg.authToken).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/); } finally { if (orig === undefined) delete process.env.AUTH_TOKEN; else process.env.AUTH_TOKEN = orig;