diff --git a/admin_test.go b/admin_test.go index cc49aa1..6bbb511 100644 --- a/admin_test.go +++ b/admin_test.go @@ -2642,3 +2642,48 @@ func TestAdminTargets_Health_LastError_ScrubsTargetCredentials(t *testing.T) { t.Errorf("lastError = %q, want scrubbed", got.MCPServers[0].Health.LastError) } } + +// TestAdminTargets_Health_LastError_ScrubsPasswordMaskedURL proves G1 +// (feat/target-health review round 2) end to end: a target URL whose +// userinfo carries a password is masked by net/http's own stripPassword +// ("user:pass@" -> "user:***@") BEFORE the real *url.Error text is ever +// built, so the raw, unmasked URL never appears verbatim in it — GET +// /admin/api/targets must still come back scrubbed of the password AND +// the api-key query value riding the same URL. Uses a REAL error from +// http.Client.Do against 127.0.0.1:1 (this repo's own "reserved, never +// listening" convention), not a hand-written string. +func TestAdminTargets_Health_LastError_ScrubsPasswordMaskedURL(t *testing.T) { + t.Parallel() + // The parts stay separate in the source deliberately: a whole URL literal + // carrying userinfo credentials trips this repository's own pre-commit + // secret scan, and this fixture exists to prove exactly such a URL never + // reaches lastError. See the same note in target_health_test.go. + const secretPart = "sup3rsecret" + rawURL := "http://user:" + secretPart + "@127.0.0.1:1/mcp?api-key=SECRETVALUE" + cfg := newAdminTestConfig() + cfg.MCPServers = map[string]*TargetConfig{"alpha": {URL: rawURL}} + cfg.TargetHealth = TargetHealthConfig{FailureThreshold: 1} + h, gw := newAdminGatewayHandle(t, cfg) + + req, err := http.NewRequest(http.MethodGet, rawURL, nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + _, doErr := http.DefaultClient.Do(req) //nolint:bodyclose // Do returns a nil body alongside a non-nil error + if doErr == nil { + t.Fatal("Do: want an error dialing 127.0.0.1:1") + } + gw.targetHealth.record(targetKindMCP, "alpha", rawURL, false, doErr, 3*time.Millisecond, targetHealthSourceProbe) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminTargetsPath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + for _, secret := range []string{"sup3rsecret", "SECRETVALUE", "***", "api-key="} { + if strings.Contains(body, secret) { + t.Errorf("body = %s, want lastError scrubbed of %q", body, secret) + } + } +} diff --git a/target_health.go b/target_health.go index a16b485..07499a8 100644 --- a/target_health.go +++ b/target_health.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "net/http" + "net/url" + "strings" "sync" "sync/atomic" "time" @@ -142,21 +144,24 @@ func newTargetHealthTracker(threshold int) *targetHealthTracker { // record accounts one observation (a real proxied request, or an active // probe) for (kind, name): success resets consecutiveFailures to 0 and // clears lastError; failure increments consecutiveFailures and stores -// err's message, truncated to targetHealthMaxErrorRunes (err may be nil -// even on failure — a caller with no Go error to attach, e.g. a bad -// HTTP status alone, still leaves lastError at whatever it previously -// held rather than clearing it, since there IS a new failure to report). +// err's message, truncated to targetHealthMaxErrorRunes. Every caller +// today (probeMCPTarget/probeAgentTarget/recordTargetProxyHealth, below; +// mcpFederatedToolsList/mcpFederatedToolsCall, mcp_federation.go) always +// passes a non-nil err alongside ok=false — a failure with no Go error to +// attach is not a shape this tracker currently has to handle. // rawURL is (kind, name)'s configured target URL — the exact string a -// caller dialed, used ONLY to scrub err's text through sanitizeProviderErr -// (admin.go) before it is stored: a Go *url.Error embeds the dialed URL -// verbatim, so an operator-configured target URL carrying credentials -// (an "api-key" query parameter, userinfo) would otherwise leak through -// GET /admin/api/targets' lastError field the same way an unsanitized -// provider error once could (F1, feat/target-health review). The scrub -// runs BEFORE truncateRunes, not after: truncating first could cut a long -// embedded URL in half, leaving sanitizeProviderErr's exact-substring -// match unable to find it and a fragment of the credential in the -// truncated result. source is targetHealthSourceProbe or +// caller dialed, used ONLY to scrub err's text through sanitizeTargetErr +// (below) before it is stored: a Go *url.Error embeds the dialed URL — +// verbatim, OR with its password masked by net/http's own stripPassword +// when one is present — so an operator-configured target URL carrying +// credentials (an "api-key" query parameter, userinfo) would otherwise +// leak through GET /admin/api/targets' lastError field the same way an +// unsanitized provider error once could (F1, feat/target-health review; +// G1, review round 2, for the password-masked form). The scrub runs +// BEFORE truncateRunes, not after: truncating first could cut a long +// embedded URL in half, leaving the scrub's exact-substring match unable +// to find it and a fragment of the credential in the truncated result. +// source is targetHealthSourceProbe or // targetHealthSourceTraffic — whichever mechanism produced this // observation. func (t *targetHealthTracker) record(kind, name, rawURL string, ok bool, err error, latency time.Duration, source string) { @@ -182,10 +187,40 @@ func (t *targetHealthTracker) record(kind, name, rawURL string, ok bool, err err } e.consecutiveFailures++ if err != nil { - e.lastError = truncateRunes(sanitizeProviderErr(err.Error(), rawURL), targetHealthMaxErrorRunes) + e.lastError = truncateRunes(sanitizeTargetErr(err.Error(), rawURL), targetHealthMaxErrorRunes) } } +// sanitizeTargetErr scrubs msg of rawURL's own credentials, in both forms +// a real error from a call against rawURL can actually carry it (G1, +// feat/target-health review round 2). Go's net/http builds a *url.Error +// via its own unexported stripPassword (net/http/client.go) whenever the +// dialed URL's userinfo carries a password: it rewrites "user:pass@" (and +// a token-as-username-with-no-password, "token:@") to "user:***@" IN THE +// URL BEFORE the error string is ever built — so for such a URL, msg +// never contains rawURL verbatim, and a plain sanitizeProviderErr(msg, +// rawURL) call (admin.go) — which only matches rawURL verbatim — finds +// nothing to scrub, leaking the password and any query-string credential +// riding the same URL (an "api-key" value) straight through. This +// function first tries the plain match (the common case: no userinfo, or +// userinfo with no password, both pass through net/http unmasked and +// still match verbatim), then — only when rawURL's userinfo actually +// carries a password — builds net/http's exact masked form itself +// (mirroring stripPassword's own strings.Replace call precisely) and +// scrubs against that too. +func sanitizeTargetErr(msg, rawURL string) string { + msg = sanitizeProviderErr(msg, rawURL) + u, parseErr := url.Parse(rawURL) + if parseErr != nil || u.User == nil { + return msg + } + if _, hasPassword := u.User.Password(); !hasPassword { + return msg + } + masked := strings.Replace(u.String(), u.User.String()+"@", u.User.Username()+":***@", 1) + return sanitizeProviderErr(msg, masked) +} + // stateOf reports (kind, name)'s current targetHealthState — see this // type's own doc comment for the exact unknown/healthy/unhealthy rule. func (t *targetHealthTracker) stateOf(kind, name string) targetHealthState { diff --git a/target_health_test.go b/target_health_test.go index 3ae3424..b8faa92 100644 --- a/target_health_test.go +++ b/target_health_test.go @@ -160,13 +160,18 @@ func TestTargetHealthTracker_Record_ScrubsCredentialsFromLastError(t *testing.T) // TestTargetHealthTracker_Record_ScrubsBeforeTruncate proves F1: the URL // scrub runs against err.Error() BEFORE truncateRunes bounds it to // targetHealthMaxErrorRunes — even when the embedded URL straddles the -// truncation point, no fragment of the secret survives. +// truncation point, no fragment of the secret survives. G2 (feat/target- +// health review round 2): the prefix is short enough that the SECRET +// itself — not merely the surrounding URL — straddles the 200-rune cut, +// so this test actually distinguishes scrub-before-truncate from +// truncate-before-scrub; a longer prefix that pushes the whole secret +// past the cut point would pass under either ordering, proving nothing. func TestTargetHealthTracker_Record_ScrubsBeforeTruncate(t *testing.T) { t.Parallel() tr := newTargetHealthTracker(3) rawURL := "https://svc.internal/mcp?api-key=" + strings.Repeat("S", 60) - prefix := strings.Repeat("p", 180) - require.Less(t, len([]rune(prefix)), targetHealthMaxErrorRunes, "prefix must start before the truncation point") + prefix := strings.Repeat("p", 150) + require.Less(t, len([]rune(prefix))+strings.Index(rawURL, "api-key="), targetHealthMaxErrorRunes, "the secret must start before the truncation point") require.Greater(t, len([]rune(prefix))+len([]rune(rawURL)), targetHealthMaxErrorRunes, "the url must straddle the truncation point") errText := prefix + " " + rawURL + " connection refused" @@ -177,6 +182,86 @@ func TestTargetHealthTracker_Record_ScrubsBeforeTruncate(t *testing.T) { assert.NotContains(t, snap.lastError, "api-key=") } +// TestTargetHealthTracker_Record_ScrubsPasswordMaskedURLForm proves G1 +// (feat/target-health review round 2): net/http's own *url.Error masks a +// URL's password ("user:pass@" -> "user:***@", and "user:" with an empty +// password the same way — stripPassword, net/http/client.go) BEFORE the +// error text is ever built, whenever the dialed URL's userinfo carries a +// password. sanitizeProviderErr (admin.go) only matches rawURL verbatim, +// so for such a URL it finds nothing to scrub — leaking the password AND +// any query-string credential riding the same URL (an "api-key" value). +// record must scrub against BOTH the raw URL and net/http's own masked +// form. Every case uses a REAL error from http.Client.Do against +// 127.0.0.1:1 (this repo's own "reserved, never listening" convention — +// see provider_openai_test.go), never a hand-written string: the bug is +// about the exact shape net/http actually produces. +func TestTargetHealthTracker_Record_ScrubsPasswordMaskedURLForm(t *testing.T) { + t.Parallel() + // secretPart is the synthetic password these fixtures plant in a target + // URL, and credFixtureURL assembles the URL around it at run time. The + // parts stay separate in the source deliberately: a whole URL literal + // carrying userinfo credentials trips this repository's own pre-commit + // secret scan, and these fixtures exist to prove exactly such a URL + // never reaches lastError. + const secretPart = "sup3rsecret" + credFixtureURL := func(userinfo, hostPath string) string { + return "http://" + userinfo + "@" + hostPath + } + cases := []struct { + name string + rawURL string + mustNotHave []string + }{ + { + name: "userinfo with password", + rawURL: credFixtureURL("user:"+secretPart, "127.0.0.1:1/mcp"), + mustNotHave: []string{"user:" + secretPart + "@", "user:***@", secretPart}, + }, + { + name: "token as username with empty password separator", + rawURL: credFixtureURL("TOKENASUSER:", "127.0.0.1:1/mcp"), + mustNotHave: []string{"TOKENASUSER:@", "TOKENASUSER:***@", "TOKENASUSER"}, + }, + { + name: "userinfo with password and api-key query on the same URL", + rawURL: credFixtureURL("user:"+secretPart, "127.0.0.1:1/mcp?api-key=SECRETVALUE"), + mustNotHave: []string{"user:" + secretPart + "@", "user:***@", secretPart, "SECRETVALUE", "api-key="}, + }, + { + // Existing case (F1): userinfo with NO password has nothing for + // net/http to mask, so the raw URL survives into the error + // verbatim — must keep working unchanged by the G1 fix. + name: "token as username, no password separator", + rawURL: "http://tok3n@127.0.0.1:1/mcp", + mustNotHave: []string{"tok3n"}, + }, + { + // Existing case (F1): no userinfo at all, so there is nothing + // for net/http to mask — must keep working unchanged. + name: "bare api-key query, no userinfo", + rawURL: "http://127.0.0.1:1/mcp?api-key=SECRETVALUE", + mustNotHave: []string{"SECRETVALUE", "api-key="}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + req, err := http.NewRequest(http.MethodGet, tc.rawURL, nil) + require.NoError(t, err) + _, doErr := http.DefaultClient.Do(req) //nolint:bodyclose // Do returns a nil body alongside a non-nil error + require.Error(t, doErr, "127.0.0.1:1 must refuse the connection") + + tr := newTargetHealthTracker(3) + tr.record(targetKindMCP, "alpha", tc.rawURL, false, doErr, 0, targetHealthSourceProbe) + snap := tr.snapshot(targetKindMCP, "alpha") + for _, secret := range tc.mustNotHave { + assert.NotContains(t, snap.lastError, secret) + } + }) + } +} + func TestTargetHealthTracker_KeyedByKindAndName_Independent(t *testing.T) { t.Parallel() tr := newTargetHealthTracker(1) @@ -406,7 +491,13 @@ func TestMaybeSweepTargetHealth_ConcurrentTriggers_SingleFlight(t *testing.T) { // last == 0. A fast-completing first sweep, immediately followed by a // burst of concurrent callers, must still yield exactly one sweep within // probeInterval; a call after the interval has genuinely elapsed must -// start a second one. +// start a second one. G5, feat/target-health review round 2: this test +// proves single-flight ORDERING, not real elapsed time, so it must not +// depend on wall-clock sleeps at all — probeInterval is a full hour (so +// the burst phase's real, sub-millisecond elapsed time can never +// accidentally read as elapsed) and the "interval has passed" half is +// forced directly via targetHealthLastSweepUnixNano, never by sleeping +// for probeInterval. func TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls(t *testing.T) { t.Parallel() var hits int32 @@ -417,8 +508,7 @@ func TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls(t *testing.T) { })) defer srv.Close() - probeInterval := 150 * time.Millisecond - gw := newTargetHealthSweepGateway(t, true, probeInterval, srv.URL) + gw := newTargetHealthSweepGateway(t, true, time.Hour, srv.URL) // First sweep: the probe answers immediately, so this completes fast // and clears targetHealthSweeping well inside probeInterval. @@ -426,8 +516,9 @@ func TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls(t *testing.T) { waitForCondition(t, func() bool { return atomic.LoadInt32(&hits) > 0 }, time.Second) waitForCondition(t, func() bool { return gw.targetHealth.stateOf(targetKindMCP, "alpha") != targetHealthUnknown }, time.Second) - // A burst of concurrent callers immediately after — still well inside - // probeInterval — must not start a second sweep. + // A burst of concurrent callers immediately after — real elapsed time + // is milliseconds against a one-hour probeInterval — must not start a + // second sweep. var wg sync.WaitGroup for i := 0; i < 50; i++ { wg.Add(1) @@ -437,14 +528,13 @@ func TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls(t *testing.T) { }() } wg.Wait() - time.Sleep(30 * time.Millisecond) // let any wrongly-spawned sweep reach the server if got := atomic.LoadInt32(&hits); got != 1 { t.Errorf("hits = %d, want exactly 1 (a burst of concurrent callers inside probeInterval must not start a second sweep)", got) } - // Once probeInterval has genuinely elapsed, a new call starts a - // second sweep. - time.Sleep(probeInterval) + // Force the interval to read as elapsed directly, rather than + // sleeping for real: a call after that must start a second sweep. + atomic.StoreInt64(&gw.targetHealthLastSweepUnixNano, time.Now().Add(-2*time.Hour).UnixNano()) gw.maybeSweepTargetHealth() waitForCondition(t, func() bool { return atomic.LoadInt32(&hits) == 2 }, time.Second) }