Repository navigation
fix: harden Rust MediaFetcher agains redirect bypass and DNS rebinding - #8569
Conversation
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
e064170 to
2f2816d
Compare
The ipnet dep added to dynamo-llm in the main fix needs to be recorded
in the sub-workspace lock files for lib/bindings/{python,kvbm} as well,
otherwise CI's cargo --locked step fails.
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
WalkthroughThe changes add SSRF (Server-Side Request Forgery) protection for media URL fetching by introducing IP/hostname blocklisting, DNS filtering via a custom resolver, and an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/llm/src/preprocessor/media/loader.rs`:
- Around line 135-152: The flag allow_private_ips currently disables both the
RFC-range IP check and the hostname blocklist (BLOCKED_HOSTS) in the loader URL
guard, which is surprising and dangerous; either (A) rename the flag to
something explicit like allow_internal_targets/allow_private_targets and update
its field doc-comment and any Python binding to reflect that it disables both
host and IP checks, or (B) split into two flags (allow_private_ips and
allow_blocked_hosts) and change the guard to check each independently using
is_blocked_ip and BLOCKED_HOSTS; at minimum add a /// doc comment on the
allow_private_ips field explaining it disables both blocklists and update any
bindings/docs to match.
- Line 23: The current hardcoded MAX_REDIRECTS constant in loader.rs is set to 3
which is lower than reqwest's default and can break valid multi-hop flows;
update the constant (MAX_REDIRECTS) to a higher value (e.g., 10 to match
reqwest's default) and ensure any code that relies on MAX_REDIRECTS (redirect
handling logic and calls to check_if_url_allowed or BlocklistResolver) continues
to re-validate each hop so security posture is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb834529-7bad-4b46-ae95-aa6bfea7dfe8
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locklib/bindings/kvbm/Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomllib/bindings/python/rust/llm/preprocessor.rslib/llm/Cargo.tomllib/llm/src/preprocessor/media/loader.rs
test_fetch_and_decode uses mockito which binds to 127.0.0.1, which the new RFC-range blocklist rejects under the default policy. The test already sets allow_direct_ip + allow_direct_port; add allow_private_ips so the loopback address isn't rejected by the blocklist. Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
…locklists The flag's name reads IP-specific but semantically it's a single 'allow internal/on-prem targets' switch that bypasses both the RFC-range IP blocklist and the literal hostname blocklist (BLOCKED_HOSTS). This is intentional — real on-prem deployments need both at once, and splitting would add config surface without a concrete use case. The doc comment now makes the bundled semantics explicit so reviewers and users aren't surprised by the hostname blocklist being gated on a flag named allow_private_ips. Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
test_serve_deployment[multimodal_agg_frontend_decoding] failed with 'Host localhost is blocked' because Python's UrlValidationPolicy.from_env() read DYN_MM_ALLOW_INTERNAL=1 while Rust's MediaFetcher just used ::default() and ignored the env — so when the frontend preprocessor (Rust) did the fetch instead of the backend worker (Python), the internal-network opt-in was silently dropped. Add MediaFetcher::from_env() mirroring the Python behavior, have MediaLoader::new fall back to it when no explicit fetcher is passed, and use from_env in the PyO3 #[new] so MediaFetcher() from Python gets env-aware defaults. Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Overview:
As title
Similar fix in rust side as #8282
Details:
metadata.google.internal,kubernetes.default.svc,localhost, …)allow_private_ipsescape hatch for on-prem / local-devcheck_if_url_allowed_with_dnsresolves hostnames and rejectsany that resolve to blocked IPs
BlocklistResolverattached to the sharedreqwest::ClientviaClientBuilder::dns_resolver— filters blocked IPs out of DNS answers before reqwest ever sees them, closing the DNS-rebinding gap that the Python side left open (because httpx has no clean equivalent of reqwest'sdns_resolverhook)redirect::Policy::customrevalidates the URL-level policy on every redirect hop, cappedWhere should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Release Notes
New Features
Security