fix(tools-manager): replace spawnSync extraction with safe extractArchive API (#98988)

LeonidasLux · vincentkoc · web-flow · commit be48a430bfa6 · 2026-07-06T00:43:03.000-07:00
* fix(tools-manager): replace spawnSync extraction with safe extractArchive API Replace the synchronous spawnSync-based archive extraction (unzip/tar) with the safe extractArchive from @openclaw/fs-safe, which enforces: - maxArchiveBytes: 100 MB (prevents oversized compressed input) - maxExtractedBytes: 500 MB (prevents decompression bomb OOM) - maxEntries: 1000 (prevents zip bomb from exhausting inodes) - timeoutMs: 60,000 (prevents hung extraction) - Path traversal and symlink/hardlink protection (built-in) Removes 5 functions (formatSpawnFailure, runExtractionCommand, extractTarGzArchive, getWindowsTarCommand, extractZipArchive) and their associated imports, replacing them with a single async wrapper around the existing @openclaw/fs-safe infrastructure (re-exported from src/infra/archive.ts). Net: -83 lines, + security boundaries across all platforms. * fix(tools-manager): add download-stream byte cap and regression tests Add a maxBytes parameter to downloadFile that checks Content-Length before reading the body and enforces a streaming byte cap during transfer, so oversized archives are rejected before hitting disk. Extract MAX_ARCHIVE_BYTES as a module-level constant shared between downloadFile and extractArchiveSafe, ensuring both gates use the same 100 MB limit. Add two regression tests: - rejects downloads with Content-Length exceeding the archive byte cap - accepts downloads with Content-Length under the archive byte cap Ref. #98988 * fix(tools-manager): replace PassThrough with Transform stream limiter - Replace PassThrough data listener with Transform that rejects overflow chunks *before* they are forwarded to the file pipeline, preventing the offending chunk from landing on disk. - Add completion guard and cleanup of partial downloads on failure so downloadTool does not leave a partial archive on disk. - Add real behavior proof: test output and live fd/rg download and extraction through the safe extractArchive code path. 🦞 diamond lobster: L2 evidence (real function calls + real objects) Ref. #98988 * fix(tools-manager): replace PassThrough with Transform stream limiter Uses a Transform to reject overflow chunks *before* they are forwarded to the file pipeline (a PassThrough data listener acts *after* emission and cannot prevent the offending chunk from landing on disk). Wraps the pipeline in a completion-guarded block so that partial downloads are removed when the transfer fails mid-way. Ref. #98988 * fix(agents): harden helper archive downloads * fix(agents): preserve archive extraction cause --------- Co-authored-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com> Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai
 
 ### Fixes
 
+- **Agent helper downloads:** bound fd and ripgrep archive downloads and extraction with declared and streamed byte caps, extraction limits, timeouts, traversal-safe unpacking, and partial-file cleanup. (#98988) Thanks @LeonidasLux.
 - **OpenAI Realtime Codex auth:** reuse external Codex OAuth profiles for Realtime voice sessions when no explicit OpenAI API key is configured.
 - **OpenAI-compatible TTS voice notes:** route configured MP3 speech output through native voice-message delivery when the channel supports it, while keeping WAV output on the audio-file path. (#83227, #80317) Thanks @HemantSudarshan.
 - **Talk transcription providers:** cold-load explicitly configured Voice Call streaming providers, including runtime aliases, when another provider registry is already active, keeping catalog and session selection aligned. (#97170, #97738) Thanks @solavrc.
diff --git a/src/agents/utils/tools-manager.test.ts b/src/agents/utils/tools-manager.test.ts
@@ -1,11 +1,12 @@
-import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { deleteTestEnvValue, setTestEnvValue } from "../../test-utils/env.js";
 
 const fetchWithSsrFGuardMock = vi.hoisted(() => vi.fn());
 const spawnSyncMock = vi.hoisted(() => vi.fn());
+const extractArchiveMock = vi.hoisted(() => vi.fn());
 
 vi.mock("../../infra/net/fetch-guard.js", () => ({
   fetchWithSsrFGuard: fetchWithSsrFGuardMock,
@@ -16,6 +17,10 @@ vi.mock("node:child_process", async (importOriginal) => ({
   spawnSync: spawnSyncMock,
 }));
 
+vi.mock("../../infra/archive.js", () => ({
+  extractArchive: extractArchiveMock,
+}));
+
 let originalAgentDir: string | undefined;
 let tempAgentDir: string | undefined;
 
@@ -24,6 +29,7 @@ beforeEach(() => {
   tempAgentDir = mkdtempSync(join(tmpdir(), "openclaw-tools-manager-"));
   setTestEnvValue("OPENCLAW_AGENT_DIR", tempAgentDir);
   fetchWithSsrFGuardMock.mockReset();
+  extractArchiveMock.mockReset();
   spawnSyncMock.mockReturnValue({
     error: new Error("ENOENT"),
     status: null,
@@ -89,7 +95,7 @@ describe("ensureTool", () => {
     expect(downloadRelease).toHaveBeenCalledOnce();
   });
 
-  it("extracts Windows zip downloads with trusted System32 tools", async () => {
+  it("extracts Windows zip downloads via safe archive API with size limits", async () => {
     vi.doMock("node:os", async (importOriginal) => ({
       ...(await importOriginal<typeof import("node:os")>()),
       arch: () => "x64",
@@ -99,6 +105,9 @@ describe("ensureTool", () => {
     const { ensureTool } = await import("./tools-manager.js");
     const releaseCheckRelease = vi.fn(async () => {});
     const downloadRelease = vi.fn(async () => {});
+    extractArchiveMock.mockImplementation(async (params: { destDir: string }) => {
+      writeFileSync(join(params.destDir, "rg.exe"), "binary");
+    });
     fetchWithSsrFGuardMock
       .mockResolvedValueOnce({
         response: new Response(JSON.stringify({ tag_name: "14.1.1" }), { status: 200 }),
@@ -110,50 +119,94 @@ describe("ensureTool", () => {
         release: downloadRelease,
         finalUrl: "https://github.com/BurntSushi/ripgrep/releases/download/14.1.1/archive.zip",
       });
-    spawnSyncMock.mockImplementation((command: string, args: string[]) => {
-      if (command === "C:\\Windows\\System32\\tar.exe") {
-        return {
-          error: undefined,
-          status: 1,
-          stderr: Buffer.from("tar failed"),
-          stdout: Buffer.alloc(0),
-        };
-      }
-      if (command === "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe") {
-        const extractDir = args.at(-1);
-        if (!extractDir) {
-          throw new Error("expected extraction destination");
-        }
-        writeFileSync(join(extractDir, "rg.exe"), "binary");
-        return {
-          error: undefined,
-          status: 0,
-          stderr: Buffer.alloc(0),
-          stdout: Buffer.alloc(0),
-        };
-      }
-      return {
-        error: new Error(`unexpected command: ${command}`),
-        status: null,
-        stderr: Buffer.alloc(0),
-        stdout: Buffer.alloc(0),
-      };
-    });
 
     await expect(ensureTool("rg", true)).resolves.toBe(join(tempAgentDir!, "bin", "rg.exe"));
 
-    expect(spawnSyncMock).toHaveBeenNthCalledWith(
-      2,
-      "C:\\Windows\\System32\\tar.exe",
-      expect.any(Array),
-      { stdio: "pipe" },
+    expect(extractArchiveMock).toHaveBeenCalledOnce();
+    expect(extractArchiveMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        archivePath: expect.stringContaining(".zip"),
+        destDir: expect.stringContaining("extract_tmp_rg_"),
+        timeoutMs: 60_000,
+        limits: {
+          maxArchiveBytes: 100 * 1024 * 1024,
+          maxExtractedBytes: 500 * 1024 * 1024,
+          maxEntries: 1_000,
+        },
+      }),
     );
-    expect(spawnSyncMock).toHaveBeenNthCalledWith(
-      3,
-      "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
-      expect.any(Array),
-      { stdio: "pipe" },
+  });
+
+  it("rejects downloads whose declared size exceeds the byte cap", async () => {
+    const response = new Response("oversized-body", {
+      status: 200,
+      headers: { "content-length": "11" },
+    });
+    const cancel = vi.spyOn(response.body!, "cancel").mockResolvedValue(undefined);
+    const release = vi.fn(async () => {});
+    fetchWithSsrFGuardMock.mockResolvedValueOnce({
+      response,
+      release,
+      finalUrl: "https://example.com/archive.tar.gz",
+    });
+    const destination = join(tempAgentDir!, "archive.tar.gz");
+    const { testing } = await import("./tools-manager.js");
+
+    await expect(
+      testing.downloadFile("https://example.com/archive.tar.gz", destination, 10),
+    ).rejects.toThrow("Download exceeds the 10-byte archive limit");
+
+    expect(cancel).toHaveBeenCalledOnce();
+    expect(release).toHaveBeenCalledOnce();
+    expect(existsSync(destination)).toBe(false);
+  });
+
+  it("rejects streamed bytes above the cap and removes the partial file", async () => {
+    const response = new Response(
+      new ReadableStream<Uint8Array>({
+        start(controller) {
+          controller.enqueue(new Uint8Array([1, 2, 3, 4, 5, 6]));
+          controller.enqueue(new Uint8Array([7, 8, 9, 10, 11, 12]));
+          controller.close();
+        },
+      }),
+      { status: 200, headers: { "content-length": "6" } },
     );
+    const release = vi.fn(async () => {});
+    fetchWithSsrFGuardMock.mockResolvedValueOnce({
+      response,
+      release,
+      finalUrl: "https://example.com/archive.tar.gz",
+    });
+    const destination = join(tempAgentDir!, "archive.tar.gz");
+    const { testing } = await import("./tools-manager.js");
+
+    await expect(
+      testing.downloadFile("https://example.com/archive.tar.gz", destination, 10),
+    ).rejects.toThrow("Download exceeded the 10-byte archive limit");
+
+    expect(release).toHaveBeenCalledOnce();
+    expect(existsSync(destination)).toBe(false);
+  });
+
+  it("accepts downloads exactly at the byte cap", async () => {
+    const body = new Uint8Array([1, 2, 3, 4]);
+    const release = vi.fn(async () => {});
+    fetchWithSsrFGuardMock.mockResolvedValueOnce({
+      response: new Response(body, {
+        status: 200,
+        headers: { "content-length": String(body.byteLength) },
+      }),
+      release,
+      finalUrl: "https://example.com/archive.tar.gz",
+    });
+    const destination = join(tempAgentDir!, "archive.tar.gz");
+    const { testing } = await import("./tools-manager.js");
+
+    await testing.downloadFile("https://example.com/archive.tar.gz", destination, body.byteLength);
+
+    expect(release).toHaveBeenCalledOnce();
+    expect(readFileSync(destination)).toEqual(Buffer.from(body));
   });
 });
 
diff --git a/src/agents/utils/tools-manager.ts b/src/agents/utils/tools-manager.ts
