diff --git a/public/diff.ts b/public/diff.ts index 385895a..afdcc49 100644 --- a/public/diff.ts +++ b/public/diff.ts @@ -47,7 +47,12 @@ export function normalizeDiffResult(raw: unknown): DiffResult | null { .map(normalizeFile) .filter((f): f is DiffFile => f !== null) - return { files, staged: o['staged'], truncated: o['truncated'] } + return { + files, + staged: o['staged'], + truncated: o['truncated'], + base: typeof o['base'] === 'string' ? o['base'] : undefined, + } } function normalizeFile(raw: unknown): DiffFile | null { @@ -103,13 +108,26 @@ function normalizeLine(raw: unknown): DiffLine | null { /* ── fetchDiff ───────────────────────────────────────────────────────────────── */ +/** Options for {@link fetchDiff}: a `base` revision (branch/tag/sha) takes + * precedence over `staged` — the server ignores `staged` when `base` is set. */ +export interface FetchDiffOpts { + staged?: boolean + base?: string +} + /** - * Fetch a diff from the server for the given repo path. - * Returns null on any error or invalid response (best-effort). + * Fetch a diff from the server for the given repo path. When `opts.base` is set + * the URL carries `&base=` (working-tree/staged are omitted); otherwise it + * carries `&staged=`. Returns null on any error or invalid response. */ -export async function fetchDiff(repoPath: string, staged: boolean): Promise { +export async function fetchDiff(repoPath: string, opts: FetchDiffOpts = {}): Promise { try { - const url = `/projects/diff?path=${encodeURIComponent(repoPath)}&staged=${staged}` + let url = `/projects/diff?path=${encodeURIComponent(repoPath)}` + if (opts.base !== undefined && opts.base !== '') { + url += `&base=${encodeURIComponent(opts.base)}` + } else { + url += `&staged=${opts.staged === true}` + } const res = await fetch(url) if (!res.ok) return null const data: unknown = await res.json() @@ -240,6 +258,9 @@ export interface DiffViewerHandle { export interface MountDiffViewerOpts { /** Called when the viewer's close button or close() is invoked. */ onClose?: () => void + /** Base revisions (branches) offered in the "compare base" picker. When + * empty/omitted no picker is rendered (backward-compatible). */ + bases?: string[] } /** @@ -257,6 +278,7 @@ export function mountDiffViewer( ): DiffViewerHandle { let destroyed = false let staged = false + let base: string | null = null // null → working-tree/staged mode; string → base-diff // ── skeleton ────────────────────────────────────────────────────────────── const root = el('div', 'df-viewer') @@ -268,9 +290,44 @@ export function mountDiffViewer( const stagedBtn = el('button', 'df-tab', 'Staged') const closeBtn = el('button', 'df-close', '✕ Close') - toolbar.append(workingBtn, stagedBtn, closeBtn) + toolbar.append(workingBtn, stagedBtn) + + // Optional "compare against base" picker: Working tree + one option per base. + const bases = opts.bases ?? [] + let baseSelect: HTMLSelectElement | null = null + if (bases.length > 0) { + baseSelect = document.createElement('select') + baseSelect.className = 'df-base-select' + baseSelect.setAttribute('aria-label', 'Compare against base') + const wtOpt = el('option', undefined, 'Working tree') + wtOpt.value = '' + baseSelect.append(wtOpt) + for (const b of bases) { + const opt = el('option', undefined, b) // textContent — inert (SEC-H4) + opt.value = b + baseSelect.append(opt) + } + baseSelect.addEventListener('change', () => { + const v = baseSelect?.value ?? '' + base = v === '' ? null : v + updateTabState() + void loadDiff() + }) + toolbar.append(baseSelect) + } + + toolbar.append(closeBtn) root.append(toolbar) + // In base mode the Working/Staged tabs are inert (a base diff can't be staged). + function updateTabState(): void { + const inBase = base !== null + workingBtn.disabled = inBase + stagedBtn.disabled = inBase + workingBtn.classList.toggle('df-tab-disabled', inBase) + stagedBtn.classList.toggle('df-tab-disabled', inBase) + } + // Content area const content = el('div', 'df-content') root.append(content) @@ -304,7 +361,7 @@ export function mountDiffViewer( content.textContent = 'Loading…' - const result = await fetchDiff(repoPath, staged) + const result = await fetchDiff(repoPath, base !== null ? { base } : { staged }) if (destroyed) return diff --git a/public/projects.ts b/public/projects.ts index d72fc3c..5d76add 100644 Binary files a/public/projects.ts and b/public/projects.ts differ diff --git a/src/http/diff.ts b/src/http/diff.ts index 2949c52..4c4fc69 100644 --- a/src/http/diff.ts +++ b/src/http/diff.ts @@ -15,8 +15,13 @@ * - diff content is carried verbatim in DiffLine.text — the FE renders it as * inert text (AC-B1.4), never HTML. * - * FR-B1.9 (`?base=`) is intentionally deferred to P2 (review #13): it needs - * a `git rev-parse --verify` allow-list before any revision reaches the CLI. + * FR-B1.9 (`?base=`) — diff a whole branch against a base commit-ish. The + * mitigation is a two-stage revision allow-list applied BEFORE any revision + * reaches the diff CLI: (1) `isPlausibleRev` — a pure syntactic boundary check + * (rejects flag-injection / `..` ranges / metachars); (2) `git rev-parse --verify` + * — git itself is the authoritative allow-list, and only its canonical sha output + * is passed to `git diff ... --`, fully decoupling the raw user string from + * the diff invocation. */ import { execFile } from 'node:child_process' @@ -33,6 +38,20 @@ import type { const execFileAsync = promisify(execFile) +// ── base-revision allow-list (pure) ───────────────────────────────────────── + +/** First-stage syntactic gate for a user-supplied `?base=` (FR-B1.9). A + * plausible commit-ish starts with an alphanumeric and uses only the safe git + * ref charset; this rejects flag injection (leading `-`), `..` ranges, + * whitespace and shell metacharacters BEFORE any git call. It is NOT a full + * ref validator — `git rev-parse --verify` (resolveBaseRev) is the + * authoritative allow-list; this only fails obvious junk fast. Never throws. */ +export function isPlausibleRev(base: string): boolean { + if (typeof base !== 'string') return false + if (base.includes('..')) return false // block A..B / A...B ranges + return /^[A-Za-z0-9][A-Za-z0-9._/@^~{}-]{0,249}$/.test(base) +} + // ── numstat (pure) ────────────────────────────────────────────────────────── /** One `git diff --numstat` row: `\t\t`; binary = `-\t-`. */ @@ -259,6 +278,9 @@ export function parseUnifiedDiff(patch: string, numstat?: Map } @@ -339,16 +361,85 @@ async function listUntracked(cwd: string, timeoutMs: number, maxBytes: number): return files } +/** Cap the file list at diffMaxFiles, propagating a truncation flag (DoS bound). */ +function boundFiles( + files: readonly DiffFile[], + diffMaxFiles: number, + truncated: boolean, +): { files: DiffFile[]; truncated: boolean } { + if (files.length > diffMaxFiles) return { files: files.slice(0, diffMaxFiles), truncated: true } + return { files: [...files], truncated } +} + /** - * Read a repo's diff (working tree or `--staged`) as structured DiffResult. - * `repoPath` must already be a validated absolute git directory (route layer, - * SEC-H7). Best-effort: git failures yield an empty result rather than throwing. + * Resolve a user-supplied base revision to a canonical sha, or null (FR-B1.9). + * Two-stage allow-list: `isPlausibleRev` (defense-in-depth — the route also + * guards) then `git rev-parse --verify --quiet --end-of-options ^{commit}`. + * Only a `[0-9a-f]{7,64}` sha is accepted; anything else (unknown ref, non-commit + * peel, junk) → null. Never throws. The raw `base` is never interpolated: it is + * a single argv element after `--end-of-options`, and only the sha reaches diff. + */ +async function resolveBaseRev( + cwd: string, + base: string, + timeoutMs: number, + maxBytes: number, +): Promise { + if (!isPlausibleRev(base)) return null + const { out } = await runGit( + cwd, + ['rev-parse', '--verify', '--quiet', '--end-of-options', `${base}^{commit}`], + timeoutMs, + maxBytes, + ) + const sha = out.trim() + return /^[0-9a-f]{7,64}$/.test(sha) ? sha : null +} + +/** + * Diff the current HEAD against a base commit-ish, three-dot (`...HEAD` — + * the changes introduced on this branch since it diverged from base, matching a + * PR view). `base` is echoed on the result; untracked files are NOT listed. + * Best-effort: an unresolvable base → empty result rather than throwing. + */ +async function getBaseDiff( + repoPath: string, + base: string, + timeout: number, + maxBytes: number, + diffMaxFiles: number, +): Promise { + const resolved = await resolveBaseRev(repoPath, base, timeout, maxBytes) + if (resolved === null) return { files: [], staged: false, truncated: false, base } + + const range = `${resolved}...` // ...HEAD; trailing `--` terminates options + const patch = await runGit(repoPath, ['diff', '--no-color', range, '--'], timeout, maxBytes) + const num = await runGit(repoPath, ['diff', '--numstat', range, '--'], timeout, maxBytes) + + const files = parseUnifiedDiff(patch.out, parseNumstat(num.out)) + const { files: bounded, truncated } = boundFiles( + files, + diffMaxFiles, + patch.truncated || num.truncated, + ) + return { files: bounded, staged: false, truncated, base } +} + +/** + * Read a repo's diff (working tree, `--staged`, or against a `base` revision) as + * a structured DiffResult. `repoPath` must already be a validated absolute git + * directory (route layer, SEC-H7). `opts.base` (when set) wins over `staged`. + * Best-effort: git failures yield an empty result rather than throwing. */ export async function getDiff(repoPath: string, opts: GetDiffOptions): Promise { - const { staged, cfg } = opts + const { staged, base, cfg } = opts const { diffTimeoutMs: timeout, diffMaxBytes: maxBytes, diffMaxFiles } = cfg - const stagedArg = staged ? ['--staged'] : [] + if (base !== undefined) { + return getBaseDiff(repoPath, base, timeout, maxBytes, diffMaxFiles) + } + + const stagedArg = staged ? ['--staged'] : [] const patch = await runGit(repoPath, ['diff', '--no-color', ...stagedArg, '--'], timeout, maxBytes) const num = await runGit(repoPath, ['diff', '--numstat', ...stagedArg, '--'], timeout, maxBytes) @@ -357,9 +448,10 @@ export async function getDiff(repoPath: string, opts: GetDiffOptions): Promise diffMaxFiles ? ((truncated = true), files.slice(0, diffMaxFiles)) : files - + const { files: bounded, truncated } = boundFiles( + files, + diffMaxFiles, + patch.truncated || num.truncated, + ) return { files: bounded, staged, truncated } } diff --git a/src/server.ts b/src/server.ts index eec054f..2d5d984 100644 --- a/src/server.ts +++ b/src/server.ts @@ -39,7 +39,7 @@ import { deriveApprovalPreview } from './http/approval-preview.js' import { listSessions } from './http/history.js' import { buildProjects, buildProjectDetail } from './http/projects.js' import { openInEditor, openFileInEditor } from './http/editor.js' -import { getDiff } from './http/diff.js' +import { getDiff, isPlausibleRev } from './http/diff.js' import { parseStatusLine } from './http/statusline.js' import { createWorktree } from './http/worktrees.js' import { createSessionManager } from './session/manager.js' @@ -801,9 +801,18 @@ export function startServer(cfg: Config): { close(): Promise } { res.status(404).json({ error: 'project not found' }) // SEC-H7 three-prong return } + // FR-B1.9: optional ?base= — diff HEAD against a base commit-ish. The + // rev-parse allow-list (in getDiff) is the real defense; isPlausibleRev + // rejects flag-injection/junk fast with a 400 before any git call. + const rawBase = req.query['base'] + const base = typeof rawBase === 'string' && rawBase !== '' ? rawBase : undefined + if (base !== undefined && !isPlausibleRev(base)) { + res.status(400).json({ error: 'invalid base revision' }) + return + } try { const staged = req.query['staged'] === '1' - res.json(await getDiff(target, { staged, cfg })) + res.json(await getDiff(target, { staged, base, cfg })) } catch (err) { console.error('[server] /projects/diff failed:', err instanceof Error ? err.message : String(err)) res.status(500).json({ error: 'failed to read diff' }) diff --git a/src/types.ts b/src/types.ts index bdb4c35..7a4a10c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -532,6 +532,7 @@ export interface DiffResult { files: DiffFile[]; staged: boolean; truncated: boolean; + base?: string; // echoed when the diff was against a base revision (?base=) } /* ── B3 worktree creation (§3.5) ── */ diff --git a/test/diff.test.ts b/test/diff.test.ts index 3539d90..5be3044 100644 --- 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= 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