feat: make CORS settings configurable for MCP server#8128
feat: make CORS settings configurable for MCP server#8128Rajneesh180 wants to merge 4 commits intojaegertracing:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable CORS behavior to the Jaeger MCP server extension so deployments can restrict allowed origins and permit additional request headers, rather than always responding with Access-Control-Allow-Origin: *.
Changes:
- Introduces
CORSConfig(allowed origins + additional allowed headers) and wires it into the MCP server config. - Updates
corsMiddlewareto match/echo allowed origins and to always include required MCP protocol headers inAccess-Control-Allow-Headers. - Expands/updates CORS-related tests with table-driven coverage for origin matching and custom headers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/config.go | Adds CORSConfig and exposes it on the extension Config via mapstructure:"cors". |
| cmd/jaeger/internal/extension/jaegermcp/factory.go | Sets backward-compatible default CORS config (AllowedOrigins: ["*"]). |
| cmd/jaeger/internal/extension/jaegermcp/server.go | Makes CORS behavior configurable; implements origin matching + configurable allowed headers. |
| cmd/jaeger/internal/extension/jaegermcp/server_test.go | Updates preflight test expectations and adds new tests for configurable origins and custom headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use ["*"] to allow any origin. Supports exact matches and wildcard patterns | ||
| // (e.g., "http://*.example.com"). |
There was a problem hiding this comment.
The AllowedOrigins comment says it “supports ... wildcard patterns”, but the implementation in isOriginAllowed has specific (and somewhat permissive) wildcard semantics. To reduce misconfiguration risk, consider documenting the exact supported pattern(s) (e.g., only * and scheme://*.example.com with a single wildcard) and/or validating unsupported patterns early in Validate().
| // Use ["*"] to allow any origin. Supports exact matches and wildcard patterns | |
| // (e.g., "http://*.example.com"). | |
| // Use ["*"] to allow any origin. Literal origins (e.g., "https://example.com") are | |
| // always matched exactly. Wildcard handling, if used, follows the MCP server's | |
| // origin-matching implementation and should be configured with care. |
| origin := r.Header.Get("Origin") | ||
| if origin != "" && isOriginAllowed(origin, cfg.AllowedOrigins) { | ||
| w.Header().Set("Access-Control-Allow-Origin", origin) | ||
| } else if containsWildcard(cfg.AllowedOrigins) { | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
| } |
There was a problem hiding this comment.
When Access-Control-Allow-Origin is set dynamically based on the request Origin, the response should also include Vary: Origin to prevent shared caches/CDNs from serving a response with the wrong CORS headers to a different origin (cache poisoning / data exposure risk). Consider adding Vary: Origin (and for preflights optionally Vary: Access-Control-Request-Method / Vary: Access-Control-Request-Headers).
| // Support wildcard subdomain matching: "http://*.example.com" | ||
| if strings.Contains(pattern, "*") { | ||
| // Split pattern at the wildcard and check prefix/suffix | ||
| parts := strings.SplitN(pattern, "*", 2) | ||
| if len(parts) == 2 && | ||
| strings.HasPrefix(origin, parts[0]) && | ||
| strings.HasSuffix(origin, parts[1]) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
isOriginAllowed currently treats any * in the configured pattern as a wildcard and matches by simple prefix/suffix. This is more permissive than the doc comment implies (“leading wildcard to match subdomains”) and can easily lead to accidentally broad matches (e.g., patterns with * in unexpected positions). Suggest either (a) enforcing/validating the supported form (single wildcard only in the subdomain position like scheme://*.example.com) or (b) updating the documentation to match the actual matching semantics.
| if origin != "" && isOriginAllowed(origin, cfg.AllowedOrigins) { | ||
| w.Header().Set("Access-Control-Allow-Origin", origin) | ||
| } else if containsWildcard(cfg.AllowedOrigins) { | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
| } | ||
|
|
||
| w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS") | ||
| w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Accept, Mcp-Session-Id, Mcp-Protocol-Version, Last-Event-ID") | ||
| w.Header().Set("Access-Control-Allow-Headers", allowHeadersStr) | ||
| w.Header().Set("Access-Control-Expose-Headers", "Mcp-Session-Id") |
There was a problem hiding this comment.
The PR description mentions “correct CORS behavior for credentialed requests”, but this middleware never sets Access-Control-Allow-Credentials: true. Echoing the origin is necessary but not sufficient for credentialed browser requests; if credentials are in scope, this likely needs a configurable allow_credentials flag and to avoid returning * when credentials are enabled. Otherwise, consider adjusting wording/comments to avoid implying credential support.
Rajneesh180
force-pushed
the
feat/configurable-cors-mcp-7827
branch
from
27acae3 to
9d809a0
Compare
Rajneesh180
force-pushed
the
feat/configurable-cors-mcp-7827
branch
from
9d809a0 to
c18ad93
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -230,3 +254,38 @@ func corsMiddleware(next http.Handler) http.Handler { | |||
| next.ServeHTTP(w, r) | |||
| }) | |||
There was a problem hiding this comment.
When an Origin header is present but not allowed, the handler still processes the request (and preflights still return 204) — it just omits Access-Control-Allow-Origin. If the intent is to "restrict which origins can access the MCP endpoint", this doesn’t actually prevent cross-site requests (it only prevents browsers from reading responses) and can still allow CSRF-style side effects or resource usage. Consider explicitly rejecting requests with a disallowed Origin (e.g., return 403 for OPTIONS and non-OPTIONS when Origin is present and not allowed), or clarify in docs/tests that this is only a browser read restriction, not access control.
| defer resp.Body.Close() | ||
|
|
||
| assert.Equal(t, http.StatusNoContent, resp.StatusCode) | ||
| assert.Equal(t, tt.expectedOrigin, resp.Header.Get("Access-Control-Allow-Origin")) |
There was a problem hiding this comment.
The middleware now conditionally adds Vary: Origin when reflecting an allowed Origin, but the new CORS tests only assert Access-Control-Allow-Origin. To prevent regressions in caching behavior, consider extending the relevant subtests to assert Vary is set to Origin when expectedOrigin is a reflected origin (and absent when expectedOrigin is "*" or empty).
| assert.Equal(t, tt.expectedOrigin, resp.Header.Get("Access-Control-Allow-Origin")) | |
| assert.Equal(t, tt.expectedOrigin, resp.Header.Get("Access-Control-Allow-Origin")) | |
| // When the origin is reflected, the middleware should set Vary: Origin | |
| // to ensure correct caching behavior. For wildcard ("*") or empty | |
| // expected origins, Vary should be absent. | |
| if tt.expectedOrigin != "" && tt.expectedOrigin == tt.requestOrigin { | |
| assert.Equal(t, "Origin", resp.Header.Get("Vary")) | |
| } else { | |
| assert.Equal(t, "", resp.Header.Get("Vary")) | |
| } |
| ) | ||
|
|
||
| // CORSConfig holds CORS configuration for the MCP server. | ||
| type CORSConfig struct { |
There was a problem hiding this comment.
Doesn't confighttp.ServerConfig have CORS config?
|
Great catch — you're right, What changed:
This also addresses all of Copilot's review points (missing Net result: -75 lines, and we're aligned with how other collector extensions (like |
| AllowedHeaders: []string{ | ||
| "Mcp-Session-Id", | ||
| "Mcp-Protocol-Version", | ||
| "Last-Event-ID", |
There was a problem hiding this comment.
why should this be in the default config? A default config can be overwritten by the users, then all these headers will become invalid
Rajneesh180
force-pushed
the
feat/configurable-cors-mcp-7827
branch
from
96c8c35 to
84c79d1
Compare
|
Addressed the review feedback:
@yurishkuro ready for re-review when you have a chance. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| assert.Equal(t, http.StatusNoContent, resp.StatusCode) | ||
| assert.Equal(t, "*", resp.Header.Get("Access-Control-Allow-Origin")) | ||
| assert.Equal(t, "GET, POST, DELETE, OPTIONS", resp.Header.Get("Access-Control-Allow-Methods")) | ||
| } |
There was a problem hiding this comment.
TestCORSPreflight no longer asserts Access-Control-Allow-Methods (and it also doesn't validate allowed request headers). Since this test is the basic regression check for the CORS behavior, it should continue to verify the expected preflight response headers produced by the server.
| allowedOrigins: []string{"http://localhost:3000", "http://localhost:8080"}, | ||
| requestOrigin: "http://localhost:8080", | ||
| expectedOrigin: "http://localhost:8080", | ||
| }, |
There was a problem hiding this comment.
The PR description mentions an "empty origins blocks all" scenario, but TestCORSConfigurableOrigins does not include a case where AllowedOrigins is empty/nil. Adding that case would help prevent regressions around the "block all" configuration behavior.
| }, | |
| }, | |
| { | |
| name: "nil origins blocks all", | |
| allowedOrigins: nil, | |
| requestOrigin: "http://example.com", | |
| expectedOrigin: "", | |
| }, | |
| { | |
| name: "empty origins blocks all", | |
| allowedOrigins: []string{}, | |
| requestOrigin: "http://example.com", | |
| expectedOrigin: "", | |
| }, |
| CORS: configoptional.Some(confighttp.CORSConfig{ | ||
| AllowedOrigins: []string{"*"}, | ||
| AllowedHeaders: []string{ | ||
| "Mcp-Session-Id", | ||
| "Mcp-Protocol-Version", | ||
| "Last-Event-ID", | ||
| "X-Custom-Header", | ||
| }, | ||
| }), |
There was a problem hiding this comment.
This test config explicitly includes the MCP protocol headers in AllowedHeaders. If the intended behavior is that MCP protocol headers are always permitted regardless of configuration, the test should avoid listing those headers and instead assert they are still allowed when only a custom header is configured (or update the implementation/description to match the actual behavior).
| // HTTP contains the HTTP server configuration for the MCP protocol endpoint. | ||
| // CORS is configured via HTTP.CORS using confighttp's built-in support. | ||
| HTTP confighttp.ServerConfig `mapstructure:"http"` |
There was a problem hiding this comment.
The PR description references adding a new jaeger_mcp-specific CORSConfig struct, but the code change instead relies on confighttp.ServerConfig.CORS for CORS settings. Either update the PR description to reflect the actual implementation approach, or introduce the described struct if that was the intent.
| s.telset, | ||
| handler, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
If s.config.HTTP.ToServer(...) returns an error, Start returns without closing the listener created earlier via ToListener, which can leak the bound socket/file descriptor. Consider closing s.listener (or the local listener) before returning on this error path.
| if err != nil { | |
| if err != nil { | |
| if s.listener != nil { | |
| _ = s.listener.Close() | |
| s.listener = nil | |
| } |
| Transport: confignet.TransportTypeTCP, | ||
| }, | ||
| CORS: configoptional.Some(confighttp.CORSConfig{ | ||
| AllowedOrigins: []string{"*"}, |
There was a problem hiding this comment.
The default MCP CORS configuration only sets AllowedOrigins, but it no longer whitelists the MCP protocol request headers (e.g., Mcp-Session-Id / Mcp-Protocol-Version / Last-Event-ID) that were previously always allowed by the custom corsMiddleware. With confighttp/rs-cors defaults, preflight requests that include these headers can be rejected unless they are added to AllowedHeaders. To preserve backward compatibility, consider adding the MCP protocol headers to the default AllowedHeaders (and optionally merging any user-configured AllowedHeaders with these defaults).
| AllowedOrigins: []string{"*"}, | |
| AllowedOrigins: []string{"*"}, | |
| AllowedHeaders: []string{"Mcp-Session-Id", "Mcp-Protocol-Version", "Last-Event-ID"}, |
) Add CORSConfig to the MCP extension configuration allowing operators to control which origins and headers are permitted for cross-origin requests to the MCP endpoint. Changes: - Add CORSConfig struct with allowed_origins and allowed_headers fields - Support exact origin matches, wildcard (*), and subdomain patterns (e.g., http://*.example.com) - Echo the matched origin in Access-Control-Allow-Origin instead of literal '*' (correct CORS behavior for credentialed requests) - Always include MCP protocol headers (Mcp-Session-Id, Mcp-Protocol-Version, Last-Event-ID) regardless of config - Default to AllowedOrigins: ["*"] for backward compatibility - Add comprehensive table-driven tests for origin matching Resolves jaegertracing#7827 Signed-off-by: Rajneesh180 <rajneeshrehsaan48@gmail.com>
Replace custom CORS middleware with confighttp's built-in CORS support via HTTP.CORS (configoptional.Optional[CORSConfig]). Key changes: - Remove custom CORSConfig struct; CORS is now configured via HTTP.CORS - Replace manual net.Listen + http.Server with ToListener() + ToServer() which provides TLS, OTel instrumentation, decompression, and auth - Replace ~60 lines of custom corsMiddleware/containsWildcard/isOriginAllowed with confighttp's rs/cors integration - Add thin mcpExposeHeadersMiddleware for Mcp-Session-Id (not exposed by confighttp.CORSConfig) - Update CORS tests to match rs/cors v1.11.1 behavior Addresses review feedback from @yurishkuro: 'Doesn't confighttp.ServerConfig have CORS config?' Signed-off-by: Rajneesh180 <rajneeshrehsaan48@gmail.com>
Signed-off-by: Rajneesh Chaudhary <96375734+Rajneesh180@users.noreply.github.com>
Rajneesh180
force-pushed
the
feat/configurable-cors-mcp-7827
branch
from
c3f4012 to
c401120
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // mcpExposeHeadersMiddleware adds the Access-Control-Expose-Headers header | ||
| // for MCP protocol headers. confighttp's CORSConfig does not expose this | ||
| // rs/cors option, so we handle it in a thin middleware. | ||
| func mcpExposeHeadersMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
| w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS") | ||
| w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Accept, Mcp-Session-Id, Mcp-Protocol-Version, Last-Event-ID") | ||
| w.Header().Set("Access-Control-Expose-Headers", "Mcp-Session-Id") | ||
|
|
||
| // Handle preflight requests | ||
| if r.Method == http.MethodOptions { | ||
| w.WriteHeader(http.StatusNoContent) | ||
| return | ||
| } | ||
|
|
||
| next.ServeHTTP(w, r) | ||
| }) |
| func TestCORSPreflight(t *testing.T) { | ||
| config := &Config{ | ||
| HTTP: confighttp.ServerConfig{ | ||
| NetAddr: confignet.AddrConfig{ | ||
| Endpoint: "localhost:0", | ||
| Transport: confignet.TransportTypeTCP, | ||
| }, | ||
| CORS: configoptional.Some(confighttp.CORSConfig{ | ||
| AllowedOrigins: []string{"*"}, | ||
| }), | ||
| }, | ||
| ServerName: "jaeger-test", | ||
| ServerVersion: "1.0.0", | ||
| } | ||
|
|
||
| server := newServer(config, componenttest.NewNopTelemetrySettings()) | ||
| host := newMockHost() | ||
| err := server.Start(context.Background(), host) | ||
| require.NoError(t, err) | ||
| defer server.Shutdown(context.Background()) | ||
|
|
||
| addr := server.listener.Addr().String() | ||
| url := fmt.Sprintf("http://%s/mcp", addr) | ||
|
|
||
| req, err := http.NewRequest(http.MethodOptions, url, http.NoBody) | ||
| require.NoError(t, err) | ||
| req.Header.Set("Origin", "http://localhost:3000") | ||
| req.Header.Set("Access-Control-Request-Method", "POST") | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
|
|
||
| assert.Equal(t, http.StatusNoContent, resp.StatusCode) | ||
| assert.Equal(t, "*", resp.Header.Get("Access-Control-Allow-Origin")) | ||
| assert.Equal(t, "GET, POST, DELETE, OPTIONS", resp.Header.Get("Access-Control-Allow-Methods")) | ||
| } |
- Use Header.Add instead of Header.Set in mcpExposeHeadersMiddleware to avoid clobbering Access-Control-Expose-Headers set by confighttp's CORS - Add AllowedHeaders: ["*"] to default CORSConfig so rs/cors mirrors all Access-Control-Request-Headers (MCP protocol headers work without being hardcoded, addressing review concern about overwritten defaults) - Strengthen TestCORSPreflight to assert Access-Control-Allow-Methods - Add nil/empty origins test cases to TestCORSConfigurableOrigins - Fix TestCORSCustomHeaders to test rs/cors auto-mirror behavior with wildcard AllowedHeaders instead of explicitly listing MCP headers Signed-off-by: Rajneesh Chaudhary <96375734+Rajneesh180@users.noreply.github.com>
|
Pushed a fix addressing the remaining review feedback. Key change: Also found a real issue with the default config: with empty Test improvements:
All tests pass locally. Ready for re-review. |
Which problem is this PR solving?
Part of #7827
The MCP server extension hardcodes
Access-Control-Allow-Origin: *and manages HTTP lifecycle (listener, server) manually. This PR switches toconfighttp.ServerConfigfor CORS and HTTP setup, aligning with how other collector extensions handle this.Description of the changes
confighttp: Usesconfighttp.ServerConfig.CORS(backed byrs/cors) instead of a hand-rolled CORS middleware. Default config setsAllowedOrigins: ["*"]for backward compatibility.confighttp: Replaced manualnet.ListenConfig+http.Server{}withconfighttp.ToListener()+confighttp.ToServer(), which provides TLS support, OTel instrumentation, decompression, and auth middleware for free.mcpExposeHeadersMiddleware: Only setsAccess-Control-Expose-Headers: Mcp-Session-Id, sinceconfighttp.CORSConfigdoesn't expose thatrs/corsoption.ToServer()fails, preventing socket leak.Example CORS configuration
How was this change tested?
TestCORSPreflight— verifies preflight204+Access-Control-Allow-OriginviaconfighttpCORSTestCORSConfigurableOrigins— 6 subtests: wildcard, exact match, blocked, subdomain match/no-match, multiple originsTestCORSCustomHeaders— verifies MCP protocol headers + custom headers inAccess-Control-Allow-HeadersFull MCP test suite passes:
go test ./cmd/jaeger/internal/extension/jaegermcp/...— all 4 packages PASS.