From fc820f0792486a85c6c2da644479676517f6e47e Mon Sep 17 00:00:00 2001 From: Devam Shah Date: Mon, 22 Jun 2026 19:35:16 +0530 Subject: [PATCH 1/2] Add CORS support to the transparent MCP proxy The transparent MCP proxy has no OPTIONS handler, so browser-based clients such as MCP Inspector get a 405 on the CORS preflight and the browser blocks the real request. This adds an opt-in CORS middleware wired as the outermost handler on the proxy, so preflights are answered before the method gate or backend can reject them. A new --allow-origins flag on 'thv proxy' enables it; with no flag the behaviour is unchanged. Origins support exact, scheme+host (any port), and wildcard matching. No Access-Control-Allow-Credentials is emitted, so a wildcard is never combined with credentials. Fixes #4297 Signed-off-by: Devam Shah --- cmd/thv/app/proxy.go | 23 +- pkg/transport/middleware/cors.go | 101 +++++++ pkg/transport/middleware/cors_test.go | 260 ++++++++++++++++++ .../proxy/transparent/transparent_proxy.go | 31 +++ 4 files changed, 413 insertions(+), 2 deletions(-) create mode 100644 pkg/transport/middleware/cors.go create mode 100644 pkg/transport/middleware/cors_test.go diff --git a/cmd/thv/app/proxy.go b/cmd/thv/app/proxy.go index f3a933a00b..1d431e0e8e 100644 --- a/cmd/thv/app/proxy.go +++ b/cmd/thv/app/proxy.go @@ -123,6 +123,9 @@ var ( // Header forwarding flags remoteForwardHeaders []string remoteForwardHeadersSecret []string + + // CORS flags + proxyCORSOrigins []string ) // Environment variable names @@ -157,6 +160,15 @@ func init() { proxyCmd.Flags().StringArrayVar(&remoteForwardHeadersSecret, "remote-forward-headers-secret", []string{}, "Headers with secret values from ToolHive secrets manager (format: Name=secret-name, can be repeated)") + // CORS — disabled by default; opt in explicitly to avoid widening the attack surface + proxyCmd.Flags().StringArrayVar(&proxyCORSOrigins, "allow-origins", []string{}, + `Allowed CORS origins for the MCP proxy endpoint (repeatable). +Supported forms: + exact: http://localhost:6274 + scheme+host: http://localhost (matches any port on localhost) + wildcard: * (allow all — use with caution) +Example: --allow-origins http://localhost:6274`) + // Mark target-uri as required if err := proxyCmd.MarkFlagRequired("target-uri"); err != nil { slog.Warn(fmt.Sprintf("Failed to mark flag as required: %v", err)) @@ -264,8 +276,14 @@ func proxyCmdFunc(cmd *cobra.Command, args []string) error { slog.Debug(fmt.Sprintf("Setting up transparent proxy to forward from host port %d to %s", port, proxyTargetURI)) + // Build optional functional options (e.g. CORS), only when configured. + var proxyOptions []transparent.Option + if len(proxyCORSOrigins) > 0 { + proxyOptions = append(proxyOptions, transparent.WithAllowedOrigins(proxyCORSOrigins)) + } + // Create the transparent proxy with middlewares - proxy := transparent.NewTransparentProxy( + proxy := transparent.NewTransparentProxyWithOptions( proxyHost, port, proxyTargetURI, @@ -279,7 +297,8 @@ func proxyCmdFunc(cmd *cobra.Command, args []string) error { nil, // onUnauthorizedResponse - not needed for local proxies "", // endpointPrefix - not configured for proxy command false, // trustProxyHeaders - not configured for proxy command - middlewares...) + middlewares, + proxyOptions...) if err := proxy.Start(ctx); err != nil { return fmt.Errorf("failed to start proxy: %w", err) } diff --git a/pkg/transport/middleware/cors.go b/pkg/transport/middleware/cors.go new file mode 100644 index 0000000000..de44df3cba --- /dev/null +++ b/pkg/transport/middleware/cors.go @@ -0,0 +1,101 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package middleware + +import ( + "log/slog" + "net/http" + "strings" + + "github.com/stacklok/toolhive/pkg/transport/types" +) + +const ( + // corsAllowedMethods lists the HTTP methods the MCP proxy accepts. + corsAllowedMethods = "GET, POST, DELETE, OPTIONS" + + // corsAllowedHeaders lists request headers MCP clients may send. + corsAllowedHeaders = "Content-Type, Accept, Mcp-Session-Id, Authorization" + + // corsExposedHeaders lists response headers that browsers may read. + corsExposedHeaders = "Mcp-Session-Id, Content-Type" + + // corsMaxAge is the preflight cache lifetime in seconds (24 hours). + corsMaxAge = "86400" +) + +// CORS returns a middleware that handles CORS preflight (OPTIONS) requests and +// injects Access-Control-Allow-* response headers. When allowedOrigins is empty +// the middleware is a no-op, preserving the default security posture. +// +// Origin matching rules (applied in order): +// - "*": matches every origin; Access-Control-Allow-Origin is set to "*". +// - Exact: "http://localhost:6274" matches only that origin. +// - Scheme+host prefix: "http://localhost" also matches any +// "http://localhost:" (e.g. the MCP Inspector default port). +// +// All OPTIONS requests are handled directly (returning 204) when this middleware +// is active so that CORS preflights never reach the backend, which previously +// returned 405 Method Not Allowed. +func CORS(allowedOrigins []string) types.MiddlewareFunction { + if len(allowedOrigins) == 0 { + return func(next http.Handler) http.Handler { return next } + } + + slog.Debug("CORS middleware configured", "allowed_origins", strings.Join(allowedOrigins, ", ")) + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := r.Header.Get("Origin") + matched := matchCORSOrigin(origin, allowedOrigins) + + if matched != "" { + h := w.Header() + h.Set("Access-Control-Allow-Origin", matched) + h.Set("Access-Control-Allow-Methods", corsAllowedMethods) + h.Set("Access-Control-Allow-Headers", corsAllowedHeaders) + h.Set("Access-Control-Expose-Headers", corsExposedHeaders) + h.Add("Vary", "Origin") + } + + // Intercept OPTIONS so preflight requests never reach the backend + // (which returns 405 because it has no OPTIONS handler). + // A matched origin gets the full preflight response; an unmatched + // origin gets 204 without CORS headers — the browser will reject + // the follow-up request, which is the correct security outcome. + if r.Method == http.MethodOptions { + if matched != "" { + w.Header().Set("Access-Control-Max-Age", corsMaxAge) + } + w.WriteHeader(http.StatusNoContent) + return + } + + next.ServeHTTP(w, r) + }) + } +} + +// matchCORSOrigin returns the Access-Control-Allow-Origin value to send when +// requestOrigin matches an entry in allowed, or "" when there is no match. +// +// The returned value is the verbatim requestOrigin (a concrete origin) except +// when an allowed entry is "*", in which case "*" is returned directly. +func matchCORSOrigin(requestOrigin string, allowed []string) string { + if requestOrigin == "" { + return "" + } + for _, entry := range allowed { + switch { + case entry == "*": + return "*" + case entry == requestOrigin: + return requestOrigin + case strings.HasPrefix(requestOrigin, entry+":"): + // "http://localhost" matches "http://localhost:6274", etc. + return requestOrigin + } + } + return "" +} diff --git a/pkg/transport/middleware/cors_test.go b/pkg/transport/middleware/cors_test.go new file mode 100644 index 0000000000..d0dccde95e --- /dev/null +++ b/pkg/transport/middleware/cors_test.go @@ -0,0 +1,260 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// okHandler is a trivial inner handler that records whether it was called. +func newCallTracker() (http.Handler, *bool) { + called := false + h := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + }) + return h, &called +} + +func TestMatchCORSOrigin(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requestOrigin string + allowed []string + expectedResult string + }{ + { + name: "empty request origin returns empty", + requestOrigin: "", + allowed: []string{"http://localhost:6274"}, + expectedResult: "", + }, + { + name: "exact match returns origin", + requestOrigin: "http://localhost:6274", + allowed: []string{"http://localhost:6274"}, + expectedResult: "http://localhost:6274", + }, + { + name: "exact match among multiple allowed origins", + requestOrigin: "http://localhost:3000", + allowed: []string{"http://localhost:6274", "http://localhost:3000"}, + expectedResult: "http://localhost:3000", + }, + { + name: "prefix match: scheme+host matches scheme+host+port", + requestOrigin: "http://localhost:6274", + allowed: []string{"http://localhost"}, + expectedResult: "http://localhost:6274", + }, + { + name: "prefix match: https scheme", + requestOrigin: "https://localhost:9000", + allowed: []string{"https://localhost"}, + expectedResult: "https://localhost:9000", + }, + { + name: "no false prefix match: partial scheme should not match", + requestOrigin: "http://localhost:6274", + allowed: []string{"http://local"}, + expectedResult: "", + }, + { + name: "wildcard returns literal asterisk", + requestOrigin: "http://example.com", + allowed: []string{"*"}, + expectedResult: "*", + }, + { + name: "non-matching origin returns empty", + requestOrigin: "http://evil.example.com", + allowed: []string{"http://localhost:6274"}, + expectedResult: "", + }, + { + name: "https origin does not match http entry", + requestOrigin: "https://localhost:6274", + allowed: []string{"http://localhost:6274"}, + expectedResult: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + result := matchCORSOrigin(tc.requestOrigin, tc.allowed) + assert.Equal(t, tc.expectedResult, result) + }) + } +} + +func TestCORS_EmptyOrigins_IsNoop(t *testing.T) { + t.Parallel() + + inner, called := newCallTracker() + mwFn := CORS(nil)(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/mcp", nil) + req.Header.Set("Origin", "http://localhost:6274") + + mwFn.ServeHTTP(rec, req) + + assert.True(t, *called, "inner handler must be called when CORS is disabled") + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), "no CORS header should be set") +} + +func TestCORS_NonOptions_MatchingOrigin_AddsCORSHeaders(t *testing.T) { + t.Parallel() + + inner, _ := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"})(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Header.Set("Origin", "http://localhost:6274") + + mwFn.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "http://localhost:6274", rec.Header().Get("Access-Control-Allow-Origin")) + assert.Equal(t, corsAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) + assert.Equal(t, corsAllowedHeaders, rec.Header().Get("Access-Control-Allow-Headers")) + assert.Equal(t, corsExposedHeaders, rec.Header().Get("Access-Control-Expose-Headers")) + assert.Contains(t, rec.Header().Get("Vary"), "Origin") +} + +func TestCORS_NonOptions_NonMatchingOrigin_NoCORSHeaders(t *testing.T) { + t.Parallel() + + inner, called := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"})(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Header.Set("Origin", "http://evil.example.com") + + mwFn.ServeHTTP(rec, req) + + assert.True(t, *called, "inner handler must still be called for non-matching non-OPTIONS") + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), "no CORS header for non-matching origin") +} + +func TestCORS_Preflight_MatchingOrigin_Returns204WithHeaders(t *testing.T) { + t.Parallel() + + inner, called := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"})(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) + req.Header.Set("Origin", "http://localhost:6274") + req.Header.Set("Access-Control-Request-Method", "POST") + req.Header.Set("Access-Control-Request-Headers", "Content-Type") + + mwFn.ServeHTTP(rec, req) + + require.Equal(t, http.StatusNoContent, rec.Code, "preflight must return 204 No Content") + assert.False(t, *called, "inner handler must NOT be called for preflight") + assert.Equal(t, "http://localhost:6274", rec.Header().Get("Access-Control-Allow-Origin")) + assert.Equal(t, corsAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) + assert.Equal(t, corsAllowedHeaders, rec.Header().Get("Access-Control-Allow-Headers")) + assert.Equal(t, corsMaxAge, rec.Header().Get("Access-Control-Max-Age")) +} + +func TestCORS_Preflight_NonMatchingOrigin_Returns204NoHeaders(t *testing.T) { + t.Parallel() + + inner, called := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"})(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) + req.Header.Set("Origin", "http://evil.example.com") + + mwFn.ServeHTTP(rec, req) + + require.Equal(t, http.StatusNoContent, rec.Code, "OPTIONS always returns 204 when CORS is active") + assert.False(t, *called, "inner handler must NOT be called for OPTIONS") + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), "no CORS header for non-matching origin") + assert.Empty(t, rec.Header().Get("Access-Control-Max-Age")) +} + +func TestCORS_PrefixMatch_LocalhostAnyPort(t *testing.T) { + t.Parallel() + + inner, _ := newCallTracker() + // "http://localhost" should match http://localhost on any port + mwFn := CORS([]string{"http://localhost"})(inner) + + origins := []string{ + "http://localhost:6274", + "http://localhost:3000", + "http://localhost:8080", + } + + for _, origin := range origins { + t.Run(origin, func(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Header.Set("Origin", origin) + + mwFn.ServeHTTP(rec, req) + + assert.Equal(t, origin, rec.Header().Get("Access-Control-Allow-Origin"), + "prefix match must set concrete origin, not the entry") + }) + } +} + +func TestCORS_Wildcard_MatchesAnyOrigin(t *testing.T) { + t.Parallel() + + inner, _ := newCallTracker() + mwFn := CORS([]string{"*"})(inner) + + origins := []string{ + "http://localhost:6274", + "http://example.com", + "https://app.example.org", + } + + for _, origin := range origins { + t.Run(origin, func(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Header.Set("Origin", origin) + + mwFn.ServeHTTP(rec, req) + + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin"), + "wildcard entry must return literal * not the request origin") + }) + } +} + +func TestCORS_NoOriginHeader_NoCORSHeaders(t *testing.T) { + t.Parallel() + + inner, called := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"})(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + // No Origin header (non-browser request) + + mwFn.ServeHTTP(rec, req) + + assert.True(t, *called, "non-browser requests must reach the backend") + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin")) +} diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index 1cad66e6a2..6391822f2a 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -32,6 +32,7 @@ import ( "github.com/stacklok/toolhive/pkg/auth" "github.com/stacklok/toolhive/pkg/bodylimit" "github.com/stacklok/toolhive/pkg/healthcheck" + "github.com/stacklok/toolhive/pkg/transport/middleware" "github.com/stacklok/toolhive/pkg/transport/proxy/socket" "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/transport/types" @@ -149,6 +150,11 @@ type TransparentProxy struct { // Shutdown timeout for graceful HTTP server shutdown (default: 30 seconds) shutdownTimeout time.Duration + + // corsOrigins holds the list of allowed CORS origins. When non-empty, the + // proxy handles OPTIONS preflight requests and injects Access-Control-Allow-* + // headers. Empty (default) keeps the current behaviour with no CORS support. + corsOrigins []string } const ( @@ -336,6 +342,22 @@ func WithReadTimeout(d time.Duration) Option { } } +// WithAllowedOrigins enables CORS support on the transparent proxy by setting the +// list of permitted origins. An empty slice (the default) leaves CORS disabled so +// the security posture is unchanged for deployments that do not need browser access. +// +// Supported forms per entry: +// - Exact origin: "http://localhost:6274" +// - Scheme+host (any port): "http://localhost" → also matches http://localhost:6274 +// - Wildcard: "*" → allow every origin +func WithAllowedOrigins(origins []string) Option { + return func(p *TransparentProxy) { + if len(origins) > 0 { + p.corsOrigins = origins + } + } +} + // NewTransparentProxy creates a new transparent proxy with optional middlewares. // The endpointPrefix parameter specifies an explicit prefix to prepend to SSE endpoint URLs. // The trustProxyHeaders parameter indicates whether to trust X-Forwarded-* headers from reverse proxies. @@ -1216,6 +1238,15 @@ func (p *TransparentProxy) Start(ctx context.Context) error { if p.stateless { finalHandler = statelessMethodGate(finalHandler) } + + // 6. Apply CORS as the outermost wrapper so OPTIONS preflights are intercepted + // before the method gate or backend can reject them. Only active when + // corsOrigins is non-empty (opt-in, no behaviour change by default). + if len(p.corsOrigins) > 0 { + finalHandler = middleware.CORS(p.corsOrigins)(finalHandler) + slog.Debug("CORS middleware applied", "allowed_origins", p.corsOrigins) + } + mux.Handle("/", finalHandler) // Use ListenConfig with SO_REUSEADDR to allow port reuse after unclean shutdown From f2df558ba4214fb72f34061ad0be24efad99887f Mon Sep 17 00:00:00 2001 From: Devam Shah Date: Wed, 24 Jun 2026 13:31:55 +0530 Subject: [PATCH 2/2] Address CORS proxy review: protocol-version header, method alignment, origin validation Resolves the maintainer panel feedback on the transparent MCP proxy CORS support: - Allow-Headers: add MCP-Protocol-Version so browser MCP clients can send the header ToolHive reads/validates on the request path (ship-blocker). - Expose-Headers: add MCP-Protocol-Version so clients can read the negotiated version; drop redundant Content-Type (already CORS-safelisted). - Stateless method mismatch: derive the advertised preflight methods from the same source of truth the request path enforces. statelessMethodGate and the CORS preflight now share statelessAllowedMethods / statefulAllowedMethods, so a stateless server advertises only "POST, OPTIONS". - Origin validation at startup: ValidateAndNormalizeOrigins rejects schemeless origins (can never match) and strips trailing slashes with a warning, instead of silently failing in the browser. - Help text: document that CORS is disabled by default and scheme/no-trailing- slash requirements on the --allow-origins flag. - NewTransparentProxy: document it as a committed stable positional-arg wrapper around NewTransparentProxyWithOptions (kept; widely used by tests). - Tests: colon-boundary invariant (evil-subdomain must not match), caller-method reflection, MCP-Protocol-Version header presence, origin validation cases, and preflight/method-gate consistency. - Regenerate docs/cli for the new --allow-origins flag. Signed-off-by: Devam Shah --- cmd/thv/app/proxy.go | 12 +- docs/cli/thv_proxy.md | 10 ++ pkg/transport/middleware/cors.go | 83 +++++++++-- pkg/transport/middleware/cors_test.go | 136 ++++++++++++++++-- .../proxy/transparent/method_gate_test.go | 22 ++- .../proxy/transparent/transparent_proxy.go | 37 ++++- 6 files changed, 275 insertions(+), 25 deletions(-) diff --git a/cmd/thv/app/proxy.go b/cmd/thv/app/proxy.go index 1d431e0e8e..8f9600178d 100644 --- a/cmd/thv/app/proxy.go +++ b/cmd/thv/app/proxy.go @@ -163,6 +163,10 @@ func init() { // CORS — disabled by default; opt in explicitly to avoid widening the attack surface proxyCmd.Flags().StringArrayVar(&proxyCORSOrigins, "allow-origins", []string{}, `Allowed CORS origins for the MCP proxy endpoint (repeatable). +CORS is disabled by default; if you handle CORS at a gateway or reverse proxy, +leaving this unset is the correct, secure choice. Each origin must include a +scheme (e.g. http://) and no trailing slash, otherwise it can never match a +browser request. Supported forms: exact: http://localhost:6274 scheme+host: http://localhost (matches any port on localhost) @@ -279,7 +283,13 @@ func proxyCmdFunc(cmd *cobra.Command, args []string) error { // Build optional functional options (e.g. CORS), only when configured. var proxyOptions []transparent.Option if len(proxyCORSOrigins) > 0 { - proxyOptions = append(proxyOptions, transparent.WithAllowedOrigins(proxyCORSOrigins)) + // Validate origins at startup so a misconfigured entry (missing scheme, + // trailing slash) fails loudly instead of silently never matching. + corsOrigins, err := middleware.ValidateAndNormalizeOrigins(proxyCORSOrigins) + if err != nil { + return fmt.Errorf("invalid --allow-origins: %w", err) + } + proxyOptions = append(proxyOptions, transparent.WithAllowedOrigins(corsOrigins)) } // Create the transparent proxy with middlewares diff --git a/docs/cli/thv_proxy.md b/docs/cli/thv_proxy.md index be2e8d92d2..9bde19cae7 100644 --- a/docs/cli/thv_proxy.md +++ b/docs/cli/thv_proxy.md @@ -97,6 +97,16 @@ thv proxy [flags] SERVER_NAME ### Options ``` + --allow-origins stringArray Allowed CORS origins for the MCP proxy endpoint (repeatable). + CORS is disabled by default; if you handle CORS at a gateway or reverse proxy, + leaving this unset is the correct, secure choice. Each origin must include a + scheme (e.g. http://) and no trailing slash, otherwise it can never match a + browser request. + Supported forms: + exact: http://localhost:6274 + scheme+host: http://localhost (matches any port on localhost) + wildcard: * (allow all — use with caution) + Example: --allow-origins http://localhost:6274 -h, --help help for proxy --host string Host for the HTTP proxy to listen on (IP or hostname) (default "127.0.0.1") --oidc-audience string Expected audience for the token diff --git a/pkg/transport/middleware/cors.go b/pkg/transport/middleware/cors.go index de44df3cba..2551d86d93 100644 --- a/pkg/transport/middleware/cors.go +++ b/pkg/transport/middleware/cors.go @@ -4,6 +4,7 @@ package middleware import ( + "fmt" "log/slog" "net/http" "strings" @@ -12,14 +13,24 @@ import ( ) const ( - // corsAllowedMethods lists the HTTP methods the MCP proxy accepts. - corsAllowedMethods = "GET, POST, DELETE, OPTIONS" + // defaultCORSAllowedMethods is the fallback preflight method list used when + // the caller does not supply an explicit set. The proxy passes a set derived + // from the server's actual capabilities (see the stateless/stateful method + // sources of truth in the transparent proxy) so the preflight never + // advertises a method the backend will reject. + defaultCORSAllowedMethods = "GET, POST, DELETE, OPTIONS" - // corsAllowedHeaders lists request headers MCP clients may send. - corsAllowedHeaders = "Content-Type, Accept, Mcp-Session-Id, Authorization" + // corsAllowedHeaders lists request headers MCP clients may send. MCP-Protocol-Version + // must be allow-listed: ToolHive reads and validates it on the request path + // (an unsupported value yields 400), so a browser MCP client cannot send it + // through CORS unless it is listed here. + corsAllowedHeaders = "Content-Type, Accept, Mcp-Session-Id, MCP-Protocol-Version, Authorization" - // corsExposedHeaders lists response headers that browsers may read. - corsExposedHeaders = "Mcp-Session-Id, Content-Type" + // corsExposedHeaders lists response headers that browsers may read. MCP-Protocol-Version + // is exposed so a browser client can read the negotiated protocol version back. + // Content-Type is omitted because it is already a CORS-safelisted response + // header and does not need to be exposed explicitly. + corsExposedHeaders = "Mcp-Session-Id, MCP-Protocol-Version" // corsMaxAge is the preflight cache lifetime in seconds (24 hours). corsMaxAge = "86400" @@ -38,12 +49,22 @@ const ( // All OPTIONS requests are handled directly (returning 204) when this middleware // is active so that CORS preflights never reach the backend, which previously // returned 405 Method Not Allowed. -func CORS(allowedOrigins []string) types.MiddlewareFunction { +// +// allowedMethods is the value advertised in Access-Control-Allow-Methods. It +// should reflect the methods the backend actually accepts so a preflight never +// succeeds for a method the real request would reject. When empty, +// defaultCORSAllowedMethods is used. +func CORS(allowedOrigins []string, allowedMethods string) types.MiddlewareFunction { if len(allowedOrigins) == 0 { return func(next http.Handler) http.Handler { return next } } - slog.Debug("CORS middleware configured", "allowed_origins", strings.Join(allowedOrigins, ", ")) + if allowedMethods == "" { + allowedMethods = defaultCORSAllowedMethods + } + + slog.Debug("CORS middleware configured", + "allowed_origins", strings.Join(allowedOrigins, ", "), "allowed_methods", allowedMethods) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -53,7 +74,7 @@ func CORS(allowedOrigins []string) types.MiddlewareFunction { if matched != "" { h := w.Header() h.Set("Access-Control-Allow-Origin", matched) - h.Set("Access-Control-Allow-Methods", corsAllowedMethods) + h.Set("Access-Control-Allow-Methods", allowedMethods) h.Set("Access-Control-Allow-Headers", corsAllowedHeaders) h.Set("Access-Control-Expose-Headers", corsExposedHeaders) h.Add("Vary", "Origin") @@ -93,9 +114,51 @@ func matchCORSOrigin(requestOrigin string, allowed []string) string { case entry == requestOrigin: return requestOrigin case strings.HasPrefix(requestOrigin, entry+":"): - // "http://localhost" matches "http://localhost:6274", etc. + // The trailing ":" boundary is load-bearing: it ensures the entry + // matches only ":" and never a longer host. Without it, + // the entry "http://localhost" would also match + // "http://localhost.evil.com". See cors_test.go for the invariant. return requestOrigin } } return "" } + +// ValidateAndNormalizeOrigins validates configured CORS origins and returns a +// normalized copy. It surfaces misconfiguration at startup instead of letting an +// origin silently never match (which produces a broken browser experience with +// no signal): +// +// - "*" (wildcard) is passed through unchanged. +// - A trailing slash (e.g. "http://localhost:6274/") is stripped — a browser +// Origin header never carries one — with a warning, so the entry still matches. +// - An entry without a scheme (e.g. "localhost:6274") can never match a browser +// Origin header (always scheme://host[:port]) and is rejected with an error. +func ValidateAndNormalizeOrigins(origins []string) ([]string, error) { + normalized := make([]string, 0, len(origins)) + for _, origin := range origins { + entry := strings.TrimSpace(origin) + + if entry == "*" { + normalized = append(normalized, entry) + continue + } + + if strings.HasSuffix(entry, "/") { + stripped := strings.TrimRight(entry, "/") + slog.Warn("CORS origin has a trailing slash that browsers never send; normalizing", + "origin", origin, "normalized", stripped) + entry = stripped + } + + // A browser Origin is scheme://host[:port]; without a scheme the entry + // can never match an incoming Origin header. + if !strings.Contains(entry, "://") { + return nil, fmt.Errorf( + "invalid CORS origin %q: missing scheme (expected e.g. %q)", origin, "http://localhost:6274") + } + + normalized = append(normalized, entry) + } + return normalized, nil +} diff --git a/pkg/transport/middleware/cors_test.go b/pkg/transport/middleware/cors_test.go index d0dccde95e..e339b540f4 100644 --- a/pkg/transport/middleware/cors_test.go +++ b/pkg/transport/middleware/cors_test.go @@ -85,6 +85,23 @@ func TestMatchCORSOrigin(t *testing.T) { allowed: []string{"http://localhost:6274"}, expectedResult: "", }, + { + // Invariant: the entry+":" colon boundary must prevent an evil + // subdomain from matching a scheme+host entry. Without the trailing + // ":" check, "http://localhost" would also match + // "http://localhost.evil.com". + name: "colon boundary: evil subdomain must NOT match scheme+host entry", + requestOrigin: "http://localhost.evil.com", + allowed: []string{"http://localhost"}, + expectedResult: "", + }, + { + // Same invariant with a port-bearing evil host. + name: "colon boundary: evil subdomain with port must NOT match", + requestOrigin: "http://localhost.evil.com:6274", + allowed: []string{"http://localhost"}, + expectedResult: "", + }, } for _, tc := range tests { @@ -100,7 +117,7 @@ func TestCORS_EmptyOrigins_IsNoop(t *testing.T) { t.Parallel() inner, called := newCallTracker() - mwFn := CORS(nil)(inner) + mwFn := CORS(nil, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/mcp", nil) @@ -116,7 +133,7 @@ func TestCORS_NonOptions_MatchingOrigin_AddsCORSHeaders(t *testing.T) { t.Parallel() inner, _ := newCallTracker() - mwFn := CORS([]string{"http://localhost:6274"})(inner) + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, "/mcp", nil) @@ -126,7 +143,7 @@ func TestCORS_NonOptions_MatchingOrigin_AddsCORSHeaders(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "http://localhost:6274", rec.Header().Get("Access-Control-Allow-Origin")) - assert.Equal(t, corsAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) + assert.Equal(t, defaultCORSAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) assert.Equal(t, corsAllowedHeaders, rec.Header().Get("Access-Control-Allow-Headers")) assert.Equal(t, corsExposedHeaders, rec.Header().Get("Access-Control-Expose-Headers")) assert.Contains(t, rec.Header().Get("Vary"), "Origin") @@ -136,7 +153,7 @@ func TestCORS_NonOptions_NonMatchingOrigin_NoCORSHeaders(t *testing.T) { t.Parallel() inner, called := newCallTracker() - mwFn := CORS([]string{"http://localhost:6274"})(inner) + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, "/mcp", nil) @@ -152,7 +169,7 @@ func TestCORS_Preflight_MatchingOrigin_Returns204WithHeaders(t *testing.T) { t.Parallel() inner, called := newCallTracker() - mwFn := CORS([]string{"http://localhost:6274"})(inner) + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) @@ -165,7 +182,7 @@ func TestCORS_Preflight_MatchingOrigin_Returns204WithHeaders(t *testing.T) { require.Equal(t, http.StatusNoContent, rec.Code, "preflight must return 204 No Content") assert.False(t, *called, "inner handler must NOT be called for preflight") assert.Equal(t, "http://localhost:6274", rec.Header().Get("Access-Control-Allow-Origin")) - assert.Equal(t, corsAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) + assert.Equal(t, defaultCORSAllowedMethods, rec.Header().Get("Access-Control-Allow-Methods")) assert.Equal(t, corsAllowedHeaders, rec.Header().Get("Access-Control-Allow-Headers")) assert.Equal(t, corsMaxAge, rec.Header().Get("Access-Control-Max-Age")) } @@ -174,7 +191,7 @@ func TestCORS_Preflight_NonMatchingOrigin_Returns204NoHeaders(t *testing.T) { t.Parallel() inner, called := newCallTracker() - mwFn := CORS([]string{"http://localhost:6274"})(inner) + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) @@ -193,7 +210,7 @@ func TestCORS_PrefixMatch_LocalhostAnyPort(t *testing.T) { inner, _ := newCallTracker() // "http://localhost" should match http://localhost on any port - mwFn := CORS([]string{"http://localhost"})(inner) + mwFn := CORS([]string{"http://localhost"}, defaultCORSAllowedMethods)(inner) origins := []string{ "http://localhost:6274", @@ -220,7 +237,7 @@ func TestCORS_Wildcard_MatchesAnyOrigin(t *testing.T) { t.Parallel() inner, _ := newCallTracker() - mwFn := CORS([]string{"*"})(inner) + mwFn := CORS([]string{"*"}, defaultCORSAllowedMethods)(inner) origins := []string{ "http://localhost:6274", @@ -247,7 +264,7 @@ func TestCORS_NoOriginHeader_NoCORSHeaders(t *testing.T) { t.Parallel() inner, called := newCallTracker() - mwFn := CORS([]string{"http://localhost:6274"})(inner) + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, "/mcp", nil) @@ -258,3 +275,102 @@ func TestCORS_NoOriginHeader_NoCORSHeaders(t *testing.T) { assert.True(t, *called, "non-browser requests must reach the backend") assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin")) } + +func TestCORS_AllowMethods_ReflectsCallerSet(t *testing.T) { + t.Parallel() + + // The advertised preflight methods must be whatever the caller passes (so a + // stateless proxy can advertise only "POST, OPTIONS"), not a hard-coded set. + const statelessMethods = "POST, OPTIONS" + + inner, _ := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"}, statelessMethods)(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) + req.Header.Set("Origin", "http://localhost:6274") + req.Header.Set("Access-Control-Request-Method", "POST") + + mwFn.ServeHTTP(rec, req) + + assert.Equal(t, statelessMethods, rec.Header().Get("Access-Control-Allow-Methods"), + "preflight must advertise exactly the caller-supplied method set") +} + +func TestCORS_AllowHeaders_IncludeMCPProtocolVersion(t *testing.T) { + t.Parallel() + + inner, _ := newCallTracker() + mwFn := CORS([]string{"http://localhost:6274"}, defaultCORSAllowedMethods)(inner) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/mcp", nil) + req.Header.Set("Origin", "http://localhost:6274") + + mwFn.ServeHTTP(rec, req) + + allowHeaders := rec.Header().Get("Access-Control-Allow-Headers") + assert.Contains(t, allowHeaders, "MCP-Protocol-Version", + "MCP-Protocol-Version must be allow-listed so browser clients can send it") + + exposeHeaders := rec.Header().Get("Access-Control-Expose-Headers") + assert.Contains(t, exposeHeaders, "MCP-Protocol-Version", + "MCP-Protocol-Version must be exposed so browser clients can read the negotiated version") + assert.NotContains(t, exposeHeaders, "Content-Type", + "Content-Type is CORS-safelisted and should not be redundantly exposed") +} + +func TestValidateAndNormalizeOrigins(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input []string + expected []string + wantErr bool + }{ + { + name: "valid origins pass through unchanged", + input: []string{"http://localhost:6274", "https://app.example.com"}, + expected: []string{"http://localhost:6274", "https://app.example.com"}, + }, + { + name: "wildcard passes through", + input: []string{"*"}, + expected: []string{"*"}, + }, + { + name: "trailing slash is normalized away", + input: []string{"http://localhost:6274/"}, + expected: []string{"http://localhost:6274"}, + }, + { + name: "surrounding whitespace is trimmed", + input: []string{" http://localhost:6274 "}, + expected: []string{"http://localhost:6274"}, + }, + { + name: "missing scheme is rejected", + input: []string{"localhost:6274"}, + wantErr: true, + }, + { + name: "bare host is rejected", + input: []string{"example.com"}, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateAndNormalizeOrigins(tc.input) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.expected, got) + }) + } +} diff --git a/pkg/transport/proxy/transparent/method_gate_test.go b/pkg/transport/proxy/transparent/method_gate_test.go index 9bfc2e35c5..a7e72d524c 100644 --- a/pkg/transport/proxy/transparent/method_gate_test.go +++ b/pkg/transport/proxy/transparent/method_gate_test.go @@ -66,8 +66,28 @@ func TestStatelessMethodGate(t *testing.T) { assert.Equal(t, tc.expectedStatus, rec.Code) if tc.expectAllow { - assert.Equal(t, "POST, OPTIONS", rec.Header().Get("Allow")) + assert.Equal(t, statelessAllowedMethods, rec.Header().Get("Allow")) } }) } } + +// TestCORSAllowedMethodsMatchGate guards the invariant that the CORS preflight +// advertises exactly the methods the server actually accepts. A stateless proxy +// only allows POST/OPTIONS, so a browser must not be told it can preflight GET +// or DELETE and then have the real request 405. +func TestCORSAllowedMethodsMatchGate(t *testing.T) { + t.Parallel() + + stateful := &TransparentProxy{stateless: false} + assert.Equal(t, statefulAllowedMethods, stateful.corsAllowedMethods(), + "stateful proxy must advertise the full method set") + + stateless := &TransparentProxy{stateless: true} + assert.Equal(t, statelessAllowedMethods, stateless.corsAllowedMethods(), + "stateless proxy must advertise only the methods the gate permits") + + // The stateless preflight must never advertise a method the gate rejects. + assert.NotContains(t, stateless.corsAllowedMethods(), "GET") + assert.NotContains(t, stateless.corsAllowedMethods(), "DELETE") +} diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index 6391822f2a..0e645de688 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -362,6 +362,11 @@ func WithAllowedOrigins(origins []string) Option { // The endpointPrefix parameter specifies an explicit prefix to prepend to SSE endpoint URLs. // The trustProxyHeaders parameter indicates whether to trust X-Forwarded-* headers from reverse proxies. // The prefixHandlers parameter is a map of path prefixes to HTTP handlers mounted before the catch-all proxy handler. +// +// It is a stable positional-argument convenience wrapper around +// NewTransparentProxyWithOptions and is retained for existing callers. New +// configuration knobs (such as WithAllowedOrigins) are exposed only as +// functional Options; reach for NewTransparentProxyWithOptions when you need them. func NewTransparentProxy( host string, port int, @@ -1243,8 +1248,10 @@ func (p *TransparentProxy) Start(ctx context.Context) error { // before the method gate or backend can reject them. Only active when // corsOrigins is non-empty (opt-in, no behaviour change by default). if len(p.corsOrigins) > 0 { - finalHandler = middleware.CORS(p.corsOrigins)(finalHandler) - slog.Debug("CORS middleware applied", "allowed_origins", p.corsOrigins) + corsMethods := p.corsAllowedMethods() + finalHandler = middleware.CORS(p.corsOrigins, corsMethods)(finalHandler) + slog.Debug("CORS middleware applied", + "allowed_origins", p.corsOrigins, "allowed_methods", corsMethods) } mux.Handle("/", finalHandler) @@ -1503,6 +1510,30 @@ func (*TransparentProxy) ForwardResponseToClients(_ context.Context, _ jsonrpc2. return fmt.Errorf("ForwardResponseToClients not implemented for TransparentProxy") } +// HTTP method sets advertised to clients. These are the single source of truth +// shared by statelessMethodGate's "Allow" header and the CORS preflight +// (Access-Control-Allow-Methods) so a browser never preflights a method the +// backend would then reject. +const ( + // statelessAllowedMethods are the methods a stateless (POST-only) MCP server + // accepts. OPTIONS is included for CORS preflight handling. + statelessAllowedMethods = "POST, OPTIONS" + + // statefulAllowedMethods are the methods a full streamable-HTTP MCP server + // accepts (GET for the SSE stream, DELETE for session teardown). + statefulAllowedMethods = "GET, POST, DELETE, OPTIONS" +) + +// corsAllowedMethods returns the method set the CORS preflight should advertise +// for this proxy, derived from the same source of truth the request path +// enforces (see statelessMethodGate). +func (p *TransparentProxy) corsAllowedMethods() string { + if p.stateless { + return statelessAllowedMethods + } + return statefulAllowedMethods +} + // statelessMethodGate wraps a handler to reject GET, HEAD, and DELETE requests with 405. // Used in stateless mode where the server only supports POST. // HEAD is blocked alongside GET because HEAD is semantically a GET without a response body; @@ -1510,7 +1541,7 @@ func (*TransparentProxy) ForwardResponseToClients(_ context.Context, _ jsonrpc2. func statelessMethodGate(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodDelete { - w.Header().Set("Allow", "POST, OPTIONS") + w.Header().Set("Allow", statelessAllowedMethods) http.Error(w, "method not allowed: server is stateless (POST only)", http.StatusMethodNotAllowed) return }