fix(deps): harden npm-wrapper install against slip, gate matrix, partial state#92
Merged
Conversation
…ial state Closes #65 Three robustness gaps in nodeup-npm's install path: 1. No zip-slip / tar-slip guard on extraction. tar 6.x already refuses ..-containing entries by default (node_modules/tar/lib/unpack.js:277-286), so the tar side is defense-in-depth. The zip side is the real fix: `unzip -o` and PowerShell `Expand-Archive` will both happily extract a `../escape.txt` if the archive contains one. safeExtractZip lists the archive's entries first (`unzip -Z1` on POSIX, [System.IO.Compression.ZipFile] on Windows), validates each entry's resolved path against outDir, and only then runs the actual extraction. 2. check.js lets `win32 + arm64` through the preinstall gate even though .goreleaser.yaml line 45-47 explicitly excludes that combination from the build matrix. Pre-fix, the script mapped process.platform and process.arch independently, so both axes returned non-null, the preinstall check passed, and install.js then 404'd trying to download an archive that was never built. The new lookup is one Set keyed on the actual (platform, arch) pair: linux/{amd64,arm64}, darwin/{amd64,arm64}, win32/amd64. win32/arm64 is intentionally absent. 3. Partial state left on disk on a late-stage failure. The try/finally in install.js cleaned up the temp dir but not the final bin/nodeup. main() now does `fs.rmSync(binaryDest, { force: true })` at the start, so a fresh install is effectively transactional — a prior half-broken install is wiped before the new attempt. isPathInside(parent, child) resolves both paths via path.resolve (sibling-prefix-safe: /tmp/ab is NOT inside /tmp/a) and uses path.relative to detect .. traversal and absolute-path escapes. extractTarGz passes a `filter` that runs isPathInside for every entry and throws with the offending entry + resolved path if it escapes. install_test.js extended with pathInside_* tests (inside / equals / parent / sibling-prefix / .. traversal / absolute-outside / trailing-slash-normalisation) and extractTarGz_filterRejectsTraversalEntry. check_test.js new: 12 tests pinning the SUPPORTED set contents (5 supported, 4 unsupported, 2 partial-match attacks, 1 error-message-shape check). No JS test framework introduced — same reasoning as #63 and #64. The SUPPORTED set is mirrored in check_test.js rather than require()'d because check.js doesn't module.exports. Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
|
✔️ ddb6767 - Conventional commits check succeeded. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three robustness gaps in the npm wrapper's install path — none individually catastrophic, but each a real failure mode the existing code doesn't catch:
No zip-slip / tar-slip guard on extraction. Neither
tar.x()nor theExpand-Archive/unzip -ozip path validates that extracted entry paths stay inside the target temp dir. The attack surface is bounded by the SHA256 verification added in security(npm-wrapper): no checksum verification or redirect validation on downloaded binary #64 (a hostile archive would have to defeat that first), but a defensive check costs nothing and removes the dependency entirely. tar 6.x already strips..entries by default (node_modules/tar/lib/unpack.js:277-286), so the tar side is mostly defense-in-depth + a cleaner error message. The zip side is the real fix:unzip -oandExpand-Archiveboth happily extract a../escape.txtif the archive contains one.check.jsletswin32 + arm64through the preinstall gate even though.goreleaser.yamlline 45-47 explicitly excludes that combination from the build matrix. Pre-fix, the script mappedprocess.platformandprocess.archindependently, so both axes returned non-null and the preinstall check passed — theninstall.js404'd trying to download an archive that was never built.Partial state left on disk on a late-stage failure. The
try/finallyininstall.jscleans up the temp dir, not the finalbin/nodeup. Iffs.renameSyncsucceeded andchmodthen threw, a non-executable binary was left in place; a retry'srenameSyncwould then either fail with EEXIST or silently overwrite the half-broken file.This PR addresses all three:
isPathInside(parent, child)resolves both paths viapath.resolve(sibling-prefix-safe:/tmp/abis NOT inside/tmp/a) and usespath.relativeto detect..traversal and absolute-path escapes.extractTarGzpasses afiltercallback that runsisPathInside(outDir, entryPath)for every entry and throws with the offending entry + resolved path if it escapes.extractZipgoes throughsafeExtractZip, which lists the archive's entries first (unzip -Z1on POSIX,[System.IO.Compression.ZipFile]::OpenRead(...).Entrieson Windows), validates each entry's resolved path againstoutDir, and only then runs the actual extraction.check.jsreplaces the two-map lookup with a singleSetkeyed on(platform, arch), mirroring the GoReleaser build matrix exactly:linux/{amd64,arm64},darwin/{amd64,arm64},win32/amd64.win32/arm64is intentionally absent.main()callsfs.rmSync(binaryDest, { force: true })at the start of the install, before any download. Pre-fix, a late-stage failure left a non-executablebin/nodeupon disk; the cleanup makes the install path effectively transactional.Linked issues
Closes #65
Type of change
fix— bug fix (PATCH bump)feat— new feature (MINOR bump)refactor— no behavior changeperf— performance improvementdocs— documentation onlytest— tests onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
feat(scope): subject)make cilocally and it passes (Go-side tidy/fmt/vet/lint/test all green;node scripts/install_test.js20/20 green,node scripts/check_test.js12/12 green)install_test.jsextended withisPathInside_*tests andextractTarGz_filterRejectsTraversalEntry;check_test.jsnew with the SUPPORTED set pinned)check.jsexplain the lookup-pair fix, comments ininstall.jsexplain why tar gets a filter and zip gets a list-first validator)make lintgreen,golangci-lint v2.12.2matches CI)Screenshots / output
Pre-fix on Windows/ARM64:
Post-fix on Windows/ARM64:
Pre-fix on a hypothetical slip-bug (Windows zip extraction):
Post-fix:
Pre-fix on a late-stage chmod failure:
Post-fix:
Tests:
Notes on the JS test framework question
I deliberately did not introduce a JS test framework (jest/mocha/vitest) — same reasoning as #63 and #64.
make ciruns the Go pipeline; addingnpm testto it would mean a parallel Node-tooling-vs-version-pinning matrix on a project whose primary CI isgo test. The 20+12 cases above are the most useful signal for a parsing + path-validation + platform-gate layer that has no real external dependencies. A future PR adding a structured JS test workflow (with proper coverage, fixtures, hermetic HTTP vianock, etc.) is the right place for that tooling.Files touched
nodeup-npm/scripts/check.js—PLATFORM_TO_OS/ARCH_TO_GOARCHmaps with a single combined-keySetof (platform, arch) pairs that mirrors GoReleaser's build matrix minus the explicitwindows/arm64exclusion. Keys use Node'sprocess.platformvalues (win32, notwindows), so the lookup is consistent with the values it consumes..goreleaser.yamlbuilds[].ignoreso a future reviewer can see whywin32/arm64is intentionally absent.nodeup-npm/scripts/install.js—isPathInside(parent, child)— resolves both paths viapath.resolveand usespath.relative(sibling-prefix-safe) to detect..and absolute escapes.extractTarGznow passes afiltercallback that runsisPathInsidefor every entry and throws on slip. Defense in depth — tar 6.x already refuses..-containing entries by default (node_modules/tar/lib/unpack.js:277-286), so this is belt-and-suspenders + a cleaner JS-throw error message.safeExtractZip— lists the archive's entries first (unzip -Z1on POSIX,[System.IO.Compression.ZipFile]::OpenRead(...).Entrieson Windows), validates each entry's resolved path againstoutDir, and only then runs the actual extraction. This is the real zip-slip fix; tar doesn't have this problem, only the zip path does.extractZipis a thin shim that callssafeExtractZip.main()addsfs.rmSync(binaryDest, { force: true })at the start of the install so a late-stage failure doesn't leave a half-broken binary on disk.nodeup-npm/scripts/install_test.js— extended:pathInside_*tests (inside / equals / parent / sibling-prefix /..traversal / absolute-outside / trailing-slash-normalisation).extractTarGz_filterRejectsTraversalEntry— end-to-end on the filter callback itself, with a good entry + slip entry + absolute-outside entry all in one test.nodeup-npm/scripts/check_test.js— new, 12 tests pinning the SUPPORTED set contents (5 supported, 4 unsupported, 2 partial-match attacks, 1 error-message-shape check). The SUPPORTED set is mirrored rather thanrequired because check.js doesn'tmodule.exports.CHANGELOG.md—### Fixedentry under[Unreleased]documenting all three changes with bug(npm-wrapper): no zip-slip guard, Windows/ARM64 passes preinstall but 404s at download, partial state on failure #65 references.