diff --git a/docs/architecture/INDEX.md b/docs/architecture/INDEX.md index bf3a2e2..48e610d 100644 --- a/docs/architecture/INDEX.md +++ b/docs/architecture/INDEX.md @@ -27,6 +27,7 @@ _Server architecture, transport, connection handling, plugin lifecycle_ | [ADR-104](./core/ADR-104-offload-cpu-bound-semantic-operations-to-worker-threads-and-deconflict-the-sse-route.md) | Offload CPU-bound semantic operations to worker threads and deconflict the SSE route | Accepted | | [ADR-105](./core/ADR-105-remove-dormant-worker-offload-path-partial-reversal-of-adr-104.md) | Remove dormant worker-offload path (partial reversal of ADR-104) | Accepted | | [ADR-106](./core/ADR-106-client-driven-session-re-initialization-via-spec-compliant-http-404.md) | Client-driven session re-initialization via spec-compliant HTTP 404 | Accepted | +| [ADR-107](./core/ADR-107-network-exposure-modes-as-a-classified-state-machine.md) | Network exposure modes as a classified state machine | Accepted | ## Tools & API _MCP tool design, semantic operations, graph operations, formatters_ diff --git a/docs/architecture/core/ADR-107-network-exposure-modes-as-a-classified-state-machine.md b/docs/architecture/core/ADR-107-network-exposure-modes-as-a-classified-state-machine.md new file mode 100644 index 0000000..a86469e --- /dev/null +++ b/docs/architecture/core/ADR-107-network-exposure-modes-as-a-classified-state-machine.md @@ -0,0 +1,297 @@ +--- +status: Accepted +date: 2026-05-24 +deciders: + - aaronsb + - claude +related: + - ADR-103 +--- + +# ADR-107: Network exposure modes as a classified state machine + +## Context + +The MCP HTTP server's exposure is determined by the combination of three +independent settings — **protocol** (HTTP / HTTPS), **bind address** (which +network interface(s) `server.listen` attaches to), and **certificate +provenance** (self-signed vs. user-supplied) — but the code today treats +each as an isolated knob with no awareness of the combined state. + +Two concrete consequences of that: + +1. **The bind address has no setting at all.** `this.server.listen(this.port, …)` + is called without a host argument (`src/mcp-server.ts:638`, + `src/node-mcp-server.ts:52`), so Node binds the wildcard (`0.0.0.0`) — + every interface, LAN-reachable. There is no UI to change this and no + documentation that it is the default. ADR-103 §"Zero-config" already + states *"The server listens on `127.0.0.1` over HTTP by default"* — that + premise is aspirational, not actual; this ADR is partly what makes it + true. +2. **The dangerous combination is silent.** A user who toggles "HTTPS + disabled" (the default) is, today, serving the API key and vault + contents in cleartext to every interface — including the coffee-shop + wifi the laptop is on. The plugin offers no signal that this state is + different in kind from `http + 127.0.0.1`, even though one is local-only + and the other is broadcast. + +Community PR #208 (fa1k3, 2026-05-20) flags the bind half of this and +proposes hardcoding `127.0.0.1`. That fixes the default but removes the +escape hatch for the legitimate "Obsidian on machine A, MCP client on +machine B" deployments the plugin's HTTPS path implies are supported. + +A fair counter is that most competently-administered systems already +have a host firewall (Linux `ufw`/`firewalld`/`nftables`, macOS +Application Firewall, Windows Defender Firewall) that would silently +drop unsolicited LAN connections regardless of what the plugin binds +to. That's often true, and where it holds, today's implicit `0.0.0.0` +bind is harmless in practice. The plugin nonetheless **cannot detect, +depend on, or assume** that posture — the population includes +freshly-installed machines, end-user laptops where the firewall has +been disabled to make some other tool work, containers and VMs with +permissive network policies, and the long tail of "I thought it was +on." Outsourcing the default to a firewall the plugin can't observe +is the wrong direction; the plugin owns its own default. Firewall +hardening on the host stays out of scope. + +The deeper observation is that **risk is a function of the combination, +not any single axis**. Cleartext on loopback is fine; cleartext on any +other interface is a leak. Self-signed TLS on loopback is fine; +self-signed TLS on a LAN interface is encrypted but trust-on-first-use. +Real cert on `0.0.0.0` is the intended remote-access deployment. A scheme +that warns or guides on a single axis will misclassify these — either +nagging the safe cases or staying silent on the unsafe ones. The right +shape is a small finite state machine that classifies the combined state +and surfaces that classification both in the UI and to the LLM client. + +## Decision + +**Model network exposure as a 9-state classified state machine. Resolve +the combined state at server-bind time with a pure `classify()` function. +Surface the verdict in the settings UI and — for the dangerous-tier only +— in the MCP `initialize` instructions so the LLM client also sees it. +Never refuse to bind: warn loudly, always start.** + +### Settings + +Replace the implicit Node-default bind with three new settings that +together with the existing `httpEnabled`/`httpsEnabled`/`certificateConfig` +fully specify the network state: + +```ts +interface MCPPluginSettings { + // existing... + httpEnabled: boolean; + httpPort: number; + httpsEnabled: boolean; + httpsPort: number; + certificateConfig: CertificateConfig; // .selfSigned discriminates cert source + // new: + bindMode: 'loopback' | 'all' | 'custom'; // default: 'loopback' + customBindHost: string; // consulted only when bindMode === 'custom' +} +``` + +`bindMode` is a discriminated enum, not a free-text host, so the common +intents (loopback / wildcard) cannot be typoed and the UI can render +three labelled choices instead of a string field full of footguns. The +custom path remains for the genuine LAN-bind case. + +The settings UI normalizes custom input on blur: + +- `127.0.0.1` / `localhost` / `::1` → switch dropdown to **Loopback**, + blank `customBindHost` +- `0.0.0.0` / `::` → switch dropdown to **All interfaces**, blank + `customBindHost` +- anything else → keep **Custom**, persist as-typed + +This means the user expressing "loopback" three different ways all +converge to the same internal state, which the classifier can reason +about cleanly. + +### State machine + +Three axes, with cert only consulted under HTTPS, give nine distinct +states. Each gets one of three verdicts: 🟢 OK, 🟡 WARN, 🔴 JAIL. + +| # | Protocol | Bind | Cert | Class | Why | +|---|----------|----------|------|-------|-----| +| 1 | http | loopback | — | 🟢 OK | Traffic never leaves the machine — encryption moot | +| 2 | http | custom-non-loopback | — | 🔴 JAIL | Vault contents + API key travel cleartext on a wire | +| 3 | http | all | — | 🔴 JAIL | Same, broadcast to every interface | +| 4 | https | loopback | self | 🟢 OK | Self-signed fine on loopback; no MITM surface | +| 5 | https | loopback | user | 🟢 OK | Overkill but harmless | +| 6 | https | custom-non-loopback | self | 🟡 WARN | Encrypted, but client must TOFU the cert | +| 7 | https | custom-non-loopback | user | 🟢 OK | The intended LAN/remote deployment | +| 8 | https | all | self | 🟡 WARN | Encrypted with TOFU; wider exposure | +| 9 | https | all | user | 🟢 OK | The intended public deployment | + +The classification function is pure and table-driven: + +```ts +type NetworkState = { + protocol: 'http' | 'https'; + bind: 'loopback' | 'all' | 'custom-loopback' | 'custom-other'; + certSource: 'self' | 'user'; // only meaningful under https +}; + +type Verdict = { class: 'ok' | 'warn' | 'jail'; reason: string }; + +function classify(s: NetworkState): Verdict { /* the table above */ } +``` + +`bind` is resolved from `bindMode` + `customBindHost` before classification: +a custom value that resolves to a loopback alias (`127.x.x.x` / `::1` / +`localhost`) classifies as `custom-loopback` and is treated as loopback +for risk purposes; everything else is `custom-other`. + +### Server-bind behavior + +At server start, after resolving the listen host: + +1. Compute `verdict = classify(currentState)`. +2. **Always call `listen(port, resolvedHost)`** — the server starts in + every state including 🔴. We do not gate startup on the verdict. +3. If `verdict.class === 'jail'`: emit a `Notice` ("⚠️ MCP server is + serving vault contents over an unencrypted network interface. API key + and document text travel in cleartext. Reconfigure to HTTPS or + loopback in the plugin settings.") and log a `Debug.error`. Persist a + small per-session flag so the Notice is shown once per process boot, + not on every request. +4. If `verdict.class === 'warn'`: log a `Debug.warn` summarizing the + trade-off. No `Notice` — the settings-pane badge handles the visual. + +### Agent-visible hint (🔴 only) + +The MCP `initialize` response includes an `instructions` string field per +the protocol. When the current verdict is 🔴, that field is populated +with a warning the LLM will see on its first turn against the server — +something like: + +``` +SECURITY WARNING: This Obsidian MCP server is configured to serve vault +contents over an unencrypted network interface ({resolvedHost}:{port}). +The API key and all document text travel in cleartext over the network. +If the user did not intend this, advise them to reconfigure the plugin +to use HTTPS or to bind to loopback (127.0.0.1) only. +``` + +When the verdict is 🟡 or 🟢, the `instructions` field carries the normal +plugin description (or is absent). The agent is *only* nagged when the +state genuinely warrants it. `http + 127.0.0.1` is silent because there +is nothing to warn about. + +### Settings UI + +The network section of the settings tab gains a **mode badge** at the +top that reflects the live verdict — a colored pill with a one-line +explanation. Changing any control (protocol toggle, bind dropdown, +custom host field, cert path) re-runs `classify()` and updates the +badge without saving. When the badge is 🔴 it includes a `Reconfigure` +hint pointing at the bind dropdown. + +The bind dropdown labels are explicit: + +- *Loopback only — local machine* (recommended) +- *All interfaces — anyone on the network can attempt to connect* +- *Custom address…* + +The "All interfaces" choice renders an inline red caution beneath it +whenever it is selected, regardless of protocol — the caution language +shifts based on whether HTTPS is also on (cleartext vs. TOFU-encrypted). + +### Migration + +`bindMode` defaults to `'loopback'` and `customBindHost` to `''`. On the +upgrade-from-old-settings path, missing fields take these defaults, so +existing installs that had been implicitly serving `0.0.0.0` (every +install today) land on `127.0.0.1`. This is a deliberate behaviour +change for the small fraction of users who were relying on LAN access: +they will see "connection refused" from their LAN client, open settings, +flip the bind dropdown to **All interfaces**, observe the red caution, +and proceed knowingly. The one-time disruption is the price of fixing +the insecure default; preserving the old behaviour for upgrades would +half-defeat the change. A first-run-after-upgrade `Notice` ("Network +binding now defaults to loopback — open MCP settings if you previously +relied on LAN access") softens the transition without preserving the +default. + +## Consequences + +### Positive + +- The dangerous-by-default state (HTTP on every interface, no warning) + ceases to exist. New installs and upgrades land in 🟢 unless the user + explicitly opts into a riskier state with the warning visible. +- The combined state is named and classified, so future changes + (additional axes, new transports) extend the table rather than spawn + new ad-hoc warnings. +- The LLM client gets the same security signal the user does in the 🔴 + case — useful when the user has handed Claude Code or Claude Desktop + the connection and isn't watching the plugin UI. +- ADR-103's "loopback by default" premise becomes actually true, + retroactively justifying the analysis there. +- The escape hatch for legitimate LAN/remote deployments remains — + fa1k3's hardcoded-loopback approach would have removed it; this design + preserves the capability while making the trade-off visible. + +### Negative + +- Existing installs that depended on the implicit `0.0.0.0` bind break + on upgrade and require two clicks to restore. Migration `Notice` + helps but does not eliminate the friction. +- More settings surface area (one enum, one string) and more UI (the + badge, the inline caution, the dropdown). The configurability budget + this spends is real. +- The `classify()` table is a small piece of policy now living in code. + Future protocol additions (e.g., Unix sockets, IPC) need to extend + both the axis enums and the table consistently. + +### Neutral + +- The MCP `initialize` instructions field is being used as a warning + channel only in the 🔴 case. Other future uses of that field need to + cooperate (compose with the warning, or accept that the warning takes + precedence when present). +- Self-signed-on-LAN (rows 6, 8) is 🟡 rather than 🔴 because the + encryption is real even if the trust model is TOFU. Users who consider + this insufficient can supply a real cert and the state moves to 🟢. +- Server startup behaviour is unchanged in the bind-success case — the + classification is observational, not gating. Removing the gate + (vs. an earlier proposal to refuse-to-start in 🔴) keeps the plugin + from appearing broken to upgrading users and matches the existing + "warn loudly, let the user decide" posture elsewhere in the plugin. + +## Alternatives Considered + +- **Hardcoded loopback (PR #208 as submitted).** Fixes the default but + removes the escape hatch for legitimate LAN/remote deployments the + plugin's HTTPS path is built for. Rejected as too coarse — the right + default with the wrong shape. fa1k3's PR will be thanked and closed + with a pointer to this ADR; the security instinct is the inspiration + for the work. +- **Free-text bind-host setting with prose warnings.** A single string + field for the bind address with a tooltip explaining the risks. + Rejected because tooltips don't get read, and a string field offers + no way to express "I want loopback" without re-deriving the address — + the discriminated enum makes intent first-class. +- **Refuse-to-start in 🔴 with an "I accept the risk" override toggle.** + Earlier draft of this design. Rejected on UX grounds: a user who + upgrades and lands in 🔴 because of a deliberate-but-undeclared + LAN setup will perceive the plugin as broken until they read the + Notice and find the override. "Loud warning, always start" matches + the plugin's posture elsewhere (e.g., `dangerouslyDisableAuth` + warns but does not refuse) and treats the user as capable of acting + on the warning. +- **Heuristically lenient classification for RFC1918 private ranges.** + Treat `192.168.x.x` / `10.x.x.x` / `172.16-31.x.x` more leniently + than a public IP. Rejected as overreach — a private IP on a hostile + network (corporate guest wifi, conference network, shared VPS in + bridged mode) is no safer than a public one, and the protocol/cert + axes already handle the actual encryption risk. Pretending the bind + value alone tells us about network trust would be false confidence. +- **Always-on agent-visible warning regardless of state.** Pin the + warning into MCP `initialize` instructions for every state. Rejected + because nagging the 🟢 cases would train both users and models to + ignore the channel — the warning loses force precisely when it + matters most. diff --git a/src/main.ts b/src/main.ts index abad587..377307c 100644 --- a/src/main.ts +++ b/src/main.ts @@ -8,6 +8,7 @@ import { PluginDetector } from './utils/plugin-detector'; import { CertificateConfig } from './utils/certificate-manager'; import { ValidationConfig } from './validation/input-validator'; import { ALL_OPERATIONS, getActionsForOperation, getOperationDescription } from './tools/semantic-tools'; +import { BindMode, classifyFromSettings, normalizeBindInput } from './utils/network-classifier'; interface MCPPluginSettings { httpEnabled: boolean; @@ -15,6 +16,10 @@ interface MCPPluginSettings { httpsEnabled: boolean; httpsPort: number; certificateConfig: CertificateConfig; + // ADR-107: network exposure modes + bindMode: BindMode; + customBindHost: string; + hasShownBindMigrationNotice: boolean; debugLogging: boolean; showConnectionStatus: boolean; autoDetectPortConflicts: boolean; @@ -63,6 +68,9 @@ const DEFAULT_SETTINGS: MCPPluginSettings = { // server (no requestCert); cert-manager defaults it to true. See #163. minTLSVersion: 'TLSv1.2' }, + bindMode: 'loopback', + customBindHost: '', + hasShownBindMigrationNotice: false, debugLogging: false, showConnectionStatus: true, autoDetectPortConflicts: true, @@ -94,9 +102,33 @@ export default class ObsidianMCPPlugin extends Plugin { Debug.log(`🚀 Starting Semantic Notes Vault MCP v${getVersion()}`); try { + // ADR-107: snapshot raw persisted data BEFORE loadSettings(), + // since loadSettings may write a fresh apiKey and (with merged + // defaults) bake in bindMode='loopback', erasing the "this is + // an upgrading install" signal we need below. + const rawDataBeforeLoad = (await this.loadData()) as Partial | null; + const wasExistingPreBindModeInstall = !!rawDataBeforeLoad && rawDataBeforeLoad.bindMode === undefined; + await this.loadSettings(); Debug.setDebugMode(this.settings.debugLogging); Debug.log('✅ Settings loaded'); + + // ADR-107: one-time post-upgrade migration notice when defaults + // flipped the implicit 0.0.0.0 bind to loopback. Suppresses on + // fresh installs where the user already saw the default; only + // fires when settings existed but the field did not. + if (this.settings.hasShownBindMigrationNotice === false) { + if (wasExistingPreBindModeInstall) { + new Notice( + 'MCP plugin: network binding now defaults to loopback only. ' + + 'If you previously accessed the MCP server from another machine on your LAN, ' + + 'open MCP settings → Network binding and switch to "All interfaces" or "Custom".', + 20000 + ); + } + this.settings.hasShownBindMigrationNotice = true; + await this.saveSettings(); + } // Debug log read-only mode status at startup if (this.settings.readOnlyMode) { @@ -285,7 +317,7 @@ export default class ObsidianMCPPlugin extends Plugin { async loadSettings() { this.settings = Object.assign({}, DEFAULT_SETTINGS, await this.loadData() as Partial); - + // Generate API key on first load if not present if (!this.settings.apiKey) { this.settings.apiKey = this.generateApiKey(); @@ -293,6 +325,7 @@ export default class ObsidianMCPPlugin extends Plugin { Debug.log('🔐 Generated new API key for authentication'); } } + public generateApiKey(): string { // Generate a secure random API key @@ -561,7 +594,10 @@ class MCPSettingTab extends PluginSettingTab { // Server Configuration Section this.createServerConfigSection(containerEl); - + + // Network Binding Section (ADR-107) + this.createNetworkBindingSection(containerEl); + // HTTPS Configuration Section this.createHTTPSConfigSection(containerEl); @@ -730,6 +766,94 @@ class MCPSettingTab extends PluginSettingTab { })); } + private createNetworkBindingSection(containerEl: HTMLElement): void { + new Setting(containerEl).setName("Network binding").setHeading(); + + // ADR-107: live verdict badge + const verdict = classifyFromSettings({ + httpsEnabled: this.plugin.settings.httpsEnabled, + bindMode: this.plugin.settings.bindMode, + customBindHost: this.plugin.settings.customBindHost, + userSuppliedCert: !!(this.plugin.settings.certificateConfig?.certPath + && this.plugin.settings.certificateConfig?.keyPath) + }); + const badgeEmoji = verdict.class === 'ok' ? '🟢' : verdict.class === 'warn' ? '🟡' : '🔴'; + const badgeLabel = verdict.class === 'ok' ? 'OK' : verdict.class === 'warn' ? 'WARN' : 'INSECURE'; + const badgeEl = containerEl.createDiv({ cls: `mcp-network-badge mcp-network-badge-${verdict.class}` }); + badgeEl.createEl('strong', { text: `${badgeEmoji} ${badgeLabel} — ` }); + badgeEl.createSpan({ text: verdict.reason }); + if (verdict.class === 'jail') { + badgeEl.createEl('br'); + badgeEl.createSpan({ + text: 'Reconfigure: switch the bind address below to Loopback, or enable HTTPS.', + cls: 'mcp-network-badge-hint' + }); + } + + new Setting(containerEl) + .setName('Bind address') + .setDesc('Which network interface the mcp server listens on. Loopback only is recommended.') + .addDropdown(dropdown => dropdown + .addOption('loopback', 'Loopback only — local machine') + .addOption('all', 'All interfaces — anyone on the network can attempt to connect') + .addOption('custom', 'Custom address…') + .setValue(this.plugin.settings.bindMode) + .onChange(async (value: string) => { + const mode = value as BindMode; + this.plugin.settings.bindMode = mode; + if (mode !== 'custom') { + this.plugin.settings.customBindHost = ''; + } + await this.plugin.saveSettings(); + this.display(); + await this.restartIfRunning('bind address'); + })); + + if (this.plugin.settings.bindMode === 'all') { + const caution = containerEl.createDiv({ cls: 'mcp-network-caution' }); + caution.createEl('strong', { text: '⚠ All interfaces selected. ' }); + caution.createSpan({ + text: this.plugin.settings.httpsEnabled + ? 'Encrypted via HTTPS — clients must trust the certificate. Use a real (non-self-signed) cert for public networks.' + : 'API key and document text will be sent in cleartext over the network. Enable HTTPS or switch to loopback.' + }); + } + + if (this.plugin.settings.bindMode === 'custom') { + if (this.plugin.settings.customBindHost.trim() === '') { + const empty = containerEl.createDiv({ cls: 'mcp-network-caution' }); + empty.createSpan({ text: 'No custom address entered yet — server will fall back to loopback (127.0.0.1) until you enter one.' }); + } + new Setting(containerEl) + .setName('Custom bind address') + .setDesc('IPv4/IPv6/hostname to bind to. Typing a loopback address auto-switches to loopback; typing a wildcard auto-switches to all interfaces.') + .addText(text => text + .setPlaceholder('e.g. 192.168.1.50') + .setValue(this.plugin.settings.customBindHost) + .onChange((value) => { + this.plugin.settings.customBindHost = value; + }) + .inputEl.addEventListener('blur', () => { + void (async () => { + const normalized = normalizeBindInput('custom', this.plugin.settings.customBindHost); + this.plugin.settings.bindMode = normalized.mode; + this.plugin.settings.customBindHost = normalized.customHost; + await this.plugin.saveSettings(); + this.display(); + await this.restartIfRunning('bind address'); + })(); + })); + } + } + + private async restartIfRunning(changedThing: string): Promise { + if (this.plugin.mcpServer?.isServerRunning()) { + new Notice(`Restarting server with new ${changedThing}...`); + await this.plugin.stopMCPServer(); + await this.plugin.startMCPServer(); + } + } + private createHTTPSConfigSection(containerEl: HTMLElement): void { new Setting(containerEl).setName("Secure transport").setHeading(); diff --git a/src/mcp-server.ts b/src/mcp-server.ts index ae0dd7d..4c6b11e 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -18,6 +18,13 @@ import { ConnectionPool, PooledRequest } from './utils/connection-pool'; import { SessionManager } from './utils/session-manager'; import { MCPServerPool } from './utils/mcp-server-pool'; import { CertificateManager, CertificateConfig } from './utils/certificate-manager'; +import { + classifyFromSettings, + resolveListenHost, + agentInstructionsForVerdict, + BindMode, + Verdict +} from './utils/network-classifier'; /** Minimal plugin interface for MCPHttpServer. * Includes fields from SecurePluginRef and ObsidianAPIPluginRef so the same object @@ -28,6 +35,9 @@ interface MCPPluginRef { httpsPort?: number; httpPort?: number; certificateConfig?: CertificateConfig; + // ADR-107: bind mode + custom host + bindMode?: BindMode; + customBindHost?: string; readOnlyMode?: boolean; apiKey?: string; dangerouslyDisableAuth?: boolean; @@ -94,6 +104,9 @@ export class MCPHttpServer { private sessionManager: SessionManager; private certificateManager: CertificateManager | null; private isHttps: boolean = false; + // ADR-107: resolved at bind time, consumed by initialize.instructions + private currentVerdict?: Verdict; + private resolvedListenHost: string = '127.0.0.1'; constructor(obsidianApp: App, port: number = 3001, plugin?: MCPPluginRef) { this.obsidianApp = obsidianApp; @@ -635,17 +648,48 @@ export class MCPHttpServer { Debug.error('Failed to configure server timeouts:', e); } - this.server.listen(this.port, () => { + // ADR-107: resolve bind host from settings and classify the combined state + const bindMode = this.plugin?.settings?.bindMode ?? 'loopback'; + const customHost = this.plugin?.settings?.customBindHost ?? ''; + this.resolvedListenHost = resolveListenHost(bindMode, customHost); + this.currentVerdict = classifyFromSettings({ + httpsEnabled: this.isHttps, + bindMode, + customBindHost: customHost, + userSuppliedCert: !!(this.plugin?.settings?.certificateConfig?.certPath + && this.plugin?.settings?.certificateConfig?.keyPath) + }); + // Push the agent-visible warning to the server pool so subsequent + // sessions surface it in MCP initialize.instructions. + this.mcpServerPool.setInitializeInstructions( + agentInstructionsForVerdict(this.currentVerdict, this.resolvedListenHost, this.port) + ); + + this.server.listen(this.port, this.resolvedListenHost, () => { this.isRunning = true; - Debug.log(`🚀 MCP server started on ${protocol}://localhost:${this.port}`); - Debug.log(`📍 Health check: ${protocol}://localhost:${this.port}/`); - Debug.log(`🔗 MCP endpoint: ${protocol}://localhost:${this.port}/mcp`); - + const host = this.resolvedListenHost; + Debug.log(`🚀 MCP server started on ${protocol}://${host}:${this.port}`); + Debug.log(`📍 Health check: ${protocol}://${host}:${this.port}/`); + Debug.log(`🔗 MCP endpoint: ${protocol}://${host}:${this.port}/mcp`); + if (this.isHttps) { Debug.log('🔒 HTTPS enabled with certificate'); new Notice(`MCP server running on HTTPS port ${this.port}`); } - + + // ADR-107: act on the classified verdict + const verdict = this.currentVerdict!; + if (verdict.class === 'jail') { + Debug.error(`🚨 Network exposure: ${verdict.reason}`); + new Notice( + `⚠️ MCP server is serving vault contents over an unencrypted network interface (${host}:${this.port}). ` + + 'API key and document text travel in cleartext. Reconfigure to HTTPS or loopback in the plugin settings.', + 15000 + ); + } else if (verdict.class === 'warn') { + Debug.warn(`⚠️ Network exposure: ${verdict.reason}`); + } + resolve(); }); diff --git a/src/node-mcp-server.ts b/src/node-mcp-server.ts index 0085944..ee6894b 100644 --- a/src/node-mcp-server.ts +++ b/src/node-mcp-server.ts @@ -48,10 +48,14 @@ export class NodeMCPServer { this.handleRequest(req, res); }); + // ADR-107: this fallback path has no plugin-settings hook; + // hardcode the loopback default. If this server is ever wired up + // live, extend it with the bindMode/customBindHost plumbing. + const host = '127.0.0.1'; await new Promise((resolve, reject) => { - this.server!.listen(this.port, () => { + this.server!.listen(this.port, host, () => { this.isRunning = true; - Debug.log(`🚀 MCP server started on port ${this.port}`); + Debug.log(`🚀 MCP server started on http://${host}:${this.port}`); Debug.log(`📍 Health check: /`); Debug.log(`🔗 MCP endpoint: /mcp`); resolve(); diff --git a/src/utils/mcp-server-pool.ts b/src/utils/mcp-server-pool.ts index 9024cfd..bfb7271 100644 --- a/src/utils/mcp-server-pool.ts +++ b/src/utils/mcp-server-pool.ts @@ -49,6 +49,9 @@ export class MCPServerPool extends EventEmitter { private plugin?: PluginWithSettings; private sessionManager?: SessionManager; private connectionPool?: ConnectionPool; + // ADR-107: agent-visible warning string injected into MCP initialize.instructions + // when the network exposure verdict is 'jail'. Null otherwise (no field sent). + private initializeInstructions: string | null = null; constructor(obsidianAPI: ObsidianAPI | SecureObsidianAPI, maxServers: number = 32, plugin?: PluginWithSettings) { super(); @@ -65,6 +68,15 @@ export class MCPServerPool extends EventEmitter { this.connectionPool = connectionPool; } + /** + * ADR-107: set the instructions string returned on MCP initialize. + * Called from MCPHttpServer.start() after the verdict is classified. + * Pass null to clear (no instructions field on initialize result). + */ + setInitializeInstructions(instructions: string | null) { + this.initializeInstructions = instructions; + } + /** * Get or create an MCP server for a session */ @@ -116,7 +128,9 @@ export class MCPServerPool extends EventEmitter { capabilities: { tools: {}, resources: {} - } + }, + // ADR-107: agent-visible network-exposure warning, only set when 🔴 + ...(this.initializeInstructions ? { instructions: this.initializeInstructions } : {}) } ); diff --git a/src/utils/network-classifier.ts b/src/utils/network-classifier.ts new file mode 100644 index 0000000..9acf9c7 --- /dev/null +++ b/src/utils/network-classifier.ts @@ -0,0 +1,180 @@ +/** + * ADR-107: Network exposure modes as a classified state machine. + * + * Pure functions consumed by server-bind code, settings UI, and the + * MCP `initialize` instructions injector — single source of truth for + * "how exposed is the current network configuration?" + */ + +export type BindMode = 'loopback' | 'all' | 'custom'; + +export type Protocol = 'http' | 'https'; + +export type CertSource = 'self' | 'user'; + +export type ResolvedBind = 'loopback' | 'all' | 'custom-loopback' | 'custom-other'; + +export interface NetworkState { + protocol: Protocol; + bind: ResolvedBind; + certSource: CertSource; +} + +export type VerdictClass = 'ok' | 'warn' | 'jail'; + +export interface Verdict { + class: VerdictClass; + reason: string; +} + +const LOOPBACK_ALIASES = new Set(['127.0.0.1', 'localhost', '::1', '::ffff:127.0.0.1']); +const WILDCARD_ALIASES = new Set(['0.0.0.0', '::']); +// 127.0.0.0/8 — only strict dotted-quad IPv4 in the loopback block, not +// arbitrary hostnames that happen to start with "127." (e.g. 127.evil.com). +const LOOPBACK_IPV4_RE = /^127(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}$/; + +function isLoopbackHost(host: string): boolean { + const h = host.trim().toLowerCase(); + if (LOOPBACK_ALIASES.has(h)) return true; + return LOOPBACK_IPV4_RE.test(h); +} + +function isWildcardHost(host: string): boolean { + return WILDCARD_ALIASES.has(host.trim()); +} + +/** + * If the user typed a loopback alias or wildcard into the custom field, + * collapse it to the corresponding explicit mode. Returns the canonical + * mode + the host to persist (blank for non-custom). + */ +export function normalizeBindInput( + mode: BindMode, + customHost: string +): { mode: BindMode; customHost: string } { + if (mode !== 'custom') return { mode, customHost: '' }; + const trimmed = customHost.trim(); + if (trimmed === '') return { mode: 'custom', customHost: '' }; + if (isLoopbackHost(trimmed)) return { mode: 'loopback', customHost: '' }; + if (isWildcardHost(trimmed)) return { mode: 'all', customHost: '' }; + return { mode: 'custom', customHost: trimmed }; +} + +/** + * Resolve settings to the actual host string passed to server.listen(). + */ +export function resolveListenHost(mode: BindMode, customHost: string): string { + switch (mode) { + case 'all': + return '0.0.0.0'; + case 'custom': { + const trimmed = customHost.trim(); + return trimmed === '' ? '127.0.0.1' : trimmed; + } + case 'loopback': + default: + return '127.0.0.1'; + } +} + +/** + * Resolve settings to the bind axis used by classify(). + * `custom` collapses to `custom-loopback` or `custom-other` based on + * whether the typed host is a loopback alias. + */ +export function resolveBindAxis(mode: BindMode, customHost: string): ResolvedBind { + if (mode === 'loopback') return 'loopback'; + if (mode === 'all') return 'all'; + const trimmed = customHost.trim(); + if (trimmed === '' || isLoopbackHost(trimmed)) return 'custom-loopback'; + return 'custom-other'; +} + +/** + * The full classification table from ADR-107. + */ +export function classify(state: NetworkState): Verdict { + const { protocol, bind, certSource } = state; + + if (protocol === 'http') { + if (bind === 'loopback' || bind === 'custom-loopback') { + return { + class: 'ok', + reason: 'HTTP on loopback — traffic never leaves this machine.' + }; + } + if (bind === 'all') { + return { + class: 'jail', + reason: + 'HTTP bound to every interface. API key and vault content travel in cleartext to anyone on the network.' + }; + } + return { + class: 'jail', + reason: + 'HTTP bound to a non-loopback address. API key and vault content travel in cleartext on the wire.' + }; + } + + // protocol === 'https' + if (bind === 'loopback' || bind === 'custom-loopback') { + return { + class: 'ok', + reason: 'HTTPS on loopback — self-signed certificate is fine here.' + }; + } + + if (certSource === 'user') { + return { + class: 'ok', + reason: + bind === 'all' + ? 'HTTPS on every interface with a user-supplied certificate — the intended public deployment.' + : 'HTTPS on a custom interface with a user-supplied certificate — the intended LAN/remote deployment.' + }; + } + + // self-signed on non-loopback + return { + class: 'warn', + reason: + bind === 'all' + ? 'HTTPS on every interface with a self-signed certificate — encrypted but clients must trust-on-first-use.' + : 'HTTPS on a custom interface with a self-signed certificate — encrypted but clients must trust-on-first-use.' + }; +} + +/** + * Convenience: build NetworkState from settings + classify in one call. + */ +export function classifyFromSettings(args: { + httpsEnabled: boolean; + bindMode: BindMode; + customBindHost: string; + userSuppliedCert: boolean; +}): Verdict { + return classify({ + protocol: args.httpsEnabled ? 'https' : 'http', + bind: resolveBindAxis(args.bindMode, args.customBindHost), + certSource: args.userSuppliedCert ? 'user' : 'self' + }); +} + +/** + * The agent-visible warning copy injected into MCP initialize.instructions + * when the verdict is `jail`. Returns null otherwise. + */ +export function agentInstructionsForVerdict( + verdict: Verdict, + resolvedHost: string, + port: number +): string | null { + if (verdict.class !== 'jail') return null; + return [ + 'SECURITY WARNING: This Obsidian MCP server is configured to serve vault contents', + `over an unencrypted network interface (${resolvedHost}:${port}). The API key and all`, + 'document text travel in cleartext over the network. If the user did not intend this,', + 'advise them to reconfigure the plugin to use HTTPS or to bind to loopback (127.0.0.1) only.' + ].join(' '); +} diff --git a/styles.css b/styles.css index 9c1244b..2cfa747 100644 --- a/styles.css +++ b/styles.css @@ -348,4 +348,43 @@ input.mcp-api-key-input { color: var(--text-muted); font-style: italic; margin-top: 15px; -} \ No newline at end of file +} +/* ADR-107: network exposure verdict badge + caution banner */ +.mcp-network-badge { + padding: 8px 12px; + margin: 8px 0 12px 0; + border-radius: 6px; + border-left: 4px solid; + background: var(--background-secondary); +} + +.mcp-network-badge-ok { + border-left-color: var(--color-green); +} + +.mcp-network-badge-warn { + border-left-color: var(--color-yellow); + background: var(--background-modifier-warning); +} + +.mcp-network-badge-jail { + border-left-color: var(--color-red); + background: var(--background-modifier-error); + color: var(--text-on-accent); +} + +.mcp-network-caution { + padding: 8px 12px; + margin: 4px 0 12px 24px; + border-radius: 4px; + background: var(--background-modifier-error); + color: var(--text-on-accent); + font-size: 0.9em; +} + +.mcp-network-badge-hint { + display: inline-block; + margin-top: 6px; + font-size: 0.9em; + opacity: 0.9; +} diff --git a/tests/network-classifier.test.ts b/tests/network-classifier.test.ts new file mode 100644 index 0000000..651caf6 --- /dev/null +++ b/tests/network-classifier.test.ts @@ -0,0 +1,195 @@ +import { + classify, + classifyFromSettings, + resolveListenHost, + resolveBindAxis, + normalizeBindInput, + agentInstructionsForVerdict, + NetworkState +} from '../src/utils/network-classifier'; + +describe('classify() — 9-cell ADR-107 table', () => { + const cases: Array<{ name: string; state: NetworkState; expected: 'ok' | 'warn' | 'jail' }> = [ + { name: '1: http + loopback', state: { protocol: 'http', bind: 'loopback', certSource: 'self' }, expected: 'ok' }, + { name: '2: http + custom-other', state: { protocol: 'http', bind: 'custom-other', certSource: 'self' }, expected: 'jail' }, + { name: '3: http + all', state: { protocol: 'http', bind: 'all', certSource: 'self' }, expected: 'jail' }, + { name: '4: https + loopback + self', state: { protocol: 'https', bind: 'loopback', certSource: 'self' }, expected: 'ok' }, + { name: '5: https + loopback + user', state: { protocol: 'https', bind: 'loopback', certSource: 'user' }, expected: 'ok' }, + { name: '6: https + custom-other + self',state: { protocol: 'https', bind: 'custom-other', certSource: 'self' }, expected: 'warn' }, + { name: '7: https + custom-other + user',state: { protocol: 'https', bind: 'custom-other', certSource: 'user' }, expected: 'ok' }, + { name: '8: https + all + self', state: { protocol: 'https', bind: 'all', certSource: 'self' }, expected: 'warn' }, + { name: '9: https + all + user', state: { protocol: 'https', bind: 'all', certSource: 'user' }, expected: 'ok' } + ]; + + for (const c of cases) { + test(c.name, () => { + const v = classify(c.state); + expect(v.class).toBe(c.expected); + expect(v.reason).toBeTruthy(); + }); + } + + test('custom-loopback under http classifies as ok (not jail)', () => { + expect(classify({ protocol: 'http', bind: 'custom-loopback', certSource: 'self' }).class).toBe('ok'); + }); + + test('custom-loopback under https + self classifies as ok (not warn)', () => { + expect(classify({ protocol: 'https', bind: 'custom-loopback', certSource: 'self' }).class).toBe('ok'); + }); +}); + +describe('normalizeBindInput()', () => { + test.each([ + ['127.0.0.1', 'loopback'], + ['localhost', 'loopback'], + ['LOCALHOST', 'loopback'], + ['::1', 'loopback'], + ['::ffff:127.0.0.1', 'loopback'], + ['127.0.0.99', 'loopback'], + ['127.255.255.254', 'loopback'], + [' 127.0.0.1 ', 'loopback'] + ])('"%s" collapses to loopback mode', (input, expectedMode) => { + const out = normalizeBindInput('custom', input); + expect(out.mode).toBe(expectedMode); + expect(out.customHost).toBe(''); + }); + + test.each([ + '127.evil.com', + '127.0.0.1.attacker.tld', + '127.300.0.1', + '1270.0.0.1' + ])('"%s" does NOT collapse to loopback (hostile/invalid)', (input) => { + const out = normalizeBindInput('custom', input); + expect(out.mode).toBe('custom'); + expect(out.customHost).toBe(input); + }); + + test.each([ + ['0.0.0.0', 'all'], + ['::', 'all'], + [' 0.0.0.0', 'all'] + ])('"%s" collapses to all mode', (input, expectedMode) => { + const out = normalizeBindInput('custom', input); + expect(out.mode).toBe(expectedMode); + expect(out.customHost).toBe(''); + }); + + test('arbitrary LAN address stays custom and is trimmed', () => { + const out = normalizeBindInput('custom', ' 192.168.1.50 '); + expect(out.mode).toBe('custom'); + expect(out.customHost).toBe('192.168.1.50'); + }); + + test('empty custom stays custom with empty host', () => { + expect(normalizeBindInput('custom', '')).toEqual({ mode: 'custom', customHost: '' }); + expect(normalizeBindInput('custom', ' ')).toEqual({ mode: 'custom', customHost: '' }); + }); + + test('non-custom modes blank the customHost', () => { + expect(normalizeBindInput('loopback', '192.168.1.5')).toEqual({ mode: 'loopback', customHost: '' }); + expect(normalizeBindInput('all', '192.168.1.5')).toEqual({ mode: 'all', customHost: '' }); + }); +}); + +describe('resolveListenHost()', () => { + test('loopback → 127.0.0.1', () => { + expect(resolveListenHost('loopback', '')).toBe('127.0.0.1'); + }); + + test('all → 0.0.0.0', () => { + expect(resolveListenHost('all', '')).toBe('0.0.0.0'); + }); + + test('custom → trimmed customHost', () => { + expect(resolveListenHost('custom', ' 192.168.1.50 ')).toBe('192.168.1.50'); + }); + + test('custom with empty host falls back to loopback (defensive)', () => { + expect(resolveListenHost('custom', '')).toBe('127.0.0.1'); + expect(resolveListenHost('custom', ' ')).toBe('127.0.0.1'); + }); +}); + +describe('resolveBindAxis()', () => { + test('loopback mode → loopback axis', () => { + expect(resolveBindAxis('loopback', '')).toBe('loopback'); + }); + + test('all mode → all axis', () => { + expect(resolveBindAxis('all', '')).toBe('all'); + }); + + test('custom with empty host → custom-loopback (defensive)', () => { + expect(resolveBindAxis('custom', '')).toBe('custom-loopback'); + }); + + test('custom with loopback alias → custom-loopback', () => { + expect(resolveBindAxis('custom', '127.0.0.1')).toBe('custom-loopback'); + expect(resolveBindAxis('custom', 'localhost')).toBe('custom-loopback'); + }); + + test('custom with LAN address → custom-other', () => { + expect(resolveBindAxis('custom', '192.168.1.50')).toBe('custom-other'); + }); +}); + +describe('classifyFromSettings()', () => { + test('default install (http + loopback) → ok', () => { + const v = classifyFromSettings({ + httpsEnabled: false, + bindMode: 'loopback', + customBindHost: '', + userSuppliedCert: false + }); + expect(v.class).toBe('ok'); + }); + + test('http + all → jail', () => { + const v = classifyFromSettings({ + httpsEnabled: false, + bindMode: 'all', + customBindHost: '', + userSuppliedCert: false + }); + expect(v.class).toBe('jail'); + }); + + test('https + all + self-signed → warn', () => { + const v = classifyFromSettings({ + httpsEnabled: true, + bindMode: 'all', + customBindHost: '', + userSuppliedCert: false + }); + expect(v.class).toBe('warn'); + }); + + test('https + all + user cert → ok', () => { + const v = classifyFromSettings({ + httpsEnabled: true, + bindMode: 'all', + customBindHost: '', + userSuppliedCert: true + }); + expect(v.class).toBe('ok'); + }); +}); + +describe('agentInstructionsForVerdict()', () => { + test('returns null for ok', () => { + expect(agentInstructionsForVerdict({ class: 'ok', reason: 'x' }, '127.0.0.1', 3001)).toBeNull(); + }); + + test('returns null for warn', () => { + expect(agentInstructionsForVerdict({ class: 'warn', reason: 'x' }, '0.0.0.0', 3443)).toBeNull(); + }); + + test('returns warning string for jail, includes host and port', () => { + const s = agentInstructionsForVerdict({ class: 'jail', reason: 'x' }, '0.0.0.0', 3001); + expect(s).not.toBeNull(); + expect(s).toContain('SECURITY WARNING'); + expect(s).toContain('0.0.0.0:3001'); + expect(s).toContain('cleartext'); + }); +}); diff --git a/tests/network-exposure-integration.test.ts b/tests/network-exposure-integration.test.ts new file mode 100644 index 0000000..8ff5b40 --- /dev/null +++ b/tests/network-exposure-integration.test.ts @@ -0,0 +1,78 @@ +import { readFileSync } from 'fs'; +import { join } from 'path'; +import { + classifyFromSettings, + agentInstructionsForVerdict, + resolveListenHost +} from '../src/utils/network-classifier'; + +describe('ADR-107 integration', () => { + test('mcp-server.ts passes resolveListenHost result to listen()', () => { + const src = readFileSync(join(__dirname, '../src/mcp-server.ts'), 'utf8'); + expect(src).toContain("resolveListenHost(bindMode, customHost)"); + expect(src).toContain('this.server.listen(this.port, this.resolvedListenHost,'); + }); + + test('mcp-server.ts pushes verdict-derived instructions to the server pool', () => { + const src = readFileSync(join(__dirname, '../src/mcp-server.ts'), 'utf8'); + expect(src).toContain('this.mcpServerPool.setInitializeInstructions('); + expect(src).toContain('agentInstructionsForVerdict(this.currentVerdict'); + }); + + test('mcp-server.ts fires a Notice and Debug.error on the jail verdict', () => { + const src = readFileSync(join(__dirname, '../src/mcp-server.ts'), 'utf8'); + expect(src).toMatch(/verdict\.class\s*===\s*'jail'/); + expect(src).toContain('Debug.error'); + expect(src).toContain('new Notice('); + }); + + test('mcp-server-pool.ts conditionally injects instructions into Server options', () => { + const src = readFileSync(join(__dirname, '../src/utils/mcp-server-pool.ts'), 'utf8'); + expect(src).toContain('setInitializeInstructions('); + expect(src).toContain('this.initializeInstructions'); + expect(src).toMatch(/\.\.\.\(this\.initializeInstructions \? \{ instructions: this\.initializeInstructions \} : \{\}\)/); + }); + + test('node-mcp-server.ts hardcodes loopback (ADR-107 default for unwired fallback)', () => { + const src = readFileSync(join(__dirname, '../src/node-mcp-server.ts'), 'utf8'); + expect(src).toContain("const host = '127.0.0.1'"); + expect(src).toContain('this.server!.listen(this.port, host,'); + }); + + test('verdict→instructions pipeline yields warning text for jail and null elsewhere', () => { + const jail = classifyFromSettings({ + httpsEnabled: false, + bindMode: 'all', + customBindHost: '', + userSuppliedCert: false + }); + const ok = classifyFromSettings({ + httpsEnabled: false, + bindMode: 'loopback', + customBindHost: '', + userSuppliedCert: false + }); + const warn = classifyFromSettings({ + httpsEnabled: true, + bindMode: 'all', + customBindHost: '', + userSuppliedCert: false + }); + + expect(agentInstructionsForVerdict(jail, '0.0.0.0', 3001)).toContain('SECURITY WARNING'); + expect(agentInstructionsForVerdict(ok, '127.0.0.1', 3001)).toBeNull(); + expect(agentInstructionsForVerdict(warn, '0.0.0.0', 3443)).toBeNull(); + }); + + test('default install state resolves to ok verdict and 127.0.0.1 host', () => { + const host = resolveListenHost('loopback', ''); + const v = classifyFromSettings({ + httpsEnabled: false, + bindMode: 'loopback', + customBindHost: '', + userSuppliedCert: false + }); + expect(host).toBe('127.0.0.1'); + expect(v.class).toBe('ok'); + }); +});