diff --git a/internal/cli/app.go b/internal/cli/app.go index 44fa370ff..bd6fff291 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -839,6 +839,15 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a mcpTokenStore = nil err = nil } + // Checked on the WHOLE configuration, before the split. The two halves + // normalize separately, so a collision that straddles them is invisible to + // either call: a user-configured " exa" is critical while the built-in + // default "exa" is optional, and they are one runtime server. Ambiguous + // identity is worth refusing to start over, because every downstream join is + // keyed on that name and one of the two entries is unreachable regardless. + if err := mcp.ValidateUniqueNames(mcpConfig); err != nil { + return writeAppError(stderr, err.Error(), 1) + } criticalMCPConfig, optionalMCPConfig := splitMCPStartupConfig(mcpConfig) mcpRuntime := mcpToolRuntime(noopMCPRuntime{}) if len(criticalMCPConfig.Servers) > 0 { @@ -847,6 +856,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a Autonomy: mcp.AutonomyLow, Execution: executionRunner, WorkspaceRoot: workspaceRoot, + SecretValues: mcpTokenStore.SecretValues, }) if registerErr != nil { closeMCPRuntime(stderr, runtime) @@ -929,6 +939,7 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a Autonomy: mcp.AutonomyLow, Execution: executionRunner, WorkspaceRoot: workspaceRoot, + SecretValues: mcpTokenStore.SecretValues, }, deps.registerMCPTools, func() { @@ -1000,8 +1011,22 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a PeerService: peerService, SandboxStore: sandboxStore, MCPConfig: mcpConfig, - MCPPermissionStore: mcpPermissionStore, - MCPTokenStore: mcpTokenStore, + // The panel needs the failures too. A startup warning on stderr scrolls + // away behind the first screen of output, so /mcp is where a user goes + // to ask what is actually running โ€” it should not answer from config + // alone and report a server that never connected as enabled. + MCPSkipped: mcpRuntime.Skipped(), + // Optional servers register on a background goroutine, so their failures + // are not known yet and a snapshot cannot carry them. Pulled instead, or + // they stay rendered from configuration alone: enabled, with no + // explanation, for a server that never connected. + MCPLateSkipped: optionalMCPRuntime.Skipped, + // And a signal to rebuild when it finishes. Pulling alone leaves an + // already-open manager showing the configuration-derived state, because a + // background completion schedules no render of its own. + MCPStartupCompleted: optionalMCPRuntime.completed(), + MCPPermissionStore: mcpPermissionStore, + MCPTokenStore: mcpTokenStore, MCPCommand: func(ctx context.Context, args []string) tui.MCPCommandResult { if ctx == nil { ctx = context.Background() diff --git a/internal/cli/app_mcp_skipped_test.go b/internal/cli/app_mcp_skipped_test.go new file mode 100644 index 000000000..73bd77e9e --- /dev/null +++ b/internal/cli/app_mcp_skipped_test.go @@ -0,0 +1,82 @@ +package cli + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/tools" + "github.com/Gitlawb/zero/internal/tui" +) + +type skippingMCPRuntime struct { + skipped []mcp.SkippedServer +} + +func (r skippingMCPRuntime) Close() error { return nil } +func (r skippingMCPRuntime) Skipped() []mcp.SkippedServer { return r.skipped } + +// Startup already knows which servers failed โ€” it prints a warning about each. +// That warning is gone by the time anyone looks, so the same set has to reach +// the TUI, which is where /mcp answers "what is actually running". +func TestRunPassesSkippedMCPServersToTheTUI(t *testing.T) { + var stdout, stderr bytes.Buffer + cwd := t.TempDir() + setCLIUserConfigRoot(t) + projectConfigPath := filepath.Join(cwd, ".zero", "config.json") + if err := os.MkdirAll(filepath.Dir(projectConfigPath), 0o700); err != nil { + t.Fatalf("create project config parent: %v", err) + } + if err := os.WriteFile(projectConfigPath, []byte("{}"), 0o600); err != nil { + t.Fatalf("write project config: %v", err) + } + var launchedOptions tui.Options + + exitCode := runWithDeps([]string{}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return config.ResolvedConfig{MaxTurns: 12}, nil + }, + // A CONFIGURED server, so it lands in the critical half startup registers + // synchronously. Startup splits MCP into a critical set registered before + // the TUI launches and an optional set (unconfigured built-in defaults) + // registered on a background goroutine; with no servers configured at all, + // the registration this test stubs is never reached. + resolveMCPConfig: func(string, bool) (config.MCPConfig, error) { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + }}, nil + }, + userConfigPath: func() (string, error) { + return filepath.Join(t.TempDir(), "zero", "config.json"), nil + }, + registerMCPTools: func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) { + return skippingMCPRuntime{skipped: []mcp.SkippedServer{ + {Name: "docs", Err: errors.New("connection refused")}, + }}, nil + }, + runTUI: func(_ context.Context, options tui.Options) int { + launchedOptions = options + return 0 + }, + }) + + if exitCode != 0 { + t.Fatalf("exit code = %d, want 0 (stderr: %s)", exitCode, stderr.String()) + } + if len(launchedOptions.MCPSkipped) != 1 || + launchedOptions.MCPSkipped[0].Name != "docs" { + t.Fatalf("MCPSkipped = %#v, want the failure startup recorded", launchedOptions.MCPSkipped) + } + // The stderr warning stays: it is what a non-interactive user sees, and the + // panel is an addition to it, not a replacement. + if !strings.Contains(stderr.String(), "docs") { + t.Errorf("startup no longer warns about the skipped server: %q", stderr.String()) + } +} diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index c8c56fab6..03cf1bebf 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -641,11 +641,44 @@ func (cfg *mcpWritableConfig) ensureRaw() { } } +// refuseColliding validates the PROSPECTIVE configuration with the same rule the +// read and startup paths use. +// +// This used to compare key spellings here and reject every trimmed collision, +// which is a second and weaker implementation of the active-identity rule. +// ValidateUniqueNames skips disabled entries, because registration skips them +// and a disabled server claims no runtime identity; the write side did not, so +// `zero mcp add docs` failed when a disabled " docs" was configured, and even a +// plain update of an enabled "docs" was refused while its disabled alias +// existed. It also could not decide the inverse case at all, because it never +// saw whether the incoming server was itself disabled. +// +// Building the combined map and handing it to the shared rule removes the +// second implementation rather than teaching it the same exceptions. +func (cfg *mcpWritableConfig) refuseColliding(name string, incoming config.MCPServerConfig) error { + prospective := make(map[string]config.MCPServerConfig, len(cfg.file.MCP.Servers)+1) + for key, server := range cfg.file.MCP.Servers { + prospective[key] = server + } + prospective[name] = incoming + return mcp.ValidateUniqueNames(config.MCPConfig{Servers: prospective}) +} + func (cfg *mcpWritableConfig) upsertServer(name string, server config.MCPServerConfig) (bool, error) { cfg.ensureRaw() if cfg.file.MCP.Servers == nil { cfg.file.MCP.Servers = map[string]config.MCPServerConfig{} } + // One config key per runtime identity. Registration trims the key, so a new + // "docs" written next to an existing " docs" produces two entries that are + // one server everywhere downstream: they share a tool count and a failure, + // map iteration decides which configuration survives, and each redacts that + // shared failure with its own credentials, so the one that did not fail can + // print the other's. Refusing at the write boundary keeps the collision out + // of the file rather than reporting it on every later load. + if err := cfg.refuseColliding(name, server); err != nil { + return false, err + } existingRaw, updated := cfg.serverRaw[name] existingServer := cfg.file.MCP.Servers[name] if !updated { diff --git a/internal/cli/mcp_server_identity_test.go b/internal/cli/mcp_server_identity_test.go new file mode 100644 index 000000000..458b50140 --- /dev/null +++ b/internal/cli/mcp_server_identity_test.go @@ -0,0 +1,123 @@ +package cli + +import ( + "sort" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// ONE CONFIG KEY PER RUNTIME IDENTITY, ENFORCED WHERE THE KEY IS WRITTEN. +// +// Registration trims the config-map key, so "docs" and " docs" are two entries +// in the file and one server everywhere downstream. They share a tool count and +// a failure, map iteration decides which configuration wins, and each row +// redacts that shared failure with its own credentials, so the row that did not +// fail can print the other one's. +// +// Validation on the way in checks the incoming server by itself and cannot see +// the collision, so the check belongs at the write. +func TestUpsertRefusesAKeyThatCollidesWithAnExistingServer(t *testing.T) { + cfg := &mcpWritableConfig{} + cfg.ensureRaw() + cfg.file.MCP.Servers = map[string]config.MCPServerConfig{ + " docs": {URL: "https://a.invalid/mcp"}, + } + + _, err := cfg.upsertServer("docs", config.MCPServerConfig{URL: "https://b.invalid/mcp"}) + if err == nil { + t.Fatalf("a colliding key was written: %#v", cfg.file.MCP.Servers) + } + for _, want := range []string{`"docs"`, `" docs"`, "rename"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not mention %s: %v", want, err) + } + } + if len(cfg.file.MCP.Servers) != 1 { + t.Errorf("the refused server was written anyway: %#v", cfg.file.MCP.Servers) + } +} + +// Updating a server through its own key is the ordinary case and must not be +// mistaken for a collision with itself. +func TestUpsertStillUpdatesTheSameKey(t *testing.T) { + cfg := &mcpWritableConfig{} + cfg.ensureRaw() + cfg.file.MCP.Servers = map[string]config.MCPServerConfig{ + " docs": {URL: "https://a.invalid/mcp"}, + } + if _, err := cfg.upsertServer(" docs", config.MCPServerConfig{URL: "https://b.invalid/mcp"}); err != nil { + t.Fatalf("updating a server through its own key was refused: %v", err) + } + if got := cfg.file.MCP.Servers[" docs"].URL; got != "https://b.invalid/mcp" { + t.Errorf("URL = %q, want the update applied", got) + } +} + +// And an unrelated new server is not blocked by an existing one. +func TestUpsertAllowsADistinctName(t *testing.T) { + cfg := &mcpWritableConfig{} + cfg.ensureRaw() + cfg.file.MCP.Servers = map[string]config.MCPServerConfig{ + "docs": {URL: "https://a.invalid/mcp"}, + } + if _, err := cfg.upsertServer("search", config.MCPServerConfig{URL: "https://b.invalid/mcp"}); err != nil { + t.Fatalf("a distinct server was refused: %v", err) + } + if len(cfg.file.MCP.Servers) != 2 { + t.Errorf("servers = %#v, want both", cfg.file.MCP.Servers) + } +} + +// THE UNIQUENESS CHECK HAS TO RUN ON THE WHOLE CONFIGURATION, BEFORE THE SPLIT. +// +// Startup separates unconfigured built-in defaults from the servers the user +// asked for, so the two halves are normalized by separate calls. A collision +// that straddles the split is invisible to both of them: a user-configured +// " exa" is critical, the built-in "exa" is optional, and they are one runtime +// server with two panel rows sharing a failure and a tool count. +func TestACollisionAcrossTheStartupSplitIsStillRefused(t *testing.T) { + defaults := config.DefaultMCPServers() + names := make([]string, 0, len(defaults)) + for name := range defaults { + names = append(names, name) + } + sort.Strings(names) + if len(names) == 0 { + t.Skip("no built-in MCP defaults to collide with") + } + name := names[0] + + // The user's own entry under a padded key: same runtime identity, different + // configuration, so it is not the untouched default. + mine := defaults[name] + mine.Headers = map[string]string{"X-Api-Key": "opaque-workspace-9f3c2b7ae1d8"} + + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + name: defaults[name], + " " + name: mine, + }} + + critical, optional := splitMCPStartupConfig(cfg) + if len(critical.Servers) != 1 || len(optional.Servers) != 1 { + t.Fatalf("the two entries did not straddle the split: critical=%v optional=%v", critical.Servers, optional.Servers) + } + // Each half on its own sees one server and no collision, which is why the + // per-half check cannot be the guard. + if err := mcp.ValidateUniqueNames(critical); err != nil { + t.Fatalf("the critical half alone reported a collision: %v", err) + } + if err := mcp.ValidateUniqueNames(optional); err != nil { + t.Fatalf("the optional half alone reported a collision: %v", err) + } + + err := mcp.ValidateUniqueNames(cfg) + if err == nil { + t.Fatal("a collision straddling the startup split was accepted") + } + if !strings.Contains(err.Error(), name) || !strings.Contains(err.Error(), "rename") { + t.Errorf("the refusal is not actionable: %v", err) + } +} diff --git a/internal/cli/mcp_startup.go b/internal/cli/mcp_startup.go index fedaeecd5..f78645e51 100644 --- a/internal/cli/mcp_startup.go +++ b/internal/cli/mcp_startup.go @@ -142,6 +142,40 @@ func (startup *optionalMCPStartup) closeRuntime() { }) } +// Skipped reports the optional servers that failed, or nothing while startup is +// still running. +// +// A NIL RETURN WAS NOT "NOT READY", IT WAS "NONE". These servers are ordinary +// rows on the /mcp panel: splitMCPStartupConfig moves them off the critical path +// so a slow one cannot delay the first response, it does not make them invisible. +// Discarding their failures here left every one of them rendered from +// configuration alone, which reports a server that never connected as enabled -- +// the single thing that panel exists to prevent. +// completed exposes the completion signal so an already-open surface can be +// told to rebuild. Nil when there is no background registration, which is what +// the consumer treats as "nothing to wait for". +func (startup *optionalMCPStartup) completed() <-chan struct{} { + if startup == nil { + return nil + } + return startup.done +} + func (startup *optionalMCPStartup) Skipped() []mcp.SkippedServer { - return nil + if startup == nil { + return nil + } + select { + case <-startup.done: + default: + // Still connecting. There is no observation yet, and blocking here would + // stall a render on a server deliberately kept off the critical path. + return nil + } + startup.mu.Lock() + defer startup.mu.Unlock() + if startup.runtime == nil { + return nil + } + return startup.runtime.Skipped() } diff --git a/internal/cli/mcp_write_collision_test.go b/internal/cli/mcp_write_collision_test.go new file mode 100644 index 000000000..cb844db50 --- /dev/null +++ b/internal/cli/mcp_write_collision_test.go @@ -0,0 +1,59 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// THE WRITE BOUNDARY MUST USE THE SAME ACTIVE-IDENTITY RULE AS THE READ PATH. +// +// ValidateUniqueNames skips disabled entries, because registration skips them +// and a disabled server claims no runtime identity. The write side compared key +// spellings instead, so it rejected combinations the running configuration +// accepts and could not see whether the incoming server was itself disabled. +func TestWriteCollisionMatchesTheDisabledServerPolicy(t *testing.T) { + withServers := func(servers map[string]config.MCPServerConfig) *mcpWritableConfig { + cfg := &mcpWritableConfig{} + cfg.file.MCP.Servers = servers + return cfg + } + enabled := config.MCPServerConfig{Type: "stdio", Command: "docs-mcp"} + disabled := config.MCPServerConfig{Type: "stdio", Command: "docs-mcp", Disabled: true} + + t.Run("two enabled keys with one canonical name are refused", func(t *testing.T) { + cfg := withServers(map[string]config.MCPServerConfig{" docs": enabled}) + err := cfg.refuseColliding("docs", enabled) + if err == nil { + t.Fatal("two enabled entries resolving to one name were accepted") + } + if !strings.Contains(err.Error(), "docs") { + t.Errorf("the error does not name the collision: %v", err) + } + }) + + t.Run("a disabled existing alias does not block the active server", func(t *testing.T) { + cfg := withServers(map[string]config.MCPServerConfig{" docs": disabled}) + if err := cfg.refuseColliding("docs", enabled); err != nil { + t.Errorf("adding an enabled server beside a disabled alias was refused: %v", err) + } + }) + + t.Run("a disabled incoming server does not collide either", func(t *testing.T) { + cfg := withServers(map[string]config.MCPServerConfig{"docs": enabled}) + if err := cfg.refuseColliding(" docs", disabled); err != nil { + t.Errorf("adding a disabled alias beside an enabled server was refused: %v", err) + } + }) + + t.Run("updating the enabled entry while its disabled alias remains", func(t *testing.T) { + cfg := withServers(map[string]config.MCPServerConfig{ + "docs": enabled, + " docs": disabled, + }) + if err := cfg.refuseColliding("docs", enabled); err != nil { + t.Errorf("updating the enabled entry beside its disabled alias was refused: %v", err) + } + }) +} diff --git a/internal/mcp/config.go b/internal/mcp/config.go index 0ea5bdce2..ba49aa10a 100644 --- a/internal/mcp/config.go +++ b/internal/mcp/config.go @@ -48,6 +48,10 @@ func NormalizeConfig(cfg config.MCPConfig) ([]Server, error) { } sort.Strings(names) + if err := ValidateUniqueNames(cfg); err != nil { + return nil, err + } + servers := make([]Server, 0, len(names)) for _, name := range names { raw := cfg.Servers[name] @@ -63,6 +67,54 @@ func NormalizeConfig(cfg config.MCPConfig) ([]Server, error) { return servers, nil } +// ValidateUniqueNames reports two enabled configuration keys that resolve to +// one runtime identity. +// +// THE NORMALIZED NAME IS AN IDENTITY, so it has to be unique. Trimming means +// "docs" and " docs " are two configuration keys and one runtime server, and +// everything downstream keys on the runtime name: tool accounting, the +// skipped-server observations the panel renders, and invalidation. Two rows then +// share one failure, both report the same state, and Go map iteration decides +// which configuration survives. +// +// It is also a confidentiality problem rather than only a wrong status. Each row +// redacts that shared error with ITS OWN configuration, so if the server that +// actually failed echoed a credential, the other row does not have that value +// among its candidates and prints it. +// +// Rejecting is the honest answer: one of the two entries is unreachable whatever +// we do, and an error naming both spellings is something an operator can act on. +// A single padded name still works, because trimming is not the problem; two +// names trimming to one is. A disabled entry claims nothing, matching the rest +// of startup, which skips it before it reaches any of these paths. +// +// Exported because startup splits the configuration before registering it, so +// the two halves normalize separately and a collision that straddles them is +// invisible to either call. A user-configured " exa" alongside the built-in +// default "exa" is exactly that shape. +func ValidateUniqueNames(cfg config.MCPConfig) error { + names := make([]string, 0, len(cfg.Servers)) + for name := range cfg.Servers { + names = append(names, name) + } + sort.Strings(names) + + claimed := make(map[string]string, len(names)) + for _, name := range names { + if cfg.Servers[name].Disabled { + continue + } + canonical := strings.TrimSpace(name) + if previous, taken := claimed[canonical]; taken { + return fmt.Errorf( + "mcp: server names %q and %q both resolve to %q; rename one so each server has its own identity", + previous, name, canonical) + } + claimed[canonical] = name + } + return nil +} + func normalizeServer(name string, raw config.MCPServerConfig) (Server, error) { name = strings.TrimSpace(name) if err := ValidateServerName(name); err != nil { diff --git a/internal/mcp/credential_fingerprint.go b/internal/mcp/credential_fingerprint.go new file mode 100644 index 000000000..12bfd5677 --- /dev/null +++ b/internal/mcp/credential_fingerprint.go @@ -0,0 +1,25 @@ +package mcp + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "sort" +) + +// CredentialFingerprint identifies a set of credential values without retaining +// them, so an observation can record WHICH material made it safe rather than +// keeping a second long-lived copy of that material. +// +// Order-independent, because the token store enumerates a map. Length-prefixed, +// because ["ab","c"] and ["a","bc"] would otherwise produce the same digest and +// a rotation between those two shapes would read as no change at all. +func CredentialFingerprint(values []string) string { + sorted := append([]string(nil), values...) + sort.Strings(sorted) + digest := sha256.New() + for _, value := range sorted { + fmt.Fprintf(digest, "%d:%s", len(value), value) + } + return hex.EncodeToString(digest.Sum(nil)) +} diff --git a/internal/mcp/oauth_secret_values_test.go b/internal/mcp/oauth_secret_values_test.go new file mode 100644 index 000000000..0a7ddb42e --- /dev/null +++ b/internal/mcp/oauth_secret_values_test.go @@ -0,0 +1,141 @@ +package mcp + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" +) + +func newSecretValuesTestStore(t *testing.T) *TokenStore { + t.Helper() + now := time.Date(2026, 8, 12, 12, 0, 0, 0, time.UTC) + store, err := NewTokenStore(TokenStoreOptions{ + FilePath: filepath.Join(t.TempDir(), "tokens.json"), + // Pinned to the file backend: NewTokenStore otherwise reads + // ZERO_OAUTH_STORAGE from the real process environment, and a developer + // with it set to the keyring would run this against their own keychain. + Env: map[string]string{}, + Now: func() time.Time { return now }, + }) + if err != nil { + t.Fatalf("NewTokenStore() error = %v", err) + } + return store +} + +// SecretValues has to find the tokens that actually exist. +// +// The login path saves IDENTITY-BOUND (SaveForServer), and the name-only Save +// has no non-test callers. A redaction pass that looked a token up by server +// name would therefore come back empty for exactly the servers that hold a real +// bearer, and would look correct in any test that seeded the store by name. +func TestSecretValuesFindsIdentityBoundTokens(t *testing.T) { + store := newSecretValuesTestStore(t) + servers, err := NormalizeConfig(config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "linear": {Type: "http", URL: "https://linear.example/mcp", Auth: "oauth"}, + }}) + if err != nil { + t.Fatalf("NormalizeConfig() error = %v", err) + } + if err := store.SaveForServer(servers[0], StoredToken{ + AccessToken: "identity-access-token", + RefreshToken: "identity-refresh-token", + }); err != nil { + t.Fatalf("SaveForServer() error = %v", err) + } + + // The name-only read is what a per-server lookup would have used. It finding + // nothing is the whole reason SecretValues enumerates instead. + if _, ok, err := store.Load("linear"); err == nil && ok { + t.Fatal("the name-only key resolved, so this test no longer proves identity-bound tokens are reachable") + } + + values := store.SecretValues() + for _, want := range []string{"identity-access-token", "identity-refresh-token"} { + if !containsValue(values, want) { + t.Errorf("SecretValues() = %q, missing %q", values, want) + } + } +} + +// A legacy name-keyed token is still live material and must be returned too. +func TestSecretValuesFindsNameKeyedTokens(t *testing.T) { + store := newSecretValuesTestStore(t) + if err := store.Save("docs", StoredToken{AccessToken: "legacy-access-token"}); err != nil { + t.Fatalf("Save() error = %v", err) + } + if !containsValue(store.SecretValues(), "legacy-access-token") { + t.Errorf("SecretValues() = %q, missing the legacy token", store.SecretValues()) + } +} + +// READING SECRETS FOR REDACTION MUST NOT WRITE. +// +// The obvious way to get the material the runtime sends is LoadForServer, which +// is what the bearer path uses. It also migrates a legacy entry to the identity +// key as a side effect, so wiring it into a view builder would make opening a +// panel rewrite the token store on disk and take its cross-process lock. This +// pins that SecretValues does not. +func TestSecretValuesDoesNotWriteToTheStore(t *testing.T) { + store := newSecretValuesTestStore(t) + if err := store.Save("docs", StoredToken{AccessToken: "legacy-access-token"}); err != nil { + t.Fatalf("Save() error = %v", err) + } + + path := store.FilePath() + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(%s) error = %v", path, err) + } + beforeInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("Stat(%s) error = %v", path, err) + } + + if len(store.SecretValues()) == 0 { + t.Fatal("SecretValues() returned nothing, so this test would pass without reading anything") + } + + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(%s) error = %v", path, err) + } + if string(before) != string(after) { + t.Error("SecretValues() rewrote the token store; a redaction pass must not mutate it") + } + if afterInfo, err := os.Stat(path); err == nil && !afterInfo.ModTime().Equal(beforeInfo.ModTime()) { + t.Error("SecretValues() touched the token store file") + } +} + +// A nil store is the production state whenever initialization soft-failed at +// startup, so the method has to answer rather than crash. +func TestSecretValuesOnNilStoreReturnsNothing(t *testing.T) { + var store *TokenStore + if got := store.SecretValues(); len(got) != 0 { + t.Errorf("SecretValues() on a nil store = %q, want nothing", got) + } +} + +// An unreadable store degrades to redacting less, never to failing the caller. +func TestSecretValuesSwallowsAnUnreadableStore(t *testing.T) { + store := newSecretValuesTestStore(t) + if err := os.WriteFile(store.FilePath(), []byte(`{"schemaVersion":999}`), 0o600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + if got := store.SecretValues(); len(got) != 0 { + t.Errorf("SecretValues() on an unreadable store = %q, want nothing", got) + } +} + +func containsValue(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} diff --git a/internal/mcp/oauth_store.go b/internal/mcp/oauth_store.go index ce730182a..575893eac 100644 --- a/internal/mcp/oauth_store.go +++ b/internal/mcp/oauth_store.go @@ -241,6 +241,51 @@ func (store *TokenStore) DeleteForServerName(serverName string) (bool, error) { return removed, nil } +// SecretValues returns the bearer material of every stored MCP token, for +// callers that must redact it out of untrusted text. It is the deliberate +// inverse of Status, which exists to be printable. +// +// Three properties matter, and each is a trap avoided: +// +// It is READ ONLY. LoadForServer is what the runtime bearer path uses, but it +// migrates a legacy name-only entry to the identity key as a side effect, and a +// redaction pass must never write to the token store, let alone take its +// cross-process lock, just because someone opened a panel. +// +// It reads EVERY key rather than one server's. Tokens are stored identity-bound +// (the login path calls SaveForServer), so a per-name Load finds nothing for +// precisely the servers that hold a real bearer. Enumerating also sidesteps +// deriving an identity here, which would otherwise mean normalizing the whole +// config and losing every server's redaction because one entry is malformed. +// +// It returns material from ALL servers, and a caller should redact all of it out +// of any one server's message. A bearer issued for one server has no business +// appearing in another's error, so if it does, that is the leak worth closing, +// not a false positive. +// +// Errors are swallowed by design: an unreadable store must degrade to redacting +// less, never to failing the render. It therefore returns nothing in that case, +// so a caller cannot read a non-empty result as proof the store was legible. +func (store *TokenStore) SecretValues() []string { + if store == nil { + return nil + } + statuses, err := store.store.Status(oauth.KeyPrefixMCP) + if err != nil { + return nil + } + values := make([]string, 0, len(statuses)*2) + for _, status := range statuses { + token, ok, err := store.store.Load(status.Key) + if err != nil || !ok { + continue + } + stored := tokenToStored(token) + values = append(values, stored.AccessToken, stored.RefreshToken) + } + return values +} + // Status returns a redaction-safe summary of every stored MCP token, sorted by // server name. It never includes the token material. func (store *TokenStore) Status() ([]TokenStatus, error) { diff --git a/internal/mcp/registry.go b/internal/mcp/registry.go index d1a2978dc..13cd79285 100644 --- a/internal/mcp/registry.go +++ b/internal/mcp/registry.go @@ -29,6 +29,11 @@ type RegisterOptions struct { ConnectTimeout time.Duration Execution *execution.Runner WorkspaceRoot string + // SecretValues reports the credential material that exists right now, and is + // sampled BEFORE the connect phase so every skipped entry can record the + // context that made its error safe to display. See SkippedServer.Credentials. + // Optional: nil records no context and claims nothing. + SecretValues func() []string } // SkippedServer records an MCP server that was not registered because it could @@ -42,6 +47,23 @@ type SkippedServer struct { // server is an out-of-the-box default the user never configured, so a // caller can skip warning loudly about it. UnconfiguredDefault bool + // Credentials fingerprints the credential material that existed when Err was + // produced. + // + // THE CONTEXT THAT MAKES AN ERROR SAFE HAS TO BE RECORDED WHERE THE ERROR IS. + // Err is the RAW failure and is redacted at display time against whatever the + // token store holds then, so a consumer needs to know whether that set is + // still the one that was hiding the credential. Sampling it at the consumer + // instead is too late: startup registers here and the interactive surface is + // built seconds later, after plugin activation and the rest of startup, and a + // refresh, a logout, or another process can rotate the store in between. The + // consumer would then compare the current set against itself, find no change, + // and print an error containing a bearer that no longer exists anywhere in the + // candidate set. + // + // Empty means no context was recorded, which is a claim of nothing rather than + // a claim that nothing changed. + Credentials string } type Runtime struct { @@ -108,6 +130,17 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP cancel context.CancelFunc err error } + // Sampled BEFORE connecting, not after. A 401 during connect triggers a + // refresh that rotates the stored bearer, and the error text captured on that + // same attempt can contain the OLD one. A sample taken afterwards would record + // the new set, match at display time, and let the old bearer through with + // nothing left in the candidate set to hide it. Sampling first means such a + // rotation reads as a change and the reason is withheld, which is the safe + // direction to be wrong in. + credentials := "" + if options.SecretValues != nil { + credentials = CredentialFingerprint(options.SecretValues()) + } results := make([]connectResult, len(servers)) var wg sync.WaitGroup for index := range servers { @@ -155,7 +188,7 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP for index, server := range servers { res := results[index] if res.err != nil { - runtime.skipped = append(runtime.skipped, SkippedServer{Name: server.Name, Err: res.err, UnconfiguredDefault: server.UnconfiguredDefault}) + runtime.skipped = append(runtime.skipped, SkippedServer{Name: server.Name, Err: res.err, UnconfiguredDefault: server.UnconfiguredDefault, Credentials: credentials}) continue } serverTools, validateErr := buildServerTools(registry, server, res.remote, res.client, options, stagedNames) @@ -164,7 +197,7 @@ func RegisterTools(ctx context.Context, registry *tools.Registry, cfg config.MCP res.cancel() } _ = res.client.Close() - runtime.skipped = append(runtime.skipped, SkippedServer{Name: server.Name, Err: validateErr, UnconfiguredDefault: server.UnconfiguredDefault}) + runtime.skipped = append(runtime.skipped, SkippedServer{Name: server.Name, Err: validateErr, UnconfiguredDefault: server.UnconfiguredDefault, Credentials: credentials}) continue } runtime.clients = append(runtime.clients, res.client) diff --git a/internal/mcp/server_identity_test.go b/internal/mcp/server_identity_test.go new file mode 100644 index 000000000..2cea357d9 --- /dev/null +++ b/internal/mcp/server_identity_test.go @@ -0,0 +1,67 @@ +package mcp + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// THE NORMALIZED NAME IS AN IDENTITY, SO IT HAS TO BE UNIQUE. +// +// Trimming means "docs" and " docs " are two config keys and one runtime +// server, and everything downstream keys on the runtime name: tool accounting, +// the skipped-server observations the panel renders, and invalidation. Two rows +// then share one failure and report the same state, with Go map iteration +// deciding which configuration survives. +// +// It is also a confidentiality problem rather than only a wrong status. Each row +// redacts that shared error with its OWN configuration, so if the server that +// actually failed echoed a credential, the other row does not have that value +// among its candidates and prints it. +func TestDuplicateNamesAfterNormalizationAreRejected(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://a.invalid/mcp"}, + " docs": {URL: "https://b.invalid/mcp"}, + }} + servers, err := NormalizeConfig(cfg) + if err == nil { + t.Fatalf("two names resolving to one identity were accepted: %#v", servers) + } + // Actionable means naming both spellings: the operator cannot see the + // collision in a config file where one key merely looks indented. + for _, want := range []string{`"docs"`, `" docs"`, "rename"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error does not mention %s: %v", want, err) + } + } +} + +// A single padded name is not the problem and must keep working: trimming is +// the intended behaviour, two names trimming to one is not. +func TestASinglePaddedNameStillNormalizes(t *testing.T) { + servers, err := NormalizeConfig(config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + " docs ": {URL: "https://a.invalid/mcp"}, + }}) + if err != nil { + t.Fatalf("a single padded name was rejected: %v", err) + } + if len(servers) != 1 || servers[0].Name != "docs" { + t.Fatalf("servers = %#v, want one server named docs", servers) + } +} + +// A disabled entry claims no identity, so it cannot collide with the live one +// that replaced it. +func TestADisabledDuplicateDoesNotCollide(t *testing.T) { + servers, err := NormalizeConfig(config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://a.invalid/mcp"}, + " docs": {URL: "https://b.invalid/mcp", Disabled: true}, + }}) + if err != nil { + t.Fatalf("a disabled duplicate was treated as a collision: %v", err) + } + if len(servers) != 1 { + t.Fatalf("servers = %#v, want only the enabled one", servers) + } +} diff --git a/internal/mcp/skipped_credentials_test.go b/internal/mcp/skipped_credentials_test.go new file mode 100644 index 000000000..55de04ff3 --- /dev/null +++ b/internal/mcp/skipped_credentials_test.go @@ -0,0 +1,131 @@ +package mcp + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/tools" +) + +// THE CONTEXT THAT MAKES AN ERROR SAFE IS RECORDED WHERE THE ERROR IS PRODUCED. +// +// A skipped entry keeps the RAW failure, and the surface that displays it +// redacts it against whatever the token store holds at that moment. So the +// consumer has to know whether that set is still the one that was hiding the +// credential, and the only place that can answer honestly is registration. +// +// Sampling at the consumer is too late twice over. Startup registers here and +// the interactive surface is built afterwards, so a refresh or a logout in +// between is invisible; and the connect attempt that PRODUCED the error can +// itself rotate the store, because a 401 triggers a refresh and the error text +// captured on that attempt can contain the bearer that was just replaced. The +// sample therefore has to be taken before connecting, not after: getting it +// wrong in that direction means the consumer compares the new set against +// itself, finds no change, and prints an error containing a bearer that is no +// longer a candidate anywhere. +func TestSkippedFailuresRecordTheCredentialsThatExistedBeforeConnecting(t *testing.T) { + registry := tools.NewRegistry() + const before = "bearer-before-rotation" + const after = "bearer-after-rotation" + + var mu sync.Mutex + current := []string{before} + + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + SecretValues: func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), current...) + }, + ClientFactory: func(context.Context, Server) (ToolClient, error) { + // What a refresh does: the stored bearer is replaced, and the failure + // reported for this attempt still quotes the old one. + mu.Lock() + current = []string{after} + mu.Unlock() + return nil, fmt.Errorf("upstream rejected %s", before) + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + skipped := runtime.Skipped() + if len(skipped) != 1 { + t.Fatalf("Skipped() = %#v, want one entry", skipped) + } + if skipped[0].Credentials == "" { + t.Fatal("the failure recorded no credential context at all") + } + if skipped[0].Credentials == CredentialFingerprint([]string{after}) { + t.Error("the context was sampled after the connect that rotated the bearer, so the rotation is invisible and the old bearer quoted in the error has nothing left to hide it") + } + if want := CredentialFingerprint([]string{before}); skipped[0].Credentials != want { + t.Errorf("Credentials = %q, want the fingerprint of the material that existed when the error was produced", skipped[0].Credentials) + } +} + +// A validation failure is the other capture site and must record the same +// context, or half the failures carry no claim. +func TestAValidationFailureAlsoRecordsTheCredentialContext(t *testing.T) { + registry := tools.NewRegistry() + const bearer = "stored-bearer-value" + + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + SecretValues: func() []string { return []string{bearer} }, + ClientFactory: func(context.Context, Server) (ToolClient, error) { + // A nameless tool fails validation in the serial commit phase. + return &fakeToolClient{listed: []RemoteTool{{Name: "", Description: "nameless"}}}, nil + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + skipped := runtime.Skipped() + if len(skipped) != 1 { + t.Fatalf("Skipped() = %#v, want one entry", skipped) + } + if want := CredentialFingerprint([]string{bearer}); skipped[0].Credentials != want { + t.Errorf("Credentials = %q, want %q", skipped[0].Credentials, want) + } +} + +// Without a source there is no claim to make, and an empty fingerprint has to +// stay distinguishable from the fingerprint of an empty set: one means "nothing +// recorded", the other means "nothing was there". +func TestNoCredentialSourceRecordsNoClaim(t *testing.T) { + registry := tools.NewRegistry() + runtime, err := RegisterTools(context.Background(), registry, config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, RegisterOptions{ + ClientFactory: func(context.Context, Server) (ToolClient, error) { + return nil, errors.New("unreachable") + }, + }) + if err != nil { + t.Fatalf("RegisterTools() error = %v", err) + } + defer runtime.Close() + + skipped := runtime.Skipped() + if len(skipped) != 1 { + t.Fatalf("Skipped() = %#v, want one entry", skipped) + } + if skipped[0].Credentials != "" { + t.Errorf("Credentials = %q, want empty: nothing was recorded", skipped[0].Credentials) + } + if CredentialFingerprint(nil) == "" { + t.Error("the fingerprint of an empty set must not be empty, or it cannot be told apart from no claim") + } +} diff --git a/internal/redaction/overlapping_secrets_test.go b/internal/redaction/overlapping_secrets_test.go new file mode 100644 index 000000000..ac808d385 --- /dev/null +++ b/internal/redaction/overlapping_secrets_test.go @@ -0,0 +1,92 @@ +package redaction + +import ( + "strings" + "testing" +) + +// A SECRET MUST NOT SURVIVE BECAUSE ANOTHER SECRET WAS REPLACED FIRST. +// +// Extra secret values are replaced one after another with ReplaceAll. When one +// configured value is a prefix of another, replacing the SHORT one first eats +// the head of the long one, and what is left of the long value no longer matches +// anything: the tail of a real credential is printed. The partial replacement +// also destroys the token shape, so the pattern passes further down cannot +// recover it either. +// +// Callers collect these values out of maps (MCP headers and env), and Go +// randomizes map iteration, so before the fix which of the two happened was +// decided fresh on every run. A test that fed them in one fixed order would have +// been green about half the time for the wrong reason, so both orders are +// asserted here. +func TestOverlappingSecretsAreRedactedWhicheverOrderTheyArrive(t *testing.T) { + const short = "abcdefgh" + const long = "abcdefghXYZ" + message := "connect failed for tenant: " + long + + for _, testCase := range []struct { + name string + values []string + }{ + {name: "short value first", values: []string{short, long}}, + {name: "long value first", values: []string{long, short}}, + } { + t.Run(testCase.name, func(t *testing.T) { + got := RedactString(message, Options{ExtraSecretValues: testCase.values}) + if strings.Contains(got, "XYZ") { + t.Errorf("the tail of the longer secret survived: %q", got) + } + if strings.Contains(got, short) { + t.Errorf("the shorter secret survived: %q", got) + } + // The diagnostic half of the message has to be left alone, or + // redaction has simply destroyed the error instead of cleaning it. + if !strings.Contains(got, "connect failed for tenant:") { + t.Errorf("redaction ate the diagnostic text: %q", got) + } + }) + } +} + +// Order-independence has to hold for the OUTPUT too, not just for the absence of +// the secret. Two runs that redact the same message with the same values in a +// different order must produce the same string, or the panel and the transcript +// disagree about what a failure looked like depending on map iteration. +func TestOverlappingSecretRedactionIsOrderIndependent(t *testing.T) { + values := []string{"tok_abcdefgh", "tok_abcdefghijkl", "hdr_abcdefgh"} + message := "auth failed: tok_abcdefghijkl rejected by hdr_abcdefgh" + + first := RedactString(message, Options{ExtraSecretValues: values}) + shuffled := []string{values[2], values[0], values[1]} + second := RedactString(message, Options{ExtraSecretValues: shuffled}) + + if first != second { + t.Errorf("redaction depends on the order the caller collected its secrets:\n %q\nvs\n %q", first, second) + } + for _, secret := range values { + if strings.Contains(first, secret) { + t.Errorf("secret %q survived: %q", secret, first) + } + } +} + +// A duplicate must not change the result. Callers append from several sources +// and the same value can arrive twice; replacing it a second time finds nothing +// and must stay harmless. +func TestDuplicateSecretValuesAreHarmless(t *testing.T) { + const secret = "sk-duplicate-value" + once := RedactString("failed: "+secret, Options{ExtraSecretValues: []string{secret}}) + twice := RedactString("failed: "+secret, Options{ExtraSecretValues: []string{secret, secret}}) + if once != twice { + t.Errorf("a repeated secret changed the result:\n %q\nvs\n %q", once, twice) + } +} + +// The empty-value guard has to survive the reordering. A blank entry must not +// become a replacement that matches everywhere. +func TestBlankSecretValuesAreIgnored(t *testing.T) { + got := RedactString("connection refused", Options{ExtraSecretValues: []string{"", " ", "\t"}}) + if got != "connection refused" { + t.Errorf("a blank secret value altered the message: %q", got) + } +} diff --git a/internal/tui/command_views.go b/internal/tui/command_views.go index b7b3f21e4..0c58a2e5b 100644 --- a/internal/tui/command_views.go +++ b/internal/tui/command_views.go @@ -9,6 +9,7 @@ import ( tea "charm.land/bubbletea/v2" "github.com/Gitlawb/zero/internal/config" + internalmcp "github.com/Gitlawb/zero/internal/mcp" "github.com/Gitlawb/zero/internal/providercatalog" "github.com/Gitlawb/zero/internal/sandbox" "github.com/Gitlawb/zero/internal/tools" @@ -70,18 +71,58 @@ func (m *model) mcpText() string { } func (m *model) refreshMCPViewState() { + late := m.lateMCPSkipped() + m.mcpLateSkippedCount = len(late) m.mcpViewStateCache = BuildMCPViewState(MCPStateOptions{ - Config: m.mcpConfig, - Registry: m.registry, - PermissionStore: m.mcpPermissionStore, - PermissionMode: string(m.permissionMode), - TokenStore: m.mcpTokenStore, + Config: m.mcpConfig, + Registry: m.registry, + PermissionStore: m.mcpPermissionStore, + PermissionMode: string(m.permissionMode), + TokenStore: m.mcpTokenStore, + Skipped: mergedMCPSkipped(m.mcpSkipped, late), + SkippedCredentials: m.mcpSkippedCredentials, }) m.mcpViewStateReady = true } +// lateMCPSkipped returns the background registration's failures, aged against +// the current configuration exactly as the startup snapshot is. They were +// observed against the configuration this session started with, so that is what +// they are compared to. +func (m *model) lateMCPSkipped() []internalmcp.SkippedServer { + if m.mcpLateSkipped == nil { + return nil + } + return retainedMCPSkipped(m.mcpLateSkipped(), m.mcpStartupConfig, m.mcpConfig) +} + +// mergedMCPSkipped combines the two sources, preferring what startup already +// knew. The critical and optional halves of the configuration are disjoint, so +// an overlap means the same server was observed twice and the earlier +// observation is the one whose credential context was recorded first. +func mergedMCPSkipped(known, late []internalmcp.SkippedServer) []internalmcp.SkippedServer { + if len(late) == 0 { + return known + } + seen := make(map[string]struct{}, len(known)) + for _, entry := range known { + seen[strings.TrimSpace(entry.Name)] = struct{}{} + } + merged := append([]internalmcp.SkippedServer(nil), known...) + for _, entry := range late { + if _, duplicate := seen[strings.TrimSpace(entry.Name)]; duplicate { + continue + } + merged = append(merged, entry) + } + return merged +} + func (m *model) mcpViewState() MCPViewState { - if m.mcpViewStateReady { + // A background registration finishes without any event this model observes, + // so the cache has to notice the new observation itself. Nothing else + // invalidates it: the configuration did not change. + if m.mcpViewStateReady && len(m.lateMCPSkipped()) == m.mcpLateSkippedCount { return m.mcpViewStateCache } // Older tests may construct a zero-value model; keep that path useful, while @@ -189,7 +230,7 @@ func (m model) applyMCPCommandResult(args string, result MCPCommandResult) (mode }, "\n") } if len(result.Config.Servers) > 0 || len(m.mcpConfig.Servers) > 0 { - m.mcpConfig = result.Config + m = m.adoptMCPConfig(result.Config) m.refreshMCPViewState() } output := strings.TrimSpace(result.Output) @@ -733,3 +774,20 @@ func (m model) skillsText() string { }, }) } + +// mcpStartupCompletedMsg reports that the background MCP registration finished. +type mcpStartupCompletedMsg struct{} + +// waitForMCPStartupCompletion turns the completion channel into one message. +// +// Optional MCP registration runs on its own goroutine so a slow server cannot +// delay the first response. That means its result arrives with no user input +// behind it, and Bubble Tea renders only in response to a message, so without +// this the manager an operator already has open keeps showing what the +// configuration said until they happen to type something or resize the terminal. +func waitForMCPStartupCompletion(done <-chan struct{}) tea.Cmd { + return func() tea.Msg { + <-done + return mcpStartupCompletedMsg{} + } +} diff --git a/internal/tui/mcp_add_wizard.go b/internal/tui/mcp_add_wizard.go index b97d122c9..d8dd033f3 100644 --- a/internal/tui/mcp_add_wizard.go +++ b/internal/tui/mcp_add_wizard.go @@ -292,7 +292,7 @@ func (m model) applyMCPAddWizardSaveResult(result MCPCommandResult, disabled boo return m } if len(result.Config.Servers) > 0 || len(m.mcpConfig.Servers) > 0 { - m.mcpConfig = result.Config + m = m.adoptMCPConfig(result.Config) m.refreshMCPViewState() } server := result.Config.Servers[wizard.serverName] diff --git a/internal/tui/mcp_candidate_bound_test.go b/internal/tui/mcp_candidate_bound_test.go new file mode 100644 index 000000000..b7a0d33e2 --- /dev/null +++ b/internal/tui/mcp_candidate_bound_test.go @@ -0,0 +1,133 @@ +package tui + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" +) + +// A DELIMITER-HEAVY CONFIGURED VALUE MUST NOT EXPAND WITHOUT BOUND. +// +// credentialCandidates walked every suffix after every space or colon and kept +// them all, and RedactString then ran a replacement pass per candidate. The +// expansion happened on the CONFIG side, outside the raw-error bound, so the +// cost did not depend on the server's error being long at all: opening or +// refreshing /mcp for that server paid it however short the failure was. +func TestADelimiterHeavyValueDoesNotExpandWithoutBound(t *testing.T) { + value := strings.Repeat("token:part ", 4000) + + candidates := credentialCandidates(value) + if len(candidates) > maxMCPCredentialCandidates { + t.Errorf("one configured value expanded into %d candidates, want at most %d", len(candidates), maxMCPCredentialCandidates) + } + + // And the whole value is still redacted, which is the point of bounding the + // tails rather than the value. + raw := config.MCPServerConfig{ + URL: "https://host.invalid/mcp", + Headers: map[string]string{"X-Workspace": value}, + } + started := time.Now() + got := redactMCPFailureReason(errors.New("502 Bad Gateway from "+value), raw, nil) + elapsed := time.Since(started) + + if strings.Contains(got, value) { + t.Error("the configured value survived redaction") + } + // Loose on purpose: it fails on the old superlinear behaviour and not on a + // slow machine. + if elapsed > 2*time.Second { + t.Errorf("redacting one failure took %s; the candidate expansion is still superlinear in the configured value", elapsed) + } +} + +// A WORK BOUND MUST NOT DROP A CREDENTIAL SPELLING. +// +// The input cap this replaces returned the whole value and nothing else once a +// configured value crossed 8 KiB. That turned a cost control into a change of +// security semantics: "Bearer " stopped offering the bare token as +// a candidate, so a failed server echoing only the token matched nothing, and an +// opaque token has no shape for the fallback redactor to recognise either. The +// first 400 characters then reached both /mcp render paths and the transcript. +// +// The bound is now on where a separator is looked for, so the tails survive at +// any length. +func TestAnOversizedCredentialStillYieldsItsToken(t *testing.T) { + token := strings.Repeat("A", 8198) + value := "Bearer " + token + + candidates := credentialCandidates(value) + if len(candidates) > maxMCPCredentialCandidates { + t.Errorf("expanded into %d candidates, want at most %d", len(candidates), maxMCPCredentialCandidates) + } + var whole, bare bool + for _, candidate := range candidates { + switch candidate { + case value: + whole = true + case token: + bare = true + } + } + if !whole { + t.Error("the whole configured value is no longer a candidate") + } + if !bare { + t.Fatal("the bare token is not a candidate, so a server echoing only the token is not redacted") + } +} + +// The header form has two separators, and both tails must survive the same way. +func TestAnOversizedHeaderCredentialYieldsBothTails(t *testing.T) { + token := strings.Repeat("B", 9000) + value := "Authorization: Bearer " + token + + var bare, afterHeader bool + for _, candidate := range credentialCandidates(value) { + switch candidate { + case token: + bare = true + case "Bearer " + token: + afterHeader = true + } + } + if !afterHeader { + t.Error("the tail is missing") + } + if !bare { + t.Error("the bare token is missing") + } +} + +// The separator scan is bounded, and the boundary is stated rather than implied: +// a separator inside the window yields its tail, one past the window does not. +// That is a cost decision, and it is safe because every convention this walks +// puts its separators in a short prefix. +func TestCredentialSeparatorScanBoundary(t *testing.T) { + token := strings.Repeat("C", 4096) + for _, testCase := range []struct { + name string + padding int + wantTail bool + }{ + {name: "separator just inside the window", padding: maxMCPCredentialSeparatorScan - 2, wantTail: true}, + {name: "separator at the last scanned byte", padding: maxMCPCredentialSeparatorScan - 1, wantTail: true}, + {name: "separator just past the window", padding: maxMCPCredentialSeparatorScan + 1, wantTail: false}, + } { + t.Run(testCase.name, func(t *testing.T) { + value := strings.Repeat("x", testCase.padding) + " " + token + var found bool + for _, candidate := range credentialCandidates(value) { + if candidate == token { + found = true + } + } + if found != testCase.wantTail { + t.Errorf("tail present = %v, want %v (padding %d)", found, testCase.wantTail, testCase.padding) + } + }) + } +} diff --git a/internal/tui/mcp_canonical_name_test.go b/internal/tui/mcp_canonical_name_test.go new file mode 100644 index 000000000..34cd17bd9 --- /dev/null +++ b/internal/tui/mcp_canonical_name_test.go @@ -0,0 +1,49 @@ +package tui + +import ( + "errors" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// ONE SERVER IDENTITY, AND IT IS THE REGISTRY'S. +// +// mcp.normalizeServer trims the config-map key, so a server configured as +// " docs " is registered, recorded in SkippedServer.Name, and counted in +// toolCounts as "docs". The view was iterating raw map keys and looking failures +// up with them, so failures[" docs "] missed and a server that never started +// rendered as enabled. That is precisely the state this panel exists to surface. +func TestFailedServerIsMatchedByItsCanonicalName(t *testing.T) { + for _, testCase := range []struct{ name, configKey string }{ + {name: "padded both sides", configKey: " docs "}, + {name: "trailing space", configKey: "docs "}, + {name: "leading space", configKey: " docs"}, + {name: "already canonical", configKey: "docs"}, + } { + t.Run(testCase.name, func(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + testCase.configKey: {URL: "https://example.invalid/mcp"}, + }} + // Registration records the CANONICAL name, which is what normalizeServer + // produced from the padded key. + skipped := []mcp.SkippedServer{{Name: "docs", Err: errors.New("dial tcp: connection refused")}} + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + if len(state.Servers) != 1 { + t.Fatalf("expected one server view, got %d", len(state.Servers)) + } + server := state.Servers[0] + if server.State != "failed" { + t.Errorf("server %q rendered as %q; a server that never started is being shown as running", testCase.configKey, server.State) + } + if server.Name != "docs" { + t.Errorf("rendered name = %q, want the canonical %q", server.Name, "docs") + } + if server.Error == "" { + t.Errorf("no failure reason rendered, so the operator is not told why it did not start") + } + }) + } +} diff --git a/internal/tui/mcp_credential_boundary_test.go b/internal/tui/mcp_credential_boundary_test.go new file mode 100644 index 000000000..c2590dfdf --- /dev/null +++ b/internal/tui/mcp_credential_boundary_test.go @@ -0,0 +1,252 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + mcppkg "github.com/Gitlawb/zero/internal/mcp" +) + +// aperiodicSecret builds an opaque credential with no period, so a suffix of it +// can never coincidentally equal one of its own prefixes. A repeating fixture +// makes the boundary tests below pass for the wrong reason. +func aperiodicSecret(n int) string { + const alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + out := make([]byte, n) + state := uint64(0x9E3779B97F4A7C15) + for i := range out { + state ^= state << 13 + state ^= state >> 7 + state ^= state << 17 + out[i] = alphabet[state%uint64(len(alphabet))] + } + return string(out) +} + +func containsValue(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} + +// THE MATCHER HAS TO COVER THE WHOLE CREDENTIAL, NOT A FIXED WINDOW. +// +// The raw error is cut before exact-value redaction, so a credential can +// straddle the cut and leave a prefix behind. The tail repair inspected a fixed +// 4 KiB at the end, which cannot find a credential that BEGINS earlier: the +// inspected span starts partway through it, and a middle is not a prefix. +func TestNoCredentialPrefixSurvivesTheRawBound(t *testing.T) { + cut := maxMCPReasonRawLen + maxMCPSecretMatchWindow + + t.Run("credential beginning before the tail window", func(t *testing.T) { + secret := aperiodicSecret(6000) + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + secret} + // Begin the credential 5000 bytes before the cut, so the last 4 KiB + // starts 904 bytes INSIDE it. + filler := strings.Repeat("A", cut-5000) + got := redactMCPFailureReason(errors.New(filler+secret+strings.Repeat("B", 4096)), raw, nil) + for size := len(secret); size > 0; size-- { + if strings.Contains(got, secret[:size]) { + t.Fatalf("a %d-byte prefix of the credential reached the panel", size) + } + } + }) + + t.Run("eight-byte credential split after seven", func(t *testing.T) { + // Exactly shortestMCPSecret. The old floor skipped remnants below eight + // bytes, so seven of these eight were displayed on purpose. + const secret = "Qw7ZmPr4" + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + secret} + got := redactMCPFailureReason(errors.New(strings.Repeat("A", cut-7)+secret+strings.Repeat("B", 64)), raw, nil) + if strings.HasSuffix(strings.TrimSpace(got), secret[:7]) { + t.Errorf("seven of the eight bytes of the credential survived: %q", got[max(0, len(got)-16):]) + } + }) + + t.Run("an ordinary failure keeps its tail", func(t *testing.T) { + // The floor exists so a candidate that merely starts with the last + // character of a message does not eat it. + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + aperiodicSecret(40)} + message := "dial tcp 10.0.0.5:443: connect: connection refused" + if got := redactMCPFailureReason(errors.New(message), raw, nil); !strings.Contains(got, message) { + t.Errorf("an unrelated failure lost its tail: %q", got) + } + }) +} + +// PROVENANCE OUTRANKS THE READABILITY HEURISTIC. +// +// credentialCandidates drops anything under shortestMCPSecret so ordinary short +// configuration such as v=1 or mode=sse is not blanked out of unrelated text. +// That trade-off is only defensible while a value might not be a credential. +// Routing values already KNOWN to be secret through it discarded them. +func TestKnownCredentialsSkipTheReadabilityFloor(t *testing.T) { + const shortSecret = "s3cr3t" // six bytes, under shortestMCPSecret + + for _, testCase := range []struct { + name string + raw config.MCPServerConfig + echo string + }{ + { + name: "oauth client secret", + raw: config.MCPServerConfig{URL: "https://host.invalid/mcp", OAuth: &config.MCPOAuthConfig{ClientSecret: shortSecret}}, + echo: "token endpoint replied error_description=" + shortSecret, + }, + { + name: "credential-bearing flag value", + raw: config.MCPServerConfig{Command: "server", Args: []string{"--api-key", shortSecret}}, + echo: "child rejected invocation: server --api-key " + shortSecret, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + if !containsValue(mcpServerSecretValues(testCase.raw), shortSecret) { + t.Errorf("a credential known by provenance was dropped for being short") + } + if got := redactMCPFailureReason(errors.New(testCase.echo), testCase.raw, nil); strings.Contains(got, shortSecret) { + t.Errorf("the credential reached the panel: %q", got) + } + }) + } + + // And the floor still protects ordinary short configuration, or the fix + // would just be blanket over-redaction. + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?mode=sse&v=1"} + if got := redactMCPFailureReason(errors.New("transport mode=sse rejected"), raw, nil); !strings.Contains(got, "mode=sse") { + t.Errorf("an ordinary short parameter was redacted out of the message: %q", got) + } +} + +// THE OAUTH ENDPOINTS PARTICIPATE IN STARTUP TOO. +// +// With a stored token a 401 triggers a refresh, oauth.PostToken posts to the +// configured TokenEndpoint, and a dial or TLS failure is wrapped in a url.Error +// that keeps the path and query. Collecting only from the main URL left those +// values outside the candidate set. +func TestOAuthEndpointCredentialsAreRedacted(t *testing.T) { + const secret = "opaque-workspace-9f3c2b7ae1d8" + for _, testCase := range []struct { + name string + oauth *config.MCPOAuthConfig + }{ + {"token endpoint", &config.MCPOAuthConfig{TokenEndpoint: "https://auth.invalid/token?workspace=" + secret}}, + {"authorization endpoint", &config.MCPOAuthConfig{AuthorizationEndpoint: "https://auth.invalid/authorize?workspace=" + secret}}, + {"registration endpoint", &config.MCPOAuthConfig{RegistrationEndpoint: "https://auth.invalid/register?workspace=" + secret}}, + {"issuer url", &config.MCPOAuthConfig{IssuerURL: "https://auth.invalid/issuer?workspace=" + secret}}, + } { + t.Run(testCase.name, func(t *testing.T) { + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp", OAuth: testCase.oauth} + got := redactMCPFailureReason(errors.New(`Post "https://auth.invalid/x?workspace=`+secret+`": dial tcp: refused`), raw, nil) + if strings.Contains(got, secret) { + t.Errorf("the endpoint credential reached the panel: %q", got) + } + if !strings.Contains(got, "auth.invalid") { + t.Errorf("the host was redacted too, so the failure is no longer diagnosable: %q", got) + } + }) + } +} + +// ONE PARSER FOR BOTH SURFACES. +// +// The collector and the target row each derived the accepted header spellings +// separately. Neither recognised the conventional attached form, so the value +// was missing from the redaction set AND printed verbatim one row below. +func TestAttachedHeaderArgumentIsRedactedOnBothSurfaces(t *testing.T) { + const secret = "opaque-workspace-token-9f3c2b7" + for _, arg := range []string{ + "-HX-Workspace-Id:" + secret, + "-HX-Workspace-Id: " + secret, + "-H=X-Workspace-Id: " + secret, + "-H X-Workspace-Id: " + secret, + "--header=X-Workspace-Id: " + secret, + "--HEADER=X-Workspace-Id: " + secret, + } { + t.Run(arg, func(t *testing.T) { + args := []string{"mcp-server", arg} + if !containsValue(sensitiveMCPArgValues(args), secret) { + t.Errorf("the header value was not collected for redaction") + } + display := strings.Join(redactedCommandArgs(args), " ") + if strings.Contains(display, secret) { + t.Errorf("the target row prints the credential: %q", display) + } + if !strings.Contains(display, "X-Workspace-Id") { + t.Errorf("the header NAME was redacted too, which loses the diagnostic: %q", display) + } + raw := config.MCPServerConfig{Command: "mcp-server", Args: args} + if got := redactMCPFailureReason(errors.New("child echoed: "+strings.Join(args, " ")), raw, nil); strings.Contains(got, secret) { + t.Errorf("the credential reached the failure reason: %q", got) + } + }) + } + + // Lowercase -h is help and takes no value. Folding case on the short form + // would make it consume the next argument and blank an unrelated word. + if values := sensitiveMCPArgValues([]string{"server", "-h", "status"}); containsValue(values, "status") { + t.Errorf("-h consumed the following argument: %v", values) + } +} + +// SAFETY MUST BE MONOTONIC OVER AN OBSERVATION. +// +// A retained startup failure keeps the RAW error and is redacted again on every +// render from whatever the token store holds at that moment. Logging out of a +// server deletes the stored bearer and leaves the configuration unchanged, so +// the observation is retained while the candidate set that was hiding the +// bearer disappears, and the next render writes the credential into the panel +// and the transcript AFTER the user asked for it to be forgotten. +func TestRetainedFailureCannotBecomeLessRedacted(t *testing.T) { + const bearer = "stored-bearer-9f3c2b7ae1d8c4" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + }} + // An echo the generic patterns cannot catch: only the stored-token candidate + // set was ever hiding this. + failure := errors.New("upstream rejected the session; echoed credential was " + bearer) + + reasonFor := func(captured string) string { + t.Helper() + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcppkg.SkippedServer{{Name: "docs", Err: failure}}, + SkippedCredentials: captured, + }) + for _, server := range state.Servers { + if server.Name == "docs" { + return server.Error + } + } + t.Fatalf("the failed server is missing from the state") + return "" + } + + // Captured while the bearer existed; the store is empty now, as after logout. + guarded := reasonFor(mcpCredentialFingerprint([]string{bearer})) + if strings.Contains(guarded, bearer) { + t.Errorf("the retained failure became less redacted once the credential went away: %q", guarded) + } + if !strings.Contains(guarded, "startup failed") { + t.Errorf("the row stopped reporting the failure at all: %q", guarded) + } + + // An unchanged credential set is still rendered normally, or the guard would + // be suppressing every failure. + if unchanged := reasonFor(mcpCredentialFingerprint(nil)); strings.Contains(unchanged, "credentials changed") { + t.Errorf("an unchanged credential set was treated as stale: %q", unchanged) + } +} + +func TestCredentialFingerprintIsOrderIndependentAndUnambiguous(t *testing.T) { + if mcpCredentialFingerprint([]string{"a", "b"}) != mcpCredentialFingerprint([]string{"b", "a"}) { + t.Error("SecretValues enumerates a map, so the fingerprint must not depend on order") + } + if mcpCredentialFingerprint([]string{"ab", "c"}) == mcpCredentialFingerprint([]string{"a", "bc"}) { + t.Error("values must be length-delimited or two different sets collide") + } +} diff --git a/internal/tui/mcp_error_redaction_test.go b/internal/tui/mcp_error_redaction_test.go new file mode 100644 index 000000000..cf7d8a002 --- /dev/null +++ b/internal/tui/mcp_error_redaction_test.go @@ -0,0 +1,90 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// A FAILING SERVER CAN ECHO ITS OWN CONFIGURED CREDENTIAL BACK AT US. +// +// Remote MCP servers are configured with arbitrary headers, so a credential can +// sit under a name nobody can predict. The generic redaction patterns match +// shapes they recognise, and `X-Workspace-Credential` is not one of them. This +// PR newly renders the startup error in both /mcp surfaces and the session +// transcript, so an echoed value would be printed and persisted. +// +// Redacting by VALUE rather than by shape is what closes that, and it does not +// depend on the `Authorization:` matcher, which control bytes can split before +// the later sanitizer strips them. +func TestFailedServerErrorRedactsConfiguredHeaderValues(t *testing.T) { + const credential = "wk-live-4f9c2b7ae1d8" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "remote": { + Type: "http", + URL: "https://mcp.example.com", + Headers: map[string]string{"X-Workspace-Credential": credential}, + }, + }} + // What a server that returns the request it choked on looks like. + skipped := []mcp.SkippedServer{{ + Name: "remote", + Err: errors.New("startup failed: upstream rejected X-Workspace-Credential: " + credential), + }} + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + if len(state.Servers) != 1 { + t.Fatalf("expected one server view, got %d", len(state.Servers)) + } + if got := state.Servers[0].Error; strings.Contains(got, credential) { + t.Fatalf("the configured credential is rendered in the /mcp surface and the transcript: %q", got) + } + if state.Servers[0].State != "failed" { + t.Errorf("state = %q, want failed", state.Servers[0].State) + } +} + +// The same for a stdio server's environment, which a launch failure often +// reports back verbatim. +func TestFailedServerErrorRedactsConfiguredEnvValues(t *testing.T) { + const secret = "sk-env-9c1f2a6b40de" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "local": {Command: "server", Env: map[string]string{"COMPANY_TOKEN": secret}}, + }} + skipped := []mcp.SkippedServer{{Name: "local", Err: errors.New("exec failed with COMPANY_TOKEN=" + secret)}} + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + if got := state.Servers[0].Error; strings.Contains(got, secret) { + t.Fatalf("the configured env secret is rendered: %q", got) + } +} + +// The message must still SAY something. Redacting by equality can punch holes +// through unrelated text, so short configured values are skipped; without that +// a server configured with "1" would blank every digit in the error. +func TestShortConfiguredValuesDoNotShredTheMessage(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "remote": {Type: "http", URL: "https://mcp.example.com", Headers: map[string]string{"X-Retry": "1", "X-Mode": "true"}}, + }} + skipped := []mcp.SkippedServer{{Name: "remote", Err: errors.New("connection refused after 1 attempt in true isolation")}} + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + got := state.Servers[0].Error + if !strings.Contains(got, "connection refused") { + t.Fatalf("the error text was destroyed by redacting trivial configured values: %q", got) + } +} + +// A server that starts fine carries no error, so nothing is rendered at all. +func TestHealthyServerHasNoErrorText(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "remote": {Type: "http", URL: "https://mcp.example.com"}, + }} + state := BuildMCPViewState(MCPStateOptions{Config: cfg}) + if got := state.Servers[0].Error; got != "" { + t.Errorf("a healthy server reported %q", got) + } +} diff --git a/internal/tui/mcp_failed_state_test.go b/internal/tui/mcp_failed_state_test.go new file mode 100644 index 000000000..51b58a8fc --- /dev/null +++ b/internal/tui/mcp_failed_state_test.go @@ -0,0 +1,286 @@ +package tui + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// The panel has to report what is running, not what is written down. MCP +// registration is best-effort โ€” a server that fails to start is recorded and +// startup continues โ€” so without the skipped set the panel calls a server that +// never connected "enabled" and the user has no way to tell from here why its +// tools are missing. +func TestBuildMCPViewStateReportsServersThatFailedToStart(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + "linear": {Type: "http", URL: "https://linear.example/mcp"}, + "offline": {Type: "http", URL: "https://offline.example/mcp", Disabled: true}, + }} + skipped := []mcp.SkippedServer{ + {Name: "docs", Err: errors.New(`exec: "docs-mcp": executable file not found in $PATH`)}, + // Disabled servers are never started, so one should not appear here. + // Assert the precedence anyway: if it ever does, the user turned this + // server off and "failed" would be a lie. + {Name: "offline", Err: errors.New("should not be reported")}, + } + + state := BuildMCPViewState(MCPStateOptions{Config: cfg, Skipped: skipped}) + + byName := make(map[string]MCPServerView, len(state.Servers)) + for _, server := range state.Servers { + byName[server.Name] = server + } + if got := byName["docs"]; got.State != "failed" || + got.Error != `exec: "docs-mcp": executable file not found in $PATH` { + t.Errorf("failed server = %#v, want state \"failed\" carrying the recorded reason", got) + } + if got := byName["linear"]; got.State != "enabled" || got.Error != "" { + t.Errorf("healthy server = %#v, want it left as enabled with no error", got) + } + if got := byName["offline"]; got.State != "disabled" || got.Error != "" { + t.Errorf("disabled server = %#v, want disabled to win over a recorded failure", got) + } +} + +// The reason is rendered from an error the server produced, so it is untrusted +// text that can carry whatever the transport echoed back โ€” including the +// credential Zero sent it. +func TestBuildMCPViewStateRedactsTheFailureReason(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "linear": {Type: "http", URL: "https://linear.example/mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{ + Name: "linear", + Err: errors.New("handshake rejected: Authorization: Bearer sk-live-abcdef0123456789abcdef"), + }}, + }) + + reason := state.Servers[0].Error + if strings.Contains(reason, "sk-live-abcdef0123456789abcdef") { + t.Fatalf("failure reason leaked the bearer token: %q", reason) + } + if !strings.Contains(reason, "handshake rejected") { + t.Errorf("redaction ate the diagnostic part of the reason: %q", reason) + } +} + +// A nil or blank error still means the server is not running. "failed" with +// nothing after it reads like a rendering bug, so fall back to a plain +// statement rather than an empty line. +func TestBuildMCPViewStateFallsBackWhenTheFailureHasNoMessage(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }} + for name, err := range map[string]error{ + "nil error": nil, + "blank error": errors.New(" "), + } { + t.Run(name, func(t *testing.T) { + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: err}}, + }) + got := state.Servers[0] + if got.State != "failed" { + t.Fatalf("state = %q, want \"failed\" even without a message", got.State) + } + if strings.TrimSpace(got.Error) == "" { + t.Error("failed server rendered with no reason at all") + } + }) + } +} + +// The reason has to survive into the text the user actually reads. +func TestRenderMCPViewShowsTheFailureReason(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: errors.New("connection refused")}}, + }) + + rendered := renderMCPView(state, 100) + if !strings.Contains(rendered, "failed") { + t.Errorf("panel does not say the server failed:\n%s", rendered) + } + if !strings.Contains(rendered, "connection refused") { + t.Errorf("panel does not show why it failed:\n%s", rendered) + } + if strings.Contains(rendered, "docs ยท enabled") { + t.Errorf("panel still calls the failed server enabled:\n%s", rendered) + } +} + +// End to end through the model: what startup recorded is what /mcp reports. +// The wiring is the whole point โ€” the builder can be correct while the panel +// still renders from a set nobody handed it. +func TestModelMCPPanelReportsStartupFailures(t *testing.T) { + m := newModel(context.Background(), Options{ + MCPConfig: config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, + MCPSkipped: []mcp.SkippedServer{ + {Name: "docs", Err: errors.New("connection refused")}, + }, + }) + panel := m.mcpText() + if !strings.Contains(panel, "failed") || !strings.Contains(panel, "connection refused") { + t.Errorf("/mcp panel did not carry the startup failure through:\n%s", panel) + } +} + +// The failure reason is the one string on this panel an MCP server writes +// itself, and it lands in a terminal. redaction.ErrorMessage removes +// credentials, not control bytes, so a hostile handshake error can clear the +// screen, move the cursor, or forge a row that looks like another server. +// +// Payload is anandh8x's from the #835 review: an escape sequence and a newline +// carrying a line shaped exactly like a real entry. +func TestMCPFailureReasonCannotInjectTerminalControl(t *testing.T) { + hostile := "connection refused\x1b[2J\n\u203a forged \u00b7 enabled" + lines := mcpManagerServerLines([]MCPServerView{ + {Name: "evil", Transport: "http", State: "failed", Error: hostile}, + }) + + joined := strings.Join(lines, "\n") + if strings.ContainsRune(joined, '\x1b') { + t.Errorf("escape byte survived into the rendered panel:\n%q", joined) + } + for _, line := range lines { + if strings.ContainsAny(line, "\r\n") { + t.Errorf("a rendered line carries its own newline, so it occupies rows the panel did not count:\n%q", line) + } + } + // The forged text may appear as inert characters, but never as its own row: + // that is what makes it read as a second server. + for _, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), "\u203a forged") { + t.Errorf("hostile reason forged a server row: %q", line) + } + } + // The real reason must still reach the user; sanitizing must not blank it. + if !strings.Contains(joined, "connection refused") { + t.Errorf("the actual failure reason was lost:\n%q", joined) + } +} + +// A server that returns megabytes of error text must not push the rest of the +// panel off screen. +func TestMCPFailureReasonIsLengthCapped(t *testing.T) { + lines := mcpManagerServerLines([]MCPServerView{ + {Name: "verbose", Transport: "http", State: "failed", Error: strings.Repeat("x", 5000)}, + }) + for _, line := range lines { + if len(line) > 512 { + t.Errorf("rendered line is %d bytes, want it capped", len(line)) + } + } +} + +// The display cap alone cannot stop the walk: escape sequences are consumed +// without producing any visible output, so a server can spend unbounded input +// against a budget that never fills. Only a bound on the raw input ends it, and +// the panel re-renders this string on every redraw. +func TestMCPFailureReasonBoundsRawInput(t *testing.T) { + raw := strings.Repeat("\x1b[2J", maxMCPReasonRawLen) + "TAIL" + got := sanitizeTerminalReason(raw) + if strings.Contains(got, "TAIL") { + t.Fatalf("sanitizeTerminalReason walked past the raw bound and reached byte %d: %q", len(raw)-4, got) + } + if got != "" { + t.Fatalf("sanitizeTerminalReason(escape sequences only) = %q, want it to render nothing", got) + } +} + +// Cutting the raw input must not leave half of a multi-byte character behind: +// the panel would show a replacement character it invented itself. Escape +// sequences render as nothing, so they carry the cut far past what the display +// cap would have removed and leave the split character as the visible tail. +func TestMCPFailureReasonRawBoundKeepsRunesWhole(t *testing.T) { + const invisible = "\x1b[2J" + // The last character starts two bytes before the bound, so the cut keeps two + // of its three bytes. + prefix := strings.Repeat(invisible, (maxMCPReasonRawLen-2)/len(invisible)) + prefix += strings.Repeat("a", maxMCPReasonRawLen-2-len(prefix)) + raw := prefix + "\u203a" + if len(raw) <= maxMCPReasonRawLen { + t.Fatalf("test setup: raw input is %d bytes, it must straddle the %d byte bound", len(raw), maxMCPReasonRawLen) + } + + got := sanitizeTerminalReason(raw) + if strings.ContainsRune(got, '\ufffd') { + t.Fatalf("the raw bound split a rune: %q", got) + } + if got != "aa" { + t.Fatalf("sanitizeTerminalReason(...) = %q, want the whole characters before the cut", got) + } +} + +// THE BARE /mcp OVERLAY, which is the surface a user actually reaches. +// +// Empty /mcp routes to openMCPManager, and the overlay reported the state +// without the reason: the recorded "why" was rendered only by +// mcpManagerServerLines inside renderMCPView, which serves /mcp list and the +// transcript. So the panel said "failed" and stopped, exactly where someone +// goes to find out why after the startup warning has scrolled away. +// +// TestModelMCPPanelReportsStartupFailures drives m.mcpText() and passes either +// way, which is how the gap survived review. This one drives the overlay. +func TestBareMCPOverlayShowsTheFailureReason(t *testing.T) { + m := newModel(context.Background(), Options{ + MCPConfig: config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + }}, + MCPSkipped: []mcp.SkippedServer{ + {Name: "docs", Err: errors.New("connection refused")}, + }, + }) + overlay := m.openMCPManager().mcpManagerOverlay(100) + if overlay == "" { + t.Fatal("bare /mcp produced no manager overlay") + } + if !strings.Contains(overlay, "failed") { + t.Errorf("overlay does not report the failed state:\n%s", overlay) + } + if !strings.Contains(overlay, "connection refused") { + t.Errorf("overlay reports the state but not the reason, so the user still has to go looking:\n%s", overlay) + } +} + +// The overlay must sanitize the reason too. It renders into the same terminal +// as the transcript path, and a reason that is safe on one surface and raw on +// the other is a hole in whichever one was forgotten. +func TestBareMCPOverlaySanitizesTheFailureReason(t *testing.T) { + m := newModel(context.Background(), Options{ + MCPConfig: config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "evil": {Type: "stdio", Command: "evil-mcp"}, + }}, + MCPSkipped: []mcp.SkippedServer{ + {Name: "evil", Err: errors.New("refused\x1b[2J\n\u203a forged \u00b7 enabled")}, + }, + }) + overlay := m.openMCPManager().mcpManagerOverlay(100) + // NOT a bare search for \x1b: the overlay is lipgloss-styled, so it is full + // of escape sequences this code wrote itself. The question is whether the + // SERVER's bytes survived, so look for its payload specifically. + if strings.Contains(overlay, "[2J") { + t.Errorf("the clear-screen sequence from a server-authored reason reached the overlay:\n%q", overlay) + } + // The forged text may appear, inert, on the reason line. What must not happen + // is it becoming its own ROW, which is what the newline in the payload is + // for: a row shaped like a real entry, claiming another server is enabled. + for _, line := range strings.Split(overlay, "\n") { + if strings.Contains(line, "forged") && !strings.Contains(line, "refused") { + t.Errorf("a server-authored reason produced a standalone row:\n%q", line) + } + } +} diff --git a/internal/tui/mcp_failure_redaction_test.go b/internal/tui/mcp_failure_redaction_test.go new file mode 100644 index 000000000..3db7626f1 --- /dev/null +++ b/internal/tui/mcp_failure_redaction_test.go @@ -0,0 +1,425 @@ +package tui + +import ( + "errors" + "path/filepath" + "strings" + "testing" + "time" + "unicode" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// THE FAILURE REASON IS WRITTEN BY THE SERVER THAT FAILED. +// +// It is rendered into the panel and persisted into the transcript, so every case +// below is a hostile or careless server echoing back something Zero sent it. The +// assertions are made against the text after sanitizeTerminalReason, because +// that is what the reader sees, and because for the split-secret case it is +// precisely the sanitizer that reassembles the credential. + +// failedServerReason builds the state and returns the reason as it will be +// displayed. Asserting on the pre-sanitized field would miss the rejoin. +func failedServerReason(t *testing.T, cfg config.MCPConfig, name string, err error, tokenStore *mcp.TokenStore) string { + t.Helper() + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + TokenStore: tokenStore, + Skipped: []mcp.SkippedServer{{Name: name, Err: err}}, + }) + for _, server := range state.Servers { + if server.Name == name { + if server.State != "failed" { + t.Fatalf("server %q state = %q, want failed", name, server.State) + } + return sanitizeTerminalReason(server.Error) + } + } + t.Fatalf("server %q missing from the view state", name) + return "" +} + +// A configured credential split by a control byte must not be reassembled. +// +// Redaction matches the configured value literally, so a secret with a byte +// pushed into the middle of it matches nothing. The sanitizer then removes that +// byte WITHOUT leaving a gap and the two halves become adjacent again, which is +// how an unredacted credential reaches the screen and the transcript. +// +// Every splitter here is one the sanitizer drops, so each one rejoins. +func TestFailedServerReasonRedactsASecretSplitByControlBytes(t *testing.T) { + const secret = "wk-live-4f9c2b7ae1d8" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://docs.example/mcp", Headers: map[string]string{ + "Authorization": secret, + }}, + }} + + for _, testCase := range []struct { + name string + splitter string + }{ + {name: "CSI colour sequence", splitter: "\x1b[31m"}, + {name: "OSC sequence terminated by BEL", splitter: "\x1b]0;title\x07"}, + {name: "bare escape byte", splitter: "\x1b"}, + {name: "backspace", splitter: "\x08"}, + {name: "NUL", splitter: "\x00"}, + {name: "DEL", splitter: "\x7f"}, + {name: "C1 control", splitter: "ย"}, + } { + t.Run(testCase.name, func(t *testing.T) { + split := "wk-live-" + testCase.splitter + "4f9c2b7ae1d8" + reason := failedServerReason(t, cfg, "docs", + errors.New("handshake rejected, sent "+split), nil) + + if strings.Contains(reason, secret) { + t.Errorf("the credential was reassembled for display: %q", reason) + } + if !strings.Contains(reason, "handshake rejected") { + t.Errorf("redaction ate the diagnostic text: %q", reason) + } + }) + } +} + +// The counterpart: a splitter the sanitizer turns into a space does not rejoin, +// so the reason keeps both halves and neither of them is the credential. This +// pins that the fix did not simply delete every control byte and call that +// redaction. +func TestFailedServerReasonKeepsWhitespaceSplitTextReadable(t *testing.T) { + const secret = "wk-live-4f9c2b7ae1d8" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://docs.example/mcp", Headers: map[string]string{ + "Authorization": secret, + }}, + }} + + reason := failedServerReason(t, cfg, "docs", + errors.New("handshake rejected, sent wk-live-\n4f9c2b7ae1d8"), nil) + + if strings.Contains(reason, secret) { + t.Errorf("a newline-separated value was joined into the credential: %q", reason) + } + if !strings.Contains(reason, "handshake rejected") { + t.Errorf("redaction ate the diagnostic text: %q", reason) + } +} + +// A secret handed to a stdio child as a command-line argument must be redacted +// out of that child's own complaint about it. connectStdio appends the captured +// stderr to the initialization error, and a child that rejects its arguments +// habitually prints the invocation back. +func TestFailedServerReasonRedactsSensitiveStdioArgumentValues(t *testing.T) { + for _, testCase := range []struct { + name string + args []string + secret string + }{ + { + name: "value in the following argument", + args: []string{"serve", "--api-key", "arg-secret-abcdefgh"}, + secret: "arg-secret-abcdefgh", + }, + { + name: "value joined with equals", + args: []string{"serve", "--api-key=arg-secret-abcdefgh"}, + secret: "arg-secret-abcdefgh", + }, + { + // A value carrying "=" must survive the cut intact, or the redaction + // set holds a truncated string that matches nothing. + name: "base64 value with padding", + args: []string{"serve", "--token=YWJjZGVmZ2hpamts=="}, + secret: "YWJjZGVmZ2hpamts==", + }, + { + name: "flag and value packed into one argument", + args: []string{"serve", "--api-key arg-secret-abcdefgh"}, + secret: "arg-secret-abcdefgh", + }, + { + name: "password flag", + args: []string{"--password", "hunter2-abcdefgh"}, + secret: "hunter2-abcdefgh", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp", Args: testCase.args}, + }} + reason := failedServerReason(t, cfg, "docs", + errors.New("initialize failed: usage: docs-mcp "+ + strings.Join(testCase.args, " ")+": unknown flag"), nil) + + if strings.Contains(reason, testCase.secret) { + t.Errorf("the argument secret reached the panel: %q", reason) + } + if !strings.Contains(reason, "initialize failed") { + t.Errorf("redaction ate the diagnostic text: %q", reason) + } + }) + } +} + +// A sensitive flag in the last position has no value to redact and must not walk +// off the end of the arguments. +func TestSensitiveArgValuesToleratesATrailingFlag(t *testing.T) { + if got := sensitiveMCPArgValues([]string{"serve", "--api-key"}); len(got) != 0 { + t.Errorf("a trailing sensitive flag produced values %q", got) + } + // A blank between the flag and the secret must be skipped rather than + // consumed as the value, or the real secret in the next position is missed. + got := sensitiveMCPArgValues([]string{"--api-key", " ", "real-secret-abcdefgh"}) + if len(got) != 1 || got[0] != "real-secret-abcdefgh" { + t.Errorf("blank argument consumed as the value: %q", got) + } + // A flag following a flag is not that flag's value. + if got := sensitiveMCPArgValues([]string{"--api-key", "--verbose"}); len(got) != 0 { + t.Errorf("a following flag was collected as a secret: %q", got) + } +} + +// A stored OAuth bearer must be redacted out of a failed server's error. +// +// No pattern can recognize an opaque token by shape, so this can only work by +// value, and the value has to come from the token store. The token is saved +// IDENTITY-BOUND here because that is what the login path does: a redaction that +// looked the token up by server name would find nothing for exactly the servers +// that hold a real bearer, and a test that seeded the store by name would pass +// anyway. +func TestFailedOAuthServerReasonRedactsTheStoredBearerToken(t *testing.T) { + const access = "opaque-access-token-abcdefgh" + const refresh = "opaque-refresh-token-abcdefgh" + + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "linear": {Type: "http", URL: "https://linear.example/mcp", Auth: "oauth"}, + }} + store := newTestMCPTokenStore(t) + servers, err := mcp.NormalizeConfig(cfg) + if err != nil { + t.Fatalf("NormalizeConfig() error = %v", err) + } + if len(servers) != 1 { + t.Fatalf("NormalizeConfig() returned %d servers, want 1", len(servers)) + } + if err := store.SaveForServer(servers[0], mcp.StoredToken{ + AccessToken: access, + RefreshToken: refresh, + TokenType: "Bearer", + }); err != nil { + t.Fatalf("SaveForServer() error = %v", err) + } + + reason := failedServerReason(t, cfg, "linear", + errors.New(`jsonrpc error: {"error":"invalid_grant","presented":"`+access+ + `","refresh":"`+refresh+`"}`), store) + + if strings.Contains(reason, access) { + t.Errorf("the stored access token reached the panel: %q", reason) + } + if strings.Contains(reason, refresh) { + t.Errorf("the stored refresh token reached the panel: %q", reason) + } + if !strings.Contains(reason, "invalid_grant") { + t.Errorf("redaction ate the diagnostic text: %q", reason) + } +} + +// The token store is nil whenever its initialization soft-failed at startup, so +// the panel has to render rather than crash. That is a production state, not +// only a test one. +func TestFailedServerReasonToleratesAMissingTokenStore(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "linear": {Type: "http", URL: "https://linear.example/mcp", Auth: "oauth"}, + }} + reason := failedServerReason(t, cfg, "linear", errors.New("connection refused"), nil) + if !strings.Contains(reason, "connection refused") { + t.Errorf("reason = %q, want the underlying failure", reason) + } +} + +// A zero-width or otherwise invisible Unicode character rejoins exactly like a +// control byte, and more comfortably: the reader sees an unbroken credential +// while equality redaction saw two fragments. Adversarial review found these +// walking straight through the first version of the fix, which only knew about +// escapes and control bytes. +func TestFailedServerReasonRedactsASecretSplitByInvisibleUnicode(t *testing.T) { + const secret = "wk-live-4f9c2b7ae1d8" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://docs.example/mcp", Headers: map[string]string{ + "Authorization": secret, + }}, + }} + + for _, testCase := range []struct { + name string + splitter string + }{ + // Written as escapes on purpose: these are invisible, and a literal one + // in the source would be unreadable. The byte order mark is also a build + // error if it lands anywhere but the first byte of a Go file. + {name: "zero width space", splitter: string(rune(0x200b))}, + {name: "zero width non-joiner", splitter: string(rune(0x200c))}, + {name: "zero width joiner", splitter: string(rune(0x200d))}, + {name: "soft hyphen", splitter: string(rune(0x00ad))}, + {name: "word joiner", splitter: string(rune(0x2060))}, + {name: "left-to-right mark", splitter: string(rune(0x200e))}, + {name: "right-to-left override", splitter: string(rune(0x202e))}, + {name: "byte order mark", splitter: string(rune(0xfeff))}, + } { + t.Run(testCase.name, func(t *testing.T) { + reason := failedServerReason(t, cfg, "docs", + errors.New("handshake rejected, sent wk-live-"+testCase.splitter+"4f9c2b7ae1d8"), nil) + + // Assert on what the READER sees, not on the bytes. These characters + // render as nothing, so a reason still holding the two fragments + // shows an intact credential on screen while a substring check on the + // raw string happily reports the secret absent. Perceived text is + // modelled here rather than borrowed from production, so the test + // states the requirement instead of restating the implementation. + perceived := dropInvisible(reason) + if strings.Contains(perceived, secret) { + t.Errorf("the credential is intact on screen: rendered %q, perceived %q", reason, perceived) + } + }) + } +} + +// Combining marks must NOT be stripped. They are ordinary content in most of the +// world's scripts, and deleting them to close a redaction hole would corrupt +// every error message written in those languages. +func TestSanitizeTerminalReasonKeepsCombiningMarks(t *testing.T) { + // Devanagari "hindi" and a decomposed Latin e-acute. + const text = "เคธเคฐเฅเคตเคฐ เคตเคฟเคซเคฒ eฬchec" + if got := sanitizeTerminalReason(text); got != text { + t.Errorf("sanitizeTerminalReason mangled legitimate text:\n got %q\n want %q", got, text) + } +} + +// The configured value is usually not the credential. +// +// Zero's own documented config spells an authenticated server as +// `"Authorization": "Bearer "`, so a server that echoes back only the +// token, without the scheme word, matched nothing. This is the same literal-match +// trap that disabled redaction in providerhealth once "Bearer " was inlined into +// the configured value. +func TestFailedServerReasonRedactsACredentialEchoedWithoutItsSchemePrefix(t *testing.T) { + // Deliberately OPAQUE. An "sk-"-style value would be caught by the shape + // patterns in the redactor and the test would pass without the fix, proving + // only that the pattern list works. Equality is the only thing that can catch + // this string, which is the whole reason configured values are collected. + const credential = "kf7Qm2wz9Lp4Rt8vN1cX" + + for _, testCase := range []struct { + name string + configure func(*config.MCPServerConfig) + }{ + { + name: "scheme prefix in a configured header", + configure: func(server *config.MCPServerConfig) { + server.Headers = map[string]string{"Authorization": "Bearer " + credential} + }, + }, + { + name: "header name and scheme in a composite argument", + configure: func(server *config.MCPServerConfig) { + server.Args = []string{"mcp-remote", "--header", "Authorization: Bearer " + credential} + }, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + server := config.MCPServerConfig{Type: "stdio", Command: "mcp-remote"} + testCase.configure(&server) + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{"docs": server}} + + // The server quotes the credential alone, which is what an upstream + // that validated the token and rejected it actually reports. + reason := failedServerReason(t, cfg, "docs", + errors.New(`handshake rejected: {"error":"invalid_token","presented":"`+credential+`"}`), nil) + + if strings.Contains(reason, credential) { + t.Errorf("the credential reached the panel without its scheme prefix: %q", reason) + } + if !strings.Contains(reason, "invalid_token") { + t.Errorf("redaction ate the diagnostic text: %q", reason) + } + }) + } +} + +// The scheme word and the header name must NOT enter the redaction set, or every +// message mentioning them loses them. The length floor is what prevents it. +// dropInvisible removes the characters that render as nothing, modelling what a +// reader actually perceives on the terminal. +func dropInvisible(value string) string { + var out strings.Builder + for _, current := range value { + if unicode.Is(unicode.Cf, current) { + continue + } + out.WriteRune(current) + } + return out.String() +} + +func TestCredentialCandidatesDoesNotCollectSchemeWords(t *testing.T) { + got := credentialCandidates("Authorization: Bearer kf7Qm2wz9Lp4Rt8vN1cX") + for _, unwanted := range []string{"Bearer", "Authorization"} { + for _, candidate := range got { + if candidate == unwanted { + t.Errorf("candidate %q would blank a common word out of every message: %q", unwanted, got) + } + } + } + if len(got) == 0 { + t.Fatal("no candidates produced") + } +} + +// A POSITIONAL argument is not a flag and does not introduce a value. +// +// isSensitiveMCPDisplayFlag strips leading dashes before matching, so it says +// yes to a bare word too. The documented GitHub server config passes the env +// var NAME positionally, and reading it as a flag put the docker image name into +// the redaction set: the pull failure then lost the one string explaining it. +func TestSensitiveArgValuesIgnoresPositionalWordsThatLookSensitive(t *testing.T) { + args := []string{ + "run", "-i", "--rm", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", + "ghcr.io/github/github-mcp-server", + } + for _, got := range sensitiveMCPArgValues(args) { + if got == "ghcr.io/github/github-mcp-server" { + t.Errorf("the docker image name was collected as a secret: %q", sensitiveMCPArgValues(args)) + } + } + + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "github": {Type: "stdio", Command: "docker", Args: args}, + }} + reason := failedServerReason(t, cfg, "github", + errors.New("initialize failed: docker: Error response from daemon: pull access denied for ghcr.io/github/github-mcp-server"), nil) + if !strings.Contains(reason, "ghcr.io/github/github-mcp-server") { + t.Errorf("the failure lost the image name that explains it: %q", reason) + } +} + +// newTestMCPTokenStore builds a store backed by a temp file. Env is set +// explicitly because NewTokenStore otherwise reads ZERO_OAUTH_STORAGE from the +// real process environment, and a developer who has it set to the keyring would +// run this test against their own OS keychain. +func newTestMCPTokenStore(t *testing.T) *mcp.TokenStore { + t.Helper() + now := time.Date(2026, 8, 12, 12, 0, 0, 0, time.UTC) + store, err := mcp.NewTokenStore(mcp.TokenStoreOptions{ + FilePath: filepath.Join(t.TempDir(), "tokens.json"), + Env: map[string]string{}, + Now: func() time.Time { return now }, + }) + if err != nil { + t.Fatalf("NewTokenStore() error = %v", err) + } + return store +} diff --git a/internal/tui/mcp_header_flag_redaction_test.go b/internal/tui/mcp_header_flag_redaction_test.go new file mode 100644 index 000000000..510bdb58b --- /dev/null +++ b/internal/tui/mcp_header_flag_redaction_test.go @@ -0,0 +1,78 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// A CREDENTIAL BEHIND A HEADER FLAG IS STILL A CREDENTIAL. +// +// isSensitiveMCPDisplayKey classifies an argument by its FLAG NAME, matching +// token/secret/auth/credential and friends. "header" and "H" match none of them, +// so the whole header-flag family went uncollected while the credential rides in +// a value whose header name the operator chose. A stdio child that rejects its +// invocation echoes that invocation into captured stderr, which reaches +// SkippedServer.Err, and this panel renders it and the transcript keeps it. +func TestHeaderFlagValuesAreRedactedFromAFailureReason(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + + for _, testCase := range []struct { + name string + args []string + }{ + {name: "long separated", args: []string{"--header", "X-Workspace-Id: " + token}}, + {name: "long equals", args: []string{"--header=X-Workspace-Id: " + token}}, + {name: "long packed", args: []string{"--header X-Workspace-Id: " + token}}, + {name: "short separated", args: []string{"-H", "X-Workspace-Id: " + token}}, + {name: "short equals", args: []string{"-H=X-Workspace-Id: " + token}}, + {name: "short packed", args: []string{"-H X-Workspace-Id: " + token}}, + {name: "mixed case long flag", args: []string{"--Header", "X-Workspace-Id: " + token}}, + } { + t.Run(testCase.name, func(t *testing.T) { + raw := config.MCPServerConfig{Command: "mcp-server", Args: testCase.args} + // The child echoes its own invocation back, which is the everyday shape. + failure := errors.New("mcp-server: unrecognized option\ninvocation: mcp-server " + strings.Join(testCase.args, " ")) + + got := redactMCPFailureReason(failure, raw, nil) + if strings.Contains(got, token) { + t.Errorf("the credential survived into the rendered failure, which is also persisted to the transcript:\n%s", got) + } + if !strings.Contains(got, mcpDisplayRedacted) { + t.Errorf("nothing was redacted at all, so the value never reached the candidate set:\n%s", got) + } + }) + } +} + +// -h IS NOT -H. Help takes no value, so folding case on the short flag would set +// pending on `-h` and put the following argument into the redaction set, blanking +// an unrelated word out of every message that mentions it. Over-collection has +// already cost this panel a readable docker image name once, so the narrow +// behaviour is asserted rather than left to chance. +func TestShortHelpFlagDoesNotClaimTheNextArgumentAsASecret(t *testing.T) { + const image = "ghcr.io/github/github-mcp-server" + raw := config.MCPServerConfig{Command: "docker", Args: []string{"-h", image}} + + got := redactMCPFailureReason(errors.New("failed to pull "+image), raw, nil) + if !strings.Contains(got, image) { + t.Errorf("the image name was redacted because -h was read as a header flag, so the error no longer explains itself:\n%s", got) + } +} + +// And the header NAME must survive. Redacting it would blank the one token that +// tells the operator which header was rejected. +func TestHeaderNameSurvivesWhileItsValueIsRedacted(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + raw := config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header", "X-Workspace-Id: " + token}} + + got := redactMCPFailureReason(errors.New("rejected header X-Workspace-Id: "+token), raw, nil) + if strings.Contains(got, token) { + t.Fatalf("credential survived:\n%s", got) + } + if !strings.Contains(got, "X-Workspace-Id") { + t.Errorf("the header name was redacted too, so the message no longer says which header was rejected:\n%s", got) + } +} diff --git a/internal/tui/mcp_manager.go b/internal/tui/mcp_manager.go index 3be6f84f2..8ea1a3768 100644 --- a/internal/tui/mcp_manager.go +++ b/internal/tui/mcp_manager.go @@ -30,8 +30,13 @@ const ( ) type mcpManagerItem struct { - Kind mcpManagerItemKind - Name string + Kind mcpManagerItemKind + // Name is the display label. It is NOT an identity: two config entries whose + // keys differ only by padding render under the same name. + Name string + // ConfigKey addresses the configuration map. Every action and every lookup + // uses it, because Name cannot distinguish the rows. + ConfigKey string Label string Meta string Detail string @@ -137,19 +142,19 @@ func (m model) handleMCPManagerKey(msg tea.KeyMsg) (model, tea.Cmd) { return m.runMCPManagerCommand([]string{"list"}) case "c": if item, ok := m.currentMCPManagerItem(); ok && item.Kind == mcpManagerItemServer { - return m.runMCPManagerCommand([]string{"check", item.Name}) + return m.runMCPManagerCommand([]string{"check", item.ConfigKey}) } case "d": if item, ok := m.currentMCPManagerItem(); ok && item.Kind == mcpManagerItemServer { - return m.runMCPManagerCommand([]string{"disable", item.Name}) + return m.runMCPManagerCommand([]string{"disable", item.ConfigKey}) } case "e": if item, ok := m.currentMCPManagerItem(); ok && item.Kind == mcpManagerItemServer { - return m.runMCPManagerCommand([]string{"enable", item.Name}) + return m.runMCPManagerCommand([]string{"enable", item.ConfigKey}) } case "r": if item, ok := m.currentMCPManagerItem(); ok && item.Kind == mcpManagerItemServer { - return m.runMCPManagerCommand([]string{"remove", item.Name}) + return m.runMCPManagerCommand([]string{"remove", item.ConfigKey}) } } } @@ -197,7 +202,7 @@ func (m model) chooseMCPManagerItem() (model, tea.Cmd) { } switch item.Kind { case mcpManagerItemServer: - return m.runMCPManagerCommand([]string{"check", item.Name}) + return m.runMCPManagerCommand([]string{"check", item.ConfigKey}) case mcpManagerItemMarketplace: return m.prefillMCPManagerCommand(item.InstallCommand), nil case mcpManagerItemAddRemote: @@ -253,11 +258,12 @@ func (m model) mcpManagerItems() []mcpManagerItem { name := displayValue(strings.TrimSpace(server.Name), "unnamed") installed[strings.ToLower(name)] = true item := mcpManagerItem{ - Kind: mcpManagerItemServer, - Name: name, - Label: name, - Meta: mcpManagerServerMeta(server), - Detail: strings.Join([]string{name, server.Transport, server.State, server.Auth, server.Target}, " "), + Kind: mcpManagerItemServer, + Name: name, + ConfigKey: server.ConfigKey, + Label: name, + Meta: mcpManagerServerMeta(server), + Detail: strings.Join([]string{name, server.Transport, server.State, server.Auth, server.Target}, " "), } if mcpManagerItemMatches(item, query) { items = append(items, item) @@ -426,13 +432,25 @@ func (m model) mcpManagerSelectionDetail(width int) []string { } switch item.Kind { case mcpManagerItemServer: - server, ok := m.mcpManagerServer(item.Name) + server, ok := m.mcpManagerServer(item.ConfigKey) if !ok { return nil } lines := []string{ fillPaletteLine(zeroTheme.ink.Bold(true).Render(server.Name)+" "+zeroTheme.faint.Render(server.Transport+" ยท "+server.State), width, transparentSurface), } + // Directly under the header, above the target, and through the same + // sanitizer mcpManagerServerLines uses. + // + // This overlay is what a bare /mcp opens, so it is the first place a user + // goes after a startup warning has scrolled away. It reported the state + // and stopped: "failed" on its own sends the reader to check their config + // when the answer is usually in the error, which is the whole point of + // recording it. The transcript path showed the reason and this one did + // not, so the fix only reached the surface people were not looking at. + if reason := sanitizeTerminalReason(server.Error); reason != "" { + lines = append(lines, fitStyledLine(zeroTheme.faint.Render(reason), width)) + } if target := strings.TrimSpace(server.Target); target != "" { lines = append(lines, fitStyledLine(zeroTheme.faint.Render(target), width)) } @@ -473,9 +491,15 @@ func firstMCPMarketplaceDetailLine(item mcpManagerItem) string { return detail } -func (m model) mcpManagerServer(name string) (MCPServerView, bool) { +// mcpManagerServer selects a row by its CONFIG KEY, not its display name. +// +// Two entries whose keys differ only by padding render under one canonical name, +// and ValidateUniqueNames deliberately accepts that when one of them is +// disabled. Matching on the name returned whichever came first, so a detail +// pane or an action chosen on the disabled row could land on the enabled one. +func (m model) mcpManagerServer(configKey string) (MCPServerView, bool) { for _, server := range m.mcpViewState().Servers { - if server.Name == name { + if server.ConfigKey == configKey { return server, true } } diff --git a/internal/tui/mcp_manager_identity_test.go b/internal/tui/mcp_manager_identity_test.go new file mode 100644 index 000000000..50f64f5f3 --- /dev/null +++ b/internal/tui/mcp_manager_identity_test.go @@ -0,0 +1,62 @@ +package tui + +import ( + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// A DISPLAY NAME IS NOT AN IDENTITY. +// +// The canonical runtime name is trimmed, which is correct for display, for +// joining against skipped-failure records, and for tool counts. It is not +// unique: ValidateUniqueNames deliberately accepts an enabled "docs" beside a +// disabled " docs", because a disabled entry claims no runtime identity. Both +// rows then render as "docs". +// +// Actions address the configuration map by exact key, so carrying only the +// trimmed name meant a padded single key sent every action to a key that does +// not exist, and with an alias pair an action chosen on one row could be +// dispatched against the other. +func TestManagerRowsCarryTheirExactConfigKey(t *testing.T) { + t.Run("a padded single key keeps its exact spelling", func(t *testing.T) { + views := buildMCPServerViews(config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + " docs ": {Type: "stdio", Command: "docs-mcp"}, + }}, nil, nil, nil, "") + if len(views) != 1 { + t.Fatalf("expected one view, got %d", len(views)) + } + if views[0].Name != "docs" { + t.Errorf("display name = %q, want the canonical %q", views[0].Name, "docs") + } + if views[0].ConfigKey != " docs " { + t.Errorf("config key = %q, want the exact map key", views[0].ConfigKey) + } + }) + + t.Run("an enabled entry and its disabled alias stay distinguishable", func(t *testing.T) { + views := buildMCPServerViews(config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "stdio", Command: "docs-mcp"}, + " docs": {Type: "stdio", Command: "docs-mcp", Disabled: true}, + }}, nil, nil, nil, "") + if len(views) != 2 { + t.Fatalf("expected two views, got %d", len(views)) + } + keys := map[string]bool{} + for _, view := range views { + if view.Name != "docs" { + t.Errorf("display name = %q, want %q for both rows", view.Name, "docs") + } + if view.ConfigKey == "" { + t.Fatal("a row carries no config key, so no action can address it") + } + if keys[view.ConfigKey] { + t.Fatalf("two rows share the config key %q, so they cannot be told apart", view.ConfigKey) + } + keys[view.ConfigKey] = true + } + if !keys["docs"] || !keys[" docs"] { + t.Errorf("config keys = %v, want both exact spellings", keys) + } + }) +} diff --git a/internal/tui/mcp_observation_context_test.go b/internal/tui/mcp_observation_context_test.go new file mode 100644 index 000000000..5c8b6ffc2 --- /dev/null +++ b/internal/tui/mcp_observation_context_test.go @@ -0,0 +1,166 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + mcppkg "github.com/Gitlawb/zero/internal/mcp" +) + +func mcpViewFor(t *testing.T, state MCPViewState, name string) MCPServerView { + t.Helper() + for _, server := range state.Servers { + if server.Name == name { + return server + } + } + t.Fatalf("server %q is missing from the panel: %#v", name, state.Servers) + return MCPServerView{} +} + +// AN OBSERVATION'S OWN CONTEXT OUTRANKS THE ONE SAMPLED AT THE SURFACE. +// +// The surface samples the token store when it is built, which is after startup +// registered these failures and after everything that runs in between could +// have rotated the store. Comparing against that sample asks "has anything +// changed since I started rendering", which is always no, instead of "is the +// material that made this error safe still here". +// +// The visible consequence: the raw error quotes a bearer that was refreshed +// away during startup, the surface-level sample matches the current store +// exactly, the row is rendered, and the redaction candidates no longer contain +// the bearer that is sitting in the text. +func TestTheObservationsOwnCredentialContextDecidesStaleness(t *testing.T) { + const rotatedAway = "bearer-rotated-away-9f3c2b7ae1" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + }} + // Only the stored-token candidate set was ever hiding this value; no generic + // pattern can recognise it. + failure := errors.New("upstream rejected the session; echoed credential was " + rotatedAway) + + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcppkg.SkippedServer{{ + Name: "docs", + Err: failure, + // Recorded at registration, while the bearer still existed. + Credentials: mcppkg.CredentialFingerprint([]string{rotatedAway}), + }}, + // Sampled when this surface was built: the store is already empty, so this + // agrees with the current state and claims nothing changed. + SkippedCredentials: mcppkg.CredentialFingerprint(nil), + }) + + docs := mcpViewFor(t, state, "docs") + if strings.Contains(docs.Error, rotatedAway) { + t.Errorf("the credential reached the panel because staleness was judged against the surface's own sample: %q", docs.Error) + } + if docs.State != "failed" { + t.Errorf("State = %q, want the failure still reported", docs.State) + } +} + +// And an observation whose context is unchanged still renders normally, or the +// per-observation check would just be suppressing everything. +func TestAnUnchangedObservationContextStillRenders(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcppkg.SkippedServer{{ + Name: "docs", + Err: errors.New("dial tcp 10.0.0.5:443: connect: connection refused"), + Credentials: mcppkg.CredentialFingerprint(nil), + }}, + SkippedCredentials: mcppkg.CredentialFingerprint(nil), + }) + if got := mcpViewFor(t, state, "docs").Error; !strings.Contains(got, "connection refused") { + t.Errorf("an observation with an unchanged context lost its reason: %q", got) + } +} + +// An observation that recorded nothing falls back to the surface's sample, +// which is what every caller that has not been taught to record one still gets. +func TestAnObservationWithoutContextFallsBackToTheSurfaceSample(t *testing.T) { + const bearer = "stored-bearer-9f3c2b7ae1d8c4" + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + }} + state := BuildMCPViewState(MCPStateOptions{ + Config: cfg, + Skipped: []mcppkg.SkippedServer{{Name: "docs", Err: errors.New("echoed " + bearer)}}, + SkippedCredentials: mcpCredentialFingerprint([]string{bearer}), + }) + if got := mcpViewFor(t, state, "docs").Error; strings.Contains(got, bearer) { + t.Errorf("the fallback stopped guarding an observation that recorded no context: %q", got) + } +} + +// A BACKGROUND REGISTRATION'S FAILURES ARE STILL THIS PANEL'S SUBJECT. +// +// Optional servers are moved off the critical path so a slow one cannot delay +// the first response. That is a scheduling decision, not a visibility one: they +// are ordinary configured rows. Their failures land after this surface exists, +// so a snapshot taken at construction cannot carry them, and the rows render +// from configuration alone -- reporting a server that never connected as +// enabled, which is the single thing the panel exists to prevent. +func TestLateFailuresReachThePanelAndInvalidateTheCache(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp"}, + "optional": {URL: "https://optional.invalid/mcp"}, + }} + var late []mcppkg.SkippedServer + m := &model{ + mcpConfig: cfg, + mcpStartupConfig: cfg, + mcpSkipped: []mcppkg.SkippedServer{{Name: "docs", Err: errors.New("docs refused")}}, + mcpLateSkipped: func() []mcppkg.SkippedServer { return late }, + } + + // Rendered before the background registration finished: the row is honest + // about what is known, and the cache is now warm. + if got := mcpViewFor(t, m.mcpViewState(), "optional").State; got != "enabled" { + t.Fatalf("State = %q before the background result, want enabled", got) + } + + late = []mcppkg.SkippedServer{{Name: "optional", Err: errors.New("optional server refused the connection")}} + + optional := mcpViewFor(t, m.mcpViewState(), "optional") + if optional.State != "failed" { + t.Errorf("State = %q, want failed: the background registration reported it as skipped", optional.State) + } + if !strings.Contains(optional.Error, "refused the connection") { + t.Errorf("the reason did not reach the panel: %q", optional.Error) + } + // The failure known at startup is untouched. + if got := mcpViewFor(t, m.mcpViewState(), "docs").State; got != "failed" { + t.Errorf("the startup failure was lost by the merge: State = %q", got) + } +} + +// A late observation is aged against the configuration it was made under, +// exactly as the startup snapshot is: replacing the server it is about leaves it +// describing something that no longer exists. +func TestALateFailureIsDroppedWhenItsSubjectIsReplaced(t *testing.T) { + startup := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "optional": {URL: "https://optional.invalid/mcp"}, + }} + replaced := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "optional": {URL: "https://different.invalid/mcp"}, + }} + m := &model{ + mcpConfig: replaced, + mcpStartupConfig: startup, + mcpLateSkipped: func() []mcppkg.SkippedServer { + return []mcppkg.SkippedServer{{Name: "optional", Err: errors.New("the old endpoint refused")}} + }, + } + optional := mcpViewFor(t, m.mcpViewState(), "optional") + if optional.State == "failed" { + t.Errorf("the replacement inherited the dead endpoint's failure: %q", optional.Error) + } +} diff --git a/internal/tui/mcp_oversized_secret_test.go b/internal/tui/mcp_oversized_secret_test.go new file mode 100644 index 000000000..543679ea3 --- /dev/null +++ b/internal/tui/mcp_oversized_secret_test.go @@ -0,0 +1,89 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// A CREDENTIAL LONGER THAN THE OVERLAP MUST NOT LEAVE A VISIBLE PREFIX. +// +// The bound cuts the raw error before redaction, and redaction matches whole +// values, so a secret the cut sliced in half matches nothing and its surviving +// prefix is ordinary text. The fixed overlap makes that require a secret longer +// than the window, and nothing caps a configured header, URL, environment or +// stored token value, so it is a configuration away rather than impossible. +// +// The server also controls what comes before it. Enough control sequences to +// spend the raw budget produce no visible text of their own, so the credential's +// prefix becomes the first thing a reader sees. +func TestAnOversizedCredentialLeavesNoVisiblePrefix(t *testing.T) { + // Longer than maxMCPSecretMatchWindow, which is the case the overlap alone + // cannot cover, and OPAQUE. A recognisable shape like "sk-live-..." is caught + // by the generic patterns whatever the bound does, so it would test nothing + // here: this has to exercise the exact-value path. + secret := strings.Repeat("Qw7ZmPr4", 700) + if len(secret) <= maxMCPSecretMatchWindow { + t.Fatalf("the fixture secret is %d bytes, which does not exceed the %d-byte window", len(secret), maxMCPSecretMatchWindow) + } + + raw := config.MCPServerConfig{ + URL: "https://host.invalid/mcp", + Headers: map[string]string{"X-Workspace": secret}, + } + + // Padding that consumes the raw budget while rendering nothing, so the + // credential starts just before the cut. + padding := strings.Repeat("\x1b[2K", maxMCPReasonRawLen/4) + got := redactMCPFailureReason(errors.New(padding+secret), raw, nil) + + for size := len(secret); size >= shortestMCPSecret; size /= 2 { + if strings.Contains(got, secret[:size]) { + t.Fatalf("a %d-character prefix of the credential survived into the panel:\n%.200q", size, got) + } + } +} + +// Through the assembled state as well, since that is what the panel and the +// transcript actually carry. +func TestAnOversizedCredentialLeavesNoPrefixInTheAssembledState(t *testing.T) { + secret := strings.Repeat("Qw7ZmPr4", 700) + padding := strings.Repeat("\x1b[2K", maxMCPReasonRawLen/4) + + state := BuildMCPViewState(MCPStateOptions{ + Config: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "docs": {URL: "https://host.invalid/mcp", Headers: map[string]string{"X-Workspace": secret}}, + }, + }, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: errors.New(padding + secret)}}, + }) + + for _, server := range state.Servers { + if server.Name != "docs" { + continue + } + rendered := server.Error + "\n" + server.Target + for size := len(secret); size >= shortestMCPSecret; size /= 2 { + if strings.Contains(rendered, secret[:size]) { + t.Fatalf("a %d-character prefix reached the panel:\n%.200q", size, rendered) + } + } + return + } + t.Fatal("the failed server is missing from the assembled state") +} + +// And an ordinary failure that merely ENDS with something secret-shaped is not +// eaten. The tail trim only runs when the bound actually truncated. +func TestAnUntruncatedFailureKeepsItsTail(t *testing.T) { + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp"} + message := "dial tcp 10.0.0.5:443: connect: connection refused" + got := redactMCPFailureReason(errors.New(message), raw, nil) + if !strings.Contains(got, message) { + t.Errorf("an ordinary failure lost its tail:\n%s", got) + } +} diff --git a/internal/tui/mcp_path_credential_test.go b/internal/tui/mcp_path_credential_test.go new file mode 100644 index 000000000..3a7902c5a --- /dev/null +++ b/internal/tui/mcp_path_credential_test.go @@ -0,0 +1,80 @@ +package tui + +import ( + "errors" + "net/url" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// AN OPAQUE PATH SEGMENT IS CREDENTIAL MATERIAL. +// +// Query values and userinfo were treated as secret-bearing while the path was +// assumed to be an identifier, and the configuration contract accepts an +// arbitrary HTTP or SSE path. Opaque path-segment credentials are an ordinary +// endpoint convention. +// +// This needs no crafted response body. A failing http.Client.Do returns a +// *url.Error carrying the request URL, which the failed-server path wraps and +// renders, so the token reaches the Error field, the panel and the transcript, +// with the Target row showing it as well. +func TestAnOpaquePathSegmentIsRedactedEverywhere(t *testing.T) { + const token = "9f3c2b7ae1d84c6fa0b5" + endpoint := "https://host.invalid/mcp/" + token + "/sse" + + // The shape a real connection failure produces. + failure := &url.Error{Op: "Post", URL: endpoint, Err: errors.New("dial tcp: connection refused")} + + state := BuildMCPViewState(MCPStateOptions{ + Config: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{"docs": {URL: endpoint, Type: "sse"}}, + }, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: failure}}, + }) + + for _, server := range state.Servers { + if server.Name != "docs" { + continue + } + if strings.Contains(server.Error, token) { + t.Errorf("the path credential reached the failure reason:\n%s", server.Error) + } + if strings.Contains(server.Target, token) { + t.Errorf("the path credential reached the target row:\n%s", server.Target) + } + // The route shape survives, or the operator cannot tell which endpoint + // failed. + if !strings.Contains(server.Target, "host.invalid") { + t.Errorf("the target lost the host, so the diagnostic is gone too: %s", server.Target) + } + return + } + t.Fatal("the failed server is missing from the assembled state") +} + +// Short route segments are structure, not secrets, and must survive. Redacting +// them would eat the diagnostic while protecting nothing. +func TestOrdinaryRouteSegmentsSurvive(t *testing.T) { + endpoint := "https://host.invalid/v1/sse" + state := BuildMCPViewState(MCPStateOptions{ + Config: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{"docs": {URL: endpoint, Type: "sse"}}, + }, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: errors.New("connection refused")}}, + }) + for _, server := range state.Servers { + if server.Name != "docs" { + continue + } + for _, want := range []string{"v1", "sse"} { + if !strings.Contains(server.Target, want) { + t.Errorf("the route segment %q was redacted out of %s", want, server.Target) + } + } + return + } + t.Fatal("the failed server is missing from the assembled state") +} diff --git a/internal/tui/mcp_provenance_test.go b/internal/tui/mcp_provenance_test.go new file mode 100644 index 000000000..c55bb11d5 --- /dev/null +++ b/internal/tui/mcp_provenance_test.go @@ -0,0 +1,134 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// THE KEY IS WHAT REMOVES THE AMBIGUITY, SO IT HAS TO SURVIVE TO THE DECISION. +// +// credentialCandidates drops values under shortestMCPSecret so ordinary +// configuration such as mode=sse or v=1 stays readable in an unrelated message. +// Header, environment and query values were flattened into bare strings before +// that heuristic ran, so a six-byte credential under a key that names it as one +// was discarded and never became an exact candidate. A child echoing the value +// on its own then reached the panel and the transcript, where generic shape +// matching has nothing left to recognise. +func TestShortValuesUnderSensitiveKeysAreRedacted(t *testing.T) { + const short = "s3cr3t" // six bytes, under the floor + + for _, testCase := range []struct { + name string + raw config.MCPServerConfig + }{ + {"environment key", config.MCPServerConfig{Command: "server", Env: map[string]string{"API_KEY": short}}}, + {"header key", config.MCPServerConfig{URL: "https://host.invalid/mcp", Headers: map[string]string{"X-Api-Key": short}}}, + {"query key", config.MCPServerConfig{URL: "https://host.invalid/mcp?api_key=" + short}}, + {"userinfo password", config.MCPServerConfig{URL: "https://user:" + short + "@host.invalid/mcp"}}, + } { + t.Run(testCase.name, func(t *testing.T) { + // A value-only echo: the sensitive key/value syntax is not present in + // the message, so only an exact candidate can catch it. + got := redactMCPFailureReason(errors.New("upstream echoed "+short), testCase.raw, nil) + if strings.Contains(got, short) { + t.Errorf("a short credential under a key that names it reached the panel: %q", got) + } + }) + } +} + +// And the floor still protects ordinary configuration, or the fix is just +// blanket over-redaction with a different justification. +func TestShortValuesUnderOrdinaryKeysStayReadable(t *testing.T) { + for _, testCase := range []struct { + name string + raw config.MCPServerConfig + message string + keep string + }{ + { + "ordinary query parameters", + config.MCPServerConfig{URL: "https://host.invalid/mcp?mode=sse&v=1"}, + "transport mode=sse rejected", "mode=sse", + }, + { + "ordinary environment value", + config.MCPServerConfig{Command: "server", Env: map[string]string{"LOG_LEVEL": "warn"}}, + "child started with log level warn", "warn", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + if got := redactMCPFailureReason(errors.New(testCase.message), testCase.raw, nil); !strings.Contains(got, testCase.keep) { + t.Errorf("an ordinary short value was redacted out of the message: %q", got) + } + }) + } +} + +// A PUBLIC MODE SELECTOR IS NOT CREDENTIAL MATERIAL. +// +// raw.Auth holds the authentication MODE, and normalization accepts only the +// value "oauth", which the panel displays as ordinary metadata. Classifying it +// by the security-sounding name of its field removed the one token that says +// which subsystem failed, from every error the OAuth stack produces. It was +// invisible while ambiguous values ran through the length floor, which +// discarded a five-character string on its own. +func TestTheOAuthModeSelectorStaysReadable(t *testing.T) { + raw := config.MCPServerConfig{ + URL: "https://host.invalid/mcp", + Auth: "oauth", + OAuth: &config.MCPOAuthConfig{ClientSecret: "s3cr3t"}, + } + for _, message := range []string{ + "oauth: fetch authorization server metadata", + "oauth discovery failed", + } { + got := redactMCPFailureReason(errors.New(message), raw, nil) + if !strings.Contains(got, "oauth") { + t.Errorf("the subsystem name was redacted out of %q: %q", message, got) + } + } + // The half that must not weaken: a real secret in the same config is still + // removed, at a length the floor would have discarded. + if got := redactMCPFailureReason(errors.New("token endpoint replied error_description=s3cr3t"), raw, nil); strings.Contains(got, "s3cr3t") { + t.Errorf("the client secret reached the panel: %q", got) + } +} + +// ONE PARSER FOR CLASSIFICATION AND FOR RENDERING. +// +// The collector split a flag packed with its value; the display did not. It +// recognised the element as sensitive, printed it verbatim, and then redacted +// the NEXT argument, so the Target row carried the credential and blanked an +// unrelated flag. That row sits directly under the reason, on both /mcp +// surfaces, and is persisted. +func TestPackedSensitiveArgumentsAreRedactedInTheTargetRow(t *testing.T) { + const secret = "sk-live-9f3c2b7ae1d8c4" + for _, testCase := range []struct { + name string + args []string + }{ + {"packed with a space", []string{"--api-key " + secret, "--verbose"}}, + {"separate elements", []string{"--api-key", secret, "--verbose"}}, + {"joined with equals", []string{"--api-key=" + secret, "--verbose"}}, + } { + t.Run(testCase.name, func(t *testing.T) { + rendered := strings.Join(redactedCommandArgs(testCase.args), " ") + if strings.Contains(rendered, secret) { + t.Errorf("the Target row prints the credential: %q", rendered) + } + if !strings.Contains(rendered, "--api-key") { + t.Errorf("the flag name was redacted too, losing which credential was rejected: %q", rendered) + } + if !strings.Contains(rendered, "--verbose") { + t.Errorf("an unrelated argument was redacted instead of the value: %q", rendered) + } + if !containsValue(sensitiveMCPArgValues(testCase.args), secret) { + t.Errorf("the collector did not classify the value as sensitive: %v", sensitiveMCPArgValues(testCase.args)) + } + }) + } +} diff --git a/internal/tui/mcp_raw_bound_test.go b/internal/tui/mcp_raw_bound_test.go new file mode 100644 index 000000000..818d38387 --- /dev/null +++ b/internal/tui/mcp_raw_bound_test.go @@ -0,0 +1,80 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// THE BOUND IS AT INGRESS, NOT AT THE END. +// +// The cap lived only in sanitizeTerminalReason, so the entire server-controlled +// string was redacted, walked rune by rune into a fresh builder and a fresh +// []rune by stripTerminalRejoiners, redacted again, and only then cut, for a +// panel that shows at most 400 runes. The existing raw-bound test calls the +// sanitizer directly, so it never exercised this path. +func TestOversizedFailureIsBoundedBeforeRedactionAndNormalization(t *testing.T) { + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp"} + huge := strings.Repeat("A", 8*1024*1024) + + got := redactMCPFailureReason(errors.New("tool name conflict: "+huge), raw, nil) + if budget := maxMCPReasonRawLen + maxMCPSecretMatchWindow; len(got) > budget { + t.Errorf("the failure reason is %d bytes against a budget of %d; the pipeline is still carrying the whole server-controlled value", len(got), budget) + } +} + +// AND THE BUDGET IS FIXED, not a function of what the other side configured. +// +// The first version kept a lookahead margin sized to the LONGEST SECRET, which +// made the real limit "the cap plus whatever the largest configured value +// happens to be". Configured values and the stored token enumeration have no +// size limit, so a two-megabyte credential raised the retained error to 65546 +// bytes against a nominal cap of 16384. A bound the other side can widen is not +// a bound. +func TestAnOversizedSecretCannotWidenTheFailureBudget(t *testing.T) { + huge := strings.Repeat("s", 2*1024*1024) + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + huge} + + got := redactMCPFailureReason(errors.New("conflict: "+strings.Repeat("B", 64*1024)), raw, nil) + budget := maxMCPReasonRawLen + maxMCPSecretMatchWindow + if len(got) > budget { + t.Errorf("a %d-byte configured secret widened the retained error to %d bytes against a fixed budget of %d", len(huge), len(got), budget) + } +} + +// A SECRET THAT STRADDLES THE CUT MUST NOT LEAVE A VISIBLE PREFIX. Slicing at +// exactly the cap would truncate the credential, and the surviving prefix would +// then match nothing and print, so the bound would have created the leak it has +// nothing to do with. +func TestASecretStraddlingTheBoundIsStillFullyRedacted(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + token} + + // Position the token so it begins just inside the cap and ends past it. + filler := strings.Repeat("A", maxMCPReasonRawLen-len(token)/2) + got := redactMCPFailureReason(errors.New(filler+token+strings.Repeat("B", 4096)), raw, nil) + + if strings.Contains(got, token) { + t.Fatalf("the whole token survived") + } + // Any prefix of the token longer than a few characters is a leak. + for size := len(token); size > 8; size-- { + if strings.Contains(got, token[:size]) { + t.Errorf("a %d-character prefix of the credential survived the bound: %q", size, token[:size]) + break + } + } +} + +// And an ordinary short failure is untouched, or the bound would be quietly +// eating normal diagnostics. +func TestOrdinaryFailureIsNotTruncatedByTheBound(t *testing.T) { + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp"} + message := "dial tcp 10.0.0.5:443: connect: connection refused" + got := redactMCPFailureReason(errors.New(message), raw, nil) + if !strings.Contains(got, message) { + t.Errorf("an ordinary failure was altered by the bound:\n%s", got) + } +} diff --git a/internal/tui/mcp_redaction_ignorable_test.go b/internal/tui/mcp_redaction_ignorable_test.go new file mode 100644 index 000000000..ac653f51f --- /dev/null +++ b/internal/tui/mcp_redaction_ignorable_test.go @@ -0,0 +1,79 @@ +package tui + +import ( + "errors" + "strings" + "testing" + "unicode" + + "github.com/Gitlawb/zero/internal/config" +) + +// readerVisible is what a terminal actually shows: default-ignorable code points +// render as nothing, so a secret split by one is contiguous to the eye. The +// assertion has to be made against this rather than against the raw string, +// because the raw string is exactly where the secret looks split. +func readerVisible(value string) string { + return strings.Map(func(r rune) rune { + if unicode.Is(unicode.Cf, r) || + unicode.Is(unicode.Other_Default_Ignorable_Code_Point, r) || + unicode.Is(unicode.Variation_Selector, r) { + return -1 + } + return r + }, value) +} + +// A configured credential split by a default-ignorable MARK must not reach +// either MCP surface. Cf characters were already covered; these are Mn, which +// the category-preserving second pass deliberately keeps. +func TestMCPFailureRedactsSecretsSplitByDefaultIgnorableMarks(t *testing.T) { + const secret = "wk-live-4f9c2b7ae1d8" + raw := config.MCPServerConfig{Env: map[string]string{"TOKEN": secret}} + + for _, splitter := range []struct { + name string + r rune + }{ + {"combining grapheme joiner", 0x034F}, + {"variation selector 16", 0xFE0F}, + {"zero width space", 0x200B}, + } { + split := "wk-live-" + string(splitter.r) + "4f9c2b7ae1d8" + got := redactMCPFailureReason(errors.New("startup failed: "+split), raw, nil) + if strings.Contains(readerVisible(got), secret) { + t.Errorf("%s: the reader-visible failure message still contains the credential: %q", splitter.name, got) + } + } +} + +// A stored OAuth token is a credential by provenance, not by length. The +// readability floor belongs to ambiguous configuration strings only. +func TestMCPFailureRedactsShortStoredTokens(t *testing.T) { + for _, token := range []string{"a1b2c3", "x", "short"} { + got := redactMCPFailureReason( + errors.New(`server rejected the handshake: {"echoed":"`+token+`"}`), + config.MCPServerConfig{}, + []string{token}, + ) + if strings.Contains(got, token) { + t.Errorf("stored token %q survived redaction: %q", token, got) + } + } +} + +// The ignorable drop must not widen into the Mn category: ordinary combining +// marks are content, and eating them would corrupt the diagnostic this pass +// exists to display. +func TestTerminalRejoinerStrippingKeepsOrdinaryCombiningMarks(t *testing.T) { + for _, value := range []string{ + "cafeฬ could not start", // e + combining acute + "naiฬˆve endpoint", // combining diaeresis + "straรŸe refused the handshake", // sharp s + "ู…ุฑุญุจุง", // arabic + } { + if got := stripTerminalRejoiners(value); got != value { + t.Errorf("legitimate text was altered: %q -> %q", value, got) + } + } +} diff --git a/internal/tui/mcp_secret_match_bound_test.go b/internal/tui/mcp_secret_match_bound_test.go new file mode 100644 index 000000000..88fe1f6a1 --- /dev/null +++ b/internal/tui/mcp_secret_match_bound_test.go @@ -0,0 +1,105 @@ +package tui + +import ( + "runtime" + "strings" + "testing" +) + +// referenceLongestPrefixSuffix is the definition, written the slow obvious way: +// the longest prefix of pattern that is also a suffix of text. +func referenceLongestPrefixSuffix(pattern, text string) int { + limit := len(pattern) + if len(text) < limit { + limit = len(text) + } + for size := limit; size > 0; size-- { + if strings.HasSuffix(text, pattern[:size]) { + return size + } + } + return 0 +} + +// THE BOUND MUST NOT CHANGE ANY ANSWER. +// +// The work is now capped by the text rather than the candidate, on the argument +// that a prefix longer than the text cannot be a suffix of it. This checks that +// argument against the definition rather than trusting it. +func TestSecretMatchBoundPreservesEveryAnswer(t *testing.T) { + patterns := []string{ + "", "a", "ab", "abc", "secret-token-value", + strings.Repeat("ab", 40), + "tok_" + strings.Repeat("z", 200), + strings.Repeat("x", 5000), + } + texts := []string{ + "", "a", "ab", "xyz", + "error: connecting with secret-token-value", + "error: connecting with secret-token-va", + "...trailing ab", + "abababababab", + strings.Repeat("ab", 30), + strings.Repeat("x", 100), + "prefix " + strings.Repeat("x", 4999), + } + for _, pattern := range patterns { + for _, text := range texts { + got := longestPrefixSuffix(pattern, text) + want := referenceLongestPrefixSuffix(pattern, text) + if got != want { + t.Errorf("longestPrefixSuffix(len %d, len %d) = %d, want %d", len(pattern), len(text), got, want) + } + } + } +} + +func allocatedMiB(fn func()) float64 { + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + fn() + runtime.ReadMemStats(&after) + return float64(after.TotalAlloc-before.TotalAlloc) / (1 << 20) +} + +// AND THE COST MUST NOT BE SET BY THE CONFIGURED VALUE. +// +// dropTrailingSecretPrefix runs on every /mcp state rebuild, once per configured +// or stored credential. longestPrefixSuffix built pattern+sentinel+text and an +// []int over the whole thing, so the candidate's length drove allocation: +// headers, env values, URL components, OAuth fields and token-store values have +// no size limit, and three 2 MiB candidates measured 54.5 MiB and 33ms against a +// render budget that is nominally fixed. +func TestSecretMatchWorkDoesNotScaleWithTheConfiguredSecret(t *testing.T) { + rendered := strings.Repeat("x", 16<<10) + // Built OUTSIDE the measured closure, or strings.Repeat itself dominates the + // measurement and hides what the function does. + smallSecret := []string{strings.Repeat("a", 64<<10)} + largeSecret := []string{strings.Repeat("a", 4<<20)} + small := allocatedMiB(func() { + _ = dropTrailingSecretPrefix(rendered, smallSecret) + }) + large := allocatedMiB(func() { + _ = dropTrailingSecretPrefix(rendered, largeSecret) + }) + // A 64x longer candidate must not cost meaningfully more. Generous, because + // this is a scaling assertion and not a fixed-size one. + if large > small+1 { + t.Errorf("a 4 MiB candidate allocated %.1f MiB against %.1f MiB for a 64 KiB one; the configured value is still sizing the work", large, small) + } +} + +// The suppression itself still works, or the bound above is satisfied by never +// matching anything. +func TestTrailingSecretPrefixIsStillDropped(t *testing.T) { + const secret = "sk-live-abcdefghijklmnopqrstuvwxyz" + rendered := "dial failed for " + secret[:20] + got := dropTrailingSecretPrefix(rendered, []string{secret}) + if strings.Contains(got, secret[:20]) { + t.Errorf("the partial credential survived: %q", got) + } + if !strings.HasPrefix(got, "dial failed for") { + t.Errorf("the message body was lost: %q", got) + } +} diff --git a/internal/tui/mcp_skipped_disabled_alias_test.go b/internal/tui/mcp_skipped_disabled_alias_test.go new file mode 100644 index 000000000..fb4f42557 --- /dev/null +++ b/internal/tui/mcp_skipped_disabled_alias_test.go @@ -0,0 +1,84 @@ +package tui + +import ( + "errors" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +func docsSkipped() []mcp.SkippedServer { + return []mcp.SkippedServer{{Name: "docs", Err: errors.New("connect timed out")}} +} + +// A DISABLED ALIAS MUST NOT DECIDE WHETHER THE ENABLED SERVER'S FAILURE SURVIVES. +// +// ValidateUniqueNames deliberately accepts an enabled "docs" alongside a disabled +// " docs": a disabled entry claims no runtime identity. canonicalMCPServers +// copied both into a map keyed by the trimmed name, so they collided, and Go +// randomises map iteration. The before and after snapshots each picked a winner +// independently, so an unrelated /mcp operation that left the enabled entry +// untouched could compare it against the disabled alias, find them different, and +// discard the failure. A server that is still unavailable was then reported as +// fine, on roughly one run in five. +// +// Looped because a single run passed most of the time even with the defect. +func TestDisabledAliasDoesNotAgeOutTheEnabledServersFailure(t *testing.T) { + withAlias := func() config.MCPConfig { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://real.example.com"}, + " docs": {Type: "http", URL: "https://disabled.example.com", Disabled: true}, + }} + } + for run := 0; run < 400; run++ { + kept := retainedMCPSkipped(docsSkipped(), withAlias(), withAlias()) + if len(kept) != 1 { + t.Fatalf("run %d: the enabled server's failure was discarded on an unchanged config, so /mcp reports a still-unavailable server as fine", run) + } + } +} + +// And the aging still works, or the fix above is satisfied by never dropping. +func TestRetainedMCPSkippedStillAgesOutAReplacedServer(t *testing.T) { + before := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://real.example.com"}, + }} + after := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://replacement.example.com"}, + }} + if kept := retainedMCPSkipped(docsSkipped(), before, after); len(kept) != 0 { + t.Errorf("a replaced endpoint inherited the old one's failure: %#v", kept) + } +} + +// Disabling the server that failed ages the observation out too: registration +// would not have run it, so there is no longer a running subject to describe. +func TestRetainedMCPSkippedDropsAFailureForADisabledServer(t *testing.T) { + before := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://real.example.com"}, + }} + after := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://real.example.com", Disabled: true}, + }} + if kept := retainedMCPSkipped(docsSkipped(), before, after); len(kept) != 0 { + t.Errorf("a disabled server kept a startup failure it can no longer have: %#v", kept) + } +} + +// Two ENABLED entries claiming one canonical name is ambiguous, not arbitrary. +// ValidateUniqueNames rejects that config so it should not arrive here, but if it +// does there is no way to say which entry the observation was about. +func TestRetainedMCPSkippedDropsAnAmbiguousCanonicalName(t *testing.T) { + ambiguous := func() config.MCPConfig { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "docs": {Type: "http", URL: "https://one.example.com"}, + " docs": {Type: "http", URL: "https://two.example.com"}, + }} + } + for run := 0; run < 100; run++ { + if kept := retainedMCPSkipped(docsSkipped(), ambiguous(), ambiguous()); len(kept) != 0 { + t.Fatalf("run %d: an observation was attributed to one of two enabled entries sharing a canonical name: %#v", run, kept) + } + } +} diff --git a/internal/tui/mcp_skipped_invalidation.go b/internal/tui/mcp_skipped_invalidation.go new file mode 100644 index 000000000..de6839219 --- /dev/null +++ b/internal/tui/mcp_skipped_invalidation.go @@ -0,0 +1,104 @@ +package tui + +import ( + "reflect" + "strings" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// adoptMCPConfig replaces the in-session MCP config and drops the startup +// failures that are no longer about anything. +// +// A SKIPPED ENTRY IS AN OBSERVATION ABOUT A PARTICULAR SERVER, NOT ABOUT A NAME. +// m.mcpSkipped is written once, at construction, from the startup registration. +// Every successful /mcp command and the add wizard replaced m.mcpConfig and +// refreshed the view while leaving that snapshot untouched, and failures are +// matched by name alone. So removing a failed "docs" endpoint and adding a +// different URL under the same name made the replacement inherit the dead +// endpoint's error and its failed state, and that false result was written into +// the command transcript as well as the panel. +// +// Nothing in the TUI re-registers MCP tools, so the replacement is not running +// either. That does not make "failed" honest: it is reported as having failed +// for a reason belonging to a server that no longer exists, which sends the +// operator to debug the wrong thing. Dropping the observation leaves the row +// reporting configuration, which is what it can actually know. +func (m model) adoptMCPConfig(next config.MCPConfig) model { + m.mcpSkipped = retainedMCPSkipped(m.mcpSkipped, m.mcpConfig, next) + m.mcpConfig = next + return m +} + +// retainedMCPSkipped keeps only the observations whose subject survived the +// change unaltered. +func retainedMCPSkipped(skipped []mcp.SkippedServer, previous config.MCPConfig, next config.MCPConfig) []mcp.SkippedServer { + if len(skipped) == 0 { + return skipped + } + before := canonicalMCPServers(previous) + after := canonicalMCPServers(next) + kept := make([]mcp.SkippedServer, 0, len(skipped)) + for _, entry := range skipped { + name := strings.TrimSpace(entry.Name) + current, present := after[name] + if !present { + // Removed. The observation has no subject any more. + continue + } + original, existed := before[name] + if !existed { + // The name was not configured when this snapshot was taken, so whatever + // is there now is not what failed. + continue + } + // Compared on the WHOLE config rather than on a rendered target, because + // the rendered form is redacted: two different credentials print + // identically, and a rotated token is a different server for this purpose. + if !reflect.DeepEqual(original, current) { + continue + } + kept = append(kept, entry) + } + return kept +} + +// canonicalMCPServers keys the configured servers the way registration does, so +// a padded config key and the trimmed name recorded in a SkippedServer refer to +// the same entry. +// +// DISABLED ENTRIES ARE EXCLUDED, because registration excludes them +// (NormalizeConfig skips raw.Disabled) and an observation is about a server that +// actually ran. ValidateUniqueNames deliberately accepts an enabled "docs" +// alongside a disabled " docs" for that reason. Copying both into a map keyed by +// the trimmed name made them collide, and Go randomises map iteration, so the +// before and after snapshots each picked a winner independently. An unrelated +// /mcp operation that left the enabled entry untouched could then compare it +// against the disabled alias, find them different, and discard the failure: a +// server that is still unavailable reported as fine, on some runs and not others. +// Measured at 20% of runs on an unchanged config before this. +// +// A canonical name claimed by two ENABLED entries is ambiguous rather than +// arbitrary. ValidateUniqueNames rejects that config so it should not arrive +// here, but if it does there is no way to say which entry an observation was +// about, so the name is dropped and the observation ages out with it. +func canonicalMCPServers(cfg config.MCPConfig) map[string]config.MCPServerConfig { + servers := make(map[string]config.MCPServerConfig, len(cfg.Servers)) + ambiguous := make(map[string]struct{}) + for name, server := range cfg.Servers { + if server.Disabled { + continue + } + canonical := strings.TrimSpace(name) + if _, clash := servers[canonical]; clash { + ambiguous[canonical] = struct{}{} + continue + } + servers[canonical] = server + } + for canonical := range ambiguous { + delete(servers, canonical) + } + return servers +} diff --git a/internal/tui/mcp_skipped_invalidation_test.go b/internal/tui/mcp_skipped_invalidation_test.go new file mode 100644 index 000000000..24db840d4 --- /dev/null +++ b/internal/tui/mcp_skipped_invalidation_test.go @@ -0,0 +1,91 @@ +package tui + +import ( + "errors" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +func mcpConfigWith(name string, server config.MCPServerConfig) config.MCPConfig { + return config.MCPConfig{Servers: map[string]config.MCPServerConfig{name: server}} +} + +// A REPLACEMENT DOES NOT INHERIT ITS PREDECESSOR'S FAILURE. +// +// m.mcpSkipped is the startup snapshot and failures are matched by name, so +// removing a failed "docs" endpoint and adding a different one under the same +// name made the new entry render as failed, carrying an error about a server +// that no longer exists, and wrote that into the command transcript too. +func TestReplacedServerDoesNotInheritTheStartupFailure(t *testing.T) { + failed := mcp.SkippedServer{Name: "docs", Err: errors.New("dial tcp: connection refused")} + + for _, testCase := range []struct { + name string + next config.MCPConfig + want bool // want the failure retained + }{ + { + name: "same server untouched", + next: mcpConfigWith("docs", config.MCPServerConfig{URL: "https://old.invalid/mcp"}), + want: true, + }, + { + name: "replaced with a different url", + next: mcpConfigWith("docs", config.MCPServerConfig{URL: "https://new.invalid/mcp"}), + want: false, + }, + { + name: "replaced with a stdio command", + next: mcpConfigWith("docs", config.MCPServerConfig{Command: "docs-mcp"}), + want: false, + }, + { + name: "removed entirely", + next: config.MCPConfig{Servers: map[string]config.MCPServerConfig{}}, + want: false, + }, + { + name: "rotated credential is a different server for this purpose", + next: mcpConfigWith("docs", config.MCPServerConfig{URL: "https://old.invalid/mcp", Headers: map[string]string{"Authorization": "Bearer new"}}), + want: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + m := model{ + mcpConfig: mcpConfigWith("docs", config.MCPServerConfig{URL: "https://old.invalid/mcp"}), + mcpSkipped: []mcp.SkippedServer{failed}, + } + m = m.adoptMCPConfig(testCase.next) + + retained := len(m.mcpSkipped) == 1 + if retained != testCase.want { + t.Fatalf("failure retained = %v, want %v", retained, testCase.want) + } + + state := BuildMCPViewState(MCPStateOptions{Config: m.mcpConfig, Skipped: m.mcpSkipped}) + for _, server := range state.Servers { + if server.State == "failed" && !testCase.want { + t.Errorf("the replacement renders as failed carrying %q, an error about a server that no longer exists", server.Error) + } + if server.State != "failed" && testCase.want { + t.Errorf("an untouched failed server stopped reporting its failure") + } + } + }) + } +} + +// A padded config key and the trimmed name in the snapshot are the same server, +// so the observation must still be found when deciding whether to keep it. +func TestSkippedInvalidationMatchesTheCanonicalName(t *testing.T) { + m := model{ + mcpConfig: mcpConfigWith(" docs ", config.MCPServerConfig{URL: "https://old.invalid/mcp"}), + mcpSkipped: []mcp.SkippedServer{{Name: "docs", Err: errors.New("refused")}}, + } + m = m.adoptMCPConfig(mcpConfigWith(" docs ", config.MCPServerConfig{URL: "https://old.invalid/mcp"})) + if len(m.mcpSkipped) != 1 { + t.Errorf("an untouched failure was dropped because the config key was padded") + } +} diff --git a/internal/tui/mcp_startup_completion_test.go b/internal/tui/mcp_startup_completion_test.go new file mode 100644 index 000000000..28bcadbdf --- /dev/null +++ b/internal/tui/mcp_startup_completion_test.go @@ -0,0 +1,118 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/config" + mcppkg "github.com/Gitlawb/zero/internal/mcp" +) + +// AN ALREADY-OPEN MANAGER HAS TO BE TOLD, NOT ASKED. +// +// Optional MCP registration runs on its own goroutine so a slow server cannot +// delay the first response, which means its result lands with no user input +// behind it. Bubble Tea renders only in response to a message, so a getter that +// reports late failures is not enough on its own: an overlay opened while a +// server was still connecting keeps rendering the configuration-derived enabled +// state until unrelated input or a resize happens to redraw it. +func TestAnOpenManagerRebuildsWhenBackgroundStartupCompletes(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "optional": {URL: "https://optional.invalid/mcp"}, + }} + var late []mcppkg.SkippedServer + done := make(chan struct{}) + + m := &model{ + width: 120, + mcpConfig: cfg, + mcpStartupConfig: cfg, + mcpManager: &mcpManagerState{}, + mcpLateSkipped: func() []mcppkg.SkippedServer { return late }, + mcpStartupCompleted: done, + } + + // Opened while the background registration is still in flight. + before := m.mcpManagerOverlay(m.width) + if !strings.Contains(before, "optional") { + t.Fatalf("SETUP INVALID: the manager does not list the server:\n%s", before) + } + if strings.Contains(strings.ToLower(before), "failed") { + t.Fatalf("SETUP INVALID: it already reads as failed before the result arrived:\n%s", before) + } + + // The background registration finishes and reports the failure. No key, no + // resize, no command: only the completion. + late = []mcppkg.SkippedServer{{Name: "optional", Err: errors.New("optional server refused the connection")}} + close(done) + + updated, _ := m.Update(mcpStartupCompletedMsg{}) + next, ok := updated.(model) + if !ok { + t.Fatalf("Update returned %T, want a model", updated) + } + + // Asserted on the CACHE, not by rendering. Rendering calls mcpViewState, + // which invalidates on demand, so an overlay drawn after the message would + // look right even if the handler did nothing: the test would pass for the + // wrong reason and stop pinning the handler at all. The cache being fresh + // BEFORE anything renders is what says the completion was consumed. + var cached string + for _, server := range next.mcpViewStateCache.Servers { + if server.Name == "optional" { + cached = server.State + "|" + server.Error + } + } + if !strings.Contains(cached, "failed") { + t.Errorf("the completion did not rebuild the view state: %q", cached) + } + if !strings.Contains(cached, "refused the connection") { + t.Errorf("the rebuilt state carries no reason: %q", cached) + } + + after := next.mcpManagerOverlay(next.width) + if !strings.Contains(strings.ToLower(after), "failed") { + t.Errorf("the open manager still shows the configured state after the background failure:\n%s", after) + } + if !strings.Contains(after, "refused the connection") { + t.Errorf("the reason never reached the open manager:\n%s", after) + } +} + +// And Init actually schedules the wait, or the message above would never be +// produced in a real session. +func TestInitWaitsForBackgroundStartupCompletion(t *testing.T) { + done := make(chan struct{}) + m := model{mcpStartupCompleted: done} + close(done) + + cmd := m.Init() + if cmd == nil { + t.Fatal("Init scheduled nothing") + } + if !producesMCPStartupCompletion(cmd) { + t.Error("Init never scheduled the wait for background MCP startup, so a completed registration reaches no open surface") + } +} + +// producesMCPStartupCompletion runs a batched command tree far enough to see +// whether the completion wait is part of it. +func producesMCPStartupCompletion(cmd tea.Cmd) bool { + if cmd == nil { + return false + } + switch msg := cmd().(type) { + case mcpStartupCompletedMsg: + return true + case tea.BatchMsg: + for _, sub := range msg { + if producesMCPStartupCompletion(sub) { + return true + } + } + } + return false +} diff --git a/internal/tui/mcp_state.go b/internal/tui/mcp_state.go index f675102d3..a8d5deff3 100644 --- a/internal/tui/mcp_state.go +++ b/internal/tui/mcp_state.go @@ -1,13 +1,16 @@ package tui import ( + "errors" "net/url" "regexp" "sort" "strings" + "unicode/utf8" "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/redaction" "github.com/Gitlawb/zero/internal/tools" ) @@ -19,6 +22,17 @@ type MCPStateOptions struct { PermissionMode string PromptCount int DeniedCount int + // Skipped are the servers registration could not start. Registration is + // best-effort so one unreachable server cannot stop Zero launching, which + // means a failure is recorded here rather than returned. Without it this + // panel reports configuration instead of reality. + Skipped []mcp.SkippedServer + // SkippedCredentials fingerprints the credential material that existed when + // Skipped was captured. See mcpCredentialFingerprint: an observation retains + // a RAW error, and redaction happens at render, so the safety of fixed text + // would otherwise depend on mutable state read later. Empty means the caller + // did not record one, and the check is skipped. + SkippedCredentials string } type mcpServerNamedTool interface { @@ -30,6 +44,12 @@ const mcpDisplayRedacted = "[REDACTED]" var mcpStateUnsafeToolNameChars = regexp.MustCompile(`[^A-Za-z0-9_]+`) +// shortestMCPSecret is the floor for treating a configured value as a credential +// to strike out of an error message. A configured "1" or "true" is not a secret, +// and redacting it by equality would punch holes through unrelated text, which is +// its own way of making an error useless. +const shortestMCPSecret = 8 + func BuildMCPViewState(options MCPStateOptions) MCPViewState { toolViews := buildMCPToolViews(options.Config, options.Registry) toolCounts := make(map[string]int, len(toolViews)) @@ -38,34 +58,232 @@ func BuildMCPViewState(options MCPStateOptions) MCPViewState { } return MCPViewState{ - Servers: buildMCPServerViews(options.Config, toolCounts), + Servers: buildMCPServerViews(options.Config, toolCounts, options.Skipped, options.TokenStore, options.SkippedCredentials), Tools: toolViews, Permissions: buildMCPPermissionSummary(options), OAuth: buildMCPOAuthSummary(options.Config, options.TokenStore), } } -func buildMCPServerViews(cfg config.MCPConfig, toolCounts map[string]int) []MCPServerView { +func buildMCPServerViews(cfg config.MCPConfig, toolCounts map[string]int, skipped []mcp.SkippedServer, tokenStore *mcp.TokenStore, capturedCredentials string) []MCPServerView { + // The whole observation, not just its error: each one carries the credential + // context that was sampled where its error was produced, and that is what the + // staleness check has to compare against. + failures := make(map[string]mcp.SkippedServer, len(skipped)) + for _, entry := range skipped { + failures[entry.Name] = entry + } + // Read the stored bearers ONCE. Every load re-reads and re-parses the whole + // store file, and the material is the same for every row anyway. nil is the + // normal case rather than a test-only one: startup soft-fails the token store + // to nil with a warning, and SecretValues is nil-safe for exactly that. + tokenSecrets := tokenStore.SecretValues() names := sortedMCPServerNames(cfg) servers := make([]MCPServerView, 0, len(names)) - for _, name := range names { - raw := cfg.Servers[name] + for _, rawName := range names { + raw := cfg.Servers[rawName] + // ONE IDENTITY, and it is the registry's. mcp.normalizeServer trims the + // config-map key before anything downstream sees it, so a server + // configured as " docs " is registered, recorded in SkippedServer.Name, and + // counted in toolCounts as "docs". This loop was iterating the RAW map keys + // and looking failures up with them, so failures[" docs "] missed and the + // entry rendered as " docs " enabled: a server that never started, shown as + // running, which is the one thing this panel exists to prevent. The tool + // count was lost the same way. + // + // Trimmed here to match normalizeServer exactly. The canonical spelling is + // also what gets displayed, because that is the name the server actually + // has everywhere else in the process. + name := strings.TrimSpace(rawName) state := "enabled" - if raw.Disabled { + message := "" + switch { + case raw.Disabled: + // Disabled wins: the user turned it off, so it was never expected to + // connect and reporting it as failed would be misleading. state = "disabled" + default: + if failure, ok := failures[name]; ok { + state = "failed" + message = redactMCPFailureReason(failure.Err, raw, tokenSecrets) + // The observation's OWN context wins. capturedCredentials is sampled + // when this surface is built, which is after registration and after + // anything that ran in between could have rotated the store, so it + // only stands in for observations that recorded nothing. + captured := failure.Credentials + if captured == "" { + captured = capturedCredentials + } + if staleMCPObservation(captured, tokenSecrets) { + message = mcpStaleObservationReason + } + + if strings.TrimSpace(message) == "" { + message = "server did not start" + } + } } servers = append(servers, MCPServerView{ Name: name, + ConfigKey: rawName, Transport: mcpServerTransport(raw), State: state, Target: mcpServerTarget(raw), Auth: strings.TrimSpace(raw.Auth), ToolCount: toolCounts[name], + Error: message, }) } return servers } +// redactMCPFailureReason turns a failed server's error into text safe to render +// AND to persist, since the panel's output goes into the transcript. +// +// The second pass is the point. Redaction matches a configured value literally, +// and the reason is written by the server, which is free to insert a byte into +// the middle of a credential it echoes back: `wk-live-\x1b[31m4f9c2b7ae1d8` is +// not equal to the configured `wk-live-4f9c2b7ae1d8`, so the first pass sees +// nothing to redact. The display sanitizer downstream then removes the escape +// WITHOUT leaving a gap, reassembling the intact credential on screen. Every +// control byte it drops rejoins the same way, so this is not specific to ANSI. +// +// So the text is normalized to what the reader will actually see, and redaction +// is run again against that. Normalizing before the first pass instead would +// work equally well; running it after keeps redaction.ErrorMessage's handling of +// a nil or wrapped error exactly as it was. +// +// The stored bearer goes in alongside the configured values because a failed +// OAuth server can echo the token in its error body, and no pattern can +// recognize an opaque token by shape. +func redactMCPFailureReason(err error, raw config.MCPServerConfig, tokenSecrets []string) string { + secrets := mcpServerSecretValues(raw) + // NO READABILITY FLOOR ON KNOWN CREDENTIAL MATERIAL. The floor exists for + // ambiguous configuration strings, where a short value is more likely to be a + // hostname fragment than a secret and blanket redaction would eat the + // diagnostic. A stored access token, refresh token or client secret is not + // ambiguous: it is a credential by provenance, whatever its length. OAuth + // bearer syntax is opaque, the store accepts any non-empty value, and a failed + // server can echo a six-character token under an arbitrary field name where no + // pattern will recognise it. + for _, value := range tokenSecrets { + if trimmed := strings.TrimSpace(value); trimmed != "" { + secrets = append(secrets, trimmed) + } + } + options := redaction.Options{ExtraSecretValues: secrets} + // BOUNDED AT THE RAW INGRESS, which is inside ErrorMessage rather than around + // it. See boundMCPFailureError: wrapping the outside of that call left the + // full server-controlled value going through every redaction pass first, so + // the work scaled with the attacker's input instead of with the cap. + bounded, truncated := boundMCPFailureError(err) + rendered := redaction.RedactString(stripTerminalRejoiners(redaction.ErrorMessage(bounded, options)), options) + // AND THE CUT ITSELF CAN MANUFACTURE A LEAK. Everything above matches whole + // values, so a credential the bound sliced in half matches nothing and its + // surviving prefix is ordinary text. The fixed overlap makes that need a + // secret longer than the window, and nothing caps a configured header, URL, + // environment or stored token value, so "longer than the window" is a + // configuration away rather than impossible. A server can also spend the raw + // budget on control sequences that later vanish, putting the start of the + // credential right at the cut. + // + // Only the TAIL can be a partial, because only the final cut splits anything, + // so this looks at the tail alone and drops any run that is a prefix of a + // configured secret. It costs one comparison per secret against a bounded + // window and does not care how long the secret is, which is the property the + // overlap could not give. + if truncated { + rendered = dropTrailingSecretPrefix(rendered, secrets) + } + return rendered +} + +// dropTrailingSecretPrefix removes a trailing run that is the beginning of a +// configured secret. +// +// Applied to the FINAL text, after both redaction passes and after the terminal +// rejoiners are gone, because that is the string a reader sees and the only one +// whose tail is the real tail. +func dropTrailingSecretPrefix(rendered string, secrets []string) string { + cut := len(rendered) + for _, secret := range secrets { + size := longestPrefixSuffix(secret, rendered) + if !recoverableSecretPrefix(size, len(secret)) { + continue + } + if start := len(rendered) - size; start < cut { + cut = start + } + } + if cut == len(rendered) { + return rendered + } + return strings.TrimRight(rendered[:cut], " ") +} + +// maxMCPSecretMatchWindow is the FIXED overlap kept past the display cap so a +// configured credential straddling the cut is still matched whole. +// +// Fixed, deliberately. The previous version sized it to the longest secret, +// which made the real budget "the cap plus whatever the largest configured value +// happens to be". Configured values and the stored token enumeration have no +// size limit, so a two-megabyte credential raised the retained error to 65546 +// bytes against a nominal cap of 16384. A bound the other side can widen is not +// a bound. +// +// A credential longer than this window can still straddle the cut and leave a +// prefix. That is a stated limit rather than an oversight: four kilobytes is far +// past any real bearer token, and the alternative is the unbounded margin this +// replaces. +const maxMCPSecretMatchWindow = 4 << 10 + +// boundMCPFailureError caps the RAW, server-controlled error before redaction or +// terminal normalization touches it. +// +// The bound used to sit outside redaction.ErrorMessage, which is the innermost +// call, so the whole value went through the exact-value replacements and the +// regular-expression passes first and only the leftovers were trimmed. Measured +// on the unfixed path, the work scaled with the input rather than with the cap: +// +// input 1 KiB -> 1ms, 0.2 MB allocated +// input 1 MiB -> 248ms, 36 MB allocated +// input 8 MiB -> 2.21s, 286 MB allocated (rendered panel: 400 runes) +// +// A remote MCP chooses that input by putting an oversized tool name into a +// conflict error, and every open or refresh of /mcp paid for it again. +// +// A short error is returned untouched, so ErrorMessage keeps its handling of nil +// and wrapped errors for the overwhelmingly common case. Only an oversized one +// is flattened, and flattening an eight-megabyte error is the entire point. +func boundMCPFailureError(err error) (error, bool) { + if err == nil { + return nil, false + } + limit := maxMCPReasonRawLen + maxMCPSecretMatchWindow + message := err.Error() + if len(message) <= limit { + return err, false + } + return errors.New(boundMCPRawText(message, limit)), true +} + +// boundMCPRawText truncates rune-safely, so nothing downstream sees a +// replacement character this function produced. +func boundMCPRawText(message string, limit int) string { + if len(message) <= limit { + return message + } + message = message[:limit] + for len(message) > 0 { + decoded, width := utf8.DecodeLastRuneInString(message) + if decoded != utf8.RuneError || width > 1 { + break + } + message = message[:len(message)-1] + } + return message +} + func buildMCPToolViews(cfg config.MCPConfig, registry *tools.Registry) []MCPToolView { if registry == nil { return nil @@ -247,22 +465,48 @@ func redactedStringMap(values map[string]string) string { return strings.Join(parts, " ") } +// redactMCPHeaderValue redacts a ": " header argument for DISPLAY, +// keeping the name. The name is what tells an operator which header was +// rejected; the value is the credential. +func redactMCPHeaderValue(value string) string { + if name, rest, ok := strings.Cut(value, ":"); ok && strings.TrimSpace(rest) != "" { + separator := ": " + if !strings.HasPrefix(rest, " ") { + separator = ":" + } + return name + separator + mcpDisplayRedacted + } + return mcpDisplayRedacted +} + func redactedCommandArgs(values []string) []string { trimmed := make([]string, 0, len(values)) redactNext := false + // THE TARGET ROW SITS UNDER THE ERROR ROW. Redacting the failure reason while + // printing the same credential verbatim one line lower gives it back with the + // other hand, and this row is persisted to the transcript too. + redactNextHeader := false for _, value := range values { if value = strings.TrimSpace(value); value != "" { if redactNext { - if looksLikeMCPDisplayURLValue(value) { + wasHeader := redactNextHeader + redactNext = false + redactNextHeader = false + switch { + case wasHeader: + trimmed = append(trimmed, redactMCPHeaderValue(value)) + case looksLikeMCPDisplayURLValue(value): trimmed = append(trimmed, redactMCPDisplayURL(value)) - } else { + default: trimmed = append(trimmed, mcpDisplayRedacted) } - redactNext = false continue } if key, rest, ok := strings.Cut(value, "="); ok { switch { + case isMCPHeaderFlag(key): + trimmed = append(trimmed, key+"="+redactMCPHeaderValue(rest)) + continue case isSensitiveMCPDisplayKey(key): trimmed = append(trimmed, key+"="+mcpDisplayRedacted) continue @@ -271,6 +515,39 @@ func redactedCommandArgs(values []string) []string { continue } } + // A FLAG AND ITS VALUE PACKED INTO ONE ELEMENT. + // + // The collector already splits this shape; the display did not, and + // classification without parsing is worse than neither. It recognised + // "--api-key sk-live-..." as sensitive further down, printed the whole + // element verbatim, and then redacted the NEXT argument, so the Target + // row showed the credential and blanked an unrelated "--verbose". That + // row sits directly under the reason this PR redacts, in both /mcp + // surfaces, and is persisted to the transcript. + if flag, rest, ok := strings.Cut(value, " "); ok { + switch { + case isMCPHeaderFlag(flag): + trimmed = append(trimmed, flag+" "+redactMCPHeaderValue(rest)) + continue + case isSensitiveMCPDisplayFlag(flag): + // The flag name stays: it is what tells the operator which + // credential the child rejected. + trimmed = append(trimmed, flag+" "+mcpDisplayRedacted) + continue + } + } + if flag, carried, ok := mcpHeaderArgument(value); ok { + if carried != "" { + // Attached "-HName: value": the credential is in THIS + // argument, so there is no next one to claim. + trimmed = append(trimmed, flag+redactMCPHeaderValue(carried)) + continue + } + trimmed = append(trimmed, value) + redactNext = true + redactNextHeader = true + continue + } if isSensitiveMCPDisplayFlag(value) { trimmed = append(trimmed, value) redactNext = true @@ -298,6 +575,12 @@ func redactMCPDisplayURL(raw string) string { if parsed.User != nil { parsed.User = nil } + if path := parsed.EscapedPath(); path != "" { + // The Target row renders alongside the Error, so a path credential shown + // here defeats redacting it there. + parsed.RawPath = redactMCPDisplayPath(path) + parsed.Path, _ = url.PathUnescape(parsed.RawPath) + } if parsed.RawQuery != "" { parsed.RawQuery = redactMCPDisplayRawQuery(parsed.RawQuery) } @@ -311,18 +594,63 @@ func redactMCPDisplayURL(raw string) string { return strings.ReplaceAll(out, "%5BREDACTED%5D", mcpDisplayRedacted) } +// opaqueURLPathSegments returns the path segments long enough to be a credential +// rather than a route. +func opaqueURLPathSegments(escapedPath string) []string { + segments := strings.Split(escapedPath, "/") + out := make([]string, 0, len(segments)) + for _, segment := range segments { + if len(segment) < shortestMCPSecret { + continue + } + out = append(out, segment) + } + return out +} + +// redactMCPDisplayPath replaces opaque segments and keeps the route shape, so +// the operator can still tell which endpoint failed. +func redactMCPDisplayPath(escapedPath string) string { + segments := strings.Split(escapedPath, "/") + for index, segment := range segments { + if len(segment) < shortestMCPSecret { + continue + } + segments[index] = mcpDisplayRedacted + } + return strings.Join(segments, "/") +} + func redactMCPDisplayRawQuery(rawQuery string) string { parts := strings.Split(rawQuery, "&") for index, part := range parts { if part == "" { continue } - key, _, hasValue := strings.Cut(part, "=") + key, rawValue, hasValue := strings.Cut(part, "=") decodedKey, err := url.QueryUnescape(key) if err != nil { decodedKey = key } - if !isSensitiveMCPDisplayKey(decodedKey) { + // THE PARAMETER NAME IS THE OPERATOR'S TO CHOOSE, so a name-based rule only + // covers the names somebody thought of. `?workspace=` carried a + // credential straight into this row while `?api_key=` next to it was + // redacted, and this Target row sits directly under the Error row that IS + // redacted, on the same panel, so the panel gave the value back with one + // hand. + // + // Length decides instead, on the same floor the candidate collector uses: + // a value long enough to be a credential is redacted, and `v=1` or + // `mode=sse` stays readable so the row still describes the endpoint. + sensitive := isSensitiveMCPDisplayKey(decodedKey) + if !sensitive && hasValue { + decodedValue, valueErr := url.QueryUnescape(rawValue) + if valueErr != nil { + decodedValue = rawValue + } + sensitive = len(strings.TrimSpace(decodedValue)) >= shortestMCPSecret + } + if !sensitive { continue } if hasValue { @@ -479,3 +807,673 @@ func mcpPermissionTarget(grant mcp.PermissionGrant) string { } return serverName + "/*" } + +// mcpServerSecretValues collects the configured values a failing server could +// echo back at us. +// +// The generic redaction patterns match shapes they recognise, like an +// `Authorization:` header or a key-looking token. A remote MCP server is +// configured with ARBITRARY headers, so a value under a name nobody can predict +// (`X-Workspace-Credential`, say) matches no pattern at all. A server that +// returns the request it failed on then puts that value straight into +// MCPServerView.Error, which this PR newly renders in both /mcp surfaces and the +// session transcript. Passing the values themselves redacts by equality rather +// than by shape, so the name does not have to be guessable. +// +// It also removes the dependence on the syntactic matcher, which terminal +// control bytes can split before the later sanitizer strips them. +// +// Short values are skipped. A configured "1" or "true" is not a credential, and +// redacting it by equality would punch holes through unrelated text, which is +// its own way of making an error message useless. +func mcpServerSecretValues(raw config.MCPServerConfig) []string { + values := make([]string, 0, len(raw.Headers)+len(raw.Env)+len(raw.Args)+2) + // AMBIGUOUS values go through credentialCandidates, whose shortestMCPSecret + // floor keeps ordinary short configuration ("v=1", "mode=sse") out of the + // redaction set. That floor is a readability trade-off, and it is only + // defensible while the value might not be a credential at all. + add := func(value string) { + values = append(values, credentialCandidates(value)...) + } + // KNOWN values skip the floor. Provenance has already settled that these are + // secret: a field named ClientSecret, or the value of a credential-bearing + // flag that sensitiveMCPArgValues identified as such. Routing them through + // the ambiguity heuristic discarded anything under eight bytes, so a + // six-byte client secret echoed back in an error_description, or a short + // value passed through --api-key, survived into the panel and the + // transcript. Stored tokens are already protected at any non-empty length; + // this makes the configured sources agree with them. + // + // The candidate walk still runs, so the " " shape is + // split as before; what changes is that the whole value is kept regardless + // of length. + addKnown := func(value string) { + if trimmed := strings.TrimSpace(value); trimmed != "" { + values = append(values, trimmed) + } + values = append(values, credentialCandidates(value)...) + } + // addClassified keeps a key/value pair together long enough to decide which + // of the two applies. A key the sensitive-name list recognises has already + // established what the value is, so the readability floor has nothing left + // to protect; an ordinary key leaves the value ambiguous and keeps it. + addClassified := func(key, value string) { + if isSensitiveMCPDisplayKey(key) { + addKnown(value) + return + } + add(value) + } + // CLASSIFIED BY THE KEY, because the key is what removes the ambiguity. A + // map value alone is just a string, and flattening it before deciding which + // heuristic applies meant "X-Api-Key: s3cr3t" contributed no exact + // candidate: the value went through the length floor that exists for + // ordinary configuration like "mode=sse". A six-byte credential under a key + // that names it as one then reached the panel and the transcript whenever a + // child echoed the value on its own, where generic shape matching has + // nothing left to recognise. + for key, value := range raw.Headers { + addClassified(key, value) + } + // Env carries the same risk for a stdio server: the child is launched with + // these, and a startup failure often reports the environment it was given. + for key, value := range raw.Env { + addClassified(key, value) + } + // Args carry it too, and more visibly: a stdio child that rejects its own + // invocation usually prints that invocation back, and connectStdio appends + // the captured stderr to the initialization error this panel renders. + for _, value := range sensitiveMCPArgValues(raw.Args) { + addKnown(value) + } + // THE ENDPOINT ITSELF CARRIES CREDENTIALS. HTTP and SSE send the configured + // URL verbatim, and it accepts both userinfo and arbitrary query keys, so + // `https://host/mcp?workspace=opaque-token` puts a credential in a parameter + // whose NAME the operator chose. The generic query redaction downstream only + // recognises conventional key names, so "workspace" walks straight through it, + // and equality redaction cannot help because nothing told it the value. A + // server that echoes its own endpoint in a failure body then reaches this + // panel and the transcript with the token intact. + // + // Collected as exact values here rather than by widening the sensitive-key + // list, which would still only cover names somebody thought of. + urlKnown, urlAmbiguous := mcpURLSecretValues(raw.URL) + for _, value := range urlKnown { + addKnown(value) + } + for _, value := range urlAmbiguous { + add(value) + } + // raw.Auth is DELIBERATELY NOT a candidate. It is the public authentication + // MODE selector, and normalization accepts only the value "oauth", which the + // panel itself displays as ordinary metadata. Feeding it to the exact-value + // redactor classified the word by the security-sounding name of its field + // rather than by what it holds, so every failure from the OAuth stack lost + // the one token naming the subsystem that failed: "oauth: fetch + // authorization server metadata" rendered as "[REDACTED]: fetch + // authorization server metadata". + // + // It went unnoticed while ambiguous values ran through the length floor, + // which discarded a five-character string on its own. Removing that floor + // for known-provenance values is what surfaced it, which is the tell that + // the field was miscategorised rather than the floor being load-bearing. + if raw.OAuth != nil { + addKnown(raw.OAuth.ClientSecret) + // THE OAUTH ENDPOINTS ARE REACHED DURING STARTUP TOO, and they carry + // credentials in exactly the way the main URL does. With a stored token a + // 401 from the server triggers a refresh, oauth.PostToken then posts to + // TokenEndpoint, and a dial or TLS failure comes back wrapped in a + // *url.Error that retains the path and query. Collecting only from + // raw.URL left an accepted endpoint such as + // "https://auth.invalid/token?workspace=" outside the candidate + // set, so the value reached the panel and the transcript intact. + for _, endpoint := range []string{ + raw.OAuth.TokenEndpoint, + raw.OAuth.AuthorizationEndpoint, + raw.OAuth.RegistrationEndpoint, + raw.OAuth.IssuerURL, + } { + endpointKnown, endpointAmbiguous := mcpURLSecretValues(endpoint) + for _, value := range endpointKnown { + addKnown(value) + } + for _, value := range endpointAmbiguous { + add(value) + } + } + } + return values +} + +// mcpURLSecretValues returns the credential-bearing parts of a configured +// endpoint: the userinfo password, the userinfo username, and every query value. +// +// Every query VALUE, not the sensitively-named ones, because the name is the +// operator's to choose and the point of equality redaction is that it does not +// have to guess. The shortestMCPSecret floor in credentialCandidates is what +// keeps ordinary short parameters (`v=1`, `mode=sse`) out of the set, so this +// does not blank harmless words out of unrelated text. +// +// The path is deliberately NOT collected. It is the part an operator needs to +// see to recognise which endpoint failed, and it is not where a credential is +// configured. +// mcpURLSecretValues splits an endpoint's credential-bearing parts into those a +// KEY has already classified as secret and those that are merely ambiguous. The +// caller applies the readability floor only to the second group. +// +// Returning one flat list made a value under a key like `api_key` +// indistinguishable from `v=1`, so a short credential was discarded by a +// heuristic that exists for ordinary configuration. +func mcpURLSecretValues(rawURL string) (known []string, ambiguous []string) { + trimmed := strings.TrimSpace(rawURL) + if trimmed == "" { + return nil, nil + } + parsed, err := url.Parse(trimmed) + if err != nil || parsed == nil { + return nil, nil + } + // BOTH REPRESENTATIONS, decoded and raw. url.Parse and url.ParseQuery hand + // back DECODED values, but the network client starts from the configured URL + // string, so an MCP can echo the escaped spelling back in its failure body. + // Exact-value redaction then knows "opaque-workspace-token-9f3c2b7ae1d8" and + // the body contains "opaque%2Dworkspace%2Dtoken%2D9f3c2b7ae1d8", which matches + // nothing and prints. %2D decodes to a hyphen, so the credential is fully + // recoverable from what was displayed and persisted. + // + // The generic patterns do not catch it either, because the parameter name is + // the operator's to choose. Collecting both forms fixes the boundary rather + // than guessing at more names. + values := make([]string, 0, 8) + // PATH SEGMENTS ARE CREDENTIAL MATERIAL TOO. + // + // Query values and userinfo were treated as secret-bearing and the path as an + // identifier, and the configuration contract accepts an arbitrary HTTP or SSE + // path. Opaque path-segment credentials are an ordinary endpoint convention, + // and this needs no crafted response body to leak: a failing http.Client.Do + // returns a *url.Error carrying the request URL, which the failed-server path + // wraps and renders. + // + // Only opaque-looking segments, by the same length floor the rest of this file + // uses. A route like "/mcp" or "/v1/sse" is structure, and redacting it would + // eat the diagnostic without protecting anything. + for _, segment := range opaqueURLPathSegments(parsed.EscapedPath()) { + values = append(values, segment) + if decoded, err := url.PathUnescape(segment); err == nil && decoded != segment { + values = append(values, decoded) + } + } + if parsed.User != nil { + // The userinfo password is a credential by POSITION. No key names it and + // no length makes it ordinary. + if password, ok := parsed.User.Password(); ok { + known = append(known, password) + } + // The username too: a token-as-username is a real shape, and the floor + // discards an ordinary short login. + values = append(values, parsed.User.Username()) + // The escaped spelling, taken from the ORIGINAL string rather than from + // parsed.User.String(). Go re-escapes canonically there and leaves + // unreserved characters alone, so a configured "%2D" comes back as "-" and + // the raw form the server echoes would still match nothing. + if rawUserinfo := rawURLUserinfo(trimmed); rawUserinfo != "" { + values = append(values, rawUserinfo) + if rawUser, rawPassword, found := strings.Cut(rawUserinfo, ":"); found { + values = append(values, rawUser, rawPassword) + } + } + } + // Raw query values, taken from RawQuery before any decoding. + for _, pair := range strings.Split(parsed.RawQuery, "&") { + if pair == "" { + continue + } + if rawKey, rawValue, found := strings.Cut(pair, "="); found { + if isSensitiveMCPDisplayKey(rawKey) { + known = append(known, rawValue) + if decoded, derr := url.QueryUnescape(rawValue); derr == nil && decoded != rawValue { + known = append(known, decoded) + } + continue + } + values = append(values, rawValue) + } + } + query, err := url.ParseQuery(parsed.RawQuery) + if err != nil { + return known, values + } + names := make([]string, 0, len(query)) + for name := range query { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + if isSensitiveMCPDisplayKey(name) { + known = append(known, query[name]...) + continue + } + values = append(values, query[name]...) + } + return known, values +} + +// credentialCandidates returns the configured value plus the shorter strings a +// server might echo INSTEAD of it. +// +// Redaction is equality on the whole configured value, and the credential is +// usually not the whole value. Zero's own documented config spells an +// authenticated server as `"Authorization": "Bearer sk-live-..."`, so a server +// that quotes back only the credential, without the scheme word, matches +// nothing and prints. The same shape arrives through a composite argument such +// as `--header "Authorization: Bearer sk-live-..."`, where the configured +// string carries a header name as well. +// +// So each tail after a space or a colon is offered as its own candidate, which +// covers " " and "
: " without +// needing to know the scheme vocabulary. The floor discards the fragments too +// short to be a credential, which is what stops "Bearer" or a header name from +// entering the set and blanking those words out of unrelated text. +// +// A value with no separator yields itself and nothing else, so the common case +// costs one entry, as before. +// maxMCPCredentialCandidates and maxMCPCredentialInput bound what one configured +// value can expand into. +// +// This walked every suffix after every space or colon and kept them all, so a +// delimiter-heavy value produced O(n) candidates and RedactString then ran a +// replacement pass for each. The expansion happened on the CONFIG side, outside +// the raw-error bound, so opening or refreshing /mcp for that server burned CPU +// and memory however short the server's error was. +// +// The tails exist for one narrow reason: a value configured as "Bearer " +// has to yield as well, because the server may echo only the credential. +// Two or three splits cover every such convention. Beyond that the suffixes stop +// being plausible credentials and start being work. +const ( + maxMCPCredentialCandidates = 8 + // maxMCPCredentialSeparatorScan bounds where a separator is LOOKED FOR, not + // how long a value may be. + // + // This used to be an input-length cap that returned the whole value and + // nothing else once crossed, which made a work bound into a change of + // security semantics: an Authorization value of "Bearer " longer + // than the cap stopped offering the bare token as a candidate, so a failed + // server echoing only the token matched nothing, and an opaque token has no + // shape for the fallback redactor to catch either. + // + // Every convention this walks, " " and + // "
: ", puts its separators in a short prefix, so + // scanning only that prefix keeps the cost fixed while the tails survive at + // any value length. + maxMCPCredentialSeparatorScan = 256 +) + +func credentialCandidates(value string) []string { + candidates := make([]string, 0, 3) + remainder := strings.TrimSpace(value) + for { + if len(remainder) >= shortestMCPSecret { + candidates = append(candidates, remainder) + } + if len(candidates) >= maxMCPCredentialCandidates { + return candidates + } + // Only the prefix is searched, so a multi-megabyte opaque token costs the + // same as a short one and still yields its tail. + window := remainder + if len(window) > maxMCPCredentialSeparatorScan { + window = window[:maxMCPCredentialSeparatorScan] + } + index := strings.IndexAny(window, " :") + if index < 0 { + return candidates + } + next := strings.TrimSpace(remainder[index+1:]) + if next == remainder { + // Defensive: without this a value of only separators could not shrink + // and the loop would not terminate. + return candidates + } + remainder = next + } +} + +// sensitiveMCPArgValues returns the VALUES a stdio server is launched with +// behind a sensitive flag, for redaction. redactedCommandArgs answers a +// different question a few lines up: it returns display strings, so the secret +// is already gone by the time it returns and nothing there can be reused here. +// Only the predicates are shared, deliberately, so the two cannot drift apart +// about which flags are sensitive. +// +// Values are trimmed because the child is launched with trimmed args +// (internal/mcp/config.go), and it can only echo back what it was given. An +// untrimmed candidate would be compared against a string the child never saw. +// +// The predicate matches on substrings, so a value behind a flag like +// --auth-type is collected as well. Redacting an enum out of a message costs +// some readability; not redacting a credential costs the credential, so the +// collection is deliberately the wider of the two. +// mcpHeaderArgument parses ONE argument into the header flag it names and the +// header text it carries, if any. +// +// ONE PARSER FOR BOTH CONSUMERS. The redaction collector and the target row +// each derived the accepted shapes separately and drifted apart: an argument +// the reason redacted printed verbatim in the target one line below. Everything +// that decides "is this a header, and where is its value" now happens here. +// +// carried is the header text when this argument holds it, and empty when the +// value is the NEXT argument. The attached form "-HName: value" is included +// because it is the conventional curl spelling and neither consumer recognised +// it: the flag name parsed as "HName:..." and matched nothing. +// +// The long form folds case; the SHORT form does not, deliberately. "-H" is the +// header flag and "-h" is help, which takes no value, so folding here would +// consume the next argument after "-h" and blank an unrelated word out of every +// message that mentions it. +func mcpHeaderArgument(value string) (flag string, carried string, ok bool) { + trimmed := strings.TrimSpace(value) + if !strings.HasPrefix(trimmed, "-") { + return "", "", false + } + // Long form first, so "--header..." is never mistaken for the short "-H". + if rest, matched := cutFoldPrefix(trimmed, "--header"); matched { + switch { + case rest == "": + return trimmed, "", true + case strings.HasPrefix(rest, "="): + return trimmed[:len(trimmed)-len(rest)], strings.TrimSpace(rest[1:]), true + case strings.HasPrefix(rest, " "): + return trimmed[:len(trimmed)-len(rest)], strings.TrimSpace(rest), true + } + return "", "", false + } + if !strings.HasPrefix(trimmed, "-H") { + return "", "", false + } + rest := trimmed[len("-H"):] + switch { + case rest == "": + return trimmed, "", true + case strings.HasPrefix(rest, "="): + return "-H", strings.TrimSpace(rest[1:]), true + case strings.HasPrefix(rest, " "): + return "-H", strings.TrimSpace(rest), true + case strings.HasPrefix(rest, "-"): + // "-H-something" is not a header spelling; refuse rather than guess. + return "", "", false + } + // Attached: "-HName: value". + return "-H", rest, true +} + +// cutFoldPrefix reports whether s begins with prefix under case folding, and +// returns what follows it. +func cutFoldPrefix(s, prefix string) (string, bool) { + if len(s) < len(prefix) || !strings.EqualFold(s[:len(prefix)], prefix) { + return "", false + } + return s[len(prefix):], true +} + +func isMCPHeaderFlag(value string) bool { + _, _, ok := mcpHeaderArgument(value) + return ok +} + +func sensitiveMCPArgValues(args []string) []string { + values := make([]string, 0, len(args)) + // A candidate that is itself a flag is never a value. Reading one would put + // something like "--verbose" into the redaction set, and every message + // mentioning it would lose the word. + isFlag := func(value string) bool { return strings.HasPrefix(value, "-") } + // HEADER FLAGS ARE THEIR OWN CLASS, because the credential is in the VALUE + // and the flag name says nothing about it. isSensitiveMCPDisplayKey matches + // token/secret/auth/credential and friends against the FLAG, so + // `--header X-Workspace-Credential: opaque-token` was never collected: the + // flag is "header", which matches none of them, and the credential rides in + // an argument whose name the operator chose. A stdio child that rejects its + // invocation echoes it into captured stderr, which this panel renders and the + // transcript persists. + // + // The long form is matched case-insensitively. The SHORT form is not, and + // that is deliberate: `-H` is the header flag, `-h` is help and takes no + // value, so folding case here would set pending on `-h` and put the next + // argument into the redaction set, blanking an unrelated word out of every + // message that mentions it. Over-collection has already cost this panel a + // readable docker image name once. + // A header ARGUMENT carries ": " while a configured header + // contributes only the value, because a map key is not part of it. Feeding the + // whole argument in would make the entire line a candidate, so a server that + // echoes the header back would have its NAME redacted too and the message + // would no longer say which header was rejected. Only the value is offered; + // credentialCandidates still splits it further for the " " + // shape. + headerValue := func(value string) string { + if _, rest, ok := strings.Cut(value, ":"); ok { + if trimmed := strings.TrimSpace(rest); trimmed != "" { + return trimmed + } + } + return value + } + pending := false + pendingHeader := false + for _, arg := range args { + arg = strings.TrimSpace(arg) + if arg == "" { + // Blanks are skipped rather than consumed, matching the display pass. + // Consuming one would take the blank as the value and leave the real + // secret in the next position unredacted. + continue + } + if pending { + wasHeader := pendingHeader + pending = false + pendingHeader = false + if !isFlag(arg) { + if wasHeader { + values = append(values, headerValue(arg)) + } else { + values = append(values, arg) + } + continue + } + // Otherwise fall through: this argument is a flag in its own right. + } + // Cut at the FIRST "=" and keep the whole tail, so base64 padding and + // values that themselves contain "=" survive intact. + if key, rest, ok := strings.Cut(arg, "="); ok && (isSensitiveMCPDisplayKey(key) || isMCPHeaderFlag(key)) { + collected := strings.TrimSpace(rest) + if isMCPHeaderFlag(key) { + collected = headerValue(collected) + } + values = append(values, collected) + continue + } + // A flag and its value packed into a single argument. The display pass + // gets this shape wrong in the other direction (it prints the whole thing + // verbatim, then redacts the following, unrelated argument), so this + // cannot be delegated to it. + if flag, rest, ok := strings.Cut(arg, " "); ok && (isSensitiveMCPDisplayFlag(flag) || isMCPHeaderFlag(flag)) { + collected := strings.TrimSpace(rest) + if isMCPHeaderFlag(flag) { + collected = headerValue(collected) + } + values = append(values, collected) + continue + } + // The attached header spelling, "-HName: value", carries its value in + // this same argument. Neither pass recognised it: the flag name parsed + // as "HName:..." and matched nothing, so the value was never collected + // here and printed whole one row below. + if _, carried, ok := mcpHeaderArgument(arg); ok && carried != "" { + values = append(values, headerValue(carried)) + continue + } + // Only an actual FLAG claims the next argument. isSensitiveMCPDisplayFlag + // strips leading dashes before matching, so it says yes to a bare + // positional word too, and the documented GitHub server config + // (`-e GITHUB_PERSONAL_ACCESS_TOKEN ghcr.io/github/github-mcp-server`) + // would put the IMAGE NAME into the redaction set: the pull failure would + // then lose the one string that explains it. A positional argument is not + // a flag and does not introduce a value. + if isFlag(arg) && (isSensitiveMCPDisplayFlag(arg) || isMCPHeaderFlag(arg)) { + pendingHeader = isMCPHeaderFlag(arg) + // The value is the next argument, if there is one. A sensitive flag in + // the last position simply has nothing to redact. + pending = true + } + } + return values +} + +// rawURLUserinfo returns the userinfo section exactly as it was configured, +// before any decoding or canonical re-escaping. +// +// parsed.User.String() is not a substitute: it re-escapes by Go rules and leaves +// unreserved characters alone, so a configured "%2D" comes back as "-". The +// server echoes what it was given, so the raw spelling is the one that has to be +// matched. +func rawURLUserinfo(rawURL string) string { + authority := rawURL + if _, rest, found := strings.Cut(authority, "//"); found { + authority = rest + } + // The userinfo ends at the first "@", and any "/" before it means there is + // none at all. + if slash := strings.IndexByte(authority, '/'); slash >= 0 { + if at := strings.IndexByte(authority, '@'); at < 0 || at > slash { + return "" + } + authority = authority[:slash] + } + at := strings.LastIndexByte(authority, '@') + if at < 0 { + return "" + } + return authority[:at] +} + +// recoverableSecretPrefix reports whether a surviving prefix of size bytes +// gives away enough of a total-byte credential to matter. +// +// There has to be SOME floor or the tail of nearly every message disappears: +// with a handful of candidates one of them almost always begins with whatever +// character the text happens to end on, and cutting there would cost a +// character for nothing. The previous floor was a flat eight bytes, which is +// wrong in the direction that counts, because seven bytes of an eight-byte +// credential is the credential. So the rule is proportional as well as +// absolute: eight or more characters is independently useful, and so is half of +// the value however short it is. +func recoverableSecretPrefix(size, total int) bool { + if size <= 0 || total <= 0 { + return false + } + return size >= shortestMCPSecret || size*2 >= total +} + +// longestPrefixSuffix returns the length of the longest prefix of pattern that +// is also a suffix of text. +// +// THE SEARCH IS SIZED TO THE CREDENTIAL, NOT TO A CONSTANT. The previous +// version inspected a fixed 4 KiB window at the end of the text, so a +// credential beginning before that window could never be matched: the inspected +// span started partway through it, and a middle is not a prefix. Measured on +// this code, a 6000-byte value positioned across the cut left 5000 of its bytes +// on the panel. +// +// KMP over "pattern + sentinel + tail" answers it in one pass. The work is +// linear in the CONFIGURED value, which the operator owns and which is already +// bounded, rather than in anything the remote server sent, so a hostile error +// cannot widen it. +func longestPrefixSuffix(pattern, text string) int { + if pattern == "" || text == "" { + return 0 + } + if len(text) > len(pattern) { + text = text[len(text)-len(pattern):] + } + // THE WORK IS BOUNDED BY THE TEXT, NOT BY THE CANDIDATE. + // + // A prefix of pattern that is a suffix of text can be at most len(text) long, + // so every longer prefix is unreachable and scanning it changes no answer. + // Without this the KMP below builds pattern+sentinel+text and an []int over + // it, so an oversized configured value drove the cost: three 2 MiB candidates + // measured 54.5 MiB and 33ms on EVERY /mcp state rebuild, against a nominal + // fixed render budget. Configured headers, env values, URL components, OAuth + // fields and stored tokens have no size limit, so that was the attacker's input + // sizing the defender's work. + // + // Truncating here rather than at the call site keeps recoverableSecretPrefix + // weighing the match against the FULL credential length, which is what decides + // whether a partial is worth cutting. + if len(pattern) > len(text) { + pattern = pattern[:len(text)] + } + const sentinel = "\x00" + if strings.Contains(pattern, sentinel) || strings.Contains(text, sentinel) { + // Unreachable for a rendered failure reason, whose control bytes are + // already stripped. Degrade to the direct answer rather than trust a + // sentinel that is not one. + for size := len(text); size > 0; size-- { + if strings.HasSuffix(text, pattern[:size]) { + return size + } + } + return 0 + } + combined := pattern + sentinel + text + failure := make([]int, len(combined)) + for i := 1; i < len(combined); i++ { + length := failure[i-1] + for length > 0 && combined[i] != combined[length] { + length = failure[length-1] + } + if combined[i] == combined[length] { + length++ + } + failure[i] = length + } + return failure[len(combined)-1] +} + +// mcpStaleObservationReason replaces a startup failure whose redaction context +// no longer exists. The row still reports that the server failed, which is what +// the observation actually established; only the reason is withheld. +const mcpStaleObservationReason = "startup failed; the details were dropped because the stored credentials changed since they were recorded" + +// SAFETY MUST BE MONOTONIC OVER AN OBSERVATION. +// +// A retained skipped entry holds the RAW startup error, and every rebuild +// redacts it again from whatever the token store holds at that moment. That +// makes the safety of fixed text depend on mutable external state, and the +// dependency runs the wrong way: `/mcp oauth logout` deletes the stored bearer +// and returns the configuration unchanged, so retainedMCPSkipped keeps the +// observation while the candidate set that was hiding the bearer disappears. +// The next render then writes the credential into the panel and the transcript, +// AFTER the user asked for it to be forgotten. Refresh rotation, deletion by +// another process, and a token-store read failure all do the same thing. +// +// Rather than retain a second copy of the credentials, which would be a second +// long-lived plaintext store, the observation carries a FINGERPRINT of the +// material that made it safe. A change means the guarantee cannot be +// reproduced, so the reason is withheld instead of re-derived. +func staleMCPObservation(captured string, current []string) bool { + if captured == "" { + // No fingerprint recorded, so there is nothing to compare and no claim to + // make. Callers that want the guarantee record one. + return false + } + return captured != mcpCredentialFingerprint(current) +} + +// mcpCredentialFingerprint identifies a set of credential values without +// retaining them. Order-independent, because SecretValues enumerates a map. +func mcpCredentialFingerprint(values []string) string { + // One implementation, shared with the registration boundary that stamps + // SkippedServer.Credentials. Two copies of a digest are two chances for the + // captured value and the compared value to be computed differently, which + // would make every observation look stale. + return mcp.CredentialFingerprint(values) +} diff --git a/internal/tui/mcp_state_entrypoint_test.go b/internal/tui/mcp_state_entrypoint_test.go new file mode 100644 index 000000000..e568d400d --- /dev/null +++ b/internal/tui/mcp_state_entrypoint_test.go @@ -0,0 +1,118 @@ +package tui + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" +) + +// THROUGH THE REAL ENTRY POINT, not the helper. +// +// The rest of this file's neighbours call redactMCPFailureReason directly, which +// proves the helper and nothing else. BuildMCPViewState is what the panel and +// the transcript actually go through, and it is where a second surface could +// reintroduce either problem: the row renderer inspects the raw query field +// separately, and the reason is cut again downstream. A helper that is clean +// while the assembled view is not would still be a leak. +func buildOneFailedServer(t *testing.T, endpoint string, failure error) MCPServerView { + t.Helper() + state := BuildMCPViewState(MCPStateOptions{ + Config: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "docs": {URL: endpoint}, + }, + }, + Skipped: []mcp.SkippedServer{{Name: "docs", Err: failure}}, + }) + for _, server := range state.Servers { + if server.Name == "docs" { + return server + } + } + t.Fatalf("the failed server is missing from the assembled state: %+v", state.Servers) + return MCPServerView{} +} + +// An arbitrary query key carrying a percent-encoded credential. "workspace" is +// not a conventionally sensitive name, so the generic patterns do not catch it, +// and url.ParseQuery hands back the decoded spelling while the server echoes the +// escaped one it was given. +func TestAssembledStateRedactsBothCredentialSpellings(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + escaped := strings.ReplaceAll(token, "-", "%2D") + + for _, testCase := range []struct { + name string + endpoint string + }{ + {"arbitrary query key", "https://host.invalid/mcp?workspace=" + escaped}, + {"userinfo username", "https://" + escaped + "@host.invalid/mcp"}, + {"userinfo password", "https://svc:" + escaped + "@host.invalid/mcp"}, + } { + t.Run(testCase.name, func(t *testing.T) { + server := buildOneFailedServer(t, testCase.endpoint, errors.New("502 Bad Gateway from "+testCase.endpoint)) + // Both the row's reason AND its target are rendered and persisted. + rendered := server.Error + "\n" + server.Target + if strings.Contains(rendered, token) { + t.Errorf("the decoded credential reached the panel:\n%s", rendered) + } + if strings.Contains(rendered, escaped) { + t.Errorf("the escaped credential reached the panel; %%2D decodes to a hyphen, so it is fully recoverable:\n%s", rendered) + } + }) + } +} + +// And the fixed budget holds at the entry point, for a hostile error AND for a +// hostile secret. The second half is the one that matters: the margin past the +// cut used to be sized to the longest configured value, so the other side could +// widen the limit simply by configuring a large credential. +func TestAssembledStateBoundsHostileFailuresAndSecrets(t *testing.T) { + budget := maxMCPReasonRawLen + maxMCPSecretMatchWindow + + for _, testCase := range []struct { + name string + endpoint string + failure string + }{ + {"multi-megabyte failure", "https://host.invalid/mcp", "tool name conflict: " + strings.Repeat("A", 8*1024*1024)}, + {"multi-megabyte configured secret", "https://host.invalid/mcp?workspace=" + strings.Repeat("s", 2*1024*1024), "conflict: " + strings.Repeat("B", 64*1024)}, + } { + t.Run(testCase.name, func(t *testing.T) { + started := time.Now() + server := buildOneFailedServer(t, testCase.endpoint, errors.New(testCase.failure)) + elapsed := time.Since(started) + + if len(server.Error) > budget { + t.Errorf("the retained reason is %d bytes against a fixed budget of %d", len(server.Error), budget) + } + // The work has to be bounded too, not only the result. Before the fix an + // eight megabyte reason took seconds here; the ceiling is loose on purpose + // so it fails on the old unbounded behaviour and not on a slow machine. + if elapsed > 2*time.Second { + t.Errorf("building the state took %s; the pipeline is still doing work proportional to the input", elapsed) + } + }) + } +} + +// A credential straddling the cut must not leave a usable prefix behind, or the +// bound would have manufactured the leak it has nothing to do with. +func TestAssembledStateRedactsASecretCrossingTheBoundary(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + endpoint := "https://host.invalid/mcp?workspace=" + token + + filler := strings.Repeat("A", maxMCPReasonRawLen-len(token)/2) + server := buildOneFailedServer(t, endpoint, errors.New(filler+token+strings.Repeat("B", 4096))) + + for size := len(token); size > 8; size-- { + if strings.Contains(server.Error, token[:size]) { + t.Errorf("a %d-character prefix of the credential crossed the boundary intact: %q", size, token[:size]) + break + } + } +} diff --git a/internal/tui/mcp_state_test.go b/internal/tui/mcp_state_test.go index 9a02d4c60..d31e13829 100644 --- a/internal/tui/mcp_state_test.go +++ b/internal/tui/mcp_state_test.go @@ -41,12 +41,14 @@ func TestBuildMCPViewStateSummarizesConfiguredServers(t *testing.T) { } assertServerView(t, state.Servers[0], MCPServerView{ Name: "docs", + ConfigKey: "docs", Transport: "stdio", State: "enabled", Target: "docs-mcp --workspace . env ZERO_DOCS_TOKEN=[REDACTED]", }) assertServerView(t, state.Servers[1], MCPServerView{ Name: "linear", + ConfigKey: "linear", Transport: "http", State: "disabled", Target: "https://linear.example/mcp headers Authorization=[REDACTED]", @@ -54,6 +56,7 @@ func TestBuildMCPViewStateSummarizesConfiguredServers(t *testing.T) { }) assertServerView(t, state.Servers[2], MCPServerView{ Name: "updates", + ConfigKey: "updates", Transport: "sse", State: "enabled", Target: "https://events.example/sse headers X-Api-Key=[REDACTED]", diff --git a/internal/tui/mcp_target_redaction_test.go b/internal/tui/mcp_target_redaction_test.go new file mode 100644 index 000000000..484ee6d19 --- /dev/null +++ b/internal/tui/mcp_target_redaction_test.go @@ -0,0 +1,61 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// THE TARGET ROW IS ON THE SAME PANEL AS THE ERROR ROW. +// +// Redacting the failure reason while printing the same credential verbatim one +// line lower hands it straight back, and this row is persisted to the transcript +// too. Every header-flag form and any long query value reached it untouched. +func TestServerTargetDoesNotPrintCredentials(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + + for _, testCase := range []struct { + name string + raw config.MCPServerConfig + }{ + {"header separated", config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header", "X-Workspace-Id: " + token}}}, + {"header equals", config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header=X-Workspace-Id: " + token}}}, + {"header packed", config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header X-Workspace-Id: " + token}}}, + {"short separated", config.MCPServerConfig{Command: "mcp-server", Args: []string{"-H", "X-Workspace-Id: " + token}}}, + {"short equals", config.MCPServerConfig{Command: "mcp-server", Args: []string{"-H=X-Workspace-Id: " + token}}}, + {"short packed", config.MCPServerConfig{Command: "mcp-server", Args: []string{"-H X-Workspace-Id: " + token}}}, + {"no space after colon", config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header", "X-Workspace-Id:" + token}}}, + {"url arbitrary query", config.MCPServerConfig{URL: "https://host.invalid/mcp?workspace=" + token}}, + } { + t.Run(testCase.name, func(t *testing.T) { + target := mcpServerTarget(testCase.raw) + if strings.Contains(target, token) { + t.Errorf("the credential is printed verbatim in the Target row, which the panel shows and the transcript keeps:\n%s", target) + } + if !strings.Contains(target, mcpDisplayRedacted) { + t.Errorf("nothing was redacted at all:\n%s", target) + } + }) + } +} + +// The header name and the endpoint still have to be readable, or the row stops +// describing the server it is there to describe. +func TestServerTargetKeepsTheReadableParts(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + + stdio := mcpServerTarget(config.MCPServerConfig{Command: "mcp-server", Args: []string{"--header", "X-Workspace-Id: " + token}}) + for _, want := range []string{"mcp-server", "--header", "X-Workspace-Id"} { + if !strings.Contains(stdio, want) { + t.Errorf("%q missing from the target row, so it no longer describes the invocation:\n%s", want, stdio) + } + } + + http := mcpServerTarget(config.MCPServerConfig{URL: "https://docs.host.invalid/mcp/v1?mode=sse&workspace=" + token}) + for _, want := range []string{"docs.host.invalid", "/mcp/v1", "mode=sse"} { + if !strings.Contains(http, want) { + t.Errorf("%q missing from the target row, so it no longer identifies the endpoint:\n%s", want, http) + } + } +} diff --git a/internal/tui/mcp_url_credential_test.go b/internal/tui/mcp_url_credential_test.go new file mode 100644 index 000000000..c9f166032 --- /dev/null +++ b/internal/tui/mcp_url_credential_test.go @@ -0,0 +1,101 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// AN ENDPOINT CAN BE A CREDENTIAL. HTTP and SSE send the configured URL +// verbatim, and it accepts userinfo and arbitrary query keys. The candidate +// collector gathered headers, env, args and OAuth material but never looked at +// the URL, and the generic query redaction downstream only recognises +// conventional key names, so a parameter the operator named "workspace" carried +// its token straight into this panel and the transcript. +func TestEndpointCredentialsAreRedactedFromAFailureReason(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + + for _, testCase := range []struct{ name, endpoint string }{ + {name: "arbitrary query name", endpoint: "https://host.invalid/mcp?workspace=" + token}, + {name: "conventional query name", endpoint: "https://host.invalid/mcp?api_key=" + token}, + {name: "userinfo password", endpoint: "https://svc:" + token + "@host.invalid/mcp"}, + {name: "userinfo username", endpoint: "https://" + token + "@host.invalid/mcp"}, + {name: "second of two parameters", endpoint: "https://host.invalid/mcp?mode=sse&tenant=" + token}, + } { + t.Run(testCase.name, func(t *testing.T) { + raw := config.MCPServerConfig{URL: testCase.endpoint} + // The server echoes its own endpoint back in the failure body, which is + // what httpStatusError retains. + failure := errors.New("502 Bad Gateway from " + testCase.endpoint) + + got := redactMCPFailureReason(failure, raw, nil) + if strings.Contains(got, token) { + t.Errorf("the endpoint credential survived into the rendered failure, which is also persisted:\n%s", got) + } + }) + } +} + +// The host and path must survive, or the operator cannot tell which endpoint +// failed. Redacting the whole URL would be safe and useless. +func TestEndpointHostAndPathSurviveRedaction(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + endpoint := "https://docs.host.invalid/mcp/v1?workspace=" + token + raw := config.MCPServerConfig{URL: endpoint} + + got := redactMCPFailureReason(errors.New("502 Bad Gateway from "+endpoint), raw, nil) + if strings.Contains(got, token) { + t.Fatalf("credential survived:\n%s", got) + } + for _, want := range []string{"docs.host.invalid", "/mcp/v1"} { + if !strings.Contains(got, want) { + t.Errorf("%q was redacted too, so the message no longer identifies the endpoint:\n%s", want, got) + } + } +} + +// Short, ordinary parameters are not credentials, and blanking them would punch +// holes through unrelated text. The shortestMCPSecret floor is what stops it. +func TestOrdinaryShortQueryValuesAreNotTreatedAsSecrets(t *testing.T) { + raw := config.MCPServerConfig{URL: "https://host.invalid/mcp?v=1&mode=sse"} + got := redactMCPFailureReason(errors.New("transport mode sse rejected, protocol v1"), raw, nil) + for _, want := range []string{"sse", "v1"} { + if !strings.Contains(got, want) { + t.Errorf("%q was redacted as if it were a credential:\n%s", want, got) + } + } +} + +// BOTH REPRESENTATIONS OF THE SAME CREDENTIAL. +// +// url.Parse and url.ParseQuery hand back DECODED values, but the network client +// starts from the configured URL string, so an MCP can echo the escaped spelling +// back in its failure body. Exact-value redaction knew only the decoded form, and +// "%2D" decodes to a hyphen, so the credential displayed and persisted in /mcp was +// fully recoverable. The parameter name is the operator's to choose, so the +// generic patterns do not catch it either. +func TestPercentEncodedEndpointCredentialsAreRedacted(t *testing.T) { + const token = "opaque-workspace-token-9f3c2b7ae1d8" + encoded := strings.ReplaceAll(token, "-", "%2D") + + for _, testCase := range []struct{ name, endpoint string }{ + {name: "arbitrary query key, percent-encoded", endpoint: "https://host.invalid/mcp?workspace=" + encoded}, + {name: "percent-encoded userinfo username", endpoint: "https://" + encoded + "@host.invalid/mcp"}, + {name: "percent-encoded userinfo password", endpoint: "https://svc:" + encoded + "@host.invalid/mcp"}, + } { + t.Run(testCase.name, func(t *testing.T) { + raw := config.MCPServerConfig{URL: testCase.endpoint} + // The server echoes the configured URL back verbatim, escapes and all. + got := redactMCPFailureReason(errors.New("502 Bad Gateway from "+testCase.endpoint), raw, nil) + + if strings.Contains(got, encoded) { + t.Errorf("the escaped credential survived; %%2D decodes to a hyphen, so it is fully recoverable:\n%s", got) + } + if strings.Contains(got, token) { + t.Errorf("the decoded credential survived:\n%s", got) + } + }) + } +} diff --git a/internal/tui/mcp_view.go b/internal/tui/mcp_view.go index fc4061722..f059afe38 100644 --- a/internal/tui/mcp_view.go +++ b/internal/tui/mcp_view.go @@ -4,6 +4,8 @@ import ( "fmt" "strings" "time" + "unicode" + "unicode/utf8" ) type MCPViewState struct { @@ -14,12 +16,21 @@ type MCPViewState struct { } type MCPServerView struct { - Name string + // Name is the CANONICAL runtime name: trimmed, and the spelling the registry, + // skipped-failure records and tool counts all use. It is the display label and + // the join key, and it is deliberately not unique across config entries. + Name string + // ConfigKey is the EXACT key in the configuration map, untrimmed. Actions + // address the config by that key, so it is the only identity that can select + // one row when a padded alias renders under the same canonical name. + ConfigKey string Transport string State string Target string Auth string ToolCount int + // Error explains a "failed" state. Empty for every other state. + Error string } type MCPToolView struct { @@ -153,6 +164,13 @@ func mcpManagerServerLines(servers []MCPServerView) []string { } parts = append(parts, transport) lines = append(lines, prefix+strings.Join(parts, " ยท ")) + // The reason sits directly under the server rather than in the actions + // line, because "failed" on its own sends the reader to check their + // config when the answer is usually in the error: a missing binary, a + // refused connection, a bad token. + if reason := sanitizeTerminalReason(server.Error); reason != "" { + lines = append(lines, " "+reason) + } if target := strings.TrimSpace(server.Target); target != "" { lines = append(lines, " "+target) } @@ -163,6 +181,148 @@ func mcpManagerServerLines(servers []MCPServerView) []string { return lines } +// maxMCPReasonLen bounds the failure reason so one verbose server cannot push +// the rest of the panel off screen. +const maxMCPReasonLen = 400 + +// maxMCPReasonRawLen bounds the input the sanitizer walks. Nothing upstream caps +// the handshake error a server hands back, and maxMCPReasonLen alone cannot end +// the walk: escape sequences are consumed without producing output, so they +// spend input against a budget that never fills. The bound sits far above the +// visible cap so a genuinely long error is still truncated by display rules +// rather than by this. +const maxMCPReasonRawLen = 16 * 1024 + +// sanitizeTerminalReason turns a server-authored string into one safe terminal +// line. +// +// The failure reason is the only value on this panel that the MCP server writes +// itself, and it goes straight to a terminal. redaction.ErrorMessage removes +// credentials, not control bytes, so without this a hostile handshake error can +// clear the screen, reposition the cursor, or embed a newline followed by text +// shaped like a real entry and forge a row for a server that does not exist. +// +// Escape sequences are consumed whole rather than dropping ESC alone: removing +// the ESC and leaving "[2J" behind would print visible junk, and an abandoned +// OSC payload can still smuggle a title-set or a hyperlink. +// stripTerminalRejoiners removes the bytes that DISAPPEAR WITHOUT LEAVING A GAP: +// escape sequences, and every other control byte except the newline, carriage +// return and tab that become spaces upstream. +// +// The name is the point. These bytes do not merely vanish, they close up behind +// themselves, so text on either side of one becomes adjacent. A server that +// echoes a credential back with an escape inserted into the middle of it sends a +// value that matches nothing during redaction and is reassembled into the intact +// credential here. Splitting this out of sanitizeTerminalReason lets redaction +// run against the same text the reader will eventually see, without inheriting +// the display truncation, which would cut a secret in half and leave the head of +// it unmatched. +// +// Newline, carriage return and tab are deliberately left alone: upstream turns +// them into spaces that survive the collapse, so they separate rather than +// rejoin, and normalizing them here would delete the word boundaries the reason +// is easier to read with. +func stripTerminalRejoiners(value string) string { + var out strings.Builder + out.Grow(len(value)) + runes := []rune(value) + for index := 0; index < len(runes); index++ { + current := runes[index] + if current == 0x1b { + index++ + if index >= len(runes) { + break + } + switch runes[index] { + case '[': // CSI: parameters, then a final byte in @ to ~ + index++ + for index < len(runes) && (runes[index] < '@' || runes[index] > '~') { + index++ + } + case ']': // OSC: runs until BEL or ST + index++ + for index < len(runes) { + if runes[index] == 0x07 { + break + } + if runes[index] == 0x1b && index+1 < len(runes) && runes[index+1] == '\\' { + index++ + break + } + index++ + } + } + continue + } + if current == '\n' || current == '\r' || current == '\t' { + out.WriteRune(current) + continue + } + // Every other control byte is dropped: none carries a display meaning + // worth preserving here. + if current < 0x20 || current == 0x7f || (current >= 0x80 && current <= 0x9f) { + continue + } + // Unicode format characters rejoin exactly like control bytes do, and + // they are the more comfortable way to do it: a zero-width space or a + // soft hyphen inside a credential is invisible on the terminal, so the + // reader sees an unbroken secret while equality redaction saw two + // fragments. Dropping the whole Cf category covers the zero-width + // characters, the word joiner, the bidi controls and the byte order + // mark together. Combining marks are deliberately NOT dropped: they are + // ordinary content in most of the world's scripts. + if unicode.Is(unicode.Cf, current) { + continue + } + // DEFAULT-IGNORABLE MARKS REJOIN THE SAME WAY, and they are Mn rather than + // Cf, so dropping Cf alone left them. A combining grapheme joiner or a + // variation selector inside a credential renders as nothing, so the reader + // sees an unbroken secret while equality redaction saw two fragments. + // + // Only the ignorable subset is dropped, NOT the Mn category: an ordinary + // combining acute is content in most of the world's scripts, and deleting + // it would corrupt the diagnostic this pass exists to show. + if unicode.Is(unicode.Other_Default_Ignorable_Code_Point, current) || + unicode.Is(unicode.Variation_Selector, current) { + continue + } + out.WriteRune(current) + } + return out.String() +} + +func sanitizeTerminalReason(value string) string { + if len(value) > maxMCPReasonRawLen { + value = value[:maxMCPReasonRawLen] + // The cut lands on an arbitrary byte. Drop a rune the bound split so the + // panel never shows a replacement character it produced itself. + for len(value) > 0 { + decoded, width := utf8.DecodeLastRuneInString(value) + if decoded != utf8.RuneError || width > 1 { + break + } + value = value[:len(value)-1] + } + } + var out strings.Builder + for _, current := range stripTerminalRejoiners(value) { + // Newlines and tabs become spaces so the reason stays on the single row + // the panel counted for it. + if current == '\n' || current == '\r' || current == '\t' { + out.WriteRune(' ') + continue + } + out.WriteRune(current) + } + // Fields also collapses the runs of spaces the substitutions above create. + collapsed := strings.Join(strings.Fields(out.String()), " ") + if trimmed := []rune(collapsed); len(trimmed) > maxMCPReasonLen { + // Truncate by rune so a multi-byte character is never cut in half. + collapsed = string(trimmed[:maxMCPReasonLen]) + "..." + } + return collapsed +} + func mcpToolLines(tools []MCPToolView) []string { grouped := map[string][]MCPToolView{} order := []string{} diff --git a/internal/tui/model.go b/internal/tui/model.go index 9473de06a..6acdd55ff 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -96,14 +96,30 @@ type model struct { // other language servers) stay warm โ€” a fresh manager per run would cold-start // the server on the first edit of every turn. Nil when cwd is unknown; runs then // fall back to a per-run manager. Torn down in quit(). - lspManager *lsp.Manager - sessionStore *sessions.Store - peerService *peermsg.Service - peerInbox []peermsg.InboundMessage - peerApprovalQueue []peermsg.InboundMessage - peerPendingApproval *peermsg.InboundMessage - sandboxStore *sandbox.GrantStore - mcpConfig config.MCPConfig + lspManager *lsp.Manager + sessionStore *sessions.Store + peerService *peermsg.Service + peerInbox []peermsg.InboundMessage + peerApprovalQueue []peermsg.InboundMessage + peerPendingApproval *peermsg.InboundMessage + sandboxStore *sandbox.GrantStore + mcpConfig config.MCPConfig + mcpSkipped []internalmcp.SkippedServer + // mcpSkippedCredentials fingerprints the credential material that existed + // when mcpSkipped was captured, so a later render cannot re-derive a weaker + // redaction for the same retained error. See staleMCPObservation. + mcpSkippedCredentials string + // mcpLateSkipped pulls failures that were recorded after this model was + // built, and mcpStartupConfig is the configuration they were observed + // against, so the same invalidation the startup snapshot gets can be applied + // to them. mcpLateSkippedCount is what the cached view state was built from, + // so a new arrival invalidates the cache without a config change. + mcpLateSkipped func() []internalmcp.SkippedServer + // mcpStartupCompleted closes when the background registration finishes, so an + // already-open manager can be told to rebuild rather than waiting for input. + mcpStartupCompleted <-chan struct{} + mcpStartupConfig config.MCPConfig + mcpLateSkippedCount int mcpPermissionStore *internalmcp.PermissionStore mcpTokenStore *internalmcp.TokenStore mcpCommand func(context.Context, []string) MCPCommandResult @@ -994,6 +1010,11 @@ func newModel(ctx context.Context, options Options) model { peerService: options.PeerService, sandboxStore: sandboxStore, mcpConfig: options.MCPConfig, + mcpSkipped: options.MCPSkipped, + mcpLateSkipped: options.MCPLateSkipped, + mcpStartupCompleted: options.MCPStartupCompleted, + mcpStartupConfig: options.MCPConfig, + mcpSkippedCredentials: mcpCredentialFingerprint(options.MCPTokenStore.SecretValues()), mcpPermissionStore: options.MCPPermissionStore, mcpTokenStore: options.MCPTokenStore, mcpCommand: options.MCPCommand, @@ -1144,6 +1165,9 @@ func (m model) armComposerBlink() (model, tea.Cmd) { func (m model) Init() tea.Cmd { cmds := []tea.Cmd{textinput.Blink, composerBlinkCmd(m.composerBlinkSeq)} + if m.mcpStartupCompleted != nil { + cmds = append(cmds, waitForMCPStartupCompletion(m.mcpStartupCompleted)) + } if m.petAnimation != nil && !m.reducedMotion { cmds = append(cmds, petTickCmd(m.petTickSeq, m.petFrameDelay())) } @@ -1477,6 +1501,13 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { return m, dragEdgeScrollTickCmd(m.edgeScrollSeq) case providerWizardOAuthMsg: return m.applyProviderWizardOAuth(msg) + case mcpStartupCompletedMsg: + // The background registration is done, so the observations it produced are + // available now. Rebuilding HERE is what makes an already-open manager show + // them: nothing else schedules a render once Bubble Tea is idle, so a getter + // that reports late failures is not on its own enough. + m.refreshMCPViewState() + return m, nil case aimlapiOnboardMsg: return m.applyAimlapiOnboard(msg) case aimlapiExistingBalanceMsg: diff --git a/internal/tui/options.go b/internal/tui/options.go index e73c7eaa5..7db044dd7 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -51,10 +51,30 @@ type Options struct { // AwaitToolReadiness gives prompt-critical integration startup a bounded // chance to publish its tools before this turn snapshots the registry. The // wait runs inside the asynchronous agent command, so the TUI stays usable. - AwaitToolReadiness func(context.Context) - SessionStore *sessions.Store - SandboxStore *sandbox.GrantStore - MCPConfig config.MCPConfig + AwaitToolReadiness func(context.Context) + SessionStore *sessions.Store + SandboxStore *sandbox.GrantStore + MCPConfig config.MCPConfig + // MCPSkipped carries the servers that failed to start, so /mcp can report + // what is actually running rather than what is configured. Startup already + // records these; without them the panel derives state from config alone and + // shows a server that never connected as "enabled" with no explanation. + MCPSkipped []mcp.SkippedServer + // MCPLateSkipped reports failures that were not known when the model was + // built. Optional servers are registered on a background goroutine so a slow + // one cannot delay the first response, which means their failures land after + // this surface exists; MCPSkipped is a snapshot and cannot carry them. + // Optional: nil means every failure was already known. + MCPLateSkipped func() []mcp.SkippedServer + // MCPStartupCompleted closes when the background MCP registration has + // finished, so an ALREADY-OPEN manager can be told to rebuild. + // + // A getter that reports late failures is not enough on its own: nothing + // schedules another render, so an overlay opened while an optional server was + // still connecting keeps showing the configuration-derived enabled state until + // unrelated input or a resize happens to redraw it. Optional: nil means there + // is no background registration to wait for. + MCPStartupCompleted <-chan struct{} MCPPermissionStore *mcp.PermissionStore MCPTokenStore *mcp.TokenStore MCPCommand func(context.Context, []string) MCPCommandResult