stacklok / toolhive / 34208465701

71%

Build Type

push

github

Committed by web-flow

Commit Message

Merge commit from fork

MCPPinger and StatelessMCPPinger construct a bare http.Client{Timeout:
timeout}, which follows Go's default redirect policy: any host, up to
10 hops. The pinger's target can be a remote MCP endpoint outside our
control, so a malicious or later-compromised remote can respond with a
3xx pointing the pinger at a loopback, private, or other host-reachable
address, and the client follows it without question -- a blind SSRF
(CWE-918). Remote health-checking is opt-in via
TOOLHIVE_REMOTE_HEALTHCHECKS, so this only reaches deployments that
enabled it, but nothing in the pinger's own code enforces that scoping.

Install networking.SameHostRedirectPolicy() as CheckRedirect on both
clients. It refuses any redirect to a different host and any
HTTPS-to-HTTP downgrade, matching the same-host-only guard the package's
data-plane forwarder (followRedirects, in transparent_proxy.go) already
applies to actual MCP traffic -- the two are meant to stay in sync, per
SameHostRedirectPolicy's own doc comment. The forwarder implements its
own redirect loop instead of using this hook because it must preserve
POST method and body across every redirect code for JSON-RPC
correctness; the pingers have no such requirement (MCPPinger only sends
GET, and StatelessMCPPinger's ping payload is not something a
same-host redirect target needs verbatim), so the standard http.Client
hook is the right fit here.

Add regression coverage for both pinger types: a same-host redirect
must still be followed (the common trailing-slash/canonicalization
case), and a cross-host redirect must fail the ping without the
redirect target ever being contacted -- asserted directly on the target
server, not just on the returned error, since a pinger that returned an
error after connecting would still have completed the SSRF.

The redirect policy's own behavior (cross-host, HTTPS downgrade,
max-chain-length) is already exhaustively unit-tested in isolation at
pkg/networking/http_client_t... (continued)

Coverage Stats

4 of 4 new or added lines in 1 file covered. (100.0%)

7 existing lines in 2 files now uncovered.

79135 of 112785 relevant lines covered (70.16%)

92.35 hits per line