Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 57 additions & 15 deletions packages/loopover-mcp/bin/loopover-mcp.js
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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"),
);
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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`);
}
}
}
Expand Down
84 changes: 84 additions & 0 deletions test/unit/mcp-cli-terminal-sanitization.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
40 changes: 35 additions & 5 deletions test/unit/support/mcp-cli-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T extends { band?: unknown; findings?: unknown }>(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<void>((resolve) => server?.close(() => resolve()));
server = null;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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") {
Expand All @@ -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") {
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -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") {
Expand Down