diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cbea5228c1..62a18be63af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Control UI/Agents: redact tool-call args, partial/final results, derived exec output, and configured custom secret patterns before streaming tool events to the Control UI, so tool output cannot expose provider or channel credentials. Fixes #72283. (#72319) Thanks @volcano303 and @BunsDev. - CLI/model probes: fail local `infer model run` probes when the provider returns no text output, so unreachable local providers and empty completions no longer look like successful smoke tests. Refs #73023. Thanks @pavelyortho-cyber. - CLI/Ollama: run local `infer model run` through the lean provider completion path and skip global model discovery for one-shot local probes, so Ollama smoke tests no longer pay full chat-agent/tool startup cost or hang before the native `/api/chat` request. Fixes #72851. Thanks @TotalRes2020. - Doctor/gateway services: ignore launchd/systemd companion services that only reference the gateway as a dependency, suppress inactive Linux extra-service warnings, and avoid rewriting a running systemd gateway command/entrypoint during doctor repair. Carries forward #39118. Thanks @therk. diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts index 9d216daef4e..8d11b2d6946 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.test.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.test.ts @@ -1,5 +1,9 @@ import type { AgentEvent } from "@mariozechner/pi-agent-core"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + onAgentEvent as registerAgentEventListener, + resetAgentEventsForTest, +} from "../infra/agent-events.js"; import type { MessagingToolSend } from "./pi-embedded-messaging.types.js"; import { handleToolExecutionEnd, @@ -959,3 +963,147 @@ describe("messaging tool media URL tracking", () => { expect(ctx.state.pendingMessagingMediaUrls.has("tool-m3")).toBe(false); }); }); + +describe("control UI credential redaction (issue #72283)", () => { + afterEach(() => { + resetAgentEventsForTest(); + }); + + it("redacts secrets in args before emitting the tool start event", async () => { + const events: Array<{ stream?: string; data?: Record }> = []; + registerAgentEventListener((evt) => { + events.push(evt as never); + }); + const { ctx } = createTestContext(); + + await handleToolExecutionStart( + ctx as never, + { + type: "tool_execution_start", + toolName: "gateway", + toolCallId: "tool-secret-args", + args: { + action: "config.apply", + raw: 'apiKey: "sk-1234567890abcdefXYZ"', + headers: { Authorization: "Bearer abcdef0123456789QWERTY=" }, + }, + } as never, + ); + + const startEvent = events.find( + (evt) => evt.stream === "tool" && (evt.data as { phase?: string })?.phase === "start", + ); + expect(startEvent).toBeDefined(); + const emittedArgs = (startEvent?.data as { args?: Record })?.args ?? {}; + const serialized = JSON.stringify(emittedArgs); + expect(serialized).not.toContain("sk-1234567890abcdefXYZ"); + expect(serialized).not.toContain("abcdef0123456789QWERTY="); + expect(serialized).toContain("config.apply"); + }); + + it("redacts secrets in exec aggregated stdout before emitting command_output", async () => { + const { ctx, onAgentEvent } = createTestContext(); + + await handleToolExecutionStart( + ctx as never, + { + type: "tool_execution_start", + toolName: "exec", + toolCallId: "tool-exec-secret", + args: { command: "cat ~/.openclaw/openclaw.json" }, + } as never, + ); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "exec", + toolCallId: "tool-exec-secret", + isError: false, + result: { + details: { + status: "completed", + aggregated: + 'OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789\napiKey: "ghp_abcdefghij1234567890"', + exitCode: 0, + durationMs: 12, + cwd: "/tmp/work", + }, + }, + } as never, + ); + + const commandOutputCalls = onAgentEvent.mock.calls + .map((call) => call[0]) + .filter((arg: unknown) => (arg as { stream?: string })?.stream === "command_output"); + expect(commandOutputCalls.length).toBeGreaterThan(0); + const lastOutput = commandOutputCalls.at(-1) as { data?: { output?: string } } | undefined; + expect(lastOutput?.data?.output).toBeDefined(); + expect(lastOutput?.data?.output).not.toContain("sk-or-v1-abcdef0123456789"); + expect(lastOutput?.data?.output).not.toContain("ghp_abcdefghij1234567890"); + expect(lastOutput?.data?.output).toContain("OPENROUTER_API_KEY="); + }); + + it("redacts details-only results before emitting the tool result event", async () => { + const events: Array<{ stream?: string; data?: Record }> = []; + registerAgentEventListener((evt) => { + events.push(evt as never); + }); + const { ctx } = createTestContext(); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "gateway", + toolCallId: "tool-details-secret", + isError: false, + result: { + details: { + config: { apiKey: "sk-1234567890abcdefXYZ", model: "gpt-4" }, + }, + }, + } as never, + ); + + const resultEvent = events.find( + (evt) => evt.stream === "tool" && (evt.data as { phase?: string })?.phase === "result", + ); + expect(resultEvent).toBeDefined(); + const serialized = JSON.stringify(resultEvent?.data?.result); + expect(serialized).not.toContain("sk-1234567890abcdefXYZ"); + expect(serialized).toContain("gpt-4"); + }); + + it("redacts primitive string results before emitting the tool result event", async () => { + const events: Array<{ stream?: string; data?: Record }> = []; + registerAgentEventListener((evt) => { + events.push(evt as never); + }); + const { ctx } = createTestContext(); + + await handleToolExecutionEnd( + ctx as never, + { + type: "tool_execution_end", + toolName: "gateway", + toolCallId: "tool-string-secret", + isError: false, + result: "OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789", + } as never, + ); + + const resultEvent = events.find( + (evt) => evt.stream === "tool" && (evt.data as { phase?: string })?.phase === "result", + ); + expect(resultEvent).toBeDefined(); + const emittedResult = resultEvent?.data?.result; + expect(typeof emittedResult).toBe("string"); + if (typeof emittedResult !== "string") { + throw new Error("expected string result"); + } + expect(emittedResult).not.toContain("sk-or-v1-abcdef0123456789"); + expect(emittedResult).toContain("OPENROUTER_API_KEY="); + }); +}); diff --git a/src/agents/pi-embedded-subscribe.handlers.tools.ts b/src/agents/pi-embedded-subscribe.handlers.tools.ts index b073bfe0010..15dfbbbabda 100644 --- a/src/agents/pi-embedded-subscribe.handlers.tools.ts +++ b/src/agents/pi-embedded-subscribe.handlers.tools.ts @@ -34,6 +34,7 @@ import { filterToolResultMediaUrls, isToolResultError, isToolResultTimedOut, + sanitizeToolArgs, sanitizeToolResult, } from "./pi-embedded-subscribe.tools.js"; import { inferToolMetaFromArgs } from "./pi-embedded-utils.js"; @@ -634,7 +635,7 @@ export function handleToolExecutionStart( phase: "start", name: toolName, toolCallId, - args: args as Record, + args: sanitizeToolArgs(args) as Record, }, }); const itemData: AgentItemEventData = { @@ -945,7 +946,8 @@ export async function handleToolExecutionEnd( }); if (isExecToolName(toolName)) { - const execDetails = readExecToolDetails(result); + // Use sanitizedResult so `aggregated` is redacted before reaching command_output. + const execDetails = readExecToolDetails(sanitizedResult); const commandItemId = buildCommandItemId(toolCallId); if ( execDetails?.status === "approval-pending" || @@ -1083,7 +1085,7 @@ export async function handleToolExecutionEnd( } if (isPatchToolName(toolName)) { - const patchSummary = readApplyPatchSummary(result); + const patchSummary = readApplyPatchSummary(sanitizedResult); const patchItemId = buildPatchItemId(toolCallId); const summaryText = patchSummary ? buildPatchSummaryText(patchSummary) : undefined; emitTrackedItemEvent(ctx, { diff --git a/src/agents/pi-embedded-subscribe.tools.test.ts b/src/agents/pi-embedded-subscribe.tools.test.ts index 6407e23360a..53b9977b2b3 100644 --- a/src/agents/pi-embedded-subscribe.tools.test.ts +++ b/src/agents/pi-embedded-subscribe.tools.test.ts @@ -1,5 +1,14 @@ -import { describe, expect, it } from "vitest"; -import { extractToolErrorMessage } from "./pi-embedded-subscribe.tools.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as loggingConfigModule from "../logging/config.js"; +import { + extractToolErrorMessage, + sanitizeToolArgs, + sanitizeToolResult, +} from "./pi-embedded-subscribe.tools.js"; + +afterEach(() => { + vi.restoreAllMocks(); +}); describe("extractToolErrorMessage", () => { it("ignores non-error status values", () => { @@ -34,3 +43,178 @@ describe("extractToolErrorMessage", () => { ).toBe("SYSTEM_RUN_DENIED: approval required"); }); }); + +function getTextContent(result: unknown, index = 0): string { + const record = result as { content: Array<{ text: string }> }; + return record.content[index].text; +} + +describe("sanitizeToolResult", () => { + it("redacts JSON-style apiKey fields in text content blocks", () => { + const result = { + content: [ + { + type: "text", + text: '{"apiKey":"sk-1234567890abcdef","model":"gpt-4"}', + }, + ], + }; + const text = getTextContent(sanitizeToolResult(result)); + expect(text).not.toContain("sk-1234567890abcdef"); + expect(text).toContain("model"); + }); + + it("redacts ENV-style credential assignments", () => { + const result = { + content: [ + { + type: "text", + text: "OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789\nMODEL=gpt-4", + }, + ], + }; + const text = getTextContent(sanitizeToolResult(result)); + expect(text).not.toContain("sk-or-v1-abcdef0123456789"); + expect(text).toContain("MODEL=gpt-4"); + }); + + it("redacts Bearer authorization tokens", () => { + const result = { + content: [{ type: "text", text: "Authorization: Bearer abcdef0123456789QWERTY=" }], + }; + const text = getTextContent(sanitizeToolResult(result)); + expect(text).not.toContain("abcdef0123456789QWERTY="); + }); + + it("preserves image content stripping behavior", () => { + const result = { + content: [{ type: "image", data: "base64imagedata", mimeType: "image/png" }], + }; + const sanitized = sanitizeToolResult(result) as { + content: Array<{ data?: string; bytes?: number; omitted?: boolean }>; + }; + expect(sanitized.content[0].data).toBeUndefined(); + expect(sanitized.content[0].omitted).toBe(true); + expect(sanitized.content[0].bytes).toBe("base64imagedata".length); + }); + + it("redacts secrets inside result.details (e.g. exec aggregated stdout)", () => { + const result = { + content: [{ type: "text", text: "ok" }], + details: { + status: "completed", + aggregated: + 'OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789\napiKey: "ghp_abcdefghij1234567890"', + exitCode: 0, + cwd: "/tmp/work", + }, + }; + const sanitized = sanitizeToolResult(result) as { + details: { status: string; aggregated: string; exitCode: number; cwd: string }; + }; + expect(sanitized.details.aggregated).not.toContain("sk-or-v1-abcdef0123456789"); + expect(sanitized.details.aggregated).not.toContain("ghp_abcdefghij1234567890"); + expect(sanitized.details.status).toBe("completed"); + expect(sanitized.details.exitCode).toBe(0); + expect(sanitized.details.cwd).toBe("/tmp/work"); + }); + + it("redacts secrets at the top level outside content/details", () => { + const result = { + output: "OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789", + metadata: { + token: "ghp_abcdefghij1234567890ABCDEF", + nested: { auth: "Bearer abcdef0123456789QWERTY=" }, + }, + summary: "ok", + }; + const sanitized = sanitizeToolResult(result) as { + output: string; + metadata: { token: string; nested: { auth: string } }; + summary: string; + }; + expect(sanitized.output).not.toContain("sk-or-v1-abcdef0123456789"); + expect(sanitized.metadata.token).not.toContain("ghp_abcdefghij1234567890ABCDEF"); + expect(sanitized.metadata.nested.auth).not.toContain("abcdef0123456789QWERTY="); + expect(sanitized.summary).toBe("ok"); + }); + + it("redacts a details-only result with no content array", () => { + const result = { + details: { + config: { apiKey: "sk-1234567890abcdefXYZ", model: "gpt-4" }, + }, + }; + const sanitized = sanitizeToolResult(result) as { + details: { config: { apiKey: string; model: string } }; + }; + expect(sanitized.details.config.apiKey).not.toContain("sk-1234567890abcdefXYZ"); + expect(sanitized.details.config.model).toBe("gpt-4"); + }); + + it("redacts primitive string results", () => { + const sanitized = sanitizeToolResult("OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789") as string; + + expect(sanitized).not.toContain("sk-or-v1-abcdef0123456789"); + expect(sanitized).toContain("OPENROUTER_API_KEY="); + }); + + it("preserves top-level arrays while redacting nested strings", () => { + const sanitized = sanitizeToolResult([ + { output: "Authorization: Bearer abcdef0123456789QWERTY=" }, + "apiKey=sk-1234567890abcdefXYZ", + ]) as Array<{ output: string } | string>; + + expect(Array.isArray(sanitized)).toBe(true); + expect(JSON.stringify(sanitized)).not.toContain("abcdef0123456789QWERTY="); + expect(JSON.stringify(sanitized)).not.toContain("sk-1234567890abcdefXYZ"); + expect((sanitized[0] as { output: string }).output).toContain("Authorization: Bearer"); + }); + + it("applies configured redact patterns to Control UI tool payloads", () => { + vi.spyOn(loggingConfigModule, "readLoggingConfig").mockReturnValue({ + redactSensitive: "off", + redactPatterns: [String.raw`\bcustom-secret-[A-Za-z0-9]+\b`], + }); + + const result = { + content: [{ type: "text", text: "value custom-secret-abc123" }], + }; + const text = getTextContent(sanitizeToolResult(result)); + + expect(text).not.toContain("custom-secret-abc123"); + expect(text).toContain("custom…c123"); + }); +}); + +describe("sanitizeToolArgs", () => { + it("redacts string-valued credentials nested anywhere in args", () => { + const args = { + apiKey: "sk-1234567890abcdefXYZ", + headers: { Authorization: "Bearer abcdef0123456789QWERTY=" }, + command: "OPENROUTER_API_KEY=sk-or-v1-abcdef0123456789 ./run.sh", + flags: ["--api-key", "sk-1234567890abcdefXYZ"], + }; + const sanitized = sanitizeToolArgs(args) as { + apiKey: string; + headers: { Authorization: string }; + command: string; + flags: string[]; + }; + expect(sanitized.apiKey).not.toContain("sk-1234567890abcdefXYZ"); + expect(sanitized.headers.Authorization).not.toContain("abcdef0123456789QWERTY="); + expect(sanitized.command).not.toContain("sk-or-v1-abcdef0123456789"); + expect(sanitized.flags.join(" ")).not.toContain("sk-1234567890abcdefXYZ"); + expect(sanitized.flags[0]).toBe("--api-key"); + }); + + it("passes through null/undefined and non-string primitives unchanged", () => { + expect(sanitizeToolArgs(undefined)).toBeUndefined(); + expect(sanitizeToolArgs(null)).toBeNull(); + expect(sanitizeToolArgs(42)).toBe(42); + expect(sanitizeToolArgs({ count: 3, file_path: "/tmp/x.txt" })).toEqual({ + count: 3, + file_path: "/tmp/x.txt", + }); + }); +}); diff --git a/src/agents/pi-embedded-subscribe.tools.ts b/src/agents/pi-embedded-subscribe.tools.ts index ebe91ecf0e9..7a334ce372f 100644 --- a/src/agents/pi-embedded-subscribe.tools.ts +++ b/src/agents/pi-embedded-subscribe.tools.ts @@ -1,5 +1,6 @@ import { getChannelPlugin, normalizeChannelId } from "../channels/plugins/index.js"; import { normalizeTargetForProvider } from "../infra/outbound/target-normalization.js"; +import { redactToolPayloadText } from "../logging/redact.js"; import { splitMediaFromOutput } from "../media/parse.js"; import { pluginRegistrationContractRegistry } from "../plugins/contracts/registry.js"; import { @@ -114,34 +115,84 @@ function isHostDenialToolText(text: string): boolean { return normalized.toLowerCase().includes("approval cannot safely bind"); } +function redactStringsDeep(value: unknown, seen = new WeakSet()): unknown { + if (typeof value === "string") { + return redactToolPayloadText(value); + } + if (Array.isArray(value)) { + if (seen.has(value)) { + return "[Circular]"; + } + seen.add(value); + return value.map((item) => redactStringsDeep(item, seen)); + } + if (value && typeof value === "object") { + if (seen.has(value)) { + return "[Circular]"; + } + seen.add(value); + const out: Record = {}; + for (const [key, child] of Object.entries(value as Record)) { + out[key] = redactStringsDeep(child, seen); + } + return out; + } + return value; +} + +export function sanitizeToolArgs(args: unknown): unknown { + return redactStringsDeep(args); +} + export function sanitizeToolResult(result: unknown): unknown { + if (typeof result === "string") { + return redactToolPayloadText(result); + } + if (Array.isArray(result)) { + return redactStringsDeep(result); + } if (!result || typeof result !== "object") { return result; } const record = result as Record; - const content = Array.isArray(record.content) ? record.content : null; - if (!content) { - return record; + // Strip image data first so the deep redaction pass doesn't waste work + // scanning base64 payloads (and so we capture the original byte counts). + const preCleaned: Record = { ...record }; + const originalContent = Array.isArray(record.content) ? record.content : null; + if (originalContent) { + preCleaned.content = originalContent.map((item) => { + if (!item || typeof item !== "object") { + return item; + } + const entry = item as Record; + if (readStringValue(entry.type) === "image") { + const data = readStringValue(entry.data); + const bytes = data ? data.length : undefined; + const cleaned = { ...entry }; + delete cleaned.data; + return Object.assign({}, cleaned, { bytes, omitted: true }); + } + return entry; + }); } - const sanitized = content.map((item) => { - if (!item || typeof item !== "object") { - return item; - } - const entry = item as Record; - const type = readStringValue(entry.type); - if (type === "text" && typeof entry.text === "string") { - return Object.assign({}, entry, { text: truncateToolText(entry.text) }); - } - if (type === "image") { - const data = readStringValue(entry.data); - const bytes = data ? data.length : undefined; - const cleaned = { ...entry }; - delete cleaned.data; - return Object.assign({}, cleaned, { bytes, omitted: true }); - } - return entry; - }); - return { ...record, content: sanitized }; + // Deep-redact the entire result so any top-level or nested string is + // protected, not just `details` and text content blocks. + const baseline = redactStringsDeep(preCleaned) as Record; + const out: Record = { ...baseline }; + const content = Array.isArray(baseline.content) ? baseline.content : null; + if (content) { + out.content = content.map((item) => { + if (!item || typeof item !== "object") { + return item; + } + const entry = item as Record; + if (readStringValue(entry.type) === "text" && typeof entry.text === "string") { + return Object.assign({}, entry, { text: truncateToolText(entry.text) }); + } + return entry; + }); + } + return out; } export function extractToolResultText(result: unknown): string | undefined { diff --git a/src/logging/redact.ts b/src/logging/redact.ts index 377224ffbfd..dcb935c6d8c 100644 --- a/src/logging/redact.ts +++ b/src/logging/redact.ts @@ -165,6 +165,22 @@ export function redactToolDetail(detail: string): string { return redactSensitiveText(detail, resolved); } +// Forces tools-mode regardless of `logging.redactSensitive` (which governs log +// output, not UI surfaces), and merges user `logging.redactPatterns` with the +// built-in defaults so both apply. +export function redactToolPayloadText(text: string): string { + if (!text) { + return text; + } + const cfg = readLoggingConfig(); + const userPatterns = cfg?.redactPatterns; + const patterns = + userPatterns && userPatterns.length > 0 + ? [...userPatterns, ...DEFAULT_REDACT_PATTERNS] + : undefined; + return redactSensitiveText(text, { mode: "tools", patterns }); +} + export function getDefaultRedactPatterns(): string[] { return [...DEFAULT_REDACT_PATTERNS]; }