fix: add CAPISCIO_REQUIRE_CHECKSUM fail-closed mode (B5 hardening)#40
fix: add CAPISCIO_REQUIRE_CHECKSUM fail-closed mode (B5 hardening)#40
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional “fail-closed” checksum verification mode to the capiscio-node binary download flow, improving integrity guarantees when CAPISCIO_REQUIRE_CHECKSUM=true while keeping the default behavior backward-compatible.
Changes:
- Computes and validates a SHA-256 checksum for downloaded binaries using
checksums.txtfrom the matching capiscio-core GitHub Release. - Introduces
CAPISCIO_REQUIRE_CHECKSUMto enforce fail-closed behavior when checksums are unavailable or incomplete.
Comments suppressed due to low confidence (1)
src/utils/binary-manager.ts:140
install()only deletes the temp directory on the success path. IfverifyChecksum()throws (e.g., mismatch or CAPISCIO_REQUIRE_CHECKSUM fail-closed), the temp dir and downloaded file will be left behind, which can accumulate over time. Consider scopingtempDiroutside the try and cleaning it up in afinally(or in the catch) so it always gets removed on failure as well.
// Verify checksum integrity
await this.verifyChecksum(tempFilePath, assetName);
// Move to install dir
// We rename it to capiscio-core (or .exe) for internal consistency
fs.copyFileSync(tempFilePath, this.binaryPath);
if (platform !== 'windows') {
fs.chmodSync(this.binaryPath, 0o755); // Make executable
}
// Cleanup
fs.rmSync(tempDir, { recursive: true, force: true });
| if (requireChecksum) { | ||
| fs.unlinkSync(filePath); | ||
| throw new Error( | ||
| 'Checksum verification required (CAPISCIO_REQUIRE_CHECKSUM=true) ' + | ||
| 'but checksums.txt is not available. Cannot verify binary integrity.' | ||
| ); | ||
| } | ||
| console.warn('Warning: Could not fetch checksums.txt. Skipping integrity verification.'); | ||
| return; | ||
| } | ||
|
|
||
| if (!expectedHash) { | ||
| if (requireChecksum) { | ||
| fs.unlinkSync(filePath); | ||
| throw new Error( |
There was a problem hiding this comment.
verifyChecksum() uses fs.unlinkSync(filePath) before throwing. If the unlink fails (e.g., file already removed, permission issues), it will throw and mask the intended checksum error. Prefer a best-effort cleanup (e.g., rmSync(..., { force: true }) or wrapping unlink in its own try/catch) so the caller reliably receives the checksum-related failure.
src/utils/binary-manager.ts
Outdated
| 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 via fs.readFileSync to compute the SHA-256. For larger binaries this can be unnecessarily memory-intensive; hashing via a read stream (or incremental reads) avoids holding the whole file in RAM and keeps install memory usage predictable.
| private async verifyChecksum(filePath: string, assetName: string): Promise<void> { | ||
| const requireChecksum = ['1', 'true', 'yes'].includes( | ||
| (process.env.CAPISCIO_REQUIRE_CHECKSUM ?? '').toLowerCase() | ||
| ); | ||
| 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] ?? null; | ||
| break; | ||
| } | ||
| } | ||
| } catch { | ||
| if (requireChecksum) { | ||
| fs.unlinkSync(filePath); | ||
| throw new Error( | ||
| 'Checksum verification required (CAPISCIO_REQUIRE_CHECKSUM=true) ' + | ||
| 'but checksums.txt is not available. Cannot verify binary integrity.' | ||
| ); | ||
| } | ||
| console.warn('Warning: Could not fetch checksums.txt. Skipping integrity verification.'); | ||
| return; | ||
| } | ||
|
|
||
| if (!expectedHash) { | ||
| if (requireChecksum) { | ||
| fs.unlinkSync(filePath); | ||
| throw new Error( | ||
| `Checksum verification required (CAPISCIO_REQUIRE_CHECKSUM=true) ` + | ||
| `but asset ${assetName} not found in checksums.txt.` | ||
| ); | ||
| } | ||
| console.warn(`Warning: Asset ${assetName} not found in checksums.txt. Skipping verification.`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
New checksum verification behavior (including CAPISCIO_REQUIRE_CHECKSUM fail-closed mode, missing checksums.txt behavior, missing asset entry, and mismatch handling/deletion) is introduced here but is not covered by existing unit tests for BinaryManager. Add focused tests that mock the checksums.txt response and verify both fail-open and fail-closed paths, since this logic runs on every install.
1. Replace unlinkSync with rmSync({ force: true }) so file-removal
errors never mask the checksum error
2. Switch to stream-based hashing (createReadStream + crypto pipe)
instead of readFileSync to avoid loading entire binary into memory
3. Add checksum.test.ts with 7 tests covering: match, mismatch,
fetch-fail (optional), fetch-fail (required), entry-missing
(optional), entry-missing (required), env-var variants
When CAPISCIO_REQUIRE_CHECKSUM=true, binary download fails if checksums.txt is unavailable or the asset is missing from it. Default behavior remains fail-open (warn and continue) for backward compatibility.
1. Replace unlinkSync with rmSync({ force: true }) so file-removal
errors never mask the checksum error
2. Switch to stream-based hashing (createReadStream + crypto pipe)
instead of readFileSync to avoid loading entire binary into memory
3. Add checksum.test.ts with 7 tests covering: match, mismatch,
fetch-fail (optional), fetch-fail (required), entry-missing
(optional), entry-missing (required), env-var variants
beonde
force-pushed
the
fix/eval-binary-checksums
branch
from
ed8412d to
df7cc4a
Compare
Adds
CAPISCIO_REQUIRE_CHECKSUM=trueenv var that makes binary download fail-closed when checksums.txt is unavailable or the asset is missing from it. Default behavior remains fail-open for backward compatibility.Part of design partner re-evaluation B5 hardening.