feat(mcp): add authenticated HTTP transport

[aram-devdocs]

Summary

Closes #81

• add plumb mcp --transport http --port <PORT> alongside the existing stdio default
• require PLUMB_MCP_TOKEN at HTTP boot and reject missing/invalid bearer tokens with 401
• keep stdio as the default transport
• add focused CLI/MCP HTTP auth tests and MCP security notes

Scope

• feat(mcp): HTTP transport + PLUMB_MCP_TOKEN auth #81 only
• no feat(mcp): lint_page_html tool #79/feat(mcp): compare_viewports tool #80 tools
• no feat(core): rule baseline/rhythm (font metrics + CDP textBoxes) #73 baseline/rhythm work
• no chore(main): release 0.0.2 #101/release tag/Homebrew/npm publication/token provisioning

Validation

Codex reported validation during implementation, but the ACP run timed out before returning details. Verified after completion:

• git diff --check HEAD~1..HEAD — passed

Follow-up CI will run the full cargo test/clippy matrix.

[github-actions]

I have enough context to write the full review. Let me compile the findings.

---

PR #205 — feat(mcp): add authenticated http transport

Scope: plumb-mcp/src/lib.rs, plumb-cli/src/commands/mcp.rs, plumb-cli/src/main.rs, crates/plumb-cli/tests/mcp_http.rs, Cargo.toml, deny.toml, docs.

---

1. Determinism

No new determinism violations introduced. Pre-audit:

• std::collections::HashMap at lib.rs:34 and SystemTime at lib.rs:39 are pre-existing. HashMap is used only in the internal config cache (config_cache: Arc<Mutex<HashMap<PathBuf, ConfigCacheEntry>>>), and SystemTime is sourced from fs::metadata().modified(), not SystemTime::now(). Neither appears in observable output. These pre-date this PR and are out of scope here.
• None of the new auth code paths introduce wall-clock reads or nondeterministic ordering.

2. Workspace Layering

Clean.

• plumb-mcp gains axum and subtle — both external crates, no internal-dep violation.
• reqwest lands in plumb-cli/[dev-dependencies] only — acceptable.
• #![forbid(unsafe_code)] remains in plumb-mcp/src/lib.rs:26.
• No println!/eprintln! introduced in library code.

3. Error Handling

McpError has #[from] io::Error so TcpListener::bind(addr).await? converts correctly. run_http surfaces the serve error before the shutdown error, matching the ordering convention in run_stdio. Library-level #![deny(clippy::unwrap_used, clippy::expect_used)] is present; test-only #![allow] is correctly scoped.

Warning — commands/mcp.rs:34 (read_http_token): Ok(token) returns the raw untrimmed string. The emptiness guard is token.trim().is_empty(), so PLUMB_MCP_TOKEN=" secret" passes the guard but stores " secret" as the bearer value. The client then needs Authorization: Bearer secret (double space). This won't be a security issue in practice, but it's a confusing footgun — the fix is one line:

// crates/plumb-cli/src/commands/mcp.rs:34
Ok(token) => Ok(token.trim().to_owned()),

Warning — lib.rs:769 docstring: The # Errors section says "Returns McpError::Service when the underlying MCP service cannot be constructed." But StreamableHttpService::new is infallible — it doesn't return Result. McpError::Service is only reachable via handler.shutdown(). The phrase "cannot be constructed" is misleading and should say "when browser shutdown fails" or similar.

4. Security

Constant-time comparison via subtle::ConstantTimeEq is correct and the right choice for bearer tokens.

Security note — token length oracle: [u8]::ct_eq returns Choice::from(0) immediately when lengths differ, leaking length information through timing. Over a loopback interface (127.0.0.1) this has near-zero practical impact, but it means a local attacker with sub-millisecond timing access could determine the token length. For a localhost-only server this is acceptable; worth flagging for any future remote-binding scenario.

Security note — middleware ordering: .layer(from_fn_with_state(...)) in Axum 0.8 wraps the inner service — requests traverse the auth middleware before reaching the StreamableHttpService. The ordering is correct.

Security note — reserve_port() TOCTOU (tests/mcp_http.rs:24): Code comment acknowledges the race window. Acceptable for a loopback-only test harness.

5. Test Coverage

crates/plumb-cli/tests/mcp_http.rs covers:

• Boot without token → exit 2 + stderr mention ✓
• Boot with empty token → exit 2 ✓
• Request without Authorization → 401 + WWW-Authenticate: Bearer ✓
• Request with wrong bearer → 401 ✓
• Request with correct bearer → 200 + mcp-session-id ✓

Missing: no test for the whitespace-token footgun described above. If read_http_token is fixed to trim, add a test http_transport_trims_whitespace_in_token that sets PLUMB_MCP_TOKEN=" secret " and verifies Authorization: Bearer secret (trimmed) is accepted.

6. Documentation

Docs prose (docs/src/mcp.md, README.md, AGENTS.md) is concise and free of AI-tell phrases from the blocklist. The "Keep the token private" section in README.md is direct and specific.

---

Punch list

#	File	Line	Severity	Issue
1	crates/plumb-cli/src/commands/mcp.rs	34	REQUEST_CHANGES	Ok(token) should be Ok(token.trim().to_owned()) — untrimmed token is a silent footgun
2	crates/plumb-mcp/src/lib.rs	769	Warning	# Errors says "service cannot be constructed" but the McpError::Service path is handler.shutdown(), not construction — fix the description
3	crates/plumb-cli/tests/mcp_http.rs	—	Warning	No test covering trimmed-whitespace token acceptance once item 1 is fixed
4	crates/plumb-mcp/src/lib.rs	104	Security note	ct_eq leaks token length; acceptable for loopback, but document the scope constraint if --bind is ever added

---

Verdict: REQUEST_CHANGES