feat(network): ADR-107 — network exposure as classified state machine#211
Conversation
Models the combined (protocol × bind × cert) state as a 9-cell table with three verdicts (🟢 OK / 🟡 WARN / 🔴 JAIL). Server always starts; 🔴 surfaces a Notice + log + MCP `initialize` instructions warning so the LLM client also sees it. New settings: `bindMode` discriminated enum + `customBindHost`. Migration flips the implicit `0.0.0.0` to a real `127.0.0.1` default, making ADR-103's "loopback by default" premise actually true. Inspired by community PR #208 (fa1k3) — preserves the security instinct while keeping the LAN/remote escape hatch the hardcoded approach would have removed.
Pure functions consumed by the bind-time guard, settings UI, and MCP initialize.instructions injector: classify(NetworkState) → Verdict, resolveListenHost(), resolveBindAxis(), normalizeBindInput(), classifyFromSettings(), agentInstructionsForVerdict(). Single source of truth for "how exposed is the current network configuration?". The 9-cell ADR-107 table maps (protocol × bind × certSource) → one of 🟢 ok / 🟡 warn / 🔴 jail. Custom-input normalization collapses loopback aliases (127.x.x.x / localhost / ::1) and wildcard aliases (0.0.0.0 / ::) to the corresponding explicit BindMode so the same intent expressed three ways converges to one state. 40 unit tests cover all 9 table cells, all normalization paths, the defensive empty-custom fallback, and the agent-instructions formatter.
Replaces the implicit 0.0.0.0 Node-default bind with a deliberate,
classified network exposure model.
Server: MCPHttpServer.start() now resolves the listen host from
settings (bindMode + customBindHost), passes it to listen(), and
classifies the combined (protocol × bind × certSource) state. On 🔴
fires a 15s Notice + Debug.error; on 🟡 logs Debug.warn; on 🟢 silent.
The verdict-derived agent-visible warning is pushed into MCPServerPool
which conditionally injects it into the MCP initialize.instructions
field for every session created after bind, so an LLM client sees the
warning on its first turn against the server. node-mcp-server.ts (the
unused fallback) is hardcoded to loopback for consistency.
Settings: MCPPluginSettings gains bindMode ('loopback' | 'all' |
'custom') default 'loopback', customBindHost default '', and a
one-shot hasShownBindMigrationNotice flag. Existing installs with a
data.json that predates bindMode see a one-time 20s Notice on next
load pointing them at the new setting; fresh installs are silent.
UI: a new "Network binding" section between server config and the
HTTPS section, with a live colored verdict badge that re-classifies
on every change, the bind dropdown (Loopback / All interfaces /
Custom), an inline red caution under "All interfaces" whose wording
adapts to whether HTTPS is enabled, and a custom-host text field
that normalizes loopback/wildcard aliases on blur (typing 127.0.0.1
collapses the dropdown to Loopback; 0.0.0.0 collapses to All).
Tests: 7 integration assertions verifying the source wires listen()
to the resolved host, pushes verdict-derived instructions into the
pool, conditionally injects on 🔴 only, and that the pool spread-
pattern omits the field on ok/warn.
Inspired by PR #208 (fa1k3) — preserves the security instinct while
keeping the LAN/remote escape hatch the hardcoded approach removed.
|
✅ Build succeeded! Artifacts are available in the Actions tab. |
Self-reviewVerdict: Request changes — one ADR-fidelity miss that makes rows 5/7/9 of the table unreachable from the UI, plus a narrow migration race and a missing affordance from the ADR. None block the security goal (the dangerous default is fixed), but row 7 (HTTPS + custom-non-loopback + user cert = 🟢) is the documented happy path for legitimate LAN deployments and the implementation can't reach it. Blocking1. Both call sites compute
Consequence: states 5, 7, 9 (the user-supplied-cert rows) are unreachable from the UI. ADR-107 explicitly calls row 7 "the intended LAN/remote deployment" and row 9 "the intended public deployment". The implementation classifies both as warn/jail. Not a security regression — we over-warn rather than under-warn — but the ADR claims behaviour the code doesn't deliver. Fix: replace the signal with the actual provenance question: userSuppliedCert: !!(cfg?.certPath && cfg?.keyPath) && cfg?.autoGenerate !== trueor similar. Add a unit test that exercises rows 5, 7, 9 from a settings shape that the UI can actually produce. Non-blocking2. Migration Notice races the apiKey auto-save
Narrow: only triggers for users upgrading who also lack an Fix: capture the pre-bindMode signal inside 3.
Fix: tighten to a numeric check, e.g. 4. ADR says 🔴 badge includes a ADR-107 §"Settings UI": "When the badge is 🔴 it includes a 5. UX edge:
What's solid
Closes/supersedes #208 as planned. Plan: address (1) in this PR before merge; file follow-ups for (2)–(5) if not bundled. |
Blocking — userSuppliedCert discriminator: - Was: certificateConfig.selfSigned === false (never set false in UI; rows 5/7/9 of the ADR-107 table unreachable, defaulting to over-warn). - Now: !!(certPath && keyPath) — actual cert provenance signal. Fixed in src/mcp-server.ts and src/main.ts settings UI badge. Non-blocking #1 — migration Notice race: - Was: isExistingPreBindModeInstall() re-read data.json after loadSettings() may have already persisted bindMode='loopback' as part of the apiKey auto-save. - Now: snapshot raw loadData() before loadSettings(), test the snapshot. Non-blocking #2 — loopback host matcher tightened: - Was: startsWith('127.') accepted '127.evil.com'. - Now: strict dotted-quad regex /^127(\.\d{1,3}){3}$/ with octet range. New tests cover hostile inputs (127.evil.com, 127.0.0.1.attacker.tld, 127.300.0.1, 1270.0.0.1). Non-blocking #3 — Reconfigure hint: - ADR-107 specifies the 🔴 badge includes a Reconfigure hint; implementation now renders 'Reconfigure: switch the bind address below to Loopback, or enable HTTPS.' beneath the verdict reason. Non-blocking #4 — empty custom host UX: - When bindMode='custom' && customBindHost is empty, render an explicit Notice that the server will fall back to loopback until a host is entered (resolves the silent 🟢 OK in that state). Lint clean, 296/296 tests pass.
|
✅ Build succeeded! Artifacts are available in the Actions tab. |
1 participant
Summary
Models the combined
(protocol × bind × certSource)state as a 9-cell classified state machine. Replaces the implicit0.0.0.0Node-default bind with a deliberate, classified network exposure model. Server always starts; 🔴 surfaces aNotice+Debug.error+ agent-visible MCPinitialize.instructionswarning so the LLM client also sees it.docs/architecture/core/ADR-107-network-exposure-modes-as-a-classified-state-machine.md(Accepted in this branch)What's in this PR
docs/adr— ADR-107 captures the 9-cell table, the "always start / loud warning" stance, agent-visibility design, migration approach, and rejected alternatives.src/utils/network-classifier.ts— pureclassify()+resolveListenHost()+resolveBindAxis()+normalizeBindInput()+classifyFromSettings()+agentInstructionsForVerdict().src/main.ts—bindMode+customBindHost+hasShownBindMigrationNoticesettings + one-time post-upgrade Notice + new "Network binding" settings section with live verdict badge, bind dropdown, custom-host field with blur-time alias normalization, and inline red caution on "All interfaces".src/mcp-server.ts— bind-time host resolution, verdict classification,Notice + Debug.erroron 🔴,Debug.warnon 🟡, pushes the agent-visible warning into the server pool.src/utils/mcp-server-pool.ts—setInitializeInstructions()setter, conditional spread into the SDKServerconstructor options so theinstructionsfield is only included when the verdict is 🔴.src/node-mcp-server.ts— hardcoded loopback (no plugin-ref to plumb through; consistent with new default).styles.css— badge + caution styling.State machine
Migration
bindMode: 'loopback', badge shows 🟢, no Notice.data.jsonlackingbindMode→ defaults applied (loopback), one-time 20s Notice on first load points the user at the new setting. Flag persisted so the Notice never re-fires.Test plan
npm run build— cleannpm run lint— 0 errors (5 pre-existing warnings unchanged)npm test— 291/291 passinitialize.instructionspopulated when a client connects127.0.0.1and auto-collapses to Loopback on blur0.0.0.0and auto-collapses to All interfaces on blurdata.json) shows the one-time migration Notice on first load after upgrade🤖 Implementation drafted with Claude Code.