Skip to content

Commit c6850a8

Browse files
committed
Tighten HeaderForward test coverage and docs
Fix the inverted chain-ordering comments on mcp_session.go's transport chain assembly and on headerForwardRoundTripper.RoundTrip. The outermost wrapper runs FIRST on the outbound request, so header-forward injects onto a request without auth/identity headers and the inner stages then overwrite with Set(). The previous wording claimed the opposite. The production behaviour is unchanged; only the doc text moved. Replace headerCapturingBackend with the shared fakeBackend by adding headersByMethod / headersFor and a tools/call handler. This removes a near-duplicate fake server in the session backend tests. Extend the header-forward test to cover overlap precedence (an inner auth stage's value wins on the wire when both sides set the same name) and AddHeadersFromSecret end-to-end via t.Setenv against the env-backed secrets provider. Add a restricted-name rejection case to prove the resolveHeaderForward guard is wired into the session-side connector. Trim diff-narrating comments from the test file: drop the past-tense narrative about which PR fixed what and describe the assertions instead. The issue reference (#5289) is kept on the test docstring.
1 parent 5ead2ff commit c6850a8

4 files changed

Lines changed: 243 additions & 147 deletions

File tree

pkg/vmcp/headerforward/transport.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,13 @@ type headerForwardRoundTripper struct {
3535
}
3636

3737
// RoundTrip injects the pre-resolved headers onto a clone of the request and
38-
// delegates to the wrapped transport. Headers already present on the request
39-
// are left untouched so inner transports (auth, identity, trace) can always
40-
// override user-supplied values for the same name.
38+
// delegates to the wrapped transport. Names already present on the inbound
39+
// request are skipped — this preserves any header the *caller* set on the
40+
// request before invoking the round-tripper chain. Inner (closer to the wire)
41+
// transports run AFTER this one and use Set() unconditionally, so on any
42+
// overlapping name those stages still win on the wire. Restricted names are
43+
// blocked at resolve time, so user-supplied config cannot reach this point
44+
// for Host, hop-by-hop, or X-Forwarded-* anyway.
4145
func (h *headerForwardRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
4246
if len(h.headers) == 0 {
4347
return h.base.RoundTrip(req)

pkg/vmcp/session/internal/backend/mcp_session.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -273,10 +273,15 @@ func createMCPClient(
273273

274274
slog.Debug("Applied authentication strategy", "strategy", strategy.Name(), "backendID", target.WorkloadID)
275275

276-
// Build shared transport chain: auth → identity propagation → header forward.
277-
// HeaderForward is the outermost stage so inner stages (auth, identity) win
278-
// on any overlapping header name — matching the ordering in
279-
// headerForwardRoundTripper.RoundTrip, which skips names already set.
276+
// Build shared transport chain (innermost first → outermost):
277+
// http.DefaultTransport → authRoundTripper → identityRoundTripper → headerForwardRoundTripper
278+
// On an outbound request, the outermost stage runs first: header-forward
279+
// injects its headers onto a request that does not yet carry auth/identity
280+
// headers, then inner stages run and call Set() unconditionally so any
281+
// overlapping name they care about (Authorization, identity headers) wins on
282+
// the wire. Restricted header names (Host, hop-by-hop, X-Forwarded-*) are
283+
// rejected at resolve time by resolveHeaderForward, so user-supplied
284+
// HeaderForward cannot inject them in the first place.
280285
// The per-transport sections below may add a size-limiting wrapper on top.
281286
base := http.RoundTripper(http.DefaultTransport)
282287
base = &authRoundTripper{

pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@ type fakeBackend struct {
5454
// resources/list failure.
5555
mu sync.Mutex
5656
methodCalls map[string]int
57+
58+
// headersByMethod records the inbound request headers keyed by JSON-RPC
59+
// method name. Tests asserting transport-chain behavior (e.g. HeaderForward)
60+
// use headersFor(method) to inspect the headers a backend actually saw.
61+
headersByMethod map[string]http.Header
5762
}
5863

5964
type jsonRPCError struct {
@@ -68,6 +73,7 @@ func newFakeBackend(t *testing.T, fb *fakeBackend) string {
6873
t.Helper()
6974
fb.t = t
7075
fb.methodCalls = make(map[string]int)
76+
fb.headersByMethod = make(map[string]http.Header)
7177

7278
mux := http.NewServeMux()
7379
mux.HandleFunc("/mcp", fb.handle)
@@ -83,6 +89,19 @@ func (f *fakeBackend) callCount(method string) int {
8389
return f.methodCalls[method]
8490
}
8591

92+
// headersFor returns a clone of the inbound HTTP headers recorded for the most
93+
// recent JSON-RPC request with the given method, or nil if no such request was
94+
// seen. Cloning under the mutex keeps the caller safe from concurrent writes.
95+
func (f *fakeBackend) headersFor(method string) http.Header {
96+
f.mu.Lock()
97+
defer f.mu.Unlock()
98+
h := f.headersByMethod[method]
99+
if h == nil {
100+
return nil
101+
}
102+
return h.Clone()
103+
}
104+
86105
// handle implements the JSON-RPC subset needed for backend init. The
87106
// streamable-HTTP transport sends POST requests with Accept:
88107
// application/json, text/event-stream — we always reply with
@@ -117,6 +136,7 @@ func (f *fakeBackend) handle(w http.ResponseWriter, r *http.Request) {
117136

118137
f.mu.Lock()
119138
f.methodCalls[msg.Method]++
139+
f.headersByMethod[msg.Method] = r.Header.Clone()
120140
f.mu.Unlock()
121141

122142
// Notifications (no id, e.g. notifications/initialized) get an empty 202.
@@ -148,6 +168,16 @@ func (f *fakeBackend) handle(w http.ResponseWriter, r *http.Request) {
148168
return
149169
}
150170
f.writeResult(w, msg.ID, map[string]any{"prompts": f.prompts})
171+
case string(mcp.MethodToolsCall):
172+
// Minimal CallToolResult with a single text content. Tests that exercise
173+
// the post-initialize transport chain (e.g. HeaderForward) need a method
174+
// they can invoke after Initialize completes; tools/call is the cheapest.
175+
f.writeResult(w, msg.ID, map[string]any{
176+
"content": []map[string]any{
177+
{"type": "text", "text": "ok"},
178+
},
179+
"isError": false,
180+
})
151181
default:
152182
f.writeError(w, msg.ID, &jsonRPCError{code: mcp.METHOD_NOT_FOUND, message: "Method not found"})
153183
}

0 commit comments

Comments
 (0)