fix(security): enforce DNS-aware URL validation#2009
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefactors url_guard to perform port-aware, resolver-driven DNS validation (async) and updates curl, http_request, and web_fetch tools to call the new async ChangesDNS-Rebinding Protection and Tool Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
coderabbitai
Bot
added
the
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/tools/impl/network/url_guard.rs (2)
91-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the explicit port before the IP-literal fast path.
Line 93 returns early for IP literals, so a URL like
https://8.8.8.8:99999is accepted without ever runningextract_port(). That misses the new “validate explicit ports first” contract and defers the failure to reqwest instead of rejecting it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/impl/network/url_guard.rs` around lines 91 - 97, The early-return that accepts IP-literal hosts (the host.parse::<std::net::IpAddr>().is_ok() check) currently runs before validating an explicit port, so URLs like "8.8.8.8:99999" bypass extract_port(); call extract_port(&url) before the IP-literal fast path and handle its error result (return Err on invalid port) so explicit port validation always runs; adjust the control flow in the function containing that host.parse check to call extract_port first, validate/propagate any port errors, then continue with the IP-literal check and the existing is_private_or_local_host logic.
72-117:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis still leaves a rebinding race between validation and connect.
The helper validates one DNS lookup and then returns the original hostname URL. The later reqwest send path resolves that hostname again, so an attacker-controlled domain can still answer with a public IP during validation and a private IP during the actual connection. To fully close this gap, the request path needs to connect using the validated
SocketAddrs or otherwise pin resolution after validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/impl/network/url_guard.rs` around lines 72 - 117, The validation currently only performs one DNS lookup and returns the original hostname (validate_url_with_dns_check / validate_url_with_dns_check_with_resolver), which leaves a race where the actual connection can resolve to a different IP; instead, modify the validation API to return the concrete validated SocketAddr(s) (or a tuple (url, Vec<SocketAddr>)) from resolve_host_ips and validate_url_with_dns_check_with_resolver and propagate that to callers so the request path uses those pinned SocketAddr(s) for the TCP connect (e.g., by dialing the SocketAddr(s) directly or configuring the HTTP client's connector while preserving the original Host header), and update all callers of validate_url_with_dns_check to accept the new return type and use the pinned addresses for the actual reqwest connect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/tools/impl/network/url_guard.rs`:
- Around line 120-128: The DNS lookup in resolve_host_ips uses the blocking
std::net::ToSocketAddrs (to_socket_addrs) on async call paths; change
resolve_host_ips to perform the resolution off the async reactor by wrapping the
blocking call in tokio::task::spawn_blocking (or use an async resolver) and
await its JoinHandle, returning the same anyhow::Result<Vec<IpAddr>>; ensure
errors from the blocking task are propagated and logged the same way (preserve
the log::debug! message and the anyhow::anyhow! error string) so callers of
resolve_host_ips need not change.
---
Outside diff comments:
In `@src/openhuman/tools/impl/network/url_guard.rs`:
- Around line 91-97: The early-return that accepts IP-literal hosts (the
host.parse::<std::net::IpAddr>().is_ok() check) currently runs before validating
an explicit port, so URLs like "8.8.8.8:99999" bypass extract_port(); call
extract_port(&url) before the IP-literal fast path and handle its error result
(return Err on invalid port) so explicit port validation always runs; adjust the
control flow in the function containing that host.parse check to call
extract_port first, validate/propagate any port errors, then continue with the
IP-literal check and the existing is_private_or_local_host logic.
- Around line 72-117: The validation currently only performs one DNS lookup and
returns the original hostname (validate_url_with_dns_check /
validate_url_with_dns_check_with_resolver), which leaves a race where the actual
connection can resolve to a different IP; instead, modify the validation API to
return the concrete validated SocketAddr(s) (or a tuple (url, Vec<SocketAddr>))
from resolve_host_ips and validate_url_with_dns_check_with_resolver and
propagate that to callers so the request path uses those pinned SocketAddr(s)
for the TCP connect (e.g., by dialing the SocketAddr(s) directly or configuring
the HTTP client's connector while preserving the original Host header), and
update all callers of validate_url_with_dns_check to accept the new return type
and use the pinned addresses for the actual reqwest connect.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dd016d7-9f5f-4502-b352-545511154326
📒 Files selected for processing (5)
src/openhuman/tools/impl/network/curl.rssrc/openhuman/tools/impl/network/http_request.rssrc/openhuman/tools/impl/network/http_request_tests.rssrc/openhuman/tools/impl/network/url_guard.rssrc/openhuman/tools/impl/network/web_fetch.rs
2 participants
Summary
http_request,curl, andweb_fetchto the existing DNS-aware URL guard before outbound requests are made.google.comDNS.Problem
validate_url()only.Solution
validate_url_with_dns_check().google.comtest with resolver-injected coverage for public and private address outcomes.Submission Checklist
## Related— no feature ID affected.Closes #NNNin the## RelatedsectionImpact
Related
Closes #1926
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/OH-1926-dns-rebinding-network-tools9eb89beeed1b9fe4a4a931151852a0d8e8d26400Validation Run
pnpm --filter openhuman-app format:check— passed via pre-push hookpnpm typecheck— passedpnpm debug rust -- --lib url_guard -- --nocapture;pnpm debug rust -- --lib http_request -- --nocapture;pnpm debug rust -- --lib curl -- --nocapture;pnpm debug rust -- --lib web_fetch -- --nocapture;pnpm debug rust -- --lib network -- --nocapturecargo fmt --manifest-path Cargo.toml --all --check;cargo check --manifest-path Cargo.toml;git diff --check;CODEX_EXPECT_REPO_PATH=/home/ubuntu/workflow/openhuman node scripts/codex-pr-preflight.mjs --strict-path --lightweightcargo check --manifest-path app/src-tauri/Cargo.tomlsuccessfully.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
http_request,curl, andweb_fetchnow perform DNS resolved-IP SSRF checks before outbound requests.Parity Contract
Duplicate / Superseded PR Handling
#1926,DNS rebinding, orvalidate_url_with_dns_checkamong open PRs before starting.Summary by CodeRabbit
Bug Fixes
Tests