fix(pnpr): block link-local redirects and trailing-dot metadata hosts

YESHYUNGSEOK · claude · YESHYUNGSEOK · commit 4e257e4152e2 · 2026-06-26T14:25:58.000+09:00
Addresses the review on this PR:

- The request-boundary check only validated the original registry URL, but
  the metadata-fetch client follows redirects: a registry on an allowed
  host could 302-redirect a server-side fetch to a link-local /
  instance-metadata host and still reach IMDS. Add a reqwest redirect
  policy that denies redirects whose target host is blocked (legitimate
  redirects to other hosts still follow, up to the default depth).
- Move the blocked-host predicate into pacquet-network as
  `is_blocked_request_host`, the single source shared by the redirect
  policy and pnpr's `reject_blocked_registries` boundary check, so the two
  can't drift.
- Normalize a trailing dot on a domain host so the root-zone FQDN form
  `metadata.google.internal.` can't bypass the metadata-host match.

Tests cover the link-local IPv4/IPv6, IPv4-mapped, metadata-hostname, and
trailing-dot cases, plus the allowed public/private/loopback hosts.

Co-authored-by: Claude &lt;noreply@anthropic.com&gt;
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/pacquet/crates/network/Cargo.toml b/pacquet/crates/network/Cargo.toml
@@ -17,6 +17,7 @@ pipe-trait  = { workspace = true }
 reqwest     = { workspace = true }
 tokio       = { workspace = true, features = ["time"] }
 tracing     = { workspace = true }
+url         = { workspace = true }
 
 [dev-dependencies]
 mockito               = { workspace = true }
diff --git a/pacquet/crates/network/src/lib.rs b/pacquet/crates/network/src/lib.rs
@@ -421,6 +421,54 @@ fn default_client_builder(settings: &NetworkSettings) -> reqwest::ClientBuilder
         .timeout(settings.fetch_timeout)
         .pool_idle_timeout(Duration::from_secs(4))
         .hickory_dns(true)
+        // Reject redirects to link-local / instance-metadata hosts: a
+        // (client-supplied) registry that 302-redirects there must not be
+        // able to turn a server-side metadata fetch into SSRF. Legitimate
+        // redirects to other hosts still follow, up to the default depth.
+        .redirect(reqwest::redirect::Policy::custom(|attempt| {
+            if attempt.previous().len() >= 10 {
+                attempt.error("too many redirects")
+            } else if is_blocked_request_host(attempt.url()) {
+                attempt.error("redirect to a link-local or instance-metadata host is not allowed")
+            } else {
+                attempt.follow()
+            }
+        }))
+}
+
+/// 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
+/// checks in [`is_blocked_request_host`] cover the addresses themselves.
+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))
+        }
+        None => false,
+    }
 }
 
 /// Load the PEM bundle named by `NODE_EXTRA_CA_CERTS` as extra trust
diff --git a/pacquet/crates/network/src/tests.rs b/pacquet/crates/network/src/tests.rs
@@ -726,3 +726,30 @@ fn default_network_concurrency_stays_within_floor_and_cap() {
     let concurrency = super::default_network_concurrency();
     assert!((64..=96).contains(&concurrency), "got {concurrency}");
 }
+
+/// [`super::is_blocked_request_host`] blocks the link-local instance-metadata
+/// range and well-known metadata hostnames (including the trailing-dot FQDN
+/// form), while allowing public, private, and loopback registries. This is
+/// the predicate the install client's redirect policy and pnpr's
+/// request-boundary check share.
+#[test]
+fn is_blocked_request_host_blocks_link_local_and_metadata_only() {
+    let blocked = |u: &str| super::is_blocked_request_host(&url::Url::parse(u).unwrap());
+
+    // Link-local IPv4 (cloud instance metadata) and the range around it.
+    assert!(blocked("http://169.254.169.254/"));
+    assert!(blocked("http://169.254.0.1:8080/path/"));
+    // Link-local IPv6 (fe80::/10) and an IPv4-mapped link-local address.
+    assert!(blocked("http://[fe80::1]/"));
+    assert!(blocked("http://[::ffff:169.254.169.254]/"));
+    // Metadata hostname: case-insensitive and trailing-dot (root-zone) FQDN.
+    assert!(blocked("https://metadata.google.internal/"));
+    assert!(blocked("https://Metadata.Google.Internal/"));
+    assert!(blocked("https://metadata.google.internal./"));
+
+    // Public, private, and loopback hosts are allowed.
+    assert!(!blocked("https://registry.npmjs.org/"));
+    assert!(!blocked("http://10.0.0.5:4873/"));
+    assert!(!blocked("http://192.168.1.10/"));
+    assert!(!blocked("http://127.0.0.1:4873/"));
+}
diff --git a/pnpr/crates/pnpr/src/resolver.rs b/pnpr/crates/pnpr/src/resolver.rs
@@ -202,33 +202,16 @@ impl Resolver {
 /// 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.
-const BLOCKED_METADATA_HOSTS: &[&str] = &["metadata.google.internal"];
-
-/// Whether a client-supplied registry URL points at a host the resolver
-/// must never fetch from: the link-local range that fronts cloud instance
-/// metadata (`169.254.0.0/16`, `fe80::/10`) plus the well-known metadata
-/// hostnames that resolve into it. Private and loopback addresses are
-/// deliberately *allowed* — resolving against an internal registry is
-/// pnpr's core use case. This is a request-boundary check on the URL the
-/// client sends; a hostname that only *resolves* to a link-local address
-/// at connect time (DNS rebinding) is out of scope here. A value that
-/// doesn't parse as a URL can't drive a fetch, so it isn't blocked.
+/// 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 {
-    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,
-    }
+    url::Url::parse(registry).is_ok_and(|url| pacquet_network::is_blocked_request_host(&url))
 }
 
 /// Reject (as `400`) a request that would point the resolver's
diff --git a/pnpr/crates/pnpr/src/resolver/tests.rs b/pnpr/crates/pnpr/src/resolver/tests.rs
@@ -218,6 +218,8 @@ fn link_local_and_metadata_registries_are_blocked_but_private_ones_are_not() {
     assert!(is_blocked_registry_host("http://[::ffff:169.254.169.254]/"));
     // Well-known metadata hostname, case-insensitively.
     assert!(is_blocked_registry_host("https://Metadata.Google.Internal/"));
+    // A trailing dot (root-zone FQDN) must not bypass the metadata-host match.
+    assert!(is_blocked_registry_host("https://metadata.google.internal./"));
 
     // Private and loopback registries are deliberately allowed — resolving
     // against an internal registry is pnpr's core use case.
