fix: always set NO_PROXY to bypass Squid for localhost#1032
Conversation
When HTTP_PROXY is set but NO_PROXY is not, HTTP clients (Go net/http, Python requests, curl, etc.) voluntarily route localhost-bound requests through Squid, which rejects them with 403 because localhost is not in the domain allowlist. This broke test frameworks that start local servers (go/echo, python/uvicorn, deno/fresh). The fix unconditionally sets NO_PROXY with localhost, 127.0.0.1, ::1, 0.0.0.0, plus the Squid and agent container IPs. The enableHostAccess and enableApiProxy branches now append to this baseline instead of conditionally creating it. Also adds an iptables RETURN rule for 0.0.0.0 as defense-in-depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR prevents localhost-bound HTTP requests from being routed through the Squid proxy by always setting a baseline NO_PROXY/no_proxy in the agent container, then appending host-access and api-proxy addresses when enabled.
Changes:
- Always initialize
NO_PROXY/no_proxywith localhost/loopback + squid/agent IPs; append host gateway / api-proxy IPs when enabled. - Add an iptables NAT
RETURNrule for destination0.0.0.0as defense-in-depth. - Add unit tests covering the new
NO_PROXYbaseline behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/docker-manager.ts | Always sets baseline NO_PROXY/no_proxy and switches host/api-proxy logic to append. |
| containers/agent/setup-iptables.sh | Adds a NAT RETURN rule for 0.0.0.0 alongside existing localhost bypass rules. |
| src/docker-manager.test.ts | Adds tests asserting baseline NO_PROXY presence and host-access append behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| environment.NO_PROXY = `localhost,127.0.0.1,::1,0.0.0.0,${networkConfig.squidIp},${networkConfig.agentIp}`; | ||
| environment.no_proxy = environment.NO_PROXY; |
There was a problem hiding this comment.
no_proxy is initialized to mirror NO_PROXY here, but later additionalEnv (from --env) can override NO_PROXY without also updating no_proxy, leaving them inconsistent. Since many HTTP clients prefer one casing over the other, this can reintroduce proxying issues for users who set --env NO_PROXY=.... Consider normalizing after additionalEnv is applied (e.g., if either key is set, set the other to the same value, with a clear precedence rule).
There was a problem hiding this comment.
Addressed in d38f98f. After additionalEnv is applied, the code now detects if NO_PROXY and no_proxy have diverged and syncs them. If --env NO_PROXY=... was passed, no_proxy gets synced to match, and vice versa. Added 2 tests covering both override directions.
Node.js Build Test Results ✅
Overall: PASS
|
Smoke Test Results — Copilot Engine✅ GitHub MCP: Last 2 merged PRs:
✅ Playwright: Overall: PASS
|
|
Smoke Test Results
Overall: PASS
|
Go Build Test Results
Overall: ✅ PASS
|
.NET Build Test Results
Overall: ✅ PASS Run outputhello-world: json-parse: {
"Name": "AWF Test",
"Version": 1,
"Success": true
}
Name: AWF Test, Success: True
|
🦀 Rust Build Test Results
Overall: ✅ PASS
|
Deno Build Test Results
Overall: ✅ PASS
|
Chroot Version Comparison Results
Result: Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Java Build Test Results
Overall: PASS ✅
|
Address review feedback: when --env overrides NO_PROXY (or no_proxy) without setting the other casing, HTTP clients that prefer the other casing would still route through Squid. After additionalEnv is applied, detect the inconsistency and sync both variables with clear precedence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Build Test: Bun Results
Overall: PASS ✅
|
Node.js Build Test Results
Overall: ✅ PASS
|
Build Test: Deno Results
Overall: ✅ PASS
|
C++ Build Test Results
Overall: PASS 🎉
|
|
Smoke Test Results — PASS
|
|
GitHub MCP review: ✅ docs: add sandbox design rationale (Docker vs microVMs); docs: update runner and architecture compatibility
|
Java Build Test Results
Overall: PASS ✅
|
Rust Build Test Results
Overall: PASS
|
|
Smoke test results for run
Overall: PASS
|
Chroot Version Comparison Results
Overall: Tests did not fully pass — Python and Node.js versions differ between host and chroot environments.
|
Go Build Test Results ✅
Overall: PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (environment.NO_PROXY !== environment.no_proxy) { | ||
| if (config.additionalEnv?.NO_PROXY) { | ||
| environment.no_proxy = environment.NO_PROXY; | ||
| } else if (config.additionalEnv?.no_proxy) { | ||
| environment.NO_PROXY = environment.no_proxy; | ||
| } | ||
| } |
There was a problem hiding this comment.
The sync logic doesn't explicitly handle the edge case where both NO_PROXY and no_proxy are provided in additionalEnv with different values. In this case, NO_PROXY takes precedence (line 460 executes), but this behavior isn't documented. Consider adding a comment explaining the precedence order, or handling this case more explicitly to make the behavior clear to future maintainers.
| # Allow localhost traffic (for stdio MCP servers and test frameworks) | ||
| echo "[iptables] Allow localhost traffic..." | ||
| iptables -t nat -A OUTPUT -o lo -j RETURN | ||
| iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN |
There was a problem hiding this comment.
The established pattern in this codebase for iptables bypass rules is to pair NAT RETURN with filter ACCEPT (see agent self-traffic bypass at lines 75-77, host gateway bypass at lines 158-161, network gateway bypass at lines 185-186). The 0.0.0.0 rule here only has NAT RETURN without a corresponding filter ACCEPT. While 0.0.0.0 isn't a typical destination address, consider following the established pattern for consistency, or add a comment explaining why the filter rule is intentionally omitted.
| iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN | |
| iptables -t nat -A OUTPUT -d 127.0.0.0/8 -j RETURN | |
| # Note: 0.0.0.0 is not used as a real destination address; this NAT RETURN | |
| # rule exists only to avoid redirecting any such traffic to Squid. A matching | |
| # filter-table ACCEPT rule is intentionally omitted. |
Summary
NO_PROXYin the agent container environment so HTTP clients don't voluntarily route localhost-bound requests through Squid (which rejects them with 403)NO_PROXYincludeslocalhost,127.0.0.1,::1,0.0.0.0, plus the Squid and agent container IPsenableHostAccessandenableApiProxynow append to this baseline instead of conditionally creatingNO_PROXYRETURNrule for0.0.0.0as defense-in-depthProblem
When
HTTP_PROXYis set butNO_PROXYis not (the default case without--enable-host-accessor--api-proxy), HTTP clients (Go'snet/http, Python'srequests,curl, etc.) see the proxy env var and voluntarily route all traffic through Squid — including requests tolocalhost. Squid checks its domain allowlist, finds localhost is not allowed, and returns 403 Forbidden.This broke test frameworks that start local servers (e.g.,
go testwith Echo, Python with uvicorn, Deno with Fresh).Changes
src/docker-manager.tsNO_PROXYwith localhost entries + agent/squid IPscontainers/agent/setup-iptables.sh0.0.0.0RETURN rule (belt-and-suspenders)src/docker-manager.test.tsTest plan
npm run build— TypeScript compiles cleanlynpm test— All 798 unit tests pass (including 3 new tests)🤖 Generated with Claude Code