From 3bfa7a7e682027b78b520b373d9a4e5a6601908a Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 10 Sep 2026 18:38:57 +0300 Subject: [PATCH 1/5] Make uninstall the full undo of install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Accept a profile name with or without the "@" everywhere: Resolve is now one rule (http(s) URL = endpoint, anything else = profile name), so `uninstall @name` no longer reports "no entry" while `show name` and `uninstall name` work — the asymmetry that started this - Remove the [profiles.] block from config.toml on uninstall, plus the tokens stored for it: with the profile gone there is no target left to `logout` with, and users were cleaning the TOML by hand - Follow the entry's args to find the profile, so `install --name X` stays undoable; warn when the removed entry was not an mcpurl one - Make each part best-effort: a half-installed setup (entry already deleted, profile still in config.toml) is exactly what needs cleaning, so only "nothing found anywhere" is an error - Update an existing profile in place instead of refusing with "pass a different --name": the URL follows the new target, flags override, and unmentioned settings (headers above all) are carried over - Edit config.toml as text (pkg/app/tomledit.go), never a TOML round-trip: comments, alignment and other profiles survive, including a comment that documents the profile below the one being removed - Write both configs via tmp+rename so an interrupted write cannot truncate them; keep an existing file's mode, .bak as before - Fix install treating a bare profile name as a URL, which minted a duplicate profile named after the host --- README.md | 14 +- cmd/mcpurl/main.go | 18 +- internal/atomicfile/atomicfile.go | 60 ++++++ internal/atomicfile/atomicfile_test.go | 66 +++++++ pkg/app/app.go | 5 +- pkg/app/commands.go | 33 ++-- pkg/app/commands_test.go | 25 +++ pkg/app/install.go | 262 +++++++++++++++++++++---- pkg/app/install_test.go | 159 ++++++++++++++- pkg/app/profile.go | 45 +++-- pkg/app/profile_test.go | 20 ++ pkg/app/tomledit.go | 108 ++++++++++ pkg/app/tomledit_test.go | 79 ++++++++ pkg/oauth/store.go | 26 +-- 14 files changed, 816 insertions(+), 104 deletions(-) create mode 100644 internal/atomicfile/atomicfile.go create mode 100644 internal/atomicfile/atomicfile_test.go create mode 100644 pkg/app/commands_test.go create mode 100644 pkg/app/tomledit.go create mode 100644 pkg/app/tomledit_test.go diff --git a/README.md b/README.md index daeb82c..84df6e4 100644 --- a/README.md +++ b/README.md @@ -63,8 +63,9 @@ mcpurl login @mysrv # browser → IdP → tokens in the keychain mcpurl token @mysrv # print a live access token (auto-refresh) for curl ``` -Undo: `mcpurl uninstall mysrv` (profile and tokens stay; `mcpurl logout -@mysrv` drops the tokens). +Undo: `mcpurl uninstall mysrv` (or `@mysrv`) — it removes the client entry, the +profile it references and the stored tokens. Re-running `install` on an +existing name updates the profile in place. > **Claude Code doesn't need a bridge** — it connects directly: > `claude mcp add --transport http mysrv https://mcp.example.com/mcp` @@ -80,13 +81,16 @@ mcpurl show [name] list profiles / show one (no secrets) mcpurl claude-config print ready-to-paste client config mcpurl install [--client claude-desktop|cursor|windsurf] write the entry into the client config -mcpurl uninstall [--client ...] remove a previously installed entry +mcpurl uninstall [--client ...] + remove the entry, its profile, tokens mcpurl version ``` The bare-URL form works with zero config against any spec-compliant server — discovery does the rest. A profile only pins what discovery can't provide -(client_id, a non-default callback port, scope overrides). +(client_id, a non-default callback port, scope overrides). The `@` is a +readability marker, not a requirement: `@mysrv` and `mysrv` are the same +target in every command. ## Profiles @@ -122,7 +126,7 @@ Resolution order for every setting: **flag > profile > discovery > default**. | `--no-sse` | false | Don't open the GET listening stream | | `--allow-http` | false | Permit plain http for non-loopback hosts | | `--timeout 10s` | `10s` | Connect/TLS timeout (no overall response timeout — tools can be slow) | -| `-v` | false | Debug log to stderr, `Authorization` redacted | +| `-v` | false | Debug log to stderr: statuses, challenges and failed-response bodies; never request headers or token material | ## How auth works diff --git a/cmd/mcpurl/main.go b/cmd/mcpurl/main.go index 1548d0b..5998cdd 100644 --- a/cmd/mcpurl/main.go +++ b/cmd/mcpurl/main.go @@ -21,6 +21,9 @@ import ( const appName = "mcpurl" +// clientHelp is the --client flag help, derived from the one supported list. +var clientHelp = "target MCP client: " + strings.Join(app.Clients(), "|") + var version = "dev" // stamped via -ldflags "-X main.version=…" // Exit codes (02-cli.md). @@ -78,11 +81,14 @@ Usage: mcpurl claude-config print MCP client config snippets mcpurl install [flags] write the entry into the MCP client's - config (persists a profile for URLs) - mcpurl uninstall [--client ...] remove a previously installed entry + config (persists/updates the profile) + mcpurl uninstall [--client ...] + remove the entry, its profile and its + stored tokens mcpurl version -Profiles: ~/.config/mcpurl/config.toml (MCPURL_CONFIG overrides). Run +Profiles: ~/.config/mcpurl/config.toml (MCPURL_CONFIG overrides). The "@" is +optional everywhere — "@mysrv" and "mysrv" are the same target. Run 'mcpurl -h' for flags.`) } @@ -251,7 +257,7 @@ func cmdLogout(args []string) int { func cmdInstall(args []string) int { var o app.Options fs := newFlagSet(appName+" install", &o) - client := fs.String("client", "claude-desktop", "target MCP client: claude-desktop|cursor|windsurf") + client := fs.String("client", "claude-desktop", clientHelp) name := fs.String("name", "", "entry/profile name (default: profile name or host)") dryRun := fs.Bool("dry-run", false, "print the resulting config without writing") target, err := parseTarget(fs, args) @@ -274,10 +280,10 @@ func cmdInstall(args []string) int { func cmdUninstall(args []string) int { fs := flag.NewFlagSet(appName+" uninstall", flag.ContinueOnError) - client := fs.String("client", "claude-desktop", "target MCP client: claude-desktop|cursor|windsurf") + client := fs.String("client", "claude-desktop", clientHelp) name, err := parseTarget(fs, args) if err != nil || name == "" { - return fail(exitUsage, errors.New("usage: mcpurl uninstall [--client ...]")) + return fail(exitUsage, errors.New("usage: mcpurl uninstall [--client ...]")) } a, err := app.New(app.Options{}, os.Stderr) if err != nil { diff --git a/internal/atomicfile/atomicfile.go b/internal/atomicfile/atomicfile.go new file mode 100644 index 0000000..72ee706 --- /dev/null +++ b/internal/atomicfile/atomicfile.go @@ -0,0 +1,60 @@ +// Package atomicfile writes a file so that readers see either the previous +// content or the new one, never a half-written mix. +// +// mcpurl replaces three kinds of file this way — the token store, the profile +// config and an MCP client's JSON — and all three are read by other processes +// (a second bridge instance, Claude Desktop) while mcpurl runs. A plain +// os.WriteFile truncates first, so an interrupted write leaves a config the +// client refuses to parse. One implementation, one place to audit the +// tmp-cleanup and permission handling. +package atomicfile + +import ( + "os" + "path/filepath" +) + +// Write replaces path with data via a temp file in the same directory plus a +// rename. Missing parent directories are created 0700; the temp file gets +// mode before it is renamed, so the file is never briefly world-readable. +// +// A symlink is followed, not replaced: config files under a dotfiles +// repository are routinely symlinked into ~/.config, and renaming over the +// link would detach it — mcpurl would keep writing a file the user no longer +// edits, in silence. +func Write(path string, data []byte, mode os.FileMode) error { + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + defer os.Remove(tmp.Name()) //nolint:errcheck // no-op after a successful rename + if err := tmp.Chmod(mode); err != nil { + tmp.Close() //nolint:errcheck // the write already failed + return err + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() //nolint:errcheck // the write already failed + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmp.Name(), path) +} + +// Mode reports the mode Write should use to replace path: the existing file's, +// or fallback for a new one. Configs belong to the user — replacing one must +// not quietly tighten or loosen it. +func Mode(path string, fallback os.FileMode) os.FileMode { + if fi, err := os.Stat(path); err == nil { + return fi.Mode().Perm() + } + return fallback +} diff --git a/internal/atomicfile/atomicfile_test.go b/internal/atomicfile/atomicfile_test.go new file mode 100644 index 0000000..8bfab34 --- /dev/null +++ b/internal/atomicfile/atomicfile_test.go @@ -0,0 +1,66 @@ +package atomicfile + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWriteCreatesAndReplaces(t *testing.T) { + path := filepath.Join(t.TempDir(), "sub", "config.toml") + require.NoError(t, Write(path, []byte("one\n"), 0o600)) + + fi, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), fi.Mode().Perm()) + + require.NoError(t, Write(path, []byte("two\n"), Mode(path, 0o600))) + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "two\n", string(data)) + + // The temp file must not survive a successful write. + entries, err := os.ReadDir(filepath.Dir(path)) + require.NoError(t, err) + assert.Len(t, entries, 1, "no .tmp-* leftovers") +} + +// Config files symlinked out of a dotfiles repository are the common case +// this must not break: rename over the link would detach it, leaving the user +// editing a file mcpurl no longer reads. +func TestWriteFollowsSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "dotfiles", "mcpurl.toml") + link := filepath.Join(dir, "config", "config.toml") + require.NoError(t, os.MkdirAll(filepath.Dir(target), 0o700)) + require.NoError(t, os.MkdirAll(filepath.Dir(link), 0o700)) + require.NoError(t, os.WriteFile(target, []byte("old\n"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + require.NoError(t, Write(link, []byte("new\n"), Mode(link, 0o600))) + + fi, err := os.Lstat(link) + require.NoError(t, err) + assert.NotZero(t, fi.Mode()&os.ModeSymlink, "the symlink must survive the write") + + data, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "new\n", string(data), "the write must land in the link target") +} + +// A user's config keeps the mode they gave it; only a new file gets 0600. +func TestModePreservesExisting(t *testing.T) { + path := filepath.Join(t.TempDir(), "mcp.json") + require.NoError(t, os.WriteFile(path, []byte("{}"), 0o644)) + assert.Equal(t, os.FileMode(0o644), Mode(path, 0o600)) + + require.NoError(t, Write(path, []byte("{}\n"), Mode(path, 0o600))) + fi, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), fi.Mode().Perm(), "replacing must not tighten the mode") + + assert.Equal(t, os.FileMode(0o600), Mode(filepath.Join(t.TempDir(), "absent"), 0o600)) +} diff --git a/pkg/app/app.go b/pkg/app/app.go index 5495c25..1818029 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -179,6 +179,9 @@ func (a *App) tokenProvider() mcp.TokenProvider { return a.flow() } +// store is the token backend for this run (--no-keychain forces files). +func (a *App) store() oauth.Store { return oauth.NewStore(a.opts.NoKeychain) } + func (a *App) flow() *oauth.Flow { return &oauth.Flow{ Endpoint: a.prof.URL, @@ -186,7 +189,7 @@ func (a *App) flow() *oauth.Flow { Scopes: a.prof.Scopes, Issuer: a.prof.Issuer, CallbackPort: a.prof.CallbackPort, - Store: oauth.NewStore(a.opts.NoKeychain), + Store: a.store(), HTTP: a.httpc, Logf: a.logf, Warnf: a.warnf, diff --git a/pkg/app/commands.go b/pkg/app/commands.go index 4b1b5e9..b537169 100644 --- a/pkg/app/commands.go +++ b/pkg/app/commands.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "maps" - "net/url" "slices" "strings" @@ -61,14 +60,14 @@ func (a *App) Logout() error { if a.prof.ClientID == "" { return errors.New("logout needs a clientID (profile or --client-id) to locate the stored token") } - a.dropTokens(oauth.NewStore(a.opts.NoKeychain), a.prof) + a.dropTokens(a.store(), a.prof) return nil } // LogoutAll drops tokens for every configured profile plus file-backend // leftovers from bare-URL runs; needs no target resolution. func (a *App) LogoutAll() error { - store := oauth.NewStore(a.opts.NoKeychain) + store := a.store() for _, name := range a.cfg.Names() { a.dropTokens(store, a.cfg.Profiles[name]) } @@ -76,6 +75,9 @@ func (a *App) LogoutAll() error { return nil } +// dropTokens deletes the stored token for p. The deletion is unconditional +// (an unreadable leftover must go too), but nothing is reported when there was +// nothing stored — "dropped tokens" for an empty store reads as a lie. func (a *App) dropTokens(store oauth.Store, p Profile) { if p.URL == "" || p.ClientID == "" { return @@ -84,11 +86,17 @@ func (a *App) dropTokens(store oauth.Store, p Profile) { if err != nil { return } + // Load first only to decide whether to report: it costs one os.ReadFile + // for the "nothing stored" case (both backends answer ErrNotFound before + // touching the keyring), and Delete must stay a no-op either way. + _, loadErr := store.Load(key) if err := store.Delete(key); err != nil { a.warnf("logout %s: %v", p.Name, err) return } - fmt.Fprintf(a.msg, "dropped tokens for %s\n", p.URL) + if !errors.Is(loadErr, oauth.ErrNotFound) { + fmt.Fprintf(a.msg, "dropped tokens for %s\n", p.URL) + } } // Show writes the profile list (name == "") or one profile without secrets. @@ -107,8 +115,7 @@ func (a *App) Show(w io.Writer, name string) error { if err != nil { return err } - name = strings.TrimPrefix(name, "@") - fmt.Fprintf(w, "profile: @%s\nurl: %s\n", name, p.URL) + fmt.Fprintf(w, "profile: @%s\nurl: %s\n", p.Name, p.URL) if p.ClientID != "" { fmt.Fprintf(w, "clientID: %s\n", p.ClientID) } @@ -142,11 +149,13 @@ func (a *App) ClaudeConfig(w io.Writer, target, name string) error { name = prof.Name } if name == "" { - if u, err := url.Parse(prof.URL); err == nil { - name = strings.Split(u.Hostname(), ".")[0] - } else { - name = "mcp" - } + name = hostLabel(prof.URL) // same default install uses + } + // The snippet is config, not shorthand: spell the profile reference the + // way install writes it, "@" included, whichever form was typed. + arg := target + if prof.Name != "" { + arg = "@" + prof.Name } fmt.Fprintf(w, `# Claude Desktop / Cursor / Windsurf (stdio bridge): { @@ -159,6 +168,6 @@ func (a *App) ClaudeConfig(w io.Writer, target, name string) error { } # Claude Code (no bridge needed — connects directly): # claude mcp add --transport http %s %s -`, name, target, name, prof.URL) +`, name, arg, name, prof.URL) return nil } diff --git a/pkg/app/commands_test.go b/pkg/app/commands_test.go new file mode 100644 index 0000000..3c5cabd --- /dev/null +++ b/pkg/app/commands_test.go @@ -0,0 +1,25 @@ +package app + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// claude-config used to be the one command that insisted on the "@" — the +// same asymmetry that made `uninstall @name` fail from the other side. +func TestClaudeConfigAcceptsBareName(t *testing.T) { + writeConfig(t) + a, err := New(Options{}, nil) + require.NoError(t, err) + + var bare, at strings.Builder + require.NoError(t, a.ClaudeConfig(&bare, "acme", "")) + require.NoError(t, a.ClaudeConfig(&at, "@acme", "")) + assert.Equal(t, at.String(), bare.String(), "both spellings render the same snippet") + assert.Contains(t, bare.String(), "https://mcp.acme.example/mcp") + assert.Contains(t, bare.String(), `"@acme"`, + "the snippet is config: spell the reference the way install writes it") +} diff --git a/pkg/app/install.go b/pkg/app/install.go index 5250cd3..79d0826 100644 --- a/pkg/app/install.go +++ b/pkg/app/install.go @@ -1,6 +1,7 @@ // install/uninstall: write the mcpServers entry into an MCP client's config // (02-cli.md). The JSON merge is surgical — only mcpServers. changes — -// and a .bak copy is written first. +// and a .bak copy is written first. The config.toml side is edited as text +// (one [profiles.] block), so comments and other profiles survive. package app @@ -9,13 +10,26 @@ import ( "encoding/json" "fmt" "io" + "maps" "net/url" "os" "path/filepath" "runtime" + "slices" "strings" + + "github.com/vmkteam/mcpurl/internal/atomicfile" ) +// knownClients is the single list of MCP clients install/uninstall know how +// to configure: the --client flag help, the "unknown client" error and the +// uninstall cross-check (a profile may back entries in several clients) all +// read it, so adding a client is one edit here plus one case below. +var knownClients = []string{"claude-desktop", "cursor", "windsurf"} + +// Clients lists the supported MCP clients, for the CLI's flag help. +func Clients() []string { return slices.Clone(knownClients) } + // clientConfigPath locates the MCP client's config file for this OS. func clientConfigPath(client string) (string, error) { home, err := os.UserHomeDir() @@ -37,7 +51,7 @@ func clientConfigPath(client string) (string, error) { case "windsurf": return filepath.Join(home, ".codeium", "windsurf", "mcp_config.json"), nil default: - return "", fmt.Errorf("unknown client %q (want claude-desktop, cursor or windsurf)", client) + return "", fmt.Errorf("unknown client %q (want %s)", client, strings.Join(knownClients, ", ")) } } @@ -50,8 +64,10 @@ func (a *App) Install(w io.Writer, client, name string, dryRun bool) error { return err } - profName := strings.TrimPrefix(a.opts.Target, "@") - if !strings.HasPrefix(a.opts.Target, "@") { + // Name is set only when the target resolved to a stored profile; a URL + // target has to persist one first. + profName := a.prof.Name + if profName == "" { if profName = name; profName == "" { profName = hostLabel(a.prof.URL) } @@ -80,52 +96,223 @@ func (a *App) Install(w io.Writer, client, name string, dryRun bool) error { return nil } -// Uninstall removes the entry (and nothing else); profile and tokens stay. +// Uninstall is the full undo of install: the client entry, the profile it +// references and the tokens stored for it. Each part is best-effort — a +// half-installed setup (entry already gone, profile still in config.toml) is +// exactly what needs cleaning up — and only an empty result is an error. +// The name may be written with or without the "@" prefix. func (a *App) Uninstall(client, name string) error { + name = strings.TrimPrefix(name, "@") path, err := clientConfigPath(client) if err != nil { return err } - if err := removeServerEntry(path, name); err != nil { + + // The entry name and the profile name can differ (install --name), so the + // profile to drop is the one the entry actually points at. + profName := name + entry, entryGone, err := removeServerEntry(path, name) + if err != nil { return err } - fmt.Fprintf(a.msg, "removed %q from %s (backup: .bak)\n", name, path) + if entryGone { + fmt.Fprintf(a.msg, "removed %q from %s (backup: .bak)\n", name, path) + if cmd := entryCommand(entry); cmd != "" && !ownBinary(cmd) { + // Not ours: uninstall is destructive and the name was a guess. + a.warnf("%q was not an mcpurl entry (command: %s) — restore it from %s.bak if that was a typo", + name, cmd, path) + } + if ref := entryProfile(entry); ref != "" { + profName = ref + } + } else { + fmt.Fprintf(a.msg, "no entry %q in %s\n", name, path) + } + + prof, hadProfile := a.cfg.Profiles[profName] + if hadProfile { + if err := a.editProfileBlock(profName, ""); err != nil { + return err + } + delete(a.cfg.Profiles, profName) + fmt.Fprintf(a.msg, "removed profile @%s from %s (backup: .bak)\n", profName, a.cfg.Path) + // Tokens are keyed by url+clientID: once the profile is gone there is + // no target left to `logout` with, so drop them here. + a.dropTokens(a.store(), prof) + a.warnOtherClients(client, profName) + } else { + fmt.Fprintf(a.msg, "no profile @%s in %s\n", profName, a.cfg.Path) + } + if !entryGone && !hadProfile { + return fmt.Errorf("nothing to remove: no entry %q in %s and no profile @%s in %s", + name, path, profName, a.cfg.Path) + } return nil } -// ensureProfile persists a profile for a bare-URL install by APPENDING a TOML -// block — the existing file (comments included) is never rewritten. A name -// clash with a different URL is an error, not an overwrite. +// warnOtherClients reports entries in the other clients' configs that still +// reference the profile just removed — they would break silently otherwise. +func (a *App) warnOtherClients(skip, profName string) { + for _, client := range knownClients { + if client == skip { + continue + } + path, err := clientConfigPath(client) + if err != nil { + continue + } + root, original, err := loadClientConfig(path) + if err != nil || original == nil { + continue + } + servers, _ := root["mcpServers"].(map[string]any) + for _, entryName := range slices.Sorted(maps.Keys(servers)) { + if entryProfile(servers[entryName]) == profName { + a.warnf("%s still has entry %q pointing at @%s (%s)", client, entryName, profName, path) + } + } + } +} + +// entryProfile extracts the "@profile" argument from a client config entry as +// loaded from JSON (map form — the typed serverEntry only goes the other way). +func entryProfile(entry any) string { + e, _ := entry.(map[string]any) + args, _ := e["args"].([]any) + for _, arg := range args { + if s, ok := arg.(string); ok && strings.HasPrefix(s, "@") { + return strings.TrimPrefix(s, "@") + } + } + return "" +} + +// entryCommand reports the binary a client config entry launches. +func entryCommand(entry any) string { + e, _ := entry.(map[string]any) + cmd, _ := e["command"].(string) + return cmd +} + +// ownBinary reports whether a command path launches mcpurl itself (install +// writes an absolute path; Windows adds the .exe suffix). +func ownBinary(cmd string) bool { + return strings.TrimSuffix(filepath.Base(cmd), ".exe") == "mcpurl" +} + +// ensureProfile persists the profile for a bare-URL install. A new profile is +// APPENDED, so the existing file (comments included) is untouched; an existing +// one is updated in place — re-running install with a new URL or new flags is +// how you change a profile, not an error. func (a *App) ensureProfile(w io.Writer, name string, dryRun bool) error { - if p, ok := a.cfg.Profiles[name]; ok { - if p.URL == a.prof.URL { + desired := a.prof + existing, exists := a.cfg.Profiles[name] + if exists { + if desired = mergeProfile(existing, a.prof); sameProfile(existing, desired) { fmt.Fprintf(a.msg, "profile @%s already exists, reusing\n", name) return nil } - return fmt.Errorf("profile @%s already points at %s — pass a different --name", name, p.URL) } + desired.Name = name - block := profileTOML(name, a.prof) + block := profileTOML(name, desired) if dryRun { - fmt.Fprintf(w, "# would append to %s:\n%s", a.cfg.Path, block) + verb := "append to" + if exists { + verb = "update in" + } + fmt.Fprintf(w, "# would %s %s:\n%s", verb, a.cfg.Path, block) return nil } - if err := os.MkdirAll(filepath.Dir(a.cfg.Path), 0o700); err != nil { + if exists { + if err := a.editProfileBlock(name, block); err != nil { + return err + } + fmt.Fprintf(a.msg, "profile @%s updated in %s (backup: .bak)\n", name, a.cfg.Path) + } else { + if err := appendProfile(a.cfg.Path, block); err != nil { + return err + } + fmt.Fprintf(a.msg, "profile @%s saved to %s\n", name, a.cfg.Path) + } + a.cfg.Profiles[name] = desired + return nil +} + +// mergeProfile overlays a fresh install onto the stored profile: the URL is +// whatever install was pointed at, the rest only when the flag was given — +// unmentioned settings (headers above all) must survive the rewrite. +func mergeProfile(old, fresh Profile) Profile { + out := old + out.URL = fresh.URL + if fresh.ClientID != "" { + out.ClientID = fresh.ClientID + } + if len(fresh.Scopes) > 0 { + out.Scopes = fresh.Scopes + } + if fresh.CallbackPort != 0 { + out.CallbackPort = fresh.CallbackPort + } + if fresh.Issuer != "" { + out.Issuer = fresh.Issuer + } + if fresh.BearerEnv != "" { + out.BearerEnv = fresh.BearerEnv + } + return out +} + +// sameProfile compares the fields profileTOML writes (headers are carried over +// verbatim by mergeProfile, so they can never differ here). +func sameProfile(a, b Profile) bool { + return a.URL == b.URL && a.ClientID == b.ClientID && a.CallbackPort == b.CallbackPort && + a.Issuer == b.Issuer && a.BearerEnv == b.BearerEnv && slices.Equal(a.Scopes, b.Scopes) +} + +func appendProfile(path, block string) error { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return err } - f, err := os.OpenFile(a.cfg.Path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) if err != nil { return err } defer f.Close() - if _, err := f.WriteString(block); err != nil { + _, err = f.WriteString(block) + return err +} + +// editProfileBlock rewrites config.toml with the profile's block replaced by +// block ("" deletes it), keeping a .bak of the previous content. +func (a *App) editProfileBlock(name, block string) error { + original, err := os.ReadFile(a.cfg.Path) + if err != nil { return err } - fmt.Fprintf(a.msg, "profile @%s saved to %s\n", name, a.cfg.Path) - return nil + out, ok := spliceProfileBlock(string(original), name, block) + if !ok { + return fmt.Errorf("profile @%s is not a [profiles.%s] section in %s — edit the file by hand", + name, name, a.cfg.Path) + } + return replaceConfig(a.cfg.Path, original, []byte(out)) } -// profileTOML renders the appended block: only what discovery can't provide. +// replaceConfig is how mcpurl rewrites a user's config file: keep a .bak of +// what was there, then swap in the new content atomically. Both config files +// (config.toml, the client JSON) go through here, so the backup convention +// lives in one place. +func replaceConfig(path string, original, data []byte) error { + if original != nil { + if err := os.WriteFile(path+".bak", original, 0o600); err != nil { + return fmt.Errorf("writing backup: %w", err) + } + } + return atomicfile.Write(path, data, atomicfile.Mode(path, 0o600)) +} + +// profileTOML renders the block: only what discovery can't provide, plus any +// headers already stored (a rewrite must not lose them). func profileTOML(name string, p Profile) string { var b strings.Builder fmt.Fprintf(&b, "\n[profiles.%s]\nurl = %q\n", name, p.URL) @@ -148,6 +335,12 @@ func profileTOML(name string, p Profile) string { if p.BearerEnv != "" { fmt.Fprintf(&b, "bearerEnv = %q\n", p.BearerEnv) } + if len(p.Headers) > 0 { + fmt.Fprintf(&b, "\n [profiles.%s.headers]\n", name) + for _, k := range slices.Sorted(maps.Keys(p.Headers)) { + fmt.Fprintf(&b, " %q = %q\n", k, p.Headers[k]) + } + } return b.String() } @@ -183,15 +376,7 @@ func writeClientConfig(path string, root map[string]any, original []byte) error return err } out = append(out, '\n') - if original != nil { - if err := os.WriteFile(path+".bak", original, 0o600); err != nil { - return fmt.Errorf("writing backup: %w", err) - } - } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return err - } - return os.WriteFile(path, out, 0o600) + return replaceConfig(path, original, out) } // serverEntry is the client-config record: absolute binary path + profile ref. @@ -225,18 +410,23 @@ func upsertServerEntry(path, name string, entry any, dryRun bool, w io.Writer) e return writeClientConfig(path, root, original) } -func removeServerEntry(path, name string) error { +// removeServerEntry deletes mcpServers. and returns what it deleted (so +// the caller can see which profile the entry referenced). A missing file or a +// missing entry is reported as ok == false, not as an error: uninstall keeps +// going and cleans up the profile side. +func removeServerEntry(path, name string) (entry any, ok bool, err error) { root, original, err := loadClientConfig(path) if err != nil { - return err + return nil, false, err } if original == nil { - return fmt.Errorf("%s does not exist", path) + return nil, false, nil } servers, _ := root["mcpServers"].(map[string]any) - if _, ok := servers[name]; !ok { - return fmt.Errorf("no entry %q in %s", name, path) + entry, ok = servers[name] + if !ok { + return nil, false, nil } delete(servers, name) - return writeClientConfig(path, root, original) + return entry, true, writeClientConfig(path, root, original) } diff --git a/pkg/app/install_test.go b/pkg/app/install_test.go index f52404c..d292b47 100644 --- a/pkg/app/install_test.go +++ b/pkg/app/install_test.go @@ -13,7 +13,7 @@ import ( ) func TestClientConfigPath(t *testing.T) { - for _, client := range []string{"claude-desktop", "cursor", "windsurf"} { + for _, client := range knownClients { p, err := clientConfigPath(client) require.NoError(t, err, client) assert.True(t, filepath.IsAbs(p), client) @@ -51,7 +51,10 @@ func TestUpsertAndRemoveServerEntry(t *testing.T) { assert.Contains(t, servers, "pencil", "existing server intact") assert.Equal(t, entry, servers["acme"]) - require.NoError(t, removeServerEntry(path, "acme")) + removed, ok, err := removeServerEntry(path, "acme") + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "acme", entryProfile(removed), "removed entry reveals its profile") data, err = os.ReadFile(path) require.NoError(t, err) require.NoError(t, json.Unmarshal(data, &root)) @@ -59,7 +62,9 @@ func TestUpsertAndRemoveServerEntry(t *testing.T) { assert.NotContains(t, servers, "acme") assert.Contains(t, servers, "pencil") - require.Error(t, removeServerEntry(path, "acme"), "double uninstall must fail") + _, ok, err = removeServerEntry(path, "acme") + require.NoError(t, err, "a second uninstall is not an error, just a no-op") + assert.False(t, ok) } func TestUpsertCreatesMissingFile(t *testing.T) { @@ -109,17 +114,157 @@ func TestEnsureProfileAppendPreservesFile(t *testing.T) { assert.Equal(t, "newsrv-cli", p.ClientID) assert.Equal(t, []string{"openid", "roles"}, p.Scopes) - // Same name, different URL → refuse. - a2, err := New(Options{Target: "https://other.example/mcp", ClientID: "x"}, io.Discard) + // Same name, different URL → update in place, keeping flags not repeated. + a2, err := New(Options{Target: "https://other.example/mcp"}, io.Discard) + require.NoError(t, err) + require.NoError(t, a2.ensureProfile(io.Discard, "newsrv", false)) + + cfg, err = Load("") + require.NoError(t, err) + p = cfg.Profiles["newsrv"] + assert.Equal(t, "https://other.example/mcp", p.URL, "install must replace the URL") + assert.Equal(t, "newsrv-cli", p.ClientID, "settings not passed again must survive") + assert.Equal(t, []string{"openid", "roles"}, p.Scopes) + assert.Len(t, cfg.Profiles, 2, "no duplicate [profiles.newsrv] block") + raw, err = os.ReadFile(a.cfg.Path) require.NoError(t, err) - require.ErrorContains(t, a2.ensureProfile(io.Discard, "newsrv", false), "different --name") + assert.Contains(t, string(raw), "[profiles.acme]", "other profiles untouched") // Same name, same URL → reuse silently. - a3, err := New(Options{Target: "https://mcp.newsrv.example/mcp"}, io.Discard) + a3, err := New(Options{Target: "https://other.example/mcp"}, io.Discard) require.NoError(t, err) require.NoError(t, a3.ensureProfile(io.Discard, "newsrv", false)) } +// A rewrite must not drop a nested [profiles..headers] table: the user +// hand-wrote it, install never sees it as a flag. +func TestEnsureProfileUpdateKeepsHeaders(t *testing.T) { + writeConfig(t) // profile "acme" with an X-Team header + a, err := New(Options{Target: "https://acme.example/v2/mcp"}, io.Discard) + require.NoError(t, err) + require.NoError(t, a.ensureProfile(io.Discard, "acme", false)) + + cfg, err := Load("") + require.NoError(t, err) + p := cfg.Profiles["acme"] + assert.Equal(t, "https://acme.example/v2/mcp", p.URL) + assert.Equal(t, "acme-cli", p.ClientID) + assert.Equal(t, 18075, p.CallbackPort) + assert.Equal(t, map[string]string{"X-Team": "team-$MCPURL_TEST_SUFFIX"}, p.Headers, + "hand-written headers must survive the rewrite, unexpanded") + + bak, err := os.ReadFile(a.cfg.Path + ".bak") + require.NoError(t, err) + assert.Contains(t, string(bak), "https://mcp.acme.example/mcp", ".bak holds the pre-edit config") +} + +// uninstall is destructive and takes a bare name: a typo that hits a foreign +// server entry must be called out, with the way back. +func TestUninstallWarnsOnForeignEntry(t *testing.T) { + writeConfig(t) + t.Setenv("HOME", t.TempDir()) + path, err := clientConfigPath("claude-desktop") + require.NoError(t, err) + require.NoError(t, upsertServerEntry(path, "pencil", + map[string]any{"command": "/usr/local/bin/pencil-mcp", "args": []any{"--x"}}, false, io.Discard)) + + var msg strings.Builder + a, err := New(Options{}, &msg) + require.NoError(t, err) + require.NoError(t, a.Uninstall("claude-desktop", "pencil")) + assert.Contains(t, msg.String(), "was not an mcpurl entry") + assert.Contains(t, msg.String(), ".bak") + + cfg, err := Load("") + require.NoError(t, err) + assert.Contains(t, cfg.Profiles, "acme", "an unrelated profile must not be touched") +} + +// Uninstall is the full undo: entry + profile, with or without the "@". +func TestUninstallRemovesEntryAndProfile(t *testing.T) { + writeConfig(t) + home := t.TempDir() + t.Setenv("HOME", home) + a, err := New(Options{Target: "@acme"}, io.Discard) + require.NoError(t, err) + require.NoError(t, a.Install(io.Discard, "claude-desktop", "", false)) + + // A fresh App: uninstall runs in its own process. + a2, err := New(Options{}, io.Discard) + require.NoError(t, err) + require.NoError(t, a2.Uninstall("claude-desktop", "@acme"), "@ prefix must be accepted") + + clientPath, err := clientConfigPath("claude-desktop") + require.NoError(t, err) + data, err := os.ReadFile(clientPath) + require.NoError(t, err) + var root map[string]any + require.NoError(t, json.Unmarshal(data, &root)) + assert.NotContains(t, root["mcpServers"].(map[string]any), "acme") + + cfg, err := Load("") + require.NoError(t, err) + assert.NotContains(t, cfg.Profiles, "acme", "the profile must be gone too") +} + +// The reported case: the entry was already removed, the profile lingered. +func TestUninstallCleansLeftoverProfile(t *testing.T) { + writeConfig(t) + t.Setenv("HOME", t.TempDir()) // no client config at all + a, err := New(Options{}, io.Discard) + require.NoError(t, err) + require.NoError(t, a.Uninstall("claude-desktop", "acme")) + + cfg, err := Load("") + require.NoError(t, err) + assert.NotContains(t, cfg.Profiles, "acme") + + // Nothing left anywhere → error, so a typo is not reported as success. + require.ErrorContains(t, a.Uninstall("claude-desktop", "acme"), "nothing to remove") +} + +// A bare profile name is a profile, not a URL: install must reference the +// existing one instead of minting a duplicate from the host label. +func TestInstallBareNameReusesProfile(t *testing.T) { + writeConfig(t) + t.Setenv("HOME", t.TempDir()) + a, err := New(Options{Target: "acme"}, io.Discard) + require.NoError(t, err) + require.NoError(t, a.Install(io.Discard, "claude-desktop", "", false)) + + cfg, err := Load("") + require.NoError(t, err) + assert.Len(t, cfg.Profiles, 1, "no second profile from hostLabel()") + assert.Contains(t, cfg.Profiles, "acme") + + path, err := clientConfigPath("claude-desktop") + require.NoError(t, err) + data, err := os.ReadFile(path) + require.NoError(t, err) + var root map[string]any + require.NoError(t, json.Unmarshal(data, &root)) + entry := root["mcpServers"].(map[string]any)["acme"] + assert.Equal(t, "acme", entryProfile(entry), "the entry must point at @acme") +} + +// install --name X @profile: the entry name differs from the profile name, so +// uninstall must follow the entry's args to find the profile to drop. +func TestUninstallFollowsEntryToProfile(t *testing.T) { + writeConfig(t) + t.Setenv("HOME", t.TempDir()) + a, err := New(Options{Target: "@acme"}, io.Discard) + require.NoError(t, err) + require.NoError(t, a.Install(io.Discard, "cursor", "acme-prod", false)) + + a2, err := New(Options{}, io.Discard) + require.NoError(t, err) + require.NoError(t, a2.Uninstall("cursor", "acme-prod")) + + cfg, err := Load("") + require.NoError(t, err) + assert.NotContains(t, cfg.Profiles, "acme", "profile referenced by the entry must go") +} + func TestInstallDryRunWritesNothing(t *testing.T) { writeConfig(t) t.Setenv("HOME", t.TempDir()) // clientConfigPath must never see the real home diff --git a/pkg/app/profile.go b/pkg/app/profile.go index 342243a..b9c0675 100644 --- a/pkg/app/profile.go +++ b/pkg/app/profile.go @@ -5,6 +5,7 @@ package app import ( + "errors" "fmt" "maps" "net/url" @@ -70,34 +71,46 @@ func Load(path string) (*Config, error) { return cfg, nil } -// Get looks up a profile by name (with or without the "@" prefix). +// Get looks up a stored profile by name (with or without the "@" prefix). +// Unlike Resolve it never accepts a URL — the caller means a profile. func (c *Config) Get(name string) (Profile, error) { name = strings.TrimPrefix(name, "@") p, ok := c.Profiles[name] if !ok { - return Profile{}, fmt.Errorf("profile %q not found in %s (known: %s)", + return Profile{}, fmt.Errorf("%q is not a profile in %s (known: %s)", name, c.Path, strings.Join(c.Names(), ", ")) } return p, nil } -// Resolve maps a CLI target ("@name" or a bare URL) to a Profile. +// Resolve maps a CLI target to a Profile. The target is an http(s) URL, or a +// profile name written either way: the "@" is a readability marker, never a +// requirement — every command accepts both spellings. func (c *Config) Resolve(target string) (Profile, error) { - if strings.HasPrefix(target, "@") { - p, err := c.Get(target) - if err != nil { - return Profile{}, err - } - if p.URL == "" { - return Profile{}, fmt.Errorf("profile %q has no url", p.Name) - } - return p, nil + if isHTTPURL(target) { + return Profile{Name: "", URL: target}, nil } - u, err := url.Parse(target) - if err != nil || u.Host == "" || (u.Scheme != "http" && u.Scheme != "https") { - return Profile{}, fmt.Errorf("target %q is neither @profile nor an http(s) URL", target) + if strings.TrimPrefix(target, "@") == "" { + return Profile{}, errors.New("empty target: want a profile name or an http(s) URL") + } + p, err := c.Get(target) + if err != nil { + // Name both readings: a mistyped URL lands here too, and the profile + // lookup alone would send the user looking in the wrong file. + return Profile{}, fmt.Errorf("%w and not an http(s) URL", err) } - return Profile{Name: "", URL: target}, nil + if p.URL == "" { + return Profile{}, fmt.Errorf("profile %q has no url", p.Name) + } + return p, nil +} + +// isHTTPURL reports whether target is a usable http(s) endpoint rather than a +// profile name. A scheme is required: "example.com/mcp" is ambiguous, and +// treating it as a profile name yields the message that lists both readings. +func isHTTPURL(target string) bool { + u, err := url.Parse(target) + return err == nil && u.Host != "" && (u.Scheme == "http" || u.Scheme == "https") } // Names returns profile names, sorted. diff --git a/pkg/app/profile_test.go b/pkg/app/profile_test.go index 9f0a40f..71698de 100644 --- a/pkg/app/profile_test.go +++ b/pkg/app/profile_test.go @@ -51,6 +51,26 @@ func TestLoadAndResolve(t *testing.T) { require.Error(t, err, "garbage target") } +// The "@" is a readability marker, not a requirement: every command takes a +// bare profile name too (that asymmetry is what made `uninstall @x` fail). +func TestResolveAcceptsBareName(t *testing.T) { + cfg, err := Load(writeSample(t)) + require.NoError(t, err) + + bare, err := cfg.Resolve("acme") + require.NoError(t, err) + at, err := cfg.Resolve("@acme") + require.NoError(t, err) + assert.Equal(t, at, bare, "both spellings must resolve identically") + + // A mistyped URL lands in the same branch, so the message must name both + // readings instead of just "profile not found". + _, err = cfg.Resolve("mcp.example.com/mcp") + require.Error(t, err) + assert.Contains(t, err.Error(), "not a profile") + assert.Contains(t, err.Error(), "http(s) URL") +} + func TestLoadMissingFile(t *testing.T) { cfg, err := Load(filepath.Join(t.TempDir(), "absent.toml")) require.NoError(t, err) diff --git a/pkg/app/tomledit.go b/pkg/app/tomledit.go new file mode 100644 index 0000000..165ea51 --- /dev/null +++ b/pkg/app/tomledit.go @@ -0,0 +1,108 @@ +// Text-level editing of config.toml: replace or drop one [profiles.] +// block, leaving every other byte of the file alone. Deliberately not a TOML +// round-trip — the file is hand-written, full of comments and deliberate +// alignment, and a marshal/unmarshal pass would silently normalize all of it +// away (02-cli.md). + +package app + +import "strings" + +// spliceProfileBlock replaces the [profiles.] section — including its +// nested tables and the blank line separating it from the block above — with +// block, or drops the section when block is empty. ok is false when no such +// section exists (an inline `profiles. = {…}`, say), which the caller +// must report rather than silently treat as a removal. +func spliceProfileBlock(src, name, block string) (out string, ok bool) { + lines := strings.SplitAfter(src, "\n") + start, end := -1, len(lines) + for i, line := range lines { + key, isTable := tableKey(line) + if !isTable { + continue + } + if profileSection(key, name) { + if start < 0 { + start = i + } + continue + } + if start >= 0 { + end = i // the next unrelated table ends the block + break + } + } + if start < 0 { + return src, false + } + for start > 0 && blankLine(lines[start-1]) { + start-- // the blank separator belongs to the block being replaced + } + // A comment sitting between two profiles documents the one below it, not + // the one being removed — give it (and its separating blank line) back. + for end > start && (blankLine(lines[end-1]) || commentLine(lines[end-1])) { + end-- + } + for start == 0 && block == "" && end < len(lines) && blankLine(lines[end]) { + end++ // nothing above to separate from: don't leave the file opening blank + } + out = strings.Join(lines[:start], "") + block + strings.Join(lines[end:], "") + if out != "" && !strings.HasSuffix(out, "\n") { + out += "\n" + } + return out, true +} + +func blankLine(line string) bool { return strings.TrimSpace(line) == "" } + +func commentLine(line string) bool { return strings.HasPrefix(strings.TrimSpace(line), "#") } + +// tableKey parses a TOML table header line: ` [profiles.acme] # note` → +// "profiles.acme". Array-of-tables headers ("[[x]]") count as headers too — +// they end the preceding section — but never match a profile. +func tableKey(line string) (string, bool) { + s := strings.TrimSpace(line) + if !strings.HasPrefix(s, "[") { + return "", false + } + inner, _, ok := strings.Cut(s[1:], "]") + if !ok { + return "", false + } + return strings.TrimSpace(inner), true +} + +// profileSection reports whether a table key addresses [profiles.] or a +// table nested under it (e.g. [profiles..headers]). +func profileSection(key, name string) bool { + parts := splitTOMLKey(key) + return len(parts) >= 2 && parts[0] == "profiles" && parts[1] == name +} + +// splitTOMLKey splits a dotted key, honouring quoted parts: `profiles."a.b"` +// → ["profiles", "a.b"]. +func splitTOMLKey(key string) []string { + var ( + parts []string + cur strings.Builder + quote rune + ) + for _, r := range key { + switch { + case quote != 0: + if r == quote { + quote = 0 + continue + } + cur.WriteRune(r) + case r == '"' || r == '\'': + quote = r + case r == '.': + parts = append(parts, strings.TrimSpace(cur.String())) + cur.Reset() + default: + cur.WriteRune(r) + } + } + return append(parts, strings.TrimSpace(cur.String())) +} diff --git a/pkg/app/tomledit_test.go b/pkg/app/tomledit_test.go new file mode 100644 index 0000000..76a12ad --- /dev/null +++ b/pkg/app/tomledit_test.go @@ -0,0 +1,79 @@ +package app + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSpliceProfileBlock(t *testing.T) { + const src = `# header comment +[profiles.keep] +url = "https://keep.example/mcp" + +[profiles.gone] +url = "https://gone.example/mcp" + + [profiles.gone.headers] + "X" = "y" + +[profiles.tail] +url = "https://tail.example/mcp" +` + out, ok := spliceProfileBlock(src, "gone", "") + require.True(t, ok) + assert.NotContains(t, out, "gone.example") + assert.NotContains(t, out, "[profiles.gone.headers]", "nested tables go with the block") + assert.Contains(t, out, "# header comment") + assert.Contains(t, out, "[profiles.keep]") + assert.Contains(t, out, "[profiles.tail]") + + // Replacement lands where the old block was, not at the end of the file. + out, ok = spliceProfileBlock(src, "gone", "\n[profiles.gone]\nurl = \"https://new.example/mcp\"\n") + require.True(t, ok) + assert.Less(t, strings.Index(out, "new.example"), strings.Index(out, "[profiles.tail]")) + + // Last block in the file, and an unknown name. + out, ok = spliceProfileBlock(src, "tail", "") + require.True(t, ok) + assert.NotContains(t, out, "tail.example") + assert.True(t, strings.HasSuffix(out, "\n")) + + _, ok = spliceProfileBlock(src, "absent", "") + assert.False(t, ok, "unknown profile must be reported, not silently 'removed'") +} + +// A comment between two profiles documents the one below it: removing the +// profile above must not take it along. +func TestSpliceProfileBlockKeepsNeighbourComments(t *testing.T) { + const src = `[profiles.gone] +url = "https://gone.example/mcp" + +# the staging server, handle with care +[profiles.tail] +url = "https://tail.example/mcp" + +# scratch notes at the end of the file +` + out, ok := spliceProfileBlock(src, "gone", "") + require.True(t, ok) + assert.NotContains(t, out, "gone.example") + assert.Contains(t, out, "# the staging server, handle with care", + "the next profile's comment must survive") + assert.Equal(t, `# the staging server, handle with care +[profiles.tail] +url = "https://tail.example/mcp" + +# scratch notes at the end of the file +`, out) + + // Same rule at the end of the file: trailing notes are not part of the + // last block. + out, ok = spliceProfileBlock(src, "tail", "") + require.True(t, ok) + assert.Contains(t, out, "# scratch notes at the end of the file") + assert.Contains(t, out, "# the staging server, handle with care", + "an orphaned comment is the user's to delete, not ours") +} diff --git a/pkg/oauth/store.go b/pkg/oauth/store.go index 5b5dfce..c82b141 100644 --- a/pkg/oauth/store.go +++ b/pkg/oauth/store.go @@ -12,6 +12,8 @@ import ( "os" "path/filepath" "time" + + "github.com/vmkteam/mcpurl/internal/atomicfile" ) // Token is the stored value. Expiry is absolute (never a relative @@ -82,28 +84,10 @@ func (s *FileStore) Save(key string, t *Token) error { return writeFileAtomic(s.Dir, key+".json", data) } -// writeFileAtomic writes data as dir/name with 0600 via tmp + rename. +// writeFileAtomic writes data as dir/name with 0600 via tmp + rename. Token +// files are ours alone, so the mode is fixed rather than preserved. func writeFileAtomic(dir, name string, data []byte) error { - if err := os.MkdirAll(dir, 0o700); err != nil { - return err - } - tmp, err := os.CreateTemp(dir, name+".tmp-*") - if err != nil { - return err - } - defer os.Remove(tmp.Name()) //nolint:errcheck // no-op after successful rename - if err := tmp.Chmod(0o600); err != nil { - tmp.Close() - return err - } - if _, err := tmp.Write(data); err != nil { - tmp.Close() - return err - } - if err := tmp.Close(); err != nil { - return err - } - return os.Rename(tmp.Name(), filepath.Join(dir, name)) + return atomicfile.Write(filepath.Join(dir, name), data, 0o600) } func (s *FileStore) Delete(key string) error { From afe68db47b80fcd7458f634e426b31d0191e3b1a Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 10 Sep 2026 18:38:57 +0300 Subject: [PATCH 2/5] Surface the 401 body instead of retrying past it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A server that refuses the credential usually says why ("malformed jwt: unexpected signature algorithm HS256"), and every one of those bytes was discarded: a response body reads exactly once, and the debug line read it first. The user saw "upstream HTTP 401", a browser window, and another 401 (docs/tasks/02-401-body-discarded.md). - Buffer a bounded prefix of every failed response before anything else reads it, and put it back, so the log line and the HTTPError a caller builds later see the same bytes - Render the server's own text in HTTPError.Error() and hand the error's text to the MCP client, which showed a bare "upstream HTTP 401" before - Parse the RFC 6750 error/error_description pair out of WWW-Authenticate and name it in the give-up message - Stop escalating to a browser login when the rejected token is one this process just minted: the authorization server issues it happily and the resource server refuses it, so a login only buys a pointless window — unless the challenge calls the token expired - Cap the 401 ladder (maxTokenCycles) and stop when the provider re-offers a token the server already rejected - Answer a failed initialize before exiting: an MCP client that only renders the error surface must still learn why the handshake failed - Log the challenge probe's own 401 body, which was dropped the same way - Redact JWT-shaped runs from server-supplied text (internal/redact): gateways quote the credential they refused, and that text now reaches a plaintext client log and the client UI --- internal/redact/redact.go | 41 ++++++ internal/redact/redact_test.go | 36 ++++++ pkg/app/app_test.go | 31 +++++ pkg/mcp/bridge.go | 24 ++-- pkg/mcp/bridge_test.go | 26 ++++ pkg/mcp/client.go | 174 +++++++++++++++++++++++--- pkg/mcp/client_test.go | 220 +++++++++++++++++++++++++++++++-- pkg/oauth/discovery.go | 56 +++++++-- pkg/oauth/discovery_test.go | 31 +++-- pkg/oauth/flow.go | 55 ++++++++- pkg/oauth/flow_test.go | 54 ++++++++ 11 files changed, 691 insertions(+), 57 deletions(-) create mode 100644 internal/redact/redact.go create mode 100644 internal/redact/redact_test.go diff --git a/internal/redact/redact.go b/internal/redact/redact.go new file mode 100644 index 0000000..41596dd --- /dev/null +++ b/internal/redact/redact.go @@ -0,0 +1,41 @@ +// Package redact strips credentials out of server-supplied text before it is +// logged or handed to an MCP client. +// +// The text a server writes is not the server's business alone: gateways quote +// the credential they just refused back at the client +// (`{"error_description":"Jwt expired: eyJhbGci…"}`), and mcpurl now copies +// failed-response bodies into `-v` output and into the JSON-RPC error the +// client renders. Without this filter an access token would land in a +// plaintext desktop-client log — the same token that sits AES-256-GCM +// encrypted in the keychain two directories away. +// +// It lives here because both pkg/mcp (response bodies, challenge headers) and +// pkg/oauth (challenge error_description) need it, and they deliberately do +// not import each other. One rule, one place to audit. +package redact + +import ( + "regexp" + "strings" +) + +// jwtRe matches a compact-serialization JWT/JWE: "eyJ" — the base64url of the +// `{"` every JOSE header starts with — plus at least one more dot-separated +// segment. The length floor keeps an ordinary word starting with "eyJ" out. +var jwtRe = regexp.MustCompile(`eyJ[\w-]{8,}(?:\.[\w-]+)+`) + +// Tokens replaces every JWT-shaped run in s with a marker, keeping the +// surrounding diagnosis intact — "Jwt expired: [redacted JWT]" still tells +// the user what went wrong. +// +// Shape is all it can match: an opaque bearer token is indistinguishable from +// an id. Treat this as the last line of defence, not a licence to print +// whatever a server sends. +func Tokens(s string) string { + // ReplaceAllString copies the whole input even with nothing to replace, + // and the overwhelming majority of server text carries no token at all. + if !strings.Contains(s, "eyJ") { + return s + } + return jwtRe.ReplaceAllString(s, "[redacted JWT]") +} diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go new file mode 100644 index 0000000..2ce4e78 --- /dev/null +++ b/internal/redact/redact_test.go @@ -0,0 +1,36 @@ +package redact + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestTokens(t *testing.T) { + const jwt = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + + tests := []struct { + name, in, want string + }{ + {"gateway echoes the credential", + `{"error_description":"Jwt expired: ` + jwt + `"}`, + `{"error_description":"Jwt expired: [redacted JWT]"}`}, + {"unsigned two-segment token", + "token=eyJhbGciOiJub25lIn0.eyJzdWIiOiJhIn0", + "token=[redacted JWT]"}, + {"signature must not survive the header", + jwt, "[redacted JWT]"}, + {"plain diagnosis is untouched", + `auth: invalid token: oidc: malformed jwt: unexpected signature algorithm "HS256"`, + `auth: invalid token: oidc: malformed jwt: unexpected signature algorithm "HS256"`}, + {"a word starting with eyJ is not a token", "eyJ.x", "eyJ.x"}, + {"empty", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Tokens(tt.in) + assert.Equal(t, tt.want, got) + assert.NotContains(t, got, jwt) + }) + } +} diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index c6632c9..5149a3a 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -1,9 +1,14 @@ package app import ( + "bytes" + "context" "io" + "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -73,6 +78,32 @@ func TestNewWithoutTarget(t *testing.T) { assert.NoError(t, a.ClaudeConfig(io.Discard, "@acme", "")) //nolint:testifylint // value check } +// A server that refuses the credential ends the run on exit code 3, with its +// own explanation both in the error the CLI prints and in the JSON-RPC error +// the MCP client shows (docs/tasks/02-401-body-discarded.md). +func TestBridgeAuthFailure(t *testing.T) { + writeConfig(t) + t.Setenv("MCPURL_TEST_BEARER", "static-key") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer error="invalid_token"`) + http.Error(w, `auth: invalid token: oidc: malformed jwt: unexpected signature algorithm "HS256"`, + http.StatusUnauthorized) + })) + defer srv.Close() + + a, err := New(Options{Target: srv.URL, BearerEnv: "MCPURL_TEST_BEARER"}, io.Discard) + require.NoError(t, err) + + const initReq = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18"}}` + var out bytes.Buffer + err = a.Bridge(context.Background(), strings.NewReader(initReq+"\n"), &out) + require.Error(t, err) + assert.True(t, IsAuthError(err), "must map to exit code 3") + assert.Contains(t, err.Error(), "malformed jwt", "the server's diagnosis must reach stderr") + assert.Contains(t, out.String(), `"code":-32603`) + assert.Contains(t, out.String(), "malformed jwt", "…and the MCP client") +} + func TestCheckScheme(t *testing.T) { tests := []struct { url string diff --git a/pkg/mcp/bridge.go b/pkg/mcp/bridge.go index 14df9a2..6f69144 100644 --- a/pkg/mcp/bridge.go +++ b/pkg/mcp/bridge.go @@ -214,12 +214,15 @@ func (b *Bridge) handleInitialize(ctx context.Context, line []byte, protocolVer b.writeOut(msg) }) if err != nil { + // Answer first, exit after: an MCP client that only renders the error + // surface must still learn why the handshake failed — including when + // auth is what failed. + b.logf("initialize failed: %v", err) + b.synthesizeError(line, err) var ae *AuthError if errors.As(err, &ae) { return err // exit 3: auth is required and unobtainable } - b.logf("initialize failed: %v", err) - b.synthesizeError(line, err) return fmt.Errorf("initialize: %w", err) } b.handshakeMu.Lock() @@ -271,11 +274,9 @@ func (b *Bridge) send(ctx context.Context, msg []byte) { } // warn, not debug: the operator must learn about upstream failures - // (rate limits, 5xx) from the client's log even without -v. + // (rate limits, 5xx) from the client's log even without -v. The upstream + // body travels inside err (HTTPError.Error), so no separate body line. b.warnf("POST failed: %v", err) - if errors.As(err, &he) && len(he.Body) > 0 { - b.logf("upstream body: %s", he.Body[:min(len(he.Body), 2048)]) - } b.synthesizeError(msg, err) } @@ -381,15 +382,14 @@ func (b *Bridge) synthesizeError(msg []byte, cause error) { if !isRequest { return } - text := "upstream error" - var he *HTTPError - if errors.As(cause, &he) { - text = fmt.Sprintf("upstream HTTP %d", he.Status) - } + // The error's own text is the message: HTTPError/AuthError already render + // the status plus whatever the server said on one line, and a transport + // or stream failure says more than a constant would. This is the only + // thing an MCP client gets to show. out, err := json.Marshal(map[string]any{ "jsonrpc": "2.0", "id": meta.ID, - "error": map[string]any{"code": -32603, "message": text}, + "error": map[string]any{"code": -32603, "message": cause.Error()}, }) if err != nil { return diff --git a/pkg/mcp/bridge_test.go b/pkg/mcp/bridge_test.go index bda5ca8..e3cbc8b 100644 --- a/pkg/mcp/bridge_test.go +++ b/pkg/mcp/bridge_test.go @@ -320,9 +320,35 @@ func TestBridgeErrorSynthesis(t *testing.T) { l := h.expect(`"code":-32603`) assert.Contains(t, l, `"id":9`) assert.Contains(t, l, "upstream HTTP 500") + assert.Contains(t, l, "boom", "the upstream body belongs in the message the client shows") h.close() } +// A handshake that dies on auth must still answer the pending initialize — +// with the reason. Exiting silently leaves the MCP client staring at a +// process that vanished (02-401-body-discarded.md). +func TestBridgeInitializeAuthFailureAnswers(t *testing.T) { + srv := httptest.NewServer(always401(nil)) + defer srv.Close() + + h := newHarness(t, srv.URL, StaticToken("key")) + h.send(initReq) + l := h.expect(`"code":-32603`) + assert.Contains(t, l, `"id":1`) + assert.Contains(t, l, "upstream HTTP 401") + assert.Contains(t, l, "malformed jwt") + + h.in.Close() + select { + case err := <-h.done: + var ae *AuthError + require.ErrorAs(t, err, &ae, "must classify as auth failure (exit 3)") + assert.Contains(t, ae.Error(), "malformed jwt", "and carry the reason to stderr") + case <-time.After(5 * time.Second): + t.Fatal("Run did not exit after the failed handshake") + } +} + func TestBridgeGetStreamNotifications(t *testing.T) { f := newFakeMCP() f.getMessages = []string{`{"jsonrpc":"2.0","method":"notifications/resources/updated"}`} diff --git a/pkg/mcp/client.go b/pkg/mcp/client.go index 8a753da..3c82842 100644 --- a/pkg/mcp/client.go +++ b/pkg/mcp/client.go @@ -17,6 +17,8 @@ import ( "strings" "sync" "time" + + "github.com/vmkteam/mcpurl/internal/redact" ) // TokenProvider supplies the Authorization bearer value. @@ -42,14 +44,41 @@ func (t StaticToken) Token(_ context.Context, rejected, _ string) (string, error return string(t), nil } +// Bounds on what a failed response contributes to logs and errors: enough for +// a server's own diagnosis ("unexpected signature algorithm HS256"), small +// enough for one stderr line and a JSON-RPC message. Nothing renders more +// than bodyLineLimit, so capturing much beyond it only costs memory. +const ( + maxErrorBody = 4 << 10 // captured into the error + bodyLineLimit = 512 // rendered on a debug line or in an error + maxDrain = 64 << 10 // discarded to keep a connection reusable +) + +// maxTokenCycles caps the 401 ladder. Terminating it is the TokenProvider's +// job (refresh, escalate, then fail); this is only the backstop against a +// provider that keeps issuing tokens the server keeps refusing. +const maxTokenCycles = 4 + // HTTPError is returned by Post for non-2xx responses that the transport -// layer does not resolve itself. +// layer does not resolve itself. The body stays unexported: it holds raw wire +// bytes, and everything that renders them has to redact first — a reader +// outside this package would have no way to know that. type HTTPError struct { - Status int - Body []byte + Status int + ContentType string + body []byte } -func (e *HTTPError) Error() string { return fmt.Sprintf("upstream HTTP %d", e.Status) } +// Error appends the server's own explanation when there is one: a bare +// "upstream HTTP 401" sends the user to the server's logs for a diagnosis the +// server already wrote (docs/tasks/02-401-body-discarded.md). +func (e *HTTPError) Error() string { + msg := fmt.Sprintf("upstream HTTP %d", e.Status) + if s := bodySummary(e.ContentType, e.body); s != "" { + msg += ": " + s + } + return msg +} // AuthError wraps a TokenProvider failure — maps to exit code 3. type AuthError struct{ Err error } @@ -173,12 +202,14 @@ func (c *Client) token(ctx context.Context, rejected, challenge string) (string, } // doAuthed obtains a token, performs the request built by build, and cycles -// the TokenProvider on 401 (up to two fresh tokens — refresh, then -// interactive). Returns the first non-401 response. +// the TokenProvider on 401 until the provider stops handing out new tokens. +// How far the ladder goes — refresh, interactive login, give up — is the +// provider's decision, not a fixed attempt count. Returns the first non-401 +// response, or the last 401 with its body intact. func (c *Client) doAuthed(ctx context.Context, build func(token string) (*http.Request, error)) (*http.Response, error) { token, err := c.token(ctx, "", "") if err != nil { - return nil, &AuthError{err} + return nil, &AuthError{Err: err} } for attempt := 0; ; attempt++ { req, err := build(token) @@ -189,21 +220,122 @@ func (c *Client) doAuthed(ctx context.Context, build func(token string) (*http.R if err != nil { return nil, fmt.Errorf("%s %s: %w", req.Method, c.Endpoint, err) } - c.debugf("%s → %d %s", req.Method, resp.StatusCode, resp.Header.Get("Content-Type")) - if resp.StatusCode != http.StatusUnauthorized || c.Tokens == nil || attempt >= 2 { + if resp.StatusCode/100 == 2 { + c.logResponse(req.Method, resp, nil) return resp, nil } - challenge := resp.Header.Get("WWW-Authenticate") - drain(resp) + // Buffer before anything reads: the log line here and the *HTTPError + // a caller builds later must both see the body, and a response body + // reads exactly once — that is how the 401 explanations disappeared. + body := captureBody(resp) + c.logResponse(req.Method, resp, body) + if resp.StatusCode != http.StatusUnauthorized || c.Tokens == nil || attempt >= maxTokenCycles { + return resp, nil + } + c.debugf("%s 401, cycling token (attempt %d)", req.Method, attempt+1) rejected := token if rejected == "" { rejected = "-" // anonymous rejection marker, see TokenProvider } - if token, err = c.token(ctx, rejected, challenge); err != nil { - return nil, &AuthError{err} + next, terr := c.token(ctx, rejected, resp.Header.Get("WWW-Authenticate")) + if terr != nil { + // The provider gave up. Carry the 401 along: the server's own + // diagnosis is the half of the message a user can act on. + return nil, &AuthError{fmt.Errorf("%w (%w)", terr, httpError(resp, body))} } + if next == token { + // Contract violation (a rejected token must not come back); + // replaying it would only fetch the same 401. + c.debugf("token provider re-offered the rejected token, not retrying") + return resp, nil + } + token = next + } +} + +// capturedBody is a body already buffered by captureBody. The marker keeps a +// second capture on the same response from re-reading (and re-allocating) the +// bytes the first one installed. +type capturedBody struct { + io.Reader + buf []byte +} + +func (capturedBody) Close() error { return nil } + +// captureBody buffers a bounded prefix of a failed response and puts it back +// as the body: the connection is released immediately and every later reader +// — debug log, newHTTPError, drain — sees the same bytes. The remainder is +// discarded rather than abandoned, so the connection returns to the idle pool +// instead of being torn down right before the retry that follows a 401. +func captureBody(resp *http.Response) []byte { + if c, ok := resp.Body.(capturedBody); ok { + return c.buf } + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBody)) + io.Copy(io.Discard, io.LimitReader(resp.Body, maxDrain)) //nolint:errcheck // keep-alive hygiene + resp.Body.Close() //nolint:errcheck // read side only + resp.Body = capturedBody{bytes.NewReader(body), body} + return body +} + +// logResponse puts everything one response says on one debug line: status, +// content type, and — for a failed one, whose body the caller has captured — +// the challenge and a bounded body prefix. RFC 6750 §3 defines +// WWW-Authenticate as what the server tells an *unauthenticated* client: it +// carries no secret and must not be redacted. +func (c *Client) logResponse(method string, resp *http.Response, body []byte) { + if c.Logf == nil { + return + } + ct := resp.Header.Get("Content-Type") + line := fmt.Sprintf("%s → %d %s", method, resp.StatusCode, ct) + if challenge := resp.Header.Get("WWW-Authenticate"); challenge != "" { + // The challenge itself is public, but its error_description is free + // text a gateway may have pasted the rejected token into. + line += ", WWW-Authenticate: " + redact.Tokens(challenge) + } + if s := bodySummary(ct, body); s != "" { + line += ", body: " + s + } + c.debugf("%s", line) +} + +// bodySummary renders a failed response body for a single-line log record or +// error message: whitespace-collapsed and truncated for textual types, a +// size/type placeholder for everything else — binary has no business on +// stderr, let alone inside a JSON-RPC string. +// +// The body is the server's text, not ours: it goes through redact.Tokens +// before anyone sees it, because gateways quote the credential they refused. +// Redaction runs before truncation so a token cannot be split across the cut. +func bodySummary(contentType string, body []byte) string { + if len(body) == 0 { + return "" + } + if contentType == "" { + contentType = http.DetectContentType(body) // sloppy server, sniff it + } + if !textualType(contentType) { + return fmt.Sprintf("<%d bytes of %s>", len(body), contentType) + } + s := strings.Join(strings.Fields(string(body)), " ") // collapses newlines + s = redact.Tokens(s) + if len(s) > bodyLineLimit { + s = s[:bodyLineLimit] + "…" + } + return strings.ToValidUTF8(s, "") // a cut rune, or plain junk, is not output +} + +// textualType reports whether a Content-Type is safe to echo: text/*, JSON +// and any structured +json syntax (application/problem+json et al). +func textualType(contentType string) bool { + mt := strings.ToLower(strings.TrimSpace(contentType)) + if i := strings.IndexByte(mt, ';'); i >= 0 { + mt = strings.TrimSpace(mt[:i]) + } + return strings.HasPrefix(mt, "text/") || mt == "application/json" || strings.HasSuffix(mt, "+json") } // Post sends one client→server JSON-RPC message. Every resulting @@ -274,11 +406,17 @@ func (c *Client) consumePost(resp *http.Response, deliver func([]byte)) error { } } -// newHTTPError captures up to 64 KB of body into the error and closes it. +// newHTTPError captures a bounded body into the error and closes the response. func newHTTPError(resp *http.Response) *HTTPError { - body, _ := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) - resp.Body.Close() - return &HTTPError{Status: resp.StatusCode, Body: body} + return httpError(resp, captureBody(resp)) +} + +func httpError(resp *http.Response, body []byte) *HTTPError { + return &HTTPError{ + Status: resp.StatusCode, + ContentType: resp.Header.Get("Content-Type"), + body: body, + } } // deliverCompact validates a JSON payload, compacts it if it contains @@ -412,7 +550,7 @@ func (c *Client) DeleteSession(ctx context.Context) { } func drain(resp *http.Response) { - io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)) //nolint:errcheck // keep-alive drain + io.Copy(io.Discard, io.LimitReader(resp.Body, maxDrain)) //nolint:errcheck // keep-alive drain resp.Body.Close() } diff --git a/pkg/mcp/client_test.go b/pkg/mcp/client_test.go index f08e67f..34d6a36 100644 --- a/pkg/mcp/client_test.go +++ b/pkg/mcp/client_test.go @@ -3,8 +3,11 @@ package mcp import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" + "strings" + "sync" "sync/atomic" "testing" "time" @@ -17,6 +20,53 @@ func newTestClient(url string, tp TokenProvider) *Client { return &Client{Endpoint: url, HTTP: &http.Client{}, Tokens: tp} } +// logCapture stands in for the CLI's -v hook. +type logCapture struct { + mu sync.Mutex + lines []string +} + +func (l *logCapture) logf(format string, args ...any) { + l.mu.Lock() + defer l.mu.Unlock() + l.lines = append(l.lines, fmt.Sprintf(format, args...)) +} + +func (l *logCapture) text() string { + l.mu.Lock() + defer l.mu.Unlock() + return strings.Join(l.lines, "\n") +} + +func (l *logCapture) count(substr string) int { + l.mu.Lock() + defer l.mu.Unlock() + n := 0 + for _, line := range l.lines { + if strings.Contains(line, substr) { + n++ + } + } + return n +} + +// authFailure is the authentik-behind-a-resource-server case verbatim: the +// server explains the rejection in the body, and no token can fix it. +const authFailure = `auth: invalid token: oidc: malformed jwt: unexpected signature algorithm "HS256"; expected ["RS256"]` + +// always401 serves that failure; requests may be nil when the count is not +// what the test is about. +func always401(requests *atomic.Int32) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + if requests != nil { + requests.Add(1) + } + w.Header().Set("WWW-Authenticate", + `Bearer resource_metadata="https://example.test/.well-known/oauth-protected-resource", error="invalid_token"`) + http.Error(w, authFailure, http.StatusUnauthorized) + } +} + func TestPostResponseModes(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Query().Get("mode") { @@ -51,24 +101,29 @@ func TestPostResponseModes(t *testing.T) { } } -// cyclingProvider escalates old → fresh → interactive, like oauth.Flow will. -type cyclingProvider struct{ calls atomic.Int32 } +// cyclingProvider escalates stored → refreshed → give up, like oauth.Flow +// does: stored is what the "store" holds, and the refreshed token derives +// from it. +type cyclingProvider struct { + stored string + calls atomic.Int32 +} func (p *cyclingProvider) Token(_ context.Context, rejected, _ string) (string, error) { p.calls.Add(1) switch rejected { case "": - return "stale", nil - case "stale": - return "fresh", nil + return p.stored, nil + case p.stored: + return p.stored + "-refreshed", nil default: - return "", errors.New("gave up") + return "", errors.New("server rejects freshly issued tokens; giving up") } } func TestPost401Cycle(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Header.Get("Authorization") != "Bearer fresh" { + if r.Header.Get("Authorization") != "Bearer stale-refreshed" { w.Header().Set("WWW-Authenticate", `Bearer resource_metadata="https://x/.well-known/oauth-protected-resource"`) w.WriteHeader(http.StatusUnauthorized) return @@ -77,7 +132,7 @@ func TestPost401Cycle(t *testing.T) { })) defer srv.Close() - c := newTestClient(srv.URL, &cyclingProvider{}) + c := newTestClient(srv.URL, &cyclingProvider{stored: "stale"}) require.NoError(t, c.Post(context.Background(), []byte(`{}`), nil), "expected recovery via token cycle") } @@ -92,6 +147,155 @@ func TestPost401Fatal(t *testing.T) { require.Error(t, c.Post(context.Background(), []byte(`{}`), nil)) } +// The server already wrote the diagnosis; -v must show it for EVERY 401 of +// the cycle and the caller must receive it (02-401-body-discarded.md). +func TestPost401SurfacesBodyAndChallenge(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(always401(&requests)) + defer srv.Close() + + var log logCapture + c := newTestClient(srv.URL, &cyclingProvider{stored: "stale"}) + c.Logf = log.logf + + err := c.Post(context.Background(), []byte(`{}`), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "upstream HTTP 401") + assert.Contains(t, err.Error(), `unexpected signature algorithm "HS256"`, + "the server's own explanation must reach the caller") + + assert.EqualValues(t, 2, requests.Load(), "stale, fresh, then the provider gave up") + assert.Equal(t, 2, log.count(`unexpected signature algorithm "HS256"`), + "every 401 body, not just the last one") + assert.Contains(t, log.text(), `error="invalid_token"`, "WWW-Authenticate must be visible") +} + +// A provider that hands back the token the server just rejected has nothing +// left to offer: replaying it would only fetch the same 401. +func TestPost401ProviderRepeatsToken(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(always401(&requests)) + defer srv.Close() + + c := newTestClient(srv.URL, &stubbornProvider{}) + err := c.Post(context.Background(), []byte(`{}`), nil) + var he *HTTPError + require.ErrorAs(t, err, &he) + assert.Equal(t, http.StatusUnauthorized, he.Status) + assert.Contains(t, he.Error(), "malformed jwt", "the 401 body must survive the retry loop") + assert.EqualValues(t, 1, requests.Load(), "no point replaying a rejected token") +} + +// How far the ladder goes is the provider's call: 0.0.2 capped it at two +// tokens, which made oauth.Flow's own give-up rung unreachable. +func TestPost401LadderIsProviderDriven(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer tok-3" { + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + c := newTestClient(srv.URL, &countingProvider{}) + require.NoError(t, c.Post(context.Background(), []byte(`{}`), nil)) +} + +// …but a provider that never gives up must not turn a 401 into an endless +// request loop. +func TestPost401LadderBackstop(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(always401(&requests)) + defer srv.Close() + + c := newTestClient(srv.URL, &countingProvider{}) + require.Error(t, c.Post(context.Background(), []byte(`{}`), nil)) + assert.EqualValues(t, maxTokenCycles+1, requests.Load(), "ladder backstop") +} + +// A binary body is reported by size and type — stderr and a JSON-RPC string +// are no place for raw bytes. +func TestFailedBinaryBodyIsNotDumped(t *testing.T) { + blob := []byte{0x00, 0x01, 0x02, 0xff, 0xfe} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + w.WriteHeader(http.StatusInternalServerError) + w.Write(blob) + })) + defer srv.Close() + + var log logCapture + c := newTestClient(srv.URL, nil) + c.Logf = log.logf + err := c.Post(context.Background(), []byte(`{}`), nil) + require.Error(t, err) + + const placeholder = "<5 bytes of application/octet-stream>" + assert.Contains(t, log.text(), placeholder) + assert.NotContains(t, log.text(), string(blob)) + assert.Contains(t, err.Error(), placeholder) +} + +// Gateways quote the credential they refused ("Jwt expired: eyJhbGci…"). +// Copying that body into -v output and into the message the MCP client +// renders must not put an access token in a plaintext desktop log — the same +// token is AES-256-GCM encrypted at rest. +func TestEchoedTokenIsRedacted(t *testing.T) { + const jwt = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ1c2VyIn0.dBjftJeZ4CVP-mB92K27uhbUJU1p1r" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + presented := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer error="invalid_token", error_description="Jwt expired: %s"`, presented)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + fmt.Fprintf(w, `{"error":"invalid_token","error_description":"Jwt expired: %s"}`, presented) + })) + defer srv.Close() + + var log logCapture + c := newTestClient(srv.URL, &cyclingProvider{stored: jwt}) + c.Logf = log.logf + err := c.Post(context.Background(), []byte(`{}`), nil) + require.Error(t, err) + + assert.NotContains(t, log.text(), jwt, "-v leaked the token the server echoed") + assert.NotContains(t, err.Error(), jwt, "the error text leaks it to the MCP client") + assert.NotContains(t, log.text(), jwt+"-refreshed", "the refreshed token leaked too") + assert.Contains(t, log.text(), "[redacted JWT]") + assert.Contains(t, err.Error(), "Jwt expired", "the diagnosis itself must survive") +} + +// The mcp counterpart of oauth.TestNoTokenMaterialInLogs: a full 401 cycle +// must never print the bearer value it sent. +func TestNoTokenMaterialInLogs(t *testing.T) { + const secret = "s3cr3t-access-token" + var requests atomic.Int32 + srv := httptest.NewServer(always401(&requests)) + defer srv.Close() + + var log logCapture + c := newTestClient(srv.URL, &cyclingProvider{stored: secret}) + c.Logf = log.logf + require.Error(t, c.Post(context.Background(), []byte(`{}`), nil)) + + assert.NotEmpty(t, log.text(), "the cycle must have logged something") + assert.NotContains(t, log.text(), secret, "debug output leaks token material") +} + +// stubbornProvider always answers with the same token, rejected or not. +type stubbornProvider struct{} + +func (stubbornProvider) Token(context.Context, string, string) (string, error) { + return "same", nil +} + +// countingProvider issues an endless supply of distinct tokens. +type countingProvider struct{ n atomic.Int32 } + +func (p *countingProvider) Token(context.Context, string, string) (string, error) { + return fmt.Sprintf("tok-%d", p.n.Add(1)), nil +} + func TestSessionCaptureAndHeaders(t *testing.T) { var sawSession, sawVersion atomic.Value srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/oauth/discovery.go b/pkg/oauth/discovery.go index 2081130..4a6395f 100644 --- a/pkg/oauth/discovery.go +++ b/pkg/oauth/discovery.go @@ -15,6 +15,8 @@ import ( "regexp" "slices" "strings" + + "github.com/vmkteam/mcpurl/internal/redact" ) const ( @@ -98,18 +100,58 @@ func requireHTTPS(raw string) error { var challengeParamRe = regexp.MustCompile(`(\w+)="([^"]*)"`) -// parseChallenge extracts resource_metadata and scope from a +// bearerChallenge is the parsed subset of a WWW-Authenticate: Bearer header: +// discovery and scope resolution need the first two, the retry ladder and the +// operator need the RFC 6750 §3.1 error pair. +type bearerChallenge struct { + ResourceMetadata string // RFC 9728 + Scope string + Error string + ErrorDescription string +} + +// detail renders the error pair for a message ("" when the server named no +// error). The challenge is what a server tells an unauthenticated client, so +// nothing in it is secret by design — but error_description is free text, and +// a gateway that pastes the rejected token into it would otherwise put that +// token in an error the MCP client displays. +func (c bearerChallenge) detail() string { + switch { + case c.Error == "": + return "" + case c.ErrorDescription == "": + return c.Error + default: + return redact.Tokens(c.Error + ": " + c.ErrorDescription) + } +} + +// staleToken reports that the resource server called the credential expired +// rather than foreign or malformed. Expiry is the one rejection a new token +// can still fix (clock skew, an early-issued token), so it keeps the browser +// rung of the ladder available — see Flow.obtain. +func (c bearerChallenge) staleToken() bool { + return c.Error == "invalid_token" && + strings.Contains(strings.ToLower(c.ErrorDescription), "expir") +} + +// parseChallenge extracts the parameters mcpurl acts on from a // WWW-Authenticate: Bearer ... header value. -func parseChallenge(h string) (resourceMetadata, scope string) { +func parseChallenge(h string) bearerChallenge { + var c bearerChallenge for _, m := range challengeParamRe.FindAllStringSubmatch(h, -1) { switch m[1] { case "resource_metadata": - resourceMetadata = m[2] + c.ResourceMetadata = m[2] case paramScope: - scope = m[2] + c.Scope = m[2] + case "error": + c.Error = m[2] + case "error_description": + c.ErrorDescription = m[2] } } - return resourceMetadata, scope + return c } func (f *Flow) fetchJSON(ctx context.Context, rawURL string, out any) error { @@ -146,7 +188,7 @@ func wellKnown(origin *url.URL, suffix, path string) string { // Returns the authorization server issuer plus PRM scopes. func (f *Flow) discoverPRM(ctx context.Context, canonical, challenge string) (issuer string, scopes []string, err error) { endpoint, _ := url.Parse(canonical) - metaURL, _ := parseChallenge(challenge) + metaURL := parseChallenge(challenge).ResourceMetadata candidates := []string{} if metaURL != "" { @@ -280,7 +322,7 @@ func (f *Flow) resolveScopes(d *discovery, challenge string) []string { return f.Scopes // explicit override: verbatim, omissions included } supported := d.AS.ScopesSupported - if _, scope := parseChallenge(challenge); scope != "" { + if scope := parseChallenge(challenge).Scope; scope != "" { return withOffline(supported, strings.Fields(scope)) } if len(d.PRMScopes) > 0 { diff --git a/pkg/oauth/discovery_test.go b/pkg/oauth/discovery_test.go index 49e498d..0226f71 100644 --- a/pkg/oauth/discovery_test.go +++ b/pkg/oauth/discovery_test.go @@ -36,13 +36,30 @@ func TestCanonicalize(t *testing.T) { } func TestParseChallenge(t *testing.T) { - rm, scope := parseChallenge(`Bearer resource_metadata="https://x/.well-known/oauth-protected-resource", scope="openid mcp", error="invalid_token"`) - assert.Equal(t, "https://x/.well-known/oauth-protected-resource", rm) - assert.Equal(t, "openid mcp", scope) - - rm, scope = parseChallenge("") - assert.Empty(t, rm) - assert.Empty(t, scope) + c := parseChallenge(`Bearer resource_metadata="https://x/.well-known/oauth-protected-resource", scope="openid mcp", error="invalid_token", error_description="token is expired"`) + assert.Equal(t, "https://x/.well-known/oauth-protected-resource", c.ResourceMetadata) + assert.Equal(t, "openid mcp", c.Scope) + assert.Equal(t, "invalid_token", c.Error) + assert.Equal(t, "token is expired", c.ErrorDescription) + assert.Equal(t, "invalid_token: token is expired", c.detail()) + assert.True(t, c.staleToken()) + + assert.Empty(t, parseChallenge("").detail()) + assert.Equal(t, bearerChallenge{}, parseChallenge("")) + + // The authentik case: invalid_token with no description at all — a + // rejection the client cannot fix by getting another token. + c = parseChallenge(`Bearer resource_metadata="https://x/.well-known/oauth-protected-resource", error="invalid_token"`) + assert.Equal(t, "invalid_token", c.detail()) + assert.False(t, c.staleToken()) + + // error_description is free text: a gateway pasting the refused token into + // it must not reach the error the MCP client renders (rejectionError). + const jwt = "eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ1c2VyIn0.dBjftJeZ4CVP-mB92K27uh" + c = parseChallenge(`Bearer error="invalid_token", error_description="Jwt expired: ` + jwt + `"`) + assert.NotContains(t, c.detail(), jwt) + assert.NotContains(t, rejectionError(c).Error(), jwt) + assert.Contains(t, c.detail(), "Jwt expired: [redacted JWT]") } // full chain: PRM at path-insertion well-known → Keycloak-style OIDC diff --git a/pkg/oauth/flow.go b/pkg/oauth/flow.go index 5fad4ea..8dfeb13 100644 --- a/pkg/oauth/flow.go +++ b/pkg/oauth/flow.go @@ -12,6 +12,8 @@ import ( "strings" "sync" "time" + + "github.com/vmkteam/mcpurl/internal/redact" ) const expirySkew = 60 * time.Second // proactive refresh window (04-oauth.md) @@ -152,6 +154,18 @@ func (f *Flow) obtain(ctx context.Context, key, canonical, rejected, challenge s return stored.AccessToken, nil } + ch := parseChallenge(challenge) + + // A 401 on a token this process just minted is not a stale credential: the + // authorization server issues it happily and the resource server refuses + // it (signing algorithm, audience/resource, issuer, clock). Another + // refresh returns the same kind of token, and a browser login returns the + // same kind again — the only thing an escalation buys the user here is a + // pointless login window (docs/tasks/02-401-body-discarded.md). + if f.selfIssued(rejected) && !ch.staleToken() { + return "", rejectionError(ch) + } + d, err := f.discover(ctx, challenge) if err != nil { return "", err @@ -180,9 +194,10 @@ func (f *Flow) obtain(ctx context.Context, key, canonical, rejected, challenge s } } - // Interactive login — exactly once per rejection cycle. + // Interactive login — at most one browser window per process, whatever the + // challenge claims. if rejected != "" && rejected == f.lastLoggedIn { - return "", errors.New("server rejects freshly issued tokens; giving up (check client/audience config)") + return "", rejectionError(ch) } t, err := f.loginAndSave(ctx, key, d, challenge) if err != nil { @@ -191,6 +206,25 @@ func (f *Flow) obtain(ctx context.Context, key, canonical, rejected, challenge s return t.AccessToken, nil } +// selfIssued reports that the rejected token is one this process obtained — +// out of a refresh or a login — rather than one loaded from the store. The +// anonymous marker "-" (see mcp.TokenProvider) never matches: both fields +// only ever hold real access tokens. +func (f *Flow) selfIssued(rejected string) bool { + return rejected != "" && (rejected == f.lastRefreshed || rejected == f.lastLoggedIn) +} + +// rejectionError names the disagreement instead of the symptom: the token is +// structurally valid by construction, so the two servers disagree about what +// a valid token looks like. +func rejectionError(ch bearerChallenge) error { + msg := "server rejects freshly issued tokens; giving up — the authorization server and the resource server disagree (check the token signing algorithm, the audience/resource, and the issuer)" + if d := ch.detail(); d != "" { + msg += "; server said: " + d + } + return errors.New(msg) +} + // loginAndSave runs the interactive flow (test hook aware), stamps and // persists the result — the single post-login bookkeeping site. func (f *Flow) loginAndSave(ctx context.Context, key string, d *discovery, challenge string) (*Token, error) { @@ -342,12 +376,23 @@ func (f *Flow) probeChallenge(ctx context.Context, canonical string) string { if err != nil { return "" } - drainBody(resp) - return resp.Header.Get("WWW-Authenticate") + challenge := resp.Header.Get("WWW-Authenticate") + body, _ := io.ReadAll(io.LimitReader(resp.Body, probeBodyLimit)) + resp.Body.Close() + // The probe's own 401 explains as much as the bridge's does — dropping it + // is what sends users to the server's logs for a diagnosis they were + // already handed. %q keeps it to one line and escapes anything binary; + // only the logged copy is redacted, discovery still gets the raw header. + f.debugf("challenge probe → %d, WWW-Authenticate: %q, body: %.512q", + resp.StatusCode, redact.Tokens(challenge), redact.Tokens(string(body))) + return challenge } +// probeBodyLimit doubles as the keep-alive drain bound (see drainBody). +const probeBodyLimit = 4 << 10 + // drainBody discards a bounded remainder and closes (keep-alive hygiene). func drainBody(resp *http.Response) { - io.Copy(io.Discard, io.LimitReader(resp.Body, 4<<10)) + io.Copy(io.Discard, io.LimitReader(resp.Body, probeBodyLimit)) resp.Body.Close() } diff --git a/pkg/oauth/flow_test.go b/pkg/oauth/flow_test.go index b864647..3018303 100644 --- a/pkg/oauth/flow_test.go +++ b/pkg/oauth/flow_test.go @@ -174,6 +174,60 @@ func TestBothExpiredSingleLogin(t *testing.T) { assert.EqualValues(t, 1, logins.Load(), "login loop") } +// A 401 on a token this process just refreshed is the server refusing a +// credential that is valid by construction. A browser login produces the same +// kind of token — it can only cost the user a window +// (docs/tasks/02-401-body-discarded.md). +func TestNoBrowserWhenFreshTokenRejected(t *testing.T) { + const challenge = `Bearer resource_metadata="https://x/.well-known/oauth-protected-resource", error="invalid_token"` + as := newFakeAS(t) + f := newTestFlow(t, as, t.TempDir()) + seedToken(t, f) + + var logins atomic.Int32 + f.loginFn = func(context.Context, *discovery, []string) (*Token, error) { + logins.Add(1) + return &Token{AccessToken: "login-access", Expiry: time.Now().Add(time.Hour)}, nil + } + + refreshed, err := f.Token(context.Background(), "", challenge) + require.NoError(t, err) + assert.Equal(t, "access-1", refreshed, "the stale stored token must be refreshed first") + + _, err = f.Token(context.Background(), refreshed, challenge) + require.Error(t, err, "a rejected fresh token must end the ladder") + assert.Contains(t, err.Error(), "signing algorithm", "the message must be actionable") + assert.Contains(t, err.Error(), "invalid_token", "and quote what the server said") + assert.Zero(t, logins.Load(), "a browser cannot help here") +} + +// The exception: a server calling the token *expired* describes something a +// new token still fixes (clock skew), so that rung stays open. +func TestExpiryChallengeStillEscalates(t *testing.T) { + const challenge = `Bearer error="invalid_token", error_description="The access token expired"` + as := newFakeAS(t) + f := newTestFlow(t, as, t.TempDir()) + seedToken(t, f) + + var logins atomic.Int32 + f.loginFn = func(context.Context, *discovery, []string) (*Token, error) { + logins.Add(1) + return &Token{AccessToken: "login-access", Expiry: time.Now().Add(time.Hour)}, nil + } + + refreshed, err := f.Token(context.Background(), "", challenge) + require.NoError(t, err) + tok, err := f.Token(context.Background(), refreshed, challenge) + require.NoError(t, err) + assert.Equal(t, "login-access", tok) + assert.EqualValues(t, 1, logins.Load()) + + // Still exactly one window: the login token being rejected too ends it. + _, err = f.Token(context.Background(), "login-access", challenge) + require.Error(t, err) + assert.EqualValues(t, 1, logins.Load(), "login loop") +} + // An AS that issues no refresh token condemns the user to a browser login per // restart. That must be said out loud at login, not discovered days later. func TestLoginWarnsWhenNoRefreshToken(t *testing.T) { From 7aaeef7bc40e56c19801716d602d4c831ecba914 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 10 Sep 2026 20:06:12 +0300 Subject: [PATCH 3/5] Move redact and atomicfile from internal to pkg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move internal/redact and internal/atomicfile under pkg/: both hold a rule shared by packages that deliberately do not import each other (redact by mcp and oauth, atomicfile by app and oauth), and the project keeps its packages under pkg/ - Fold Mode into atomicfile.Write, which now applies the mode to a new file only and leaves an existing file's own alone — the os.WriteFile convention, and one exported function instead of two --- internal/atomicfile/atomicfile.go | 60 ------------------- pkg/app/install.go | 4 +- pkg/atomicfile/atomicfile.go | 50 ++++++++++++++++ .../atomicfile/atomicfile_test.go | 39 ++++++------ pkg/mcp/client.go | 2 +- pkg/oauth/discovery.go | 2 +- pkg/oauth/flow.go | 2 +- pkg/oauth/store.go | 2 +- {internal => pkg}/redact/redact.go | 0 {internal => pkg}/redact/redact_test.go | 0 10 files changed, 73 insertions(+), 88 deletions(-) delete mode 100644 internal/atomicfile/atomicfile.go create mode 100644 pkg/atomicfile/atomicfile.go rename {internal => pkg}/atomicfile/atomicfile_test.go (68%) rename {internal => pkg}/redact/redact.go (100%) rename {internal => pkg}/redact/redact_test.go (100%) diff --git a/internal/atomicfile/atomicfile.go b/internal/atomicfile/atomicfile.go deleted file mode 100644 index 72ee706..0000000 --- a/internal/atomicfile/atomicfile.go +++ /dev/null @@ -1,60 +0,0 @@ -// Package atomicfile writes a file so that readers see either the previous -// content or the new one, never a half-written mix. -// -// mcpurl replaces three kinds of file this way — the token store, the profile -// config and an MCP client's JSON — and all three are read by other processes -// (a second bridge instance, Claude Desktop) while mcpurl runs. A plain -// os.WriteFile truncates first, so an interrupted write leaves a config the -// client refuses to parse. One implementation, one place to audit the -// tmp-cleanup and permission handling. -package atomicfile - -import ( - "os" - "path/filepath" -) - -// Write replaces path with data via a temp file in the same directory plus a -// rename. Missing parent directories are created 0700; the temp file gets -// mode before it is renamed, so the file is never briefly world-readable. -// -// A symlink is followed, not replaced: config files under a dotfiles -// repository are routinely symlinked into ~/.config, and renaming over the -// link would detach it — mcpurl would keep writing a file the user no longer -// edits, in silence. -func Write(path string, data []byte, mode os.FileMode) error { - if target, err := filepath.EvalSymlinks(path); err == nil { - path = target - } - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o700); err != nil { - return err - } - tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") - if err != nil { - return err - } - defer os.Remove(tmp.Name()) //nolint:errcheck // no-op after a successful rename - if err := tmp.Chmod(mode); err != nil { - tmp.Close() //nolint:errcheck // the write already failed - return err - } - if _, err := tmp.Write(data); err != nil { - tmp.Close() //nolint:errcheck // the write already failed - return err - } - if err := tmp.Close(); err != nil { - return err - } - return os.Rename(tmp.Name(), path) -} - -// Mode reports the mode Write should use to replace path: the existing file's, -// or fallback for a new one. Configs belong to the user — replacing one must -// not quietly tighten or loosen it. -func Mode(path string, fallback os.FileMode) os.FileMode { - if fi, err := os.Stat(path); err == nil { - return fi.Mode().Perm() - } - return fallback -} diff --git a/pkg/app/install.go b/pkg/app/install.go index 79d0826..eaeebb1 100644 --- a/pkg/app/install.go +++ b/pkg/app/install.go @@ -18,7 +18,7 @@ import ( "slices" "strings" - "github.com/vmkteam/mcpurl/internal/atomicfile" + "github.com/vmkteam/mcpurl/pkg/atomicfile" ) // knownClients is the single list of MCP clients install/uninstall know how @@ -308,7 +308,7 @@ func replaceConfig(path string, original, data []byte) error { return fmt.Errorf("writing backup: %w", err) } } - return atomicfile.Write(path, data, atomicfile.Mode(path, 0o600)) + return atomicfile.Write(path, data, 0o600) } // profileTOML renders the block: only what discovery can't provide, plus any diff --git a/pkg/atomicfile/atomicfile.go b/pkg/atomicfile/atomicfile.go new file mode 100644 index 0000000..9c251f0 --- /dev/null +++ b/pkg/atomicfile/atomicfile.go @@ -0,0 +1,50 @@ +// Package atomicfile replaces a file so readers see either the old content or +// the new one, never a half-written mix. mcpurl rewrites the token store, the +// profile config and an MCP client's JSON while other processes (a second +// bridge, Claude Desktop) read them; os.WriteFile truncates first, so an +// interrupted write leaves a config the client refuses to parse. +package atomicfile + +import ( + "os" + "path/filepath" +) + +// Write replaces path with data via a temp file in the same directory plus a +// rename, creating missing parents 0700. +// +// mode applies to a new file only — an existing one keeps its own, the way +// os.WriteFile behaves; these are the user's configs, not ours to tighten. The +// temp file is chmod'ed before the rename, so the result is never briefly +// world-readable. A symlink is followed rather than replaced: a config +// symlinked out of a dotfiles repository must stay linked, or mcpurl would go +// on writing a file the user no longer edits, in silence. +func Write(path string, data []byte, mode os.FileMode) error { + if target, err := filepath.EvalSymlinks(path); err == nil { + path = target + } + if fi, err := os.Stat(path); err == nil { + mode = fi.Mode().Perm() + } + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + defer os.Remove(tmp.Name()) //nolint:errcheck // no-op after a successful rename + if err := tmp.Chmod(mode); err != nil { + tmp.Close() //nolint:errcheck // the write already failed + return err + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() //nolint:errcheck // the write already failed + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmp.Name(), path) +} diff --git a/internal/atomicfile/atomicfile_test.go b/pkg/atomicfile/atomicfile_test.go similarity index 68% rename from internal/atomicfile/atomicfile_test.go rename to pkg/atomicfile/atomicfile_test.go index 8bfab34..26afd66 100644 --- a/internal/atomicfile/atomicfile_test.go +++ b/pkg/atomicfile/atomicfile_test.go @@ -15,21 +15,31 @@ func TestWriteCreatesAndReplaces(t *testing.T) { fi, err := os.Stat(path) require.NoError(t, err) - assert.Equal(t, os.FileMode(0o600), fi.Mode().Perm()) + assert.Equal(t, os.FileMode(0o600), fi.Mode().Perm(), "a new file gets the mode asked for") - require.NoError(t, Write(path, []byte("two\n"), Mode(path, 0o600))) + require.NoError(t, Write(path, []byte("two\n"), 0o600)) data, err := os.ReadFile(path) require.NoError(t, err) assert.Equal(t, "two\n", string(data)) - // The temp file must not survive a successful write. entries, err := os.ReadDir(filepath.Dir(path)) require.NoError(t, err) - assert.Len(t, entries, 1, "no .tmp-* leftovers") + assert.Len(t, entries, 1, "no .tmp-* leftovers after a successful write") } -// Config files symlinked out of a dotfiles repository are the common case -// this must not break: rename over the link would detach it, leaving the user +// A user's config keeps the mode they gave it — mode is for creation only. +func TestWriteKeepsExistingMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "mcp.json") + require.NoError(t, os.WriteFile(path, []byte("{}"), 0o644)) + + require.NoError(t, Write(path, []byte("{}\n"), 0o600)) + fi, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), fi.Mode().Perm(), "replacing must not tighten the mode") +} + +// Config files symlinked out of a dotfiles repository are the common case this +// must not break: renaming over the link would detach it, leaving the user // editing a file mcpurl no longer reads. func TestWriteFollowsSymlink(t *testing.T) { dir := t.TempDir() @@ -40,27 +50,12 @@ func TestWriteFollowsSymlink(t *testing.T) { require.NoError(t, os.WriteFile(target, []byte("old\n"), 0o600)) require.NoError(t, os.Symlink(target, link)) - require.NoError(t, Write(link, []byte("new\n"), Mode(link, 0o600))) + require.NoError(t, Write(link, []byte("new\n"), 0o600)) fi, err := os.Lstat(link) require.NoError(t, err) assert.NotZero(t, fi.Mode()&os.ModeSymlink, "the symlink must survive the write") - data, err := os.ReadFile(target) require.NoError(t, err) assert.Equal(t, "new\n", string(data), "the write must land in the link target") } - -// A user's config keeps the mode they gave it; only a new file gets 0600. -func TestModePreservesExisting(t *testing.T) { - path := filepath.Join(t.TempDir(), "mcp.json") - require.NoError(t, os.WriteFile(path, []byte("{}"), 0o644)) - assert.Equal(t, os.FileMode(0o644), Mode(path, 0o600)) - - require.NoError(t, Write(path, []byte("{}\n"), Mode(path, 0o600))) - fi, err := os.Stat(path) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0o644), fi.Mode().Perm(), "replacing must not tighten the mode") - - assert.Equal(t, os.FileMode(0o600), Mode(filepath.Join(t.TempDir(), "absent"), 0o600)) -} diff --git a/pkg/mcp/client.go b/pkg/mcp/client.go index 3c82842..0689360 100644 --- a/pkg/mcp/client.go +++ b/pkg/mcp/client.go @@ -18,7 +18,7 @@ import ( "sync" "time" - "github.com/vmkteam/mcpurl/internal/redact" + "github.com/vmkteam/mcpurl/pkg/redact" ) // TokenProvider supplies the Authorization bearer value. diff --git a/pkg/oauth/discovery.go b/pkg/oauth/discovery.go index 4a6395f..d1a7593 100644 --- a/pkg/oauth/discovery.go +++ b/pkg/oauth/discovery.go @@ -16,7 +16,7 @@ import ( "slices" "strings" - "github.com/vmkteam/mcpurl/internal/redact" + "github.com/vmkteam/mcpurl/pkg/redact" ) const ( diff --git a/pkg/oauth/flow.go b/pkg/oauth/flow.go index 8dfeb13..53ea569 100644 --- a/pkg/oauth/flow.go +++ b/pkg/oauth/flow.go @@ -13,7 +13,7 @@ import ( "sync" "time" - "github.com/vmkteam/mcpurl/internal/redact" + "github.com/vmkteam/mcpurl/pkg/redact" ) const expirySkew = 60 * time.Second // proactive refresh window (04-oauth.md) diff --git a/pkg/oauth/store.go b/pkg/oauth/store.go index c82b141..a6609e0 100644 --- a/pkg/oauth/store.go +++ b/pkg/oauth/store.go @@ -13,7 +13,7 @@ import ( "path/filepath" "time" - "github.com/vmkteam/mcpurl/internal/atomicfile" + "github.com/vmkteam/mcpurl/pkg/atomicfile" ) // Token is the stored value. Expiry is absolute (never a relative diff --git a/internal/redact/redact.go b/pkg/redact/redact.go similarity index 100% rename from internal/redact/redact.go rename to pkg/redact/redact.go diff --git a/internal/redact/redact_test.go b/pkg/redact/redact_test.go similarity index 100% rename from internal/redact/redact_test.go rename to pkg/redact/redact_test.go From d006c02c298068b6c99de1a54e6940fd7310dad7 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 10 Sep 2026 20:08:34 +0300 Subject: [PATCH 4/5] Require Go 1.26.7 to clear stdlib advisories - Bump the go directive from 1.26.5, which CI installs verbatim via setup-go's go-version-file, so govulncheck reported five standard library vulnerabilities fixed in 1.26.6 (net/http, crypto/tls, encoding/asn1) and failed the security job - No source change: the advisories are in the toolchain, not in mcpurl, and govulncheck is clean on 1.26.7 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index d4a1013..82596fc 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/vmkteam/mcpurl -go 1.26.5 +go 1.26.7 require ( github.com/BurntSushi/toml v1.6.0 From 840029c3986892ee5dfc2047637df09ac6ebd7ae Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Thu, 10 Sep 2026 20:13:03 +0300 Subject: [PATCH 5/5] Keep the security job green on current scanners - Suppress G703 on the .bak write: the path is the config mcpurl was pointed at (MCPURL_CONFIG, or a client's fixed location), which is the same reasoning that already excludes G304 in CI. Scoped to the one line so taint analysis stays on for new code - Add .gitleaks.toml allowing JWT-shaped fixtures under _test.go for the jwt rule alone: the redaction tests must carry tokens matching the shape, and these are the public jwt.io samples --- .gitleaks.toml | 14 ++++++++++++++ pkg/app/install.go | 4 ++++ 2 files changed, 18 insertions(+) create mode 100644 .gitleaks.toml diff --git a/.gitleaks.toml b/.gitleaks.toml new file mode 100644 index 0000000..4ab43c0 --- /dev/null +++ b/.gitleaks.toml @@ -0,0 +1,14 @@ +# Default rules plus one narrow exemption. +[extend] +useDefault = true + +# mcpurl redacts JWT-shaped runs out of server-supplied text (pkg/redact), and +# the tests for that have to carry fixtures that look exactly like a JWT — a +# fixture that does not match the shape would test nothing. These are the +# public jwt.io sample tokens: header {"alg":"RS256"}, payload {"sub":"user"}, +# no secret behind them. Scoped to the jwt rule and to _test.go so a real +# credential committed anywhere else is still caught. +[[allowlists]] +description = "JWT-shaped fixtures in tests" +targetRules = ["jwt"] +paths = ['''.*_test\.go$'''] diff --git a/pkg/app/install.go b/pkg/app/install.go index eaeebb1..2ec0087 100644 --- a/pkg/app/install.go +++ b/pkg/app/install.go @@ -304,6 +304,10 @@ func (a *App) editProfileBlock(name, block string) error { // lives in one place. func replaceConfig(path string, original, data []byte) error { if original != nil { + // #nosec G703 -- path is the config file mcpurl was pointed at: + // MCPURL_CONFIG, or a client's own fixed location. Managing that file + // (and putting the backup beside it) is the feature, which is why + // G304 is excluded in .github/workflows/ci.yml for the same reason. if err := os.WriteFile(path+".bak", original, 0o600); err != nil { return fmt.Errorf("writing backup: %w", err) }