web-terminal/docs/REVIEW_REPORT.md

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). ✅