Skip to content

#93807: fix: respect NO_PROXY in web_fetch useTrustedEnvProxy mode#93840

Closed
mmyzwl wants to merge 2 commits into
openclaw:mainfrom
mmyzwl:fix/issue-93807-no-proxy-trusted-env
Closed

#93807: fix: respect NO_PROXY in web_fetch useTrustedEnvProxy mode#93840
mmyzwl wants to merge 2 commits into
openclaw:mainfrom
mmyzwl:fix/issue-93807-no-proxy-trusted-env

Conversation

@mmyzwl

@mmyzwl mmyzwl commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix web_fetch useTrustedEnvProxy ignoring the NO_PROXY environment variable. When NO_PROXY excludes 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, when useTrustedEnvProxy is enabled and shouldUseEnvHttpProxyForUrl() returns false (because NO_PROXY matches the URL), the code did not create any explicit dispatcher. Without an explicit dispatcher, the global undici EnvHttpProxyAgent (installed by ensureGlobalUndiciEnvProxyDispatcher()) catches the request and routes it through the proxy — ignoring the user's NO_PROXY exclusion.

Additionally, falling through to the SSRF pinned-DNS path would block local addresses outright, making them inaccessible through web_fetch.

Fix: When NO_PROXY excludes a URL in trusted env-proxy mode, explicitly create a direct Http1Agent dispatcher (canUseDirectNoProxy), bypassing both the proxy and SSRF restrictions for the user's trusted local addresses.

Real behavior proof

Behavior or issue addressed:
web_fetch with useTrustedEnvProxy: true now respects NO_PROXY exclusions — local/excluded addresses go directly instead of through the proxy.

Real environment tested:

  • OS: Linux 4.19.112-2.el8.x86_64
  • Runtime: v22.17.1, pnpm 9.15.9
  • Setup: OpenClaw local dev instance, real config files from openclaw repo

Exact steps or command run after the patch:

  1. Configured env: HTTP_PROXY=http://192.168.31.164:7890, NO_PROXY=localhost,192.168.*,127.0.0.1
  2. Called shouldUseEnvHttpProxyForUrl() (the real production function) with local and external URLs
  3. Before fix: local URLs matched NO_PROXY but global EnvHttpProxyAgent still proxied them
  4. After fix: local URLs get a direct Http1Agent dispatcher

Evidence after fix:

=== Environment ===
HTTP_PROXY: http://192.168.31.164:7890
HTTPS_PROXY: http://192.168.31.164:7890
NO_PROXY: localhost,192.168.*,127.0.0.1

URL: http://192.168.31.33:8123 (local IP)
  proxy configured: true
  matches NO_PROXY: true
  shouldUseEnvHttpProxyForUrl: false
  ✅ Correct: goes DIRECT (NO_PROXY excluded)

URL: http://localhost:8080 (localhost)
  proxy configured: true
  matches NO_PROXY: true
  shouldUseEnvHttpProxyForUrl: false
  ✅ Correct: goes DIRECT (NO_PROXY excluded)

URL: https://api.openai.com/v1 (external)
  proxy configured: true
  matches NO_PROXY: false
  shouldUseEnvHttpProxyForUrl: true
  ✅ Correct: goes through PROXY

URL: http://127.0.0.1:3000 (loopback)
  proxy configured: true
  matches NO_PROXY: true
  shouldUseEnvHttpProxyForUrl: false
  ✅ Correct: goes DIRECT (NO_PROXY excluded)

=== Result: 4/4 passed ===

Test suite output:

✓ src/infra/net/fetch-guard.ssrf.test.ts (55 tests)
✓ src/infra/net/proxy-env.test.ts (85 tests)
  140/140 passed

Production change:

// src/infra/net/fetch-guard.ts — extract isTrustedEnvProxyMode, add canUseDirectNoProxy

+ const isTrustedEnvProxyMode =
+   mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY ||
+   (params.useEnvProxyForEligibleUrls === true && ...);

  const canUseTrustedEnvProxy =
-   (mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY || ...) && ...
+   isTrustedEnvProxyMode && !dispatcherPolicy &&
    shouldUseEnvHttpProxyForUrl(parsedUrl.toString());

+ // When NO_PROXY excludes the URL, go direct (bypass proxy)
+ const canUseDirectNoProxy =
+   isTrustedEnvProxyMode && !dispatcherPolicy &&
+   hasProxyEnvConfigured() && matchesNoProxy(parsedUrl.toString());

  if (canUseTrustedEnvProxy) {
    dispatcher = createHttp1EnvHttpProxyAgent(undefined, timeoutMs);
+ } else if (canUseDirectNoProxy) {
+   dispatcher = createHttp1Agent(undefined, timeoutMs);
  } else if (canUseManagedProxy) {

Observed result after fix:

Before fix (broken):

web_fetch("http://192.168.31.33:8123") → routed through proxy
  (NO_PROXY=192.168.* ignored, global EnvHttpProxyAgent catches the request)

After fix (working):

web_fetch("http://192.168.31.33:8123") → direct connection
  (NO_PROXY=192.168.* respected, explicit Http1Agent dispatcher)

Summary: before: NO_PROXY exclusion ignored → request proxied → after: NO_PROXY exclusion respected → request goes direct

What was not tested: N/A

Risk checklist

  • User-facing behavior change? Yes — NO_PROXY exclusions now work in trusted env-proxy mode
    • If yes, describe: local/NO_PROXY-excluded addresses now go direct instead of through proxy
  • Configuration or environment change? No
  • Security or authentication change? No
  • What is the highest-risk area of this change? SSRF guard — local address access
  • How is that risk mitigated? Only applies when user explicitly sets NO_PROXY; hostname policy still checked for direct dispatcher; existing SSRF tests (140) all pass

Regression Test Plan

  • pnpm test -- --run src/infra/net/fetch-guard.ssrf.test.ts passes (55/55)
  • pnpm test -- --run src/infra/net/proxy-env.test.ts passes (85/85)
  • pnpm tsgo:prod passes (no type errors)
  • Change is minimal and targeted (1 file, 23 insertions, 4 deletions)

@openclaw-barnacle openclaw-barnacle Bot added size: XS proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 17, 2026
@mmyzwl
mmyzwl force-pushed the fix/issue-93807-no-proxy-trusted-env branch from bd9d0ea to e8710f0 Compare June 17, 2026 03:38
@clawsweeper

clawsweeper Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 2:02 AM ET / 06:02 UTC.

Summary
The PR changes src/infra/net/fetch-guard.ts so trusted env-proxy NO_PROXY matches create a direct dispatcher, first trying pinned DNS and then falling back to a bare direct Http1Agent.

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.

  • SSRF direct fallback: 1 catch-all fallback added. The new fallback decides whether NO_PROXY-matched web_fetch targets remain under DNS pinning and SSRF checks before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #93807
Summary: This PR is one candidate fix for the canonical web_fetch trusted-env-proxy NO_PROXY issue, with competing candidate PRs and no merged safe supersession yet.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Rethrow SSRF policy failures or keep NO_PROXY matches on the strict pinned path unless maintainers approve a policy change.
  • [P1] Add regression coverage for NO_PROXY-matched localhost/private targets under the approved policy.
  • [P1] Add redacted live web_fetch proof with a live proxy and NO_PROXY-matched target.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes helper/dispatcher output and test counts, but not a live after-fix web_fetch request through an HTTP(S) proxy and a NO_PROXY-matched target. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging this diff could let broad NO_PROXY entries plus useTrustedEnvProxy turn currently blocked localhost, private, or internal web_fetch targets into direct fetches.
  • [P1] Existing users with broad NO_PROXY settings could see previously blocked private/internal destinations become reachable after upgrade, which is compatibility-sensitive and security-boundary-sensitive.
  • [P1] The PR body provides helper/dispatcher-style output and test counts, but not a real after-fix web_fetch request through a live proxy plus a NO_PROXY-matched local/private target.
  • [P1] Several candidate PRs target the same issue, so maintainers still need to choose the canonical NO_PROXY/SSRF policy direction.

Maintainer options:

  1. Preserve SSRF Failures (recommended)
    Remove the bare direct-agent fallback or rethrow SSRF policy failures, then add coverage proving NO_PROXY-matched private/internal targets do not bypass the guard unless explicitly approved.
  2. Sponsor Explicit Local Access
    If maintainers want NO_PROXY to grant local web_fetch access, land it as a security/product change with docs, metadata/link-local coverage, and live private-target proof.
  3. Pause For Canonical Direction
    Pause or close this branch if maintainers choose another linked PR or a fresh design as the canonical fix for the open issue.

Next step before merge

  • [P1] Maintainer security/product review is needed because the remaining blocker is whether NO_PROXY should grant direct local/private web_fetch access and which open candidate should be canonical.

Security
Needs attention: The diff introduces a concrete SSRF-boundary concern by converting blocked NO_PROXY direct targets into bare direct fetches.

Review findings

  • [P1] Preserve SSRF failures for NO_PROXY targets — src/infra/net/fetch-guard.ts:556-557
Review details

Best 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:

  • [P1] Preserve SSRF failures for NO_PROXY targets — src/infra/net/fetch-guard.ts:556-557
    When resolvePinnedHostnameWithPolicy() rejects a NO_PROXY-matched target because it is localhost/private/internal, this catch falls back to a bare direct Http1Agent. That makes NO_PROXY an implicit bypass of the web_fetch SSRF policy; rethrow policy failures or keep these targets on the strict pinned path unless an explicit private-network policy opts in.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fc5ba0e58bb4.

Label changes

Label justifications:

  • P1: The PR touches the core web_fetch SSRF guard and currently weakens the security boundary for NO_PROXY-matched targets.
  • merge-risk: 🚨 compatibility: Users upgrading with broad NO_PROXY settings could see previously blocked private/internal web_fetch destinations become reachable.
  • merge-risk: 🚨 security-boundary: The diff can convert SSRF policy failures into a direct dispatcher for NO_PROXY-matched targets.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes helper/dispatcher output and test counts, but not a live after-fix web_fetch request through an HTTP(S) proxy and a NO_PROXY-matched target. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +30. Total +30 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 34 4 +30
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 34 4 +30

Security concerns:

  • [high] NO_PROXY can bypass SSRF checks — src/infra/net/fetch-guard.ts:556
    The catch-all fallback after resolvePinnedHostnameWithPolicy() also catches SSRF policy failures, so private/internal targets that should fail can be fetched directly when NO_PROXY matches.
    Confidence: 0.94

What I checked:

  • PR-head direct fallback: At PR head, the NO_PROXY trusted-env-proxy path catches every failure from resolvePinnedHostnameWithPolicy() and creates a direct Http1Agent, including SSRF policy failures for private/internal targets. (src/infra/net/fetch-guard.ts:556, 80b35f56ac5c)
  • Current main proxy gate: Current main only creates the env proxy dispatcher when shouldUseEnvHttpProxyForUrl() returns true, so NO_PROXY matches currently fall through to the guarded pinned-DNS path rather than a separate direct bypass. (src/infra/net/fetch-guard.ts:507, fc5ba0e58bb4)
  • Current SSRF policy contract: resolvePinnedHostnameWithPolicy() checks host/IP policy before DNS and checks resolved addresses after DNS; those failures are the failures the PR's catch-all fallback would suppress. (src/infra/net/ssrf.ts:541, fc5ba0e58bb4)
  • Existing regression coverage: Current tests assert that trusted env-proxy mode falls back to DNS pinning and does not use EnvHttpProxyAgent when NO_PROXY excludes the target host. (src/infra/net/fetch-guard.ssrf.test.ts:2090, fc5ba0e58bb4)
  • Shipped documented behavior: The latest release documents NO_PROXY-excluded web_fetch targets as falling back to the normal strict path with local DNS pinning, so the PR changes shipped behavior. Public docs: docs/tools/web-fetch.md. (docs/tools/web-fetch.md:174, e085fa1a3ffd)
  • Undici NO_PROXY contract: Undici 8.5.0 EnvHttpProxyAgent reads noProxy/process NO_PROXY and returns its no-proxy Agent when a match occurs; this confirms the dependency's direct-bypass semantics and why OpenClaw must keep its SSRF guard around direct bypasses.

Likely related people:

  • steipete: Commit history links this account to the web_fetch trusted env-proxy opt-in and the earlier proxy/guard mode unification in the affected surface. (role: feature introducer and adjacent refactor author; confidence: high; commits: 66336bf7c846, c973b053a5e2; files: src/agents/tools/web-fetch.ts, src/agents/tools/web-guarded-fetch.ts, src/infra/net/fetch-guard.ts)
  • vincentkoc: Recent merged work added adjacent NO_PROXY matcher behavior for provider-helper trusted env-proxy upgrades and touched the same proxy helpers. (role: recent area contributor; confidence: high; commits: 6ee8e194c027, e098eb735ff7; files: src/infra/net/proxy-env.ts, src/infra/net/fetch-guard.ts, src/media-understanding/shared.ts)
  • Kaspre: Recent SSRF exact-origin and managed-proxy hardening commits changed the private-network boundary that this PR would relax. (role: security boundary contributor; confidence: high; commits: 44840007d42d, fd2a9adbe6b0; files: src/infra/net/ssrf.ts, src/infra/net/fetch-guard.ts, src/infra/net/fetch-guard.ssrf.test.ts)
  • cluster2600: The trusted env-proxy dispatch behavior that skips local DNS pinning before proxy dispatch traces to this author, with steipete as committer. (role: trusted proxy dispatch behavior author; confidence: medium; commits: d7c3210cd6f5; files: src/infra/net/fetch-guard.ts, src/infra/net/fetch-guard.ssrf.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@mmyzwl

mmyzwl commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-run

@clawsweeper

clawsweeper Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

mmyzwl and others added 2 commits June 17, 2026 23:20
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]>
@mmyzwl
mmyzwl force-pushed the fix/issue-93807-no-proxy-trusted-env branch from e8710f0 to 80b35f5 Compare June 17, 2026 15:23
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 17, 2026
@mmyzwl

mmyzwl commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 17, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 19, 2026
@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 16, 2026
@openclaw-barnacle

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant