Skip to content

Conversation

@StarpTech
Copy link
Contributor

@StarpTech StarpTech commented Nov 19, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced header filtering to prevent hop-by-hop headers (Proxy-Authenticate, Proxy-Authorization, Alt-Svc, Proxy-Connection) from being forwarded to downstream GraphQL servers, while ensuring authorized headers are properly transmitted. Improved error handling for missing request headers.✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

The PR adds filtering logic to prevent specific hop-by-hop and filtered headers (e.g., Proxy-Authenticate, Proxy-Authorization, Alt-Svc, Proxy-Connection) from being forwarded to downstream GraphQL servers during request execution, along with test coverage verifying the filtering behavior.

Changes

Cohort / File(s) Summary
Header forwarding filtering logic
router/pkg/mcpserver/server.go
Introduces a set of headers to skip during request forwarding, adds filtering logic in GraphQL query execution to exclude these headers, and enhances error handling for missing request headers extraction from context.
Header filtering test verification
router-tests/mcp_test.go
Adds a new subtest validating that hop-by-hop headers are not forwarded while permitted headers remain forwarded, verifying Content-Type, Accept, and Accept-Encoding propagation across the forwarding chain.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The header filtering implementation is straightforward—defining a skip set and applying it during forwarding
  • Test addition follows existing patterns and verifies expected behavior clearly
  • Changes are localized to a specific concern (header forwarding)

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing hop-by-hop and filtered headers from being forwarded to the upstream GraphQL server, which is the core objective of this PR.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dde0ec0 and 67d053b.

📒 Files selected for processing (2)
  • router-tests/mcp_test.go (1 hunks)
  • router/pkg/mcpserver/server.go (2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.

Applied to files:

  • router-tests/mcp_test.go
📚 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.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-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-09-01T13:44:13.045Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.045Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false as the first check, so no external nil checks are needed when calling c.retryOptions.IsEnabled(). This is a consistent pattern used across multiple IsEnabled() methods in the codebase.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.

Applied to files:

  • router-tests/mcp_test.go
📚 Learning: 2025-09-01T13:44:13.045Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/router_config.go:221-221
Timestamp: 2025-09-01T13:44:13.045Z
Learning: The IsEnabled() method on SubgraphRetryOptions in router/core/router.go safely handles nil receivers by returning false, so no external nil checks are needed when calling c.retryOptions.IsEnabled().

Applied to files:

  • router-tests/mcp_test.go
🧬 Code graph analysis (1)
router-tests/mcp_test.go (3)
router-tests/testenv/testenv.go (4)
  • Run (105-122)
  • Config (285-342)
  • SubgraphsConfig (367-380)
  • Environment (1733-1769)
router/pkg/config/config.go (6)
  • Config (1010-1085)
  • MCPConfiguration (970-980)
  • HeaderRules (254-259)
  • GlobalHeaderRule (261-265)
  • RequestHeaderRule (279-302)
  • HeaderRuleOperationPropagate (270-270)
router/core/router.go (2)
  • Option (172-172)
  • WithHeaderRules (1730-1734)
⏰ 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: build_push_image
  • GitHub Check: integration_test (./events)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: image_scan
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/pkg/mcpserver/server.go (2)

40-61: LGTM! Well-designed header filtering list.

The skippedHeaders map correctly includes:

  • RFC 7230 hop-by-hop headers (Connection, Keep-Alive, Te, Trailer, Transfer-Encoding, Upgrade)
  • Proxy-related headers for security
  • Request-specific headers that are explicitly set later (Content-Type, Accept at lines 723-724)
  • WebSocket headers that don't apply to GraphQL requests

All keys are in canonical form, which is correct for use with http.Header.


710-720: LGTM! Efficient header filtering implementation.

The filtering logic correctly:

  1. Iterates through headers from the context
  2. Skips headers present in the skippedHeaders map (lines 712-715)
  3. Forwards all other headers to the GraphQL request

The explicit setting of Accept and Content-Type headers at lines 723-724 after the filtering loop ensures these critical headers have the correct values for GraphQL requests, regardless of what the client sent.

router-tests/mcp_test.go (1)

878-983: LGTM! Comprehensive test coverage for header filtering.

This test effectively validates the header filtering implementation by:

  1. Configuring wildcard header forwarding (.*) to prove the MCP server filters before the router
  2. Sending multiple categories of headers that should be filtered (hop-by-hop, proxy, content-negotiation)
  3. Including a control header (X-Allowed-Header) to verify non-filtered headers are forwarded
  4. Asserting that filtered headers are absent or replaced with correct values
  5. Verifying Content-Type and Accept-Encoding are set appropriately by the MCP server and Go HTTP client

The test follows the existing pattern established at line 760 and provides end-to-end validation of the filtering behavior.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Nov 19, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-67f36459054b4770d92333b2c0fd2d01a7294393

@StarpTech StarpTech merged commit d508fa1 into main Nov 19, 2025
38 of 39 checks passed
@StarpTech StarpTech deleted the dustin/dont-forward-hop-headers branch November 19, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants