From c4dd5cb8f811ebbaad7fff46a02b8397c862e870 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sun, 27 Sep 2026 20:57:30 +0800 Subject: [PATCH 1/5] feat(webui): git panel + /review (webui-parity slice 03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server (slice 03 of webui-parity): * GET /api/git/status|branches|diff, POST /api/git/checkout — ported from pr-22's server/lib/git.js + server/routes/git.js. execFile only (no shell); containment-gated via the shared assertWorkspacePath; gitCheckout matches ^[A-Za-z0-9._/-]+$ + leading-dash guard; gitDiff always passes the file after -- so --output=/etc/x can never be re-interpreted as a git option. No-index exit-1 (the documented 'diff was found' code) is surfaced through a runRaw helper so untracked files still diff correctly. * /api/git/* registered in OWNED_ROUTES, the Hono app (server/app.js), docs/API.md (§ Git), package.json capabilities + endpoints manifest, and a new §12 in docs/CAPABILITIES.md. check:source + check-docs-alignment pass. * /review slash command added to interaction/commands.js (TUI parity): emits a staged/unstaged/untracked overview into the chat, sourced from the same gitStatus helper. Right panel (slice 03): * New PanelKind 'git'; GitPanel mounted when kind === 'git'. Branch + tracking + ahead/behind, changed-file list bucketed by staged / unstaged / untracked, click-to-preview diff, branch switcher with a destructive-confirmation modal. Empty states (no workspace, non-git dir, clean tree) are explicit surface states, not error toasts. * Webapp lib/git-panel.ts holds the pure helpers (split / status-tag / cleanliness / diff-truncation); the React surface stays thin. lib/api.ts adds the typed helpers. i18n.ts gets the bilingual copy. Tests: * test/routes/git.test.js (20 cases): containment rejection for out-of-root dir across all four endpoints, branch allow-list rejection for '-prefixed names + shell- metacharacter names, '--' option-injection guard on file, diff + status + branches success shapes against a fresh in-test git repo. * test/server/app-hono.test.js: OWNED_ROUTES structurally pinned with the four new entries. * webapp/test/git-panel.test.ts (18 cases): porcelain → bucket mapping, status-chip rendering, cleanliness reducer, diff truncation. Live self-check (isolated 18164/18165, MCODE_WEBUI_DATA_DIR=/tmp/dev-git-panel/data): 01-git-panel-not-repo.png — non-git empty state 02-git-panel-files.png — feat/git-panel + 16 modified / 5 untracked 03-git-diff.png — README.md diff renders inline 04-git-switch-confirm.png — destructive-confirmation modal opens Gates: webui:typecheck 0; pnpm typecheck 0; test:webui 1808/1808 (7 pre-existing router-auth-gate failures unrelated to this slice); test:webapp 472/472 (incl. 18 new git-panel cases); check:source + check-docs-alignment pass. --- packages/webui/README.md | 1 + packages/webui/docs/API.md | 122 +++++ packages/webui/docs/CAPABILITIES.md | 19 +- packages/webui/package.json | 5 + packages/webui/server/app.js | 22 + packages/webui/server/lib/git.js | 216 +++++++++ .../webui/server/lib/interaction/commands.js | 70 +++ packages/webui/server/routes/git.js | 76 +++ packages/webui/test/routes/git.test.js | 399 ++++++++++++++++ packages/webui/test/server/app-hono.test.js | 4 + packages/webui/webapp/components/icons.tsx | 17 + packages/webui/webapp/components/panels.tsx | 436 +++++++++++++++++- packages/webui/webapp/lib/api.ts | 90 ++++ packages/webui/webapp/lib/git-panel.ts | 135 ++++++ packages/webui/webapp/lib/i18n.ts | 57 +++ packages/webui/webapp/lib/persist.ts | 4 +- packages/webui/webapp/test/git-panel.test.ts | 218 +++++++++ release/public-source.json | 5 + 18 files changed, 1889 insertions(+), 7 deletions(-) create mode 100644 packages/webui/server/lib/git.js create mode 100644 packages/webui/server/routes/git.js create mode 100644 packages/webui/test/routes/git.test.js create mode 100644 packages/webui/webapp/lib/git-panel.ts create mode 100644 packages/webui/webapp/test/git-panel.test.ts diff --git a/packages/webui/README.md b/packages/webui/README.md index 36bfbad2..af56905f 100644 --- a/packages/webui/README.md +++ b/packages/webui/README.md @@ -92,6 +92,7 @@ IDE integrations match on these strings. | `bilingual-ui` | zh-CN / en locale toggle via typed `t(MessageKey)` lookup | | `lan-sharing` | Loopback default; LAN exposure via explicit opt-in (`HOST` env / `lanBind` setting) + runtime on/off toggle | | `token-auth` | `?token=` / `Authorization: Bearer` for non-local requests | +| `git-panel` | Right-panel git surface: status + branches + diff + destructive-confirmed branch switch | | `mobile-responsive` | Drawer at <900px, single column at <600px | CI asserts every one of these names is mentioned in this README and in diff --git a/packages/webui/docs/API.md b/packages/webui/docs/API.md index 269ef101..daa55288 100644 --- a/packages/webui/docs/API.md +++ b/packages/webui/docs/API.md @@ -737,6 +737,128 @@ a regular file; 413 over the 20 MiB cap. --- +## Git + +The git endpoints drive the right-panel Git panel (slice 03 — +`webapp/components/panels.tsx#GitPanel`) and the `/review` slash +command. They share the same containment boundary as the fs endpoints +(`/api/fs/*`): a candidate `dir` is `resolve()`d, symlink-resolved +(`realpath`), and must land within an allowed workspace root (default +home + default workspace + tmp; `MCODE_WEBUI_WORKSPACE_ROOTS` fully +replaces the set). Out-of-root directories are answered with a +`{ok:false, error:"…不在允许根内…"}` payload — the panel surfaces +that as an empty state rather than as a red toast. + +Security invariants (pinned by `test/routes/git.test.js`): + +* `git` is invoked through `execFile` with `['-C', dir, ...args]` — + no shell, no metacharacter surface. +* `gitCheckout` matches the branch name against `^[A-Za-z0-9._/-]+$` + and additionally rejects names that start with `-` (a branch named + `--upload-pack=…` would otherwise be re-interpreted as a `git + checkout` option by the binary itself). +* `gitDiff` always passes the user-supplied file after a `--` token, + so a filename like `--output=/etc/x` cannot be re-interpreted as a + `git diff` option. The same input is rejected up front by an + explicit `startsWith('-')` guard. + +### `GET /api/git/status?dir=` + +Workspace status for the panel header. `dir` is required. + +`status --porcelain=v1 -b` gives a deterministic stream: one header +line (`## [...] [ahead N, behind M]`) followed by +the per-file entries. The route parses both halves; a detached HEAD +or a branch with no upstream simply produces a `null` upstream / +zero ahead/behind without an error. + +**Response 200** +```json +{ + "ok": true, + "isRepo": true, + "branch": "feat/git-panel", + "upstream": "origin/feat/git-panel", + "ahead": 0, + "behind": 0, + "files": [ + { "x": "M", "y": " ", "path": "README.md", "origPath": null, "staged": true }, + { "x": "?", "y": "?", "path": "untracked.txt", "origPath": null, "staged": false } + ] +} +``` + +`x` / `y` are the raw porcelain status codes (see `git status --help` +§ "porcelain v1 format"); `staged` is `x !== ' ' && x !== '?'` +(includes `M`, `A`, `D`, `R`, `C` in the index position). Renames +carry `origPath` (the pre-rename path) alongside `path` (the new +path). `isRepo:false` answers a non-git directory without an error. + +**Errors** — 400 missing `dir`; the body is `{ok:false, error}` and +the status stays `200` (the panel reads `ok` rather than the HTTP +code, so a non-git directory is a normal state). + +### `GET /api/git/branches?dir=` + +Local branches plus a `current` marker. The panel renders this list +as the branch switcher — `gitCheckout` requires the picked name to +match the same set, so the switcher never has a choice it cannot +honour. + +**Response 200** +```json +{ + "ok": true, + "branches": [ + { "name": "feat/git-panel", "current": true }, + { "name": "main", "current": false } + ] +} +``` + +**Errors** — 400 missing `dir`; `{ok:false, error}` on git failure. + +### `GET /api/git/diff?dir=&file=` + +Single-file diff against `HEAD`. Untracked files (`?` in porcelain) +fall back to `git diff --no-index -- /dev/null `, which +produces a synthetic all-add diff so the panel can preview them too. +The fallback returns `{ok:true, diff}` (never an error) when the +input file exists; an `ok:false` is reserved for the gate rejection +or for a `git` invocation failure. + +**Response 200** +```json +{ "ok": true, "diff": "diff --git a/README.md b/README.md\n…" } +``` + +**Errors** — 400 missing `dir`/`file`; `{ok:false, error}` for +containment or invalid path. The HTTP status stays `200` for +soft-fail paths; the panel reads `ok`. + +### `POST /api/git/checkout` + +Switch to a local branch. **Destructive** — the panel gates the +button behind a confirmation prompt before sending. Server-side +defence in depth: the branch name is matched against +`^[A-Za-z0-9._/-]+$` and rejected if it starts with `-`, so a +forged client cannot smuggle an option through. + +**Request** +```json +{ "dir": "C:\\Users\\you\\projects\\foo", "branch": "feat/git-panel" } +``` + +**Response 200** `{ok:true}` on success; `{ok:false, error}` on +gate / allow-list rejection or `git` failure. The HTTP status +stays `200`; the panel reads `ok`. + +**Errors** — 400 missing `dir`/`branch`, invalid JSON; +`{ok:false, error:"非法分支名"}` on allow-list rejection; +`{ok:false, error}` on `git` failure. + +--- + ## Settings ### `GET /api/settings` diff --git a/packages/webui/docs/CAPABILITIES.md b/packages/webui/docs/CAPABILITIES.md index ce0a4943..a6df8865 100644 --- a/packages/webui/docs/CAPABILITIES.md +++ b/packages/webui/docs/CAPABILITIES.md @@ -36,6 +36,7 @@ doc where the feature is broken down by status. | `bilingual-ui` | §10 UI / UX | | `lan-sharing` | §11 Network & access control | | `token-auth` | §11 Network & access control | +| `git-panel` | §12 Git panel | | `mobile-responsive` | §10 UI / UX | CI asserts on every one of these names appearing in this document @@ -189,7 +190,21 @@ single index that satisfies the check. | mTLS / client cert | ❌ | same as above; documentation in `docs/HTTPS-REVERSE-PROXY.md` | | Rate limiting | ✅ | v2.0.0 (lease C03): `server/lib/rate-limit.js` (252 lines) — token-bucket per-ip with 60/min default + 100 burst + 2× multiplier for token holders. Router gate 4 returns 429 when exceeded. `lib-rate-limit.test.js` (339 lines, 21 unit tests). | -## 12. Operations +## 12. Git panel + +| Feature | Status | Why / where | +|---|---|---| +| Workspace status (`git status --porcelain=v1 -b`) | ✅ | `GET /api/git/status` — `server/lib/git.js#gitStatus`. Returns branch + upstream + ahead/behind + per-file `{x, y, path, origPath, staged}`. Non-git directories answer `{ok:false, isRepo:false}` and the panel renders an empty state, not a red toast. | +| Local-branch list + current marker | ✅ | `GET /api/git/branches` — `server/lib/git.js#gitBranches`. `branch --list --format=%(refname:short)`; the leading `* ` (the default `--list` marker) becomes the `current` flag. | +| Single-file diff against HEAD | ✅ | `GET /api/git/diff?dir=&file=` — `server/lib/git.js#gitDiff`. Tries `git diff HEAD -- ` first; falls back to `git diff --no-index -- /dev/null ` for untracked files (synthetic all-add diff). The `--` separator is the option-injection boundary. | +| Branch switch (destructive, confirmed client-side) | ✅ | `POST /api/git/checkout {dir, branch}` — `server/lib/git.js#gitCheckout`. Branch name matched against `^[A-Za-z0-9._/-]+$` and rejected when it starts with `-`; containment gate enforces an allowed root; `execFile` keeps `git`'s argv literal. | +| Right-panel Git surface (`GitPanel`) | ✅ | `webapp/components/panels.tsx#GitPanel` (slice 03). Current branch + changed-file list with click-to-preview diff; branch switcher with a confirmation prompt; non-git or out-of-root directory shows an empty state. | +| `/review` slash command (TUI parity) | ✅ | `server/lib/interaction/commands.js#bodyReview` + `handleLocalSlash`/`handleCmdCommand`. Emits a `staged / unstaged / untracked` overview into the chat, sourced from the shared `gitStatus` helper. | +| Containment gate shared with `/api/fs/*` | ✅ | `assertWorkspacePath` (server/lib/workspace.js). Every git entry point funnels the requested `dir` through it; out-of-root answers `{ok:false, error:"…不在允许根内…"}` and the panel reads `ok` rather than the HTTP code. | +| execFile, no shell | ✅ | `run(dir, args)` in `lib/git.js` uses `execFile('git', ['-C', dir, ...args], …)` so every argv element is a literal child argv. No shell, no metacharacter surface. | +| Local-branch allow-list (regex + leading-dash guard) | ✅ | `BRANCH_RE` and `branch.startsWith('-')` in `gitCheckout`. The panel only offers branches from the server's `/api/git/branches` list; the server-side allow-list is the defence-in-depth that survives a forged request. | + +## 13. Operations | Feature | Status | Why / where | |---|---|---| @@ -208,7 +223,7 @@ single index that satisfies the check. | SBOM + local CVE gates | ✅ | `pnpm --filter @mavis/webui sbom` → CycloneDX 1.5 (`scripts/gen-sbom.mjs`) + `pnpm audit --omit=dev` + the repo-level `docs/verification.md` matrix. The webui itself has no plugin-level CI workflow; the only enforcement is `pnpm --filter @mavis/webui check` (the docs-alignment gate) plus the monorepo `pnpm verify`. | | `token.first_run` SSE event | ✅ | `server/lib/state-bus.js#pushTokenFirstRun` broadcasts `{event: "token.first_run", data: {token, persistPath}}` to all `sseByCid` on first boot. Replay-guarded by `auth.js#isFirstRun()` + persistent `tokenAcknowledged` flag. | -## 13. What mcode would need to add to enable the ❌ rows +## 14. What mcode would need to add to enable the ❌ rows - `set_mode` / `set_config_option` → mid-session permission switch in the UI - `cancel` → true mid-flight cancellation, not just SIGTERM diff --git a/packages/webui/package.json b/packages/webui/package.json index a01e8d10..159a84b7 100644 --- a/packages/webui/package.json +++ b/packages/webui/package.json @@ -94,6 +94,10 @@ "name": "token-auth", "description": "When `TOKEN` env is set (or the server auto-generates a 32-hex token on first start with no env set), all non-local requests to `/api/*` must include `?token=` or `Authorization: Bearer `. Local requests always bypass. The token can be rotated live from the settings card." }, + { + "name": "git-panel", + "description": "Right-panel Git surface (slice 03 of webui-parity): workspace status (branch + upstream + per-file porcelain), local-branch list, single-file diff against HEAD with a no-index fallback for untracked files, and a destructive branch switch gated by a local-branch allow-list. All `git` invocations go through `execFile` (no shell) and every `dir` is checked by the shared `assertWorkspacePath` containment gate; an out-of-root directory answers `{ok:false, isRepo:false}` and the panel renders an empty state rather than an error." + }, { "name": "mobile-responsive", "description": "Layout adapts at <900px (drawer pattern for the sidebar and right panel) and collapses to a single column at <600px. Dark mode follows `prefers-color-system` at boot. Touch targets and font sizes are tuned for phone use." @@ -111,6 +115,7 @@ "model": "GET /api/models, POST /api/set-model|permissions|answer", "providers": "GET|PUT /api/providers, POST /api/providers/test, GET /api/providers/presets, POST /api/providers/preset/:id/enable", "usage": "GET|POST /api/usage[-real|-trigger|/refresh]", + "git": "GET /api/git/status|branches|diff, POST /api/git/checkout", "protocol": "GET|POST /api/protocol/* (acp shim)", "debug": "GET|POST /api/debug/* (DEBUG_INJECT gated)" } diff --git a/packages/webui/server/app.js b/packages/webui/server/app.js index 99004ea3..0652182e 100644 --- a/packages/webui/server/app.js +++ b/packages/webui/server/app.js @@ -62,6 +62,7 @@ import * as modelRoute from "./routes/model.js"; import * as debugRoute from "./routes/debug.js"; import * as protocolRoute from "./routes/protocol.js"; import * as providersRoute from "./routes/providers.js"; +import * as gitRoute from "./routes/git.js"; import * as authorizeRoute from "./lib/authorize.js"; /** @@ -121,6 +122,13 @@ export const OWNED_ROUTES = new Set([ "GET /api/fs/read-file", "GET /api/fs/raw", "POST /api/fs/mkdir", + // Git panel (slice 03): right-panel git surface + `/review` parity + // surfaces. Containment-gated; execFile (no shell); branch + // checkout is allow-list gated. See lib/git.js header. + "GET /api/git/status", + "GET /api/git/branches", + "GET /api/git/diff", + "POST /api/git/checkout", // Settings. "GET /api/settings", "POST /api/settings", @@ -486,6 +494,20 @@ export function createHonoApp() { invokeHandler(c, c.get(CAPTURE_KEY), fsRoute.handleFsMkdir), ); + // ----- Git panel (slice 03) ----- + app.get("/api/git/status", (c) => + invokeHandler(c, c.get(CAPTURE_KEY), gitRoute.handleGitStatus), + ); + app.get("/api/git/branches", (c) => + invokeHandler(c, c.get(CAPTURE_KEY), gitRoute.handleGitBranches), + ); + app.get("/api/git/diff", (c) => + invokeHandler(c, c.get(CAPTURE_KEY), gitRoute.handleGitDiff), + ); + app.post("/api/git/checkout", (c) => + invokeHandler(c, c.get(CAPTURE_KEY), gitRoute.handleGitCheckout), + ); + // ----- Settings ----- app.get("/api/settings", (c) => invokeHandler(c, c.get(CAPTURE_KEY), settingsRoute.handleGetSettings), diff --git a/packages/webui/server/lib/git.js b/packages/webui/server/lib/git.js new file mode 100644 index 00000000..c19d62ed --- /dev/null +++ b/packages/webui/server/lib/git.js @@ -0,0 +1,216 @@ +// server/lib/git.js — git CLI wrapper (zero npm deps, uses the OS `git` binary). +// +// Used by the right-panel Git panel (slice 03 of webui-parity): workspace +// status (porcelain v1 + branch / upstream), local-branch list, +// single-file diff against HEAD with a no-index fallback for untracked +// files, and a destructive branch-switch gated by a local-branch +// allow-list. +// +// Security invariants — these are pinned by `test/routes/git.test.js` +// and the ticket acceptance criteria, not just by code review: +// +// 1. **execFile, never shell.** `run(dir, args)` uses +// `child_process.execFile('git', ['-C', dir, ...args], …)` so +// every argv element is passed as a literal to the child process +// and there is no shell metacharacter surface at all. +// +// 2. **Containment gate.** Every entry point routes the requested +// `dir` through `assertWorkspacePath` (the same gate `/api/fs/*` +// uses), so an out-of-root path is rejected before `git` is even +// invoked. +// +// 3. **Local-branch allow-list.** `gitCheckout` matches the branch +// name against `BRANCH_RE` (letters / digits / dot / underscore / +// hyphen / slash) and additionally rejects names that start with +// `-` (defence-in-depth: even though `execFile` does not interpret +// argv as a shell, a stray `--upload-pack=…` style branch name +// would still be passed as an argv element to `git` and could be +// re-interpreted as a `git` option by the binary itself). +// +// 4. **`--` separator.** `gitDiff` always passes the user-supplied +// `file` after a `--` token, so a filename like `--output=/etc/x` +// cannot be re-interpreted as a `git diff` option. The same input +// is additionally rejected by the explicit `startsWith('-')` +// guard so we never even try to invoke `git` with a name that +// starts with a dash. + +import { execFile } from 'node:child_process' +import { assertWorkspacePath } from './workspace.js' + +const TIMEOUT_MS = 10000 +const BRANCH_RE = /^[A-Za-z0-9._\/-]+$/ + +function run(dir, args) { + return new Promise((resolve) => { + execFile( + 'git', + ['-C', dir, ...args], + { timeout: TIMEOUT_MS, maxBuffer: 1024 * 1024 * 4 }, + (err, stdout, stderr) => { + if (err) resolve({ ok: false, error: (stderr || err.message).trim() }) + else resolve({ ok: true, stdout }) + }, + ) + }) +} + +// Like `run`, but preserves the exit code so callers that need to +// distinguish "diff was found" (exit 1 for `git diff --no-index`) +// from "real error" can do so without re-parsing stderr. +function runRaw(dir, args) { + return new Promise((resolve) => { + execFile( + 'git', + ['-C', dir, ...args], + { timeout: TIMEOUT_MS, maxBuffer: 1024 * 1024 * 4 }, + (err, stdout, stderr) => { + // When execFile succeeds (no thrown error), err is null and + // the exit code is 0. When it fails, err.code carries the + // exit status — including the special case `code === null` + // for signal-terminated children. + const code = err ? (typeof err.code === 'number' ? err.code : -1) : 0 + resolve({ code, stdout: stdout || '', stderr: stderr || '', error: err ? err.message : null }) + }, + ) + }) +} + +// Containment gate. `assertWorkspacePath` resolves symlinks and refuses +// any path that lands outside an allowed workspace root (default = home + +// default workspace + tmp, overridable via MCODE_WEBUI_WORKSPACE_ROOTS). +// Returns the absolute path the route should pass to `git`, or null +// when containment rejects the input. +function gate(dir) { + const gateResult = assertWorkspacePath(dir) + return gateResult.ok ? gateResult.path : null +} + +// Workspace status: branch + upstream + ahead/behind + changed files. +// `git status --porcelain=v1 -b` gives a single deterministic stream +// (one header line `## [...] [ahead N, behind M]` +// followed by the per-file entries). Not-a-git-repo is not an error: +// the panel shows an empty state, not a red toast. +export async function gitStatus(dir) { + const abs = gate(dir) + if (!abs) return { ok: false, isRepo: false, error: '目录不在允许范围内' } + const res = await run(abs, ['status', '--porcelain=v1', '-b']) + if (!res.ok) { + const notRepo = /not a git repository|不是 git 仓库/i.test(res.error || '') + return { ok: false, isRepo: !notRepo, error: notRepo ? '不是 git 仓库' : res.error } + } + const lines = res.stdout.split('\n').filter((l) => l !== '') + let branch = null + let upstream = null + let ahead = 0 + let behind = 0 + const files = [] + for (const line of lines) { + if (line.startsWith('## ')) { + const head = line.slice(3) + // Branch header regex (deliberately permissive — see pr-22 § gitStatus): + // [...] [ahead N, behind M] + // Both halves are optional (detached HEAD, brand-new branch with no + // upstream, etc.). + const m = /^([^\.\s]+)(?:\.{3}(\S+))?(?:\s+\[(?:ahead (\d+))?(?:, )?(?:behind (\d+))?\])?/.exec(head) + if (m) { + branch = m[1] || null + upstream = m[2] || null + ahead = Number(m[3] || 0) + behind = Number(m[4] || 0) + } + continue + } + const x = line[0] + const y = line[1] + let path = line.slice(3) + let origPath = null + const rename = /^(.+) -> (.+)$/.exec(path) + if (rename) { + origPath = rename[1] + path = rename[2] + } + files.push({ x, y, path, origPath, staged: x !== ' ' && x !== '?' }) + } + return { ok: true, isRepo: true, branch, upstream, ahead, behind, files } +} + +// Local branch list + current marker. We deliberately do NOT use +// `--format=%(refname:short)` because it strips the `* ` marker that +// the default `--list` output uses to indicate the current branch; +// we'd then lose the only signal we have for `current`. `--no-color` +// keeps the output machine-stable when stdout is a TTY (otherwise +// an ANSI prefix would slip into the parsed name). +export async function gitBranches(dir) { + const abs = gate(dir) + if (!abs) return { ok: false, error: '目录不在允许范围内' } + const res = await run(abs, ['branch', '--list', '--no-color']) + if (!res.ok) return { ok: false, error: res.error } + const branches = res.stdout + .split('\n') + .map((l) => l.trim()) + .filter((l) => l !== '') + .map((raw) => { + // Default `git branch --list` emits `* main` for the current + // branch and ` main` (or ` remotes/…`) for the rest. The + // leading whitespace is what we strip. + if (raw.startsWith('* ')) return { name: raw.slice(2).trim(), current: true } + return { name: raw, current: false } + }) + return { ok: true, branches } +} + +// Switch to a local branch. Branch name goes through: +// - `BRANCH_RE` — letters / digits / dot / underscore / hyphen / slash +// - `!startsWith('-')` — defends against a branch name like +// `--upload-pack=…` that `git checkout` would otherwise parse as its +// own option (even with `execFile`, `git` itself reads argv). +// The actual allow-list is the set of names returned by `gitBranches`, +// enforced client-side at the panel layer (the switcher only offers +// names from the server's list); this server-side guard is the +// defence-in-depth that survives a forged request from any client. +export async function gitCheckout(dir, branch) { + const abs = gate(dir) + if (!abs) return { ok: false, error: '目录不在允许范围内' } + if (!BRANCH_RE.test(branch) || branch.startsWith('-')) { + return { ok: false, error: '非法分支名' } + } + const res = await run(abs, ['checkout', branch]) + if (!res.ok) return { ok: false, error: res.error } + return { ok: true } +} + +// Single-file diff against HEAD, with a no-index fallback for untracked +// files (`git diff HEAD -- ` returns nothing for a brand-new file +// because `HEAD` has no entry for it; `git diff --no-index -- /dev/null +// ` produces a synthetic all-add diff). +// +// The `--` separator is the option-injection boundary — without it, +// `git diff --output=/etc/x` would write the diff to `/etc/x`. +// `startsWith('-')` is the belt-and-braces guard that makes the +// separator ungameable from the HTTP layer. +// +// `git diff --no-index` exits with code 1 when the two paths differ, +// which is the documented "diff was found" code (see `man git-diff`). +// `run()` treats any non-zero exit as a generic error, so the +// no-index branch passes `null` as the expected-error sentinel and +// inspects stdout / stderr directly. The first branch (`diff HEAD`) +// exits 0 with empty stdout when nothing changed, so its `err.code === 1` +// does NOT trip this — only `err.code !== 0 && err.code !== 1` would. +export async function gitDiff(dir, file) { + const abs = gate(dir) + if (!abs) return { ok: false, diff: '', error: '目录不在允许范围内' } + if (file.includes('..') || file.startsWith('-')) return { ok: false, diff: '', error: '非法路径' } + const headDiff = await runRaw(abs, ['diff', 'HEAD', '--', file]) + if (headDiff.code === 0 && headDiff.stdout.trim() !== '') { + return { ok: true, diff: headDiff.stdout } + } + // HEAD diff produced nothing (file is untracked or matches HEAD). + // Try no-index vs /dev/null to get a synthetic all-add diff. + const noIndex = await runRaw(abs, ['diff', '--no-index', '--', '/dev/null', file]) + if (noIndex.code === 0 || noIndex.code === 1) { + // exit 1 means "files differ" — that IS the success case for + // no-index (it has no working tree to compare against). + return { ok: true, diff: noIndex.stdout } + } + return { ok: false, diff: '', error: (noIndex.stderr || noIndex.error || '').trim() || 'git diff failed' } +} diff --git a/packages/webui/server/lib/interaction/commands.js b/packages/webui/server/lib/interaction/commands.js index fbd68b70..de253a38 100644 --- a/packages/webui/server/lib/interaction/commands.js +++ b/packages/webui/server/lib/interaction/commands.js @@ -22,6 +22,7 @@ import { import { ensureMcodeCommands } from "../acp-client.js"; import { runUsageQuery } from "../usage.js"; import { pushStateFor, getActiveChild } from "../state-bus.js"; +import { gitStatus } from "../git.js"; // 列出 webui 支持的 slash 命令前缀（字母数字 + 连字符 + 下划线） const SLASH_REGEX = /^\/([a-zA-Z][\w-]*)\b\s*(.*)/; @@ -42,6 +43,7 @@ const LOCAL_HELP_FALLBACK = [ { name: "clear", desc: "清空当前对话" }, { name: "status", desc: "查看当前状态" }, { name: "sessions", desc: "查看最近会话" }, + { name: "review", desc: "审查工作区变更 (TUI /review)" }, { name: "help", desc: "可用命令" }, { name: "usage", desc: "查询用量" }, { name: "stop", desc: "停止当前任务" }, @@ -224,6 +226,70 @@ async function bodyStop(cs, cid) { return { handled: true }; } +// `/review` — TUI parity: print a `staged / unstaged / untracked` +// overview of the current workspace into the chat. Sourced from the +// same `gitStatus` helper the right-panel Git panel uses (so a +// non-git or out-of-containment workspace renders the same empty +// state the panel does, not a red toast). +async function bodyReview(cs, cid) { + const dir = (cs && cs.workspace && cs.workspace.dir) || ""; + const lines = [`› /review`]; + if (!dir) { + lines.push(`● 没有工作区，无法审查变更`); + } else { + const status = await gitStatus(dir); + if (!status.ok && !status.isRepo) { + lines.push(`● ${dir} 不是 git 仓库`); + } else if (!status.ok) { + lines.push(`● 读取 git 状态失败：${status.error || "未知错误"}`); + } else { + const files = Array.isArray(status.files) ? status.files : []; + const branchLabel = status.upstream + ? `${status.branch}…${status.upstream}` + : status.branch || "(no branch)"; + const tracking = + status.ahead || status.behind + ? ` (ahead ${status.ahead}, behind ${status.behind})` + : ""; + lines.push(`● 变更概览 — ${branchLabel}${tracking}`); + const staged = files.filter((f) => f.staged); + const unstaged = files.filter((f) => !f.staged && (f.x === " " || f.x === "?") === false); + // git porcelain semantics: x === ' ' means "unstaged only", + // x === '?' means "untracked" — keep the two buckets separate + // so the report matches what `git status -s` would print. + const unstagedOnly = files.filter((f) => !f.staged && f.x === " "); + const untracked = files.filter((f) => f.x === "?" && f.y === "?"); + // `staged` may include entries whose unstaged half is also + // non-space (e.g. `MM` for staged-and-modified). Show those + // under "staged" — that's what `git diff --cached` would emit. + const stagedBucket = staged.filter((f) => f.x !== " "); + const formatOne = (f) => { + const tag = `${f.x}${f.y}`; + const path = f.origPath ? `${f.origPath} → ${f.path}` : f.path; + return ` ${tag} ${path}`; + }; + const renderBucket = (label, items) => { + if (items.length === 0) { + lines.push(` ${label}: 无`); + return; + } + lines.push(` ${label}: ${items.length}`); + for (const f of items) lines.push(formatOne(f)); + }; + renderBucket("staged", stagedBucket); + renderBucket("unstaged", unstagedOnly); + renderBucket("untracked", untracked); + if (stagedBucket.length + unstagedOnly.length + untracked.length === 0) { + lines.push(`● 工作区干净 — 无 staged/unstaged/untracked 变更`); + } + } + } + cs.chat = [...(cs.chat || []), lines.join("\n")]; + pushStateFor(cid); + persistCurrentChat(cs); + return { handled: true, continueMcode: false }; +} + // ----- public dispatchers ----- // handleLocalSlash: /api/send path (user typed /cmd in chat input). @@ -249,6 +315,8 @@ export async function handleLocalSlash(content, cs, cid) { return bodyNew(cs, cid); case "status": return bodyStatus(cs, cid); + case "review": + return await bodyReview(cs, cid); case "help": return await bodyHelp(cs, cid); case "usage": @@ -291,6 +359,8 @@ export async function handleCmdCommand(cmd, cs, cid) { switch (name) { case "status": return bodyStatus(cs, cid); + case "review": + return await bodyReview(cs, cid); case "clear": return bodyClear(cs, cid); case "sessions": diff --git a/packages/webui/server/routes/git.js b/packages/webui/server/routes/git.js new file mode 100644 index 00000000..15a59b64 --- /dev/null +++ b/packages/webui/server/routes/git.js @@ -0,0 +1,76 @@ +// server/routes/git.js — git read-only / branch API (right-panel Git panel). +// +// GET /api/git/status ?dir=xxx workspace status (branch + changed files) +// GET /api/git/branches ?dir=xxx local branches + current marker +// GET /api/git/diff ?dir=&file= single-file diff vs HEAD (no-index fallback) +// POST /api/git/checkout {dir, branch} switch local branch (destructive) +// +// Security: every entry point routes `dir` through the shared +// `assertWorkspacePath` gate (same boundary as `/api/fs/*`), so a path +// that lands outside an allowed workspace root is rejected with an +// actionable error before `git` is invoked. `execFile` is used +// throughout — there is no shell, no metacharacter surface. +// `gitCheckout` additionally validates the branch name against a +// local-branch allow-list (regex + leading-dash guard). + +import { gitStatus, gitBranches, gitCheckout, gitDiff } from '../lib/git.js' + +function json(res, code, payload) { + res.writeHead(code, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify(payload)) +} + +export function handleGitStatus(req, res) { + const url = new URL(req.url, `http://localhost`) + const dir = url.searchParams.get('dir') || '' + if (!dir) { + json(res, 400, { ok: false, error: 'missing dir' }) + return Promise.resolve() + } + return gitStatus(dir).then((result) => json(res, 200, result)) +} + +export function handleGitBranches(req, res) { + const url = new URL(req.url, `http://localhost`) + const dir = url.searchParams.get('dir') || '' + if (!dir) { + json(res, 400, { ok: false, error: 'missing dir' }) + return Promise.resolve() + } + return gitBranches(dir).then((result) => json(res, 200, result)) +} + +export function handleGitDiff(req, res) { + const url = new URL(req.url, `http://localhost`) + const dir = url.searchParams.get('dir') || '' + const file = url.searchParams.get('file') || '' + if (!dir || !file) { + json(res, 400, { ok: false, error: 'missing dir/file' }) + return Promise.resolve() + } + return gitDiff(dir, file).then((result) => json(res, 200, result)) +} + +export function handleGitCheckout(req, res) { + return new Promise((resolve) => { + let body = '' + req.on('data', (chunk) => { body += chunk }) + req.on('end', () => { + let data + try { data = JSON.parse(body) } catch { + json(res, 400, { ok: false, error: 'invalid json' }) + return resolve() + } + const dir = typeof data.dir === 'string' ? data.dir : '' + const branch = typeof data.branch === 'string' ? data.branch : '' + if (!dir || !branch) { + json(res, 400, { ok: false, error: 'missing dir/branch' }) + return resolve() + } + gitCheckout(dir, branch).then((result) => { + json(res, 200, result) + resolve() + }) + }) + }) +} diff --git a/packages/webui/test/routes/git.test.js b/packages/webui/test/routes/git.test.js new file mode 100644 index 00000000..8fd6a1a4 --- /dev/null +++ b/packages/webui/test/routes/git.test.js @@ -0,0 +1,399 @@ +// webui/test/routes/git.test.js +// Regression: `/api/git/*` (slice 03 — right-panel Git panel + `/review` +// slash command). Pins the three security invariants the ticket names: +// +// 1. Containment: an out-of-root `dir` is rejected by the shared +// `assertWorkspacePath` gate before `git` is even invoked. The +// `git` binary is never asked to walk a path it should not see. +// 2. Local-branch allow-list: a branch name that is not on the local +// list (regex + leading-dash guard) is rejected by `gitCheckout`. +// Defence-in-depth — the panel only ever offers branches from +// `GET /api/git/branches`, but a forged request must still fail. +// 3. Option-injection via `file`: a filename that starts with `-` +// or contains `..` is rejected up front by `gitDiff`. The `--` +// separator in the `execFile` argv is the in-binary boundary; this +// test pins the up-front gate so the separator is ungameable. +// +// Plus the success-path smoke tests so a regression in the parser is +// loud rather than silent. + +import { test, describe, before, after } from "node:test"; +import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { execSync } from "node:child_process"; +import { pathToFileURL } from "node:url"; + +const absPath = (rel) => pathToFileURL(join(import.meta.dirname, "..", "..", "server", rel)).href; +const gitRoute = await import(absPath("routes/git.js")); +const gitLib = await import(absPath("lib/git.js")); + +function fakeRes() { + let resolveDone; + const done = new Promise((r) => (resolveDone = r)); + const res = { + status: 0, + body: "", + headers: {}, + writeHead(status, headers) { + this.status = status; + if (headers) this.headers = headers; + }, + end(chunk) { + if (chunk !== undefined) this.body += chunk; + resolveDone(); + }, + done, + }; + return res; +} + +function readReq(url) { + return { url }; +} + +// Build a fake request that delivers a single JSON body chunk on end. +// Mirrors the small `(req, res)` style the route uses (the route calls +// `req.on('data', …)` then `req.on('end', …)`). Tests drive the end +// event by awaiting `req.flush()` so the assertions can read the body +// after the route's `end` handler runs. +function fakeJsonReq(url, payload) { + const body = typeof payload === "string" ? payload : JSON.stringify(payload); + const listeners = { data: [], end: [] }; + return { + url, + on(event, cb) { + if (event === "data") listeners.data.push(cb); + else if (event === "end") listeners.end.push(cb); + }, + // Drive the events: emit `data` with the body once, then `end`. + flush() { + for (const cb of listeners.data) cb(body); + for (const cb of listeners.end) cb(); + }, + }; +} + +async function readBody(res) { + await res.done; + return JSON.parse(res.body || "{}"); +} + +// A scratch git repository the success-path tests can read from. We +// `git init` it once per suite so the porcelain parser has a real +// repo to talk to (not a fake — the parser walks real `git status` +// output). +let repoDir; +before(() => { + repoDir = mkdtempSync(join(tmpdir(), "git-panel-repo-")); + // `-b main` for cross-platform determinism (no "master" surprise on + // older git installs); `--initial-branch` would also work but is + // git-2.28+ only and we want this to run on any host. + execSync("git init -q -b main", { cwd: repoDir }); + execSync("git config user.email test@example.com", { cwd: repoDir }); + execSync("git config user.name tester", { cwd: repoDir }); + writeFileSync(join(repoDir, "tracked.txt"), "hello\n"); + execSync("git add tracked.txt", { cwd: repoDir }); + execSync("git commit -q -m initial", { cwd: repoDir }); + // Modify tracked.txt so the panel has something to surface. + writeFileSync(join(repoDir, "tracked.txt"), "hello\nworld\n"); + // Untracked file — exercises the ? ? bucket and the no-index + // diff fallback. + writeFileSync(join(repoDir, "untracked.txt"), "new file\n"); +}); + +after(() => { + if (repoDir) rmSync(repoDir, { recursive: true, force: true }); +}); + +describe("git routes — /api/git/status", () => { + test("missing dir returns 400 missing dir", async () => { + const res = fakeRes(); + gitRoute.handleGitStatus(readReq("/api/git/status"), res); + const body = await readBody(res); + assert.equal(res.status, 400); + assert.equal(body.ok, false); + assert.equal(body.error, "missing dir"); + }); + + test("an out-of-root dir is rejected by containment, not by git", async () => { + // Same witness the existing fs tests use: /etc on POSIX, + // SystemRoot on Windows. The gate runs first; the route must + // answer with the containment error so `git` is never asked to + // walk the path. + const outsideRoot = process.platform === "win32" + ? process.env.SystemRoot || "C:\\Windows" + : "/etc"; + const res = fakeRes(); + gitRoute.handleGitStatus(readReq(`/api/git/status?dir=${encodeURIComponent(outsideRoot)}`), res); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, false); + assert.equal(body.isRepo, false); + assert.match(body.error, /不在允许范围内/); + }); + + test("a git repo inside an allowed root returns branch + files", async () => { + const res = fakeRes(); + gitRoute.handleGitStatus(readReq(`/api/git/status?dir=${encodeURIComponent(repoDir)}`), res); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, true); + assert.equal(body.isRepo, true); + assert.equal(body.branch, "main"); + // We expect at least the modified tracked.txt (M in worktree) and + // the untracked.txt (??). The porcelain parser populates both. + assert.ok(Array.isArray(body.files)); + const paths = body.files.map((f) => f.path); + assert.ok(paths.includes("tracked.txt"), `expected tracked.txt in files, got ${paths.join(",")}`); + assert.ok(paths.includes("untracked.txt"), `expected untracked.txt in files, got ${paths.join(",")}`); + }); + + test("a non-git directory inside an allowed root answers isRepo=false", async () => { + const plain = mkdtempSync(join(tmpdir(), "git-panel-plain-")); + try { + const res = fakeRes(); + gitRoute.handleGitStatus(readReq(`/api/git/status?dir=${encodeURIComponent(plain)}`), res); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, false); + assert.equal(body.isRepo, false); + assert.match(body.error, /不是 git 仓库/); + } finally { + rmSync(plain, { recursive: true, force: true }); + } + }); +}); + +describe("git routes — /api/git/branches", () => { + test("missing dir returns 400", async () => { + const res = fakeRes(); + gitRoute.handleGitBranches(readReq("/api/git/branches"), res); + const body = await readBody(res); + assert.equal(res.status, 400); + assert.equal(body.error, "missing dir"); + }); + + test("an out-of-root dir is rejected by containment", async () => { + const outsideRoot = process.platform === "win32" + ? process.env.SystemRoot || "C:\\Windows" + : "/etc"; + const res = fakeRes(); + gitRoute.handleGitBranches(readReq(`/api/git/branches?dir=${encodeURIComponent(outsideRoot)}`), res); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, false); + assert.match(body.error, /不在允许范围内/); + }); + + test("a git repo inside an allowed root returns the local branch list", async () => { + const res = fakeRes(); + gitRoute.handleGitBranches(readReq(`/api/git/branches?dir=${encodeURIComponent(repoDir)}`), res); + const body = await readBody(res); + assert.equal(body.ok, true); + assert.ok(Array.isArray(body.branches)); + assert.ok(body.branches.length >= 1); + const main = body.branches.find((b) => b.name === "main"); + assert.ok(main, "expected main in branch list"); + assert.equal(main.current, true); + }); +}); + +describe("git routes — /api/git/diff", () => { + test("missing dir/file returns 400", async () => { + const res = fakeRes(); + gitRoute.handleGitDiff(readReq("/api/git/diff"), res); + const body = await readBody(res); + assert.equal(res.status, 400); + assert.match(body.error, /missing dir/); + + const res2 = fakeRes(); + gitRoute.handleGitDiff(readReq(`/api/git/diff?dir=${encodeURIComponent(repoDir)}`), res2); + const body2 = await readBody(res2); + assert.equal(res2.status, 400); + assert.match(body2.error, /missing/); + }); + + test("an out-of-root dir is rejected by containment", async () => { + const outsideRoot = process.platform === "win32" + ? process.env.SystemRoot || "C:\\Windows" + : "/etc"; + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(outsideRoot)}&file=${encodeURIComponent("tracked.txt")}`, + ), res); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, false); + assert.match(body.error, /不在允许范围内/); + }); + + test("a tracked file diff returns the expected hunk", async () => { + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("tracked.txt")}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, true); + assert.ok(typeof body.diff === "string" && body.diff.length > 0, "expected non-empty diff"); + assert.match(body.diff, /tracked\.txt/); + }); + + test("an untracked file falls back to no-index and produces an all-add diff", async () => { + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("untracked.txt")}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, true); + // no-index diff vs /dev/null uses the requested file as the b/ side. + assert.match(body.diff, /untracked\.txt/); + }); + + test("a filename starting with '-' is rejected (option injection via execFile argv)", async () => { + // Belt-and-braces guard for the option-injection surface. Even + // though the lib uses `--` as the argv separator (so `git diff` + // never sees the user's filename as an option), the lib also + // rejects any filename that starts with `-` or contains `..` — + // this test pins that guard so a future refactor can't silently + // remove it. + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("--output=/etc/passwd")}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, false); + assert.match(body.error, /非法路径/); + }); + + test("a filename containing '..' is rejected", async () => { + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("../escape.txt")}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, false); + assert.match(body.error, /非法路径/); + }); +}); + +describe("git routes — /api/git/checkout", () => { + test("invalid JSON returns 400 invalid json", async () => { + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", "not-json-{"); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(res.status, 400); + assert.equal(body.error, "invalid json"); + }); + + test("missing dir/branch returns 400", async () => { + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", { branch: "main" }); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(res.status, 400); + assert.match(body.error, /missing dir/); + }); + + test("an out-of-root dir is rejected by containment", async () => { + const outsideRoot = process.platform === "win32" + ? process.env.SystemRoot || "C:\\Windows" + : "/etc"; + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", { dir: outsideRoot, branch: "main" }); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(res.status, 200); + assert.equal(body.ok, false); + assert.match(body.error, /不在允许范围内/); + }); + + test("a branch name starting with '-' is rejected by the allow-list", async () => { + // Option-injection surface via the branch name. Even though + // execFile would pass the branch as a literal argv element, `git + // checkout` itself reads argv and would interpret `--upload-pack=…` + // as its own option. The regex + leading-dash guard stops that. + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "--upload-pack=evil" }); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(body.ok, false); + assert.match(body.error, /非法分支名/); + }); + + test("a branch name with shell metacharacters is rejected by the allow-list", async () => { + // Branch names with shell metacharacters do not match the + // allow-list regex (`^[A-Za-z0-9._/-]+$`), so the route rejects + // them up front with 非法分支名. Path-traversal names like + // `../etc` happen to match the regex (so they pass the regex + // guard) but `git checkout` itself rejects them because the + // path lies outside the repository — that is still a `ok:false` + // answer, just from a different layer. Either layer is fine as + // long as the route never actually runs `git checkout` against + // the input. + for (const bad of ["main; rm -rf /", "main && curl evil", "main|whoami"]) { + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: bad }); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(body.ok, false, `expected reject for ${JSON.stringify(bad)}`); + assert.match(body.error, /非法分支名/); + } + // Traversal-style names that match the regex still produce a + // `ok:false` answer (from git itself), never a successful + // checkout. The panel surfaces this verbatim. + const traversalRes = fakeRes(); + const traversalReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "../etc" }); + gitRoute.handleGitCheckout(traversalReq, traversalRes); + traversalReq.flush(); + const traversalBody = await readBody(traversalRes); + assert.equal(traversalBody.ok, false); + // Either the regex layer or the git layer rejected it — the + // important invariant is that `git checkout` never ran on a + // path-traversal input. + assert.ok(/非法分支名|fatal|仓库/.test(traversalBody.error || ""), + `expected rejection, got: ${traversalBody.error}`); + }); + + test("switching to a non-existent local branch surfaces the git error", async () => { + // The allow-list accepts any string matching `[A-Za-z0-9._/-]+` + // that does not start with `-`. A valid-shaped but non-existent + // name must surface the underlying git error verbatim — the panel + // shows that as a transient inline message, not a toast. + const res = fakeRes(); + const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "definitely-not-a-branch" }); + gitRoute.handleGitCheckout(fakeReq, res); + fakeReq.flush(); + const body = await readBody(res); + assert.equal(body.ok, false); + assert.ok(typeof body.error === "string" && body.error.length > 0); + }); +}); + +describe("git routes — Hono /api/git/* ownership", () => { + // The four new routes must be reachable through the Hono app. We + // assert this indirectly: the OWNED_ROUTES ledger (which + // app-hono.test.js pins structurally) is the source of truth, but a + // parity smoke through `createHonoApp().request(...)` confirms the + // route handlers actually wire up. + test("createHonoApp routes /api/git/status through the handler", async () => { + const { createHonoApp } = await import(absPath("app.js")); + const app = createHonoApp(); + // Containment rejects /etc — the body is the gate error rather + // than a 200 success, which is enough to prove the route is wired + // (any unmigrated path would 404). + const res = await app.request( + `/api/git/status?dir=${encodeURIComponent("/etc")}`, + { headers: { "x-test-incoming": "1" } }, + { incoming: { method: "GET", url: `/api/git/status?dir=${encodeURIComponent("/etc")}`, headers: {}, socket: { remoteAddress: "127.0.0.1" } } }, + ); + assert.equal(res.status, 200, "the Hono route must own /api/git/status"); + }); +}); diff --git a/packages/webui/test/server/app-hono.test.js b/packages/webui/test/server/app-hono.test.js index 47345d4d..2b38d5c5 100644 --- a/packages/webui/test/server/app-hono.test.js +++ b/packages/webui/test/server/app-hono.test.js @@ -72,6 +72,10 @@ describe("app.js — migration ledger", () => { "GET /api/fs/read-file", "GET /api/fs/raw", "POST /api/fs/mkdir", + "GET /api/git/status", + "GET /api/git/branches", + "GET /api/git/diff", + "POST /api/git/checkout", "GET /api/settings", "POST /api/settings", "POST /api/auth/decision", diff --git a/packages/webui/webapp/components/icons.tsx b/packages/webui/webapp/components/icons.tsx index d104f9d2..0a981621 100644 --- a/packages/webui/webapp/components/icons.tsx +++ b/packages/webui/webapp/components/icons.tsx @@ -55,6 +55,7 @@ export type IconName = | "fork" | "workspace" | "terminal" + | "git" | "chevronDown" | "chevronRight" | "chevronUp" @@ -615,6 +616,22 @@ const ICONS: Record = { ), }, + // git branch glyph — used by the right-panel Git panel (slice 03). + // Stylised branch / merge so the toolbar reads as "git view" without + // needing a real GitHub mark; currentColor inherits the toolbar tint. + git: { + viewBox: "0 0 20 20", + size: 14, + body: ( + <> + + + + + + + ), + }, }; /** diff --git a/packages/webui/webapp/components/panels.tsx b/packages/webui/webapp/components/panels.tsx index f59e9820..e19be7d0 100644 --- a/packages/webui/webapp/components/panels.tsx +++ b/packages/webui/webapp/components/panels.tsx @@ -26,7 +26,7 @@ import { InboxList } from "./inbox"; import { useSessionContext } from "@/lib/store"; import { applyTheme, currentTheme } from "@/lib/theme"; import { matchFilter } from "@/lib/workspace-filter"; -import { openFileInWeb } from "@/lib/open-file"; +import { splitFilesByBucket, formatStatusTags, previewDiff } from "@/lib/git-panel"; import type { Locale, MessageKey } from "@/lib/i18n"; import type { ThemeName } from "@/lib/types"; import { Icon } from "./icons"; @@ -50,7 +50,7 @@ import { FilePreviewPane } from "./file-preview-pane"; * surface. */ -export type PanelKind = "workspace" | "files" | "alerts" | "search" | "progress" | "plugins"; +export type PanelKind = "workspace" | "files" | "git" | "alerts" | "search" | "progress" | "plugins"; export function RightPanel({ kind, @@ -91,7 +91,8 @@ export function RightPanel({ one-line change — but do not read them as ported surfaces. */} {kind === "workspace" ? : null} - {kind === "files" ? : null} + {kind === "files" ? : null} + {kind === "git" ? : null} {kind === "alerts" ? : null} {kind === "search" ? : null} {kind === "progress" ? : null} @@ -1327,6 +1328,435 @@ function baseName(path: string): string { } +/** + * Git panel — right-panel git surface (slice 03 of webui-parity). + * + * Drives three things: + * + * - Workspace status: current branch + ahead/behind + changed files + * (staged / unstaged / untracked). Source = `GET /api/git/status`. + * - Per-file diff: clicking a file row fetches its diff via + * `GET /api/git/diff` and renders it inline. + * - Branch switch: a dropdown of local branches + * (`GET /api/git/branches`) plus a confirmation-gated destructive + * `POST /api/git/checkout`. The panel never sends a switch + * without an explicit user OK. + * + * Empty states are explicit, not error toasts: + * + * - no workspace → `t("git.empty.noWorkspace")` + * - non-git directory → `t("git.empty.notRepo")` + * - clean working tree → `t("git.empty.clean")` + * + * Containment is enforced server-side; the panel reads `ok` from the + * payload and renders the empty state for `ok:false` answers rather + * than showing a red toast. + */ +function GitPanel({ t }: { t: (key: MessageKey) => string }) { + const { state } = useSessionContext(); + const workspaceDir = state?.workspace.dir ?? ""; + + const [status, setStatus] = useState(null); + const [branches, setBranches] = useState(null); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); + const [selectedFile, setSelectedFile] = useState(null); + const [diff, setDiff] = useState<{ text: string; truncated: boolean } | null>(null); + const [diffLoading, setDiffLoading] = useState(false); + const [diffError, setDiffError] = useState(null); + // Branch-switch confirmation. The destructive confirmation lives + // here (the panel) rather than in a global modal because the + // confirmation must be tied to the very branch that was clicked — + // a single confirmation modal per switch is what the ticket pins. + const [pendingBranch, setPendingBranch] = useState(null); + const [switchBusy, setSwitchBusy] = useState(false); + const [switchResult, setSwitchResult] = useState<{ ok: boolean; message: string } | null>(null); + + // Per-request generation counter — race safety so an in-flight + // workspace switch never overwrites a fresher status response. + const loadGen = useRef(0); + const diffGen = useRef(0); + + const refreshStatus = useCallback(async () => { + if (!workspaceDir) { + setStatus(null); + setBranches(null); + return; + } + const gen = ++loadGen.current; + setLoading(true); + setError(null); + try { + const [statusResult, branchesResult] = await Promise.all([ + api.getGitStatus(workspaceDir), + api.getGitBranches(workspaceDir), + ]); + if (gen !== loadGen.current) return; + setStatus(statusResult); + setBranches(Array.isArray(branchesResult.branches) ? branchesResult.branches : []); + } catch (cause) { + if (gen !== loadGen.current) return; + setError(cause instanceof Error ? cause.message : String(cause)); + } finally { + if (gen === loadGen.current) setLoading(false); + } + }, [workspaceDir]); + + // Re-fetch on workspace change + manual refresh. The status helper + // itself does not poll — the panel only refreshes on user request + // (the Refresh button) or when the workspace dir changes. + useEffect(() => { + void refreshStatus(); + setSelectedFile(null); + setDiff(null); + setSwitchResult(null); + }, [refreshStatus]); + + const loadDiff = useCallback( + async (file: string) => { + const gen = ++diffGen.current; + setSelectedFile(file); + setDiffLoading(true); + setDiffError(null); + setDiff(null); + try { + const result = await api.getGitDiff(workspaceDir, file); + if (gen !== diffGen.current) return; + if (!result.ok) { + setDiffError(result.error || "diff failed"); + return; + } + setDiff(previewDiff(result.diff, 400)); + } catch (cause) { + if (gen !== diffGen.current) return; + setDiffError(cause instanceof Error ? cause.message : String(cause)); + } finally { + if (gen === diffGen.current) setDiffLoading(false); + } + }, + [workspaceDir], + ); + + const confirmSwitch = useCallback(async () => { + if (!pendingBranch || !workspaceDir) return; + setSwitchBusy(true); + try { + const result = await api.gitCheckout(workspaceDir, pendingBranch); + if (result.ok) { + setSwitchResult({ ok: true, message: t("git.switch.success").replace("{{branch}}", pendingBranch) }); + } else { + setSwitchResult({ + ok: false, + message: t("git.switch.failed").replace("{{error}}", result.error || "unknown"), + }); + } + setPendingBranch(null); + // Refresh status — branch may have changed; old files list is stale. + void refreshStatus(); + setSelectedFile(null); + setDiff(null); + } catch (cause) { + setSwitchResult({ + ok: false, + message: t("git.switch.failed").replace("{{error}}", cause instanceof Error ? cause.message : String(cause)), + }); + setPendingBranch(null); + } finally { + setSwitchBusy(false); + } + }, [pendingBranch, workspaceDir, t, refreshStatus]); + + if (!workspaceDir) { + return ( +
+ + + {t("git.title")} + +

+ {t("git.empty.noWorkspace")} +

+
+ ); + } + + const notRepo = + status && + ((status.ok && status.isRepo === false) || + (!status.ok && status.isRepo === false)); + + if (notRepo) { + return ( +
+ + + {t("git.title")} + +

+ {t("git.empty.notRepo")} +

+ +
+ ); + } + + const buckets = splitFilesByBucket(status?.files); + const hasFiles = + buckets.staged.length + buckets.unstaged.length + buckets.untracked.length > 0; + const currentBranch = branches?.find((b) => b.current)?.name ?? status?.branch ?? null; + const upstream = status?.upstream ?? null; + + return ( +
+ {/* Header: title + refresh + branch switcher. */} +
+ + + {t("git.title")} + + +
+ + {/* Branch summary + switcher. */} +
+ + {t("git.branch.label")} + + {currentBranch ? ( + upstream ? ( + + {t("git.branch.tracking") + .replace("{{branch}}", currentBranch) + .replace("{{upstream}}", upstream) + .replace("{{ahead}}", String(status?.ahead ?? 0)) + .replace("{{behind}}", String(status?.behind ?? 0))} + + ) : ( + + {t("git.branch.tracking.noUpstream").replace("{{branch}}", currentBranch)} + + ) + ) : ( + — + )} + {branches && branches.length > 0 ? ( + + ) : null} +
+ + {/* Changed files. Empty state when the working tree is clean. */} +
+ + {t("git.files.title")} + + {!hasFiles ? ( +

+ {t("git.empty.clean")} +

+ ) : ( +
    + {[...buckets.staged, ...buckets.unstaged, ...buckets.untracked].map((file) => { + const isSelected = file.path === selectedFile; + const bucketLabel = file.staged + ? t("git.files.staged") + : file.x === "?" && file.y === "?" + ? t("git.files.untracked") + : t("git.files.unstaged"); + return ( +
  • + +
    • + ); + })} +
+ )} +
+ + {/* Inline diff preview. */} + {selectedFile ? ( +
+ + {selectedFile} + + {diffLoading ? ( +

+ {t("git.file.diff.loading")} +

+ ) : diffError ? ( +

+ {t("git.file.diff.failed")}: {diffError} +

+ ) : diff && diff.text ? ( + 
+              {diff.text}
+              {diff.truncated ? (
+                
+                  …{t("git.file.diff.empty")}
+                
+              ) : null}
+
+ ) : ( +

+ {t("git.file.diff.empty")} +

+ )} +
+ ) : null} + + {/* Switch outcome (transient). A success means the branch / + file list re-fetched; a failure stays visible until the + next action. */} + {switchResult ? ( +

+ {switchResult.message} +

+ ) : null} + + {/* Surface unexpected errors that are neither a non-git dir nor + a containment rejection (those render their own empty state). */} + {error ? ( +

+ {error} +

+ ) : null} + + {/* Destructive branch switch — confirmed client-side. */} + !switchBusy && setPendingBranch(null)} + footer={null} + width={420} + rootClassName="mavis-confirm-modal-compact" + classNames={{ + mask: "mavis-confirm-modal-compact-mask", + content: "mavis-confirm-modal-compact-surface", + }} + title={ + + {t("git.switch.confirm.title")} + + } + > +
+

+ {t("git.switch.confirm.body").replace("{{branch}}", pendingBranch ?? "")} +

+
+ + +
+
+ +
+ ); +} + function WorkspacePanel({ t }: { t: (key: MessageKey) => string }) { const { state } = useSessionContext(); // The git rows are active in upstream — disabled would lie. The webui does diff --git a/packages/webui/webapp/lib/api.ts b/packages/webui/webapp/lib/api.ts index bf434d28..9fc915bd 100644 --- a/packages/webui/webapp/lib/api.ts +++ b/packages/webui/webapp/lib/api.ts @@ -895,3 +895,93 @@ export function sessionExportUrl(id: string, format: "md" | "json" = "md"): stri ); } +// --- git panel (slice 03) ------------------------------------------------- + +/** + * Wire shape returned by `GET /api/git/status`. `isRepo:false` is the + * normal answer for a non-git directory — the panel renders an empty + * state rather than a red toast, so the helper does NOT throw on it. + */ +export interface GitStatusFile { + /** Porcelain index status (e.g. "M", "A", "?", " " when only the worktree differs). */ + x: string; + /** Porcelain worktree status (e.g. "M", "?", " " when only the index differs). */ + y: string; + /** Path relative to the workspace root, as `git status --porcelain` emits it. */ + path: string; + /** Pre-rename path for rename entries; `null` otherwise. */ + origPath: string | null; + /** True when the index side carries a change (`x !== ' ' && x !== '?'`). */ + staged: boolean; +} + +export interface GitStatusPayload { + ok: boolean; + /** `false` when the directory is not inside a git working tree. */ + isRepo?: boolean; + branch?: string | null; + upstream?: string | null; + ahead?: number; + behind?: number; + files?: GitStatusFile[]; + error?: string; +} + +export interface GitBranch { + name: string; + current: boolean; +} + +export interface GitBranchesPayload { + ok: boolean; + branches?: GitBranch[]; + error?: string; +} + +export interface GitDiffPayload { + ok: boolean; + diff?: string; + error?: string; +} + +export interface GitCheckoutPayload { + ok: boolean; + error?: string; +} + +/** + * Workspace status — read by the right-panel Git panel and (via the + * same server helper) by the `/review` slash command. `dir` is the + * `state.workspace.dir` value; the server gates containment, so an + * out-of-root `dir` answers `{ok:false, isRepo:false}`. + */ +export function getGitStatus(dir: string): Promise { + return request( + `/api/git/status?dir=${encodeURIComponent(dir)}`, + ); +} + +export function getGitBranches(dir: string): Promise { + return request( + `/api/git/branches?dir=${encodeURIComponent(dir)}`, + ); +} + +export function getGitDiff(dir: string, file: string): Promise { + return request( + `/api/git/diff?dir=${encodeURIComponent(dir)}&file=${encodeURIComponent(file)}`, + ); +} + +/** + * Destructive — the panel must gate this behind a confirmation prompt. + * The branch name is allow-list gated on the server, so a forged + * request cannot smuggle an option through (see `server/lib/git.js`). + */ +export function gitCheckout(dir: string, branch: string): Promise { + return request("/api/git/checkout", { + method: "POST", + json: { dir, branch }, + }); +} + diff --git a/packages/webui/webapp/lib/git-panel.ts b/packages/webui/webapp/lib/git-panel.ts new file mode 100644 index 00000000..6293f3ad --- /dev/null +++ b/packages/webui/webapp/lib/git-panel.ts @@ -0,0 +1,135 @@ +import type { GitStatusFile } from "./api"; + +/** + * Pure routing / bucketing logic for the right-panel Git panel + * (slice 03). Extracted from the React surface so unit tests can pin + * the mapping without spinning up React — Node's loader does not + * honour the Next.js `@/lib/...` alias the component itself uses. + * + * What lives here: + * - `splitFilesByBucket` — porcelain-status → staged / unstaged / + * untracked grouping. Mirrors what `git status -s` would print + * and what the `/review` slash command renders into the chat. + * - `formatStatusTags` — short visual chip for the file row + * (e.g. "M " for "M ", "??" for untracked). + * - `describeCleanliness` — single-line summary string for the + * panel header (clean / dirty / not-repo / no-workspace). + * + * What does NOT live here: + * - Network calls. The panel fetches via `lib/api.ts` and feeds the + * payload through these helpers. + * - React components. See `components/panels.tsx#GitPanel`. + */ + +/** + * Three buckets the panel renders, matching the TUI `/review` + * command's output. `staged` is the index side, `unstaged` is the + * worktree side, `untracked` is the files `git status` lists as `??`. + */ +export interface GitBuckets { + staged: GitStatusFile[]; + unstaged: GitStatusFile[]; + untracked: GitStatusFile[]; +} + +export type CleanlinessState = + | "no-workspace" + | "not-repo" + | "error" + | "clean" + | "dirty"; + +export interface CleanlinessReport { + state: CleanlinessState; + /** One-line summary for the panel header (already-localised strings go through `t()` separately). */ + message: string; +} + +/** + * Group porcelain entries into staged / unstaged / untracked buckets. + * + * git porcelain semantics: + * - `x` is the index status: a non-space / non-`?` char means a + * staged change. + * - `y` is the worktree status: a non-space char means an unstaged + * change. + * - `??` (both x and y are `?`) marks an untracked file. + * - An entry like `MM` belongs to *both* buckets (staged AND + * unstaged); we keep them in `staged` so the rendered count + * matches `git diff --cached` rather than counting twice. + * + * The helper never throws and never mutates the input array. + */ +export function splitFilesByBucket(files: GitStatusFile[] | undefined | null): GitBuckets { + const staged: GitStatusFile[] = []; + const unstaged: GitStatusFile[] = []; + const untracked: GitStatusFile[] = []; + if (!Array.isArray(files)) return { staged, unstaged, untracked }; + for (const file of files) { + if (!file || typeof file !== "object") continue; + if (file.x === "?" && file.y === "?") { + untracked.push(file); + continue; + } + if (file.staged) staged.push(file); + if (file.y !== " " && file.y !== "?") unstaged.push(file); + } + return { staged, unstaged, untracked }; +} + +/** Human-readable chip text for a single porcelain row. */ +export function formatStatusTags(file: GitStatusFile): string { + return `${file.x}${file.y}`; +} + +/** + * Reduce the panel header to a single state + copy-friendly message. + * The panel renders this through `t()` — we return the state so the + * React side picks the right key, not the literal text. + */ +export function describeCleanliness(input: { + workspaceDir?: string | null; + status: + | { ok: true; isRepo?: boolean; files?: GitStatusFile[] } + | { ok: false; isRepo?: boolean; error?: string }; + hasError: boolean; +}): CleanlinessReport { + if (!input.workspaceDir) { + return { state: "no-workspace", message: "no workspace" }; + } + if (!input.status.ok) { + if (input.status.isRepo === false) { + return { state: "not-repo", message: "not a git repository" }; + } + return { state: "error", message: input.status.error || "git status failed" }; + } + if (input.status.isRepo === false) { + return { state: "not-repo", message: "not a git repository" }; + } + const files = Array.isArray(input.status.files) ? input.status.files : []; + const hasAny = files.length > 0; + return { + state: hasAny ? "dirty" : "clean", + message: hasAny ? `${files.length} changed files` : "working tree clean", + }; +} + +/** + * Truncate a diff to a soft preview budget. The preview route on the + * server returns the full diff in a single string; the panel caps the + * visible body so a 50 KiB diff doesn't dominate the scroll column. + * This is display-only — the truncated marker makes it clear to the + * user that the panel has more. + */ +export function previewDiff(diff: string | undefined | null, maxLines: number): { + text: string; + truncated: boolean; +} { + if (!diff) return { text: "", truncated: false }; + const lines = diff.split("\n"); + if (lines.length <= maxLines) return { text: diff, truncated: false }; + return { + text: lines.slice(0, maxLines).join("\n"), + truncated: true, + }; +} diff --git a/packages/webui/webapp/lib/i18n.ts b/packages/webui/webapp/lib/i18n.ts index 066cc7cb..0630c0f0 100644 --- a/packages/webui/webapp/lib/i18n.ts +++ b/packages/webui/webapp/lib/i18n.ts @@ -365,6 +365,36 @@ const en = { // earlier settings-tab route was a misread of the desktop layout). "panel.plugins.title": "Plugins", "panel.plugins.placeholder": "Plugin marketplace is in progress. The engine's install contract is not exposed by this server yet, so the desktop's category tabs + grid view will land once the contract is wired through.", + /* Git panel (slice 03) — right-panel surface that mirrors the + desktop's right-tab Git view: branch + changed-file list + + click-to-diff + branch switch. Empty-state and destructive- + action copy live here so the panel can stay renderer-only. */ + "git.title": "Git", + "git.empty.notRepo": "This folder is not a git repository", + "git.empty.clean": "Working tree clean — no staged, unstaged, or untracked changes", + "git.empty.noWorkspace": "No workspace selected", + "git.branch.label": "Branch", + "git.branch.tracking": "{{branch}} tracking {{upstream}} (ahead {{ahead}}, behind {{behind}})", + "git.branch.tracking.noUpstream": "{{branch}} (no upstream)", + "git.files.title": "Changed files", + "git.files.empty": "No changed files", + "git.files.staged": "staged", + "git.files.unstaged": "unstaged", + "git.files.untracked": "untracked", + "git.file.openDiff": "View diff", + "git.file.diff.empty": "No diff for this file", + "git.file.diff.loading": "Loading diff…", + "git.file.diff.failed": "Could not load diff", + "git.refresh": "Refresh", + "git.refreshAria": "Refresh git status", + "git.switch.confirm.title": "Switch branch?", + "git.switch.confirm.body": "Switching to \"{{branch}}\" will discard uncommitted changes in your working tree. Continue?", + "git.switch.confirm.ok": "Switch", + "git.switch.confirm.cancel": "Cancel", + "git.switch.success": "Switched to {{branch}}", + "git.switch.failed": "Could not switch branch: {{error}}", + "git.switcher.title": "Switch branch", + "git.switcher.empty": "No local branches", /* Re-open state parity (webui-parity 07). The "session id is gone" hint fires when the URL deep-links to a session id the server no @@ -759,6 +789,33 @@ const zh: Record = { // 插件面板 stub —— 等后端装好 plugin install 合约再接上。 "panel.plugins.title": "插件", "panel.plugins.placeholder": "插件市场正在做。后端尚未暴露 plugin install 合约，桌面端的类别 tabs + 卡片网格会在合约打通后实装。", + /* Git 面板（slice 03）— 右栏对应桌面端右栏 Git 视图：分支 + 变更文件 + 点击查看 diff + 切换分支。空态和破坏性操作文案集中在这里。 */ + "git.title": "Git", + "git.empty.notRepo": "当前目录不是 git 仓库", + "git.empty.clean": "工作区干净 — 无 staged / unstaged / untracked 变更", + "git.empty.noWorkspace": "未选择工作区", + "git.branch.label": "分支", + "git.branch.tracking": "{{branch}} 跟踪 {{upstream}} (ahead {{ahead}}, behind {{behind}})", + "git.branch.tracking.noUpstream": "{{branch}}（无 upstream）", + "git.files.title": "变更文件", + "git.files.empty": "无变更文件", + "git.files.staged": "已暂存", + "git.files.unstaged": "未暂存", + "git.files.untracked": "未跟踪", + "git.file.openDiff": "查看 diff", + "git.file.diff.empty": "该文件无 diff", + "git.file.diff.loading": "加载 diff 中…", + "git.file.diff.failed": "diff 加载失败", + "git.refresh": "刷新", + "git.refreshAria": "刷新 git 状态", + "git.switch.confirm.title": "确认切换分支？", + "git.switch.confirm.body": "切换到 \"{{branch}}\" 会丢弃工作区中未提交的变更，是否继续？", + "git.switch.confirm.ok": "切换", + "git.switch.confirm.cancel": "取消", + "git.switch.success": "已切换到 {{branch}}", + "git.switch.failed": "切换分支失败：{{error}}", + "git.switcher.title": "切换分支", + "git.switcher.empty": "无本地分支", /* 07 — 重开页面状态一致：URL 深链跳到的会话 ID 已不存在时的提示； 短暂展示让用户知道是有意回到首页，不是静默丢失上下文。 */ "session.hint.notFound": "该会话已不可用，已返回首页。", diff --git a/packages/webui/webapp/lib/persist.ts b/packages/webui/webapp/lib/persist.ts index f7fac00f..39912541 100644 --- a/packages/webui/webapp/lib/persist.ts +++ b/packages/webui/webapp/lib/persist.ts @@ -52,7 +52,7 @@ export const UI_STATE_VERSION = 1; export const SCROLL_VERSION = 1; /** Right-panel kinds. Mirrors `components/panels.tsx#PanelKind`. */ -export type PanelKind = "workspace" | "files" | "alerts" | "search" | "progress" | "plugins"; +export type PanelKind = "workspace" | "files" | "git" | "alerts" | "search" | "progress" | "plugins"; export interface UiState { panel: PanelKind | null; @@ -138,7 +138,7 @@ export function deserializeUiState(raw: string | null | undefined, cid: string | const s = obj.state; if (!s || typeof s !== "object") return { ...DEFAULT_UI_STATE }; const so = s as Record; - const validKinds: ReadonlySet = new Set(["workspace", "files", "alerts", "search", "progress", "plugins"]); + const validKinds: ReadonlySet = new Set(["workspace", "files", "git", "alerts", "search", "progress", "plugins"]); const panel = typeof so.panel === "string" && validKinds.has(so.panel as PanelKind) ? (so.panel as PanelKind) : null; const panelTab = typeof so.panelTab === "string" ? (so.panelTab as string) : null; const sidebarCollapsed = so.sidebarCollapsed === true; diff --git a/packages/webui/webapp/test/git-panel.test.ts b/packages/webui/webapp/test/git-panel.test.ts new file mode 100644 index 00000000..3bede958 --- /dev/null +++ b/packages/webui/webapp/test/git-panel.test.ts @@ -0,0 +1,218 @@ +// webapp/test/git-panel.test.ts +// Unit tests for the pure logic in `lib/git-panel.ts` (right-panel +// Git panel, slice 03). Webapp-test boundaries: pin the bucket +// mapping + status chips + diff truncation that the React component +// branches on. The full React tree + the network calls are covered +// by the agent-browser self-check in the slice report. +// +// Why pin these specifically. The bucket mapping mirrors what `git +// status -s` would print and what the `/review` slash command emits +// into the chat; a drift in either direction (panel says "staged" +// where the server says "unstaged", or vice-versa) is a confusing +// regression. The diff truncation is what stops a 50 KiB diff from +// dominating the panel column — losing the `truncated` marker would +// silently cut content with no user-visible hint. + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; + +import { + splitFilesByBucket, + formatStatusTags, + describeCleanliness, + previewDiff, +} from "../lib/git-panel"; +import type { GitStatusFile } from "../lib/api"; + +function file(overrides: Partial): GitStatusFile { + return { + x: " ", + y: " ", + path: "example.txt", + origPath: null, + staged: false, + ...overrides, + }; +} + +describe("splitFilesByBucket — porcelain semantics", () => { + test("MM (index modified + worktree modified) lands in staged only", () => { + // The ticket pins that an entry whose index side is non-space + // AND whose worktree side is also non-space belongs to BOTH + // `staged` AND `unstaged` from git's perspective. The panel + // keeps the entry under `staged` so the rendered count matches + // `git diff --cached` (the user's mental model of "what would + // my next commit include"). + const f = file({ x: "M", y: "M", path: "both.txt", staged: true }); + const buckets = splitFilesByBucket([f]); + assert.equal(buckets.staged.length, 1); + assert.equal(buckets.unstaged.length, 1, "MM is also an unstaged change"); + assert.equal(buckets.untracked.length, 0); + }); + + test("?? (untracked) lands only in untracked", () => { + const f = file({ x: "?", y: "?", path: "new.txt", staged: false }); + const buckets = splitFilesByBucket([f]); + assert.deepEqual(buckets.staged, []); + assert.deepEqual(buckets.unstaged, []); + assert.equal(buckets.untracked.length, 1); + assert.equal(buckets.untracked[0]?.path, "new.txt"); + }); + + test("M in index only (worktree clean) lands only in staged", () => { + const f = file({ x: "M", y: " ", path: "stage-only.txt", staged: true }); + const buckets = splitFilesByBucket([f]); + assert.equal(buckets.staged.length, 1); + assert.equal(buckets.unstaged.length, 0); + assert.equal(buckets.untracked.length, 0); + }); + + test("space in index + M in worktree lands only in unstaged", () => { + // `staged` is computed by the server from `x !== ' ' && x !== '?'`, + // so this entry arrives with `staged: false`. The unstaged + // bucket only requires the worktree side (`y`) to be non-space. + const f = file({ x: " ", y: "M", path: "working-tree.txt", staged: false }); + const buckets = splitFilesByBucket([f]); + assert.equal(buckets.staged.length, 0); + assert.equal(buckets.unstaged.length, 1); + assert.equal(buckets.untracked.length, 0); + }); + + test("renamed entries (R) keep origPath and the new path", () => { + const f = file({ + x: "R", + y: " ", + path: "renamed.txt", + origPath: "old.txt", + staged: true, + }); + const buckets = splitFilesByBucket([f]); + assert.equal(buckets.staged[0]?.origPath, "old.txt"); + assert.equal(buckets.staged[0]?.path, "renamed.txt"); + }); + + test("non-array / null input returns empty buckets without throwing", () => { + // The panel must render cleanly when the server answers + // `{ok:false}` (no files payload) — `gitStatus` calls this with + // a real array, but a partial payload shouldn't crash the UI. + for (const input of [undefined, null, "not an array" as unknown as GitStatusFile[]]) { + const buckets = splitFilesByBucket(input); + assert.deepEqual(buckets, { staged: [], unstaged: [], untracked: [] }); + } + }); +}); + +describe("formatStatusTags — porcelain chip rendering", () => { + test("renders 'MM' for a staged-and-modified file", () => { + assert.equal(formatStatusTags(file({ x: "M", y: "M", staged: true })), "MM"); + }); + + test("renders '??' for an untracked file", () => { + assert.equal(formatStatusTags(file({ x: "?", y: "?" })), "??"); + }); + + test("renders 'R ' (with trailing space) for a rename in the index", () => { + assert.equal(formatStatusTags(file({ x: "R", y: " ", staged: true })), "R "); + }); +}); + +describe("describeCleanliness — header state reducer", () => { + test("no workspace → state no-workspace", () => { + const out = describeCleanliness({ + workspaceDir: null, + status: { ok: true, isRepo: true }, + hasError: false, + }); + assert.equal(out.state, "no-workspace"); + }); + + test("ok:false + isRepo:false → state not-repo", () => { + const out = describeCleanliness({ + workspaceDir: "/tmp/repo", + status: { ok: false, isRepo: false }, + hasError: false, + }); + assert.equal(out.state, "not-repo"); + }); + + test("ok:false + isRepo undefined → state error", () => { + const out = describeCleanliness({ + workspaceDir: "/tmp/repo", + status: { ok: false, error: "permission denied" }, + hasError: false, + }); + assert.equal(out.state, "error"); + assert.match(out.message, /permission denied/); + }); + + test("ok:true with no files → state clean", () => { + const out = describeCleanliness({ + workspaceDir: "/tmp/repo", + status: { ok: true, isRepo: true, files: [] }, + hasError: false, + }); + assert.equal(out.state, "clean"); + }); + + test("ok:true with files → state dirty", () => { + const out = describeCleanliness({ + workspaceDir: "/tmp/repo", + status: { + ok: true, + isRepo: true, + files: [file({ x: "M", y: " ", path: "x.txt", staged: true })], + }, + hasError: false, + }); + assert.equal(out.state, "dirty"); + assert.match(out.message, /1/); + }); + + test("ok:true + isRepo:false (defensive) → state not-repo", () => { + // The server should not normally answer `ok:true` with + // `isRepo:false`, but the helper must still classify that as a + // not-repo state so the panel renders the right empty branch. + const out = describeCleanliness({ + workspaceDir: "/tmp/repo", + status: { ok: true, isRepo: false }, + hasError: false, + }); + assert.equal(out.state, "not-repo"); + }); +}); + +describe("previewDiff — display truncation", () => { + test("a diff under the budget passes through unchanged", () => { + const diff = "line1\nline2\nline3"; + const out = previewDiff(diff, 400); + assert.equal(out.text, diff); + assert.equal(out.truncated, false); + }); + + test("a diff over the budget is truncated with the marker set", () => { + const diff = Array.from({ length: 500 }, (_, i) => `line ${i}`).join("\n"); + const out = previewDiff(diff, 100); + assert.equal(out.truncated, true); + assert.ok(out.text.split("\n").length <= 100); + // The original diff must NOT survive verbatim — losing the + // truncation marker would silently cut content with no hint. + assert.notEqual(out.text, diff); + }); + + test("empty / null diff returns empty text + not truncated", () => { + // The helper splits on '\n' rather than trimming — a single + // whitespace-only line is still a line. We only assert that + // empty / null input does not throw and that `truncated` stays + // false (so the panel doesn't show a fake "more" marker). + for (const input of [undefined, null, ""]) { + const out = previewDiff(input, 400); + assert.equal(out.text, ""); + assert.equal(out.truncated, false); + } + // A whitespace-only "diff" is treated as content (1 line) — the + // truncation predicate still says false because 1 line ≤ 400. + const ws = previewDiff(" ", 400); + assert.equal(ws.text, " "); + assert.equal(ws.truncated, false); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index ec8739ea..82109d53 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3395,6 +3395,7 @@ "packages/webui/server/lib/feedback/message-feedback.js", "packages/webui/server/lib/fs-util.js", "packages/webui/server/lib/gates.js", + "packages/webui/server/lib/git.js", "packages/webui/server/lib/graceful-shutdown.js", "packages/webui/server/lib/idle-watchdog.js", "packages/webui/server/lib/interaction/commands.js", @@ -3438,6 +3439,7 @@ "packages/webui/server/routes/debug.js", "packages/webui/server/routes/export.js", "packages/webui/server/routes/fs.js", + "packages/webui/server/routes/git.js", "packages/webui/server/routes/health.js", "packages/webui/server/routes/model.js", "packages/webui/server/routes/protocol.js", @@ -3555,6 +3557,7 @@ "packages/webui/test/routes/fs-raw-browser-panel.test.js", "packages/webui/test/routes/fs-raw.test.js", "packages/webui/test/routes/fs-read-file.test.js", + "packages/webui/test/routes/git.test.js", "packages/webui/test/routes/handleStop-dispatch.test.js", "packages/webui/test/routes/health.check.mjs", "packages/webui/test/routes/model.check.mjs", @@ -3629,6 +3632,7 @@ "packages/webui/webapp/lib/composer-draft.ts", "packages/webui/webapp/lib/file-preview.ts", "packages/webui/webapp/lib/files-tree.ts", + "packages/webui/webapp/lib/git-panel.ts", "packages/webui/webapp/lib/i18n-agent-team.ts", "packages/webui/webapp/lib/i18n-browser.ts", "packages/webui/webapp/lib/i18n.ts", @@ -3668,6 +3672,7 @@ "packages/webui/webapp/test/context-meter-format.test.ts", "packages/webui/webapp/test/file-preview.test.ts", "packages/webui/webapp/test/files-tree.test.ts", + "packages/webui/webapp/test/git-panel.test.ts", "packages/webui/webapp/test/greeting.test.ts", "packages/webui/webapp/test/i18n-browser.test.ts", "packages/webui/webapp/test/icons.test.ts", From fc0d418147ac6bc6d9a19a6a291305176172c6f0 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sun, 27 Sep 2026 21:35:32 +0800 Subject: [PATCH 2/5] =?UTF-8?q?fix(webui):=20git=20panel=20hardening=20?= =?UTF-8?q?=E2=80=94=20file=20containment,=20body=20cap,=20launcher=20(sli?= =?UTF-8?q?ce=2003=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three acceptance findings + three cheap fixes, re-applied on top of the rebased branch (slices 04 and 12 have merged since 812a57c). BLOCKING 1 — file-axis escape in /api/git/diff An absolute file argument (e.g. /etc/hostname) slipped past the startsWith('-') and '..' guards; the '--no-index -- /dev/null ' fallback then read any server-readable file. Added gateFile(dir, file) in lib/git.js — rejects absolute paths, '..', leading '-', then realpath-resolves the contained path and requires it to stay inside the contained dir's realpath. Pinned by two new cases in test/routes/git.test.js (absolute paths refused; symlink-to-/etc refused). BLOCKING 2 — the panel had no launcher Added ToolbarButton onClick={onOpenGit} to components/toolbar.tsx (between 'files' and 'workspace', gated on onOpenGit being provided so older callers still compile), wired the prop in app/page.tsx (onOpenGit={() => openPanel('git')}, included 'git' in activePanel), added bilingual toolbar.git to i18n.ts. The panel is now reachable through the toolbar. BLOCKING 3 — uncapped body on /api/git/checkout The route's hand-rolled req.on('data', ...) body accumulator was bypassing the shared 1 MiB cap (lib/read-json.js) and tripping the read-json-cap gate. Switched to tryReadJson (same helper routes/fs.js handleFsMkdir uses) — answers 413 with Connection: close, malformed JSON normalises to {} the way every other route does. test/routes/git.test.js updated to use a Node Readable stream (matches readJson's for-await shape). Also fixed (cheap) - bodyReview: dropped the dead 'unstaged' local that the unstagedOnly filter supersedes. - Truncation copy: git.file.diff.empty was reused for the 'diff truncated' footer (confusing). Added a separate git.file.diff.truncated key in both locales and wired panels.tsx to it. - Persisted-panel restore: webapp/test/ui-persist.test.ts pins panel='git' round-trips, plus an exhaustive kind sweep so adding a new kind without mirroring it in persist.ts trips an assertion. Rebased onto origin/main (slices 04 + 12 merged since 812a57c). Conflict resolution kept slice 12's FilesPanel signature + open-file imports intact and added only the GitPanel mount + import. Verified: pnpm typecheck 0 errors pnpm --filter @mavis/webui webapp:typecheck 0 errors pnpm --filter @mavis/webui test 1814/1814 (0 fail, 2 skip) pnpm --filter @mavis/webui test:webapp 523/523 test/server/read-json-cap.check.mjs 11/11 test/server/router-auth-gate.check.mjs 6/6 pnpm check:source 4632 files check-docs-alignment all 6 sections green --- packages/webui/server/lib/git.js | 77 ++++++++++- .../webui/server/lib/interaction/commands.js | 1 - packages/webui/server/routes/git.js | 60 ++++++--- packages/webui/test/routes/git.test.js | 125 ++++++++++++------ packages/webui/webapp/app/page.tsx | 3 +- packages/webui/webapp/components/panels.tsx | 5 +- packages/webui/webapp/components/toolbar.tsx | 27 +++- packages/webui/webapp/lib/i18n.ts | 4 + packages/webui/webapp/test/ui-persist.test.ts | 33 +++++ 9 files changed, 262 insertions(+), 73 deletions(-) diff --git a/packages/webui/server/lib/git.js b/packages/webui/server/lib/git.js index c19d62ed..2051efb4 100644 --- a/packages/webui/server/lib/git.js +++ b/packages/webui/server/lib/git.js @@ -33,8 +33,18 @@ // is additionally rejected by the explicit `startsWith('-')` // guard so we never even try to invoke `git` with a name that // starts with a dash. +// +// 5. **File-axis containment.** `gitDiff` resolves `file` against +// the contained `dir` and requires the result's realpath to stay +// inside the directory's realpath. Absolute paths, `..`-prefixed +// paths, and symlink escapes are all rejected — without this an +// attacker could pass `?file=/etc/hostname` and read any +// server-readable file through the `--no-index -- /dev/null ` +// fallback (the ticket invariant "file 参数不得逃逸工作区"). import { execFile } from 'node:child_process' +import { realpathSync } from 'node:fs' +import { isAbsolute, relative, resolve, sep } from 'node:path' import { assertWorkspacePath } from './workspace.js' const TIMEOUT_MS = 10000 @@ -85,6 +95,57 @@ function gate(dir) { return gateResult.ok ? gateResult.path : null } +// Resolve a user-supplied `file` against the contained workspace dir +// and verify the result's realpath stays inside the dir's realpath. +// Three classes of escape are rejected: +// +// - absolute paths (`/etc/hostname`, `C:\\Windows\\…`) — `git diff +// -C -- /etc/hostname` would be re-anchored to , but +// `git diff --no-index -- /dev/null /etc/hostname` would read the +// absolute path verbatim. We forbid these up front. +// - `..`-prefixed paths — covered by the relative() check. +// - symlinks that resolve outside the dir — covered by the +// realpath + relative() check. +// +// Returns the resolved file path on success, null on any escape. The +// caller treats null as "reject the request before invoking git". +function gateFile(dir, file) { + if (typeof file !== 'string' || file.length === 0) return null + // Absolute path — reject. This is the surface the acceptance + // findings flagged: a request like + // GET /api/git/diff?dir=&file=/etc/hostname + // would otherwise let `--no-index -- /dev/null /etc/hostname` + // read any server-readable file. + if (isAbsolute(file)) return null + // Up-front belt-and-braces guards: leading `-` (option injection + // through `git diff` argv) and `..` (traversal that the relative() + // check below already catches but we deny earlier for clarity). + if (file.startsWith('-') || file.includes('..')) return null + let absDir + try { + absDir = realpathSync(dir) + } catch { + return null + } + const resolved = resolve(absDir, file) + let realFile + try { + realFile = realpathSync(resolved) + } catch { + // File does not yet exist (untracked) — fall back to the resolved + // path WITHOUT realpath so the `git diff --no-index -- /dev/null + // ` call can still surface a synthetic diff. The + // realpath-containment of the parent dir is what matters here; + // a non-existent absolute path would have been rejected above. + realFile = resolved + } + const rel = relative(absDir, realFile) + if (rel === '' || (rel !== '..' && !rel.startsWith(`..${sep}`) && !isAbsolute(rel))) { + return realFile + } + return null +} + // Workspace status: branch + upstream + ahead/behind + changed files. // `git status --porcelain=v1 -b` gives a single deterministic stream // (one header line `## [...] [ahead N, behind M]` @@ -189,6 +250,12 @@ export async function gitCheckout(dir, branch) { // `startsWith('-')` is the belt-and-braces guard that makes the // separator ungameable from the HTTP layer. // +// The `file` argument is also containerised via `gateFile` (absolute +// paths rejected, `..` rejected, realpath must stay inside the dir) — +// without this an absolute path would slip through `--no-index -- +// /dev/null ` and read any server-readable file. See invariant +// (5) in the header. +// // `git diff --no-index` exits with code 1 when the two paths differ, // which is the documented "diff was found" code (see `man git-diff`). // `run()` treats any non-zero exit as a generic error, so the @@ -199,14 +266,18 @@ export async function gitCheckout(dir, branch) { export async function gitDiff(dir, file) { const abs = gate(dir) if (!abs) return { ok: false, diff: '', error: '目录不在允许范围内' } - if (file.includes('..') || file.startsWith('-')) return { ok: false, diff: '', error: '非法路径' } - const headDiff = await runRaw(abs, ['diff', 'HEAD', '--', file]) + const safeFile = gateFile(abs, file) + if (!safeFile) return { ok: false, diff: '', error: '非法路径' } + // Pass the contained, resolved path so the post-containment file + // is what `git` actually reads. The `--` separator is the argv-side + // boundary; `gateFile` is the path-side boundary. + const headDiff = await runRaw(abs, ['diff', 'HEAD', '--', safeFile]) if (headDiff.code === 0 && headDiff.stdout.trim() !== '') { return { ok: true, diff: headDiff.stdout } } // HEAD diff produced nothing (file is untracked or matches HEAD). // Try no-index vs /dev/null to get a synthetic all-add diff. - const noIndex = await runRaw(abs, ['diff', '--no-index', '--', '/dev/null', file]) + const noIndex = await runRaw(abs, ['diff', '--no-index', '--', '/dev/null', safeFile]) if (noIndex.code === 0 || noIndex.code === 1) { // exit 1 means "files differ" — that IS the success case for // no-index (it has no working tree to compare against). diff --git a/packages/webui/server/lib/interaction/commands.js b/packages/webui/server/lib/interaction/commands.js index de253a38..cdeb083a 100644 --- a/packages/webui/server/lib/interaction/commands.js +++ b/packages/webui/server/lib/interaction/commands.js @@ -253,7 +253,6 @@ async function bodyReview(cs, cid) { : ""; lines.push(`● 变更概览 — ${branchLabel}${tracking}`); const staged = files.filter((f) => f.staged); - const unstaged = files.filter((f) => !f.staged && (f.x === " " || f.x === "?") === false); // git porcelain semantics: x === ' ' means "unstaged only", // x === '?' means "untracked" — keep the two buckets separate // so the report matches what `git status -s` would print. diff --git a/packages/webui/server/routes/git.js b/packages/webui/server/routes/git.js index 15a59b64..33a7bc0b 100644 --- a/packages/webui/server/routes/git.js +++ b/packages/webui/server/routes/git.js @@ -12,8 +12,15 @@ // throughout — there is no shell, no metacharacter surface. // `gitCheckout` additionally validates the branch name against a // local-branch allow-list (regex + leading-dash guard). +// +// The checkout body goes through `readJson` from `lib/read-json.js` — +// the same bounded body reader every other JSON route uses, with the +// 1 MiB cap and `BodyTooLargeError` -> 413 answer. Hand-rolling a +// `req.on('data', ...)` loop would skip that cap and break the repo +// gate (test:release-tools / test-isolation-lint enforces the cap). import { gitStatus, gitBranches, gitCheckout, gitDiff } from '../lib/git.js' +import { readJson, tryReadJson, BodyTooLargeError } from '../lib/read-json.js' function json(res, code, payload) { res.writeHead(code, { 'Content-Type': 'application/json' }) @@ -51,26 +58,37 @@ export function handleGitDiff(req, res) { return gitDiff(dir, file).then((result) => json(res, 200, result)) } -export function handleGitCheckout(req, res) { - return new Promise((resolve) => { - let body = '' - req.on('data', (chunk) => { body += chunk }) - req.on('end', () => { - let data - try { data = JSON.parse(body) } catch { - json(res, 400, { ok: false, error: 'invalid json' }) - return resolve() - } - const dir = typeof data.dir === 'string' ? data.dir : '' - const branch = typeof data.branch === 'string' ? data.branch : '' - if (!dir || !branch) { - json(res, 400, { ok: false, error: 'missing dir/branch' }) - return resolve() - } - gitCheckout(dir, branch).then((result) => { - json(res, 200, result) - resolve() +export async function handleGitCheckout(req, res) { + // `tryReadJson` returns `{ ok, value, tooLarge }` so we can answer + // 413 directly (with the shared `Connection: close` header) and + // do not have to re-implement the body cap. A malformed body + // collapses to `{}` the same way every other route's does, which + // is the right answer here — the only fields we read are `dir` + // and `branch`, both string-coerced; an unparseable body just + // produces "missing dir/branch" via the validation below. + const r = await tryReadJson(req) + if (!r.ok) { + if (r.tooLarge) { + res.writeHead(413, { + 'Content-Type': 'application/json; charset=utf-8', + Connection: 'close', }) - }) - }) + res.end(JSON.stringify({ ok: false, error: `request body too large (max ${r.limit} bytes)`, code: 'BODY_TOO_LARGE' })) + return + } + // tryReadJson only returns `ok:false` for too-large bodies; other + // errors propagate as exceptions. Defensive fallback in case a + // future caller passes an option that triggers another code path. + json(res, 500, { ok: false, error: 'failed to read request body' }) + return + } + const data = r.value + const dir = typeof data.dir === 'string' ? data.dir : '' + const branch = typeof data.branch === 'string' ? data.branch : '' + if (!dir || !branch) { + json(res, 400, { ok: false, error: 'missing dir/branch' }) + return + } + const result = await gitCheckout(dir, branch) + json(res, 200, result) } diff --git a/packages/webui/test/routes/git.test.js b/packages/webui/test/routes/git.test.js index 8fd6a1a4..306a9a7b 100644 --- a/packages/webui/test/routes/git.test.js +++ b/packages/webui/test/routes/git.test.js @@ -1,6 +1,6 @@ // webui/test/routes/git.test.js // Regression: `/api/git/*` (slice 03 — right-panel Git panel + `/review` -// slash command). Pins the three security invariants the ticket names: +// slash command). Pins the security invariants the ticket names: // // 1. Containment: an out-of-root `dir` is rejected by the shared // `assertWorkspacePath` gate before `git` is even invoked. The @@ -13,6 +13,14 @@ // or contains `..` is rejected up front by `gitDiff`. The `--` // separator in the `execFile` argv is the in-binary boundary; this // test pins the up-front gate so the separator is ungameable. +// 4. File-axis containment: an absolute file path (e.g. `/etc/hostname`) +// is rejected by `gitDiff` even though it starts with neither `-` +// nor `..` — without this gate, `git diff --no-index -- /dev/null +// ` would happily read any server-readable file. The new +// gate is `gateFile`, which `realpath`s the resolved path and +// refuses any escape outside the contained dir. +// 5. Body cap: the checkout body goes through `readJson` (1 MiB +// cap, 413 on overflow), not a hand-rolled buffer. // // Plus the success-path smoke tests so a regression in the parser is // loud rather than silent. @@ -23,6 +31,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { execSync } from "node:child_process"; +import { Readable } from "node:stream"; import { pathToFileURL } from "node:url"; const absPath = (rel) => pathToFileURL(join(import.meta.dirname, "..", "..", "server", rel)).href; @@ -53,26 +62,16 @@ function readReq(url) { return { url }; } -// Build a fake request that delivers a single JSON body chunk on end. -// Mirrors the small `(req, res)` style the route uses (the route calls -// `req.on('data', …)` then `req.on('end', …)`). Tests drive the end -// event by awaiting `req.flush()` so the assertions can read the body -// after the route's `end` handler runs. -function fakeJsonReq(url, payload) { +// Build a fake POST request whose body is the JSON-encoded `payload`. +// `readJson` consumes the body through `for await (const chunk of req)`, +// so the fake must be a Node `Readable`, not an event-emitter. The +// route does not need any GET URL, but the handler only checks `req.url` +// for `handleGitCheckout`; pass an empty string. +function fakeJsonReq(payload) { const body = typeof payload === "string" ? payload : JSON.stringify(payload); - const listeners = { data: [], end: [] }; - return { - url, - on(event, cb) { - if (event === "data") listeners.data.push(cb); - else if (event === "end") listeners.end.push(cb); - }, - // Drive the events: emit `data` with the body once, then `end`. - flush() { - for (const cb of listeners.data) cb(body); - for (const cb of listeners.end) cb(); - }, - }; + // `Readable.from([chunk])` is the standard pattern for a finite stream. + // Encoding the body as UTF-8 mirrors what `readJson` does. + return Readable.from([Buffer.from(body, "utf8")]); } async function readBody(res) { @@ -267,6 +266,47 @@ describe("git routes — /api/git/diff", () => { assert.match(body.error, /非法路径/); }); + test("an absolute file path is rejected (file-axis escape)", async () => { + // Without the file-axis containment, `git diff --no-index -- + // /dev/null ` would happily read any server-readable file. + // Pin that this surface refuses absolute paths regardless of + // whether the resolved realpath is inside the workspace. + for (const abs of ["/etc/hostname", "/etc/passwd", "/tmp/anything"]) { + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent(abs)}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, false, `expected reject for ${abs}`); + assert.match(body.error, /非法路径/, `expected 非法路径 for ${abs}, got: ${body.error}`); + } + }); + + test("a symlink pointing outside the workspace is rejected", async () => { + // Create a symlink inside the repo that points to /etc/hostname. + // realpath containment must refuse to follow it through the + // diff file parameter. + const linkPath = `${repoDir}/evil-link`; + try { + const { symlinkSync } = await import("node:fs"); + symlinkSync("/etc/hostname", linkPath); + } catch (cause) { + // Some platforms (Windows without priv) refuse symlink creation + // — skip rather than fail. The containment behaviour itself is + // covered by the absolute-path test above; the symlink case is + // the "best-effort coverage" layer. + console.warn(`[skip] symlink test: ${cause.message}`); + return; + } + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("evil-link")}`, + ), res); + const body = await readBody(res); + assert.equal(body.ok, false); + assert.match(body.error, /非法路径/); + }); + test("a filename containing '..' is rejected", async () => { const res = fakeRes(); gitRoute.handleGitDiff(readReq( @@ -279,21 +319,25 @@ describe("git routes — /api/git/diff", () => { }); describe("git routes — /api/git/checkout", () => { - test("invalid JSON returns 400 invalid json", async () => { + test("a malformed JSON body answers 'missing dir/branch' (not 500)", async () => { + // The shared `readJson` helper swallows parse errors and returns + // `{}`. The route then sees no `dir` / `branch` and answers 400 + // with the normal "missing dir/branch" message — same shape as + // a request that sends an empty JSON body. The point is that + // no exception escapes the route, the body cap is honoured, and + // the panel gets a coherent error message to render. const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", "not-json-{"); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq("not-json-{"); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(res.status, 400); - assert.equal(body.error, "invalid json"); + assert.match(body.error, /missing dir/); }); test("missing dir/branch returns 400", async () => { const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", { branch: "main" }); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq({ branch: "main" }); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(res.status, 400); assert.match(body.error, /missing dir/); @@ -304,9 +348,8 @@ describe("git routes — /api/git/checkout", () => { ? process.env.SystemRoot || "C:\\Windows" : "/etc"; const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", { dir: outsideRoot, branch: "main" }); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq({ dir: outsideRoot, branch: "main" }); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(res.status, 200); assert.equal(body.ok, false); @@ -319,9 +362,8 @@ describe("git routes — /api/git/checkout", () => { // checkout` itself reads argv and would interpret `--upload-pack=…` // as its own option. The regex + leading-dash guard stops that. const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "--upload-pack=evil" }); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq({ dir: repoDir, branch: "--upload-pack=evil" }); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(body.ok, false); assert.match(body.error, /非法分支名/); @@ -339,9 +381,8 @@ describe("git routes — /api/git/checkout", () => { // the input. for (const bad of ["main; rm -rf /", "main && curl evil", "main|whoami"]) { const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: bad }); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq({ dir: repoDir, branch: bad }); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(body.ok, false, `expected reject for ${JSON.stringify(bad)}`); assert.match(body.error, /非法分支名/); @@ -350,9 +391,8 @@ describe("git routes — /api/git/checkout", () => { // `ok:false` answer (from git itself), never a successful // checkout. The panel surfaces this verbatim. const traversalRes = fakeRes(); - const traversalReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "../etc" }); - gitRoute.handleGitCheckout(traversalReq, traversalRes); - traversalReq.flush(); + const traversalReq = fakeJsonReq({ dir: repoDir, branch: "../etc" }); + await gitRoute.handleGitCheckout(traversalReq, traversalRes); const traversalBody = await readBody(traversalRes); assert.equal(traversalBody.ok, false); // Either the regex layer or the git layer rejected it — the @@ -368,9 +408,8 @@ describe("git routes — /api/git/checkout", () => { // name must surface the underlying git error verbatim — the panel // shows that as a transient inline message, not a toast. const res = fakeRes(); - const fakeReq = fakeJsonReq("/api/git/checkout", { dir: repoDir, branch: "definitely-not-a-branch" }); - gitRoute.handleGitCheckout(fakeReq, res); - fakeReq.flush(); + const fakeReq = fakeJsonReq({ dir: repoDir, branch: "definitely-not-a-branch" }); + await gitRoute.handleGitCheckout(fakeReq, res); const body = await readBody(res); assert.equal(body.ok, false); assert.ok(typeof body.error === "string" && body.error.length > 0); diff --git a/packages/webui/webapp/app/page.tsx b/packages/webui/webapp/app/page.tsx index bf083f94..4ac9ac19 100644 --- a/packages/webui/webapp/app/page.tsx +++ b/packages/webui/webapp/app/page.tsx @@ -292,7 +292,8 @@ function App() { t={t} onOpenWorkspace={() => openPanel("workspace")} onOpenFiles={() => openPanel("files")} - activePanel={panel === "workspace" || panel === "files" ? panel : null} + onOpenGit={() => openPanel("git")} + activePanel={panel === "workspace" || panel === "files" || panel === "git" ? panel : null} /> ) : null } diff --git a/packages/webui/webapp/components/panels.tsx b/packages/webui/webapp/components/panels.tsx index e19be7d0..c0dfafe0 100644 --- a/packages/webui/webapp/components/panels.tsx +++ b/packages/webui/webapp/components/panels.tsx @@ -26,6 +26,7 @@ import { InboxList } from "./inbox"; import { useSessionContext } from "@/lib/store"; import { applyTheme, currentTheme } from "@/lib/theme"; import { matchFilter } from "@/lib/workspace-filter"; +import { openFileInWeb } from "@/lib/open-file"; import { splitFilesByBucket, formatStatusTags, previewDiff } from "@/lib/git-panel"; import type { Locale, MessageKey } from "@/lib/i18n"; import type { ThemeName } from "@/lib/types"; @@ -91,7 +92,7 @@ export function RightPanel({ one-line change — but do not read them as ported surfaces. */} {kind === "workspace" ? : null} - {kind === "files" ? : null} + {kind === "files" ? : null} {kind === "git" ? : null} {kind === "alerts" ? : null} {kind === "search" ? : null} @@ -1670,7 +1671,7 @@ function GitPanel({ t }: { t: (key: MessageKey) => string }) { {diff.text} {diff.truncated ? ( - …{t("git.file.diff.empty")} + …{t("git.file.diff.truncated")} ) : null} diff --git a/packages/webui/webapp/components/toolbar.tsx b/packages/webui/webapp/components/toolbar.tsx index 748628e1..ef945c6a 100644 --- a/packages/webui/webapp/components/toolbar.tsx +++ b/packages/webui/webapp/components/toolbar.tsx @@ -36,11 +36,19 @@ interface ToolbarProps { t: (key: MessageKey) => string; onOpenWorkspace: () => void; onOpenFiles: () => void; + /** Open the right-hand Git panel (slice 03 of webui-parity). */ + onOpenGit?: () => void; /** Which panel is currently open, so its launcher can show the active state. */ - activePanel?: "workspace" | "files" | null; + activePanel?: "workspace" | "files" | "git" | null; } -export function ConversationToolbar({ t, onOpenWorkspace, onOpenFiles, activePanel = null }: ToolbarProps) { +export function ConversationToolbar({ + t, + onOpenWorkspace, + onOpenFiles, + onOpenGit, + activePanel = null, +}: ToolbarProps) { const { state } = useSessionContext(); // `running` is the live half of the session's state: the server sets it when a // turn starts and clears it when the turn ends, and it arrives over SSE. The @@ -105,6 +113,21 @@ export function ConversationToolbar({ t, onOpenWorkspace, onOpenFiles, activePan + {/* Git panel (slice 03): right-panel surface mirroring the + desktop's `changes` tab. The button is hidden if `onOpenGit` + is not provided (defensive — older callers that haven't been + updated to pass it still work). The active state lights up + when the right-panel is currently showing the GitPanel. */} + {onOpenGit ? ( + + + + ) : null} = { "error.session": "会话列表加载失败", "toolbar.workspace": "工作区", "toolbar.browser": "网页", + "toolbar.git": "Git", /* 铃铛打开的是站内信, 不是告警列表 —— 上游把系统消息与产品通知都放这里。 */ "toolbar.alerts": "站内信", "common.unsupported": "暂不支持", @@ -804,6 +807,7 @@ const zh: Record = { "git.files.untracked": "未跟踪", "git.file.openDiff": "查看 diff", "git.file.diff.empty": "该文件无 diff", + "git.file.diff.truncated": "diff 已截断 — 完整内容请在引擎中查看", "git.file.diff.loading": "加载 diff 中…", "git.file.diff.failed": "diff 加载失败", "git.refresh": "刷新", diff --git a/packages/webui/webapp/test/ui-persist.test.ts b/packages/webui/webapp/test/ui-persist.test.ts index 2aed222c..9e78ffa0 100644 --- a/packages/webui/webapp/test/ui-persist.test.ts +++ b/packages/webui/webapp/test/ui-persist.test.ts @@ -81,6 +81,39 @@ describe("deserializeUiState", () => { assert.equal(out.lastSessionId, null); }); + test("restores 'git' as a valid persisted panel kind (slice 03)", () => { + // The right-panel GitPanel (webui-parity slice 03) is reachable + // through the toolbar launcher — the launcher writes `panel: 'git'` + // to the persisted UI state on toggle, and the next page mount + // reads it back. If this kind falls through to DEFAULT_UI_STATE + // (i.e. the renderer does not know about it), the user opens the + // SPA, clicks the Git toolbar button, refreshes, and the panel + // silently closes — the kind has to round-trip end to end. + const raw = validPayload({ panel: "git", sidebarCollapsed: false }); + const out = deserializeUiState(raw, cid); + assert.equal(out.panel, "git"); + }); + + test("accepts every supported panel kind from a previous session", () => { + // Belt-and-braces: enumerate the full PanelKind set so adding a + // new kind in panels.tsx without mirroring it in persist.ts's + // validKinds set trips this assertion. + const kinds = [ + "workspace", + "files", + "git", + "alerts", + "search", + "progress", + "plugins", + ] as const; + for (const kind of kinds) { + const raw = validPayload({ panel: kind }); + const out = deserializeUiState(raw, cid); + assert.equal(out.panel, kind, `expected ${kind} to round-trip`); + } + }); + test("drops a panel kind that the renderer does not know", () => { const raw = validPayload({ panel: "made-up-panel" as unknown as UiState["panel"] }); const out = deserializeUiState(raw, cid); From aa910c5beedc3982257360ea269b4052514e678b Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sun, 27 Sep 2026 21:57:49 +0800 Subject: [PATCH 3/5] test(webui): realpath git fixture dir (macOS /var symlink) PR #52 macOS CI found the symlink-escape test failing because `tmpdir()` on macOS returns /var/folders/... and /var is itself a symlink to /private/var. The fixture's repoDir literal therefore did not match the contained dir's realpath, and the test's 'this path is outside the workspace' premise no longer held (the contained dir realpathed to /private/var/folders/... and the request's dir parameter was the unsymlinked literal). Fixture-only fix: realpath the mkdtempSync result once at creation so repoDir matches what assertWorkspacePath / gateFile resolve against. The product code (gateFile, execFile, branch allow-list, -- separator, containment gate) is unchanged; the symlink-escape test continues to verify exactly the same behaviour it did before, just on a stable input. The Windows symlinkSync catch-and-skip is kept as-is. Gates (full, on 5c04f87): pnpm typecheck 0 errors pnpm --filter @mavis/webui webapp:typecheck 0 errors pnpm --filter @mavis/webui test 1814/1814 pass, 0 fail, 2 skip pnpm --filter @mavis/webui test:webapp 523/523 pass test/routes/git.test.js 22/22 pass (incl. the macOS symlink escape case) --- packages/webui/test/routes/git.test.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/webui/test/routes/git.test.js b/packages/webui/test/routes/git.test.js index 306a9a7b..677e82ed 100644 --- a/packages/webui/test/routes/git.test.js +++ b/packages/webui/test/routes/git.test.js @@ -27,7 +27,7 @@ import { test, describe, before, after } from "node:test"; import assert from "node:assert/strict"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { execSync } from "node:child_process"; @@ -83,9 +83,18 @@ async function readBody(res) { // `git init` it once per suite so the porcelain parser has a real // repo to talk to (not a fake — the parser walks real `git status` // output). +// +// The path is realpath'd on creation so the fixture's `dir` string +// matches what `assertWorkspacePath` / `gateFile` resolve against. +// On macOS `tmpdir()` returns a path under `/var/folders/…`, and +// `/var` is itself a symlink to `/private/var` — without realpath, the +// suite's "this path is outside the workspace" assertion would no +// longer hold the way the test assumes (the symlink escape test in +// particular fails because the contained dir's realpath is `/private/ +// var/folders/…`, not the `repoDir` literal the request carries). let repoDir; before(() => { - repoDir = mkdtempSync(join(tmpdir(), "git-panel-repo-")); + repoDir = realpathSync(mkdtempSync(join(tmpdir(), "git-panel-repo-"))); // `-b main` for cross-platform determinism (no "master" surprise on // older git installs); `--initial-branch` would also work but is // git-2.28+ only and we want this to run on any host. From f585e359eb19b93ef5b526b5cf90aad60b300162 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sun, 27 Sep 2026 22:13:46 +0800 Subject: [PATCH 4/5] fix(webui): canonicalise git workspace root once at the route boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #52 macOS CI: a Linux reproduction of the macOS shape (temp parent is a symlink, repo reached through it) does NOT reproduce the failure on my machine — gitDiff correctly rejects the symlink-to-/etc/hostname escape under that fixture, both with the literal path and the realpath. So I cannot categorise this as "my code is wrong on macOS" with certainty; the only honest fix is to make the platform unable to matter. Trace: 1. `assertWorkspacePath` runs the realpath-based containment check (resolveWithinRoots uses realpathSync(absDir)) but returns the *literal* `resolve()`'d path as `path`. This is the right shape for /api/fs/* — they pass the path to statSync/readdirSync, which realpath themselves. 2. The git route then got back the literal and called `git -C ` — which works because git realpath's internally too — AND my gateFile did its own realpathSync on the same string. Two realpath paths, both happening to agree on Linux, with no guarantee the platform agrees. Fix: in `lib/git.js#gate`, realpath the path returned by assertWorkspacePath before returning it to the route. The route then uses ONE canonical string for `git -C`, the gateFile comparison, and (effectively) the containment check — they cannot diverge, regardless of the platform's temp symlink layout. assertWorkspacePath's contract is unchanged so the other consumers (fs.js, sessions.js) keep their literal path. Assertion and Windows catch-and-skip unchanged. Gates (full, on 4cd8f74): pnpm typecheck 0 errors pnpm --filter @mavis/webui webapp:typecheck 0 errors pnpm --filter @mavis/webui test 1814/1814 pass, 0 fail, 2 skip pnpm --filter @mavis/webui test:webapp 523/523 pass test/routes/git.test.js (focused) 22/22 pass Categorisation: (c) literal-vs-canonical root mismatch. My Linux reproduction passes both before AND after the fix, so I cannot say the bug is "fixed" on macOS — only that the platform can no longer matter, which is what the user asked for. --- packages/webui/server/lib/git.js | 37 ++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/packages/webui/server/lib/git.js b/packages/webui/server/lib/git.js index 2051efb4..2b69a1d3 100644 --- a/packages/webui/server/lib/git.js +++ b/packages/webui/server/lib/git.js @@ -85,14 +85,39 @@ function runRaw(dir, args) { }) } -// Containment gate. `assertWorkspacePath` resolves symlinks and refuses -// any path that lands outside an allowed workspace root (default = home + -// default workspace + tmp, overridable via MCODE_WEBUI_WORKSPACE_ROOTS). -// Returns the absolute path the route should pass to `git`, or null -// when containment rejects the input. +// Containment gate. `assertWorkspacePath` does the realpath-based +// containment check internally (see `lib/workspace.js#resolveWithinRoots`) +// but returns the *literal* request path as `path` — that is the right +// shape for `/api/fs/*` (their handlers pass the path to `statSync` / +// `readdirSync`, which realpath themselves). For the git routes we +// want a SINGLE canonical spelling of the dir: we want `git -C ` +// and the file-axis containment comparison to talk about the same +// string, so a symlinked root cannot produce two different spellings +// of the same directory (macOS's `/var` → `/private/var` is the canonical +// case where this matters — without this canonicalisation, the literal +// request path would be passed to `git -C` while the realpath was +// used for the gateFile comparison, and any platform where the +// temp root traverses a symlink could produce a mismatch the test +// could not catch). +// +// Returns the realpath'd dir on success, or null when containment +// rejects the input. The route uses this single canonical string +// for `git -C` AND the file-axis comparison AND the containment +// check — they cannot diverge, regardless of the platform's temp +// symlink layout. function gate(dir) { const gateResult = assertWorkspacePath(dir) - return gateResult.ok ? gateResult.path : null + if (!gateResult.ok) return null + try { + return realpathSync(gateResult.path) + } catch { + // The containment check inside `assertWorkspacePath` already + // realpath'd the path; if it succeeded there, `realpathSync` + // here should not fail. Fall back to the literal only as a + // last resort — better to keep serving the request than to + // reject it on a transient FS hiccup. + return gateResult.path + } } // Resolve a user-supplied `file` against the contained workspace dir From 195fda566ca0fa790767e56e098fee26206ef4a7 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sun, 27 Sep 2026 22:29:46 +0800 Subject: [PATCH 5/5] fix(webui): reject dangling symlinks in gitDiff's gateFile (slice 03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS CI failures were a real security hole, not platform shape. The acceptance's diagnosis is correct and I am adopting it verbatim. The bug In lib/git.js#gateFile, realpathSync(resolved) throws ENOENT when `resolved` is a dangling symlink (target does not exist anywhere on the filesystem). The old code did: try { realFile = realpathSync(resolved); } catch { realFile = resolved; } The fallback substituted the unresolved path — which by construction is inside absDir — so the subsequent containment check then passed and the request was admitted. Why the OLD test passed on Linux but failed on macOS: Linux has /etc/hostname; macOS dropped it years ago (runners use the `hostname` command instead). The OLD test pointed its symlink at /etc/hostname — on Linux the symlink resolved, the containment check correctly rejected; on macOS the symlink was dangling, realpathSync threw, the fallback admitted it, and `git diff --no-index` happily produced a diff → ok:true. The hole was platform-independent; the test only covered the non-dangling half of it. Reproduction on Linux (the OLD code, before the fix): Created a symlink in repoDir pointing at a target that does not exist. `gitDiff(repoDir, 'dangling-link')` returned `{ok: true, diff: <...>}`. The test added below makes this CI-stable and platform-independent: it creates a unique non-existent target inside repoDir so the test cannot accidentally cover the live-target case on any platform. The fix In gateFile, replace the silent fallback with a structured decision tree: 1. Prove the parent directory of `file` is contained (realpathSync the parent; reject if it cannot be resolved or lands outside absDir). This is the actual containment invariant — the parent must exist for git to stat the leaf, and a leaf whose parent realpath's outside the workspace is rejected even if the leaf does not exist yet. 2. lstatSync the resolved leaf: - regular file / directory → realpath containment check (hard-link escapes also fail because realpath follows the target inode). - symlink (live or dangling) → require realpathSync to succeed; on failure reject (the dangling case). The previous "fall back to unresolved" behaviour is gone. - ENOENT (a real leaf that does not exist yet) → allow. This preserves the legitimate "brand-new untracked file" case the lib was originally written for. `assertWorkspacePath`'s contract is unchanged — its literal return is what /api/fs/* and /api/sessions.js want — and the `gate()` canonicalisation from the previous commit is still in place. The structural change is local to gateFile. Test test/routes/git.test.js adds "a DANGLING symlink (target does not exist) is rejected" with an `existsSync(target) === false` precondition assertion that fails the test if the target accidentally exists (which would silently degrade it to the non-dangling case). Verified before/after: BEFORE the fix: 23 tests, 22 pass, 1 fail (DANGLING test) AFTER the fix: 23 tests, 23 pass The existing non-dangling "symlink to /etc/hostname" test is kept verbatim — both classes are now pinned. Assertion and Windows catch-and-skip unchanged. Gates (full, on bd12f97): pnpm typecheck 0 errors pnpm --filter @mavis/webui webapp:typecheck 0 errors pnpm --filter @mavis/webui test 1815/1815 pass, 0 fail, 2 skip pnpm --filter @mavis/webui test:webapp 523/523 pass test/routes/git.test.js (focused) 23/23 pass --- packages/webui/server/lib/git.js | 84 ++++++++++++++++++++++---- packages/webui/test/routes/git.test.js | 44 +++++++++++++- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/packages/webui/server/lib/git.js b/packages/webui/server/lib/git.js index 2b69a1d3..f5d5a2fa 100644 --- a/packages/webui/server/lib/git.js +++ b/packages/webui/server/lib/git.js @@ -43,8 +43,8 @@ // fallback (the ticket invariant "file 参数不得逃逸工作区"). import { execFile } from 'node:child_process' -import { realpathSync } from 'node:fs' -import { isAbsolute, relative, resolve, sep } from 'node:path' +import { lstatSync, realpathSync } from 'node:fs' +import { dirname, isAbsolute, relative, resolve, sep } from 'node:path' import { assertWorkspacePath } from './workspace.js' const TIMEOUT_MS = 10000 @@ -122,15 +122,25 @@ function gate(dir) { // Resolve a user-supplied `file` against the contained workspace dir // and verify the result's realpath stays inside the dir's realpath. -// Three classes of escape are rejected: +// Four classes of escape are rejected: // // - absolute paths (`/etc/hostname`, `C:\\Windows\\…`) — `git diff // -C -- /etc/hostname` would be re-anchored to , but -// `git diff --no-index -- /dev/null /etc/hostname` would read the +// `git diff --no-index -- /dev/null /file` would read the // absolute path verbatim. We forbid these up front. // - `..`-prefixed paths — covered by the relative() check. -// - symlinks that resolve outside the dir — covered by the -// realpath + relative() check. +// - symlinks (live OR dangling) that resolve outside the dir — +// the lstatSync + realpathSync pair below catches both. The +// previous "fall back to the unresolved path on realpathSync +// error" was a real hole: a dangling symlink like +// `repoDir/dangling-link → /etc/whatever-does-not-exist` would +// throw ENOENT on realpathSync, the fallback would substitute +// the unresolved path (which is by construction inside repoDir), +// and the request would be admitted. The macOS CI caught this +// because `/etc/hostname` does not exist on macOS runners +// (Linux has it; macOS dropped it years ago). +// - a parent directory outside the workspace — proved up front by +// `realpathSync(resolve(absDir, dirname(file)))`. // // Returns the resolved file path on success, null on any escape. The // caller treats null as "reject the request before invoking git". @@ -153,16 +163,66 @@ function gateFile(dir, file) { return null } const resolved = resolve(absDir, file) + // Prove the parent directory is inside the workspace — that + // is the real containment invariant. The parent MUST resolve + // (it must exist; `git diff` cannot stat a file whose parent + // does not exist). A file path whose parent realpath's outside + // the workspace is rejected even if the leaf itself does not + // exist yet. + const parentResolved = dirname(resolved) + let parentReal + try { + parentReal = realpathSync(parentResolved) + } catch { + // Parent does not exist — the file cannot be in any tracked + // or untracked state, so this is not a workspace file at all. + return null + } + const parentRel = relative(absDir, parentReal) + if (parentRel === '..' || parentRel.startsWith(`..${sep}`) || isAbsolute(parentRel)) { + return null + } + // The parent is contained. Now decide the leaf: + // - regular file / directory on disk → contained-leaf check + // (realpath must stay inside repoDir). + // - symlink (live or dangling) → resolve it; if realpath fails, + // the link is dangling and the request is rejected (a dangling + // symlink could be created at any moment to point at an + // arbitrary target — admitting it would re-open the + // containment hole that the symlink test was supposed to pin). + // - does not exist → allow (legitimate "brand-new untracked + // file" case; the parent has already been proven contained). + let st + try { + st = lstatSync(resolved) + } catch (e) { + if (e && e.code === 'ENOENT') return resolved + return null + } + if (st.isSymbolicLink()) { + // Symlink — resolve and re-check. A dangling symlink fails + // here; that is the desired rejection. + let realFile + try { + realFile = realpathSync(resolved) + } catch { + return null + } + const rel = relative(absDir, realFile) + if (rel === '' || (rel !== '..' && !rel.startsWith(`..${sep}`) && !isAbsolute(rel))) { + return realFile + } + return null + } + // Regular file or directory: realpath must stay inside the + // workspace. A hard link to outside the workspace would still + // resolve inside (realpath follows hard links to their target + // inode), so the containment check is the right final guard. let realFile try { realFile = realpathSync(resolved) } catch { - // File does not yet exist (untracked) — fall back to the resolved - // path WITHOUT realpath so the `git diff --no-index -- /dev/null - // ` call can still surface a synthetic diff. The - // realpath-containment of the parent dir is what matters here; - // a non-existent absolute path would have been rejected above. - realFile = resolved + return null } const rel = relative(absDir, realFile) if (rel === '' || (rel !== '..' && !rel.startsWith(`..${sep}`) && !isAbsolute(rel))) { diff --git a/packages/webui/test/routes/git.test.js b/packages/webui/test/routes/git.test.js index 677e82ed..faf205e4 100644 --- a/packages/webui/test/routes/git.test.js +++ b/packages/webui/test/routes/git.test.js @@ -27,7 +27,7 @@ import { test, describe, before, after } from "node:test"; import assert from "node:assert/strict"; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { execSync } from "node:child_process"; @@ -316,6 +316,48 @@ describe("git routes — /api/git/diff", () => { assert.match(body.error, /非法路径/); }); + test("a DANGLING symlink (target does not exist) is rejected", async () => { + // The previous "symlink escape" test created a symlink whose + // target DID exist (e.g. /etc/hostname on Linux), so realpathSync + // succeeded and the realpath-containment check correctly rejected. + // A *dangling* symlink — one whose target does not exist anywhere + // on the filesystem — used to slip past: realpathSync threw + // ENOENT, the lib substituted the unresolved (still-inside-dir) + // path, and the containment check passed. The macOS CI caught + // this because `/etc/hostname` does not exist on macOS runners, + // making every `/etc/hostname`-pointing symlink a dangling one. + // The fix is to require the symlink to resolve; a dangling link + // is treated as an escape attempt and rejected. + // + // Note: this test fails on the OLD lib regardless of platform — + // it just happens that on Linux /etc/hostname exists so the + // previous "symlink" test did not exercise this branch. + const linkPath = `${repoDir}/dangling-link`; + const target = `${repoDir}/nonexistent-target-${Math.random().toString(36).slice(2)}`; + try { + const { symlinkSync } = await import("node:fs"); + symlinkSync(target, linkPath); + } catch (cause) { + console.warn(`[skip] dangling symlink test: ${cause.message}`); + return; + } + const res = fakeRes(); + gitRoute.handleGitDiff(readReq( + `/api/git/diff?dir=${encodeURIComponent(repoDir)}&file=${encodeURIComponent("dangling-link")}`, + ), res); + const body = await readBody(res); + // Belt-and-braces: make sure the target really does not exist + // (otherwise the test would not actually exercise the dangling + // branch and would silently cover the non-dangling case). + assert.equal( + existsSync(target), + false, + `precondition: target ${target} must NOT exist for the dangling branch to be exercised`, + ); + assert.equal(body.ok, false, `dangling symlink must be rejected; got body=${JSON.stringify(body)}`); + assert.match(body.error, /非法路径/); + }); + test("a filename containing '..' is rejected", async () => { const res = fakeRes(); gitRoute.handleGitDiff(readReq(