fix: address review report across security, architecture, quality, tests
Implements the fixes from docs/REVIEW_REPORT.md (4-agent parallel review). typecheck clean; 341 tests pass (16 files, +113); build:web ok; coverage thresholds (80%) enforced in vitest.config.ts. Critical: - multi-device approval race: release held approval only when the last client detaches (closing one mirror no longer cancels another's prompt) - unbounded session creation (DoS): Config.maxSessions cap (env MAX_SESSIONS), enforced in manager via the existing M4 exit(-1) path - signal-handler leak: named SIGINT/SIGTERM/uncaughtException refs removed in close() - terminal-session initialInput timer tracked + cleared on dispose - tabs.addEntry null-as-cast type hole removed (build session before entry) Should-fix: - security-headers middleware + Origin/CSRF guard on DELETE /live-sessions[/:id] - history.ts converted to fs/promises (async /sessions handler) - removed dead clientDims map + blur protocol message end-to-end - per-connection WS message rate limit (Config.maxMsgsPerSec) - /sessions behavior kept; documented as accepted LAN risk (TECH_DOC §7) Tests: - new tmux / preview-grid / terminal-session (jsdom) / tabs (jsdom) suites - extended history/config/manager/integration coverage incl. regressions Hygiene: - parsePositiveInt -> parseNonNegativeInt; ALLOWED_ORIGINS scheme validation - log-injection sanitize; isLoopback handles 127.0.0.0/8 + IPv4-mapped - operational constants moved into Config - extracted public/preview-grid.ts (DRY launcher/manage) - doc sweeps: ARCHITECTURE §8 runtime-handle exception, stale comments
This commit is contained in:
Yaojia Wang
84
docs/REVIEW_REPORT.md
Normal file
84
docs/REVIEW_REPORT.md
Normal file
@@ -0,0 +1,84 @@
|
||||
# Web-Terminal — Comprehensive Review Report
|
||||
|
||||
**Date:** 2026-06-20 · **Branch:** main (`97d5732`) · **Scope:** full project (v0.1–v0.5)
|
||||
**Method:** 4 specialized agents in parallel — security, architecture, code quality, test coverage.
|
||||
|
||||
**Ground truth:** `tsc` typecheck clean (strict + `noUncheckedIndexedAccess`) · **228/228 tests pass** (12 files) · `npm audit` 0 vulns · ESLint not configured · coverage tooling configured (v8) but **`@vitest/coverage-v8` not installed** and **no threshold enforced**.
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
The project is **fundamentally sound**. The load-bearing design invariant — PTY lifecycle decoupled from WebSocket lifecycle — is correctly implemented, the byte-shuttle separation is intact (zero ANSI parsing on the server), and the v0.4 multi-device relaxation of invariant #5 is consistent end-to-end. Backend unit tests are genuinely high quality.
|
||||
|
||||
**No CRITICAL issues.** The notable items are: a real multi-device approval race, unbounded session creation (DoS), a history-leak endpoint, a handful of HIGH code-hygiene bugs (signal-handler leak, untracked timer, type hole), and a structural test gap (entire frontend + several backend paths untested, no enforced coverage).
|
||||
|
||||
| Dimension | Grade | One-line |
|
||||
|---|---|---|
|
||||
| Security | B | Core CSWSH defense solid; gaps are DoS + a LAN info-leak endpoint + missing headers |
|
||||
| Architecture | A− | Design honored on every invariant; one real race + documentation/dead-state drift |
|
||||
| Code quality | B− | Clean types, but 6 HIGH hygiene bugs (resource/timer/cast) to fix before "done" |
|
||||
| Test coverage | C+ | Backend strong; frontend 0%, no enforced threshold, several backend paths untested |
|
||||
|
||||
---
|
||||
|
||||
## Priority Action List (deduped, ranked)
|
||||
|
||||
### 🔴 Fix before shipping further
|
||||
|
||||
1. **Multi-device approval race** (`server.ts:332`) — on `ws.close`, `resolvePending(boundSessionId)` fires for *every* disconnecting client of a shared session, so closing one mirror **cancels a pending approval another device is looking at**. Release only on last detach (`session.clients.size === 0`), and move approval state into the manager. *(Arch 5a — directly undermines the headline v0.4 feature.)*
|
||||
|
||||
2. **Unbounded session creation → DoS** (`manager.ts:91`) — repeated `attach{sessionId:null}` spawns unlimited PTYs + 2 MB ring buffers each; no cap. Add `MAX_SESSIONS` (default ~20) enforced in `handleAttach`; reject over-limit with an `exit` frame. *(Sec H1.)*
|
||||
|
||||
3. **Signal-handler leak** (`server.ts:364`) — `SIGINT`/`SIGTERM`/`uncaughtException` handlers added every `startServer()`, never removed in `close()` (tests paper over it with `setMaxListeners(50)`). Capture refs, `process.off()` in `close()`. *(CQ H1.)*
|
||||
|
||||
4. **Untracked timer fires after `dispose()`** (`terminal-session.ts:250`) — the 700 ms `initialInput` `setTimeout` is never tracked/cancelled; survives only by the WS-readyState guard, not a `disposed` check. Track it, check `this.disposed`, clear on dispose. *(CQ H2.)*
|
||||
|
||||
5. **`null as unknown as TerminalSession` type hole** (`tabs.ts:150`) — entry pushed to `this.tabs` with a null session typed non-null; if `new TerminalSession()` throws, later `refreshTab` crashes. Build `session` first, then the entry. *(CQ H3.)*
|
||||
|
||||
### 🟠 Should fix
|
||||
|
||||
6. **CSRF on destructive routes + missing headers** — `DELETE /live-sessions[/:id]` is state-changing with no Origin/CSRF check (WS upgrade checks Origin; plain HTTP routes don't), and no `X-Frame-Options`/CSP enables clickjacking of Kill-All. Add the Origin check (or rely on a forced JSON/preflight) to mutating routes; add a security-headers middleware. *(Arch 5b + Sec H2 — same root.)*
|
||||
|
||||
7. **`/sessions` leaks Claude history to all LAN devices** (`history.ts:80`, `server.ts:114`) — returns cwd paths + first 120 chars of first prompt + resumable session UUIDs, unauthenticated. Gate behind the same loopback check used for `/hook`, or remove. *(Sec H3.)*
|
||||
|
||||
8. **Sync `fs` blocks the event loop** (`history.ts:66`) — `readdirSync`/`statSync`/`readSync` in the `GET /sessions` handler stall all WS/PTY traffic. Convert to `fs.promises` + async handler. *(CQ H4.)*
|
||||
|
||||
9. **Dead `clientDims` state from the min→latest-writer pivot** (`types.ts:155`, `session.ts:196`) — map is written/deleted but **never read**; `clearClientDims` + the `blur` protocol message are now no-ops. Delete them (or document as no-ops). *(Arch 2a.)*
|
||||
|
||||
10. **Slim `server.ts` (≈408 lines)** back toward thin wiring — extract `http/routes.ts` and move the held-approval state machine into the manager. This is the root cause of #1. *(Arch debt.)*
|
||||
|
||||
11. **Per-connection WS message rate limit** (`server.ts:249`) — post-attach `input`/`resize` floods saturate CPU/IO; `maxPayload` caps size, not frequency. Add a leaky-bucket per socket. *(Sec M3.)*
|
||||
|
||||
### 🟡 Test gaps (raise the floor)
|
||||
|
||||
12. Install `@vitest/coverage-v8`, add `thresholds: { lines/functions/branches/statements: 80 }` to `vitest.config.ts`, wire `--coverage` into CI. Today the 80% rule is unmeasured.
|
||||
13. **Frontend is 0% covered** — `terminal-session.ts` (400 lines: reconnect state machine, `buildWsUrl` scheme selection, `hide()`→`blur`, `dispose()`), `tabs.ts` (last-close→launcher v0.5 invariant), `launcher.ts`, `manage.ts`. Add jsdom config + mock `WebSocket`.
|
||||
14. **Untested backend paths:** `manager.killById`/`handleHookEvent`, `history.listSessions` (fs traversal), `tmux.hasSession`/`killSession`, `config.resolveUseTmux`, `isLoopback`, and the `/live-sessions*` + `blur`/`approve`/`reject` routes in `server.ts`.
|
||||
|
||||
### ⚪ Lower priority / hygiene
|
||||
|
||||
- **DRY:** `launcher.ts` & `manage.ts` are ~80% duplicated (card factory, `relTime`, `statusText`, preview fetch, re-declared `LiveSession`/`Preview`). Extract a shared `preview-grid.ts`; import `LiveSessionInfo` from `src/types.ts` instead of re-declaring. *(Arch 6a + CQ M1.)*
|
||||
- **Stale comments/docs:** "min across clients" JSDoc in `manager.ts:21`, `session.ts:179`, `types.ts:153`; duplicate JSDoc on `setClientDims`; CLAUDE.md still says "auto-show as tabs" (v0.5 uses the launcher chooser). Sweep to match latest-writer-wins. *(Arch 2b/6b + CQ M6.)*
|
||||
- **`ALLOWED_ORIGINS` scheme validation** (`config.ts:107`) — reject non-http(s) entries to avoid exotic-scheme cross-match. *(Sec M1.)*
|
||||
- **Log injection** (`server.ts:260`, `protocol.ts:64`) — sanitize/truncate the user-controlled `type` field before logging. *(Sec M2.)*
|
||||
- **`cwd` hardening** (`protocol.ts:144`) — `path.resolve()` + comment; within current threat model informational only. *(Sec L2 / CQ H5.)*
|
||||
- **`isLoopback` robustness** (`server.ts:65`) — misses `127.0.0.0/8` and hex IPv4-mapped forms; fine on macOS/Linux today, brittle behind proxies. *(Sec L1.)*
|
||||
- Misc CQ: `parsePositiveInt` accepts 0 (rename `parseNonNegativeInt`); `isConnecting` double-reset in `error` handler; `confirm()` blocks in `manage.ts`; magic `700` ms; operational constants (`PERM_TIMEOUT_MS`, `REAP_INTERVAL_MS`, `PREVIEW_BYTES`) not in `Config` despite invariant #8.
|
||||
|
||||
---
|
||||
|
||||
## Note: the immutability tension (intentional)
|
||||
|
||||
The code reviewer flagged in-place mutation of `Session` runtime fields (`detachedAt`, `lastOutputAt`, `claudeStatus`, `clients` Set) as a violation of the project's "never mutate" rule (CQ H6). The architect's read — which the docs support — is that this is **by design**: `SessionMeta` is fully `readonly` and immutable; mutable *runtime handles* are deliberately held separately (CLAUDE.md, ARCHITECTURE invariant #4), and replacing a Set on every keystroke would be wasteful. **Recommendation:** keep the design, but resolve the contradiction in writing — carve out the runtime-state exception in the coding-style rule, or extract a `SessionState` object replaced atomically. Don't silently leave a stated rule contradicted.
|
||||
|
||||
---
|
||||
|
||||
## What's working well (don't regress)
|
||||
|
||||
- **CSWSH defense** (`origin.ts`) is correct: empty/undefined Origin rejected; scheme+host+port all matched; `noServer:true` single-upgrade-handler can't be raced. ✅
|
||||
- **Byte-shuttle invariant** intact — server does zero ANSI parsing; `input.data` passed verbatim; terminal semantics live only in the browser. ✅
|
||||
- **Ring buffer (M2)** is the cleanest module: real UTF-8 byte accounting, whole-chunk eviction, `\x1b[0m` reset on replay — no codepoint/escape can be cut. ✅
|
||||
- **Session decoupling** verified by tests against a live shell: PTY survives WS disconnect, reconnect replays scrollback. ✅
|
||||
- All child processes use `execFileSync` with arg arrays — **no command injection** via tmux names. ✅
|
||||
- Backend unit tests use AAA, inject deterministic timestamps, mock at the `node-pty` boundary, and cover named invariants (M2–M7, L1–L4). ✅
|
||||
Reference in New Issue
Block a user