Greptile Summary

This PR routes fetchWithSsrFGuard through the system HTTP proxy (via EnvHttpProxyAgent) in STRICT mode whenever proxy env vars are present, fixing real-world failures in proxy-only network environments. However, the implementation has two security regressions that should be addressed before merging.

Key issues:

• ALL_PROXY-only deployments silently lose DNS-pinning protection: hasProxyEnvConfigured() returns true for ALL_PROXY/all_proxy, but EnvHttpProxyAgent explicitly ignores those variables (documented in proxy-env.ts). When only ALL_PROXY is set, the new branch fires, EnvHttpProxyAgent is used, but no proxy is actually applied — the connection becomes a direct connection without the pinned DNS lookup, stripping the anti-DNS-rebinding guarantee. Using hasEnvHttpProxyConfigured() (already available in proxy-env.ts) would restrict the branch to cases where EnvHttpProxyAgent will actually route through a proxy.
• STRICT mode silently adopts TRUSTED_ENV_PROXY semantics for any proxy-env deployment: The distinction between modes exists precisely so that operators can opt into the weaker DNS-rebinding posture that proxy routing entails. Unconditionally using EnvHttpProxyAgent in STRICT mode when proxy vars are set erases that boundary. A safer alternative is to use createPinnedDispatcher with { mode: "env-proxy" } which threads EnvHttpProxyAgent through the existing pinned-dispatcher path.
• canUseTrustedEnvProxy is dead code: It is a strict subset of hasProxyEnvConfigured() and no longer affects control flow.

Confidence Score: 2/5

• Not safe to merge as-is — the ALL_PROXY-only scenario silently removes DNS-pinning SSRF protection, and STRICT mode loses its anti-DNS-rebinding guarantee for any proxy-env deployment.
• The functional goal (making web_fetch work through HTTP proxies) is correct, and the SSRF hostname validation call is preserved. However, the implementation condition uses hasProxyEnvConfigured() which includes ALL_PROXY, a value ignored by EnvHttpProxyAgent, creating a case where the pinned dispatcher is bypassed but no proxy is used. Additionally, unconditionally using EnvHttpProxyAgent (without the pinned lookup) in STRICT mode removes the DNS-rebinding protection that is the core security property of that mode.
• src/infra/net/fetch-guard.ts — the changed condition on line 198 and its interaction with ALL_PROXY semantics and STRICT mode DNS-pinning.

This is a comment left during a code review.
Path: src/infra/net/fetch-guard.ts
Line: 198

Comment:
**`ALL_PROXY` triggers `EnvHttpProxyAgent` but is silently ignored by it**

`hasProxyEnvConfigured()` (in `proxy-env.ts`) returns `true` for `ALL_PROXY`/`all_proxy`, but your own codebase documents that `EnvHttpProxyAgent` ignores `ALL_PROXY` (see the comment in `resolveEnvHttpProxyUrl`):

```
// - ALL_PROXY is ignored by EnvHttpProxyAgent
```

So when *only* `ALL_PROXY` is set (no `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY` variants), this new branch fires:
1. `hasProxyEnvConfigured()` returns `true`
2. `new EnvHttpProxyAgent()` is selected — bypassing `createPinnedDispatcher`
3. `EnvHttpProxyAgent` has no configured proxy (it ignored `ALL_PROXY`), so it falls back to a **direct connection without the pinned lookup**
4. DNS is re-resolved freely at connection time — the anti-DNS-rebinding protection provided by `createPinnedDispatcher` is silently lost

The fix should use `hasEnvHttpProxyConfigured()` (already exported from `proxy-env.ts`) which only returns `true` when `EnvHttpProxyAgent` would actually route through a proxy:

```suggestion
      if (canUseTrustedEnvProxy || hasEnvHttpProxyConfigured()) {
```

This ensures the branch is only taken when a proxy will actually be used, preserving DNS-pinning protection for `ALL_PROXY`-only configurations.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/infra/net/fetch-guard.ts
Line: 196-198

Comment:
**`STRICT` mode silently loses DNS-pinning protection when proxy env vars are present**

The separation between `STRICT` and `TRUSTED_ENV_PROXY` modes was architecturally intentional. The whole point of the pinned dispatcher in `STRICT` mode is to prevent DNS rebinding attacks: DNS is resolved once, validated against the SSRF policy, and then the connection is pinned to that exact IP so a racing DNS change cannot redirect the connection to an internal address.

When `EnvHttpProxyAgent` is used instead, the CONNECT tunnel carries the **original hostname** to the proxy, which then performs its own DNS resolution at connection time. The validated IP from `resolvePinnedHostnameWithPolicy` is never actually used for the connection. This means:

1. `resolvePinnedHostnameWithPolicy("evil-rebinding.example")` resolves to `1.2.3.4` → passes SSRF check ✓
2. DNS TTL expires; attacker's nameserver now points `evil-rebinding.example` → `192.168.1.10`
3. `EnvHttpProxyAgent` sends `CONNECT evil-rebinding.example:443` → proxy resolves → `192.168.1.10`
4. SSRF protection bypassed

`TRUSTED_ENV_PROXY` mode was an **explicit operator opt-in** that accepted this trade-off. This PR promotes the same weakened posture to `STRICT` mode for any deployment with proxy env vars — without any explicit acknowledgement in the mode name or call sites.

A safer approach that preserves the mode boundary would be to create the dispatcher via `createPinnedDispatcher` with `{ mode: "env-proxy" }`:

```typescript
} else if (params.pinDns !== false) {
  const proxyPolicy = hasEnvHttpProxyConfigured()
    ? { ...params.dispatcherPolicy, mode: "env-proxy" as const }
    : params.dispatcherPolicy;
  dispatcher = createPinnedDispatcher(pinned, proxyPolicy, params.policy);
}
```

This routes through the proxy while still threading the pinned `lookup` through the `connect` option of `EnvHttpProxyAgent`, preserving the DNS-pinning contract at the undici layer for the proxy connection itself.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/infra/net/fetch-guard.ts
Line: 196-198

Comment:
**`canUseTrustedEnvProxy` is now dead code**

Since the updated condition is `canUseTrustedEnvProxy || hasProxyEnvConfigured()`, and `canUseTrustedEnvProxy` is defined as `mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY && hasProxyEnvConfigured()`, it is always a strict subset of `hasProxyEnvConfigured()`. The variable can never be `true` when `hasProxyEnvConfigured()` is `false`, so it no longer changes the branching outcome. The variable (and the double call to `hasProxyEnvConfigured()`) can be removed:

```suggestion
      const hasProxy = hasProxyEnvConfigured();
      if (hasProxy) {
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix(infra/net): use ..."}

Thanks for the thorough review. All three points were valid — pushed an updated commit addressing all of them:

1. ALL_PROXY edge case — Switched to hasEnvHttpProxyConfigured() which only returns true when HTTP_PROXY/HTTPS_PROXY is actually set (matching EnvHttpProxyAgent's behavior).

2. DNS-rebinding protection — Replaced the bare EnvHttpProxyAgent() with createPinnedDispatcher(pinned, { mode: "env-proxy" }). This uses the existing env-proxy mode in createPinnedDispatcher which threads the pinned DNS lookup into the connect option of EnvHttpProxyAgent. STRICT mode's DNS-pinning contract is now preserved even when routing through the proxy.

3. Dead code — canUseTrustedEnvProxy is no longer redundant since we reverted to only using bare EnvHttpProxyAgent for the explicit TRUSTED_ENV_PROXY mode.

The updated approach: STRICT mode + proxy env → pinned dispatcher with mode: "env-proxy" (DNS pinning preserved, routed through proxy). TRUSTED_ENV_PROXY → bare EnvHttpProxyAgent (explicit opt-in, no pinning). No proxy → direct pinned dispatcher (existing behavior).

Rebased on latest main (c3b05fc) — the CI failures were from being behind main (type errors in channelContentConfig.ts, groupTemplates.ts, etc.), unrelated to this change.

Regarding the Codex P1 about DNS pinning being lost in env-proxy mode: the env-proxy path in createPinnedDispatcher (in ssrf.ts) already handles this — it passes connect: withPinnedLookup(lookup, policy.connect) to EnvHttpProxyAgent. The connect option controls the target connection (after the CONNECT tunnel to the proxy), so the pinned DNS lookup IS threaded through. This mode was intentionally designed for exactly this use case.

Pushed fix for the protocol-sensitive proxy detection — now passes parsedUrl.protocol to hasEnvHttpProxyConfigured() so http:// URLs only trigger env-proxy when HTTP_PROXY is actually set.

Pushed fix for the explicit-proxy and direct-fallback concerns:

• Only override to env-proxy when params.dispatcherPolicy?.mode is not already set
• Callers with mode: "explicit-proxy" (Telegram media downloads) or mode: "direct" (fallback recovery) are left untouched
• The env-proxy override only fires for the default no-mode case (web_fetch, web_search, etc.)

Added !params.policy?.allowPrivateNetwork guard — callers that explicitly allow private/local hosts (BlueBubbles attachments, remote memory over LAN) stay on the direct pinned path. Env-proxy only activates for the default public-internet case. Full pnpm check passes locally.

Added !params.policy?.dangerouslyAllowPrivateNetwork — both the legacy and current alias are now honored. Callers using either field stay on the direct pinned path.

Note: pnpm check has a pre-existing type error from f1ce679 (Discord commandDeploymentMode — merged to main just now). Our file passes all checks independently.

Bumping this — the PR has 7 bot reviews but zero human reviews so far.

This fix is critical for anyone running OpenClaw behind an HTTP CONNECT proxy (e.g., OpenShell sandboxes, corporate networks). Without it, fetch-guard blocks all outbound requests because DNS doesn't resolve inside the proxy namespace — we currently work around it with a post-install patch script (openclaw-fetch-guard-patch.py), which is fragile and breaks on every update.

The latest commit from @kkav004 addresses the dangerouslyAllowPrivateNetwork alias. The bot feedback about STRICT mode DNS-pinning is worth discussing, but in proxy environments the proxy itself handles DNS resolution and enforces network policy, so the pinning tradeoff is acceptable — that's exactly what TRUSTED_ENV_PROXY mode was designed for.

Would really appreciate human eyes on this. Happy to test any requested changes.

cc @steipete @obviyus

Also cc @scoootscooob @gumadeiras — would appreciate a human review on this one. It's been open since March 19 with only bot reviews so far.

Addressed the latest review thread in 3a37bf4: the direct-path bypass is now keyed to the current request hostname instead of any non-empty allowedHostnames, and src/infra/net/fetch-guard.ssrf.test.ts now covers both the matching-host direct path and unrelated-allowlist proxy path.

Landed on main.

• Landed source commit: 3a37bf4
• Merge commit: 082778d

Thanks @kkav004.