14 KiB
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:
200structuredDiffResult(withbaseechoed when supplied)400 {error}— missingpath, or abasethat 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) yields200withfiles: []— 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, nountrackedentries, 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 assertgetDiffstill returns empty (defense-in-depth) — optional.- Implement
resolveBaseRev+ the base branch ingetDiff→ 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=feature→200,DiffResultwithbase:'feature', verbatim content.GET /projects/diff?path=<repo>&base=-rf→400.GET /projects/diff?path=<repo>&base=ghost-branch→200withfiles:[].- Wire
baseparse +isPlausibleRev400 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(nostaged=);fetchDiff('/repo',{staged:true})builds the current&staged=trueURL (update the existing signature-based tests).normalizeDiffResult({files:[],staged:false,truncated:false,base:'main'})→.base==='main'; a payload withoutbase→.base===undefinedand still valid.- Implement
fetchDiffopts +normalizeDiffResultpass-through → GREEN.
5. Base picker — test/diff.test.ts (jsdom) — extend describe('mountDiffViewer') (:353):
mountDiffViewer(container,'/repo',{bases:['main','dev']})renders a<select>with optionsWorking tree+main+dev.- Selecting
main(dispatchchange) triggers a fetch whose URL containsbase=mainand disables the Working/Staged tabs; selectingWorking treerestores astaged-mode fetch. - Empty/absent
bases→ no<select>(backward-compatible with existing tests that callmountDiffViewer(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 / coverage — npm 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 toundefined) → normal working-tree diff.base+staged=1both set →basewins; staged silently ignored; resultstaged: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 byisPlausibleRev(leading-) →400; even if it slipped through,--end-of-optionsin rev-parse and the trailing--ingit diffneutralize it. - Range injection (
base=a..b,base=a...b) →isPlausibleRevrejects..; we construct the...ourselves from a single resolved sha. - Unknown ref (typo, deleted branch) → rev-parse
--verifymiss →resolveBaseRevreturnsnull→ emptyDiffResult(no crash). Rare in practice since the picker only offers real worktree branches. - Unrelated histories (no merge-base for
<base>...HEAD) →git differrors →runGitreturns empty (:302-308) → empty result. basepeels to a tree/tag-of-tree, not a commit →^{commit}peel fails →null→ empty.- Detached HEAD in the repo →
HEADstill 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 sinceisPlausibleRevalready blocks leading-, the flag can be dropped without loss if a legacy git is hit. - jsdom picker with
bases:[]or omitted → no select rendered; existingmountDiffViewer(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 a400; (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 togit 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 diffare read-only;baseintroduces no write path, so the route keeps its no-Origin-guard status (same threat model as/projects,:660). No new state-changing surface → norequireAllowedOrigin/ CSRF change needed. - Path containment — unchanged:
isValidGitDirthree-prong (:122-133) still gatespath;basecannot escape the repo (rev-parse resolves insidecwd). - DoS bounds — the extra rev-parse spawn reuses
diffTimeoutMs/diffMaxBytesviarunGit(:289-301); no unbounded work added. - Frontend XSS —
baseis echoed and rendered only viatextContent/<option>.textContent; the SEC-H4 "zero innerHTML" invariant ofpublic/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.5–2 days. Backend guard +
getDiffbranch (~0.5d incl. tests), route wiring (~0.25d), FE picker +projects.tswiring (~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/diffroute, and B3 worktree/branch data inProjectDetail.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
DiffResultwithout churn.