fix(mcp-server): bind HTTP server to 127.0.0.1; document validation escalation#43766
Conversation
…ment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43766 does not have the 'implementation' label and has only 13 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR hardens the gh aw mcp-server HTTP/SSE transport against cross-origin and DNS-rebinding style attacks introduced by changes in modelcontextprotocol/go-sdk (>= v1.6.0) by restricting the HTTP listener to localhost and clarifying an intentional validation escalation behavior.
Changes:
- Bind the MCP HTTP server to
127.0.0.1:<port>(instead of all interfaces) and fix the startup URL formatting. - Document why unknown MCP tool names in
argumentValidationMiddlewareintentionally escalate to a JSON-RPCCodeMethodNotFounderror.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_server_http.go | Restricts the HTTP listener to loopback and corrects the startup message formatting. |
| pkg/cli/mcp_argument_validation.go | Adds rationale for escalating unknown tool-name cases to a protocol-level error. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — 3 suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Display URL mismatch (
mcp_server_http.go:91): Server binds to127.0.0.1but prints(localhost/redacted) — on IPv6-preferred systemslocalhost→::1and clients will fail to connect. Should display(127.0.0.1/redacted) - No regression test for the bind address (
mcp_server_http.go:84): This is a security-critical invariant; a single test asserting the addr starts with127.0.0.1:protects against future regressions. - Double-negative in comment (
mcp_server_http.go:83):DisableLocalhostProtection=falseis confusing — rephrase to state the SDK default is active.
Positive Highlights
- ✅ Correct security fix: binding to
127.0.0.1is the right mitigation for the SDK v1.6.0 regression. - ✅ Defense-in-depth noted: the comment correctly calls out both layers (bind address + SDK localhost header check).
- ✅ Excellent explanatory comment in
mcp_argument_validation.go— the escalation rationale is clear and will help future reviewers. - ✅ Existing test suite (
TestArgumentValidationMiddleware_UnknownToolReturnsMethodNotFound) already covers the escalation path added inmcp_argument_validation.go.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 47.9 AIC · ⌖ 5.4 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
|
|
||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Starting MCP server on http://localhost"+addr)) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting MCP server on http://localhost:%d", port))) |
There was a problem hiding this comment.
[/diagnosing-bugs] The display URL uses (localhost/redacted) but the server only listens on 127.0.0.1(IPv4 loopback). On systems wherelocalhostresolves to::1` (IPv6), clients using the displayed URL will fail to connect.
💡 Suggested fix
Change the display message to match the actual bind address:
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting MCP server on (127.0.0.1/redacted) port)))This ensures users who copy-paste the printed URL will always reach the running server.
@copilot please address this.
There was a problem hiding this comment.
Addressed on 72f6955: the startup message now uses mcpHTTPServerDisplayURL() and prints http://127.0.0.1:<port> so it matches the actual bind address.
| // prevents the server from being reachable on non-localhost interfaces. | ||
| // The SDK's built-in localhost protection (DisableLocalhostProtection=false) | ||
| // additionally rejects requests with non-localhost Host headers. | ||
| addr := fmt.Sprintf("127.0.0.1:%d", port) |
There was a problem hiding this comment.
[/tdd] The security-critical address binding change has no regression test. Without a test, this fix can silently revert to 0.0.0.0 in a future refactor.
💡 Suggested test (unit or integration)
Add a test in mcp_server_http_test.go that asserts the Addr field of the constructed http.Server starts with 127.0.0.1::
func TestRunHTTPServer_BindsToLoopback(t *testing.T) {
// Extract addr construction logic into a helper or test it indirectly
// by verifying the format string produces a loopback address.
addr := fmt.Sprintf("127.0.0.1:%d", 12345)
if !strings.HasPrefix(addr, "127.0.0.1:") {
t.Errorf("addr %q does not bind to loopback", addr)
}
}A more meaningful test would start the server on a random port and assert it is not reachable on 0.0.0.0.
@copilot please address this.
There was a problem hiding this comment.
Addressed on 72f6955: pkg/cli/mcp_server_http_test.go now covers both loopback helpers with TestMCPHTTPServerAddr_BindsToLoopback and TestMCPHTTPServerDisplayURL_UsesLoopbackAddress.
| // of all interfaces) is the safest default for a local MCP server — it | ||
| // prevents the server from being reachable on non-localhost interfaces. | ||
| // The SDK's built-in localhost protection (DisableLocalhostProtection=false) | ||
| // additionally rejects requests with non-localhost Host headers. |
There was a problem hiding this comment.
[/codebase-design] The comment says DisableLocalhostProtection=false which is a double-negative that reads as "protection is enabled" — but this is never set explicitly in this file, so the reader must infer what the SDK default is and whether this code sets it.
💡 Suggested clarification
Rephrase to remove the double-negative and make the invariant explicit:
// The SDK's localhost-protection middleware (enabled by default —
// DisableLocalhostProtection is not set) also rejects requests whose
// Host header is not a loopback address, providing a second layer of defence.
@copilot please address this.
There was a problem hiding this comment.
Addressed on 72f6955: StreamableHTTPOptions now explicitly sets DisableLocalhostProtection: false, and the nearby comment states the Host-header checks remain enabled as the second layer.
There was a problem hiding this comment.
REQUEST_CHANGES — the loopback-binding intent is correct and important, but there are blocking issues in both files before this can merge.
Blocking issues (3)
mcp_argument_validation.go
- Wrong error code:
CodeMethodNotFoundis semantically incorrect for a registry miss on a tool that was already dispatched. UseCodeInternalErrorinstead. MCP clients that respect-32601may permanently drop the tool from their session. - Misleading comment: "silently hide the gap" is wrong — the original path produces a visible tool-level error. The comment should explain that escalation makes server misconfiguration impossible to ignore, not that the alternative is silent.
mcp_server_http.go
- Phantom SDK field: the comment cites
DisableLocalhostProtection=falseas an active Host-header defense, but this field does not appear ingo-sdk v1.6.1. Either verify and fix the field name, or remove the claim so operators don't rely on a layer that may not exist. localhostvs127.0.0.1mismatch in startup message: on IPv6-first systemslocalhostresolves to::1; the printed URL will refuse connections. Print `(127.0.0.1/redacted) to match the actual bind address.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 90.2 AIC · ⌖ 8.14 AIC · ⊞ 5.5K
Comment /review to run again
| // This should not happen in practice because the registry is | ||
| // exhaustive, but escalating to a protocol-level error here | ||
| // (CodeMethodNotFound) is intentional: returning a tool-level | ||
| // result for an unknown tool would silently hide the gap. |
There was a problem hiding this comment.
Wrong JSON-RPC error code: CodeMethodNotFound will mislead MCP clients into thinking the tool does not exist, potentially causing them to remove it from their available tool set.
💡 Details and suggested fix
At this point in the middleware, next() has already been invoked and returned a tool result — the tool was dispatched and ran. The tool exists; the gap is that its name is absent from the local mcpToolParams registry, which is a server-side misconfiguration, not a missing RPC method.
Using jsonrpc.CodeMethodNotFound (-32601) tells the client there
There was a problem hiding this comment.
Addressed on 72f6955: the registry-miss path now returns jsonrpc.CodeInternalError, and the matching test was updated to assert the internal-error code.
| // This should not happen in practice because the registry is | ||
| // exhaustive, but escalating to a protocol-level error here | ||
| // (CodeMethodNotFound) is intentional: returning a tool-level | ||
| // result for an unknown tool would silently hide the gap. |
There was a problem hiding this comment.
Wrong JSON-RPC error code: CodeMethodNotFound will mislead MCP clients into thinking the tool does not exist, which may cause them to stop offering it in the session.
💡 Details and suggested fix
At this point in the middleware, next() has already been invoked and returned a tool-level error result — the tool was found and dispatched; it only rejected the invalid arguments. The tool exists. The gap is that its name is absent from the mcpToolParams registry, a server-side misconfiguration.
jsonrpc.CodeMethodNotFound (-32601) means "there is no such method". Well-behaved MCP clients may silently drop the tool from their available set upon receiving this. The correct code for an unexpected server-side inconsistency is CodeInternalError (-32603).
There was a problem hiding this comment.
Addressed on 72f6955: this path now uses jsonrpc.CodeInternalError for the server-side registry inconsistency instead of CodeMethodNotFound.
| // The tool name is not in the hardcoded registry (mcpToolParams). | ||
| // This should not happen in practice because the registry is | ||
| // exhaustive, but escalating to a protocol-level error here | ||
| // (CodeMethodNotFound) is intentional: returning a tool-level |
There was a problem hiding this comment.
Comment reasoning is inaccurate: the phrase "returning a tool-level result ... would silently hide the gap" is incorrect and will mislead future readers.
💡 Details
The original path — returning the tool-level error unchanged — would not be silent. The original result is already a visible tool-level error about invalid arguments; it surfaces to the caller just fine. The real motivation for escalating to a protocol-level error is to make server misconfiguration loud and impossible to ignore during development. Suggested replacement:
// The tool name is not in the hardcoded registry (mcpToolParams).
// This should not happen because the registry is exhaustive.
// Escalating to a protocol-level error (rather than passing through
// the original tool result) ensures any registry gap surfaces
// loudly as a server misconfiguration rather than being swallowed
// by normal error-handling paths.There was a problem hiding this comment.
Addressed on 72f6955: the comment now explains that escalation makes a registry gap fail loudly as a server bug, rather than claiming the original tool-level error would be silent.
| // cross-origin protection by default, so binding to 127.0.0.1 (instead | ||
| // of all interfaces) is the safest default for a local MCP server — it | ||
| // prevents the server from being reachable on non-localhost interfaces. | ||
| // The SDK's built-in localhost protection (DisableLocalhostProtection=false) |
There was a problem hiding this comment.
DisableLocalhostProtection=false references a non-existent SDK field: the comment claims the SDK enforces a Host-header check, but this field does not appear in go-sdk v1.6.1 — the defense-in-depth layer it describes may not exist.
💡 Details
The comment says: "The SDK's built-in localhost protection (DisableLocalhostProtection=false) additionally rejects requests with non-localhost Host headers."
A search of github.com/modelcontextprotocol/go-sdk v1.6.1 finds no DisableLocalhostProtection field in StreamableHTTPOptions or any other type. If this protection was removed or renamed in v1.6.0 (as the broader PR description implies), citing it as an active defense is actively misleading — operators may rely on a non-existent layer instead of adding their own controls.
Either verify the field name against the current SDK source and correct the comment, or remove the second-layer claim entirely and note that 127.0.0.1 binding is the only protection in place:
// Bind to loopback only. Since v1.6.0, the SDK no longer enables
// cross-origin protection by default, so binding to 127.0.0.1 is
// the only active safeguard against external access.There was a problem hiding this comment.
Addressed on 72f6955: the code now sets DisableLocalhostProtection: false directly in StreamableHTTPOptions, so the Host-header protection claim is backed by the actual configuration.
| } | ||
|
|
||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Starting MCP server on http://localhost"+addr)) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting MCP server on http://localhost:%d", port))) |
There was a problem hiding this comment.
Startup message prints localhost but server only listens on 127.0.0.1: on IPv6-first systems where localhost resolves to ::1, copying this URL will silently fail to connect.
💡 Details
The server binds 127.0.0.1:<port> (IPv4 loopback only). The startup message still says (localhost/redacted) On many modern Linux and macOS systems, localhostresolves to::1 (IPv6) by default (/etc/hostsorder,ndots, or gai.conf` preferring IPv6). Any user or integration script that copies the printed URL will get a connection refused.
Options:
- Print `(127.0.0.1/redacted) to match the actual bind address.
- Also bind
[::1]:%dand keep thelocalhostmessage.
The simplest safe fix:
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting MCP server on (127.0.0.1/redacted) port)))There was a problem hiding this comment.
Addressed on 72f6955: the startup URL now prints 127.0.0.1 instead of localhost, so copy-pasting the message reaches the IPv4 loopback listener.
There was a problem hiding this comment.
Review: fix(mcp-server): bind HTTP server to 127.0.0.1
Security fix is correct. Binding to 127.0.0.1 instead of 0.0.0.0 is the right approach to prevent DNS-rebinding and cross-origin POST attacks on the local MCP server.
One comment worth clarifying (non-blocking):
The new comment on line 82–83 references DisableLocalhostProtection=false as a second-layer SDK defense, but this field is never explicitly set in the StreamableHTTPOptions{} struct constructed just above it. Given that the PR description notes v1.6.0 removed default cross-origin protection, it is worth verifying whether localhost Host-header protection is a separate feature with a different default, or whether it too was removed. Either explicitly set the field or rephrase the comment so it does not overstate the active defenses.
Everything else looks good:
127.0.0.1:%dbinding is correct and idiomatic for loopback-only servers.- The display message fix (`"Starting MCP server on (localhost/redacted) correctly avoids string concatenation that would have produced a malformed URL.
- The
CodeMethodNotFoundescalation comment inmcp_argument_validation.gois accurate and helpful.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 76 AIC · ⌖ 5.72 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/cli/mcp_server_http.go:83
The comment asserts DisableLocalhostProtection=false provides SDK-level Host-header protection as a second layer, but this field is never explicitly set in the StreamableHTTPOptions{} struct above. The PR description notes that v1.6.0 removed the default cross-origin protection — it is unclear whether the localhost-protection (Host-header check) is a separate feature that still defaults to enabled, or was also removed.
To make the defense-in-depth claim accurate and self-documenting, p…
pkg/cli/mcp_server_http.go:29
The responseWriter wrapper captures the status code in its own statusCode field, but it does not override WriteHeader. As a result, when the inner handler calls w.WriteHeader(code), the call goes to the embedded http.ResponseWriter (correctly) but wrapped.statusCode stays at the default 200 — so the logged status code will always show 200 even for non-200 responses.
This is a pre-existing issue in the same file, but the PR adds logging that depends on wrapped.statusCode being…
🤖 PR Triage
Score breakdown: Impact 42 + Urgency 26 + Quality 14 Rationale: Security fix binding MCP HTTP server to
|
🤖 PR Triage — Run §28834001067 (carry-over ×3)
Score Breakdown:
Rationale: Security fix for DNS-rebinding vector. Review together with #43716 as Labels: already applied from prior run.
|
…ule-review Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Handled on |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
@copilot please run the
|
Current HEAD |
🤖 PR Triage — §28848998303
Carry-over × 4 — DNS-rebinding security fix (MCP HTTP → loopback). Unresolved bot change requests. Needs human security reviewer to merge.
|
|
🎉 This pull request is included in a new release. Release: |
The
modelcontextprotocol/go-sdkv1.6.0 removed the default cross-origin protection, leaving the HTTP MCP server exposed to DNS-rebinding and cross-origin POST attacks when bound to all interfaces (0.0.0.0).Changes
mcp_server_http.go— bind to loopback onlyaddrfrom:%d→127.0.0.1:%d, restricting the server to localhost. The SDK's built-in localhost protection (DisableLocalhostProtection=false) also rejects requests with non-localhostHostheaders as a second layer.mcp_argument_validation.go— document intentional escalationCodeMethodNotFoundescalation path explaining why an unknown tool name inargumentValidationMiddlewarereturns a protocol-level JSON-RPC error rather than a tool-level result (returning a tool result would silently hide a gap in themcpToolParamsregistry).