fix(security): verify release checksum and harden install download#373
fix(security): verify release checksum and harden install download#373
Conversation
github-actions
bot
added
the
size/L
|
|
📝 WalkthroughWalkthroughAdds checksum manifest handling and verification across release and install flows: the release workflow downloads and validates Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Entry Point
participant Install as Install Script
participant Release as GitHub Release (checksums.txt)
participant Source as Download Source (mirror)
participant Verify as Checksum Verifier
participant Extract as Archive Extractor
participant FS as File System
CLI->>Install: start install()
Install->>Release: read checksums.txt (from package or release)
Release-->>Install: checksums manifest (or error)
loop For each source
Install->>Source: download artifact (HTTPS, curl)
Source-->>Install: binary data or NetworkError
alt NetworkError
Install->>Install: try next source
else success
Install->>FS: write temp file
Install->>Verify: verifyChecksum(file, expectedHash)
Verify->>FS: stream & compute SHA-256
FS-->>Verify: digest
alt Checksum matches
Verify-->>Install: ok
Install->>Extract: extract archive (tar / PowerShell)
Extract->>FS: place binaries
FS-->>Extract: done
Extract-->>Install: success
else Checksum mismatch
Verify-->>Install: throw ChecksumError
Install->>Install: abort (no further retries)
end
end
end
Install->>FS: cleanup temp
FS-->>Install: cleaned
Install-->>CLI: exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@896e72d50dadefcb2f2ab664d6a116b947d697b4🧩 Skill updatenpx skills add larksuite/cli#fix/install-hardening -y -g |
Greptile SummaryThis PR hardens the npm install script for Confidence Score: 5/5Safe to merge — all prior P0/P1 concerns are addressed and no new blocking issues found. Both previously flagged issues (wrong error type for missing checksums.txt, and weak filename-only check in the release workflow) are fully resolved. The implementation is clean: no shell injection, defense-in-depth URL and TLS enforcement, stream-based SHA-256 verification before extraction, proper error class routing, side-effect-free exports, and four targeted tests. No P0 or P1 findings remain. No files require special attention.
|
| Filename | Overview |
|---|---|
| scripts/install.js | Major refactor: adds error class hierarchy, URL allowlisting, curl transport hardening via execFileSync args array, stream-based SHA-256 checksum verification, and side-effect-free module export. Logic is sound and well-commented. |
| scripts/install.test.js | New test file covering hash match, hash mismatch, entry lookup, missing file (PackageIntegrityError), and missing entry (ChecksumError) — all cases exercised correctly with proper teardown. |
| .github/workflows/release.yml | Adds fetch + regex-verified validation of checksums.txt from the GitHub Release before npm publish. The goreleaser config already sets name_template: checksums.txt, so the download pattern resolves correctly. |
| package.json | Adds checksums.txt to the files array so it is bundled in the published npm tarball, enabling offline checksum verification at install time. |
Sequence Diagram
sequenceDiagram
participant npm as npm install
participant install as install.js
participant cs as checksums.txt (bundled)
participant gh as github.com (primary)
participant mirror as registry.npmmirror.com (fallback)
npm->>install: node scripts/install.js
install->>cs: getExpectedChecksum(archiveName)
alt checksums.txt missing
cs-->>install: PackageIntegrityError
install-->>npm: "package looks broken"
else entry missing
cs-->>install: ChecksumError
install-->>npm: [SECURITY] abort
else found
cs-->>install: expectedHash (SHA-256)
end
install->>install: validate URL (HTTPS + allowlist)
install->>gh: curl --proto =https --proto-redir =https ... (download archive)
alt primary succeeds
gh-->>install: archive bytes
else NetworkError
install->>mirror: curl (same flags, fallback)
alt fallback succeeds
mirror-->>install: archive bytes
else NetworkError
mirror-->>install: NetworkError
install-->>npm: "all download sources failed"
end
end
install->>install: verifyChecksum(archivePath, expectedHash)
alt SHA-256 mismatch
install-->>npm: [SECURITY] integrity check failure
else match
install->>install: extract + chmod 0o755
install-->>npm: "installed successfully"
end
Reviews (2): Last reviewed commit: "fix(install): harden checksum verificati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.test.js (1)
4-14: Minor style inconsistency in imports.Lines 4-5 use the
node:protocol prefix while lines 6-9 use bare module names. This is purely cosmetic but consider aligning for consistency.♻️ Suggested alignment
const { test } = require("node:test"); const assert = require("node:assert"); -const fs = require("fs"); -const os = require("os"); -const path = require("path"); -const crypto = require("crypto"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const crypto = require("node:crypto");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.test.js` around lines 4 - 14, The imports in scripts/install.test.js mix the "node:" protocol and bare module names; standardize them for consistency by either prefixing all built-ins with "node:" (e.g., node:fs, node:os, node:path, node:crypto) or removing the "node:" prefix from test/assert to match the others—apply the change where require is used and ensure references to verifyChecksum, getExpectedChecksum, and ChecksumError remain unchanged.
🤖 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.test.js`:
- Around line 4-14: The imports in scripts/install.test.js mix the "node:"
protocol and bare module names; standardize them for consistency by either
prefixing all built-ins with "node:" (e.g., node:fs, node:os, node:path,
node:crypto) or removing the "node:" prefix from test/assert to match the
others—apply the change where require is used and ensure references to
verifyChecksum, getExpectedChecksum, and ChecksumError remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fdaed1d-dd6e-4223-b32d-4c35369fc89e
📒 Files selected for processing (4)
.github/workflows/release.ymlpackage.jsonscripts/install.jsscripts/install.test.js
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 61-71: The grep pattern interpolates ${VERSION} directly (in the
loop that iterates over plat and checks checksums.txt) so dots and other regex
metacharacters in VERSION can alter matching; update the check to perform an
exact filename match by either escaping regex metacharacters in VERSION before
use or switching to a literal match approach (e.g., use grep -F / fixed-string
or use awk to compare the second field exactly to the constructed filename
"lark-cli-${VERSION}-${plat}") so malformed versions cannot bypass checksum
validation.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: c62efff4-b182-474f-bee3-f8fe8e1382a0
📒 Files selected for processing (3)
.github/workflows/release.ymlscripts/install.jsscripts/install.test.js
✅ Files skipped from review due to trivial changes (1)
- scripts/install.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.js
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Resource not accessible by integration - https://docs.github.com/rest/git/trees#create-a-tree |
2 participants
Summary
checksums.txtshipped in the npm tarball, aborting install with a[SECURITY]-prefixed error on mismatch.scripts/install.jsdownload path: allowlist initial hosts (github.com,registry.npmmirror.com), boundcurlconnect/total time and redirect count, and fall back across mirrors on transient failures.scripts/install.jsside-effect-free onrequiresoscripts/install.test.jscan unit-testverifyChecksum/getExpectedChecksumwithout triggering a real install.checksums.txtas a release asset from.github/workflows/release.ymland include it in the published npm package viapackage.jsonfiles.Test plan
npm install -g @larksuite/clisucceeds end-to-end on darwin/linux/windows against a real release[SECURITY]abort with the expected guidancenode --test scripts/install.test.jspasses locally (4 tests covering hash match / mismatch / lookup / missing entry)checksums.txtalongside the archivesSummary by CodeRabbit
Bug Fixes
New Features
Tests