From bba647aa03b7045e8296b3073abd67c5e9d37637 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Mon, 28 Sep 2026 20:28:06 -0500 Subject: [PATCH] fix(capability): reject host allow-list entries that could never match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both capability host allow-lists are compared against a URL's hostname, so an entry written as a URL or with a port matches nothing. Nothing reports that: the list parses, the assistant starts, and the setting silently does nothing. Only the dial list was checked, and its check missed the likeliest mistake. It rejected a slash but not a port, despite an error message promising otherwise — so CAPABILITY_MCP_ENDPOINT_HOSTS=gateway.example:8080 started cleanly and then refused every endpoint, taking every provider's tools out of every prompt with no configuration error to point at. CAPABILITY_IDENTITY_FORWARD_HOSTS had no check at all, and it is the one staging now depends on for three providers. A port there means provider tool calls quietly stop carrying the caller, so reads that should act as the person asking lose their identity instead. One validator now guards both, names the setting at fault, and detects a port with net.SplitHostPort so a bare IPv6 address still passes. Every shipped and deployed value is already a bare host, so nothing in service changes. Co-Authored-By: Claude Opus 5 (1M context) --- internal/capability/compose.go | 35 +++++++++++----- internal/capability/compose_test.go | 65 +++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/internal/capability/compose.go b/internal/capability/compose.go index 240f4ed..c64a600 100644 --- a/internal/capability/compose.go +++ b/internal/capability/compose.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" "net/url" "regexp" @@ -339,6 +340,9 @@ func Compose(ctx context.Context, docs []CapabilityDocument, opts ComposeOptions // Sanctioned identity-forwarding hosts. A malformed entry fails composition // closed, exactly as a malformed SSRF allow-list entry does. + if err := validateHostAllowList("CAPABILITY_IDENTITY_FORWARD_HOSTS", opts.IdentityForwardHosts); err != nil { + return nil, err + } sanctioned, err := parseHostAllowList(opts.IdentityForwardHosts, nil) if err != nil { return nil, err @@ -356,7 +360,7 @@ func Compose(ctx context.Context, docs []CapabilityDocument, opts ComposeOptions // Sanctioned MCP endpoint hosts (the gateway). Same fail-closed parse as the // two allow-lists above: a mis-typed entry must not silently leave the dial // site unguarded. - if err := validateEndpointHosts(opts.AllowedMCPEndpointHosts); err != nil { + if err := validateHostAllowList("CAPABILITY_MCP_ENDPOINT_HOSTS", opts.AllowedMCPEndpointHosts); err != nil { return nil, err } dialable, err := parseHostAllowList(opts.AllowedMCPEndpointHosts, nil) @@ -586,22 +590,31 @@ func ScopeDocuments(docs []CapabilityDocument, expectedProject string, logger *s return kept } -// validateEndpointHosts rejects an AllowedMCPEndpointHosts entry that is not a -// bare host. parseHostAllowList only hard-errors on a malformed CIDR, and this -// knob takes no CIDRs, so without this a mis-typed entry — a full URL -// ("https://gateway.example/mcp"), a host:port, a CIDR — would normalize into a -// string that can never match u.Hostname() and the allow-list would quietly -// sanction nothing. That failure mode is the worst one available here: every -// endpoint gets skipped and provider tools disappear cluster-wide with no -// configuration error to point at. Fail closed at startup instead. -func validateEndpointHosts(entries []string) error { +// validateHostAllowList rejects an entry that is not a bare host, for either of +// the two host allow-lists. +// +// Both are matched against a URL's hostname, so an entry written as a URL or as +// host:port can never match anything. Nothing else notices: the list parses, the +// assistant starts, and the setting silently does nothing — which for the dial +// allow-list means every provider tool disappears, and for the identity list +// means provider calls quietly stop carrying the caller. Refusing to start is +// the kinder outcome. +// +// knob names the setting in the error so an operator knows which one to fix. +func validateHostAllowList(knob string, entries []string) error { for _, e := range entries { trimmed := strings.TrimSpace(e) if trimmed == "" { continue // blank entries are dropped, as elsewhere } if strings.ContainsAny(trimmed, "/ \t") { - return fmt.Errorf("invalid MCP endpoint host %q: expected a bare host, not a URL, host:port, or CIDR", e) + return fmt.Errorf("invalid %s entry %q: expected a bare host, not a URL or CIDR", knob, e) + } + // A port is the easy mistake to make, since the endpoints these match + // nearly always carry one. SplitHostPort succeeds only on host:port, so + // a bare IPv6 address still passes. + if _, _, err := net.SplitHostPort(trimmed); err == nil { + return fmt.Errorf("invalid %s entry %q: expected a bare host, drop the port", knob, e) } } return nil diff --git a/internal/capability/compose_test.go b/internal/capability/compose_test.go index 8200846..4553672 100644 --- a/internal/capability/compose_test.go +++ b/internal/capability/compose_test.go @@ -1070,3 +1070,68 @@ func TestComposeObserver_PanickingCallbackDoesNotBreakTheTurn(t *testing.T) { t.Fatalf("expected the panic to be logged; logs:\n%s", buf.String()) } } + +// A host allow-list entry written as a URL or with a port can never match the +// hostname it is compared against, so composition refuses to start rather than +// leaving the setting silently doing nothing. +func TestComposeHostAllowLists_RejectEntriesThatCouldNeverMatch(t *testing.T) { + doc := mcpEndpointDoc("http://gateway.example/mcp") + + for _, tc := range []struct { + name string + opts ComposeOptions + want string + }{ + { + name: "dial list with a port", + opts: ComposeOptions{AllowedMCPEndpointHosts: []string{"gateway.example:8080"}}, + want: "CAPABILITY_MCP_ENDPOINT_HOSTS", + }, + { + name: "dial list as a URL", + opts: ComposeOptions{AllowedMCPEndpointHosts: []string{"https://gateway.example/mcp"}}, + want: "CAPABILITY_MCP_ENDPOINT_HOSTS", + }, + { + name: "identity list with a port", + opts: ComposeOptions{IdentityForwardHosts: []string{"compute-mcp.compute-system.svc.cluster.local:8080"}}, + want: "CAPABILITY_IDENTITY_FORWARD_HOSTS", + }, + { + name: "identity list as a URL", + opts: ComposeOptions{IdentityForwardHosts: []string{"http://compute-mcp.compute-system.svc.cluster.local/mcp"}}, + want: "CAPABILITY_IDENTITY_FORWARD_HOSTS", + }, + } { + t.Run(tc.name, func(t *testing.T) { + composed, err := Compose(context.Background(), []CapabilityDocument{doc}, tc.opts) + if err == nil { + composed.Close() + t.Fatalf("compose succeeded; want a rejection naming %s", tc.want) + } + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %q, want it to name %s", err, tc.want) + } + }) + } +} + +// A bare IPv6 address is a legitimate host and must survive the port check. +func TestComposeHostAllowLists_BareIPv6IsAccepted(t *testing.T) { + var dialed []string + session := newFakeSession("streams_list") + doc := mcpEndpointDoc("http://[2001:db8::1]/mcp") + composed, err := Compose(context.Background(), []CapabilityDocument{doc}, ComposeOptions{ + connect: dialRecorder(map[string]*fakeSession{"http://[2001:db8::1]/mcp": session}, &dialed), + AllowedMCPEndpointHosts: []string{"2001:db8::1"}, + IdentityForwardHosts: []string{"2001:db8::1"}, + }) + if err != nil { + t.Fatalf("compose: %v", err) + } + defer composed.Close() + + if len(dialed) != 1 { + t.Fatalf("dialed = %v, want the sanctioned endpoint", dialed) + } +}