fix: allow allowed_private_hosts to override DNS validation (Fixes #5122)#5136
fix: allow allowed_private_hosts to override DNS validation (Fixes #5122)#5136ArchBirdie wants to merge 1 commit into
Conversation
- Split private_host_allowed into explicitly_allowed and is_private checks - Skip DNS resolution validation when host is in allowed_private_hosts - Fixes issue where domain names resolving to private IPs were blocked - Applies fix to both web_fetch and http_request tools"
|
@SimianAstronaut7 should I just update in my other pull request branch that implements the http_request private hosts feature? ( this kinda builds on top of #4924 but I'm not sure it's in main yet? ) I'm new to github / sdlc, so I apolgize for not knowing the best way forward... |
|
Thanks for the PR. Before I do a full review, could you please update this branch against current GitHub currently reports the PR as dirty, and the current diff still contains literal conflict markers in This also appears to overlap with #4924, which is still open and is the branch that introduced the related After conflict resolution, please refresh validation evidence for the current head, especially the cases covering:
|
|
Yes, this one was going to build on top of 4924 to fix functionality around whitelisting internal hosts when DNS is involved.
I will do as you've suggested, and merge both of these into one fresh PR with the current codebase soon.
Noted the additional test requirements.
…On Friday, May 1st, 2026 at 5:50 PM, Dan Gilles ***@***.***> wrote:
Audacity88 left a comment [(zeroclaw-labs/zeroclaw#5136)](#5136 (comment))
Thanks for the PR. Before I do a full review, could you please update this branch against current master and resolve the merge conflicts?
GitHub currently reports the PR as dirty, and the current diff still contains literal conflict markers in src/tools/http_request.rs (<<<<<<< Updated upstream / >>>>>>> Stashed changes). The linked issue [#5122](#5122) still looks valid and open, but this branch needs to be clean before the fix can be reviewed.
This also appears to overlap with [#4924](#4924), which is still open and is the branch that introduced the related allowed_private_hosts / blocked_domains work for http_request. When you update this, please clarify whether [#5136](#5136) is meant to be a follow-up on top of [#4924](#4924), a replacement, or a separate minimal fix for [#5122](#5122).
After conflict resolution, please refresh validation evidence for the current head, especially the cases covering:
- web_fetch.allowed_private_hosts with domain names resolving to private IPs
- http_request.allowed_private_hosts with the same scenario
- unchanged blocking for private hosts that are not explicitly allowed
—
Reply to this email directly, [view it on GitHub](#5136 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AYEKAT7HER4QJ5NGUZ6H6WD4YTPU7AVCNFSM6AAAAACXFFY5FSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGNRQG42TENBZHA).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Thanks for following up on this. I’m going to close this PR as stale because the branch is dirty against current The underlying bug in #5122 should stay open: domain names in |
Summary
Describe this PR in 2-5 bullets:
Label Snapshot (required)
Change Metadata
Linked Issue
Supersede Attribution (required when Supersedes # is used)
Validation Evidence (required)
Commands and result summary:
cargo fmt --all -- --check
cargo clippy --all-targets -- -D warnings
cargo test
Security Impact (required)
Security Note: This fix actually improves security by making the allowed_private_hosts configuration work as intended. Previously, users might have bypassed security by using IPs instead of domains, but now the explicit allowlist functions correctly.
Privacy and Data Hygiene (required)
Compatibility / Migration
i18n Follow-Through (required when docs or user-facing wording changes)
Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Agent Collaboration Notes (recommended)
Rollback Plan (required)
Risks and Mitigations
Risk: None identified
This is a narrowly-scoped bug fix that makes existing configuration work as intended. The change is minimal (~30 lines across 2 files), well-tested, and backward compatible.