-
Notifications
You must be signed in to change notification settings - Fork 300
Fix: Add Session ID and Header Support to SSE Transport (#660) #661
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
base: main
Are you sure you want to change the base?
Conversation
- Add MessageExtraInfo support to SSE transport matching StreamableHTTP behavior - Pass request headers and auth info through SSE message handling - Add RequestWithAuth type for consistent auth info passing - Filter internal headers (content-type, content-length, upgrade) before passing to MCP server - Add comprehensive tests for header and auth handling in both transports - Ensure transport initialization before processing SSE messages This ensures both SSE and StreamableHTTP transports have consistent header and authentication handling capabilities, allowing MCP servers to access request context regardless of transport type.
🦋 Changeset detectedLatest commit: 2eb4334 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
threepointone
left a comment
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.
this looks good to me, but I'll let @deathbyknowledge have a look before we land it since he'd touched this part of the code
Thank you @threepointone ! Please let us know if you want any modifications. I tried to keep it similar to how Streamable HTTP was implemented. |
deathbyknowledge
left a comment
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.
Thanks for the PR! While we want to add support for the extra parameter for the SSE transport, there are a few open questions that I'd like to ask.
Specially regarding request.auth, I'd appreciate more context about your auth middleware implementation.
Most commonly MCPs have implemented auth with workers-oauth-provider through ctx.props (which is populated by the library). Docs that cover some of this can be found here
| method: "POST" | ||
| }); | ||
|
|
||
| // Spy on the transport to capture the extraInfo |
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.
Doesn't seem like this is testing anything new here. What am I missing?
| // Only custom headers like x-custom-header should be preserved in requestInfo.headers | ||
| const sseText = await readSSEEvent(response); | ||
| const result = parseSSEData(sseText); | ||
| expectValidToolsList(result); |
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.
On a closer look it seems like the 3 tests are doing the same. They are either adding custom headers to the request or setting an auth property.
Then the test checks that the request was successful and that the tool list is correct but there are no assertions relevant to either the custom headers or the auth field.
One thing that can be done is update the MCP behind the worker to do/return differently in these scenarios.
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.
I decided to create a tool in the workers.ts test utility that actually returns the requestInfo it receives to make it more thorough of a test.
packages/agents/src/mcp/index.ts
Outdated
| } | ||
|
|
||
| // Ensure transport is initialized before processing messages | ||
| if (!this._transport) { |
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.
This should not be needed, onStart is called every time a request is routed and the DO is initialized. Were you facing issues with this._transport not being initialized?
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.
Sorry, I did this just to make sure it was properly initialized. I will remove this :)
packages/agents/src/mcp/index.ts
Outdated
| // Remove internal headers that are not part of the original request | ||
| delete headers["content-type"]; |
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.
These headers are part of the original request, they should not be removed
packages/agents/src/mcp/index.ts
Outdated
| return new McpSSETransport(() => this.getWebSocket()); | ||
| return new McpSSETransport( | ||
| () => this.getWebSocket(), | ||
| this.getSessionId() |
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.
In the StreamableHTTP we use the pattern here to get this, so we can avoid drilling fields through the constructor.
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.
Sounds good. Will do 🫡
Originally I was worried about breaking existing uses of this class, but it seems to be internal only and limited to this part of the codebase.
packages/agents/src/mcp/utils.ts
Outdated
| const error = await agent.onSSEMcpMessage(sessionId, messageBody); | ||
| const error = await agent.onSSEMcpMessage(sessionId, messageBody, { | ||
| headers: Object.fromEntries(request.headers.entries()), | ||
| auth: (request as RequestWithAuth).auth |
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.
Hmm this normally runs inside your Worker fetch handler, how did the request get the auth property?
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.
You're right, for the purposes of the Cloudflare users we're aiding, we actually only needed the headers (and not auth explicitly) so our tests weren't covering the auth header. I added it in with the hopes of meeting parity with StreamableHTTP but I decided to instead reduce the scope of this PR to just include header and session management information.
|
@deathbyknowledge I went ahead and updated the tests, reverted some of the changes I made to include auth information, and refactored the SSE constructor to use an |
deathbyknowledge
left a comment
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.
Looks good! There are a few nits and one type error breaking the CI but this should be ready to merge after those are addressed 👍
…construction in onSSEMcpMessage
a910d78 to
65c36f5
Compare
|
@deathbyknowledge refactored to use the |
|
Ah looks like the TS issue still exists... fixing now! |
Overview
This is a PR to fix issue #660. The SSE transport in MCP was missing critical request context that StreamableHTTP transport already provided. When using SSE, MCP servers couldn't access:
This created a feature gap between the two transports, limiting SSE-based implementations. Even though SSE is technically deprecated, our analysis of major companies with MCP servers shows that many are still dependent on SSE. To disable SSE support for their users would require all of them to update their MCP JSON configs (which is quite a hassle).
This PR brings full feature parity to the SSE transport in the following ways:
Key Changes
Testing
I added a few tests to the existing SSE based test file. The build passes successfully and the changes have been validated to ensure: