From 7506cdad2ffa9e6442f58464f0bddf7bc238e73c Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Thu, 16 Jul 2026 14:22:13 +0800 Subject: [PATCH] fix(mcp): sanitize every terminal path that prints API-controlled text sanitizePlainTextTerminalOutput strips ANSI escapes and control characters, but guarded exactly one call site: validateConfigCli's warning loop. Every other command that printed server-controlled free text wrote it straight to the terminal, so a hostile API response could repaint the screen, erase the lines above it, or park a convincing fake verdict beside the real one. The terminal cannot tell this CLI's text from the payload's. Sanitized: slop-risk and issue-slop's findings and score line, decision-pack's summary and rerunGuidance, repo-decision's nextActions and rerunGuidance, maintain status/queue's action fields, and doctor's check output. issue-slop is the sharpest of these -- the body it assesses is routinely a third party's issue text, so hostile input is the expected case, not a hypothetical one. Both slop commands' score lines are sanitized alongside their findings: the whole payload is the API's, so leaving `band` raw would have kept the exact command exploitable by the exact response the findings are protected from. Two of the listed paths are deliberately left alone, verified rather than assumed. decisionPackToolSummary interpolates the user's own --login plus payload.freshness, and freshness only reaches the string inside an equality guard against "stale"/"rebuilding", so the API cannot route text of its own through it. repoDecisionToolSummary takes only --login and --repo, both typed by the user. Neither is reachable by a payload. cache.rerunGuidance IS sanitized, though its only in-repo producer is a local literal: getDecisionPackWithCache returns the API payload verbatim on the success path, so a response carrying its own `cache` object prints straight through. The local literal is only the fallback. --json stays unsanitized by design: JSON.stringify escapes U+001B, so an escape sequence cannot survive into the printed document, and stripping bytes there would corrupt the contract callers parse. A test pins that reasoning. doctor's details already pass through sanitizeDiagnosticText, but that redacts tokens and local paths and is indifferent to escapes -- so the terminal pass is applied at the print boundary, covering every check source. Tests drive each real command against a fixture answering with a colour + cursor-up + line-erase + OSC + NUL payload. All six fail against the pre-fix CLI; the --json test passes on both sides, which is the point of it. Closes #6261 --- packages/loopover-mcp/bin/loopover-mcp.js | 72 ++++++++++++---- .../mcp-cli-terminal-sanitization.test.ts | 84 +++++++++++++++++++ test/unit/support/mcp-cli-harness.ts | 40 +++++++-- 3 files changed, 176 insertions(+), 20 deletions(-) create mode 100644 test/unit/mcp-cli-terminal-sanitization.test.ts diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index e9ad410e6e..c541545153 100755 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -2282,7 +2282,18 @@ async function maintainCli(args) { if (subcommand === "status") { const payload = await apiGet(queueBase); const actions = payload.pendingActions ?? []; - emit(payload, [`Agent approval queue for ${repoFullName}: ${actions.length} pending.`, ...actions.map((action) => `- ${action.id} ${action.actionClass} on #${action.pullNumber} ${action.reason ?? ""}`)].join("\n")); + // #6261: every field here is the API's. `emit` sends this string to the terminal only on the plain-text path + // (--json re-serializes `payload` instead), so sanitizing the composed line costs the JSON contract nothing. + emit( + payload, + [ + `Agent approval queue for ${repoFullName}: ${actions.length} pending.`, + ...actions.map( + (action) => + `- ${sanitizePlainTextTerminalOutput(action.id)} ${sanitizePlainTextTerminalOutput(action.actionClass)} on #${sanitizePlainTextTerminalOutput(action.pullNumber)} ${sanitizePlainTextTerminalOutput(action.reason ?? "")}`, + ), + ].join("\n"), + ); return; } // #2236 — explicit queue listing so maintainers can discover ids for approve/reject (alias: pending). @@ -2294,10 +2305,12 @@ async function maintainCli(args) { [ `Pending agent actions for ${repoFullName}: ${actions.length}.`, ...actions.map((action) => { - const kind = action.actionClass ?? action.kind ?? "unknown"; - const target = action.pullNumber != null ? `#${action.pullNumber}` : (action.target ?? "—"); - const summary = action.reason ?? action.summary ?? ""; - return `- ${action.id} ${kind} ${target}${summary ? ` ${summary}` : ""}`; + // #6261: sanitize each field as it is read, so the fallback chains can't smuggle an escape in through + // whichever branch happens to win (`kind` alone has three sources). + const kind = sanitizePlainTextTerminalOutput(action.actionClass ?? action.kind ?? "unknown"); + const target = action.pullNumber != null ? `#${sanitizePlainTextTerminalOutput(action.pullNumber)}` : sanitizePlainTextTerminalOutput(action.target ?? "—"); + const summary = sanitizePlainTextTerminalOutput(action.reason ?? action.summary ?? ""); + return `- ${sanitizePlainTextTerminalOutput(action.id)} ${kind} ${target}${summary ? ` ${summary}` : ""}`; }), ].join("\n"), ); @@ -2560,6 +2573,18 @@ async function lintPrTextCli(args) { for (const fix of payload.fixes ?? []) process.stdout.write(`- ${fix}\n`); } +// Strip ANSI escapes + control characters from text this CLI prints as plain text. Rule (#6261): every value that +// reaches a terminal from a source the user does not control -- an API response, or free text the API echoed back +// from a third-party issue/PR -- goes through this first. Otherwise a hostile string can repaint the screen, +// rewrite earlier lines, or fake a success next to a real failure, since the terminal cannot tell our text from +// the payload's. +// +// Two things deliberately do NOT go through it: +// - `--json` output. JSON.stringify escapes U+001B (and the rest of U+0000-U+001F) as a \u001b literal, so an escape +// sequence cannot survive into the printed document -- and sanitizing there would corrupt the machine-readable +// contract callers parse. +// - Our own literals, and values the user themself passed in (--login, --repo). Those are already the user's, +// and the CLI prints no colour of its own -- there is no intentional ANSI in this file to preserve. function sanitizePlainTextTerminalOutput(value) { return String(value) .replace(/\x1b(?:\[[0-?]*[ -/]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\)|[PX^_][^\x1b]*(?:\x1b\\)|[@-_])/g, "") @@ -2657,8 +2682,11 @@ async function slopRiskCli(args) { process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); return; } - process.stdout.write(`Slop risk: ${payload.slopRisk} (${payload.band})\n`); - for (const finding of payload.findings ?? []) process.stdout.write(`- ${finding.title}: ${finding.detail}\n`); + // #6261: the whole payload is the API's, so the score line is sanitized alongside the findings -- leaving `band` + // raw would keep this exact command exploitable by the exact response the findings are being protected from. + process.stdout.write(`Slop risk: ${sanitizePlainTextTerminalOutput(payload.slopRisk)} (${sanitizePlainTextTerminalOutput(payload.band)})\n`); + for (const finding of payload.findings ?? []) + process.stdout.write(`- ${sanitizePlainTextTerminalOutput(finding.title)}: ${sanitizePlainTextTerminalOutput(finding.detail)}\n`); } function printIssueSlopHelp() { @@ -2690,8 +2718,11 @@ async function issueSlopCli(args) { process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); return; } - process.stdout.write(`Issue slop risk: ${payload.slopRisk} (${payload.band})\n`); - for (const finding of payload.findings ?? []) process.stdout.write(`- ${finding.title}: ${finding.detail}\n`); + // #6261: same as slop-risk, and the sharper case of the two -- the body being assessed is routinely a THIRD + // party's issue text, so a hostile issue is the expected input here, not a hypothetical one. + process.stdout.write(`Issue slop risk: ${sanitizePlainTextTerminalOutput(payload.slopRisk)} (${sanitizePlainTextTerminalOutput(payload.band)})\n`); + for (const finding of payload.findings ?? []) + process.stdout.write(`- ${sanitizePlainTextTerminalOutput(finding.title)}: ${sanitizePlainTextTerminalOutput(finding.detail)}\n`); } function printDecisionPackHelp() { @@ -2716,9 +2747,12 @@ async function decisionPackCli(options) { process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); return; } + // #6261: decisionPackToolSummary is left alone -- verified, not assumed. It interpolates `login` (the user's own + // --login/env value) and `payload.freshness`, and freshness only ever reaches the string inside an equality guard + // against the literals "stale"/"rebuilding", so the API cannot route text of its own choosing through it. process.stdout.write(`${decisionPackToolSummary(login, payload)}\n`); - if (payload.summary) process.stdout.write(`${payload.summary}\n`); - if (payload.cache?.rerunGuidance) process.stdout.write(`Rerun when: ${payload.cache.rerunGuidance}\n`); + if (payload.summary) process.stdout.write(`${sanitizePlainTextTerminalOutput(payload.summary)}\n`); + if (payload.cache?.rerunGuidance) process.stdout.write(`Rerun when: ${sanitizePlainTextTerminalOutput(payload.cache.rerunGuidance)}\n`); } function printRepoDecisionHelp() { @@ -2746,10 +2780,12 @@ async function repoDecisionCli(options) { process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); return; } + // #6261: repoDecisionToolSummary is left alone for the same reason -- it interpolates only `login` and + // `repoFullName`, both of which the user typed on their own command line. No payload text reaches it. process.stdout.write(`${repoDecisionToolSummary(login, repoFullName, payload)}\n`); const actions = payload.decision?.nextActions ?? payload.decision?.publicNextActions ?? []; - for (const action of actions.slice(0, 3)) process.stdout.write(`- ${action}\n`); - if (payload.cache?.rerunGuidance) process.stdout.write(`Rerun when: ${payload.cache.rerunGuidance}\n`); + for (const action of actions.slice(0, 3)) process.stdout.write(`- ${sanitizePlainTextTerminalOutput(action)}\n`); + if (payload.cache?.rerunGuidance) process.stdout.write(`Rerun when: ${sanitizePlainTextTerminalOutput(payload.cache.rerunGuidance)}\n`); } function runCacheCli(args) { @@ -3708,9 +3744,15 @@ async function doctor(options) { if (group.nextCommand?.command) process.stdout.write(` ${group.nextCommand.command}\n`); continue; } + // #6261: a check's `detail` is the one field here that carries text this CLI didn't write -- an API error + // message, an npm-registry error, a compatibility report's `error`. Some of those already pass through + // sanitizeDiagnosticText, but that redacts tokens and local paths; it is indifferent to escape sequences. So + // the terminal pass belongs here at the print boundary, where it covers every check source at once. for (const check of group.checks ?? []) { - process.stdout.write(`- ${check.status}: ${check.name} - ${check.detail}\n`); - if (check.remediation) process.stdout.write(` ${check.remediation}\n`); + process.stdout.write( + `- ${sanitizePlainTextTerminalOutput(check.status)}: ${sanitizePlainTextTerminalOutput(check.name)} - ${sanitizePlainTextTerminalOutput(check.detail)}\n`, + ); + if (check.remediation) process.stdout.write(` ${sanitizePlainTextTerminalOutput(check.remediation)}\n`); } } } diff --git a/test/unit/mcp-cli-terminal-sanitization.test.ts b/test/unit/mcp-cli-terminal-sanitization.test.ts new file mode 100644 index 0000000000..ec75f46984 --- /dev/null +++ b/test/unit/mcp-cli-terminal-sanitization.test.ts @@ -0,0 +1,84 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { closeFixtureServer, runAsync, startFixtureServer } from "./support/mcp-cli-harness"; + +// #6261: sanitizePlainTextTerminalOutput guarded exactly one output path (validateConfigCli's warnings) while +// every other command that prints API-controlled free text wrote it straight to the terminal. A hostile response +// could therefore repaint the screen, erase the lines above it, or park a convincing fake verdict next to the real +// one -- the terminal cannot tell our text from the payload's. +// +// Each test drives a real command against a fixture that answers with the attack string, and asserts the escape +// never lands. All of them fail against the pre-fix CLI. + +const ESC = "\u001b"; +/** A realistic payload: colour + cursor-up + line-erase + an OSC title-set (BEL-terminated) + a bare NUL. */ +const INJECTION = `${ESC}[31mRED${ESC}[0m${ESC}[1A${ESC}[2K${ESC}]0;pwned\u0007\u0000TAIL`; +/** Exactly the class the sanitizer strips: C0/C1 controls and DEL. */ +const CONTROL_CHARS = /[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f-\u009f]/; + +/** The text must still be readable -- sanitized, not merely dropped -- with every escape gone. */ +function expectNeutralized(output: string) { + expect(output).not.toContain(ESC); + expect(output).not.toMatch(CONTROL_CHARS); + expect(output).toContain("TAIL"); +} + +describe("loopover-mcp CLI — terminal-escape sanitization (#6261)", () => { + let tempDir: string | null = null; + + afterEach(async () => { + await closeFixtureServer(); + if (tempDir) rmSync(tempDir, { recursive: true, force: true }); + tempDir = null; + }); + + async function env() { + tempDir = mkdtempSync(join(tmpdir(), "loopover-cli-")); + const url = await startFixtureServer({ terminalInjection: INJECTION }); + return { LOOPOVER_API_URL: url, LOOPOVER_TOKEN: "session-token", LOOPOVER_CONFIG_DIR: tempDir, LOOPOVER_API_TIMEOUT_MS: "1000" }; + } + + it("slop-risk: a hostile finding title/detail and band cannot reach the terminal", async () => { + const out = await runAsync(["slop-risk", "--changed-file", "src/widget.ts:80:2", "--description", "A description."], await env()); + expectNeutralized(out); + }); + + it("issue-slop: a hostile finding title/detail and band cannot reach the terminal", async () => { + // The sharpest case: the body assessed here is routinely a third party's issue text. + const out = await runAsync(["issue-slop", "--title", "Fix bug", "--body", "Some body."], await env()); + expectNeutralized(out); + }); + + it("decision-pack: a hostile summary and rerunGuidance cannot reach the terminal", async () => { + const out = await runAsync(["decision-pack", "--login", "JSONbored"], await env()); + expectNeutralized(out); + }); + + it("repo-decision: a hostile nextActions entry and rerunGuidance cannot reach the terminal", async () => { + const out = await runAsync(["repo-decision", "--login", "JSONbored", "--repo", "JSONbored/gittensory"], await env()); + expectNeutralized(out); + }); + + it("maintain status: a hostile action reason/actionClass cannot reach the terminal", async () => { + const out = await runAsync(["maintain", "status", "--repo", "owner/repo"], await env()); + expectNeutralized(out); + }); + + it("maintain queue: a hostile action reason/actionClass cannot reach the terminal", async () => { + const out = await runAsync(["maintain", "queue", "--repo", "owner/repo"], await env()); + expectNeutralized(out); + }); + + // --json is deliberately NOT sanitized: JSON.stringify escapes U+001B as a \u001b literal, so an escape + // sequence cannot survive into the printed document, and stripping bytes there would corrupt the + // machine-readable contract callers parse. This pins that reasoning instead of trusting it. + it("--json keeps the payload verbatim yet still cannot emit a raw escape", async () => { + const out = await runAsync(["maintain", "status", "--repo", "owner/repo", "--json"], await env()); + expect(out).not.toContain(ESC); + expect(out).not.toMatch(CONTROL_CHARS); + const parsed = JSON.parse(out) as { pendingActions: Array<{ reason: string }> }; + expect(parsed.pendingActions[0]!.reason).toBe(INJECTION); + }); +}); diff --git a/test/unit/support/mcp-cli-harness.ts b/test/unit/support/mcp-cli-harness.ts index 5226b0243f..bab7050a22 100644 --- a/test/unit/support/mcp-cli-harness.ts +++ b/test/unit/support/mcp-cli-harness.ts @@ -8,6 +8,12 @@ import { expect } from "vitest"; export const bin = join(process.cwd(), "packages/loopover-mcp/bin/loopover-mcp.js"); let server: Server | null = null; +/** #6261: put `injection` in every free-text field of a slop assessment that reaches plain-text output. */ +function withTerminalInjection(fixture: T, injection?: string): T { + if (!injection) return fixture; + return { ...fixture, band: injection, findings: [{ title: injection, detail: injection }] }; +} + export async function closeFixtureServer() { if (server) await new Promise((resolve) => server?.close(() => resolve())); server = null; @@ -126,6 +132,9 @@ export function readDecisionPackCacheText(configDir: string) { export async function startFixtureServer( options: { + /** #6261: when set, the routes whose free text reaches plain-text terminal output return this string in + * those fields, standing in for a hostile API. Tests assert it can't reach the terminal un-neutered. */ + terminalInjection?: string; latestVersion?: string; latestRecommendedMcpVersion?: string; minMcpVersion?: string; @@ -230,7 +239,13 @@ export async function startFixtureServer( response.end(options.decisionPackErrorBody ?? JSON.stringify({ error: "decision_pack_unavailable" })); return; } - response.end(JSON.stringify(decisionPackFixture())); + response.end( + JSON.stringify( + options.terminalInjection + ? { ...decisionPackFixture(), summary: options.terminalInjection, cache: { rerunGuidance: options.terminalInjection } } + : decisionPackFixture(), + ), + ); return; } if (request.url === "/v1/contributors/JSONbored/repos/JSONbored/gittensory/decision" && request.method === "GET") { @@ -240,7 +255,16 @@ export async function startFixtureServer( response.end(options.repoDecisionErrorBody ?? JSON.stringify({ error: "repo_decision_unavailable" })); return; } - response.end(JSON.stringify({ status: "ready", login: "JSONbored", repoFullName: "JSONbored/gittensory", decision: decisionPackFixture().repoDecisions[0] })); + const repoDecision = decisionPackFixture().repoDecisions[0]; + response.end( + JSON.stringify({ + status: "ready", + login: "JSONbored", + repoFullName: "JSONbored/gittensory", + decision: options.terminalInjection ? { ...repoDecision, nextActions: [options.terminalInjection] } : repoDecision, + ...(options.terminalInjection ? { cache: { rerunGuidance: options.terminalInjection } } : {}), + }), + ); return; } if (request.url === "/v1/agent/plan-next-work" && request.method === "POST") { @@ -299,12 +323,12 @@ export async function startFixtureServer( tests?: string[]; testFiles?: string[]; }; - response.end(JSON.stringify(slopRiskFixture(body))); + response.end(JSON.stringify(withTerminalInjection(slopRiskFixture(body), options.terminalInjection))); return; } if (request.url === "/v1/lint/issue-slop" && request.method === "POST") { const body = (await readJsonRequest(request)) as { title?: string; body?: string }; - response.end(JSON.stringify(issueSlopFixture(body))); + response.end(JSON.stringify(withTerminalInjection(issueSlopFixture(body), options.terminalInjection))); return; } if (request.url === "/v1/opportunities/find" && request.method === "POST") { @@ -352,7 +376,13 @@ export async function startFixtureServer( } // #784 maintainer controls (agent approval queue + kill-switch). if (request.url === "/v1/repos/owner/repo/agent/pending-actions" && request.method === "GET") { - response.end(JSON.stringify({ repoFullName: "owner/repo", pendingActions: [{ id: "pa-1", actionClass: "merge", pullNumber: 7, reason: "clean", status: "pending" }] })); + const action = { id: "pa-1", actionClass: "merge", pullNumber: 7, reason: "clean", status: "pending" }; + response.end( + JSON.stringify({ + repoFullName: "owner/repo", + pendingActions: [options.terminalInjection ? { ...action, reason: options.terminalInjection, actionClass: options.terminalInjection } : action], + }), + ); return; } if (request.url === "/v1/repos/owner/repo/maintainer-noise" && request.method === "GET") {