fix(dl): verify download integrity#168
Merged
Merged
Conversation
…d on missing checksum
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds optional per-download checksum verification, SHA-256 digest verification for OCI layer/blob files, new DownloadError variants for checksum/digest mismatches, integrates checksum use into PackageInstaller direct-downloads, and tightens install-time verification checks. ChangesChecksum and Digest Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/soar-operations/src/install.rs (1)
940-971:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize checksum casing in the final verification step.
Download::verify_checksum()now compares hex case-insensitively, but this install-time match still uses case-sensitive==/!=. An uppercasepkg.bsumcan pass the download step and then fail here during install.Suggested fix
- match (final_checksum, pkg.bsum.as_ref()) { - (Some(calculated), Some(expected)) if calculated != *expected => { + match (final_checksum, pkg.bsum.as_ref()) { + (Some(ref calculated), Some(expected)) + if !calculated.eq_ignore_ascii_case(expected) => + { events.emit(SoarEvent::Verifying { op_id, pkg_name: pkg.pkg_name.clone(), pkg_id: pkg.pkg_id.clone(), stage: VerifyStage::Failed("checksum mismatch".into()), }); return Err(SoarError::Custom( "Invalid checksum, skipped installation.".into(), )); } - (Some(ref calculated), Some(expected)) if calculated == expected => { + (Some(_), Some(_)) => { events.emit(SoarEvent::Verifying { op_id, pkg_name: pkg.pkg_name.clone(), pkg_id: pkg.pkg_id.clone(), stage: VerifyStage::Passed, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-operations/src/install.rs` around lines 940 - 971, The checksum comparison in the install verification uses case-sensitive equality on final_checksum and pkg.bsum causing false mismatches; update the match/conditional logic around final_checksum and pkg.bsum (the arms handling Some(calculated), Some(expected)) to compare case-insensitively (e.g., normalize both hex strings to the same case or use an ASCII-case-insensitive comparison) so the VerifyStage::Passed/Failed and the SoarError paths behave consistently with Download::verify_checksum().crates/soar-dl/src/download.rs (1)
271-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify existing files before returning from
Promptmode.The new checksum gate only covers
OverwriteMode::Skip. IfOverwriteMode::Prompthits the early return at Line 297,execute()can still report success for a pre-existing corrupted file without ever callingverify_checksum(), which is especially easy to hit becausePromptis the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-dl/src/download.rs` around lines 271 - 316, The Prompt branch returns early when the user declines overwrite without verifying the existing file; update the OverwriteMode::Prompt handling (in the block that calls prompt_overwrite(&output_path)) to mirror the Skip behavior: if resume_info.is_none() and the user declines, call self.verify_checksum(&output_path) and return Ok(output_path) only if it succeeds, otherwise log/warn, remove the corrupted file (fs::remove_file(&output_path)?), set resume_info = None and continue so the file will be re-downloaded; use the existing symbols prompt_overwrite, verify_checksum, output_path, resume_info and OverwriteMode::Prompt to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/soar-dl/src/oci.rs`:
- Around line 165-191: verify_layer_digest currently only runs in
download_layer_impl, letting cached-file fast paths (where metadata.len() ==
layer.size) and direct blob returns from execute()/download_blob() skip
integrity checks; update the code so that any path that accepts or returns a
file for a layer always calls verify_layer_digest(path, digest) before trusting
it. Concretely: after the metadata.len() == layer.size fast-return in the code
that handles cached layers, call verify_layer_digest and
propagate/remove-on-error as download_layer_impl does; similarly, ensure
execute() and download_blob() invoke verify_layer_digest for
ghcr.io/...@sha256:... blob downloads (and handle DigestMismatch by removing the
file and returning the same DownloadError). Touch the functions
verify_layer_digest, download_layer_impl, download_blob, and execute to add
these verification calls and consistent error handling.
In `@crates/soar-operations/src/install.rs`:
- Around line 767-778: The current gating logic allows checksum-less installs if
the repo has signature verification and a pubkey, but doesn't ensure any
signature artifacts exist; update the check in install.rs so has_signing also
verifies there is at least one signature file for this package (e.g., look for
"*.sig" in the package's install/extract directory or package metadata that
lists signatures) or call the same pre-check that verify_signatures() uses to
detect available sig files; change the condition that computes has_signing
(which currently uses config.get_repository(&pkg.repo_name) /
repo.signature_verification() / repo.pubkey) to also require presence of
signature artifacts for pkg (reference pkg.bsum, pkg.pkg_name, pkg.pkg_id and
the verify_signatures() behavior) and return the same refusal error if no
checksum and no actual signatures are present.
---
Outside diff comments:
In `@crates/soar-dl/src/download.rs`:
- Around line 271-316: The Prompt branch returns early when the user declines
overwrite without verifying the existing file; update the OverwriteMode::Prompt
handling (in the block that calls prompt_overwrite(&output_path)) to mirror the
Skip behavior: if resume_info.is_none() and the user declines, call
self.verify_checksum(&output_path) and return Ok(output_path) only if it
succeeds, otherwise log/warn, remove the corrupted file
(fs::remove_file(&output_path)?), set resume_info = None and continue so the
file will be re-downloaded; use the existing symbols prompt_overwrite,
verify_checksum, output_path, resume_info and OverwriteMode::Prompt to locate
where to change.
In `@crates/soar-operations/src/install.rs`:
- Around line 940-971: The checksum comparison in the install verification uses
case-sensitive equality on final_checksum and pkg.bsum causing false mismatches;
update the match/conditional logic around final_checksum and pkg.bsum (the arms
handling Some(calculated), Some(expected)) to compare case-insensitively (e.g.,
normalize both hex strings to the same case or use an ASCII-case-insensitive
comparison) so the VerifyStage::Passed/Failed and the SoarError paths behave
consistently with Download::verify_checksum().
🪄 Autofix (Beta)
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: 373b3e66-4ea3-44c0-b666-679fb3904dc3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/soar-core/src/package/install.rscrates/soar-dl/Cargo.tomlcrates/soar-dl/src/download.rscrates/soar-dl/src/error.rscrates/soar-dl/src/oci.rscrates/soar-operations/src/install.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/soar-operations/src/install.rs (1)
899-910:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
--no-verifyfor minisign checks too.Lines 899-910 still call
verify_signatures()even whenno_verifyis true, so a malformed or invalid.sigcan abort an install that the user explicitly asked not to verify. Gate this block on!no_verify, and just clean up.sigfiles when verification is disabled.Proposed fix
- if let Some(repository) = config.get_repository(&pkg.repo_name) { - if repository.signature_verification() { + if !no_verify { + if let Some(repository) = config.get_repository(&pkg.repo_name) { + if repository.signature_verification() { + events.emit(SoarEvent::Verifying { + op_id, + pkg_name: pkg.pkg_name.clone(), + pkg_id: pkg.pkg_id.clone(), + stage: VerifyStage::Signature, + }); + + if let Some(ref pubkey) = repository.pubkey { + verified_sig_count = verify_signatures(pubkey, &install_dir)?; + } else { + warn!( + "{}#{} - Signature verification skipped as no pubkey was found.", + pkg.pkg_name, pkg.pkg_id + ); + } + } else { + cleanup_sig_files(&install_dir); + } + } else { + cleanup_sig_files(&install_dir); + } - events.emit(SoarEvent::Verifying { - op_id, - pkg_name: pkg.pkg_name.clone(), - pkg_id: pkg.pkg_id.clone(), - stage: VerifyStage::Signature, - }); - - if let Some(ref pubkey) = repository.pubkey { - verified_sig_count = verify_signatures(pubkey, &install_dir)?; - } else { - warn!( - "{}#{} - Signature verification skipped as no pubkey was found.", - pkg.pkg_name, pkg.pkg_id - ); - } - } } else { - // Clean up .sig files for packages without signature verification cleanup_sig_files(&install_dir); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-operations/src/install.rs` around lines 899 - 910, The verify_signatures() call within the repository.signature_verification() branch must be skipped when the no_verify flag is set; update the conditional that currently reads around config.get_repository(&pkg.repo_name) / repository.signature_verification() to also require !no_verify before emitting SoarEvent::Verifying and invoking verify_signatures(pubkey, &install_dir), and when verification is disabled (no_verify true) ensure you remove any .sig files from install_dir as cleanup instead of attempting signature verification; reference repository.signature_verification(), verify_signatures(), no_verify, install_dir, and the SoarEvent::Verifying emission when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/soar-operations/src/install.rs`:
- Around line 922-927: The current check uses verified_sig_count to allow
checksum-less installs, but verify_signatures() increments for any .sig/file
pair and can pass even if the actual installed binary is unsigned; update the
logic so that the verification proves coverage for the package's actual
binaries: change verify_signatures() (and its callers around the blocks using
verified_sig_count) to return or populate the set/list of successfully verified
file paths (or a bool per expected file), then replace the boolean/count check
with a verification that all expected binary paths for pkg (e.g., the main
executable(s) referenced on pkg or pkg.files/pkg.primary_files) are present in
that verified set; apply the same fix to the other similar block around lines
1064-1123 so that installs without pkg.bsum are only allowed if every trusted
binary is individually signature-verified.
---
Outside diff comments:
In `@crates/soar-operations/src/install.rs`:
- Around line 899-910: The verify_signatures() call within the
repository.signature_verification() branch must be skipped when the no_verify
flag is set; update the conditional that currently reads around
config.get_repository(&pkg.repo_name) / repository.signature_verification() to
also require !no_verify before emitting SoarEvent::Verifying and invoking
verify_signatures(pubkey, &install_dir), and when verification is disabled
(no_verify true) ensure you remove any .sig files from install_dir as cleanup
instead of attempting signature verification; reference
repository.signature_verification(), verify_signatures(), no_verify,
install_dir, and the SoarEvent::Verifying emission when making this change.
🪄 Autofix (Beta)
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: f3aa7595-5e67-4cae-b14b-773293b80e6e
📒 Files selected for processing (3)
crates/soar-dl/src/oci.rscrates/soar-operations/src/install.rscrates/soar-utils/src/system.rs
✅ Files skipped from review due to trivial changes (1)
- crates/soar-utils/src/system.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/soar-dl/src/oci.rs
Comment on lines
+922
to
+927
| if !no_verify && pkg.bsum.is_none() && verified_sig_count == 0 { | ||
| return Err(SoarError::Custom(format!( | ||
| "Refusing to install {}#{}: no checksum and no valid signature found to verify integrity (use --no-verify to override)", | ||
| pkg.pkg_name, pkg.pkg_id | ||
| ))); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't accept checksum-less installs after verifying just any one file.
verified_sig_count > 0 is enough to pass the new integrity gate, but verify_signatures() increments for any .sig/file pair in install_dir. That means a package with one signed auxiliary file and an unsigned executable still satisfies the check. The verification result needs to prove coverage for the installed binary/binaries you actually trust, not just “at least one valid signature exists somewhere in the directory.”
Also applies to: 1064-1123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/soar-operations/src/install.rs` around lines 922 - 927, The current
check uses verified_sig_count to allow checksum-less installs, but
verify_signatures() increments for any .sig/file pair and can pass even if the
actual installed binary is unsigned; update the logic so that the verification
proves coverage for the package's actual binaries: change verify_signatures()
(and its callers around the blocks using verified_sig_count) to return or
populate the set/list of successfully verified file paths (or a bool per
expected file), then replace the boolean/count check with a verification that
all expected binary paths for pkg (e.g., the main executable(s) referenced on
pkg or pkg.files/pkg.primary_files) are present in that verified set; apply the
same fix to the other similar block around lines 1064-1123 so that installs
without pkg.bsum are only allowed if every trusted binary is individually
signature-verified.
Member
Author
There was a problem hiding this comment.
Technically usually only the main binary has sig so it's usually okay.
Fine for now. Will come back to it later.
Contributor
There was a problem hiding this comment.
@QaidVoid, understood — by convention only binary/executable files get .sig files, so verified_sig_count > 0 reliably means the main binary was verified. The existing long-term memory already captures this convention.
Would you like me to open a follow-up GitHub issue to track tightening the check in the future (e.g., verifying the specific binary paths rather than relying on the convention)?
🧠 Learnings used
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 168
File: crates/soar-operations/src/install.rs:922-927
Timestamp: 2026-06-03T07:26:18.534Z
Learning: In `crates/soar-operations/src/install.rs` (pkgforge/soar), the `verify_signatures()` function is safe to use as a binary-coverage check: by convention, only binary/executable files in a package directory have corresponding `.sig` files. All other files (`.png`, `.desktop`, `LICENSE`, `CHECKSUM`, etc.) exist without signatures. Therefore, `verified_sig_count > 0` reliably indicates that at least one binary was signature-verified, not merely any auxiliary file.
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 154
File: crates/soar-cli/src/install.rs:1096-1096
Timestamp: 2026-02-11T16:23:58.081Z
Learning: In crates/soar-cli/src/install.rs, FileLock uses only pkg_name (not a composite key with repo_name/pkg_id) intentionally. Different package variants with the same pkg_name but different pkg_id or repo_name will symlink to the same binary names in the bin directory. Locking at the pkg_name level prevents symlink race conditions during concurrent installations of different variants.
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 137
File: crates/soar-cli/src/apply.rs:149-153
Timestamp: 2026-01-09T16:14:21.516Z
Learning: In crates/soar-cli/src/apply.rs, for URL packages, passing non-installed records (is_installed=false) as existing_install is intentional. The query filters by repo_name, pkg_name, and pkg_id, ensuring the record is for the same package. This allows resuming partial installs and preserving settings (portable paths, profile) from previous installs.
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 157
File: crates/soar-operations/src/list.rs:111-117
Timestamp: 2026-02-14T06:08:23.011Z
Learning: In crates/soar-operations/src/list.rs, the `list_installed` function intentionally passes `None` for the `is_installed` parameter to `CoreRepository::list_filtered`. This is to include broken and partially installed packages in the results, allowing users to see packages that may be in an incomplete or broken state for diagnostics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements