Skip to content

Commit 0736543

Browse files
aron-muonclaude
andcommitted
Fix transparent proxy for remote MCP servers behind redirects
Three issues prevented MCPRemoteProxy from connecting to third-party upstream MCP servers that use HTTP redirects: 1. X-Forwarded-Host leaked the proxy's hostname to the upstream. The upstream used it to construct 307 redirect URLs pointing back to the proxy, creating a redirect loop. Fix: skip SetXForwarded() for remote upstreams (isRemote == true). 2. Go's http.Transport.RoundTrip does not follow redirects, but httputil.ReverseProxy uses Transport directly. Upstream 307/308 redirects (e.g. HTTPS→HTTP scheme changes, path canonicalization) were returned to the MCP client which cannot follow them through the proxy. Fix: add forwardFollowingRedirects that transparently follows up to 10 redirects, preserving method and body for 307/308 (RFC 7538). 3. When disableUpstreamTokenInjection is true, the client's ToolHive JWT was still forwarded to the upstream in the Authorization header. Fix: add strip-auth middleware that removes the Authorization header before forwarding. Also adds debug logging for outbound request headers and upstream response status codes to aid diagnosis of remote proxy issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 90b3a2a commit 0736543

3 files changed

Lines changed: 101 additions & 24 deletions

File tree

pkg/runner/middleware.go

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package runner
55

66
import (
77
"fmt"
8+
"net/http"
89

910
"github.com/stacklok/toolhive/pkg/audit"
1011
"github.com/stacklok/toolhive/pkg/auth"
@@ -45,6 +46,7 @@ func GetSupportedMiddlewareFactories() map[string]types.MiddlewareFactory {
4546
headerfwd.HeaderForwardMiddlewareName: headerfwd.CreateMiddleware,
4647
validating.MiddlewareType: validating.CreateMiddleware,
4748
mutating.MiddlewareType: mutating.CreateMiddleware,
49+
stripAuthMiddlewareType: createStripAuthMiddleware,
4850
}
4951
}
5052

@@ -342,9 +344,10 @@ func addUpstreamSwapMiddleware(
342344
return middlewares, nil
343345
}
344346

345-
// Skip upstream token injection if explicitly disabled
347+
// When upstream token injection is disabled, strip the Authorization header
348+
// so the client's ToolHive JWT doesn't leak to the upstream server.
346349
if config.EmbeddedAuthServerConfig.DisableUpstreamTokenInjection {
347-
return middlewares, nil
350+
return addAuthHeaderStripMiddleware(middlewares)
348351
}
349352

350353
// Use provided config or defaults
@@ -402,6 +405,45 @@ func injectUpstreamProviderIfNeeded(
402405
return cedar.InjectUpstreamProvider(authzCfg, providerName)
403406
}
404407

408+
// stripAuthMiddlewareType is the type identifier for the auth header stripping middleware.
409+
const stripAuthMiddlewareType = "strip-auth"
410+
411+
// addAuthHeaderStripMiddleware adds a middleware that removes the Authorization header
412+
// before forwarding to the upstream. This prevents the client's ToolHive JWT from
413+
// leaking to upstream servers that don't expect it.
414+
func addAuthHeaderStripMiddleware(
415+
middlewares []types.MiddlewareConfig,
416+
) ([]types.MiddlewareConfig, error) {
417+
mwConfig, err := types.NewMiddlewareConfig(stripAuthMiddlewareType, struct{}{})
418+
if err != nil {
419+
return nil, fmt.Errorf("failed to create strip-auth middleware config: %w", err)
420+
}
421+
return append(middlewares, *mwConfig), nil
422+
}
423+
424+
// createStripAuthMiddleware is the factory function for the auth header stripping middleware.
425+
func createStripAuthMiddleware(_ *types.MiddlewareConfig, runner types.MiddlewareRunner) error {
426+
mw := &stripAuthMiddleware{}
427+
runner.AddMiddleware(stripAuthMiddlewareType, mw)
428+
return nil
429+
}
430+
431+
// stripAuthMiddleware removes the Authorization header from requests.
432+
type stripAuthMiddleware struct{}
433+
434+
// Handler returns the middleware function.
435+
func (*stripAuthMiddleware) Handler() types.MiddlewareFunction {
436+
return func(next http.Handler) http.Handler {
437+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
438+
r.Header.Del("Authorization")
439+
next.ServeHTTP(w, r)
440+
})
441+
}
442+
}
443+
444+
// Close cleans up resources.
445+
func (*stripAuthMiddleware) Close() error { return nil }
446+
405447
// addAWSStsMiddleware adds AWS STS middleware if configured.
406448
// Returns an error if AWSStsConfig is set but RemoteURL is empty, because
407449
// SigV4 signing is only meaningful for remote MCP servers.

pkg/runner/middleware_test.go

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
271271
name string
272272
config *RunConfig
273273
wantAppended bool
274+
wantType string // expected middleware type when appended
274275
}{
275276
{
276277
name: "nil EmbeddedAuthServerConfig returns input unchanged",
@@ -284,15 +285,17 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
284285
UpstreamSwapConfig: nil,
285286
},
286287
wantAppended: true,
288+
wantType: upstreamswap.MiddlewareType,
287289
},
288290
{
289-
name: "EmbeddedAuthServerConfig with DisableUpstreamTokenInjection skips middleware",
291+
name: "DisableUpstreamTokenInjection adds strip-auth middleware instead",
290292
config: func() *RunConfig {
291293
cfg := createMinimalAuthServerConfig()
292294
cfg.DisableUpstreamTokenInjection = true
293295
return &RunConfig{EmbeddedAuthServerConfig: cfg}
294296
}(),
295-
wantAppended: false,
297+
wantAppended: true,
298+
wantType: stripAuthMiddlewareType,
296299
},
297300
{
298301
name: "EmbeddedAuthServerConfig set with explicit UpstreamSwapConfig uses provided config",
@@ -303,6 +306,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
303306
},
304307
},
305308
wantAppended: true,
309+
wantType: upstreamswap.MiddlewareType,
306310
},
307311
{
308312
name: "EmbeddedAuthServerConfig with custom header strategy config",
@@ -314,6 +318,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
314318
},
315319
},
316320
wantAppended: true,
321+
wantType: upstreamswap.MiddlewareType,
317322
},
318323
}
319324

@@ -333,20 +338,20 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
333338
// Should have one additional entry.
334339
require.Len(t, got, len(initial)+1)
335340
added := got[len(got)-1]
336-
assert.Equal(t, upstreamswap.MiddlewareType, added.Type)
337-
338-
// Verify serialized params contain the expected config.
339-
var params upstreamswap.MiddlewareParams
340-
require.NoError(t, json.Unmarshal(added.Parameters, &params))
341+
assert.Equal(t, tt.wantType, added.Type)
341342

342-
if tt.config.UpstreamSwapConfig != nil {
343-
// Should use the provided config
344-
require.NotNil(t, params.Config)
345-
assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy)
346-
assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName)
347-
} else {
348-
// Should use defaults (empty config is valid)
349-
require.NotNil(t, params.Config)
343+
// For upstreamswap type, verify serialized params
344+
if tt.wantType == upstreamswap.MiddlewareType {
345+
var params upstreamswap.MiddlewareParams
346+
require.NoError(t, json.Unmarshal(added.Parameters, &params))
347+
348+
if tt.config.UpstreamSwapConfig != nil {
349+
require.NotNil(t, params.Config)
350+
assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy)
351+
assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName)
352+
} else {
353+
require.NotNil(t, params.Config)
354+
}
350355
}
351356
})
352357
}
@@ -359,6 +364,7 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
359364
name string
360365
config *RunConfig
361366
wantUpstreamSwap bool
367+
wantStripAuth bool
362368
wantHeaderStrategy string
363369
}{
364370
{
@@ -372,13 +378,14 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
372378
wantUpstreamSwap: false,
373379
},
374380
{
375-
name: "DisableUpstreamTokenInjection omits upstream-swap",
381+
name: "DisableUpstreamTokenInjection adds strip-auth instead of upstream-swap",
376382
config: func() *RunConfig {
377383
cfg := createMinimalAuthServerConfig()
378384
cfg.DisableUpstreamTokenInjection = true
379385
return &RunConfig{EmbeddedAuthServerConfig: cfg}
380386
}(),
381387
wantUpstreamSwap: false,
388+
wantStripAuth: true,
382389
},
383390
{
384391
name: "explicit UpstreamSwapConfig is used",
@@ -400,20 +407,25 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
400407
err := PopulateMiddlewareConfigs(tt.config)
401408
require.NoError(t, err)
402409

403-
var found bool
410+
var foundSwap bool
411+
var foundStrip bool
404412
var foundConfig *types.MiddlewareConfig
405413
for i, mw := range tt.config.MiddlewareConfigs {
406414
if mw.Type == upstreamswap.MiddlewareType {
407-
found = true
415+
foundSwap = true
408416
foundConfig = &tt.config.MiddlewareConfigs[i]
409-
break
417+
}
418+
if mw.Type == stripAuthMiddlewareType {
419+
foundStrip = true
410420
}
411421
}
412-
assert.Equal(t, tt.wantUpstreamSwap, found,
422+
assert.Equal(t, tt.wantUpstreamSwap, foundSwap,
413423
"upstream-swap middleware presence mismatch")
424+
assert.Equal(t, tt.wantStripAuth, foundStrip,
425+
"strip-auth middleware presence mismatch")
414426

415427
// Verify config values if we expect the middleware and have specific expectations
416-
if found && tt.wantHeaderStrategy != "" {
428+
if foundSwap && tt.wantHeaderStrategy != "" {
417429
var params upstreamswap.MiddlewareParams
418430
require.NoError(t, json.Unmarshal(foundConfig.Parameters, &params))
419431
require.NotNil(t, params.Config)

pkg/transport/proxy/transparent/transparent_proxy.go

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -522,6 +522,14 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error)
522522
req.Host = req.URL.Host
523523
}
524524

525+
slog.Debug("outbound request to upstream",
526+
"method", req.Method,
527+
"url", req.URL.String(),
528+
"host", req.Host,
529+
"accept", req.Header.Get("Accept"),
530+
"content_type", req.Header.Get("Content-Type"),
531+
)
532+
525533
reqBody := readRequestBody(req)
526534

527535
// thv proxy does not provide the transport type, so we need to detect it from the request
@@ -611,6 +619,13 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error)
611619
return nil, err
612620
}
613621

622+
slog.Debug("upstream response received",
623+
"status", resp.StatusCode,
624+
"url", req.URL.String(),
625+
"content_type", resp.Header.Get("Content-Type"),
626+
"mcp_session_id", resp.Header.Get("Mcp-Session-Id"),
627+
)
628+
614629
// Check for 401 Unauthorized response (bearer token authentication failure)
615630
if resp.StatusCode == http.StatusUnauthorized {
616631
//nolint:gosec // G706: logging target URI from config
@@ -1006,7 +1021,15 @@ func (p *TransparentProxy) Start(ctx context.Context) error {
10061021
FlushInterval: -1,
10071022
Rewrite: func(pr *httputil.ProxyRequest) {
10081023
pr.SetURL(targetURL)
1009-
pr.SetXForwarded()
1024+
1025+
// Only set X-Forwarded-* headers for local backends.
1026+
// For remote upstreams, these headers leak the proxy's hostname
1027+
// (X-Forwarded-Host) to third-party servers, which can cause
1028+
// 307 redirect loops when the upstream uses that header to
1029+
// construct redirect URLs pointing back to the proxy.
1030+
if !p.isRemote {
1031+
pr.SetXForwarded()
1032+
}
10101033

10111034
// Route to the originating backend pod when session metadata contains backend_url.
10121035
// Falls back to static targetURL when the session doesn't exist or has no backend_url.

0 commit comments

Comments
 (0)