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: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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.
There was a problem hiding this comment.
@matthisholleville could you address this please?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
cc. @ezynda3 |
|
+1 |
|
ping @ezynda3 |
|
Hello @ezynda3, can you review this please. We really need this feature |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/transport/streamable_http.go (1)
755-755: Add missing header argument to sendHTTP callThe
sendHTTPsignature now requires aheader http.Headerparameter. At client/transport/streamable_http.go:755, passnilfor the header:- resp, err := c.sendHTTP(ctx, http.MethodPost, bytes.NewReader(responseBody), "application/json, text/event-stream") + resp, err := c.sendHTTP(ctx, http.MethodPost, bytes.NewReader(responseBody), "application/json, text/event-stream", nil)
🧹 Nitpick comments (4)
client/transport/streamable_http_test.go (2)
126-143: Nit: fix comment typo and tighten handler.
- Typo: “headersas” → “headers as”.
- Optional: explicitly set
Cache-Control: no-storeon responses to avoid caching in proxies during tests.- // Echo back the request headersas the response result + // Echo back the request headers as the response result + w.Header().Set("Cache-Control", "no-store")
237-269: Solid header propagation test; consider also asserting no header mutation.Current transport assigns headers and later sets additional keys; if it doesn’t clone, the caller’s header can be mutated. Add an assertion to catch this.
- request := JSONRPCRequest{ + hdr := http.Header{"X-Test-Header": {"test-header-value"}} + request := JSONRPCRequest{ JSONRPC: "2.0", ID: mcp.NewRequestId(int64(1)), Method: "debug/echo_header", Params: params, - Header: http.Header{"X-Test-Header": {"test-header-value"}}, + Header: hdr, } @@ if hdr, ok := result["X-Test-Header"].([]any); !ok || len(hdr) == 0 || hdr[0] != "test-header-value" { t.Errorf("Expected X-Test-Header to be ['test-header-value'], got %v", result["X-Test-Header"]) } + + // Ensure transport did not mutate caller's header map + if got := hdr.Get("Content-Type"); got != "" { + t.Errorf("caller header mutated; unexpected Content-Type: %q", got) + }client/transport/streamable_http.go (2)
372-385: Use errors.Is for OAuth sentinel, not string compare.Avoid brittle string comparison.
- authHeader, err := c.oauthHandler.GetAuthorizationHeader(ctx) + authHeader, err := c.oauthHandler.GetAuthorizationHeader(ctx) if err != nil { - if err.Error() == "no valid token available, authorization required" { + if errors.Is(err, ErrOAuthAuthorizationRequired) { return nil, &OAuthAuthorizationRequiredError{ Handler: c.oauthHandler, } } return nil, fmt.Errorf("failed to get authorization header: %w", err) }
399-403: 404 session-terminated handling conflicts with initialize path.
sendHTTPturns 404 intoErrSessionTerminatedand returns nil resp, butSendRequest’s initialize special‑case expects to check status/body — unreachable with nil resp. Consider moving 404 handling toSendRequest(after you haveresp), clearing the session and returningErrSessionTerminatedfor non‑initialize, while for initialize let normal status handling proceed.Also applies to: 261-270
📜 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(4 hunks)client/transport/streamable_http_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/transport/streamable_http_test.go (2)
client/transport/interface.go (1)
JSONRPCRequest(58-64)mcp/types.go (3)
JSONRPCRequest(327-332)NewRequestId(262-264)Params(180-180)
client/client.go (1)
mcp/types.go (4)
Params(180-180)UnsubscribeRequest(658-662)CompleteRequest(1088-1092)CompleteResult(1105-1117)
🪛 GitHub Actions: golangci-lint
client/transport/streamable_http.go
[error] 755-755: golangci-lint run failed: not enough arguments in call to c.sendHTTP at client/transport/streamable_http.go:755:79
🪛 GitHub Actions: go
client/transport/streamable_http.go
[error] 755-755: not enough arguments in call to c.sendHTTP
🔇 Additional comments (6)
client/transport/interface.go (1)
6-6: JSONRPCRequest gains per-request headers — good addition.Using
http.Headerwithjson:"-"is correct and keeps it out of the payload. Ensure transport clones and merges (not assigns) to avoid mutating caller maps and to preserve defaults.Also applies to: 63-63
client/transport/streamable_http_test.go (1)
73-73: Non-breaking test enhancement.Including
headersindebug/echoresponses is fine and doesn’t affect existing parsing ofresult.client/client.go (2)
145-151: API updated to thread per-request headers — looks good.Private method signature change keeps surface stable; passing
http.Headerhere is the right choke point.
204-205: Approve code changes: headers correctly forwarded.
AllsendRequestcalls consistently passrequest.Headerornilwhere appropriate.client/transport/streamable_http.go (2)
548-549: Callers pass nil header where per-request headers aren’t applicable.Good: preserves existing behavior for notifications and GET listen loop.
Also applies to: 640-641
350-354: Do not assign req.Header directly — clone and merge to avoid mutating caller and preserve defaults.Assigning
req.Header = headeraliases the caller’s map and you then write into it (e.g., Content-Type), causing data races if reused concurrently and unexpected mutation. Merge or clone first. Also preserve any default headers NewRequestWithContext sets.- // request headers - if header != nil { - req.Header = header - } + // Start from existing headers; clone and merge custom headers if provided. + if header != nil { + // Clone to avoid mutating caller's map + for k, vs := range header { + for _, v := range vs { + req.Header.Add(k, v) + } + } + }Optional (recommended) to make per-request headers highest precedence: set defaults and transport/global headers first, then overlay
headerlast.
|
@matthisholleville looks like tests are failing... |
Enhanced the flexibility of HTTP requests in the streamable HTTP client by allowing additional headers to be specified. This change aims to support more diverse server requirements and improve the adaptability of our client transport layer. Signed-off-by: Matthis Holleville <[email protected]>
|
@ezynda3 fixed |
|
@matthisholleville still failing... |
|
@ezynda3 sorry, it's good now |
|
ping @ezynda3 |
|
Can we merge @ezynda3 ? |
|
@matthisholleville can you address the coderabbit comment first please? |
- Enhanced OAuth error detection by using `errors.Is` for more reliable error handling. - Corrected a typo in a comment and improved code readability in tests by using a variable for headers. Signed-off-by: Matthis Holleville <[email protected]>
|
@ezynda3 done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/streamable_http_test.go (1)
237-270: Test correctly validates custom header propagation.The test properly verifies that custom headers set via
JSONRPCRequest.Headerare propagated to the HTTP request. The assertion logic correctly handles the JSON marshaling behavior wherehttp.Headervalues ([]string) become []any when unmarshaled.Optional: Consider renaming the type-asserted variable on line 267 to avoid shadowing the outer
hdr:- if hdr, ok := result["X-Test-Header"].([]any); !ok || len(hdr) == 0 || hdr[0] != "test-header-value" { - t.Errorf("Expected X-Test-Header to be ['test-header-value'], got %v", result["X-Test-Header"]) + if headerValues, ok := result["X-Test-Header"].([]any); !ok || len(headerValues) == 0 || headerValues[0] != "test-header-value" { + t.Errorf("Expected X-Test-Header to be ['test-header-value'], got %v", result["X-Test-Header"]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/transport/streamable_http.go(6 hunks)client/transport/streamable_http_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/streamable_http.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/streamable_http_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/streamable_http_test.go
🧬 Code graph analysis (1)
client/transport/streamable_http_test.go (2)
client/transport/interface.go (1)
JSONRPCRequest(58-64)mcp/types.go (3)
JSONRPCRequest(327-332)NewRequestId(262-264)Params(180-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (2)
client/transport/streamable_http_test.go (2)
73-73: Good test enhancement.Adding the headers field to the debug/echo response enables verification of header propagation in tests without breaking existing assertions.
126-143: LGTM!The new mock endpoint correctly validates the session and echoes request headers as the result, following the established patterns in the test server.
Signed-off-by: Matthis Holleville <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/streamable_http_test.go (1)
237-270: Excellent test coverage for header propagation!The test correctly verifies that custom headers are propagated through the request chain. The implementation properly:
- Constructs an
http.Headerwith a custom header- Passes it via the
JSONRPCRequest.Headerfield- Validates the header appears in the echoed response
The validation logic correctly handles JSON unmarshaling of header values (string arrays become
[]any).Optional enhancement: Consider adding an assertion that verifies custom headers don't accidentally override system headers (e.g.,
Content-Type,Mcp-Session-Id). For example:// Verify system headers are still present if contentType, ok := result["Content-Type"].([]any); !ok || len(contentType) == 0 { t.Errorf("Expected Content-Type header to be preserved") }This would provide additional confidence in the header merging behavior, though the current test already validates the primary requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/streamable_http_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/streamable_http_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/streamable_http_test.go
🧬 Code graph analysis (1)
client/transport/streamable_http_test.go (2)
client/transport/interface.go (1)
JSONRPCRequest(58-64)mcp/types.go (3)
JSONRPCRequest(327-332)NewRequestId(262-264)Params(180-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (2)
client/transport/streamable_http_test.go (2)
73-73: Nice addition for debugging!Adding the headers field to the response provides visibility into what headers the mock server received, which is helpful for debugging and verification.
126-143: LGTM! Well-structured test endpoint.The new
debug/echo_headerendpoint correctly validates the session and echoes request headers back to the client. The implementation follows the established pattern of other mock endpoints and includes proper error handling.
To maintain consistency and ensure the integrity of HTTP headers during tests, system headers like Content-Type are now verified to be preserved. This change enhances the reliability of our testing framework by ensuring essential headers are not inadvertently removed or altered during the testing process. Signed-off-by: Matthis Holleville <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/transport/streamable_http_test.go (2)
237-275: LGTM! Test correctly verifies header propagation.The test properly verifies that custom headers are sent and echoed back, and that system headers remain intact. The logic is sound and follows existing test patterns in this file.
Recommended: Consider adopting testify for consistency with coding guidelines.
The coding guidelines specify using
testify/assertandtestify/require, but the entire test file uses the standardtestingpackage. While the current approach is functional and consistent with existing tests in this file, migrating to testify would improve alignment with project standards and provide clearer assertion messages. Given the scope of such a change, this could be addressed in a follow-up refactor.Optional: More explicit type assertions for better debugging.
Lines 267-268 and 272-273 use compact type assertions. Splitting these into separate checks could make debugging easier:
headerValues, ok := result["X-Test-Header"].([]any) if !ok { t.Errorf("Expected X-Test-Header to be an array, got %T", result["X-Test-Header"]) } if len(headerValues) == 0 { t.Errorf("Expected X-Test-Header to have at least one value") } else if headerValues[0] != "test-header-value" { t.Errorf("Expected X-Test-Header[0] to be 'test-header-value', got %v", headerValues[0]) }However, the current compact form is also acceptable and matches the style of other tests in the file.
73-73: Remove unusedheadersfield fromdebug/echoendpoint response.The
headersfield added on line 73 is never accessed or validated by any test. TheTestSendRequesttest unmarshals the response into a struct without aHeadersfield, and no other test references it. The separateSendRequestWithHeadertest uses the dedicateddebug/echo_headerendpoint. Remove this unused field to keep the mock server response clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/streamable_http_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
client/transport/streamable_http_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
client/transport/streamable_http_test.go
🧬 Code graph analysis (1)
client/transport/streamable_http_test.go (2)
client/transport/interface.go (1)
JSONRPCRequest(58-64)mcp/types.go (3)
JSONRPCRequest(327-332)NewRequestId(262-264)Params(180-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (1)
client/transport/streamable_http_test.go (1)
126-143: LGTM!The new
debug/echo_headerendpoint correctly validates the session and echoes request headers. The implementation is consistent with other debug endpoints in the mock server.Minor observation: The endpoint name uses singular "header" but echoes all headers (plural) - a tiny naming inconsistency, though not a functional issue.
3 participants
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