fix(agents): don't echo raw rejected attachment name in diagnostic + test

lsr911 · lsr911 · commit e8ab8a4ca873 · 2026-07-11T16:06:17.000+08:00
Besides detecting C1 controls (0x80-0x9f) in attachment names, the rejection
diagnostic (attachments_invalid_name) interpolated the raw untrusted name,
re-introducing the same control bytes (e.g. U+009B CSI) into an error string
that can reach a terminal. Escape C0/DEL/C1 to visible \xNN and cap the name
by code point before embedding it.

Add a regression test driving the real resolveAcpSessionsSpawnImageAttachments
path: a C1-bearing name is rejected with the byte escaped (never raw) across
the full 0x80-0x9f range, and an ordinary image name still succeeds.

Signed-off-by: lsr911 &lt;liao.shirong@xydigit.com&gt;
diff --git a/src/agents/subagent-attachments.test.ts b/src/agents/subagent-attachments.test.ts
@@ -0,0 +1,76 @@
+// Regression tests for attachment-name validation, focused on C1 control
+// rejection and on not leaking raw control bytes back into the diagnostic.
+import { describe, expect, it } from "vitest";
+import type { OpenClawConfig } from "../config/types.openclaw.js";
+import { resolveAcpSessionsSpawnImageAttachments } from "./subagent-attachments.js";
+
+const CONFIG = {
+  tools: { sessions_spawn: { attachments: { enabled: true } } },
+} as unknown as OpenClawConfig;
+
+// base64 for "hello" (5 bytes), well under the default per-file limit.
+const HELLO_B64 = "aGVsbG8=";
+
+function errorOf(result: ReturnType<typeof resolveAcpSessionsSpawnImageAttachments>): string {
+  return result && result.status === "error" ? result.error : "";
+}
+
+describe("resolveAcpSessionsSpawnImageAttachments name validation", () => {
+  it("rejects a C1-bearing name and escapes it in the diagnostic", () => {
+    // U+009B is the C1 CSI introducer (alternative ANSI escape prefix, ESC [).
+    const CSI = String.fromCharCode(0x9b);
+    const result = resolveAcpSessionsSpawnImageAttachments({
+      config: CONFIG,
+      attachments: [
+        {
+          name: `bad${CSI}name.png`,
+          content: HELLO_B64,
+          encoding: "base64",
+          mimeType: "image/png",
+        },
+      ],
+    });
+
+    expect(result?.status).toBe("error");
+    const error = errorOf(result);
+    expect(error).toContain("attachments_invalid_name");
+    // The rejected name is echoed with the C1 byte escaped, not raw.
+    expect(error).toContain("\\x9b");
+    expect(error).not.toContain(CSI);
+  });
+
+  it("rejects every byte across the full C1 range 0x80-0x9f", () => {
+    for (let code = 0x80; code <= 0x9f; code += 1) {
+      const result = resolveAcpSessionsSpawnImageAttachments({
+        config: CONFIG,
+        attachments: [
+          {
+            name: `n${String.fromCharCode(code)}m.png`,
+            content: HELLO_B64,
+            encoding: "base64",
+            mimeType: "image/png",
+          },
+        ],
+      });
+      expect(result?.status).toBe("error");
+      expect(errorOf(result)).toContain("attachments_invalid_name");
+      // No raw C1 byte survives into the returned diagnostic.
+      expect(errorOf(result)).not.toContain(String.fromCharCode(code));
+    }
+  });
+
+  it("accepts an ordinary image attachment name", () => {
+    const result = resolveAcpSessionsSpawnImageAttachments({
+      config: CONFIG,
+      attachments: [
+        {
+          name: "photo.png",
+          content: HELLO_B64,
+          encoding: "base64",
+          mimeType: "image/png",
+        },
+      ],
+    });
+    expect(result?.status).toBe("ok");
+  });
+});
diff --git a/src/agents/subagent-attachments.ts b/src/agents/subagent-attachments.ts
@@ -152,23 +152,39 @@ function failAttachment(error: string): never {
   throw new Error(error);
 }
 
+// Renders a rejected, untrusted attachment name for diagnostics without
+// leaking raw control bytes: C0 (0x00-0x1f), DEL (0x7f), and C1 (0x80-0x9f)
+// are escaped to visible \xNN sequences (C1 includes the U+009B CSI
+// introducer), and the result is capped by code point so it stays single-line
+// and bounded even for adversarial names.
+function describeRejectedName(name: string): string {
+  const chars = Array.from(name, (char) => {
+    const code = char.codePointAt(0) ?? 0;
+    if (code < 0x20 || code === 0x7f || (code >= 0x80 && code <= 0x9f)) {
+      return `\\x${code.toString(16).padStart(2, "0")}`;
+    }
+    return char;
+  });
+  return chars.length > 80 ? `${chars.slice(0, 80).join("")}...` : chars.join("");
+}
+
 function validateAttachmentName(name: string): void {
   if (!name) {
     failAttachment("attachments_invalid_name (empty)");
   }
   if (name.includes("/") || name.includes("\\") || name.includes("\u0000")) {
-    failAttachment(`attachments_invalid_name (${name})`);
+    failAttachment(`attachments_invalid_name (${describeRejectedName(name)})`);
   }
   if (
     Array.from(name).some((char) => {
       const code = char.codePointAt(0) ?? 0;
       return code < 0x20 || code === 0x7f || (code >= 0x80 && code <= 0x9f);
     })
   ) {
-    failAttachment(`attachments_invalid_name (${name})`);
+    failAttachment(`attachments_invalid_name (${describeRejectedName(name)})`);
   }
   if (name === "." || name === ".." || name === ".manifest.json") {
-    failAttachment(`attachments_invalid_name (${name})`);
+    failAttachment(`attachments_invalid_name (${describeRejectedName(name)})`);
   }
 }
 
