← Back to PRs

#9529: security(archive): validate entries against path traversal (Zip Slip)

by leszekszpunar open 2026-02-05 09:47 View on GitHub →
commands agents stale
## Summary Closes #9512 Both `extractArchive()` in `src/agents/skills-install.ts` and the extraction block in `src/commands/signal-install.ts` use system `tar`/`unzip` commands without validating archive entry paths. A malicious archive with entries like `../../etc/crontab` could write files outside the intended target directory (CWE-22, "Zip Slip"). **Changes:** - Add `entryEscapesTarget()` helper that checks if a resolved entry path escapes the target directory using `path.resolve()` + `startsWith()` (same pattern as the secure `src/infra/archive.ts`) - Add `validateArchiveEntries()` that lists archive contents (`tar tf` / `unzip -Z1`) and validates all entries before extraction - Apply pre-extraction validation to both `skills-install.ts` and `signal-install.ts` - Add unit tests for `entryEscapesTarget()` covering normal paths, `../` traversal, absolute paths, and platform-specific backslash handling **Security note:** This follows the same defense pattern already used in `src/infra/archive.ts:72-96` (JSZip-based extraction), extending it to the system-command-based extraction paths. ## Test plan - [x] 7 unit tests pass for `entryEscapesTarget()` (normal paths, traversal, absolute, backslash, directory entries) - [x] Lint passes (oxlint) - [x] Existing functionality preserved (validation returns early-ok when binary not found) <!-- greptile_comment --> <h2>Greptile Overview</h2> <h3>Greptile Summary</h3> This PR hardens two system-command-based archive extraction paths against Zip Slip (CWE-22) by listing archive contents (`tar tf` / `unzip -Z1`) and validating that each entry stays within the intended destination directory before extracting. Changes are primarily in `src/agents/skills-install.ts` (new `entryEscapesTarget()` + `validateArchiveEntries()` called from `extractArchive()`) and `src/commands/signal-install.ts` (inline pre-extraction validation). Unit tests were added for `entryEscapesTarget()`. Key issues to address before merge: - The new `entryEscapesTarget()` validation does not normalize backslashes, which can allow traversal-style entries to bypass the check on POSIX while still being interpreted as separators by some extractors. - `signal-install.ts` performs the listing step without checking for `tar`/`unzip` availability, which can cause an unhandled failure on systems missing those tools. <h3>Confidence Score: 3/5</h3> - This PR is close to safe to merge but has a couple of correctness/security gaps in the new validation paths. - The intent and overall approach are sound, but the current path-escape check can be bypassed via backslash-separated paths depending on extractor behavior, and one call site runs archive listing without guarding for missing system tools, which can introduce new runtime failures. - src/agents/skills-install.ts, src/commands/signal-install.ts <!-- greptile_other_comments_section --> <!-- /greptile_comment --> lobster-biscuit

Most Similar PRs