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.

[Sayt-0]

Verdict: REQUEST CHANGES

Thanks for the follow-up work — a lot of the previous round landed cleanly. Confirmed fixed at b3620f83: NXDOMAIN-only resolver delegation, DesktopRunning checked before the DNS preflight (verified: 0 resolver calls for 10 requests when Desktop is absent), default -> 500 in agentSourceHTTPError, malformed local YAML back to 500, truthy kill-switch parsing, Desktop transport + cooldown surviving a true->false->true flap, and the testing import removed from production code. The OCI-vs-URL asymmetry (pkg/remote/pull.go:69 vs the old pkg/config/sources.go:343) is a convincing diagnosis, and the PR also quietly fixes a latent misuse — after this change no production caller passes unsafe=true to NewSafeClient.

Five things still block. Four of them are cheap. Details inline.

1. A data race, reproduced under -race — lazy Desktop-transport creation writes DisableCompression on the shared *http.Transport that concurrent requests are reading. The PR's own "concurrent" test cannot catch it.
2. *.localhost in validateAgentURL now accepts plaintext http://evil.localhost/agent.yaml (base rejected it) and serves it over http.DefaultTransport with no SSRF guard at all. Unrelated to PAC egress, and untested.
3. Six security contracts still promise the guarantee this PR scopes down — including agent-schema.json, which feeds IDE autocomplete. The PR body's "Retains SSRF protections ... for every supported consumer" is contradicted by your own resolution comment and should be corrected before merge.
4. sanitizeURLForLog keeps the URL path, and NewSafeClient(_, false) now reaches that log site for the first time — webhook secrets (Slack/Discord/Telegram) live in the path.
5. The design decision itself is still unvalidated. Your own note says as much. The single fact that decides whether this is a blocker or a footnote: does Docker Desktop's proxy refuse RFC1918 / link-local / metadata destinations, including on PAC DIRECT? I had no Desktop+PAC environment, so I could not establish it. If it does, the residual risk drops sharply. If it does not, fetch takes model-chosen URLs and this is prompt-injectable SSRF.

Plus, not anchorable inline because the files are untouched — five stale security comments, all reachable through a now-PAC-routed client and all inaccurate whenever Desktop is running:

• agent-schema.json:2354 — "after DNS resolution, so DNS rebinding is also blocked". No generator: AGENTS.md and .agents/skills/bump-config-version/SKILL.md require a manual edit coordinated with types.go; pkg/config/schema_test.go only cross-validates.
• pkg/config/latest/types.go:1603-1611 — same claim.
• pkg/toolinstall/registry.go:167-173 — "NewSafeClient enforces dial-time SSRF protection".
• pkg/skills/cache.go:26-33 — "refuses such targets at dial time, after DNS resolution, defeating DNS rebinding".
• pkg/httpclient/ssrf.go:246 — LocalhostOnlyRedirects still hard-codes exact localhost, now inconsistent with the two widened predicates (see the sources.go thread).

I have a handful of minor/nit items (an ALL_PROXY claim repeated in 8 doc paragraphs that the code does not implement, raw err.Error() incl. the source URL in the new 502 bodies, no verdict cache on the per-request DNS preflight, and a latent probe-amplification path in desktopDetectionCache) — happy to post those separately if useful, but they should not gate this.

Suggestion on scoping: the HTTP-status refactor and the startup retry are independent and uncontroversial, and between them cover remediations (2) and (3) of #3998. Splitting them out would ship most of the user-visible fix now and let the transport work take the PAC validation and security sign-off it needs. As it stands, 17 commits across 31 files mix four concerns and none can be reverted independently.

For the record on what I checked: CI is fully green (20 checks), golangci-lint run ./... -> 0 issues, the custom cop suite -> no offenses, go mod tidy -diff clean, wasm build OK, and go test -race -count=10 -shuffle=on ./pkg/desktop/transport/... ./pkg/httpclient/... clean. None of the findings below are lint-visible. The one pkg/tools/builtin/openapi failure I hit was environmental (ambient HTTP_PROXY in my sandbox) and reproduces identically at base — not yours.

[Sayt-0]

blocker (design decision, not a coding defect) — NXDOMAIN delegation + TOCTOU, and this is the item that needs an explicit sign-off

The narrowing from "any resolver error" to dnsErr.IsNotFound is a real improvement over the previous round. But NXDOMAIN is arguably the most load-bearing case: in split-horizon corporate DNS, internal names are precisely the ones that NXDOMAIN locally and resolve through the proxy. Since fetch takes model-chosen URLs, a prompt-injected http://jenkins.internal/ or http://intranet.corp/ is handed to the PAC proxy with no allow_private_ips: true anywhere. The reachable set goes from zero proxy-only internal names to all proxy-reachable internal names.

Separately, proxySafe resolves and Desktop then resolves again independently — a check-time/use-time gap. A domain that answers public here and 169.254.169.254 at connect time is exactly the rebinding that SSRFDialControl's own comment (ssrf.go:64-69) was written to defeat.

To be fair on severity, and I want this on the record: base already trusted an explicitly configured HTTP_PROXY/HTTPS_PROXY to enforce destination policy — that is the documented proxyDialAllowlist exception at ssrf.go:99-104. So this is not different in kind. It is different in default: (a) active merely because Desktop is running, with no operator action; (b) extended to every guarded consumer rather than the #3998 source-fetch case; (c) PAC DIRECT also escapes the guard; (d) NO_PROXY no longer provides an exit.

#3998 anticipated exactly this and asked for it to be arbitrated — remediation (1) scoped the Desktop transport to docker.com source URLs and flagged the broad option as "worth security review". Your resolution comment is refreshingly honest that neither the live PAC smoke test nor the security acceptance has happened. I could not close that gap either: no Desktop+PAC environment here.

The one fact that settles this: does Docker Desktop's host proxy refuse RFC1918 / link-local / metadata destinations, including when PAC returns DIRECT? If yes, residual risk drops sharply and this becomes a documentation matter. If no, it is prompt-injectable SSRF. Please get that answered by the Desktop networking owners and record it on the PR.

Any of these also unblocks without needing the answer: scope the PAC branch to trusted Docker hosts; fail closed on NXDOMAIN for guarded transports and require an explicit host allowlist or allow_private_ips for proxy-only names; or make Desktop routing opt-in for guarded consumers.

Test gap worth closing regardless: there is no end-to-end test of the guarded happy path. newDesktopAwareTransport(true) appears only in TestDesktopAwareTransportProxySafe (unit-tests proxySafe, never round-trips) and in TestDesktopAwareTransportDesktopWinsOverNoProxyUntilKillSwitch (fakes newDesktopTransport). NewDesktopAwareSSRFSafeTransport() — the actual production constructor — is never called from any test.