From ed6094b3014c56882da15a7942130b00ea410c6e Mon Sep 17 00:00:00 2001 From: Simone Date: Fri, 24 Apr 2026 06:23:29 +0200 Subject: [PATCH] fix(codex): sanitize approval preview text (#70569) Harden Codex app-server approval preview text sanitization and truncation handling. Thanks @Lucenx9. Co-authored-by: Lucenx9 <185146821+Lucenx9@users.noreply.github.com> --- .../src/app-server/approval-bridge.test.ts | 249 ++++++++++++++++++ .../codex/src/app-server/approval-bridge.ts | 168 ++++++++++-- 2 files changed, 392 insertions(+), 25 deletions(-) diff --git a/extensions/codex/src/app-server/approval-bridge.test.ts b/extensions/codex/src/app-server/approval-bridge.test.ts index 01ad1e3f922..976e5389fcf 100644 --- a/extensions/codex/src/app-server/approval-bridge.test.ts +++ b/extensions/codex/src/app-server/approval-bridge.test.ts @@ -119,6 +119,185 @@ describe("Codex app-server approval bridge", () => { ); }); + it("sanitizes command previews before forwarding approval text and events", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-sanitized-command", + decision: "allow-once", + }); + + await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-sanitized", + command: ["pnpm", "test\n--watch", "\u001b[31mextensions/codex/src/app-server\u001b[0m"], + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + expect(requestPayload).toEqual( + expect.objectContaining({ + description: + "Command: pnpm test --watch extensions/codex/src/app-server\nSession: agent:main:session-1", + }), + ); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ + status: "pending", + command: "pnpm test --watch extensions/codex/src/app-server", + }), + }), + ); + }); + + it("preserves visible OSC-8 link labels in command previews", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-osc", + decision: "allow-once", + }); + const esc = "\u001b"; + + await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-osc", + command: `prefix ${esc}]8;;https://example.com${esc}\\VISIBLE${esc}]8;;${esc}\\ suffix`, + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + expect(requestPayload).toEqual( + expect.objectContaining({ + description: "Command: prefix VISIBLE suffix\nSession: agent:main:session-1", + }), + ); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ command: "prefix VISIBLE suffix" }), + }), + ); + }); + + it("strips bidi and invisible formatting controls from command previews", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-bidi", + decision: "allow-once", + }); + + await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-bidi", + command: "echo safe\u202e cod.exe\u2066 hidden\u2069 \ufeffdone\u{e0100}", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + expect(requestPayload).toEqual( + expect.objectContaining({ + description: "Command: echo safe cod.exe hidden done\nSession: agent:main:session-1", + }), + ); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ command: "echo safe cod.exe hidden done" }), + }), + ); + }); + + it("marks oversized unsafe command previews as omitted", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-omitted-command", + decision: "allow-once", + }); + const esc = "\u001b"; + const oversizedPrefix = `${esc}]8;;https://example.com${esc}\\`.repeat(300); + + await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-omitted", + command: [oversizedPrefix, "TAIL"], + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + expect(requestPayload).toEqual( + expect.objectContaining({ + description: + "Command: [preview truncated or unsafe content omitted]\nSession: agent:main:session-1", + }), + ); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ + commandPreviewOmitted: true, + }), + }), + ); + }); + + it("marks clipped command previews even when a safe prefix remains", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-clipped-command", + decision: "allow-once", + }); + + await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-clipped", + command: `${"a".repeat(5000)} tail`, + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + const description = (requestPayload as { description: string }).description; + expect(description).toContain("[preview truncated or unsafe content omitted]"); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ + commandPreviewOmitted: true, + }), + }), + ); + }); + it("fails closed when no approval route is available", async () => { const params = createParams(); mockCallGatewayTool.mockResolvedValueOnce({ @@ -149,6 +328,43 @@ describe("Codex app-server approval bridge", () => { ); }); + it("sanitizes reason previews before forwarding approval text and events", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-sanitized-reason", + decision: null, + }); + + await handleCodexAppServerApprovalRequest({ + method: "item/fileChange/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "patch-sanitized", + reason: "needs write access\nfor \u001b[31m/tmp\u001b[0m\tplease", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + expect(requestPayload).toEqual( + expect.objectContaining({ + description: "Reason: needs write access for /tmp please\nSession: agent:main:session-1", + }), + ); + expect(params.onAgentEvent).toHaveBeenCalledWith( + expect.objectContaining({ + stream: "approval", + data: expect.objectContaining({ + status: "unavailable", + reason: "needs write access for /tmp please", + }), + }), + ); + }); + it("fails closed for unsupported native approval methods without requesting plugin approval", async () => { const params = createParams(); @@ -277,6 +493,39 @@ describe("Codex app-server approval bridge", () => { expect(description).toContain("High-risk targets:"); }); + it("strips terminal and invisible controls from permission descriptions", async () => { + const params = createParams(); + mockCallGatewayTool.mockResolvedValueOnce({ + id: "plugin:approval-permission-controls", + decision: "allow-once", + }); + + await handleCodexAppServerApprovalRequest({ + method: "item/permissions/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "perm-controls", + permissions: { + network: { allowHosts: ["exa\u009b31mmple.com", "safe\u202e.example.com"] }, + fileSystem: { roots: ["/tmp/\u001b[31mproject\u001b[0m"] }, + }, + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + const [, , requestPayload] = mockCallGatewayTool.mock.calls[0] ?? []; + const description = (requestPayload as { description: string }).description; + expect(description).toContain("example.com"); + expect(description).toContain("safe .example.com"); + expect(description).toContain("/tmp/project"); + expect(description).not.toContain("\u009b"); + expect(description).not.toContain("\u202e"); + expect(description).not.toContain("\u001b"); + }); + it("ignores approval requests that are missing explicit thread or turn ids", async () => { const params = createParams(); diff --git a/extensions/codex/src/app-server/approval-bridge.ts b/extensions/codex/src/app-server/approval-bridge.ts index efa4416fc63..eaed2ad8f55 100644 --- a/extensions/codex/src/app-server/approval-bridge.ts +++ b/extensions/codex/src/app-server/approval-bridge.ts @@ -13,6 +13,35 @@ import { isJsonObject, type JsonObject, type JsonValue } from "./protocol.js"; const PERMISSION_DESCRIPTION_MAX_LENGTH = 700; const PERMISSION_SAMPLE_LIMIT = 2; const PERMISSION_VALUE_MAX_LENGTH = 48; +const APPROVAL_PREVIEW_SCAN_MAX_LENGTH = 4096; +const APPROVAL_PREVIEW_OMITTED = "[preview truncated or unsafe content omitted]"; +const ANSI_OSC_SEQUENCE_RE = new RegExp( + String.raw`(?:\u001b]|\u009d)[^\u001b\u009c\u0007]*(?:\u0007|\u001b\\|\u009c)`, + "g", +); +const ANSI_CONTROL_SEQUENCE_RE = new RegExp( + String.raw`(?:\u001b\[[0-?]*[ -/]*[@-~]|\u009b[0-?]*[ -/]*[@-~]|\u001b[@-Z\\-_])`, + "g", +); +const CONTROL_CHARACTER_RE = new RegExp(String.raw`[\u0000-\u001f\u007f-\u009f]+`, "g"); +const INVISIBLE_FORMATTING_CONTROL_RE = new RegExp( + String.raw`[\u00ad\u034f\u061c\u200b-\u200f\u202a-\u202e\u2060-\u206f\ufeff\ufe00-\ufe0f\u{e0100}-\u{e01ef}]`, + "gu", +); +const DANGLING_TERMINAL_SEQUENCE_SUFFIX_RE = new RegExp( + String.raw`(?:\u001b\][^\u001b\u009c\u0007]*|\u009d[^\u001b\u009c\u0007]*|\u001b\[[0-?]*[ -/]*|\u009b[0-?]*[ -/]*|\u001b)$`, +); + +type ApprovalPreviewSource = { + value: string; + clipped: boolean; +}; + +type SanitizedApprovalPreview = { + text?: string; + omitted: boolean; +}; + export async function handleCodexAppServerApprovalRequest(params: { method: string; requestParams: JsonValue | undefined; @@ -161,8 +190,16 @@ function buildApprovalContext(params: { readString(params.requestParams, "itemId") ?? readString(params.requestParams, "callId") ?? readString(params.requestParams, "approvalId"); - const command = readDisplayCommand(params.requestParams); - const reason = readString(params.requestParams, "reason"); + const commandPreview = sanitizeApprovalPreview( + readDisplayCommandPreview(params.requestParams), + 180, + ); + const reasonPreview = sanitizeApprovalPreview( + readStringPreview(params.requestParams, "reason"), + 180, + ); + const command = commandPreview.text; + const reason = reasonPreview.text; const kind = approvalKindForMethod(params.method); const permissionLines = params.method === "item/permissions/requestApproval" @@ -179,10 +216,14 @@ function buildApprovalContext(params: { const subject = permissionLines[0] ?? (command - ? `Command: ${truncate(command, 180)}` - : reason - ? `Reason: ${truncate(reason, 180)}` - : `Request method: ${params.method}`); + ? `Command: ${formatApprovalPreviewSubject(command, commandPreview.omitted)}` + : commandPreview.omitted + ? `Command: ${APPROVAL_PREVIEW_OMITTED}` + : reason + ? `Reason: ${formatApprovalPreviewSubject(reason, reasonPreview.omitted)}` + : reasonPreview.omitted + ? `Reason: ${APPROVAL_PREVIEW_OMITTED}` + : `Request method: ${params.method}`); const description = permissionLines.length > 0 ? joinDescriptionLinesWithinLimit(permissionLines, PERMISSION_DESCRIPTION_MAX_LENGTH) @@ -205,7 +246,9 @@ function buildApprovalContext(params: { eventDetails: { ...(itemId ? { itemId } : {}), ...(command ? { command } : {}), + ...(commandPreview.omitted ? { commandPreviewOmitted: true } : {}), ...(reason ? { reason } : {}), + ...(reasonPreview.omitted ? { reasonPreviewOmitted: true } : {}), }, }; } @@ -394,12 +437,7 @@ function sanitizePermissionPathValue(value: string): string { } function sanitizePermissionScalar(value: string): string { - let sanitized = ""; - for (let index = 0; index < value.length; index += 1) { - const code = value.charCodeAt(index); - sanitized += code < 32 || code === 127 ? " " : value[index]; - } - return sanitized.replace(/\s+/g, " ").trim(); + return sanitizeVisibleScalar(value); } function permissionHostRisks(value: string): string[] { @@ -531,34 +569,64 @@ function emitApprovalEvent(params: EmbeddedRunAttemptParams, data: AgentApproval params.onAgentEvent?.({ stream: "approval", data: data as unknown as Record }); } -function readDisplayCommand(record: JsonObject | undefined): string | undefined { - const actionCommand = readCommandActions(record); +function readDisplayCommandPreview( + record: JsonObject | undefined, +): ApprovalPreviewSource | undefined { + const actionCommand = readCommandActionsPreview(record); if (actionCommand) { return actionCommand; } - return readCommand(record); + return readCommandPreview(record); } -function readCommandActions(record: JsonObject | undefined): string | undefined { +function readCommandActionsPreview( + record: JsonObject | undefined, +): ApprovalPreviewSource | undefined { const actions = record?.commandActions; if (!Array.isArray(actions)) { return undefined; } - const commands = actions - .map((action) => (isJsonObject(action) ? readString(action, "command") : undefined)) - .filter((command): command is string => Boolean(command)); - return commands.length > 0 ? commands.join(" && ") : undefined; + let source: ApprovalPreviewSource | undefined; + for (const action of actions) { + const command = isJsonObject(action) ? readString(action, "command") : undefined; + if (!command) { + continue; + } + source = appendPreviewPart(source, command, " && "); + if (source.clipped) { + break; + } + } + return source; } -function readCommand(record: JsonObject | undefined): string | undefined { +function readCommandPreview(record: JsonObject | undefined): ApprovalPreviewSource | undefined { const command = record?.command; if (typeof command === "string") { - return command; + return previewSource(command); } - if (Array.isArray(command) && command.every((part) => typeof part === "string")) { - return command.join(" "); + if (!Array.isArray(command)) { + return undefined; } - return undefined; + let source: ApprovalPreviewSource | undefined; + for (const part of command) { + if (typeof part !== "string") { + return undefined; + } + source = appendPreviewPart(source, part, " "); + if (source.clipped) { + break; + } + } + return source; +} + +function readStringPreview( + record: JsonObject | undefined, + key: string, +): ApprovalPreviewSource | undefined { + const value = readString(record, key); + return value === undefined ? undefined : previewSource(value); } function readString(record: JsonObject | undefined, key: string): string | undefined { @@ -570,6 +638,56 @@ function truncate(value: string, maxLength: number): string { return value.length <= maxLength ? value : `${value.slice(0, Math.max(0, maxLength - 3))}...`; } +function previewSource(value: string): ApprovalPreviewSource { + return { + value: value.slice(0, APPROVAL_PREVIEW_SCAN_MAX_LENGTH), + clipped: value.length > APPROVAL_PREVIEW_SCAN_MAX_LENGTH, + }; +} + +function appendPreviewPart( + source: ApprovalPreviewSource | undefined, + part: string, + separator: string, +): ApprovalPreviewSource { + const prefix = source?.value ? `${source.value}${separator}` : ""; + const value = `${prefix}${part}`; + const clipped = source?.clipped === true || value.length > APPROVAL_PREVIEW_SCAN_MAX_LENGTH; + return { + value: value.slice(0, APPROVAL_PREVIEW_SCAN_MAX_LENGTH), + clipped, + }; +} + +function sanitizeApprovalPreview( + source: ApprovalPreviewSource | undefined, + maxLength: number, +): SanitizedApprovalPreview { + if (!source || !source.value) { + return { omitted: false }; + } + const rawPreview = source.value.replace(DANGLING_TERMINAL_SEQUENCE_SUFFIX_RE, ""); + const sanitized = sanitizeVisibleScalar(rawPreview); + if (!sanitized) { + return { omitted: true }; + } + return { text: truncate(sanitized, maxLength), omitted: source.clipped }; +} + +function sanitizeVisibleScalar(value: string): string { + return value + .replace(ANSI_OSC_SEQUENCE_RE, "") + .replace(ANSI_CONTROL_SEQUENCE_RE, "") + .replace(INVISIBLE_FORMATTING_CONTROL_RE, " ") + .replace(CONTROL_CHARACTER_RE, " ") + .replace(/\s+/g, " ") + .trim(); +} + +function formatApprovalPreviewSubject(text: string, omitted: boolean): string { + return omitted ? `${text} ${APPROVAL_PREVIEW_OMITTED}` : text; +} + function joinDescriptionLinesWithinLimit(lines: string[], maxLength: number): string { let description = ""; for (const line of lines) {