fix(tools-manager): replace spawnSync extraction with safe extractArchive API#98988
Conversation
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Codex review: needs maintainer review before merge. Reviewed July 2, 2026, 9:34 PM ET / 01:34 UTC. Summary PR surface: Source +7, Tests +44. Total +51 across 2 files. Reproducibility: yes. Source inspection of current main shows the tool manager writes the release asset to disk and extracts with unbounded Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Merge the shared fs-safe extraction path after normal maintainer and CI review, keeping the download cap aligned with the extraction compressed-size limit. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of current main shows the tool manager writes the release asset to disk and extracts with unbounded Is this the best way to solve the issue? Yes. Reusing the shared fs-safe archive boundary plus adding the compressed download stream cap is the narrowest maintainable fix; wrapping raw subprocess extraction would duplicate security policy and keep more platform-specific behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4440226d52c2. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +7, Tests +44. Total +51 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
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. openclaw#98988
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
- 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. openclaw#98988
|
@clawsweeper re-review Changes made:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
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. openclaw#98988
- 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. openclaw#98988
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. openclaw#98988
800fad7 to
e572844
Compare
…hive 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.
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. openclaw#98988
- 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. openclaw#98988
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. openclaw#98988
e572844 to
8ec764c
Compare
|
Merged via squash.
|
…hive API (openclaw#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. openclaw#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. openclaw#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. openclaw#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>
…hive API (openclaw#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. openclaw#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. openclaw#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. openclaw#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>
Summary
Problem:
tools-manager.tsusesspawnSync("unzip"/"tar")for archive extraction without any size limits, entry caps, or timeout protection, making the process vulnerable to ZIP bombs / decompression bombs if a malicious or corrupted archive is provided.Solution: Replace the synchronous spawnSync-based extraction with the safe
extractArchiveAPI from@openclaw/fs-safe(re-exported fromsrc/infra/archive.ts), which enforces size limits, entry count caps, timeout, and path traversal protection.What changed: Removed 5 functions and 3 unused imports; added a single
extractArchiveSafeasync wrapper with explicit limits (maxArchiveBytes: 100MB, maxExtractedBytes: 500MB, maxEntries: 1000, timeoutMs: 60000). Updated tests to mock the new API.What did NOT change: Extraction behavior for legitimate archives, binary discovery and installation logic, error handling for unsupported archive formats.
What Problem This Solves
Problem: The tool manager's archive extraction had no bounds on input size or output volume, creating a denial-of-service surface through decompression bombs.
Why This Change Was Made: The existing
@openclaw/fs-safeinfrastructure already provides all necessary protections -- reusing it is the correct pattern rather than re-implementing limits on raw spawnSync calls.User Impact: No user-visible change. Tool binaries (fd/rg) continue to download and install as before. Users and operators gain protection against oversized or malicious archives.
Root Cause
Trigger condition: Any download of a fd or ripgrep archive from GitHub releases (or any future tool) that is corrupted or maliciously oversized.
Root cause analysis:
extractTarGzArchive()andextractZipArchive()intools-manager.tsdirectly shelled out to system tar/unzip without any pre-flight size check, extraction budget, or timeout. OpenClaw had a safe archive layer but the tool manager bypassed it.Affected scope: All platforms (Linux, macOS, Windows). Only the tool manager (src/agents/utils/tools-manager.ts) -- other extraction paths already use
@openclaw/fs-safe.Linked context
Real behavior proof
Behavior addressed: Safe archive extraction with size limits, entry caps, and timeout for tool manager downloads. The download stream byte cap now uses a Transform to reject overflow chunks before forwarding them to disk, and partial downloads are cleaned up on failure.
Real environment tested: Windows 10, Node 24, @openclaw/fs-safe@0.3.0
Exact steps or command run after this patch:
pnpm test -- src/agents/utils/tools-manager.test.ts npx tsx _evidence_collect.mjsAfter-fix evidence:
Observed result after the fix: Both fd and rg downloaded and extracted via
extractArchiveSafe(wraps@openclaw/fs-safewith 100 MB maxArchiveBytes, 500 MB maxExtractedBytes, 1000 maxEntries, 60s timeout). Both binaries verified as executable. Unit tests pass (7/7). The Transform-based stream limiter rejects overflow chunks before forwarding them, and partial downloads are cleaned up. This closes both the proof gap and the stream-cap defect identified in review.What was not tested: Extraction from a deliberately oversized/malicious archive (the stream cap and archive limits are unit-tested; extraction limits are independently verified in @openclaw/fs-safe tests).
Tests and validation
Unit tests
Integration / E2E
The @openclaw/fs-safe package's extractArchive is independently tested in src/infra/archive.test.ts for: archive size limits, entry count limits, symlink/hardlink traversal protection, absolute entry path rejection, staged extraction with safe merge.
Risk checklist
🔧 Post-review fix (2026-07-02)
Problem: The PR HEAD (9077b05) replaced
src/agents/utils/tools-manager.tswith a one-line placeholder, removingensureTool/getToolPathexports.Fix: Restored the full implementation via the correct Transform-stream-limit commit. The branch now contains:
extractArchiveAPI from@openclaw/fs-safewith size limitsVerification: ✅ 7/7 unit tests passed | ✅ Type check clean | ✅ Both exports (ensureTool, getToolPath) confirmed