-
Couldn't load subscription status.
- Fork 703
feat(srv/stream): Require Protocol Version Header in HTTP #423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change introduces server-side validation of the Changes
Possibly related issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mcp/types.go(1 hunks)server/streamable_http.go(6 hunks)server/streamable_http_test.go(5 hunks)
🔇 Additional comments (7)
mcp/types.go (1)
101-103: LGTM! Well-documented constant addition.The new
DEFAULT_NEGOTIATED_VERSIONconstant is properly documented and correctly references the MCP specification. The value appropriately matchesLATEST_PROTOCOL_VERSION.server/streamable_http_test.go (3)
26-26: Good practice to centralize the test constant.Using a centralized constant for the protocol version in tests improves maintainability.
630-648: Excellent error handling improvement!The addition of the error channel and early failure detection makes the test more robust and easier to debug when failures occur.
797-971: Comprehensive test coverage for protocol version validation!The test thoroughly covers all HTTP methods (POST, GET, DELETE) with different protocol version scenarios:
- Negotiated version from initialization
- Supported but different version
- Unsupported/invalid version
This aligns perfectly with the MCP specification requirements.
server/streamable_http.go (3)
209-211: Good constant naming consistency.The new
headerKeyProtocolVersionconstant follows the established naming pattern and uses the correct header name as specified in the MCP documentation.
509-556: Excellent refactoring of validation logic!The centralized validation approach with separate methods for session and protocol version validation:
- Improves code maintainability and readability
- Ensures consistent validation across all HTTP methods
- Provides clear error messages with supported versions
- Correctly defaults to
DEFAULT_NEGOTIATED_VERSIONwhen header is missingThe implementation aligns perfectly with the MCP specification requirements.
245-246: Consistent validation across all HTTP methods.The validation is properly applied to POST (except for initialize requests), GET, and DELETE methods, ensuring uniform enforcement of the protocol version requirement as specified in the MCP documentation.
Also applies to: 349-351, 449-451
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| return | ||
| } | ||
| } else if !s.validateRequestHeaders(w, r) { | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to store this new header in the session state during initialization if the server is stateful. From the mcp spec, I am interpreting that when the header is "invalid" is when the client uses a different protocol version header than what was initialized. Otherwise, what you have is good is my book
If using HTTP, the client MUST include the MCP-Protocol-Version: HTTP header on all subsequent requests to the MCP server, allowing the MCP server to respond based on the MCP protocol version.
The protocol version sent by the client SHOULD be the one negotiated during initialization.
If the server receives a request with an invalid or unsupported MCP-Protocol-Version, it MUST respond with 400 Bad Request.
Description
Implement the subtask of adding protocol version header validation to
StreamableHTTPServerin mcp-go.Type of Change
Checklist
MCP Spec Compliance
Summary by CodeRabbit