feat(cockpit): approval preview — command/diff above Approve/Reject (W1)

Remote one-tap approval was blind (you'd tap Approve without seeing Claude wants to run `rm -rf` or rewrite a config). The pending tool's actual command / diff now renders above the approval bar, on every attached device, riding the same broadcast + late-joiner rails as the existing `gate` field. - src/http/approval-preview.ts (new, pure, never-throws): deriveApprovalPreview — Bash → command; Edit/Write/MultiEdit/NotebookEdit → a synthetic DiffFile; else null. Every line sanitized via sanitizeField (strips control/ANSI); caps 40 lines / 200 chars/line / 4KB (security limits, UTF-8-safe byte clamp). - src/types.ts: additive optional `preview?: ApprovalPreview` on the status ServerMessage + handleHookEvent (older clients ignore it). - src/server.ts /hook/permission: derive preview from tool_input, store on the PendingApproval entry, re-send to late joiners exactly like `gate`. - manager.ts threads it; public/tabs.ts renderApprovalPreview (command → <pre> textContent; diff → reused innerHTML-free renderDiffFile). Unknown tools / plan gates fall back to today's name-only bar. Attacker-influenced tool input → rendered via textContent/diff-renderer only, never innerHTML. Verified independently: typecheck + build:web clean, 1692 pass.
2026-07-12 20:04:18 +02:00
parent debf47d99e
commit e062065cd3
12 changed files with 808 additions and 8 deletions
--- a/test/http/approval-preview.test.ts
+++ b/test/http/approval-preview.test.ts
@@ -0,0 +1,189 @@
+/**
+ * test/http/approval-preview.test.ts (W1) — deriveApprovalPreview unit tests.
+ *
+ * Pure, never-throws derivation of a bounded, sanitized command/diff preview
+ * from an untrusted hook tool_input. Node env (no DOM).
+ */
+
+import { describe, it, expect } from 'vitest'
+import {
+  deriveApprovalPreview,
+  PREVIEW_MAX_LINES,
+  PREVIEW_MAX_BYTES,
+} from '../../src/http/approval-preview.js'
+import type { ApprovalPreview, DiffFile } from '../../src/types.js'
+
+/** Narrow a preview to its 'diff' variant (throws in-test if wrong shape). */
+function asDiff(p: ApprovalPreview | null): DiffFile {
+  expect(p).not.toBeNull()
+  expect(p!.kind).toBe('diff')
+  return (p as { kind: 'diff'; file: DiffFile }).file
+}
+
+describe('deriveApprovalPreview — Bash → command', () => {
+  it('extracts the command string', () => {
+    const p = deriveApprovalPreview('Bash', { command: 'ls -la', description: 'x' })
+    expect(p).toEqual({ kind: 'command', text: 'ls -la' })
+  })
+
+  it('preserves newlines in a multi-line command', () => {
+    const p = deriveApprovalPreview('Bash', { command: 'a\nb' })
+    expect(p).toMatchObject({ kind: 'command' })
+    expect((p as { text: string }).text).toContain('\n')
+    expect((p as { text: string }).text).toBe('a\nb')
+  })
+
+  it('strips ANSI/control injection (ESC, BEL) from the command', () => {
+    const p = deriveApprovalPreview('Bash', { command: '\x1b[31mrm\x1b[0m\x07' })
+    const text = (p as { text: string }).text
+    expect(text).not.toContain('\x1b')
+    expect(text).not.toContain('\x07')
+    expect(text).toBe('[31mrm[0m') // control bytes gone, printable payload remains
+  })
+
+  it('returns null when command is missing / not a string', () => {
+    expect(deriveApprovalPreview('Bash', { description: 'x' })).toBeNull()
+    expect(deriveApprovalPreview('Bash', { command: 42 })).toBeNull()
+  })
+})
+
+describe('deriveApprovalPreview — Edit → diff', () => {
+  it('builds one hunk with removed + added lines and correct counts', () => {
+    const file = asDiff(
+      deriveApprovalPreview('Edit', {
+        file_path: '/p/f.ts',
+        old_string: 'a\nb',
+        new_string: 'c',
+      }),
+    )
+    expect(file.newPath).toBe('/p/f.ts')
+    expect(file.removed).toBe(2)
+    expect(file.added).toBe(1)
+    expect(file.hunks).toHaveLength(1)
+    const kinds = file.hunks[0]!.lines.map((l) => `${l.kind}:${l.text}`)
+    expect(kinds).toEqual(['removed:a', 'removed:b', 'added:c'])
+  })
+
+  it('sanitizes the file path (strips control chars)', () => {
+    const file = asDiff(
+      deriveApprovalPreview('Edit', {
+        file_path: '/p/\x1b[2Jevil.ts',
+        old_string: 'x',
+        new_string: 'y',
+      }),
+    )
+    expect(file.newPath).not.toContain('\x1b')
+    expect(file.newPath).toBe('/p/[2Jevil.ts')
+  })
+
+  it('returns null for an Edit missing old_string', () => {
+    expect(deriveApprovalPreview('Edit', { file_path: '/p/f', new_string: 'c' })).toBeNull()
+  })
+})
+
+describe('deriveApprovalPreview — Write → all-added diff', () => {
+  it('emits an all-added hunk with removed===0', () => {
+    const file = asDiff(deriveApprovalPreview('Write', { file_path: '/p/f', content: 'x\ny' }))
+    expect(file.removed).toBe(0)
+    expect(file.added).toBe(2)
+    expect(file.status).toBe('added')
+    expect(file.hunks[0]!.lines.map((l) => l.kind)).toEqual(['added', 'added'])
+  })
+
+  it('returns null when content is not a string', () => {
+    expect(deriveApprovalPreview('Write', { file_path: '/p/f' })).toBeNull()
+  })
+})
+
+describe('deriveApprovalPreview — MultiEdit → one hunk per edit', () => {
+  it('produces a hunk for each edit', () => {
+    const file = asDiff(
+      deriveApprovalPreview('MultiEdit', {
+        file_path: '/p/f',
+        edits: [
+          { old_string: 'a', new_string: 'b' },
+          { old_string: 'c', new_string: 'd' },
+        ],
+      }),
+    )
+    expect(file.hunks).toHaveLength(2)
+    expect(file.removed).toBe(2)
+    expect(file.added).toBe(2)
+  })
+
+  it('drops malformed edit entries; null when all are invalid', () => {
+    expect(
+      deriveApprovalPreview('MultiEdit', { file_path: '/p/f', edits: [{ old_string: 'a' }] }),
+    ).toBeNull()
+    const file = asDiff(
+      deriveApprovalPreview('MultiEdit', {
+        file_path: '/p/f',
+        edits: [{ old_string: 'a' }, { old_string: 'x', new_string: 'y' }],
+      }),
+    )
+    expect(file.hunks).toHaveLength(1)
+  })
+
+  it('returns null when edits is not an array', () => {
+    expect(deriveApprovalPreview('MultiEdit', { file_path: '/p/f', edits: 'nope' })).toBeNull()
+  })
+})
+
+describe('deriveApprovalPreview — NotebookEdit', () => {
+  it('previews new_source as an all-added diff', () => {
+    const file = asDiff(
+      deriveApprovalPreview('NotebookEdit', { notebook_path: '/n.ipynb', new_source: 'print(1)' }),
+    )
+    expect(file.newPath).toBe('/n.ipynb')
+    expect(file.added).toBe(1)
+    expect(file.removed).toBe(0)
+  })
+})
+
+describe('deriveApprovalPreview — truncation (DoS containment)', () => {
+  it('caps line count and flags truncated for a huge command', () => {
+    const command = Array.from({ length: PREVIEW_MAX_LINES + 20 }, (_, i) => `line${i}`).join('\n')
+    const p = deriveApprovalPreview('Bash', { command })
+    expect(p).toMatchObject({ kind: 'command', truncated: true })
+    const text = (p as { text: string }).text
+    expect(text.split('\n').length).toBeLessThanOrEqual(PREVIEW_MAX_LINES)
+  })
+
+  it('caps total bytes for a huge single-line blob', () => {
+    const command = 'x'.repeat(PREVIEW_MAX_BYTES * 4)
+    const p = deriveApprovalPreview('Bash', { command })
+    expect(p).toMatchObject({ truncated: true })
+    const text = (p as { text: string }).text
+    expect(Buffer.byteLength(text, 'utf8')).toBeLessThanOrEqual(PREVIEW_MAX_BYTES)
+  })
+
+  it('caps a giant Write and keeps the serialized diff within the byte cap', () => {
+    const content = Array.from({ length: 500 }, (_, i) => `row-${i}-`.repeat(50)).join('\n')
+    const p = deriveApprovalPreview('Write', { file_path: '/p/f', content })
+    expect(p).toMatchObject({ kind: 'diff', truncated: true })
+    const file = asDiff(p)
+    const emitted = file.hunks.flatMap((h) => h.lines)
+    expect(emitted.length).toBeLessThanOrEqual(PREVIEW_MAX_LINES)
+    const bytes = emitted.reduce((n, l) => n + Buffer.byteLength(l.text, 'utf8'), 0)
+    expect(bytes).toBeLessThanOrEqual(PREVIEW_MAX_BYTES)
+  })
+})
+
+describe('deriveApprovalPreview — unknown tools & malformed input (never throws)', () => {
+  it('returns null for a non-previewable tool', () => {
+    expect(deriveApprovalPreview('WebFetch', { url: 'http://x' })).toBeNull()
+    expect(deriveApprovalPreview('ExitPlanMode', {})).toBeNull()
+  })
+
+  it('returns null for undefined tool name', () => {
+    expect(deriveApprovalPreview(undefined, { command: 'ls' })).toBeNull()
+  })
+
+  it('returns null (never throws) for null / array / number / string tool_input', () => {
+    expect(() => deriveApprovalPreview('Bash', null)).not.toThrow()
+    expect(deriveApprovalPreview('Bash', null)).toBeNull()
+    expect(deriveApprovalPreview('Bash', [1, 2])).toBeNull()
+    expect(deriveApprovalPreview('Edit', 42)).toBeNull()
+    expect(deriveApprovalPreview('Bash', 'ls -la')).toBeNull()
+  })
+})
--- a/test/integration/server.test.ts
+++ b/test/integration/server.test.ts
@@ -712,6 +712,74 @@ describe('startServer — integration', () => {
    },
  )

+  // ── W1: /hook/permission carries a bounded preview + re-sends to late joiners ─
+  itPty(
+    'W1 [needs real PTY (sandbox-off)] /hook/permission broadcasts a preview and re-sends it to a late joiner',
+    async () => {
+      const ws1 = new WebSocket(`ws://127.0.0.1:${port}${cfg.wsPath}`, {
+        headers: { Origin: `http://127.0.0.1:${port}` },
+      })
+      await waitForOpen(ws1, 3_000)
+      ws1.send(JSON.stringify({ type: 'attach', sessionId: null }))
+      const attached = (await waitForMessage(
+        ws1,
+        (m) =>
+          typeof m === 'object' && m !== null && (m as Record<string, unknown>)['type'] === 'attached',
+        5_000,
+      )) as Record<string, unknown>
+      const sessionId = attached['sessionId'] as string
+
+      // Arm the pending-status listener on ws1 before firing (push is synchronous).
+      const pendingPromise = waitForMessage(
+        ws1,
+        (m) =>
+          typeof m === 'object' && m !== null && (m as Record<string, unknown>)['pending'] === true,
+        5_000,
+      )
+
+      // Fire a Bash PermissionRequest carrying tool_input — HELD until approve.
+      const permPromise = fetch(`http://127.0.0.1:${port}/hook/permission`, {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json', 'X-Webterm-Session': sessionId },
+        body: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'echo hi' } }),
+      })
+
+      const pending = (await pendingPromise) as Record<string, unknown>
+      expect(pending['status']).toBe('waiting')
+      const preview = pending['preview'] as Record<string, unknown> | undefined
+      expect(preview).toBeDefined()
+      expect(preview!['kind']).toBe('command')
+      expect(String(preview!['text'])).toContain('echo hi')
+
+      // Late joiner: a SECOND ws attaching to the same session while the approval
+      // is held must receive the preview on attach (re-sent like `gate`).
+      const ws2 = new WebSocket(`ws://127.0.0.1:${port}${cfg.wsPath}`, {
+        headers: { Origin: `http://127.0.0.1:${port}` },
+      })
+      await waitForOpen(ws2, 3_000)
+      const joinPending = waitForMessage(
+        ws2,
+        (m) =>
+          typeof m === 'object' && m !== null && (m as Record<string, unknown>)['pending'] === true,
+        5_000,
+      )
+      ws2.send(JSON.stringify({ type: 'attach', sessionId }))
+      const joined = (await joinPending) as Record<string, unknown>
+      const joinedPreview = joined['preview'] as Record<string, unknown> | undefined
+      expect(joinedPreview).toBeDefined()
+      expect(joinedPreview!['kind']).toBe('command')
+      expect(String(joinedPreview!['text'])).toContain('echo hi')
+
+      // Approve over ws1 to release the held POST, then clean up.
+      ws1.send(JSON.stringify({ type: 'approve' }))
+      await permPromise.then((r) => r.json()).catch(() => undefined)
+      ws1.close()
+      ws2.close()
+      await waitForClose(ws1, 3_000).catch(() => undefined)
+      await waitForClose(ws2, 3_000).catch(() => undefined)
+    },
+  )
+
  // ── H1: tmux keepalive — a shell survives a full server restart ────────────
  itTmux(
    'H1 [needs tmux + real PTY] shell state survives a server restart',
--- a/test/manager.test.ts
+++ b/test/manager.test.ts
@@ -799,6 +799,39 @@ describe('handleHookEvent — gate broadcast (B4)', () => {
  });
 });

+// ── handleHookEvent — W1 preview broadcast ────────────────────────────────────
+describe('handleHookEvent — preview broadcast (W1)', () => {
+  it('puts the preview arg on the broadcast status frame (deep equal)', () => {
+    const mgr = createSessionManager(CFG);
+    const a = createMockWs();
+    const b = createMockWs();
+    const s = mgr.handleAttach(a, null, DIMS, 1_000);
+    mgr.handleAttach(b, s.meta.id, DIMS, 2_000);
+    a.sent.length = 0;
+    b.sent.length = 0;
+
+    const preview = { kind: 'command', text: 'ls -la' } as const;
+    mgr.handleHookEvent(s.meta.id, 'waiting', 'Bash', true, 'tool', undefined, undefined, preview);
+
+    for (const ws of [a, b]) {
+      const status = parseSent(ws).find((m) => m.type === 'status');
+      expect(status.preview).toEqual(preview);
+    }
+  });
+
+  it('omits the preview key when no preview is supplied', () => {
+    const mgr = createSessionManager(CFG);
+    const ws = createMockWs();
+    const s = mgr.handleAttach(ws, null, DIMS, 1_000);
+    ws.sent.length = 0;
+
+    mgr.handleHookEvent(s.meta.id, 'waiting', 'Bash', true, 'tool');
+
+    const status = parseSent(ws).find((m) => m.type === 'status');
+    expect(status).not.toHaveProperty('preview');
+  });
+});
+
 // ── handleStatusLine (B2 telemetry) ───────────────────────────────────────────
 describe('handleStatusLine', () => {
  it('stores telemetry on the session and broadcasts it to all clients', () => {
--- a/test/tabs.test.ts
+++ b/test/tabs.test.ts
@@ -9,6 +9,7 @@
 */

 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+import type { ApprovalPreview } from '../src/types.js'

 // ── Mock TerminalSession ──────────────────────────────────────────────────────
 let constructed = 0
@@ -22,6 +23,8 @@ class FakeTerminalSession {
  pendingTool: string | undefined = undefined
  /** B4: which gate the held approval is — 'tool' (two buttons) or 'plan' (three). */
  pendingGate: 'tool' | 'plan' | null = null
+  /** W1: bounded command/diff preview of the held tool (or null). */
+  pendingPreview: ApprovalPreview | null = null
  /** VC: nonce that counts false→true pendingApproval flips (stale-gate guard). */
  pendingEpoch = 0
  /** B2: latest statusLine telemetry (single source of truth). */
@@ -693,6 +696,75 @@ describe('TabApp — B4 plan-gate approval bar', () => {
  })
 })

+describe('TabApp — W1 approval preview on the bar', () => {
+  /** Open one tab, hold a tool-gate approval carrying `preview`, render the bar. */
+  function openWithPreview(preview: ApprovalPreview | null, tool = 'Bash'): FakeTerminalSession {
+    const { paneHost, tabBar } = makeHosts()
+    const app = new TabApp(paneHost, tabBar)
+    app.newTab()
+    const session = FakeTerminalSession.instances.at(-1)!
+    session.pendingApproval = true
+    session.pendingGate = 'tool'
+    session.pendingTool = tool
+    session.pendingPreview = preview
+    session.cbs.onClaudeStatus?.('waiting') // triggers updateApprovalBar
+    return session
+  }
+
+  it('renders a command preview as a .approval-cmd whose textContent is the command', () => {
+    openWithPreview({ kind: 'command', text: 'rm -rf /tmp/x' })
+    const bar = document.getElementById('approvalbar')!
+    const cmd = bar.querySelector('.approval-cmd')
+    expect(cmd).not.toBeNull()
+    expect(cmd!.textContent).toBe('rm -rf /tmp/x')
+    // Preview sits before the buttons; buttons still present (two for a tool gate).
+    expect(bar.querySelectorAll('button')).toHaveLength(2)
+  })
+
+  it('renders a diff preview via renderDiffFile (.df-file present, path shown)', () => {
+    const preview: ApprovalPreview = {
+      kind: 'diff',
+      file: {
+        oldPath: '/p/f.ts',
+        newPath: '/p/f.ts',
+        status: 'modified',
+        added: 1,
+        removed: 1,
+        binary: false,
+        hunks: [{ header: '', lines: [{ kind: 'removed', text: 'a' }, { kind: 'added', text: 'b' }] }],
+      },
+    }
+    openWithPreview(preview)
+    const bar = document.getElementById('approvalbar')!
+    expect(bar.querySelector('.approval-preview .df-file')).not.toBeNull()
+    expect(bar.querySelector('.df-path')!.textContent).toBe('/p/f.ts')
+  })
+
+  it('shows the "… truncated" note when preview.truncated is set', () => {
+    openWithPreview({ kind: 'command', text: 'x', truncated: true })
+    const bar = document.getElementById('approvalbar')!
+    expect(bar.querySelector('.approval-truncated')).not.toBeNull()
+  })
+
+  it('no preview → today\'s name-only bar (regression: label + 2 buttons, no preview)', () => {
+    openWithPreview(null, 'Bash')
+    const bar = document.getElementById('approvalbar')!
+    expect(bar.style.display).toBe('flex')
+    expect(bar.querySelector('.approval-preview')).toBeNull()
+    expect(bar.querySelectorAll('button')).toHaveLength(2)
+    expect(bar.querySelector('.approval-label')!.textContent).toBe('Claude wants to use Bash')
+  })
+
+  it('renders attacker-influenced content via textContent only (no HTML injection)', () => {
+    openWithPreview({ kind: 'command', text: '<img src=x onerror=alert(1)>' })
+    const bar = document.getElementById('approvalbar')!
+    const cmd = bar.querySelector('.approval-cmd')!
+    expect(cmd.textContent).toBe('<img src=x onerror=alert(1)>') // literal text
+    expect(cmd.querySelector('img')).toBeNull() // NOT parsed into an element
+    expect(bar.querySelector('img')).toBeNull()
+  })
+})
+
 describe('TabApp — B4 permission-mode relay', () => {
  it('openProject with a non-default mode upgrades the launch command (AC-B4.1)', () => {
    const { paneHost, tabBar } = makeHosts()
--- a/test/terminal-session.test.ts
+++ b/test/terminal-session.test.ts
@@ -220,6 +220,59 @@ describe('status frame (H3 pending approval)', () => {
  })
 })

+describe('W1 approval preview on the status frame', () => {
+  beforeEach(() => setLocation('http:', 'lan:3000'))
+
+  function openSession(): { s: InstanceType<typeof TerminalSession>; ws: ReturnType<typeof MockWebSocket.last> } {
+    const s = new TerminalSession({ sessionId: null, onSessionId: vi.fn() })
+    s.connect()
+    const ws = MockWebSocket.last()
+    ws.openIt()
+    return { s, ws }
+  }
+
+  it('sets pendingPreview from a pending status frame', () => {
+    const { s, ws } = openSession()
+    ws.message(
+      JSON.stringify({
+        type: 'status',
+        status: 'waiting',
+        detail: 'Bash',
+        pending: true,
+        gate: 'tool',
+        preview: { kind: 'command', text: 'ls -la' },
+      }),
+    )
+    expect(s.pendingPreview).toEqual({ kind: 'command', text: 'ls -la' })
+  })
+
+  it('clears pendingPreview when a follow-up status is not pending', () => {
+    const { s, ws } = openSession()
+    ws.message(
+      JSON.stringify({ type: 'status', status: 'waiting', pending: true, preview: { kind: 'command', text: 'ls' } }),
+    )
+    expect(s.pendingPreview).not.toBeNull()
+    ws.message(JSON.stringify({ type: 'status', status: 'working', pending: false }))
+    expect(s.pendingPreview).toBeNull()
+  })
+
+  it('leaves pendingPreview null when a pending status carries no preview', () => {
+    const { s, ws } = openSession()
+    ws.message(JSON.stringify({ type: 'status', status: 'waiting', detail: 'WebFetch', pending: true, gate: 'tool' }))
+    expect(s.pendingApproval).toBe(true)
+    expect(s.pendingPreview).toBeNull()
+  })
+
+  it('approve() clears the held preview locally (resolved)', () => {
+    const { s, ws } = openSession()
+    ws.message(
+      JSON.stringify({ type: 'status', status: 'waiting', pending: true, preview: { kind: 'command', text: 'ls' } }),
+    )
+    s.approve()
+    expect(s.pendingPreview).toBeNull()
+  })
+})
+
 describe('reconnect backoff', () => {
  beforeEach(() => {
    setLocation('http:', 'lan:3000')