Security review: MCP Tier 1 surface

This review covers the local stdio MCP surface shipped in issue #80 (MCP-08): the stdio server (packages/mcp/src/server.ts), subprocess delegation (subprocess.ts), stage-scope runtime (scope.ts), visibility tracker (visibility.ts), tool registry (registry/*), install surface (packages/cli/src/mcp/*), and the backend scope resolver (packages/backend/src/mcp-scope-http.ts), against the security boundaries ADR-0007 declares.

Verdict

No Critical, High, or Medium findings. Every security property ADR-0007 promises holds in the implementation. Four Low/informational observations are explicitly accepted with the defense-in-depth rationale that makes them safe.

Findings (severity-rated)

Token handling — RESOLVED (verified correct)

The MCP server never reads, prints, returns, or stores the bearer token. It spawns tv <argv> --json with an allowlisted environment (HOME, USERPROFILE, APPDATA, XDG_CONFIG_HOME, plus THALIX_BASE_URL only — subprocess.ts buildAllowedEnv); the tv subprocess reads the credential from its own credential store. redactSecrets scrubs any tv_-shaped token from stdout, stderr, and spawn-error text before it can reach an MCP client. Covered by tests/security/mcp-subprocess-security.test.mjs ("environment allowlist drops credentials", "token-shaped output is redacted").

Grant scoping — RESOLVED (server-side)

The effective grant rank and contract version come from the backend RPC resolve_mcp_stage_scope (scope.ts → tv mcp scope-state), and the tool allowlist comes from the stage's CONTEXT.md frontmatter. The client-side visibility filter (visibility.ts) is a UX convenience only; every tool call spawns tv, which re-authorizes against the actual target server-side. Client-side visibility never grants access — a tool hidden from the list is denied locally, and a tool shown in the list is still denied by the backend if the grant no longer holds.

Audit parity — RESOLVED

Scope-resolution decisions (initial/changed/denied) are recorded in mcp_scope_audit_events via record_mcp_scope_audit (scope.ts audit()), including effective rank, visible names, and hidden-tool count. Each operation is audited by the tv subprocess exactly as a direct CLI call would — MCP adds one audit domain, never bypasses one.

Tool-argument injection — RESOLVED (no surface)

Tool arguments pass through validateToolInput (strict JSON-schema subset, additionalProperties: false, unknown-property and oneOf rejection), then buildArgv maps them to argv with pushIfDefined/pushFlagIfTrue/pushRepeated (no string concatenation), and the subprocess spawns with shell: false and an argv array. No argument can be interpreted as a flag or shell metacharacter. Covered by "argv metacharacters are passed as data without a shell".

Credential leakage — RESOLVED

A grep of the tool registry finds no secret, token, password, or key in any tool description or schema. The install config (packages/cli/src/mcp/config.ts) writes command + args only — no credential. The subprocess environment drops every non-allowlisted variable, so no credential leaks via env.

Low — 5-second scope-cache TOCTOU — ACCEPTED

The resolved visible-tool set is cached for 5 s (scope.ts DEFAULT_SCOPE_CACHE_TTL_MS). A grant revoked inside that window leaves a tool nominally "visible" client-side. Accepted: the tv subprocess re-authorizes server-side on every call, so a revoked grant still denies the operation; the stale list can at most offer a tool that then fails closed.

Low — scope audit is client-triggered — ACCEPTED

record_mcp_scope_audit is driven by the MCP client process; a hard kill mid-resolution can lose a scope-denial audit row. Accepted: the underlying operation is independently audited by the tv subprocess; the scope audit is additive evidence, not the only record.

Low — fail-closed scope resolution/audit — ACCEPTED

If resolve_mcp_stage_scope or record_mcp_scope_audit is transiently unreachable, tools/call rejects. Accepted: fail-closed is the correct posture — a transient backend outage blocks MCP tools (an availability cost), never widens access.

Low — vault_mcp_context exposes workspace/stage UUIDs — ACCEPTED

The in-process discovery tool returns workspaceId/stageId. Accepted: these are anti-enumerated UUIDs already returned by the CLI surface, not secrets.

Verification evidence

• grep -rniE 'tv_[A-Za-z0-9_-]{32,}|…' packages/mcp/src/registry/ → no secret/token in tool descriptions.
• tests/security/mcp-subprocess-security.test.mjs → 4/4 pass (env allowlist, token redaction, argv-as-data, output-limit fail-closed).
• bun run typecheck → green.
• Grant enforcement confirmed server-side: resolve_mcp_stage_scope (backend) + per-call tv re-authorization; client-side visibility is filtering only.