From c258e03125cf9612376342d5daf6b50963b2150b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 18 Apr 2026 23:05:23 +0800 Subject: [PATCH] fix: pre-landing review fixes (9 findings from specialist + adversarial review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (Claude subagent + Codex) surfaced 9 bugs across CRITICAL/HIGH severity. All fixed: 1. tab-session.ts:setTabContent — state mutation moved AFTER the setContent await. Prior order left phantom HTML in replay metadata if setContent threw (timeout, browser crash), which a later viewport --scale would silently replay. Now loadedHtml is only recorded on successful load. 2. browser-manager.ts:setDeviceScaleFactor — rollback now forces a second recreateContext after restoring the old fields. The fallback path in the original recreateContext builds a blank context using whatever this.deviceScaleFactor/currentViewport hold at that moment (which were the NEW values we were trying to apply). Rolling back the fields without a second recreate left the live context at new-scale while state tracked old-scale. Now: restore fields, force re-recreate with old values, only if that ALSO fails do we return a combined error. 3. commands.ts:buildUnknownCommandError — Levenshtein tiebreak simplified to 'd <= 2 && d < bestDist' (strict less). Candidates are pre-sorted alphabetically, so first equal-distance wins by default. The prior '(d === bestDist && best !== undefined && cand < best)' clause was dead code. 4. tab-session.ts:onMainFrameNavigated — now clears loadedHtml, not just refs + frame. Without this, a user who load-html'd then clicked a link (or had a form submit / JS redirect / OAuth flow) would retain the stale replay metadata. The next viewport --scale would silently revert the tab to the ORIGINAL loaded HTML, losing whatever the post-navigation content was. Silent data corruption. Browser-emitted navigations trigger this path via wirePageEvents. 5. browser-manager.ts:saveState + restoreState — tab ownership now flows through BrowserState.owner. Without this, a scoped agent's viewport --scale would strand them: tab IDs change during recreate, ownership map held stale IDs, owner lookup failed. New IDs had no owner, so writes without tabId were denied (DoS). Worse, if the agent sent a stale tabId the server's swallowed-tab-switch-error path would let the command hit whatever tab was currently active (cross-tab authz bypass). Now: clear ownership before restore, re-add per-tab with new IDs. 6. meta-commands.ts:state load — disk-loaded state.pages is now explicit allowlist (url, isActive, storage:null) instead of object spread. Spreading accepted loadedHtml, loadedHtmlWaitUntil, and owner from a user-writable state file, letting a tampered state.json smuggle HTML past load-html's safe-dirs / extension / magic-byte / 50MB-cap validators, or forge tab ownership. Now stripped at the boundary. 7. url-validation.ts:normalizeFileUrl — preserves query string + fragment across normalization. file://./app.html?route=home#login previously resolved to a filesystem path that URL-encoded '?' as %3F and '#' as %23, or (for absolute forms) pathToFileURL dropped them entirely. SPAs and fixture URLs with query params 404'd or loaded the wrong route. Now: split on ?/# before path resolution, reattach after. 8. url-validation.ts:validateNavigationUrl — reattaches parsed.search + parsed.hash to the normalized file:// URL. Same fix at the main validator for absolute paths that go through fileURLToPath round-trip. 9. server.ts:writeAuditEntry — audit entries now include aliasOf when the user typed an alias ('setcontent' → cmd: 'load-html', aliasOf: 'setcontent'). Previously the isAliased variable was computed but dropped, losing the raw input from the forensic trail. Completes the plan's codex v3 P2 requirement. Also added bm.getCurrentViewport() and switched 'viewport --scale'- without-size to read from it (more reliable than page.viewportSize() on headed/transition contexts). Tests pass: exit 0, no failures. Build clean. --- browse/src/audit.ts | 4 ++++ browse/src/browser-manager.ts | 39 +++++++++++++++++++++++++++++++++-- browse/src/commands.ts | 13 ++++++------ browse/src/meta-commands.ts | 10 ++++++++- browse/src/server.ts | 2 ++ browse/src/tab-session.ts | 14 +++++++++++-- browse/src/url-validation.ts | 39 +++++++++++++++++++++++++++-------- browse/src/write-commands.ts | 7 ++++--- 8 files changed, 105 insertions(+), 23 deletions(-) diff --git a/browse/src/audit.ts b/browse/src/audit.ts index 5ac59f6d..b6e54638 100644 --- a/browse/src/audit.ts +++ b/browse/src/audit.ts @@ -18,6 +18,9 @@ import * as fs from 'fs'; export interface AuditEntry { ts: string; cmd: string; + /** If the agent typed an alias (e.g. 'setcontent'), the raw input is preserved here + * while `cmd` holds the canonical name ('load-html'). Omitted when cmd === rawCmd. */ + aliasOf?: string; args: string; origin: string; durationMs: number; @@ -56,6 +59,7 @@ export function writeAuditEntry(entry: AuditEntry): void { hasCookies: entry.hasCookies, mode: entry.mode, }; + if (entry.aliasOf) record.aliasOf = entry.aliasOf; if (truncatedError) record.error = truncatedError; fs.appendFileSync(auditPath, JSON.stringify(record) + '\n'); diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index e67bb73a..2885d1cc 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -37,6 +37,12 @@ export interface BrowserState { */ loadedHtml?: string; loadedHtmlWaitUntil?: 'load' | 'domcontentloaded' | 'networkidle'; + /** + * Tab owner clientId for multi-agent isolation. Survives context recreation so + * scoped agents don't get locked out of their own tabs after viewport --scale. + * In-memory only. + */ + owner?: string; }>; } @@ -882,6 +888,8 @@ export class BrowserManager { // can replay it via setTabContent. Never persisted to disk. const session = this.tabSessions.get(id); const loaded = session?.getLoadedHtml(); + // Preserve tab ownership through recreation so scoped agents aren't locked out. + const owner = this.tabOwnership.get(id); pages.push({ url: url === 'about:blank' ? '' : url, @@ -889,6 +897,7 @@ export class BrowserManager { storage, loadedHtml: loaded?.html, loadedHtmlWaitUntil: loaded?.waitUntil, + owner, }); } @@ -908,6 +917,12 @@ export class BrowserManager { await this.context.addCookies(state.cookies); } + // Clear stale ownership — the old tab IDs are gone. We'll re-add per-tab + // owners below as each saved tab gets a fresh ID. Without this reset, old + // tabId → clientId entries would linger and match new tabs with the same + // sequential IDs, silently granting ownership to the wrong clients. + this.tabOwnership.clear(); + // Re-create pages let activeId: number | null = null; for (const saved of state.pages) { @@ -918,6 +933,12 @@ export class BrowserManager { this.tabSessions.set(id, newSession); this.wirePageEvents(page); + // Restore tab ownership for the new ID — preserves scoped-agent isolation + // across context recreation (viewport --scale, user-agent change, handoff). + if (saved.owner) { + this.tabOwnership.set(id, saved.owner); + } + if (saved.loadedHtml) { // Replay load-html content via setTabContent — this rehydrates // TabSession.loadedHtml so the next saveState sees it. page.setContent() @@ -1068,10 +1089,19 @@ export class BrowserManager { const err = await this.recreateContext(); if (err !== null) { - // recreateContext already warned and reset to a blank tab; roll back the fields - // so the next call doesn't inherit the failed values. + // recreateContext's fallback path built a blank context using the NEW scale + + // viewport (the fields we just set). Rolling the fields back without a second + // recreate would leave the live context at new-scale while state says old-scale. + // Roll back fields FIRST, then force a second recreate against the old values + // so live state matches tracked state. this.deviceScaleFactor = prevScale; this.currentViewport = prevViewport; + const rollbackErr = await this.recreateContext(); + if (rollbackErr !== null) { + // Second recreate also failed — we're in a clean blank slate via fallback, but + // with old scale. Return the original error so the caller sees the primary failure. + return `${err} (rollback also encountered: ${rollbackErr})`; + } return err; } return null; @@ -1082,6 +1112,11 @@ export class BrowserManager { return this.deviceScaleFactor; } + /** Read current tracked viewport (for tests + `viewport --scale` size fallback). */ + getCurrentViewport(): { width: number; height: number } { + return { ...this.currentViewport }; + } + // ─── Handoff: Headless → Headed ───────────────────────────── /** * Hand off browser control to the user by relaunching in headed mode. diff --git a/browse/src/commands.ts b/browse/src/commands.ts index fff5c9a6..22c30694 100644 --- a/browse/src/commands.ts +++ b/browse/src/commands.ts @@ -238,17 +238,18 @@ export function buildUnknownCommandError( let msg = `Unknown command: '${command}'.`; // Suggestion via Levenshtein, gated on input length to avoid noisy short-input matches. + // Candidates are pre-sorted alphabetically, so strict "d < bestDist" gives us the + // closest match with alphabetical tiebreak for free — first equal-distance candidate + // wins because subsequent equal-distance candidates fail the strict-less check. if (command.length >= 4) { let best: string | undefined; - let bestDist = 3; + let bestDist = 3; // sentinel: distance 3 would be rejected by the <= 2 gate below const candidates = [...commandSet, ...Object.keys(aliasMap)].sort(); for (const cand of candidates) { const d = levenshtein(command, cand); - if (d < bestDist || (d === bestDist && best !== undefined && cand < best)) { - if (d <= 2) { - best = cand; - bestDist = d; - } + if (d <= 2 && d < bestDist) { + best = cand; + bestDist = d; } } if (best) msg += ` Did you mean '${best}'?`; diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 0ba9d8d6..6eb597c9 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -632,9 +632,17 @@ export async function handleMetaCommand( // Close existing pages, then restore (replace, not merge) bm.setFrame(null); await bm.closeAllPages(); + // Allowlist disk-loaded page fields — NEVER accept loadedHtml, loadedHtmlWaitUntil, + // or owner from disk. Those are in-memory-only invariants; allowing them would let + // a tampered state file smuggle HTML past load-html's safe-dirs + magic-byte + size + // checks, or forge tab ownership for cross-agent authorization bypass. await bm.restoreState({ cookies: validatedCookies, - pages: data.pages.map((p: any) => ({ ...p, storage: null })), + pages: data.pages.map((p: any) => ({ + url: typeof p.url === 'string' ? p.url : '', + isActive: Boolean(p.isActive), + storage: null, + })), }); return `State loaded: ${data.cookies.length} cookies, ${data.pages.length} pages`; } diff --git a/browse/src/server.ts b/browse/src/server.ts index f460fb0e..3a825c1e 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1160,6 +1160,7 @@ async function handleCommandInternal( writeAuditEntry({ ts: new Date().toISOString(), cmd: command, + aliasOf: isAliased ? rawCommand : undefined, args: args.join(' '), origin: browserManager.getCurrentUrl(), durationMs: successDuration, @@ -1204,6 +1205,7 @@ async function handleCommandInternal( writeAuditEntry({ ts: new Date().toISOString(), cmd: command, + aliasOf: isAliased ? rawCommand : undefined, args: args.join(' '), origin: browserManager.getCurrentUrl(), durationMs: errorDuration, diff --git a/browse/src/tab-session.ts b/browse/src/tab-session.ts index 06a00c4b..73994268 100644 --- a/browse/src/tab-session.ts +++ b/browse/src/tab-session.ts @@ -157,11 +157,18 @@ export class TabSession { } /** - * Called on main-frame navigation to clear stale refs and frame context. + * Called on main-frame navigation to clear stale refs, frame context, and any + * load-html replay metadata. Runs for every main-frame nav — explicit goto/back/ + * forward/reload AND browser-emitted navigations (link clicks, form submits, JS + * redirects, OAuth). Without clearing loadedHtml here, a user who load-html'd and + * then clicked a link would silently revert to the original HTML on the next + * viewport --scale. */ onMainFrameNavigated(): void { this.clearRefs(); this.activeFrame = null; + this.loadedHtml = null; + this.loadedHtmlWaitUntil = undefined; } // ─── Loaded HTML (load-html replay) ─────────────────────── @@ -174,9 +181,12 @@ export class TabSession { */ async setTabContent(html: string, opts: { waitUntil?: SetContentWaitUntil } = {}): Promise { const waitUntil = opts.waitUntil ?? 'domcontentloaded'; + // Call setContent FIRST — only record the replay metadata after a successful load. + // If setContent throws (timeout, crash), we must not leave phantom HTML that a + // later viewport --scale would replay. + await this.page.setContent(html, { waitUntil, timeout: 15000 }); this.loadedHtml = html; this.loadedHtmlWaitUntil = waitUntil; - await this.page.setContent(html, { waitUntil, timeout: 15000 }); } /** Get stored HTML + waitUntil for state replay. Returns null if no load-html happened. */ diff --git a/browse/src/url-validation.ts b/browse/src/url-validation.ts index 18c01fdc..a619f182 100644 --- a/browse/src/url-validation.ts +++ b/browse/src/url-validation.ts @@ -128,7 +128,25 @@ async function resolvesToBlockedIp(hostname: string): Promise { export function normalizeFileUrl(url: string): string { if (!url.toLowerCase().startsWith('file:')) return url; - const rest = url.slice('file:'.length); + // Split off query + fragment BEFORE touching the path — SPAs + fixture URLs rely + // on these. path.resolve would URL-encode `?` and `#` as `%3F`/`%23` (and + // pathToFileURL drops them entirely), silently routing preview URLs to the + // wrong fixture. Extract, normalize the path, reattach at the end. + // + // Parse order: `?` before `#` per RFC 3986 — '?' in a fragment is literal. + // Find the FIRST `?` or `#`, whichever comes first, and take everything + // after (including the delimiter) as the trailing segment. + const qIdx = url.indexOf('?'); + const hIdx = url.indexOf('#'); + let delimIdx = -1; + if (qIdx >= 0 && hIdx >= 0) delimIdx = Math.min(qIdx, hIdx); + else if (qIdx >= 0) delimIdx = qIdx; + else if (hIdx >= 0) delimIdx = hIdx; + + const pathPart = delimIdx >= 0 ? url.slice(0, delimIdx) : url; + const trailing = delimIdx >= 0 ? url.slice(delimIdx) : ''; + + const rest = pathPart.slice('file:'.length); // file:/// or longer → standard absolute; pass through unchanged (caller validates path). if (rest.startsWith('///')) { @@ -136,7 +154,7 @@ export function normalizeFileUrl(url: string): string { if (rest === '///' || rest === '////') { throw new Error('Invalid file URL: file:/// has no path. Use file:///.'); } - return url; + return pathPart + trailing; } // Everything else: must start with // (we accept file://... only) @@ -161,19 +179,19 @@ export function normalizeFileUrl(url: string): string { if (afterDoubleSlash.startsWith('~/')) { const rel = afterDoubleSlash.slice(2); const absPath = path.join(os.homedir(), rel); - return pathToFileURL(absPath).href; + return pathToFileURL(absPath).href + trailing; } // cwd-relative with explicit ./ : file://./ if (afterDoubleSlash.startsWith('./')) { const rel = afterDoubleSlash.slice(2); const absPath = path.resolve(process.cwd(), rel); - return pathToFileURL(absPath).href; + return pathToFileURL(absPath).href + trailing; } // localhost host explicitly allowed: file://localhost/ (pass through to standard parser). if (afterDoubleSlash.toLowerCase().startsWith('localhost/')) { - return url; + return pathPart + trailing; } // Ambiguous: file:/// — treat as cwd-relative ONLY if is a @@ -193,7 +211,7 @@ export function normalizeFileUrl(url: string): string { // Simple-segment cwd-relative: file://docs/page.html → cwd/docs/page.html const absPath = path.resolve(process.cwd(), afterDoubleSlash); - return pathToFileURL(absPath).href; + return pathToFileURL(absPath).href + trailing; } /** @@ -232,6 +250,8 @@ export async function validateNavigationUrl(url: string): Promise { } // Convert URL → filesystem path with proper decoding (handles %20, %2F, etc.) + // fileURLToPath strips query + hash; we reattach them after validation so SPA + // fixture URLs like file:///tmp/app.html?route=home#login survive intact. let fsPath: string; try { fsPath = fileURLToPath(parsed); @@ -244,9 +264,10 @@ export async function validateNavigationUrl(url: string): Promise { // is suspicious. path.resolve will normalize it; check the result against safe dirs. validateReadPath(fsPath); - // Return the canonical file:// URL derived from the filesystem path. - // This guarantees page.goto() gets a well-formed URL regardless of input shape. - return pathToFileURL(fsPath).href; + // Return the canonical file:// URL derived from the filesystem path + original + // query + hash. This guarantees page.goto() gets a well-formed URL regardless + // of input shape while preserving SPA route/query params. + return pathToFileURL(fsPath).href + parsed.search + parsed.hash; } if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { diff --git a/browse/src/write-commands.ts b/browse/src/write-commands.ts index 1c7d547f..d925ac08 100644 --- a/browse/src/write-commands.ts +++ b/browse/src/write-commands.ts @@ -478,9 +478,10 @@ export async function handleWriteCommand( w = Math.min(Math.max(Math.round(rawW) || 1280, 1), 16384); h = Math.min(Math.max(Math.round(rawH) || 720, 1), 16384); } else { - // --scale without WxH → read current size from page - const current = page.viewportSize(); - if (!current) throw new Error('viewport --scale: could not read current viewport size.'); + // --scale without WxH → use BrowserManager's tracked viewport (source of truth + // since setViewport + launchContext keep it in sync). Falls back reliably on + // headed → headless transitions or contexts with viewport:null. + const current = bm.getCurrentViewport(); w = current.width; h = current.height; }