fix: honor env proxy for provider guarded fetch#72480
fix: honor env proxy for provider guarded fetch#72480mjamiv wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes provider-guarded fetch to honor standard HTTP proxy env vars ( Confidence Score: 5/5This PR is safe to merge — the change is minimal, well-scoped, and backed by focused regression tests. The logic correctly gates the env-proxy upgrade on two conditions (!dispatcherPolicy && shouldUseEnvHttpProxyForUrl(url)) and delegates to the existing, already-audited withTrustedEnvProxyGuardedFetchMode helper. The NO_PROXY SSRF bypass concern noted in prior review threads is fully handled by shouldUseEnvHttpProxyForUrl (which calls matchesNoProxy internally). fetch-guard.ts also re-checks shouldUseEnvHttpProxyForUrl on each redirect hop, so redirect-induced proxy bypass is not a risk. No security surface changes were introduced. No files require special attention. Reviews (1): Last reviewed commit: "fix: honor env proxy for provider guarde..." | Re-trigger Greptile |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Current main calls fetchWithSsrFGuard from buildGuardedModelFetch without a trusted env-proxy mode, while fetch-guard defaults to strict DNS-pinned behavior; the PR tests cover the HTTPS_PROXY/no-dispatcher case and explicit-dispatcher preservation. Next step before merge Security Review findings
Review detailsBest possible solution: Refresh the branch onto current main, keep the no-dispatcher plus shouldUseEnvHttpProxyForUrl gate and regression tests, place the changelog bullet in the current Unreleased Fixes section, then land it and update related issue #70453. Do we have a high-confidence way to reproduce the issue? Yes. Current main calls fetchWithSsrFGuard from buildGuardedModelFetch without a trusted env-proxy mode, while fetch-guard defaults to strict DNS-pinned behavior; the PR tests cover the HTTPS_PROXY/no-dispatcher case and explicit-dispatcher preservation. Is this the best way to solve the issue? Yes, functionally. Reusing withTrustedEnvProxyGuardedFetchMode behind shouldUseEnvHttpProxyForUrl and a no-dispatcherPolicy guard is the narrow maintainable fix; the remaining blocker is branch freshness, not the core approach. Full review comments:
Overall correctness: patch is correct Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 8f20d03373f8. |
944ac13 to
d545cd2
Compare
|
Added the required Unreleased changelog entry and pushed c6564dd. Local validation passed:
|
c6564dd to
4c6af04
Compare
|
Refreshed this branch onto current upstream/main and force-with-lease pushed 4c6af04. This keeps the provider guarded-fetch env-proxy patch plus the required changelog entry, resolving the merge conflict that appeared after the changelog-only push. Local validation passed after the refresh:
|
|
Post-#76887 status: this is still the focused fix for #70453, but GitHub now reports the branch as When rebasing, preserve #76887's scoped fake-IP host policy and the fix that keeps target host allowlists out of explicit proxy-host validation. After rebase, the useful proof is the focused provider transport test plus Testbox |
|
I rebased this patch locally onto current
Maintainer rebase proof from the local patch:
So this PR is still the right focused fix for #70453; it just needs that rebase shape applied to the branch. |
Summary
Root Cause
fetchWithSsrFGuard, but the default strict mode DNS-pins the final provider hostname before an ambient HTTP(S) proxy can resolve it.HTTPS_PROXYis configured.User-visible / Behavior Changes
Provider model requests now honor standard env proxy routing in the guarded-fetch path when there is no explicit provider transport override. Explicit provider transport settings keep their current behavior.
Security Impact
This only selects the existing trusted env-proxy guarded-fetch preset for provider-owned model request URLs when env proxy rules apply.
Repro + Verification
Environment
HTTPS_PROXYset, no explicit provider dispatcher policySteps
HTTPS_PROXY.Expected
Actual
Evidence
pnpm test src/agents/provider-transport-fetch.test.tspnpm exec oxfmt --check --threads=1 src/agents/provider-transport-fetch.ts src/agents/provider-transport-fetch.test.tsgit diff --checkpnpm check:changed --base upstream/mainHuman Verification
pnpm check:changed --base upstream/main.Compatibility / Migration
Risks and Mitigations
buildProviderRequestDispatcherPolicy()returns no dispatcher policy, with a regression test for explicit dispatcher preservation.