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

* 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. https://github.com/openclaw/openclaw/pull/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. https://github.com/openclaw/openclaw/pull/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. https://github.com/openclaw/openclaw/pull/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>
2026-08-28 05:16:23 -06:00 · 2026-07-06 15:43:03 +08:00
parent 23f492fcaf
commit be48a430bf
3 changed files with 170 additions and 137 deletions
@@ -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.
@@ -1,4 +1,4 @@
-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";
@@ -6,6 +6,7 @@ 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));
  });
 });

@@ -3,7 +3,7 @@
 *
 * Locates or downloads pinned helper binaries such as fd and ripgrep.
 */
-import { type SpawnSyncReturns, spawnSync } from "node:child_process";
+import { spawnSync } from "node:child_process";
 import { randomUUID } from "node:crypto";
 import {
  chmodSync,
@@ -16,20 +16,22 @@ import {
 } from "node:fs";
 import { arch, platform } from "node:os";
 import { join } from "node:path";
-import { Readable } from "node:stream";
+import { Readable, Transform } from "node:stream";
 import { pipeline } from "node:stream/promises";
 import type { ReadableStream as NodeReadableStream } from "node:stream/web";
 import chalk from "chalk";
+import { extractArchive } from "../../infra/archive.js";
 import { fetchWithSsrFGuard } from "../../infra/net/fetch-guard.js";
-import {
-  getWindowsPowerShellExePath,
-  getWindowsSystem32ExePath,
-} from "../../infra/windows-install-roots.js";
 import { APP_NAME, getBinDir } from "../config.js";

 const TOOLS_DIR = getBinDir();
 const NETWORK_TIMEOUT_MS = 10_000;
 const DOWNLOAD_TIMEOUT_MS = 120_000;
+const MAX_ARCHIVE_BYTES = 100 * 1024 * 1024;
+const MAX_EXTRACTED_BYTES = 500 * 1024 * 1024;
+const MAX_ARCHIVE_ENTRIES = 1_000;
+const ARCHIVE_EXTRACT_TIMEOUT_MS = 60_000;
+const CONTENT_LENGTH_RE = /^\d+$/;

 async function cancelUnreadResponseBody(response: Response): Promise<void> {
  if (!response.bodyUsed) {
@@ -161,8 +163,7 @@ async function getLatestVersion(repo: string): Promise<string> {
  }
 }

-// Download a file from URL
-async function downloadFile(url: string, dest: string): Promise<void> {
+async function downloadFile(url: string, dest: string, maxBytes: number): Promise<void> {
  const guarded = await fetchWithSsrFGuard({
    url,
    timeoutMs: DOWNLOAD_TIMEOUT_MS,
@@ -180,8 +181,44 @@ async function downloadFile(url: string, dest: string): Promise<void> {
      throw new Error("No response body");
    }

+    const rawContentLength = response.headers.get("content-length");
+    if (rawContentLength !== null) {
+      const contentLength = rawContentLength.trim();
+      if (CONTENT_LENGTH_RE.test(contentLength)) {
+        const declaredBytes = Number(contentLength);
+        if (!Number.isSafeInteger(declaredBytes) || declaredBytes > maxBytes) {
+          await cancelUnreadResponseBody(response);
+          throw new Error(`Download exceeds the ${maxBytes}-byte archive limit`);
+        }
+      }
+    }
+
    const fileStream = createWriteStream(dest);
-    await pipeline(Readable.fromWeb(response.body as NodeReadableStream<Uint8Array>), fileStream);
+
+    let downloadCompleted = false;
+    try {
+      let downloadedBytes = 0;
+      const byteCap = new Transform({
+        transform(chunk: Uint8Array, _encoding, callback) {
+          downloadedBytes += chunk.byteLength;
+          if (downloadedBytes > maxBytes) {
+            callback(new Error(`Download exceeded the ${maxBytes}-byte archive limit`));
+            return;
+          }
+          callback(null, chunk);
+        },
+      });
+      await pipeline(
+        Readable.fromWeb(response.body as NodeReadableStream<Uint8Array>),
+        byteCap,
+        fileStream,
+      );
+      downloadCompleted = true;
+    } finally {
+      if (!downloadCompleted) {
+        rmSync(dest, { force: true });
+      }
+    }
  } finally {
    await guarded.release();
  }
@@ -211,89 +248,28 @@ function findBinaryRecursively(rootDir: string, binaryFileName: string): string
  return null;
 }

-function formatSpawnFailure(result: SpawnSyncReturns<Buffer>): string {
-  if (result.error?.message) {
-    return result.error.message;
-  }
-  const stderr = result.stderr?.toString().trim();
-  if (stderr) {
-    return stderr;
-  }
-  const stdout = result.stdout?.toString().trim();
-  if (stdout) {
-    return stdout;
-  }
-  return `exit status ${result.status ?? "unknown"}`;
-}
-
-function runExtractionCommand(command: string, args: string[]): string | null {
-  const result = spawnSync(command, args, { stdio: "pipe" });
-  if (!result.error && result.status === 0) {
-    return null;
-  }
-  return `${command}: ${formatSpawnFailure(result)}`;
-}
-
-function extractTarGzArchive(archivePath: string, extractDir: string, assetName: string): void {
-  const failure = runExtractionCommand("tar", ["xzf", archivePath, "-C", extractDir]);
-  if (failure) {
-    throw new Error(`Failed to extract ${assetName}: ${failure}`);
-  }
-}
-
-function getWindowsTarCommand(): string {
-  return getWindowsSystem32ExePath("tar.exe");
-}
-
-function extractZipArchive(archivePath: string, extractDir: string, assetName: string): void {
-  const failures: string[] = [];
-
-  if (platform() === "win32") {
-    // Windows ships bsdtar as tar.exe, which supports zip files. Prefer the
-    // System32 binary over Git Bash's GNU tar, which does not handle zip archives.
-    const tarFailure = runExtractionCommand(getWindowsTarCommand(), [
-      "xf",
+async function extractArchiveSafe(
+  archivePath: string,
+  extractDir: string,
+  assetName: string,
+): Promise<void> {
+  try {
+    await extractArchive({
      archivePath,
-      "-C",
-      extractDir,
-    ]);
-    if (!tarFailure) {
-      return;
-    }
-    failures.push(tarFailure);
-
-    const script =
-      "& { param($archive, $destination) $ErrorActionPreference = 'Stop'; Expand-Archive -LiteralPath $archive -DestinationPath $destination -Force }";
-    const powershellFailure = runExtractionCommand(getWindowsPowerShellExePath(), [
-      "-NoLogo",
-      "-NoProfile",
-      "-NonInteractive",
-      "-ExecutionPolicy",
-      "Bypass",
-      "-Command",
-      script,
-      archivePath,
-      extractDir,
-    ]);
-    if (!powershellFailure) {
-      return;
-    }
-    failures.push(powershellFailure);
-  } else {
-    const unzipFailure = runExtractionCommand("unzip", ["-q", archivePath, "-d", extractDir]);
-    if (!unzipFailure) {
-      return;
-    }
-    failures.push(unzipFailure);
-
-    const tarFailure = runExtractionCommand("tar", ["xf", archivePath, "-C", extractDir]);
-    if (!tarFailure) {
-      return;
-    }
-    failures.push(tarFailure);
+      destDir: extractDir,
+      timeoutMs: ARCHIVE_EXTRACT_TIMEOUT_MS,
+      limits: {
+        maxArchiveBytes: MAX_ARCHIVE_BYTES,
+        maxExtractedBytes: MAX_EXTRACTED_BYTES,
+        maxEntries: MAX_ARCHIVE_ENTRIES,
+      },
+    });
+  } catch (err) {
+    throw new Error(
+      `Failed to extract ${assetName}: ${err instanceof Error ? err.message : String(err)}`,
+      { cause: err },
+    );
  }
-
-  throw new Error(`Failed to extract ${assetName}: ${failures.join("; ")}`);
 }

 // Download and install a tool
@@ -326,8 +302,9 @@ async function downloadTool(tool: "fd" | "rg"): Promise<string> {
  const binaryExt = plat === "win32" ? ".exe" : "";
  const binaryPath = join(TOOLS_DIR, config.binaryName + binaryExt);

-  // Download
-  await downloadFile(downloadUrl, archivePath);
+  // Download with byte cap so oversized archives are rejected before
+  // hitting disk, not just during extraction.
+  await downloadFile(downloadUrl, archivePath, MAX_ARCHIVE_BYTES);

  // Extract into a unique temp directory. fd and rg downloads can run concurrently
  // during startup, so sharing a fixed directory causes races.
@@ -338,10 +315,8 @@ async function downloadTool(tool: "fd" | "rg"): Promise<string> {
  mkdirSync(extractDir, { recursive: true });

  try {
-    if (assetName.endsWith(".tar.gz")) {
-      extractTarGzArchive(archivePath, extractDir, assetName);
-    } else if (assetName.endsWith(".zip")) {
-      extractZipArchive(archivePath, extractDir, assetName);
+    if (assetName.endsWith(".tar.gz") || assetName.endsWith(".zip")) {
+      await extractArchiveSafe(archivePath, extractDir, assetName);
    } else {
      throw new Error(`Unsupported archive format: ${assetName}`);
    }
@@ -441,3 +416,7 @@ export async function ensureTool(tool: "fd" | "rg", silent = false): Promise<str
    return undefined;
  }
 }
+
+export const testing = {
+  downloadFile,
+};