server: default --allowed-hosts to loopback for local (non-remote) deployments#3342
server: default --allowed-hosts to loopback for local (non-remote) deployments#3342adilburaksen wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a secure-by-default behavior for local development by defaulting the --allowed-hosts flag to a loopback-only allowlist (127.0.0.1, localhost, ::1) when the server binds to a loopback address and the flag is not explicitly set. This change effectively mitigates DNS rebinding attacks. The review feedback points out a potential issue in the isLoopbackAddress helper, where the fallback prefix check strings.HasPrefix(host, "127.") could incorrectly match domain names starting with 127. (e.g., 127.example.com), and provides a code suggestion to refine this check.
| // Fall back to a 127.* prefix check for non-canonical IPv4 inputs. | ||
| return strings.HasPrefix(host, "127.") |
There was a problem hiding this comment.
The fallback prefix check strings.HasPrefix(host, "127.") can incorrectly match domain names that start with 127. (for example, 127.example.com or 127-server.local if resolved/configured as the address). If a remote deployment binds to such a domain, it would be incorrectly classified as a loopback address, causing the server to restrict --allowed-hosts to loopback only and breaking remote access.
To prevent this, we should ensure that the remaining characters after 127. only contain digits and dots (which are characteristic of non-canonical IPv4 representations like 127.1 or 127.0.1).
// Fall back to a 127.* prefix check for non-canonical IPv4 inputs.
if strings.HasPrefix(host, "127.") {
for i := 4; i < len(host); i++ {
if (host[i] < '0' || host[i] > '9') && host[i] != '.' {
return false
}
}
return len(host) > 4
}
return false
Description
--allowed-hostsand--allowed-originsdefault to["*"]. The server defaults to the HTTP transport bound to127.0.0.1, so out of the box for local development the Host check is a no-op (wildcard). As the existing startup warning already notes, a wildcard--allowed-hostsleaves the server "vulnerable to DNS rebinding attacks" — a web page open in the developer's browser can rebind an attacker domain to127.0.0.1(requests then become same-origin, so CORS does not apply) and reach the MCP/tool endpoints, which execute against the developer's configured data sources.This makes that the default rather than something the user must remember to harden.
Change
--allowed-hostsnow defaults to a loopback allowlist (127.0.0.1,localhost,::1) only when the server is bound to a loopback address and the user did not pass--allowed-hostsexplicitly.0.0.0.0,::, a specific host — i.e. remote/Cloud Run/containers) → existing["*"]behavior is preserved, so remote deployments are unaffected.--allowed-hosts(including*) → always respected unchanged.Detection of "explicitly set" uses Cobra's
flags.Changed("allowed-hosts"). The existing wildcard DNS-rebinding warning is preserved and now only fires when the wildcard actually survives.--allowed-originsis intentionally left unchanged (scope kept to the rebinding-relevant Host defense).Testing
go build ./...andgo test ./cmd/... ./internal/server/...pass;gofmtclean. AddedTestAllowedHostsContextAwareDefaultcovering: loopback bind + no flag →evil.com403 /localhost200; non-loopback bind → wildcard preserved (evil.com200); explicit flag respected.