fix(memory): apply env proxy guard to withRemoteHttpResponse#52191
fix(memory): apply env proxy guard to withRemoteHttpResponse#52191XIYBHK wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42db2bf351
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
withTrustedEnvProxyGuardedFetchMode() only changes mode, but fetchWithSsrFGuard() still unconditionally calls resolvePinnedHostnameWithPolicy() before the mode check (src/infra/net/fetch-guard.ts:192-199). That resolver always performs dns.lookup() (src/infra/net/ssrf.ts:310-335), so the fake-IP / DNS-over-proxy setups described in this commit will still fail with ENOTFOUND before EnvHttpProxyAgent is even created. In other words, this branch changes the dispatcher after the failing local-resolution step, so memory status --deep and memory search remain broken for the reported proxy environments.
Useful? React with 👍 / 👎.
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
hasProxyEnvConfigured() is broader than the transport selected here: it returns true for ALL_PROXY and any non-empty proxy var, while EnvHttpProxyAgent only honors HTTP(S) proxy vars with per-protocol semantics (src/infra/net/proxy-env.ts:28-55, and src/infra/net/proxy-env.test.ts:69-80). With only ALL_PROXY set—or with an http:// memory endpoint plus only HTTPS_PROXY—this code still enters trusted_env_proxy mode, drops the pinned dispatcher, and then makes a direct request. That means a sizable set of proxy configurations still will not work, and in the ALL_PROXY case the request can bypass the user's proxy entirely when local DNS happens to resolve.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes memory embedding failures for users behind HTTP proxies by applying the same proxy-env guard already used in the web-tools fetch path.
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/memory/remote-http.ts
Line: 39-41
Comment:
**Fix covers SSRF-blocked fake-IPs but may not resolve true DNS failures**
The PR description states the root cause is `getaddrinfo ENOTFOUND` — DNS truly failing to resolve the embedding-provider hostname. However, looking at `fetchWithSsrFGuard` in `fetch-guard.ts` (lines 192–201), `resolvePinnedHostnameWithPolicy` is called **unconditionally before** the mode branch is evaluated:
```typescript
// fetch-guard.ts — runs in both STRICT and TRUSTED_ENV_PROXY modes
const pinned = await resolvePinnedHostnameWithPolicy(parsedUrl.hostname, {
lookupFn: params.lookupFn,
policy: params.policy,
});
const canUseTrustedEnvProxy =
mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY && hasProxyEnvConfigured();
```
`resolvePinnedHostnameWithPolicy` always calls `dnsLookup` (see `ssrf.ts` line 332), so if the OS cannot resolve the hostname at all (`ENOTFOUND`), the error is thrown before `EnvHttpProxyAgent` is ever used — in both modes.
The fix **does** correctly handle the more common Clash TUN fake-IP scenario where DNS resolves to a private range IP (e.g. `198.18.x.x`) that would be blocked by the SSRF Phase-2 check in strict mode. With `allowedHostnames` matching the configured hostname, `skipPrivateNetworkChecks` becomes `true`, the Phase-2 check is skipped, and `EnvHttpProxyAgent` routes the request through the proxy.
If there are environments where local DNS cannot resolve the hostname at all (e.g. certain corporate split-DNS setups), users would still hit `ENOTFOUND`. Skipping `resolvePinnedHostnameWithPolicy` (or making its DNS step conditional on `canUseTrustedEnvProxy`) in `fetch-guard.ts` would be required to address that scenario — but that's out of scope for this PR and the same limitation already exists in the web-tools proxy path.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): apply proxy guard to withRe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, | ||
| ); |
There was a problem hiding this comment.
The PR description states the root cause is getaddrinfo ENOTFOUND — DNS truly failing to resolve the embedding-provider hostname. However, looking at fetchWithSsrFGuard in fetch-guard.ts (lines 192–201), resolvePinnedHostnameWithPolicy is called unconditionally before the mode branch is evaluated:
// fetch-guard.ts — runs in both STRICT and TRUSTED_ENV_PROXY modes
const pinned = await resolvePinnedHostnameWithPolicy(parsedUrl.hostname, {
lookupFn: params.lookupFn,
policy: params.policy,
});
const canUseTrustedEnvProxy =
mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY && hasProxyEnvConfigured();resolvePinnedHostnameWithPolicy always calls dnsLookup (see ssrf.ts line 332), so if the OS cannot resolve the hostname at all (ENOTFOUND), the error is thrown before EnvHttpProxyAgent is ever used — in both modes.
The fix does correctly handle the more common Clash TUN fake-IP scenario where DNS resolves to a private range IP (e.g. 198.18.x.x) that would be blocked by the SSRF Phase-2 check in strict mode. With allowedHostnames matching the configured hostname, skipPrivateNetworkChecks becomes true, the Phase-2 check is skipped, and EnvHttpProxyAgent routes the request through the proxy.
If there are environments where local DNS cannot resolve the hostname at all (e.g. certain corporate split-DNS setups), users would still hit ENOTFOUND. Skipping resolvePinnedHostnameWithPolicy (or making its DNS step conditional on canUseTrustedEnvProxy) in fetch-guard.ts would be required to address that scenario — but that's out of scope for this PR and the same limitation already exists in the web-tools proxy path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/remote-http.ts
Line: 39-41
Comment:
**Fix covers SSRF-blocked fake-IPs but may not resolve true DNS failures**
The PR description states the root cause is `getaddrinfo ENOTFOUND` — DNS truly failing to resolve the embedding-provider hostname. However, looking at `fetchWithSsrFGuard` in `fetch-guard.ts` (lines 192–201), `resolvePinnedHostnameWithPolicy` is called **unconditionally before** the mode branch is evaluated:
```typescript
// fetch-guard.ts — runs in both STRICT and TRUSTED_ENV_PROXY modes
const pinned = await resolvePinnedHostnameWithPolicy(parsedUrl.hostname, {
lookupFn: params.lookupFn,
policy: params.policy,
});
const canUseTrustedEnvProxy =
mode === GUARDED_FETCH_MODE.TRUSTED_ENV_PROXY && hasProxyEnvConfigured();
```
`resolvePinnedHostnameWithPolicy` always calls `dnsLookup` (see `ssrf.ts` line 332), so if the OS cannot resolve the hostname at all (`ENOTFOUND`), the error is thrown before `EnvHttpProxyAgent` is ever used — in both modes.
The fix **does** correctly handle the more common Clash TUN fake-IP scenario where DNS resolves to a private range IP (e.g. `198.18.x.x`) that would be blocked by the SSRF Phase-2 check in strict mode. With `allowedHostnames` matching the configured hostname, `skipPrivateNetworkChecks` becomes `true`, the Phase-2 check is skipped, and `EnvHttpProxyAgent` routes the request through the proxy.
If there are environments where local DNS cannot resolve the hostname at all (e.g. certain corporate split-DNS setups), users would still hit `ENOTFOUND`. Skipping `resolvePinnedHostnameWithPolicy` (or making its DNS step conditional on `canUseTrustedEnvProxy`) in `fetch-guard.ts` would be required to address that scenario — but that's out of scope for this PR and the same limitation already exists in the web-tools proxy path.
How can I resolve this? If you propose a fix, please make it concise.
|
Thanks for the thorough analysis, @greptile-apps. Re: the P2 observation on unconditional Re: CI failures — The 2 failing jobs ( |
|
Good clarification on both points. On the P2 DNS observation: Makes sense to keep this scoped. The Clash TUN fake-IP → private IP → SSRF block is the real-world failure mode you're fixing, and making DNS conditional on proxy mode would indeed be a broader change to On CI failures: Acknowledged. If those subpath tests are failing on The approach is sound — you're applying the same pattern already validated in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5919180e75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }); | ||
| }; | ||
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
Fresh evidence from this commit is that withRemoteHttpResponse() now sets trusted proxy mode at src/memory/remote-http.ts:40, but fetchWithSsrFGuard() still performs resolvePinnedHostnameWithPolicy() before it checks mode (src/infra/net/fetch-guard.ts:210-216). In the exact proxy setups this patch targets (where local DNS cannot resolve the remote embedding host), that unconditional pre-resolution still fails with ENOTFOUND, so memory calls remain unavailable even when proxy env vars are present.
Useful? React with 👍 / 👎.
| }); | ||
| }; | ||
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
Fresh evidence from this commit is that the mode switch now keys off hasProxyEnvConfigured() at src/memory/remote-http.ts:40, which treats ALL_PROXY as sufficient, while the EnvHttpProxyAgent path only supports HTTP(S)-style proxy vars (src/infra/net/proxy-env.ts:29-55, with coverage in src/infra/net/proxy-env.test.ts:69-78). With only ALL_PROXY (or protocol-mismatched vars), this branch drops pinned DNS dispatch but may not actually proxy the request, creating direct unpinned traffic and bypassing expected proxy routing.
Useful? React with 👍 / 👎.
…y compatibility When HTTP_PROXY/HTTPS_PROXY/ALL_PROXY is configured, withRemoteHttpResponse() now uses withTrustedEnvProxyGuardedFetchMode() so fetchWithSsrFGuard() skips local DNS pre-resolution and routes through the environment proxy instead. This fixes ENOTFOUND errors in proxy environments (e.g. Clash TUN fake-IP) where dns.lookup() cannot resolve the embedding provider hostname directly. Closes openclaw#52162
XIYBHK
force-pushed
the
fix/memory-embeddings-proxy-guard
branch
from
5919180 to
725261b
Compare
|
Rebased onto current The fix has been ported to the new file location — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 725261b6a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }); | ||
| }; | ||
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
Fresh evidence from this commit is the new withTrustedEnvProxyGuardedFetchMode(...) path at withRemoteHttpResponse, but fetchWithSsrFGuard still calls resolvePinnedHostnameWithPolicy before checking mode (src/infra/net/fetch-guard.ts:176-183). In the exact proxy environments this patch targets (where local DNS cannot resolve the remote host), the request still fails with ENOTFOUND before EnvHttpProxyAgent is selected, so memory remote calls remain broken despite the mode switch.
Useful? React with 👍 / 👎.
| }); | ||
| }; | ||
| const { response, release } = await fetchWithSsrFGuard( | ||
| hasProxyEnvConfigured() ? withTrustedEnvProxyGuardedFetchMode(baseOptions) : baseOptions, |
There was a problem hiding this comment.
This new condition uses hasProxyEnvConfigured(), which treats any non-empty proxy var (including ALL_PROXY) as sufficient, but EnvHttpProxyAgent follows HTTP(S)-specific vars and ignores ALL_PROXY (src/infra/net/proxy-env.ts:29-47, plus src/infra/net/proxy-env.test.ts). With only ALL_PROXY set, or with protocol-mismatched vars (for example HTTPS_PROXY for an http:// endpoint), this code still enters trusted mode and can send the request directly without pinned DNS, bypassing the intended proxy path.
Useful? React with 👍 / 👎.
|
Closing this as implemented after Codex review. Current What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Review notes: reviewed against bc73141e8284; fix evidence: commit f408bba9de72. |
2 participants
Summary
withRemoteHttpResponse()callsfetchWithSsrFGuard()without setting a mode, defaulting toSTRICT. In strict mode, local DNS pre-resolution viadns.lookup()runs before the HTTP request, which fails withgetaddrinfo ENOTFOUNDin proxy environments where DNS must go through the proxy.withRemoteHttpResponse()now checkshasProxyEnvConfigured()and, when true, wraps fetch options withwithTrustedEnvProxyGuardedFetchMode()sofetchWithSsrFGuard()usesEnvHttpProxyAgentinstead of pinned DNS.fetchWithSsrFGuard()itself, SSRF policy, or any other code path.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
openclaw memory status --deepandopenclaw memory searchnow work correctly when environment proxy variables (HTTP_PROXY,HTTPS_PROXY,ALL_PROXY) are configured and local DNS cannot resolve the embedding provider hostname.Security Impact (required)
fetchWithSsrFGuardis used, only the mode changes fromSTRICTtoTRUSTED_ENV_PROXYwhen env proxy is detected, which is the same pattern already used byfetchWithWebToolsNetworkGuardinsrc/agents/tools/web-guarded-fetch.ts.Repro + Verification
Environment
HTTPS_PROXY=http://127.0.0.1:7890(Clash TUN with fake-IP mode)Steps
HTTPS_PROXYenv var pointing to an HTTP proxyopenclaw memory status --deep --agent mainExpected
Embeddings status shows as available
Actual (before fix)
getaddrinfo ENOTFOUND <hostname>— Embeddings: unavailableEvidence
post-json.test.ts,embeddings-remote-fetch.test.ts); TypeScript type check passesHuman Verification (required)
fetchWithWebToolsNetworkGuardinsrc/agents/tools/web-guarded-fetch.ts(line 46-48). Unit tests and type check pass.hasProxyEnvConfigured()returns false and behavior is unchanged (default strict mode).Review Conversations
Compatibility / Migration
Failure Recovery (if this breaks)
withRemoteHttpResponse()goes back to callingfetchWithSsrFGuard()without a mode.TRUSTED_ENV_PROXYmode somehow causes issues, embedding requests may fail differently (proxy errors instead of DNS errors). Reverting restores previous behavior.Risks and Mitigations
TRUSTED_ENV_PROXYmode, SSRF DNS pinning is bypassed for the embedding endpoint when a proxy is configured, relying on the proxy for network-level access control.web-guarded-fetch.ts). The SSRFpolicy(allowed hostnames frombuildRemoteBaseUrlPolicy) is still validated byresolvePinnedHostnameWithPolicybefore the mode branch. The proxy itself provides the network boundary.This PR was prepared with AI assistance (Claude Code). The change was code-reviewed and the pattern was verified against the existing proxy-aware code path in
web-guarded-fetch.ts.