Fail closed on hostless readback URLs; ack the Stop gate before the latch may clear

Blocking 1: verifyObservedOnApprovedDomain treated a hostless observed
URL (file:///…, data:…, about:blank — registrableHost '') as approved,
returning ok with content and updating lastLinks. Now any PRESENT
observed.url whose registrable host differs from the grant — including
the empty host — drops the bridge to lost and refuses (same per-action
status mapping); only an absent url passes, since extraction gave
nothing to verify. navigateApprovedLink's pre-check now uses the shared
hostMatchesGrant helper (behavior-identical); the frameNavigated
listener and the post-read check document why they don't.

Blocking 2: the renderer POSTs /browse/control/stop fire-and-forget, so
clearing the latch merely because a poll started after stopRequestedAt
could execute a command handed out before the server gate actually set.
The latch now carries a main-owned ack: while armed and un-acked the
poller itself POSTs the idempotent /browse/control/stop and records
ackAt on 2xx (no dependence on the renderer's POST); every handed-out
command is gated until a poll starts after the ack, and only such a
post-ack poll (command or empty) clears the latch. A fresh Stop resets
the ack. Stop <1s is unchanged (requestedAt set immediately on IPC);
post-resume commands execute; Stop with no new turn stays stopped.
Removed the dead old-latch isStopRequested/isStopRequestedSince API.

Author: ianu82

src/main/browser-bridge.test.ts

@@ -414,6 +414,38 @@ describe('browser-bridge domain-grant immutability', () => {
     expect(bridge.currentState()).toBe('lost');
   });
 
+  // FAIL CLOSED on hostless readback URLs: file:///, data:, about:blank have
+  // no registrable host — the tab left the approved site, so the observation
+  // must be refused, never returned as approved content.
+  for (const hostlessUrl of ['about:blank', 'file:///tmp/x.html', 'data:text/html,hi']) {
+    it(`inspect readback on ${hostlessUrl} is refused + lost (fail closed)`, async () => {
+      await connect();
+      FakeCdpSocket.instances[0].page = { ...FakeCdpSocket.instances[0].page, url: hostlessUrl };
+      const r = await bridge.inspect();
+      expect(r.status).toBe('permission_denied');
+      expect(r.observed).toBeUndefined();
+      expect(bridge.currentState()).toBe('lost');
+    });
+
+    it(`scroll readback on ${hostlessUrl} is refused + lost (fail closed)`, async () => {
+      await connect();
+      FakeCdpSocket.instances[0].page = { ...FakeCdpSocket.instances[0].page, url: hostlessUrl };
+      const r = await bridge.scroll('down');
+      expect(r.status).toBe('permission_denied');
+      expect(r.observed).toBeUndefined();
+      expect(bridge.currentState()).toBe('lost');
+    });
+
+    it(`wait readback on ${hostlessUrl} is refused + lost (fail closed)`, async () => {
+      await connect();
+      FakeCdpSocket.instances[0].page = { ...FakeCdpSocket.instances[0].page, url: hostlessUrl };
+      const r = await bridge.wait(0);
+      expect(r.status).toBe('permission_denied');
+      expect(r.observed).toBeUndefined();
+      expect(bridge.currentState()).toBe('lost');
+    });
+  }
+
   it('a grant under a multi-label suffix does NOT extend to sibling sites (github.io)', async () => {
     // PSL isolation end-to-end: approving foo.github.io must not let the
     // bridge follow a link to bar.github.io — they are unrelated sites that
@@ -527,31 +559,44 @@ describe('browser-bridge conversation binding lifecycle', () => {
 });
 
 describe('browser-bridge local Stop latch', () => {
-  it('records WHEN Stop was pressed so the poller can gate only raced commands', () => {
+  it('records WHEN Stop was pressed, un-acked until the server confirms its gate', () => {
     const before = Date.now();
-    expect(bridge.isStopRequested()).toBe(false);
+    expect(bridge.getStopLatch()).toEqual({ requestedAt: null, ackAt: null });
     bridge.requestStop();
-    expect(bridge.isStopRequested()).toBe(true);
-    // The latch answers "was Stop pressed at-or-after t?" — true for a poll
-    // that started before the stop (raced), false for one started after.
-    expect(bridge.isStopRequestedSince(before)).toBe(true);
-    expect(bridge.isStopRequestedSince(Date.now() + 1)).toBe(false);
+    const latch = bridge.getStopLatch();
+    expect(latch.requestedAt).toBeGreaterThanOrEqual(before);
+    expect(latch.ackAt).toBeNull();
+  });
+
+  it('ackStopRequest records the server-gate confirmation; a fresh Stop resets it', () => {
+    bridge.requestStop();
+    bridge.ackStopRequest();
+    expect(bridge.getStopLatch().ackAt).not.toBeNull();
+    // A new Stop needs a new ack — the previous gate may have been resumed.
+    bridge.requestStop();
+    expect(bridge.getStopLatch().requestedAt).not.toBeNull();
+    expect(bridge.getStopLatch().ackAt).toBeNull();
+  });
+
+  it('a stray ack with no Stop latched must not fabricate latch state', () => {
+    bridge.ackStopRequest();
+    expect(bridge.getStopLatch()).toEqual({ requestedAt: null, ackAt: null });
   });
 
-  it('is cleared by clearStopRequest (the poller after a post-stop poll cycle)', () => {
+  it('is cleared by clearStopRequest (the poller after a post-ack poll cycle)', () => {
     bridge.requestStop();
+    bridge.ackStopRequest();
     bridge.clearStopRequest();
-    expect(bridge.isStopRequested()).toBe(false);
-    expect(bridge.isStopRequestedSince(0)).toBe(false);
+    expect(bridge.getStopLatch()).toEqual({ requestedAt: null, ackAt: null });
   });
 
   it('is cleared belt-and-braces by a fresh attach and by disposeAllBridges', async () => {
     bridge.requestStop();
     await bridge.attach('TAB-1');
-    expect(bridge.isStopRequested()).toBe(false);
+    expect(bridge.getStopLatch().requestedAt).toBeNull();
 
     bridge.requestStop();
     bridge.disposeAllBridges();
-    expect(bridge.isStopRequested()).toBe(false);
+    expect(bridge.getStopLatch().requestedAt).toBeNull();
   });
 });

src/main/browser-bridge.ts

@@ -27,6 +27,7 @@ import {
   type BrowserStatusResult,
   type ObservedResult,
   type ObservedLink,
+  hostMatchesGrant,
   isReadonlyCdpMethod,
   registrableHost,
 } from '../shared/browser-bridge-types';
@@ -83,43 +84,58 @@ export function getConversationId(): string | null {
   return conversationId;
 }
 
-// Local user-Stop signal. Stop/resume lifecycle: the SERVER is the single
-// source of truth. The renderer's Stop button sets the SERVER control gate
-// (POST /browse/control/stop) — while stopped the server hands out NO
-// commands — and a fresh user turn resumes it server-side
+// Local user-Stop latch. THE canonical Stop-lifecycle description lives here
+// (the poller's comments point at this block).
+//
+// Stop/resume lifecycle: the SERVER is the single source of truth. Stop sets
+// the SERVER control gate (POST /browse/control/stop) — while stopped the
+// server hands out NO commands — and a fresh user turn resumes it server-side
 // (resume_on_new_turn). Re-approval is required only after
 // take-over/lost/revoke, never after Stop.
 //
 // This local latch exists ONLY to close the hand-out→execute race: a command
-// the server handed to the wire just before the Stop landed (Stop leaves the
+// the server handed to the wire before its stop gate was set (Stop leaves the
 // bridge connected, so the poller's currentState() re-check can't catch it).
-// It records WHEN Stop was pressed (a timestamp, set via IPC BROWSER_STOP);
-// the poller compares it against when its long-poll started and gates only
-// commands that could predate the stop, then clears the latch. It must NEVER
-// gate post-resume commands — those come from a server whose gate was already
-// stopped and then resumed by a new turn. Also cleared on attach()/dispose as
-// belt-and-braces.
+// `requestedAt` is set immediately on IPC BROWSER_STOP (Stop takes effect
+// locally in <1s). The renderer also POSTs /browse/control/stop, but it does
+// so fire-and-forget — the server gate is only KNOWN to be set once an ack is
+// recorded, so the MAIN process re-POSTs the (idempotent) stop from the
+// poller each cycle while un-acked and records `ackAt` when a 2xx lands
+// (main-owned ack: no dependence on the renderer's POST succeeding).
+//
+// The poller gates every handed-out command while the latch is armed
+// (requestedAt set, no ack yet). The ack is awaited BEFORE the poll in the
+// same single-in-flight loop, so once acked, any command a later poll returns
+// was necessarily handed out by a server whose gate had already been set —
+// i.e. resumed by a fresh user turn — and the latch is cleared and the
+// command executes. The latch must never outlive its ack to gate post-resume
+// commands (that was the old bug: a boolean latch persisted until
+// re-approval). Also cleared on attach()/dispose as belt-and-braces.
 let stopRequestedAt: number | null = null;
+let stopAckAt: number | null = null;
 
 export function requestStop(): void {
   stopRequestedAt = Date.now();
+  // A fresh Stop needs a fresh server ack — the previous gate may have been
+  // resumed since.
+  stopAckAt = null;
 }
 
-// Was Stop pressed at-or-after `sinceMs`? Used by the poller to detect the
-// raced case: a stop that landed while its long-poll was outstanding.
-export function isStopRequestedSince(sinceMs: number): boolean {
-  return stopRequestedAt !== null && stopRequestedAt >= sinceMs;
+// Record that the server confirmed (2xx) its stop gate is set. No-op when no
+// Stop is latched (a stray ack must not fabricate a latch state).
+export function ackStopRequest(): void {
+  if (stopRequestedAt !== null) stopAckAt = Date.now();
 }
 
-// Is any un-cleared Stop latched at all (regardless of timing)?
-export function isStopRequested(): boolean {
-  return stopRequestedAt !== null;
+export function getStopLatch(): { requestedAt: number | null; ackAt: number | null } {
+  return { requestedAt: stopRequestedAt, ackAt: stopAckAt };
 }
 
-// The latch's raced-case job is done (the poller gated or safely bypassed
-// it); post-resume commands must flow freely.
+// The latch's raced-case job is done (the server gate is known set and the
+// poller finished a post-ack cycle); post-resume commands must flow freely.
 export function clearStopRequest(): void {
   stopRequestedAt = null;
+  stopAckAt = null;
 }
 
 // Main-side bridge-state subscribers (e.g. the command poller). Distinct from
@@ -218,6 +234,7 @@ export function __resetBridgeForTest(): void {
   bridgeStateListeners.clear();
   conversationId = null;
   stopRequestedAt = null;
+  stopAckAt = null;
 }
 
 async function defaultListTargets(base: string): Promise<CdpTarget[]> {
@@ -409,6 +426,7 @@ export async function attach(
   // Belt-and-braces: a fresh approval starts a clean session, so any stale
   // Stop latch is meaningless (resume itself is server-side, on a new turn).
   stopRequestedAt = null;
+  stopAckAt = null;
   // New approval supersedes any in-flight approve() awaiting connect.
   approvalGeneration += 1;
   pendingApproval = { targetId, cancel: () => {} };
@@ -490,6 +508,8 @@ export async function approve(): Promise<{ ok: boolean; state: BridgeState; reas
       if (!frame.url) return;
       const host = registrableHost(frame.url);
       // Empty host (about:blank etc.) — ignore, no cross-host expansion.
+      // (Deliberately NOT hostMatchesGrant: transient hostless frames are
+      // tolerated here; the post-read verification fails them closed.)
       if (host && host !== approvedTarget.domain) {
         handleLost('The tab navigated to a different site, so the approval no longer applies.');
       }
@@ -651,32 +671,36 @@ async function extractObserved(): Promise<ObservedResult> {
 // client-side redirect) between the frameNavigated listener firing and the
 // readback — the grant is for the approved domain ONLY, so an off-domain
 // observation is discarded, the bridge drops to lost (re-approval required),
-// and the caller gets a refusal instead of unapproved-site content. Returns
-// the refusal result, or null when the observation is on-grant.
+// and the caller gets a refusal instead of unapproved-site content.
+//
+// FAIL CLOSED on hostless URLs: when observed.url IS present but yields no
+// registrable host (file:///…, data:…, about:blank), the tab left the
+// approved site — refuse, don't approve. (This is why the check is spelled
+// out here instead of using hostMatchesGrant: the helper's empty-host-refuses
+// semantics match, but the missing-url pass below does not.) Only an ABSENT
+// observed.url passes unverified — extraction gave nothing to check.
+// Returns the refusal result, or null when the observation is on-grant.
 function verifyObservedOnApprovedDomain(
   action: BrowserActionType,
   observed: ObservedResult,
 ): BrowserActionResult | null {
   const target = approvedTarget;
   if (!target || !observed.url) return null;
   const host = registrableHost(observed.url);
-  if (!host || host === target.domain) return null;
+  if (host === target.domain) return null;
   // Keep the per-action status mapping: a navigate that LANDED off-domain is
   // a failed navigation; any other primitive observing off-domain content is
   // a permission refusal.
-  const reason =
-    action === 'navigate'
-      ? 'That link leaves the approved site.'
-      : 'The tab is on a different site than the one you approved.';
+  const offSiteReason = 'The tab is on a different site than the one you approved.';
   handleLost(
     action === 'navigate'
       ? 'The page redirected to a different site, so the approval no longer applies.'
-      : 'The tab is on a different site than the one you approved.',
+      : offSiteReason,
   );
   return {
     status: action === 'navigate' ? 'navigation_failed' : 'permission_denied',
     action,
-    reason,
+    reason: action === 'navigate' ? 'That link leaves the approved site.' : offSiteReason,
   };
 }
 
@@ -710,7 +734,7 @@ export async function navigateApprovedLink(href: string): Promise<BrowserActionR
       reason: 'That link is not one of the links found on the approved page.',
     };
   }
-  if (registrableHost(href) !== target.domain) {
+  if (!hostMatchesGrant(href, target.domain)) {
     return {
       status: 'navigation_failed',
       action: 'navigate',
@@ -799,6 +823,7 @@ export function disposeAllBridges(): void {
   bridgeState = 'disconnected';
   conversationId = null;
   stopRequestedAt = null;
+  stopAckAt = null;
   // Tear down the Chrome WE launched (app quit / full drain). If Chrome was
   // already running on the port when we attached, managedChrome is null and we
   // leave the user's own Chrome alone.