131 lines
14 KiB
Markdown
131 lines
14 KiB
Markdown
# Diff against a base branch (?base=<rev>)
|
||
|
||
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=feature` → `200`, `DiffResult` with `base:'feature'`, verbatim content.
|
||
- `GET /projects/diff?path=<repo>&base=-rf` → `400`.
|
||
- `GET /projects/diff?path=<repo>&base=ghost-branch` → `200` 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 / 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 to `undefined`) → normal working-tree diff.
|
||
- **`base` + `staged=1` both set** → `base` 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 repo** → `HEAD` 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 XSS** — `base` 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.5–2 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. |