security(install): verify lockfile tarball URLs#842
Conversation
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 📝 WalkthroughWalkthroughValidates lockfile-provided tarball URLs against registry dist.tarball during fetch. Adds ERR_AUBE_TARBALL_URL_MISMATCH (exit 34), integrates verification into the fetch path when a tarball_url_override is present, documents the error, and adds unit and integration tests asserting mismatch failures. ChangesTarball URL validation against registry metadata
Sequence DiagramsequenceDiagram
participant FetchTask
participant Validation
participant Registry
FetchTask->>Validation: verify_lockfile_tarball_url(lockfile_url, registry_name, version)
Validation->>Registry: fetch single-version metadata (name, version)
Registry-->>Validation: metadata with dist.tarball
Validation->>Validation: compare lockfile_url with dist.tarball (exact or npm mirror path)
alt URLs match
Validation-->>FetchTask: validation passes
else URLs mismatch or metadata lacks URL
Validation-->>FetchTask: error ERR_AUBE_TARBALL_URL_MISMATCH (redacted URLs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 370ef30. Configure here.
Greptile SummaryThis PR adds a supply-chain guard that fetches per-version registry metadata to cross-check any explicit tarball URL stored in the lockfile before allowing the download to proceed, failing with the new
Confidence Score: 5/5Safe to merge; the new verification gate is correctly wired to only execute on uncached packages with lockfile-pinned URLs, the exit-code ordering is correct, and the mirror-bypass logic is well-tested. The core logic is sound: tampered lockfile URLs are caught before any download, integrity checks still run after, and warm-cache installs are unaffected. The one observation about verification occupying a semaphore slot alongside the download is a throughput concern on large cold installs but does not affect correctness or security. No files require special attention; the suggestion to move verify_lockfile_tarball_url above sem.acquire() in fetch.rs is a performance improvement, not a blocker. Important Files Changed
Reviews (3): Last reviewed commit: "fix(install): allow npm lockfiles across..." | Re-trigger Greptile |
| miette!( | ||
| code = aube_codes::errors::ERR_AUBE_TARBALL_URL_MISMATCH, | ||
| "{}@{}: failed to verify lockfile tarball URL against registry metadata: {}", | ||
| registry_name, | ||
| version, | ||
| e | ||
| ) | ||
| })?; | ||
| let Some(expected_url) = meta.dist.as_ref().map(|dist| dist.tarball.as_str()) else { | ||
| return Err(miette!( |
There was a problem hiding this comment.
ERR_AUBE_TARBALL_URL_MISMATCH
When fetch_single_version_metadata fails for reasons unrelated to a URL mismatch — network timeout, DNS failure, HTTP 5xx, or even a 404 for the specific version — the error is wrapped and emitted as ERR_AUBE_TARBALL_URL_MISMATCH. The code's own description ("A registry lockfile entry's explicit tarball URL didn't match the registry metadata") only covers the true mismatch case; wrapping network errors under it tells the user their lockfile was tampered with when the real cause is a registry connectivity problem. This is particularly confusing in a security-sensitive code path where distinguishing "tampered lockfile" from "registry unreachable" matters. The map_err on the metadata fetch should use a network/registry error code (e.g. ERR_AUBE_REGISTRY_REQUEST_FAILED) or just propagate the upstream error unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube/src/commands/install/fetch.rs (1)
986-997: 💤 Low valueError mapping could be more specific, but fail-closed approach is security-appropriate.
All registry fetch errors (network failures, 404s, timeouts, etc.) are mapped to
ERR_AUBE_TARBALL_URL_MISMATCH. While the error message does include the underlying cause, a user facing a network outage might find it initially confusing to see "tarball URL mismatch" rather than "cannot reach registry."However, from a security perspective, this fail-closed approach is reasonable: if the system cannot verify the lockfile URL against registry metadata, blocking the install is the safer choice. The underlying error is preserved in the message chain, so diagnosing the root cause remains possible.
Consider whether a more specific error message like "failed to fetch registry metadata to verify lockfile tarball URL: {e}" would improve clarity without weakening the security posture.
🤖 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/aube/src/commands/install/fetch.rs` around lines 986 - 997, The error mapping for client.fetch_single_version_metadata currently always uses ERR_AUBE_TARBALL_URL_MISMATCH which can mislead users on network failures; update the map_err closure to preserve the same error code but change the message to something like "failed to fetch registry metadata to verify lockfile tarball URL: {}" while continuing to include the underlying error (e) and the registry_name/version context, so the call that produces meta remains fail-closed but has a clearer, more accurate top-level message.
🤖 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.
Nitpick comments:
In `@crates/aube/src/commands/install/fetch.rs`:
- Around line 986-997: The error mapping for
client.fetch_single_version_metadata currently always uses
ERR_AUBE_TARBALL_URL_MISMATCH which can mislead users on network failures;
update the map_err closure to preserve the same error code but change the
message to something like "failed to fetch registry metadata to verify lockfile
tarball URL: {}" while continuing to include the underlying error (e) and the
registry_name/version context, so the call that produces meta remains
fail-closed but has a clearer, more accurate top-level message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 449cf4cb-77c9-4d0b-8a48-a54868260c6e
📒 Files selected for processing (4)
crates/aube-codes/src/errors.rscrates/aube/src/commands/install/fetch.rsdocs/error-codes.data.jsontest/lockfile_settings.bats
|
Addressed review feedback in c243ee5:
Validation rerun:
This comment was generated by Codex. |
|
Addressed the failing BATS shards by allowing lockfile tarball URLs from the public npm registry to verify against a configured registry mirror when the tarball path matches exactly. Exact URL matching still passes, path tampering still fails, and arbitrary non-npm hosts do not get mirror leniency. Verification run:
One note: full This comment was generated by Codex. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube/src/commands/install/fetch.rs (1)
1110-1116: 💤 Low valueConsider adding a test case documenting mirror→npm rejection.
The current tests cover the npm→mirror direction but don't explicitly test the reverse (lockfile from mirror, expected from npm). While
rejects_mirror_match_from_arbitrary_hostpartially covers this, an explicit test would document that mirror leniency is intentionally one-directional:📝 Suggested additional test
#[test] fn tarball_url_match_rejects_mirror_lockfile_against_public_npm() { // Mirror leniency is one-directional: npm→mirror allowed, mirror→npm not. // A lockfile created against a mirror must match exactly when switching to npm. assert!(!lockfile_tarball_url_matches_metadata( "http://localhost:4873/is-odd/-/is-odd-3.0.1.tgz", "https://registry.npmjs.org/is-odd/-/is-odd-3.0.1.tgz", )); }🤖 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/aube/src/commands/install/fetch.rs` around lines 1110 - 1116, Add an explicit unit test that ensures mirror→npm URL matches are rejected by lockfile_tarball_url_matches_metadata; create a test (e.g., tarball_url_match_rejects_mirror_lockfile_against_public_npm) that asserts lockfile_tarball_url_matches_metadata("http://localhost:4873/...tgz", "https://registry.npmjs.org/...tgz") returns false, documenting that mirror leniency is one-directional (npm→mirror allowed, mirror→npm not) and referencing the existing function lockfile_tarball_url_matches_metadata to locate the matching logic.
🤖 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.
Nitpick comments:
In `@crates/aube/src/commands/install/fetch.rs`:
- Around line 1110-1116: Add an explicit unit test that ensures mirror→npm URL
matches are rejected by lockfile_tarball_url_matches_metadata; create a test
(e.g., tarball_url_match_rejects_mirror_lockfile_against_public_npm) that
asserts lockfile_tarball_url_matches_metadata("http://localhost:4873/...tgz",
"https://registry.npmjs.org/...tgz") returns false, documenting that mirror
leniency is one-directional (npm→mirror allowed, mirror→npm not) and referencing
the existing function lockfile_tarball_url_matches_metadata to locate the
matching logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e616b6c6-edec-41e2-ba47-963ba696bc23
📒 Files selected for processing (1)
crates/aube/src/commands/install/fetch.rs
Summary
Tests
This PR was generated by Codex.
Note
Medium Risk
Adds a supply-chain guard on the install fetch path with extra registry metadata calls when tarball URLs are pinned; behavior change for tampered lockfiles and npmjs.org mirror URL rules.
Overview
Lockfile tarball URL verification runs during install fetch when the lockfile pins an explicit registry tarball URL (e.g.
lockfile-include-tarball-url=trueor npm-alias entries). Before downloading, install fetches per-version registry metadata and comparesdist.tarballto the lockfile URL; mismatches abort withERR_AUBE_TARBALL_URL_MISMATCH(exit 34).Matching allows an exact string match, or—when the lockfile URL is on
registry.npmjs.org—the same/-/…tgzpath on another host (e.g. Verdaccio mirror) so local mirrors still work. Tampered paths or arbitrary hosts pretending to mirror npm are rejected.Docs and
docs/error-codes.data.jsoninclude the new code. A BATS test corrupts a lockfile tarball URL and expects frozen install to fail with the new error.Reviewed by Cursor Bugbot for commit 40a4064. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests
Documentation