fix(deps): verify npm-wrapper binary download against release checksums#91
Merged
Conversation
Closes #64 Pre-fix, nodeup-npm/scripts/install.js followed the GitHub-releases redirect from github.com to objects.githubusercontent.com with zero validation, then trusted whatever bytes came back: any host in the Location header would be GET'd, and its bytes chmod 755'd into ./bin/nodeup. A TLS-stripping proxy, a compromised CDN bucket, a DNS-poisoned resolver, or a corrupt mirror could swap in an attacker-controlled binary and the user would have no way to know. This commit closes the integrity gap before extraction: - followHops(startUrl, hopsLeft, acceptStatus) walks the redirect chain while validating every Location header against ALLOWED_REDIRECT_HOSTS (Set(['objects.githubusercontent.com'])). Any off-CDN target aborts the install with the offending hostname named. MAX_REDIRECT_HOPS = 2 bounds the walker at the known GitHub chain length. - fetchExpectedHash(repo, tag, archiveName) downloads the release's checksums.txt first (fail fast — no point pulling an archive we can't verify) and parses out the expected SHA256 for our exact archive basename via parseChecksumsTxt. The parser accepts both GoReleaser's two-space shape and sha256sum -b / --tag's binary- mode * shape, with or without space between * and the filename. - downloadTo(url, destPath) streams the archive to disk while piping it through crypto.createHash('sha256') — the hash is computed as bytes arrive, no second pass over the file. On mismatch, the half-saved archive is unlinkSync'd before die() runs, so a forensic investigation of the failing install doesn't leave the (potentially attacker-controlled) bytes sitting in tmp. - nodeup-npm/scripts/install_test.js: 12 pure-helper smoke tests covering parseChecksumsTxt (two-space, binary-mode-*, malformed- line skipping, basename keying), followHops (allowlisted redirect, off-CDN redirect rejection, hop-count ceiling, missing- Location-header rejection, unexpected-status rejection), and end-to-end integrity (missing-archive-in-checksums error names the offender + available set, hash mismatch is detected, streaming hash equals direct hash). No JS test framework — same reasoning as the tar runtime-dep fix in #63. CHANGELOG: ### Fixed entry under [Unreleased] documents the redirect allowlist, the SHA256 stream-while-downloading pattern, and the unlink-on-mismatch cleanup. Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
|
✔️ b02bde7 - 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
nodeup-npm/scripts/install.jsdidrequire('https').get(url)and followed the redirect fromgithub.comtoobjects.githubusercontent.comwith zero validation — any host in theLocationheader would be GET'd and its bytes written straight into./bin/nodeup. Worse, even if the redirect target was honest, the script trusted the body bytes it received: a TLS-stripping proxy, a compromised CDN bucket, a DNS-poisoned resolver, or a corrupt mirror could swap in an attacker-controlled binary and the user would never know — the file ischmod 755'd and added to$PATHby npm's bin linking.This PR closes the entire integrity gap before extraction:
Redirect host allowlist.
followHops(startUrl, hopsLeft, acceptStatus)is a recursive walker that validates everyLocationheader againstALLOWED_REDIRECT_HOSTS = new Set(['objects.githubusercontent.com']). Any redirect to a non-allowlisted host aborts the install with the offending hostname named.MAX_REDIRECT_HOPS = 2(the known GitHub chain length — one hop fromgithub.comto the S3-backed CDN) bounds the walker; longer chains throw before any byte is written to disk. Same-host redirects are always permitted (defensive — GitHub never sends a same-host 30x in practice, but a future change toLocation: /fooshouldn't break us).SHA256 verification against the release's
checksums.txt. GoReleaser publishes achecksums.txtalongside every artifact (verified at.goreleaser.yamlline 62-64).fetchExpectedHash(repo, tag, archiveName)downloads it first and parses out the expected hash for our exact archive basename — fail fast if the release didn't publish it or our artifact isn't in it, so we don't waste bytes pulling a multi-MB archive we can't verify.downloadTo(url, destPath)then streams the archive to disk while piping it throughcrypto.createHash('sha256')— the hash is computed as bytes arrive, no second pass over the file. On mismatch, the half-saved archive isunlinkSync'd beforedie()runs — a forensic investigation of the failing install doesn't leave the (potentially attacker-controlled) bytes sitting in tmp.parseChecksumsTxtaccepts both GoReleaser (<hash> <filename>, two-space) andsha256sum -b/--tag(<hash> *<filename>, binary-mode*) shapes. Comment lines and blank lines are skipped; malformed lines are silently dropped. The trailing-token keying means the parser is robust to any directory prefix a future GoReleaser config might add.nodeup-npm/scripts/install_test.js(new, 12 tests, no JS test framework) exercises every pure helper. Each test runs through a tinyprocess.stdout.write+assert.strictEqualshim — failure exits 1, success prints0 issues. (N tests)(matching golangci-lint's idiom). Coverage:parseChecksumsTxt: two-space separator, binary-mode*, malformed-line skipping, basename keying.followHops: allowlisted redirect (the real GitHub chain), off-CDN redirect rejection, hop-count ceiling, missing-Location-header rejection, unexpected-status rejection.Linked issues
Closes #64
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; the JS-side smoke tests are out ofmake ci's scope by design —make ciruns the Go pipeline that the.github/workflows/ci.ymljob runs, and addingnodetest steps to that pipeline is a separate decision; the install.js tests run withnode nodeup-npm/scripts/install_test.js)nodeup-npm/scripts/install_test.js, 12 tests covering parseChecksumsTxt / followHops / integrity paths)make lintgreen,golangci-lint v2.12.2matches CI)Screenshots / output
Pre-fix (current main), a redirect to
evil.example.comis followed silently:Post-fix, the same redirect throws with the offending hostname named:
Pre-fix, a TLS-stripped body would be
chmod 755'd into./bin/nodeupwith no warning. Post-fix:Tests:
Notes on the JS test framework question
I deliberately did not introduce a JS test framework (jest/mocha/vitest) for what is a 12-case pure-helper smoke check — the same reasoning I used for the
tarruntime-dep fix in #63.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 12 tests above are the most useful signal for a parsing + redirect-validation 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/install.js—httpsGet(targetUrl)(single-GET helper for an explicit URL string).followHops(startUrl, hopsLeft, acceptStatus)— recursive redirect walker with host allowlist (ALLOWED_REDIRECT_HOSTS) and hop ceiling (MAX_REDIRECT_HOPS = 2).downloadTo(url, destPath)— streams the body to disk while piping throughcrypto.createHash('sha256'); returns{ path, sha256 }.downloadText(url)— same redirect handling but returns the body as a string (used forchecksums.txt).parseChecksumsTxt(body)— accepts GoReleaser /sha256sum -b/sha256sum --tagshapes; key is basename, value is the SHA256 hex.fetchExpectedHash(repo, tag, archiveName)— fetcheschecksums.txt, parses it, returns the expected hash for our archive name. Errors name both the offender and the available set.main(): fetch expected hash first (fail fast), then download archive with streaming hash, thendie()on mismatch (afterunlinkSync'ing the bad bytes). Extraction runs only on a verified match.nodeup-npm/scripts/install_test.js— new file, 12 tests, no JS test framework. Mirrors the helpers from install.js (because install.js is a script invoked by npm directly and doesn'tmodule.exports); mirrors the regex + redirect-allowlist semantics exactly. Each test printsok <name>on success and exits 1 with the offending case on failure.CHANGELOG.md—### Fixedentry under[Unreleased]documenting the redirect allowlist, the SHA256 stream-while-downloading pattern, the unlink-on-mismatch cleanup, and the test surface. References security(npm-wrapper): no checksum verification or redirect validation on downloaded binary #64.