fix(security): verify SHA-256 checksum on OpenShell binary download#697
fix(security): verify SHA-256 checksum on OpenShell binary download#697
Conversation
📝 WalkthroughWalkthroughTwo installation scripts now download and verify SHA-256 checksums before extracting OpenShell archives, using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/install-openshell.sh (2)
82-98: Checksum verification correctly implemented with proper cleanup.This script has the EXIT trap at line 80, ensuring
$tmpdiris cleaned up regardless of success or failure. The checksum verification logic is consistent withscripts/install.sh.Same minor note about grep precision applies here:
♻️ Optional: Use fixed-string grep for precision
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 82 - 98, The checksum verification uses grep "$ASSET" "$CHECKSUM_FILE" which can mis-match substrings; update the verification command to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum -a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
96-97: Consider handling missing checksum entry explicitly.If
grep "$ASSET"finds no match (asset not listed in checksum file), the pipeline will fail with "SHA-256 checksum verification failed." This is technically correct, but the error message could be misleading—the failure isn't a mismatch but a missing entry.♻️ Optional: More informative error handling
info "Verifying SHA-256 checksum..." -(cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ - || fail "SHA-256 checksum verification failed for $ASSET" +(cd "$tmpdir" && { + checksum_line=$(grep -F "$ASSET" "$CHECKSUM_FILE") \ + || fail "Asset $ASSET not found in $CHECKSUM_FILE" + echo "$checksum_line" | shasum -a 256 -c - +}) || fail "SHA-256 checksum verification failed for $ASSET"This is optional since the current behavior is safe (fails closed), just less descriptive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-openshell.sh` around lines 96 - 97, The failure message is ambiguous when the checksum file lacks an entry for ASSET; before piping into shasum, test for a matching line in CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the grep output into a variable), and if no match call fail with a clear message like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the matching line to shasum -a 256 -c - and keep the existing failure path for actual checksum mismatches.scripts/install.sh (1)
339-356: Good security improvement with checksum verification.The implementation correctly downloads and verifies the SHA-256 checksum before extraction. A few observations:
Missing tmpdir cleanup on failure: Unlike
install-openshell.shwhich hastrap 'rm -rf "$tmpdir"' EXIT(line 80), this script does not set up a trap. If checksum verification fails,$tmpdiris left behind.Grep pattern could be more precise:
grep "$ASSET"could match multiple lines if a filename is a substring of another (e.g.,openshell-x86_64matching bothopenshell-x86_64-apple-darwin.tar.gzand a hypotheticalopenshell-x86_64-v2-apple-darwin.tar.gz). Consider anchoring the pattern.♻️ Suggested improvements
tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' EXIT CHECKSUM_FILE="openshell-checksums-sha256.txt"For more precise grep matching (optional):
- (cd "$tmpdir" && grep "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \ + (cd "$tmpdir" && grep -F "$ASSET" "$CHECKSUM_FILE" | shasum -a 256 -c -) \Using
-Ftreats the pattern as a fixed string rather than a regex, which is safer and slightly faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 339 - 356, The script currently downloads checksums into $tmpdir but doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses grep "$ASSET" which can match substrings; add a trap like the one in install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating tmpdir to ensure removal on exit/failure, and make the verification grep precise by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ... "$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is checked.
🤖 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-openshell.sh`:
- Around line 82-98: The checksum verification uses grep "$ASSET"
"$CHECKSUM_FILE" which can mis-match substrings; update the verification command
to use a fixed-string or exact-match grep (e.g., use grep -F or grep -x) so
CHECKSUM_FILE is searched precisely for the ASSET entry before piping to shasum
-a 256 -c -; modify the line that runs (cd "$tmpdir" && grep "$ASSET"
"$CHECKSUM_FILE" | shasum -a 256 -c -) to reference the chosen grep option and
ensure variables GH_TOKEN, ASSET, CHECKSUM_FILE and tmpdir remain unchanged.
- Around line 96-97: The failure message is ambiguous when the checksum file
lacks an entry for ASSET; before piping into shasum, test for a matching line in
CHECKSUM_FILE (e.g., use grep -q -- "$ASSET" "$CHECKSUM_FILE" or capture the
grep output into a variable), and if no match call fail with a clear message
like "No checksum entry for $ASSET in $CHECKSUM_FILE"; otherwise feed the
matching line to shasum -a 256 -c - and keep the existing failure path for
actual checksum mismatches.
In `@scripts/install.sh`:
- Around line 339-356: The script currently downloads checksums into $tmpdir but
doesn't register a cleanup trap (leaving tmpdir if verification fails) and uses
grep "$ASSET" which can match substrings; add a trap like the one in
install-openshell.sh (trap 'rm -rf "$tmpdir"' EXIT) immediately after creating
tmpdir to ensure removal on exit/failure, and make the verification grep precise
by using fixed/anchored matching (e.g., use grep -F or anchor the pattern to the
exact filename when reading CHECKSUM_FILE) in the (cd "$tmpdir" && grep ...
"$CHECKSUM_FILE" | shasum -a 256 -c -) step so only the exact ASSET entry is
checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aed8a69f-7e86-4cd5-94b6-bd88862a8fe3
📒 Files selected for processing (3)
scripts/install-openshell.shscripts/install.shtest/runner.test.js
1 participant
Summary
Both
scripts/install.shandscripts/install-openshell.shdownload the OpenShell binary viacurl/ghand install it (often as root) without any integrity verification. OpenShell releases includeopenshell-checksums-sha256.txtalongside the tarballs — verified present in v0.0.10 through v0.0.13.Both files now:
openshell-checksums-sha256.txtalongside the tarball (bothghandcurlpaths)shasum -a 256 -cbefore extractionTest plan
openshell-checksums-sha256.txtandshasum -a 256 -cnemoclaw onboardinstalls OpenShell with checksum verificationSummary by CodeRabbit
New Features
Tests