feat(diff): diff a whole branch vs a base (?base=<rev>) — review before landing (W3)

The git-diff viewer can now diff the current branch against a base commit-ish (e.g. main) — review an agent's whole branch from your phone before merging, not just uncommitted changes. Completes the long-deferred FR-B1.9. - src/http/diff.ts: getDiff() gains an optional base. Three-layer defense so an attacker-supplied base never reaches a shell or acts as a git option: (1) isPlausibleRev() rejects leading '-', '..'/'...' ranges, metachars, control chars, >250 chars; (2) git rev-parse --verify --quiet --end-of-options <base>^{commit} — only a resolved 7-64 hex sha is accepted, else empty result; (3) git diff --no-color <sha>... -- (three-dot = the branch's changes since divergence, PR-style). execFile, no shell, timeout + maxBuffer bound. - src/types.ts: additive optional base on DiffResult. - src/server.ts GET /projects/diff reads ?base (400 on !isPlausibleRev); read-only. - public/diff.ts: a "compare base" <select> (Working tree + one option per base), disables Working/Staged tabs in base mode. public/projects.ts derives bases from the worktree branches ∪ current branch. Verified: typecheck + build:web clean, 1763 pass (diff tests 87 green). The 1 red in the plain full run is the pre-existing real-PTY "ring buffer" flake (times out at default 5s under sandbox load; 27/27 at --test-timeout=30000) — unrelated.
2026-07-12 20:43:32 +02:00
parent 3076843e9c
commit b119c31019
9 changed files with 523 additions and 25 deletions
--- a/test/diff.test.ts
+++ b/test/diff.test.ts
@@ -129,6 +129,24 @@ describe('normalizeDiffResult', () => {
    const result = normalizeDiffResult({ files: [], staged: false, truncated: true })
    expect(result?.truncated).toBe(true)
  })
+
+  it('passes through an optional base revision (FR-B1.9)', () => {
+    const result = normalizeDiffResult({ files: [], staged: false, truncated: false, base: 'main' })
+    expect(result).not.toBeNull()
+    expect(result?.base).toBe('main')
+  })
+
+  it('leaves base undefined when the payload omits it', () => {
+    const result = normalizeDiffResult({ files: [], staged: false, truncated: false })
+    expect(result).not.toBeNull()
+    expect(result?.base).toBeUndefined()
+  })
+
+  it('ignores a non-string base (coerces to undefined)', () => {
+    const result = normalizeDiffResult({ files: [], staged: false, truncated: false, base: 123 })
+    expect(result).not.toBeNull()
+    expect(result?.base).toBeUndefined()
+  })
 })

 // ── fetchDiff ─────────────────────────────────────────────────────────────────
@@ -141,7 +159,7 @@ describe('fetchDiff', () => {
    }))
    vi.stubGlobal('fetch', mockFetch)

-    const result = await fetchDiff('/some/repo', false)
+    const result = await fetchDiff('/some/repo', { staged: false })
    expect(mockFetch).toHaveBeenCalledOnce()
    const url = mockFetch.mock.calls[0]?.[0] as string
    expect(url).toContain('/projects/diff')
@@ -157,20 +175,52 @@ describe('fetchDiff', () => {
    }))
    vi.stubGlobal('fetch', mockFetch)

-    await fetchDiff('/repo', true)
+    await fetchDiff('/repo', { staged: true })
    const url = mockFetch.mock.calls[0]?.[0] as string
    expect(url).toContain('staged=true')
  })

+  it('defaults to staged=false when called with no options', async () => {
+    const mockFetch = vi.fn(async () => ({ ok: true, json: async () => makeResult() }))
+    vi.stubGlobal('fetch', mockFetch)
+
+    await fetchDiff('/repo')
+    const url = mockFetch.mock.calls[0]?.[0] as string
+    expect(url).toContain('staged=false')
+  })
+
+  it('builds a &base=<rev> URL (omitting staged) when base is set (FR-B1.9)', async () => {
+    const mockFetch = vi.fn(async () => ({
+      ok: true,
+      json: async () => makeResult({ base: 'main' }),
+    }))
+    vi.stubGlobal('fetch', mockFetch)
+
+    const result = await fetchDiff('/repo', { base: 'main' })
+    const url = mockFetch.mock.calls[0]?.[0] as string
+    expect(url).toContain('base=main')
+    expect(url).not.toContain('staged=')
+    expect(result?.base).toBe('main')
+  })
+
+  it('URL-encodes a base containing slashes (feature/x)', async () => {
+    const mockFetch = vi.fn(async () => ({ ok: true, json: async () => makeResult() }))
+    vi.stubGlobal('fetch', mockFetch)
+
+    await fetchDiff('/repo', { base: 'feature/x' })
+    const url = mockFetch.mock.calls[0]?.[0] as string
+    expect(url).toContain(`base=${encodeURIComponent('feature/x')}`)
+  })
+
  it('returns null on non-ok response', async () => {
    vi.stubGlobal('fetch', vi.fn(async () => ({ ok: false, json: async () => ({}) })))
-    const result = await fetchDiff('/repo', false)
+    const result = await fetchDiff('/repo', { staged: false })
    expect(result).toBeNull()
  })

  it('returns null on fetch error (network failure)', async () => {
    vi.stubGlobal('fetch', vi.fn(async () => { throw new Error('net') }))
-    const result = await fetchDiff('/repo', false)
+    const result = await fetchDiff('/repo', { staged: false })
    expect(result).toBeNull()
  })

@@ -179,7 +229,7 @@ describe('fetchDiff', () => {
      ok: true,
      json: async () => ({ not: 'a diff result' }),
    })))
-    const result = await fetchDiff('/repo', false)
+    const result = await fetchDiff('/repo', { staged: false })
    expect(result).toBeNull()
  })
 })
@@ -480,4 +530,88 @@ describe('mountDiffViewer', () => {
    await expect(new Promise((r) => setTimeout(r, 0))).resolves.toBeUndefined()
    handle.destroy()
  })
+
+  // ── FR-B1.9: base-branch picker ─────────────────────────────────────────────
+
+  it('renders NO base picker when bases is absent (backward-compatible)', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => makeResult() })))
+    const container = makeContainer()
+    const handle = mountDiffViewer(container, '/repo', {})
+    await new Promise((r) => setTimeout(r, 0))
+    expect(container.querySelector('.df-base-select')).toBeNull()
+    handle.destroy()
+  })
+
+  it('renders NO base picker when bases is empty', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => makeResult() })))
+    const container = makeContainer()
+    const handle = mountDiffViewer(container, '/repo', { bases: [] })
+    await new Promise((r) => setTimeout(r, 0))
+    expect(container.querySelector('.df-base-select')).toBeNull()
+    handle.destroy()
+  })
+
+  it('renders a <select> with "Working tree" + one option per base', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => makeResult() })))
+    const container = makeContainer()
+    const handle = mountDiffViewer(container, '/repo', { bases: ['main', 'dev'] })
+    await new Promise((r) => setTimeout(r, 0))
+
+    const select = container.querySelector('.df-base-select') as HTMLSelectElement | null
+    expect(select).not.toBeNull()
+    const labels = Array.from(select!.options).map((o) => o.textContent)
+    expect(labels).toEqual(['Working tree', 'main', 'dev'])
+    handle.destroy()
+  })
+
+  it('selecting a base fetches with base=<rev> and disables the Working/Staged tabs', async () => {
+    const mockFetch = vi.fn(async () => ({ ok: true, json: async () => makeResult({ base: 'main' }) }))
+    vi.stubGlobal('fetch', mockFetch)
+
+    const container = makeContainer()
+    const handle = mountDiffViewer(container, '/repo', { bases: ['main', 'dev'] })
+    await new Promise((r) => setTimeout(r, 0))
+
+    const select = container.querySelector('.df-base-select') as HTMLSelectElement
+    select.value = 'main'
+    select.dispatchEvent(new Event('change'))
+    await new Promise((r) => setTimeout(r, 0))
+
+    const lastUrl = mockFetch.mock.calls.at(-1)?.[0] as string
+    expect(lastUrl).toContain('base=main')
+    expect(lastUrl).not.toContain('staged=')
+
+    const workingBtn = container.querySelector('.df-tab') as HTMLButtonElement
+    const stagedBtn = container.querySelectorAll('.df-tab')[1] as HTMLButtonElement
+    expect(workingBtn.disabled).toBe(true)
+    expect(stagedBtn.disabled).toBe(true)
+    handle.destroy()
+  })
+
+  it('selecting "Working tree" restores a staged-mode fetch and re-enables tabs', async () => {
+    const mockFetch = vi.fn(async () => ({ ok: true, json: async () => makeResult() }))
+    vi.stubGlobal('fetch', mockFetch)
+
+    const container = makeContainer()
+    const handle = mountDiffViewer(container, '/repo', { bases: ['main'] })
+    await new Promise((r) => setTimeout(r, 0))
+
+    const select = container.querySelector('.df-base-select') as HTMLSelectElement
+    // Enter base mode …
+    select.value = 'main'
+    select.dispatchEvent(new Event('change'))
+    await new Promise((r) => setTimeout(r, 0))
+    // … then back to Working tree.
+    select.value = ''
+    select.dispatchEvent(new Event('change'))
+    await new Promise((r) => setTimeout(r, 0))
+
+    const lastUrl = mockFetch.mock.calls.at(-1)?.[0] as string
+    expect(lastUrl).toContain('staged=false')
+    expect(lastUrl).not.toContain('base=')
+
+    const workingBtn = container.querySelector('.df-tab') as HTMLButtonElement
+    expect(workingBtn.disabled).toBe(false)
+    handle.destroy()
+  })
 })
--- a/test/http/diff.test.ts
+++ b/test/http/diff.test.ts
@@ -18,6 +18,7 @@ import {
  parseNumstat,
  parseUnifiedDiff,
  getDiff,
+  isPlausibleRev,
  type NumstatEntry,
  type GetDiffOptions,
 } from '../../src/http/diff.js'
@@ -30,6 +31,44 @@ const LIMITS: GetDiffOptions['cfg'] = {
  diffMaxFiles: 300,
 }

+// ── isPlausibleRev (FR-B1.9 syntactic allow-list) ────────────────────────────
+
+describe('isPlausibleRev', () => {
+  it('accepts ordinary commit-ish forms', () => {
+    for (const ok of ['main', 'feature/x', 'HEAD~3', 'v1.2.0', 'main^', 'HEAD@{1}',
+      '0123456789abcdef0123456789abcdef01234567']) {
+      expect(isPlausibleRev(ok)).toBe(true)
+    }
+  })
+
+  it('rejects the empty string', () => {
+    expect(isPlausibleRev('')).toBe(false)
+  })
+
+  it('rejects an over-long revision (>250 chars)', () => {
+    expect(isPlausibleRev('a'.repeat(300))).toBe(false)
+    expect(isPlausibleRev('a'.repeat(250))).toBe(true) // boundary
+  })
+
+  it('rejects flag/option injection (leading hyphen)', () => {
+    expect(isPlausibleRev('-rf')).toBe(false)
+    expect(isPlausibleRev('--output=/etc/passwd')).toBe(false)
+    expect(isPlausibleRev('--upload-pack=touch /tmp/pwn')).toBe(false)
+  })
+
+  it('rejects range syntax (.. and ...)', () => {
+    expect(isPlausibleRev('a..b')).toBe(false)
+    expect(isPlausibleRev('a...b')).toBe(false)
+  })
+
+  it('rejects whitespace and shell metacharacters', () => {
+    for (const bad of ['x y', 'a\tb', '`id`', '$(x)', 'a;b', 'a|b', 'a&b', 'a>b', "a'b", 'a"b',
+      'main; rm -rf /', 'a\x00b']) {
+      expect(isPlausibleRev(bad)).toBe(false)
+    }
+  })
+})
+
 // ── parseNumstat ──────────────────────────────────────────────────────────────

 describe('parseNumstat', () => {
@@ -346,3 +385,74 @@ describe('getDiff (real git repo)', () => {
    expect(result.truncated).toBe(false)
  })
 })
+
+// ── getDiff against a base revision (FR-B1.9, real git repo) ──────────────────
+
+describe('getDiff against a base revision (FR-B1.9)', () => {
+  let repo: string
+
+  beforeAll(async () => {
+    repo = await fs.mkdtemp(path.join(os.tmpdir(), 'webterm-basediff-'))
+    await git(repo, 'init', '-q', '-b', 'main')
+    await git(repo, 'config', 'user.email', 'test@example.com')
+    await git(repo, 'config', 'user.name', 'Test')
+    await git(repo, 'config', 'commit.gpgsign', 'false')
+    await fs.writeFile(path.join(repo, 'app.txt'), 'base one\nbase two\n')
+    await git(repo, 'add', '.')
+    await git(repo, 'commit', '-q', '-m', 'base commit on main')
+    // Branch off and add a feature-only change (committed on `feature`, not main).
+    await git(repo, 'checkout', '-q', '-b', 'feature')
+    await fs.writeFile(path.join(repo, 'feature.txt'), 'brand new feature file\n')
+    await git(repo, 'add', '.')
+    await git(repo, 'commit', '-q', '-m', 'feature work')
+  })
+
+  afterAll(async () => {
+    await fs.rm(repo, { recursive: true, force: true })
+  })
+
+  it('diffs the whole branch against main (three-dot), echoing base', async () => {
+    const result = await getDiff(repo, { staged: false, base: 'main', cfg: LIMITS })
+    expect(result.base).toBe('main')
+    expect(result.staged).toBe(false)
+    expect(result.truncated).toBe(false)
+    const feat = result.files.find((f) => f.newPath === 'feature.txt')
+    expect(feat).toBeDefined()
+    expect(feat?.status).toBe('added')
+    // numstat-consistent counts for the committed feature file
+    expect(feat?.added).toBe(1)
+    expect(feat?.removed).toBe(0)
+    // A base diff never lists untracked entries.
+    expect(result.files.every((f) => f.status !== 'untracked')).toBe(true)
+  })
+
+  it('does NOT include working-tree-only untracked files in a base diff', async () => {
+    await fs.writeFile(path.join(repo, 'scratch.txt'), 'not committed\n')
+    const result = await getDiff(repo, { staged: false, base: 'main', cfg: LIMITS })
+    expect(result.files.find((f) => f.newPath === 'scratch.txt')).toBeUndefined()
+    await fs.rm(path.join(repo, 'scratch.txt'))
+  })
+
+  it('returns an empty diff when HEAD equals the base (no branch changes)', async () => {
+    const result = await getDiff(repo, { staged: false, base: 'feature', cfg: LIMITS })
+    expect(result.base).toBe('feature')
+    expect(result.files).toEqual([])
+    expect(result.truncated).toBe(false)
+  })
+
+  it('returns an empty result for an unknown/unresolvable base (rev-parse miss)', async () => {
+    const result = await getDiff(repo, { staged: false, base: 'no-such-branch', cfg: LIMITS })
+    expect(result.base).toBe('no-such-branch')
+    expect(result.files).toEqual([])
+    expect(result.truncated).toBe(false)
+  })
+
+  it('never runs a diff for an implausible base (defense-in-depth, no throw)', async () => {
+    // The route rejects this with 400 before getDiff; getDiff must still be safe.
+    for (const bad of ['-x', '--output=/tmp/x', 'main; rm -rf /', 'a..b']) {
+      const result = await getDiff(repo, { staged: false, base: bad, cfg: LIMITS })
+      expect(result.files).toEqual([])
+      expect(result.base).toBe(bad)
+    }
+  })
+})
--- a/test/integration/worktree.test.ts
+++ b/test/integration/worktree.test.ts
@@ -87,6 +87,28 @@ async function makeRealRepo(): Promise<string> {
  return dir
 }

+/** Temp git repo on `main` with a `feature` branch that adds one committed file
+ *  containing a would-be XSS payload. Returns its absolute path. */
+async function makeTwoBranchRepo(): Promise<string> {
+  const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'webterm-base-'))
+  tmpDirs.push(dir)
+  const git = (args: string[]): void => {
+    execFileSync('git', args, { cwd: dir, stdio: 'ignore' })
+  }
+  git(['init', '-b', 'main'])
+  git(['config', 'user.email', 'test@localhost'])
+  git(['config', 'user.name', 'Test'])
+  git(['config', 'commit.gpgsign', 'false'])
+  await fs.writeFile(path.join(dir, 'base.txt'), 'base\n', 'utf8')
+  git(['add', '.'])
+  git(['commit', '-m', 'base on main'])
+  git(['checkout', '-b', 'feature'])
+  await fs.writeFile(path.join(dir, 'feat.txt'), '<script>x</script>\n', 'utf8')
+  git(['add', '.'])
+  git(['commit', '-m', 'feature work'])
+  return dir
+}
+
 afterEach(async () => {
  while (handles.length > 0) await handles.pop()?.close()
  for (const d of tmpDirs.splice(0)) await fs.rm(d, { recursive: true, force: true }).catch(() => undefined)
@@ -200,4 +222,54 @@ describe('GET /projects/diff (B1)', () => {
    // The diff carries the raw text verbatim (the FE renders it inert via textContent).
    expect(flat).toContain('<script>x</script>')
  })
+
+  // ── FR-B1.9: ?base=<rev> whole-branch diff ─────────────────────────────────
+
+  itGit('diffs the whole branch against a base (?base=main), echoing base', async () => {
+    const repo = await makeTwoBranchRepo() // HEAD is on `feature`
+    const { port } = await spawnServer()
+
+    const res = await fetch(
+      `http://127.0.0.1:${port}/projects/diff?path=${encodeURIComponent(repo)}&base=main`,
+    )
+    expect(res.status).toBe(200)
+    const diff = (await res.json()) as DiffResult
+    expect(diff.base).toBe('main')
+    expect(diff.staged).toBe(false)
+    const feat = diff.files.find((f) => f.newPath === 'feat.txt')
+    expect(feat).toBeDefined()
+    expect(feat?.status).toBe('added')
+    expect(JSON.stringify(diff)).toContain('<script>x</script>')
+  })
+
+  itGit('rejects a flag-injection base with 400 (?base=-rf)', async () => {
+    const repo = await makeTwoBranchRepo()
+    const { port } = await spawnServer()
+    const res = await fetch(
+      `http://127.0.0.1:${port}/projects/diff?path=${encodeURIComponent(repo)}&base=-rf`,
+    )
+    expect(res.status).toBe(400)
+  })
+
+  itGit('rejects a metacharacter base with 400 (?base=main;rm)', async () => {
+    const repo = await makeTwoBranchRepo()
+    const { port } = await spawnServer()
+    const res = await fetch(
+      `http://127.0.0.1:${port}/projects/diff?path=${encodeURIComponent(repo)}` +
+        `&base=${encodeURIComponent('main; rm -rf /')}`,
+    )
+    expect(res.status).toBe(400)
+  })
+
+  itGit('returns 200 with an empty diff for an unknown base (?base=ghost-branch)', async () => {
+    const repo = await makeTwoBranchRepo()
+    const { port } = await spawnServer()
+    const res = await fetch(
+      `http://127.0.0.1:${port}/projects/diff?path=${encodeURIComponent(repo)}&base=ghost-branch`,
+    )
+    expect(res.status).toBe(200)
+    const diff = (await res.json()) as DiffResult
+    expect(diff.base).toBe('ghost-branch')
+    expect(diff.files).toEqual([])
+  })
 })
--- a/test/worktree-form.test.ts
+++ b/test/worktree-form.test.ts
@@ -411,6 +411,29 @@ describe('renderProjectDetail — B1 View Diff', () => {
    )
  })

+  it('passes the repo worktree branches + current branch as diff bases (FR-B1.9)', () => {
+    const detail = makeDetail({
+      path: '/home/user/proj',
+      isGit: true,
+      branch: 'main',
+      worktrees: [
+        { path: '/home/user/proj', branch: 'main', isMain: true, isCurrent: true },
+        { path: '/home/user/proj-wt/feat', branch: 'feat', isMain: false, isCurrent: false },
+      ],
+    })
+    const root = renderProjectDetail(detail, makeHooks(), makeCbs())
+    ;(root.querySelector('.proj-diff-toggle') as HTMLButtonElement).click()
+
+    expect(mockMountDiffViewer).toHaveBeenCalledWith(
+      expect.any(HTMLElement),
+      '/home/user/proj',
+      expect.objectContaining({ bases: expect.arrayContaining(['main', 'feat']) }),
+    )
+    // Deduped: `main` appears in both the current branch and a worktree → once.
+    const opts = mockMountDiffViewer.mock.calls[0]?.[2] as { bases: string[] }
+    expect(opts.bases.filter((b) => b === 'main')).toHaveLength(1)
+  })
+
  it('clicking "View Diff" shows the diff panel', () => {
    const root = renderProjectDetail(makeDetail({ isGit: true }), makeHooks(), makeCbs())
    const panel = root.querySelector('.proj-diff-panel') as HTMLElement