feat: use cors configuration also on mcp server#2363
Conversation
WalkthroughThreads a cors.Config from router options into MCP server initialization, adds a config-driven CORS middleware path in the MCP server implementation, exposes WithCORS(...), and updates tests to assert custom Allow-Headers propagate to MCP responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2363 +/- ##
=======================================
Coverage ? 32.33%
=======================================
Files ? 213
Lines ? 22779
Branches ? 0
=======================================
Hits ? 7366
Misses ? 14499
Partials ? 914 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alepane21
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/mcpserver/server.go (1)
804-839: Update the function comment to reflect the new signature.The function comment (lines 804-816) describes accepting methods as parameters and mentions a usage example that no longer matches the current signature. The function now accepts a
corsConfig *cors.Configparameter instead.Apply this diff to update the comment:
-// WithCORS creates a reusable CORS middleware that can be used with any HTTP handler. -// It handles preflight OPTIONS requests and sets appropriate CORS headers. -// -// Example usage: -// -// corsMiddleware := WithCORS("GET", "POST", "PUT", "DELETE") -// http.Handle("/api/", corsMiddleware(apiHandler)) -// -// The middleware sets the following CORS headers: -// - Access-Control-Allow-Origin: * -// - Access-Control-Allow-Methods: specified methods + OPTIONS -// - Access-Control-Allow-Headers: Content-Type, Authorization -// - Access-Control-Max-Age: 86400 (24 hours) +// WithCORS creates a reusable CORS middleware that can be used with any HTTP handler. +// It handles preflight OPTIONS requests and sets appropriate CORS headers based on the provided configuration. +// If corsConfig is nil or disabled, the middleware passes requests through without modification. +// +// Example usage: +// +// corsMiddleware := WithCORS(corsConfig) +// http.Handle("/api/", corsMiddleware(apiHandler))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/core/router.go(1 hunks)router/pkg/mcpserver/server.go(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/core/router.gorouter/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/mcpserver/server.go
🧬 Code graph analysis (1)
router/core/router.go (1)
router/pkg/mcpserver/server.go (1)
WithCORSConfig(311-315)
🔇 Additional comments (8)
router/pkg/mcpserver/server.go (7)
19-20: LGTM!The new imports support the CORS configuration functionality correctly.
96-97: LGTM!The CorsConfig field is well-documented and follows the established pattern for other option fields.
118-118: LGTM!The corsConfig field properly stores the CORS configuration at the server level.
245-245: LGTM!The initialization correctly copies the CORS configuration from options to the server struct.
311-315: LGTM!The WithCORSConfig function follows the established functional options pattern and provides a clean API for configuring CORS.
335-335: LGTM!The CORS configuration is correctly wired into the middleware.
843-853: The design decision to hardcode CORS origin and methods appears intentional but inconsistently documented and implemented.The function includes a comment stating it "Only used for web browsers, not for API clients," which suggests protocol-specific requirements. However, this creates an inconsistency:
config.AllowHeadersis merged with MCP-specific headers (line 846)config.MaxAgeis used with a fallback default (lines 848-852)config.AllowOriginsis hardcoded to"*"(line 844)config.AllowMethodsis hardcoded to"GET, PUT, POST, DELETE, OPTIONS"(line 845)The comment above the
WithCORSfunction (lines 814-816) claims "Access-Control-Allow-Methods: specified methods + OPTIONS," which contradicts the actual hardcoded implementation. This inconsistency between documented intent and actual behavior means developers who pass acors.Configwill be surprised thatAllowOriginsandAllowMethodsare silently ignored whileAllowHeadersandMaxAgeare respected.Consider either:
- Documenting this behavior more clearly (e.g., "MCP requires these specific methods and wildcard origin; only custom headers and max-age from config are applied")
- Merging
config.AllowMethodswith the required MCP methods (like is done for headers)- Respecting
config.AllowOriginswhen a non-wildcard origin is configuredrouter/core/router.go (1)
917-917: LGTM!The CORS configuration is correctly propagated from the router to the MCP server. The router's CORS options (r.corsOptions) are guaranteed to be initialized by lines 264-266, so this is safe.
…on-failure-on-ecs-deployment' into ale/eng-8562-mcp-server-connection-failure-on-ecs-deployment
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router-tests/mcp_test.go(2 hunks)router/core/router.go(1 hunks)router/pkg/mcpserver/server.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/router.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/mcp_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
🧬 Code graph analysis (1)
router-tests/mcp_test.go (3)
router-tests/testenv/testenv.go (3)
Run(105-122)Config(285-342)Environment(1733-1769)router/pkg/config/config.go (2)
Config(1020-1095)MCPConfiguration(980-990)router/core/router.go (2)
Option(172-172)WithCors(1590-1594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
router-tests/mcp_test.go (2)
19-19: LGTM!Import added correctly to support the new CORS configuration test.
564-612: LGTM!The test properly validates that custom CORS headers from router configuration are propagated to the MCP server. It follows the existing test patterns and verifies both custom headers (
Test,X-Custom-Auth) and default MCP headers are present in the response.router/pkg/mcpserver/server.go (5)
19-20: LGTM!Necessary imports for the new CORS functionality.
96-98: LGTM!The
CorsConfigfield is appropriately added to theOptionsstruct, following the existing pattern for configuration options.
311-315: LGTM!The
WithCORSoption function follows the established pattern for option setters in this codebase.
335-342: LGTM!The middleware is properly constructed and applied to the MCP endpoint handler.
804-833: LGTM!The middleware correctly handles preflight OPTIONS requests and sets CORS headers for all responses.
…on-failure-on-ecs-deployment' into ale/eng-8562-mcp-server-connection-failure-on-ecs-deployment
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/pkg/mcpserver/server.go (1)
837-847: CORS header construction is correct and resolves prior duplicate-header issueThe union of
config.AllowHeaderswith the MCP-required defaults and deduplication viastringsx.RemoveDuplicatesnow treats each default header as its own slice element, so duplicates are correctly removed when callers pre-include any of these headers. The max-age defaulting to 24h when unset is also reasonable.
🧹 Nitpick comments (2)
router/pkg/mcpserver/server.go (2)
311-315: Add GoDoc for exported WithCORS helper
WithCORSis exported and follows the same pattern as the otherWith*option setters, but it lacks a GoDoc comment. Adding one will keep linters happy and improve discoverability, e.g.:// WithCORS sets the CORS configuration for the MCP server. func WithCORS(corsCfg cors.Config) func(*Options) { return func(o *Options) { o.CorsConfig = corsCfg } }
804-817: Update corsMiddleware documentation to match current API and behaviorThe doc block still shows
corsMiddleware("GET", "POST", ...)and describes method/header behavior that no longer matches the implementation (which now takes acors.Configand delegates specifics tosetCORSHeaders). It would be good to refresh:
- The example to use
corsMiddleware(corsCfg).- The described headers/methods to align with
setCORSHeaders(hard-coded methods, union ofconfig.AllowHeaderswith the MCP-required defaults, max-age behavior).This avoids future confusion for readers reusing this middleware elsewhere.
Also applies to: 817-833
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/mcpserver/server.go(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/mcpserver/server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/pkg/mcpserver/server.go (3)
19-20: New imports correctly scoped for CORS utilitiesThe additions of
stringsxandcorsare used only in the new CORS helpers and keep dependencies localized; no issues here.
96-98: CORS config threading through Options and GraphQLSchemaServer looks soundStoring
cors.ConfigonOptionsand copying it intoGraphQLSchemaServer.corsConfigprovides a clear, immutable snapshot of the CORS settings at server construction time and is consistent with the other option fields.Also applies to: 118-119, 245-245
335-341: CORS middleware wrapping of /mcp route is straightforwardUsing
middleware := corsMiddleware(s.corsConfig)and applying it only to the/mcphandler keeps CORS concerns localized and preserves the existing behavior while letting you inject config-driven headers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/mcpserver/server.go (1)
310-321: WithCORS cleanly enforces MCP‑specific CORS defaults while reusing router configTaking
cors.Configby value here and overridingAllowOrigins/AllowMethodswhile augmentingAllowHeadersand normalizingMaxAgegives the MCP server its own policy without mutating the router’s shared config, which is a good separation. If you want slightly tidier responses, you could optionally de‑duplicatecorsCfg.AllowHeaders(e.g. using the same helper the router CORS code uses) in case upstream config already includes some of the MCP headers, and consider spelling the MCP header as"Last-Event-Id"to match the canonical form asserted in tests—but both are polish rather than correctness issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/mcp_test.go(5 hunks)router/pkg/mcpserver/server.go(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/mcp_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/pkg/mcpserver/server.go
🧬 Code graph analysis (1)
router-tests/mcp_test.go (1)
router/core/router.go (2)
Option(172-172)WithCors(1590-1594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
router/pkg/mcpserver/server.go (2)
73-97: CORS config plumbing into GraphQLSchemaServer looks consistent; just confirm it’s always set when neededAdding
CorsConfig cors.ConfigtoOptions, mirroring it ascorsConfigonGraphQLSchemaServer, and wiring it throughNewGraphQLSchemaServerintoServeis coherent and keeps CORS concerns encapsulated in the server instance. Please just double‑check that all call sites which expect MCP CORS behavior actually applyWithCORS(...)(or otherwise populateOptions.CorsConfig), socors.Newis never called with an unintended zero‑value config that might silently disable CORS on/mcp.Also applies to: 99-118, 188-247
19-24: CORS middleware wrapping only the /mcp route is scoped and straightforwardImporting
router/pkg/corsand applyingmiddleware := cors.New(s.corsConfig)only around the/mcphandler keeps CORS behavior localized to the MCP endpoint while reusing the shared configuration; the integration withserver.NewStreamableHTTPServerlooks correct and doesn’t introduce any obvious side effects.Also applies to: 323-348
router-tests/mcp_test.go (3)
376-423: Updated Last-Event-Id expectation matches middleware’s header casingSwitching the assertion to
"Last-Event-Id"in the preflight test aligns the expectation with the header value emitted by the CORS middleware while keeping the rest of the CORS checks (methods, headers, max‑age) intact; this looks correct.
425-542: Non‑OPTIONS CORS tests now match typical middleware behaviorFor POST, GET, PUT, and DELETE you now assert only
Access-Control-Allow-Originand explicitly expect emptyAccess-Control-Allow-Methods,Access-Control-Allow-Headers, andAccess-Control-Max-Age. That matches how a focused CORS implementation usually behaves on actual requests (vs preflight) and lines up with the new middleware’s semantics.
19-24: Custom‑header test nicely validates reuse of router CORS config in MCPImporting
router/pkg/corsand wiringRouterOptions: []core.Option{ core.WithCors(&cors.Config{ AllowHeaders: []string{"Test", "X-Custom-Auth"}, }) }into the new test is a good way to assert that router CORS headers flow through to the MCP server. Verifying that the MCP OPTIONS response includes both the MCP‑specific headers and the router‑configured"Test"/"X-Custom-Auth"entries gives solid coverage that the shared config is actually being reused. If you want this to mimic a real browser preflight even more closely, you could also setAccess-Control-Request-MethodandAccess-Control-Request-Headerson the OPTIONS request, but it’s not required for this assertion to be meaningful.Also applies to: 544-592
alepane21
deleted the
ale/eng-8562-mcp-server-connection-failure-on-ecs-deployment
branch
2 participants
We allow the MCP server to forward almost all the headers that it receives, but the CORS middleware of the MCP server was not changed to allow any header other than the basic ones.
I've used the CORS framework that we already have in place and used its configuration also in the MCP server.
The HTTP Methods and some headers are forced based on the needs of the MCP protocol.
I've used directly the cors middleware from the cors pkg, some tests were failing so I had to fix them:
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist