Skip to content
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

Request Metadata Context #61

Open
EItanya opened this issue Mar 29, 2025 · 18 comments
Open

Request Metadata Context #61

EItanya opened this issue Mar 29, 2025 · 18 comments
Labels
enhancement New feature or request

Comments

@EItanya
Copy link
Contributor

EItanya commented Mar 29, 2025

First off I just want to say that this library is awesome, and I'm really glad you created it!

Is your feature request related to a problem? Please describe.

I am developing a server using this library in which I want to perform certain logic based on the identity of the client. For example, the client is using sse or the new streamable HTTP protocol, and there is a Bearer token present in the request which I need when performing the logic in the server.

The function call_tool for example currently has the following signature:

	async fn call_tool(
		&self,
		request: CallToolRequestParam,
		context: RequestContext<RoleServer>,
	) -> std::result::Result<CallToolResult, McpError> {

The context has information about the client, but it's limited to exactly what's in the spec. It would be very useful if a consumer of this library could also include some generic metadata struct to pass information from the source of the request into the handlers.

Describe the solution you'd like

The metadata source could take many forms:

  1. It could be a trait which the consumer must implement
  2. It could be some sort of generic HashMap<String, serde_json::Value> to allow for generic json values.

I haven't dug too deep into the exact solution, but I think this would have to live on the sink/stream layer, for example instead of a tokio::sync::mpsc::Sender<ClientJsonRpcMessage> it might need to be tokio::sync::mpsc::Sender<Option<Metadata>, ClientJsonRpcMessage> or something like that. Thereby allowing the user to control it.

Describe alternatives you've considered

Another alternative I considered is attempting to add a generic metadata mechanism to the upstream protocol. However I don't think that's actually necessary. All of the protocols already have ways of passing metadata back and forth, so all that's needed is a way within the library itself to access it.

Additional context
Add any other context or screenshots about the feature request here.

@EItanya EItanya added the enhancement New feature or request label Mar 29, 2025
@4t145
Copy link
Collaborator

4t145 commented Mar 29, 2025

I am actually working on this in the latest revision in #55 and #56, it could be a trait WithMeta. You can review my code and give us your suggestions!

@4t145
Copy link
Collaborator

4t145 commented Mar 29, 2025

@EItanya
Copy link
Contributor Author

EItanya commented Mar 29, 2025

Awesome, I'll take a look now!

@EItanya
Copy link
Contributor Author

EItanya commented Mar 29, 2025

Ok, my basic response is that I think the metadata should be always available on the RequestContext rather than on in the params. In the current structure, _meta has been added to the args, but given that it's optional, I think it could be cleaner for it to be a field on the context, for example:

/// Request execution context
#[derive(Debug, Clone)]
pub struct RequestContext<R: ServiceRole> {
    /// this token will be cancelled when the [`CancelledNotification`] is received.
    pub ct: CancellationToken,
    pub id: RequestId,
    /// An interface to fetch the remote client or server
    pub peer: Peer<R>,
    /// Optional request metadata
    pub meta: Option<RequestMeta>
}

Let me know if that doesn't make sense

@4t145
Copy link
Collaborator

4t145 commented Mar 29, 2025

@EItanya I do prefer an interface like this but it would be a challange for serialization and deserialization since the _meta field is a part of request param. Maybe we should implement Serialize and Deserialize for Request in model to extract metadata in the deserialization period?

@EItanya
Copy link
Contributor Author

EItanya commented Mar 29, 2025

since the _meta field is a part of request param

Is this a requirement, or a current implementation detail. Is meta part of the spec, I haven't seen it.

Leaving aside the specific implementation for one second, typically metadata features like this are implemented alongside the protocol, rather than inside of it. For example, in this case the _meta object should be an object which may optionally be set/accessed by a user of the library, but does not impact the actual message format on the wire or within the library. Message format in this case being JsonRPC. That separation allows the metadata to be dynamic, rather than needing to conform to the spec.

From reading the code, it seems like currently the metadata is being encoded into the JsonRPC params, which ties the metadata very strongly to the protocol, rather than allowing it to exist separately.

I'll give you a concrete example. Let's say I was using sse like the example here, but I wanted to add JWT validation into the server. In order to use those claims in the server, or forward them along to other services, I would need a way to pass that data into the chain regardless of the message type. I do think it's worth working with the upstream community to add Metadata to the protocol, but I also think that many take a while. In the meantime I recommend something like the following.

    tracing::debug!(session_id, ?message, "new client message");
    let tx = {
        let rg = app.txs.read().await;
        rg.get(session_id.as_str())
            .ok_or(StatusCode::NOT_FOUND)?
            .clone()
    };
    let meta = Meta::from_claims(claims);
    if tx.send((message, meta)).await.is_err() {
        tracing::error!("send message error");
        return Err(StatusCode::GONE);
    }

The code above is of course just for example, but notice the message metadata pairing. This could of course be an optional channel type for advanced use-cases.

@4t145
Copy link
Collaborator

4t145 commented Mar 30, 2025

  1. It's a part of spec: https://github.com/modelcontextprotocol/specification/blob/3ba3181c7779da74b24f0c083eb7055b6fc9d928/schema/2025-03-26/schema.ts#L37-L67
  2. Now I understand that you are talking about another meta which could be extracted from HTTP headers or other context, my suggestion is to import something like the Extension in http crate, and then we can put the whole header map in it.

@EItanya
Copy link
Contributor Author

EItanya commented Mar 30, 2025

Ohhh, interesting, I didn't realize there was meta as part of the spec, that's good to know. In that case I agree with what you said: Maybe we should implement Serialize and Deserialize for Request in model to extract metadata in the deserialization period?. I think we should implement is for Request

Can you expand on this:

Now I understand that you are talking about another meta which could be extracted from HTTP headers or other context, my suggestion is to import something like the Extension in http crate, and then we can put the whole header map in it.

@4t145
Copy link
Collaborator

4t145 commented Mar 30, 2025

@EItanya Are you familiar with Rust's http library? Extension in http is a container can carry anything in the context https://docs.rs/http/latest/http/struct.Extensions.html

@EItanya
Copy link
Contributor Author

EItanya commented Mar 31, 2025

So I looked into it a bit, and I'm not quite sure how this would help me. I have an Axum SSE server very similar to the example one, and I need to send metadata from the post request into the server handler. I could parse the message and perform some logic before sending it along, but that feels like a waste, so I'd rather perform all necessary checks/logic in the handler. Specifically I want to perform RBAC based on the tool_name using the claims extracted from the bearer token.

@4t145
Copy link
Collaborator

4t145 commented Mar 31, 2025

@EItanya I mean, if we have some thing like Extensions in message, you can inject the whole http header map or anything else into received messages. And then you can carry any token you want to mcp server handlers.

@EItanya
Copy link
Contributor Author

EItanya commented Mar 31, 2025

Ahh yea, that would be perfect

@4t145
Copy link
Collaborator

4t145 commented Apr 1, 2025

Update, in #56 you can add custom extensions for request and notification now.

@pimeys
Copy link

pimeys commented Apr 1, 2025

I would also really appreciate to get the request headers somehow in the service calls. We have a need to forward the headers as-is to the underlying service from the mcp proxy. Is this somehow already possible? I was reading the code, but it's not super easy to inject the headers from the axum routes to the service endpoints currently...

@4t145
Copy link
Collaborator

4t145 commented Apr 2, 2025

@pimeys soon we will support it

@EItanya
Copy link
Contributor Author

EItanya commented Apr 2, 2025

@4t145, the extensions changes in your PR are separate from the protocol update. Do you think we can pull it into its own PR? I can help if you'd like

@4t145
Copy link
Collaborator

4t145 commented Apr 2, 2025

@EItanya We can remove the streamable http part and merge it. And implement streamable http transport later.

@EItanya
Copy link
Contributor Author

EItanya commented Apr 2, 2025

That would be perfect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants