From eaba315cbda99aa9db15afa4c03bb6009bb040d8 Mon Sep 17 00:00:00 2001 From: can1357 Date: Wed, 17 Jun 2026 01:39:27 +0200 Subject: [PATCH] security(coding-agent): sanitized artifact names and wrapped extension and MCP tools - Sanitized artifact filenames by normalizing tool names before composing spill paths. - Applied `wrapToolWithMetaNotice` to custom tool adapters and RPC-host tools in agent-session setup. - Wrapped SDK-registered extension/custom tools with the same meta-notice adapter during session creation. --- packages/coding-agent/src/mcp/render.ts | 12 ++++++++++- packages/coding-agent/src/mcp/tool-bridge.ts | 3 +++ packages/coding-agent/src/sdk.ts | 8 +++++++- .../coding-agent/src/session/agent-session.ts | 7 ++++--- .../coding-agent/src/session/artifacts.ts | 20 ++++++++++++++++++- .../coding-agent/src/tools/output-meta.ts | 20 ++++++++++++++++--- 6 files changed, 61 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/src/mcp/render.ts b/packages/coding-agent/src/mcp/render.ts index 68fcfa5dd..12344017f 100644 --- a/packages/coding-agent/src/mcp/render.ts +++ b/packages/coding-agent/src/mcp/render.ts @@ -18,6 +18,7 @@ import { JSON_TREE_SCALAR_LEN_EXPANDED, renderJsonTreeLines, } from "../tools/json-tree"; +import { formatStyledTruncationWarning, stripOutputNotice } from "../tools/output-meta"; import { formatExpandHint, truncateToWidth } from "../tools/render-utils"; import { renderStatusLine } from "../tui"; import type { MCPToolDetails } from "./tool-bridge"; @@ -78,7 +79,14 @@ export function renderMCPResult( // Output section const textContent = result.content?.find(c => c.type === "text")?.text ?? ""; - const trimmedOutput = textContent.trimEnd(); + // Strip the LLM-facing spill notice before parsing/rendering: a spilled + // result appends `[Showing… artifact://N]` to the body, which would break + // JSON detection and bury the recovery link. Surface it as a styled warning + // instead, mirroring the built-in read/bash/ssh/browser renderers. + const trimmedOutput = stripOutputNotice(textContent, result.details?.meta).trimEnd(); + const truncationWarning = result.details?.meta?.truncation + ? formatStyledTruncationWarning(result.details.meta, theme) + : null; if (!trimmedOutput) { lines.push(theme.fg("dim", "(no output)")); @@ -104,6 +112,7 @@ export function renderMCPResult( } else if (tree.truncated) { lines.push(theme.fg("dim", "…")); } + if (truncationWarning) lines.push(truncationWarning); return new Text(lines.join("\n"), 0, 0); } } catch { @@ -128,5 +137,6 @@ export function renderMCPResult( lines.push(formatExpandHint(theme, expanded, true)); } + if (truncationWarning) lines.push(truncationWarning); return new Text(lines.join("\n"), 0, 0); } diff --git a/packages/coding-agent/src/mcp/tool-bridge.ts b/packages/coding-agent/src/mcp/tool-bridge.ts index 4063d8153..6d6e75db8 100644 --- a/packages/coding-agent/src/mcp/tool-bridge.ts +++ b/packages/coding-agent/src/mcp/tool-bridge.ts @@ -15,6 +15,7 @@ import type { RenderResultOptions, } from "../extensibility/custom-tools/types"; import type { Theme } from "../modes/theme/theme"; +import type { OutputMeta } from "../tools/output-meta"; import { ToolAbortError, throwIfAborted } from "../tools/tool-errors"; import { callTool } from "./client"; import { renderMCPCall, renderMCPResult } from "./render"; @@ -71,6 +72,8 @@ export interface MCPToolDetails { provider?: string; /** Provider display name (e.g., "Claude Code", "MCP Config") */ providerName?: string; + /** Structured output metadata (set by the spill wrapper when output is truncated to an artifact). */ + meta?: OutputMeta; } /** * Format MCP content for LLM consumption. diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index e57b19d84..f322caaa4 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -2008,7 +2008,13 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} return { definition, extensionPath: "" }; }) ?? []), ]; - const wrappedExtensionTools: Tool[] = wrapRegisteredTools(allCustomTools, extensionRunner); + // `wrapToolWithMetaNotice` runs the centralized large-output → artifact spill. + // Built-in tools get it in `createTools`; extension, SDK-custom, image-gen, + // TTS, and startup (non-deferred) MCP tools all funnel through here, so apply + // it once at this adapter boundary (idempotent — a no-op if already wrapped). + const wrappedExtensionTools: Tool[] = wrapRegisteredTools(allCustomTools, extensionRunner).map( + wrapToolWithMetaNotice, + ); // All built-in tools are active (conditional tools like git/ask return null from factory if disabled) const toolRegistry = new Map(); diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index a91d50427..8d9abd94f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -250,7 +250,7 @@ import { } from "../tool-discovery/tool-index"; import { assertEditableFile } from "../tools/auto-generated-guard"; import type { CheckpointState } from "../tools/checkpoint"; -import { outputMeta } from "../tools/output-meta"; +import { outputMeta, wrapToolWithMetaNotice } from "../tools/output-meta"; import { normalizeLocalScheme, resolveToCwd } from "../tools/path-utils"; import { isAutoQaEnabled } from "../tools/report-tool-issue"; import { getLatestTodoPhasesFromEntries, type TodoItem, type TodoPhase } from "../tools/todo"; @@ -4611,7 +4611,7 @@ export class AgentSession { }); for (const customTool of mcpTools) { - const wrapped = CustomToolAdapter.wrap(customTool, getCustomToolContext) as AgentTool; + const wrapped = wrapToolWithMetaNotice(CustomToolAdapter.wrap(customTool, getCustomToolContext) as AgentTool); const finalTool = ( this.#extensionRunner ? new ExtensionToolWrapper(wrapped, this.#extensionRunner) : wrapped ) as AgentTool; @@ -4671,8 +4671,9 @@ export class AgentSession { this.#rpcHostToolNames.clear(); for (const tool of rpcTools) { + const metaWrapped = wrapToolWithMetaNotice(tool); const finalTool = ( - this.#extensionRunner ? new ExtensionToolWrapper(tool, this.#extensionRunner) : tool + this.#extensionRunner ? new ExtensionToolWrapper(metaWrapped, this.#extensionRunner) : metaWrapped ) as AgentTool; this.#toolRegistry.set(finalTool.name, finalTool); this.#rpcHostToolNames.add(finalTool.name); diff --git a/packages/coding-agent/src/session/artifacts.ts b/packages/coding-agent/src/session/artifacts.ts index bd7676d32..0cd61d9a7 100644 --- a/packages/coding-agent/src/session/artifacts.ts +++ b/packages/coding-agent/src/session/artifacts.ts @@ -7,6 +7,24 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; +/** + * Sanitize a tool name for safe use as the middle segment of the artifact + * filename (`${id}.${toolType}.log`). Built-in tool names are fixed, but MCP, + * extension, and RPC-host tool names are arbitrary and may contain path + * separators (`/`, `\`) or traversal sequences (`..`) that would otherwise let + * a spilled artifact escape the artifacts directory. Collapse everything + * outside `[A-Za-z0-9_-]` to `_`, and cap the length so an arbitrarily long + * name cannot overflow the filesystem's filename limit (ENAMETOOLONG). Fall + * back to `tool` when nothing survives. + */ +function sanitizeToolType(toolType: string): string { + const sanitized = toolType + .replace(/[^A-Za-z0-9_-]+/g, "_") + .slice(0, 64) + .replace(/^_+|_+$/g, ""); + return sanitized.length > 0 ? sanitized : "tool"; +} + /** * Manages artifact storage for a session. * @@ -83,7 +101,7 @@ export class ArtifactManager { async allocatePath(toolType: string): Promise<{ id: string; path: string }> { await this.#ensureDir(); const id = String(this.allocateId()); - const filename = `${id}.${toolType}.log`; + const filename = `${id}.${sanitizeToolType(toolType)}.log`; return { id, path: path.join(this.#dir, filename) }; } diff --git a/packages/coding-agent/src/tools/output-meta.ts b/packages/coding-agent/src/tools/output-meta.ts index 942328072..ee0c1d89b 100644 --- a/packages/coding-agent/src/tools/output-meta.ts +++ b/packages/coding-agent/src/tools/output-meta.ts @@ -12,6 +12,7 @@ import type { AgentToolUpdateCallback, } from "@oh-my-pi/pi-agent-core"; import type { ImageContent, TextContent } from "@oh-my-pi/pi-ai"; +import { logger } from "@oh-my-pi/pi-utils"; import { getDefault, type Settings } from "../config/settings"; import { formatGroupedDiagnosticMessages } from "../lsp/utils"; import type { Theme } from "../modes/theme/theme"; @@ -616,9 +617,22 @@ async function spillLargeResultToArtifact( const totalBytes = Buffer.byteLength(fullText, "utf-8"); if (totalBytes <= threshold) return result; - // Save full output as artifact - const artifactId = await sessionManager.saveArtifact(fullText, toolName); - if (!artifactId) return result; + // Save the full output as an artifact so the elided bytes stay recoverable. + // In a persistent session this hits `Bun.write`, which can throw (disk full, + // permissions). The spill wraps arbitrary tools (built-in, MCP, extension, + // RPC-host); a save failure must never convert a successful call into an + // error, nor re-expose the full (possibly context-blowing) output. Mirror + // `enforceInlineByteCap`: always truncate past the threshold, and only + // attach the `artifact://` recovery link when the save actually succeeded. + let artifactId: string | undefined; + try { + artifactId = await sessionManager.saveArtifact(fullText, toolName); + } catch (error) { + logger.warn("Failed to spill large tool result to artifact", { + tool: toolName, + error: error instanceof Error ? error.message : String(error), + }); + } // Truncate: middle elision when a head budget is configured, otherwise tail-only. const useMiddle = headBytes > 0;