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.
This commit is contained in:
can1357
2026-06-17 01:53:24 +02:00
parent abe4453234
commit eaba315cbd
6 changed files with 61 additions and 9 deletions
+11 -1
View File
@@ -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);
}
@@ -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.
+7 -1
View File
@@ -2008,7 +2008,13 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
return { definition, extensionPath: "<sdk>" };
}) ?? []),
];
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<string, Tool>();
@@ -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);
+19 -1
View File
@@ -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) };
}
+17 -3
View File
@@ -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;