mcp: expose Mcp-Session-Id via confighttp CORS exposed_headers#8128
mcp: expose Mcp-Session-Id via confighttp CORS exposed_headers#8128Rajneesh180 wants to merge 1 commit into
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.
27acae3 to
9d809a0
Compare
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
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.
| 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"}, |
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")) | ||
| } |
|
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. |
|
@yurishkuro gentle ping — latest push from two weeks ago addressed the remaining feedback: switched |
| Endpoint: ports.PortToHostPort(ports.MCPHTTP), | ||
| Transport: confignet.TransportTypeTCP, | ||
| }, | ||
| CORS: configoptional.Some(confighttp.CORSConfig{ |
There was a problem hiding this comment.
remove these defaults, they are unsafe and it should be up to the user to provide such defaults if they want
|
there are merge conflicts |
github-actions
Bot
added
the
waiting-for-author
github-actions
Bot
removed
the
waiting-for-author
github-actions
Bot
removed
the
waiting-for-author
Rajneesh180
changed the title
b6c407e to
690545c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
|
|
||
| allowHeaders := resp.Header.Get("Access-Control-Allow-Headers") |
There was a problem hiding this comment.
Header names are case-insensitive, and rs/cors may canonicalize mirrored Access-Control-Request-Headers values (e.g., "Mcp-Session-Id" / "X-Custom-Header") before returning Access-Control-Allow-Headers. These assertions currently require lowercase substrings, which can make the test fail even when behavior is correct. Consider normalizing allowHeaders (e.g., strings.ToLower) before checking contents, or performing case-insensitive matching.
| allowHeaders := resp.Header.Get("Access-Control-Allow-Headers") | |
| allowHeaders := strings.ToLower(resp.Header.Get("Access-Control-Allow-Headers")) |
| defer server.Shutdown(context.Background()) | ||
|
|
||
| addr := server.listener.Addr().String() |
There was a problem hiding this comment.
The new CORS tests start the server and immediately issue HTTP requests without waiting for the Serve goroutine to start accepting connections. This can cause occasional connection-refused flakes (this file already has waitForServer/startTestServer helpers used elsewhere). Consider calling waitForServer(addr) after Start, and using t.Cleanup to ensure Shutdown runs even if an assertion fails.
| defer server.Shutdown(context.Background()) | |
| addr := server.listener.Addr().String() | |
| t.Cleanup(func() { | |
| require.NoError(t, server.Shutdown(context.Background())) | |
| }) | |
| addr := server.listener.Addr().String() | |
| require.NoError(t, waitForServer(addr)) |
690545c to
7606cfc
Compare
|
Right,
@yurishkuro the diff is 3 files, ~12 lines of production code. Latest push rebased on current main and cleaned up the tests (switched to |
7606cfc to
bb24fc4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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", | ||
| MaxSpanDetailsPerRequest: 20, | ||
| MaxSearchResults: 100, | ||
| } |
| req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize"}`)) | ||
| require.NoError(t, err) | ||
| req.Header.Set("Origin", "http://localhost:3000") | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
|
|
|
if otel CORS doesn't provide the ability to set those please create a PR in OTEL for that. The underlying cors library that OTEL uses does seem to support this header: |
github-actions
Bot
added
the
waiting-for-author
bb24fc4 to
b9cbff6
Compare
github-actions
Bot
removed
the
waiting-for-author
`CORSConfig` maps `AllowedOrigins`, `AllowedHeaders`, and `MaxAge` into `cors.Options`, but doesn't map `ExposedHeaders` even though `rs/cors` supports it. This means there's no way to set `Access-Control-Expose-Headers` through receiver config — anyone who needs it has to write a custom middleware. This came up in jaegertracing/jaeger#8128 where the Jaeger UI needs specific headers exposed over CORS but has to work around this gap with a hand-rolled middleware. ### Changes - Add `ExposedHeaders []string` to `CORSConfig` - Wire it into `cors.Options` in `ToServer()` - Add test - Update README Signed-off-by: Rajneesh Chaudhary <[email protected]>
b9cbff6 to
2721f23
Compare
…rtracing#7827) confighttp v0.154.0 added ExposedHeaders to CORSConfig (open-telemetry/opentelemetry-collector#15119), so the MCP server no longer needs a custom middleware to send Access-Control-Expose-Headers. Browser-based MCP clients can read the Mcp-Session-Id response header by listing it under cors::exposed_headers. - bump confighttp to v0.154.0 along with its interlocked collector modules - drop the custom expose-headers middleware - document the CORS setup for browser clients in README and Config - cover CORS preflight, origin matching, allowed-headers auto-mirror, and exposed_headers in server tests Signed-off-by: Rajneesh Chaudhary <[email protected]>
| go.opentelemetry.io/collector/client v1.60.0 | ||
| go.opentelemetry.io/collector/component v1.60.0 | ||
| go.opentelemetry.io/collector/component/componentstatus v0.153.0 | ||
| go.opentelemetry.io/collector/component/componenttest v0.153.0 | ||
| go.opentelemetry.io/collector/config/configauth v1.59.0 | ||
| go.opentelemetry.io/collector/component/componenttest v0.154.0 | ||
| go.opentelemetry.io/collector/config/configauth v1.60.0 | ||
| go.opentelemetry.io/collector/config/configgrpc v0.153.0 | ||
| go.opentelemetry.io/collector/config/confighttp v0.153.0 | ||
| go.opentelemetry.io/collector/config/confighttp v0.154.0 | ||
| go.opentelemetry.io/collector/config/confighttp/xconfighttp v0.153.0 | ||
| go.opentelemetry.io/collector/config/configmiddleware v1.59.0 | ||
| go.opentelemetry.io/collector/config/confignet v1.59.0 | ||
| go.opentelemetry.io/collector/config/configoptional v1.59.0 | ||
| go.opentelemetry.io/collector/config/configmiddleware v1.60.0 | ||
| go.opentelemetry.io/collector/config/confignet v1.60.0 | ||
| go.opentelemetry.io/collector/config/configoptional v1.60.0 | ||
| go.opentelemetry.io/collector/config/configretry v1.59.0 | ||
| go.opentelemetry.io/collector/config/configtelemetry v0.153.0 | ||
| go.opentelemetry.io/collector/config/configtls v1.59.0 | ||
| go.opentelemetry.io/collector/confmap v1.59.0 | ||
| go.opentelemetry.io/collector/config/configtls v1.60.0 | ||
| go.opentelemetry.io/collector/confmap v1.60.0 |
| go.opentelemetry.io/collector/confmap/xconfmap v0.154.0 | ||
| go.opentelemetry.io/collector/connector v0.153.0 | ||
| go.opentelemetry.io/collector/connector/forwardconnector v0.153.0 | ||
| go.opentelemetry.io/collector/consumer v1.59.0 | ||
| go.opentelemetry.io/collector/consumer v1.60.0 |
| go.opentelemetry.io/collector/extension v1.60.0 | ||
| go.opentelemetry.io/collector/extension/extensionauth v1.60.0 | ||
| go.opentelemetry.io/collector/extension/extensioncapabilities v0.153.0 | ||
| go.opentelemetry.io/collector/extension/zpagesextension v0.153.0 | ||
| go.opentelemetry.io/collector/featuregate v1.59.0 | ||
| go.opentelemetry.io/collector/featuregate v1.60.0 | ||
| go.opentelemetry.io/collector/otelcol v0.153.0 | ||
| go.opentelemetry.io/collector/pdata v1.59.0 | ||
| go.opentelemetry.io/collector/pdata v1.60.0 | ||
| go.opentelemetry.io/collector/pdata/xpdata v0.153.0 | ||
| go.opentelemetry.io/collector/pipeline v1.59.0 | ||
| go.opentelemetry.io/collector/processor v1.59.0 |
| resp, err := http.DefaultClient.Do(req) | ||
| require.NoError(t, err) |
| resp, err := http.DefaultClient.Do(req) | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
|
|
||
| exposed := resp.Header.Get("Access-Control-Expose-Headers") | ||
| assert.Contains(t, exposed, "Mcp-Session-Id", | ||
| "exposed_headers config must surface Mcp-Session-Id so browsers can read it from cross-origin responses") |
Rajneesh180
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8128 +/- ##
==========================================
- Coverage 96.90% 96.84% -0.07%
==========================================
Files 336 336
Lines 17921 17921
==========================================
- Hits 17367 17356 -11
- Misses 401 413 +12
+ Partials 153 152 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I don't see any functional changes in this PR. |
github-actions
Bot
added
the
waiting-for-author
Which problem is this PR solving?
Part of #7827
The MCP protocol requires clients to read the
Mcp-Session-Idresponse header, so the server must sendAccess-Control-Expose-Headers: Mcp-Session-Idfor browser-based (cross-origin) clients to access it.confighttp.CORSConfigpreviously did not map theExposedHeadersoption fromrs/cors, which is why an earlier revision of this PR carried a small middleware.Per the review feedback, the capability now lives upstream: open-telemetry/opentelemetry-collector#15119 added
ExposedHeaderstoconfighttp.CORSConfig, released in confighttp v0.154.0. This PR drops the middleware entirely and relies on standard confighttp configuration.Description of the changes
go.opentelemetry.io/collector/config/confighttpto v0.154.0 (with its interlocked collector modules), which addscors::exposed_headerssupportserver.gois identical to mainexposed_headers: ["Mcp-Session-Id"]) in the extension README and Config commentHow was this change tested?
TestCORSExposeHeadersMcpSessionID— verifiescors::exposed_headerssurfacesAccess-Control-Expose-Headers: Mcp-Session-Idon a real cross-origin requestTestCORSPreflight— preflight 204 +Access-Control-Allow-Origin+ allowed methodsTestCORSConfigurableOrigins— wildcard, exact match, blocked, subdomain match/no-match, multiple origins, nil/emptyTestCORSCustomHeaders— allowed-headers auto-mirror covers MCP protocol headersgo test ./cmd/jaeger/internal/extension/jaegermcp/...— all packages pass;go build ./cmd/jaeger/...clean