From d9da0b94c86e9573b3c1ed52463cc837a3757144 Mon Sep 17 00:00:00 2001 From: phatlc Date: Mon, 3 Aug 2026 00:36:37 +0700 Subject: [PATCH 1/2] feat(cli): wrap and unwrap the Claude Desktop config for one server Add `mcpsnoop wrap ` and `mcpsnoop unwrap `, which edit Claude Desktop's claude_desktop_config.json so a named server starts through mcpsnoop, and put it back. Only the target server's byte range is rewritten. Decoding into a json.RawMessage returns the value's original bytes and InputOffset gives its exact span, so the user's indentation, key order, other servers and trailing newline all survive; a decode-and-re-encode of the whole file could not do that. wrap copies the config to .mcpsnoop.bak before touching it, and unwrap restores those bytes verbatim when the rest of the file is still as wrap left it, so the round trip is byte for byte. When the config changed in the meantime, unwrap rewrites only the entry and keeps the backup, so an unrelated edit is never clobbered. Both directions are idempotent, --dry-run writes nothing, and a missing config, an unknown server, an unknown client and a non-stdio entry each report what to do next. Clients live in a registry that a file registers itself into from init, so a second client is a new file rather than an edit to wrap.go. Config path resolution goes through os.UserConfigDir, which already resolves to the three directories Claude Desktop uses, so there is no runtime.GOOS switch to get wrong. Fixes #136 --- README.md | 19 +- cmd/mcpsnoop/main.go | 2 +- cmd/mcpsnoop/wrap.go | 621 ++++++++++++++++++++++++++++ cmd/mcpsnoop/wrap_claude_desktop.go | 23 ++ cmd/mcpsnoop/wrap_test.go | 549 ++++++++++++++++++++++++ docs/TRY_IT.md | 15 +- internal/paths/paths.go | 19 + internal/paths/paths_test.go | 59 +++ 8 files changed, 1304 insertions(+), 3 deletions(-) create mode 100644 cmd/mcpsnoop/wrap.go create mode 100644 cmd/mcpsnoop/wrap_claude_desktop.go create mode 100644 cmd/mcpsnoop/wrap_test.go diff --git a/README.md b/README.md index ad5d7eb..8d6629a 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,22 @@ To use it for real, wrap your server in your client's MCP config. Everything after `--` is the command that normally launches your server. Swap in whatever you already use, like `python server.py`, `npx -y @scope/server`, or a -compiled binary. Then use your client as usual and open the UI. +compiled binary. + +On Claude Desktop you don't have to make that edit by hand. + +```bash +mcpsnoop wrap my-server # route my-server through mcpsnoop +mcpsnoop unwrap my-server # put it back +``` + +`wrap` finds `claude_desktop_config.json`, copies it to +`claude_desktop_config.json.mcpsnoop.bak`, and rewrites only that one server's +entry, so your formatting, key order and every other server are left alone. +`unwrap` restores the file. Restart Claude Desktop after either, since MCP +servers are launched once at startup. + +Then use your client as usual and open the UI. ```bash mcpsnoop @@ -114,6 +129,8 @@ Explicit command-line flags override values from the config file. | `mcpsnoop diff` | compare tools and calls across two captured sessions | | `mcpsnoop open` | open a saved session in the TUI | | `mcpsnoop prune` | delete saved session logs older than a cutoff | +| `mcpsnoop wrap ` | route one of Claude Desktop's servers through mcpsnoop | +| `mcpsnoop unwrap ` | put that server's entry back the way it was | | `mcpsnoop remote ` | print the SSH tunnel command | | `mcpsnoop demo` | play a scripted session | diff --git a/cmd/mcpsnoop/main.go b/cmd/mcpsnoop/main.go index df9114f..c95ad0b 100644 --- a/cmd/mcpsnoop/main.go +++ b/cmd/mcpsnoop/main.go @@ -308,7 +308,7 @@ Repeated shim flags can live in a .mcpsnoop.toml file in the current directory.` flags.SetInterspersed(false) cmd.SetVersionTemplate("mcpsnoop {{.Version}}\n") - cmd.AddCommand(newHTTPCmd(), newExportCmd(), newCheckCmd(), newBaselineCmd(), newDiffCmd(), newOpenCmd(), newPruneCmd(), newRemoteCmd(), newDemoCmd(), newVersionCmd()) + cmd.AddCommand(newHTTPCmd(), newExportCmd(), newCheckCmd(), newBaselineCmd(), newDiffCmd(), newOpenCmd(), newPruneCmd(), newWrapCmd(), newUnwrapCmd(), newRemoteCmd(), newDemoCmd(), newVersionCmd()) return cmd } diff --git a/cmd/mcpsnoop/wrap.go b/cmd/mcpsnoop/wrap.go new file mode 100644 index 0000000..4d0aaf8 --- /dev/null +++ b/cmd/mcpsnoop/wrap.go @@ -0,0 +1,621 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "maps" + "os" + "path/filepath" + "slices" + "strings" + + "github.com/spf13/cobra" + + "github.com/kerlenton/mcpsnoop/internal/jsonwire" +) + +// wrapClient is one MCP client whose config wrap and unwrap can edit. +// +// Adding a second client is meant to be a new file, never an edit to this one: +// write a file that calls registerWrapClient from init with the client's name, +// the well-known path to its config, and the object key its servers live under. +// wrap_claude_desktop.go is the template. +type wrapClient struct { + name string // the --client value + display string // how the client is named in output + serversKey string // the config object holding one entry per server + configPath func() (string, error) // the well-known config location + restartHint string // what the user has to do for the edit to take effect +} + +var wrapClients = map[string]wrapClient{} + +// registerWrapClient adds a client to the registry. It is called from init, so a +// name collision is a build-time mistake rather than a runtime surprise, and +// panicking is the only way to say so before main starts. +func registerWrapClient(c wrapClient) { + if _, dup := wrapClients[c.name]; dup { + panic("mcpsnoop: duplicate wrap client " + c.name) + } + wrapClients[c.name] = c +} + +func lookupWrapClient(name string) (wrapClient, error) { + c, ok := wrapClients[name] + if !ok { + return wrapClient{}, badInput("unknown client %q; known clients: %s", + name, strings.Join(slices.Sorted(maps.Keys(wrapClients)), ", ")) + } + return c, nil +} + +// backupSuffix names the copy wrap takes of the untouched config. It sits next +// to the config rather than under the mcpsnoop state directory so it is +// discoverable by anyone looking at the file they are worried about, and so it +// survives an MCPSNOOP_HOME change. +const backupSuffix = ".mcpsnoop.bak" + +// wrapFault is an error that carries the exit code it should produce: 2 when the +// user can fix it by typing something else (unknown client, unknown server, a +// server that is not stdio), 1 when it is file state (missing, unreadable or +// malformed config, a failed write). That split is the one prune and check use. +type wrapFault struct { + code int + err error +} + +func (f wrapFault) Error() string { return f.err.Error() } +func (f wrapFault) Unwrap() error { return f.err } + +func badInput(format string, a ...any) error { return wrapFault{2, fmt.Errorf(format, a...)} } +func badState(format string, a ...any) error { return wrapFault{1, fmt.Errorf(format, a...)} } + +func reportFault(cmd *cobra.Command, verb string, err error) error { + fmt.Fprintf(cmd.ErrOrStderr(), "mcpsnoop %s: %v\n", verb, err) + var fault wrapFault + if errors.As(err, &fault) { + return exitCode(fault.code) + } + return exitCode(1) +} + +// wrapperPath is indirected so tests get a deterministic command instead of the +// test binary, the same seam convention runShimFn and runHTTPFn use. +var wrapperPath = mcpsnoopPath + +// mcpsnoopPath is the command wrap writes into the config. It is the absolute +// path of the running binary, not the bare "mcpsnoop" the README shows, because +// a desktop client is a GUI app: it spawns servers with the launchd or Explorer +// default PATH, which holds neither ~/go/bin nor /opt/homebrew/bin, so a bare +// name often will not resolve there even though it resolves in your shell. +func mcpsnoopPath() string { + exe, err := os.Executable() + if err != nil || exe == "" { + return "mcpsnoop" + } + return exe +} + +func newWrapCmd() *cobra.Command { + var clientName, configPath string + var dryRun bool + cmd := &cobra.Command{ + Use: "wrap ", + Short: "Route one of a client's MCP servers through mcpsnoop", + Long: "Edit an MCP client's config so the named server starts through mcpsnoop, which is the one manual step between installing mcpsnoop and seeing traffic.\n\n" + + "Only that server's entry is rewritten. The config is copied to " + backupSuffix + " first, every other byte of the file is left exactly as it was, and mcpsnoop unwrap puts the entry back. Running it twice is a no-op, and --dry-run shows the change without writing anything.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if err := runWrap(cmd, clientName, configPath, args[0], dryRun); err != nil { + return reportFault(cmd, "wrap", err) + } + return nil + }, + } + addWrapFlags(cmd, &clientName, &configPath, &dryRun) + return cmd +} + +func newUnwrapCmd() *cobra.Command { + var clientName, configPath string + var dryRun bool + cmd := &cobra.Command{ + Use: "unwrap ", + Short: "Take mcpsnoop back out of a client's MCP server entry", + Long: "Undo mcpsnoop wrap for one server, so the client launches it directly again.\n\n" + + "When the rest of the config is still as wrap left it, the file is restored byte for byte from " + backupSuffix + " and the backup is removed. When it has changed since, only the named server's entry is rewritten so those changes survive, and the backup is kept. Running it on a server that is not wrapped is a no-op, and --dry-run writes nothing.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if err := runUnwrap(cmd, clientName, configPath, args[0], dryRun); err != nil { + return reportFault(cmd, "unwrap", err) + } + return nil + }, + } + addWrapFlags(cmd, &clientName, &configPath, &dryRun) + return cmd +} + +func addWrapFlags(cmd *cobra.Command, clientName, configPath *string, dryRun *bool) { + flags := cmd.Flags() + flags.SortFlags = false + flags.StringVar(clientName, "client", claudeDesktopClient, "MCP client whose config to edit") + flags.StringVar(configPath, "config", "", "path to the client config, defaults to its well-known location") + flags.BoolVar(dryRun, "dry-run", false, "show the change without writing anything") +} + +// wrapTarget is a located server entry plus everything needed to write the file +// back. +type wrapTarget struct { + client wrapClient + path string + config []byte // the config file, verbatim + mode fs.FileMode // its current permissions, preserved across the rewrite + member jsonMember // where the server's entry sits in config + entry serverEntry // that entry, parsed +} + +func (t wrapTarget) backupPath() string { return t.path + backupSuffix } + +// resolveTarget locates one server's entry in a client config. Both commands +// start here, so a missing config, a malformed one, and an unknown server report +// identically whichever way you came in. +func resolveTarget(clientName, configPath, server string) (wrapTarget, error) { + client, err := lookupWrapClient(clientName) + if err != nil { + return wrapTarget{}, err + } + path := configPath + if path == "" { + if path, err = client.configPath(); err != nil { + return wrapTarget{}, badState("%w", err) + } + } + + config, err := os.ReadFile(path) + switch { + case errors.Is(err, fs.ErrNotExist): + return wrapTarget{}, badState("no %s config at %s; add the server there first, or pass --config with the path to it", client.display, path) + case err != nil: + return wrapTarget{}, badState("cannot read %s: %w", path, err) + } + + // Preserve the config's own permissions. A stat failure here is not worth + // aborting for, so fall back to owner-only, which is the safe direction: an + // mcpServers entry routinely carries API keys in its env block. + mode := fs.FileMode(0o600) + if info, err := os.Stat(path); err == nil { + mode = info.Mode().Perm() + } + + member, err := findServerMember(config, client.serversKey, server, path) + if err != nil { + return wrapTarget{}, err + } + entry, err := parseServerEntry(member.value, server) + if err != nil { + return wrapTarget{}, err + } + return wrapTarget{client: client, path: path, config: config, mode: mode, member: member, entry: entry}, nil +} + +func runWrap(cmd *cobra.Command, clientName, configPath, server string, dryRun bool) error { + t, err := resolveTarget(clientName, configPath, server) + if err != nil { + return err + } + out := cmd.OutOrStdout() + + if isWrapped(t.entry.command) { + // Nothing is written, which is what makes a second wrap idempotent, and it + // is also what keeps the backup holding the pre-wrap config rather than a + // wrapped one. + fmt.Fprintf(out, "%q is already wrapped in %s, nothing to do\n", server, t.client.display) + return nil + } + if t.entry.command == "" { + return badInput("%q is not a stdio server, so there is no command to wrap; mcpsnoop proxies a streamable-HTTP server with `mcpsnoop http --target ` instead", server) + } + + // The wrapped entry keeps every other key the user wrote, and everything the + // server used to be launched with moves behind mcpsnoop's own "--". + command := wrapperPath() + args := append([]string{"--", t.entry.command}, t.entry.args...) + members := maps.Clone(t.entry.members) + if err := setMember(members, "command", command); err != nil { + return err + } + if err := setMember(members, "args", args); err != nil { + return err + } + rewritten, err := spliceMember(t.config, t.member, members) + if err != nil { + return err + } + + before, after := commandLine(t.entry.command, t.entry.args), commandLine(command, args) + if dryRun { + fmt.Fprintln(out, "dry run, nothing was written") + printChange(out, t.path, before, after) + return nil + } + + // The backup is written and closed before the config is touched, so a backup + // that cannot be written aborts with the config still untouched. + if err := writeFileAtomic(t.backupPath(), t.config, 0o600); err != nil { + return badState("cannot write the backup %s: %w", t.backupPath(), err) + } + if err := writeFileAtomic(t.path, rewritten, t.mode); err != nil { + return badState("cannot write %s: %w", t.path, err) + } + + fmt.Fprintf(out, "wrapped %q in %s\n", server, t.client.display) + printChange(out, t.path, before, after) + fmt.Fprintf(out, " backup: %s\n", t.backupPath()) + fmt.Fprintf(out, "%s, then run mcpsnoop to watch the traffic\n", t.client.restartHint) + return nil +} + +func runUnwrap(cmd *cobra.Command, clientName, configPath, server string, dryRun bool) error { + t, err := resolveTarget(clientName, configPath, server) + if err != nil { + return err + } + out := cmd.OutOrStdout() + + if !isWrapped(t.entry.command) { + fmt.Fprintf(out, "%q is not wrapped in %s, nothing to do\n", server, t.client.display) + return nil + } + command, args, err := unwrappedCommand(t.entry.args, server) + if err != nil { + return err + } + + members := maps.Clone(t.entry.members) + if err := setMember(members, "command", command); err != nil { + return err + } + if len(args) > 0 { + if err := setMember(members, "args", args); err != nil { + return err + } + } else { + // A server with no arguments has no args key. Leaving an empty list behind + // would be a change to the entry that wrap never made. + delete(members, "args") + } + rewritten, err := spliceMember(t.config, t.member, members) + if err != nil { + return err + } + + // Prefer a literal byte-for-byte restore, but only when this run can prove the + // backup describes the same config: if the user edited the file while it was + // wrapped, restoring the backup would silently throw those edits away, so the + // spliced result is written instead and the backup is kept. + restored, backup := false, t.backupPath() + if original, err := os.ReadFile(backup); err == nil && sameJSON(original, rewritten) { + rewritten, restored = original, true + } + + before, after := commandLine(t.entry.command, t.entry.args), commandLine(command, args) + if dryRun { + fmt.Fprintln(out, "dry run, nothing was written") + printChange(out, t.path, before, after) + printRestoreNote(out, backup, restored, dryRun) + return nil + } + + if err := writeFileAtomic(t.path, rewritten, t.mode); err != nil { + return badState("cannot write %s: %w", t.path, err) + } + if restored { + // Only now, with the original back in place, is the backup redundant. + if err := os.Remove(backup); err != nil && !errors.Is(err, fs.ErrNotExist) { + return badState("unwrapped %s, but cannot remove the backup %s: %w", t.path, backup, err) + } + } + + fmt.Fprintf(out, "unwrapped %q in %s\n", server, t.client.display) + printChange(out, t.path, before, after) + printRestoreNote(out, backup, restored, dryRun) + fmt.Fprintf(out, "%s\n", t.client.restartHint) + return nil +} + +func printChange(out io.Writer, path, before, after string) { + fmt.Fprintf(out, " config: %s\n", path) + fmt.Fprintf(out, " before: %s\n", before) + fmt.Fprintf(out, " after: %s\n", after) +} + +// printRestoreNote says which of unwrap's two endings happened, or would have: +// the whole file put back from the backup, or just this entry rewritten because +// the rest of the config had moved on since it was wrapped. +func printRestoreNote(out io.Writer, backup string, restored, dryRun bool) { + switch { + case restored && dryRun: + fmt.Fprintf(out, " would restore the config byte for byte from %s, and remove it\n", backup) + case restored: + fmt.Fprintf(out, " restored the config byte for byte from %s, and removed it\n", backup) + default: + if _, err := os.Stat(backup); err != nil { + return // never wrapped by this mcpsnoop, so there is nothing to say + } + fmt.Fprintf(out, " the rest of the config changed since it was wrapped, so only this entry is rewritten and the original stays at %s\n", backup) + } +} + +func commandLine(command string, args []string) string { + return strings.Join(append([]string{command}, args...), " ") +} + +// serverEntry is one server's config entry. The members are kept raw so every +// key mcpsnoop does not model, env, type, or a client-specific extension, +// survives a rewrite exactly as the user wrote it. +type serverEntry struct { + members map[string]json.RawMessage + command string + args []string +} + +func parseServerEntry(raw json.RawMessage, server string) (serverEntry, error) { + var members map[string]json.RawMessage + if err := json.Unmarshal(raw, &members); err != nil { + return serverEntry{}, badState("the entry for %q is not a JSON object: %w", server, err) + } + entry := serverEntry{members: members} + if v, ok := members["command"]; ok { + if err := json.Unmarshal(v, &entry.command); err != nil { + return serverEntry{}, badState("the %q entry's \"command\" is not a string", server) + } + } + if v, ok := members["args"]; ok { + if err := json.Unmarshal(v, &entry.args); err != nil { + return serverEntry{}, badState("the %q entry's \"args\" is not a list of strings", server) + } + } + return entry, nil +} + +// isWrapped reports whether a command already runs through mcpsnoop. +// +// filepath.Base is deliberately not used. It does not split on a backslash off +// Windows, so a Windows config inspected on any other OS, which is exactly what +// a test does, would look unwrapped and get wrapped a second time. labelFor +// splits the same way for the same reason. +func isWrapped(command string) bool { + name := command + if i := strings.LastIndexAny(name, `/\`); i >= 0 { + name = name[i+1:] + } + if base, ok := strings.CutSuffix(strings.ToLower(name), ".exe"); ok { + name = base + } + return strings.EqualFold(name, "mcpsnoop") +} + +// unwrappedCommand recovers the command a wrapped entry originally ran. Anything +// before the first "--" is mcpsnoop's own flags, so an entry a user wrote by hand +// as `mcpsnoop --redact-secrets -- npx x` unwraps to `npx x` and loses only the +// flags, which is the whole point of unwrapping. +func unwrappedCommand(args []string, server string) (string, []string, error) { + i := slices.Index(args, "--") + if i < 0 { + return "", nil, badState("the %q entry runs mcpsnoop but its args hold no \"--\", so the server command it wrapped cannot be recovered; edit the entry by hand", server) + } + rest := args[i+1:] + if len(rest) == 0 { + return "", nil, badState("the %q entry runs mcpsnoop with nothing after \"--\", so there is no server command to restore; edit the entry by hand", server) + } + return rest[0], rest[1:], nil +} + +// setMember encodes one value into an entry, through jsonwire rather than +// encoding/json. encoding/json escapes &, < and > by default, so an argument +// like --url=https://host/path?a=1&b=2 would land in the user's config with its +// & written as \u0026, changing the argument their server is launched with. +func setMember(members map[string]json.RawMessage, key string, value any) error { + raw, err := jsonwire.Marshal(value) + if err != nil { + return badState("cannot encode %q: %w", key, err) + } + members[key] = raw + return nil +} + +// jsonMember is one member of a JSON object together with the exact byte range +// its value occupies in the enclosing document. +type jsonMember struct { + name string + start int + end int + value json.RawMessage +} + +// objectMembers returns every member of the JSON object obj with its byte span. +// +// Decoding into a json.RawMessage hands back the value's original bytes, +// internal whitespace included, and InputOffset is the offset just past them, so +// end-len(value) is the value's exact start. That is what lets wrap rewrite one +// server's entry and leave the user's key order, indentation and every other +// server in the file untouched, which reformatting the whole document through +// Unmarshal and MarshalIndent could not do. +func objectMembers(obj []byte) ([]jsonMember, error) { + dec := json.NewDecoder(bytes.NewReader(obj)) + tok, err := dec.Token() + if err != nil { + return nil, err + } + if delim, ok := tok.(json.Delim); !ok || delim != '{' { + return nil, errors.New("expected a JSON object") + } + var members []jsonMember + for dec.More() { + nameTok, err := dec.Token() + if err != nil { + return nil, err + } + name, ok := nameTok.(string) + if !ok { + return nil, errors.New("expected a JSON object") + } + var value json.RawMessage + if err := dec.Decode(&value); err != nil { + return nil, err + } + end := int(dec.InputOffset()) + start := end - len(value) + if start < 0 || end > len(obj) || !bytes.Equal(obj[start:end], value) { + // Belt and braces. The span drives a write over somebody's config, so it + // is checked against the source rather than trusted. + return nil, fmt.Errorf("cannot locate the bytes of %q", name) + } + members = append(members, jsonMember{name: name, start: start, end: end, value: value}) + } + return members, nil +} + +func findServerMember(config []byte, serversKey, server, path string) (jsonMember, error) { + top, err := objectMembers(config) + if err != nil { + return jsonMember{}, badState("cannot read %s as a JSON object: %w", path, err) + } + i := slices.IndexFunc(top, func(m jsonMember) bool { return m.name == serversKey }) + if i < 0 { + return jsonMember{}, badState("%s has no %q section, so it configures no MCP servers", path, serversKey) + } + section := top[i] + + entries, err := objectMembers(section.value) + if err != nil { + return jsonMember{}, badState("the %q section of %s is not a JSON object: %w", serversKey, path, err) + } + j := slices.IndexFunc(entries, func(m jsonMember) bool { return m.name == server }) + if j < 0 { + names := make([]string, len(entries)) + for k, e := range entries { + names[k] = e.name + } + slices.Sort(names) + if len(names) == 0 { + return jsonMember{}, badInput("no server named %q in %s; its %q section is empty", server, path, serversKey) + } + return jsonMember{}, badInput("no server named %q in %s; it configures %s", server, path, strings.Join(names, ", ")) + } + + // The spans came out of the section's own bytes, so shift them to the file. + entry := entries[j] + entry.start += section.start + entry.end += section.start + return entry, nil +} + +// spliceMember rewrites just the bytes of member and returns the whole file, +// with every byte outside that range identical to what came in. +func spliceMember(config []byte, member jsonMember, members map[string]json.RawMessage) ([]byte, error) { + block, err := encodeMembers(config, member, members) + if err != nil { + return nil, err + } + out := make([]byte, 0, len(config)-(member.end-member.start)+len(block)) + out = append(out, config[:member.start]...) + out = append(out, block...) + out = append(out, config[member.end:]...) + if !json.Valid(out) { + return nil, badState("the rewritten config would not be valid JSON, so nothing was written") + } + return out, nil +} + +// encodeMembers renders an entry the way the file around it is written: on one +// line if the entry it replaces was on one line, otherwise indented to continue +// from the entry's own line, with the file's own indent unit. +func encodeMembers(config []byte, member jsonMember, members map[string]json.RawMessage) ([]byte, error) { + var buf bytes.Buffer + enc := jsonwire.NewEncoder(&buf) + if bytes.ContainsRune(member.value, '\n') { + enc.SetIndent(lineIndent(config, member.start), indentUnit(config)) + } + if err := enc.Encode(members); err != nil { + return nil, badState("cannot encode the server entry: %w", err) + } + return bytes.TrimSuffix(buf.Bytes(), []byte("\n")), nil +} + +// lineIndent is the leading whitespace of the line offset sits on, which is the +// indentation a spliced entry has to continue from. +func lineIndent(config []byte, offset int) string { + line := config[bytes.LastIndexByte(config[:offset], '\n')+1 : offset] + return string(line[:len(line)-len(bytes.TrimLeft(line, " \t"))]) +} + +// indentUnit guesses the file's own indentation from its first indented line, so +// a wrapped entry keeps looking like the rest of the config instead of switching +// a four-space file to two. Two spaces is the fallback, matching the README. +func indentUnit(config []byte) string { + for raw := range bytes.Lines(config) { + line := bytes.TrimRight(raw, "\r\n") + indent := line[:len(line)-len(bytes.TrimLeft(line, " \t"))] + if len(indent) > 0 && len(indent) < len(line) { + return string(indent) + } + } + return " " +} + +// sameJSON reports whether two configs describe the same document, ignoring +// formatting and key order. Unmarshalling into any and re-encoding sorts object +// keys, so the comparison survives the reordering a re-encoded entry causes. +func sameJSON(a, b []byte) bool { + na, err := normalizeJSON(a) + if err != nil { + return false + } + nb, err := normalizeJSON(b) + if err != nil { + return false + } + return bytes.Equal(na, nb) +} + +func normalizeJSON(data []byte) ([]byte, error) { + var v any + if err := json.Unmarshal(data, &v); err != nil { + return nil, err + } + return jsonwire.Marshal(v) +} + +// writeFileAtomic replaces path's contents in one step, so an interrupted write +// can never leave a client staring at half a config. The temp file is created in +// the config's own directory, which keeps the rename on a single filesystem. +func writeFileAtomic(path string, data []byte, mode fs.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), ".mcpsnoop-wrap-*") + if err != nil { + return err + } + name := tmp.Name() + defer os.Remove(name) + if err := tmp.Chmod(mode); err != nil { + tmp.Close() + return err + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(name, path) +} diff --git a/cmd/mcpsnoop/wrap_claude_desktop.go b/cmd/mcpsnoop/wrap_claude_desktop.go new file mode 100644 index 0000000..25fa735 --- /dev/null +++ b/cmd/mcpsnoop/wrap_claude_desktop.go @@ -0,0 +1,23 @@ +package main + +import "github.com/kerlenton/mcpsnoop/internal/paths" + +// claudeDesktopClient is the --client value for Claude Desktop, and the default +// wrap and unwrap assume. +const claudeDesktopClient = "claude-desktop" + +// This file is the whole of Claude Desktop's support for wrap and unwrap, and it +// is the template for the next client: copy it, change the four fields, and the +// commands pick the client up with no edit to wrap.go. Everything below the +// registry is client independent, because every MCP client so far stores its +// servers the same way, as one object per server under a single top-level key. +func init() { + registerWrapClient(wrapClient{ + name: claudeDesktopClient, + display: "Claude Desktop", + serversKey: "mcpServers", + configPath: paths.ClaudeDesktopConfig, + restartHint: "quit Claude Desktop completely and start it again, since MCP servers " + + "are launched once at startup", + }) +} diff --git a/cmd/mcpsnoop/wrap_test.go b/cmd/mcpsnoop/wrap_test.go new file mode 100644 index 0000000..d0db363 --- /dev/null +++ b/cmd/mcpsnoop/wrap_test.go @@ -0,0 +1,549 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// wrapFixture is deliberately not a testdata file. It is written into t.TempDir +// by each test, so the bytes under test are the ones in this source and nothing +// on the way to a Windows checkout can rewrite them. +// +// It is also deliberately awkward: a four-space indent rather than the README's +// two, a top-level key that is not mcpServers, an entry carrying env as well as +// command and args, a second entry written on one line, and a trailing newline. +// Every one of those is something a whole-file re-encode would quietly destroy. +const wrapFixture = `{ + "globalShortcut": "Ctrl+Space", + "mcpServers": { + "everything": { + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-everything" + ], + "env": { + "TOKEN": "secret" + } + }, + "other": { "command": "python", "args": ["server.py"] } + } +} +` + +// stubWrapperPath pins the command wrap writes, so assertions do not depend on +// where the test binary happens to live. +const stubWrapperPath = "/opt/homebrew/bin/mcpsnoop" + +func newWrapTest(t *testing.T, config string) string { + t.Helper() + orig := wrapperPath + wrapperPath = func() string { return stubWrapperPath } + t.Cleanup(func() { wrapperPath = orig }) + + path := filepath.Join(t.TempDir(), "claude_desktop_config.json") + if err := os.WriteFile(path, []byte(config), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func executeWrapCmd(t *testing.T, newCmd func() *cobra.Command, args ...string) (int, string, string) { + t.Helper() + cmd := newCmd() + cmd.SetArgs(args) + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + if err == nil { + return 0, stdout.String(), stderr.String() + } + var code exitCode + if !errors.As(err, &code) { + t.Fatalf("unexpected command error: %v", err) + } + return int(code), stdout.String(), stderr.String() +} + +func wrapOK(t *testing.T, newCmd func() *cobra.Command, args ...string) string { + t.Helper() + code, stdout, stderr := executeWrapCmd(t, newCmd, args...) + if code != 0 || stderr != "" { + t.Fatalf("exit %d, stderr %q, stdout %q", code, stderr, stdout) + } + return stdout +} + +func readConfig(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(data) +} + +// readEntry returns one server's command and args as the client would read them. +func readEntry(t *testing.T, path, server string) (string, []string) { + t.Helper() + var doc struct { + MCPServers map[string]struct { + Command string `json:"command"` + Args []string `json:"args"` + URL string `json:"url"` + } `json:"mcpServers"` + } + if err := json.Unmarshal([]byte(readConfig(t, path)), &doc); err != nil { + t.Fatalf("config is not valid JSON: %v", err) + } + entry, ok := doc.MCPServers[server] + if !ok { + t.Fatalf("config has no server %q", server) + } + return entry.Command, entry.Args +} + +// TestWrapThenUnwrapRestoresTheConfigByteForByte is the acceptance criterion the +// whole design exists for. A decode-and-re-encode implementation cannot pass it: +// it reorders keys and reflows the file. +func TestWrapThenUnwrapRestoresTheConfigByteForByte(t *testing.T) { + path := newWrapTest(t, wrapFixture) + + wrapOK(t, newWrapCmd, "everything", "--config", path) + if got := readConfig(t, path); got == wrapFixture { + t.Fatal("wrap did not change the config") + } + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("unwrap did not restore the config byte for byte:\n got %q\nwant %q", got, wrapFixture) + } + if _, err := os.Stat(path + backupSuffix); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("a byte-for-byte restore should remove the backup, stat gave %v", err) + } +} + +// TestWrapRewritesOnlyTheNamedEntry pins the byte-splice: the bytes outside the +// target entry, including the sibling server's one-line formatting, the +// unrelated top-level key and the trailing newline, come through untouched. +func TestWrapRewritesOnlyTheNamedEntry(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + got := readConfig(t, path) + + for _, untouched := range []string{ + ` "globalShortcut": "Ctrl+Space",`, + ` "other": { "command": "python", "args": ["server.py"] }`, + ` "mcpServers": {`, + } { + if !strings.Contains(got, untouched) { + t.Fatalf("wrap disturbed bytes outside the target entry, %q is gone:\n%s", untouched, got) + } + } + if !strings.HasSuffix(got, "}\n") { + t.Fatalf("wrap dropped the trailing newline:\n%q", got) + } + if !strings.Contains(got, `"TOKEN": "secret"`) { + t.Fatalf("wrap dropped the entry's env block:\n%s", got) + } + + command, args := readEntry(t, path, "everything") + if command != stubWrapperPath { + t.Fatalf("command = %q, want %q", command, stubWrapperPath) + } + want := []string{"--", "npx", "-y", "@modelcontextprotocol/server-everything"} + if !slices.Equal(args, want) { + t.Fatalf("args = %q, want %q", args, want) + } + + backup, err := os.ReadFile(path + backupSuffix) + if err != nil { + t.Fatal(err) + } + if string(backup) != wrapFixture { + t.Fatalf("the backup should hold the original bytes:\n got %q\nwant %q", backup, wrapFixture) + } +} + +func TestWrapIsIdempotent(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + afterFirst := readConfig(t, path) + + stdout := wrapOK(t, newWrapCmd, "everything", "--config", path) + if !strings.Contains(stdout, "already wrapped") { + t.Fatalf("a second wrap should say so, got %q", stdout) + } + if got := readConfig(t, path); got != afterFirst { + t.Fatalf("a second wrap changed the config:\n got %q\nwant %q", got, afterFirst) + } + // The backup must still hold the pre-wrap config, not a wrapped one, or the + // byte-for-byte restore would put a wrapped entry back. + backup, err := os.ReadFile(path + backupSuffix) + if err != nil { + t.Fatal(err) + } + if string(backup) != wrapFixture { + t.Fatalf("a second wrap overwrote the backup:\n%s", backup) + } + + // And unwrap still gets all the way home from there. + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("unwrap after a doubled wrap:\n got %q\nwant %q", got, wrapFixture) + } +} + +func TestUnwrapIsIdempotent(t *testing.T) { + path := newWrapTest(t, wrapFixture) + + stdout := wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if !strings.Contains(stdout, "not wrapped") { + t.Fatalf("unwrap on a plain entry should say so, got %q", stdout) + } + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("unwrap on a plain entry changed the config:\n%s", got) + } + + wrapOK(t, newWrapCmd, "everything", "--config", path) + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + stdout = wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if !strings.Contains(stdout, "not wrapped") { + t.Fatalf("a second unwrap should say so, got %q", stdout) + } + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("a second unwrap changed the config:\n%s", got) + } +} + +func TestWrapAndUnwrapDryRunWriteNothing(t *testing.T) { + path := newWrapTest(t, wrapFixture) + + stdout := wrapOK(t, newWrapCmd, "everything", "--config", path, "--dry-run") + for _, want := range []string{"dry run", "npx -y @modelcontextprotocol/server-everything", stubWrapperPath} { + if !strings.Contains(stdout, want) { + t.Fatalf("dry run should report %q, got %q", want, stdout) + } + } + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("--dry-run wrote to the config:\n%s", got) + } + if _, err := os.Stat(path + backupSuffix); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("--dry-run created a backup, stat gave %v", err) + } + + wrapOK(t, newWrapCmd, "everything", "--config", path) + wrapped := readConfig(t, path) + stdout = wrapOK(t, newUnwrapCmd, "everything", "--config", path, "--dry-run") + if !strings.Contains(stdout, "dry run") { + t.Fatalf("unwrap --dry-run should say so, got %q", stdout) + } + if got := readConfig(t, path); got != wrapped { + t.Fatalf("unwrap --dry-run wrote to the config:\n%s", got) + } + if _, err := os.Stat(path + backupSuffix); err != nil { + t.Fatalf("unwrap --dry-run removed the backup: %v", err) + } +} + +// TestUnwrapKeepsAnEditMadeWhileWrapped is the reason the restore is conditional. +// Restoring the backup unconditionally would throw away a server the user added +// after wrapping. +func TestUnwrapKeepsAnEditMadeWhileWrapped(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + const added = ` "third": { "command": "sh" },` + "\n" + edited := strings.Replace(readConfig(t, path), ` "mcpServers": {`+"\n", ` "mcpServers": {`+"\n"+added, 1) + if err := os.WriteFile(path, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + + stdout := wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if !strings.Contains(stdout, "changed since it was wrapped") { + t.Fatalf("unwrap should say the config moved on, got %q", stdout) + } + got := readConfig(t, path) + if !strings.Contains(got, `"third"`) { + t.Fatalf("unwrap clobbered an edit made while wrapped:\n%s", got) + } + if command, args := readEntry(t, path, "everything"); command != "npx" || len(args) != 2 { + t.Fatalf("the target entry was not unwrapped, command %q args %q", command, args) + } + if _, err := os.Stat(path + backupSuffix); err != nil { + t.Fatalf("the backup should be kept when the restore could not be confirmed: %v", err) + } +} + +// TestUnwrapWithoutABackupStillUnwraps: the backup is an optimisation for the +// byte-for-byte restore, never a dependency. +func TestUnwrapWithoutABackupStillUnwraps(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + if err := os.Remove(path + backupSuffix); err != nil { + t.Fatal(err) + } + + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + command, args := readEntry(t, path, "everything") + if command != "npx" { + t.Fatalf("command = %q, want npx", command) + } + want := []string{"-y", "@modelcontextprotocol/server-everything"} + if !slices.Equal(args, want) { + t.Fatalf("args = %q, want %q", args, want) + } + if !sameJSON([]byte(readConfig(t, path)), []byte(wrapFixture)) { + t.Fatalf("unwrap without a backup should still restore the document:\n%s", readConfig(t, path)) + } +} + +func TestWrapReportsAProblemTheUserCanFix(t *testing.T) { + for _, tc := range []struct { + name string + args []string + config string + code int + want []string + }{ + { + name: "unknown server", + args: []string{"nope"}, config: wrapFixture, code: 2, + want: []string{`no server named "nope"`, "everything", "other"}, + }, + { + name: "unknown client", + args: []string{"everything", "--client", "emacs"}, config: wrapFixture, code: 2, + want: []string{`unknown client "emacs"`, claudeDesktopClient}, + }, + { + name: "not a stdio server", + args: []string{"remote"}, code: 2, + config: `{"mcpServers":{"remote":{"url":"https://example.test/mcp","type":"http"}}}`, + want: []string{"not a stdio server", "mcpsnoop http --target"}, + }, + { + name: "malformed config", + args: []string{"everything"}, config: "{ not json", code: 1, + want: []string{"as a JSON object"}, + }, + { + name: "no mcpServers section", + args: []string{"everything"}, config: `{"globalShortcut":"Ctrl+Space"}`, code: 1, + want: []string{`no "mcpServers" section`}, + }, + { + name: "empty mcpServers section", + args: []string{"everything"}, config: `{"mcpServers":{}}`, code: 2, + want: []string{`no server named "everything"`, "is empty"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + path := newWrapTest(t, tc.config) + code, _, stderr := executeWrapCmd(t, newWrapCmd, append(tc.args, "--config", path)...) + if code != tc.code { + t.Fatalf("exit %d, want %d (stderr %q)", code, tc.code, stderr) + } + for _, want := range tc.want { + if !strings.Contains(stderr, want) { + t.Fatalf("stderr %q should mention %q", stderr, want) + } + } + if got := readConfig(t, path); got != tc.config { + t.Fatalf("a failed wrap wrote to the config:\n%s", got) + } + }) + } +} + +func TestWrapNamesTheMissingConfig(t *testing.T) { + missing := filepath.Join(t.TempDir(), "claude_desktop_config.json") + code, _, stderr := executeWrapCmd(t, newWrapCmd, "everything", "--config", missing) + if code != 1 { + t.Fatalf("exit %d, want 1 (stderr %q)", code, stderr) + } + for _, want := range []string{missing, "Claude Desktop", "--config"} { + if !strings.Contains(stderr, want) { + t.Fatalf("stderr %q should mention %q", stderr, want) + } + } +} + +// TestUnwrapRecoversTheCommandAfterTheFirstSeparator covers an entry a user wrote +// by hand with mcpsnoop's own flags in front of the wrapped command. +func TestUnwrapRecoversTheCommandAfterTheFirstSeparator(t *testing.T) { + const config = `{ + "mcpServers": { + "everything": { + "command": "mcpsnoop", + "args": ["--redact-secrets", "--", "npx", "-y", "server", "--", "extra"] + } + } +} +` + path := newWrapTest(t, config) + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + + command, args := readEntry(t, path, "everything") + if command != "npx" { + t.Fatalf("command = %q, want npx", command) + } + // Everything after the first "--" belongs to the server, including a second + // "--" that is the server's own argument. + want := []string{"-y", "server", "--", "extra"} + if !slices.Equal(args, want) { + t.Fatalf("args = %q, want %q", args, want) + } +} + +// TestUnwrapRefusesAnEntryItCannotRecover: better a clear error than a guess at +// what the user meant to run. +func TestUnwrapRefusesAnEntryItCannotRecover(t *testing.T) { + for _, config := range []string{ + `{"mcpServers":{"everything":{"command":"mcpsnoop","args":["--redact-secrets"]}}}`, + `{"mcpServers":{"everything":{"command":"mcpsnoop","args":["--"]}}}`, + } { + path := newWrapTest(t, config) + code, _, stderr := executeWrapCmd(t, newUnwrapCmd, "everything", "--config", path) + if code != 1 { + t.Fatalf("exit %d, want 1 (stderr %q)", code, stderr) + } + if !strings.Contains(stderr, "edit the entry by hand") { + t.Fatalf("stderr %q should say what to do", stderr) + } + if got := readConfig(t, path); got != config { + t.Fatalf("a failed unwrap wrote to the config:\n%s", got) + } + } +} + +// TestWrapRecognisesAWindowsWrappedEntry. Windows paths are inspected on every +// OS, by this test among others, and filepath.Base does not split a backslash +// off Windows, so an already-wrapped entry would be wrapped again. +func TestWrapRecognisesAWindowsWrappedEntry(t *testing.T) { + const config = `{"mcpServers":{"everything":{"command":"C:\\Program Files\\mcpsnoop\\MCPSnoop.exe","args":["--","npx","server"]}}}` + path := newWrapTest(t, config) + + if stdout := wrapOK(t, newWrapCmd, "everything", "--config", path); !strings.Contains(stdout, "already wrapped") { + t.Fatalf("a windows-style mcpsnoop command should read as wrapped, got %q", stdout) + } + if got := readConfig(t, path); got != config { + t.Fatalf("wrap rewrote an already-wrapped entry:\n%s", got) + } +} + +// TestWrapLeavesMarkupInAnArgumentAlone pins the jsonwire encoder. encoding/json +// would write & as \u0026 into the user's config, changing the argument the +// server is launched with. +func TestWrapLeavesMarkupInAnArgumentAlone(t *testing.T) { + const config = `{"mcpServers":{"everything":{"command":"node","args":["--url=https://a.test/x?a=1&b=2<3"]}}}` + path := newWrapTest(t, config) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + got := readConfig(t, path) + if !strings.Contains(got, "https://a.test/x?a=1&b=2<3") { + t.Fatalf("wrap escaped an argument:\n%s", got) + } + _, args := readEntry(t, path, "everything") + want := []string{"--", "node", "--url=https://a.test/x?a=1&b=2<3"} + if !slices.Equal(args, want) { + t.Fatalf("args = %q, want %q", args, want) + } +} + +// TestWrapKeepsAOneLineEntryOnOneLine, and unwrap drops an args key the original +// entry never had rather than leaving an empty list behind. +func TestWrapKeepsAOneLineEntryOnOneLine(t *testing.T) { + const config = `{ + "mcpServers": { + "everything": { "command": "server-everything" } + } +} +` + path := newWrapTest(t, config) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + got := readConfig(t, path) + if strings.Count(got, "\n") != strings.Count(config, "\n") { + t.Fatalf("wrap reflowed a one-line entry:\n%s", got) + } + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if got := readConfig(t, path); got != config { + t.Fatalf("unwrap did not restore a one-line entry:\n got %q\nwant %q", got, config) + } +} + +// TestWrapKeepsTheConfigPermissions. A config's env block routinely holds API +// keys, so a rewrite must not widen the file, and the backup is owner-only +// whatever the config is. +func TestWrapKeepsTheConfigPermissions(t *testing.T) { + if os.PathSeparator == '\\' { + t.Skip("unix permission bits") + } + path := newWrapTest(t, wrapFixture) + if err := os.Chmod(path, 0o640); err != nil { + t.Fatal(err) + } + wrapOK(t, newWrapCmd, "everything", "--config", path) + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o640 { + t.Fatalf("config mode = %o, want 640", got) + } + backup, err := os.Stat(path + backupSuffix) + if err != nil { + t.Fatal(err) + } + if got := backup.Mode().Perm(); got != 0o600 { + t.Fatalf("backup mode = %o, want 600", got) + } +} + +// TestWrapClientsAreRegisteredNotHardcoded pins the extension seam: the commands +// read the registry, so a second client is a new file rather than an edit here. +func TestWrapClientsAreRegisteredNotHardcoded(t *testing.T) { + client, err := lookupWrapClient(claudeDesktopClient) + if err != nil { + t.Fatal(err) + } + if client.serversKey != "mcpServers" || client.display == "" || client.restartHint == "" { + t.Fatalf("claude desktop is registered incomplete: %+v", client) + } + path, err := client.configPath() + if err != nil { + t.Fatal(err) + } + if filepath.Base(path) != "claude_desktop_config.json" { + t.Fatalf("configPath() = %q", path) + } + + registerWrapClient(wrapClient{name: "test-client", display: "Test Client", serversKey: "servers", + configPath: func() (string, error) { return "", nil }, restartHint: "restart it"}) + t.Cleanup(func() { delete(wrapClients, "test-client") }) + + const config = `{"servers":{"everything":{"command":"node","args":["s.js"]}}}` + cfgPath := newWrapTest(t, config) + wrapOK(t, newWrapCmd, "everything", "--client", "test-client", "--config", cfgPath) + if !strings.Contains(readConfig(t, cfgPath), stubWrapperPath) { + t.Fatalf("wrap did not edit the registered client's config:\n%s", readConfig(t, cfgPath)) + } + wrapOK(t, newUnwrapCmd, "everything", "--client", "test-client", "--config", cfgPath) + if got := readConfig(t, cfgPath); got != config { + t.Fatalf("unwrap for a registered client:\n got %q\nwant %q", got, config) + } +} diff --git a/docs/TRY_IT.md b/docs/TRY_IT.md index 45795d2..fa5d827 100644 --- a/docs/TRY_IT.md +++ b/docs/TRY_IT.md @@ -21,7 +21,15 @@ it. claude mcp add everything -- mcpsnoop -- npx -y @modelcontextprotocol/server-everything ``` -For Claude Desktop, add the same wrap to your `claude_desktop_config.json`. +For Claude Desktop, add the server to your `claude_desktop_config.json` as usual, +then let mcpsnoop make the same wrap for you. + +```bash +mcpsnoop wrap everything +``` + +That backs the config up to `claude_desktop_config.json.mcpsnoop.bak` and +rewrites only the `everything` entry, into this. ```json { @@ -34,6 +42,11 @@ For Claude Desktop, add the same wrap to your `claude_desktop_config.json`. } ``` +`command` gets the full path to the mcpsnoop binary rather than the bare name +above, because Claude Desktop launches servers with the desktop session's PATH, +which usually does not include `~/go/bin`. Editing the file by hand works just as +well. When you're done, `mcpsnoop unwrap everything` puts it back. + ## Watch it live 1. Run `mcpsnoop` in one terminal. The TUI opens and waits for MCP traffic. diff --git a/internal/paths/paths.go b/internal/paths/paths.go index aeff68f..5b39c14 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -113,6 +113,25 @@ func Base() string { return base } +// ClaudeDesktopConfig returns the well-known path to Claude Desktop's MCP +// config file, the one `mcpsnoop wrap` edits. +// +// os.UserConfigDir already resolves to exactly the three directories Claude +// Desktop keeps it in: $HOME/Library/Application Support on darwin, %AppData% +// on windows, and $XDG_CONFIG_HOME or ~/.config on linux. So there is no +// runtime.GOOS switch here, and therefore no per-OS branch to get wrong. +// +// Unlike Base this creates nothing, not even the parent directory. The path +// belongs to another application, so mcpsnoop reads it and, when asked, writes +// it back, but never brings it into existence. +func ClaudeDesktopConfig() (string, error) { + dir, err := os.UserConfigDir() + if err != nil { + return "", fmt.Errorf("cannot resolve the user config directory (%w); pass --config with the path to claude_desktop_config.json", err) + } + return filepath.Join(dir, "Claude", "claude_desktop_config.json"), nil +} + // SocketPath is the unix socket the hub listens on and shims connect to. func SocketPath() string { return filepath.Join(Base(), "hub.sock") diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 913aa6d..5714a9f 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -1,7 +1,11 @@ package paths import ( + "errors" + "io/fs" + "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -81,6 +85,61 @@ func TestCheckLabelKeepsADoubledDotContained(t *testing.T) { } } +// TestClaudeDesktopConfigTracksTheOSConfigDir keeps the one assumption the +// helper rests on honest across every platform CI builds for: os.UserConfigDir +// already resolves to the directory Claude Desktop keeps its config under, so +// there is no per-OS branch here to drift. +func TestClaudeDesktopConfigTracksTheOSConfigDir(t *testing.T) { + dir, err := os.UserConfigDir() + if err != nil { + t.Skipf("no user config dir on this machine: %v", err) + } + got, err := ClaudeDesktopConfig() + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(dir, "Claude", "claude_desktop_config.json"); got != want { + t.Fatalf("ClaudeDesktopConfig() = %q, want %q", got, want) + } + // The path belongs to another application, so resolving it must not bring any + // part of it into existence the way Base and its callers deliberately do. + // Comparing existence either side of the call says so wherever the test runs, + // whether or not Claude Desktop is installed on the machine. + before := exists(filepath.Dir(got)) + if _, err := ClaudeDesktopConfig(); err != nil { + t.Fatal(err) + } + if exists(filepath.Dir(got)) != before { + t.Fatalf("ClaudeDesktopConfig changed whether %q exists", filepath.Dir(got)) + } +} + +func exists(path string) bool { + _, err := os.Stat(path) + return !errors.Is(err, fs.ErrNotExist) +} + +// TestClaudeDesktopConfigFollowsXDGConfigHome is linux-only because that is the +// only platform where os.UserConfigDir consults XDG. +func TestClaudeDesktopConfigFollowsXDGConfigHome(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("XDG_CONFIG_HOME is only consulted on linux") + } + root := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", root) + + got, err := ClaudeDesktopConfig() + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(root, "Claude", "claude_desktop_config.json"); got != want { + t.Fatalf("ClaudeDesktopConfig() = %q, want %q", got, want) + } + if exists(filepath.Dir(got)) { + t.Fatalf("ClaudeDesktopConfig created %q under a fresh config home", filepath.Dir(got)) + } +} + func TestToolBaselinesDirUsesConfiguredStateRoot(t *testing.T) { root := t.TempDir() t.Setenv("MCPSNOOP_HOME", root) From ceb1f87c22eb9e7f0244b16d4d6a2a89e40bd626 Mon Sep 17 00:00:00 2001 From: Kerlenton Date: Sat, 8 Aug 2026 22:05:53 +0300 Subject: [PATCH 2/2] fix(wrap): stop unwrap reverting an edit it cannot see --- README.md | 10 +- cmd/mcpsnoop/wrap.go | 303 ++++++++++++++++++++++++---- cmd/mcpsnoop/wrap_claude_desktop.go | 18 +- cmd/mcpsnoop/wrap_test.go | 295 ++++++++++++++++++++++++++- internal/paths/paths_test.go | 51 ++++- 5 files changed, 618 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index a52275c..9332c30 100644 --- a/README.md +++ b/README.md @@ -58,10 +58,12 @@ mcpsnoop unwrap my-server # put it back ``` `wrap` finds `claude_desktop_config.json`, copies it to -`claude_desktop_config.json.mcpsnoop.bak`, and rewrites only that one server's -entry, so your formatting, key order and every other server are left alone. -`unwrap` restores the file. Restart Claude Desktop after either, since MCP -servers are launched once at startup. +`claude_desktop_config.json.mcpsnoop.bak` the first time, and rewrites only that +one server's entry, so your formatting and every other server are left alone. +Inside the rewritten entry the keys come back in alphabetical order. `unwrap` +restores the file, and removes the backup once no server is wrapped any more. +Restart Claude Desktop after either, since MCP servers are launched once at +startup. Then use your client as usual and open the UI. diff --git a/cmd/mcpsnoop/wrap.go b/cmd/mcpsnoop/wrap.go index 4d0aaf8..a57bde5 100644 --- a/cmd/mcpsnoop/wrap.go +++ b/cmd/mcpsnoop/wrap.go @@ -53,10 +53,16 @@ func lookupWrapClient(name string) (wrapClient, error) { return c, nil } -// backupSuffix names the copy wrap takes of the untouched config. It sits next -// to the config rather than under the mcpsnoop state directory so it is -// discoverable by anyone looking at the file they are worried about, and so it -// survives an MCPSNOOP_HOME change. +// backupSuffix names the copy wrap takes of the config before it wraps anything. +// It sits next to the config rather than under the mcpsnoop state directory so +// it is discoverable by anyone looking at the file they are worried about, and +// so it survives an MCPSNOOP_HOME change. +// +// There is one per config, and the first wrap is the one that writes it. A later +// wrap of a second server leaves it alone: overwriting would replace the +// untouched config with one mcpsnoop had already edited, and then unwrapping +// that second server would match it, restore it, and delete the only copy of the +// file as the user wrote it while the first server was still wrapped. const backupSuffix = ".mcpsnoop.bak" // wrapFault is an error that carries the exit code it should produce: 2 when the @@ -87,6 +93,12 @@ func reportFault(cmd *cobra.Command, verb string, err error) error { // test binary, the same seam convention runShimFn and runHTTPFn use. var wrapperPath = mcpsnoopPath +// writeConfigHook runs just before the config is re-read and written. It is the +// only way to test the recheck, since the window it guards is a few microseconds +// wide and a test that raced for it would be the flakiest thing in the suite. +// Nil outside tests. +var writeConfigHook func() + // mcpsnoopPath is the command wrap writes into the config. It is the absolute // path of the running binary, not the bare "mcpsnoop" the README shows, because // a desktop client is a GUI app: it spawns servers with the launchd or Explorer @@ -107,7 +119,7 @@ func newWrapCmd() *cobra.Command { Use: "wrap ", Short: "Route one of a client's MCP servers through mcpsnoop", Long: "Edit an MCP client's config so the named server starts through mcpsnoop, which is the one manual step between installing mcpsnoop and seeing traffic.\n\n" + - "Only that server's entry is rewritten. The config is copied to " + backupSuffix + " first, every other byte of the file is left exactly as it was, and mcpsnoop unwrap puts the entry back. Running it twice is a no-op, and --dry-run shows the change without writing anything.", + "Only that server's entry is rewritten, and its keys come back in alphabetical order. Every other byte of the file is left exactly as it was. The first wrap copies the config to " + backupSuffix + " and a later wrap of a second server keeps that copy, so it always holds the config as it was before mcpsnoop touched anything. mcpsnoop unwrap puts the entry back. Running it twice is a no-op, and --dry-run shows the change without writing anything.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if err := runWrap(cmd, clientName, configPath, args[0], dryRun); err != nil { @@ -127,7 +139,7 @@ func newUnwrapCmd() *cobra.Command { Use: "unwrap ", Short: "Take mcpsnoop back out of a client's MCP server entry", Long: "Undo mcpsnoop wrap for one server, so the client launches it directly again.\n\n" + - "When the rest of the config is still as wrap left it, the file is restored byte for byte from " + backupSuffix + " and the backup is removed. When it has changed since, only the named server's entry is rewritten so those changes survive, and the backup is kept. Running it on a server that is not wrapped is a no-op, and --dry-run writes nothing.", + "When the rest of the config is still as wrap left it, the file is restored byte for byte from " + backupSuffix + ". When it has changed since, only the named server's entry is rewritten so those changes survive. The backup is removed once no server in the config runs through mcpsnoop any more, and kept otherwise, since it holds a copy of everything in the config including any secrets in env blocks. Running it on a server that is not wrapped is a no-op, and --dry-run writes nothing.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if err := runUnwrap(cmd, clientName, configPath, args[0], dryRun); err != nil { @@ -164,13 +176,21 @@ func (t wrapTarget) backupPath() string { return t.path + backupSuffix } // resolveTarget locates one server's entry in a client config. Both commands // start here, so a missing config, a malformed one, and an unknown server report // identically whichever way you came in. -func resolveTarget(clientName, configPath, server string) (wrapTarget, error) { +func resolveTarget(clientName, configPath, server string, configGiven bool) (wrapTarget, error) { client, err := lookupWrapClient(clientName) if err != nil { return wrapTarget{}, err } path := configPath if path == "" { + // An explicitly empty --config is refused rather than treated as absent. + // A script written as `mcpsnoop wrap "$SRV" --config "$CFG"` with CFG unset + // would otherwise edit the user's live Claude Desktop config and drop a + // backup beside it, which is the one case where this writes to a file the + // caller never named. + if configGiven { + return wrapTarget{}, badInput("--config was given an empty path; omit it to use the well-known location") + } if path, err = client.configPath(); err != nil { return wrapTarget{}, badState("%w", err) } @@ -181,7 +201,7 @@ func resolveTarget(clientName, configPath, server string) (wrapTarget, error) { case errors.Is(err, fs.ErrNotExist): return wrapTarget{}, badState("no %s config at %s; add the server there first, or pass --config with the path to it", client.display, path) case err != nil: - return wrapTarget{}, badState("cannot read %s: %w", path, err) + return wrapTarget{}, badState("cannot read %s as JSON: %w", path, err) } // Preserve the config's own permissions. A stat failure here is not worth @@ -204,7 +224,7 @@ func resolveTarget(clientName, configPath, server string) (wrapTarget, error) { } func runWrap(cmd *cobra.Command, clientName, configPath, server string, dryRun bool) error { - t, err := resolveTarget(clientName, configPath, server) + t, err := resolveTarget(clientName, configPath, server, cmd.Flags().Changed("config")) if err != nil { return err } @@ -215,6 +235,14 @@ func runWrap(cmd *cobra.Command, clientName, configPath, server string, dryRun b // is also what keeps the backup holding the pre-wrap config rather than a // wrapped one. fmt.Fprintf(out, "%q is already wrapped in %s, nothing to do\n", server, t.client.display) + // Only the name is matched, not the path, so an entry can be wrapped around + // an mcpsnoop that has since moved or been deleted. Saying nothing there + // leaves the user running the one command that would fix it and being told + // there is nothing to fix. + if current := wrapperPath(); t.entry.command != current { + fmt.Fprintf(out, " it runs %s, not this binary at %s\n", t.entry.command, current) + fmt.Fprintf(out, " run mcpsnoop unwrap %q and wrap it again to point it here\n", server) + } return nil } if t.entry.command == "" { @@ -245,12 +273,15 @@ func runWrap(cmd *cobra.Command, clientName, configPath, server string, dryRun b } // The backup is written and closed before the config is touched, so a backup - // that cannot be written aborts with the config still untouched. - if err := writeFileAtomic(t.backupPath(), t.config, 0o600); err != nil { - return badState("cannot write the backup %s: %w", t.backupPath(), err) + // that cannot be written aborts with the config still untouched. An existing + // one is kept, since it is the earlier and therefore less-edited snapshot. + if _, err := os.Stat(t.backupPath()); errors.Is(err, fs.ErrNotExist) { + if err := writeFileAtomic(t.backupPath(), t.config, 0o600); err != nil { + return badState("cannot write the backup %s: %w", t.backupPath(), err) + } } - if err := writeFileAtomic(t.path, rewritten, t.mode); err != nil { - return badState("cannot write %s: %w", t.path, err) + if err := t.writeConfig(rewritten); err != nil { + return err } fmt.Fprintf(out, "wrapped %q in %s\n", server, t.client.display) @@ -261,7 +292,7 @@ func runWrap(cmd *cobra.Command, clientName, configPath, server string, dryRun b } func runUnwrap(cmd *cobra.Command, clientName, configPath, server string, dryRun bool) error { - t, err := resolveTarget(clientName, configPath, server) + t, err := resolveTarget(clientName, configPath, server, cmd.Flags().Changed("config")) if err != nil { return err } @@ -298,7 +329,7 @@ func runUnwrap(cmd *cobra.Command, clientName, configPath, server string, dryRun // backup describes the same config: if the user edited the file while it was // wrapped, restoring the backup would silently throw those edits away, so the // spliced result is written instead and the backup is kept. - restored, backup := false, t.backupPath() + restored, removed, backup := false, false, t.backupPath() if original, err := os.ReadFile(backup); err == nil && sameJSON(original, rewritten) { rewritten, restored = original, true } @@ -307,48 +338,111 @@ func runUnwrap(cmd *cobra.Command, clientName, configPath, server string, dryRun if dryRun { fmt.Fprintln(out, "dry run, nothing was written") printChange(out, t.path, before, after) - printRestoreNote(out, backup, restored, dryRun) + printRestoreNote(out, backup, restored, restored && !anyWrapped(rewritten, t.client.serversKey), dryRun) return nil } - if err := writeFileAtomic(t.path, rewritten, t.mode); err != nil { - return badState("cannot write %s: %w", t.path, err) + if err := t.writeConfig(rewritten); err != nil { + return err } - if restored { - // Only now, with the original back in place, is the backup redundant. + // The backup goes only when nothing in the file is wrapped any more. Removing + // it here because this one entry came back would take the copy of the + // untouched config away while another server still runs through mcpsnoop. + if restored && !anyWrapped(rewritten, t.client.serversKey) { if err := os.Remove(backup); err != nil && !errors.Is(err, fs.ErrNotExist) { return badState("unwrapped %s, but cannot remove the backup %s: %w", t.path, backup, err) } + removed = true } fmt.Fprintf(out, "unwrapped %q in %s\n", server, t.client.display) printChange(out, t.path, before, after) - printRestoreNote(out, backup, restored, dryRun) + printRestoreNote(out, backup, restored, removed, dryRun) fmt.Fprintf(out, "%s\n", t.client.restartHint) return nil } +// writeConfig writes the config back, refusing if the file moved since it was +// read. wrap and unwrap are a read, a rewrite and a write, and Claude Desktop +// rewrites this same file whenever a connector is toggled, so without this the +// whole of somebody else's edit disappears with both sides reporting success. +// It narrows the window rather than closing it, which a lock file would do at +// the cost of a stale lock nobody can explain. +func (t wrapTarget) writeConfig(rewritten []byte) error { + if writeConfigHook != nil { + writeConfigHook() + } + switch current, err := os.ReadFile(t.path); { + case err != nil: + return badState("cannot re-read %s before writing it: %w", t.path, err) + case !bytes.Equal(current, t.config): + return badState("%s changed while mcpsnoop was working on it, so nothing was written; run the command again", t.path) + } + if err := writeFileAtomic(t.path, rewritten, t.mode); err != nil { + return badState("cannot write %s: %w", t.path, err) + } + return nil +} + +// anyWrapped reports whether any server in the config still runs through +// mcpsnoop, which is what decides whether the backup is still needed. +func anyWrapped(config []byte, serversKey string) bool { + top, err := objectMembers(config) + if err != nil { + return true // cannot tell, so keep the backup + } + i := slices.IndexFunc(top, func(m jsonMember) bool { return m.name == serversKey }) + if i < 0 { + return false + } + entries, err := objectMembers(top[i].value) + if err != nil { + return true + } + for _, e := range entries { + entry, err := parseServerEntry(e.value, e.name) + if err != nil { + return true + } + if isWrapped(entry.command) { + return true + } + } + return false +} + func printChange(out io.Writer, path, before, after string) { fmt.Fprintf(out, " config: %s\n", path) fmt.Fprintf(out, " before: %s\n", before) fmt.Fprintf(out, " after: %s\n", after) } -// printRestoreNote says which of unwrap's two endings happened, or would have: -// the whole file put back from the backup, or just this entry rewritten because -// the rest of the config had moved on since it was wrapped. -func printRestoreNote(out io.Writer, backup string, restored, dryRun bool) { +// printRestoreNote says which of unwrap's endings happened, or would have: the +// whole file put back from the backup, or just this entry rewritten because the +// rest of the config had moved on since it was wrapped. Either way it says what +// became of the backup, because it holds a second copy of everything in the +// config, env blocks and their API keys included, and a user who is not told it +// is still there has no reason to go looking for it. +func printRestoreNote(out io.Writer, backup string, restored, removed, dryRun bool) { + restore, remove := "restored", "removed" + if dryRun { + restore, remove = "would restore", "would remove" + } switch { - case restored && dryRun: - fmt.Fprintf(out, " would restore the config byte for byte from %s, and remove it\n", backup) + case restored && removed: + fmt.Fprintf(out, " %s the config byte for byte from %s, and %s it, since nothing is wrapped any more\n", + restore, backup, remove) + return case restored: - fmt.Fprintf(out, " restored the config byte for byte from %s, and removed it\n", backup) + fmt.Fprintf(out, " %s the config byte for byte from %s\n", restore, backup) default: if _, err := os.Stat(backup); err != nil { return // never wrapped by this mcpsnoop, so there is nothing to say } - fmt.Fprintf(out, " the rest of the config changed since it was wrapped, so only this entry is rewritten and the original stays at %s\n", backup) + fmt.Fprintf(out, " the rest of the config changed since it was wrapped, so only this entry is rewritten\n") } + fmt.Fprintf(out, " %s stays at %s; it is a copy of the config, secrets in env blocks included, so delete it once you are happy\n", + "the backup", backup) } func commandLine(command string, args []string) string { @@ -456,6 +550,7 @@ func objectMembers(obj []byte) ([]jsonMember, error) { return nil, errors.New("expected a JSON object") } var members []jsonMember + seen := map[string]bool{} for dec.More() { nameTok, err := dec.Token() if err != nil { @@ -465,6 +560,14 @@ func objectMembers(obj []byte) ([]jsonMember, error) { if !ok { return nil, errors.New("expected a JSON object") } + // A repeated name is refused rather than resolved. Every JSON parser takes + // the last one and this walk finds the first, so editing here would rewrite + // an entry the client never reads: wrap would report success, the traffic + // would not change, and the user would have nothing to go on. + if seen[name] { + return nil, fmt.Errorf("the member %q appears twice, so it is ambiguous which one the client reads", name) + } + seen[name] = true var value json.RawMessage if err := dec.Decode(&value); err != nil { return nil, err @@ -484,7 +587,7 @@ func objectMembers(obj []byte) ([]jsonMember, error) { func findServerMember(config []byte, serversKey, server, path string) (jsonMember, error) { top, err := objectMembers(config) if err != nil { - return jsonMember{}, badState("cannot read %s as a JSON object: %w", path, err) + return jsonMember{}, badState("cannot read %s as JSON: %w", path, err) } i := slices.IndexFunc(top, func(m jsonMember) bool { return m.name == serversKey }) if i < 0 { @@ -494,7 +597,7 @@ func findServerMember(config []byte, serversKey, server, path string) (jsonMembe entries, err := objectMembers(section.value) if err != nil { - return jsonMember{}, badState("the %q section of %s is not a JSON object: %w", serversKey, path, err) + return jsonMember{}, badState("cannot read the %q section of %s as JSON: %w", serversKey, path, err) } j := slices.IndexFunc(entries, func(m jsonMember) bool { return m.name == server }) if j < 0 { @@ -539,13 +642,22 @@ func spliceMember(config []byte, member jsonMember, members map[string]json.RawM func encodeMembers(config []byte, member jsonMember, members map[string]json.RawMessage) ([]byte, error) { var buf bytes.Buffer enc := jsonwire.NewEncoder(&buf) - if bytes.ContainsRune(member.value, '\n') { + indented := bytes.ContainsRune(member.value, '\n') + if indented { enc.SetIndent(lineIndent(config, member.start), indentUnit(config)) } if err := enc.Encode(members); err != nil { return nil, badState("cannot encode the server entry: %w", err) } - return bytes.TrimSuffix(buf.Bytes(), []byte("\n")), nil + block := bytes.TrimSuffix(buf.Bytes(), []byte("\n")) + // encoding/json ends every line with a bare \n, so a Windows config came back + // with CRLF outside the rewritten entry and LF inside it. Mixed terminators in + // one file are a whole-file diff the next time anything normalises it, on the + // platform this command goes out of its way to support. + if indented && bytes.Contains(config, []byte("\r\n")) { + block = bytes.ReplaceAll(block, []byte("\n"), []byte("\r\n")) + } + return block, nil } // lineIndent is the leading whitespace of the line offset sits on, which is the @@ -570,8 +682,12 @@ func indentUnit(config []byte) string { } // sameJSON reports whether two configs describe the same document, ignoring -// formatting and key order. Unmarshalling into any and re-encoding sorts object -// keys, so the comparison survives the reordering a re-encoded entry causes. +// formatting and key order. Key order has to be ignored because re-encoding an +// entry sorts its keys, so a byte comparison would never match. +// +// The answer decides whether unwrap overwrites the whole file with the backup, +// so a false yes silently throws away whatever the user changed in between. +// normalizeJSON is written for that, not for convenience. func sameJSON(a, b []byte) bool { na, err := normalizeJSON(a) if err != nil { @@ -581,21 +697,124 @@ func sameJSON(a, b []byte) bool { if err != nil { return false } - return bytes.Equal(na, nb) + return na == nb } -func normalizeJSON(data []byte) ([]byte, error) { - var v any - if err := json.Unmarshal(data, &v); err != nil { - return nil, err +// normalizeJSON renders a document in a canonical form that keeps every +// distinction the file makes. +// +// json.Unmarshal into any cannot be used here. It lands every number in a +// float64, so a config carrying an account id of 9007199254740992 compares equal +// to the same config after the user corrects it to ...93, and unwrap then +// restores the backup over the correction and deletes the backup. It also keeps +// only the last of a repeated key, hiding a difference in the other copy. +// Decoding with UseNumber keeps the literal the file spelled, and a repeated +// name is an error rather than a silent collapse. +func normalizeJSON(data []byte) (string, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.UseNumber() + var buf bytes.Buffer + if err := normalizeValue(dec, &buf); err != nil { + return "", err + } + // Trailing bytes would mean two documents in one file, which is not a config. + if _, err := dec.Token(); !errors.Is(err, io.EOF) { + return "", errors.New("trailing content after the JSON document") + } + return buf.String(), nil +} + +// normalizeValue writes one value canonically, sorting object keys so a +// re-encoded entry still matches, and rejecting a repeated name so two documents +// that differ only in the copy a parser discards are never called equal. +func normalizeValue(dec *json.Decoder, buf *bytes.Buffer) error { + tok, err := dec.Token() + if err != nil { + return err + } + delim, ok := tok.(json.Delim) + if !ok { + return writeScalar(tok, buf) + } + switch delim { + case '{': + members := map[string]string{} + for dec.More() { + nameTok, err := dec.Token() + if err != nil { + return err + } + name, ok := nameTok.(string) + if !ok { + return errors.New("expected an object member name") + } + if _, dup := members[name]; dup { + return fmt.Errorf("the object member %q appears twice", name) + } + var value bytes.Buffer + if err := normalizeValue(dec, &value); err != nil { + return err + } + members[name] = value.String() + } + buf.WriteByte('{') + for i, name := range slices.Sorted(maps.Keys(members)) { + if i > 0 { + buf.WriteByte(',') + } + if err := writeScalar(name, buf); err != nil { + return err + } + buf.WriteByte(':') + buf.WriteString(members[name]) + } + buf.WriteByte('}') + case '[': + buf.WriteByte('[') + for i := 0; dec.More(); i++ { + if i > 0 { + buf.WriteByte(',') + } + if err := normalizeValue(dec, buf); err != nil { + return err + } + } + buf.WriteByte(']') + default: + return fmt.Errorf("unexpected %q", delim) + } + // The closing delimiter, which json.Decoder hands back as its own token. + _, err = dec.Token() + return err +} + +// writeScalar renders one scalar. A json.Number goes out as the literal the file +// spelled, which is the whole point of decoding with UseNumber. +func writeScalar(tok json.Token, buf *bytes.Buffer) error { + if n, ok := tok.(json.Number); ok { + buf.WriteString(n.String()) + return nil } - return jsonwire.Marshal(v) + raw, err := jsonwire.Marshal(tok) + if err != nil { + return err + } + buf.Write(raw) + return nil } // writeFileAtomic replaces path's contents in one step, so an interrupted write // can never leave a client staring at half a config. The temp file is created in // the config's own directory, which keeps the rename on a single filesystem. +// +// A symlink is followed first. os.Rename does not follow the final component, so +// renaming onto a link replaces the link with a regular file, which is how a +// config kept under stow or chezmoi silently stops being the file the dotfile +// repository manages. Resolving first writes the file the user actually keeps. func writeFileAtomic(path string, data []byte, mode fs.FileMode) error { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + path = resolved + } tmp, err := os.CreateTemp(filepath.Dir(path), ".mcpsnoop-wrap-*") if err != nil { return err diff --git a/cmd/mcpsnoop/wrap_claude_desktop.go b/cmd/mcpsnoop/wrap_claude_desktop.go index 25fa735..ec1a881 100644 --- a/cmd/mcpsnoop/wrap_claude_desktop.go +++ b/cmd/mcpsnoop/wrap_claude_desktop.go @@ -7,10 +7,20 @@ import "github.com/kerlenton/mcpsnoop/internal/paths" const claudeDesktopClient = "claude-desktop" // This file is the whole of Claude Desktop's support for wrap and unwrap, and it -// is the template for the next client: copy it, change the four fields, and the -// commands pick the client up with no edit to wrap.go. Everything below the -// registry is client independent, because every MCP client so far stores its -// servers the same way, as one object per server under a single top-level key. +// is the template for the next client: copy it, change the fields, and the +// commands pick the client up with no edit to wrap.go. +// +// The entry shape really is client independent. Every MCP client so far stores +// its servers as one object per server under a single top-level key, so +// serversKey is all that varies and every key mcpsnoop does not model survives +// the rewrite. The location is not. VS Code reads .vscode/mcp.json in the +// workspace as well as an mcp.json in the user profile, and says "When you use +// multiple profiles, each profile can have its own MCP server configuration"; +// Cursor and Claude Code split the same way. configPath returns one path and +// takes no arguments, so for those the user has to pass --config until it grows +// a notion of scope. wrapperPath is not client independent either once a +// workspace-scoped client is added, since a machine-local absolute path is +// wrong in a file a team commits. func init() { registerWrapClient(wrapClient{ name: claudeDesktopClient, diff --git a/cmd/mcpsnoop/wrap_test.go b/cmd/mcpsnoop/wrap_test.go index d0db363..9fde70f 100644 --- a/cmd/mcpsnoop/wrap_test.go +++ b/cmd/mcpsnoop/wrap_test.go @@ -337,7 +337,7 @@ func TestWrapReportsAProblemTheUserCanFix(t *testing.T) { { name: "malformed config", args: []string{"everything"}, config: "{ not json", code: 1, - want: []string{"as a JSON object"}, + want: []string{"as JSON", "invalid character"}, }, { name: "no mcpServers section", @@ -547,3 +547,296 @@ func TestWrapClientsAreRegisteredNotHardcoded(t *testing.T) { t.Fatalf("unwrap for a registered client:\n got %q\nwant %q", got, config) } } + +// TestWrapFollowsASymlinkedConfig. A config kept under stow or chezmoi is a +// symlink into a dotfile repository. os.Rename does not follow the final +// component, so renaming onto the link replaced it with a regular file and the +// repository copy stopped being the file Claude Desktop reads, with nothing said +// about it either way. +func TestWrapFollowsASymlinkedConfig(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "dotfiles", "claude.json") + if err := os.MkdirAll(filepath.Dir(real), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(real, []byte(wrapFixture), 0o644); err != nil { + t.Fatal(err) + } + link := newWrapTest(t, wrapFixture) + if err := os.Remove(link); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, link); err != nil { + t.Skipf("this filesystem will not take a symlink: %v", err) + } + + wrapOK(t, newWrapCmd, "everything", "--config", link) + + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("the symlink was replaced by a regular file, so the dotfile repository no longer owns the config") + } + if !strings.Contains(readConfig(t, real), stubWrapperPath) { + t.Fatal("the edit did not reach the file the link points at") + } +} + +// TestUnwrapKeepsAnEditJSONNormalisationWouldHide. The restore overwrites the +// whole file, so the check that the backup still describes it has to keep every +// distinction the file makes. Deciding it by unmarshalling into any does not: +// every number lands in a float64, so correcting an id from ...92 to ...93 while +// wrapped compared equal, and unwrap put the old value back and deleted the +// backup, leaving no way to it. +func TestUnwrapKeepsAnEditJSONNormalisationWouldHide(t *testing.T) { + const withID = `{ + "accountId": 9007199254740992, + "mcpServers": { + "everything": { + "command": "npx", + "args": ["server.js"] + } + } +} +` + path := newWrapTest(t, withID) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + edited := strings.Replace(readConfig(t, path), "9007199254740992", "9007199254740993", 1) + if err := os.WriteFile(path, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + + if got := readConfig(t, path); !strings.Contains(got, "9007199254740993") { + t.Fatalf("the correction was reverted by the restore:\n%s", got) + } + if _, err := os.Stat(path + backupSuffix); err != nil { + t.Fatal("the backup was removed even though it no longer describes the config") + } +} + +// TestWrapRefusesADuplicateServerName. Every JSON parser keeps the last of a +// repeated name and this walk finds the first, so editing here rewrote an entry +// the client never reads. wrap reported success, the traffic did not change, and +// there was nothing to go on. +func TestWrapRefusesADuplicateServerName(t *testing.T) { + const duplicated = `{ + "mcpServers": { + "everything": { "command": "old", "args": ["v1"] }, + "everything": { "command": "new", "args": ["v2"] } + } +} +` + path := newWrapTest(t, duplicated) + before := readConfig(t, path) + + code, _, stderr := executeWrapCmd(t, newWrapCmd, "everything", "--config", path) + if code != 1 { + t.Fatalf("exit = %d, want 1", code) + } + if !strings.Contains(stderr, "appears twice") { + t.Fatalf("the message must name the duplicate: %q", stderr) + } + if readConfig(t, path) != before { + t.Fatal("the config was rewritten anyway") + } +} + +// TestWrapKeepsTheUntouchedBackupAcrossTwoServers. There is one backup per +// config, and its whole value is being the file as it was before mcpsnoop +// touched anything. Overwriting it on the second wrap put an already-wrapped +// config there, and unwrapping that second server then matched it, restored it +// and deleted it while the first server was still wrapped, leaving a modified +// config and no backup at all under a message that reads as an all-clear. +func TestWrapKeepsTheUntouchedBackupAcrossTwoServers(t *testing.T) { + path := newWrapTest(t, wrapFixture) + backup := path + backupSuffix + + wrapOK(t, newWrapCmd, "everything", "--config", path) + wrapOK(t, newWrapCmd, "other", "--config", path) + if got := readConfig(t, backup); got != wrapFixture { + t.Fatalf("the second wrap overwrote the backup:\n%s", got) + } + + wrapOK(t, newUnwrapCmd, "other", "--config", path) + if _, err := os.Stat(backup); err != nil { + t.Fatal("the backup went while \"everything\" was still wrapped") + } + + out := wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if got := readConfig(t, path); got != wrapFixture { + t.Fatalf("the config did not come back byte for byte:\n%s", got) + } + if _, err := os.Stat(backup); err == nil { + t.Fatal("the backup stayed even though nothing is wrapped any more") + } + if !strings.Contains(out, "nothing is wrapped any more") { + t.Fatalf("unwrap must say why it removed the backup: %q", out) + } +} + +// TestUnwrapSaysTheBackupIsStillThere. On the splice path the backup stays, and +// it holds a second copy of every env block in the config. A user who is not +// told it is there has no reason to go looking for it. +func TestUnwrapSaysTheBackupIsStillThere(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + edited := strings.Replace(readConfig(t, path), "Ctrl+Space", "Alt+Space", 1) + if err := os.WriteFile(path, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + + out := wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if !strings.Contains(out, "secrets in env blocks included") { + t.Fatalf("unwrap must say the backup is a copy of the config: %q", out) + } + if _, err := os.Stat(path + backupSuffix); err != nil { + t.Fatal("the backup should be kept when the config moved on") + } +} + +// TestWrapKeepsTheConfigsLineEndings. encoding/json ends every line with a bare +// \n, so a Windows config came back with CRLF outside the rewritten entry and LF +// inside it. Mixed terminators in one file are a whole-file diff the next time +// anything normalises it, on the platform this command goes out of its way to +// support. +func TestWrapKeepsTheConfigsLineEndings(t *testing.T) { + path := newWrapTest(t, strings.ReplaceAll(wrapFixture, "\n", "\r\n")) + + wrapOK(t, newWrapCmd, "everything", "--config", path) + + got := readConfig(t, path) + if cr, lf := strings.Count(got, "\r\n"), strings.Count(got, "\n"); cr != lf { + t.Fatalf("%d CRLF against %d LF, so the file now mixes terminators:\n%q", cr, lf, got) + } + wrapOK(t, newUnwrapCmd, "everything", "--config", path) + if got := readConfig(t, path); got != strings.ReplaceAll(wrapFixture, "\n", "\r\n") { + t.Fatalf("a CRLF config did not survive the round trip:\n%q", got) + } +} + +// TestWrapRefusesAnEmptyConfigFlag. Cobra cannot tell a flag that was never +// passed from one passed as empty, and treating both as absent meant a script +// written as `mcpsnoop wrap "$SRV" --config "$CFG"` with CFG unset edited the +// user's live Claude Desktop config. It is the one path where this writes to a +// file the caller never named. +func TestWrapRefusesAnEmptyConfigFlag(t *testing.T) { + for _, newCmd := range []func() *cobra.Command{newWrapCmd, newUnwrapCmd} { + code, _, stderr := executeWrapCmd(t, newCmd, "everything", "--config", "") + if code != 2 { + t.Fatalf("exit = %d, want 2", code) + } + if !strings.Contains(stderr, "empty path") { + t.Fatalf("the message must say what was wrong: %q", stderr) + } + } +} + +// TestWrapRefusesToWriteOverAConfigThatMoved. wrap is a read, a rewrite and a +// write, and Claude Desktop rewrites this same file whenever a connector is +// toggled. Without the recheck the whole of somebody else's edit disappears and +// both sides report success. +func TestWrapRefusesToWriteOverAConfigThatMoved(t *testing.T) { + path := newWrapTest(t, wrapFixture) + t.Cleanup(func() { writeConfigHook = nil }) + + moved := strings.Replace(wrapFixture, `"Ctrl+Space"`, `"Alt+Space"`, 1) + writeConfigHook = func() { + writeConfigHook = nil + if err := os.WriteFile(path, []byte(moved), 0o644); err != nil { + t.Fatal(err) + } + } + + code, _, stderr := executeWrapCmd(t, newWrapCmd, "everything", "--config", path) + if code != 1 { + t.Fatalf("exit = %d, want 1", code) + } + if !strings.Contains(stderr, "changed while mcpsnoop was working on it") { + t.Fatalf("the message must say why nothing was written: %q", stderr) + } + if got := readConfig(t, path); got != moved { + t.Fatalf("the other writer's config was overwritten:\n%s", got) + } +} + +// TestWrapSaysWhenTheWrappedCommandIsNotThisBinary. isWrapped matches the name +// and not the path, so an entry can be wrapped around an mcpsnoop that has since +// moved. Saying only "nothing to do" left the user running the one command that +// would fix it and being told there was nothing to fix. +func TestWrapSaysWhenTheWrappedCommandIsNotThisBinary(t *testing.T) { + path := newWrapTest(t, wrapFixture) + wrapOK(t, newWrapCmd, "everything", "--config", path) + + orig := wrapperPath + wrapperPath = func() string { return "/usr/local/bin/mcpsnoop" } + t.Cleanup(func() { wrapperPath = orig }) + + out := wrapOK(t, newWrapCmd, "everything", "--config", path) + if !strings.Contains(out, "nothing to do") { + t.Fatalf("a second wrap is still a no-op: %q", out) + } + if !strings.Contains(out, stubWrapperPath) || !strings.Contains(out, "/usr/local/bin/mcpsnoop") { + t.Fatalf("the note must name both paths: %q", out) + } + if !strings.Contains(out, "unwrap") { + t.Fatalf("the note must say how to re-point it: %q", out) + } +} + +// TestSameJSONKeepsEveryDistinctionTheFileMakes. sameJSON decides whether unwrap +// overwrites the whole config with the backup, so a false yes throws away +// whatever the user changed in between. json.Unmarshal into any answers this +// question wrongly twice over, collapsing large integers into a float64 and +// keeping only the last of a repeated name. +func TestSameJSONKeepsEveryDistinctionTheFileMakes(t *testing.T) { + for _, tc := range []struct { + name string + a, b string + same bool + }{ + {"reordered keys", `{"a":1,"b":2}`, `{"b":2,"a":1}`, true}, + {"reindented", "{\n \"a\": [1, 2]\n}", `{"a":[1,2]}`, true}, + {"integers past float64", `{"id":9007199254740992}`, `{"id":9007199254740993}`, false}, + {"integer against float", `{"id":1}`, `{"id":1.0}`, false}, + {"exponent against digits", `{"id":100}`, `{"id":1e2}`, false}, + {"a discarded duplicate", `{"a":9,"a":2}`, `{"a":7,"a":2}`, false}, + {"a genuine difference", `{"a":1}`, `{"a":2}`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := sameJSON([]byte(tc.a), []byte(tc.b)); got != tc.same { + t.Fatalf("sameJSON(%s, %s) = %v, want %v", tc.a, tc.b, got, tc.same) + } + }) + } +} + +// TestUnwrapKeepsTheBackupWhileAHandWrappedServerRemains. A user can wrap an +// entry by hand before mcpsnoop ever runs, and then the backup mcpsnoop takes +// holds a config that is already partly wrapped. Restoring it is right, removing +// it is not: the other server still runs through mcpsnoop and the copy of the +// config is still the only way back. +func TestUnwrapKeepsTheBackupWhileAHandWrappedServerRemains(t *testing.T) { + handWrapped := strings.Replace(wrapFixture, + `"other": { "command": "python", "args": ["server.py"] }`, + `"other": { "command": "`+stubWrapperPath+`", "args": ["--", "python", "server.py"] }`, 1) + path := newWrapTest(t, handWrapped) + + wrapOK(t, newWrapCmd, "everything", "--config", path) + out := wrapOK(t, newUnwrapCmd, "everything", "--config", path) + + if got := readConfig(t, path); got != handWrapped { + t.Fatalf("the config did not come back byte for byte:\n%s", got) + } + if _, err := os.Stat(path + backupSuffix); err != nil { + t.Fatal(`the backup went while "other" still runs through mcpsnoop`) + } + if strings.Contains(out, "nothing is wrapped any more") { + t.Fatalf("unwrap claimed the config is clear while a server is still wrapped: %q", out) + } +} diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 5714a9f..6d91013 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -101,16 +101,51 @@ func TestClaudeDesktopConfigTracksTheOSConfigDir(t *testing.T) { if want := filepath.Join(dir, "Claude", "claude_desktop_config.json"); got != want { t.Fatalf("ClaudeDesktopConfig() = %q, want %q", got, want) } - // The path belongs to another application, so resolving it must not bring any - // part of it into existence the way Base and its callers deliberately do. - // Comparing existence either side of the call says so wherever the test runs, - // whether or not Claude Desktop is installed on the machine. - before := exists(filepath.Dir(got)) - if _, err := ClaudeDesktopConfig(); err != nil { +} + +// TestClaudeDesktopConfigCreatesNothing. The path belongs to another +// application, so resolving it must not bring any part of it into existence the +// way Base and its callers deliberately do. +// +// The user config dir is pointed at an empty root first. Sampling existence on +// the real one proves nothing, because every machine with Claude Desktop +// installed already has the directory and the check passes whatever the helper +// does, which is how it passed with an os.MkdirAll injected into it. +func TestClaudeDesktopConfigCreatesNothing(t *testing.T) { + root := setUserConfigDir(t) + + got, err := ClaudeDesktopConfig() + if err != nil { t.Fatal(err) } - if exists(filepath.Dir(got)) != before { - t.Fatalf("ClaudeDesktopConfig changed whether %q exists", filepath.Dir(got)) + if !strings.HasPrefix(got, root) { + t.Fatalf("ClaudeDesktopConfig() = %q, which is not under the redirected root %q", got, root) + } + for dir := filepath.Dir(got); len(dir) > len(root); dir = filepath.Dir(dir) { + if exists(dir) { + t.Fatalf("resolving the path created %q", dir) + } + } + if exists(got) { + t.Fatalf("resolving the path created %q", got) + } +} + +// setUserConfigDir points os.UserConfigDir at an empty directory and returns it, +// through whichever variable the running platform actually consults. +func setUserConfigDir(t *testing.T) string { + t.Helper() + root := t.TempDir() + switch runtime.GOOS { + case "windows": + t.Setenv("AppData", root) + return root + case "darwin": + t.Setenv("HOME", root) + return filepath.Join(root, "Library", "Application Support") + default: + t.Setenv("XDG_CONFIG_HOME", root) + return root } }