docs: cover the Console in the threat model and correct the redaction rule

[dichovsky]

Closes #31

What changed and why

docs/design/security.md is the document an audit reads to check crew's controls. It had three gaps against the code, all re-verified against main @ d311767. Per the authority order in docs/README.md, the code and the SRS are the correct side of each, so this is a documentation correction with no behavior change. docs/design/security.md is the only file touched (+61 / −13).

1. The Console — the product's only network listener — was absent from the threat model. The document was 224 lines and sixteen headings with no occurrence of crew ui, while src/ui/server.ts implements a real HTTP security boundary that nobody could review against the doc. A new ### Console network exposure section under ## Threats and controls, placed after ### Identity spoofing (its nearest neighbour — both are about who is allowed to act), now states the controls that exist:

• Loopback-only bind — server.listen(options.port, '127.0.0.1', …) (src/ui/server.ts:767), FR-U02 (srs.md:739-741).
• A per-run token from randomBytes (src/ui/index.ts:98, 32 bytes / 64 hex), required on every request as ?token= or Authorization: Bearer and checked before any routing — authorized() (src/ui/server.ts:202-212), enforced with a 401 at :648-651. FR-U04 (srs.md:745-748).
• Constant-time comparison — tokenEquals (src/ui/server.ts:192-200) via timingSafeEqual, with a length mismatch treated as an ordinary miss rather than an early return.
• The Host-header allowlist — only 127.0.0.1:<boundPort> or localhost:<boundPort>, else 403, evaluated before the token (src/ui/server.ts:630-634). This is the DNS-rebinding guard, and it is the reason loopback binding alone is not the whole story.
• Cache-Control: no-store on every response path — respond() (:233), respondMethodNotAllowed() (:242-247), and the SSE stream (:460-463).
• A request target Node's parser accepts but WHATWG URL parsing rejects returns a 400 USAGE envelope instead of destroying the socket (:638-646), FR-U10.
• One sentence that the Console invents no authority: Store-domain-only access (FR-U11), non-consuming dashboard reads (FR-U12), sanitized pane peek (FR-U24), and confirmation on destructive actions (FR-U25).

Plus the caveat the Acceptance asks for: src/ui/index.ts:112 prints http://127.0.0.1:<port>/?token=<token>, so the startup URL is a bearer credential for the lifetime of the run. That is deliberate — FR-U04's own Verify clause scopes the token to "only inside the URL" — and it is a different secret from the launch token that FR-J15 (srs.md:698) keeps off every surface. The section says so explicitly, so the two are not confused in a future audit. The consequence (it lands in shell history and scrollback, and any local process can reach the port with only the token in the way) is added to ## Residual risks as one bullet.

Governing ADR: ADR-0012 (docs/adr/0012-optional-local-ui-server.md), cited alongside FR-U01/U06/U07 for the foreground-only, optional, never-detaching lifecycle.

2. Stale requirement id. The redaction rule was labeled FR-J08, which the current SRS defines as "Human sanitization" (srs.md:677). The redactor is FR-J12–FR-J15 (srs.md:686-699), and the SRS records the split itself at srs.md:1312 (| FR-J08 | FR-J12, FR-J13, FR-J14, FR-J15 | split |). src/format.ts already cites the newer ids. Relabeled FR-J12–FR-J15.

3. A documented mechanism that does not exist. The same paragraph claimed doctor and setup report "whether the variables they check are set or unset". No code path does that. src/doctor.ts touches the environment only through isExecutableOnPath (:78, :96); src/setup/index.ts calls it once (:410) and otherwise reads HOME/USERPROFILE only to derive a path; isExecutableOnPath (src/which.ts:39) reads PATH and nothing else. Documenting a guardrail that isn't there is worse than documenting none, because an audit trusts the wrong thing. The passage now states the real mechanism — executable-presence probing, reported by executable name — and the real reason no credential value leaks: nothing copies an environment value into a record in the first place (FR-J13), the property already described correctly two paragraphs later and guarded by the program-level secret-stuffing test.

The credential-name list was also materially wrong, not merely mis-framed. It listed TOKEN, KEY, SECRET, PASSWORD, PASSWD, CREDENTIAL, AUTH, SESSION, COOKIE, PRIVATE, "or ends with _PAT". The actual CREDENTIAL_KEY regex (src/format.ts:122-123) also matches pwd, authorization, and bearer, the compounds api[-_]?key, access[-_]?key, client[-_]?secret, and a pat that is bare or --prefixed, not only _PAT. The list is rewritten to match the regex exactly, and re-scoped to its actual job — keyed-pair redaction in free text — rather than a fictional env guardrail. The existing launch_token / db_credential / signing_key examples are kept, with the monkey / author negatives that the regex's (?:^|[_-])…$ anchoring produces.

Verification

Docs-only change: no tests added or updated, claiming the docs-only exemption in .github/ISSUE_TRIAGE.md. No src/** or bin/** file changes, so coverage cannot move. The full gate was still run, under Node 24.18.0 (the CI version), after rebasing onto origin/main.

Command	Exit	Result
npm run typecheck	0	pass
npm run lint	0	pass
npm run format:check	0	All matched files use Prettier code style!
npm run build	0	pass (dist/ui-assets/main.js 88.4kb)
npm run test:coverage	1	1395 passed, 3 skipped, 2 pre-existing environment failures

Both failures are in tests/integration/commands/doctor.test.ts ("degrades a raw filesystem read failure in project roles instead of aborting doctor" and "degrades unreadable roles and teams directories to whole-listing warnings"). They were proven pre-existing by stashing the change, confirming a clean tree, and re-running that file alone — identical two failures, same names, same assertions (Tests 2 failed | 31 passed (33)). Root cause is environmental: the sandbox runs as uid 0, so the tests' chmod 000 does not actually make the directories unreadable and the expected INVALID_CONFIG warnings never fire. A Markdown-only edit cannot influence them.

Because vitest suppresses the coverage table on a failing run, the 95% thresholds were not evaluated locally. This PR's CI run on GitHub-hosted runners is the authority for the gate.

Rebased onto origin/main @ d311767 immediately before pushing and the gate re-run afterward.

Related open PRs

None. Every other open routine PR touches disjoint files, verified by diffing each against origin/main: #49 and #64 (docs/design/srs.md), #56 (README.md, CLAUDE.md, AGENTS.md, package.json, docs/design/srs.md, docs/design/product-spec.md), #57 (README.md, docs-site/sections/modules.tsx, docs/design/architecture.md, docs/design/product-spec.md), #61 (docs/design/cli-contract.md, docs/design/data-model.md), #63 (docs/design/configuration.md). None touches docs/design/security.md.

Out of scope

Deliberately untouched, each tracked separately:

• The identical set/unset fiction in the src/format.ts:118-120 comment ("mirrors the name-based env-guardrail set documented in security.md") — Redaction: format.ts comment cites a "name-based env-guardrail set" that no code implements #68. Keeping it out preserves this PR's docs-only exemption.
• architecture.md §4.1 source-tree staleness — architecture.md §4.1 source tree is substantially stale #35.
• docs-site prose drift — docs-site prose drift: lifecycle omits abandoned, deferred-seam roster, glossary count #42.
• No entry was added to the ## Security acceptance tests list: no tests change here, and the redaction and no-credential-leak properties are already listed.
• No FR-*/NFR-* id was added, renumbered, or invented. Every id cited was grepped in docs/design/srs.md first. The Host-header allowlist, Cache-Control: no-store, and the constant-time token comparison are described as implementation controls without an id, because the SRS and ADR-0012 define none for them — a genuine traceability gap, filed separately as SRS: three shipped Console HTTP controls have no FR behind them #70.

[dichovsky]

Routine review of ea87f6e. Roster per .github/ISSUE_TRIAGE.md: general code reviewer (always) plus, for a docs-only diff, verification of the new prose against the code it describes and against the authority order. The security reviewer's path triggers (src/store/, src/process.ts, src/which.ts, src/fs-safe.ts, src/setup/, Launcher) are not met by this diff, but the prose asserts security properties of src/ui/server.ts, so those assertions were checked with the same adversarial posture.

Scope, ID hygiene, and formatting are clean. Two claims in the new prose are wrong, and both are the same defect class issue #31 was filed to remove — a security document asserting a protection the code does not provide. Findings below were reproduced independently before being reported.

HIGH

H1 — docs/design/security.md:79-80: the constant-time claim is the inverse of the code.

The new text reads "The comparison is constant time, and a length mismatch is treated as an ordinary miss rather than an early return, so response timing does not leak the expected token."

src/ui/server.ts:193-200:

return (
  providedBytes.byteLength === expectedBytes.byteLength &&
  timingSafeEqual(providedBytes, expectedBytes)
);

&& short-circuits, so on a length mismatch timingSafeEqual is never called — that is exactly an early return. The comparison is constant time only among equal-length candidates, and response timing does distinguish a wrong-length guess from a wrong-value guess. What the source comment at :192 actually means is that a length mismatch does not throw (timingSafeEqual raises a RangeError on unequal-length buffers); the prose read that as a timing guarantee and stated it unconditionally.

Practical exposure is small — the token length is fixed and effectively public, since it is printed in the URL — but an unqualified "does not leak" in the threat model is precisely what an auditor will rely on. Fix by scoping the claim: constant time for equal-length inputs, with a length mismatch short-circuiting to a plain miss rather than throwing, leaking only the already-public length.

H2 — docs/design/security.md:178-184: two universal claims about the environment are both falsifiable.

1. "No crew command reports the state of an environment variable." crew setup <target> does, by name: src/setup/index.ts:56-64 throws DEPENDENCY_MISSING with the user-facing message cannot resolve the home directory (HOME unset) for global setup, covered by tests/integration/commands/setup.test.ts:346. crew team --launch does the same — src/launcher/derive.ts:76-79, cannot derive the worktree base: set XDG_DATA_HOME, HOME, or USERPROFILE. Relatedly, the preceding clause says doctor and setup consult the environment "only to answer 'is this program present?'"; setup also reads HOME/USERPROFILE to derive a path (src/setup/index.ts:58, :81), which is not that question.

2. "nothing copies an environment value into a record in the first place." The launch token is a credential environment value copied verbatim into a stored record: src/agents.ts:27-29 reads CREW_LAUNCH_TOKEN, src/agents.ts:99-105 passes it into the join, src/store/agents.ts:150-160 inserts it, and src/store/schema.ts:16 is the launch_token column. The reason it never leaks is a rendering guardrail — FR-J15 (srs.md:698) and FR-H28 (srs.md:606-608) — which is the opposite of "holds at the source". The sentence also contradicts the one three lines above it, which correctly grants the env-derived worktree base path as an exception.

The underlying security property is intact in both cases: no credential value is emitted (neither message prints a value, and displayPath at src/setup/index.ts:79-86 collapses $HOME to ~), and the launch token is kept off every surface. Only the stated reasoning is wrong — but a threat model is read for its reasoning.

MEDIUM

M1 — docs/design/security.md:99, :267: "lands in shell history" is the wrong vector and contradicts a higher-authority document. The URL is printed output (src/ui/index.ts:112-117), not a typed command, so it reaches shell history only if the Operator pastes it — which the next clause already covers. docs/design/cli-contract.md:375 (authority level 3; security.md sits outside the order entirely) says "browser history and terminal scrollback", which is accurate because crew ui opens the URL by default (src/ui/index.ts:118-121). That default is also worth a clause the section currently omits while enumerating where the credential lands: the URL is passed as argv to the opener process, so it is visible in ps//proc to other local users for the life of that call.

M2 — docs/design/security.md:265-268: the residual-risk bullet implies a stale token is a live exposure. It says the token "persists in shell history and scrollback for longer than the run itself", framed as what stands between a local process and the Workspace. But :81 ("The token lives and dies with the run") and :98 ("for the lifetime of the run") say the leftover string is dead once the server stops, and cli-contract.md:376 agrees ("restarting crew ui invalidates it"). The real residual risk is exposure during a long-running session, not after it.

M3 — docs/design/security.md:90-92: FR-U11 is cited for a claim it does not make. FR-U11 (srs.md:775) covers only that every Console read and write uses an existing Store domain method. "The same invariants as the CLI" is FR-U18 (srs.md:799), which is not cited.

LOW

• :192-193 — api key, access key, and client secret are shown as code spans with a space, but KEYED_PAIR (src/format.ts:121-122) restricts keys to [A-Za-z0-9_-], so a space form never matches. Executed against the real regex: api key → false, api_key/api-key/apikey → true. The following clause explains the separator; the literal spans still invite the wrong reading.
• :179 — "they read PATH through isExecutableOnPath" is incomplete for setup, whose version probe goes through resolveExecutableOnPath (src/platforms/shared.ts:216).
• :78-79 — "a request without it gets 401" is locally imprecise: a foreign Host gets 403 (src/ui/server.ts:630-634) and a malformed target gets 400 (:638-647), both before the token check. The load-bearing half — "reaches no handler" — is correct in all three cases, and the section states both other paths correctly further down.
• :87 — "Every response carries Cache-Control: no-store" is true of every response crew writes (all three writeHead sites: :233, :242-246, :459-463) but not of Node's own parser-level clientError replies. "Every response the Console writes" would be exactly true.
• src/format.ts:120 (outside the diff) — its comment now points at the framing this PR removes. Correctly left alone under the scope rule; tracked as #68.

Verified correct — recorded so a re-review need not redo it

The most dangerous candidate claim holds: no request path bypasses the token. handleRequest (src/ui/server.ts:629-671) is the only createServer listener in src/, and authorized() at :648 runs before route() at :660. Bundled assets, SSE, /api/health, /api/peek, the session inventories, every action POST, 404s, and the 405 path are all reached only from inside route(). No favicon case, no OPTIONS/HEAD short circuit. Corroborated by tests/integration/ui-server-assets.test.ts:130,154 and ui-server.test.ts:500.

Also verified: loopback bind is a hardcoded literal with no override, and --port cannot change the interface (src/ui/server.ts:767, src/ui/index.ts:53-61); the token is randomBytes(32) → 64 hex, per run (src/ui/index.ts:22, :98); the Host allowlist is exactly the two forms, 403, and is the first statement in handleRequest, so the "before the token" ordering claim is right; the 400 USAGE envelope and its FR-U10 citation; FR-U01/U02/U04/U06/U07/U10/U12/U24/U25 all exist and say what is claimed; ADR-0012 is the right ADR. The FR-J08 → FR-J12–FR-J15 relabel is correct, including the SRS's own split record at srs.md:1312, and no stale redaction reference survives repo-wide. The credential-word list now matches CREDENTIAL_KEY exactly — all 14 words, same order, compounds present, and the claimed positives/negatives (launch_token/db_credential/signing_key → true, monkey/author → false) execute as documented. Scope discipline is clean: every changed line traces to one of #31's three bullets, and nothing accurate was removed. No FR-*/NFR-* id was added, renumbered, or invented. npx prettier --check docs/design/security.md passes.

Disposition

H1 and H2 are one-to-two-sentence rewrites. Under .github/ISSUE_TRIAGE.md the implementer gets exactly one fix pass, covering H1, H2, and the cheap M/L items; the reviewer then re-checks only the changed areas. The PR stays a draft until that pass is verified.

---

Generated by Claude Code

[Copilot]

Pull request overview

This PR updates docs/design/security.md to bring the security/threat-model documentation back in sync with the shipped Console (UI server) controls and with the current SRS requirement IDs, without changing runtime behavior.

Changes:

• Add a threat-model section for the Console’s network exposure and its implemented request/response guards.
• Correct the redaction rule’s requirement references from the stale FR-J08 label to FR-J12–FR-J15 and align the documented mechanism with actual behavior (no fictional env-var “set/unset” reporting).
• Add a residual-risk note about the Console startup URL embedding a live bearer token.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dichovsky]

Re-review of the fix pass (ea87f6e..15201f3), scoped to the changed areas only, per .github/ISSUE_TRIAGE.md. Nothing CRITICAL or HIGH survives.

H1 — resolved. The inverted timing claim is gone. New text scopes the guarantee correctly: timingSafeEqual for same-length tokens, an unequal-length token short-circuiting "to a plain miss instead of throwing, which leaks only the token's length — a fixed length that is already public, since the token is printed in the startup URL." That matches src/ui/server.ts:193-200, including the reason the source comment at :192 says what it says. (Copilot's line-80 comment reported the same defect independently; this is the fix for it.)

H2 — resolved, and the replacement is verifiable. Both false universals are gone.

• The presence-only claim is now hedged with its own counterexamples named in the text (setup's HOME unset DEPENDENCY_MISSING, the Launcher's XDG_DATA_HOME, HOME, or USERPROFILE), keeping the true and stronger property — no credential environment value is emitted. The ~ collapse is real: displayPath (src/setup/index.ts:79-86).
• The launch token is now stated plainly as the one credential env value copied into a record, secret through a rendering guardrail rather than through never being stored. Both newly cited ids check out: FR-U18 (srs.md:799-802) says exactly "invoke the corresponding existing command or domain operation and preserve its authority and invariants", and FR-H28 (srs.md:606-608) independently confirms the substance of the correction — "The launch token shall exist only in pane environment variables and Agent rows and shall never appear in any output". The reap rationale is real too: reapByLaunchTokenTx (src/store/agents.ts:197-201) selects on launch_token.

M1, M2, M3 — resolved. "shell history" is gone from both sites and replaced with the CLI contract's own vector ("browser history and terminal scrollback", cli-contract.md:375), plus the opener-argv exposure that was previously missing. The residual-risk bullet is rescoped to during-run and now states the token dies with the server, agreeing with :81, :98, and cli-contract.md:376. FR-U18 is cited alongside FR-U11 for the invariant half.

All four LOW items — resolved. Compound forms now show only shapes the regex matches; resolveExecutableOnPath is named; the 401 sentence is qualified with "a request that reaches it"; "every response the Console writes".

One edit outside the originally-cited ranges, and it is correct. :226-227 (pre-existing text, not part of the ea87f6e diff) carried the identical false claim H2 ordered removed — "upheld at the source (crew never copies an environment value into a record)". Leaving it would have made the document contradict its own H2 fix three paragraphs later. The correction is the parenthetical only, now "(it renders probe names and stored record fields, never a credential environment value)". This traces to issue #31's third bullet, so it stays inside scope rather than widening it.

Scope is otherwise unchanged: docs-only, one file, no FR-*/NFR-* id added, renumbered, or invented — the only newly cited ids are FR-U18 and FR-H28, both pre-existing and verified. npx prettier --check docs/design/security.md passes.

Marking ready for review. The gate ran green locally except for the two known uid-0 doctor.test.ts failures, which reproduce on a clean tree; this PR's CI run on GitHub-hosted runners remains the authority.

---

Generated by Claude Code