ACP: sanitize terminal tool titles (#55137)

* ACP: sanitize terminal tool titles

Co-authored-by: nexrin <268879349+nexrin@users.noreply.github.com>

* Config: refresh config baseline and stabilize restart pid test

---------

Co-authored-by: nexrin <268879349+nexrin@users.noreply.github.com>
This commit is contained in:
Jacob Tomlinson
2026-03-26 07:12:24 -07:00
committed by GitHub
parent 883239a560
commit 464e2c10a5
7 changed files with 70 additions and 20 deletions

View File

@@ -10,7 +10,11 @@ import {
resolvePermissionRequest,
shouldStripProviderAuthEnvVarsForAcpServer,
} from "./client.js";
import { extractAttachmentsFromPrompt, extractTextFromPrompt } from "./event-mapper.js";
import {
extractAttachmentsFromPrompt,
extractTextFromPrompt,
formatToolTitle,
} from "./event-mapper.js";
const envVar = (...parts: string[]) => parts.join("_");
@@ -625,6 +629,27 @@ describe("resolvePermissionRequest", () => {
expect(prompt).not.toHaveBeenCalled();
expect(res).toEqual({ outcome: { outcome: "cancelled" } });
});
it("sanitizes tool titles before logging and prompting", async () => {
const prompt = vi.fn(async () => false);
const log = vi.fn();
const res = await resolvePermissionRequest(
makePermissionRequest({
toolCall: {
toolCallId: "tool-ansi",
title: 'exec: \u001b[2K\u001b[1A\u001b[2K[permission] Allow "safe"? (y/N) \nnext',
status: "pending",
},
}),
{ prompt, log },
);
expect(prompt).toHaveBeenCalledWith("exec", 'exec: [permission] Allow "safe"? (y/N) \\nnext');
expect(log).toHaveBeenCalledWith(
'\n[permission requested] exec: [permission] Allow "safe"? (y/N) \\nnext (exec) [other]',
);
expect(res).toEqual({ outcome: { outcome: "selected", optionId: "reject" } });
});
});
describe("acp event mapper", () => {
@@ -750,4 +775,14 @@ describe("acp event mapper", () => {
},
]);
});
it("escapes inline control characters in tool titles", () => {
const title = formatToolTitle("exec", {
command: '\u001b[2K\u001b[1A\u001b[2K[permission] Allow "safe"? (y/N) \nnext',
});
expect(title).toBe(
'exec: command: \\x1b[2K\\x1b[1A\\x1b[2K[permission] Allow "safe"? (y/N) \\nnext',
);
});
});

View File

@@ -24,6 +24,7 @@ import {
omitEnvKeysCaseInsensitive,
} from "../secrets/provider-env-vars.js";
import { DANGEROUS_ACP_TOOLS } from "../security/dangerous-tools.js";
import { sanitizeTerminalText } from "../terminal/safe-text.js";
const SAFE_AUTO_APPROVE_TOOL_IDS = new Set(["read", "search", "web_search", "memory_search"]);
const TRUSTED_SAFE_TOOL_ALIASES = new Set(["search"]);
@@ -294,7 +295,7 @@ export async function resolvePermissionRequest(
const prompt = deps.prompt ?? promptUserPermission;
const cwd = deps.cwd ?? process.cwd();
const options = params.options ?? [];
const toolTitle = params.toolCall?.title ?? "tool";
const toolTitle = sanitizeTerminalText(params.toolCall?.title ?? "tool");
const toolName = resolveToolNameForPermission(params);
const toolKind = resolveToolKindForPermission(toolName);

View File

@@ -306,7 +306,9 @@ export function formatToolTitle(
const safe = raw.length > 100 ? `${raw.slice(0, 100)}...` : raw;
return `${key}: ${safe}`;
});
return `${base}: ${parts.join(", ")}`;
// Sanitize at the source so session updates and permission requests never
// inherit raw control bytes from untrusted tool arguments.
return escapeInlineControlChars(`${base}: ${parts.join(", ")}`);
}
export function inferToolKind(name?: string): ToolKind {

View File

@@ -64,24 +64,27 @@ afterEach(() => {
describe.runIf(process.platform !== "win32")("findGatewayPidsOnPortSync", () => {
it("parses lsof output and filters non-openclaw/current processes", () => {
const gatewayPidA = process.pid + 1000;
const gatewayPidB = process.pid + 2000;
const foreignPid = process.pid + 3000;
spawnSyncMock.mockReturnValue({
error: undefined,
status: 0,
stdout: [
`p${process.pid}`,
"copenclaw",
"p4100",
`p${gatewayPidA}`,
"copenclaw-gateway",
"p4200",
`p${foreignPid}`,
"cnode",
"p4300",
`p${gatewayPidB}`,
"cOpenClaw",
].join("\n"),
});
const pids = findGatewayPidsOnPortSync(18789);
expect(pids).toEqual([4100, 4300]);
expect(pids).toEqual([gatewayPidA, gatewayPidB]);
expect(spawnSyncMock).toHaveBeenCalledWith(
"/usr/sbin/lsof",
["-nP", "-iTCP:18789", "-sTCP:LISTEN", "-Fpc"],
@@ -103,11 +106,13 @@ describe.runIf(process.platform !== "win32")("findGatewayPidsOnPortSync", () =>
describe.runIf(process.platform !== "win32")("cleanStaleGatewayProcessesSync", () => {
it("kills stale gateway pids discovered on the gateway port", () => {
const stalePidA = process.pid + 1000;
const stalePidB = process.pid + 2000;
spawnSyncMock
.mockReturnValueOnce({
error: undefined,
status: 0,
stdout: ["p6001", "copenclaw", "p6002", "copenclaw-gateway"].join("\n"),
stdout: [`p${stalePidA}`, "copenclaw", `p${stalePidB}`, "copenclaw-gateway"].join("\n"),
})
.mockReturnValue({
error: undefined,
@@ -118,20 +123,21 @@ describe.runIf(process.platform !== "win32")("cleanStaleGatewayProcessesSync", (
const killed = cleanStaleGatewayProcessesSync();
expect(killed).toEqual([6001, 6002]);
expect(killed).toEqual([stalePidA, stalePidB]);
expect(resolveGatewayPortMock).toHaveBeenCalledWith(undefined, process.env);
expect(killSpy).toHaveBeenCalledWith(6001, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(6002, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(6001, "SIGKILL");
expect(killSpy).toHaveBeenCalledWith(6002, "SIGKILL");
expect(killSpy).toHaveBeenCalledWith(stalePidA, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(stalePidB, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(stalePidA, "SIGKILL");
expect(killSpy).toHaveBeenCalledWith(stalePidB, "SIGKILL");
});
it("uses explicit port override when provided", () => {
const stalePid = process.pid + 1000;
spawnSyncMock
.mockReturnValueOnce({
error: undefined,
status: 0,
stdout: ["p7001", "copenclaw"].join("\n"),
stdout: [`p${stalePid}`, "copenclaw"].join("\n"),
})
.mockReturnValue({
error: undefined,
@@ -142,15 +148,15 @@ describe.runIf(process.platform !== "win32")("cleanStaleGatewayProcessesSync", (
const killed = cleanStaleGatewayProcessesSync(19999);
expect(killed).toEqual([7001]);
expect(killed).toEqual([stalePid]);
expect(resolveGatewayPortMock).not.toHaveBeenCalled();
expect(spawnSyncMock).toHaveBeenCalledWith(
"/usr/sbin/lsof",
["-nP", "-iTCP:19999", "-sTCP:LISTEN", "-Fpc"],
expect.objectContaining({ encoding: "utf8", timeout: 2000 }),
);
expect(killSpy).toHaveBeenCalledWith(7001, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(7001, "SIGKILL");
expect(killSpy).toHaveBeenCalledWith(stalePid, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(stalePid, "SIGKILL");
});
it("returns empty when no stale listeners are found", () => {

View File

@@ -4,6 +4,7 @@ import { sanitizeForLog, splitGraphemes, stripAnsi, visibleWidth } from "./ansi.
describe("terminal ansi helpers", () => {
it("strips ANSI and OSC8 sequences", () => {
expect(stripAnsi("\u001B[31mred\u001B[0m")).toBe("red");
expect(stripAnsi("\u001B[2K\u001B[1Ared")).toBe("red");
expect(stripAnsi("\u001B]8;;https://openclaw.ai\u001B\\link\u001B]8;;\u001B\\")).toBe("link");
});

View File

@@ -1,8 +1,9 @@
const ANSI_SGR_PATTERN = "\\x1b\\[[0-9;]*m";
// Full CSI: ESC [ <params> <final byte> covers cursor movement, erase, and SGR.
const ANSI_CSI_PATTERN = "\\x1b\\[[\\x20-\\x3f]*[\\x40-\\x7e]";
// OSC-8 hyperlinks: ESC ] 8 ; ; url ST ... ESC ] 8 ; ; ST
const OSC8_PATTERN = "\\x1b\\]8;;.*?\\x1b\\\\|\\x1b\\]8;;\\x1b\\\\";
const ANSI_REGEX = new RegExp(ANSI_SGR_PATTERN, "g");
const ANSI_CSI_REGEX = new RegExp(ANSI_CSI_PATTERN, "g");
const OSC8_REGEX = new RegExp(OSC8_PATTERN, "g");
const graphemeSegmenter =
typeof Intl !== "undefined" && "Segmenter" in Intl
@@ -10,7 +11,7 @@ const graphemeSegmenter =
: null;
export function stripAnsi(input: string): string {
return input.replace(OSC8_REGEX, "").replace(ANSI_REGEX, "");
return input.replace(OSC8_REGEX, "").replace(ANSI_CSI_REGEX, "");
}
export function splitGraphemes(input: string): string[] {

View File

@@ -6,6 +6,10 @@ describe("sanitizeTerminalText", () => {
expect(sanitizeTerminalText("a\u009bb\u0085c")).toBe("abc");
});
it("strips cursor and erase ANSI sequences", () => {
expect(sanitizeTerminalText("\u001b[2K\u001b[1Arewritten")).toBe("rewritten");
});
it("escapes line controls while preserving printable text", () => {
expect(sanitizeTerminalText("a\tb\nc\rd")).toBe("a\\tb\\nc\\rd");
});