fix(sources): rss review round 3 — canonicalize mapped-IPv6 at the egress seam, +fixes

Review P2 (security): check_hop_target and check_addrs handed IPs to the
injected EgressPolicy without canonicalizing. An IPv4-mapped IPv6 like
::ffff:10.0.0.1 reached a policy as IpAddr::V6, so a policy checking
V4-private and V6-reserved ranges separately allowed it while the OS
dual-stack connect delivered the TCP connection to IPv4 10.0.0.1 — the
seam failing to hold for injected policies (not the documented AllowAll
SSRF stance). Both seam call points now to_canonical() before check_ip
(unmapping mapped-v6 to V4), reporting the canonical address in
EgressDenied; check_addrs still returns the original addrs to connect to.
Regression tests: bracketed mapped literal on the initial URL and on a
redirect hop, plus a check_addrs unit test via a DenyV4 policy that can
only match once canonicalized (the AAAA-record path).

Review P3s:
- OPML over-cap file cut mid-UTF-8-sequence reported "invalid UTF-8"
  instead of the size limit. read_bytes_bounded now reads raw bytes; the
  size check runs before String::from_utf8, so an over-size file always
  reports the size limit. Test pins it with a multibyte pad.
- Seam test gaps: the redirect->hostname->PolicyDns composition
  (DNS-rebinding-via-redirect) and the Some(Validators{None,None})-answered
  -with-304 distinction (sent_validator vs is_some) are now pinned.
- Dangling forward references: error.rs/egress.rs doc comments and the
  max_error_chars test cited docs that land later in this stack; softened
  to "published later in this stack" rather than exact paths/line numbers,
  so nothing dangles or claims falsely if phase 1 merges alone.
- Convention: quick_xml::XmlVersion imported at the top instead of an
  inline path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: gracexmatin

crates/skardi/src/sources/providers/rss/egress.rs

@@ -7,9 +7,9 @@
 //! targets). An operator — or Skardi Cloud — supplies a real [`EgressPolicy`]
 //! through the fetcher's constructor to restrict egress; the reserved-range
 //! taxonomy that would refuse loopback/link-local/private/CGNAT/unique-local
-//! targets is Cloud policy, specified in
-//! `docs/superpowers/specs/2026-08-03-rss-cloud-egress-design.md`, not shipped
-//! here.
+//! targets is Cloud policy, specified in the RSS Cloud egress design doc
+//! (`docs/superpowers/specs/2026-08-03-rss-cloud-egress-design.md`, added
+//! later in this stack), not shipped here.
 //!
 //! The seam is enforced at the DNS-resolver layer ([`PolicyDns`]) so an
 //! injected policy holds against DNS rebinding: reqwest only ever connects to
@@ -97,14 +97,22 @@ pub(crate) fn check_addrs(
     addrs: Vec<SocketAddr>,
 ) -> Result<Vec<SocketAddr>, EgressDenied> {
     for addr in &addrs {
-        if let Err(reason) = policy.check_ip(addr.ip()) {
+        // Canonicalize for the CHECK only: a dual-stack connect to an
+        // IPv4-mapped v6 (an AAAA answer like `::ffff:10.0.0.1`) reaches the
+        // unmapped V4, so the policy must judge that V4 or the mapped form
+        // bypasses a V4-private rule. `to_canonical` unmaps mapped-v6 to V4 and
+        // leaves everything else unchanged.
+        let ip = addr.ip().to_canonical();
+        if let Err(reason) = policy.check_ip(ip) {
             return Err(EgressDenied {
                 host: host.to_string(),
-                ip: addr.ip(),
+                ip,
                 reason,
             });
         }
     }
+    // Return the ORIGINAL addrs: canonicalization gated the check, but the
+    // caller must connect to exactly what the lookup returned.
     Ok(addrs)
 }
 
@@ -140,6 +148,7 @@ impl Resolve for PolicyDns {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::net::Ipv4Addr;
 
     /// Test-only denying policy: refuses every address. OSS ships no such
     /// policy — the seam exists so a caller can inject one.
@@ -151,6 +160,22 @@ mod tests {
         }
     }
 
+    /// Test-only policy that denies exactly one V4 address and nothing else.
+    /// It cannot match a raw IPv4-mapped v6 (`::ffff:10.0.0.1` arrives as
+    /// `IpAddr::V6`), so it only refuses if `check_addrs` canonicalized first —
+    /// which is precisely what the mapped-v6 test relies on.
+    #[derive(Debug)]
+    struct DenyV4(Ipv4Addr);
+    impl EgressPolicy for DenyV4 {
+        fn check_ip(&self, ip: IpAddr) -> Result<(), EgressReason> {
+            if ip == IpAddr::V4(self.0) {
+                Err("test-denied".into())
+            } else {
+                Ok(())
+            }
+        }
+    }
+
     #[test]
     fn allow_all_permits_every_address() {
         // The OSS default refuses nothing — including addresses a cloud policy
@@ -180,6 +205,18 @@ mod tests {
         assert_eq!(err.reason, "test-denied");
     }
 
+    #[test]
+    fn check_addrs_canonicalizes_mapped_ipv6_before_policy() {
+        // The resolver/AAAA-record equivalent of the fetcher's mapped-literal
+        // case: a lookup answer of `::ffff:10.0.0.1` is a v6 spelling of the V4
+        // 10.0.0.1 that a dual-stack connect reaches. DenyV4 refuses only the
+        // raw V4, so this passes only because check_addrs canonicalizes before
+        // consulting the policy.
+        let policy = DenyV4("10.0.0.1".parse().unwrap());
+        let addrs: Vec<SocketAddr> = vec!["[::ffff:10.0.0.1]:80".parse().unwrap()];
+        assert!(check_addrs(&policy, "evil.example", addrs).is_err());
+    }
+
     #[test]
     fn egress_denied_display_names_host_reason_and_ip() {
         // Contractual: stored verbatim as feeds.last_error. A cloud policy that

crates/skardi/src/sources/providers/rss/error.rs

@@ -22,9 +22,10 @@ use thiserror::Error;
 /// A bound on *length* only. What content may reach `feeds.last_error` at all is
 /// a separate question, argued in `engine.rs`'s module doc.
 ///
-/// `docs/rss.md` and `docs/rss/semantics.yaml` publish the number as a bare
-/// `512`; neither is Rust and neither can reference this constant, so both name
-/// it as `MAX_ERROR_CHARS`'s value and this is where it is defined.
+/// The RSS docs (`docs/rss.md` and `docs/rss/semantics.yaml`, published later
+/// in this stack) will spell the number as a bare `512`; neither is Rust and
+/// neither can reference this constant, so both name it as `MAX_ERROR_CHARS`'s
+/// value and this is where it is defined.
 pub const MAX_ERROR_CHARS: usize = 512;
 
 /// Bound `text` to `max_chars` *characters*, cutting on a char boundary so a
@@ -115,21 +116,22 @@ mod tests {
 
     /// The published number, spelled literally.
     ///
-    /// `docs/rss.md:974` ("bounded at 512 characters") and
-    /// `docs/rss/semantics.yaml:85` (the `last_error` column description) both
-    /// state 512 as this provider's diagnostic-length contract, and neither is
-    /// Rust: neither can reference the constant, so neither would notice it
-    /// changing. Every other assertion in this crate compares against
-    /// `MAX_ERROR_CHARS` itself and so agrees with any value it is given —
-    /// verified by mutation (512 → 200 and 512 → 999 both left the suite
-    /// green). Same discipline as `mod.rs`'s
+    /// The RSS docs (`docs/rss.md` and `docs/rss/semantics.yaml`, published
+    /// later in this stack) will state 512 as this provider's
+    /// diagnostic-length contract, and neither is Rust: neither can reference
+    /// the constant, so neither would notice it changing. Every other
+    /// assertion in this crate compares against `MAX_ERROR_CHARS` itself and so
+    /// agrees with any value it is given — verified by mutation (512 → 200 and
+    /// 512 → 999 both left the suite green). Same discipline as `mod.rs`'s
     /// `schema_metadata_carries_surface_version`, which spells `"1"` rather than
-    /// reading `RSS_SURFACE_VERSION`.
+    /// reading `RSS_SURFACE_VERSION`. Once those docs land, the phase that adds
+    /// them owns keeping their literal in step with this test (see the phase-4
+    /// note); until then this pins the value against silent drift.
     #[test]
     fn max_error_chars_is_the_number_the_docs_publish() {
         assert_eq!(
             MAX_ERROR_CHARS, 512,
-            "docs/rss.md and docs/rss/semantics.yaml both publish 512; change them together"
+            "the RSS docs (published later in this stack) will state 512; keep them in step"
         );
     }
 

crates/skardi/src/sources/providers/rss/fetch.rs

@@ -338,6 +338,12 @@ impl FeedFetcher {
             _ => None,
         };
         if let Some(ip) = ip {
+            // Canonicalize before checking: a dual-stack OS connect to an
+            // IPv4-mapped v6 literal (`::ffff:10.0.0.1`) reaches the unmapped
+            // V4 (10.0.0.1), so the policy must judge that same V4 — otherwise
+            // a V4-private rule is bypassed by the mapped form. Report the
+            // canonical address too, so the error names the rule that matched.
+            let ip = ip.to_canonical();
             self.policy.check_ip(ip).map_err(|reason| EgressDenied {
                 host: ip.to_string(),
                 ip,
@@ -707,6 +713,42 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn empty_validators_answered_with_304_is_a_status_error() {
+        // The `Some`-but-empty companion to
+        // `unconditional_304_without_validators_is_a_status_error`:
+        // `attempt_hop` maps a `304` to `NotModified` only when a conditional
+        // header was *actually sent*, tracked by `sent_validator` — not merely
+        // whether a `Validators` was passed. A `Some(Validators { etag: None,
+        // last_modified: None })` attaches no `If-None-Match`/`If-Modified-Since`
+        // header, so a `304` answering it has no validator behind it and must
+        // surface as the terminal status it is, exactly as the `None` case does
+        // — never a fabricated `NotModified` with no cached body to back it up.
+        let server = MockFeedServer::start(|_req| MockResponse::status(304)).await;
+        let f = test_fetcher();
+        let v = Validators {
+            etag: None,
+            last_modified: None,
+        };
+        let err = f
+            .fetch(&format!("{}/f", server.url()), Some(&v))
+            .await
+            .unwrap_err();
+        assert!(
+            matches!(err, FetchError::Status { status: 304 }),
+            "got {err}"
+        );
+        let req = &server.requests()[0];
+        assert!(
+            req.header("if-none-match").is_none(),
+            "an empty Validators must attach no If-None-Match"
+        );
+        assert!(
+            req.header("if-modified-since").is_none(),
+            "an empty Validators must attach no If-Modified-Since"
+        );
+    }
+
     #[tokio::test]
     async fn validators_are_not_resent_after_redirect() {
         // The module doc devotes a section to "validators cover only the
@@ -1046,6 +1088,26 @@ mod tests {
         }
     }
 
+    #[tokio::test]
+    async fn injected_policy_refuses_mapped_ipv6_literal_on_initial_url() {
+        // The IPv4-mapped-v6 literal path: `::ffff:10.0.0.1` is a v6 spelling of
+        // the V4 10.0.0.1 that a dual-stack connect actually reaches. The deny
+        // list holds the V4, so check_hop_target must canonicalize before the
+        // check. Before the fix the policy saw an unmatched V6, allowed it, and
+        // the fetch failed with a Transport error from the attempted connect to
+        // port 9 — so this pins the canonicalization.
+        let policy = Arc::new(DenyList(vec!["10.0.0.1".parse().unwrap()]));
+        let f = fetcher_with_policy(policy);
+        let err = f
+            .fetch("http://[::ffff:10.0.0.1]:9/f", None)
+            .await
+            .unwrap_err();
+        match err {
+            FetchError::Egress(e) => assert_eq!(e.reason, "test-denied", "got {e}"),
+            other => panic!("expected Egress, got {other:?}"),
+        }
+    }
+
     #[tokio::test]
     async fn injected_policy_refuses_redirect_target() {
         // The redirect path: the loopback mock (allowed) 302s to a denied
@@ -1071,6 +1133,33 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn injected_policy_refuses_mapped_ipv6_redirect_target() {
+        // The redirect path with an IPv4-mapped-v6 target: the loopback mock
+        // (allowed) 302s to `::ffff:10.0.0.1`, a v6 spelling of the denied V4.
+        // check_hop_target must canonicalize before the check so the V4 deny
+        // rule matches and the mapped target is never connected to.
+        let server = MockFeedServer::start(|_req| {
+            MockResponse::status(302).with_header("location", "http://[::ffff:10.0.0.1]/f")
+        })
+        .await;
+        let policy = Arc::new(DenyList(vec!["10.0.0.1".parse().unwrap()]));
+        let f = fetcher_with_policy(policy);
+        let err = f
+            .fetch(&format!("{}/start", server.url()), None)
+            .await
+            .unwrap_err();
+        match err {
+            FetchError::Egress(e) => assert_eq!(e.reason, "test-denied", "got {e}"),
+            other => panic!("expected Egress, got {other:?}"),
+        }
+        assert_eq!(
+            server.requests().len(),
+            1,
+            "the denied redirect target must never be connected to"
+        );
+    }
+
     #[tokio::test]
     async fn injected_policy_refuses_hostname_via_resolver() {
         // The resolver path: a hostname (`localhost`) that resolves to a denied
@@ -1090,6 +1179,49 @@ mod tests {
         assert!(matches!(err, FetchError::Egress(_)), "got {err}");
     }
 
+    #[tokio::test]
+    async fn injected_policy_refuses_hostname_redirect_target_via_resolver() {
+        // The DNS-rebinding-via-redirect path, composing the two preceding
+        // tests: `injected_policy_refuses_redirect_target` refuses a redirect
+        // to an IP *literal*, and `injected_policy_refuses_hostname_via_resolver`
+        // refuses an initial-URL *hostname* via PolicyDns — this refuses a
+        // redirect whose target is a hostname the resolver then denies.
+        //
+        // Making it work on loopback turns on the IPv4/IPv6 asymmetry: hop 0 is
+        // `server.url()`, the 127.0.0.1 literal `check_hop_target`'s IP arm must
+        // ALLOW so the first hop connects; the 302 then points at `localhost`
+        // (same port, via the recorded `host` header — the handler runs before
+        // `server` exists, so it cannot read `server.url()` directly), which
+        // PolicyDns resolves to the dual-stack set and `check_addrs` denies as a
+        // whole because `::1` is on the deny list. Denying only `::1` while
+        // allowing 127.0.0.1 is exactly what lets hop 0 through yet refuses hop
+        // 1's hostname — otherwise both hops share loopback and no policy could
+        // allow one without the other. This relies on `localhost` resolving
+        // dual-stack (a set containing `::1`), which holds on the CI runner.
+        let server = MockFeedServer::start(|req| {
+            let host = req.header("host").expect("reqwest sends a host header");
+            let redirect_to = format!("http://{}/denied", host.replace("127.0.0.1", "localhost"));
+            MockResponse::status(302).with_header("location", &redirect_to)
+        })
+        .await;
+        let policy = Arc::new(DenyList(vec!["::1".parse().unwrap()]));
+        let f = fetcher_with_policy(policy);
+        let err = f
+            .fetch(&format!("{}/start", server.url()), None)
+            .await
+            .unwrap_err();
+        match err {
+            FetchError::Egress(e) => assert_eq!(e.reason, "test-denied", "got {e}"),
+            other => panic!("expected Egress, got {other:?}"),
+        }
+        assert_eq!(
+            server.requests().len(),
+            1,
+            "only /start was ever connected; the denied hostname target was \
+             refused before any connection"
+        );
+    }
+
     /// The child half of `proxy_env_vars_do_not_bypass_the_egress_policy`:
     /// the actual fetch-under-proxy-variables check, meant to run in a
     /// subprocess whose environment the parent set at spawn. `#[ignore]`