-
Notifications
You must be signed in to change notification settings - Fork 742
fix: STDIO and SSE transports need MCP_FULL_PROXY_ADDRESS #708
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?
fix: STDIO and SSE transports need MCP_FULL_PROXY_ADDRESS #708
Conversation
Hi @richardkmichael I was testing this out and saw that there was a build error: https://github.com/modelcontextprotocol/inspector/actions/runs/17087872859/job/48455657789?pr=708 For future branches, this shouldn't be an issue since we added a precommit hook to format using prettier. |
I believe the merge has caused this, but I will look closer. My local branch passes, and the commits I pushed also pass. Prettier fails on the CI jobs launched following the merge commits. https://github.com/modelcontextprotocol/inspector/pull/708/commits I have also noticed that the prettier which runs in the build action is not the same version as the project ( What do you think? I was intending to send a PR to adjust the CI build action. |
Looks like it was bad resolution of the merge conflict. Is a tool doing this integration?
Probably easiest if I rebase to main and sort out the conflict. |
@olaservo I was thinking to rebase, if you don't need this ref. That way, there's no "patch" commit, resolving the merge error. |
The STDIO and SSE transports use SSE between the client and the proxy server. SSE requires the client to initiate the MCP conversation (event-stream) at the endpoint provided by the server. The endpoint provided by the server must obey where the client sees the server, rather than being assumed to be server-root `/message`. The Streamable HTTP endpoint does not need the full proxy address because the response is an event-stream directly, and does not use SSE.
Tests verify that proxyFullAddress query parameter is sent for both STDIO and SSE transports, but not Streamable HTTP, when MCP_PROXY_FULL_ADDRESS is configured. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
b463ab2
to
61db791
Compare
Motivation and Context
#630
The MCP client needs to know the full path to the proxy server when opening the SSE transport for STDIO and SSE endpoints.
The client knows where it expects to see the server, so pass this to the server, to use when building the endpoint.
How Has This Been Tested?
Start nginx as proxy for the backed. Using an nginx proxy:
nginx -c inspector.conf
(below).Open the Inspector, set the Inspector Proxy Address to a URL through nginx:
http://localhost:9000/mcp/inspector/backend
.Breaking Changes
No
Types of changes
Checklist
Additional context
There are other ways to fix this, but my hunch is treating the client as authoritative will be most flexible different deployment scenarios, and also prevent a server-side / client-side configuration mismatch.
Also, I'd add a server test for this, but there are no server tests at the moment. Are server tests desired?
Issue #437 is related, for which there is a fix in a fork, not yet submitted as a PR. It looks like the right approach, appending to the query params from the env. This would close out the current proxy-related issues.