From db7ab858a884d3d18eef0f9e4bee9030319d30ab Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:16:12 -0500 Subject: [PATCH] feat(mcp): resolve MCP server secrets from the credential store Enabling an MCP server that needs secrets meant writing those secrets into the config file. Server entries can now name a credential instead: `envFrom` maps an environment variable to a credential-store name, so the file holds the name and the store holds the value. References resolve at stdio spawn and are merged over the verbatim env. Every failure names the credential and never the value, and lookups run in sorted order so a server missing several always names the same one first. Ships a memlawb entry, disabled, with its URL and namespace verbatim and its passphrase and API key as references. memlawb is a zero-knowledge memory backend whose passphrase is the encryption key, which is exactly the kind of value that should not sit in a config file. Seeding a disabled default took more than the flag. `zero mcp enable` enables by deleting the "disabled" key, which is right for a default that ships enabled and wrong for one that does not: with the key gone the seeded value survives the merge, so the command reported success and changed nothing. Enabling a ships-disabled default now writes an explicit false, and the current state of an entry with no key is read as the default's shipped state. Two files outside the planned set had to change, both because the new field opened a path through them. The merge had no clause for it, so a user-level reference was silently dropped when merged over the default. And a project config that retargets command or url while inheriting the user's references would hand a repository-chosen binary the user's stored passphrase, which is the same hole the existing headers and env guard exists to close; that guard now covers references too. The unconfigured-default check needed proving rather than changing. It compares whole structs, so the new field joins automatically, and a hand-written comparison would have ignored it silently. Both directions are now pinned, along with the resolved default surviving the retired-default migration. One guard is deliberately not claimed as proven: the early return that keeps a server without references from opening the store is pinned by a zero-lookups test, but removing it does not turn that test red, because an empty reference map resolves to an empty loop either way. The scenario that matters most is still manual and nothing here stands in for it: with the memlawb standalone binary on PATH and neither Node nor Bun installed, storing both secrets, enabling, and completing a save and a recall. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- internal/cli/extensions.go | 8 + internal/cli/mcp_config.go | 176 +++++++++++++++++++- internal/cli/mcp_secret_test.go | 199 ++++++++++++++++++++++ internal/config/mcp_defaults.go | 52 ++++++ internal/config/mcp_defaults_test.go | 144 ++++++++++++++++ internal/config/mcp_merge.go | 11 +- internal/config/types.go | 17 +- internal/mcp/client.go | 80 ++++++++- internal/mcp/client_credentials_test.go | 210 ++++++++++++++++++++++++ internal/mcp/config.go | 20 ++- internal/mcp/config_test.go | 70 ++++++++ 11 files changed, 972 insertions(+), 15 deletions(-) create mode 100644 internal/cli/mcp_secret_test.go create mode 100644 internal/mcp/client_credentials_test.go diff --git a/internal/cli/extensions.go b/internal/cli/extensions.go index 8dd1e9f00..71005f7d0 100644 --- a/internal/cli/extensions.go +++ b/internal/cli/extensions.go @@ -171,6 +171,8 @@ func runMCPWithContext(ctx context.Context, args []string, stdout io.Writer, std return runMCPToggle(args[1:], stdout, stderr, deps, true) case "check": return runMCPCheck(ctx, args[1:], stdout, stderr, deps) + case "secret": + return runMCPSecret(args[1:], stdout, stderr, deps) case "permissions": return runMCPPermissions(args[1:], stdout, stderr, deps) case "tools": @@ -619,7 +621,13 @@ Commands: list List configured MCP servers, or tools with --tools oauth Manage OAuth credentials for remote MCP servers permissions Manage persistent MCP tool permissions + secret set Store a secret an MCP server references by name (envFrom) tools Inspect configured MCP tools + +Memory (memlawb): store the two secrets, then turn it on. + zero mcp secret set memlawb-passphrase + zero mcp secret set memlawb-api-key + zero mcp enable memlawb `) return err } diff --git a/internal/cli/mcp_config.go b/internal/cli/mcp_config.go index c8c56fab6..fcce853af 100644 --- a/internal/cli/mcp_config.go +++ b/internal/cli/mcp_config.go @@ -10,6 +10,8 @@ import ( "runtime" "strings" + "github.com/charmbracelet/x/term" + "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/mcp" "github.com/Gitlawb/zero/internal/redaction" @@ -235,9 +237,147 @@ func runMCPToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDep } else if _, err := fmt.Fprintf(stdout, "MCP server %s was already %s in %s.\n", serverName, state, configPath); err != nil { return exitCrash } + if !disabled { + if _, err := fmt.Fprint(stdout, mcpEnableNotice(serverName)); err != nil { + return exitCrash + } + } + return exitSuccess +} + +// mcpEnableNotice returns the extra guidance `zero mcp enable ` prints +// for a server whose secrets are credential references, or "" for every other +// server. +// +// It names the minimum memlawb version deliberately. The credential-reference +// field is unknown to older zero binaries: such a binary drops envFrom when it +// reads config.json and preserves it when it rewrites the file, so the child +// starts with no passphrase and dies on memlawb's own missing-passphrase check, +// with nothing in that error pointing at the stale binary or the dropped field. +func mcpEnableNotice(serverName string) string { + if strings.TrimSpace(serverName) != "memlawb" { + return "" + } + return fmt.Sprintf(` +memlawb needs two secrets. They live in Zero's credential store, never in config.json: + + zero mcp secret set %s + zero mcp secret set %s + +Requires memlawb %s or newer on your PATH (the release whose `+"`memlawb mcp`"+` reads +MEMLAWB_PASSPHRASE from its environment); check with `+"`memlawb --version`"+`. +`, config.MemlawbPassphraseCredential, config.MemlawbAPIKeyCredential, config.MemlawbMinimumVersion) +} + +// runMCPSecret stores a named secret in Zero's credential store, so an MCP +// server entry can reference it by name (envFrom) instead of carrying its value +// in config.json. +func runMCPSecret(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + if len(args) == 0 { + return writeExecUsageError(stderr, "usage: zero mcp secret set ") + } + switch args[0] { + case "-h", "--help", "help": + if err := writeMCPSecretHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + case "set": + return runMCPSecretSet(args[1:], stdout, stderr, deps) + default: + return writeExecUsageError(stderr, fmt.Sprintf("unknown mcp secret subcommand %q", args[0])) + } +} + +func runMCPSecretSet(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + options, positional, help, err := parseMCPConfigPositionalCommand(args, "secret set") + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if help { + if err := writeMCPSecretHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + } + if len(positional) != 1 { + return writeExecUsageError(stderr, "usage: zero mcp secret set [--json]") + } + name := strings.ToLower(strings.TrimSpace(positional[0])) + if name == "" { + return writeExecUsageError(stderr, "credential name is required") + } + + value, err := readMCPSecretValue(deps.stdin, stdout, name) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, "failed to resolve user config: "+err.Error(), exitCrash) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if err := store.Set(name, value); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + + if options.json { + payload := struct { + Credential string `json:"credential"` + Stored bool `json:"stored"` + Backend string `json:"backend"` + }{Credential: name, Stored: true, Backend: store.Backend()} + if err := writePrettyJSON(stdout, payload); err != nil { + return exitCrash + } + return exitSuccess + } + // The value is never echoed back, on any path. + if _, err := fmt.Fprintf(stdout, "Stored credential %s (%s backend).\nReference it from an MCP server with \"envFrom\": {\"SOME_VAR\": \"%s\"}.\n", name, store.Backend(), name); err != nil { + return exitCrash + } return exitSuccess } +// readMCPSecretValue takes the secret from an interactive prompt when stdin is a +// terminal (echo off) and from standard input otherwise, so the value can be +// piped in without ever appearing in a shell history or an argv. +func readMCPSecretValue(stdin io.Reader, stdout io.Writer, name string) (string, error) { + if file, ok := stdin.(*os.File); ok && term.IsTerminal(file.Fd()) { + if _, err := fmt.Fprintf(stdout, "Value for %s (input hidden): ", name); err != nil { + return "", err + } + typed, err := term.ReadPassword(file.Fd()) + if _, printErr := fmt.Fprintln(stdout); printErr != nil { + return "", printErr + } + if err != nil { + return "", fmt.Errorf("read %s: %w", name, err) + } + value := strings.TrimSpace(string(typed)) + if value == "" { + return "", execUsageError{fmt.Sprintf("no value entered for %s", name)} + } + return value, nil + } + if stdin == nil { + return "", execUsageError{fmt.Sprintf("no value for %s on standard input", name)} + } + data, err := io.ReadAll(stdin) + if err != nil { + return "", fmt.Errorf("read %s from standard input: %w", name, err) + } + value := strings.TrimSpace(string(data)) + if value == "" { + return "", execUsageError{fmt.Sprintf("no value for %s on standard input", name)} + } + return value, nil +} + func runMCPCheck(ctx context.Context, args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { if ctx == nil { ctx = context.Background() @@ -782,20 +922,33 @@ func (cfg *mcpWritableConfig) setServerDisabled(name string, disabled bool) (boo server = map[string]json.RawMessage{} } - current := false + // With no "disabled" key in the entry, the current state is whatever the + // built-in default ships as — false for a default that ships enabled, true + // for one like memlawb that does not. Assuming false here made + // `zero mcp enable memlawb` report "already enabled" and write nothing. + current := config.DefaultMCPServerShipsDisabled(name) if rawDisabled, ok := server["disabled"]; ok && len(rawDisabled) > 0 && string(rawDisabled) != "null" { if err := json.Unmarshal(rawDisabled, ¤t); err != nil { return false, false, err } } changed := current != disabled - if disabled { + switch { + case disabled: data, err := json.Marshal(true) if err != nil { return false, false, err } server["disabled"] = data - } else { + case config.DefaultMCPServerShipsDisabled(name): + // Deleting the key would leave the seeded Disabled:true in force: the + // merge only lifts a disable when the user layer states one explicitly. + data, err := json.Marshal(false) + if err != nil { + return false, false, err + } + server["disabled"] = data + default: delete(server, "disabled") } data, err := json.Marshal(server) @@ -985,6 +1138,23 @@ Flags: return err } +func writeMCPSecretHelp(w io.Writer) error { + _, err := fmt.Fprint(w, `Usage: + zero mcp secret set [flags] + +Stores a secret in Zero's credential store under . The value is read from +standard input, or prompted for (hidden) when stdin is a terminal, so it never +appears in argv or shell history. An MCP server entry then references it by name: + + "envFrom": { "MEMLAWB_PASSPHRASE": "memlawb-passphrase" } + +Flags: + --json Print command result as JSON + -h, --help Show this help +`) + return err +} + func writeMCPCheckHelp(w io.Writer) error { _, err := fmt.Fprint(w, `Usage: zero mcp check [flags] diff --git a/internal/cli/mcp_secret_test.go b/internal/cli/mcp_secret_test.go new file mode 100644 index 000000000..2f05a07be --- /dev/null +++ b/internal/cli/mcp_secret_test.go @@ -0,0 +1,199 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +const ( + testMemlawbPassphrase = "correct-horse-battery-staple" + testMemlawbAPIKey = "mk_live_zero_test_key" +) + +func storeMemlawbSecrets(t *testing.T, configPath string) { + t.Helper() + for name, value := range map[string]string{ + config.MemlawbPassphraseCredential: testMemlawbPassphrase, + config.MemlawbAPIKeyCredential: testMemlawbAPIKey, + } { + var out, errBuf bytes.Buffer + code := runWithDeps([]string{"mcp", "secret", "set", name}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + stdin: strings.NewReader(value + "\n"), + }) + if code != exitSuccess { + t.Fatalf("secret set %s exit=%d stderr=%s", name, code, errBuf.String()) + } + if strings.Contains(out.String(), value) { + t.Fatalf("secret set echoed the value: %q", out.String()) + } + } +} + +func TestRunMCPSecretSetStoresValueFromStdin(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeMCPCommandRawConfig(t, configPath, `{"activeProvider":"fast"}`) + storeMemlawbSecrets(t, configPath) + + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + value, ok, err := store.Get(config.MemlawbPassphraseCredential) + if err != nil { + t.Fatal(err) + } + if !ok || value != testMemlawbPassphrase { + t.Fatalf("stored passphrase = %q, %v", value, ok) + } + // The value must not land in config.json. + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(data), testMemlawbPassphrase) { + t.Fatalf("config.json carries the secret: %s", data) + } +} + +func TestRunMCPSecretSetRejectsEmptyValue(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + var out, errBuf bytes.Buffer + code := runWithDeps([]string{"mcp", "secret", "set", "memlawb-passphrase"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + stdin: strings.NewReader(" \n"), + }) + if code == exitSuccess { + t.Fatalf("an empty value must not be stored, stdout=%q", out.String()) + } +} + +// AE2: enabling memlawb with both secrets in the credential store writes +// reference NAMES to config.json and neither VALUE. +func TestRunMCPEnableMemlawbWritesReferencesNotValues(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeMCPCommandRawConfig(t, configPath, `{"activeProvider":"fast"}`) + storeMemlawbSecrets(t, configPath) + + var out, errBuf bytes.Buffer + code := runWithDeps([]string{"mcp", "enable", "memlawb"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("enable exit=%d stderr=%s", code, errBuf.String()) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + written := string(data) + if strings.Contains(written, testMemlawbPassphrase) || strings.Contains(written, testMemlawbAPIKey) { + t.Fatalf("config.json carries a secret value: %s", written) + } + + cfg, err := config.ResolveMCP(config.ResolveOptions{UserConfigPath: configPath}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + memlawb := cfg.Servers["memlawb"] + if memlawb.Disabled { + t.Fatalf("`mcp enable memlawb` must turn the seeded default on: %#v", memlawb) + } + if memlawb.EnvFrom["MEMLAWB_PASSPHRASE"] != config.MemlawbPassphraseCredential || + memlawb.EnvFrom["MEMLAWB_API_KEY"] != config.MemlawbAPIKeyCredential { + t.Fatalf("resolved server lost its credential references: %#v", memlawb) + } + if memlawb.Env["MEMLAWB_PASSPHRASE"] != "" || memlawb.Env["MEMLAWB_API_KEY"] != "" { + t.Fatalf("secrets must never be verbatim env: %#v", memlawb.Env) + } +} + +// Positive control for the assertion above: the same read of the same writer +// DOES find a secret when one is written inline, so the absence assertion is +// not passing against a writer that writes nothing. +func TestRunMCPAddInlineSecretsAreWrittenToConfig(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeMCPCommandRawConfig(t, configPath, `{"activeProvider":"fast"}`) + + var out, errBuf bytes.Buffer + code := runWithDeps([]string{ + "mcp", "add", "memlawb", + "--env", "MEMLAWB_PASSPHRASE=" + testMemlawbPassphrase, + "--env", "MEMLAWB_API_KEY=" + testMemlawbAPIKey, + "--", "memlawb", "mcp", + }, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("add exit=%d stderr=%s", code, errBuf.String()) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + written := string(data) + if !strings.Contains(written, testMemlawbPassphrase) || !strings.Contains(written, testMemlawbAPIKey) { + t.Fatalf("inline env values should be written verbatim; the absence check above is worthless without this: %s", written) + } +} + +func TestRunMCPEnableMemlawbPrintsMinimumVersion(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeMCPCommandRawConfig(t, configPath, `{"activeProvider":"fast"}`) + + var out, errBuf bytes.Buffer + code := runWithDeps([]string{"mcp", "enable", "memlawb"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("enable exit=%d stderr=%s", code, errBuf.String()) + } + printed := out.String() + if !strings.Contains(printed, config.MemlawbMinimumVersion) { + t.Fatalf("enable must print the minimum memlawb version: %q", printed) + } + for _, want := range []string{"zero mcp secret set", config.MemlawbPassphraseCredential, config.MemlawbAPIKeyCredential} { + if !strings.Contains(printed, want) { + t.Fatalf("enable output missing %q:\n%s", want, printed) + } + } + // The notice is memlawb-specific: enabling another default must not carry it. + out.Reset() + code = runWithDeps([]string{"mcp", "enable", "exa"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("enable exa exit=%d stderr=%s", code, errBuf.String()) + } + if strings.Contains(out.String(), config.MemlawbMinimumVersion) { + t.Fatalf("the memlawb notice leaked onto another server: %q", out.String()) + } +} + +func TestRunMCPDisableMemlawbAfterEnable(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeMCPCommandRawConfig(t, configPath, `{"activeProvider":"fast"}`) + var out, errBuf bytes.Buffer + if code := runWithDeps([]string{"mcp", "enable", "memlawb"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }); code != exitSuccess { + t.Fatalf("enable exit=%d stderr=%s", code, errBuf.String()) + } + if code := runWithDeps([]string{"mcp", "disable", "memlawb"}, &out, &errBuf, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }); code != exitSuccess { + t.Fatalf("disable exit=%d stderr=%s", code, errBuf.String()) + } + cfg, err := config.ResolveMCP(config.ResolveOptions{UserConfigPath: configPath}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if !cfg.Servers["memlawb"].Disabled { + t.Fatal("disable must turn it back off") + } +} diff --git a/internal/config/mcp_defaults.go b/internal/config/mcp_defaults.go index 5047883a4..27e6c0f44 100644 --- a/internal/config/mcp_defaults.go +++ b/internal/config/mcp_defaults.go @@ -14,15 +14,67 @@ import ( // // Exa's hosted MCP server works anonymously with rate limits. Users can add an // Exa API key for higher limits. +// +// memlawb is the exception: it ships DISABLED because it cannot run without a +// passphrase and a service key, which only the user can supply. Its two secrets +// are credential references (EnvFrom), so `zero mcp secret set` puts the values +// in the credential store and config.json only ever names them: +// +// zero mcp secret set memlawb-passphrase +// zero mcp enable memlawb func DefaultMCPServers() map[string]MCPServerConfig { return map[string]MCPServerConfig{ "exa": { Type: "http", URL: "https://mcp.exa.ai/mcp", }, + "memlawb": { + Type: "stdio", + Command: "memlawb", + Args: []string{"mcp"}, + Env: map[string]string{ + "MEMLAWB_URL": "https://memory.gitlawb.com", + "MEMLAWB_NAMESPACE": "user:me", + }, + EnvFrom: map[string]string{ + "MEMLAWB_PASSPHRASE": MemlawbPassphraseCredential, + "MEMLAWB_API_KEY": MemlawbAPIKeyCredential, + }, + Disabled: true, + }, } } +const ( + // MemlawbPassphraseCredential and MemlawbAPIKeyCredential are the credential + // store names the seeded memlawb entry points at. They are names, never + // values. + MemlawbPassphraseCredential = "memlawb-passphrase" + MemlawbAPIKeyCredential = "memlawb-api-key" + + // MemlawbMinimumVersion is the oldest memlawb release whose `memlawb mcp` + // reads MEMLAWB_PASSPHRASE from its environment. `zero mcp enable memlawb` + // prints it because the failure mode is otherwise unreadable: an older zero + // binary drops the unknown envFrom field when it reads config.json and + // preserves it when it rewrites the file, so the child dies on its + // missing-passphrase check with nothing pointing at the stale binary. + MemlawbMinimumVersion = "0.1.0" +) + +// DefaultMCPServerShipsDisabled reports whether Zero's built-in default for name +// is seeded with the disabled flag set. +// +// It exists for the enable path. Enabling normally means deleting the "disabled" +// key from the user's entry, which is correct for a default that ships enabled +// and wrong for one that does not: with the key absent, mergeMCPServer sees no +// explicit decision from the user layer and the seeded Disabled:true survives, +// so `zero mcp enable memlawb` would report success and change nothing. Such a +// default needs an explicit "disabled": false written instead. +func DefaultMCPServerShipsDisabled(name string) bool { + server, ok := DefaultMCPServers()[strings.TrimSpace(name)] + return ok && server.Disabled +} + // IsDefaultMCPServer reports whether name is one of Zero's built-in default MCP // servers. The config commands use it so a default can be disabled/enabled even // though it is not written to the user's config file until overridden. diff --git a/internal/config/mcp_defaults_test.go b/internal/config/mcp_defaults_test.go index 9f54f7fe9..a6401bd9e 100644 --- a/internal/config/mcp_defaults_test.go +++ b/internal/config/mcp_defaults_test.go @@ -3,6 +3,8 @@ package config import ( "os" "path/filepath" + "reflect" + "strings" "testing" ) @@ -291,3 +293,145 @@ func TestResolveMCPDisabledRetiredEntryStaysReEnableable(t *testing.T) { t.Fatal("the disable must still carry to the successor") } } + +func TestDefaultMCPServersSeedsMemlawbDisabled(t *testing.T) { + memlawb, ok := DefaultMCPServers()["memlawb"] + if !ok { + t.Fatal("expected a memlawb default entry") + } + if !memlawb.Disabled { + t.Fatal("memlawb must ship disabled: it needs a passphrase and a key before it can run") + } + if memlawb.Command != "memlawb" || len(memlawb.Args) != 1 || memlawb.Args[0] != "mcp" { + t.Fatalf("unexpected memlawb launch: %#v", memlawb) + } + if memlawb.Env["MEMLAWB_URL"] == "" || memlawb.Env["MEMLAWB_NAMESPACE"] == "" { + t.Fatalf("url and namespace are not secrets and belong in env verbatim: %#v", memlawb.Env) + } + if memlawb.EnvFrom["MEMLAWB_PASSPHRASE"] == "" || memlawb.EnvFrom["MEMLAWB_API_KEY"] == "" { + t.Fatalf("the passphrase and the key must be credential references: %#v", memlawb.EnvFrom) + } + for key := range memlawb.Env { + if key == "MEMLAWB_PASSPHRASE" || key == "MEMLAWB_API_KEY" { + t.Fatalf("secret %s must never be a verbatim env value", key) + } + } +} + +func TestResolveMCPSeedsMemlawbDisabled(t *testing.T) { + cfg, err := ResolveMCP(ResolveOptions{}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + memlawb, ok := cfg.Servers["memlawb"] + if !ok { + t.Fatal("expected the memlawb default to be seeded with no user config") + } + if !memlawb.Disabled { + t.Fatalf("the memlawb default must resolve disabled: %#v", memlawb) + } +} + +func TestResolveMCPUserCanEnableMemlawbDefault(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"mcp":{"servers":{"memlawb":{"disabled":false}}}}`), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := ResolveMCP(ResolveOptions{UserConfigPath: path}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + memlawb := cfg.Servers["memlawb"] + if memlawb.Disabled { + t.Fatalf("an explicit enable must lift the shipped disable: %#v", memlawb) + } + if memlawb.Command != "memlawb" || memlawb.EnvFrom["MEMLAWB_PASSPHRASE"] == "" { + t.Fatalf("enabling must keep the seeded launch and references: %#v", memlawb) + } +} + +func TestResolveMCPUserEnvFromOverridesDefault(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"mcp":{"servers":{"memlawb":{"disabled":false,"envFrom":{"MEMLAWB_PASSPHRASE":"work-passphrase"}}}}}`), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := ResolveMCP(ResolveOptions{UserConfigPath: path}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if got := cfg.Servers["memlawb"].EnvFrom["MEMLAWB_PASSPHRASE"]; got != "work-passphrase" { + t.Fatalf("user credential reference did not survive the merge: %q", got) + } +} + +func TestIsUnconfiguredDefaultTracksEnvFrom(t *testing.T) { + if !IsUnconfiguredDefault("memlawb", DefaultMCPServers()["memlawb"]) { + t.Fatal("an untouched memlawb default should be reported as unconfigured") + } + custom := DefaultMCPServers()["memlawb"] + custom.EnvFrom = map[string]string{"MEMLAWB_PASSPHRASE": "work-passphrase"} + if IsUnconfiguredDefault("memlawb", custom) { + t.Fatal("a server pointing at its own credential names is no longer unconfigured") + } +} + +func TestResolveMCPRetiredDefaultMigrationLeavesMemlawbAlone(t *testing.T) { + // The firecrawl -> exa carry must not reach any other default. + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"mcp":{"servers":{"firecrawl":{"disabled":true}}}}`), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := ResolveMCP(ResolveOptions{UserConfigPath: path}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if !cfg.Servers["exa"].Disabled { + t.Fatal("the disable must still carry to the successor") + } + memlawb := cfg.Servers["memlawb"] + if !reflect.DeepEqual(memlawb, DefaultMCPServers()["memlawb"]) { + t.Fatalf("the retired-default migration must leave memlawb exactly as seeded: %#v", memlawb) + } +} + +func TestResolveMCPProjectCannotRetargetWhileInheritingCredentialReferences(t *testing.T) { + // Credential references are inheritable credential material like env and + // headers: a project entry that swaps the command while inheriting them + // gets the user's secrets resolved into a binary the repo chose. + dir := t.TempDir() + userPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(userPath, []byte(`{"mcp":{"servers":{"custom":{"command":"real-mcp","envFrom":{"TOKEN":"some-credential"}}}}}`), 0o600); err != nil { + t.Fatal(err) + } + projectPath := filepath.Join(dir, "project.json") + if err := os.WriteFile(projectPath, []byte(`{"mcp":{"servers":{"custom":{"command":"repo-mcp"}}}}`), 0o600); err != nil { + t.Fatal(err) + } + _, err := ResolveMCP(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath}) + if err == nil { + t.Fatal("ResolveMCP error = nil, want a refusal to retarget while inheriting credentials") + } + if !strings.Contains(err.Error(), "custom") { + t.Fatalf("error = %v, want the server named", err) + } +} + +func TestResolveMCPProjectMayRetargetWhenItNamesItsOwnCredentials(t *testing.T) { + // Control for the refusal above: nothing is inherited, so the merge stands. + dir := t.TempDir() + userPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(userPath, []byte(`{"mcp":{"servers":{"custom":{"command":"real-mcp","envFrom":{"TOKEN":"some-credential"}}}}}`), 0o600); err != nil { + t.Fatal(err) + } + projectPath := filepath.Join(dir, "project.json") + if err := os.WriteFile(projectPath, []byte(`{"mcp":{"servers":{"custom":{"command":"repo-mcp","envFrom":{"TOKEN":"repo-credential"}}}}}`), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := ResolveMCP(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath}) + if err != nil { + t.Fatalf("ResolveMCP: %v", err) + } + if got := cfg.Servers["custom"].EnvFrom["TOKEN"]; got != "repo-credential" { + t.Fatalf("project reference did not apply: %q", got) + } +} diff --git a/internal/config/mcp_merge.go b/internal/config/mcp_merge.go index 4a1717f38..d7133e6c9 100644 --- a/internal/config/mcp_merge.go +++ b/internal/config/mcp_merge.go @@ -33,7 +33,7 @@ func mergeProjectMCPConfig(dst *MCPConfig, src MCPConfig) error { base := dst.Servers[name] candidate := mergeMCPServer(base, server, false) if projectMCPServerTargetChanges(base, server) && hasInheritedMCPCredentialMaterial(server, candidate) { - return fmt.Errorf("project MCP server %q cannot override target while inheriting user credentials; set headers/env/oauth explicitly or use a new server name", name) + return fmt.Errorf("project MCP server %q cannot override target while inheriting user credentials; set headers/env/envFrom/oauth explicitly or use a new server name", name) } candidate.ProjectConfigured = true dst.Servers[name] = candidate @@ -58,6 +58,9 @@ func mergeMCPServer(base MCPServerConfig, next MCPServerConfig, canReenable bool if next.Env != nil { base.Env = copyMCPStringMap(next.Env) } + if next.EnvFrom != nil { + base.EnvFrom = copyMCPStringMap(next.EnvFrom) + } if strings.TrimSpace(next.URL) != "" { base.URL = next.URL } @@ -133,6 +136,12 @@ func hasInheritedMCPCredentialMaterial(project MCPServerConfig, candidate MCPSer if project.Env == nil && hasMCPStringMapMaterial(candidate.Env) { return true } + // A credential reference carries no value, but inheriting one still hands + // the user's stored secret to whatever binary or endpoint the project layer + // just pointed the server at. + if project.EnvFrom == nil && hasMCPStringMapMaterial(candidate.EnvFrom) { + return true + } if project.OAuth == nil && hasMCPOAuthMaterial(candidate.OAuth) { return true } diff --git a/internal/config/types.go b/internal/config/types.go index 50b153179..8e68769f4 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -453,10 +453,17 @@ type MCPConfig struct { } type MCPServerConfig struct { - Type string `json:"type,omitempty"` - Command string `json:"command,omitempty"` - Args []string `json:"args,omitempty"` - Env map[string]string `json:"env,omitempty"` + Type string `json:"type,omitempty"` + Command string `json:"command,omitempty"` + Args []string `json:"args,omitempty"` + Env map[string]string `json:"env,omitempty"` + // EnvFrom maps a child environment variable to the NAME of a credential in + // Zero's credential store; the value never appears here. The launch path + // resolves each reference at spawn time (see mcp.Server.EnvFrom), so a + // secret's name lives in config.json while its value stays in the store. + // Env and EnvFrom are separate on purpose: a verbatim Env value is written + // to disk, and anything that must not be is named here instead. + EnvFrom map[string]string `json:"envFrom,omitempty"` URL string `json:"url,omitempty"` Headers map[string]string `json:"headers,omitempty"` Auth string `json:"auth,omitempty"` @@ -646,6 +653,7 @@ func (server *MCPServerConfig) UnmarshalJSON(data []byte) error { Command string `json:"command"` Args []string `json:"args"` Env map[string]string `json:"env"` + EnvFrom map[string]string `json:"envFrom"` URL string `json:"url"` Headers map[string]string `json:"headers"` Auth string `json:"auth"` @@ -661,6 +669,7 @@ func (server *MCPServerConfig) UnmarshalJSON(data []byte) error { server.Command = raw.Command server.Args = raw.Args server.Env = raw.Env + server.EnvFrom = raw.EnvFrom server.URL = raw.URL server.Headers = raw.Headers server.Auth = raw.Auth diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 064e7f213..9fbce3f46 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -9,11 +9,13 @@ import ( "io" "os" "os/exec" + "sort" "strconv" "strings" "sync" "time" + "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/execution" ) @@ -85,6 +87,75 @@ func Connect(ctx context.Context, server Server) (ToolClient, error) { type ConnectOptions struct { Execution *execution.Runner WorkspaceRoot string + // Credentials resolves a stdio server's EnvFrom references. nil opens the + // user's credential store, and only when a server actually names a + // reference — an ordinary server must not make startup touch the keyring. + Credentials CredentialResolver +} + +// CredentialResolver reads a named secret out of Zero's credential store. +// *credstore.Store satisfies it; tests inject a fake. +type CredentialResolver interface { + Get(name string) (value string, found bool, err error) +} + +// resolveCredentialEnv turns a stdio server's EnvFrom references into the +// environment values the child is spawned with. It is the only place a stored +// secret enters the launch path, and it runs before anything is spawned so a +// missing credential fails the connect rather than producing a server that +// starts and then dies on its own missing-config check. +// +// Every error names the CREDENTIAL, never the value: these strings reach logs, +// `zero mcp check`, and the startup warning line. +func resolveCredentialEnv(server Server, resolver CredentialResolver) (map[string]string, error) { + if len(server.EnvFrom) == 0 { + return nil, nil + } + if resolver == nil { + store, err := config.ProviderKeyStore() + if err != nil { + return nil, fmt.Errorf("start MCP server %s: open credential store: %w", server.Name, err) + } + resolver = store + } + variables := make([]string, 0, len(server.EnvFrom)) + for variable := range server.EnvFrom { + variables = append(variables, variable) + } + // Sorted so a server missing several credentials always names the same one. + sort.Strings(variables) + + resolved := make(map[string]string, len(server.EnvFrom)) + for _, variable := range variables { + credential := strings.TrimSpace(server.EnvFrom[variable]) + if credential == "" { + return nil, fmt.Errorf("start MCP server %s: envFrom %s names no credential", server.Name, variable) + } + value, found, err := resolver.Get(credential) + if err != nil { + return nil, fmt.Errorf("start MCP server %s: read credential %q: %w", server.Name, credential, err) + } + if !found || value == "" { + return nil, fmt.Errorf("start MCP server %s: credential %q is not stored; store it with `zero mcp secret set %s`", server.Name, credential, credential) + } + resolved[variable] = value + } + return resolved, nil +} + +// stdioEnv merges the resolved credentials over the server's verbatim env. +func stdioEnv(server Server, resolved map[string]string) map[string]string { + if len(resolved) == 0 { + return server.Env + } + merged := make(map[string]string, len(server.Env)+len(resolved)) + for key, value := range server.Env { + merged[key] = value + } + for key, value := range resolved { + merged[key] = value + } + return merged } func ConnectWithOptions(ctx context.Context, server Server, options ConnectOptions) (ToolClient, error) { @@ -137,6 +208,11 @@ func (b *boundedBuffer) String() string { } func connectStdio(ctx context.Context, server Server, options ConnectOptions) (*Client, error) { + resolved, err := resolveCredentialEnv(server, options.Credentials) + if err != nil { + return nil, err + } + childEnv := stdioEnv(server, resolved) var cmd *exec.Cmd var cleanup func() cleanupTransferred := false @@ -153,7 +229,7 @@ func connectStdio(ctx context.Context, server Server, options ConnectOptions) (* prepared, err := options.Execution.Prepare(ctx, execution.Request{ Origin: execution.OriginMCPServer, Mode: execution.ModeDurable, - Command: execution.Command{Name: server.Command, Args: append([]string(nil), server.Args...), Env: mergeProcessEnv(server.Env)}, + Command: execution.Command{Name: server.Command, Args: append([]string(nil), server.Args...), Env: mergeProcessEnv(childEnv)}, WorkingDirectory: workspaceRoot, WorkspaceRoots: []string{workspaceRoot}, Approval: execution.ApprovalContext{PolicyVersion: execution.PolicyVersion}, @@ -165,7 +241,7 @@ func connectStdio(ctx context.Context, server Server, options ConnectOptions) (* cleanup = prepared.Cleanup } else { cmd = exec.CommandContext(ctx, server.Command, server.Args...) - cmd.Env = mergeProcessEnv(server.Env) + cmd.Env = mergeProcessEnv(childEnv) } stdin, err := cmd.StdinPipe() if err != nil { diff --git a/internal/mcp/client_credentials_test.go b/internal/mcp/client_credentials_test.go new file mode 100644 index 000000000..98a08ee79 --- /dev/null +++ b/internal/mcp/client_credentials_test.go @@ -0,0 +1,210 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "os" + "strings" + "testing" + "time" +) + +// fakeCredentialStore stands in for Zero's credential store. It records every +// lookup so a test can prove a server with no references never reaches it. +type fakeCredentialStore struct { + values map[string]string + err error + lookups []string +} + +func (store *fakeCredentialStore) Get(name string) (string, bool, error) { + store.lookups = append(store.lookups, name) + if store.err != nil { + return "", false, store.err + } + value, ok := store.values[name] + return value, ok, nil +} + +func credentialHelperServer(t *testing.T, envFrom map[string]string, env map[string]string) Server { + t.Helper() + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + merged := map[string]string{"ZERO_MCP_ENV_HELPER": "1"} + for key, value := range env { + merged[key] = value + } + return Server{ + Name: "memlawb", + Type: ServerTypeStdio, + Command: executable, + Args: []string{"-test.run=TestMCPEnvHelperProcess", "--"}, + Env: merged, + EnvFrom: envFrom, + } +} + +func TestStdioCredentialReferencesReachChildEnvironment(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + store := &fakeCredentialStore{values: map[string]string{ + "memlawb-passphrase": "correct-horse-battery-staple", + "memlawb-api-key": "mk_live_fake", + }} + server := credentialHelperServer(t, + map[string]string{"MEMLAWB_PASSPHRASE": "memlawb-passphrase", "MEMLAWB_API_KEY": "memlawb-api-key"}, + map[string]string{"MEMLAWB_URL": "https://memory.gitlawb.com"}, + ) + + client, err := ConnectWithOptions(ctx, server, ConnectOptions{Credentials: store}) + if err != nil { + t.Fatalf("ConnectWithOptions() error = %v", err) + } + defer client.Close() + + read := func(name string) string { + result, err := client.CallTool(ctx, "env", map[string]any{"name": name}) + if err != nil { + t.Fatalf("CallTool(%s) error = %v", name, err) + } + return TextContent(result.Content) + } + if got := read("MEMLAWB_PASSPHRASE"); got != "correct-horse-battery-staple" { + t.Fatalf("child MEMLAWB_PASSPHRASE = %q, want the store's value", got) + } + if got := read("MEMLAWB_API_KEY"); got != "mk_live_fake" { + t.Fatalf("child MEMLAWB_API_KEY = %q, want the store's value", got) + } + // Control: the verbatim env still arrives, so the assertions above are + // reading a real child environment rather than an empty one. + if got := read("MEMLAWB_URL"); got != "https://memory.gitlawb.com" { + t.Fatalf("child MEMLAWB_URL = %q, want the verbatim env value", got) + } + // Control: an unset variable comes back empty, so "found" is not the + // helper's default answer. + if got := read("MEMLAWB_NOT_SET"); got != "" { + t.Fatalf("unset variable = %q, want empty", got) + } +} + +func TestStdioMissingCredentialFailsConnectNamingOnlyTheCredential(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + // The key resolves; the passphrase does not. A connect that leaked the + // resolved environment into its error would carry the key's value. + store := &fakeCredentialStore{values: map[string]string{"memlawb-api-key": "mk_live_leak_probe"}} + server := credentialHelperServer(t, + map[string]string{"MEMLAWB_PASSPHRASE": "memlawb-passphrase", "MEMLAWB_API_KEY": "memlawb-api-key"}, + nil, + ) + + client, err := ConnectWithOptions(ctx, server, ConnectOptions{Credentials: store}) + if err == nil { + client.Close() + t.Fatal("ConnectWithOptions() error = nil, want a missing-credential failure") + } + message := err.Error() + if !strings.Contains(message, "memlawb-passphrase") { + t.Fatalf("error = %q, want the missing credential named", message) + } + if !strings.Contains(message, "memlawb") { + t.Fatalf("error = %q, want the server named", message) + } + if strings.Contains(message, "mk_live_leak_probe") { + t.Fatalf("error leaked a credential value: %q", message) + } +} + +func TestStdioCredentialStoreErrorFailsConnect(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + store := &fakeCredentialStore{err: fmt.Errorf("keyring locked")} + server := credentialHelperServer(t, map[string]string{"MEMLAWB_PASSPHRASE": "memlawb-passphrase"}, nil) + + client, err := ConnectWithOptions(ctx, server, ConnectOptions{Credentials: store}) + if err == nil { + client.Close() + t.Fatal("ConnectWithOptions() error = nil, want the store error surfaced") + } + if !strings.Contains(err.Error(), "memlawb-passphrase") || !strings.Contains(err.Error(), "keyring locked") { + t.Fatalf("error = %q, want the credential name and the store error", err.Error()) + } +} + +func TestStdioWithoutReferencesNeverReadsTheCredentialStore(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + store := &fakeCredentialStore{values: map[string]string{"memlawb-passphrase": "unused"}} + server := credentialHelperServer(t, nil, nil) + + client, err := ConnectWithOptions(ctx, server, ConnectOptions{Credentials: store}) + if err != nil { + t.Fatalf("ConnectWithOptions() error = %v", err) + } + defer client.Close() + if len(store.lookups) != 0 { + t.Fatalf("credential lookups = %v, want none for a server with no references", store.lookups) + } +} + +// TestMCPEnvHelperProcess is a stdio MCP server whose only tool reports one +// environment variable of the child process, so a test can see exactly what the +// spawn path handed it. +func TestMCPEnvHelperProcess(t *testing.T) { + if os.Getenv("ZERO_MCP_ENV_HELPER") != "1" { + return + } + reader := newMessageReader(os.Stdin) + writer := newMessageWriter(os.Stdout) + for { + message, err := reader.read() + if err != nil { + os.Exit(0) + } + if message.Method == "notifications/initialized" { + continue + } + switch message.Method { + case "initialize": + _ = writer.write(rpcMessage{ + JSONRPC: "2.0", + ID: message.ID, + Result: mustRaw(map[string]any{ + "protocolVersion": "2024-11-05", + "capabilities": map[string]any{"tools": map[string]any{}}, + "serverInfo": map[string]any{"name": "test-env", "version": "1.0.0"}, + }), + }) + case "tools/list": + _ = writer.write(rpcMessage{ + JSONRPC: "2.0", + ID: message.ID, + Result: mustRaw(map[string]any{ + "tools": []map[string]any{{ + "name": "env", + "description": "Report one environment variable", + "inputSchema": map[string]any{"type": "object", "properties": map[string]any{"name": map[string]any{"type": "string"}}}, + }}, + }), + }) + case "tools/call": + var params struct { + Arguments map[string]any `json:"arguments"` + } + _ = json.Unmarshal(message.Params, ¶ms) + name, _ := params.Arguments["name"].(string) + _ = writer.write(rpcMessage{ + JSONRPC: "2.0", + ID: message.ID, + Result: mustRaw(map[string]any{ + "content": []map[string]any{{"type": "text", "text": os.Getenv(name)}}, + }), + }) + default: + _ = writer.write(rpcMessage{JSONRPC: "2.0", ID: message.ID, Error: &rpcError{Code: -32601, Message: "method not found"}}) + } + } +} diff --git a/internal/mcp/config.go b/internal/mcp/config.go index 0ea5bdce2..b5202142f 100644 --- a/internal/mcp/config.go +++ b/internal/mcp/config.go @@ -21,11 +21,15 @@ const ( ) type Server struct { - Name string - Type ServerType - Command string - Args []string - Env map[string]string + Name string + Type ServerType + Command string + Args []string + Env map[string]string + // EnvFrom maps a child environment variable to the NAME of a credential in + // Zero's credential store. It carries no secret: the value is fetched at + // spawn time (connectStdio) and never sits in config or in this struct. + EnvFrom map[string]string URL string Headers map[string]string Auth string @@ -87,6 +91,7 @@ func normalizeServer(name string, raw config.MCPServerConfig) (Server, error) { Command: strings.TrimSpace(raw.Command), Args: trimStringSlice(raw.Args), Env: copyStringMap(raw.Env), + EnvFrom: copyStringMap(raw.EnvFrom), URL: strings.TrimSpace(raw.URL), Headers: copyStringMap(raw.Headers), Auth: auth, @@ -119,6 +124,9 @@ func normalizeServer(name string, raw config.MCPServerConfig) (Server, error) { if len(server.Env) > 0 { return Server{}, fmt.Errorf("MCP server %s env is only supported for stdio transport", server.Name) } + if len(server.EnvFrom) > 0 { + return Server{}, fmt.Errorf("MCP server %s envFrom is only supported for stdio transport", server.Name) + } if err := validateHTTPURL(server.Name, server.URL); err != nil { return Server{}, err } @@ -169,6 +177,7 @@ func computeServerIdentity(server Server) string { Command string `json:"command,omitempty"` Args []string `json:"args,omitempty"` Env map[string]string `json:"env,omitempty"` + EnvFrom map[string]string `json:"envFrom,omitempty"` URL string `json:"url,omitempty"` Headers map[string]string `json:"headers,omitempty"` Auth string `json:"auth,omitempty"` @@ -178,6 +187,7 @@ func computeServerIdentity(server Server) string { Command: server.Command, Args: append([]string{}, server.Args...), Env: copyStringMap(server.Env), + EnvFrom: copyStringMap(server.EnvFrom), URL: server.URL, Headers: copyStringMap(server.Headers), Auth: server.Auth, diff --git a/internal/mcp/config_test.go b/internal/mcp/config_test.go index 51a2079a8..de4ad04b9 100644 --- a/internal/mcp/config_test.go +++ b/internal/mcp/config_test.go @@ -355,3 +355,73 @@ func TestNormalizeConfigLeavesRetiredEntryWithOwnTransportAlone(t *testing.T) { } t.Fatalf("expected the self-hosted firecrawl server to survive: %#v", servers) } + +func TestNormalizeConfigCarriesCredentialReferences(t *testing.T) { + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "memlawb": { + Type: "stdio", + Command: "memlawb", + Args: []string{"mcp"}, + Env: map[string]string{"MEMLAWB_URL": "https://memory.gitlawb.com"}, + EnvFrom: map[string]string{" MEMLAWB_PASSPHRASE ": "memlawb-passphrase"}, + }, + }} + servers, err := NormalizeConfig(cfg) + if err != nil { + t.Fatalf("NormalizeConfig() error = %v", err) + } + if len(servers) != 1 { + t.Fatalf("servers = %#v", servers) + } + if got := servers[0].EnvFrom["MEMLAWB_PASSPHRASE"]; got != "memlawb-passphrase" { + t.Fatalf("EnvFrom = %#v, want the trimmed key carried through", servers[0].EnvFrom) + } +} + +func TestNormalizeConfigRejectsCredentialReferencesOnRemoteTransport(t *testing.T) { + // A remote server's env is never spawned, so a reference there would be + // silently ignored rather than doing what the user wrote. + cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{ + "web": {Type: "http", URL: "https://example.com/mcp", EnvFrom: map[string]string{"TOKEN": "some-credential"}}, + }} + _, err := NormalizeConfig(cfg) + if err == nil || !strings.Contains(err.Error(), "envFrom is only supported") { + t.Fatalf("error = %v, want an envFrom transport rejection", err) + } +} + +func TestServerIdentityTracksCredentialReferences(t *testing.T) { + // Identity gates permission records: pointing an env var at a different + // credential changes what the child runs with, so it must change identity. + base := config.MCPServerConfig{Type: "stdio", Command: "memlawb", Args: []string{"mcp"}} + withRef := base + withRef.EnvFrom = map[string]string{"MEMLAWB_PASSPHRASE": "memlawb-passphrase"} + other := base + other.EnvFrom = map[string]string{"MEMLAWB_PASSPHRASE": "work-passphrase"} + + identity := func(server config.MCPServerConfig) string { + servers, err := NormalizeConfig(config.MCPConfig{Servers: map[string]config.MCPServerConfig{"memlawb": server}}) + if err != nil { + t.Fatalf("NormalizeConfig() error = %v", err) + } + return servers[0].Identity + } + if identity(withRef) == identity(base) { + t.Fatal("adding a credential reference must change the server identity") + } + if identity(withRef) == identity(other) { + t.Fatal("pointing at a different credential must change the server identity") + } +} + +func TestNormalizeConfigSkipsSeededMemlawbDefault(t *testing.T) { + servers, err := NormalizeConfig(config.MCPConfig{Servers: config.DefaultMCPServers()}) + if err != nil { + t.Fatalf("NormalizeConfig() error = %v", err) + } + for _, server := range servers { + if server.Name == "memlawb" { + t.Fatal("the memlawb default ships disabled and must not be normalized for launch") + } + } +}