feat(security): verify binary SHA-256 checksum on download (B5)#39
feat(security): verify binary SHA-256 checksum on download (B5)#39
Conversation
- Fetch checksums.txt from release assets after binary download - Compute SHA-256 of downloaded binary using Node crypto - On mismatch: delete binary and throw Error with clear message - Graceful fallback: warn if checksums.txt not available (older releases) Addresses: design-partner-eval B5 (P1 security)
There was a problem hiding this comment.
Pull request overview
Adds SHA-256 checksum verification for downloaded capiscio-core release binaries in the Node.js wrapper to improve download integrity (B5 security eval).
Changes:
- Downloads
checksums.txtfrom the same GitHub Release as the binary. - Computes SHA-256 of the downloaded temp binary and compares to the published checksum.
- On mismatch, deletes the temp file and throws an error; falls back with warnings if checksums are unavailable.
| const fileBuffer = fs.readFileSync(filePath); | ||
| const actualHash = crypto.createHash('sha256').update(fileBuffer).digest('hex'); |
There was a problem hiding this comment.
verifyChecksum reads the entire downloaded binary into memory with fs.readFileSync just to hash it; this can be expensive for larger release assets and blocks the event loop. Prefer computing the SHA-256 via a read stream (e.g., fs.createReadStream) and updating the hash incrementally.
| const fileBuffer = fs.readFileSync(filePath); | |
| const actualHash = crypto.createHash('sha256').update(fileBuffer).digest('hex'); | |
| const hash = crypto.createHash('sha256'); | |
| await pipeline(fs.createReadStream(filePath), hash); | |
| const actualHash = hash.digest('hex'); |
| // Verify checksum integrity | ||
| await this.verifyChecksum(tempFilePath, assetName); | ||
|
|
There was a problem hiding this comment.
If verifyChecksum throws (e.g., checksum mismatch), the temp directory created for the download is not cleaned up because cleanup only happens after the copy step. Consider scoping tempDir/tempFilePath outside the try block and using a finally to rmSync(tempDir, { recursive: true, force: true }) on both success and failure.
| private async verifyChecksum(filePath: string, assetName: string): Promise<void> { | ||
| const checksumsUrl = `https://github.com/${REPO_OWNER}/${REPO_NAME}/releases/download/${VERSION}/checksums.txt`; | ||
|
|
||
| let expectedHash: string | null = null; | ||
| try { | ||
| const resp = await axios.get(checksumsUrl, { timeout: 30000 }); | ||
| const lines = (resp.data as string).trim().split('\n'); | ||
| for (const line of lines) { | ||
| const parts = line.trim().split(/\s+/); | ||
| if (parts.length === 2 && parts[1] === assetName) { | ||
| expectedHash = parts[0]; | ||
| break; | ||
| } | ||
| } | ||
| } catch { | ||
| console.warn('Warning: Could not fetch checksums.txt. Skipping integrity verification.'); | ||
| return; | ||
| } | ||
|
|
||
| if (!expectedHash) { | ||
| console.warn(`Warning: Asset ${assetName} not found in checksums.txt. Skipping verification.`); | ||
| return; | ||
| } | ||
|
|
||
| const fileBuffer = fs.readFileSync(filePath); | ||
| const actualHash = crypto.createHash('sha256').update(fileBuffer).digest('hex'); | ||
|
|
||
| if (actualHash !== expectedHash) { | ||
| // Remove the tampered file | ||
| fs.unlinkSync(filePath); | ||
| throw new Error( | ||
| `Binary integrity check failed for ${assetName}. ` + | ||
| `Expected SHA-256: ${expectedHash}, got: ${actualHash}. ` + | ||
| 'The downloaded file does not match the published checksum.' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Checksum verification introduces new security-critical behavior (successful verification, mismatch error + deletion, and fallback when checksums.txt is missing), but there are no unit tests asserting these paths. Since this repo already has BinaryManager install tests, please add coverage by mocking axios.get for both the binary and checksums.txt, and mocking fs.readFileSync/hashing to simulate match/mismatch.
src/utils/binary-manager.ts
Outdated
| const lines = (resp.data as string).trim().split('\n'); | ||
| for (const line of lines) { | ||
| const parts = line.trim().split(/\s+/); | ||
| if (parts.length === 2 && parts[1] === assetName) { | ||
| expectedHash = parts[0]; |
There was a problem hiding this comment.
Checksum parsing is likely to miss valid sha256sum formats where the filename is prefixed with * (binary mode) or includes a leading path like ./..., which would make parts[1] !== assetName and silently skip verification. Consider normalizing the parsed filename (strip leading * and optional ./) and normalizing hashes to lowercase before comparing.
Summary
Adds SHA-256 checksum verification for downloaded capiscio-core binaries (B5 - design partner eval, part 3/3).
Changes
checksums.txtfrom the same releasecrypto.createHash('sha256')checksums.txtis not available (older releases), log a warning and continueRelated PRs
checksums.txtin releaseEvaluation Plan
Design partner eval item B5 (P1 — Security)