feat: add support for custom HTTP headers in client requests #546
Conversation
This update introduces the ability to include custom HTTP headers in requests sent from the client. This enhancement facilitates more flexible and secure communication with servers by allowing clients to pass additional information in the header of each request, such as authentication tokens or custom metadata. This feature is crucial for integrating with APIs that require specific headers for access control, content negotiation, or tracking purposes. Signed-off-by: Matthis Holleville <[email protected]>
WalkthroughAdds per-request HTTP header propagation through the client and StreamableHTTP transport by threading http.Header from Client.sendRequest into transport.JSONRPCRequest and applying it in HTTP requests. Updates call sites. Extends tests to assert header echoing. Also adds 404 session termination handling and OAuth authorization header retrieval in HTTP transport. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/transport/streamable_http_test.go (1)
133-133: Fix typo in comment.There's a typo in the comment.
- // Echo back the request headersas the response result + // Echo back the request headers as the response resultclient/client.go (1)
134-134: Consider documenting the header parameter.The new
headerparameter insendRequestwould benefit from documentation explaining its purpose and usage, especially since this is a key part of the per-request header feature.Add a comment to the parameter in the function signature:
func (c *Client) sendRequest( ctx context.Context, method string, params any, - header http.Header, + header http.Header, // Custom HTTP headers to include in this request ) (*json.RawMessage, error) {client/transport/streamable_http.go (1)
373-385: OAuth error handling could be more consistent.The OAuth authorization error is checked twice - once as a string comparison and once returning the same error type. This could be simplified for better maintainability.
// Add OAuth authorization if configured if c.oauthHandler != nil { authHeader, err := c.oauthHandler.GetAuthorizationHeader(ctx) if err != nil { - // If we get an authorization error, return a specific error that can be handled by the client - if err.Error() == "no valid token available, authorization required" { + // If we get an authorization error, check if it's the specific OAuth error + if errors.Is(err, ErrOAuthAuthorizationRequired) { return nil, &OAuthAuthorizationRequiredError{ Handler: c.oauthHandler, } } return nil, fmt.Errorf("failed to get authorization header: %w", err) } req.Header.Set("Authorization", authHeader) }This assumes that
GetAuthorizationHeaderwould returnErrOAuthAuthorizationRequireddirectly instead of a string-based error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/client.go(11 hunks)client/transport/interface.go(2 hunks)client/transport/streamable_http.go(9 hunks)client/transport/streamable_http_test.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/transport/streamable_http_test.go (3)
client/transport/interface.go (1)
JSONRPCRequest(58-64)mcp/types.go (3)
JSONRPCRequest(325-330)NewRequestId(259-261)Params(177-177)testdata/mockstdio_server.go (1)
JSONRPCRequest(13-18)
client/client.go (1)
mcp/types.go (4)
Params(177-177)UnsubscribeRequest(635-639)CompleteRequest(1021-1025)CompleteResult(1038-1050)
🔇 Additional comments (4)
client/transport/interface.go (1)
63-63: LGTM! The Header field addition enables per-request header customization.The addition of the
Headerfield withjson:"-"tag is appropriate as it allows per-request headers to be passed through the transport layer without affecting the JSON-RPC payload serialization.client/transport/streamable_http_test.go (1)
237-269: LGTM! Comprehensive test for per-request header propagation.The test properly validates that custom headers are sent and received by the server, confirming the header propagation feature works as expected.
client/client.go (1)
185-185: LGTM! Consistent header propagation across all client methods.The header parameter is correctly propagated from all public methods through
sendRequestto the transport layer. Methods that don't have headers in their request structures appropriately passnil.Also applies to: 230-230, 311-311, 323-323, 331-331, 375-375, 423-423, 435-435, 443-443, 520-520
client/transport/streamable_http.go (1)
399-403: Good addition of 404 session termination handling.The automatic session cleanup on 404 responses is a useful feature that helps maintain consistency when the server indicates the session no longer exists.
| // request headers | ||
| if header != nil { | ||
| req.Header = header | ||
| } | ||
|
|
There was a problem hiding this comment.
Header assignment may not preserve all default headers.
When a custom header is provided, the code replaces the entire req.Header with the provided header. This could potentially lose important default headers that http.NewRequestWithContext sets up (like Host, User-Agent, etc.).
Consider merging headers instead of replacing:
- // request headers
- if header != nil {
- req.Header = header
- }
-
+ // Merge custom headers if provided
+ if header != nil {
+ for key, values := range header {
+ for _, value := range values {
+ req.Header.Add(key, value)
+ }
+ }
+ }Alternatively, if the intention is to allow complete header control, consider cloning the header first to avoid modifying the original:
- // request headers
- if header != nil {
- req.Header = header
- }
-
+ // Use custom headers if provided
+ if header != nil {
+ req.Header = header.Clone()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // request headers | |
| if header != nil { | |
| req.Header = header | |
| } | |
| // Merge custom headers if provided | |
| if header != nil { | |
| for key, values := range header { | |
| for _, value := range values { | |
| req.Header.Add(key, value) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In client/transport/streamable_http.go around lines 350 to 354, the code
replaces req.Header with the provided header which can discard default headers;
instead merge the provided header into the existing req.Header (iterating keys
and appending values) so you preserve defaults like Host and User-Agent, or if
full control was intended, first clone the provided header and assign the clone
to req.Header to avoid mutating the caller's header; implement one of these two
approaches and ensure you append values rather than overwrite per header key.
|
cc. @ezynda3 |
Description
This update introduces the ability to include custom HTTP headers in requests sent from the client. This enhancement facilitates more flexible and secure communication with servers by allowing clients to pass additional information in the header of each request, such as simple authentication tokens or custom metadata.
Fixes #544
Type of Change
Checklist
Additional Information
Summary by CodeRabbit
New Features
Improvements
Tests