feat: fail closed when checksums.txt is missing during install#1503
feat: fail closed when checksums.txt is missing during install#1503dc-bytedance wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTwo functions in ChangesFail-closed checksum verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.test.js`:
- Around line 135-144: The test for verifyChecksum with the name "verifyChecksum
throws [SECURITY] on null/empty expectedHash (fail-closed)" only tests the null
case but does not test the empty string case as the test name suggests. Add an
additional test case (or modify the existing one) to also verify that
verifyChecksum throws a [SECURITY] error when expectedHash is passed as an empty
string (""). Ensure the test uses the same assertion pattern with assert.throws
and validates that the error message matches the /^\[SECURITY\]/ regex pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c988e8c9-3adf-4523-b086-8e1acf9108e1
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1503 +/- ##
=======================================
Coverage 73.52% 73.52%
=======================================
Files 783 783
Lines 74630 74630
=======================================
Hits 54874 54874
Misses 15662 15662
Partials 4094 4094 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c7e7e8ad4042a2ca2f9714d7d4a2743fbd00f834🧩 Skill updatenpx skills add larksuite/cli#feat/harden-npm-install -y -g |
dc-bytedance
force-pushed
the
feat/harden-npm-install
branch
from
ae88ea7 to
c7e7e8a
Compare
1 participant
Summary
The npm
postinstall(scripts/install.js) verifies the downloaded Go binary against the package-shippedchecksums.txt, but previously skipped verification entirely whenchecksums.txtwas missing (fail-open) — letting an unverified or tampered binary install silently. This change makes it fail-closed: a missing manifest now aborts the install with a clear[SECURITY]error and a non-zero exit, so an unverified binary is never copied into place.Changes
scripts/install.js:getExpectedChecksumnow throws[SECURITY] checksums.txt not found; refusing to install an unverified binary.whenchecksums.txtis missing (previously logged[WARN]and returnednullto skip verification).scripts/install.js:verifyChecksumnow throws[SECURITY] missing expected checksum; ...for a non-string/empty expected hash, replacing the silentif (expectedHash === null) return;early-return (the second half of the fail-open path).scripts/install.test.js: updated the missing-manifest case to assert the fail-closed throw; added a regression test for the empty-hash guard.Normal installs are unaffected: published packages always ship
checksums.txt(package.jsonfilesincludes it and the release workflow enforces its presence before publish). A missing manifest only occurs for tampered or non-standard packages — exactly what fail-closed should reject.Test Plan
node --test scripts/install.test.jspassed (33/33)install()path with nochecksums.txt— aborted with[SECURITY], exit 1, no binary written tobin/; with a validchecksums.txtit passed the checksum gateRelated Issues
N/A
Summary by CodeRabbit