Enforce SHA-256 package lock integrity#2
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces "Package lock integrity v2" for the mako package manager: deterministic SHA-256 content hashing of manifests and recursive ChangesPackage lock integrity v2
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Resolver
participant Lockfile
participant Filesystem
CLI->>Resolver: pkg install
Resolver->>Lockfile: read_lockfile / validate version
Resolver->>Lockfile: check locked_source_matches
Resolver->>Filesystem: fetch/resolve dependency if missing
Resolver->>Filesystem: compute content_hash
Resolver->>Lockfile: compare against locked content_hash
Lockfile-->>Resolver: mismatch error or success
Resolver-->>CLI: install result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
attahn
marked this pull request as ready for review
July 18, 2026 12:03
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/pkg.rs`:
- Around line 72-74: Update excluded_package_entry and its other call site so
exclusions for .git, .mako, target, and mako.lock apply only to entries directly
under the package root; preserve hashing for same-named nested source
directories such as src/target.
- Around line 248-256: Update the file-handling branch around full.is_file() to
include the sibling mako.toml manifest in the files collected for dependency
hashing, alongside the package file. Reuse the existing path normalization and
error-handling conventions, and preserve the current behavior for package
directories.
- Around line 592-596: Update the dependency-resolution flow around the
resolved-name early return so locked_source_matches is evaluated whenever
resolved already contains the package name, including update mode, before
returning the existing resolution. Preserve the current behavior for matching
sources and return an error for mismatched paths or git sources regardless of
queue order.
- Around line 240-258: Update hash_path_dep to inspect full with
symlink_metadata and reject symlinks before exists, is_file, or is_dir
traversal. Also add the same symlink_metadata validation in the registry-copy
flow around the child path handling near the referenced copy logic, rejecting
symlink entries before they are opened or copied so neither boundary
dereferences them.
- Around line 139-153: Update locked_source_matches and the lock-path validation
to reject absolute or otherwise non-relative paths after normalization. For path
dependencies, require the normalized relative path to match the expected path;
for git and registry dependencies, also require locked.path to equal the derived
.mako/deps location before any hashing or filesystem access. Ensure the hashing
flow uses only these validated paths.
- Around line 441-454: Update the lock-generation and restore flows around
normalized_lock_path, hash_path_dep, and the corresponding sections near the
other referenced locations so mutable git/registry caches are never treated as
authoritative. Online refreshes must fetch current source before hashing, while
restores must use the exact locked registry version and persisted resolved git
commit, including for floating refs.
🪄 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: f8f3f648-135f-440b-9f9b-9a709be6bbfe
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/pkg_manager/app/mako.lockis excluded by!**/*.lockmako.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
CHANGELOG.mdCargo.tomldocs/CLI.mddocs/GUIDE.mddocs/STDLIB.mddocs/book/src/ch10-packages.mddocs/book/src/ch13-tooling.mddocs/howto/04-packages.mdsrc/main.rssrc/pkg.rs
💤 Files with no reviewable changes (1)
- src/main.rs
Comment on lines
+72
to
+74
| fn excluded_package_entry(name: &str) -> bool { | ||
| matches!(name, ".git" | ".mako" | "target" | "mako.lock") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope local-state exclusions to the package root.
This basename check runs recursively, so src/target/module.mko and similar legitimate sources are never hashed. Restrict these exclusions to root-level local-state entries, or reject such source layouts explicitly.
Proposed adjustment
- if excluded_package_entry(&name) {
+ if dir == root && excluded_package_entry(&name) {
continue;
}Also applies to: 205-207
🤖 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 `@src/pkg.rs` around lines 72 - 74, Update excluded_package_entry and its other
call site so exclusions for .git, .mako, target, and mako.lock apply only to
entries directly under the package root; preserve hashing for same-named nested
source directories such as src/target.
Comment on lines
+139
to
+153
| fn locked_source_matches(dep: &ManifestDep, locked: &LockedPackage) -> Result<bool, String> { | ||
| if let Some(path) = &dep.path { | ||
| let expected_path = normalized_lock_path(Path::new(path))?; | ||
| return Ok(locked.source == "path" && locked.path.as_deref() == Some(&expected_path)); | ||
| } | ||
| if dep.git.is_some() { | ||
| return Ok(locked.source == "git" | ||
| && locked.git == dep.git | ||
| && locked.rev == dep.rev | ||
| && locked.tag == dep.tag | ||
| && locked.branch == dep.branch); | ||
| } | ||
| if dep.version.is_some() { | ||
| return Ok(locked.source == "registry"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject lock paths outside their expected source location.
Validation permits absolute paths, while git/registry source matching ignores locked.path. Line 609 can therefore hash an arbitrary filesystem location from a crafted lockfile. Require a normalized relative path and verify git/registry paths equal their derived .mako/deps location.
Also applies to: 775-780
🤖 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 `@src/pkg.rs` around lines 139 - 153, Update locked_source_matches and the
lock-path validation to reject absolute or otherwise non-relative paths after
normalization. For path dependencies, require the normalized relative path to
match the expected path; for git and registry dependencies, also require
locked.path to equal the derived .mako/deps location before any hashing or
filesystem access. Ensure the hashing flow uses only these validated paths.
Comment on lines
+240
to
+258
| fn hash_path_dep(full: &Path) -> Result<String, String> { | ||
| if !full.exists() { | ||
| return "missing".into(); | ||
| return Err(format!( | ||
| "cannot hash missing package path: {}", | ||
| full.display() | ||
| )); | ||
| } | ||
|
|
||
| let mut files = Vec::new(); | ||
| if full.is_file() { | ||
| return fs::read(full) | ||
| .map(|b| simple_hash(&b)) | ||
| .unwrap_or_else(|_| "missing".into()); | ||
| } | ||
| let mut buf = Vec::new(); | ||
| let manifest = full.join("mako.toml"); | ||
| if let Ok(b) = fs::read(&manifest) { | ||
| buf.extend_from_slice(&b); | ||
| } | ||
| let mut mko: Vec<_> = fs::read_dir(full) | ||
| .into_iter() | ||
| .flatten() | ||
| .filter_map(|e| e.ok()) | ||
| .map(|e| e.path()) | ||
| .filter(|p| p.extension().and_then(|x| x.to_str()) == Some("mko")) | ||
| .collect(); | ||
| mko.sort(); | ||
| for p in mko { | ||
| if let Ok(b) = fs::read(&p) { | ||
| buf.extend_from_slice(p.to_string_lossy().as_bytes()); | ||
| buf.extend_from_slice(&b); | ||
| } | ||
| } | ||
| if buf.is_empty() { | ||
| "empty".into() | ||
| let name = full | ||
| .file_name() | ||
| .ok_or_else(|| format!("package file has no name: {}", full.display()))?; | ||
| files.push(( | ||
| normalized_relative_path(Path::new(name))?, | ||
| full.to_path_buf(), | ||
| )); | ||
| } else if full.is_dir() { | ||
| collect_package_files(full, full, &mut files)?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject symlinks before they are dereferenced.
hash_path_dep follows a symlink supplied as full. Registry copies also dereference child symlinks in Lines 502-513, converting them to regular cached files before hashing. Check symlink_metadata at both boundaries and fail before traversal or copying.
Also applies to: 480-493
🤖 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 `@src/pkg.rs` around lines 240 - 258, Update hash_path_dep to inspect full with
symlink_metadata and reject symlinks before exists, is_file, or is_dir
traversal. Also add the same symlink_metadata validation in the registry-copy
flow around the child path handling near the referenced copy logic, rejecting
symlink entries before they are opened or copied so neither boundary
dereferences them.
Comment on lines
+248
to
+256
| let mut files = Vec::new(); | ||
| if full.is_file() { | ||
| return fs::read(full) | ||
| .map(|b| simple_hash(&b)) | ||
| .unwrap_or_else(|_| "missing".into()); | ||
| } | ||
| let mut buf = Vec::new(); | ||
| let manifest = full.join("mako.toml"); | ||
| if let Ok(b) = fs::read(&manifest) { | ||
| buf.extend_from_slice(&b); | ||
| } | ||
| let mut mko: Vec<_> = fs::read_dir(full) | ||
| .into_iter() | ||
| .flatten() | ||
| .filter_map(|e| e.ok()) | ||
| .map(|e| e.path()) | ||
| .filter(|p| p.extension().and_then(|x| x.to_str()) == Some("mko")) | ||
| .collect(); | ||
| mko.sort(); | ||
| for p in mko { | ||
| if let Ok(b) = fs::read(&p) { | ||
| buf.extend_from_slice(p.to_string_lossy().as_bytes()); | ||
| buf.extend_from_slice(&b); | ||
| } | ||
| } | ||
| if buf.is_empty() { | ||
| "empty".into() | ||
| let name = full | ||
| .file_name() | ||
| .ok_or_else(|| format!("package file has no name: {}", full.display()))?; | ||
| files.push(( | ||
| normalized_relative_path(Path::new(name))?, | ||
| full.to_path_buf(), | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the manifest when hashing file dependencies.
For full.is_file(), only that file is hashed, although its parent mako.toml controls version and transitive dependencies. Manifest changes therefore bypass integrity verification. Hash the sibling manifest too, or require package paths to be directories.
🤖 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 `@src/pkg.rs` around lines 248 - 256, Update the file-handling branch around
full.is_file() to include the sibling mako.toml manifest in the files collected
for dependency hashing, alongside the package file. Reuse the existing path
normalization and error-handling conventions, and preserve the current behavior
for package directories.
Comment on lines
+441
to
+454
| path: Some(normalized_lock_path(dest.strip_prefix(project).map_err( | ||
| |_| { | ||
| format!( | ||
| "git dependency cache {} is outside project {}", | ||
| dest.display(), | ||
| project.display() | ||
| ) | ||
| }, | ||
| )?)?), | ||
| git: dep.git.clone(), | ||
| rev: dep.rev.clone(), | ||
| tag: dep.tag.clone(), | ||
| branch: dep.branch.clone(), | ||
| content_hash: hash_path_dep(&dest), | ||
| content_hash: hash_path_dep(&dest)?, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not treat mutable caches as authoritative package state.
Online updates currently hash existing git/registry caches, blessing stale or tampered content. Conversely, restoring a missing cache resolves the current manifest range rather than the locked version; floating git refs also lack a resolved commit. Refresh updates from source, but restore installs using the exact registry version and persisted git commit.
Also applies to: 480-493, 618-622
🤖 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 `@src/pkg.rs` around lines 441 - 454, Update the lock-generation and restore
flows around normalized_lock_path, hash_path_dep, and the corresponding sections
near the other referenced locations so mutable git/registry caches are never
treated as authoritative. Online refreshes must fetch current source before
hashing, while restores must use the exact locked registry version and persisted
resolved git commit, including for floating refs.
Comment on lines
+592
to
+596
| if !locked_source_matches(&dep, lp)? { | ||
| return Err(format!( | ||
| "lockfile source for `{key}` does not match mako.toml — run `mako pkg update`" | ||
| )); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Check source consistency before the already-resolved early return.
Lines 569-585 bypass this check for later occurrences of the same package name. Compatible versions from different paths or git sources are therefore silently collapsed depending on queue order. Apply locked_source_matches whenever resolved already contains the name, including update mode.
🤖 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 `@src/pkg.rs` around lines 592 - 596, Update the dependency-resolution flow
around the resolved-name early return so locked_source_matches is evaluated
whenever resolved already contains the package name, including update mode,
before returning the existing resolution. Preserve the current behavior for
matching sources and return an error for mismatched paths or git sources
regardless of queue order.
This was referenced Jul 18, 2026
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
2 participants
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.
Replaces package lock content hashes with deterministic SHA-256 and verifies locked dependency source metadata and content before reuse.
Malformed lockfiles, unsafe paths, symbolic links, and unreadable transitive manifests now fail closed.
Tested with:
cargo test --bin mako(passed)cargo run --release -- test examples/testing(338 passed)Summary by CodeRabbit
New Features
mako pkg installnow verifies locked dependency content and fails on changes, missing files, unreadable manifests, or malformed lockfiles.mako pkg updaterefreshes dependencies, accepts intentional changes, and migrates legacy lockfiles.Documentation