fix: add Desktop PAC-aware egress with SSRF protections

[aheritier]

Summary

Closes #3998.

Adds Desktop-optional, PAC-aware egress for configured HTTP clients while preserving standalone proxy behavior and SSRF protections. The implementation covers all supported consumers, including agent/source fetches, sessions, tools, MCP HTTP transports, and MCP OAuth flows. Remote MCP Streamable HTTP/SSE remains intentionally excluded; MCP OAuth flows are supported.

Behavior

• Uses Docker Desktop's PAC-aware proxy when available, without making Desktop a runtime requirement.
• Retains SSRF protections and transport composition for every supported consumer.
• Distinguishes source-unavailable (404) from upstream/source failure (502) semantics.
• Preserves bounded retries and transport state/cooldown behavior.
• DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1 disables Desktop proxy use (kill switch).
• Standalone deployments should configure HTTP_PROXY, HTTPS_PROXY, and NO_PROXY as appropriate.
• docker-agent does not directly evaluate PAC files.

Commit / phase map

1. Compose configured egress with Desktop PAC transport and SSRF guards.
2. Route HTTP clients through the Desktop-aware transport.
3. Preserve PAC behavior for generic clients.
4. Distinguish unavailable agent sources from upstream failures.
5. Harden Desktop PAC egress checks.
6. Retain cached PAC transport state and cooldown behavior.
7. Preserve wrapped Desktop transports.
8. Restore the SSRF protection documentation anchor.
9. Document Desktop PAC egress controls and standalone proxy guidance.
10. Rename and document the Desktop proxy kill switch, including MCP OAuth coverage.

Validation

• task build passed.
• task test passed.
• task lint passed.
• All commits are signed; branch reviewed and ready to merge.

Manual PAC-only / Desktop-host smoke validation was not run and remains environment-limited coverage.

[melmennaoui]

Review: Desktop PAC-aware egress with SSRF protections

Right direction and a high-quality implementation overall — but I'd request changes. Three items should be fixed before merge (one security-relevant), and the guarded-proxy design deserves an explicit security sign-off since it changes the SSRF trust model. The issue itself flagged this approach as the "alternative worth security review," and that review should be visible on the PR.

What the PR does

Root-cause fix matches the issue precisely: urlSource.Read swaps NewSSRFSafeTransport for a new NewDesktopAwareSSRFSafeTransport, so URL agent sources reach Docker Desktop's PAC proxy instead of dialing direct in PAC-only networks. It also implements the issue's other two asks: bounded startup retries in source_loader.go ({2s, 15s, 70s}) and typed errors (ErrAgentNotFound → 404, ErrAgentSourceUnavailable → 502) replacing the silent 500 "agent not found". The same routing is extended to all guarded/unguarded HTTP consumers (fetch, api, openapi, a2a, MCP OAuth, generic clients), with a DOCKER_AGENT_DISABLE_DESKTOP_PROXY=1 kill switch and thorough docs.

Strengths worth calling out

• Lazy, per-request Desktop detection with no constructor I/O. Package-level clients like skillsHTTPClient stay safe to build at init, and clients created before Desktop starts pick up the proxy later — a real resilience improvement over the old construct-time probe.
• The fallback composition is preserved correctly. When the proxy branch fails with a socket error, requests fall back to the SSRF-guarded direct transport with the existing cooldown and body-replay safety. No protection is lost on the fallback path.
• It quietly fixes a latent misuse. a2a.go, api.go, and openapi.go were passing allowPrivateIPs as NewSafeClient's unsafe flag — a branch whose own doc comment says it "exists ONLY for tests" (raw shared DefaultTransport, no dedicated redirect bound). The new NewAllowPrivateIPsClient gives those paths a cloned pool and BoundedRedirects(10). After this PR, no production caller passes unsafe=true.
• Docs are honest and match the code, including the subtle "a DNS lookup failure may be delegated to the proxy" caveat. Pinning the retry schedule to outlive the 1-minute detection cache (TestSourceRetryScheduleOutlivesDesktopDetectionCache) shows real care.

Must-address

1. The guarded proxy path weakens the anti-rebinding guarantee — and fails open on resolver errors.
NewDesktopTransport clones the SSRF-safe transport and replaces its DialContext with the unix-socket dial, so the dial-time SSRFDialControl guard is inactive on the proxy branch. The compensating control is proxySafe(): resolve locally, require all-public. That's a check-time/use-time gap — the Desktop proxy re-resolves the hostname when it connects, so an attacker-controlled domain can answer public at check time and 169.254.169.254 at connect time. This is exactly the rebinding scenario the dial-time guard was written to defeat (its own comment says so). Some residual risk here may be an acceptable trade-off for PAC support, but it needs an explicit security decision, and "Retains SSRF protections" in the PR body overstates the proxy branch. One zero-cost tightening: proxySafe currently returns true on any resolver error, including timeouts and SERVFAIL. Restrict delegation to genuine NXDOMAIN (errors.As(err, &dnsErr) && dnsErr.IsNotFound) — which is also all the test (unresolvable host delegates to proxy) actually exercises.

2. Guarded standalone requests pay a wasted DNS lookup on every call.
In desktopAwareTransport.RoundTrip, t.proxySafe() (a full LookupIP) runs before DesktopRunning is consulted. On any deployment without Desktop — the standalone servers this PR explicitly supports — every guarded request now does a blocking DNS resolution whose result is discarded (the direct transport resolves again in its dialer). Reorder: kill switch/loopback → DesktopRunning (memoized, cheap) → only then proxySafe. Even with Desktop running, consider a short-TTL per-host cache for the verdict.

3. agentSourceHTTPError returns nil for unrecognized errors, and getAgentConfig returns it directly.
Today LoadAgentConfig only produces the two typed errors, so it's unreachable — but the day someone adds an untyped error path, the handler returns nil on failure and echo emits a 200 with an empty body, silently. Give the switch a default that maps to 500 (the createSession/runAgent call sites can keep the nil-sentinel pattern; the direct-return site shouldn't).

Should-fix / design notes

• ErrAgentSourceUnavailable is wrapped too broadly. loadTeam/loadTeamWithConfig/LoadAgentConfig wrap every teamloader.Load/config.Load failure — including YAML parse errors and config validation on a perfectly reachable (or local file) source. A malformed local agent file now returns 502 Bad Gateway, which is semantically wrong and will mislead triage. Type the error at the fetch boundary (source Read) instead, or unwrap-and-classify.
• DesktopRunning holds detectionMu across the probe. IsDockerDesktopRunning is a ping with a 3-second timeout; once per TTL expiry (1 min), every in-flight request across all desktop-aware transports serializes behind it — and context.WithoutCancel means a caller's short deadline won't shorten the wait. A wedged Desktop backend stalls all outbound HTTP by up to 3s each minute. Narrow the critical section to the override read and let the memoizer's singleflight dedup; consider serving the stale value while refreshing.
• Behavior change for allow_private_ips users on Desktop: private-host traffic (non-loopback) is now proxy-first. Fallback to direct only triggers on socket-level errors (isProxySocketError) — a corporate proxy answering 403/502 for an intranet host will fail the request rather than fall back, where it previously worked direct. PAC usually says DIRECT for RFC1918, so this is likely fine, but it's the most plausible field regression; it deserves a line in the docs/release notes.
• fetch.go builds NewAllowPrivateIPsClient(h.timeout).Transport per tool call — the timeout is silently dropped (it lives on the discarded client), and it replaces the previously shared DefaultTransport with a fresh transport per invocation, so connections are never reused. Export a transport constructor (newAllowPrivateIPsTransport already exists) instead of the client-for-transport dance.
• The 4×-duplicated selection pattern (NewSafeClient(t.timeout, false) then conditionally overwrite with NewAllowPrivateIPsClient) constructs and discards a client, copy-pasted across a2a/api/openapi. Fold it into one constructor (NewToolClient(timeout, allowPrivateIPs)).
• import "testing" in production code (pkg/desktop/transport/transport.go, for SetDesktopRunningForTest). Harmless since Go 1.13, but unconventional and linter bait — a small transporttest helper package or a plain setter returning a restore func is cleaner.
• Kill switch parses only "1", while the repo convention (DOCKER_AGENT_AUTO_UPDATE) accepts 1/true/yes/on. DOCKER_AGENT_DISABLE_DESKTOP_PROXY=true silently doing nothing is a support-ticket generator.

Tests

Coverage is genuinely good (hermetic Desktop detection override, cooldown preservation, detection-flip caching, typed-error HTTP mapping per route). Gaps:

• No test of the guarded happy path end-to-end: guarded=true + public resolution + Desktop running → desktop branch used; private resolution → direct. proxySafe is only tested in isolation.
• TestDesktopAwareTransportDisableCompressionBeforeAndAfterDirectFallback doesn't test what its name claims — it never round-trips; the final assertion is on the isLoopbackHost helper.
• The headline scenario — PAC-only egress with a real Desktop — was never exercised (the PR body admits it; the issue was "identified by source inspection… not reproduced"). Given 27 files of egress changes, a manual Desktop+PAC smoke test should gate the release, with the kill switch as the escape hatch.

Nits / follow-ups (fine to defer)

• Several commits are fix-ups of things introduced earlier in the same PR ("rename the kill switch", "restore docs anchor", "make tests hermetic") — squash-merge.
• isLoopbackHost misses *.localhost (RFC 6761) and hostnames resolving to loopback; for unguarded clients those now take a proxy detour before falling back.
• On Desktop flapping (true→false→true), transportForDesktopState rebuilds the desktop transport, dropping the old pool and cooldown state without CloseIdleConnections.
• The issue's cache-key aggravator (hashURL embeds desktopVersion/gordonTag, so every Desktop upgrade cold-starts the URL cache) is not addressed — reasonable to defer, but worth a follow-up issue.
• The 500→404/502 change on /api/agents/* and session routes is consumer-visible — give the Desktop/Gordon UI team a heads-up.

[Sayt-0]

Thanks for tracking this down — #3998 is a real, well-diagnosed bug, and the OCI-vs-URL asymmetry (pkg/remote/pull.go already uses the Desktop transport, urlSource.Read does not) is convincing evidence. The commit-per-phase split, the hermetic detection hook, keeping loopback direct, shipping a kill switch and documenting the behaviour across every affected tool page are all solid.

Requesting changes on one substantive point plus four cheap ones. Details are inline.

Blocking — the security property, not the feature. For guarded clients this PR replaces an enforced SSRF control with an advisory, fail-open one whenever Docker Desktop runs. NewDesktopTransport overwrites the guarded DialContext, so SSRFDialControl never executes on the proxy branch, and proxySafe then returns true on any resolver error. Since fetch takes model-chosen URLs, a prompt-injected http://intranet.corp/ that fails local resolution is handed to the corporate PAC proxy, which resolves it — with no allow_private_ips: true involved.

#3998 anticipated this trade-off and asked for it to be arbitrated: remediation (1) scoped the Desktop transport to docker.com source URLs and flagged the broader option as "worth security review: teach NewSSRFSafeTransport the Desktop proxy socket while keeping its dial-time allowlist". This PR takes the broad route — every guarded consumer (fetch, api, openapi, a2a, webhook, skills, toolinstall, MCP OAuth, tui/image, URL sources) — without preserving the enforcement point. That decision deserves to be explicit, ideally with a security reviewer.

Any of these unblocks: scope the PAC branch to trusted Docker hosts; or fail closed on local DNS errors with proxy-side resolution behind an explicit allowlist; or move IsPublicIP enforcement into whatever performs the final dial.

Blocking-adjacent: an explicitly configured HTTPS_PROXY/NO_PROXY is now silently ignored for guarded clients — reproduced locally, the configured proxy is never contacted. New versus baseline, undocumented and untested.

Cheap fixes before merge, all covered inline: the kill switch accepts only the exact string "1" while every other boolean env var in the repo is permissive; the proxySafe DNS lookup runs before the DesktopRunning() check, costing an extra lookup per request and per redirect hop even where Docker Desktop is absent; every teamloader.Load/config.Load error becomes a 502, so malformed local YAML now reports as a gateway failure; and agentSourceHTTPError returning nil in its default branch lets getAgentConfig answer 200 with an empty body.

On validation. The description notes that PAC-only/Desktop-host smoke validation was not run. Given that PAC behaviour is the entire point of the change and that an SSRF control is being relaxed, task build/test/lint alone looks insufficient. Minimum ask: one PAC-only manual run, a test pinning the NO_PROXY interaction, and a test for the true → false → true detection flap — the current cooldown test only performs a single transition.

Suggestion. The HTTP-status refactor and the startup retry are independent and uncontroversial, and between them they cover remediations (2) and (3) of #3998. Splitting them into their own PR would ship most of the user-visible fix now and let the transport work take the security review and PAC validation it needs; as it stands, 881 additions across 12 commits mix four concerns and none can be reverted independently.

One caveat on the evidence: findings 1 and 2 are code-proven and the proxy-precedence one was reproduced locally, but no Docker Desktop + PAC environment was available, so whether Desktop's own proxy refuses private destinations and compensates for the missing client-side guard remains unverified. If it does, the first finding downgrades considerably — which is precisely the fact worth establishing before merge.

[aheritier]

Thanks for the detailed review. The reviewed feedback has been addressed across the follow-up commits below:

• f6ad02fd — corrected routing and error semantics: Desktop-first for eligible routing; remote fetch failures return 502, missing agents 404, and invalid/local configuration 500; guarded resolver delegation occurs only for NXDOMAIN. Also covered the guarded/direct selection behavior and related route handling.
• 58bc9f9f — tightened proxy/environment behavior and lifecycle details: there is no automatic HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY precedence; DOCKER_AGENT_DISABLE_DESKTOP_PROXY is truthy per request, and opting out restores standard environment-proxy behavior. The stale detection/concurrency handling, transport pooling, compression behavior, and associated tests were corrected.
• b9102874 — completed the test and implementation cleanup: hermetic proxy-precedence coverage, guarded-path and round-trip coverage, race/concurrency fixes, build-once transport reuse, and removal of the production testing seam.
• 3deb9fd9 — corrected the documentation scope, including the PAC DIRECT case and the trust boundary.

The final security/behavior decision is explicit: Desktop-selected egress—including PAC DIRECT—is outside docker-agent’s local dial-time SSRF enforcement, and the documentation is scoped accordingly. The local direct/fallback path remains guarded; this is not being described as retaining dial-time SSRF enforcement on the Desktop-selected path.

Outstanding NON-ACTION (not claimed resolved): a live Desktop PAC-only smoke test and independent security acceptance of the guarded-proxy trust model. Neither has been performed or accepted by these changes.

These commits address the implementation, error mapping, proxy opt-out/precedence, resolver, concurrency, pooling, test, and documentation feedback described above. This is a feedback-resolution summary, not an approval or a claim that review-level change requests are approved. Please re-review the current head 3deb9fd9979264a503eb923f4fd6072241eb6db0.