From 1b718b433ca26d7541100e9f8caf6a688d7d1341 Mon Sep 17 00:00:00 2001 From: Favour Ohans Date: Thu, 17 Sep 2026 04:08:02 +0100 Subject: [PATCH 1/4] Stdio MCP porting: structure travels with double consent, secrets never do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes the earlier rule that command-based servers are never applied: hooks are code and port with double consent, so stdio servers get the same treatment — with the one inviolable line intact. Classification extracts the re-creatable shape (command, args, env NAMES) and drops env values at extraction, so no downstream layer can see one. Portability is earned: any machine-local absolute path, bare script filename, or localhost URL keeps the server blocked. Export carries the structure only behind a repeatable --mcp , symmetric to hooks, and the travel picker gains an opt-in MCP group whose hints show the command and the env names it needs. Apply revalidates the untrusted manifest entry in full (name shape, control-byte-free strings, env-name shape, size caps) before assembling the claude mcp add argv — no shell anywhere. Env values resolve on the target: the guided apply asks with a new masked prompt (asterisks on screen, plaintext only in memory; plain mode says its input is visible), and the static path reads the machine's own environment, failing closed by variable name before any file writes — deliberately no flag, so secrets never touch argv or shell history. A missing command binary warns and never refuses. Seven new tests: classification with a planted env value swept from every output, consent-gated manifest shape, picker group and flag echo, argv construction with every refusal, the static-path env round trip through a shim, the guided flow end to end proving the typed secret reaches argv but never a rendered frame, and the masked prompt unit. Threat model and README updated accordingly. --- README.md | 2 +- docs/THREAT-MODEL.md | 2 +- src/apply/mcp.ts | 79 +++++++++++++++++++++++++++++++++++-- src/commands/apply.ts | 14 ++++++- src/commands/export.ts | 5 ++- src/commands/guided.ts | 85 +++++++++++++++++++++++++++++++++++++--- src/export/collect.ts | 36 +++++++++++++++-- src/main.ts | 4 +- src/scan/classify.ts | 57 ++++++++++++++++++++++++++- src/scan/codex.ts | 1 + src/scan/opencode.ts | 1 + src/scan/scanner.ts | 1 + src/scan/types.ts | 1 + src/tui/components.ts | 29 +++++++++++++- src/tui/plain.ts | 11 ++++++ test/codex.test.mjs | 3 +- test/scan.test.mjs | 3 ++ test/stdio-mcp.test.mjs | Bin 0 -> 12104 bytes test/surface.test.mjs | 2 +- 19 files changed, 313 insertions(+), 23 deletions(-) create mode 100644 test/stdio-mcp.test.mjs diff --git a/README.md b/README.md index 5dabc71..4e2fcf6 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ The scanner reads an explicit allowlist of paths and nothing else. These are exc File contents are scanned at export too: a file whose content matches a token pattern (known key prefixes, private-key blocks, high-entropy strings) is refused by default and carried only after explicit consent — a y/N in the guided flow, `--allow-secret ` in flag mode. Findings name the file and line, never the matched value. -MCP server entries are recorded as a name plus a portability class. Secret values are never copied; a server that needs one gets flagged so you can re-enter it on the target. Hooks are shell commands, so each one is confirmed individually before it is included, and confirmed again on the machine that applies it. +MCP server entries are recorded as a name plus a portability class. Remote servers travel as name and sanitized URL. Command-based (stdio) servers can travel too, as structure only — the command, its args, and the NAMES of the env variables it needs — behind a repeatable `--mcp ` at export and again at apply; the values are typed fresh on the target (the guided apply asks with a masked prompt, the scripted path reads the target machine's own environment and fails closed naming the missing variable — deliberately no flag, so secrets never touch argv or shell history). Secret values are never copied anywhere. Hooks are shell commands, so each one is confirmed individually before it is included, and confirmed again on the machine that applies it. ## No telemetry diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index fc02098..784a8a8 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -28,7 +28,7 @@ Each guarantee names the code that enforces it. All of them are covered by tests **Writes cannot escape the target.** Beyond lexical path containment, every write path is walked component by component and refused if any existing component is a symlink. The target directory itself may be a symlink; nothing under it may be (`resolveForWrite` in `src/apply/apply.ts`). -**Code execution needs consent on both machines.** Hooks and `statusLine` entries are shell commands, and an enabled plugin installs marketplace code. They enter a bundle only when named with `--hook` or `--plugin` at export, and they apply only when named again at apply; a marketplace source survives the gate only when a confirmed plugin references it. Behind the gates sits a settings allowlist: a bundle's `settings.json` may carry only the keys export can produce (the portable preference keys, `statusLine`, `hooks`, `enabledPlugins`, `extraKnownMarketplaces`, each shape-checked), so keys that execute code on the target (`apiKeyHelper`, `env`, auth refresh scripts) refuse the bundle whole rather than riding past a consent filter that does not know them (`assertPortableSettings` in `src/apply/apply.ts`). A consequence with teeth: adding a new portable settings key is no longer a schema-invisible change, because an older apply will refuse a newer bundle that carries it — a future key must bump the manifest schema version, or knowingly accept that older applies fail loud on such bundles. The schema version itself is an advisory compatibility signal, not a trust boundary: every receive-side control (path allowlist, per-agent key allowlists, per-root containment) is enforced regardless of the schema a bundle claims, so no check may ever gate on it. MCP registration is opt-in per server with `--mcp`, goes through the agent's own CLI with name and URL only, and never carries tokens, headers, or environment values. +**Code execution needs consent on both machines.** Hooks and `statusLine` entries are shell commands, and an enabled plugin installs marketplace code. They enter a bundle only when named with `--hook` or `--plugin` at export, and they apply only when named again at apply; a marketplace source survives the gate only when a confirmed plugin references it. Behind the gates sits a settings allowlist: a bundle's `settings.json` may carry only the keys export can produce (the portable preference keys, `statusLine`, `hooks`, `enabledPlugins`, `extraKnownMarketplaces`, each shape-checked), so keys that execute code on the target (`apiKeyHelper`, `env`, auth refresh scripts) refuse the bundle whole rather than riding past a consent filter that does not know them (`assertPortableSettings` in `src/apply/apply.ts`). A consequence with teeth: adding a new portable settings key is no longer a schema-invisible change, because an older apply will refuse a newer bundle that carries it — a future key must bump the manifest schema version, or knowingly accept that older applies fail loud on such bundles. The schema version itself is an advisory compatibility signal, not a trust boundary: every receive-side control (path allowlist, per-agent key allowlists, per-root containment) is enforced regardless of the schema a bundle claims, so no check may ever gate on it. MCP registration is opt-in per server with `--mcp` on BOTH sides and goes through the agent's own CLI. Remote servers carry name and sanitized URL only. Command-based (stdio) servers carry structure only — command, args, env NAMES — with values dropped at classification time so no downstream layer can see one (superseding the earlier rule that stdio servers are never applied); on the target, values are typed fresh in the guided flow or read from the machine's own environment, the untrusted manifest entry is fully revalidated (string hygiene, env-name shape, size caps) before argv assembly, and no shell is ever involved. Servers pinned to a machine (local paths, localhost endpoints) remain blocked. Tokens, headers, and environment values never travel in any form. **Applies are reversible.** What an apply overwrites is backed up first, the record of the apply lands before the first destructive write, and `agent-sync undo` restores it or aborts untouched if the backup is incomplete. diff --git a/src/apply/mcp.ts b/src/apply/mcp.ts index b3f2d6c..b518e7a 100644 --- a/src/apply/mcp.ts +++ b/src/apply/mcp.ts @@ -3,17 +3,33 @@ import { sanitizeRemoteEndpoint } from "../scan/classify.js"; import type { ManifestMcpServer } from "../export/collect.js"; const SERVER_NAME = /^[A-Za-z0-9][A-Za-z0-9_.-]*$/; +const ENV_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; +const MAX_ARGS = 64; +const MAX_ENV_NAMES = 32; +const MAX_STRING_LENGTH = 2000; export interface McpRegistration { name: string; args: string[]; } +// Env values for a stdio server are resolved ON the applying machine, never +// carried: the guided flow prompts for them, the static path reads the +// target's own environment. +export type McpEnvResolver = (server: string, envName: string) => string | undefined; + +export function processEnvResolver(env: Record = process.env): McpEnvResolver { + return (_server, envName) => env[envName]; +} + // The manifest is untrusted input, so everything the export side sanitized is -// revalidated here before it can reach an argv. +// revalidated here before it can reach an argv. Stdio entries get the full +// treatment: name shape, control-byte-free strings, env-name shape, and size +// caps — then argv assembly with no shell anywhere. export function planMcpRegistrations( servers: ManifestMcpServer[], requested: string[], + resolveEnv: McpEnvResolver = processEnvResolver(), ): McpRegistration[] { const byName = new Map(servers.map((server) => [server.name, server])); const registrations: McpRegistration[] = []; @@ -23,6 +39,13 @@ export function planMcpRegistrations( if (server === undefined) { throw new Error(`--mcp ${name} does not match any server in this bundle's manifest.`); } + if (!SERVER_NAME.test(name)) { + throw new Error(`--mcp ${name}: server name is not safe to pass along. Nothing was registered.`); + } + if (server.transport === "stdio" || server.command !== undefined) { + registrations.push(planStdioRegistration(name, server, resolveEnv)); + continue; + } if (server.status !== "candidate") { throw new Error( `--mcp ${name} is not portable (${server.status}): ${server.reason} Nothing was registered.`, @@ -31,9 +54,6 @@ export function planMcpRegistrations( if (server.url === undefined) { throw new Error(`--mcp ${name}: the bundle records no re-addable URL for this server.`); } - if (!SERVER_NAME.test(name)) { - throw new Error(`--mcp ${name}: server name is not safe to pass along. Nothing was registered.`); - } const endpoint = sanitizeRemoteEndpoint(server.url); if (!endpoint.ok || endpoint.url === undefined) { throw new Error(`--mcp ${name}: ${endpoint.reason ?? "URL is not clean"}. Nothing was registered.`); @@ -48,6 +68,57 @@ export function planMcpRegistrations( return registrations; } +function planStdioRegistration( + name: string, + server: ManifestMcpServer, + resolveEnv: McpEnvResolver, +): McpRegistration { + if (typeof server.command !== "string" || !cleanString(server.command)) { + throw new Error(`--mcp ${name}: the bundle's command is not a clean string. Nothing was registered.`); + } + const args = server.args ?? []; + if (!Array.isArray(args) || args.length > MAX_ARGS || !args.every((arg) => typeof arg === "string" && cleanString(arg))) { + throw new Error(`--mcp ${name}: the bundle's args are not clean strings. Nothing was registered.`); + } + const envNames = server.envNames ?? []; + if ( + !Array.isArray(envNames) || + envNames.length > MAX_ENV_NAMES || + !envNames.every((envName) => typeof envName === "string" && ENV_NAME.test(envName)) + ) { + throw new Error(`--mcp ${name}: the bundle's env names are not valid variable names. Nothing was registered.`); + } + + const argv = ["mcp", "add", "--transport", "stdio", "--scope", "user", name]; + for (const envName of envNames) { + const value = resolveEnv(name, envName); + if (value === undefined || value.length === 0) { + throw new Error( + `--mcp ${name} needs ${envName}: set it in this machine's environment (or run the guided apply, which asks for it). Values are never carried in bundles.`, + ); + } + argv.push("--env", `${envName}=${value}`); + } + argv.push("--", server.command, ...args); + return { name, args: argv }; +} + +function cleanString(text: string): boolean { + return text.length > 0 && text.length <= MAX_STRING_LENGTH && !/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/.test(text); +} + +// A missing binary is a warning, not a refusal: the registration still +// lands, and the user may well install the runner right after applying. +export function commandOnPath(command: string): boolean { + const probe = process.platform === "win32" ? "where" : "which"; + try { + execFileSync(probe, [command], { stdio: ["ignore", "ignore", "ignore"] }); + return true; + } catch { + return false; + } +} + export function runMcpRegistration(registration: McpRegistration): void { try { execFileSync("claude", registration.args, { stdio: ["ignore", "pipe", "pipe"] }); diff --git a/src/commands/apply.ts b/src/commands/apply.ts index fe3f9f1..0f5b157 100644 --- a/src/commands/apply.ts +++ b/src/commands/apply.ts @@ -11,7 +11,7 @@ import { planApply, splitBundleByRoot, } from "../apply/apply.js"; -import { planMcpRegistrations, runMcpRegistration } from "../apply/mcp.js"; +import { commandOnPath, planMcpRegistrations, runMcpRegistration } from "../apply/mcp.js"; import { stringList } from "./export.js"; import { chooseEntry, runGuidedApply } from "./guided.js"; @@ -185,8 +185,18 @@ export const applyCommand: CommandDef = { } } + for (const registration of registrations) { + const server = bundle.manifest.mcpServers.find((candidate) => candidate.name === registration.name); + if (server?.transport === "stdio" && typeof server.command === "string" && !commandOnPath(server.command)) { + io.out(`Note: ${server.command} is not on this machine's PATH yet; the ${server.name} registration still lands.`); + } + } + const unregistered = bundle.manifest.mcpServers.filter( - (server) => server.status === "candidate" && server.url !== undefined && !requestedMcp.includes(server.name), + (server) => + !requestedMcp.includes(server.name) && + ((server.status === "candidate" && server.url !== undefined) || + (server.transport === "stdio" && server.command !== undefined)), ); if (unregistered.length > 0) { io.out( diff --git a/src/commands/export.ts b/src/commands/export.ts index b835ab9..1725a09 100644 --- a/src/commands/export.ts +++ b/src/commands/export.ts @@ -16,6 +16,7 @@ const HELP = [ " --plugin Include one plugin reference (repeatable), e.g. --plugin ponytail@ponytail", " --skip Leave one scanned item behind (repeatable), e.g. --skip skill/boxd-cli or --skip settings", " --allow-secret Carry a file despite a secret-content finding (repeatable, refuse-by-default)", + " --mcp Carry one command-based MCP server's structure (repeatable; env values never travel)", " --dry-run Print exactly what would be packed and write nothing", " --json Print the manifest as JSON instead of the summary", " --help Show help", @@ -30,6 +31,7 @@ export const exportCommand: CommandDef = { plugin: { type: "string", description: "Include one plugin reference (repeatable)", multiple: true }, skip: { type: "string", description: "Leave one scanned item behind (repeatable)", multiple: true }, "allow-secret": { type: "string", description: "Carry a file despite a secret-content finding (repeatable)", multiple: true }, + mcp: { type: "string", description: "Carry one command-based MCP server's structure (repeatable)", multiple: true }, "dry-run": { type: "boolean", description: "Print the packing list and write nothing" }, json: { type: "boolean", description: "Print the manifest as JSON" }, }, @@ -49,8 +51,9 @@ export const exportCommand: CommandDef = { const selectedPlugins = stringList(values.plugin); const skips = stringList(values.skip); const allowSecrets = stringList(values["allow-secret"]); + const selectedMcp = stringList(values.mcp); - const plan = collectExport({ confirmedHooks, selectedPlugins, skips, allowSecrets }); + const plan = collectExport({ confirmedHooks, selectedPlugins, skips, allowSecrets, selectedMcp }); if (plan.diagnostics.some((diagnostic) => diagnostic.severity === "error")) { for (const diagnostic of plan.diagnostics) io.err(`${diagnostic.severity}: ${diagnostic.message}`); diff --git a/src/commands/guided.ts b/src/commands/guided.ts index e9937b3..f501fe0 100644 --- a/src/commands/guided.ts +++ b/src/commands/guided.ts @@ -15,7 +15,7 @@ import { type AgentRoots, } from "../apply/apply.js"; import type { LoadedBundle } from "../apply/bundle.js"; -import { planMcpRegistrations, runMcpRegistration, type McpRegistration } from "../apply/mcp.js"; +import { commandOnPath, planMcpRegistrations, runMcpRegistration, type McpRegistration } from "../apply/mcp.js"; import type { JsonValue } from "../scan/classify.js"; import { commandSummary, scanClaudeCode } from "../scan/scanner.js"; import { scanCodex } from "../scan/codex.js"; @@ -58,6 +58,7 @@ interface Selections { skips: string[]; plugins: string[]; hooks: string[]; + mcp?: string[]; allowSecrets?: string[]; dest: string; } @@ -69,6 +70,7 @@ export function flagEcho(selections: Selections): string { for (const skip of [...selections.skips].sort()) parts.push("--skip", quote(skip)); for (const plugin of [...selections.plugins].sort()) parts.push("--plugin", quote(plugin)); for (const hook of [...selections.hooks].sort()) parts.push("--hook", quote(hook)); + for (const server of [...(selections.mcp ?? [])].sort()) parts.push("--mcp", quote(server)); for (const path of [...(selections.allowSecrets ?? [])].sort()) parts.push("--allow-secret", quote(path)); return parts.join(" "); } @@ -131,7 +133,28 @@ export function buildTravelGroups(report: ScanReport, others: ScanReport[] = []) }), }); } - // Exclusions no longer occupy the picker (the owner's call): the review's + const stdioServers = report.items.filter( + (item) => item.scope === "user" && item.kind === "mcp_server" && item.stdio !== undefined, + ); + if (stdioServers.length > 0) { + groups.push({ + title: "MCP servers \u00b7 commands, re-created with your consent", + items: stdioServers.map((item) => { + const stdio = item.stdio; + const entry: MultiGroup["items"][number] = { + value: `mcp/${item.name}`, + label: item.name, + preselected: false, + }; + if (stdio !== undefined) { + const envSuffix = stdio.envNames.length > 0 ? ` (needs ${stdio.envNames.join(", ")})` : ""; + entry.hint = `${[stdio.command, ...stdio.args].join(" ")}${envSuffix}`; + } + return entry; + }), + }); + } + // Exclusions no longer occupy the picker: the review's // closing line accounts for them where the leaves-the-machine story lives. return groups; } @@ -234,8 +257,9 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid const skips = groups .filter((group) => group.locked !== true) .flatMap((group) => group.items.map((item) => item.value)) - .filter((token) => !chosen.has(token) && !token.startsWith("plugin/")); + .filter((token) => !chosen.has(token) && !token.startsWith("plugin/") && !token.startsWith("mcp/")); const plugins = travel.filter((token) => token.startsWith("plugin/")).map((token) => token.slice("plugin/".length)); + const selectedMcp = travel.filter((token) => token.startsWith("mcp/")).map((token) => token.slice("mcp/".length)); let hooks: string[] = []; if (hookGroup !== null) { @@ -251,6 +275,7 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid const collectOptions: Parameters[0] = { confirmedHooks: hooks, selectedPlugins: plugins, + selectedMcp, skips, }; if (overrides.userDir !== undefined) collectOptions.userDir = overrides.userDir; @@ -303,7 +328,7 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid if (choice === "skip") { ui.outro( "Nothing was written.", - `To pack a different way, scripted: ${flagEcho({ skips, plugins, hooks, allowSecrets, dest })}`, + `To pack a different way, scripted: ${flagEcho({ skips, plugins, hooks, mcp: selectedMcp, allowSecrets, dest })}`, ); return 0; } @@ -319,7 +344,7 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid const destPath = join(overrides.destDir ?? process.cwd(), dest); writeDestination(destPath, dest, plan); - const selections: Selections = { skips, plugins, hooks, allowSecrets, dest }; + const selections: Selections = { skips, plugins, hooks, mcp: selectedMcp, allowSecrets, dest }; ui.outro( `Packed ${plan.entries.length} files (${formatBytes(totalBytes)}) -> ${destLabel(dest)}`, `Next time, scripted: ${flagEcho(selections)}`, @@ -547,7 +572,25 @@ export async function runGuidedApply( } const requestedMcp: string[] = []; + const stdioConsents: { name: string; command: string; envNames: string[] }[] = []; for (const server of bundle.manifest.mcpServers) { + if (server.transport === "stdio" && typeof server.command === "string") { + // The consent line is built from manifest fields, never from a + // planned argv, so no env value can exist yet to leak into it. + const envNames = server.envNames ?? []; + const envDisplay = envNames.map((envName) => `--env ${envName}=...`).join(" "); + const commandLine = [server.command, ...(server.args ?? [])].join(" "); + const consent = await ui.confirm( + `Register MCP server ${server.name}? (claude mcp add --transport stdio ${envDisplay}${envDisplay.length > 0 ? " " : ""}-- ${commandLine})`, + false, + ); + if (consent === null) return 2; + if (consent) { + requestedMcp.push(server.name); + stdioConsents.push({ name: server.name, command: server.command, envNames }); + } + continue; + } if (server.status !== "candidate" || server.url === undefined) continue; let registration: McpRegistration | undefined; try { @@ -564,12 +607,28 @@ export async function runGuidedApply( if (consent) requestedMcp.push(server.name); } + // Secrets are typed fresh on this machine, after consent and before any + // write; they live only in this map on their way into the argv. + const secretValues = new Map(); + for (const consent of stdioConsents) { + for (const envName of consent.envNames) { + const value = await ui.secret(`${consent.name} needs ${envName}`); + if (value === null) return 2; + secretValues.set(`${consent.name}\u0000${envName}`, value); + } + if (!commandOnPath(consent.command)) { + ui.note([`note: ${consent.command} is not on this machine's PATH yet; the registration still lands.`]); + } + } + // Assemble done; from here it is exactly the flag path. The split runs // AFTER the gates because gating rewrites settings.json in the bundle, // and every root is planned before the first root writes. gateSettingsHooks(bundle, confirmedHooks); gateSettingsPlugins(bundle, confirmedPlugins); - const registrations = planMcpRegistrations(bundle.manifest.mcpServers, requestedMcp); + const registrations = planMcpRegistrations(bundle.manifest.mcpServers, requestedMcp, (serverName, envName) => + secretValues.get(`${serverName}\u0000${envName}`), + ); const slices = splitBundleByRoot(bundle, roots); const planned = slices.map((slice) => ({ slice, plan: planApply(slice.bundle, slice.root) })); let creates = 0; @@ -641,6 +700,7 @@ interface GuidedUi { groupMultiselect(message: string, groups: MultiGroup[], coach?: string): Promise; select(message: string, items: typeof DESTINATIONS): Promise; confirm(message: string, initial?: boolean): Promise; + secret(message: string): Promise; review(lines: string[], dest: string): Promise<"pack" | "skip" | "dest" | null>; outro(...lines: string[]): void; close(): void; @@ -691,6 +751,11 @@ function pickerUi(overrides: GuidedOverrides & { wordmark?: boolean }): GuidedUi if (result.cancelled) open = false; return result.cancelled ? null : result.value; }, + async secret(message) { + const result = await flow.secret(message); + if (result.cancelled) open = false; + return result.cancelled ? null : result.value; + }, // The review screen: the bundle tree, then "Pack it?" with the // destination as a named default. y packs, n leaves, d changes the // destination, esc cancels. The picker still owns no writes. @@ -778,6 +843,14 @@ function plainUi(io: CommandIo, overrides: GuidedOverrides): GuidedUi { if (result.cancelled) io.err("Cancelled. Nothing was written."); return result.cancelled ? null : result.value; }, + async secret(message) { + const result = await plain.secret(message); + if (result.cancelled) { + io.err("Cancelled. Nothing was written."); + return null; + } + return result.value; + }, // Plain mode reads the same tree and answers one y/N; changing the // destination in plain mode is the scripted flags' job, which the echo // teaches on pack and on skip alike. diff --git a/src/export/collect.ts b/src/export/collect.ts index 8042b41..71038d6 100644 --- a/src/export/collect.ts +++ b/src/export/collect.ts @@ -47,7 +47,12 @@ export interface ManifestMcpServer { reason: string; envRefs?: string[]; url?: string; - transport?: "http" | "sse"; + transport?: "http" | "sse" | "stdio"; + // Present only when the exporter consented with --mcp: the re-creatable + // command shape. Env NAMES only; values never enter a manifest. + command?: string; + args?: string[]; + envNames?: string[]; } export interface ManifestHook { @@ -106,6 +111,7 @@ export interface CollectOptions { opencodeConfigDir?: string; confirmedHooks?: string[]; selectedPlugins?: string[]; + selectedMcp?: string[]; skips?: string[]; allowSecrets?: string[]; } @@ -123,6 +129,7 @@ export function collectExport(options: CollectOptions = {}): ExportPlan { const userDir = options.userDir ?? process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude"); const confirmedHooks = options.confirmedHooks ?? []; const selectedPlugins = options.selectedPlugins ?? []; + const selectedMcp = options.selectedMcp ?? []; const skips = new Set(options.skips ?? []); const allowSecrets = new Set(options.allowSecrets ?? []); @@ -231,6 +238,23 @@ export function collectExport(options: CollectOptions = {}): ExportPlan { } const includedPlugins = new Set(selectedPlugins.filter((name) => pluginNames.has(name))); + // Stdio definitions are code specs, so they travel only when named with + // --mcp — symmetric to hooks. Remote entries keep traveling as data. + const stdioCapable = new Set( + userItems + .filter((item) => item.kind === "mcp_server" && item.stdio !== undefined) + .map((item) => item.name), + ); + for (const requested of selectedMcp) { + if (!stdioCapable.has(requested)) { + diagnostics.push({ + severity: "error", + message: `--mcp ${requested} does not match any portable command-based server.`, + }); + } + } + const includedMcp = new Set(selectedMcp.filter((name) => stdioCapable.has(name))); + const settingsContent = buildPortableSettings(join(userDir, "settings.json"), { includePreferences: !skips.has("settings"), includedHooks: included, @@ -301,7 +325,7 @@ export function collectExport(options: CollectOptions = {}): ExportPlan { .sort((a, b) => (a.path < b.path ? -1 : 1)), mcpServers: userItems .filter((item) => item.kind === "mcp_server") - .map((item) => toManifestServer(item)), + .map((item) => toManifestServer(item, includedMcp)), hooks: hookItems.map((item) => ({ name: item.name, included: included.has(item.name) })), }; if (hasCodex || hasOpencode) manifest.agents = agents; @@ -326,13 +350,19 @@ export function collectExport(options: CollectOptions = {}): ExportPlan { return { manifest, entries, skipped, secretFindings, diagnostics }; } -function toManifestServer(item: ScanItem): ManifestMcpServer { +function toManifestServer(item: ScanItem, includedMcp: Set): ManifestMcpServer { const server: ManifestMcpServer = { name: item.name, status: item.status, reason: item.reason }; if (item.envRefs !== undefined && item.envRefs.length > 0) server.envRefs = item.envRefs; if (item.url !== undefined) { server.url = item.url; server.transport = item.transport ?? "http"; } + if (item.stdio !== undefined && includedMcp.has(item.name)) { + server.transport = "stdio"; + server.command = item.stdio.command; + server.args = item.stdio.args; + server.envNames = item.stdio.envNames; + } return server; } diff --git a/src/main.ts b/src/main.ts index 180660f..7841eb0 100644 --- a/src/main.ts +++ b/src/main.ts @@ -6,7 +6,9 @@ import { undoCommand } from "./commands/undo.js"; import { chooseEntry, runGuided } from "./commands/guided.js"; export { classifyMcpServer, isSensitiveKey, sanitizeRemoteEndpoint, scanSecretReferences } from "./scan/classify.js"; -export { planMcpRegistrations } from "./apply/mcp.js"; +export { planMcpRegistrations, processEnvResolver, commandOnPath } from "./apply/mcp.js"; +export type { McpEnvResolver, McpRegistration } from "./apply/mcp.js"; +export type { StdioDefinition } from "./scan/classify.js"; export { scanClaudeCode, PORTABLE_SETTINGS_KEYS } from "./scan/scanner.js"; export { scanCodex, CODEX_PORTABLE_SETTINGS_KEYS } from "./scan/codex.js"; export { scanOpencode, OPENCODE_PORTABLE_SETTINGS_KEYS, stripJsonc } from "./scan/opencode.js"; diff --git a/src/scan/classify.ts b/src/scan/classify.ts index 00b4f87..6bde05f 100644 --- a/src/scan/classify.ts +++ b/src/scan/classify.ts @@ -1,11 +1,18 @@ export type JsonValue = string | number | boolean | null | JsonValue[] | { [key: string]: JsonValue }; +export interface StdioDefinition { + command: string; + args: string[]; + envNames: string[]; +} + export interface McpClassification { status: "candidate" | "needs_secret" | "blocked" | "unsupported"; reason: string; envRefs: string[]; url?: string; transport?: "http" | "sse"; + stdio?: StdioDefinition; } export interface EndpointCheck { @@ -207,11 +214,29 @@ export function classifyMcpServer(value: JsonValue): McpClassification { const executable = hasExecutableSurface(value, strings); if (executable || hasLocalPath || hasLocalUrl) { + // A command-based server whose strings are free of machine-local paths + // and local URLs is portable STRUCTURE: command, args, and env NAMES. + // Env values are dropped right here at extraction, so no downstream + // layer can ever see one. Anything pinned to this machine stays blocked. + if (executable && !hasLocalPath && !hasLocalUrl) { + const stdio = extractStdioDefinition(value, secrets.envRefs); + if (stdio !== null) { + const needsSecrets = secrets.hasSecretReference || stdio.envNames.length > 0; + return { + status: needsSecrets ? "needs_secret" : "candidate", + reason: needsSecrets + ? "Command-based server; the command travels with consent, and env values are entered fresh on the target." + : "Command-based server; the command travels with consent.", + envRefs: secrets.envRefs, + stdio, + }; + } + } const reason = hasLocalUrl ? "Localhost MCP endpoints cannot work from another machine." : hasLocalPath ? "References a machine-local path that will not exist on the target." - : "Stdio and command-based MCP servers run local programs and are never applied."; + : "Stdio and command-based MCP servers run local programs; this entry has no re-creatable command shape."; return { status: "blocked", reason, envRefs: secrets.envRefs }; } @@ -254,6 +279,36 @@ export function classifyMcpServer(value: JsonValue): McpClassification { }; } +// Pulls the re-creatable shape out of a stdio entry: command, string args, +// and the env NAMES the server expects (object keys plus $VAR references). +// Values under env are intentionally never read past their keys. +function extractStdioDefinition(value: JsonValue, envRefs: string[]): StdioDefinition | null { + if (value === null || typeof value !== "object" || Array.isArray(value)) return null; + const record = value as { [key: string]: JsonValue }; + if (typeof record.command !== "string" || record.command.trim().length === 0) return null; + const rawArgs = record.args; + const args: string[] = []; + if (rawArgs !== undefined) { + if (!Array.isArray(rawArgs)) return null; + for (const arg of rawArgs) { + if (typeof arg !== "string") return null; + // A bare script filename ("serve.py") is a machine-local file in + // disguise: it resolves against some working directory that only + // exists here. Package specifiers and flags pass; loose scripts do not. + if (/\.(py|sh|js|ts|mjs|cjs)$/i.test(arg) && !arg.startsWith("@") && !arg.includes("/")) return null; + args.push(arg); + } + } + const envNames = new Set(envRefs); + const env = record.env; + if (env !== null && env !== undefined && typeof env === "object" && !Array.isArray(env)) { + for (const name of Object.keys(env)) { + if (ENV_VAR_NAME.test(name)) envNames.add(name); + } + } + return { command: record.command.trim(), args, envNames: [...envNames].sort() }; +} + function hasExecutableSurface(value: JsonValue, strings: string[]): boolean { if (value !== null && typeof value === "object" && !Array.isArray(value)) { const record = value as { [key: string]: JsonValue }; diff --git a/src/scan/codex.ts b/src/scan/codex.ts index 2669e33..397268d 100644 --- a/src/scan/codex.ts +++ b/src/scan/codex.ts @@ -117,6 +117,7 @@ function scanConfigToml(path: string, scope: Scope, items: ScanItem[], diagnosti item.url = classification.url; item.transport = classification.transport ?? "http"; } + if (classification.stdio !== undefined) item.stdio = classification.stdio; items.push(item); } } diff --git a/src/scan/opencode.ts b/src/scan/opencode.ts index 046f81d..09af0f5 100644 --- a/src/scan/opencode.ts +++ b/src/scan/opencode.ts @@ -141,6 +141,7 @@ function scanOpencodeConfig(path: string, scope: Scope, items: ScanItem[], diagn item.url = classification.url; item.transport = classification.transport ?? "http"; } + if (classification.stdio !== undefined) item.stdio = classification.stdio; items.push(item); } break; diff --git a/src/scan/scanner.ts b/src/scan/scanner.ts index f25f47d..6d6b7ca 100644 --- a/src/scan/scanner.ts +++ b/src/scan/scanner.ts @@ -327,6 +327,7 @@ function scanMcpConfig( item.url = classification.url; item.transport = classification.transport ?? "http"; } + if (classification.stdio !== undefined) item.stdio = classification.stdio; items.push(item); } } diff --git a/src/scan/types.ts b/src/scan/types.ts index f74047a..e5d428f 100644 --- a/src/scan/types.ts +++ b/src/scan/types.ts @@ -22,6 +22,7 @@ export interface ScanItem { envRefs?: string[]; url?: string; transport?: "http" | "sse"; + stdio?: { command: string; args: string[]; envNames: string[] }; } export interface ExcludedPath { diff --git a/src/tui/components.ts b/src/tui/components.ts index 547496c..227095c 100644 --- a/src/tui/components.ts +++ b/src/tui/components.ts @@ -30,6 +30,7 @@ export interface Flow { note(lines: string[]): void; outro(text: string): void; confirm(message: string, initial?: boolean): Promise>; + secret(message: string): Promise>; select(message: string, items: MultiItem[]): Promise>; groupMultiselect(message: string, groups: MultiGroup[], options?: { required?: boolean; coach?: string }): Promise>; } @@ -208,7 +209,7 @@ function toggle(state: MultiState, id: number): void { else state.selected.add(id); } -// The drawing the owner signed off: chip in the header, coach line until the +// The design of record: chip in the header, coach line until the // first toggle, plain group headers with air between groups, full-row // highlight on the active item, hints demoted to one detail line, the locked // section a single counted line unless v expands it, and a three-entry footer @@ -384,6 +385,32 @@ export function createFlow(screen: Screen, theme: Theme): Flow { } }, + // Masked line entry for values that must never echo: asterisks on + // screen, the real characters only in memory, esc or ctrl+c cancels. + async secret(message) { + let value = ""; + for (;;) { + screen.renderLive([ + `${theme.paint("accent", g.stepActive)} ${theme.paint("bright", message)}`, + `${bar} ${"*".repeat(Math.min(value.length, 40))}${theme.paint("inverse", " ")}`, + `${theme.paint("accent", g.railEnd)} ${theme.paint("dim", ["type the value", "enter confirm", "esc cancel"].join(` ${g.sep} `))}`, + ]); + const key = await screen.waitKey(); + if (key.name === "cancel" || key.name === "escape") { + cancelCommit(message); + return cancelled(); + } + if (key.name === "return" || key.name === "enter") { + if (value.length === 0) continue; + screen.clearLive(); + summaryCommit(message, "********"); + return done(value); + } + if (key.name === "backspace") value = value.slice(0, -1); + else if (key.char !== null) value += key.char; + } + }, + async select(message, items) { if (items.length === 0) throw new Error("select needs at least one option."); let cursor = 0; diff --git a/src/tui/plain.ts b/src/tui/plain.ts index 415b6f8..0d61538 100644 --- a/src/tui/plain.ts +++ b/src/tui/plain.ts @@ -75,6 +75,17 @@ export class Plain { } } + // Plain mode cannot mask input (it reads cooked lines), so it says so + // before asking; the value still never appears in any later output. + async secret(message: string): Promise> { + for (;;) { + const answer = await this.ask(`${message} (input is visible in this mode)`); + if (answer === null) return cancelled(); + if (answer.trim().length > 0) return done(answer.trim()); + this.say("A value is required, or ctrl+c to cancel."); + } + } + async select(message: string, items: MultiItem[]): Promise> { for (;;) { const listing = items diff --git a/test/codex.test.mjs b/test/codex.test.mjs index 0072205..2c15bd9 100644 --- a/test/codex.test.mjs +++ b/test/codex.test.mjs @@ -82,8 +82,9 @@ test("codex mcp servers classify with the shared classifier", () => { assert.equal(linear.status, "candidate"); assert.equal(linear.url, "https://mcp.linear.app/mcp"); const local = item("local"); - assert.equal(local.status, "blocked"); + assert.equal(local.status, "needs_secret"); assert.deepEqual(local.envRefs, ["LOCAL_TOKEN"]); + assert.deepEqual(local.stdio, { command: "npx", args: ["-y", "some-mcp"], envNames: ["LOCAL_TOKEN"] }); }); test("project scope picks up config and root AGENTS.md", () => { diff --git a/test/scan.test.mjs b/test/scan.test.mjs index 5677c74..6f289fe 100644 --- a/test/scan.test.mjs +++ b/test/scan.test.mjs @@ -149,7 +149,10 @@ test("mcp servers are classified without values", () => { test("local-scope servers nested under projects are found", () => { assert.equal(item("nestedremote").status, "candidate"); + // python3 serve.py resolves against a working directory that only exists + // here, so the bare-script guard keeps it blocked. assert.equal(item("nestedlocal").status, "blocked"); + assert.equal(item("nestedlocal").stdio, undefined); }); test("a dollar sign inside a secret value never emits a fragment env ref", () => { diff --git a/test/stdio-mcp.test.mjs b/test/stdio-mcp.test.mjs new file mode 100644 index 0000000000000000000000000000000000000000..707db92e2ce5957cb73e1469502ca36abfe009ed GIT binary patch literal 12104 zcmdT~ZFAek5$4snG|YC&~aXzGNU+(M871m<&oty9m^v_z>!1*0w@lU#B%iC z`|R!kIFPhur*3CDp0S9-z3lBhyZh`Ol0}i1m0GDvn`)rW^)xT_K!wvvm+D4M%Y31{ zERXe=I(XmRNn}81jK(RVIEXDM~{+Jzg%Wf zfB9&hr14o%=8-nNA<=x1$JBp-(TS;y{9atdm0lF>LsH7mQipM~!a$V^X}v0wN;l2@ zg{QsMsznheYnsV>J$}oRtUp5$R&#fgQ*7|n@!?7S0x$Yq>oRqE-cjnj#2aEdvq?11>35ok@sD*Dr}`02_Q!fU z)g`qTX_!5Wieo*42FtKY@{Ct1>+Erz#5&$9igZamO%!JPX;{a4AE=;~mr<#8Hn`KE zB?v|_W*IHQBny5c-Q0;VU8RmEFOJTRkB&~%uIlN)&-9goMtm;_ynzyk1q)q;0Ti;c z^PQbw8eZrp`9fp7bG$xj+?Rv#cLvG}XMokv01`t}#YsM#W6aQ_dF(_l4)>m)Ob*T_ z&wn}lW$*CS1T%W1gG$H4sJebI)Y;|mGE8ed{M-F+zkKi&?IP8cD)YShj(^oKfP^8FpP`@LF0OEmCf$ymLjW$yup`jR zTqJ2~pj5Bamx;d8r3V$QRH>u7G|8nNtE#NEy5VVhX7B-1VcM5JKRrAQ7BNlh{R#8R zDyNwU_Wl`lRVCTX03|v457bXDkDdqQL9$sgUHU5p)7I%&d3jMA4??n(ePuUl>Go_j zV`nAx^rb)pY^Zl)gk!a_JU2qQQSnljm%21#K1%8eQ%W)&mS|DcvVa=Bc~uoqG!Sba z%y=}y*g$#)VNuW{wjrz0B!o4ZBFYzwFpDuTE3Pp~SkAE6Yj3!uqx%7NUxj8qf*n=* zx_U6AdEUU`WtTL1|K!&PXD3HLPo9s}CbVzd1*>GC0bFdRzifT!E{o}h)YE(8jxCZb z(~z+zE)K|%ck~LAn~{OR<(VE;d7hFBaHg@Fzu6%r;5Z3*cR1X*#=>sd7o=~1b)XQW zqCRrPk^7B@*aGp*QM~lD&LVPmjZ^jWg1sMLwYu36)74d7wjIRl`ef6sY080agH#0l zT$9fli}7{>!hq==V9zu0!yb7q1Fy2BH+wbHbk7T&(UAQ-n&m5Gr<+M#YI% zVTCx17{Xc|C;<)nhfAZ*(>%J+F+}Pdqr`?%GY{Eai!X2itz9Gb|Awf?d(G-J9jw`~ z^BiI zgh%mi!g&Jy&pJ$fYKHR)tJ*;2nbxs6GaA9M>UEB_E+*~(L@0vm?{@*TQ}z~}A0Ue( z!)kWWnZ2`*uCXVe?52C!?FpejPGT^PUjJ@Gy)hfxdY6k~1T}kT?;Ijqup0 zA2!nMeNn^AGB6}jg0k|+{z-o=l1Xgr21@Ssvr5l7zc4dV)9!XIXBl zBr*Xp9N&+5PjO&Kx};ii^zd|9=0vkuZQ$LDyh8XC1_@@T8R%@;jlNWcGu4%m;7Nd4 z-~T%W%n;jumv}g9$0?D5tnC#EY@<{&q;C)?Q_uEas2Sy+Y?V!%Qh-JU7?NwE!IWhx zjWko4LL=p{rn~8tK6RvL(F}#P^;FrHr2z6b4zS_UIFrT*8EQQnKk#|-ub@h*R8ycS zEAw}FQr{y**kiPr*@lJkkvA(?s=*e}XBzCw5yf!O$XI#MYY)RljpL%Tph&5NjXk;Ml4xp zOlxAX<*swBqC^@ZK3m-d<)S2%!l`wG03PfF9Ces*B8lzW`Ne6n%9_FkNFu^>h2+VA zsTV;t`xl}iGT(w&5z?e0t!DtUJk0a_!v2JyZL}cbVJ)_+A2dFromWuJgwV5R;uz7k zDoPXQVc?+2Nr=#G@)Yg@`AC?g!zj%S9263eMFP$McMIo?!jzuYCS*1MR$aoiBw0~! zi}H{lFjC4aT}DbPgvh$l4qdfzigd_v3gr5|DQR&rvk7R!+Y`UMXao_F!M%!-@8Q^#|08Qj|o{?wmP`>&KCfO*j$a%fJ$icBYu-QHdv!25+dq2#=;`CLgQv%~)FG>ORZW?6|L9=y>zPc= z#IWt=)=qvsc-&05wJVoFEU>BX!QRQkGDxBur&2UxG(Y6mT-+D7 zyjBEnM??CySiYq&*sB|vd9px-X>CEN><#JV`cihlhs=AJBbSnmw4=Ng}KIj_B;n{4`_A?S1xxr@sGQd7rJQKNh<2PP4a+g7$t)>iOH|yO%%s z`fCX%$kS|%yN4dOXj~J?^&9NLg2=YN3rc(hw;{>SfeKh3OE4EyIN|F-O6qF*E#_fl zSx`C0yHYRqPM(a_TiHI&?`Hhjsuec74CwvMTby)L&t}!M(UX{%Pr&CQg49(yHm0ha zEz}yQAm9e2P_Sm@bG9di;&whEsr;rbRo(9|DV*D;zfI2=KXAUA!M8t_^WpZcQ_j^< zM~imyWc@1^qS!9&jwl z5j2|D*##9IUORbCYbUfJd+|6{KKEs*WK@9r%(-n9JfXl-FDOU->fSwRAwoY^U)m?$ zvRu~6Ys%f4?bw#-wBN+r#Qg)^Uadq)1JxkZijO`p^OghBu^i)q+4vG}dN*LQCwb9G z{qfgZA9=U6r91S$^WJ{V;! z=5M&yjZyKTD;%k~gWYi+e-2kDg?Kgu{s02uDWq=>nnP&;-O(surB7o}5gv-5|NpG5 zv0vFPwi;fANwxKh6BOngZ`bL^Uhngrn>eyHjft*?@@}1pN$YJyOmcem%YDmDaxAtY z^KgtvAs}~;ME{#@lA&Ng-;H&z4Pycw$c>4gLjuQsYRrS+U=dIobF^*Nd-O;!KEcVj z3a|Pf0+T#R~tO0T&O zNh;b4G`=eHy$ikMzR-enl@b_AfZ$=^3AlTwcJ&ZNpwp577O?ue`2u|G&PNF zIRpw72a5MJZI z4cIdq0f~)^bcE4-ij)14JkgLoH;IQ_C_?Hln`k?V3V2L!m+zFy)aOtl`#bS;kU{y)MqFOQB+*zF-+_FR<3G3o1ceQR{eyTLs&;Q&~~;Qfk~H615To z3~W$k=i0wI5?0pqcX-)4D?5FN+O~u?s;euM_Q0Lmx}LqQ-QZiN0sXs`Wvvy-H>6@L zZIs+x?QIM>{mbf`!OaK&r5Lmb4P9u0i!X{M%!LNXNz8?1^W8DrZS>)2f1Aqf+zhxT z4x$D}3jzO5=yL_7yq*+X$@{}X5WPM;N? zNBA`GHZLVk%=J<2smmrdJG5T8b3Uc41Pks{>~BM`i#^z|Amb|8rg)D8igh~O9q z$Cx#*;us4JqEs5khf_gy5T#i*i?;UHvq#y3AOn>sRQV{Gf#7mBfPIcz)Z56!ee~GI zCS { assert.deepEqual(Object.keys(GLOBAL_FLAGS).sort(), ["help", "version"]); assert.deepEqual(ALL_COMMANDS.map((command) => command.word), ["scan", "export", "apply", "undo"]); assert.deepEqual(Object.keys(ALL_COMMANDS[0].flags).sort(), ["json", "no-project", "project"]); - assert.deepEqual(Object.keys(ALL_COMMANDS[1].flags).sort(), ["allow-secret", "dry-run", "hook", "json", "plugin", "skip"]); + assert.deepEqual(Object.keys(ALL_COMMANDS[1].flags).sort(), ["allow-secret", "dry-run", "hook", "json", "mcp", "plugin", "skip"]); assert.deepEqual(Object.keys(ALL_COMMANDS[2].flags).sort(), ["dry-run", "hook", "mcp", "no-input", "plain", "plugin", "target"]); assert.deepEqual(Object.keys(ALL_COMMANDS[3].flags).sort(), ["target"]); }); From 97f4cd745b4ccaffc41b8ed913cb8c5df04b1f30 Mon Sep 17 00:00:00 2001 From: Favour Ohans Date: Thu, 17 Sep 2026 04:20:14 +0100 Subject: [PATCH 2/4] stdio review round 1: no value in any echo, no display before validation, the gate reruns on apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dry runs plan with a placeholder resolver, so a resolved secret cannot exist to be echoed, and every argv display goes through a masker that replaces env values with the name and an ellipsis — including claude's own stderr when a registration fails, which could otherwise quote the argv back into our error message. String hygiene now rejects the entire C0 range: CR, LF and TAB were accepted, and a CR-bearing arg could both forge the consent frame and register. Validation now precedes display everywhere — the guided flow validates a stdio entry through the full gate before building any consent text, so a hostile manifest refuses the whole apply without a single byte of it reaching a frame, and the static path's hint lines sanitize names and commands it never validated. The consent block itself is no longer one truncatable line: command and args wrap across full lines with no elision, because that display is the security boundary. The stdio branch now performs the same complete re-derivation the remote branch always did: the exact export-side classifier reruns over the untrusted entry, closing the blocked-class smuggling hole and the portability-gate bypasses found live (script filenames with directory prefixes or in the command position, more script extensions, private and shorthand-IP URLs in args). Token-shaped args are refused at classification: an argument that looks like a credential is a value, and values never travel — the fix is moving it to env. Five new tests: dry-run placeholder round trip both with and without the env set, C0 rejection, the re-derivation refusal table, the hostile-manifest consent-forgery attempt asserting the forged text reaches no frame and nothing is written, and the wrap/mask units. --- docs/THREAT-MODEL.md | 2 +- src/apply/mcp.ts | 71 ++++++++++++++++++++++++++++++++++++---- src/commands/apply.ts | 16 ++++++--- src/commands/guided.ts | 40 +++++++++++++++------- src/main.ts | 4 +-- src/scan/classify.ts | 40 ++++++++++++++++++---- test/stdio-mcp.test.mjs | Bin 12104 -> 17481 bytes 7 files changed, 141 insertions(+), 32 deletions(-) diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index 784a8a8..77fb305 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -28,7 +28,7 @@ Each guarantee names the code that enforces it. All of them are covered by tests **Writes cannot escape the target.** Beyond lexical path containment, every write path is walked component by component and refused if any existing component is a symlink. The target directory itself may be a symlink; nothing under it may be (`resolveForWrite` in `src/apply/apply.ts`). -**Code execution needs consent on both machines.** Hooks and `statusLine` entries are shell commands, and an enabled plugin installs marketplace code. They enter a bundle only when named with `--hook` or `--plugin` at export, and they apply only when named again at apply; a marketplace source survives the gate only when a confirmed plugin references it. Behind the gates sits a settings allowlist: a bundle's `settings.json` may carry only the keys export can produce (the portable preference keys, `statusLine`, `hooks`, `enabledPlugins`, `extraKnownMarketplaces`, each shape-checked), so keys that execute code on the target (`apiKeyHelper`, `env`, auth refresh scripts) refuse the bundle whole rather than riding past a consent filter that does not know them (`assertPortableSettings` in `src/apply/apply.ts`). A consequence with teeth: adding a new portable settings key is no longer a schema-invisible change, because an older apply will refuse a newer bundle that carries it — a future key must bump the manifest schema version, or knowingly accept that older applies fail loud on such bundles. The schema version itself is an advisory compatibility signal, not a trust boundary: every receive-side control (path allowlist, per-agent key allowlists, per-root containment) is enforced regardless of the schema a bundle claims, so no check may ever gate on it. MCP registration is opt-in per server with `--mcp` on BOTH sides and goes through the agent's own CLI. Remote servers carry name and sanitized URL only. Command-based (stdio) servers carry structure only — command, args, env NAMES — with values dropped at classification time so no downstream layer can see one (superseding the earlier rule that stdio servers are never applied); on the target, values are typed fresh in the guided flow or read from the machine's own environment, the untrusted manifest entry is fully revalidated (string hygiene, env-name shape, size caps) before argv assembly, and no shell is ever involved. Servers pinned to a machine (local paths, localhost endpoints) remain blocked. Tokens, headers, and environment values never travel in any form. +**Code execution needs consent on both machines.** Hooks and `statusLine` entries are shell commands, and an enabled plugin installs marketplace code. They enter a bundle only when named with `--hook` or `--plugin` at export, and they apply only when named again at apply; a marketplace source survives the gate only when a confirmed plugin references it. Behind the gates sits a settings allowlist: a bundle's `settings.json` may carry only the keys export can produce (the portable preference keys, `statusLine`, `hooks`, `enabledPlugins`, `extraKnownMarketplaces`, each shape-checked), so keys that execute code on the target (`apiKeyHelper`, `env`, auth refresh scripts) refuse the bundle whole rather than riding past a consent filter that does not know them (`assertPortableSettings` in `src/apply/apply.ts`). A consequence with teeth: adding a new portable settings key is no longer a schema-invisible change, because an older apply will refuse a newer bundle that carries it — a future key must bump the manifest schema version, or knowingly accept that older applies fail loud on such bundles. The schema version itself is an advisory compatibility signal, not a trust boundary: every receive-side control (path allowlist, per-agent key allowlists, per-root containment) is enforced regardless of the schema a bundle claims, so no check may ever gate on it. MCP registration is opt-in per server with `--mcp` on BOTH sides and goes through the agent's own CLI. Remote servers carry name and sanitized URL only. Command-based (stdio) servers carry structure only — command, args, env NAMES — with values dropped at classification time so no downstream layer can see one (superseding the earlier rule that stdio servers are never applied); on the target, values are typed fresh in the guided flow or read from the machine's own environment, the untrusted manifest entry is fully revalidated before anything is displayed or assembled — string hygiene across the whole C0 range, env-name shape, size caps, and a rerun of the same portability gate export uses — and env values are masked in every echoed or dry-run line, and no shell is ever involved. Servers pinned to a machine (local paths, localhost endpoints) remain blocked. Tokens, headers, and environment values never travel in any form. **Applies are reversible.** What an apply overwrites is backed up first, the record of the apply lands before the first destructive write, and `agent-sync undo` restores it or aborts untouched if the backup is incomplete. diff --git a/src/apply/mcp.ts b/src/apply/mcp.ts index b518e7a..0786750 100644 --- a/src/apply/mcp.ts +++ b/src/apply/mcp.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { sanitizeRemoteEndpoint } from "../scan/classify.js"; +import { classifyMcpServer, sanitizeRemoteEndpoint } from "../scan/classify.js"; import type { ManifestMcpServer } from "../export/collect.js"; const SERVER_NAME = /^[A-Za-z0-9][A-Za-z0-9_.-]*$/; @@ -68,11 +68,19 @@ export function planMcpRegistrations( return registrations; } -function planStdioRegistration( +// Everything a stdio entry must prove before it may appear on ANY screen or +// reach an argv: name shape, control-byte-free strings, env-name shape, size +// caps, and a rerun of the exact export-side portability gate — the complete +// re-derivation of the untrusted manifest. Exported so the guided flow can +// validate BEFORE it builds a consent line: a string that has not passed +// this function must never be displayed. +export function assertPortableStdioServer( name: string, server: ManifestMcpServer, - resolveEnv: McpEnvResolver, -): McpRegistration { +): { command: string; args: string[]; envNames: string[] } { + if (!SERVER_NAME.test(name)) { + throw new Error(`--mcp ${name}: server name is not safe to pass along. Nothing was registered.`); + } if (typeof server.command !== "string" || !cleanString(server.command)) { throw new Error(`--mcp ${name}: the bundle's command is not a clean string. Nothing was registered.`); } @@ -88,6 +96,25 @@ function planStdioRegistration( ) { throw new Error(`--mcp ${name}: the bundle's env names are not valid variable names. Nothing was registered.`); } + const reclassified = classifyMcpServer({ + command: server.command, + args: [...args], + env: Object.fromEntries(envNames.map((envName) => [envName, ""])), + }); + if (reclassified.stdio === undefined) { + throw new Error( + `--mcp ${name}: this definition does not pass the portability gate (${reclassified.reason}) Nothing was registered.`, + ); + } + return { command: server.command, args: [...args], envNames: [...envNames] }; +} + +function planStdioRegistration( + name: string, + server: ManifestMcpServer, + resolveEnv: McpEnvResolver, +): McpRegistration { + const { command, args, envNames } = assertPortableStdioServer(name, server); const argv = ["mcp", "add", "--transport", "stdio", "--scope", "user", name]; for (const envName of envNames) { @@ -99,12 +126,20 @@ function planStdioRegistration( } argv.push("--env", `${envName}=${value}`); } - argv.push("--", server.command, ...args); + argv.push("--", command, ...args); return { name, args: argv }; } +// For hint lines that mention entries nobody validated yet (the unregistered +// list, the PATH note): strip anything that could steer a terminal and cap +// the length, so a hostile name or command cannot forge output. +export function displayString(text: string, cap = 120): string { + const stripped = text.replace(/[\u0000-\u001f\u007f]/g, "\ufffd"); + return stripped.length > cap ? `${stripped.slice(0, cap)}...` : stripped; +} + function cleanString(text: string): boolean { - return text.length > 0 && text.length <= MAX_STRING_LENGTH && !/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/.test(text); + return text.length > 0 && text.length <= MAX_STRING_LENGTH && !/[\u0000-\u001f\u007f]/.test(text); } // A missing binary is a warning, not a refusal: the registration still @@ -119,16 +154,38 @@ export function commandOnPath(command: string): boolean { } } +// Anywhere a registration's argv is shown or echoed back, env values are +// replaced with the name and an ellipsis. The only place a real value may +// exist is the argv handed to execFile. +export function maskRegistrationDisplay(text: string, registration: McpRegistration): string { + let masked = text; + for (let index = 0; index < registration.args.length - 1; index += 1) { + if (registration.args[index] !== "--env") continue; + const pair = registration.args[index + 1]; + if (pair === undefined) continue; + const equals = pair.indexOf("="); + if (equals <= 0) continue; + const value = pair.slice(equals + 1); + if (value.length === 0) continue; + masked = masked.split(pair).join(`${pair.slice(0, equals)}=...`); + masked = masked.split(value).join("..."); + } + return masked; +} + export function runMcpRegistration(registration: McpRegistration): void { try { execFileSync("claude", registration.args, { stdio: ["ignore", "pipe", "pipe"] }); } catch (error) { - const detail = + const rawDetail = error !== null && typeof error === "object" && "stderr" in error && Buffer.isBuffer(error.stderr) ? error.stderr.toString("utf8").trim() : error instanceof Error ? error.message : String(error); + // A CLI that echoes bad argv back would otherwise put the secret in our + // error message; redact before it can reach any frame or log. + const detail = maskRegistrationDisplay(rawDetail, registration); throw new Error(`registering MCP server ${registration.name} via \`claude mcp add\` failed: ${detail}`); } } diff --git a/src/commands/apply.ts b/src/commands/apply.ts index 0f5b157..21f2d93 100644 --- a/src/commands/apply.ts +++ b/src/commands/apply.ts @@ -11,7 +11,7 @@ import { planApply, splitBundleByRoot, } from "../apply/apply.js"; -import { commandOnPath, planMcpRegistrations, runMcpRegistration } from "../apply/mcp.js"; +import { commandOnPath, displayString, maskRegistrationDisplay, planMcpRegistrations, processEnvResolver, runMcpRegistration } from "../apply/mcp.js"; import { stringList } from "./export.js"; import { chooseEntry, runGuidedApply } from "./guided.js"; @@ -108,7 +108,13 @@ export const applyCommand: CommandDef = { } // Registrations are validated before any file write so a bad --mcp refuses // the whole apply, not half of it. - const registrations = planMcpRegistrations(bundle.manifest.mcpServers, requestedMcp); + // A dry run must never resolve real secrets: it plans with a "..." + // placeholder resolver, so the value cannot exist to be echoed. + const registrations = planMcpRegistrations( + bundle.manifest.mcpServers, + requestedMcp, + dryRun ? () => "..." : processEnvResolver(), + ); // A bundle may span several agents; each agent root gets its own plan, // marker and backups, and every root is planned (write-checked) before @@ -173,7 +179,7 @@ export const applyCommand: CommandDef = { let failedRegistrations = 0; for (const registration of registrations) { if (dryRun) { - io.out(`Would register MCP server ${registration.name}: claude ${registration.args.join(" ")}`); + io.out(maskRegistrationDisplay(`Would register MCP server ${registration.name}: claude ${registration.args.join(" ")}`, registration)); continue; } try { @@ -188,7 +194,7 @@ export const applyCommand: CommandDef = { for (const registration of registrations) { const server = bundle.manifest.mcpServers.find((candidate) => candidate.name === registration.name); if (server?.transport === "stdio" && typeof server.command === "string" && !commandOnPath(server.command)) { - io.out(`Note: ${server.command} is not on this machine's PATH yet; the ${server.name} registration still lands.`); + io.out(`Note: ${displayString(server.command)} is not on this machine's PATH yet; the ${displayString(server.name)} registration still lands.`); } } @@ -200,7 +206,7 @@ export const applyCommand: CommandDef = { ); if (unregistered.length > 0) { io.out( - `Bundle records ${unregistered.length} portable MCP server(s) not registered; pass --mcp to register: ${unregistered.map((server) => server.name).join(", ")}`, + `Bundle records ${unregistered.length} portable MCP server(s) not registered; pass --mcp to register: ${unregistered.map((server) => displayString(server.name, 60)).join(", ")}`, ); } if (failedRegistrations > 0) { diff --git a/src/commands/guided.ts b/src/commands/guided.ts index f501fe0..402a515 100644 --- a/src/commands/guided.ts +++ b/src/commands/guided.ts @@ -15,7 +15,7 @@ import { type AgentRoots, } from "../apply/apply.js"; import type { LoadedBundle } from "../apply/bundle.js"; -import { commandOnPath, planMcpRegistrations, runMcpRegistration, type McpRegistration } from "../apply/mcp.js"; +import { assertPortableStdioServer, commandOnPath, planMcpRegistrations, runMcpRegistration, type McpRegistration } from "../apply/mcp.js"; import type { JsonValue } from "../scan/classify.js"; import { commandSummary, scanClaudeCode } from "../scan/scanner.js"; import { scanCodex } from "../scan/codex.js"; @@ -356,6 +356,17 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid } } +// Full-content wrapping for consent blocks: every character lands on some +// line; nothing hides past an ellipsis. +export function wrapDisplay(text: string, width: number): string[] { + if (text.length <= width) return [text]; + const lines: string[] = []; + for (let index = 0; index < text.length; index += width) { + lines.push(index === 0 ? text.slice(0, width) : ` ${text.slice(index, index + width)}`); + } + return lines; +} + function destLabel(dest: string): string { return dest === "agent-sync-bundle" ? "agent-sync-bundle/" : dest; } @@ -575,19 +586,26 @@ export async function runGuidedApply( const stdioConsents: { name: string; command: string; envNames: string[] }[] = []; for (const server of bundle.manifest.mcpServers) { if (server.transport === "stdio" && typeof server.command === "string") { - // The consent line is built from manifest fields, never from a - // planned argv, so no env value can exist yet to leak into it. - const envNames = server.envNames ?? []; - const envDisplay = envNames.map((envName) => `--env ${envName}=...`).join(" "); - const commandLine = [server.command, ...(server.args ?? [])].join(" "); - const consent = await ui.confirm( - `Register MCP server ${server.name}? (claude mcp add --transport stdio ${envDisplay}${envDisplay.length > 0 ? " " : ""}-- ${commandLine})`, - false, - ); + // Validation precedes display: a string that has not passed the + // portability-and-hygiene gate must never reach a frame, so a + // hostile manifest cannot forge or steer the consent screen. The + // block is wrapped across full lines with no elision — what you + // read is the complete command, because this display IS the + // security boundary. + const validated = assertPortableStdioServer(server.name, server); + const commandLine = [validated.command, ...validated.args].join(" "); + ui.note([ + `mcp server ${server.name} wants to register on this machine:`, + ...wrapDisplay(` command: ${commandLine}`, 76), + validated.envNames.length > 0 + ? ` env (values asked next, never carried): ${validated.envNames.join(", ")}` + : " env: none", + ]); + const consent = await ui.confirm(`Register MCP server ${server.name}?`, false); if (consent === null) return 2; if (consent) { requestedMcp.push(server.name); - stdioConsents.push({ name: server.name, command: server.command, envNames }); + stdioConsents.push({ name: server.name, command: validated.command, envNames: validated.envNames }); } continue; } diff --git a/src/main.ts b/src/main.ts index 7841eb0..707ea59 100644 --- a/src/main.ts +++ b/src/main.ts @@ -6,7 +6,7 @@ import { undoCommand } from "./commands/undo.js"; import { chooseEntry, runGuided } from "./commands/guided.js"; export { classifyMcpServer, isSensitiveKey, sanitizeRemoteEndpoint, scanSecretReferences } from "./scan/classify.js"; -export { planMcpRegistrations, processEnvResolver, commandOnPath } from "./apply/mcp.js"; +export { planMcpRegistrations, processEnvResolver, commandOnPath, assertPortableStdioServer, maskRegistrationDisplay, displayString } from "./apply/mcp.js"; export type { McpEnvResolver, McpRegistration } from "./apply/mcp.js"; export type { StdioDefinition } from "./scan/classify.js"; export { scanClaudeCode, PORTABLE_SETTINGS_KEYS } from "./scan/scanner.js"; @@ -21,7 +21,7 @@ export { Plain, plainModeRequested } from "./tui/plain.js"; export type { ScanReport, ScanItem } from "./scan/types.js"; export { collectExport, skipToken, MANIFEST_SCHEMA_VERSION } from "./export/collect.js"; export type { Manifest, ManifestPlugin, ExportPlan } from "./export/collect.js"; -export { chooseEntry, runGuided, runGuidedApply, flagEcho, applyFlagEcho, buildTravelGroups, buildHookGroup, buildReviewLines, fitReviewLines } from "./commands/guided.js"; +export { chooseEntry, runGuided, runGuidedApply, flagEcho, applyFlagEcho, buildTravelGroups, buildHookGroup, buildReviewLines, fitReviewLines, wrapDisplay } from "./commands/guided.js"; export { wordmarkLines, wordmarkWidth } from "./tui/wordmark.js"; export { scanContentForSecrets } from "./export/secrets.js"; export { createTar } from "./export/tar.js"; diff --git a/src/scan/classify.ts b/src/scan/classify.ts index 6bde05f..ab38779 100644 --- a/src/scan/classify.ts +++ b/src/scan/classify.ts @@ -1,3 +1,5 @@ +import { scanContentForSecrets } from "../export/secrets.js"; + export type JsonValue = string | number | boolean | null | JsonValue[] | { [key: string]: JsonValue }; export interface StdioDefinition { @@ -58,7 +60,9 @@ function isPrivateHost(hostname: string): boolean { function isPrivateIpv4(host: string): boolean { const octets = host.split(".").map(Number); - if (octets.length !== 4 || !octets.every((octet) => Number.isInteger(octet) && octet >= 0 && octet <= 255)) { + // Shorthand numeric forms ("127.1", "10.5") resolve as IPs too, so any + // all-numeric dotted host gets the range checks, not only 4-octet ones. + if (octets.length < 1 || octets.length > 4 || !octets.every((octet) => Number.isInteger(octet) && octet >= 0)) { return false; } const [a = 0, b = 0] = octets; @@ -286,19 +290,29 @@ function extractStdioDefinition(value: JsonValue, envRefs: string[]): StdioDefin if (value === null || typeof value !== "object" || Array.isArray(value)) return null; const record = value as { [key: string]: JsonValue }; if (typeof record.command !== "string" || record.command.trim().length === 0) return null; + const command = record.command.trim(); + // A script filename anywhere ("serve.py", "scripts/serve.py") is a + // machine-local file in disguise: it resolves against a working directory + // that only exists here. Package specifiers (@scope/name) pass; loose + // scripts do not — in the command or in any arg. + if (looksLikeLooseScript(command)) return null; const rawArgs = record.args; const args: string[] = []; if (rawArgs !== undefined) { if (!Array.isArray(rawArgs)) return null; for (const arg of rawArgs) { if (typeof arg !== "string") return null; - // A bare script filename ("serve.py") is a machine-local file in - // disguise: it resolves against some working directory that only - // exists here. Package specifiers and flags pass; loose scripts do not. - if (/\.(py|sh|js|ts|mjs|cjs)$/i.test(arg) && !arg.startsWith("@") && !arg.includes("/")) return null; + if (looksLikeLooseScript(arg)) return null; + // A URL pinned to loopback or a private range cannot work elsewhere. + const url = arg.match(/^https?:\/\//i) ? tryParseHost(arg) : null; + if (url !== null && isPrivateHost(url)) return null; + // An argument that looks like a credential IS a value; values never + // travel. The fix on the source machine is moving it into env. + if (scanContentForSecrets(Buffer.from(arg, "utf8")).length > 0) return null; args.push(arg); } } + if (scanContentForSecrets(Buffer.from(command, "utf8")).length > 0) return null; const envNames = new Set(envRefs); const env = record.env; if (env !== null && env !== undefined && typeof env === "object" && !Array.isArray(env)) { @@ -306,7 +320,21 @@ function extractStdioDefinition(value: JsonValue, envRefs: string[]): StdioDefin if (ENV_VAR_NAME.test(name)) envNames.add(name); } } - return { command: record.command.trim(), args, envNames: [...envNames].sort() }; + return { command, args, envNames: [...envNames].sort() }; +} + +const LOOSE_SCRIPT = /\.(py|sh|bash|zsh|js|ts|mjs|cjs|rb|pl|php|lua)$/i; + +function looksLikeLooseScript(text: string): boolean { + return LOOSE_SCRIPT.test(text) && !text.startsWith("@"); +} + +function tryParseHost(raw: string): string | null { + try { + return new URL(raw).hostname; + } catch { + return null; + } } function hasExecutableSurface(value: JsonValue, strings: string[]): boolean { diff --git a/test/stdio-mcp.test.mjs b/test/stdio-mcp.test.mjs index 707db92e2ce5957cb73e1469502ca36abfe009ed..2733c75719e8fdeea1d00a99f0ed2a5c863650b8 100644 GIT binary patch delta 3932 zcma)9U2hy$8CKQup#W+jBq1rbUZ#mMQ|->$aU8cBxm9C3B_ws)B+yE6XpU#j?o2#8 zGn{kQ_SUTS4tfPtbA`k|pchDR+lpQw0e6TWfWklMA3!|sIkWcogBGu}vz~L#`}w@j z`<}o4^lu+N^M}v<`9V~YmTFrnip=g3?TFlHG6gBwPqo48A}5oing$|D(p*-*3ueC_ z6@!7u<8E~;nB^CN%(rR7-xk?WY7%-&#+1ul)55nEEtE=Syb*M(kAk53+`qu5{`kw_ zv#;DaWu!K~r;1@7(|ng=HEOG2PGu%?O*^Sc_-7=OB8#PhbZm1{Wt0@G4z`MnSb6o^ z^9#>HK=t8Ee?OBH14+yDq)2mL6$Om(VpaY5!i&!&)zb@?UpPe+6}dK6$6CH!g|odE zu&LdyP4FJIPF277{?)fxQj7Lz<-vPP??1SEdu_c-9xH8Y87XPn*75e@;-W|6fW|F| z4zA-%XvEfp17V`XXIG%6MV+b>%uYJZ9mSUagL*wo^C%m}QX?iOstBwjE=V>6u?>MP zvTcOjS>!?ZA^(b@fo_CD?#k$9n#t_xD#8RhW z=U|99=Y_%MpAAzbi2>u_!lsVd+!_b(O@Xj-uSIum+I&BF;|2+wQu2w647?PY;#8Rt zX`|A-ue}!e0WH5qd;4B*3eD-?qg)Im^0iz2>-6na=v~MEx=Rlq(U^18D-`(-2u@_t zqlcc@$TuMrPgOt7ruQhY@~io?XR3dmed$G3(Sl)p7H1Db=A#yl?^Yk3{ciQ|bLS_7 zzaQPdyB<1A)85FpI|Bwu4U_Sq08ITiiG+J`TF|Bvd6d9=E(}yB7Ro{yv6*HmV$c^x zQo(h`E5qCoauiKf+GzH#o`CxznrYmeVd#zEh#}sp$*v2 z)G=?@V2ALb`Dey)0>C5Ne-6N1aYVC(YCp1Qdh#ip{N(XGe z4=J(iMru@=f;ny4T~O`V+U!EL4eeS(_1Uv$zJ*<2n;uNlUQqqxBAwIH7^XHHzA^0e zqzZeg82Ht%p1VA;ZHvp>x&V^1rK+Pu4#ZDTnQ4)CY2M|Vu?G4ReK^zBqdc+!LT}<1 zSco!#ZUbXtS+7_tz-W6^3X@>V)RgdvE;!>8B;?BCwQeIGzUSU~K{za91j(Ns3c@(; z+ZLb5T@TGmryqcir^{g`^FDe!c+)z8f#klON0?LX8>-!7x^!IryeGF)muP-T)zj18 zo78&=G$~*v%&y(x9e_rzIZf;hb82JlfYvO+_vO?)<`r8TY9qgS_uiXptL>G$>-X2z zAGGh>T3_n|oNgi>pFdg?f4aC_ox1ev z%S4$(4T>}yqW>>bKFl(%Edy&D>TMBLzrS?ZXHbNu{{ax{maq*;lm-qUSN10dx%$r~ z4}BRuN6LJf6eOxkdv$6UL~i`;PUnhxg;|9GRZn(onD0$V#dT|x6^yeJkCMHimW~a^ zpc;&Uh_B~446!BLdzPCv`7EtP zOV=zIYA9fYuQyLA?KN6xZouR#vi&^+QTl#{3ss9!JGuHPEiW(A98F#)eotNd2z8c5 z(!Ux6L1^pR;}kmsJXYwC#?-^KNqpZL9faOt@3@ImQH|S%%wvyffSJ@e?~1@>3lTUN zr5mFR_!|FtB98gdZljL)hrJ~&>tVT|+IoR6Np6|HnPxEn90Zv&Y-4OwJ!xXZ<=U^i z6!CQxs>tf0<4Uk#^XfM>2T delta 65 zcmX@v!FVEW!$*zD*EFS66k;@za}rBaQWbKO3ltJlQWSJ`ONtWniVO0KN)(DqQZn-u VCf6FtO@1yf#T28t`Ge+tRscvf7~uc_ From a84323303158fe5197aa89324b5fe5d15a96a5c1 Mon Sep 17 00:00:00 2001 From: Favour Ohans Date: Thu, 17 Sep 2026 04:27:05 +0100 Subject: [PATCH 3/4] stdio review round 2: the consent wrap holds at 80 columns, refusals sanitize, and the hygiene class covers C1 and bidi MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrap width now follows the live terminal (columns minus the note prefix and continuation indent), so the zero-elision promise holds on the default 80-column terminal where the fixed width was quietly re-truncated by the renderer — an attacker controls layout, so a fixed hidden tail was addressable. The one error message that can carry an unvalidated server name sanitizes it first, closing the last raw path to stderr. And string hygiene grows past C0+DEL to everything a terminal or a reader can be steered by: the C1 range (U+009B is a single-codepoint CSI), zero-widths, bidi and directional-isolate controls, line and paragraph separators, and the BOM — commands and args have no legitimate use for any of them. displayString strips the same class. Two new tests: the 80-column consent wrap proving the tail of a long arg stays on screen, and the hostile-name refusal asserting raw ESC never reaches the message while C1 and bidi args refuse outright. --- src/apply/mcp.ts | 18 ++++++++++++++---- src/commands/guided.ts | 13 ++++++++++++- test/stdio-mcp.test.mjs | Bin 17481 -> 20301 bytes 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/apply/mcp.ts b/src/apply/mcp.ts index 0786750..3097597 100644 --- a/src/apply/mcp.ts +++ b/src/apply/mcp.ts @@ -40,7 +40,7 @@ export function planMcpRegistrations( throw new Error(`--mcp ${name} does not match any server in this bundle's manifest.`); } if (!SERVER_NAME.test(name)) { - throw new Error(`--mcp ${name}: server name is not safe to pass along. Nothing was registered.`); + throw new Error(`--mcp ${displayString(name, 60)}: server name is not safe to pass along. Nothing was registered.`); } if (server.transport === "stdio" || server.command !== undefined) { registrations.push(planStdioRegistration(name, server, resolveEnv)); @@ -79,7 +79,9 @@ export function assertPortableStdioServer( server: ManifestMcpServer, ): { command: string; args: string[]; envNames: string[] } { if (!SERVER_NAME.test(name)) { - throw new Error(`--mcp ${name}: server name is not safe to pass along. Nothing was registered.`); + // The one message that may carry an unvalidated name sanitizes it first; + // main.ts prints error messages verbatim to stderr. + throw new Error(`--mcp ${displayString(name, 60)}: server name is not safe to pass along. Nothing was registered.`); } if (typeof server.command !== "string" || !cleanString(server.command)) { throw new Error(`--mcp ${name}: the bundle's command is not a clean string. Nothing was registered.`); @@ -134,12 +136,20 @@ function planStdioRegistration( // list, the PATH note): strip anything that could steer a terminal and cap // the length, so a hostile name or command cannot forge output. export function displayString(text: string, cap = 120): string { - const stripped = text.replace(/[\u0000-\u001f\u007f]/g, "\ufffd"); + const stripped = text.replace(/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069\ufeff]/g, "\ufffd"); return stripped.length > cap ? `${stripped.slice(0, cap)}...` : stripped; } function cleanString(text: string): boolean { - return text.length > 0 && text.length <= MAX_STRING_LENGTH && !/[\u0000-\u001f\u007f]/.test(text); + // C0+DEL, the C1 range (U+009B is a one-codepoint CSI on xterm-class + // terminals), zero-widths, bidi and directional-isolate controls + // (trojan-source reordering), line/paragraph separators, and the BOM: + // commands and args have no legitimate use for any of them. + return ( + text.length > 0 && + text.length <= MAX_STRING_LENGTH && + !/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069\ufeff]/.test(text) + ); } // A missing binary is a warning, not a refusal: the registration still diff --git a/src/commands/guided.ts b/src/commands/guided.ts index 402a515..a019b76 100644 --- a/src/commands/guided.ts +++ b/src/commands/guided.ts @@ -596,7 +596,10 @@ export async function runGuidedApply( const commandLine = [validated.command, ...validated.args].join(" "); ui.note([ `mcp server ${server.name} wants to register on this machine:`, - ...wrapDisplay(` command: ${commandLine}`, 76), + // Width follows the live terminal: the note prefix costs 3 cells, + // fit() truncates at columns-1, and continuation lines indent 4, so + // columns-8 keeps every wrapped character on screen even at 80. + ...wrapDisplay(` command: ${commandLine}`, Math.max(20, ui.width() - 8)), validated.envNames.length > 0 ? ` env (values asked next, never carried): ${validated.envNames.join(", ")}` : " env: none", @@ -719,6 +722,7 @@ interface GuidedUi { select(message: string, items: typeof DESTINATIONS): Promise; confirm(message: string, initial?: boolean): Promise; secret(message: string): Promise; + width(): number; review(lines: string[], dest: string): Promise<"pack" | "skip" | "dest" | null>; outro(...lines: string[]): void; close(): void; @@ -774,6 +778,9 @@ function pickerUi(overrides: GuidedOverrides & { wordmark?: boolean }): GuidedUi if (result.cancelled) open = false; return result.cancelled ? null : result.value; }, + width() { + return screen.columns; + }, // The review screen: the bundle tree, then "Pack it?" with the // destination as a named default. y packs, n leaves, d changes the // destination, esc cancels. The picker still owns no writes. @@ -869,6 +876,10 @@ function plainUi(io: CommandIo, overrides: GuidedOverrides): GuidedUi { } return result.value; }, + width() { + // Plain mode never truncates, so wrapping is cosmetic there. + return 4096; + }, // Plain mode reads the same tree and answers one y/N; changing the // destination in plain mode is the scripted flags' job, which the echo // teaches on pack and on skip alike. diff --git a/test/stdio-mcp.test.mjs b/test/stdio-mcp.test.mjs index 2733c75719e8fdeea1d00a99f0ed2a5c863650b8..9529e509fa312b69f9efca4817c954c7448a056b 100644 GIT binary patch delta 1087 zcmZvbO=}ZD7{@8}1*=dL4ZYYtoxZROF-b!j+SH_!wjeF`V8KIQz{&2j&DiZsJ2RU= zOQ?be4+@pJi(Wm5&`%-aRXhoP0loC%&Dl-T#+QY_WS+PG{{GK?eBSfjDsh`8idAfRbnEiTldvH*r(hiX#;y&a{plb-8;0n2hhNK6eB3Wm3R6se*=I(eR zjP^Nbl>3a+5E@L^;0UFF8uNYR;MJ(4o{cNBIzBqQ%p{cIv0&VhLg+LQPNftL^P+Tn zplXMPxNP2)(xzN0nlGg#Hxzt#QSREJM-#_{l8nRyq}s{na&B{Tb7g(`{^EnR&CN$< zX#RW%INiq*khn^4G^d3%Gdg$0yy@#rwL9&k0eOAh=BW~F?U?U(3m1b^<%Ns$!}8l} zZ9z=B^MmGeVx+$GW;LNS3o#)N9P21SBK2x^MU=Msq)k_pGtG|BPusQeVQD&BWj>2_XemNv z3xY_YW#Y7u4ML((cbdm;S-2!#+v)07eii%bEF|+LT^zpJ77*u{SA|vU_rUePOopa8 zuS}R9v#F6PRcNDIT~+pPYc8pqUkg(w8(K2H>saX`HXfj9e^e=yI%Ba1mS-dp#>!>j zQ5YsEm*KH?yvvoA?8FdB8?dsm)cK5jU|fsD>|`9+tz1xa7i0SW8Tv=lAP9Wo#ujyO zc_C76=8E^sr$VV>Va-*mmMWp!Q9hT)%X6;lzF6$s2N(oZvg5+B?P#V`w@hJXysy=C HYli;@J!flg delta 32 ocmX>*kMU#&k From dfb15a3516cec4f30a096245076ef29c74505f9f Mon Sep 17 00:00:00 2001 From: Favour Ohans Date: Thu, 17 Sep 2026 04:29:54 +0100 Subject: [PATCH 4/4] stdio review round 3: the wrap counts cells, the hygiene class is Cc/Cf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wrapDisplay accumulates visual cells instead of slicing by JS characters, so CJK and emoji content can no longer overflow the renderer's budget and hide consent text behind an ellipsis on narrow terminals — the same boundary finding, now closed for non-ASCII too. String hygiene collapses from a growing codepoint list to the Unicode properties that define the category: Cc (every control, C0 through C1) and Cf (every invisible format character — zero-widths, bidi and isolate controls, BOM, Arabic letter mark, soft hyphen, word joiner), plus the Zl/Zp separators explicitly. Invisibles let a displayed command read identically to a different argv; none of them have a legitimate place in a command line. displayString strips the same class. Tests: a 120-CJK-character arg keeps its tail marker through the wrap with every line inside the cell budget, and U+061C, U+2060 and U+00AD args refuse. --- src/apply/mcp.ts | 12 +++++++----- src/commands/guided.ts | 22 +++++++++++++++++----- test/stdio-mcp.test.mjs | 23 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/apply/mcp.ts b/src/apply/mcp.ts index 3097597..4c3b06e 100644 --- a/src/apply/mcp.ts +++ b/src/apply/mcp.ts @@ -136,19 +136,21 @@ function planStdioRegistration( // list, the PATH note): strip anything that could steer a terminal and cap // the length, so a hostile name or command cannot forge output. export function displayString(text: string, cap = 120): string { - const stripped = text.replace(/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069\ufeff]/g, "\ufffd"); + const stripped = text.replace(/[\p{Cc}\p{Cf}\u2028\u2029]/gu, "\ufffd"); return stripped.length > cap ? `${stripped.slice(0, cap)}...` : stripped; } function cleanString(text: string): boolean { - // C0+DEL, the C1 range (U+009B is a one-codepoint CSI on xterm-class - // terminals), zero-widths, bidi and directional-isolate controls - // (trojan-source reordering), line/paragraph separators, and the BOM: + // The whole class, not a list: Cc is every control (C0, DEL, C1 — U+009B + // is a one-codepoint CSI on xterm-class terminals), Cf is every invisible + // format character (zero-widths, bidi and isolate controls, BOM, ALM, soft + // hyphen, word joiner), plus the Zl/Zp separators explicitly. Invisibles + // let a displayed command read identically to a different argv, and // commands and args have no legitimate use for any of them. return ( text.length > 0 && text.length <= MAX_STRING_LENGTH && - !/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069\ufeff]/.test(text) + !/[\p{Cc}\p{Cf}\u2028\u2029]/u.test(text) ); } diff --git a/src/commands/guided.ts b/src/commands/guided.ts index a019b76..825e01b 100644 --- a/src/commands/guided.ts +++ b/src/commands/guided.ts @@ -22,7 +22,7 @@ import { scanCodex } from "../scan/codex.js"; import { scanOpencode } from "../scan/opencode.js"; import type { ScanItem, ScanReport } from "../scan/types.js"; import { createTheme } from "../tui/theme.js"; -import { Screen } from "../tui/terminal.js"; +import { Screen, visualWidth } from "../tui/terminal.js"; import { createFlow, type Flow, type MultiGroup } from "../tui/components.js"; import { Plain } from "../tui/plain.js"; import { wordmarkLines } from "../tui/wordmark.js"; @@ -357,13 +357,25 @@ export async function runGuided(io: CommandIo, mode: GuidedMode, overrides: Guid } // Full-content wrapping for consent blocks: every character lands on some -// line; nothing hides past an ellipsis. +// line; nothing hides past an ellipsis. Widths are visual CELLS, not JS +// characters — CJK and emoji are two cells each, and a char-counted slice +// would overflow the renderer's budget and get truncated right back. export function wrapDisplay(text: string, width: number): string[] { - if (text.length <= width) return [text]; + if (visualWidth(text) <= width) return [text]; const lines: string[] = []; - for (let index = 0; index < text.length; index += width) { - lines.push(index === 0 ? text.slice(0, width) : ` ${text.slice(index, index + width)}`); + let current = ""; + let cells = 0; + for (const character of text) { + const characterCells = visualWidth(character); + if (cells + characterCells > width && current.length > 0) { + lines.push(lines.length === 0 ? current : ` ${current}`); + current = ""; + cells = 0; + } + current += character; + cells += characterCells; } + lines.push(lines.length === 0 ? current : ` ${current}`); return lines; } diff --git a/test/stdio-mcp.test.mjs b/test/stdio-mcp.test.mjs index 9529e50..cc9b1dc 100644 --- a/test/stdio-mcp.test.mjs +++ b/test/stdio-mcp.test.mjs @@ -494,3 +494,26 @@ test("round 2: an unvalidated hostile name never reaches stderr unsanitized, and ); } }); + + +test("round 3: wide characters cannot resurrect the consent elision, and Cf invisibles refuse", async () => { + const { wrapDisplay } = await import("../dist/main.js"); + const wide = "\u5b57".repeat(120) + "WIDETAILMARK"; + const wrapped = wrapDisplay(` command: npx ${wide}`, 72); + assert.ok(wrapped.join("").includes("WIDETAILMARK"), "the wide tail fell off the wrap"); + for (const line of wrapped) { + const content = line.startsWith(" ") ? line.slice(4) : line; + let cells = 0; + for (const ch of content) cells += /[\u1100-\u9fff]/.test(ch) ? 2 : 1; + assert.ok(cells <= 72, `a wrapped line overflows the cell budget: ${cells}`); + } + + const clean = { name: "x", status: "needs_secret", reason: "", transport: "stdio", command: "npx", envNames: [] }; + for (const dirty of ["a\u061cb", "a\u2060b", "a\u00adb"]) { + assert.throws( + () => planMcpRegistrations([{ ...clean, args: [dirty] }], ["x"], () => "v"), + /not clean strings/, + JSON.stringify(dirty), + ); + } +});