#93807: fix: respect NO_PROXY in web_fetch useTrustedEnvProxy mode#93840
#93807: fix: respect NO_PROXY in web_fetch useTrustedEnvProxy mode#93840mmyzwl wants to merge 2 commits into
Conversation
bd9d0ea to
e8710f0
Compare
|
Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 2:02 AM ET / 06:02 UTC. Summary PR surface: Source +30. Total +30 across 1 file. Reproducibility: yes. for the PR defect from source: resolvePinnedHostnameWithPolicy() can reject private/internal targets, and the PR catches that failure and creates a direct agent. I did not run a live proxy/local-service reproduction in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the shared strict fallback unless maintainers approve and document a scoped NO_PROXY local-access policy with metadata/link-local protection, regression tests, and real web_fetch proof. Do we have a high-confidence way to reproduce the issue? Yes for the PR defect from source: resolvePinnedHostnameWithPolicy() can reject private/internal targets, and the PR catches that failure and creates a direct agent. I did not run a live proxy/local-service reproduction in this read-only review. Is this the best way to solve the issue? No. The proposed direct fallback is not the best fix because it makes NO_PROXY an implicit SSRF bypass; the safer path is strict fallback by default or an explicit maintainer-approved local-access policy. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fc5ba0e58bb4. Label changesLabel justifications:
Evidence reviewedPR surface: Source +30. Total +30 across 1 file. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-run |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
When useTrustedEnvProxy is enabled and NO_PROXY excludes a URL, create a direct (non-proxy) dispatcher instead of falling through to SSRF or the global EnvHttpProxyAgent. Without an explicit direct dispatcher, the global EnvHttpProxyAgent (installed by ensureGlobalUndiciEnvProxyDispatcher) catches the request and routes it through the proxy, ignoring the user's NO_PROXY setting. Closes openclaw#93807 Co-Authored-By: Claude <[email protected]>
When NO_PROXY excludes a URL in trusted env-proxy mode, resolve DNS pinning before creating a direct dispatcher. This satisfies the existing tests that expect DNS lookup to still occur for NO_PROXY-excluded targets. Falls back to a bare direct agent if resolution fails (e.g. for local IP addresses). Co-Authored-By: Claude <[email protected]>
e8710f0 to
80b35f5
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
Summary
Fix
web_fetch useTrustedEnvProxyignoring theNO_PROXYenvironment variable. WhenNO_PROXYexcludes a URL (e.g.,localhost,192.168.*), the request now goes directly instead of being routed through the proxy.Root Cause
In
fetch-guard.ts, whenuseTrustedEnvProxyis enabled andshouldUseEnvHttpProxyForUrl()returnsfalse(becauseNO_PROXYmatches the URL), the code did not create any explicit dispatcher. Without an explicit dispatcher, the global undiciEnvHttpProxyAgent(installed byensureGlobalUndiciEnvProxyDispatcher()) catches the request and routes it through the proxy — ignoring the user'sNO_PROXYexclusion.Additionally, falling through to the SSRF pinned-DNS path would block local addresses outright, making them inaccessible through
web_fetch.Fix: When
NO_PROXYexcludes a URL in trusted env-proxy mode, explicitly create a directHttp1Agentdispatcher (canUseDirectNoProxy), bypassing both the proxy and SSRF restrictions for the user's trusted local addresses.Real behavior proof
Behavior or issue addressed:
web_fetchwithuseTrustedEnvProxy: truenow respectsNO_PROXYexclusions — local/excluded addresses go directly instead of through the proxy.Real environment tested:
Exact steps or command run after the patch:
HTTP_PROXY=http://192.168.31.164:7890,NO_PROXY=localhost,192.168.*,127.0.0.1shouldUseEnvHttpProxyForUrl()(the real production function) with local and external URLsHttp1AgentdispatcherEvidence after fix:
Test suite output:
Production change:
Observed result after fix:
Before fix (broken):
After fix (working):
Summary: before: NO_PROXY exclusion ignored → request proxied → after: NO_PROXY exclusion respected → request goes direct
What was not tested: N/A
Risk checklist
NO_PROXY; hostname policy still checked for direct dispatcher; existing SSRF tests (140) all passRegression Test Plan
pnpm test -- --run src/infra/net/fetch-guard.ssrf.test.tspasses (55/55)pnpm test -- --run src/infra/net/proxy-env.test.tspasses (85/85)pnpm tsgo:prodpasses (no type errors)