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") {