install: warn instead of failing when checksums.txt is missing - #1711
install: warn instead of failing when checksums.txt is missing#1711liangshuo-1 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
size/M
📝 WalkthroughWalkthroughChecksum verification in the install script now fails open: when checksums.txt is missing, getExpectedChecksum logs a warning and returns null instead of throwing; verifyChecksum returns early when the expected hash is null. Corresponding tests are updated to match, removing the prior fail-closed security-error assertions. ChangesChecksum verification behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
liangshuo-1
changed the title
![coderabbitai[bot]](https://avatars.github.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.js`:
- Around line 267-272: The checksum fallback in verifyChecksum currently fails
open by returning null when checksums.txt is missing, which lets install proceed
without integrity validation. Update the logic around verifyChecksum and its
call site in install() so the default behavior remains fail-closed, and only
skip verification when an explicit opt-in env flag such as
LARK_CLI_SKIP_CHECKSUM is set. Also confirm the published package always
includes checksums.txt so this branch is limited to edge or dev scenarios.
🪄 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: 4d60cfe2-2575-4066-8273-dc6385bc39b7
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
| if (!fs.existsSync(checksumsPath)) { | ||
| throw new Error( | ||
| "[SECURITY] checksums.txt not found; refusing to install an unverified binary." | ||
| console.error( | ||
| "[WARN] checksums.txt not found, skipping checksum verification" | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Fail-open checksum verification allows installing an unverified binary.
Returning null when checksums.txt is absent causes verifyChecksum (Line 289) to skip integrity checks entirely, so install() will download, extract, and chmod 0o755 a binary whose SHA-256 was never validated (Lines 241-256). This weakens the supply-chain integrity guarantee: any download source in downloadUrls (including the npmmirror fallback) is trusted implicitly when the checksum file is missing.
I recognize this is a deliberate revert of #1503 per the PR objectives. Flagging so the tradeoff is explicit: consider gating fail-open behind an opt-in env flag (e.g. LARK_CLI_SKIP_CHECKSUM=1) so the default remains fail-closed, and confirm checksums.txt is always shipped in the published npm package so this branch is only reachable in edge/dev cases.
🤖 Prompt for 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.
In `@scripts/install.js` around lines 267 - 272, The checksum fallback in
verifyChecksum currently fails open by returning null when checksums.txt is
missing, which lets install proceed without integrity validation. Update the
logic around verifyChecksum and its call site in install() so the default
behavior remains fail-closed, and only skip verification when an explicit opt-in
env flag such as LARK_CLI_SKIP_CHECKSUM is set. Also confirm the published
package always includes checksums.txt so this branch is limited to edge or dev
scenarios.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1711 +/- ##
=======================================
Coverage 74.52% 74.52%
=======================================
Files 851 851
Lines 87155 87155
=======================================
Hits 64952 64952
Misses 17231 17231
Partials 4972 4972 ☔ 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@7dbed50529a5ea315a065851a680b55a69f0c089🧩 Skill updatenpx skills add larksuite/cli#revert-checksum-fail-closed-1503 -y -g |
1 participant
Reverts #1503 (commit
ec6fdc9b).What this reverts
PR #1503 made the npm install script fail closed when
checksums.txtwas missing:getExpectedChecksumthrew[SECURITY] checksums.txt not foundandverifyChecksumthrew on a null/empty expected hash.This restores the prior behavior:
getExpectedChecksum— logs[WARN] checksums.txt not found, skipping checksum verificationand returnsnull.verifyChecksum— skips verification whenexpectedHash === null.Note
This re-opens the security posture #1503 closed: installs will proceed with an unverified binary when
checksums.txtis absent.All 32 tests in
scripts/install.test.jspass.Summary by CodeRabbit