Allow more accept headers#429
Merged
halter73 merged 1 commit intomodelcontextprotocol:mainfrom May 20, 2025
Merged
Conversation
eiriktsarpalis
approved these changes
May 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The StreamableHttpHandler was a bit pickier about accept headers than I think it should be. While the spec does say the following:
https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#sending-messages-to-the-server
I don't think it's necessary to the server to be so strict enforcing this. If a client wants to say it accepts "
*/*", it can deal with the fallout if it really cannot handle a JSON or SSE response. The most visible impact of this will probably be the response you see if you enter a Streamable HTTP endpoint URL directly into your browser's address bar since it does typically accept "*/*". Now, instead of seeing a 406 response with the following body:{ "error": { "code": -32000, "message": "Not Acceptable: Client must accept text/event-stream" }, "id": "", "jsonrpc": "2.0" }You'll see a 404 response with this body instead:
{ "error": { "code": -32001, "message": "Session not found" }, "id": "", "jsonrpc": "2.0" }I think both of these are about just as useful to someone hitting a Streamable HTTP endpoint URL directly with the browser in conveying that something is responding to the endpoint, but a plain GET request from the browser without the right headers is unsupported.
We could decide to still reject "
*/*" considering the spec says clients MUST list both application/json and text/event-stream, but it would take a little more code. By leveraging the MatchesMediaType() method, we support accept headers like "*/*" and headers with extra parameters like the "text/event-stream, application/json;q=0.9" from the test. Whatever we decide for "*/*" we should ignore these extra parameters.