From 5b9b2d96033511d682377eaf7f80ae2dd548449b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 15:10:16 +0000 Subject: [PATCH 1/3] fix(output): neutralize terminal control sequences in human output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Human-mode output wrote API-supplied strings straight to the terminal. A Customer's ID is whatever App User ID the app handed RevenueCat, and the v2 schema constrains it only by length, so an ID can carry OSC 52 — which makes a terminal replace the reader's clipboard as soon as `rc customers list` paints the row. Same bytes reached the card view, the interactive browser, and the stderr pagination hint. Everything the Renderer and the browser show now passes through output.Sanitize, which turns C0/C1 controls and DEL into their escaped literal so remote text can only be shown, never acted on. Newline and tab pass through; --json is untouched, since the JSON encoder already escapes control bytes and agents need the exact value. Refs HackerOne #4039390 --- docs/design-system.md | 18 +++ internal/cli/customers_escape_test.go | 86 +++++++++++++++ internal/output/card.go | 38 ++++--- internal/output/output.go | 68 +++++++----- internal/output/untrusted.go | 58 ++++++++++ internal/output/untrusted_test.go | 152 ++++++++++++++++++++++++++ internal/tui/browser.go | 66 ++++++++++- internal/tui/browser_escape_test.go | 56 ++++++++++ 8 files changed, 493 insertions(+), 49 deletions(-) create mode 100644 internal/cli/customers_escape_test.go create mode 100644 internal/output/untrusted.go create mode 100644 internal/output/untrusted_test.go create mode 100644 internal/tui/browser_escape_test.go diff --git a/docs/design-system.md b/docs/design-system.md index b435d22e..2c046a71 100644 --- a/docs/design-system.md +++ b/docs/design-system.md @@ -44,6 +44,20 @@ semantic styles (`StyleTitle`, `StyleSuccess`, `StyleError`, `StyleDim`, …). | `Render(v)` | humanized fields | any API object (JSON only under `--json`) | | `RenderTable` / `RenderCard` | | lists / result summaries | +Every one of those writes its text through `output.Sanitize` first. Most of +what the CLI shows came from somewhere else — API responses, store metadata, +an App User ID a customer picked for themselves — and a terminal *acts* on +control bytes rather than drawing them (OSC 52 rewrites the clipboard, CSI +moves the cursor, CR overwrites the line above). Sanitized, remote text can +only ever be shown: controls become their escaped literal (`\x1b`), newline +and tab pass through. The interactive browser sanitizes on the way into a +frame, so lazily-loaded children are covered too. `--json` is untouched: the +JSON encoder already escapes control bytes, so agents keep the exact value. + +Strings the CLI styled itself (`Paint`, `Panel`, `Link`) carry deliberate +escapes and are built from our own literals, so they skip sanitizing — never +route remote text through them. + ### Interaction (`internal/tui`) - `tui.Form(...)` and `tui.Confirm*` are the only ways to prompt. They apply @@ -67,6 +81,10 @@ Simple creates stay promptless. colors outside the token layer, raw `huh.NewForm`, or hand-rolled `AssumeYes` checks fail CI. Deliberate exceptions live in an allow list with a written reason. +- Escape-neutralization tests — `internal/output/untrusted_test.go` (every + `Renderer` slot), `internal/tui/browser_escape_test.go` (every browser + view), and `internal/cli/customers_escape_test.go` (end to end, from an + API response to stdout). A writer that prints remote text raw fails here. - `TestOutputSnapshots` — layout and copy of representative commands are locked into golden files (`UPDATE_SNAPSHOTS=1` to regenerate after intentional changes). diff --git a/internal/cli/customers_escape_test.go b/internal/cli/customers_escape_test.go new file mode 100644 index 00000000..beabf2d3 --- /dev/null +++ b/internal/cli/customers_escape_test.go @@ -0,0 +1,86 @@ +package cli_test + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// A Customer's ID is whatever App User ID the app handed RevenueCat, and the +// v2 schema constrains it only by length (docs/specs/v2-developer.yaml — +// `Customer.id`: string, maxLength 1500, no pattern). So an ID can carry +// terminal control bytes, and human-mode output must show them rather than let +// the terminal act on them: OSC 52 replaces the reader's clipboard. +const osc52CustomerID = "rcbb_target\x1b]52;c;UkNCQjE5MQ==\x07" + +// JSON-encoded form of the same ID, as the API would send it on the wire. +const osc52CustomerIDJSON = `rcbb_target\u001b]52;c;UkNCQjE5MQ==\u0007` + +func customerEscapeServer(t *testing.T) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + customer := `{"object":"customer","id":"` + osc52CustomerIDJSON + `","project_id":"proj_esc",` + + `"first_seen_at":1700000000000,"last_seen_at":1700000000000,"last_seen_platform":"ios",` + + `"last_seen_country":"` + osc52CustomerIDJSON + `","last_seen_app_version":"1.0.0",` + + `"active_entitlements":{"object":"list","items":[{"object":"customer.active_entitlement","entitlement_id":"` + osc52CustomerIDJSON + `"}]}}` + switch { + case strings.HasSuffix(r.URL.Path, "/subscriptions"): + io.WriteString(w, `{"object":"list","items":[{"object":"subscription","id":"`+osc52CustomerIDJSON+`","store":"app_store","status":"active"}]}`) + case strings.HasSuffix(r.URL.Path, "/purchases"): + io.WriteString(w, `{"object":"list","items":[]}`) + case strings.HasSuffix(r.URL.Path, "/customers"): + io.WriteString(w, `{"object":"list","items":[`+customer+`],"next_page":"/v2/projects/proj_esc/customers?starting_after=x"}`) + default: + io.WriteString(w, customer) + } + })) + t.Cleanup(server.Close) + return server +} + +func TestCustomers_HumanOutputNeverLetsACustomerIDDriveTheTerminal(t *testing.T) { + server := customerEscapeServer(t) + t.Setenv("RC_BASE_URL", server.URL) + + for _, args := range [][]string{ + {"customers", "list"}, + {"customers", "show", "cus_escape"}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + args = append(args, "--no-input", "--no-color", "--project-id", "proj_esc", "--api-key", "sk_esc") + stdout, stderr, err := runAgentCmd(t, args...) + if err != nil { + t.Fatalf("execute: %v\nstderr: %s", err, stderr) + } + for name, stream := range map[string]string{"stdout": stdout, "stderr": stderr} { + if strings.ContainsRune(stream, 0x1b) || strings.ContainsRune(stream, 0x07) { + t.Errorf("%s hands the terminal a control sequence from the customer ID:\n%q", name, stream) + } + } + if !strings.Contains(stdout, `\x1b]52`) { + t.Errorf("the ID should still be readable as an escaped literal:\n%s", stdout) + } + }) + } +} + +// The agent contract is unchanged: --json carries the exact bytes, encoded. +func TestCustomersList_JSONStillCarriesTheRawIDEncoded(t *testing.T) { + server := customerEscapeServer(t) + t.Setenv("RC_BASE_URL", server.URL) + + stdout, _, err := runAgentCmd(t, "customers", "list", "--json", "--no-input", + "--project-id", "proj_esc", "--api-key", "sk_esc") + if err != nil { + t.Fatalf("execute: %v", err) + } + if strings.ContainsRune(stdout, 0x1b) { + t.Errorf("--json leaked a raw escape byte:\n%q", stdout) + } + if !strings.Contains(stdout, osc52CustomerIDJSON) { + t.Errorf("--json must keep the exact ID:\n%s", stdout) + } +} diff --git a/internal/output/card.go b/internal/output/card.go index 465ea335..89629c12 100644 --- a/internal/output/card.go +++ b/internal/output/card.go @@ -72,16 +72,16 @@ func (r *Renderer) RenderCard(c Card) error { emptyStyle := lipgloss.NewStyle().Faint(true).Italic(true) if c.Title != "" { - fmt.Fprintln(r.stdout, r.style(StyleAccent, "▍ ")+r.style(titleStyle, c.Title)) + fmt.Fprintln(r.stdout, r.style(StyleAccent, "▍ ")+r.style(titleStyle, Sanitize(c.Title))) } if c.Subtitle != "" { - fmt.Fprintln(r.stdout, " "+r.style(subtitleStyle, c.Subtitle)) + fmt.Fprintln(r.stdout, " "+r.style(subtitleStyle, Sanitize(c.Subtitle))) } for _, s := range c.Sections { fmt.Fprintln(r.stdout) if s.Heading != "" { - fmt.Fprintln(r.stdout, r.style(headingStyle, s.Heading)) + fmt.Fprintln(r.stdout, r.style(headingStyle, Sanitize(s.Heading))) } switch { case len(s.Chips) > 0: @@ -95,7 +95,7 @@ func (r *Renderer) RenderCard(c Card) error { if msg == "" { msg = "none" } - fmt.Fprintln(r.stdout, r.style(emptyStyle, " "+msg)) + fmt.Fprintln(r.stdout, r.style(emptyStyle, " "+Sanitize(msg))) } } return nil @@ -110,8 +110,9 @@ func (r *Renderer) writeChips(chips []Chip) { } func (r *Renderer) styleChip(c Chip) string { + label := Sanitize(c.Label) if r.noColor { - return "[" + c.Label + "]" + return "[" + label + "]" } base := lipgloss.NewStyle().Padding(0, 1).Bold(true) white := lipgloss.Color("15") @@ -129,15 +130,20 @@ func (r *Renderer) styleChip(c Chip) string { default: base = base.Background(NeutralGray).Foreground(white) } - return base.Render(c.Label) + return base.Render(label) } func (r *Renderer) writeCardTable(t CardTable) { - widths := make([]int, len(t.Columns)) - for i, c := range t.Columns { + columns := sanitizeAll(t.Columns) + rows := make([][]string, len(t.Rows)) + for i, row := range t.Rows { + rows[i] = sanitizeAll(row) + } + widths := make([]int, len(columns)) + for i, c := range columns { widths[i] = len(c) } - for _, row := range t.Rows { + for _, row := range rows { for i, cell := range row { if i >= len(widths) { continue @@ -149,14 +155,14 @@ func (r *Renderer) writeCardTable(t CardTable) { } headerStyle := lipgloss.NewStyle().Bold(true) fmt.Fprint(r.stdout, " ") - for i, c := range t.Columns { + for i, c := range columns { if i > 0 { fmt.Fprint(r.stdout, " ") } fmt.Fprint(r.stdout, r.style(headerStyle, padRight(c, widths[i]))) } fmt.Fprintln(r.stdout) - for _, row := range t.Rows { + for _, row := range rows { fmt.Fprint(r.stdout, " ") for i, cell := range row { if i > 0 { @@ -169,14 +175,16 @@ func (r *Renderer) writeCardTable(t CardTable) { } func (r *Renderer) writeLines(lines []CardLine) { + safe := make([]CardLine, len(lines)) keyWidth := 0 - for _, l := range lines { - if len(l.Key) > keyWidth { - keyWidth = len(l.Key) + for i, l := range lines { + safe[i] = CardLine{Key: Sanitize(l.Key), Value: Sanitize(l.Value)} + if len(safe[i].Key) > keyWidth { + keyWidth = len(safe[i].Key) } } keyStyle := lipgloss.NewStyle().Faint(true) - for _, l := range lines { + for _, l := range safe { fmt.Fprintf(r.stdout, " %s %s\n", r.style(keyStyle, padRight(l.Key+":", keyWidth+1)), l.Value) } } diff --git a/internal/output/output.go b/internal/output/output.go index 7533c1a8..dff8c7d9 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -187,13 +187,15 @@ func (r *Renderer) renderHuman(v any) error { } keys := humanKeyOrder(m) width := 0 - for _, k := range keys { - if len(k) > width { - width = len(k) + labels := make([]string, len(keys)) + for i, k := range keys { + labels[i] = Sanitize(k) + if len(labels[i]) > width { + width = len(labels[i]) } } - for _, k := range keys { - fmt.Fprintf(r.stdout, "%s %s\n", r.style(r.dim, padRight(k, width)), humanFieldValue(k, m[k])) + for i, k := range keys { + fmt.Fprintf(r.stdout, "%s %s\n", r.style(r.dim, padRight(labels[i], width)), humanFieldValue(k, m[k])) } return nil } @@ -232,14 +234,16 @@ func humanKeyOrder(m map[string]json.RawMessage) []string { } // humanValue renders one JSON value on one line: scalars verbatim, short -// composites as compact JSON, long ones summarized. +// composites as compact JSON, long ones summarized. Composites stay +// JSON-encoded, which already escapes control bytes; bare strings go through +// Sanitize. func humanValue(raw json.RawMessage) string { var s string if json.Unmarshal(raw, &s) == nil { if s == "" { return "—" } - return s + return Sanitize(s) } trimmed := string(raw) if trimmed == "null" { @@ -322,11 +326,16 @@ func (r *Renderer) RenderTable(t Table) error { fmt.Fprintln(r.stderr, r.style(r.info, "• ")+"no results") return nil } - widths := make([]int, len(t.Columns)) - for i, c := range t.Columns { + columns := sanitizeAll(t.Columns) + rows := make([][]string, len(t.Rows)) + for i, row := range t.Rows { + rows[i] = sanitizeAll(row) + } + widths := make([]int, len(columns)) + for i, c := range columns { widths[i] = len(c) } - for _, row := range t.Rows { + for _, row := range rows { for i, cell := range row { if i >= len(widths) { continue @@ -337,14 +346,14 @@ func (r *Renderer) RenderTable(t Table) error { } } headerStyle := lipgloss.NewStyle().Bold(true) - for i, c := range t.Columns { + for i, c := range columns { if i > 0 { fmt.Fprint(r.stdout, " ") } fmt.Fprint(r.stdout, r.style(headerStyle, padRight(c, widths[i]))) } fmt.Fprintln(r.stdout) - for _, row := range t.Rows { + for _, row := range rows { for i, cell := range row { if i > 0 { fmt.Fprint(r.stdout, " ") @@ -378,21 +387,23 @@ func (r *Renderer) Success(msg string) { if r.json || r.quiet { return } - fmt.Fprintln(r.stderr, r.style(r.success, "✓ ")+msg) + fmt.Fprintln(r.stderr, r.style(r.success, "✓ ")+Sanitize(msg)) } func (r *Renderer) Info(msg string) { if r.json || r.quiet { return } - fmt.Fprintln(r.stderr, r.style(r.info, "· ")+msg) + fmt.Fprintln(r.stderr, r.style(r.info, "· ")+Sanitize(msg)) } // Hyperlink wraps styledLabel in an OSC 8 terminal hyperlink pointing at url. // Supporting terminals make it clickable; others render the label text. This is -// the one place the OSC 8 escape lives. +// the one place the OSC 8 escape lives. url is sanitized: a control byte in a +// server-supplied URL would close this sequence early and leave the rest of +// the value to be interpreted as a new one. func Hyperlink(styledLabel, url string) string { - return "\x1b]8;;" + url + "\x1b\\" + styledLabel + "\x1b]8;;\x1b\\" + return "\x1b]8;;" + Sanitize(url) + "\x1b\\" + styledLabel + "\x1b]8;;\x1b\\" } // LinkText renders a clickable hyperlink (OSC 8) with a custom label instead of @@ -400,7 +411,7 @@ func Hyperlink(styledLabel, url string) string { // falls back to "label (url)" so the URL stays copyable. func (r *Renderer) LinkText(label, url string) string { if r.noColor { - return label + " (" + url + ")" + return label + " (" + Sanitize(url) + ")" } return Hyperlink(lipgloss.NewStyle().Foreground(BrandRed).Underline(true).Render(label), url) } @@ -410,7 +421,7 @@ func (r *Renderer) LinkText(label, url string) string { // terminal), so nothing leaks escape codes into piped or --no-color output. func (r *Renderer) Link(url string) string { if r.noColor { - return url + return Sanitize(url) } return Hyperlink(lipgloss.NewStyle().Foreground(BrandRed).Underline(true).Render(url), url) } @@ -430,7 +441,7 @@ func (r *Renderer) Hint(msg string) { if r.json || r.quiet { return } - fmt.Fprintln(r.stderr, r.style(r.dim, " "+msg)) + fmt.Fprintln(r.stderr, r.style(r.dim, " "+Sanitize(msg))) } // Title starts a visually distinct section: a brand-colored bar plus a bold @@ -440,7 +451,7 @@ func (r *Renderer) Title(msg string) { return } fmt.Fprintln(r.stderr) - fmt.Fprintln(r.stderr, r.style(r.accent, "▍ ")+r.style(StyleTitle, msg)) + fmt.Fprintln(r.stderr, r.style(r.accent, "▍ ")+r.style(StyleTitle, Sanitize(msg))) } // Lead is the orienting sentence(s) under a Title: what this flow is for @@ -452,7 +463,7 @@ func (r *Renderer) Lead(text string) { return } const width = 76 - words := strings.Fields(text) + words := strings.Fields(Sanitize(text)) line := " " for _, w := range words { if len(line)+1+len(w) > width { @@ -478,7 +489,7 @@ func (r *Renderer) Notice(lines ...string) { fmt.Fprintln(r.stderr) bar := lipgloss.NewStyle().Foreground(InfoBlue).Bold(true) for _, line := range lines { - fmt.Fprintln(r.stderr, r.style(bar, "▐ ")+line) + fmt.Fprintln(r.stderr, r.style(bar, "▐ ")+Sanitize(line)) } fmt.Fprintln(r.stderr) } @@ -490,7 +501,7 @@ func (r *Renderer) Answer(key, value string) { if r.json || r.quiet { return } - fmt.Fprintf(r.stderr, "%s %s %s\n", r.style(r.success, "✓"), r.style(r.dim, padRight(key, 26)), value) + fmt.Fprintf(r.stderr, "%s %s %s\n", r.style(r.success, "✓"), r.style(r.dim, padRight(Sanitize(key), 26)), Sanitize(value)) } // Plan renders the guided-command plan: a titled, numbered list of the @@ -512,12 +523,13 @@ func (r *Renderer) Field(key, value string, note ...string) { if r.json || r.quiet { return } + value = Sanitize(value) if len(note) > 0 && note[0] != "" { // Pad the value only when a note follows so notes column-align and // bare values carry no trailing whitespace. - value = padRight(value, 15) + " " + r.style(r.dim, "· "+note[0]) + value = padRight(value, 15) + " " + r.style(r.dim, "· "+Sanitize(note[0])) } - fmt.Fprintf(r.stderr, " %s %s\n", r.style(r.dim, padRight(key, 26)), value) + fmt.Fprintf(r.stderr, " %s %s\n", r.style(r.dim, padRight(Sanitize(key), 26)), value) } // Blank prints an empty separator line between logical sections. @@ -532,7 +544,7 @@ func (r *Renderer) Warn(msg string) { if r.json || r.quiet { return } - fmt.Fprintln(r.stderr, r.style(r.warn, "! ")+msg) + fmt.Fprintln(r.stderr, r.style(r.warn, "! ")+Sanitize(msg)) } // AlwaysWarn writes a warning to stderr even in --json mode. @@ -540,12 +552,12 @@ func (r *Renderer) AlwaysWarn(msg string) { if r.quiet { return } - fmt.Fprintln(r.stderr, r.style(r.warn, "! ")+msg) + fmt.Fprintln(r.stderr, r.style(r.warn, "! ")+Sanitize(msg)) } func (r *Renderer) Error(msg string) { if r.json { return } - fmt.Fprintln(r.stderr, r.style(r.errSty, "✗ ")+msg) + fmt.Fprintln(r.stderr, r.style(r.errSty, "✗ ")+Sanitize(msg)) } diff --git a/internal/output/untrusted.go b/internal/output/untrusted.go new file mode 100644 index 00000000..6a730ff5 --- /dev/null +++ b/internal/output/untrusted.go @@ -0,0 +1,58 @@ +package output + +import ( + "fmt" + "strings" +) + +// Sanitize renders terminal control sequences inert. Everything the CLI shows +// a human passes through here first, because most of what it shows came from +// somewhere else: API responses, store metadata, App User IDs a customer chose +// for themselves. A terminal *acts* on those bytes rather than printing them — +// OSC 52 rewrites the clipboard, CSI moves the cursor, CR overwrites the line +// just printed — so an identifier is enough to drive the reader's terminal. +// After this, remote text can only ever be shown. +// +// Newline and tab survive: they move the cursor the same way ordinary text +// does, and some payloads legitimately carry them. Every other C0/C1 control +// and DEL becomes its escaped literal (`\x1b`), which is also what a human +// needs to see to understand what the value actually contains. +// +// Sanitize is not applied to strings the CLI styled itself (Paint, Panel, +// Link) — those carry deliberate escapes and are built from our own literals. +func Sanitize(s string) string { + if !strings.ContainsFunc(s, isControl) { + return s + } + var b strings.Builder + b.Grow(len(s) + 8) + for _, r := range s { + switch { + case !isControl(r): + b.WriteRune(r) + case r > 0x7f: + fmt.Fprintf(&b, `\u%04x`, r) + default: + fmt.Fprintf(&b, `\x%02x`, r) + } + } + return b.String() +} + +// sanitizeAll is the slice form, for table rows and other cell collections. +func sanitizeAll(values []string) []string { + out := make([]string, len(values)) + for i, v := range values { + out[i] = Sanitize(v) + } + return out +} + +// isControl reports whether r drives the terminal instead of printing. +// C1 (0x80–0x9f) counts: terminals in 8-bit mode read 0x9b as CSI. +func isControl(r rune) bool { + if r == '\n' || r == '\t' { + return false + } + return r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) +} diff --git a/internal/output/untrusted_test.go b/internal/output/untrusted_test.go new file mode 100644 index 00000000..e9a22b6c --- /dev/null +++ b/internal/output/untrusted_test.go @@ -0,0 +1,152 @@ +package output_test + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/revenuecat/cli/internal/output" +) + +// osc52 asks the terminal to replace the clipboard with "RCBB191". An App User +// ID is enough to carry it, so it stands in here for any remote string. +const osc52 = "rcbb_target\x1b]52;c;UkNCQjE5MQ==\x07" + +func TestSanitize_NeutralizesControlsAndLeavesTextAlone(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"plain text untouched", "cus_abc123", "cus_abc123"}, + {"unicode untouched", "日本語 café 🐈", "日本語 café 🐈"}, + {"newline and tab survive", "line\nnext\tcell", "line\nnext\tcell"}, + {"clipboard write", osc52, `rcbb_target\x1b]52;c;UkNCQjE5MQ==\x07`}, + {"cursor movement", "a\x1b[2Jb", `a\x1b[2Jb`}, + {"carriage return overwrite", "real\rfake", `real\x0dfake`}, + {"del", "a\x7fb", `a\x7fb`}, + {"c1 csi", "a\u009bb", `a\u009bb`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := output.Sanitize(tc.in); got != tc.want { + t.Errorf("Sanitize(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestRenderTable_EscapesControlSequencesAndKeepsAlignment(t *testing.T) { + r, out, _ := newR(false) + err := r.RenderTable(output.Table{ + Columns: []string{"ID", "PLATFORM"}, + Rows: [][]string{{osc52, "ios"}, {"cus_plain", "android"}}, + }) + if err != nil { + t.Fatal(err) + } + assertNoEscapes(t, "table stdout", out.String()) + if !strings.Contains(out.String(), `\x1b]52`) { + t.Errorf("expected the sequence shown as an escaped literal:\n%q", out.String()) + } + lines := strings.Split(strings.TrimRight(out.String(), "\n"), "\n") + for i, l := range lines[1:] { + if len(l) != len(lines[0]) { + t.Errorf("row %d width %d != header width %d (escaping must be included in the width)", i+1, len(l), len(lines[0])) + } + } +} + +// Escaping is not a side effect of --no-color: the colored path styles the +// value after it has been neutralized. +func TestRenderTable_EscapesControlSequencesWithColorEnabled(t *testing.T) { + var out, errb strings.Builder + r := output.NewRenderer(&out, &errb, false, false, false, "") + err := r.RenderTable(output.Table{Columns: []string{"ID"}, Rows: [][]string{{osc52}}}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(out.String(), "\x1b]52") || strings.ContainsRune(out.String(), 0x07) { + t.Errorf("colored table leaked the clipboard sequence:\n%q", out.String()) + } +} + +func TestRenderCard_EscapesControlSequencesInEverySlot(t *testing.T) { + r, out, _ := newR(false) + err := r.RenderCard(output.Card{ + Title: osc52, + Subtitle: osc52, + Sections: []output.CardSection{ + {Heading: "Active entitlements", Chips: []output.Chip{{Label: osc52}}}, + {Heading: "Subscriptions", Table: &output.CardTable{Columns: []string{"ID"}, Rows: [][]string{{osc52}}}}, + {Heading: "Attributes", Lines: []output.CardLine{{Key: osc52, Value: osc52}}}, + }, + }) + if err != nil { + t.Fatal(err) + } + assertNoEscapes(t, "card stdout", out.String()) +} + +func TestRenderHuman_EscapesControlSequencesInKeysAndValues(t *testing.T) { + r, out, _ := newR(false) + if err := r.Render(map[string]string{"id": osc52, osc52: "value"}); err != nil { + t.Fatal(err) + } + assertNoEscapes(t, "human stdout", out.String()) +} + +func TestChatter_EscapesControlSequences(t *testing.T) { + cases := map[string]func(r *output.Renderer){ + "success": func(r *output.Renderer) { r.Success(osc52) }, + "info": func(r *output.Renderer) { r.Info(osc52) }, + "warn": func(r *output.Renderer) { r.Warn(osc52) }, + "always": func(r *output.Renderer) { r.AlwaysWarn(osc52) }, + "error": func(r *output.Renderer) { r.Error(osc52) }, + "hint": func(r *output.Renderer) { r.Hint(osc52) }, + "title": func(r *output.Renderer) { r.Title(osc52) }, + "lead": func(r *output.Renderer) { r.Lead(osc52) }, + "notice": func(r *output.Renderer) { r.Notice(osc52) }, + "answer": func(r *output.Renderer) { r.Answer("Customer", osc52) }, + "field": func(r *output.Renderer) { r.Field("Customer", osc52, osc52) }, + "plan": func(r *output.Renderer) { r.Plan([]string{osc52}) }, + "link": func(r *output.Renderer) { r.LinkLine("https://example.com/" + osc52) }, + } + for name, call := range cases { + t.Run(name, func(t *testing.T) { + r, out, errb := newR(false) + call(r) + assertNoEscapes(t, name+" stderr", errb.String()) + assertNoEscapes(t, name+" stdout", out.String()) + }) + } +} + +// --json is the agent contract and must keep carrying the exact bytes, safely: +// the JSON encoder escapes them, so nothing reaches the terminal as a sequence. +func TestRenderJSON_KeepsControlBytesEncodedNotExecuted(t *testing.T) { + r, out, _ := newR(true) + if err := r.Render(map[string]string{"id": osc52}); err != nil { + t.Fatal(err) + } + assertNoEscapes(t, "json stdout", out.String()) + var got struct { + Data map[string]string `json:"data"` + } + if err := json.Unmarshal(out.Bytes(), &got); err != nil { + t.Fatal(err) + } + if got.Data["id"] != osc52 { + t.Errorf("--json must round-trip the raw value; got %q", got.Data["id"]) + } +} + +func assertNoEscapes(t *testing.T, where, s string) { + t.Helper() + if strings.ContainsRune(s, 0x1b) { + t.Errorf("%s carries a raw escape byte a terminal would act on:\n%q", where, s) + } + if strings.ContainsRune(s, 0x07) { + t.Errorf("%s carries a raw BEL:\n%q", where, s) + } +} diff --git a/internal/tui/browser.go b/internal/tui/browser.go index 04cf27f3..a163bda0 100644 --- a/internal/tui/browser.go +++ b/internal/tui/browser.go @@ -130,21 +130,75 @@ type bframe struct { } func newListFrame(title string, items []BrowserItem) bframe { - return bframe{kind: kindList, title: title, all: items} + return bframe{kind: kindList, title: output.Sanitize(title), all: sanitizeItems(items)} } func newTableFrame(title string, cols []string, items []BrowserItem) bframe { - return bframe{kind: kindTable, title: title, all: items, tableCols: cols} + return bframe{kind: kindTable, title: output.Sanitize(title), all: sanitizeItems(items), tableCols: cols} } func newDetailFrame(item BrowserItem) bframe { return bframe{ kind: kindDetail, - item: item, + item: sanitizeItem(item), autoLoading: item.AutoLoad != nil, } } +// Frames are the only way data enters the browser, so sanitizing here covers +// every view: remote text (customer IDs, product names) would otherwise reach +// the terminal as escape sequences it acts on rather than characters it draws. +// Lazily-loaded children come back through these same constructors. +func sanitizeItems(items []BrowserItem) []BrowserItem { + out := make([]BrowserItem, len(items)) + for i, it := range items { + out[i] = sanitizeItem(it) + } + return out +} + +func sanitizeItem(it BrowserItem) BrowserItem { + it.ID = output.Sanitize(it.ID) + it.Label = output.Sanitize(it.Label) + it.Meta = output.Sanitize(it.Meta) + it.Row = sanitizeCells(it.Row) + fields := make([]BrowserField, len(it.Fields)) + for i, f := range it.Fields { + fields[i] = BrowserField{Key: output.Sanitize(f.Key), Value: output.Sanitize(f.Value)} + } + it.Fields = fields + links := make([]BrowserLink, len(it.Links)) + for i, l := range it.Links { + l.Label = output.Sanitize(l.Label) + links[i] = l + } + it.Links = links + return it +} + +func sanitizeSections(sections []BrowserSection) []BrowserSection { + out := make([]BrowserSection, len(sections)) + for i, sec := range sections { + sec.Title = output.Sanitize(sec.Title) + sec.Empty = output.Sanitize(sec.Empty) + rows := make([]BrowserSectionRow, len(sec.Rows)) + for j, row := range sec.Rows { + rows[j] = BrowserSectionRow{Cells: sanitizeCells(row.Cells), Item: row.Item} + } + sec.Rows = rows + out[i] = sec + } + return out +} + +func sanitizeCells(cells []string) []string { + out := make([]string, len(cells)) + for i, c := range cells { + out[i] = output.Sanitize(c) + } + return out +} + func (f *bframe) visible() []BrowserItem { if f.filter == "" { return f.all @@ -302,7 +356,7 @@ func (m *browser) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.err != nil { f.autoErr = msg.err.Error() } else { - f.sections = msg.sections + f.sections = sanitizeSections(msg.sections) } // Clamp cursor so it stays within the new slot list. slots := detailSlots(f) @@ -484,7 +538,7 @@ func (m *browser) View() string { } if m.loadErr != "" { return m.renderHeader("Error") + - "\n " + brErr.Render("Error: "+m.loadErr) + + "\n " + brErr.Render("Error: "+output.Sanitize(m.loadErr)) + "\n\n Press any key to dismiss.\n" } f := m.top() @@ -780,7 +834,7 @@ func (m *browser) viewDetail(f *bframe) string { if f.autoLoading { sb.WriteString("\n " + brDim.Render("Loading…") + "\n") } else if f.autoErr != "" { - sb.WriteString("\n " + brErr.Render("Error: "+f.autoErr) + "\n") + sb.WriteString("\n " + brErr.Render("Error: "+output.Sanitize(f.autoErr)) + "\n") } else { for _, sec := range f.sections { sb.WriteString("\n " + brSection.Render(sec.Title) + "\n") diff --git a/internal/tui/browser_escape_test.go b/internal/tui/browser_escape_test.go new file mode 100644 index 00000000..f3296b3b --- /dev/null +++ b/internal/tui/browser_escape_test.go @@ -0,0 +1,56 @@ +package tui + +import ( + "strings" + "testing" +) + +// The interactive browser draws the same remote strings the non-interactive +// renderer does — a customer ID carrying OSC 52 would replace the reader's +// clipboard the moment the list paints. +const browserOSC52 = "rcbb_target\x1b]52;c;UkNCQjE5MQ==\x07" + +func TestBrowserViews_NeverDrawRemoteControlSequences(t *testing.T) { + item := BrowserItem{ + ID: browserOSC52, + Label: browserOSC52, + Meta: browserOSC52, + Row: []string{browserOSC52, "ios"}, + Fields: []BrowserField{{Key: "ID", Value: browserOSC52}}, + Links: []BrowserLink{{Label: browserOSC52}}, + } + sections := []BrowserSection{{ + Title: browserOSC52, + Cols: []string{"PRODUCT", "STORE"}, + Rows: []BrowserSectionRow{{Cells: []string{browserOSC52, "app_store"}}}, + }} + + frames := map[string]bframe{ + "list": newListFrame(browserOSC52, []BrowserItem{item}), + "table": newTableFrame(browserOSC52, []string{"ID", "PLATFORM"}, []BrowserItem{item}), + "detail": newDetailFrame(item), + } + for name, frame := range frames { + t.Run(name, func(t *testing.T) { + m := &browser{stack: []bframe{frame}, width: 120, height: 40} + // Sub-resources arrive asynchronously, after the frame was built. + m.Update(autoLoadedMsg{frameIdx: 0, sections: sections}) + view := m.View() + if strings.Contains(view, "\x1b]52") || strings.ContainsRune(view, 0x07) { + t.Errorf("%s view draws the clipboard sequence:\n%q", name, view) + } + if !strings.Contains(view, `\x1b]52`) { + t.Errorf("%s view should show the value as an escaped literal:\n%s", name, view) + } + }) + } +} + +func TestBrowserErrors_NeverDrawRemoteControlSequences(t *testing.T) { + frame := newListFrame("Customers", nil) + frame.autoErr = "server said: " + browserOSC52 + m := &browser{stack: []bframe{frame}, width: 120, height: 40, loadErr: "server said: " + browserOSC52} + if view := m.View(); strings.Contains(view, "\x1b]52") { + t.Errorf("error view draws the clipboard sequence:\n%q", view) + } +} From 2bc5a499849005da3f64c4d6119a6a71ae6a2a33 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 15:11:40 +0000 Subject: [PATCH 2/3] fix(output): sanitize a link's URL before it becomes the label too Link() styles the raw URL as its own label, so a control byte in a server-supplied URL reached the terminal through the label even though the OSC 8 target was already neutralized. --- internal/output/output.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/output/output.go b/internal/output/output.go index dff8c7d9..9f480e92 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -410,8 +410,9 @@ func Hyperlink(styledLabel, url string) string { // the raw URL, so long auth URLs don't dominate the output. With color off it // falls back to "label (url)" so the URL stays copyable. func (r *Renderer) LinkText(label, url string) string { + url = Sanitize(url) if r.noColor { - return label + " (" + Sanitize(url) + ")" + return label + " (" + url + ")" } return Hyperlink(lipgloss.NewStyle().Foreground(BrandRed).Underline(true).Render(label), url) } @@ -420,8 +421,9 @@ func (r *Renderer) LinkText(label, url string) string { // back to the plain URL when color is off (our proxy for a dumb/non-interactive // terminal), so nothing leaks escape codes into piped or --no-color output. func (r *Renderer) Link(url string) string { + url = Sanitize(url) if r.noColor { - return Sanitize(url) + return url } return Hyperlink(lipgloss.NewStyle().Foreground(BrandRed).Underline(true).Render(url), url) } From 3f590ea9d5857328a919bef26d5333d880b7969e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 15:15:23 +0000 Subject: [PATCH 3/3] docs(output): scope the Sanitize doc comment to what actually calls it --- internal/output/untrusted.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/output/untrusted.go b/internal/output/untrusted.go index 6a730ff5..b0912ce5 100644 --- a/internal/output/untrusted.go +++ b/internal/output/untrusted.go @@ -5,13 +5,13 @@ import ( "strings" ) -// Sanitize renders terminal control sequences inert. Everything the CLI shows -// a human passes through here first, because most of what it shows came from -// somewhere else: API responses, store metadata, App User IDs a customer chose -// for themselves. A terminal *acts* on those bytes rather than printing them — -// OSC 52 rewrites the clipboard, CSI moves the cursor, CR overwrites the line -// just printed — so an identifier is enough to drive the reader's terminal. -// After this, remote text can only ever be shown. +// Sanitize renders terminal control sequences inert. Every value the Renderer +// and the interactive browser show passes through here first, because most of +// what they show came from somewhere else: API responses, store metadata, App +// User IDs a customer chose. A terminal *acts* on those bytes rather than +// printing them — OSC 52 rewrites the clipboard, CSI moves the cursor, CR +// overwrites the line just printed — so an identifier is enough to drive the +// reader's terminal. After this, remote text can only ever be shown. // // Newline and tab survive: they move the cursor the same way ordinary text // does, and some payloads legitimately carry them. Every other C0/C1 control