Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 592ceb3d-3ff5-4421-9eed-43c65454419e

Reviewing files that changed from the base of the PR and between 8cf21e1 and 74fff66.

• pacquet/crates/network/src/tests.rs

• pacquet/crates/network/src/tests.rs

• Create PR with unit tests

Description

reject_blocked_registries only validates the user-supplied registry URL string, but registry
metadata fetches use a redirect-following reqwest client; an attacker can point
registry/namedRegistries at an allowed host they control that 302-redirects to a blocked
link-local/metadata host, causing the server to still connect to IMDS. This defeats the intended
SSRF mitigation for both /resolve and /verify-lockfile.

Code

pnpr/crates/pnpr/src/resolver.rs[R216-251]

+fn is_blocked_registry_host(registry: &str) -> bool {
+    let Ok(url) = url::Url::parse(registry) else {
+        return false;
+    };
+    match url.host() {
+        Some(url::Host::Ipv4(addr)) => addr.is_link_local(),
+        Some(url::Host::Ipv6(addr)) => {
+            // fe80::/10, plus any IPv4-mapped form of a link-local v4 address.
+            (addr.segments()[0] & 0xffc0) == 0xfe80
+                || addr.to_ipv4_mapped().is_some_and(|v4| v4.is_link_local())
+        }
+        Some(url::Host::Domain(host)) => {
+            BLOCKED_METADATA_HOSTS.iter().any(|blocked| host.eq_ignore_ascii_case(blocked))
+        }
+        None => false,
+    }
+}
+
+/// Reject (as `400`) a request that would point the resolver's
+/// server-side metadata fetches at a blocked host — see
+/// [`is_blocked_registry_host`]. Both the default registry and every
+/// named-registry alias are checked. The message stays generic so a
+/// registry URL carrying credentials isn't echoed back.
+fn reject_blocked_registries(request: &ResolveRequest) -> Option<Response> {
+    let registries =
+        request.registry.iter().chain(request.named_registries.values()).map(String::as_str);
+    for registry in registries {
+        if is_blocked_registry_host(registry) {
+            return Some(json_error(
+                StatusCode::BAD_REQUEST,
+                "registry host is not allowed (link-local and instance-metadata addresses are blocked)",
+            ));
+        }
+    }
+    None
+}

Evidence

The PR adds only a request-boundary host check (is_blocked_registry_host /
reject_blocked_registries) and does not account for redirect targets. The shared HTTP client
builder does not set any redirect policy, and other code paths rely on reqwest following redirects
(e.g., tarball resolution reads response.url() as the post-redirect URL), indicating redirects are
followed in practice; metadata fetches use the same client via client.get(&url).

pnpr/crates/pnpr/src/resolver.rs[202-251]
pacquet/crates/network/src/lib.rs[405-424]
pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs[119-147]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[90-111]

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 new guard (`reject_blocked_registries`/`is_blocked_registry_host`) only checks the original registry URL the client sends. However, the HTTP client used for server-side metadata fetches follows redirects, so an attacker-controlled registry can 302/307 to a link-local/metadata host and still trigger SSRF.
## Issue Context
Because reqwest redirect-follow happens *during* the request, validating only the initial URL at the request boundary is insufficient; the redirect target must be rejected before the client issues the follow-up request.
## Fix Focus Areas
- pacquet/crates/network/src/lib.rs[405-424]
- pnpr/crates/pnpr/src/resolver.rs[202-251]
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[90-111]
### Implementation sketch
- Add a reqwest redirect policy on the shared client builder (or on a dedicated “registry-metadata client”) using `reqwest::redirect::Policy::custom`.
- In the policy callback, inspect the *next* URL (`attempt.url()`) and **deny** the redirect if its host is link-local / instance-metadata (use the same logic as `is_blocked_registry_host`, ideally refactored into a shared helper that takes a `Url`/`Host`).
- Ensure this blocks both IPv4/IPv6 link-local and the metadata hostname(s), without breaking legitimate non-blocked redirects.
- Add a test that sets `registry` to an attacker-controlled URL that redirects to `http://169.254.169.254/` and assert the resolver/verify request is rejected (or fails closed without issuing the redirect-follow request).

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

Description

pacquet_network::is_blocked_request_host only blocks the exact domain metadata.google.internal,
so a registry URL with host metadata is not rejected by either pnpr’s request-boundary check or
the shared redirect policy. In deployments where DNS/search domains resolve metadata to the
instance-metadata service, this can re-enable SSRF to IMDS despite the new guard.

Code

pacquet/crates/network/src/lib.rs[R442-469]

+const BLOCKED_REQUEST_HOSTS: &[&str] = &["metadata.google.internal"];
+
+/// Whether a URL targets a host the install / resolver clients must never
+/// connect to: the link-local range that fronts cloud instance metadata
+/// (`169.254.0.0/16`, `fe80::/10`, and IPv4-mapped link-local addresses)
+/// plus the well-known metadata hostnames that resolve into it.
+///
+/// Used both as a request-boundary check (on a URL a client supplies) and
+/// as the client redirect policy in `default_client_builder`, so a
+/// redirect can't reach a blocked host that a boundary check on the
+/// *original* URL would miss (e.g. an allowed host that `302`s to
+/// `http://169.254.169.254/`). A trailing dot on a domain
+/// (`metadata.google.internal.`) is normalized away before matching.
+/// Private and loopback addresses are deliberately allowed — resolving
+/// against an internal registry (or a loopback dev registry) is legitimate.
+#[must_use]
+pub fn is_blocked_request_host(url: &url::Url) -> bool {
+    match url.host() {
+        Some(url::Host::Ipv4(addr)) => addr.is_link_local(),
+        Some(url::Host::Ipv6(addr)) => {
+            // fe80::/10, plus any IPv4-mapped form of a link-local v4 address.
+            (addr.segments()[0] & 0xffc0) == 0xfe80
+                || addr.to_ipv4_mapped().is_some_and(|v4| v4.is_link_local())
+        }
+        Some(url::Host::Domain(host)) => {
+            let host = host.strip_suffix('.').unwrap_or(host);
+            BLOCKED_REQUEST_HOSTS.iter().any(|blocked| host.eq_ignore_ascii_case(blocked))
+        }

Evidence

The blocklist constant contains only metadata.google.internal, and domain blocking is performed by
an equality check against that list, so metadata will not match. pnpr’s boundary check delegates
to this predicate, so the same hostname gap applies at the request boundary too.

pacquet/crates/network/src/lib.rs[439-471]
pnpr/crates/pnpr/src/resolver.rs[202-215]

Agent prompt

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

### Issue description
`is_blocked_request_host` only matches domain hosts against a tiny exact blocklist (`metadata.google.internal`). This leaves `metadata` (a common shortname used in some environments via DNS search domains) unblocked, which can undermine the intended “block instance metadata hostnames” protection.

### Issue Context
- The blocklist is intended to cover metadata hostnames that don’t “look like” link-local IPs.
- pnpr’s request-boundary check delegates to `pacquet_network::is_blocked_request_host`, so any hostname gap affects both `/resolve` and `/verify-lockfile`.

### Fix Focus Areas
- pacquet/crates/network/src/lib.rs[442-469]
- pacquet/crates/network/src/tests.rs[729-756]
- pnpr/crates/pnpr/src/resolver/tests.rs[208-232]

### Suggested change
- Add `"metadata"` to `BLOCKED_REQUEST_HOSTS`.
- Add test coverage asserting `https://metadata/` is blocked (both in pacquet_network and pnpr resolver tests).
- Keep comparisons case-insensitive and retain trailing-dot normalization.

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

Description

pacquet_network::is_blocked_request_host only strips one trailing '.' from domain hosts before
comparing to BLOCKED_REQUEST_HOSTS, so inputs ending with multiple trailing dots will not match
the metadata hostname blocklist. Because pnpr’s boundary check and pacquet’s redirect policy both
delegate to this predicate, any missed match here weakens the intended SSRF protection for metadata
hostnames.

Code

pacquet/crates/network/src/lib.rs[R466-468]

+        Some(url::Host::Domain(host)) => {
+            let host = host.strip_suffix('.').unwrap_or(host);
+            BLOCKED_REQUEST_HOSTS.iter().any(|blocked| host.eq_ignore_ascii_case(blocked))

Evidence

The blocklist match for domain hosts only removes a single trailing dot before comparing to
BLOCKED_REQUEST_HOSTS, so any domain string ending with more than one dot will not be normalized
to the blocked hostname. pnpr’s registry-host rejection explicitly delegates to this shared
predicate, and pacquet’s redirect policy also uses it, so the shared normalization behavior directly
affects SSRF defenses in both places.

pacquet/crates/network/src/lib.rs[424-436]
pacquet/crates/network/src/lib.rs[458-471]
pacquet/crates/network/src/tests.rs[730-755]
pnpr/crates/pnpr/src/resolver.rs[205-215]
pnpr/crates/pnpr/src/resolver.rs[217-233]

Agent prompt

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

## Issue description
`is_blocked_request_host` normalizes `metadata.google.internal.` by stripping a single trailing dot, but it does not normalize domain hosts with **multiple** trailing dots (e.g. `metadata.google.internal..`). This can cause the metadata-hostname blocklist check to miss equivalent root-terminated forms.
### Issue Context
This predicate is security-sensitive and is shared by:
- pacquet’s reqwest redirect policy (to prevent redirect-based SSRF)
- pnpr’s request-boundary validation of client-supplied registry URLs
### Fix Focus Areas
- pacquet/crates/network/src/lib.rs[466-469]
- pacquet/crates/network/src/tests.rs[730-755]
### Suggested fix
- Replace `strip_suffix('.')` with `trim_end_matches('.')` (or an equivalent loop) so *all* trailing dots are removed before comparison.
- Add a unit test case asserting the double-trailing-dot form is blocked (e.g. `https://metadata.google.internal../`).

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

Description

The doc comment above is_blocked_registry_host describes a local hostname blocklist and an IP
check “below”, but the function actually delegates all logic to
pacquet_network::is_blocked_request_host. This mismatch can mislead future edits in this
security-sensitive area.

Code

pnpr/crates/pnpr/src/resolver.rs[R202-215]

+/// Hostnames that resolve into the link-local instance-metadata range but
+/// don't look like a link-local address. Kept tiny and explicit — the IP
+/// check below covers the addresses themselves.
+/// Whether a client-supplied registry URL string points at a host the
+/// resolver must never fetch from. Delegates to
+/// [`pacquet_network::is_blocked_request_host`] so this request-boundary
+/// check and the install client's redirect policy share one definition —
+/// an attacker can't slip past the boundary by pointing `registry` at an
+/// allowed host that redirects to a link-local / instance-metadata host.
+/// A value that doesn't parse as a URL can't drive a fetch, so it isn't
+/// blocked here.
+fn is_blocked_registry_host(registry: &str) -> bool {
+    url::Url::parse(registry).is_ok_and(|url| pacquet_network::is_blocked_request_host(&url))
+}

Evidence

The added comment claims local logic (“hostnames … kept tiny … the IP check below”), but the
implementation is a thin wrapper around pacquet_network::is_blocked_request_host.

pnpr/crates/pnpr/src/resolver.rs[202-215]

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::resolver::is_blocked_registry_host` delegates to `pacquet_network::is_blocked_request_host`, but its doc comment currently reads like this file owns a hostname blocklist and an IP check. This is misleading in a security guard path.
### Issue Context
This comment sits immediately above the request-boundary SSRF validation.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver.rs[202-215]
### Suggested fix
Update the comment to explicitly say the predicate delegates to `pacquet_network::is_blocked_request_host` (where the hostname list and IP logic live), or remove the stale sentences about a local hostname list/IP check.

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

Description

The new redirect-policy test asserts on format!("{err:?}") containing a substring; Debug
formatting for reqwest::Error is not a stable contract and may change on dependency upgrades. This
can cause test-only failures even if the redirect policy behavior is still correct.

Code

pacquet/crates/network/src/tests.rs[R781-783]

+    let debug = format!("{err:?}");
+    eprintln!("err={debug}");
+    assert!(debug.contains("link-local or instance-metadata host"), "err={debug}");

Evidence

The test currently formats the error with {:?} and matches a substring, coupling the test to debug
formatting rather than a stable error kind or API surface.

pacquet/crates/network/src/tests.rs[773-785]

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 test `redirect_to_a_link_local_host_is_rejected_by_the_client` checks the `Debug` string of a `reqwest::Error` for a substring. This is brittle because `Debug` output can change even when behavior is correct.
### Issue Context
This is an end-to-end test ensuring the custom redirect policy rejects redirects to blocked hosts.
### Fix Focus Areas
- pacquet/crates/network/src/tests.rs[773-784]
### Suggested fix
Prefer assertions on more stable signals, e.g.:
- `assert!(err.is_redirect())` (reqwest provides helpers for error kinds), and/or
- check `err.to_string()` (still stringly, but typically more stable than `Debug`), and/or
- if available, assert on an underlying source/type rather than formatting.
Keep the existing behavioral assertion that the initial mock endpoint was hit.

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

Description

reject_blocked_registries only validates the user-supplied registry URL string, but registry
metadata fetches use a redirect-following reqwest client; an attacker can point
registry/namedRegistries at an allowed host they control that 302-redirects to a blocked
link-local/metadata host, causing the server to still connect to IMDS. This defeats the intended
SSRF mitigation for both /resolve and /verify-lockfile.

Code

pnpr/crates/pnpr/src/resolver.rs[R216-251]

+fn is_blocked_registry_host(registry: &str) -> bool {
+    let Ok(url) = url::Url::parse(registry) else {
+        return false;
+    };
+    match url.host() {
+        Some(url::Host::Ipv4(addr)) => addr.is_link_local(),
+        Some(url::Host::Ipv6(addr)) => {
+            // fe80::/10, plus any IPv4-mapped form of a link-local v4 address.
+            (addr.segments()[0] & 0xffc0) == 0xfe80
+                || addr.to_ipv4_mapped().is_some_and(|v4| v4.is_link_local())
+        }
+        Some(url::Host::Domain(host)) => {
+            BLOCKED_METADATA_HOSTS.iter().any(|blocked| host.eq_ignore_ascii_case(blocked))
+        }
+        None => false,
+    }
+}
+
+/// Reject (as `400`) a request that would point the resolver's
+/// server-side metadata fetches at a blocked host — see
+/// [`is_blocked_registry_host`]. Both the default registry and every
+/// named-registry alias are checked. The message stays generic so a
+/// registry URL carrying credentials isn't echoed back.
+fn reject_blocked_registries(request: &ResolveRequest) -> Option<Response> {
+    let registries =
+        request.registry.iter().chain(request.named_registries.values()).map(String::as_str);
+    for registry in registries {
+        if is_blocked_registry_host(registry) {
+            return Some(json_error(
+                StatusCode::BAD_REQUEST,
+                "registry host is not allowed (link-local and instance-metadata addresses are blocked)",
+            ));
+        }
+    }
+    None
+}

Evidence

The PR adds only a request-boundary host check (is_blocked_registry_host /
reject_blocked_registries) and does not account for redirect targets. The shared HTTP client
builder does not set any redirect policy, and other code paths rely on reqwest following redirects
(e.g., tarball resolution reads response.url() as the post-redirect URL), indicating redirects are
followed in practice; metadata fetches use the same client via client.get(&url).

pnpr/crates/pnpr/src/resolver.rs[202-251]
pacquet/crates/network/src/lib.rs[405-424]
pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs[119-147]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[90-111]

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 new guard (`reject_blocked_registries`/`is_blocked_registry_host`) only checks the original registry URL the client sends. However, the HTTP client used for server-side metadata fetches follows redirects, so an attacker-controlled registry can 302/307 to a link-local/metadata host and still trigger SSRF.
## Issue Context
Because reqwest redirect-follow happens *during* the request, validating only the initial URL at the request boundary is insufficient; the redirect target must be rejected before the client issues the follow-up request.
## Fix Focus Areas
- pacquet/crates/network/src/lib.rs[405-424]
- pnpr/crates/pnpr/src/resolver.rs[202-251]
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[90-111]
### Implementation sketch
- Add a reqwest redirect policy on the shared client builder (or on a dedicated “registry-metadata client”) using `reqwest::redirect::Policy::custom`.
- In the policy callback, inspect the *next* URL (`attempt.url()`) and **deny** the redirect if its host is link-local / instance-metadata (use the same logic as `is_blocked_registry_host`, ideally refactored into a shared helper that takes a `Url`/`Host`).
- Ensure this blocks both IPv4/IPv6 link-local and the metadata hostname(s), without breaking legitimate non-blocked redirects.
- Add a test that sets `registry` to an attacker-controlled URL that redirects to `http://169.254.169.254/` and assert the resolver/verify request is rejected (or fails closed without issuing the redirect-follow request).

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

Description

pacquet_network::is_blocked_request_host only strips one trailing '.' from domain hosts before
comparing to BLOCKED_REQUEST_HOSTS, so inputs ending with multiple trailing dots will not match
the metadata hostname blocklist. Because pnpr’s boundary check and pacquet’s redirect policy both
delegate to this predicate, any missed match here weakens the intended SSRF protection for metadata
hostnames.

Code

pacquet/crates/network/src/lib.rs[R466-468]

+        Some(url::Host::Domain(host)) => {
+            let host = host.strip_suffix('.').unwrap_or(host);
+            BLOCKED_REQUEST_HOSTS.iter().any(|blocked| host.eq_ignore_ascii_case(blocked))

Evidence

The blocklist match for domain hosts only removes a single trailing dot before comparing to
BLOCKED_REQUEST_HOSTS, so any domain string ending with more than one dot will not be normalized
to the blocked hostname. pnpr’s registry-host rejection explicitly delegates to this shared
predicate, and pacquet’s redirect policy also uses it, so the shared normalization behavior directly
affects SSRF defenses in both places.

pacquet/crates/network/src/lib.rs[424-436]
pacquet/crates/network/src/lib.rs[458-471]
pacquet/crates/network/src/tests.rs[730-755]
pnpr/crates/pnpr/src/resolver.rs[205-215]
pnpr/crates/pnpr/src/resolver.rs[217-233]

Agent prompt

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

## Issue description
`is_blocked_request_host` normalizes `metadata.google.internal.` by stripping a single trailing dot, but it does not normalize domain hosts with **multiple** trailing dots (e.g. `metadata.google.internal..`). This can cause the metadata-hostname blocklist check to miss equivalent root-terminated forms.
### Issue Context
This predicate is security-sensitive and is shared by:
- pacquet’s reqwest redirect policy (to prevent redirect-based SSRF)
- pnpr’s request-boundary validation of client-supplied registry URLs
### Fix Focus Areas
- pacquet/crates/network/src/lib.rs[466-469]
- pacquet/crates/network/src/tests.rs[730-755]
### Suggested fix
- Replace `strip_suffix('.')` with `trim_end_matches('.')` (or an equivalent loop) so *all* trailing dots are removed before comparison.
- Add a unit test case asserting the double-trailing-dot form is blocked (e.g. `https://metadata.google.internal../`).

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

Description

The doc comment above is_blocked_registry_host describes a local hostname blocklist and an IP
check “below”, but the function actually delegates all logic to
pacquet_network::is_blocked_request_host. This mismatch can mislead future edits in this
security-sensitive area.

Code

pnpr/crates/pnpr/src/resolver.rs[R202-215]

+/// Hostnames that resolve into the link-local instance-metadata range but
+/// don't look like a link-local address. Kept tiny and explicit — the IP
+/// check below covers the addresses themselves.
+/// Whether a client-supplied registry URL string points at a host the
+/// resolver must never fetch from. Delegates to
+/// [`pacquet_network::is_blocked_request_host`] so this request-boundary
+/// check and the install client's redirect policy share one definition —
+/// an attacker can't slip past the boundary by pointing `registry` at an
+/// allowed host that redirects to a link-local / instance-metadata host.
+/// A value that doesn't parse as a URL can't drive a fetch, so it isn't
+/// blocked here.
+fn is_blocked_registry_host(registry: &str) -> bool {
+    url::Url::parse(registry).is_ok_and(|url| pacquet_network::is_blocked_request_host(&url))
+}

Evidence

The added comment claims local logic (“hostnames … kept tiny … the IP check below”), but the
implementation is a thin wrapper around pacquet_network::is_blocked_request_host.

pnpr/crates/pnpr/src/resolver.rs[202-215]

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::resolver::is_blocked_registry_host` delegates to `pacquet_network::is_blocked_request_host`, but its doc comment currently reads like this file owns a hostname blocklist and an IP check. This is misleading in a security guard path.
### Issue Context
This comment sits immediately above the request-boundary SSRF validation.
### Fix Focus Areas
- pnpr/crates/pnpr/src/resolver.rs[202-215]
### Suggested fix
Update the comment to explicitly say the predicate delegates to `pacquet_network::is_blocked_request_host` (where the hostname list and IP logic live), or remove the stale sentences about a local hostname list/IP check.

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

Description

The new redirect-policy test asserts on format!("{err:?}") containing a substring; Debug
formatting for reqwest::Error is not a stable contract and may change on dependency upgrades. This
can cause test-only failures even if the redirect policy behavior is still correct.

Code

pacquet/crates/network/src/tests.rs[R781-783]

+    let debug = format!("{err:?}");
+    eprintln!("err={debug}");
+    assert!(debug.contains("link-local or instance-metadata host"), "err={debug}");

Evidence

The test currently formats the error with {:?} and matches a substring, coupling the test to debug
formatting rather than a stable error kind or API surface.

pacquet/crates/network/src/tests.rs[773-785]

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 test `redirect_to_a_link_local_host_is_rejected_by_the_client` checks the `Debug` string of a `reqwest::Error` for a substring. This is brittle because `Debug` output can change even when behavior is correct.
### Issue Context
This is an end-to-end test ensuring the custom redirect policy rejects redirects to blocked hosts.
### Fix Focus Areas
- pacquet/crates/network/src/tests.rs[773-784]
### Suggested fix
Prefer assertions on more stable signals, e.g.:
- `assert!(err.is_redirect())` (reqwest provides helpers for error kinds), and/or
- check `err.to_string()` (still stringly, but typically more stable than `Debug`), and/or
- if available, assert on an underlying source/type rather than formatting.
Keep the existing behavioral assertion that the initial mock endpoint was hit.

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