-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Support for http request injection propagation to tools #816
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
src/mcp/server/sse.py
Outdated
request_context: RequestData = { | ||
"headers": dict(request.headers), | ||
"method": request.method, | ||
"url": str(request.url), | ||
"client": request.client, | ||
"path_params": request.path_params, | ||
"query_params": dict(request.query_params), | ||
} |
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.
Can we instead of passing an open dict, just pass the request object? This gives the full power to the consumer. They can still isinstance
check to see what specific object they are getting. This way we are a bit more generic than specifiying an open dict.
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.
My only concern with that is that we are putting starlette.requests into the core and types. Not sure I'm super happy about that, but equally we use starlette everywhere in the SDK
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 should pass the request.scope
, which is a dictionary, and contains all the connection information the user will ever need.
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.
The latest changes use request
, not the request.scope
I mentioned tho 👀
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 saw request.scope
only after pushed the update... will update in a few hours
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.
@Kludex would that work for any transport? the problem with the general RequestData is that it's:
- Has no clear structure.
- Might de-facto depend on HTTP specifics
I was wondering if we can make this a generic and actually pass the origianl request objects. This could be in gRPC context something different than Websocket or HTTP.
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.
Honestly none of the options are great here. I'd love to use generics to make this properly typed, but that would break backwards compatibility for everyone using RequestContext[SessionT, LifespanContextT]
- they'd get errors about missing type arguments.
So for now, i'd suggest to go with request.scope as a dict. It's not perfect but:
- No breaking changes
- No framework dependencies in core types
- Gets us all the request data we need
For WebSocket, it'll work the same way - we can pass the WebSocket's ASGI scope which contains initial handshake headers etc
The scope structure is consistent across HTTP and WebSocket per the ASGI spec, so tools can handle both transports uniformly.
Not ideal, but it gets the job done without breaking existing code. I'd love to revisit this in sdk v2 though
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.
Honestly none of the options are great here. I'd love to use generics to make this properly typed, but that would break backwards compatibility for everyone using RequestContext[SessionT, LifespanContextT] - they'd get errors about missing type arguments.
The way to do for this is to set a default value on the TypeVar
- so people don't get errors.
from typing_extensions import TypeVar
T = TypeVar("T", default=...)`
I don't consider type changes as breaking changes btw.
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.
ah, cool, thanks, didn't realise typing_extensions had TypeVar with default
src/mcp/server/sse.py
Outdated
request_context: RequestData = { | ||
"headers": dict(request.headers), | ||
"method": request.method, | ||
"url": str(request.url), | ||
"client": request.client, | ||
"path_params": request.path_params, | ||
"query_params": dict(request.query_params), | ||
} |
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 should pass the request.scope
, which is a dictionary, and contains all the connection information the user will ever need.
] | ||
|
||
|
||
def run_context_server(server_port: int) -> None: |
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.
Can we avoid running uvicorn in the test suite and just use the httpx.AsyncClient
with the ASGITransport
?
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.
will do as a follow up, as sse tests are using it everywhere
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.
That's probably why I found some flaky issues the other day. We shouldn't have those sleeps around.
When will you plan to release this PR @ihrpr ? |
Is there a guide on how this is supposed to be used? |
Found out how to use it: from mcp.server.fastmcp import Context
def get_authentication_headers(context: Context) -> dict[str, str]:
"""A function that gives the headers for the request.
"""
request_object = context.request_context.request
assert request_object is not None
assert hasattr(request_object, "headers")
headers: dict[str, str] = request_object.headers
return headers |
This PR implements request context propagation in the MCP Python SDK for SSE transport, build on top of PR #380 and addressing concerns in the comments. The implementation captures HTTP request metadata (headers, method, URL, etc.) for each individual request and makes it available to request handlers for authentication and observability purposes.
Commends addressed based on PR #380
Implementation Details
Follow ups