diff --git a/Makefile b/Makefile index 2dfd8a7c5..19850f833 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ # MCPProxy Makefile -.PHONY: help build build-server build-docker build-deb swagger swagger-verify frontend-build frontend-dev backend-dev clean test test-coverage test-e2e test-e2e-oauth lint dev-setup docs-setup docs-dev docs-build docs-clean bench-discovery +.PHONY: help build build-server build-docker build-deb swagger swagger-verify frontend-build frontend-dev backend-dev clean test test-coverage test-e2e test-e2e-oauth test-descendant-pids test-e2e-cleanup-check lint dev-setup docs-setup docs-dev docs-build docs-clean bench-discovery SWAGGER_BIN ?= $(HOME)/go/bin/swag SWAGGER_OUT ?= oas @@ -20,6 +20,8 @@ help: @echo " make test-coverage - Run tests with coverage" @echo " make test-e2e - Run all E2E tests" @echo " make test-e2e-oauth - Run OAuth E2E tests with Playwright" + @echo " make test-descendant-pids - Unit test for the E2E cleanup trap's process-tree walk" + @echo " make test-e2e-cleanup-check - Integration test: E2E cleanup trap reaps only its own processes" @echo " make lint - Run linter" @echo " make dev-setup - Install development dependencies (swag, frontend, Playwright)" @echo "" @@ -176,6 +178,22 @@ test-e2e: test-e2e-oauth @echo "🧪 Running E2E tests..." ./scripts/test-api-e2e.sh +# Unit test for descendant_pids (scripts/descendant-pids.sh), the process-tree +# walk the E2E cleanup trap uses to reap only what a run itself spawned. +# Hermetic — no built binary required. +test-descendant-pids: + @echo "🧪 Running descendant_pids unit test..." + ./scripts/descendant-pids.test.sh + +# Integration test: proves the E2E cleanup trap reaps only what its own run +# started (a decoy mcpproxy on another port survives) and that it actually +# reaps a real orphan of the run (the launcher-test fixture). Requires a +# built ./mcpproxy binary; not part of `test-e2e` since it re-runs the whole +# E2E suite as a subprocess. +test-e2e-cleanup-check: + @echo "🧪 Running E2E cleanup-trap safety check..." + ./scripts/test-api-e2e-cleanup-check.sh + # Documentation site commands docs-setup: @echo "📦 Installing documentation dependencies..." diff --git a/ROADMAP.md b/ROADMAP.md index 979964fa1..20d5c0a68 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1035,3 +1035,4 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 94/113 (83%) | | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | +| [110-catalog-popularity](./specs/110-catalog-popularity/) | `in-flight` | 19/23 (83%) | diff --git a/cmd/generate-types/main.go b/cmd/generate-types/main.go index 007f9684b..6e9a7d28e 100644 --- a/cmd/generate-types/main.go +++ b/cmd/generate-types/main.go @@ -325,6 +325,25 @@ export interface IsolationDefaults { working_dir?: string; } +`) + + // Tier constants - generated from internal/contracts/tier.go + // Spec 109 FR-028/X11: one pure function (contracts.AnnotationTier) + // computes this everywhere — the Web/macOS/CLI surfaces never derive + // their own. `unknown` is returned only by the review payload composer, + // never by AnnotationTier itself. + sb.WriteString(`export const TierRead = 'read' as const; +export const TierWrite = 'write' as const; +export const TierDestructive = 'destructive' as const; +export const TierUnannotated = 'unannotated' as const; +export const TierUnknown = 'unknown' as const; +export type Tier = + | typeof TierRead + | typeof TierWrite + | typeof TierDestructive + | typeof TierUnannotated + | typeof TierUnknown; + `) // Tool types @@ -341,6 +360,9 @@ export interface IsolationDefaults { // Tool-level quarantine status surfaced by the same approval record. // Optional because non-quarantined tools simply omit the field. approval_status?: string; + // Computed by contracts.AnnotationTier (Spec 109 FR-028) — never derive + // this from annotations on the frontend (X11). + tier?: Tier; // Why the trust_mode: scan gate held this tool for review (spec 086 // FR-018). held_signals names the matched deterministic check ids, e.g. // "tpa.TPA-2026-0001.hidden_instruction". All three are absent unless the diff --git a/cmd/mcpproxy/catalog_cmd.go b/cmd/mcpproxy/catalog_cmd.go new file mode 100644 index 000000000..4d84d11db --- /dev/null +++ b/cmd/mcpproxy/catalog_cmd.go @@ -0,0 +1,412 @@ +package main + +import ( + "context" + "fmt" + "os" + "strings" + "sync" + + "github.com/spf13/cobra" + + clioutput "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" +) + +// installMemoryPopularityProviderOnce guards installMemoryPopularityProvider +// so a CLI process that ends up calling it more than once (e.g. 'catalog +// show' after an in-process 'catalog search' fallback) installs a single +// provider rather than leaking one per call. +var installMemoryPopularityProviderOnce sync.Once + +// installMemoryPopularityProvider installs a memory-only (no bbolt store) +// popularity provider for the CLI's in-process paths (Spec 110 FR-010): the +// in-process 'catalog search' fallback (no daemon running) and 'catalog +// show'. The core daemon (internal/runtime) installs its own bbolt-backed +// provider instead — this one is never wired there. +func installMemoryPopularityProvider() { + installMemoryPopularityProviderOnce.Do(func() { + registries.SetPopularityProvider(registries.NewGitHubStarsProvider(registries.PopularityOptions{})) + }) +} + +// printCatalogDeprecationNotice prints the FR-066 deprecation note to +// stderr: 'registry search'/'registry add' remain as aliases (scripts must +// not break), but point at their 'catalog' equivalent. +func printCatalogDeprecationNotice(oldCmd, newCmd string) { + fmt.Fprintf(os.Stderr, "Note: 'mcpproxy %s' is deprecated; use 'mcpproxy %s' instead.\n", oldCmd, newCmd) +} + +// Catalog command flags (Spec 109 FR-060/066). +var ( + catalogSearchSource string + catalogSearchTag string + catalogSearchLimit int + catalogAddName string + catalogAddEnv []string + catalogAddEnabled bool +) + +// GetCatalogCommand builds the `catalog` command group (Spec 109 FR-066): a +// source-agnostic search/browse/add flow layered over the same registries +// `registry` manages as sources. +// +// 'catalog' supersedes 'registry search'/'registry add' for DISCOVERY; the +// commands that manage catalog sources themselves — 'registry +// list'/'add-source'/'edit'/'remove' — are unchanged and keep their name +// (renaming a command group breaks scripts, research D17). +func GetCatalogCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "catalog", + Short: "Browse and add MCP servers from the catalog", + Long: `Search across every enabled catalog source (registry) in one call, or browse +the curated official/popular sections with no query. + + mcpproxy catalog search github # search every source + mcpproxy catalog search # browse official + popular + mcpproxy catalog show official/io.github.github/github-mcp-server + mcpproxy catalog add official/io.github.github/github-mcp-server + mcpproxy upstream approve # approve once trusted + +'catalog search'/'catalog add' supersede 'registry search'/'registry add'. +'registry list'/'add-source'/'edit'/'remove' still manage catalog SOURCES.`, + } + cmd.PersistentFlags().StringVarP(®istryConfigPath, "config", "c", "", "Path to MCP configuration file") + cmd.AddCommand(newCatalogSearchCmd(), newCatalogShowCmd(), newCatalogAddCmd()) + return cmd +} + +func newCatalogSearchCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "search [query]", + Short: "Search the catalog across every enabled source", + Long: `Search every enabled catalog source at once (FR-060), ranked official-first, +then verified, then popularity, then text relevance. Omit the query to browse +the curated "official" and "popular" sections instead.`, + Args: cobra.MaximumNArgs(1), + RunE: func(_ *cobra.Command, args []string) error { + query := "" + if len(args) > 0 { + query = args[0] + } + + cfg, err := loadRegistryConfig() + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeConfigNotFound, err.Error()). + WithRecoveryCommand("mcpproxy doctor"), clioutput.ErrCodeConfigNotFound) + } + formatter, err := GetOutputFormatter() + if err != nil { + return err + } + + ctx, cancel := registryContext() + defer cancel() + resp, err := catalogSearch(ctx, cfg, query, catalogSearchSource, catalogSearchTag, catalogSearchLimit) + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeOperationFailed, err.Error()), clioutput.ErrCodeOperationFailed) + } + return renderCatalogSearch(formatter, resp) + }, + } + cmd.Flags().StringVar(&catalogSearchSource, "source", "", "Narrow to one catalog source id (use 'registry list' to see ids)") + cmd.Flags().StringVarP(&catalogSearchTag, "tag", "t", "", "Filter by tag") + cmd.Flags().IntVarP(&catalogSearchLimit, "limit", "l", 20, "Maximum number of results (default 20, max 50)") + return cmd +} + +func newCatalogShowCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "show /", + Short: "Show one catalog entry's details", + Args: cobra.ExactArgs(1), + RunE: func(_ *cobra.Command, args []string) error { + source, id, err := parseCatalogRef(args[0]) + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeInvalidInput, err.Error()). + WithGuidance("Pass '/', e.g. official/io.github.github/github-mcp-server"), clioutput.ErrCodeInvalidInput) + } + + cfg, err := loadRegistryConfig() + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeConfigNotFound, err.Error()). + WithRecoveryCommand("mcpproxy doctor"), clioutput.ErrCodeConfigNotFound) + } + formatter, err := GetOutputFormatter() + if err != nil { + return err + } + + ctx, cancel := registryContext() + defer cancel() + registries.SetRegistriesFromConfig(cfg) + installMemoryPopularityProvider() + reg := registries.FindRegistry(source) + if reg == nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeServerNotFound, fmt.Sprintf("catalog source %q not found", source)). + WithGuidance("Use 'mcpproxy registry list' to see source ids"), clioutput.ErrCodeServerNotFound) + } + entry, err := registries.FindServerByID(ctx, source, id, nil) + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeServerNotFound, err.Error()). + WithGuidance("Use 'mcpproxy catalog search' to find the id"), clioutput.ErrCodeServerNotFound) + } + + hit := registries.BuildCatalogHit(reg, *entry) + result := registries.ToCatalogResult(hit, catalogAddedFromConfig(cfg)(hit)) + return renderCatalogShow(formatter, result) + }, + } + return cmd +} + +func newCatalogAddCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "add /", + Short: "Add a catalog entry as a (quarantined) upstream server", + Long: `Add a server found via 'catalog search'/'catalog show' as an upstream server. +The server is added quarantined by default; approve it once you trust it: + mcpproxy upstream approve + +This is the same keystone add operation as 'registry add ' (Spec +070) — it just takes the combined "/" ref catalog results print.`, + Args: cobra.ExactArgs(1), + RunE: func(_ *cobra.Command, args []string) error { + source, id, err := parseCatalogRef(args[0]) + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeInvalidInput, err.Error()). + WithGuidance("Pass '/', e.g. official/io.github.github/github-mcp-server"), clioutput.ErrCodeInvalidInput) + } + + env, err := parseRegistryEnv(catalogAddEnv) + if err != nil { + return err + } + + cfg, err := loadRegistryConfig() + if err != nil { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeConfigNotFound, err.Error()). + WithRecoveryCommand("mcpproxy doctor"), clioutput.ErrCodeConfigNotFound) + } + + // add MUST go through the daemon (keystone op is server-side, same as + // 'registry add'). + client, ok := newDaemonClient(cfg, nil) + if !ok { + return outputError(clioutput.NewStructuredError(clioutput.ErrCodeConnectionFailed, + "adding from the catalog requires a running mcpproxy daemon"). + WithGuidance("Start the daemon, then retry"). + WithRecoveryCommand("mcpproxy serve"), clioutput.ErrCodeConnectionFailed) + } + + ctx, cancel := registryContext() + defer cancel() + enabled := catalogAddEnabled + result, err := client.AddFromRegistry(ctx, source, id, catalogAddName, env, &enabled) + if err != nil { + return registryAddErrorOutput(err) + } + + outputFormat := ResolveOutputFormat() + if outputFormat == "json" || outputFormat == "yaml" { + formatter, _ := GetOutputFormatter() + out, _ := formatter.Format(result) + fmt.Println(out) + return nil + } + + // FR-063: identical wording to 'registry add' and the Web/macOS "Add + // to MCPProxy" action. + fmt.Println(registryAddMessage(result.Name, result.Quarantined)) + return nil + }, + } + cmd.Flags().StringVar(&catalogAddName, "name", "", "Override the server name") + cmd.Flags().StringArrayVar(&catalogAddEnv, "env", nil, "Set an environment variable (KEY=VALUE); repeatable") + cmd.Flags().BoolVar(&catalogAddEnabled, "enabled", true, "Whether the added server is enabled") + return cmd +} + +// parseCatalogRef splits a "/" ref on the FIRST '/' only, since +// an official-protocol id is itself reverse-DNS-shaped and contains further +// slashes (e.g. "io.github.github/github-mcp-server"). +func parseCatalogRef(ref string) (source, id string, err error) { + parts := strings.SplitN(ref, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return "", "", fmt.Errorf("invalid catalog ref %q: expected '/'", ref) + } + return parts[0], parts[1], nil +} + +// catalogSearch is daemon-first with an in-process fallback (mirrors +// 'registry search'), so catalog discovery works whether or not a daemon is +// running. +func catalogSearch(ctx context.Context, cfg *config.Config, q, source, tag string, limit int) (*cliclient.CatalogSearchResponse, error) { + if client, ok := newDaemonClient(cfg, nil); ok { + if resp, derr := client.CatalogSearch(ctx, q, source, tag, limit); derr == nil { + return resp, nil + } + // Fall through to in-process on daemon error. + } + // Load the effective registry list (built-in defaults + the user's + // configured sources) once, here, so catalogSearchInProcess itself stays + // a pure function over whatever registries.ListRegistries() currently + // returns — which is also what makes it independently testable against a + // fixture list (registries.SetRegistriesForTest). + registries.SetRegistriesFromConfig(cfg) + installMemoryPopularityProvider() + return catalogSearchInProcess(ctx, cfg, q, source, tag, limit) +} + +// catalogSearchInProcess mirrors handleCatalogSearch (internal/httpapi) but +// computes "added" directly from the loaded config, since the CLI's +// in-process fallback has no scoped caller to narrow against. It reads +// whatever registries.ListRegistries() currently returns rather than loading +// it itself — see catalogSearch, its only production caller. +func catalogSearchInProcess(ctx context.Context, cfg *config.Config, q, source, tag string, limit int) (*cliclient.CatalogSearchResponse, error) { + // Source is applied inside SearchAll, BEFORE ranking/truncation to + // limit — filtering after truncation could silently drop a narrower + // source's real matches that simply lost out to an official/verified + // source for one of the truncated top-`limit` slots (same fix as + // httpapi.handleCatalogSearch). + hits, sections, unavailable := registries.SearchAll(ctx, q, tag, limit, registries.SearchOptions{Source: source}) + + added := catalogAddedFromConfig(cfg) + resp := &cliclient.CatalogSearchResponse{Query: q, Unavailable: unavailable} + if resp.Unavailable == nil { + resp.Unavailable = []registries.SourceError{} + } + for _, h := range hits { + resp.Results = append(resp.Results, registries.ToCatalogResult(h, added(h))) + } + if sections != nil { + resp.Sections = &cliclient.CatalogSections{} + for _, h := range sections.Official { + resp.Sections.Official = append(resp.Sections.Official, registries.ToCatalogResult(h, added(h))) + } + for _, h := range sections.Popular { + resp.Sections.Popular = append(resp.Sections.Popular, registries.ToCatalogResult(h, added(h))) + } + } + return resp, nil +} + +// catalogAddedFromConfig returns a predicate reporting whether a catalog hit +// matches an already-configured server (contracts/rest-api.md#catalog "added"): +// a registry-sourced server also needs a matching source, a manual add +// matches on install target alone. +func catalogAddedFromConfig(cfg *config.Config) func(registries.CatalogHit) bool { + byRegistryAndTarget := make(map[string]bool) + byTargetOnly := make(map[string]bool) + if cfg != nil { + for _, s := range cfg.Servers { + if s == nil { + continue + } + target := catalogInstallTargetForConfigServer(s) + if s.SourceRegistryID == "" { + // Manual add: matches any source by install target alone. + byTargetOnly[target] = true + } else { + // Registry-sourced: must also match its own source, so it + // never falsely matches a different source's identical + // install target (contracts/rest-api.md#catalog "added"). + byRegistryAndTarget[s.SourceRegistryID+"\x00"+target] = true + } + } + } + return func(h registries.CatalogHit) bool { + target := registries.CatalogInstallTarget(registries.ToCatalogResult(h, false).Install) + if byRegistryAndTarget[h.Source+"\x00"+target] { + return true + } + return byTargetOnly[target] + } +} + +func catalogInstallTargetForConfigServer(s *config.ServerConfig) string { + if s.URL != "" { + return "url:" + s.URL + } + return "cmd:" + s.Command + " " + strings.Join(s.Args, " ") +} + +// renderCatalogSearch prints a search response as a table (or the raw +// formatter output for json/yaml). +func renderCatalogSearch(formatter clioutput.OutputFormatter, resp *cliclient.CatalogSearchResponse) error { + if _, isTable := formatter.(*clioutput.TableFormatter); isTable { + if resp.Sections != nil { + fmt.Println("Official:") + printCatalogTable(formatter, resp.Sections.Official) + fmt.Println("\nPopular:") + printCatalogTable(formatter, resp.Sections.Popular) + } else { + printCatalogTable(formatter, resp.Results) + } + for _, u := range resp.Unavailable { + fmt.Printf("⚠ %s unavailable: %s\n", u.Source, u.Reason) + } + return nil + } + out, err := formatter.Format(resp) + if err != nil { + return err + } + fmt.Println(out) + return nil +} + +func printCatalogTable(formatter clioutput.OutputFormatter, results []registries.CatalogResult) { + headers := []string{"SOURCE", "ID", "TITLE", "TRANSPORT", "ADDED"} + rows := make([][]string, 0, len(results)) + for _, r := range results { + added := "" + if r.Added { + added = "✓" + } + rows = append(rows, []string{r.Source, r.ID, truncateStr(r.Title, 40), r.Transport, added}) + } + out, err := formatter.FormatTable(headers, rows) + if err == nil { + fmt.Print(out) + } + fmt.Printf("Found %d results. Add one with: mcpproxy catalog add /\n", len(results)) +} + +func renderCatalogShow(formatter clioutput.OutputFormatter, result registries.CatalogResult) error { + if _, isTable := formatter.(*clioutput.TableFormatter); isTable { + fmt.Printf("%s\n", result.Title) + fmt.Printf(" ref: %s/%s\n", result.Source, result.ID) + if result.Publisher != "" { + fmt.Printf(" publisher: %s\n", result.Publisher) + } + fmt.Printf(" official: %v\n", result.Official) + fmt.Printf(" verified: %v\n", result.Verified) + fmt.Printf(" transport: %s\n", result.Transport) + if result.Install.URL != "" { + fmt.Printf(" install url: %s\n", result.Install.URL) + } else { + fmt.Printf(" install cmd: %s %s\n", result.Install.Command, strings.Join(result.Install.Args, " ")) + } + if result.Description != "" { + fmt.Printf(" description: %s\n", result.Description) + } + for _, in := range result.RequiredInputs { + secret := "" + if in.SecretLike { + secret = " (secret)" + } + fmt.Printf(" requires: %s%s\n", in.Name, secret) + } + fmt.Printf(" added: %v\n", result.Added) + return nil + } + out, err := formatter.Format(result) + if err != nil { + return err + } + fmt.Println(out) + return nil +} diff --git a/cmd/mcpproxy/catalog_cmd_test.go b/cmd/mcpproxy/catalog_cmd_test.go new file mode 100644 index 000000000..50c439b04 --- /dev/null +++ b/cmd/mcpproxy/catalog_cmd_test.go @@ -0,0 +1,228 @@ +package main + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" +) + +// TestParseCatalogRef_SplitsOnFirstSlashOnly pins that an official-protocol +// id (itself reverse-DNS-shaped, e.g. "io.github.github/github-mcp-server") +// is not mangled by the "/" split (FR-066). +func TestParseCatalogRef_SplitsOnFirstSlashOnly(t *testing.T) { + source, id, err := parseCatalogRef("official/io.github.github/github-mcp-server") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if source != "official" { + t.Errorf("source = %q, want %q", source, "official") + } + if id != "io.github.github/github-mcp-server" { + t.Errorf("id = %q, want %q", id, "io.github.github/github-mcp-server") + } +} + +func TestParseCatalogRef_RejectsMissingSlash(t *testing.T) { + if _, _, err := parseCatalogRef("no-slash-here"); err == nil { + t.Fatal("expected an error for a ref with no '/'") + } +} + +// TestCatalogAddedFromConfig pins the join rule (contracts/rest-api.md#catalog +// "added"): a registry-sourced configured server needs source AND install +// target; a manual add matches on install target alone. +func TestCatalogAddedFromConfig(t *testing.T) { + cfg := &config.Config{ + Servers: []*config.ServerConfig{ + {Name: "manual", URL: "https://manual.example.com/mcp"}, + {Name: "from-official", Command: "npx", Args: []string{"server-x"}, SourceRegistryID: "official"}, + }, + } + added := catalogAddedFromConfig(cfg) + + manualHit := registries.CatalogHit{Source: "smithery", Entry: registries.ServerEntry{ID: "m", URL: "https://manual.example.com/mcp"}} + if !added(manualHit) { + t.Error("expected a manual add to match by install target regardless of source") + } + + officialHit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "x", InstallCmd: "npx server-x"}} + if !added(officialHit) { + t.Error("expected the registry-sourced server to match its own source + target") + } + + wrongSourceHit := registries.CatalogHit{Source: "smithery", Entry: registries.ServerEntry{ID: "x", InstallCmd: "npx server-x"}} + if added(wrongSourceHit) { + t.Error("a registry-sourced configured server must not match a different source with the same target") + } + + noMatchHit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "y", InstallCmd: "npx server-y"}} + if added(noMatchHit) { + t.Error("expected no match for an unrelated entry") + } +} + +func withCatalogCLIFixture(t *testing.T, body string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + t.Cleanup(registries.AllowPrivateRegistryFetchForTest()) + t.Cleanup(registries.SetRegistriesForTest([]registries.RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: srv.URL, Provenance: "official"}, + })) +} + +// TestCatalogSearchInProcess_Search is the T100 "search" golden: a non-empty +// query prints a flat results table. +func TestCatalogSearchInProcess_Search(t *testing.T) { + withCatalogCLIFixture(t, `[{"id":"gh","name":"GitHub Tool","description":"d"}]`) + cfg := &config.Config{} + + resp, err := catalogSearchInProcess(context.Background(), cfg, "github", "", "", 20) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(resp.Results) != 1 || resp.Results[0].ID != "gh" { + t.Fatalf("unexpected results: %+v", resp.Results) + } + if resp.Sections != nil { + t.Fatalf("expected nil sections for a non-empty query, got %+v", resp.Sections) + } + + formatter, err := GetOutputFormatter() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + out := captureOutput(func() { + if err := renderCatalogSearch(formatter, resp); err != nil { + t.Fatalf("render error: %v", err) + } + }) + if !strings.Contains(out, "GitHub Tool") || !strings.Contains(out, "official") || !strings.Contains(out, "gh") { + t.Errorf("expected the table to name the source, id and title, got:\n%s", out) + } +} + +// TestCatalogSearchInProcess_BrowseSections is the T100 "browse sections" +// golden: an empty query prints Official/Popular sections. +func TestCatalogSearchInProcess_BrowseSections(t *testing.T) { + withCatalogCLIFixture(t, `[{"id":"gh","name":"GitHub Tool"}]`) + cfg := &config.Config{} + + resp, err := catalogSearchInProcess(context.Background(), cfg, "", "", "", 20) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.Sections == nil { + t.Fatal("expected sections for an empty query") + } + + formatter, err := GetOutputFormatter() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + out := captureOutput(func() { + if err := renderCatalogSearch(formatter, resp); err != nil { + t.Fatalf("render error: %v", err) + } + }) + if !strings.Contains(out, "Official:") || !strings.Contains(out, "Popular:") { + t.Errorf("expected Official/Popular section headers, got:\n%s", out) + } +} + +// TestCatalogSearchInProcess_SourceFilterAppliesBeforeTruncation is the CLI +// counterpart of the REST regression: with limit=1, an official source's +// single entry fills the only truncated slot ahead of a lower-ranked +// "other" source's entry. Narrowing to source=other must still surface it +// rather than coming back empty just because it lost the pre-filter +// truncation race. +func TestCatalogSearchInProcess_SourceFilterAppliesBeforeTruncation(t *testing.T) { + officialSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"id":"official-tool","name":"Official Tool"}]`)) + })) + t.Cleanup(officialSrv.Close) + otherSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"id":"other-tool","name":"Other Tool"}]`)) + })) + t.Cleanup(otherSrv.Close) + t.Cleanup(registries.AllowPrivateRegistryFetchForTest()) + t.Cleanup(registries.SetRegistriesForTest([]registries.RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: officialSrv.URL, Provenance: "official"}, + {ID: "other", Name: "Other", ServersURL: otherSrv.URL}, + })) + cfg := &config.Config{} + + resp, err := catalogSearchInProcess(context.Background(), cfg, "tool", "other", "", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(resp.Results) != 1 || resp.Results[0].ID != "other-tool" { + t.Fatalf("expected other-tool to survive source filtering despite limit=1, got: %+v", resp.Results) + } +} + +// TestCatalogShow is the T100 "show" golden. +func TestCatalogShow(t *testing.T) { + withCatalogCLIFixture(t, `[{"id":"gh","name":"GitHub Tool","description":"desc","url":"https://x.example.com/mcp"}]`) + + // withCatalogCLIFixture already installed the fixture registry list — + // unlike catalogSearch's production path, this test must NOT also call + // registries.SetRegistriesFromConfig, which would replace it with the + // real built-in defaults and go to the network. + reg := registries.FindRegistry("official") + if reg == nil { + t.Fatal("expected the fixture 'official' source to be registered") + } + entry, err := registries.FindServerByID(context.Background(), "official", "gh", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + hit := registries.BuildCatalogHit(reg, *entry) + result := registries.ToCatalogResult(hit, false) + + formatter, err := GetOutputFormatter() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + out := captureOutput(func() { + if err := renderCatalogShow(formatter, result); err != nil { + t.Fatalf("render error: %v", err) + } + }) + if !strings.Contains(out, "GitHub Tool") || !strings.Contains(out, "official/gh") || !strings.Contains(out, "https://x.example.com/mcp") { + t.Errorf("unexpected show output:\n%s", out) + } +} + +// TestPrintCatalogDeprecationNotice pins FR-066: 'registry search'/'registry +// add' print a deprecation note pointing at the 'catalog' equivalent. +func TestPrintCatalogDeprecationNotice(t *testing.T) { + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + printCatalogDeprecationNotice("registry add", "catalog add official/gh") + + w.Close() + os.Stderr = old + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + + out := buf.String() + if !strings.Contains(out, "deprecated") || !strings.Contains(out, "catalog add official/gh") { + t.Errorf("unexpected deprecation notice: %q", out) + } +} diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 6c30940b5..98b3b7310 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -206,6 +206,7 @@ func main() { rootCmd.AddCommand(newSandboxExecCommand()) rootCmd.AddCommand(searchCmd) rootCmd.AddCommand(GetRegistryCommand()) + rootCmd.AddCommand(GetCatalogCommand()) rootCmd.AddCommand(toolsCmd) rootCmd.AddCommand(callCmd) rootCmd.AddCommand(codeCmd) diff --git a/cmd/mcpproxy/registry_add_message_test.go b/cmd/mcpproxy/registry_add_message_test.go new file mode 100644 index 000000000..f10c700a7 --- /dev/null +++ b/cmd/mcpproxy/registry_add_message_test.go @@ -0,0 +1,27 @@ +package main + +import ( + "strings" + "testing" +) + +// TestRegistryAddMessage (Spec 109 FR-063 / T025): the CLI prints "Added +// to MCPProxy (quarantined for review)" — the same wording the +// Web/macOS "Add to MCPProxy" action uses after a successful add. +func TestRegistryAddMessage(t *testing.T) { + quarantined := registryAddMessage("github-server", true) + if !strings.Contains(quarantined, "Added github-server to MCPProxy") { + t.Fatalf("expected message to contain %q, got %q", "Added github-server to MCPProxy", quarantined) + } + if !strings.Contains(quarantined, "quarantined for review") { + t.Fatalf("expected quarantined message to say %q, got %q", "quarantined for review", quarantined) + } + + notQuarantined := registryAddMessage("github-server", false) + if !strings.Contains(notQuarantined, "Added github-server to MCPProxy") { + t.Fatalf("expected message to contain %q, got %q", "Added github-server to MCPProxy", notQuarantined) + } + if strings.Contains(notQuarantined, "quarantined") { + t.Fatalf("an enabled (non-quarantined) add must not claim quarantine, got %q", notQuarantined) + } +} diff --git a/cmd/mcpproxy/registry_cmd.go b/cmd/mcpproxy/registry_cmd.go index b2950853c..0e307e0c7 100644 --- a/cmd/mcpproxy/registry_cmd.go +++ b/cmd/mcpproxy/registry_cmd.go @@ -361,6 +361,7 @@ The printed ID column is what you pass to 'registry add'.`, if registryID == "" { return fmt.Errorf("--registry is required (use 'mcpproxy registry list' to see available ids)") } + printCatalogDeprecationNotice("registry search", fmt.Sprintf("catalog search %s --source %s", query, registryID)) ctx, cancel := registryContext() defer cancel() @@ -431,6 +432,7 @@ inputs, supply them with --env KEY=VALUE.`, Args: cobra.ExactArgs(2), RunE: func(_ *cobra.Command, args []string) error { registryID, serverID := args[0], args[1] + printCatalogDeprecationNotice("registry add", fmt.Sprintf("catalog add %s/%s", registryID, serverID)) env, err := parseRegistryEnv(registryAddEnv) if err != nil { @@ -468,11 +470,7 @@ inputs, supply them with --env KEY=VALUE.`, return nil } - fmt.Printf("✅ Added '%s'", result.Name) - if result.Quarantined { - fmt.Printf(" (quarantined — approve with: mcpproxy upstream approve %s)", result.Name) - } - fmt.Println() + fmt.Println(registryAddMessage(result.Name, result.Quarantined)) return nil }, } @@ -482,6 +480,18 @@ inputs, supply them with --env KEY=VALUE.`, return cmd } +// registryAddMessage formats the CLI's success confirmation after adding a +// server from a registry (Spec 109 FR-063): "Added to MCPProxy +// (quarantined for review)" — the same wording the Web/macOS "Add to +// MCPProxy" action uses after a successful add. +func registryAddMessage(name string, quarantined bool) string { + msg := fmt.Sprintf("✅ Added %s to MCPProxy", name) + if quarantined { + msg += fmt.Sprintf(" (quarantined for review — approve with: mcpproxy upstream approve %s)", name) + } + return msg +} + // registryAddErrorOutput maps a *cliclient.RegistryAddError to a structured CLI // error. For missing_required_input it names the exact --env keys to supply. func registryAddErrorOutput(err error) error { diff --git a/cmd/mcpproxy/tools_cmd.go b/cmd/mcpproxy/tools_cmd.go index a95da8f37..8acd47ff2 100644 --- a/cmd/mcpproxy/tools_cmd.go +++ b/cmd/mcpproxy/tools_cmd.go @@ -93,8 +93,14 @@ Examples: traceTransport bool // Enable HTTP/SSE frame-by-frame tracing // Global list filter flags (T019) - toolsStatusFilter string // enabled | disabled | config-denied - toolsRiskFilter string // read | write | destructive + toolsStatusFilter string // enabled | disabled | config-denied + // toolsTierFilter / toolsRiskFilter (Spec 109 FR-028, X11): --risk is kept + // as an alias of --tier for old scripts/muscle-memory; whichever is + // non-empty wins (tier preferred if both are set). Both filter on the + // backend-computed `tier` field (contracts.AnnotationTier) — never a + // locally-derived value. + toolsTierFilter string // read | write | destructive | unannotated + toolsRiskFilter string // alias of --tier toolsApprovalFilter string // approved | pending | changed ) @@ -136,10 +142,16 @@ func groupByServer(targets []serverToolTarget) map[string][]string { // applyGlobalToolFilters applies client-side filters to the global tool list. // statusFilter: "enabled" | "disabled" | "config-denied" | "" -// riskFilter: "read" | "write" | "destructive" | "" +// tierFilter: "read" | "write" | "destructive" | "unannotated" | "" // approvalFilter: "approved" | "pending" | "changed" | "" -func applyGlobalToolFilters(tools []map[string]interface{}, statusFilter, riskFilter, approvalFilter string) []map[string]interface{} { - if statusFilter == "" && riskFilter == "" && approvalFilter == "" { +// +// Spec 109 FR-028 / X11: tierFilter matches the backend-computed `tier` field +// verbatim. It used to read `annotations.operation_type` — a field that does +// not exist on an MCP tool's annotations (operation_type is an intent- +// declaration concept) — so `--risk read` matched nothing over a fixture of +// read-annotated tools. `tier` is what `GET /tools` actually sends. +func applyGlobalToolFilters(tools []map[string]interface{}, statusFilter, tierFilter, approvalFilter string) []map[string]interface{} { + if statusFilter == "" && tierFilter == "" && approvalFilter == "" { return tools } @@ -165,12 +177,9 @@ func applyGlobalToolFilters(tools []map[string]interface{}, statusFilter, riskFi } } - if riskFilter != "" { - opType := "" - if ann, ok := t["annotations"].(map[string]interface{}); ok { - opType, _ = ann["operation_type"].(string) - } - if !strings.EqualFold(opType, riskFilter) { + if tierFilter != "" { + tier := getStringField(t, "tier") + if !strings.EqualFold(tier, tierFilter) { continue } } @@ -187,6 +196,64 @@ func applyGlobalToolFilters(tools []map[string]interface{}, statusFilter, riskFi return out } +// filterToolMetadataByTier applies --tier/--risk to the standalone-mode +// (no-daemon) tool list, computing each tool's tier locally via +// contracts.AnnotationTier — the same function the daemon's +// enrichServerTools uses to populate the "tier" field applyGlobalToolFilters +// reads. Unlike --status/--approval (rejected outright for standalone mode, +// since those need daemon-persisted state this path never touches), tier +// needs nothing but the tool's own annotations (review round 6, finding 4). +// An empty tierFilter returns tools unchanged. +func filterToolMetadataByTier(tools []*config.ToolMetadata, tierFilter string) []*config.ToolMetadata { + if tierFilter == "" { + return tools + } + filtered := make([]*config.ToolMetadata, 0, len(tools)) + for _, tool := range tools { + if tool == nil { + continue + } + if strings.EqualFold(string(contracts.AnnotationTier(tool.Annotations)), tierFilter) { + filtered = append(filtered, tool) + } + } + return filtered +} + +// standaloneNoToolsMessage is the table-mode diagnostic runToolsListStandalone +// prints when the final tool list is empty. +// +// zcode review round 6 (backend fix for finding 4): before +// filterToolMetadataByTier existed, this branch was reachable only when the +// server genuinely exposed zero tools, so guessing "doesn't support tools / +// not properly configured / connection issues" was reasonable. Once a +// --tier/--risk filter can ALSO empty a non-empty discovery result, those +// three guesses are all false for that case — telling an operator who ran +// `--tier destructive` against a server that has tools, just none of that +// tier, that the server "doesn't support tools" is the same misleading +// signal finding 4 was originally about, one step further down the same +// code path. discoveredCount is the count BEFORE tierFilter was applied. +func standaloneNoToolsMessage(serverName, tierFilter string, discoveredCount int) string { + if tierFilter != "" && discoveredCount > 0 { + return fmt.Sprintf("No tools on server '%s' match --tier/--risk=%s (%d tool(s) discovered, none in that tier)\n", + serverName, tierFilter, discoveredCount) + } + return fmt.Sprintf("No tools found on server '%s'\n"+ + "This could indicate:\n"+ + " Server doesn't support tools\n"+ + " Server is not properly configured\n"+ + " Connection issues during tool discovery\n", serverName) +} + +// resolvedTierFilter returns the effective tier filter from --tier / --risk +// (an alias of --tier, Spec 109 FR-028). --tier wins when both are set. +func resolvedTierFilter() string { + if toolsTierFilter != "" { + return toolsTierFilter + } + return toolsRiskFilter +} + // GetToolsCommand returns the tools command for adding to the root command func GetToolsCommand() *cobra.Command { return toolsCmd @@ -219,7 +286,10 @@ func initToolsFlags() { // Global-list filter flags (T019) toolsListCmd.Flags().StringVar(&toolsStatusFilter, "status", "", "Filter by state: enabled, disabled, config-denied") - toolsListCmd.Flags().StringVar(&toolsRiskFilter, "risk", "", "Filter by risk: read, write, destructive") + // Spec 109 FR-028/X11: --tier is canonical; --risk is kept as an alias + // (risk stays the scan-score term elsewhere in the CLI). + toolsListCmd.Flags().StringVar(&toolsTierFilter, "tier", "", "Filter by tier: read, write, destructive, unannotated") + toolsListCmd.Flags().StringVar(&toolsRiskFilter, "risk", "", "Alias of --tier") toolsListCmd.Flags().StringVar(&toolsApprovalFilter, "approval", "", "Filter by approval: approved, pending, changed") // Note: -o/--output flag is inherited from root command via globalOutputFormat @@ -306,7 +376,7 @@ func runToolsListGlobal(ctx context.Context, globalConfig *config.Config, logger } // Apply client-side filters - tools = applyGlobalToolFilters(tools, toolsStatusFilter, toolsRiskFilter, toolsApprovalFilter) + tools = applyGlobalToolFilters(tools, toolsStatusFilter, resolvedTierFilter(), toolsApprovalFilter) return outputGlobalTools(tools) } @@ -442,7 +512,7 @@ func serverToolRows(tools []map[string]interface{}) (headers []string, rows [][] // of the two upstream-controlled columns, NAME and DESCRIPTION — is directly // testable. func globalToolRows(tools []map[string]interface{}) (headers []string, rows [][]string) { - headers = []string{"NAME", "SERVER", "STATE", "APPROVAL", "HELD", "USAGE", "LAST USED", "DESCRIPTION"} + headers = []string{"NAME", "SERVER", "STATE", "TIER", "APPROVAL", "HELD", "USAGE", "LAST USED", "DESCRIPTION"} for _, t := range tools { name := sanitizeName(getStringField(t, "name")) srv := getStringField(t, "server_name") @@ -456,6 +526,13 @@ func globalToolRows(tools []map[string]interface{}) (headers []string, rows [][] state = "disabled" } + // Spec 109 FR-028/X11: rendered verbatim from the backend-computed + // `tier` field — never derived here. + tier := getStringField(t, "tier") + if tier == "" { + tier = "-" + } + approval := getStringField(t, "approval_status") if approval == "" { approval = "-" @@ -470,7 +547,7 @@ func globalToolRows(tools []map[string]interface{}) (headers []string, rows [][] desc := sanitizeCell(getStringField(t, "description"), maxToolDescriptionCell) - rows = append(rows, []string{name, srv, state, approval, formatToolHold(t), usage, lastUsed, desc}) + rows = append(rows, []string{name, srv, state, tier, approval, formatToolHold(t), usage, lastUsed, desc}) } return headers, rows } @@ -710,6 +787,17 @@ func runToolsListClientMode(ctx context.Context, client *cliclient.Client, serve return cliError("failed to get server tools from daemon", err) } + // Apply the same --status/--tier(--risk)/--approval client-side filters as + // the global list. GET /api/v1/servers/{id}/tools shares enrichServerTools + // with the global endpoint (spec 050), so its maps carry the identical + // "tier"/"disabled"/"config_denied"/"approval_status" fields + // applyGlobalToolFilters reads — before this fix, `--server` bypassed + // filtering entirely and always printed every tool unfiltered (review + // round 6, finding 4: an operator auditing one server for destructive + // tools with `--server=x --tier destructive` saw the full unfiltered + // list and could wrongly conclude there were none). + tools = applyGlobalToolFilters(tools, toolsStatusFilter, resolvedTierFilter(), toolsApprovalFilter) + // Output results return outputTools(tools, logger) } @@ -759,6 +847,18 @@ func runToolsListStandalone(ctx context.Context, serverName string, globalConfig serverName, getAvailableServerNames(globalConfig)) } + // --status and --approval read daemon-persisted state (per-tool disabled + // state, config-denied checks, approval records) that this standalone path + // has no access to — it connects directly to the upstream server and never + // touches the daemon's storage. Silently ignoring the flag would print + // every tool unfiltered with no indication the filter never ran (review + // round 6, finding 4); fail fast and say so instead. --tier/--risk is + // still honored below since it needs only the tool's own annotations. + if toolsStatusFilter != "" || toolsApprovalFilter != "" { + return fmt.Errorf("--status and --approval require the daemon (no running daemon detected for standalone server '%s'): "+ + "start mcpproxy (mcpproxy serve) and retry, or drop these flags — --tier/--risk still works standalone", serverName) + } + // Human banner/progress goes to stderr so machine formats (-o json|yaml) // keep stdout parseable (see docs/cli-output-formatting.md). fmt.Fprintf(os.Stderr, "MCP Tools List - Server: %s\n", serverName) @@ -815,15 +915,19 @@ func runToolsListStandalone(ctx context.Context, serverName string, globalConfig return fmt.Errorf("failed to list tools: %w", err) } + // Apply --tier/--risk (review round 6, finding 4). Unlike --status/ + // --approval (rejected above), tier needs only the tool's own + // annotations — no daemon round trip — so it works standalone too, via + // the same AnnotationTier the daemon's enrichServerTools uses. + discoveredCount := len(tools) + tierFilter := resolvedTierFilter() + tools = filterToolMetadataByTier(tools, tierFilter) + // Output results using unified formatter if len(tools) == 0 { outputFormat := ResolveOutputFormat() if outputFormat == "table" { - fmt.Printf("No tools found on server '%s'\n", serverName) - fmt.Printf("This could indicate:\n") - fmt.Printf(" Server doesn't support tools\n") - fmt.Printf(" Server is not properly configured\n") - fmt.Printf(" Connection issues during tool discovery\n") + fmt.Print(standaloneNoToolsMessage(serverName, tierFilter, discoveredCount)) return nil } // For JSON/YAML, output empty array diff --git a/cmd/mcpproxy/tools_cmd_test.go b/cmd/mcpproxy/tools_cmd_test.go index ca24c9271..1e4be08ac 100644 --- a/cmd/mcpproxy/tools_cmd_test.go +++ b/cmd/mcpproxy/tools_cmd_test.go @@ -4,12 +4,15 @@ import ( "bytes" "context" "encoding/json" + "net/http" + "net/http/httptest" "os" "runtime" "strings" "testing" "time" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/socket" @@ -137,23 +140,15 @@ func TestApplyGlobalToolFilters_Status(t *testing.T) { } // TestApplyGlobalToolFilters_Risk verifies the risk filter matches annotations. -func TestApplyGlobalToolFilters_Risk(t *testing.T) { +// TestApplyGlobalToolFilters_Tier is the corrected form of the filter's +// former "risk" test — see tools_tier_test.go for the X11 regression this +// replaces (the field it used to read, annotations.operation_type, does not +// exist on the real GET /tools payload; `tier` does). +func TestApplyGlobalToolFilters_Tier(t *testing.T) { tools := []map[string]interface{}{ - { - "name": "read_file", - "server_name": "srv", - "annotations": map[string]interface{}{"operation_type": "read"}, - }, - { - "name": "write_file", - "server_name": "srv", - "annotations": map[string]interface{}{"operation_type": "write"}, - }, - { - "name": "delete_repo", - "server_name": "srv", - "annotations": map[string]interface{}{"operation_type": "destructive"}, - }, + {"name": "read_file", "server_name": "srv", "tier": "read"}, + {"name": "write_file", "server_name": "srv", "tier": "write"}, + {"name": "delete_repo", "server_name": "srv", "tier": "destructive"}, } got := applyGlobalToolFilters(tools, "", "read", "") @@ -492,3 +487,155 @@ func TestOutputGlobalTools_HeldColumn(t *testing.T) { assert.Contains(t, outStr, "HELD", "table must contain the HELD column") assert.Contains(t, outStr, "TPA-2026-0001", "held tool must surface its matched TPA signature") } + +// TestRunToolsListStandalone_RejectsStatusAndApprovalFilters is a regression +// test for review round 6, finding 4: `--server` bypassed `--status`/ +// `--tier`/`--risk`/`--approval` filtering entirely (they were only ever +// applied in the global, no-`--server` path), so `mcpproxy tools list +// --server=x --tier destructive` silently printed every tool on that server +// unfiltered — an operator auditing one server for destructive tools could +// wrongly conclude there were none. +// +// The standalone (no-daemon) path has no access to the daemon-persisted +// disabled/config-denied/approval state --status and --approval need, so +// rather than silently ignoring them (the exact bug this finding reports), +// it must fail fast with a clear, actionable error instead. +func TestRunToolsListStandalone_RejectsStatusAndApprovalFilters(t *testing.T) { + origStatus, origApproval := toolsStatusFilter, toolsApprovalFilter + defer func() { toolsStatusFilter, toolsApprovalFilter = origStatus, origApproval }() + + cfg := &config.Config{ + Servers: []*config.ServerConfig{ + {Name: "demo", Command: "true"}, + }, + } + logger := zap.NewNop() + + toolsStatusFilter = "disabled" + toolsApprovalFilter = "" + err := runToolsListStandalone(context.Background(), "demo", cfg, logger) + require.Error(t, err, "--status must be rejected, not silently ignored, without a daemon") + assert.Contains(t, err.Error(), "--status and --approval require the daemon") + + toolsStatusFilter = "" + toolsApprovalFilter = "pending" + err = runToolsListStandalone(context.Background(), "demo", cfg, logger) + require.Error(t, err, "--approval must be rejected, not silently ignored, without a daemon") + assert.Contains(t, err.Error(), "--status and --approval require the daemon") +} + +// TestFilterToolMetadataByTier_ReviewRound6Finding4 covers the standalone +// (no-daemon) half of the fix: --tier/--risk needs only the tool's own +// annotations, computed locally via the same contracts.AnnotationTier the +// daemon uses, so it is honored even without a daemon. +func TestFilterToolMetadataByTier_ReviewRound6Finding4(t *testing.T) { + destructive := true + readOnly := true + tools := []*config.ToolMetadata{ + {Name: "delete_repo", Annotations: &config.ToolAnnotations{DestructiveHint: &destructive}}, + {Name: "list_repos", Annotations: &config.ToolAnnotations{ReadOnlyHint: &readOnly}}, + {Name: "mystery_tool"}, + } + + assert.Equal(t, tools, filterToolMetadataByTier(tools, ""), "empty filter must return the list unchanged") + + destructiveOnly := filterToolMetadataByTier(tools, "destructive") + require.Len(t, destructiveOnly, 1) + assert.Equal(t, "delete_repo", destructiveOnly[0].Name) + + readOnlyOnly := filterToolMetadataByTier(tools, "read") + require.Len(t, readOnlyOnly, 1) + assert.Equal(t, "list_repos", readOnlyOnly[0].Name) + + unannotatedOnly := filterToolMetadataByTier(tools, "unannotated") + require.Len(t, unannotatedOnly, 1) + assert.Equal(t, "mystery_tool", unannotatedOnly[0].Name) + + assert.Empty(t, filterToolMetadataByTier(tools, "write"), "no tool is tier=write in this fixture") +} + +// TestRunToolsListClientMode_AppliesTierFilter is the daemon-mode half of the +// review round 6, finding 4 regression: `mcpproxy tools list --server=x +// --tier destructive` against a running daemon used to print every tool on +// that server, ignoring --tier entirely (only the global, no-`--server` path +// ever called applyGlobalToolFilters). GET /api/v1/servers/{id}/tools shares +// enrichServerTools with the global endpoint (spec 050), so its maps already +// carry the same "tier" field — the fix is applying the identical filter, +// not adding new data. +func TestRunToolsListClientMode_AppliesTierFilter(t *testing.T) { + origTier, origRisk, origStatus, origApproval := toolsTierFilter, toolsRiskFilter, toolsStatusFilter, toolsApprovalFilter + defer func() { + toolsTierFilter, toolsRiskFilter, toolsStatusFilter, toolsApprovalFilter = origTier, origRisk, origStatus, origApproval + }() + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/v1/status": + _ = json.NewEncoder(w).Encode(map[string]interface{}{"success": true}) + case "/api/v1/servers/demo/tools": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "success": true, + "data": map[string]interface{}{ + "tools": []map[string]interface{}{ + {"name": "delete_repo", "server_name": "demo", "tier": "destructive"}, + {"name": "list_repos", "server_name": "demo", "tier": "read"}, + }, + }, + }) + default: + t.Errorf("unexpected request path %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer ts.Close() + + client := cliclient.NewClientWithAPIKey(ts.URL, "", nil) + + setOutputGlobals(t, "json", false) + toolsTierFilter, toolsRiskFilter, toolsStatusFilter, toolsApprovalFilter = "destructive", "", "", "" + + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + defer func() { os.Stdout = oldStdout }() + + err := runToolsListClientMode(context.Background(), client, "demo", zap.NewNop()) + + w.Close() + var buf bytes.Buffer + _, _ = buf.ReadFrom(r) + outStr := buf.String() + + require.NoError(t, err) + assert.Contains(t, outStr, "delete_repo", "the destructive tool must survive --tier destructive") + assert.NotContains(t, outStr, "list_repos", "--server must filter out the non-matching tier just like the global list does") +} + +// TestStandaloneNoToolsMessage_TierFilteredResult is a regression test for a +// defect zcode's review round 6 found in the finding-4 fix itself: emptying +// the standalone tool list via --tier/--risk fell into the same "no tools +// found" branch as a server that genuinely has none, printing three false +// diagnostic guesses ("doesn't support tools", "not properly configured", +// "connection issues") for a server that has tools, just none in that tier. +func TestStandaloneNoToolsMessage_TierFilteredResult(t *testing.T) { + msg := standaloneNoToolsMessage("demo", "destructive", 5) + assert.Contains(t, msg, "No tools on server 'demo' match --tier/--risk=destructive") + assert.Contains(t, msg, "5 tool(s) discovered") + assert.NotContains(t, msg, "doesn't support tools", "a tier-filtered empty result must not claim the server lacks tool support") + assert.NotContains(t, msg, "not properly configured") +} + +// TestStandaloneNoToolsMessage_GenuinelyNoTools proves the original +// diagnostic still appears when the server truly exposed zero tools (no +// filter involved, or a filter with nothing to filter). +func TestStandaloneNoToolsMessage_GenuinelyNoTools(t *testing.T) { + msg := standaloneNoToolsMessage("demo", "", 0) + assert.Contains(t, msg, "No tools found on server 'demo'") + assert.Contains(t, msg, "doesn't support tools") + + // A tier filter with nothing discovered at all: still the generic + // message, since there is nothing to attribute to filtering. + msg = standaloneNoToolsMessage("demo", "destructive", 0) + assert.Contains(t, msg, "No tools found on server 'demo'") +} diff --git a/cmd/mcpproxy/tools_tier_test.go b/cmd/mcpproxy/tools_tier_test.go new file mode 100644 index 000000000..a59f1448c --- /dev/null +++ b/cmd/mcpproxy/tools_tier_test.go @@ -0,0 +1,114 @@ +package main + +import ( + "bytes" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Spec 109 FR-028 / T015 (X11): `mcpproxy tools list --tier`/`--risk` filter +// on the backend-computed `tier` field, and the table carries a TIER column +// — none of it derived locally. + +// TestApplyGlobalToolFilters_TierX11Regression pins the bug this PR fixes: +// the filter used to inspect `annotations.operation_type`, a field that does +// not exist anywhere on the real GET /tools payload (operation_type is an +// intent-declaration concept, unrelated to a tool's MCP annotations). Over a +// realistic fixture — a tool whose annotations mark it read-only, the way the +// backend actually reports one — `--risk read` matched nothing. +func TestApplyGlobalToolFilters_TierX11Regression(t *testing.T) { + realisticReadOnlyTool := []map[string]interface{}{ + { + "name": "list_repos", + "server_name": "github", + // This is what a real read-only tool's annotations look like — + // no "operation_type" key anywhere, because that key belongs to + // intent declarations, not MCP tool annotations. + "annotations": map[string]interface{}{"readOnlyHint": true}, + // This is the field the backend actually computes and sends + // (contracts.AnnotationTier) — what the filter must use. + "tier": "read", + }, + } + + got := applyGlobalToolFilters(realisticReadOnlyTool, "", "read", "") + require.Len(t, got, 1, "X11: --risk/--tier must match the backend-computed tier field, not a nonexistent annotations.operation_type") + assert.Equal(t, "list_repos", got[0]["name"]) +} + +func TestApplyGlobalToolFilters_TierAndRiskAreAliases(t *testing.T) { + tools := []map[string]interface{}{ + {"name": "delete_file", "server_name": "fs", "tier": "destructive"}, + {"name": "write_file", "server_name": "fs", "tier": "write"}, + {"name": "read_file", "server_name": "fs", "tier": "read"}, + {"name": "mystery_tool", "server_name": "fs", "tier": "unannotated"}, + } + + for _, tc := range []struct { + filter string + want string + }{ + {"destructive", "delete_file"}, + {"write", "write_file"}, + {"read", "read_file"}, + {"unannotated", "mystery_tool"}, + } { + got := applyGlobalToolFilters(tools, "", tc.filter, "") + require.Len(t, got, 1, "tier=%s", tc.filter) + assert.Equal(t, tc.want, got[0]["name"], "tier=%s", tc.filter) + } +} + +// TestResolvedTierFilter_RiskIsAnAliasOfTier proves --risk and --tier drive +// the same filter, with --tier taking priority if a caller somehow sets both. +func TestResolvedTierFilter_RiskIsAnAliasOfTier(t *testing.T) { + origTier, origRisk := toolsTierFilter, toolsRiskFilter + defer func() { toolsTierFilter, toolsRiskFilter = origTier, origRisk }() + + toolsTierFilter, toolsRiskFilter = "", "" + assert.Equal(t, "", resolvedTierFilter()) + + toolsTierFilter, toolsRiskFilter = "", "write" + assert.Equal(t, "write", resolvedTierFilter(), "--risk alone must apply") + + toolsTierFilter, toolsRiskFilter = "read", "" + assert.Equal(t, "read", resolvedTierFilter(), "--tier alone must apply") + + toolsTierFilter, toolsRiskFilter = "destructive", "read" + assert.Equal(t, "destructive", resolvedTierFilter(), "--tier wins when both are set") +} + +// TestOutputGlobalTools_TierColumn proves the TIER column exists and renders +// the backend-computed value verbatim. +func TestOutputGlobalTools_TierColumn(t *testing.T) { + tools := []map[string]interface{}{ + { + "name": "delete_repo", + "server_name": "github", + "description": "Delete a repository", + "approval_status": "approved", + "tier": "destructive", + "usage": float64(1), + }, + } + + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + defer func() { os.Stdout = oldStdout }() + + setOutputGlobals(t, "table", false) + err := outputGlobalTools(tools) + + w.Close() + var buf bytes.Buffer + _, _ = buf.ReadFrom(r) + outStr := buf.String() + + require.NoError(t, err) + assert.Contains(t, outStr, "TIER", "table must contain a TIER column") + assert.Contains(t, outStr, "destructive", "TIER column must render the backend-computed tier verbatim") +} diff --git a/cmd/mcpproxy/upstream_add_config_mode_test.go b/cmd/mcpproxy/upstream_add_config_mode_test.go index 6aea508ad..a76e7ec57 100644 --- a/cmd/mcpproxy/upstream_add_config_mode_test.go +++ b/cmd/mcpproxy/upstream_add_config_mode_test.go @@ -28,7 +28,9 @@ func TestUpstreamAddConfigModeQuarantineFollowsTrustMode(t *testing.T) { TrustMode: trustMode, Quarantined: explicit, } - require.NoError(t, runUpstreamAddConfigMode(req, cfg)) + added, err := runUpstreamAddConfigMode(req, cfg) + require.NoError(t, err) + require.True(t, added) require.Len(t, cfg.Servers, 1) return cfg.Servers[0] } diff --git a/cmd/mcpproxy/upstream_add_secret.go b/cmd/mcpproxy/upstream_add_secret.go new file mode 100644 index 000000000..4bd12e2b7 --- /dev/null +++ b/cmd/mcpproxy/upstream_add_secret.go @@ -0,0 +1,117 @@ +package main + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" +) + +// applySecretFlags stores each --secret-env/--secret-header value in the OS +// keyring under its Spec 109 FR-065 ref name (internal/secret.RefName) and +// merges ${keyring:} into env/headers in place, so the caller's +// AddServerRequest never carries the raw value. It refuses outright — before +// writing anything — when the keyring is unavailable, naming the provider's +// own reason, rather than silently falling back to plaintext. +// +// It returns the refs THIS call wrote. On success the caller (runUpstreamAdd) +// is responsible for rolling them back if a LATER step (trust-mode +// validation happens first now, but config load, the daemon/config-mode add +// itself, or a --if-not-exists skip) fails or doesn't result in the server +// actually being added — this function has no visibility into those later +// steps. On its OWN failure (a bad flag format, or the second of two flags +// failing to store), it rolls back everything it wrote so far itself and +// returns no refs, so the caller never has to distinguish "nothing was +// written" from "something was written and already cleaned up". +func applySecretFlags(resolver *secret.Resolver, serverName string, secretEnvs, secretHeaders []string, env, headers map[string]string) ([]string, error) { + if len(secretEnvs) == 0 && len(secretHeaders) == 0 { + return nil, nil + } + + if ok, reason := resolver.KeyringAvailability(); !ok { + return nil, fmt.Errorf("cannot store --secret-env/--secret-header: OS keyring unavailable (%s)", reason) + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + existing, err := resolver.ListAll(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list existing keyring entries: %w", err) + } + taken := make(map[string]bool, len(existing)) + for _, ref := range existing { + if ref.Type == secret.SecretTypeKeyring { + taken[ref.Name] = true + } + } + // A ref this call itself just wrote must also count as taken for the + // remaining flags in the same invocation, or two identically-named + // --secret-env flags (or a rerun before ListAll's cache would see it) + // would compute the same ref twice instead of a[-2] suffix (D28: never + // overwrite an existing secret). + takenFn := func(n string) bool { return taken[n] } + + var writtenRefs []string + // fail rolls back everything written so far in this call before + // returning err, so a failure partway through (e.g. the second of two + // --secret-env flags) never leaves the first flag's secret orphaned. + fail := func(err error) ([]string, error) { + for _, ref := range writtenRefs { + _ = resolver.Delete(ctx, secret.Ref{Type: secret.SecretTypeKeyring, Name: ref}) + } + return nil, err + } + + for _, kv := range secretEnvs { + name, value, ok := strings.Cut(kv, "=") + if !ok { + return fail(fmt.Errorf("invalid --secret-env format: %q (expected KEY=value)", kv)) + } + ref := secret.RefName(serverName, "env", name, takenFn) + if err := resolver.Store(ctx, secret.Ref{Type: secret.SecretTypeKeyring, Name: ref}, value); err != nil { + return fail(fmt.Errorf("failed to store secret for env %s: %w", name, err)) + } + taken[ref] = true + writtenRefs = append(writtenRefs, ref) + env[name] = fmt.Sprintf("${keyring:%s}", ref) + } + + for _, kv := range secretHeaders { + name, value, ok := strings.Cut(kv, ":") + if !ok { + return fail(fmt.Errorf("invalid --secret-header format: %q (expected 'Name: value')", kv)) + } + name = strings.TrimSpace(name) + value = strings.TrimSpace(value) + ref := secret.RefName(serverName, "header", name, takenFn) + if err := resolver.Store(ctx, secret.Ref{Type: secret.SecretTypeKeyring, Name: ref}, value); err != nil { + return fail(fmt.Errorf("failed to store secret for header %s: %w", name, err)) + } + taken[ref] = true + writtenRefs = append(writtenRefs, ref) + headers[name] = fmt.Sprintf("${keyring:%s}", ref) + } + + return writtenRefs, nil +} + +// rollbackKeyringRefs is the runUpstreamAdd-level counterpart of +// applySecretFlags' internal rollback: it deletes refs that WERE +// successfully written by applySecretFlags but must not survive because a +// later step (trust-mode validation, config load, the daemon/config-mode add +// itself, or a --if-not-exists skip) didn't result in the server actually +// being added. Best-effort: a delete failure here is not surfaced, since the +// original error is what the user needs to see. +func rollbackKeyringRefs(resolver *secret.Resolver, refs []string) { + if len(refs) == 0 { + return + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + for _, ref := range refs { + _ = resolver.Delete(ctx, secret.Ref{Type: secret.SecretTypeKeyring, Name: ref}) + } +} diff --git a/cmd/mcpproxy/upstream_add_secret_test.go b/cmd/mcpproxy/upstream_add_secret_test.go new file mode 100644 index 000000000..b8f3fcbfb --- /dev/null +++ b/cmd/mcpproxy/upstream_add_secret_test.go @@ -0,0 +1,223 @@ +package main + +import ( + "context" + "fmt" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" +) + +// fakeKeyringProvider is an in-memory secret.Provider standing in for the OS +// keyring, so these tests never touch the real Keychain/Secret Service/WinCred. +type fakeKeyringProvider struct { + available bool + store map[string]string + deleted []string // names passed to Delete, in call order (rollback pinning) + + // failStoreOnNthCall, when > 0, makes the Nth call to Store (1-indexed) + // fail without writing, so tests can pin the internal rollback of + // everything stored by the calls before it. + failStoreOnNthCall int + storeCalls int +} + +func newFakeKeyringProvider(available bool) *fakeKeyringProvider { + return &fakeKeyringProvider{available: available, store: map[string]string{}} +} + +func (f *fakeKeyringProvider) CanResolve(secretType string) bool { return secretType == "keyring" } +func (f *fakeKeyringProvider) Resolve(_ context.Context, ref secret.Ref) (string, error) { + return f.store[ref.Name], nil +} +func (f *fakeKeyringProvider) Store(_ context.Context, ref secret.Ref, value string) error { + f.storeCalls++ + if f.failStoreOnNthCall > 0 && f.storeCalls == f.failStoreOnNthCall { + return fmt.Errorf("simulated keyring failure on call %d", f.storeCalls) + } + f.store[ref.Name] = value + return nil +} +func (f *fakeKeyringProvider) Delete(_ context.Context, ref secret.Ref) error { + f.deleted = append(f.deleted, ref.Name) + delete(f.store, ref.Name) + return nil +} +func (f *fakeKeyringProvider) List(_ context.Context) ([]secret.Ref, error) { + refs := make([]secret.Ref, 0, len(f.store)) + for name := range f.store { + refs = append(refs, secret.Ref{Type: "keyring", Name: name}) + } + return refs, nil +} +func (f *fakeKeyringProvider) IsAvailable() bool { return f.available } + +func newTestSecretResolver(fake *fakeKeyringProvider) *secret.Resolver { + r := secret.NewResolver() + r.RegisterProvider("keyring", fake) + return r +} + +// TestApplySecretFlags_WritesKeyringRefsIntoEnvAndHeaders pins FR-065: values +// are written to the keyring, never left in env/headers, and both maps get +// ${keyring:} instead. +func TestApplySecretFlags_WritesKeyringRefsIntoEnvAndHeaders(t *testing.T) { + fake := newFakeKeyringProvider(true) + resolver := newTestSecretResolver(fake) + + env := map[string]string{} + headers := map[string]string{} + writtenRefs, err := applySecretFlags(resolver, "github", + []string{"GITHUB_TOKEN=sk-live-abc123"}, + []string{"Authorization: Bearer xyz"}, + env, headers, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(writtenRefs) != 2 { + t.Errorf("expected 2 written refs, got %+v", writtenRefs) + } + + envRef, ok := env["GITHUB_TOKEN"] + if !ok || envRef != "${keyring:github-env-github-token}" { + t.Errorf("unexpected env ref: %q", envRef) + } + if fake.store["github-env-github-token"] != "sk-live-abc123" { + t.Errorf("expected the raw value stored in the keyring, got %q", fake.store["github-env-github-token"]) + } + + headerRef, ok := headers["Authorization"] + if !ok || headerRef != "${keyring:github-header-authorization}" { + t.Errorf("unexpected header ref: %q", headerRef) + } + if fake.store["github-header-authorization"] != "Bearer xyz" { + t.Errorf("expected the raw header value stored in the keyring, got %q", fake.store["github-header-authorization"]) + } +} + +// TestApplySecretFlags_EnvHeaderSameNameCollision pins FR-065's headline +// collision case: an env var and a header with the SAME NAME write two +// DISTINCT keyring entries, and each field resolves to its own ref. +func TestApplySecretFlags_EnvHeaderSameNameCollision(t *testing.T) { + fake := newFakeKeyringProvider(true) + resolver := newTestSecretResolver(fake) + + env := map[string]string{} + headers := map[string]string{} + _, err := applySecretFlags(resolver, "github", + []string{"API_KEY=env-value"}, + []string{"API_KEY: header-value"}, + env, headers, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if env["API_KEY"] != "${keyring:github-env-api-key}" { + t.Errorf("unexpected env ref: %q", env["API_KEY"]) + } + if headers["API_KEY"] != "${keyring:github-header-api-key}" { + t.Errorf("unexpected header ref: %q", headers["API_KEY"]) + } + if fake.store["github-env-api-key"] != "env-value" { + t.Errorf("env-value stored under the wrong ref: %+v", fake.store) + } + if fake.store["github-header-api-key"] != "header-value" { + t.Errorf("header-value stored under the wrong ref: %+v", fake.store) + } +} + +// TestApplySecretFlags_TakenNameGetsSuffix pins D28: a pre-existing keyring +// entry is left unchanged and the new ref gets a numeric suffix. +func TestApplySecretFlags_TakenNameGetsSuffix(t *testing.T) { + fake := newFakeKeyringProvider(true) + fake.store["github-env-api-key"] = "PRE-EXISTING-UNRELATED-VALUE" + resolver := newTestSecretResolver(fake) + + env := map[string]string{} + if _, err := applySecretFlags(resolver, "github", []string{"API_KEY=new-value"}, nil, env, map[string]string{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if env["API_KEY"] != "${keyring:github-env-api-key-2}" { + t.Errorf("expected the -2 suffixed ref, got %q", env["API_KEY"]) + } + if fake.store["github-env-api-key"] != "PRE-EXISTING-UNRELATED-VALUE" { + t.Error("the pre-existing entry must be left unchanged (D28: never overwrite)") + } + if fake.store["github-env-api-key-2"] != "new-value" { + t.Errorf("expected the new value under the -2 ref, got %+v", fake.store) + } +} + +// TestApplySecretFlags_KeyringUnavailableRefuses pins FR-065: an unavailable +// keyring refuses with the reason, writing nothing. +func TestApplySecretFlags_KeyringUnavailableRefuses(t *testing.T) { + fake := newFakeKeyringProvider(false) + resolver := newTestSecretResolver(fake) + + env := map[string]string{} + _, err := applySecretFlags(resolver, "github", []string{"API_KEY=value"}, nil, env, map[string]string{}) + if err == nil { + t.Fatal("expected an error when the keyring is unavailable") + } + if len(env) != 0 { + t.Errorf("expected env untouched on refusal, got %+v", env) + } + if len(fake.store) != 0 { + t.Errorf("expected nothing written to the keyring on refusal, got %+v", fake.store) + } +} + +// TestApplySecretFlags_NoFlagsIsNoOp pins that the OS keyring is never probed +// (and no error possible) when neither flag was passed. +func TestApplySecretFlags_NoFlagsIsNoOp(t *testing.T) { + fake := newFakeKeyringProvider(false) // unavailable — must not matter + resolver := newTestSecretResolver(fake) + + if _, err := applySecretFlags(resolver, "github", nil, nil, map[string]string{}, map[string]string{}); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestApplySecretFlags_InvalidFormat pins input validation for both flags. +func TestApplySecretFlags_InvalidFormat(t *testing.T) { + fake := newFakeKeyringProvider(true) + resolver := newTestSecretResolver(fake) + + if _, err := applySecretFlags(resolver, "github", []string{"NOEQUALS"}, nil, map[string]string{}, map[string]string{}); err == nil { + t.Error("expected an error for a --secret-env value with no '='") + } + if _, err := applySecretFlags(resolver, "github", nil, []string{"NoColonHere"}, map[string]string{}, map[string]string{}); err == nil { + t.Error("expected an error for a --secret-header value with no ':'") + } +} + +// TestApplySecretFlags_SecondStoreFailureRollsBackFirst pins the review +// round 1 finding: if the second of two --secret-env/--secret-header flags +// fails to store, the first flag's already-stored secret must not be left +// orphaned in the keyring. +func TestApplySecretFlags_SecondStoreFailureRollsBackFirst(t *testing.T) { + fake := newFakeKeyringProvider(true) + fake.failStoreOnNthCall = 2 + resolver := newTestSecretResolver(fake) + + env := map[string]string{} + writtenRefs, err := applySecretFlags(resolver, "github", + []string{"FIRST_TOKEN=first-value", "SECOND_TOKEN=second-value"}, + nil, env, map[string]string{}, + ) + if err == nil { + t.Fatal("expected an error when the second Store call fails") + } + if len(writtenRefs) != 0 { + t.Errorf("expected no refs returned on failure (everything rolled back), got %+v", writtenRefs) + } + if len(fake.store) != 0 { + t.Errorf("expected the first flag's secret to be rolled back, got %+v", fake.store) + } + if len(fake.deleted) != 1 || fake.deleted[0] != "github-env-first-token" { + t.Errorf("expected exactly one rollback delete for the first ref, got %+v", fake.deleted) + } +} diff --git a/cmd/mcpproxy/upstream_cmd.go b/cmd/mcpproxy/upstream_cmd.go index cd52c46f7..0749d942c 100644 --- a/cmd/mcpproxy/upstream_cmd.go +++ b/cmd/mcpproxy/upstream_cmd.go @@ -25,6 +25,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/logs" "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" ) var ( @@ -274,6 +275,12 @@ Examples: upstreamAddIfNotExists bool upstreamAddNoQuarantine bool upstreamAddTrustMode string + // upstreamAddSecretEnvs/Headers are the Spec 109 FR-065 secret flags: + // each value is written to the OS keyring under the FR-065 ref name + // (internal/secret.RefName) and the config gets ${keyring:} instead + // of the raw value. + upstreamAddSecretEnvs []string + upstreamAddSecretHeaders []string // Remove command flags upstreamRemoveYes bool @@ -353,6 +360,8 @@ func init() { upstreamAddCmd.Flags().BoolVar(&upstreamAddIfNotExists, "if-not-exists", false, "Don't error if server already exists") upstreamAddCmd.Flags().BoolVar(&upstreamAddNoQuarantine, "no-quarantine", false, "Don't quarantine the new server (use with caution)") upstreamAddCmd.Flags().StringVar(&upstreamAddTrustMode, "trust-mode", "", "Per-server trust tier governing admission AND tool-change approval: auto (approve without scanning), scan (auto-approve only when the offline TPA scan is green), manual (human reviews every change). Unset inherits the default (manual)") + upstreamAddCmd.Flags().StringArrayVar(&upstreamAddSecretEnvs, "secret-env", nil, "Environment variable to store in the OS keyring instead of the config, in KEY=value format (repeatable, FR-065)") + upstreamAddCmd.Flags().StringArrayVar(&upstreamAddSecretHeaders, "secret-header", nil, "HTTP header to store in the OS keyring instead of the config, in 'Name: value' format (repeatable, FR-065)") // Remove command flags upstreamRemoveCmd.Flags().BoolVar(&upstreamRemoveYes, "yes", false, "Skip confirmation prompt") @@ -1274,11 +1283,40 @@ func runUpstreamAdd(cmd *cobra.Command, args []string) error { } // GH #938: refuse a typo'd tier before anything is written, with the same - // vocabulary the REST layer reports in its 400. + // vocabulary the REST layer reports in its 400. Checked BEFORE the + // secret-write block below: applySecretFlags writes to the OS keyring, + // so validating trust-mode first means a typo'd tier can never orphan a + // secret that was written only to have the whole add aborted moments + // later (review round 1). if err := validateTrustModeFlag(upstreamAddTrustMode); err != nil { return err } + // FR-065: --secret-env/--secret-header write to the OS keyring instead + // of the config, under the shared per-kind ref name, and merge + // ${keyring:} into the same env/headers maps above. + resolver := secret.NewResolver() + var writtenSecretRefs []string + if len(upstreamAddSecretEnvs) > 0 || len(upstreamAddSecretHeaders) > 0 { + var err error + writtenSecretRefs, err = applySecretFlags(resolver, serverName, upstreamAddSecretEnvs, upstreamAddSecretHeaders, env, headers) + if err != nil { + return err + } + } + // Every return path below this point that does NOT end with the server + // actually being added (a daemon/config-mode failure, or a + // --if-not-exists skip telling the user "skipped" while the secret WAS + // stored) must not leave an orphaned keyring entry behind — added is set + // true only once runUpstreamAddDaemonMode/runUpstreamAddConfigMode + // confirms a genuine add (review round 1). + added := false + defer func() { + if !added { + rollbackKeyringRefs(resolver, writtenSecretRefs) + } + }() + // Build the request req := &cliclient.AddServerRequest{ Name: serverName, @@ -1319,11 +1357,15 @@ func runUpstreamAdd(cmd *cobra.Command, args []string) error { // Check if daemon is running if client, ok := newDaemonClient(globalConfig, nil); ok { - return runUpstreamAddDaemonMode(ctx, client, req) + wasAdded, addErr := runUpstreamAddDaemonMode(ctx, client, req) + added = wasAdded + return addErr } // Direct config file mode - return runUpstreamAddConfigMode(req, globalConfig) + wasAdded, addErr := runUpstreamAddConfigMode(req, globalConfig) + added = wasAdded + return addErr } // outputSkipNotice prints a human skip notice (for --if-not-exists / @@ -1346,17 +1388,23 @@ func outputSkipNotice(notice string, payload map[string]interface{}) error { return nil } -func runUpstreamAddDaemonMode(ctx context.Context, client *cliclient.Client, req *cliclient.AddServerRequest) error { +// runUpstreamAddDaemonMode returns (added, err): added is true only when the +// daemon actually created the server. A --if-not-exists skip (nil error, +// added=false) is deliberately distinguished from a genuine add so the +// caller (runUpstreamAdd) knows whether to roll back any --secret-env/ +// --secret-header values it already wrote to the keyring for this request +// (review round 1: a skip must not leave the secret orphaned). +func runUpstreamAddDaemonMode(ctx context.Context, client *cliclient.Client, req *cliclient.AddServerRequest) (bool, error) { result, err := client.AddServer(ctx, req) if err != nil { // Check if it's "already exists" error and --if-not-exists is set if upstreamAddIfNotExists && strings.Contains(err.Error(), "already exists") { - return outputSkipNotice( + return false, outputSkipNotice( fmt.Sprintf("Server '%s' already exists (skipped)", req.Name), map[string]interface{}{"name": req.Name, "skipped": true}, ) } - return outputError(output.NewStructuredError(output.ErrCodeOperationFailed, err.Error()). + return false, outputError(output.NewStructuredError(output.ErrCodeOperationFailed, err.Error()). WithGuidance("Check the server name and configuration"), output.ErrCodeOperationFailed) } @@ -1373,20 +1421,21 @@ func runUpstreamAddDaemonMode(ctx context.Context, client *cliclient.Client, req } } - return nil + return true, nil } -func runUpstreamAddConfigMode(req *cliclient.AddServerRequest, globalConfig *config.Config) error { +// runUpstreamAddConfigMode returns (added, err) — see runUpstreamAddDaemonMode. +func runUpstreamAddConfigMode(req *cliclient.AddServerRequest, globalConfig *config.Config) (bool, error) { // Check if server already exists for _, srv := range globalConfig.Servers { if srv.Name == req.Name { if upstreamAddIfNotExists { - return outputSkipNotice( + return false, outputSkipNotice( fmt.Sprintf("Server '%s' already exists (skipped)", req.Name), map[string]interface{}{"name": req.Name, "skipped": true}, ) } - return fmt.Errorf("server '%s' already exists", req.Name) + return false, fmt.Errorf("server '%s' already exists", req.Name) } } @@ -1425,7 +1474,7 @@ func runUpstreamAddConfigMode(req *cliclient.AddServerRequest, globalConfig *con // Save config configPath := config.GetConfigPath(globalConfig.DataDir) if err := config.SaveConfig(globalConfig, configPath); err != nil { - return fmt.Errorf("failed to save config: %w", err) + return false, fmt.Errorf("failed to save config: %w", err) } // Output success @@ -1434,7 +1483,7 @@ func runUpstreamAddConfigMode(req *cliclient.AddServerRequest, globalConfig *con fmt.Println(" ⚠️ New servers are quarantined by default. Start the daemon and approve in the web UI.") } - return nil + return true, nil } // runUpstreamRemove handles the 'upstream remove' command @@ -1626,11 +1675,13 @@ func runUpstreamAddJSON(cmd *cobra.Command, args []string) error { // Check if daemon is running if client, ok := newDaemonClient(globalConfig, nil); ok { - return runUpstreamAddDaemonMode(ctx, client, req) + _, err := runUpstreamAddDaemonMode(ctx, client, req) + return err } // Direct config file mode - return runUpstreamAddConfigMode(req, globalConfig) + _, err = runUpstreamAddConfigMode(req, globalConfig) + return err } // validateServerName validates server name format (alphanumeric, hyphens, underscores, 1-64 chars) diff --git a/cmd/mcpproxy/upstream_cmd_test.go b/cmd/mcpproxy/upstream_cmd_test.go index 6b49d81c3..74cd1316c 100644 --- a/cmd/mcpproxy/upstream_cmd_test.go +++ b/cmd/mcpproxy/upstream_cmd_test.go @@ -642,7 +642,7 @@ func TestAddHTTPServerConfigMode(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) w.Close() os.Stdout = oldStdout @@ -717,7 +717,7 @@ func TestAddHTTPServerConfigMode(t *testing.T) { _, w, _ := os.Pipe() os.Stdout = w - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) w.Close() os.Stdout = oldStdout @@ -769,7 +769,7 @@ func TestAddHTTPServerConfigMode(t *testing.T) { } cfg.DataDir = tmpDir - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) if err == nil { t.Error("Expected error for duplicate server") @@ -830,7 +830,7 @@ func TestAddHTTPServerConfigMode(t *testing.T) { os.Stdout = wOut os.Stderr = wErr - err = runUpstreamAddConfigMode(req, cfg) + added, err := runUpstreamAddConfigMode(req, cfg) wOut.Close() wErr.Close() @@ -843,6 +843,13 @@ func TestAddHTTPServerConfigMode(t *testing.T) { if err != nil { t.Errorf("Expected no error with --if-not-exists, got: %v", err) } + // review round 1: added must be false on a skip, distinct from a + // genuine add with a nil error — runUpstreamAdd uses this to decide + // whether to roll back any --secret-env/--secret-header values + // already written to the keyring for this request. + if added { + t.Error("expected added=false for an --if-not-exists skip") + } if !strings.Contains(bufErr.String(), "already exists") || !strings.Contains(bufErr.String(), "skipped") { t.Error("Expected skip message on stderr for existing server") } @@ -890,7 +897,7 @@ func TestAddStdioServerConfigMode(t *testing.T) { _, w, _ := os.Pipe() os.Stdout = w - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) w.Close() os.Stdout = oldStdout @@ -967,7 +974,7 @@ func TestAddStdioServerConfigMode(t *testing.T) { _, w, _ := os.Pipe() os.Stdout = w - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) w.Close() os.Stdout = oldStdout @@ -1370,7 +1377,7 @@ func TestNewServerQuarantineDefault(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w - err = runUpstreamAddConfigMode(req, cfg) + _, err = runUpstreamAddConfigMode(req, cfg) w.Close() os.Stdout = oldStdout diff --git a/docs/registries.md b/docs/registries.md index 052bade47..7586dd5ba 100644 --- a/docs/registries.md +++ b/docs/registries.md @@ -113,16 +113,18 @@ Equivalent surfaces: - **REST:** `POST /api/v1/registries` with `{ "url": "https://…", "protocol": "…", "id": "…", "name": "…" }`. - **CLI:** `mcpproxy registry add-source `. -- **Web UI:** the **Repositories** page has an **Add Registry** button (URL + optional - protocol/name) and a **Registries** section listing every configured source as a +- **Web UI:** Settings → **Catalog Sources** has an **Add Registry** button (URL + optional + protocol/name) and lists every configured source as a card with a neutral **Official / Custom** badge (official cards also carry a **Built-in** tag). There is no warning gate — adding a custom source goes straight through. Custom cards expose a **kebab (⋮) menu** with **Edit** (reuses the add dialog, pre-filled, id read-only) and **Delete** (destructive confirmation); - official cards are read-only. -- **macOS tray:** the **Registries** sidebar tab lists every configured registry - with its provenance/trust badge, offers an **Add Registry** affordance, - and shows a one-time third-party warning before the first custom add. + official cards are read-only. Server discovery itself lives in the catalog-first + **Add Server** page (`/add-server`, Catalog tab), not here. +- **macOS tray:** Settings → **Catalog Sources** lists every configured registry + with its provenance/trust badge and offers an **Add Registry** affordance; + server discovery lives in the Add Server sheet's **Catalog** tab (aggregated + across every enabled source, same as the Web UI). Errors share a stable code across surfaces: `invalid_registry_url` (400), `registries_locked` (403), `registry_shadows_builtin` / `duplicate_registry` (409). @@ -217,6 +219,40 @@ Because every add surface (MCP, REST, CLI) funnels through the same keystone, a packages-only server is added as stdio and a remotes-only server as http identically across all surfaces. +## Catalog popularity signal + +The catalog's `Popular` landing section (and the popularity tiebreak in +search ranking) is backed by two real, source-native signals — no synthetic +scoring: + +- **GitHub stars** for any hit whose `source_code_url` resolves to a + `github.com//` URL (the official registry and reference + sources both carry one for almost every entry). Fetched from + `GET https://api.github.com/repos/{owner}/{repo}`, cached (with ETag + revalidation) for 24h, capped at 4 concurrent requests and a rolling + budget of 50 requests/hour without a token or 4,000/hour with one. +- **Docker Hub pull counts** (`pull_count` from the `docker-mcp-catalog` + registry's listing) map to `Installs`. Docker's own `star_count` is never + used — it lives on a different scale from GitHub stars (single digits vs. + tens of thousands) and the two are never summed or converted into one + another. + +Popularity fetching never blocks a search beyond a short bounded wait +(800ms by default): a cold miss is queued for a background fetch and simply +shows no stars on the current page, arriving on the next one. GitHub is +never listed in a search's `unavailable[]` — it is not a catalog source. + +**`MCPPROXY_GITHUB_TOKEN`** (not the generic `GITHUB_TOKEN`, which is +deliberately never read) raises the GitHub rate-limit budget from 50 to +4,000 requests/hour. Set it if your catalog has many distinct repositories +and stars are taking a while to fill in. + +**`MCPPROXY_CATALOG_POPULARITY=false`** (or `0`/`off`) disables all outbound +GitHub requests — useful for air-gapped or offline setups. Docker's +source-native install counts are unaffected either way, since they come from +the registry listing the Docker source already fetches, not from a separate +popularity call. + ## Adding a discovered server See [registry-add.md](features/registry-add.md). New servers are quarantined by diff --git a/e2e/web-ui-sweep/visual-a11y-sweep.spec.ts b/e2e/web-ui-sweep/visual-a11y-sweep.spec.ts index 930e199bd..5fc9c3772 100644 --- a/e2e/web-ui-sweep/visual-a11y-sweep.spec.ts +++ b/e2e/web-ui-sweep/visual-a11y-sweep.spec.ts @@ -434,6 +434,62 @@ test('the footer never overlaps the page content', async ({ page }) => { expect(overlap, 'main content bleeds under the footer').toBeLessThanOrEqual(1) }) +// Spec 109 PR-a review round 1 (H4) / round 3 finding 2, T007's Playwright +// half: proves the --z-header/--z-sidebar fix (frontend/src/assets/z-index.css, +// frontend/tests/unit/z-index-scale.spec.ts) holds under REAL layout and +// paint, not just token ordering in jsdom. Round 1 shipped these tokens +// inverted, which put the sticky TopHeader back on top of the open mobile +// drawer instead of the other way around — the exact class of stacking bug +// this z-index scale exists to prevent (see z-index.css's own header +// comment). `elementFromPoint` at a coordinate the sticky header and the open +// drawer both occupy is the only way to prove which one the browser actually +// painted on top; a bounding-box/CSS-value assertion would pass even if a +// third ancestor's stacking context silently swallowed the token. +test('mobile drawer sidebar paints above the sticky header, not under it (H4)', async ({ page }) => { + await page.setViewportSize({ width: 390, height: 844 }) + await goto(page, '/') + + const header = page.locator('header') + await expect(header).toBeVisible() + const headerBox = await header.boundingBox() + expect(headerBox, 'header has no box').not.toBeNull() + // A point inside the sticky header's own bounding box — this is exactly + // where the header used to win when the tokens were inverted. + const point = { x: headerBox!.x + headerBox!.width / 2, y: headerBox!.y + headerBox!.height / 2 } + + // Sanity check: before the drawer opens, that point IS the header (proves + // the point is meaningful, not e.g. sitting over a hole in the header). + const beforeOpen = await page.evaluate( + (p) => !!document.elementFromPoint(p.x, p.y)?.closest('header'), + point, + ) + expect(beforeOpen, 'test point does not land on the header before the drawer opens').toBe(true) + + await page.locator('label[for="sidebar-drawer"][aria-label="Open navigation menu"]').click() + await expect(page.locator('#sidebar-drawer')).toBeChecked() + + // daisyUI's drawer opens via a `visibility`/`opacity` CSS transition + // (`allow-discrete`, ~0.2-0.3s), not instantly when the checkbox flips — + // during that brief window the drawer is not yet paintable/hit-testable + // and elementFromPoint legitimately still returns the header underneath, + // exactly like a real user would see for a couple of frames. That is + // normal opening animation, not the H4 regression (a permanently wrong + // SETTLED z-index, not a transient mid-transition frame), so poll for the + // settled state instead of asserting on the very next frame. + const elementAtPoint = () => + page.evaluate((p) => { + const el = document.elementFromPoint(p.x, p.y) + return { + insideHeader: !!el?.closest('header'), + insideDrawer: !!el?.closest('.drawer-side'), + } + }, point) + + await expect + .poll(elementAtPoint, { message: 'drawer never settled above the header at the shared point' }) + .toEqual({ insideHeader: false, insideDrawer: true }) +}) + // --------------------------------------------------------------------------- // F30 — accessible names, live region, table caption. // --------------------------------------------------------------------------- diff --git a/frontend/src/assets/z-index.css b/frontend/src/assets/z-index.css new file mode 100644 index 000000000..99b2b97e8 --- /dev/null +++ b/frontend/src/assets/z-index.css @@ -0,0 +1,28 @@ +/* + * Spec 109 FR-055 (navigation-map.md): one z-index scale, defined in one + * place, so no sidebar/header element can ever paint over a modal (audit H4). + * + * header < sidebar < dropdown < modal < toast + * + * Sidebar outranks header, not the other way around: `SidebarNav`'s + * `.drawer-side` is a `position: fixed` sibling of `.drawer-content` (which + * holds the sticky `TopHeader`), and below the `lg` drawer breakpoint it is + * a full-screen overlay that must paint OVER the header when it opens. A + * review round on this file's first version had `--z-header` above + * `--z-sidebar`, which put the sticky header back on top of the open mobile + * drawer — the same class of bug this scale exists to prevent, just on the + * other pair of elements. + * + * Modals themselves should render in the browser's top layer via + * `.showModal()` rather than relying on this scale (the top layer + * always wins regardless of these values) — `--z-modal` exists for the rare + * non- overlay (e.g. a full-screen click-catcher) that still needs to + * sit above the header/sidebar but below a real modal. + */ +:root { + --z-header: 30; + --z-sidebar: 40; + --z-dropdown: 50; + --z-modal: 60; + --z-toast: 70; +} diff --git a/frontend/src/components/AddSecretModal.vue b/frontend/src/components/AddSecretModal.vue index 3af62a7b1..d741c2f69 100644 --- a/frontend/src/components/AddSecretModal.vue +++ b/frontend/src/components/AddSecretModal.vue @@ -1,5 +1,5 @@