mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-11 08:40:22 +02:00
fix: pre-landing review fixes (9 findings from specialist + adversarial review)
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.
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user