Description

createCodexDynamicToolBridge attempts to base media trust decisions on the pre-middleware tool result by passing mediaTrustResult: rawResult into filterToolResultMediaUrls. However, the middleware runner passes the same object reference (event.result) into middleware handlers without cloning/freezing. A middleware can mutate event.result.details in-place (e.g., remove mcpServer/mcpTool) and thereby also mutate rawResult.

Impact:

• isToolResultMediaTrusted() treats results with details.mcpServer/details.mcpTool as external/untrusted.
• If a middleware removes/rewrites these provenance fields in-place, the subsequent trust check may incorrectly treat an external tool result as trusted and allow non-HTTP (local) media paths to pass filtering.
• This undermines the intended provenance anchoring and can lead to local file path exposure/unsafe media handling depending on downstream consumers of toolMediaUrls.

Vulnerable flow:

• Input: tool output object rawResult
• Mutation opportunity: middleware handler receives {...event, result: current} where current is the same object
• Sink: filterToolResultMediaUrls(toolName, mediaUrls, mediaTrustResult) where mediaTrustResult is expected to be immutable/raw

Recommendation

Treat tool results as immutable across middleware boundaries.

Options:

1. Deep-clone the tool result before passing it into middleware, and separately keep an immutable snapshot for provenance decisions.
2. Deep-freeze (or structuredClone + freeze) the provenance object used for trust checks so middleware cannot mutate it.

Example fix (clone before middleware + keep snapshot):

const rawResult = await tool.execute(...);
const rawSnapshot = structuredClone(rawResult);

const middlewareResult = await middlewareRunner.applyToolResultMiddleware({
  ...,
  ​// pass a clone so handlers can't mutate the snapshot/raw
  result: structuredClone(rawResult),
});

collectToolTelemetry({
  ...,
  result: middlewareResult,
  mediaTrustResult: rawSnapshot,
});

Also consider updating the middleware runner to defensively clone event.result per handler invocation (or freeze it) to prevent accidental in-place mutation.

Description

createAgentToolResultMiddlewareRunner validates the shape of result.content blocks but allows extremely large aggregate payloads and does not validate or cap result.details at all.

Impact (attacker-controlled plugin / compromised middleware):

• A middleware can return up to 200 content blocks.
• Each text block can be 100,000 chars; each image.data can be 5,000,000 chars.
• There is no cumulative size cap, so a single tool result can approach ~1,000,000,000 chars of image data alone (200 × 5,000,000), which can cause process OOM, excessive GC, serialization overhead, and downstream amplification.
• details is accepted with any size/shape, enabling additional memory blowups and expensive serialization (and it is read by downstream code, e.g. error detection).

Downstream amplification example:

• In extensions/codex/src/app-server/dynamic-tools.ts, image blocks are converted into data:${mimeType};base64,${data} strings, further increasing memory usage per block and potentially sending massive payloads over IPC/HTTP.

Vulnerable code:

const MAX_MIDDLEWARE_CONTENT_BLOCKS = 200;
const MAX_MIDDLEWARE_TEXT_CHARS = 100_000;
const MAX_MIDDLEWARE_IMAGE_DATA_CHARS = 5_000_000;
...
return value.content.every(isValidMiddlewareContentBlock);
​// NOTE: no validation / size cap for `value.details`

Recommendation

Add defense-in-depth validation for the entire tool result, including details, and enforce aggregate size limits.

Suggested approach:

1. Cap total characters across all text blocks and all image.data fields (and ideally cap total bytes after base64 decode for images).
2. Validate details to be JSON-serializable and bounded (e.g., depth/keys/total string length), or drop it entirely from middleware outputs.
3. Optionally restrict mimeType to a safe allowlist (e.g., image/png, image/jpeg, image/webp) and verify image.data is base64.

Example (sketch):

const MAX_TOTAL_TEXT_CHARS = 200_000;
const MAX_TOTAL_IMAGE_DATA_CHARS = 5_000_000; // total, not per-block
const MAX_DETAILS_JSON_CHARS = 50_000;

function safeJsonSize(value: unknown, maxChars: number): boolean {
  try {
    return JSON.stringify(value).length <= maxChars;
  } catch {
    return false;
  }
}

function isValidMiddlewareToolResult(value: unknown): value is OpenClawAgentToolResult {
  if (!isRecord(value) || !Array.isArray(value.content)) return false;
  if (value.content.length > MAX_MIDDLEWARE_CONTENT_BLOCKS) return false;

  let totalText = 0;
  let totalImage = 0;
  for (const block of value.content) {
    if (!isValidMiddlewareContentBlock(block)) return false;
    if (block.type === "text") totalText += block.text.length;
    if (block.type === "image") totalImage += block.data.length;
    if (totalText > MAX_TOTAL_TEXT_CHARS || totalImage > MAX_TOTAL_IMAGE_DATA_CHARS) return false;
  }

  ​// either drop `details` or strictly bound it
  if ("details" in value && !safeJsonSize((value as any).details, MAX_DETAILS_JSON_CHARS)) return false;

  return true;
}

1. extensions/codex/src/app-server/dynamic-tools.ts, line 327-349 (link)

   Allowlist-based isToolResultError may over-classify valid statuses as errors

   The status allowlist ("ok", "success", "completed", "running", "0", "") treats any unknown string — e.g. "partial", "pending", "in_progress", "cancelled", "warning" — as an error. When a tool produces one of these non-standard but non-failure statuses, rawIsError will be true, which: (1) signals isError: true to bundled middleware like tokenjuice (possibly skipping compression), and (2) suppresses media-URL telemetry collection for successful tool calls. Given that this value was previously hardcoded to false, it may be worth expanding the allowlist or switching to an explicit error-detection blocklist ("failed", "error", "timeout", etc.) to reduce the false-positive surface.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: extensions/codex/src/app-server/dynamic-tools.ts
   Line: 327-349
   
   Comment:
   **Allowlist-based `isToolResultError` may over-classify valid statuses as errors**
   
   The status allowlist (`"ok"`, `"success"`, `"completed"`, `"running"`, `"0"`, `""`) treats any unknown string — e.g. `"partial"`, `"pending"`, `"in_progress"`, `"cancelled"`, `"warning"` — as an error. When a tool produces one of these non-standard but non-failure statuses, `rawIsError` will be `true`, which: (1) signals `isError: true` to bundled middleware like tokenjuice (possibly skipping compression), and (2) suppresses media-URL telemetry collection for successful tool calls. Given that this value was previously hardcoded to `false`, it may be worth expanding the allowlist or switching to an explicit error-detection blocklist (`"failed"`, `"error"`, `"timeout"`, etc.) to reduce the false-positive surface.
   
   How can I resolve this? If you propose a fix, please make it concise.