Files
web-terminal/docs/plans/w3-diff-vs-base.md

14 KiB
Raw Blame History

Diff against a base branch (?base=)

Adds an optional base revision to the read-only git-diff side-channel so the viewer can compare a whole branch against main (or any commit-ish), not just the working tree / index. This lands the FR-B1.9 deferral called out in src/http/diff.ts:18-19, using the exact mitigation named there: a git rev-parse --verify allow-list before any revision reaches the diff CLI. The diff parsers and the render core stay untouched; only a two-stage revision guard (backend), a reflected base field, and a toolbar picker (frontend) are added.


Contract

Route (unchanged path, one new optional query param)

GET /projects/diff (src/server.ts:661-678)

Param Type Notes
path string (required) absolute git dir; validated by isValidGitDir (src/server.ts:122-133) — unchanged
staged 0|1 (optional) current behavior; ignored when base is present
base string (optional) a commit-ish (branch/tag/sha/HEAD~N). When present → three-dot diff git diff <base>... --; untracked files are not listed

Response is the existing DiffResult JSON, now with an optional reflected base:

  • 200 structured DiffResult (with base echoed when supplied)
  • 400 {error} — missing path, or a base that fails the syntactic pre-check (flag injection / junk)
  • 404 {error} — path not a git dir (unchanged)
  • Best-effort: a syntactically-valid but unknown/unrelated base (rev-parse miss, no merge-base) yields 200 with files: [] — consistent with the module's "git failure → empty, never throw" house style (src/http/diff.ts:14, :346).

Message / data types — src/types.ts (coordination edit)

Extend DiffResult (src/types.ts:475-479) with one optional field so the shape stays backward-compatible and the viewer's required-field validation is unaffected:

export interface DiffResult {
  files: DiffFile[];
  staged: boolean;
  truncated: boolean;
  base?: string;   // NEW — echoed when the diff was against a base revision
}

No other shared type changes. GetDiffOptions lives in src/http/diff.ts:260-263 (not types.ts) and gains base?: string.

Env vars — src/config.ts

None required. rev-parse + diff reuse the existing diffTimeoutMs / diffMaxBytes bounds (src/config.ts:347-362). (Optional kill-switch DIFF_BASE_ENABLED (default true) could be added mirroring worktreeEnabled at src/config.ts:372 if a runtime disable is wanted — deferred, not needed for correctness.)

New/changed function signatures — src/http/diff.ts

export function isPlausibleRev(base: string): boolean            // pure boundary check
async function resolveBaseRev(cwd, base, timeoutMs, maxBytes): Promise<string | null>   // rev-parse --verify → canonical sha | null
export interface GetDiffOptions { staged: boolean; base?: string; cfg: Pick<Config,...> }  // +base
export async function getDiff(repoPath, opts): Promise<DiffResult>   // branches on opts.base

Files to change

Path Concrete change
src/types.ts Coordination edit. Add optional base?: string to DiffResult (:475-479).
src/http/diff.ts Add exported pure isPlausibleRev (charset + no-.. + no-leading-- + length≤250). Add resolveBaseRev (runs git rev-parse --verify --quiet --end-of-options <base>^{commit} via runGit :289-309; return trimmed /^[0-9a-f]{7,64}$/ sha or null). Add base? to GetDiffOptions (:260-263). In getDiff (:347-365): if opts.base set → resolved = resolveBaseRev(...); null{files:[],staged:false,truncated:false,base:opts.base}; else run git diff --no-color <resolved>... -- and git diff --numstat <resolved>... --, skip listUntracked (:322-340), set staged:false, echo base:opts.base. Working-tree path unchanged.
src/server.ts Diff route (:661-678): read base (typeof q==='string' && q!=='' ? q : undefined); if present and !isPlausibleRev(base)400 {error:'invalid base revision'}; else pass base into getDiff(target,{staged,base,cfg}). Import isPlausibleRev from ./http/diff.js. Route stays no-Origin-guard (read-only, unchanged threat model).
public/diff.ts fetchDiff (:110-120): change signature to fetchDiff(repoPath, opts:{staged?:boolean; base?:string}); build URL with &base=<enc> (omit staged) when base set, else &staged=. normalizeDiffResult (:38-51): pass through optional base (typeof o['base']==='string' ? o['base'] : undefined; keep other fields required). MountDiffViewerOpts (:240-243): add bases?: string[]. mountDiffViewer (:253-347): add a <select> "compare-base" control to the toolbar (:265-272) — first option Working tree (base=null), then one option per bases[]; track base: string | null; when a base is chosen disable/grey the Working/Staged tabs and loadDiff calls fetchDiff(repoPath,{base}); back on "Working tree" restores fetchDiff(repoPath,{staged}). Render core (renderDiff/renderDiffFile/renderLine/renderHunk) untouched.
public/projects.ts buildDiffSection (:611-643): add bases: string[] param; pass { bases, onClose } into mountDiffViewer (:625). renderProjectDetail (:672, call site :709): derive bases = unique of detail.worktrees.map(w=>w.branch) (WorktreeInfo.branch, src/types.ts:292) [detail.branch], filtered to defined strings; pass into buildDiffSection(detail.path, diffRef, bases). This is the "reuse worktree/branch data" wiring.
test/http/diff.test.ts unit isPlausibleRev + getDiff base integration (below).
test/integration/worktree.test.ts route-level base tests (real startServer).
test/diff.test.ts jsdom fetchDiff/normalizeDiffResult/picker tests.
test/worktree-form.test.ts assert bases reach the mountDiffViewer mock.

TDD steps (ordered)

1. Pure guard — test/http/diff.test.ts (node) — add a describe('isPlausibleRev'):

  • accepts main, feature/x, HEAD~3, v1.2.0, a 40-hex sha, main^, HEAD@{1}.
  • rejects '', a 300-char string, -rf/--output=x (leading -), a..b, x y (whitespace), `id` / $(x) / ; (metachars), \x00.
  • Implement isPlausibleRev → GREEN. (Matches the existing pure-parser layer at :34-266.)

2. getDiff with base — test/http/diff.test.ts (node, real repo) — extend the describe('getDiff (real git repo)') block (:274). In a beforeAll-style setup, commit on main, then git(repo,'checkout','-b','feature'), commit a change:

  • getDiff(repo,{staged:false,base:'main',cfg:LIMITS})result.base==='main', result.staged===false, the feature-only change present, no untracked entries, counts numstat-consistent (mirror :292-302).
  • base:'main' while HEAD===main → files:[] (empty, no changes).
  • base:'no-such-branch'{files:[], truncated:false} (rev-parse miss → empty; assert never throws, like :340-347).
  • base:'-x' never reaches here (route-guarded) but assert getDiff still returns empty (defense-in-depth) — optional.
  • Implement resolveBaseRev + the base branch in getDiff → GREEN.

3. Route — test/integration/worktree.test.ts (node, startServer) — this file already covers GET /projects/diff (header comment :2-11); add, using its itGit + temp-repo + two-branch setup:

  • GET /projects/diff?path=<repo>&base=feature200, DiffResult with base:'feature', verbatim content.
  • GET /projects/diff?path=<repo>&base=-rf400.
  • GET /projects/diff?path=<repo>&base=ghost-branch200 with files:[].
  • Wire base parse + isPlausibleRev 400 into the route → GREEN.

4. Frontend fetch/normalize — test/diff.test.ts (jsdom) — the file mocks fetch; add:

  • fetchDiff('/repo',{base:'main'}) builds /projects/diff?path=%2Frepo&base=main (no staged=); fetchDiff('/repo',{staged:true}) builds the current &staged=true URL (update the existing signature-based tests).
  • normalizeDiffResult({files:[],staged:false,truncated:false,base:'main'}).base==='main'; a payload without base.base===undefined and still valid.
  • Implement fetchDiff opts + normalizeDiffResult pass-through → GREEN.

5. Base picker — test/diff.test.ts (jsdom) — extend describe('mountDiffViewer') (:353):

  • mountDiffViewer(container,'/repo',{bases:['main','dev']}) renders a <select> with options Working tree + main + dev.
  • Selecting main (dispatch change) triggers a fetch whose URL contains base=main and disables the Working/Staged tabs; selecting Working tree restores a staged-mode fetch.
  • Empty/absent bases → no <select> (backward-compatible with existing tests that call mountDiffViewer(container,'/repo',{})).
  • Implement toolbar select + state → GREEN.

6. Wiring — test/worktree-form.test.ts (jsdom) — it already mocks mountDiffViewer (:31) and tests renderProjectDetail/buildDiffSection (:402-421). Add: give a ProjectDetail with worktrees:[{branch:'main',...},{branch:'feat',...}], click "View Diff", assert the mockMountDiffViewer was called with bases containing main and feat. Implement buildDiffSection + renderProjectDetail derivation → GREEN.

7. Refactor / coveragenpm test; confirm the 80% gate holds (every new branch — isPlausibleRev both arms, resolveBaseRev hit/miss, getDiff base/no-base, route 400/200, picker on/off — is exercised above).


Edge cases & failure modes

  • base='' → treated as absent (route coerces to undefined) → normal working-tree diff.
  • base + staged=1 both setbase wins; staged silently ignored; result staged:false. (Documented; the picker disables the staged tab in base mode so the UI can't send both.)
  • Flag injection (base=-rf, --output=/etc/passwd) → rejected by isPlausibleRev (leading -) → 400; even if it slipped through, --end-of-options in rev-parse and the trailing -- in git diff neutralize it.
  • Range injection (base=a..b, base=a...b) → isPlausibleRev rejects ..; we construct the ... ourselves from a single resolved sha.
  • Unknown ref (typo, deleted branch) → rev-parse --verify miss → resolveBaseRev returns null → empty DiffResult (no crash). Rare in practice since the picker only offers real worktree branches.
  • Unrelated histories (no merge-base for <base>...HEAD) → git diff errors → runGit returns empty (:302-308) → empty result.
  • base peels to a tree/tag-of-tree, not a commit^{commit} peel fails → null → empty.
  • Detached HEAD in the repoHEAD still resolves; three-dot works.
  • Huge branch diff → existing diffMaxFiles/diffMaxBytes/timeout truncation applies unchanged (:360-364).
  • Rename/binary/new/deleted across the base range → handled by the untouched parseUnifiedDiff/parseNumstat (numstat is authoritative for counts, :187-205).
  • Old git without --end-of-options (pre-2.24) → not a concern in 2026, but since isPlausibleRev already blocks leading -, the flag can be dropped without loss if a legacy git is hit.
  • jsdom picker with bases:[] or omitted → no select rendered; existing mountDiffViewer(container,'/repo',{}) tests keep passing.

Security

  • Revision allow-list (the core mitigation, src/http/diff.ts:18-19) — two stages, both before the diff CLI: (1) isPlausibleRev — a pure boundary check (/^[A-Za-z0-9][A-Za-z0-9._/@^~{}-]{0,249}$/, reject ..) rejecting flag-injection/junk fast with a 400; (2) git rev-parse --verify --quiet --end-of-options <base>^{commit} — git itself is the authoritative allow-list, and its output (a canonical 40/64-hex sha) is what's passed to git diff, fully decoupling the raw user string from the diff invocation.
  • No shell — all git calls stay execFile('git',[...]) (:296), args as an array; trailing -- terminates options on every diff command (:352-353), matching the file's SEC note (:11-12).
  • Read-only — rev-parse and git diff are read-only; base introduces no write path, so the route keeps its no-Origin-guard status (same threat model as /projects, :660). No new state-changing surface → no requireAllowedOrigin / CSRF change needed.
  • Path containment — unchanged: isValidGitDir three-prong (:122-133) still gates path; base cannot escape the repo (rev-parse resolves inside cwd).
  • DoS bounds — the extra rev-parse spawn reuses diffTimeoutMs/diffMaxBytes via runGit (:289-301); no unbounded work added.
  • Frontend XSSbase is echoed and rendered only via textContent/<option>.textContent; the SEC-H4 "zero innerHTML" invariant of public/diff.ts (:8-9) is preserved (render core untouched).
  • Rate-limit — parity with the existing diff route (no per-route limiter today); base adds one bounded read-only spawn per request, no new amplification. If the route is later rate-limited, this feature needs no change.

Effort & dependencies

  • Effort: ~1.52 days. Backend guard + getDiff branch (~0.5d incl. tests), route wiring (~0.25d), FE picker + projects.ts wiring (~0.75d incl. jsdom tests), polish/coverage (~0.25d).
  • Depends on: the shipped B1 diff stack — src/http/diff.ts, public/diff.ts, the /projects/diff route, and B3 worktree/branch data in ProjectDetail.worktrees (src/types.ts:290-312) which the picker reuses. No new features required.
  • Unlocks / adjacent: W13 "Stage / commit / push from the diff viewer" (a base-vs-branch view is the natural surface for review-before-push) and W10 "PR + CI status chip" (comparing a feature branch against its PR base). Keeping the render core and parsers unchanged means those build on the same DiffResult without churn.