feat: add SHA-256 checksum verification to install.js#592
Open
MaxHuang22 wants to merge 6 commits intolarksuite:mainfrom
Open
feat: add SHA-256 checksum verification to install.js#592MaxHuang22 wants to merge 6 commits intolarksuite:mainfrom
MaxHuang22 wants to merge 6 commits intolarksuite:mainfrom
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 (1)
📝 WalkthroughWalkthroughAdds SHA-256 checksum handling for published CLI binaries: release workflow downloads Changes
Sequence DiagramsequenceDiagram
participant CI as CI (Release)
participant GH as GitHub Releases
participant NPM as npm Registry
participant User as User Environment
participant Install as install.js
participant Curl as curl
participant FS as File System
CI->>GH: Authenticate + download `checksums.txt` for tag
CI->>NPM: Publish package (includes `checksums.txt`)
User->>NPM: npm install `@larksuite/cli`
NPM-->>User: Package (with `checksums.txt`)
User->>Install: Run postinstall (install.js)
Install->>Install: assertAllowedHost(downloadUrl)
Install->>Curl: Download archive (--max-redirs 3)
Curl-->>Install: Archive file
Install->>FS: Read `checksums.txt`
Install->>Install: compute SHA-256(archive)
Install->>Install: compare with expected hash
alt Match
Install->>FS: extract archive to bin
Install-->>User: installation success
else Mismatch or missing entry
Install-->>User: throw `[SECURITY] Checksum mismatch` / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 60.92% 61.69% +0.76%
==========================================
Files 399 402 +3
Lines 34089 35073 +984
==========================================
+ Hits 20770 21639 +869
- Misses 11395 11429 +34
- Partials 1924 2005 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2a19f0c23120d34ac19f681a66ee62a20788a1be🧩 Skill updatenpx skills add MaxHuang22/cli#feat/install-checksum-verification -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.js (1)
42-61:⚠️ Potential issue | 🟠 MajorValidate redirect targets against the host allowlist, or disable automatic redirects.
The
assertAllowedHost()check at line 50 only validates the initial URL. However,curl --locationwill followLocationheaders to other hosts without validation. While--max-redirs 3limits the redirect count, curl has no built-in option to enforce host allowlisting on redirects (--proto-redircontrols protocol types, not hosts). Either validate each redirect target or use--location-trustedwith explicit protocol constraints, or remove--locationto reject redirects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` around lines 42 - 61, The current download function uses curl with "--location" so redirects are followed without re-checking hosts; update the download(url, destPath) implementation to remove the "--location" flag from args and implement manual redirect handling: perform the request, on 3xx responses read the Location header, call assertAllowedHost(newUrl) before following, enforce the existing "--max-redirs" limit (or the args' max redirs value) and other timeout options, and continue following only when the redirected host is allowed; preserve the isWindows check that adds "--ssl-revoke-best-effort" and keep using execFileSync("curl", ...) for each request/redirect step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-21-install-checksum.md`:
- Around line 150-152: The fenced expected-output block containing [
'getExpectedChecksum', 'verifyChecksum' ] should be labeled as a text fence for
markdown linting; update the code fence that wraps getExpectedChecksum and
verifyChecksum so it begins with ```text and ends with ``` to mark it as plain
text output.
In `@docs/superpowers/specs/2026-04-21-install-checksum-design.md`:
- Around line 7-21: The fenced code blocks in the spec (the trust-chain diagram
starting with "GoReleaser (CI) generates dist/checksums.txt", the install flow
block containing "download archive (github, fallback mirror) -> expectedHash =
getExpectedChecksum(archiveName)", and the test-case outline starting with
"getExpectedChecksum - returns correct hash...") are unlabeled and trigger
markdownlint; add the language label "text" to each triple-backtick fence for
those blocks (and the other unlabeled fences noted around the similar sections)
so they read ```text ... ``` to silence the linter while preserving content.
In `@scripts/install.js`:
- Around line 106-110: The installer currently returns null when checksums.txt
is missing (the fs.existsSync(checksumsPath) branch), allowing installs to
proceed unverified; change this to fail-closed by throwing an Error or exiting
non-zero with a clear message when checksumsPath is missing, and add an explicit
opt-out (e.g., read an env var like SKIP_CHECKSUMS=true or a CLI flag) so
callers of the verification logic (the checksum verification flow that expects
the checksums content) can opt out for local dev; update the log text to reflect
the required opt-out variable and ensure callers no longer treat null as "skip"
but instead propagate the error.
---
Outside diff comments:
In `@scripts/install.js`:
- Around line 42-61: The current download function uses curl with "--location"
so redirects are followed without re-checking hosts; update the download(url,
destPath) implementation to remove the "--location" flag from args and implement
manual redirect handling: perform the request, on 3xx responses read the
Location header, call assertAllowedHost(newUrl) before following, enforce the
existing "--max-redirs" limit (or the args' max redirs value) and other timeout
options, and continue following only when the redirected host is allowed;
preserve the isWindows check that adds "--ssl-revoke-best-effort" and keep using
execFileSync("curl", ...) for each request/redirect step.
🪄 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: 9ccca54a-0a7d-40a9-b9dd-c580b973148e
📒 Files selected for processing (6)
.github/workflows/release.ymldocs/superpowers/plans/2026-04-21-install-checksum.mddocs/superpowers/specs/2026-04-21-install-checksum-design.mdpackage.jsonscripts/install.jsscripts/install.test.js
Change-Id: I5444e3f34642d7c0740b6422a70ca6921a85e363
Change-Id: I87548be25d30c384e743da17b1d161b9d9f0ea87
Change-Id: Ifc2067bf1b824b02257dba7b53716fbe18d0f6b6
Change-Id: I2580782866049f1f62a2597e86b7bf59d0e50925
Change-Id: I2d7c44d9d5b9075158f63c0f8cf66c1e0abe3d8d
MaxHuang22
force-pushed
the
feat/install-checksum-verification
branch
from
33f4b8d to
537967b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 48-53: The release workflow step that downloads checksums is
fragile: it derives the tag from package.json and doesn't fail if checksums.txt
is missing, which can silently disable verification in getExpectedChecksum
(scripts/install.js). Update the GitHub Action to use the triggering tag
(github.ref_name) instead of reading package.json, and add an explicit check
after gh release download to fail the job if checksums.txt is not present (exit
nonzero and log a clear error). Optionally add a follow-up step that runs the
install verification (e.g., invoke the verification path in scripts/install.js
or a dry-run installation) to ensure checksums.txt actually validates artifacts
before publishing.
🪄 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: 3a8a3355-5498-49d6-8fee-b9c11af93389
📒 Files selected for processing (4)
.github/workflows/release.ymlpackage.jsonscripts/install.jsscripts/install.test.js
✅ Files skipped from review due to trivial changes (2)
- package.json
- scripts/install.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.js
…orkflow Address CodeRabbit review: use GITHUB_REF_NAME instead of parsing package.json to avoid version drift, and add explicit file check to fail loudly if checksums.txt is missing or empty. Change-Id: I8a5658412b6afc338ad2a642baba146cceafd0fc
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
1 participant
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
Downloaded binary archives in
scripts/install.jswere not verified against any checksum, leaving the install path vulnerable to CDN tampering, MITM, or corruption. This PR bundles GoReleaser'schecksums.txtin the npm package and verifies the SHA-256 of every downloaded archive before extraction.Changes
scripts/install.jsto be side-effect-free onrequire()viarequire.main === moduleguardgetExpectedChecksum(archiveName, checksumsDir?)to parse GoReleaser checksums.txt formatverifyChecksum(archivePath, expectedHash)with[SECURITY]-prefixed error on mismatchassertAllowedHost(url)host allowlist (github.com, objects.githubusercontent.com, registry.npmmirror.com) and--max-redirs 3to curl args indownload()install()flow (after download, before extraction)checksums.txttopackage.jsonfilesarrayDownload checksums from releasestep to.github/workflows/release.ymlpublish-npm jobscripts/install.test.jswith 7 unit tests for exported functionsTest Plan
make unit-testpassedmake validatepassed (build, vet, unit test, integration test)node --test scripts/install.test.js(7/7 pass),node -e "require('./scripts/install.js')"(side-effect-free)Related Issues
N/A
Summary by CodeRabbit