feat: add SHA-256 checksum verification to install.js#592
Merged
MaxHuang22 merged 7 commits intolarksuite:mainfrom Apr 23, 2026
Merged
feat: add SHA-256 checksum verification to install.js#592MaxHuang22 merged 7 commits intolarksuite:mainfrom
MaxHuang22 merged 7 commits intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds checksum-based integrity checks for published CLI binaries: release workflow now downloads Changes
Sequence DiagramsequenceDiagram
participant CI as CI (Release)
participant GH as GitHub Releases
participant NPM as npm Registry
participant User as Developer Machine
participant Install as install.js
participant Curl as curl
participant FS as File System
CI->>GH: Authenticate and 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 (node scripts/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 Missing or Mismatch
Install-->>User: throw `[SECURITY] Checksum mismatch` / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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% 62.06% +1.13%
==========================================
Files 399 407 +8
Lines 34089 35799 +1710
==========================================
+ Hits 20770 22217 +1447
- Misses 11395 11554 +159
- Partials 1924 2028 +104 ☔ 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@80642154212743408792c6c57e7691cfd19604b6🧩 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
- verifyChecksum: switch from readFileSync to streaming 64KB chunks to avoid loading entire archive (10-100MB) into memory - Export and test assertAllowedHost: 7 cases covering allowed hosts, rejection, case normalization, port handling, invalid URL - Add ALLOWED_HOSTS comment clarifying it only gates initial URL - Add getExpectedChecksum tests for malformed/tab-separated lines Change-Id: Ida639def89c242b3b261a76effae08fd414a10dc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.js (1)
106-129: Parser handles GoReleaser format correctly; minor parameter-shadowing nit.Using
indexOf(" ")as the hash/name separator correctly rejects tab-separated and single-space malformed lines (matching the new tests). The fail-open on missingchecksums.txtis intentional per spec §4.2.Nit: the parameter name
archiveNameshadows the module-level constant on line 39, which can be mildly confusing when readinginstall()call sites. Renaming the parameter (e.g.,name) is optional.♻️ Optional rename to avoid shadowing
-function getExpectedChecksum(archiveName, checksumsDir) { +function getExpectedChecksum(name, checksumsDir) { const dir = checksumsDir || path.join(__dirname, ".."); const checksumsPath = path.join(dir, "checksums.txt"); @@ - const name = trimmed.slice(idx + 2); - if (name === archiveName) return hash; + const entryName = trimmed.slice(idx + 2); + if (entryName === name) return hash; } - throw new Error(`Checksum entry not found for ${archiveName}`); + throw new Error(`Checksum entry not found for ${name}`); }Based on learnings, the null-return when
checksums.txtis missing is intentional (spec §4.2, CItest -sguard as the publish-time safety net) and is deliberately not being flagged here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` around lines 106 - 129, getExpectedChecksum currently uses a parameter named archiveName which shadows the module-level constant archiveName; rename the parameter (e.g., to name or archiveFile) and update all references inside getExpectedChecksum and any call sites (e.g., install() calls that pass the archiveName) to use the new parameter name so there is no shadowing between the function parameter and the module-level archiveName constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.js`:
- Around line 106-129: getExpectedChecksum currently uses a parameter named
archiveName which shadows the module-level constant archiveName; rename the
parameter (e.g., to name or archiveFile) and update all references inside
getExpectedChecksum and any call sites (e.g., install() calls that pass the
archiveName) to use the new parameter name so there is no shadowing between the
function parameter and the module-level archiveName constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be5fc262-b14b-4468-822f-98e83eae616a
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.test.js
liangshuo-1
approved these changes
Apr 23, 2026
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
2 participants
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)using streaming 64KB-chunk hash to avoid loading entire archive into memory;[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.jsonfilesarray.github/workflows/release.ymlpublish-npm job: useGITHUB_REF_NAMEinstead of parsingpackage.json, add explicittest -s checksums.txtguardscripts/install.test.jswith 16 unit tests covering all exported functionsDesign Decisions
test -s checksums.txtguard provides a compensating fail-closed control there. Switching client-side to fail-closed can follow once the pipeline is proven stable.Test Plan
make unit-testpassedmake validatepassed (build, vet, unit test, integration test)node --test scripts/install.test.js(16/16 pass),node -e "require('./scripts/install.js')"(side-effect-free)Related Issues
N/A
Summary by CodeRabbit
Release Notes
New Features
Tests