web_download: re-validate redirects against the SSRF blocklist#82
Merged
Conversation
web_download validated the initial URL but then handed it to `curl -L` / `aria2c`, which follow redirects at the OS level with no check — so a model-supplied (or prompt-injected) public URL that 302s to 169.254.169.254, 127.0.0.1, or an RFC1918 host slipped straight past the initial-hop guard and its response was written into a workspace file the model can read back. web_fetch already re-validated every hop via ssrf_safe_client; web_download did not. Fix: resolve_download_redirects walks the redirect chain in-process with ssrf_safe_client (whose redirect policy errors on a private hop) and returns only the validated terminal URL, which is what the downloader fetches. The GET used to resolve never reads the body, so `.send()` returns on headers alone — a multi-GB file isn't downloaded — and a 30s timeout keeps a slow resolve from hanging the turn. This also covers the HuggingFace path, whose resolve/… URL 302s to a CDN host (the standard hf_hub_download two-step). The downloader now runs with redirect following off: curl drops -L so an unexpected 3xx fails closed via --fail instead of being followed. aria2c has no redirect-disable flag, but since the URL is pre-resolved there is no hop left to follow; a public content host re-redirecting to a private one is the same residual DNS-rebind-class risk validate_url already documents. Tests: curl fallback carries no -L/--location and keeps --fail; private/link-local/RFC1918 URLs are rejected before any network I/O on both resolve_download and resolve_download_redirects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
davidrhodus
added a commit
that referenced
this pull request
Jul 10, 2026
* Apply rustfmt to the review-fix code (unblock CI) The CI `fmt · test` job runs `cargo fmt --all --check` before the test step, and several hand-written lines from the recent review fixes (#79, #81, #82 and the loop-firing timeout) didn't match rustfmt — mostly long assert!/match/call expressions that rustfmt reflows across multiple lines. Format failing blocked the Test step from ever running, so CI showed red despite a green local suite. Formatting-only: `git diff -w` shows nothing but line reflows; the four touched crates' tests still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix two clippy errors surfaced on stable 1.97 CI's stable toolchain moved to rust 1.97, whose tightened lints error (under -D warnings) on two pre-existing spots the 1.93 CI never flagged. Once the fmt fix let the Test job reach the Clippy step, they blocked it: - web.rs: redundant `&` in a format! arg (useless_borrows_in_formatting) - app/run.rs: sort_by comparator that is really a keyed reverse sort (unnecessary_sort_by) → sort_by_key(Reverse(..)) Both are behavior-preserving. Verified with the CI toolchain: `cargo +stable clippy -p hi-agent -p hi-ai -p hi-tools -p hi-tui -p hi -p hi-eval -- -D warnings` is clean, fmt --check clean, tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the last follow-up from the #79 review.
web_downloadvalidated the initial URL (added in #79) but then handed it tocurl -L/aria2c, which follow redirects at the OS level with no SSRF check. So a model-supplied — or prompt-injected — public URL that 302-redirects to169.254.169.254(cloud metadata),127.0.0.1:6379, or an RFC1918 host slipped straight past the initial-hop guard, and its response was written into a workspace file the model can then read back.web_fetchalready re-validated every hop viassrf_safe_client;web_downloaddid not.Fix
resolve_download_redirectswalks the redirect chain in-process withssrf_safe_client(whose redirect policy errors on a private hop) and returns only the validated terminal URL — which is what the downloader fetches:.send()returns on headers alone (a multi-GB file is not downloaded), and a 30 s timeout keeps a slow resolve from hanging the turn.resolve/…URL 302s to a CDN host — the same resolve-then-download two-stephf_hub_downloaduses. The CDN URL supports range requests, so aria2c's parallel connections still work.The downloader now runs with redirect following off:
-L, so an unexpected 3xx fails closed via--failinstead of being followed.validate_urlalready documents (it resolves-then-connects); this fix inherits that posture rather than widening it.Test plan
-L/--locationand keeps--fail; private/link-local/RFC1918 URLs (169.254.169.254,127.0.0.1,10.0.0.1) are rejected before any network I/O on bothresolve_downloadandresolve_download_redirects(literal IPs, sovalidate_urlshort-circuits — deterministic, no network).cargo test -p hi-tools: 149 pass;cargo clippy -p hi-tools --all-targets: clean.🤖 Generated with Claude Code