fix(install): add SHA-256 verification for cli.js download#3389
fix(install): add SHA-256 verification for cli.js download#3389
Conversation
The install script downloads cli.js from GitHub Releases but does not verify its integrity, unlike the bun installer which checks a pinned SHA-256 hash. This adds checksum verification using a companion cli.js.sha256 release artifact (same pattern as the bun hash check). When the checksum file is not yet published, the installer warns and continues — once CI publishes cli.js.sha256, verification activates automatically with no further install.sh changes needed. Fixes #3327 Agent: security-auditor Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
la14-1
left a comment
There was a problem hiding this comment.
Security Review: PR #3389 — SHA-256 verification for cli.js download
Does the fix address the vulnerability in #3327?
Partially. This PR adds the client-side verification logic to install.sh, which is the necessary first half. However, as #3327 explicitly notes, both halves are needed:
- install.sh side (this PR): Download
cli.js.sha256, verify the hash ofcli.jsbefore installing. Done correctly. - CI side (not in this PR): Publish
cli.js.sha256alongsidecli.jsin thecli-latestrelease. Without this, the verification is a no-op — the script gracefully degrades with "No cli.js.sha256 checksum published yet — skipping verification".
Findings
Positive:
- The
sha256_filehelper already exists on main (portable macOS/Linux implementation). The PR reuses it correctly. - The
tr -d '[:space:]'on the expected hash is a good defensive measure against trailing newlines in the checksum file. - Graceful degradation is implemented at two levels: (a) no checksum file published yet, (b) no sha256sum/shasum binary available. Both log warnings instead of failing.
- The error messaging on mismatch is excellent — clear "possible supply chain attack" language with the expected vs actual hashes and a pointer to the issues page.
- The
--proto '=https'flag on the checksum download enforces HTTPS, preventing downgrade attacks. - The
curlfor the checksum file uses2>/dev/nullto suppress errors when the file doesn't exist yet, which is appropriate.
Concerns:
- TOCTOU is not a risk here — the downloaded
cli.jsis verified in the sametmpdirbefore being copied to the install location. No race window. - The checksum file format is simple — just the hex digest, no filename. This matches the implementation (
tr -d '[:space:]'strips any whitespace). Consistent with common practice. - Until CI publishes the
.sha256file, this provides zero protection. The graceful skip means an attacker who compromises only thecli.jsartifact (but not.sha256because it doesn't exist) gets through silently. This is acceptable as a phased rollout — the warning makes it visible that verification is not happening.
Recommendation: This PR is correct and safe to merge. The CI-side companion change (publishing cli.js.sha256 in the release workflow) should be tracked as a follow-up — without it, the verification is dormant. Is there an issue tracking the CI side?
-- refactor/security-auditor
Security Review: SHA-256 Checksum VerificationVerdict: LGTM with minor notes Strengths
Notes (non-blocking)
Security ImpactThis is a positive security improvement. It adds defense-in-depth against CDN cache poisoning and accidental artifact corruption. The same-origin limitation is acceptable for v1; GPG signatures would be the next step for full supply-chain protection. -- refactor/security-auditor |
Why: Prevents MITM/tampered binary downloads by verifying SHA-256 of cli.js before execution — closes the last unverified download in the install path (bun installer already has hash verification).
Changes
cli.js.sha256companion file from the same GitHub Releasesha256_file()helper (works on both macOSshasumand Linuxsha256sum)Remaining work (separate PR, requires human review)
.github/workflows/release.ymlmust be updated to publishcli.js.sha256alongsidecli.jsin thecli-latestrelease. Workflow changes are off-limits for automated PRs per team rules. Once CI publishes the checksum file, this verification activates automatically.Fixes #3327