From b119c31019c1fc436832ce7fd74292570a440975 Mon Sep 17 00:00:00 2001 From: Yaojia Wang Date: Sun, 12 Jul 2026 20:43:32 +0200 Subject: [PATCH] =?UTF-8?q?feat(diff):=20diff=20a=20whole=20branch=20vs=20?= =?UTF-8?q?a=20base=20(=3Fbase=3D)=20=E2=80=94=20review=20before=20la?= =?UTF-8?q?nding=20(W3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ^{commit} — only a resolved 7-64 hex sha is accepted, else empty result; (3) git diff --no-color ... -- (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" 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= 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() + }) }) diff --git a/test/http/diff.test.ts b/test/http/diff.test.ts index 3ad9112..22bf335 100644 --- 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) + } + }) +}) diff --git a/test/integration/worktree.test.ts b/test/integration/worktree.test.ts index 8d28acf..3d58bbb 100644 --- a/test/integration/worktree.test.ts +++ b/test/integration/worktree.test.ts @@ -87,6 +87,28 @@ async function makeRealRepo(): Promise { 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 { + 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'), '\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('') }) + + // ── FR-B1.9: ?base= 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('') + }) + + 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([]) + }) }) diff --git a/test/worktree-form.test.ts b/test/worktree-form.test.ts index 27f617e..f4e6cfd 100644 --- 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