Reject loopback HTTP MCP servers in network isolation mode#44113
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds compile-time validation intended to prevent workflows from combining AWF network isolation (sandbox.agent.sudo: false) with HTTP MCP servers configured via loopback URLs (e.g. localhost), and updates the repo’s shipped MemPalace MCP import and compiled Daily Fact workflow to use host.docker.internal.
Changes:
- Add sandbox validation that rejects loopback HTTP MCP server URLs when network isolation is enabled.
- Add/extend tests covering localhost/host.docker.internal MCP URL behavior and a Daily Fact workflow smoke check.
- Update the shared MemPalace MCP import and regenerate the
daily-fact.lock.ymloutput to reflect the new URL.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/sandbox_validation.go | Adds a new validation pass for loopback HTTP MCP server URLs under network isolation. |
| pkg/workflow/sandbox_validation_test.go | Adds a compile-path regression test expecting localhost MCP URLs to be rejected in network isolation mode. |
| pkg/workflow/sandbox_test.go | Extends unit tests for sandbox validation behavior with host.docker.internal vs localhost cases. |
| pkg/workflow/importable_tools_test.go | Updates test fixture MCP URL away from loopback. |
| pkg/workflow/daily_fact_workflow_test.go | Adds a smoke test to ensure shipped Daily Fact workflow/import avoids localhost MCP URLs. |
| .github/workflows/shared/mcp/mempalace.md | Updates MemPalace MCP URL to host.docker.internal. |
| .github/workflows/daily-fact.lock.yml | Regenerated compiled workflow output reflecting the MemPalace URL change and related domain list changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Low
| func validateNetworkIsolationLoopbackMCPServers(workflowData *WorkflowData) error { | ||
| if workflowData == nil || !isAWFNetworkIsolationEnabled(workflowData) || len(workflowData.ResolvedMCPServers) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fixed in b90ae95. The validateNetworkIsolationLoopbackMCPServers validator was removed entirely — the compiler already rewrites localhost URLs to host.docker.internal and adds --add-host host.docker.internal:host-gateway to the gateway container (see rewriteLocalhostToDockerHost), so the new validator was fighting existing behavior and conflicting with TestMCPGatewayDockerCommandAddsHostGatewayForLocalhostHTTPMCPInBridgeMode.
| mcp-servers: | ||
| mempalace: | ||
| type: http | ||
| url: http://localhost:8765/mcp | ||
| url: http://host.docker.internal:8765/mcp | ||
| allowed: |
There was a problem hiding this comment.
Fixed in b90ae95. Changed --host 127.0.0.1 to --host 0.0.0.0 so the MemPalace server listens on all interfaces and is reachable from the MCP gateway container via host.docker.internal → host-gateway in network isolation mode.
| "localhost HTTP MCP servers are incompatible with sandbox.agent.sudo: false because AWF auto-enables host access for loopback URLs, and host access cannot be combined with network isolation", | ||
| "Replace localhost with host.docker.internal (for example, `http://host.docker.internal:8765/mcp`) or disable network isolation:\n\nsandbox:\n agent:\n sudo: true", |
There was a problem hiding this comment.
Fixed in b90ae95. The entire validateNetworkIsolationLoopbackMCPServers validator and its misleading error text were removed — the compiler's existing rewriteLocalhostToDockerHost path handles localhost in network isolation correctly without needing a rejection validator.
|
@copilot please run the
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work on this one 🎉 — catching the
|
… 0.0.0.0 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed all three review threads and pushed the fixes:
|
|
@copilot please run the Unresolved review threads to address (newest first):
|
🤖 PR Triage
Rationale: Compiler now explicitly rejects Run §28909358158
|
|
@lpcox could you strengthen the specification around this kind of scenarios? |
|
@copilot please run the
|
All three review threads have been addressed in commit b90ae95 and replied to. Local validation passes ( |
|
🎉 This pull request is included in a new release. Release: |
Daily Factwas compiling into a configuration that can never start: an imported HTTP MCP server usedlocalhost, which triggers AWF host access, but the workflow also runs withnetwork-isolationenabled by default. This changes the compiler to fail that combination up front and updates the shipped MemPalace import to use a compatible host address.Compile-time guardrail
localhost,127.0.0.1,::1) whensandbox.agent.sudo: false/ AWF network isolation is enabled.host.docker.internalor opt out of network isolation.Shipped workflow fix
http://host.docker.internal:8765/mcp.daily-fact.lock.ymlso the checked-in workflow no longer carries the invalid runtime combination.Regression coverage
host.docker.internalcase.host.docker.internal.