• Create PR with unit tests
• Commit unit tests in branch pnpr-sec

Description

pick_from_registry_with_guard() records blocked versions using the parsed manifest version string,
but pick_package() filters by packument version-map keys; if those differ, the rejected candidate is
never excluded and the loop can error with AllVersionsBlockedError even though other acceptable
versions exist.

Code

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[R565-584]

+        let version_str = version.version.to_string();
+        match guard.check(&opts.spec.name, &version_str).await? {
+            PackageVersionGuardDecision::Allow => {
+                return Ok(Some(PickedFromRegistry { meta: pick_result.meta, version }));
+            }
+            PackageVersionGuardDecision::Reject { reason } => {
+                tracing::debug!(
+                    target: "pacquet_resolving_npm_resolver",
+                    name = %opts.spec.name,
+                    version = %version_str,
+                    reason = %reason,
+                    "package version rejected by resolver guard",
+                );
+                // A `false` return means the picker re-selected a version we
+                // already blocked, so filtering can't exclude it (e.g. the
+                // parsed version string doesn't match the packument key).
+                // Stop rather than loop forever — every match is blocked.
+                if !blocked_versions.insert(version_str) {
+                    return Err(all_versions_blocked(opts.spec, reason));
+                }

Evidence

The guard loop blocks version.version.to_string() values, but the filtering logic compares against
raw packument version strings (map keys). Since manifest hydration does not validate
key↔manifest-version consistency, a registry can supply mismatched key/version and defeat the
filter, causing repeated selection of the same rejected candidate and a false “all blocked” outcome.

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[527-586]
pacquet/crates/resolving-npm-resolver/src/pick_package.rs[764-789]
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[320-333]
pacquet/crates/registry/src/package_versions.rs[117-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The guard repick loop stores `blocked_versions` as `PackageVersion.version.to_string()`, but filtering is done against packument `versions` map keys. If the registry serves inconsistent metadata (e.g., key "1.0.0+build" with manifest field `version: "1.0.0"`), the rejected candidate won't be filtered out and the resolver can repeatedly re-select it and then raise `AllVersionsBlockedError`, incorrectly failing resolution.
## Issue Context
- The version chooser operates on packument version keys.
- Hydration parses the manifest independently and does not enforce that the parsed `manifest.version` matches the packument key.
- pnpr’s OSV feature enables this guard in a server context where the client controls registry URLs, so a malicious registry response can trigger this denial-of-service / incorrect failure.
## Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[527-588]
- pacquet/crates/resolving-npm-resolver/src/pick_package.rs[764-789]
- pacquet/crates/registry/src/package_versions.rs[117-145]
## Suggested fix approach
1. Change the picker API to return the *selected packument version key* alongside the hydrated `Arc<PackageVersion>` (e.g., `picked_version_key: String`).
2. Store that key in `blocked_versions` (or store both key and canonical string), so the next pick reliably excludes the rejected candidate.
3. Add a regression test using a packument where the `versions` key and manifest `version` differ, ensuring the resolver repicks another allowed version instead of returning `AllVersionsBlockedError`.
(Alternative if API change is too invasive: on rejection, fall back to additionally blocking the packument key by locating it via pointer equality (`Arc::ptr_eq`) against hydrated manifests—only on mismatch paths—to guarantee progress without changing the public picker signature.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

For tarball resolutions, OSV checks look up vulnerabilities using the lockfile key’s name@version
without cross-checking the tarball URL that will actually be fetched; when trustLockfile is set, a
tampered lockfile can claim a safe name@version while pointing resolution.tarball at a different
(vulnerable) artifact, bypassing OSV enforcement. This is a trust-boundary mismatch: the URL is the
artifact identity for fetch, but the OSV gate uses only the key identity.

Code

pnpr/crates/pnpr/src/resolver.rs[R601-647]

+fn osv_violations_for_lockfile(index: &OsvIndex, lockfile: &Lockfile) -> Vec<serde_json::Value> {
+    let Some(packages) = lockfile.packages.as_ref() else {
+        return Vec::new();
+    };
+
+    let mut seen = std::collections::HashSet::new();
+    let mut violations = Vec::new();
+    for (package_key, snapshot) in packages {
+        if !is_osv_checkable_resolution(&snapshot.resolution) {
+            continue;
+        }
+        let name = package_key.name.to_string();
+        let version = package_key.suffix.version().to_string();
+        let ids = index.vulnerability_ids(&name, &version);
+        if ids.is_empty() {
+            continue;
+        }
+        // Dedup only the rare vulnerable hits — several lockfile keys can
+        // share one name@version via peer suffixes — so the common
+        // (non-vulnerable) entry never pays for the set.
+        if !seen.insert((name.clone(), version.clone())) {
+            continue;
+        }
+        violations.push(serde_json::json!({
+            "name": name,
+            "version": version,
+            "code": OSV_VULNERABILITY_CODE,
+            "reason": format!(
+                "is listed in the local OSV database as vulnerable ({})",
+                ids.join(", "),
+            ),
+        }));
+    }
+    violations
+}
+
+fn is_osv_checkable_resolution(resolution: &LockfileResolution) -> bool {
+    match resolution {
+        LockfileResolution::Registry(_) => true,
+        // A frozen lockfile is attacker-controlled, so gate on the tarball
+        // URL rather than the tamper-prone `git_hosted` flag or strict URL
+        // parsing — otherwise `gitHosted: true` or a barely-malformed URL
+        // would let a vulnerable package opt out of the OSV scan. Mirrors
+        // the npm verifier's URL-based gate.
+        LockfileResolution::Tarball(tarball) => {
+            is_http_tarball_url(&tarball.tarball) && !is_git_hosted_tarball_url(&tarball.tarball)
+        }

Evidence

The OSV scan chooses the identity from the lockfile key (name/version), while tarball fetching uses
the tarball URL directly for tarball resolutions, so a mismatch can cause OSV to check the wrong
identity when lockfile verification is skipped via trustLockfile.

pnpr/crates/pnpr/src/resolver.rs[601-647]
pacquet/crates/package-manager/src/install_package_by_snapshot.rs[589-613]
pacquet/crates/lockfile/src/resolution.rs[7-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OSV enforcement for lockfile tarball entries uses `package_key.name` + `package_key.suffix.version()` as the vulnerability identity, but tarball fetch identity is the tarball URL. Under `trustLockfile`, a malicious/tampered lockfile can spoof `name@version` while pointing `resolution.tarball` at a different artifact, causing OSV checks to assess the wrong identity.
### Issue Context
- `osv_violations_for_lockfile()` computes `(name, version)` from the lockfile key and never validates it against `TarballResolution.tarball`.
- For tarball resolutions, the fetch path uses the tarball URL directly, independent of the lockfile key.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver.rs[601-653]
### Suggested fix
- For `LockfileResolution::Tarball` entries that are considered OSV-checkable (http(s) and not git-hosted), add a best-effort extraction of the version (and optionally bare package name) from the tarball URL’s last path segment (strip query/fragment, handle basic percent-encoding, avoid strict URL parsing).
- If a version can be extracted and it *differs* from `package_key.suffix.version()`, treat this as a policy violation (distinct code) or force OSV lookup using the extracted version (and include a mismatch violation) so the OSV gate can’t be bypassed by lying in the lockfile key.
- If extraction fails (nonstandard tarball naming), fall back to current behavior to avoid breaking legitimate registries, but consider emitting a debug log so operators can detect unverifiable tarball identities.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

is_osv_checkable_resolution() skips Tarball entries when tarball.git_hosted == Some(true) and
when reqwest::Url::parse() fails, so OSV enforcement can be suppressed by lockfile-controlled
fields/strings rather than actual dependency identity. This is security-relevant because
fresh_frozen_input_lockfile() can return the client-supplied lockfile unchanged, making the OSV
scan the primary new gate on that path.

Code

pnpr/crates/pnpr/src/resolver.rs[R633-649]

+fn is_osv_checkable_resolution(resolution: &LockfileResolution) -> bool {
+    match resolution {
+        LockfileResolution::Registry(_) => true,
+        LockfileResolution::Tarball(tarball) => {
+            if tarball.git_hosted == Some(true) {
+                return false;
+            }
+            reqwest::Url::parse(&tarball.tarball).is_ok_and(|url| {
+                let scheme = url.scheme();
+                scheme == "http" || scheme == "https"
+            })
+        }
+        LockfileResolution::Directory(_)
+        | LockfileResolution::Git(_)
+        | LockfileResolution::Binary(_)
+        | LockfileResolution::Variations(_) => false,
+    }

Evidence

The OSV scan’s predicate explicitly trusts git_hosted and strict URL parsing to decide whether to
scan a lockfile entry, while other parts of the repo document that gitHosted is tamper-prone and
gate trust using URL-prefix rules instead. pnpr’s resolve handler uses this OSV scan to gate the
frozen-lockfile short-circuit, which can return the client-supplied lockfile unchanged, so skipping
scan directly weakens OSV enforcement on that path.

pnpr/crates/pnpr/src/resolver.rs[261-273]
pnpr/crates/pnpr/src/resolver.rs[633-649]
pnpr/crates/pnpr/src/resolver/resolve.rs[208-273]
pacquet/crates/lockfile/src/resolution.rs[446-461]
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs[863-888]
REVIEW_GUIDE.md[30-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
pnpr’s OSV lockfile scan currently decides whether a `LockfileResolution::Tarball` is OSV-checkable by trusting (1) the lockfile’s `tarball.git_hosted` boolean and (2) successful `reqwest::Url::parse()`.
Because lockfiles are attacker-controlled input (per repo review rules) and pnpr has a frozen-lockfile short-circuit that can accept the client lockfile largely as-is, an attacker can craft a lockfile that suppresses OSV scanning by setting `gitHosted: true` or providing a tarball URL string that fails URL parsing.
## Issue Context
Other security-sensitive code in this repo explicitly treats `gitHosted` as tamper-prone and gates on URL patterns instead (see `pacquet_lockfile::is_git_hosted_tarball_url`). The npm verifier also does not require `Url::parse()` success to keep an entry checkable.
## Fix Focus Areas
- pnpr/crates/pnpr/src/resolver.rs[633-649]
## Suggested fix
- Stop using `tarball.git_hosted` as an exclusion for OSV scanning (or only treat it as a *hint*).
- Avoid requiring `Url::parse()` success. Prefer a cheap, case-insensitive scheme prefix check (`http://` or `https://`) similar to existing verifier code.
- If you need to exclude git-hosted tarballs from OSV checks, base it on `pacquet_lockfile::is_git_hosted_tarball_url(&tarball.tarball)` (which exists specifically to avoid trusting the tamperable flag), not the lockfile-provided `git_hosted` field.
Example direction:
- `Tarball` is OSV-checkable if it is http(s) **and** `!is_git_hosted_tarball_url(url)`.
Add/adjust tests to cover:
- a tarball with `gitHosted: true` but a non-git-hosted https URL still being scanned
- a tarball URL that is not strictly parseable by `Url::parse()` but still starts with `https://` being scanned

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

SemverRange::affects applies OSV range events sequentially and is order-sensitive, but events are
ingested in file order without validation or normalization. A malformed/tampered local OSV dump can
reorder events to yield false negatives (allowing vulnerable versions) or false positives (blocking
safe versions) when OSV is enabled.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R152-180]

+impl SemverRange {
+    fn affects(&self, version: &Version) -> bool {
+        let mut affected = false;
+        for event in &self.events {
+            match event {
+                SemverEvent::Introduced(introduced) => {
+                    if version >= introduced {
+                        affected = true;
+                    }
+                }
+                SemverEvent::Fixed(fixed) => {
+                    if version >= fixed {
+                        affected = false;
+                    }
+                }
+                SemverEvent::LastAffected(last_affected) => {
+                    if version > last_affected {
+                        affected = false;
+                    }
+                }
+                SemverEvent::Limit(limit) => {
+                    if version >= limit {
+                        affected = false;
+                    }
+                }
+            }
+        }
+        affected
+    }

Evidence

Range events are evaluated in sequence (toggling affected), so the outcome depends on event order;
however, events are collected directly from the OSV JSON array without ordering validation/sorting.
That makes the vulnerability decision sensitive to malformed or tampered OSV records.

pnpr/crates/pnpr/src/resolver/osv.rs[152-180]
pnpr/crates/pnpr/src/resolver/osv.rs[397-403]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OSV SEMVER/ECOSYSTEM range evaluation is order-dependent (`affected` toggles as events are processed), but the loader accepts OSV event arrays as-is. If an OSV record’s events are out of canonical order, vulnerability decisions can be wrong.
### Issue Context
- `semver_range_from_osv` currently collects parsed events without sorting/validating ordering.
- `SemverRange::affects` assumes events are in the correct sequence.
### Fix Focus Areas
- Ensure event ordering is deterministic and correct for evaluation, e.g.:
- validate events are monotonic (by bound version) and fail fast on disorder; or
- sort events by bound version with a defined tie-breaker (e.g. Introduced before Fixed at same version), and optionally emit a warning when normalization changes order.
- Consider including advisory id / package name in warnings/errors to aid debugging.
### Fix Focus Areas (paths/lines)
- pnpr/crates/pnpr/src/resolver/osv.rs[152-180]
- pnpr/crates/pnpr/src/resolver/osv.rs[397-403]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

load_from_directory() fingerprints the OSV DB by hashing file_name bytes and record bytes
back-to-back without any delimiter/length prefix, so different directory states can produce the same
fingerprint input stream. pnpr uses that fingerprint in can_trust_policy() to accept cached
lockfile-verdict passes, so if the OSV directory is writable by an attacker they can change advisory
contents while preserving the fingerprint and keep stale cache hits trusted.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R288-329]

+fn load_from_directory(path: &Path) -> Result<OsvIndex, RegistryError> {
+    let mut entries = std::fs::read_dir(path)
+        .map_err(|err| {
+            invalid_config(format!("failed to read OSV directory {}: {err}", path.display()))
+        })?
+        .collect::<Result<Vec<_>, _>>()
+        .map_err(|err| {
+            invalid_config(format!("failed to read OSV directory {}: {err}", path.display()))
+        })?;
+    entries.sort_by_key(std::fs::DirEntry::path);
+
+    let mut packages = HashMap::new();
+    let mut hasher = Sha256::new();
+    for entry in entries {
+        let entry_path = entry.path();
+        if !entry_path.is_file()
+            || entry_path.extension().is_none_or(|extension| extension != "json")
+        {
+            continue;
+        }
+        hasher.update(entry_path.file_name().and_then(|name| name.to_str()).unwrap_or_default());
+        // Bound the read itself with `take` rather than trusting a
+        // pre-read `metadata().len()`, which a concurrent swap could
+        // invalidate (TOCTOU) — matching the zip loader's guarantee.
+        let file = File::open(&entry_path).map_err(|err| {
+            invalid_config(format!("failed to read OSV record {}: {err}", entry_path.display()))
+        })?;
+        let mut bytes = Vec::new();
+        file.take(MAX_OSV_RECORD_BYTES + 1).read_to_end(&mut bytes).map_err(|err| {
+            invalid_config(format!("failed to read OSV record {}: {err}", entry_path.display()))
+        })?;
+        if bytes.len() as u64 > MAX_OSV_RECORD_BYTES {
+            return Err(invalid_config(format!(
+                "OSV record {} is over the {MAX_OSV_RECORD_BYTES}-byte per-record limit",
+                entry_path.display(),
+            )));
+        }
+        hasher.update(&bytes);
+        ingest_record_bytes(&mut packages, &bytes)?;
+    }
+    Ok(OsvIndex { packages, fingerprint: format!("sha256:{:x}", hasher.finalize()) })
+}

Evidence

The directory fingerprint currently hashes an ambiguous concatenation of file-name bytes and file
bytes; that fingerprint is then used as a trusted policy marker for lockfile-verdict cache hits, so
ambiguity can keep cached passes trusted under changed OSV data.

pnpr/crates/pnpr/src/resolver/osv.rs[288-329]
pnpr/crates/pnpr/src/resolver/osv.rs[82-88]
pnpr/crates/pnpr/src/resolver.rs[738-745]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pnpr` computes an OSV *directory* fingerprint by hashing `filename` and then raw file bytes for each record with no delimiter/length prefix. This makes the hashed byte stream ambiguous (it’s just concatenation), so two different directory states can produce the same hash input without breaking SHA-256. That fingerprint is later used as a policy key for the lockfile-verdict cache; a collision/ambiguity can cause pnpr to incorrectly trust cached “clean” results under changed vulnerability data.
Concrete example of ambiguity (same concatenated stream):
- State A: file `"a"` with bytes `"bc"`, then file `"d"` with bytes `"e"` → stream `a bc d e`
- State B: file `"ab"` with bytes `"c"`, then file `"d"` with bytes `"e"` → stream `ab c d e`
## Issue Context
This only matters when the OSV DB directory is attacker-writable (e.g., shared cache dir, multi-tenant host, or other local-write threat model), but in that model it weakens the intended safety property: “OSV DB change ⇒ new fingerprint ⇒ cached verdicts invalidated.”
## Fix Focus Areas
- pnpr/crates/pnpr/src/resolver/osv.rs[288-329]
- pnpr/crates/pnpr/src/resolver/osv.rs[82-88]
- pnpr/crates/pnpr/src/resolver.rs[738-755]
## Suggested fix
When hashing each record, incorporate structure so boundaries are unambiguous:
- hash a fixed tag for each component (e.g., `b"name\0"`, `b"bytes\0"`), **or**
- hash length-prefixed data (e.g., `u64::to_le_bytes(name_len)` + `name_bytes` + `u64::to_le_bytes(bytes_len)` + `bytes`), **and**
- consider hashing the full relative path (or at least include a separator) rather than `file_name()`.
This keeps fingerprints collision-resistant *at the encoding level* and preserves the cache invalidation guarantee.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

OSV SEMVER/ECOSYSTEM range events with unparsable versions are silently dropped, which can cause
ranges (or whole advisories) to be ignored and produce false negatives when OSV is enabled. A
corrupted/tampered local OSV dump can therefore selectively disable vulnerability blocking while
pnpr still starts because the npm index remains non-empty.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R392-413]

+fn semver_event_from_osv(event: OsvEvent) -> Option<SemverEvent> {
+    if let Some(introduced) = event.introduced {
+        return parse_osv_version(&introduced).map(SemverEvent::Introduced);
+    }
+    if let Some(fixed) = event.fixed {
+        return parse_osv_version(&fixed).map(SemverEvent::Fixed);
+    }
+    if let Some(last_affected) = event.last_affected {
+        return parse_osv_version(&last_affected).map(SemverEvent::LastAffected);
+    }
+    if let Some(limit) = event.limit {
+        return parse_osv_version(&limit).map(SemverEvent::Limit);
+    }
+    None
+}
+
+fn parse_osv_version(raw: &str) -> Option<Version> {
+    if raw == "0" {
+        return Version::parse("0.0.0").ok();
+    }
+    Version::parse(raw).ok()
+}

Evidence

The ingestion path skips advisories that end up with empty versions and ranges, while range
events are built via filter_map that drops unparsable versions; together this proves malformed
semver can silently remove advisory coverage and lead to missed vulnerability matches.

pnpr/crates/pnpr/src/resolver/osv.rs[342-363]
pnpr/crates/pnpr/src/resolver/osv.rs[384-413]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OSV semver range parsing currently treats an unparsable event version as “no event” and drops it. If a range becomes empty (or an advisory ends up with neither explicit `versions` nor parsed `ranges`), the advisory is silently skipped, weakening enforcement.
### Issue Context
This OSV index is a security control when `osv.enabled` is true. Silent acceptance of malformed semver data can turn real advisories into non-enforced entries without any startup failure or explicit warning.
### Fix Focus Areas
- Make semver parsing of `SEMVER`/`ECOSYSTEM` ranges strict when OSV is enabled: if an event field is present but cannot be parsed (except the special-case `"0"`), treat the OSV DB as invalid and fail startup.
- If strict failure is too aggressive, at least record and surface validation errors (warn + count, and optionally refuse to start when any are observed) so enforcement can’t silently degrade.
- pnpr/crates/pnpr/src/resolver/osv.rs[342-363]
- pnpr/crates/pnpr/src/resolver/osv.rs[384-413]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

OSV record ingestion treats any present withdrawn field as withdrawn, even if it is JSON null,
which can incorrectly skip active advisories and allow vulnerable versions to be treated as safe.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R348-352]

+    let record: OsvRecord = serde_json::from_slice(bytes)
+        .map_err(|err| invalid_config(format!("failed to parse OSV record: {err}")))?;
+    if record.withdrawn.is_some() {
+        return Ok(());
+    }

Evidence

withdrawn is an Option, so a JSON null is still Some(...). The ingestion logic then uses
is_some() to decide to skip the record, which incorrectly treats withdrawn: null as withdrawn.

pnpr/crates/pnpr/src/resolver/osv.rs[191-199]
pnpr/crates/pnpr/src/resolver/osv.rs[344-352]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ingest_record_bytes` skips an OSV record whenever `withdrawn` deserializes as `Some(_)`. Because `withdrawn` is typed as `Option<serde_json::Value>`, an explicit JSON `null` becomes `Some(Value::Null)`, causing the record to be dropped even though it is not meaningfully withdrawn.
### Issue Context
This code gates resolver-time blocking and lockfile verification when OSV is enabled. Incorrectly skipping advisories weakens the protection.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver/osv.rs[191-199]
- pnpr/crates/pnpr/src/resolver/osv.rs[344-366]
### Suggested fix
- Change the check to only skip when `withdrawn` is present *and non-null*, e.g.:
- `if record.withdrawn.as_ref().is_some_and(|v| !v.is_null()) { ... }`
- Or model it more strictly (preferred): `withdrawn: Option<String>` (or `Option<DateTime<...>>`) and skip only on `Some(_)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

load_from_zip() hashes the OSV zip via one open and then re-opens the path to parse it; if the DB
is replaced between those operations, the in-memory index can contain advisories from different
bytes than the stored fingerprint. Because the verdict cache trusts/records this fingerprint, a
startup-time swap can make pnpr incorrectly trust cached “clean” lockfile verdicts under a changed
vulnerability dataset.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R234-241]

+fn load_from_zip(path: &Path) -> Result<OsvIndex, RegistryError> {
+    let fingerprint = file_fingerprint(path)?;
+    let file = File::open(path).map_err(|err| {
+        invalid_config(format!("failed to open OSV database {}: {err}", path.display()))
+    })?;
+    let mut archive = zip::ZipArchive::new(file).map_err(|err| {
+        invalid_config(format!("failed to read OSV zip {}: {err}", path.display()))
+    })?;

Evidence

The zip loader fingerprints the OSV DB via file_fingerprint(path) and then re-opens path for
ZipArchive, so the fingerprint can be computed from different bytes than the parsed archive if the
file is swapped between opens; this fingerprint is then used to trust/record verdict-cache policy.

pnpr/crates/pnpr/src/resolver/osv.rs[234-266]
pnpr/crates/pnpr/src/resolver/osv.rs[311-330]
pnpr/crates/pnpr/src/resolver.rs[737-755]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`load_from_zip()` computes `fingerprint` from a separate file open than the one used to build `ZipArchive`. If the OSV zip is atomically replaced between these two opens, `OsvIndex.fingerprint` can describe different bytes than the advisories actually loaded. Since this fingerprint is used in `can_trust_policy()` and is recorded into the verdict cache policy snapshot, a mismatch can cause cached lockfile verifications to be trusted under the wrong vulnerability dataset.
### Issue Context
- OSV zip path updates are plausibly done via atomic rename/replace (common for DB refresh jobs).
- Verdict-cache trust explicitly depends on `OsvIndex::can_trust_policy()`.
### Fix
Open the zip file **once**, compute the fingerprint from that same handle (read/seek back to start), then pass the same handle into `ZipArchive::new(...)`. This removes the TOCTOU window where the path can be swapped between fingerprinting and parsing.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver/osv.rs[234-266]
- pnpr/crates/pnpr/src/resolver/osv.rs[311-330]
- pnpr/crates/pnpr/src/resolver.rs[737-755]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

When osv.enabled is set, OsvIndex::load_from_config only checks path.exists() and then treats
any non-directory path as a zip, so pointing osv.path at a FIFO/device/socket can block
indefinitely during File::open() / file_fingerprint() reads at startup. This is a startup-time
DoS surface (and a severe reliability footgun) because pnpr loads the OSV DB before binding the
listen socket.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R33-61]

+    pub(crate) fn load_from_config(config: &Config) -> Result<Option<Arc<Self>>, RegistryError> {
+        if !config.osv.enabled {
+            return Ok(None);
+        }
+        let path = config.osv.path.clone().unwrap_or_else(|| default_osv_path(config));
+        if !path.exists() {
+            return Err(invalid_config(format!(
+                "OSV is enabled but database {} does not exist; download the npm OSV dump to this path or set osv.path",
+                path.display(),
+            )));
+        }
+        let index = Self::load_from_path(&path)?;
+        // An enabled-but-empty index would make the guard a no-op and
+        // silently disable enforcement, so treat it as misconfiguration.
+        if index.packages.is_empty() {
+            return Err(invalid_config(format!(
+                "OSV is enabled but database {} yielded no npm advisories; check that the path points at the npm OSV dump",
+                path.display(),
+            )));
+        }
+        Ok(Some(Arc::new(index)))
+    }
+
+    pub(crate) fn load_from_path(path: &Path) -> Result<Self, RegistryError> {
+        if path.is_dir() {
+            return load_from_directory(path);
+        }
+        load_from_zip(path)
+    }

Evidence

The OSV loader gates only on exists() and then reads the path as a zip when it is not a directory;
the fingerprinting helper reads until EOF, which can block on FIFOs/special files. pnpr invokes this
loader during startup before binding the listener, so a block prevents service availability.

pnpr/crates/pnpr/src/resolver/osv.rs[33-61]
pnpr/crates/pnpr/src/resolver/osv.rs[234-266]
pnpr/crates/pnpr/src/resolver/osv.rs[311-330]
pnpr/crates/pnpr/src/server.rs[270-282]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When OSV is enabled, the configured `osv.path` is only validated with `Path::exists()`. If it points to a special file (FIFO, device node, socket) or otherwise non-regular path, `File::open()` and/or the subsequent read loop used for fingerprinting may block indefinitely, preventing pnpr from starting.
### Issue Context
pnpr loads the OSV database at startup (before binding the server socket). The loader currently treats any existing non-directory path as a zip and streams it to EOF for fingerprinting.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver/osv.rs[33-61]
- pnpr/crates/pnpr/src/resolver/osv.rs[234-266]
- pnpr/crates/pnpr/src/resolver/osv.rs[311-330]
- pnpr/crates/pnpr/src/server.rs[270-282]
### Implementation notes
- In `OsvIndex::load_from_path` (or earlier in `load_from_config`), validate the path type:
- if `path.is_dir()` => directory loader
- else if `path.is_file()` => zip loader
- else => return `InvalidConfig` explaining the OSV DB must be a regular file or directory
- This avoids blocking on FIFOs/sockets/devices and makes misconfiguration fail fast with a clear error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

When PackageVersionGuard rejects all candidate versions, pick_from_registry_with_guard returns
Ok(None), which the resolver chain interprets as “this resolver does not handle this dependency” and
raises SpecNotSupported. This misreports an OSV/policy block (or “no acceptable version”) as an
unsupported spec and can change fallback behavior and user-facing diagnostics.

Code

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[R555-575]

+        let version_str = version.version.to_string();
+        match guard.check(&opts.spec.name, &version_str).await? {
+            PackageVersionGuardDecision::Allow => {
+                return Ok(Some(PickedFromRegistry { meta: pick_result.meta, version }));
+            }
+            PackageVersionGuardDecision::Reject { reason } => {
+                tracing::debug!(
+                    target: "pacquet_resolving_npm_resolver",
+                    name = %opts.spec.name,
+                    version = %version_str,
+                    reason,
+                    "package version rejected by resolver guard",
+                );
+                // A `false` return means the picker re-selected a version we
+                // already blocked, so filtering can't exclude it (e.g. the
+                // parsed version string doesn't match the packument key).
+                // Stop rather than loop forever, treating the package as
+                // having no acceptable version.
+                if !blocked_versions.insert(version_str) {
+                    return Ok(None);
+                }

Evidence

The new guard path returns Ok(None) when a rejected version is re-selected (or when no acceptable
version remains), but the resolver contract defines Ok(None) as “resolver does not handle this dep”;
the deps-resolver maps that to SpecNotSupported, yielding a misleading error for guard-driven
rejection.

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[526-575]
pacquet/crates/resolving-resolver-base/src/resolve.rs[461-489]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1823-1831]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pick_from_registry_with_guard()` returns `Ok(None)` when the guard keeps rejecting candidates until nothing acceptable remains. In the resolver-chain contract, `Ok(None)` means “I don’t claim this dependency”, and the deps-resolver converts that into `SpecNotSupported`, which is misleading for OSV/policy blocks.
### Issue Context
- Resolver contract: `Ok(None)` = defer to next resolver.
- deps-resolver treats `None` as “specifier not supported”.
- The guard feature increases how often “no acceptable version remains” can happen even for otherwise supported npm specs.
### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[526-579]
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1823-1831]
- pacquet/crates/resolving-resolver-base/src/resolve.rs[461-489]
### What to change
- Introduce a concrete error for “no acceptable versions remain after applying guard” (and ideally for “no matching version” generally) and return `Err(ResolveError)` instead of `Ok(None)` in that case.
- Preserve `Ok(None)` only for truly “unsupported spec/protocol” situations (parse failures, workspace-path fallthrough, etc.).
- Optionally include the last rejection `reason` in the error message/code so clients can tell this was blocked by OSV/policy rather than “unsupported”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

When osv.enabled is true, loading an OSV zip/directory can succeed with an empty packages map
(e.g. wrong directory/zip contents, only withdrawn/non-npm records), which makes the guard always
return Allow and silently disables the intended vulnerability blocking. This creates a false sense
of protection because pnpr starts normally and both resolution-time and lockfile checks will report
no OSV violations.

Code

pnpr/crates/pnpr/src/resolver/osv.rs[R212-235]

+fn load_from_zip(path: &Path) -> Result<OsvIndex, RegistryError> {
+    let fingerprint = file_fingerprint(path)?;
+    let file = File::open(path).map_err(|err| {
+        invalid_config(format!("failed to open OSV database {}: {err}", path.display()))
+    })?;
+    let mut archive = zip::ZipArchive::new(file).map_err(|err| {
+        invalid_config(format!("failed to read OSV zip {}: {err}", path.display()))
+    })?;
+    let mut packages = HashMap::new();
+    for index in 0..archive.len() {
+        let mut entry = archive.by_index(index).map_err(|err| {
+            invalid_config(format!("failed to read OSV zip entry in {}: {err}", path.display()))
+        })?;
+        if !entry.is_file() || !entry.name().ends_with(".json") {
+            continue;
+        }
+        let mut bytes = Vec::with_capacity(entry.size().min(1024 * 1024) as usize);
+        entry.read_to_end(&mut bytes).map_err(|err| {
+            invalid_config(format!("failed to read OSV zip entry {}: {err}", entry.name()))
+        })?;
+        ingest_record_bytes(&mut packages, &bytes)?;
+    }
+    Ok(OsvIndex { packages, fingerprint })
+}

Evidence

Both zip and directory loaders return an index even if no advisories are ingested, and the guard’s
decision path explicitly returns Allow when no vulnerability IDs are found (which will be the case
for all packages if the index is empty).

pnpr/crates/pnpr/src/resolver/osv.rs[212-265]
pnpr/crates/pnpr/src/resolver/osv.rs[65-87]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
With `osv.enabled = true`, `OsvIndex::load_from_path` returns `Ok(OsvIndex { packages, ... })` even when no npm advisories were ingested. Because `vulnerability_ids()` then returns an empty list for every lookup, the guard always yields `Allow`, disabling OSV enforcement silently.
## Issue Context
This is a security control; when enabled, it should either enforce or fail loudly. An empty/ineffective DB should be treated as invalid configuration (or at minimum an explicit degraded-mode warning + refusal to enable enforcement).
## Fix Focus Areas
- pnpr/crates/pnpr/src/resolver/osv.rs[212-265]
- pnpr/crates/pnpr/src/resolver/osv.rs[65-87]
- pnpr/crates/pnpr/src/resolver/osv.rs[26-39]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools