refactor(desktop): simplify what the security fixes added

Quality pass from four parallel reviews (reuse, simplification, efficiency,
altitude). No behavior change; every gate is unchanged.

Reuse. resolveHostAddresses now calls preferIpv4 instead of re-deriving the
IPv4-first rule inline — one 106-line file had two implementations of the rule
its own TSDoc says callers depend on. url-guard's three host-normalization sites
had two different rules (only one stripped a trailing dot); they share guardHost
now. os-auth's grace check gained the same backwards-clock guard input-activity
already had, since it is the same kind of security window. And a hand-rolled
IPv6 bracket strip in input-validation.server.ts now calls the
unwrapIpv6Brackets already imported at the top of that file.

Simplification. Credential grants are a nested Map rather than a composite
string key, which deletes grantKey, the NUL sentinel, and SECRET_OPERATIONS —
and makes revoke-by-credential a single delete, so a third operation added later
cannot be missed by a revoke that forgot to enumerate it. The 15-term
focusableItself chain is a local array. The 4-line token rationale was pasted
above seven required copies of a 3-line expression; it is stated once now, and
the duplicated isSensitiveValueField TSDoc likewise. PTY_REPLY is a labelled
pattern table rather than six alternations on one line. tagName is upper-cased
once per loop body instead of three times. A side-effecting .filter() is a loop.
senderHasUserGesture's TSDoc no longer documents the implementation it replaced.

Efficiency. The expired-first eviction sweep is removed: every entry gets the
same TTL and a refreshed host is re-inserted at the back, so insertion order IS
expiry order — the sweep could never find an entry the front eviction does not
already hold, and scanned all 256 on every insert to learn that. preferIpv4 uses
ipaddr.IPv4.isValid rather than isValid + parse, which parsed each address
twice. dispose() no longer copies the key set to then get and delete per key.

Also: validateDatabaseHost tests the allow-flag before scanning rather than
after, the blocked-address log line reports the address actually blocked rather
than an arbitrary record, and the preload's two autocomplete-token idioms became
one reader.

Deliberately not done, and why: moving PTY_REPLY into terminal/ and making the
channel gate a predicate (changes the dispatcher shape on both arms); a
consume-once submit gate (behavior change, needs a paste path); folding the
three-way request dispatch into one guardAgentRequest export; hoisting the
release-repo identity into packages/desktop-bridge, which is worth doing and
would make electron-builder.yml, update-feed.ts and updater.ts one fact instead
of three; a shared helper prelude in execInPage so isSecretField stops being
seven copies; and a CDP-sourced focus verdict so the driver stops trusting a
page-derived signal at all. The last three are the ones worth a follow-up.

Author: waleedlatif1

apps/desktop/src/main/browser-agent/page-functions.ts

@@ -303,10 +303,7 @@ export function clickElement(id: number): unknown {
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)
@@ -359,10 +356,7 @@ export function focusElementForTyping(id: number): unknown {
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)
@@ -416,25 +410,14 @@ export function readActiveElementState(): unknown {
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)
       .some((token) => token === 'current-password' || token === 'new-password')
   }
 
-  /**
-   * Fields whose value is as sensitive as a password but which the agent must
-   * still be able to FILL: one-time codes and payment details.
-   *
-   * Deliberately separate from isSecretField. That one also gates keystrokes
-   * (activeElementSecrecy feeds the driver's press-key guard), so folding these
-   * tokens into it would stop the agent completing a checkout or an OTP prompt —
-   * work it is legitimately asked to do. Only the value is withheld here.
-   */
+  /** Sensitive-but-fillable fields; see collectSnapshot's copy for why. */
   const isSensitiveValueField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
@@ -548,10 +531,7 @@ export function activeElementSecrecy(): string {
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)
@@ -594,24 +574,31 @@ export function activeElementSecrecy(): string {
     // detector is `attachShadow` throwing, which is destructive. Narrowing this
     // needs the driver to stop trusting a page-derived signal, not a better
     // guess here.
+    // Declared inside the function: this is injected as source, so it cannot
+    // reference module scope, but a local is fine and keeps the list diffable.
+    // IFRAME/FRAME are here so the frame branch below, not this early return,
+    // classifies them.
+    const FOCUSABLE_TAGS = [
+      'INPUT',
+      'TEXTAREA',
+      'SELECT',
+      'BUTTON',
+      'A',
+      'AREA',
+      'SUMMARY',
+      'DIALOG',
+      'VIDEO',
+      'AUDIO',
+      'EMBED',
+      'OBJECT',
+      'IFRAME',
+      'FRAME',
+    ]
     const focusableItself =
       active === active.ownerDocument.body ||
       active.isContentEditable ||
       active.hasAttribute('tabindex') ||
-      tag === 'INPUT' ||
-      tag === 'TEXTAREA' ||
-      tag === 'SELECT' ||
-      tag === 'BUTTON' ||
-      tag === 'A' ||
-      tag === 'AREA' ||
-      tag === 'SUMMARY' ||
-      tag === 'DIALOG' ||
-      tag === 'VIDEO' ||
-      tag === 'AUDIO' ||
-      tag === 'EMBED' ||
-      tag === 'OBJECT' ||
-      tag === 'IFRAME' ||
-      tag === 'FRAME'
+      FOCUSABLE_TAGS.indexOf(tag) !== -1
     if (!shadow && !focusableItself) return 'opaque'
     if (tag === 'IFRAME' || tag === 'FRAME') {
       let inner: Document | null = null
@@ -636,10 +623,7 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)
@@ -712,10 +696,7 @@ export function pressKeyOnPage(
   const isSecretField = (node: Element | null): boolean => {
     if (!node || node.tagName !== 'INPUT') return false
     if (String((node as HTMLInputElement).type || '').toLowerCase() === 'password') return true
-    // Space-separated detail tokens are spec-legal and WebAuthn recommends
-    // `current-password webauthn`, so whole-string equality missed real values
-    // on exactly the type=text credential fields where autocomplete is the
-    // only signal there is.
+    // Token membership, not equality — see the module header.
     const hint = String(node.getAttribute('autocomplete') || '').toLowerCase()
     return hint
       .split(/\s+/)

apps/desktop/src/main/browser-agent/url-guard.ts

@@ -19,6 +19,25 @@ export interface UrlGuardResult {
 
 const OK: UrlGuardResult = { ok: true }
 
+/**
+ * A URL's host in the one form every guard here compares against.
+ *
+ * IPv6 brackets are unwrapped so the address classifiers see a bare address,
+ * and a trailing dot is dropped — it is a legal absolute name that resolves the
+ * same, so leaving it on would let `intranet.` and `intranet` be judged and
+ * cached as two different hosts. Null when the URL does not parse or carries no
+ * host, which every caller treats as nothing to block.
+ */
+function guardHost(rawUrl: string): string | null {
+  let hostname: string
+  try {
+    hostname = new URL(rawUrl).hostname
+  } catch {
+    return null
+  }
+  return unwrapIpv6Brackets(hostname).replace(/\.$/, '') || null
+}
+
 /**
  * Whether an address is off limits to the embedded browser.
  *
@@ -66,7 +85,10 @@ export async function checkAgentUrl(rawUrl: string): Promise<UrlGuardResult> {
     return { ok: false, error: 'URL must be absolute and start with http:// or https://' }
   }
 
-  const host = unwrapIpv6Brackets(url.hostname)
+  const host = guardHost(url.href)
+  if (!host) {
+    return { ok: false, error: 'That address has no host to check.' }
+  }
 
   // IP literal: classify directly, no DNS lookup needed.
   if (isIpLiteral(host)) {
@@ -126,7 +148,7 @@ const HOST_VERDICT_TTL_MS = 30_000
 
 /**
  * Ceiling on the cache. A hostile page can name unlimited hostnames, so this is
- * bounded rather than left to grow; the oldest entry is evicted first.
+ * bounded rather than left to grow.
  */
 const MAX_HOST_VERDICTS = 256
 
@@ -187,15 +209,7 @@ export function clearHostVerdictCache(): void {
  * stick.
  */
 export async function isBlockedSubresourceUrl(rawUrl: string): Promise<boolean> {
-  let hostname: string
-  try {
-    hostname = new URL(rawUrl).hostname
-  } catch {
-    return false
-  }
-  // A trailing dot is a legal absolute name that resolves the same, so it is
-  // stripped for the key rather than caching the same host twice.
-  const host = unwrapIpv6Brackets(hostname).replace(/\.$/, '')
+  const host = guardHost(rawUrl)
   if (!host) return false
   if (isIpLiteral(host)) return isBlockedAddress(host)
 
@@ -236,12 +250,7 @@ export async function isBlockedSubresourceUrl(rawUrl: string): Promise<boolean>
  * requests still relying on this alone.
  */
 export function isBlockedRequestUrl(rawUrl: string): boolean {
-  try {
-    // isPrivateIpHost strips IPv6 brackets itself; unwrap again for the
-    // loopback carve-out, which takes a bare address.
-    const host = new URL(rawUrl).hostname
-    return isPrivateIpHost(host) && !isLoopbackIp(unwrapIpv6Brackets(host))
-  } catch {
-    return false
-  }
+  const host = guardHost(rawUrl)
+  if (!host) return false
+  return isPrivateIpHost(host) && !isLoopbackIp(host)
 }

apps/desktop/src/main/browser-credentials/os-auth.ts

@@ -29,24 +29,18 @@ const AUTH_GRACE_MS = 30_000
  * which Universal Clipboard syncs to the user's other devices. Ordering them
  * either way lets one consent authorize an exposure the prompt never described.
  */
-export const SECRET_OPERATIONS = ['reveal', 'copy'] as const
-
-type SecretOperation = (typeof SECRET_OPERATIONS)[number]
+export type SecretOperation = 'reveal' | 'copy'
 
 /**
  * Proof of presence per credential AND operation.
  *
- * Keyed on both rather than holding one scalar per credential: a single slot
- * would be overwritten on each grant, so reveal → copy → reveal prompts three
- * times inside one window even though each was already proven. Re-prompting for
- * something the user just authorized is what teaches people to approve without
- * reading.
+ * Nested rather than one scalar per credential: a single slot would be
+ * overwritten on each grant, so reveal → copy → reveal prompts three times
+ * inside one window even though each was already proven. Nesting also keeps
+ * revoke-by-credential a single delete, so a third operation added later cannot
+ * be left behind by a revoke that forgot to enumerate it.
  */
-const provenUntil = new Map<string, number>()
-
-function grantKey(credentialId: string, operation: SecretOperation): string {
-  return `${credentialId}\u0000${operation}`
-}
+const provenUntil = new Map<string, Map<SecretOperation, number>>()
 
 export interface SecretAuthRequest {
   /**
@@ -70,11 +64,15 @@ export interface SecretAuthRequest {
 }
 
 function hasFreshProof(credentialId: string, operation: SecretOperation): boolean {
-  const key = grantKey(credentialId, operation)
-  const expiry = provenUntil.get(key)
+  const grants = provenUntil.get(credentialId)
+  const expiry = grants?.get(operation)
   if (expiry === undefined) return false
-  if (Date.now() >= expiry) {
-    provenUntil.delete(key)
+  // Also lapsed when the remaining time exceeds the whole window, which is what
+  // a backwards clock step looks like — otherwise a corrected clock would leave
+  // a grant standing far longer than it was granted for.
+  const remaining = expiry - Date.now()
+  if (remaining <= 0 || remaining > AUTH_GRACE_MS) {
+    grants?.delete(operation)
     return false
   }
   return true
@@ -87,14 +85,8 @@ function hasFreshProof(credentialId: string, operation: SecretOperation): boolea
  * Called with no id, it revokes everything.
  */
 export function revokeSecretAuthorization(credentialId?: string): void {
-  if (credentialId === undefined) {
-    provenUntil.clear()
-    return
-  }
-  // Every operation's grant for this credential, since the key carries both.
-  for (const operation of SECRET_OPERATIONS) {
-    provenUntil.delete(grantKey(credentialId, operation))
-  }
+  if (credentialId === undefined) provenUntil.clear()
+  else provenUntil.delete(credentialId)
 }
 
 /**
@@ -124,7 +116,9 @@ export async function authorizeForSecret({
 }: SecretAuthRequest): Promise<boolean> {
   if (hasFreshProof(credentialId, operation)) return true
   if (!(await promptForSecret(reason, action))) return false
-  provenUntil.set(grantKey(credentialId, operation), Date.now() + AUTH_GRACE_MS)
+  const grants = provenUntil.get(credentialId) ?? new Map<SecretOperation, number>()
+  grants.set(operation, Date.now() + AUTH_GRACE_MS)
+  provenUntil.set(credentialId, grants)
   return true
 }
 

apps/desktop/src/main/ipc.ts

@@ -316,14 +316,8 @@ function localFilesystemRequestNeedsToolAuthorization(request: unknown): boolean
 }
 
 /**
- * Whether the caller has a real user gesture behind it.
- *
- * Answered from the main process's own record of OS input, never by asking the
- * renderer. The previous implementation ran
- * `navigator.userActivation?.isActive === true` through
- * `frame.executeJavaScript`, which evaluates in the page's main world — the
- * same world as the compromised page this gate exists to stop, which need only
- * redefine `navigator.userActivation` to make the check always pass.
+ * Whether the caller has a real user gesture behind it, answered from the main
+ * process's own record of OS input rather than by asking the renderer.
  */
 function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean {
   return hasRecentDiscreteInput(event.sender)
@@ -337,8 +331,17 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean
  * mouse reports, and DCS/OSC responses. All machine-generated and
  * self-delimiting, which is what makes them safe to enumerate.
  */
-const PTY_REPLY =
-  /^(?:\u001b\[[0-9;?]*[Rc]|\u001b\[[IO]|\u001b\[M[\s\S]{3}|\u001b\[<[0-9;]*[mM]|\u001bP[\s\S]*?\u001b\\|\u001b\][\s\S]*?\u0007)+$/
+const PTY_REPLY_PATTERNS = [
+  /\u001b\[[0-9;?]*[Rc]/, // DSR cursor position, device attributes
+  /\u001b\[[IO]/, // focus in/out (mode 1004)
+  /\u001b\[M[\s\S]{3}/, // X10 mouse report
+  /\u001b\[<[0-9;]*[mM]/, // SGR mouse report
+  /\u001bP[\s\S]*?\u001b\\/, // DCS response
+  /\u001b\][\s\S]*?\u0007/, // OSC response
+]
+const PTY_REPLY = new RegExp(
+  `^(?:${PTY_REPLY_PATTERNS.map((pattern) => pattern.source).join('|')})+$`
+)
 
 /**
  * Whether a terminal-write payload needs a person behind it.

apps/desktop/src/main/terminal/index.ts

@@ -315,11 +315,11 @@ export class TerminalService {
   private reapFinishedRuns(terminalId: string): void {
     const pending = this.pendingRuns.get(terminalId)
     if (!pending) return
-    const stillRunning = pending.filter((handle) => {
-      if (!isRunComplete(handle)) return true
-      handle.dispose()
-      return false
-    })
+    const stillRunning: TmuxRunHandle[] = []
+    for (const handle of pending) {
+      if (isRunComplete(handle)) handle.dispose()
+      else stillRunning.push(handle)
+    }
     if (stillRunning.length === 0) this.pendingRuns.delete(terminalId)
     else this.pendingRuns.set(terminalId, stillRunning)
   }
@@ -501,7 +501,10 @@ export class TerminalService {
     }
     this.sessions.clear()
     this.tmuxCache.clear()
-    for (const terminalId of [...this.pendingRuns.keys()]) this.releasePendingRuns(terminalId)
+    for (const handles of this.pendingRuns.values()) {
+      for (const handle of handles) handle.dispose()
+    }
+    this.pendingRuns.clear()
     this.activeId = null
     // A stale claim here is what let Cmd-W close a shell that no longer exists.
     this.setPanelFocused(false)