diff --git a/internal/commands/api.go b/internal/commands/api.go index a2223e6a..959df762 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -3,6 +3,7 @@ package commands import ( "encoding/json" "fmt" + "net/url" "regexp" "strings" @@ -47,7 +48,10 @@ func newAPIGetCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } resp, err := app.Account().Get(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -85,7 +89,10 @@ func newAPIPostCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } // Parse JSON data var body any @@ -134,7 +141,10 @@ func newAPIPutCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } // Parse JSON data var body any @@ -176,7 +186,10 @@ func newAPIDeleteCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } resp, err := app.Account().Delete(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -201,20 +214,92 @@ func apiPathArgs(cmd *cobra.Command, args []string) error { return nil } -// parsePath extracts and normalizes the API path. -// Handles full URLs and relative paths. The leading slash is stripped because -// the SDK's accountPath and buildURL both add one — keeping it here would -// double-slash and, on Windows, MSYS/Git Bash converts /path to C:\...\path. -func parsePath(input string) string { - urlPattern := regexp.MustCompile(`^https?://[^/]+/[0-9]+(/.*)`) - if matches := urlPattern.FindStringSubmatch(input); len(matches) > 1 { - return matches[1] +// accountSegmentPattern matches a leading / segment in an API +// path so it can be dropped — the SDK re-prefixes the configured account. The +// trailing path is optional so a bare "/" (no further segments) +// also matches, leaving m[2] empty. +var accountSegmentPattern = regexp.MustCompile(`^/([0-9]+)(/.*)?$`) + +// parsePath normalizes the user-supplied API path against the configured base +// URL. It accepts relative paths ("projects.json", "/projects.json") and +// absolute Basecamp URLs whose host matches baseURL — from which the path is +// extracted and a leading / segment dropped (the SDK re-prefixes +// the configured account). Absolute URLs on ANY other host are rejected so the +// bearer token is never attached to a request bound for a foreign host. +// +// A leading account segment must match the configured accountID: the SDK +// re-prefixes the configured account, so silently stripping a foreign account +// id would retarget the request (including DELETEs) at the wrong account. +// When accountID is empty (no configured account to compare against), the +// segment is stripped as before. +// +// All leading slashes and a mixed-case scheme are normalized first so neither +// "//https://evil/…" nor "HTTPS://evil/…" can smuggle an absolute URL past the +// host check (URL schemes are case-insensitive per RFC 3986 §3.1). +func parsePath(input, baseURL, accountID string) (string, error) { + candidate := strings.TrimLeft(input, "/") + lower := strings.ToLower(candidate) + if strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") { + u, err := url.Parse(candidate) + if err != nil || u.Host == "" { + return "", output.ErrUsage("invalid API URL: " + input) + } + base, baseErr := url.Parse(baseURL) + if baseErr != nil || base.Host == "" || !sameHostPort(u, base) { + return "", output.ErrUsage("API path must be relative or a Basecamp URL on the configured host; refusing to send credentials to " + input) + } + // Same host: use the path (+ query), dropping a leading / + // — but only when it matches the configured account. + path := u.EscapedPath() + if m := accountSegmentPattern.FindStringSubmatch(path); m != nil { + if accountID != "" && m[1] != accountID { + return "", output.ErrUsage(fmt.Sprintf("URL is for account %s; refusing to retarget to configured account %s", m[1], accountID)) + } + // A bare "/" leaves m[2] empty; the request targets the + // account root, so normalize to "/" rather than an empty path. + if path = m[2]; path == "" { + path = "/" + } + } + // A same-host URL with no path at all (e.g. "https://host" or a + // query-only "https://host?foo=bar") leaves path empty since the + // account-segment pattern can't match ""; normalize to the root. + if path == "" { + path = "/" + } + if u.RawQuery != "" { + path += "?" + u.RawQuery + } + return path, nil } - // Strip leading slash — the SDK prefixes the account path. - input = strings.TrimPrefix(input, "/") + // Relative path — return without leading slashes; the SDK prefixes the + // account path (keeping one here would double-slash and, on Windows, + // MSYS/Git Bash converts /path to C:\...\path). + return candidate, nil +} + +// sameHostPort reports whether u and base address the same host: hostnames +// compared case-insensitively, ports compared after normalizing an omitted +// port to the scheme default. A matching hostname on a different port is +// still a different origin and must not receive credentials. +func sameHostPort(u, base *url.URL) bool { + if !strings.EqualFold(u.Hostname(), base.Hostname()) { + return false + } + return effectivePort(u) == effectivePort(base) +} - return input +// effectivePort returns the URL's explicit port, or the scheme's default +// (80 for http, 443 for https) when omitted. +func effectivePort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + if strings.EqualFold(u.Scheme, "http") { + return "80" + } + return "443" } // apiSummary generates a summary from the API response. diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 103ec1f0..901bc363 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -12,6 +12,11 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +const ( + testBaseURL = "https://3.basecampapi.com" + testAccountID = "12345" +) + func TestParsePath(t *testing.T) { tests := []struct { input string @@ -21,13 +26,96 @@ func TestParsePath(t *testing.T) { {"/projects.json", "projects.json"}, {"buckets/123/todos/456.json", "buckets/123/todos/456.json"}, {"/buckets/123/todos/456.json", "buckets/123/todos/456.json"}, - {"https://3.basecampapi.com/999/projects.json", "/projects.json"}, + // Same-host absolute URLs: extract the path, dropping the account + // segment when it matches the configured account. + {"https://3.basecampapi.com/12345/projects.json", "/projects.json"}, {"https://3.basecampapi.com/12345/buckets/1/todos/2.json", "/buckets/1/todos/2.json"}, + // Same host but no account segment — accepted, path used as-is. + {"https://3.basecampapi.com/projects.json", "/projects.json"}, + // Same-host URL with no path at all normalizes to the account root. + {"https://3.basecampapi.com", "/"}, + // Same-host query-only URL normalizes to root and keeps the query. + {"https://3.basecampapi.com?foo=bar", "/?foo=bar"}, + // A bare "/" with no trailing segment still matches and + // strips to the account root path rather than leaving it re-prefixed. + {"https://3.basecampapi.com/12345", "/"}, + // Bare "/" plus a query strips to root and keeps the query. + {"https://3.basecampapi.com/12345?foo=bar", "/?foo=bar"}, + // Query strings are preserved. + {"https://3.basecampapi.com/12345/projects.json?page=2", "/projects.json?page=2"}, + // Schemes are case-insensitive (RFC 3986): a same-host uppercase scheme + // still extracts the path. + {"HTTPS://3.basecampapi.com/12345/projects.json", "/projects.json"}, + // An explicit default port is still the configured host. + {"https://3.basecampapi.com:443/12345/projects.json", "/projects.json"}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, parsePath(tt.input)) + got, err := parsePath(tt.input, testBaseURL, testAccountID) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestParsePathRejectsForeignAccount guards against silent account +// retargeting: the SDK re-prefixes the configured account, so stripping a +// different account's id from a pasted URL would aim the request (including +// DELETEs) at the configured account instead of the one the URL names. +func TestParsePathRejectsForeignAccount(t *testing.T) { + for _, input := range []string{ + "https://3.basecampapi.com/999/projects.json", + "https://3.basecampapi.com/999/buckets/1/todos/2.json", + "https://3.basecampapi.com/999/projects.json?page=2", + // A bare foreign "/" (no trailing path) must still be + // rejected — the broadened pattern must not open a retargeting hole. + "https://3.basecampapi.com/999", + } { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to retarget") + assert.Empty(t, got) + }) + } + + t.Run("empty configured account strips the segment as before", func(t *testing.T) { + got, err := parsePath("https://3.basecampapi.com/999/projects.json", testBaseURL, "") + require.NoError(t, err) + assert.Equal(t, "/projects.json", got) + }) +} + +// TestParsePathRejectsForeignHosts guards against bearer-token exfiltration: an +// absolute URL whose host differs from the configured base URL must be rejected +// before it reaches the SDK, which would otherwise attach the Authorization +// header and send credentials to the foreign host. Covers mixed-case schemes, +// leading-slash smuggling attempts, and a same-hostname non-default port. +func TestParsePathRejectsForeignHosts(t *testing.T) { + bad := []string{ + "https://evil.com/projects.json", + "http://evil.com/projects.json", + "https://evil.com/", + "https://evil.com", + "https://evil.example/999/projects.json", // foreign host, numeric form + "http://127.0.0.1:9999/projects.json", + "HTTPS://evil.example/projects.json", // uppercase scheme + "HtTpS://evil.example/projects.json", // mixed-case scheme + "/https://evil.example/projects.json", // leading-slash smuggling + "/HTTPS://evil.example/projects.json", // leading slash + uppercase + "//https://evil.example/projects.json", // double-slash smuggling + "///https://evil.example/x", // triple-slash smuggling + "https://3.basecampapi.com:8443/projects.json", // same hostname, non-default port + "https://3.basecampapi.com:443.evil.com/projects.json", // port-lookalike host smuggling + "https://3.basecampapi.com:abc/x", // non-numeric port + } + + for _, input := range bad { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL, testAccountID) + require.Error(t, err) + assert.Empty(t, got) }) } } diff --git a/internal/commands/timeline.go b/internal/commands/timeline.go index 99d2a903..f71c205b 100644 --- a/internal/commands/timeline.go +++ b/internal/commands/timeline.go @@ -18,6 +18,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // NewTimelineCmd creates the timeline command for viewing activity feeds. @@ -395,19 +396,26 @@ func formatEvent(e basecamp.TimelineEvent) string { timeStr = "--:--" } + // API-controlled fields are sanitized before rendering: rune truncation + // preserves escape bytes, and the alt-screen watch TUI would otherwise + // execute embedded OSC/ANSI sequences (terminal injection). + // Strip before the empty check so a name that's only escape sequences + // still falls back to the placeholder rather than rendering blank. creatorName := "Someone" - if e.Creator != nil && e.Creator.Name != "" { - creatorName = e.Creator.Name + if e.Creator != nil { + if name := richtext.SanitizeSingleLine(e.Creator.Name); name != "" { + creatorName = name + } } - action := e.Action + action := richtext.SanitizeSingleLine(e.Action) if action == "" { action = "updated" } - title := e.Title + title := richtext.SanitizeSingleLine(e.Title) if title == "" { - title = e.SummaryExcerpt + title = richtext.SanitizeSingleLine(e.SummaryExcerpt) } // Truncate at rune boundary for proper Unicode handling if len([]rune(title)) > 40 { @@ -422,6 +430,19 @@ func formatEvent(e basecamp.TimelineEvent) string { ) } +// watchLabel builds a watch-status label from an API-controlled name, +// sanitizing it for the single-line terminal sink. An all-escape name collapses +// to "" after sanitization, which would render a blank trailing value (e.g. +// "activity for "); in that case fall back to the already-resolved ID so the +// label always names something concrete. +func watchLabel(format, name, fallbackID string) string { + safeName := richtext.SanitizeSingleLine(name) + if safeName == "" { + safeName = fallbackID + } + return fmt.Sprintf(format, safeName) +} + func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, interval time.Duration, limit, page int, all bool) error { if all { return output.ErrUsage("--all cannot be used with --watch") @@ -507,7 +528,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, if err != nil { return output.ErrUsage("Invalid person ID") } - description = fmt.Sprintf("activity for %s", personName) + description = watchLabel("activity for %s", personName, resolvedPersonID) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { result, err := app.Account().Timeline().PersonProgress(ctx, personID, opts) if err != nil { @@ -525,7 +546,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, if err != nil { return output.ErrUsage("Invalid project ID") } - description = fmt.Sprintf("activity in %s", projectName) + description = watchLabel("activity in %s", projectName, resolvedProjectID) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { r, err := app.Account().Timeline().ProjectTimeline(ctx, projectIDInt, opts) if err != nil { diff --git a/internal/commands/timeline_test.go b/internal/commands/timeline_test.go new file mode 100644 index 00000000..0bd24a99 --- /dev/null +++ b/internal/commands/timeline_test.go @@ -0,0 +1,80 @@ +package commands + +import ( + "strings" + "testing" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/charmbracelet/x/ansi" + "github.com/stretchr/testify/assert" +) + +// TestFormatEventSanitizesSingleLine verifies that API-controlled fields +// rendered into the single-line alt-screen watch event string are collapsed to +// one line: bare CR between words becomes a separator (words are not glued), +// and embedded newlines/tabs/escape sequences cannot break the layout or inject +// into the terminal. +func TestFormatEventSanitizesSingleLine(t *testing.T) { + e := basecamp.TimelineEvent{ + Creator: &basecamp.Person{Name: "Ann\rBeth"}, + Action: "com\nmented", + Title: "hello\tworld\x1b[31m\r\nagain\u009b0m", + } + + // Strip lipgloss's own styling so we assert on the API-controlled content. + got := ansi.Strip(formatEvent(e)) + + assert.NotContains(t, got, "\n") + assert.NotContains(t, got, "\r") + assert.NotContains(t, got, "\t") + assert.NotContains(t, got, "\x1b") + assert.NotContains(t, got, "\u009b") + + // Words remain space-separated rather than glued together. + assert.Contains(t, got, "Ann Beth") + assert.Contains(t, got, "com mented") + assert.Contains(t, got, "hello world") + assert.NotContains(t, got, "AnnBeth") + assert.NotContains(t, got, "commented") +} + +// TestFormatEventEmptyFieldsFallBack verifies the empty-check fallbacks still +// fire when a field sanitizes down to the empty string (all whitespace/control). +func TestFormatEventEmptyFieldsFallBack(t *testing.T) { + e := basecamp.TimelineEvent{ + Creator: &basecamp.Person{Name: "\x1b[0m\r\n"}, + Action: "\t \r ", + Title: "\x1b[31m", + SummaryExcerpt: "fell\rback", + } + + got := ansi.Strip(formatEvent(e)) + + assert.Contains(t, got, "Someone") + assert.Contains(t, got, "updated") + // Title was empty, so SummaryExcerpt is used and also single-lined. + assert.Contains(t, got, "fell back") + assert.False(t, strings.Contains(got, "fellback")) +} + +// TestWatchLabelSanitizesName verifies a normal name is sanitized for the +// single-line watch-status sink and kept when non-empty. +func TestWatchLabelSanitizesName(t *testing.T) { + got := watchLabel("activity for %s", "Ann\x1b[31m Beth\r", "12345") + assert.Equal(t, "activity for Ann Beth", got) + assert.NotContains(t, got, "\x1b") +} + +// TestWatchLabelFallsBackToID verifies that an all-escape name — which collapses +// to "" after sanitization — falls back to the already-resolved ID so the label +// never renders with a blank trailing value ("activity for "). +func TestWatchLabelFallsBackToID(t *testing.T) { + allEscape := "\x1b[2J\x1b]0;x\x07\x08\x7f" + + person := watchLabel("activity for %s", allEscape, "12345") + assert.Equal(t, "activity for 12345", person) + assert.NotContains(t, person, "\x1b") + + project := watchLabel("activity in %s", allEscape, "67890") + assert.Equal(t, "activity in 67890", project) +} diff --git a/internal/output/envelope.go b/internal/output/envelope.go index 1bf01cd8..fa83ca6d 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -1,10 +1,13 @@ package output import ( + "bytes" "encoding/json" "fmt" "io" "os" + "sort" + "strconv" "strings" "github.com/itchyny/gojq" @@ -13,6 +16,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/observability" "github.com/basecamp/basecamp-cli/internal/presenter" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // NormalizeData converts json.RawMessage and other types to standard Go types. @@ -202,7 +206,13 @@ func (w *Writer) write(v any) error { // early-return so that --agent --jq still emits the diagnostic. if w.opts.Format == FormatQuiet { if resp, ok := v.(*Response); ok && resp.noticeDiagnostic && resp.Notice != "" { - fmt.Fprintf(w.opts.ErrWriter, "notice: %s\n", resp.Notice) + // The notice may interpolate API-controlled strings and stderr is + // a terminal sink; sanitize and keep the diagnostic to one line. + // Sanitize first, then gate: an all-escape notice collapses to "" + // and must not emit a blank "notice: " line. + if notice := sanitizeText(resp.Notice, true, false); notice != "" { + fmt.Fprintf(w.opts.ErrWriter, "notice: %s\n", notice) + } } } @@ -282,6 +292,12 @@ func (w *Writer) writeJQ(v any) error { return ErrJQRuntime(fmt.Errorf("unmarshal: %w", err)) } + // Sanitization is TTY-gated: --jq to a terminal strips escape sequences + // and control characters to prevent terminal injection, while piped or + // redirected output is the machine-consumption contract (matching + // writeJSON) and passes bytes through verbatim. + tty := isTTY(w.opts.Writer) + iter := code.Run(input) for { result, ok := iter.Next() @@ -291,10 +307,24 @@ func (w *Writer) writeJQ(v any) error { if err, ok := result.(error); ok { return ErrJQRuntime(err) } - // Print strings without JSON encoding for cleaner output + // Print strings without JSON encoding for cleaner output. + // On a TTY, SanitizeTerminal guards against terminal injection: --jq + // can select API-controlled strings, and unlike JSON marshaling (which + // escapes control bytes) this path would otherwise emit raw OSC/CSI + // sequences. if s, ok := result.(string); ok { + if tty { + s = richtext.SanitizeTerminal(s) + } fmt.Fprintln(w.opts.Writer, s) } else { + // Compound results (arrays/objects) are marshaled, but Go's JSON + // encoder only escapes C0 controls — UTF-8-encoded C1 controls + // (U+0080–U+009F) pass through raw and execute on C1-honoring + // terminals. On a TTY, sanitize every string leaf (and key) first. + if tty { + result = sanitizeJSONValue(result) + } raw, err := json.Marshal(result) if err != nil { return ErrJQRuntime(fmt.Errorf("result not serializable: %w", err)) @@ -305,8 +335,64 @@ func (w *Writer) writeJQ(v any) error { return nil } -// isTTY checks if the writer is a terminal. -func isTTY(w io.Writer) bool { +// sanitizeJSONValue recursively strips terminal escape sequences and control +// characters from every string in a jq result — map keys, map values, and +// slice elements — before it is marshaled for a TTY-bound --jq sink. +// Non-string leaves (numbers, bools, nil, and gojq's numeric types) pass +// through unchanged. +func sanitizeJSONValue(v any) any { + switch val := v.(type) { + case string: + return richtext.SanitizeTerminal(val) + case []any: + for i, elem := range val { + val[i] = sanitizeJSONValue(elem) + } + return val + case map[string]any: + return sanitizeJSONMap(val) + default: + return v + } +} + +// sanitizeJSONMap rebuilds a map with terminal-safe keys without ever +// dropping an entry. Stripping keys would let a hostile "\x1b[31mtitle" +// collapse onto a legitimate "title" and silently replace or hide that +// field. Instead, keys already free of escapes and controls keep their name +// — a hostile key can never displace one — and keys changed by sanitization +// are visibly escaped (strconv.Quote), re-quoting with delimiters until the +// name is unique so even a literal key that mimics escape notation cannot +// be overwritten. Escaped keys are processed in sorted order so the +// resulting names are deterministic. +func sanitizeJSONMap(val map[string]any) map[string]any { + out := make(map[string]any, len(val)) + var escaped []string + for k, elem := range val { + if richtext.SanitizeTerminal(k) == k { + out[k] = sanitizeJSONValue(elem) + } else { + escaped = append(escaped, k) + } + } + sort.Strings(escaped) + for _, k := range escaped { + quoted := strconv.Quote(k) + name := quoted[1 : len(quoted)-1] + for { + if _, taken := out[name]; !taken { + break + } + name = strconv.Quote(name) + } + out[name] = sanitizeJSONValue(val[k]) + } + return out +} + +// isTTY checks if the writer is a terminal. It is a package variable so tests +// can simulate a TTY for in-memory writers. +var isTTY = func(w io.Writer) bool { if f, ok := w.(*os.File); ok { fi, err := f.Stat() if err != nil { @@ -317,6 +403,15 @@ func isTTY(w io.Writer) bool { return false } +// writeJSON emits the full envelope as JSON. Sanitization is TTY-gated, +// matching writeJQ: when the target is a pipe or file, JSON is the +// machine-consumption contract and every byte passes through verbatim so +// data fidelity is preserved for programmatic consumers (FormatAuto only +// selects JSON when stdout is not a TTY, so this is the common case). When +// the target is a terminal — forced --json/--agent on an interactive TTY — +// string leaves are C1/escape-sanitized (via sanitizeJSONValue) so raw C1 +// controls cannot execute on a C1-honoring terminal. Other terminal-facing +// sinks (styled, markdown, --ids, and --jq on a TTY) sanitize the same way. func (w *Writer) writeJSON(v any) error { toEncode := v if resp, ok := v.(*Response); ok { @@ -325,6 +420,29 @@ func (w *Writer) writeJSON(v any) error { respCopy.Data = NormalizeData(resp.Data) toEncode = &respCopy } + // On a TTY, sanitize string leaves to strip C1/escape controls: Go's JSON + // encoder escapes C0 controls but passes UTF-8-encoded C1 controls + // (U+0080–U+009F) through raw. Piped/redirected output stays verbatim. + if isTTY(w.opts.Writer) { + raw, err := json.Marshal(toEncode) + if err != nil { + return err + } + // Decode with UseNumber so JSON numbers stay json.Number (a named + // string type) instead of float64. Large Basecamp IDs (>2^53) would + // otherwise lose precision or render in exponent form. json.Number + // passes through sanitizeJSONValue untouched (its `case string:` does + // not match a named type), so the exact numeric text is preserved. + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + var decoded any + if err := dec.Decode(&decoded); err != nil { + return err + } + enc := json.NewEncoder(w.opts.Writer) + enc.SetIndent("", " ") + return enc.Encode(sanitizeJSONValue(decoded)) + } enc := json.NewEncoder(w.opts.Writer) enc.SetIndent("", " ") return enc.Encode(toEncode) @@ -350,17 +468,29 @@ func (w *Writer) writeIDs(v any) error { case []map[string]any: for _, item := range d { if id, ok := item["id"]; ok { - fmt.Fprintln(w.opts.Writer, id) + fmt.Fprintln(w.opts.Writer, stripIfString(id)) } } case map[string]any: if id, ok := d["id"]; ok { - fmt.Fprintln(w.opts.Writer, id) + fmt.Fprintln(w.opts.Writer, stripIfString(id)) } } return nil } +// stripIfString makes a string value safe for the line-oriented IDs sink: +// escape sequences and control characters are stripped, and remaining +// whitespace (including the newlines and tabs SanitizeTerminal preserves) +// collapses to single spaces so one value cannot span or inject extra +// lines. Non-string values (json.Number ids, etc.) pass through unchanged. +func stripIfString(v any) any { + if s, ok := v.(string); ok { + return sanitizeText(s, true, false) + } + return v +} + func (w *Writer) writeCount(v any) error { resp, ok := v.(*Response) if !ok { @@ -427,12 +557,16 @@ func (w *Writer) writeLiteralMarkdown(v any) error { type ResponseOption func(*Response) // WithSummary adds a summary to the response. +// The value is stored verbatim so machine (JSON) output preserves the +// original content; terminal sinks (styled/markdown renderers, quiet-mode +// stderr diagnostics) sanitize at render time. func WithSummary(s string) ResponseOption { return func(r *Response) { r.Summary = s } } // WithNotice adds an informational notice to the response. // Use this for non-error messages like truncation warnings. +// Like WithSummary, the value is stored verbatim; terminal sinks sanitize. func WithNotice(s string) ResponseOption { return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false } } @@ -536,17 +670,25 @@ func (w *Writer) presentStyledEntity(resp *Response) bool { var out strings.Builder r := NewRenderer(w.opts.Writer, true) - if resp.Summary != "" { - out.WriteString(r.Summary.Render(resp.Summary)) + // sanitizeText (single-line) defends against terminal injection from + // API-controlled summary/notice content and keeps each value on one line. + // Sanitization happens only here at the render sink — machine (JSON) + // output carries these verbatim. + // Sanitize first, then gate: an all-escape summary/notice collapses to "" + // and must emit no blank styled line or trailing spacer. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + out.WriteString(r.Summary.Render(summary)) out.WriteString("\n") } - if resp.Notice != "" { - out.WriteString(r.Hint.Render(resp.Notice)) + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + out.WriteString(r.Hint.Render(notice)) out.WriteString("\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { out.WriteString("\n") } @@ -595,15 +737,20 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool { var out strings.Builder mr := NewMarkdownRenderer(w.opts.Writer) - if resp.Summary != "" { - out.WriteString("## " + resp.Summary + "\n") + // Sink-level ANSI stripping (see presentStyledEntity). Sanitize first, then + // gate: an all-escape summary/notice collapses to "" and must not emit an + // empty "## " heading or "*...*" line. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + out.WriteString("## " + summary + "\n") } - if resp.Notice != "" { - out.WriteString("*" + resp.Notice + "*\n") + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + out.WriteString("*" + notice + "*\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { out.WriteString("\n") } @@ -620,9 +767,9 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool { if len(resp.Breadcrumbs) > 0 { out.WriteString("\n### Hints\n\n") for _, bc := range resp.Breadcrumbs { - line := "- `" + bc.Cmd + "`" + line := "- `" + sanitizeText(bc.Cmd, true, false) + "`" if bc.Description != "" { - line += " — " + bc.Description + line += " — " + sanitizeText(bc.Description, true, false) } out.WriteString(line + "\n") } diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 834c2b9d..1d6c7266 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "math" "strings" "testing" @@ -593,6 +594,47 @@ func TestWriterIDsFormat(t *testing.T) { assert.Equal(t, "123\n456\n", output) } +// TestWriterIDsFormatStripsANSI verifies that string id values (an +// API-controlled raw print sink) are sanitized of terminal escape sequences. +func TestWriterIDsFormatStripsANSI(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatIDs, + Writer: &buf, + }) + + data := []map[string]any{ + {"id": "\x1b]0;evil\x07abc-123"}, + {"id": "\x1b[31m456\x1b[0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "abc-123\n456\n", buf.String()) + assert.NotContains(t, buf.String(), "\x1b") +} + +// TestWriterIDsFormatSingleLine verifies that a string id cannot span or +// inject extra lines into the line-oriented IDs sink: newlines, tabs, and +// carriage returns collapse to single spaces, keeping one id per line. +func TestWriterIDsFormatSingleLine(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatIDs, + Writer: &buf, + }) + + data := []map[string]any{ + {"id": "abc\n999\ndef"}, + {"id": "tab\there"}, + {"id": "cr\rjoined"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "abc 999 def\ntab here\ncr joined\n", buf.String()) +} + func TestWriterCountFormat(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -2267,12 +2309,13 @@ func TestWriterStyledErrorIncludesRequestID(t *testing.T) { Code: basecamp.CodeAPI, Message: "server error", HTTPStatus: 500, - RequestID: "req-cli-123\x1b[31m\nnext", + RequestID: "req-cli-123\x1b[31m\nnext\rword", }) require.NoError(t, writeErr, "Err() failed") output := ansi.Strip(buf.String()) - assert.Contains(t, output, "Request ID: req-cli-123 next") + assert.Contains(t, output, "Request ID: req-cli-123 next word", + "CR separates words instead of joining them") assert.NotContains(t, output, "[31m") } @@ -2341,6 +2384,21 @@ func TestFormatCellStripsANSIEscapes(t *testing.T) { input: "\x1b[1;31mred bold\x1b[0m", expected: "red bold", }, + { + name: "C1 CSI survives ansi.Strip but not SanitizeTerminal", + input: string(rune(0x9b)) + "31mred" + string(rune(0x9b)) + "0m", // U+009B + expected: "31mred0m", + }, + { + name: "C1 OSC with BEL terminator", + input: string(rune(0x9d)) + "0;evil\adone", // U+009D + expected: "0;evildone", + }, + { + name: "DEL character", + input: "De\x7fleted", + expected: "Deleted", + }, } for _, tt := range tests { @@ -2351,6 +2409,65 @@ func TestFormatCellStripsANSIEscapes(t *testing.T) { } } +// TestWriterStyledTableStripsC1Controls verifies that the styled table +// renderer strips UTF-8-encoded C1 controls and DEL from API-controlled data. +// ansi.Strip alone leaves these intact, and C1-honoring terminals execute +// them directly (e.g. `api get` rendering a hostile record title). +func TestWriterStyledTableStripsC1Controls(t *testing.T) { + c1CSI := string(rune(0x9b)) // U+009B CSI + c1OSC := string(rune(0x9d)) // U+009D OSC + + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + + data := []map[string]any{ + {"id": 1, "title": c1CSI + "31mRed alert" + c1CSI + "0m"}, + {"id": 2, "title": c1OSC + "0;evil\aDe\x7fleted"}, + } + require.NoError(t, w.OK(data)) + + out := buf.String() + assert.Contains(t, out, "Red alert") + assert.Contains(t, out, "Deleted") + assert.NotContains(t, out, c1CSI) + assert.NotContains(t, out, c1OSC) + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "\a") +} + +// TestWriterStyledStringDataStripsC1Controls covers renderData's string and +// detail-object paths: C1 controls and DEL in string payloads and object +// values must not reach the terminal. +func TestWriterStyledStringDataStripsC1Controls(t *testing.T) { + c1CSI := string(rune(0x9b)) // U+009B CSI + + t.Run("string data", func(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + require.NoError(t, w.OK(c1CSI+"31mplain string"+c1CSI+"0m")) + + out := buf.String() + assert.Contains(t, out, "plain string") + assert.NotContains(t, out, c1CSI) + }) + + t.Run("detail object", func(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + require.NoError(t, w.OK(map[string]any{ + "id": 1, + "title": c1CSI + "31mdetail value" + c1CSI + "0m", + "note": "De\x7fleted", + })) + + out := buf.String() + assert.Contains(t, out, "detail value") + assert.Contains(t, out, "Deleted") + assert.NotContains(t, out, c1CSI) + assert.NotContains(t, out, "\x7f") + }) +} + // ============================================================================= // WithDisplayData Contract Tests // ============================================================================= @@ -2475,6 +2592,27 @@ func TestFormatCellStripsANSIFromArrayElements(t *testing.T) { result := formatCell(input) assert.Equal(t, "Task One", result) }) + + t.Run("map elements whose only field is an escaped string id", func(t *testing.T) { + // The id arm is the last fallback; a hostile string id must be + // sanitized like its sibling arms rather than printed raw. + input := []any{ + map[string]any{"id": "12\x1b]0;pwned\x07\u009b31m3\x7f"}, + } + result := formatCell(input) + assert.NotContains(t, result, "\x1b", "ESC leaked from string id") + assertNoC1OrDEL(t, result, "formatCell string id") + assert.Contains(t, result, "123", "sanitized id digits should survive") + }) + + t.Run("map elements with non-string id are unaffected", func(t *testing.T) { + input := []any{ + map[string]any{"id": json.Number("456")}, + map[string]any{"id": float64(789)}, + } + result := formatCell(input) + assert.Equal(t, "456, 789", result) + }) } func TestFormatCellCollapsesNewlines(t *testing.T) { @@ -3146,6 +3284,376 @@ func TestWriterJQFilterStringOutput(t *testing.T) { assert.Equal(t, "Buy milk\nShip feature", lines) } +// forceTTY makes isTTY report true for the duration of the test, simulating a +// terminal-attached writer for in-memory buffers. jq output sanitization is +// TTY-gated, so tests exercising the sanitizing path must opt in. +func forceTTY(t *testing.T) { + t.Helper() + orig := isTTY + isTTY = func(io.Writer) bool { return true } + t.Cleanup(func() { isTTY = orig }) +} + +// TestWriterJQFilterStringStripsANSIOnTTY verifies that jq string results are +// sanitized of terminal escape sequences before printing to a TTY. Unlike +// JSON output (which escapes control bytes), string results print raw, so +// --jq selecting an API-controlled string must not emit OSC/CSI sequences to +// the terminal. +func TestWriterJQFilterStringStripsANSIOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + data := []map[string]any{ + {"id": 1, "title": "\x1b]0;evil\x07Set title"}, + {"id": 2, "title": "\x1b[31mRed alert\x1b[0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "Set title\nRed alert", strings.TrimSpace(buf.String())) + assert.NotContains(t, buf.String(), "\x1b") +} + +// TestWriterJQFilterStringStripsC1ControlsOnTTY verifies that UTF-8-encoded +// Unicode C1 controls (which survive ansi.Strip but execute on C1-honoring +// terminals) and DEL are stripped from jq string results printed to a TTY. +func TestWriterJQFilterStringStripsC1ControlsOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + data := []map[string]any{ + {"id": 1, "title": "\u009b31mRed alert\u009b0m"}, // U+009B CSI + {"id": 2, "title": "\u009d0;evil\aSet title"}, // U+009D OSC, BEL-terminated + {"id": 3, "title": "De\u007fleted"}, // DEL + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "31mRed alert0m\n0;evilSet title\nDeleted", strings.TrimSpace(buf.String())) + assert.NotContains(t, buf.String(), "\u009b") + assert.NotContains(t, buf.String(), "\u009d") + assert.NotContains(t, buf.String(), "\u007f") + assert.NotContains(t, buf.String(), "\a") +} + +// TestWriterJQFilterCompoundStripsC1Controls verifies that compound jq +// results (arrays/objects) printed to a TTY are sanitized before marshaling. +// Go's JSON encoder escapes C0 controls but emits UTF-8-encoded C1 controls +// (U+0080–U+009F) raw, so string leaves and map keys must be stripped before +// they reach the terminal. +func TestWriterJQFilterCompoundStripsC1Controls(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + data := []map[string]any{ + {"id": 1, "title": "\u009b31mRed alert\u009b0m"}, // U+009B CSI in value + {"id": 2, "ti\u009dtle": "nested", "note": "\u009cclean"}, // U+009D OSC in key, U+009C ST in value + {"id": 3, "nested": map[string]any{"deep": "\u0090payload\u009c"}}, // C1 in nested object + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + // Raw C1 bytes must not survive (U+009B is 0xC2 0x9B in UTF-8, etc.) + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\u009c") + assert.NotContains(t, out, "\u009d") + assert.NotContains(t, out, "\u0090") + + // Legitimate content survives with structure intact. Keys containing + // controls are visibly escaped rather than stripped, so they can never + // collide with a clean key. + var result []map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 3) + assert.Equal(t, "31mRed alert0m", result[0]["title"]) + assert.Equal(t, "nested", result[1]["ti\\u009dtle"], "escaped map key") + assert.Equal(t, "clean", result[1]["note"]) + nested, ok := result[2]["nested"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "payload", nested["deep"]) +} + +// TestWriterJQFilterCompoundKeyCollision verifies that a hostile map key +// that sanitizes to the same string as a legitimate key cannot replace or +// hide it: clean keys keep their slots and values \u2014 including a literal key +// that mimics escape notation \u2014 and hostile keys are preserved under +// visibly-escaped names, re-quoted until unique, instead of being dropped. +func TestWriterJQFilterCompoundKeyCollision(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + data := map[string]any{ + "title": "legit", + "\x1b[31mtitle": "impostor", + "\u009b31mtitle": "c1 impostor", + "\\x1b[31mtitle": "lookalike", // literal text matching the ESC key's escaped form + } + err := w.OK(data) + require.NoError(t, err) + + var result map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 4, "no entry silently dropped") + assert.Equal(t, "legit", result["title"], "clean key keeps its slot") + assert.Equal(t, "lookalike", result["\\x1b[31mtitle"], "clean lookalike keeps its slot") + assert.Equal(t, "impostor", result["\"\\\\x1b[31mtitle\""], "colliding hostile key re-quoted") + assert.Equal(t, "c1 impostor", result["\\u009b31mtitle"]) + assert.NotContains(t, buf.String(), "\x1b") + assert.NotContains(t, buf.String(), "\u009b") +} + +// TestWriterJQFilterStringVerbatimWhenPiped verifies that jq string results +// written to a non-TTY sink (pipes, files, the default in tests) preserve +// every byte verbatim: non-TTY output is the machine-consumption contract +// (matching writeJSON), and sanitizing would silently corrupt data fidelity. +func TestWriterJQFilterStringVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + c1CSI := string(rune(0x9b)) // U+009B, UTF-8-encoded C1 CSI + data := []map[string]any{ + {"id": 1, "title": "\x1b[31mRed alert\x1b[0m"}, // ESC-prefixed CSI + {"id": 2, "title": c1CSI + "31mStill red" + c1CSI + "0m"}, // C1 CSI + {"id": 3, "title": "De\x7fleted"}, // DEL + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, + "\x1b[31mRed alert\x1b[0m\n"+c1CSI+"31mStill red"+c1CSI+"0m\nDe\x7fleted", + strings.TrimSpace(buf.String())) +} + +// TestWriterJQFilterCompoundVerbatimWhenPiped verifies that compound jq +// results written to a non-TTY sink are marshaled without sanitization — +// string leaves and map keys keep their original bytes. +func TestWriterJQFilterCompoundVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + c1CSI := string(rune(0x9b)) // U+009B, UTF-8-encoded C1 CSI + data := []map[string]any{ + {"id": 1, "title": c1CSI + "31mRed alert" + c1CSI + "0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + var result []map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 1) + assert.Equal(t, c1CSI+"31mRed alert"+c1CSI+"0m", result[0]["title"], + "C1 bytes must round-trip through --jq when piped") +} + +// TestWriterMarkdownEntityBreadcrumbsStripEscapes verifies that the entity +// Markdown sink (presentMarkdownEntity) sanitizes API-controlled breadcrumb +// Cmd/Description before writing them, matching the generic Markdown renderer. +// Markdown output carries no legitimate ANSI, so no escape or C1 byte may +// survive. +func TestWriterMarkdownEntityBreadcrumbsStripEscapes(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatMarkdown, + Writer: &buf, + }) + + data := map[string]any{ + "id": float64(12345), + "content": "Fix the login bug", + "completed": false, + } + err := w.OK(data, + WithEntity("todo"), // routes through presentMarkdownEntity + WithBreadcrumbs(Breadcrumb{ + Action: "show", + Cmd: "basecamp \x1b]0;pwncmd\x07show \u009b31m1", // ESC OSC + C1 CSI + Description: "View details and \x1b[31mmore", // ESC CSI + }), + ) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "Hints", "entity sink should render breadcrumbs") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\u009d") + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "pwncmd", "OSC payload must be stripped") +} + +// TestWriterStyledFormatHeaderStripsControlsInKey verifies that an +// API-controlled DATA map KEY (rendered as a styled header/label via +// formatHeader) cannot smuggle C1 controls, DEL, or an OSC payload to the +// terminal. Styled output legitimately contains ESC-prefixed SGR codes, so +// this asserts on the injection classes that never appear in legitimate +// styling. +func TestWriterStyledFormatHeaderStripsControlsInKey(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatStyled, + Writer: &buf, + }) + + data := map[string]any{ + "na\x1b]0;pwnosc\x07me": "value-osc", // ESC OSC injection in key + "ti\u009b31mtle": "value-c1", // C1 CSI in key + "de\x7fl": "value-del", // DEL in key + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "value-osc", "cell values still render") + assert.NotContains(t, out, "pwnosc", "OSC payload must be stripped from header") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\x7f") +} + +// TestWriterMarkdownFormatHeaderStripsEscapesInKey verifies the same +// formatHeader choke point for the Markdown sink. Markdown carries no +// legitimate ANSI, so neither ESC-prefixed sequences nor C1 controls in a +// map key may reach the rendered label. +func TestWriterMarkdownFormatHeaderStripsEscapesInKey(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatMarkdown, + Writer: &buf, + }) + + data := map[string]any{ + "na\x1b[31mme": "value-esc", // ESC CSI in key + "ti\u009b31mtle": "value-c1", // C1 CSI in key + "de\x7fscr\x1b]0;pwnosc\x07r": "value-osc", // DEL + ESC OSC in key + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "value-esc", "cell values still render") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "pwnosc") +} + +// TestWriterJSONStripsC1OnTTY verifies that forced --json/--agent output to an +// interactive terminal has string leaves sanitized of C1/escape controls: the +// Go JSON encoder escapes C0 controls but emits UTF-8-encoded C1 controls +// (U+0080-U+009F) raw, so a hostile value would otherwise execute on a +// C1-honoring terminal. +func TestWriterJSONStripsC1OnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "title": "\u009b31mRed alert\u009b0m", // C1 CSI + "note": "De\x7fleted", // DEL + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.NotContains(t, out, "\u009b", "C1 CSI must be stripped on a TTY") + assert.NotContains(t, out, "\x7f", "DEL must be stripped on a TTY") + + var resp struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) + assert.Equal(t, "31mRed alert0m", resp.Data["title"]) + assert.Equal(t, "Deleted", resp.Data["note"]) +} + +// TestWriterJSONPreservesLargeIntOnTTY verifies that the TTY-sanitize path +// preserves large integer IDs exactly. The path round-trips through +// json.Marshal → json.Decoder, and decoding must use UseNumber so numbers stay +// json.Number rather than float64 — otherwise IDs above 2^53 would lose +// precision or render in exponent form (e.g. 9.007...e+15). json.Number is a +// named string type, so it does NOT match sanitizeJSONValue's `case string:` +// arm (a type switch matches only the exact string type); it falls through to +// the default case and is returned unchanged, so its exact numeric text survives. +func TestWriterJSONPreservesLargeIntOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "id": 9007199254740993, // 2^53 + 1: not exactly representable as float64 + "title": "\u009b31mRed\u009b0m", + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "9007199254740993", + "large integer ID must survive the TTY round-trip with exact digits") + assert.NotContains(t, out, "9.007", "ID must not render in exponent/float form") + assert.NotContains(t, out, "9007199254740992", "ID must not lose precision") +} + +// TestWriterJSONVerbatimWhenPiped verifies that JSON written to a non-TTY sink +// (pipes, files, the default in tests) preserves every byte verbatim: piped +// JSON is the machine-consumption contract, and sanitizing would silently +// corrupt data fidelity for programmatic consumers. +func TestWriterJSONVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "title": "\u009b31mRed alert\u009b0m", // C1 CSI + "note": "De\x7fleted", // DEL + } + err := w.OK(data) + require.NoError(t, err) + + var resp struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) + assert.Equal(t, "\u009b31mRed alert\u009b0m", resp.Data["title"], + "C1 bytes must round-trip when piped") + assert.Equal(t, "De\x7fleted", resp.Data["note"], "DEL must round-trip when piped") +} + func TestWriterJQFilterSelect(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -3648,3 +4156,328 @@ func TestPluralNoun(t *testing.T) { assert.Equal(t, tt.expected, PluralNoun(tt.input), "PluralNoun(%q)", tt.input) } } + +// hostileText exercises every terminal-injection payload class the renderers +// must neutralize: an ESC-based OSC title set, a UTF-8-encoded C1 CSI color +// pair (survives ansi.Strip), a bare CR (must separate words, not glue them), +// and DEL. +const hostileText = "Evil\x1b]0;pwned\x07 \u009b31mProject\u009b0m\rna\u007fme" + +// allEscapeText is composed entirely of ESC-based escape sequences and bare +// C0/DEL control characters — no printable runes survive sanitization, so +// sanitizeText collapses it to the empty string. Sinks must gate on the +// sanitized value so this emits no blank line, heading, or marker. +const allEscapeText = "\x1b[2J\x1b[0m\x1b]0;x\x07\x08\x7f" + +// TestWithSummaryNoticeVerbatim verifies that summary/notice values are +// stored verbatim — consistent with error/hint/data, the JSON machine +// contract carries original content and sanitization happens only at +// terminal sinks. +func TestWithSummaryNoticeVerbatim(t *testing.T) { + t.Run("WithSummary", func(t *testing.T) { + r := &Response{} + WithSummary(hostileText)(r) + assert.Equal(t, hostileText, r.Summary) + }) + + t.Run("WithNotice", func(t *testing.T) { + r := &Response{} + WithNotice(hostileText)(r) + assert.Equal(t, hostileText, r.Notice) + assert.False(t, r.noticeDiagnostic) + }) + + t.Run("WithDiagnostic", func(t *testing.T) { + r := &Response{} + WithDiagnostic(hostileText)(r) + assert.Equal(t, hostileText, r.Notice) + assert.True(t, r.noticeDiagnostic) + }) +} + +// TestJSONEnvelopeSummaryNoticeVerbatim verifies the machine contract +// end-to-end: the JSON envelope carries summary and notice byte-for-byte. +func TestJSONEnvelopeSummaryNoticeVerbatim(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + err := w.OK(map[string]string{"id": "1"}, WithSummary(hostileText), WithNotice(hostileText)) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded)) + assert.Equal(t, hostileText, decoded["summary"]) + assert.Equal(t, hostileText, decoded["notice"]) +} + +// TestQuietDiagnosticStderrSanitized verifies the quiet-mode stderr +// diagnostic — a terminal sink outside the renderers — strips escape +// sequences and keeps the notice to a single line. +func TestQuietDiagnosticStderrSanitized(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{Format: FormatQuiet, Writer: &stdout, ErrWriter: &stderr}) + + err := w.OK(map[string]string{"id": "1"}, WithDiagnostic(hostileText)) + require.NoError(t, err) + + assert.Equal(t, "notice: Evil 31mProject0m name\n", stderr.String()) +} + +// TestQuietDiagnosticStderrEmptyAfterSanitize verifies that a diagnostic notice +// that collapses to "" after sanitization (all-escape content) emits nothing on +// stderr — no blank "notice: " line. +func TestQuietDiagnosticStderrEmptyAfterSanitize(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{Format: FormatQuiet, Writer: &stdout, ErrWriter: &stderr}) + + require.NoError(t, w.OK(map[string]string{"id": "1"}, WithDiagnostic(allEscapeText))) + + assert.Empty(t, stderr.String(), "all-escape diagnostic must not emit a blank notice line") +} + +// TestSummaryNoticeEmptyAfterSanitizeSinks verifies that summary/notice values +// that collapse to "" after sanitization (all-escape content) leave the four +// single-line sinks — styled and markdown, generic and entity — byte-identical +// to rendering with no summary/notice at all. This pins the sanitize-first, +// gate-on-non-empty rule: no blank styled line, no empty "## " heading or +// "*...*" marker, and no trailing spacer newline. +func TestSummaryNoticeEmptyAfterSanitizeSinks(t *testing.T) { + render := func(format Format, entity bool, withSummaryNotice bool) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + var opts []ResponseOption + if withSummaryNotice { + opts = append(opts, WithSummary(allEscapeText), WithNotice(allEscapeText)) + } + var data any = map[string]string{"id": "1"} + if entity { + data = map[string]any{ + "id": float64(100), + "subject": "Weekly update", + "content": "Body", + "creator": map[string]any{"name": "Bob"}, + "created_at": "2026-03-01T09:00:00Z", + } + opts = append(opts, WithEntity("message")) + } + require.NoError(t, w.OK(data, opts...)) + return buf.String() + } + + cases := []struct { + name string + format Format + entity bool + }{ + {"styled generic", FormatStyled, false}, + {"markdown generic", FormatMarkdown, false}, + {"styled entity", FormatStyled, true}, + {"markdown entity", FormatMarkdown, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + baseline := render(tc.format, tc.entity, false) + hostile := render(tc.format, tc.entity, true) + // Byte-identical output proves no empty styled line, "## " heading, + // "*...*" marker, or trailing spacer newline was emitted for the + // collapsed summary/notice. + assert.Equal(t, baseline, hostile, + "all-escape summary/notice must render identically to no summary/notice") + assertNoC1OrDEL(t, hostile, tc.name) + }) + } +} + +// assertNoC1OrDEL fails if any C1 control (U+0080-U+009F) or DEL leaked into +// output. C1 controls survive ansi.Strip but are executed directly by +// C1-honoring terminals. +func assertNoC1OrDEL(t *testing.T, out, sink string) { + t.Helper() + for _, r := range out { + if (r >= 0x80 && r <= 0x9f) || r == 0x7f { + t.Fatalf("terminal control %U leaked into %s output", r, sink) + } + } +} + +// assertErrorSinksSanitized renders err through the styled, markdown, and +// plain RenderError sinks and asserts no injection payload survives in any +// of them, while the neutralized text (wantFragment) still renders. +func assertErrorSinksSanitized(t *testing.T, err error, wantFragment string) { + t.Helper() + + var styledBuf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &styledBuf}) + require.NoError(t, w.Err(err)) + styled := styledBuf.String() + + var mdBuf bytes.Buffer + w = New(Options{Format: FormatMarkdown, Writer: &mdBuf}) + require.NoError(t, w.Err(err)) + markdown := mdBuf.String() + + // The plain branch is unreachable through Writer formats (FormatStyled + // forces styling), so drive the renderer directly. + e := AsError(err) + var plainBuf bytes.Buffer + renderErr := NewRenderer(&plainBuf, false).RenderError(&plainBuf, &ErrorResponse{Error: e.Message, Hint: e.Hint}) + require.NoError(t, renderErr) + plain := plainBuf.String() + + // Styled output legitimately carries the renderer's own SGR codes, so + // assert on the hostile payload classes: no OSC introducer, no CR, no + // C1/DEL controls. + assert.Contains(t, styled, "\x1b[", "styled output should be styled") + assert.NotContains(t, styled, "\x1b]", "OSC leaked into styled output") + assert.NotContains(t, styled, "\r", "CR leaked into styled output") + assertNoC1OrDEL(t, styled, "styled") + assert.Contains(t, ansi.Strip(styled), wantFragment) + + for sink, out := range map[string]string{"markdown": markdown, "plain": plain} { + assert.NotContains(t, out, "\x1b", "ESC leaked into %s output", sink) + assert.NotContains(t, out, "\r", "CR leaked into %s output", sink) + assertNoC1OrDEL(t, out, sink) + assert.Contains(t, out, wantFragment, sink) + } +} + +// TestRenderErrorSanitizesAmbiguousMatches drives C1/CSI-laden project names +// through an ambiguous-match error, which interpolates them into the hint. +func TestRenderErrorSanitizesAmbiguousMatches(t *testing.T) { + err := ErrAmbiguous("project", []string{hostileText, "Second " + hostileText}) + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// TestRenderErrorSanitizesNotFoundHint drives hostile person names through a +// not-found error, which interpolates them into both message and hint. +func TestRenderErrorSanitizesNotFoundHint(t *testing.T) { + err := ErrNotFoundHint("Person", hostileText, "Use 'basecamp people' to find "+hostileText) + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// TestRenderErrorSanitizesSDKErrorMessage drives a hostile server-supplied +// message through an SDK error. +func TestRenderErrorSanitizesSDKErrorMessage(t *testing.T) { + err := &basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error: " + hostileText, + HTTPStatus: 500, + } + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// assertSinkNeutralized asserts a rendered terminal sink neither leaks a +// terminal-control payload class nor glues CR-separated words, while the +// neutralized hostileText still renders on a single line. Styled output +// legitimately carries the renderer's own SGR codes, so it is checked for the +// hostile payload classes (OSC, CR, C1/DEL) rather than a blanket ESC ban. +func assertSinkNeutralized(t *testing.T, out, sink string, styled bool) { + t.Helper() + if styled { + assert.NotContains(t, out, "\x1b]", "OSC leaked into %s output", sink) + } else { + assert.NotContains(t, out, "\x1b", "ESC leaked into %s output", sink) + } + assert.NotContains(t, out, "\r", "CR leaked into %s output", sink) + assertNoC1OrDEL(t, out, sink) + // The bare CR in hostileText must separate "0m" and "name" with a space + // (not glue them) and must not break the value across lines. + assert.Contains(t, ansi.Strip(out), "31mProject0m name", + "%s: neutralized hostile text must render single-line and un-glued", sink) +} + +// TestSummaryNoticeSinksSanitized drives hostile summary/notice content through +// all four single-line summary/notice sinks — styled and markdown, generic and +// entity presenters — asserting escape/C1/CR payloads are neutralized and that +// a bare CR between words keeps them space-separated on a single line. +func TestSummaryNoticeSinksSanitized(t *testing.T) { + // Notice carries a pure CR-between-words payload to pin the no-glue rule. + const notice = "before\rafter" + + render := func(format Format, entity bool) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + opts := []ResponseOption{WithSummary(hostileText), WithNotice(notice)} + var data any = map[string]string{"id": "1"} + if entity { + data = map[string]any{ + "id": float64(100), + "subject": "Weekly update", + "content": "Body", + "creator": map[string]any{"name": "Bob"}, + "created_at": "2026-03-01T09:00:00Z", + } + opts = append(opts, WithEntity("message")) + } + require.NoError(t, w.OK(data, opts...)) + return buf.String() + } + + cases := []struct { + name string + format Format + entity bool + styled bool + }{ + {"styled generic", FormatStyled, false, true}, + {"markdown generic", FormatMarkdown, false, false}, + {"styled entity", FormatStyled, true, true}, + {"markdown entity", FormatMarkdown, true, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out := render(tc.format, tc.entity) + assertSinkNeutralized(t, out, tc.name, tc.styled) + + stripped := ansi.Strip(out) + assert.Contains(t, stripped, "before after", + "CR-separated notice words must stay space-separated") + assert.NotContains(t, stripped, "beforeafter", + "CR must not glue notice words together") + assert.NotContains(t, out, "before\nafter", + "single-line notice must not break across lines") + }) + } +} + +// TestBreadcrumbSinksSanitized drives hostile breadcrumb Cmd/Description (a +// previously uncovered sink) through the styled and markdown renderers. +func TestBreadcrumbSinksSanitized(t *testing.T) { + render := func(format Format) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + require.NoError(t, w.OK( + map[string]string{"id": "1"}, + WithBreadcrumbs(Breadcrumb{Action: "act", Cmd: hostileText, Description: hostileText}), + )) + return buf.String() + } + + assertSinkNeutralized(t, render(FormatStyled), "styled breadcrumb", true) + assertSinkNeutralized(t, render(FormatMarkdown), "markdown breadcrumb", false) +} + +// TestAttachmentMetaSinksSanitized drives hostile attachment metadata +// (content_type/filesize — a previously uncovered sink) through the styled and +// markdown renderers. +func TestAttachmentMetaSinksSanitized(t *testing.T) { + render := func(format Format) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + data := map[string]any{ + "id": float64(1), + "content_attachments": []any{ + map[string]any{ + "filename": "file.bin", + "content_type": hostileText, + "filesize": hostileText, + }, + }, + } + require.NoError(t, w.OK(data)) + return buf.String() + } + + assertSinkNeutralized(t, render(FormatStyled), "styled attachment meta", true) + assertSinkNeutralized(t, render(FormatMarkdown), "markdown attachment meta", false) +} diff --git a/internal/output/render.go b/internal/output/render.go index 4f903a83..69ec012d 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -9,7 +9,6 @@ import ( "sort" "strings" "time" - "unicode" "charm.land/lipgloss/v2" "charm.land/lipgloss/v2/table" @@ -113,19 +112,27 @@ func terminalInfo(w io.Writer) (width int, isTTY bool) { func (r *Renderer) RenderResponse(w io.Writer, resp *Response) error { var b strings.Builder - // Summary line - if resp.Summary != "" { - b.WriteString(r.Summary.Render(resp.Summary)) + // Summary line. sanitizeText (single-line) guards against terminal + // injection from API-controlled summary/notice content and keeps the + // value on one line: CR is normalized to a separator so words don't glue, + // and embedded newlines/tabs are collapsed. Sanitization happens only here + // at the terminal sink — machine (JSON) output carries these verbatim. + // Sanitize first, then gate on the sanitized value: an all-escape summary + // collapses to "" and must emit no blank styled line or trailing spacer. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + b.WriteString(r.Summary.Render(summary)) b.WriteString("\n") } // Notice (e.g., truncation warning) - if resp.Notice != "" { - b.WriteString(r.Hint.Render(resp.Notice)) + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + b.WriteString(r.Hint.Render(notice)) b.WriteString("\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { b.WriteString("\n") } @@ -159,6 +166,13 @@ func (r *Renderer) RenderResponse(w io.Writer, resp *Response) error { func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { var b strings.Builder + // Error and hint frequently interpolate API-controlled strings + // (project/person names, server messages), so sanitize them for the + // terminal before rendering. sanitizeText normalizes CR first so word + // boundaries survive SanitizeTerminal. + errMsg := sanitizeText(resp.Error, false, true) + hint := sanitizeText(resp.Hint, false, true) + if r.styled { // Create a styled error box with border errorIcon := "✗" @@ -169,7 +183,7 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { // border (2) + padding (2) r.width-4, 40) - errorMsg := wrapText(resp.Error, maxWidth) + errorMsg := wrapText(errMsg, maxWidth) // Build content lines var contentLines []string @@ -179,9 +193,9 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { contentLines = append(contentLines, r.Data.Render(line)) } - if resp.Hint != "" { + if hint != "" { contentLines = append(contentLines, "") - hintMsg := wrapText(resp.Hint, maxWidth) + hintMsg := wrapText(hint, maxWidth) for i, line := range strings.Split(hintMsg, "\n") { if i == 0 { contentLines = append(contentLines, r.Hint.Render("→ "+line)) @@ -208,11 +222,11 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { b.WriteString("\n") } else { // Plain text output (no styling) - b.WriteString("Error: " + resp.Error) + b.WriteString("Error: " + errMsg) b.WriteString("\n") - if resp.Hint != "" { - b.WriteString("Hint: " + resp.Hint) + if hint != "" { + b.WriteString("Hint: " + hint) b.WriteString("\n") } if requestID := errorRequestID(resp); requestID != "" { @@ -258,18 +272,28 @@ func wrappedRequestIDLines(requestID string, width int) []string { } func sanitizeRequestID(requestID string) string { - requestID = ansi.Strip(requestID) - requestID = strings.Map(func(r rune) rune { - switch { - case r == '\n' || r == '\r' || r == '\t': - return ' ' - case unicode.IsControl(r): - return -1 - default: - return r - } - }, requestID) - return strings.Join(strings.Fields(requestID), " ") + return sanitizeText(requestID, true, false) +} + +// sanitizeText prepares an API-controlled string for a terminal sink. +// CR/CRLF are normalized to newlines first — SanitizeTerminal deletes bare +// CR, which would join adjacent words instead of separating them. When +// convertHTML is set, HTML is converted to markdown before sanitization +// because entity decoding (e.g. ›) can reintroduce C1 controls that +// were not present in the raw input; SanitizeTerminal therefore always runs +// last. When singleLine is set, remaining whitespace is collapsed to single +// spaces so the value occupies exactly one output line. +func sanitizeText(s string, singleLine, convertHTML bool) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + s = strings.ReplaceAll(s, "\r", "\n") + if convertHTML && richtext.IsHTML(s) { + s = richtext.HTMLToMarkdown(s) + } + s = richtext.SanitizeTerminal(s) + if singleLine { + s = strings.Join(strings.Fields(s), " ") + } + return s } func escapeMarkdownText(s string) string { @@ -390,7 +414,7 @@ func (r *Renderer) renderData(b *strings.Builder, data any) { } case string: - b.WriteString(r.Data.Render(ansi.Strip(d))) + b.WriteString(r.Data.Render(richtext.SanitizeTerminal(d))) b.WriteString("\n") case nil: @@ -399,7 +423,7 @@ func (r *Renderer) renderData(b *strings.Builder, data any) { default: // Fallback: format as string - b.WriteString(r.Data.Render(ansi.Strip(fmt.Sprintf("%v", data)))) + b.WriteString(r.Data.Render(richtext.SanitizeTerminal(fmt.Sprintf("%v", data)))) b.WriteString("\n") } } @@ -766,7 +790,7 @@ func topLevelAttachmentSections(data map[string]any) []attachmentSection { func attachmentDisplayName(att map[string]any) string { for _, key := range []string{"filename", "caption", "path", "url", "sgid"} { if value, ok := att[key].(string); ok && value != "" { - return ansi.Strip(value) + return richtext.SanitizeTerminal(value) } } return "attachment" @@ -775,19 +799,19 @@ func attachmentDisplayName(att map[string]any) string { func attachmentDisplayMeta(att map[string]any) string { var parts []string if contentType, ok := att["content_type"].(string); ok && contentType != "" { - parts = append(parts, contentType) + parts = append(parts, sanitizeText(contentType, true, false)) } if filesize, ok := att["filesize"].(string); ok && filesize != "" { - parts = append(parts, filesize+" bytes") + parts = append(parts, sanitizeText(filesize, true, false)+" bytes") } if path, ok := att["path"].(string); ok && path != "" { - parts = append(parts, "saved to "+path) + parts = append(parts, "saved to "+sanitizeText(path, true, false)) } if status, ok := att["download_status"].(string); ok && status != "" { - parts = append(parts, status) + parts = append(parts, sanitizeText(status, true, false)) } if errText, ok := att["download_error"].(string); ok && errText != "" { - parts = append(parts, "error: "+errText) + parts = append(parts, "error: "+sanitizeText(errText, true, false)) } return strings.Join(parts, " · ") } @@ -795,11 +819,11 @@ func attachmentDisplayMeta(att map[string]any) string { func commentCreatorName(comment map[string]any) string { if creator, ok := comment["creator"].(map[string]any); ok { if name, ok := creator["name"].(string); ok && name != "" { - return ansi.Strip(name) + return richtext.SanitizeTerminal(name) } } if name, ok := comment["creator_name"].(string); ok && name != "" { - return ansi.Strip(name) + return richtext.SanitizeTerminal(name) } return "Unknown" } @@ -810,13 +834,7 @@ func commentTimestamp(comment map[string]any) string { func commentBody(comment map[string]any) string { content, _ := comment["content"].(string) - content = ansi.Strip(content) - if richtext.IsHTML(content) { - content = richtext.HTMLToMarkdown(content) - } - content = strings.ReplaceAll(content, "\r\n", "\n") - content = strings.ReplaceAll(content, "\r", "\n") - return strings.TrimSpace(content) + return strings.TrimSpace(sanitizeText(content, false, true)) } func (r *Renderer) renderCommentsSection(b *strings.Builder, comments []map[string]any) { @@ -962,9 +980,9 @@ func (r *Renderer) renderBreadcrumbs(b *strings.Builder, crumbs []Breadcrumb) { b.WriteString(r.Muted.Render("Hints:")) b.WriteString("\n") for _, bc := range crumbs { - line := r.Data.Render(" " + bc.Cmd) + line := r.Data.Render(" " + sanitizeText(bc.Cmd, true, false)) if bc.Description != "" { - line += r.Subtle.Render(" # " + bc.Description) + line += r.Subtle.Render(" # " + sanitizeText(bc.Description, true, false)) } b.WriteString(line + "\n") } @@ -981,6 +999,11 @@ func (r *Renderer) renderStats(b *strings.Builder, stats map[string]any) { } func formatHeader(key string) string { + // Keys can be API-controlled response/field keys (see detectColumns and + // renderObject callers), so strip terminal escapes before they reach a + // styled or markdown header/label. sanitizeText (single-line) mirrors the + // treatment of cell values; literal, clean keys are unchanged. + key = sanitizeText(key, true, false) key = strings.ReplaceAll(key, "_", " ") key = strings.TrimSuffix(key, " on") key = strings.TrimSuffix(key, " at") @@ -999,10 +1022,7 @@ func formatCell(val any) string { case nil: return "" case string: - v = ansi.Strip(v) - if richtext.IsHTML(v) { - v = richtext.HTMLToMarkdown(v) - } + v = sanitizeText(v, false, true) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1035,7 +1055,7 @@ func formatCell(val any) string { for _, item := range v { switch elem := item.(type) { case string: - s := ansi.Strip(elem) + s := richtext.SanitizeTerminal(elem) if strings.ContainsAny(s, "\n\r") { s = strings.Join(strings.Fields(s), " ") } @@ -1056,27 +1076,34 @@ func formatCell(val any) string { // from the calendar/reports API response and use summary as their // display name (Schedule::Entry#title delegates to summary in bc3). if filename, ok := elem["filename"].(string); ok && filename != "" { - items = append(items, ansi.Strip(filename)) + items = append(items, richtext.SanitizeTerminal(filename)) } else if caption, ok := elem["caption"].(string); ok && caption != "" { - items = append(items, ansi.Strip(caption)) + items = append(items, richtext.SanitizeTerminal(caption)) } else if name, ok := elem["name"].(string); ok && name != "" { - items = append(items, ansi.Strip(name)) + items = append(items, richtext.SanitizeTerminal(name)) } else if title, ok := elem["title"].(string); ok && title != "" { - items = append(items, ansi.Strip(title)) + items = append(items, richtext.SanitizeTerminal(title)) } else if summary, ok := elem["summary"].(string); ok && summary != "" { - items = append(items, ansi.Strip(summary)) + items = append(items, richtext.SanitizeTerminal(summary)) } else if path, ok := elem["path"].(string); ok && path != "" { - items = append(items, ansi.Strip(path)) + items = append(items, richtext.SanitizeTerminal(path)) } else if id, ok := elem["id"]; ok { - items = append(items, fmt.Sprintf("%v", id)) + // String ids are API-controlled and reach a terminal sink, + // so sanitize them like the sibling arms. Non-string ids + // (e.g. json.Number) carry no escape bytes and are safe. + if s, ok := id.(string); ok { + items = append(items, richtext.SanitizeTerminal(s)) + } else { + items = append(items, fmt.Sprintf("%v", id)) + } } default: - items = append(items, ansi.Strip(fmt.Sprintf("%v", item))) + items = append(items, richtext.SanitizeTerminal(fmt.Sprintf("%v", item))) } } return strings.Join(items, ", ") default: - return ansi.Strip(fmt.Sprintf("%v", v)) + return richtext.SanitizeTerminal(fmt.Sprintf("%v", v)) } } @@ -1103,10 +1130,7 @@ func formatDetailValue(key string, val any) string { case nil: return "" case string: - v = ansi.Strip(v) - if richtext.IsHTML(v) { - v = richtext.HTMLToMarkdown(v) - } + v = sanitizeText(v, false, true) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1195,17 +1219,24 @@ func NewMarkdownRenderer(w io.Writer) *MarkdownRenderer { func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { var b strings.Builder - // Summary as heading - if resp.Summary != "" { - b.WriteString("## " + resp.Summary + "\n") + // Summary as heading. sanitizeText (single-line) guards against terminal + // injection and keeps the heading/notice on one line. Sanitization happens + // only at terminal sinks like this one — machine (JSON) output carries + // summary/notice verbatim. + // Sanitize first, then gate: an all-escape summary/notice collapses to "" + // and must not emit an empty "## " heading or "*...*" line. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + b.WriteString("## " + summary + "\n") } // Notice (e.g., truncation warning) - if resp.Notice != "" { - b.WriteString("*" + resp.Notice + "*\n") + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + b.WriteString("*" + notice + "*\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { b.WriteString("\n") } @@ -1217,9 +1248,9 @@ func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { if len(resp.Breadcrumbs) > 0 { b.WriteString("\n### Hints\n\n") for _, bc := range resp.Breadcrumbs { - line := "- `" + bc.Cmd + "`" + line := "- `" + sanitizeText(bc.Cmd, true, false) + "`" if bc.Description != "" { - line += " — " + bc.Description + line += " — " + sanitizeText(bc.Description, true, false) } b.WriteString(line + "\n") } @@ -1239,9 +1270,11 @@ func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { func (r *MarkdownRenderer) RenderError(w io.Writer, resp *ErrorResponse) error { var b strings.Builder - b.WriteString("**Error:** " + resp.Error + "\n") - if resp.Hint != "" { - b.WriteString("\n*Hint: " + resp.Hint + "*\n") + // Error and hint interpolate API-controlled strings; sanitize for the + // terminal (see (*Renderer).RenderError). + b.WriteString("**Error:** " + sanitizeText(resp.Error, false, true) + "\n") + if hint := sanitizeText(resp.Hint, false, true); hint != "" { + b.WriteString("\n*Hint: " + hint + "*\n") } if requestID := sanitizeRequestID(errorRequestID(resp)); requestID != "" { b.WriteString("\nRequest ID: " + escapeMarkdownText(requestID) + "\n") @@ -1275,13 +1308,13 @@ func (r *MarkdownRenderer) renderData(b *strings.Builder, data any) { } case string: - b.WriteString(ansi.Strip(d) + "\n") + b.WriteString(richtext.SanitizeTerminal(d) + "\n") case nil: b.WriteString("*No data*\n") default: - fmt.Fprintf(b, "%v\n", ansi.Strip(fmt.Sprintf("%v", data))) + fmt.Fprintf(b, "%v\n", richtext.SanitizeTerminal(fmt.Sprintf("%v", data))) } } diff --git a/internal/presenter/format.go b/internal/presenter/format.go index 55dc9282..2126eae6 100644 --- a/internal/presenter/format.go +++ b/internal/presenter/format.go @@ -52,6 +52,11 @@ func formatDate(val any, locale Locale) string { if !ok || str == "" { return "" } + // Strip terminal escape sequences up front so a timestamp with embedded + // escapes still parses, and the fallback below is already sanitized. A date + // value renders in a single-line detail field, so collapse embedded + // CR/newlines/tabs too — a malformed value can't break the row. + str = richtext.SanitizeSingleLine(str) // Try ISO8601 full timestamp if t, err := time.Parse(time.RFC3339, str); err == nil { @@ -71,6 +76,11 @@ func formatRelativeTime(val any, locale Locale) string { if !ok || str == "" { return "" } + // Strip terminal escape sequences up front so a timestamp with embedded + // escapes still parses, and the fallback below is already sanitized. A + // relative-time value renders in a single-line detail field, so collapse + // embedded CR/newlines/tabs too — a malformed value can't break the row. + str = richtext.SanitizeSingleLine(str) t, err := time.Parse(time.RFC3339, str) if err != nil { @@ -129,7 +139,12 @@ func formatPeople(val any) string { for _, item := range arr { if m, ok := item.(map[string]any); ok { if name, ok := m["name"].(string); ok { - names = append(names, name) + // Names render in a single-line detail row (e.g. todo.assignees), + // so collapse embedded CR/newlines/tabs to spaces rather than + // letting a crafted name break the row or glue words together. + if stripped := richtext.SanitizeSingleLine(name); stripped != "" { + names = append(names, stripped) + } } } } @@ -168,7 +183,11 @@ func parseDockItems(val any) []dockItem { result := make([]dockItem, len(items)) for i, m := range items { title, _ := m["title"].(string) + // A dock title/name occupies one line of the dock listing, so collapse + // embedded CR/newlines/tabs to spaces to keep each row intact. + title = richtext.SanitizeSingleLine(title) name, _ := m["name"].(string) + name = richtext.SanitizeSingleLine(name) if title == "" { title = name } @@ -263,7 +282,9 @@ func dockPosition(m map[string]any) int { func formatPerson(val any) string { if m, ok := val.(map[string]any); ok { if name, ok := m["name"].(string); ok { - return name + // A person name is a single key/value detail row value, so collapse + // embedded CR/newlines/tabs to spaces to keep the row on one line. + return richtext.SanitizeSingleLine(name) } } return "" @@ -295,8 +316,14 @@ func formatText(val any) string { case nil: return "" case string: + // Strip terminal escape sequences from API-controlled strings before + // they reach a styled/markdown sink (terminal injection defense). + v = richtext.SanitizeTerminal(v) if richtext.IsHTML(v) { - return richtext.HTMLToMarkdown(v) + // Defense-in-depth: strip the generated markdown too before it can + // reach a styled/markdown sink. The input is already stripped above + // and HTMLToMarkdown never emits ESC, so this is belt-and-suspenders. + return richtext.SanitizeTerminal(richtext.HTMLToMarkdown(v)) } return v case bool: diff --git a/internal/presenter/format_test.go b/internal/presenter/format_test.go index 416bb55a..71a6a8ad 100644 --- a/internal/presenter/format_test.go +++ b/internal/presenter/format_test.go @@ -2,7 +2,9 @@ package presenter import ( "encoding/json" + "strings" "testing" + "time" ) func TestFormatDock(t *testing.T) { @@ -133,3 +135,232 @@ func TestFormatDockTitleFallsBackToName(t *testing.T) { t.Errorf("formatDock(title fallback) = %q, want %q", got, want) } } + +func TestFormatDockStripsANSIFromTitleAndName(t *testing.T) { + dock := []any{ + map[string]any{ + "title": "\x1b]8;;http://evil.example\x07To-dos\x1b]8;;\x07", + "name": "\x1b[31mtodoset\x1b[0m", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 To-dos (todoset)" + if got != want { + t.Errorf("formatDock(ANSI in title/name) = %q, want %q", got, want) + } +} + +func TestFormatDockTitleFallbackStripsANSIFromName(t *testing.T) { + dock := []any{ + map[string]any{ + "name": "\x1b[31mtodoset\x1b[0m", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 todoset" + if got != want { + t.Errorf("formatDock(ANSI in name fallback) = %q, want %q", got, want) + } +} + +func TestFormatPeopleStripsANSI(t *testing.T) { + people := []any{ + map[string]any{"name": "\x1b[31mAlice\x1b[0m"}, + map[string]any{"name": "Bob"}, + } + + got := formatPeople(people) + want := "Alice, Bob" + if got != want { + t.Errorf("formatPeople(ANSI names) = %q, want %q", got, want) + } +} + +// TestFormatPeopleStripsC1Controls covers UTF-8-encoded Unicode C1 controls +// (U+0080-U+009F) and DEL, which survive ansi.Strip but are executed by +// C1-honoring terminals. +func TestFormatPeopleStripsC1Controls(t *testing.T) { + people := []any{ + map[string]any{"name": "\u009b31mAlice\u009b0m"}, // U+009B CSI + map[string]any{"name": "\u009d0;evil\aBob"}, // U+009D OSC, BEL-terminated + map[string]any{"name": "Ca\u007frol"}, // DEL + } + + got := formatPeople(people) + want := "31mAlice0m, 0;evilBob, Carol" + if got != want { + t.Errorf("formatPeople(C1 names) = %q, want %q", got, want) + } +} + +func TestFormatDateStripsANSIFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} { + got := formatDate(in, locale) + if strings.ContainsRune(got, 0x1b) { + t.Errorf("formatDate(%q) = %q, want no escape byte", in, got) + } + } +} + +// TestFormatDateStripsC1ControlsFromUnparseableInput covers UTF-8-encoded C1 +// controls and DEL, which survive ansi.Strip but execute on C1-honoring +// terminals. +func TestFormatDateStripsC1ControlsFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\u009b31mX", "\u009d0;pwn\abad", "not-a-date\u007fX"} { + got := formatDate(in, locale) + for _, r := range got { + if r < 0x20 && r != '\n' && r != '\t' || r == 0x7f || (r >= 0x80 && r <= 0x9f) { + t.Errorf("formatDate(%q) = %q, want no control rune U+%04X", in, got, r) + } + } + } +} + +func TestFormatRelativeTimeStripsANSIFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} { + got := formatRelativeTime(in, locale) + if strings.ContainsRune(got, 0x1b) { + t.Errorf("formatRelativeTime(%q) = %q, want no escape byte", in, got) + } + } +} + +func TestFormatDateParsesTimestampWithEmbeddedANSI(t *testing.T) { + locale := NewLocale("") + want := formatDate("2026-07-01T12:00:00Z", locale) + for _, in := range []string{ + "\x1b[31m2026-07-01T12:00:00Z\x1b[0m", + "2026-07-01T\x1b[31m12:00:00Z", + } { + got := formatDate(in, locale) + if got != want { + t.Errorf("formatDate(%q) = %q, want formatted date %q", in, got, want) + } + } + + wantDateOnly := formatDate("2026-07-01", locale) + in := "\x1b[31m2026-07-01\x1b[0m" + if got := formatDate(in, locale); got != wantDateOnly { + t.Errorf("formatDate(%q) = %q, want formatted date %q", in, got, wantDateOnly) + } +} + +func TestFormatRelativeTimeParsesTimestampWithEmbeddedANSI(t *testing.T) { + locale := NewLocale("") + clean := time.Now().Add(-2 * time.Hour).Format(time.RFC3339) + want := formatRelativeTime(clean, locale) + for _, in := range []string{ + "\x1b[31m" + clean + "\x1b[0m", + clean[:10] + "\x1b[31m" + clean[10:], + } { + got := formatRelativeTime(in, locale) + if got != want { + t.Errorf("formatRelativeTime(%q) = %q, want relative time %q", in, got, want) + } + } +} + +func TestFormatPeopleDropsAllEscapeName(t *testing.T) { + people := []any{ + map[string]any{"name": "Alice"}, + map[string]any{"name": "\x1b[0m\x1b]8;;\x07"}, + } + + got := formatPeople(people) + want := "Alice" + if got != want { + t.Errorf("formatPeople(all-escape name dropped) = %q, want %q", got, want) + } +} + +// assertSingleLine fails if s contains a newline, carriage return, or tab — +// i.e. if a single-line sink leaked whitespace that could break its row. +func assertSingleLine(t *testing.T, label, s string) { + t.Helper() + if strings.ContainsAny(s, "\n\r\t") { + t.Errorf("%s = %q, want single line (no \\n/\\r/\\t)", label, s) + } +} + +// formatPeople feeds a single-line detail row (e.g. todo.assignees), so a name +// with a bare CR plus embedded newline/tab must collapse to one line with the +// CR acting as a word separator rather than gluing words together. +func TestFormatPeopleCollapsesWhitespace(t *testing.T) { + people := []any{ + map[string]any{"name": "Ann\rBob"}, + map[string]any{"name": "Carol\nDave\tErin"}, + } + + got := formatPeople(people) + want := "Ann Bob, Carol Dave Erin" + if got != want { + t.Errorf("formatPeople(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatPeople", got) +} + +// formatPerson is a single key/value detail row value, so embedded +// CR/newlines/tabs collapse to spaces. +func TestFormatPersonCollapsesWhitespace(t *testing.T) { + got := formatPerson(map[string]any{"name": "Ann\rBob\nCarol\tDave"}) + want := "Ann Bob Carol Dave" + if got != want { + t.Errorf("formatPerson(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatPerson", got) +} + +// A dock title/name occupies one line of the dock listing, so embedded +// CR/newlines/tabs must collapse rather than break the row layout. +func TestFormatDockCollapsesWhitespaceInTitleAndName(t *testing.T) { + dock := []any{ + map[string]any{ + "title": "To\rdos\nList", + "name": "todo\tset", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 To dos List (todo set)" + if got != want { + t.Errorf("formatDock(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatDock", got) +} + +// A date value renders in a single-line detail field; an unparseable value with +// embedded CR/newlines/tabs falls back to the sanitized string, which must stay +// on one line. +func TestFormatDateCollapsesWhitespaceInUnparseableInput(t *testing.T) { + locale := NewLocale("") + got := formatDate("not-a-date\rmore\nstuff\there", locale) + want := "not-a-date more stuff here" + if got != want { + t.Errorf("formatDate(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatDate", got) +} + +// A relative-time value renders in a single-line detail field; an unparseable +// value with embedded CR/newlines/tabs falls back to the sanitized string, +// which must stay on one line. +func TestFormatRelativeTimeCollapsesWhitespaceInUnparseableInput(t *testing.T) { + locale := NewLocale("") + got := formatRelativeTime("not-a-date\rmore\nstuff\there", locale) + want := "not-a-date more stuff here" + if got != want { + t.Errorf("formatRelativeTime(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatRelativeTime", got) +} diff --git a/internal/presenter/presenter_test.go b/internal/presenter/presenter_test.go index 8b256f7e..7396297d 100644 --- a/internal/presenter/presenter_test.go +++ b/internal/presenter/presenter_test.go @@ -324,6 +324,25 @@ func TestRenderTemplateLargeID(t *testing.T) { } } +func TestRenderTemplateSanitizesSingleLine(t *testing.T) { + // RenderTemplate feeds single-line sinks (headlines, affordance commands). + // An API value with a bare CR between words, embedded \n/\t, and a C1/ESC + // sequence must collapse to a single space-separated line with no control + // or escape bytes — a bare CR must NOT glue words together. + data := map[string]any{"content": "alpha\rbeta\ngamma\tdelta\x1b[31m\x9b0m"} + + got := RenderTemplate("{{.content}}", data) + + for _, r := range []rune{'\n', '\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(got, r) { + t.Errorf("RenderTemplate output %q still contains control/escape byte %q", got, r) + } + } + if got != "alpha beta gamma delta" { + t.Errorf("RenderTemplate = %q, want words space-separated (not glued): %q", got, "alpha beta gamma delta") + } +} + func TestEvalCondition(t *testing.T) { data := map[string]any{"completed": false} @@ -1617,6 +1636,217 @@ func TestRenderTaskItemCommaInAssigneeName(t *testing.T) { } } +func TestExtractPeopleNamesStripsANSI(t *testing.T) { + // Person names come from the API and must not carry ESC/OSC sequences + // into terminal output via the markdown task-list path. + val := []any{ + map[string]any{"name": "\x1b]0;pwn\x07Bob"}, + map[string]any{"name": "\x1b[31mAlice"}, + } + names := extractPeopleNames(val) + if len(names) != 2 { + t.Fatalf("Expected 2 names, got %d: %v", len(names), names) + } + if names[0] != "Bob" { + t.Errorf("names[0] = %q, want %q", names[0], "Bob") + } + if names[1] != "Alice" { + t.Errorf("names[1] = %q, want %q", names[1], "Alice") + } + for _, n := range names { + if strings.ContainsRune(n, '\x1b') { + t.Errorf("name %q still contains an escape sequence", n) + } + } +} + +func TestExtractPeopleNamesCollapsesToSingleLine(t *testing.T) { + // A person name carrying a bare CR between words plus embedded \n/\t and a + // C1/ESC sequence must collapse to one space-separated line — the bare CR + // must NOT glue words together, and no control/escape bytes may survive. + val := []any{ + map[string]any{"name": "Ada\rLovelace\nGrace\tHopper\x1b[31m\x9b"}, + } + names := extractPeopleNames(val) + if len(names) != 1 { + t.Fatalf("Expected 1 name, got %d: %v", len(names), names) + } + if names[0] != "Ada Lovelace Grace Hopper" { + t.Errorf("names[0] = %q, want words space-separated (not glued): %q", names[0], "Ada Lovelace Grace Hopper") + } + for _, r := range []rune{'\n', '\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(names[0], r) { + t.Errorf("name %q still contains control/escape byte %q", names[0], r) + } + } +} + +func TestRenderTaskItemStripsANSIInAssigneeName(t *testing.T) { + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Review PR", + "completed": false, + "due_on": "", + "assignees": []any{map[string]any{"name": "\x1b]0;pwn\x07Bob"}}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown task-list output should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "@Bob") { + t.Errorf("Should render stripped assignee name '@Bob', got:\n%s", out) + } +} + +func TestRenderListMarkdownStripsANSIInGroupHeading(t *testing.T) { + // Group headings come from a raw API string (bucket.name) and must not + // carry ESC/OSC sequences into the markdown task-list output. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b]0;pwn\x07Project Alpha"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b[31mProject Beta"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown group heading should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "## Project Alpha") { + t.Errorf("Should render stripped heading '## Project Alpha', got:\n%s", out) + } + if !strings.Contains(out, "## Project Beta") { + t.Errorf("Should render stripped heading '## Project Beta', got:\n%s", out) + } +} + +func TestRenderListMarkdownGroupHeadingAllEscapeFallsBackToOther(t *testing.T) { + // A group name that is entirely ESC/OSC sequences strips to "". The + // empty-check must run AFTER richtext.SanitizeTerminal so it falls back + // to "Other" instead of emitting a blank "## " heading. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b]0;pwn\x07\x1b[31m\x1b[2J"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Real Project"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown group heading should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "## Other") { + t.Errorf("All-escape group name should render under '## Other', got:\n%s", out) + } + if strings.Contains(out, "## \n") { + t.Errorf("Should not emit a blank '## ' heading, got:\n%q", out) + } +} + +func TestRenderListMarkdownGroupHeadingCollapsesToSingleLine(t *testing.T) { + // A group name (bucket.name) with a bare CR between words plus embedded + // \n/\t and a C1/ESC sequence must collapse to a single "## " heading on + // one line: the bare CR must not glue words, embedded newlines must not + // spill the heading across lines, and no control/escape bytes may survive. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + // Two buckets so headings are not suppressed (single-group runs hide them). + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Project\rAlpha\nTeam\tOne\x1b[31m\x9b0m"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Second Bucket"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + for _, r := range []rune{'\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(out, r) { + t.Errorf("Markdown group heading should not contain control/escape byte %q, got:\n%q", r, out) + } + } + if !strings.Contains(out, "## Project Alpha Team One\n") { + t.Errorf("Should render collapsed single-line heading '## Project Alpha Team One', got:\n%q", out) + } +} + +func TestRenderListMarkdownChokepointStripsANSI(t *testing.T) { + // Defense-in-depth: an escape injected into any API field on the literal + // markdown path must be absent from the final rendered markdown. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Innocuous \x1b]8;;http://evil\x07title", "completed": false, "due_on": "", + "assignees": []any{map[string]any{"name": "\x1b[31mMallory"}}, + "bucket": map[string]any{"name": "\x1b[2JGroup"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + if strings.ContainsRune(buf.String(), '\x1b') { + t.Errorf("Final markdown must contain no escape bytes, got:\n%q", buf.String()) + } +} + // ============================================================================= // HTML → Markdown Conversion Tests // ============================================================================= @@ -1731,6 +1961,83 @@ func TestRenderHeadlineHTMLPreservesLiteralAsterisks(t *testing.T) { } } +func TestRenderHeadlineHTMLStripsEscape(t *testing.T) { + schema := &EntitySchema{ + Identity: Identity{Label: "content"}, + } + // An ESC embedded in the HTML content must not survive into the rendered + // headline: it is stripped before reaching the styled (Primary.Render) sink. + data := map[string]any{ + "content": "

danger\x1b[31mred\x1b[0m

", + } + got := RenderHeadline(schema, data) + if strings.Contains(got, "\x1b") { + t.Errorf("RenderHeadline must strip ESC from HTML content, got: %q", got) + } +} + +// The identity-label fallback in renderHeadlineRaw formats a raw data value +// without going through RenderTemplate, so a plain (non-HTML) value with a bare +// CR plus embedded newline/tab must collapse to a single-line headline rather +// than break across lines or glue words together. +func TestRenderHeadlineCollapsesWhitespace(t *testing.T) { + schema := &EntitySchema{ + Identity: Identity{Label: "content"}, + } + data := map[string]any{ + "content": "Line one\rLine two\ttabbed\nlast", + } + got := RenderHeadline(schema, data) + want := "Line one Line two tabbed last" + if got != want { + t.Errorf("RenderHeadline(embedded whitespace) = %q, want %q", got, want) + } + if strings.ContainsAny(got, "\n\r\t") { + t.Errorf("RenderHeadline = %q, want single line (no \\n/\\r/\\t)", got) + } +} + +// Guard: renderTaskListMarkdown emits a genuinely multi-line document (one line +// per task item). It is intentionally left on SanitizeTerminal — a blanket +// conversion to SanitizeSingleLine would collapse the whole list to one line. +// This test fails loudly if that ever happens. +func TestRenderTaskListMarkdownPreservesNewlines(t *testing.T) { + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + {"content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}}, + {"content": "Write tests", "completed": true, "due_on": "", "assignees": []any{}}, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + out := buf.String() + + if !strings.Contains(out, "- [ ] Fix bug\n") { + t.Errorf("task list should keep the first item on its own line, got:\n%q", out) + } + if !strings.Contains(out, "- [x] Write tests\n") { + t.Errorf("task list should keep the second item on its own line, got:\n%q", out) + } + if strings.Count(out, "\n") < 2 { + t.Errorf("multi-line task list collapsed to a single line, got:\n%q", out) + } +} + +func TestFormatTextHTMLStripsEscape(t *testing.T) { + // An ESC embedded in HTML content must not survive HTMLToMarkdown into the + // returned string, which can reach a styled/markdown sink. + got := formatText("

danger\x1b[31mred\x1b[0m

") + if strings.Contains(got, "\x1b") { + t.Errorf("formatText must strip ESC from HTML content, got: %q", got) + } +} + func TestRenderTaskItemHTMLContent(t *testing.T) { schema := LookupByName("todo") if schema == nil { diff --git a/internal/presenter/render.go b/internal/presenter/render.go index 377ab7cf..e0bab283 100644 --- a/internal/presenter/render.go +++ b/internal/presenter/render.go @@ -335,6 +335,8 @@ func renderAffordances(b *strings.Builder, schema *EntitySchema, data map[string b.WriteString("\n") // Find max command width for alignment + // RenderTemplate strips terminal escapes from the interpolated, API-controlled + // command (centralized in template.go). maxCmd := 0 renderedCmds := make([]string, len(visible)) for i, a := range visible { @@ -548,7 +550,12 @@ func renderTaskListMarkdown(w io.Writer, schema *EntitySchema, data []map[string if i > 0 { b.WriteString("\n") } - heading := g.name + // Group headings come from a raw API string (e.g. bucket.name via + // extractDotPath) that never passes through formatText. Sanitize for + // a single-line sink before the empty-check so an all-escape-sequence + // name falls back to "Other" instead of emitting a blank "## ", and + // so an embedded CR/newline/tab can't break the heading across lines. + heading := richtext.SanitizeSingleLine(g.name) if heading == "" { heading = "Other" } @@ -559,7 +566,11 @@ func renderTaskListMarkdown(w io.Writer, schema *EntitySchema, data []map[string } } - _, err := io.WriteString(w, b.String()) + // Defense-in-depth: this is the literal-markdown chokepoint. Output is plain + // generated markdown that never legitimately carries ANSI escape bytes + // (styling lives on the separate lipgloss path), so a final strip closes any + // future API-controlled sink without harming legitimate content. + _, err := io.WriteString(w, richtext.SanitizeTerminal(b.String())) return err } @@ -645,7 +656,9 @@ func extractPeopleNames(val any) []string { for _, item := range arr { if m, ok := item.(map[string]any); ok { if name, ok := m["name"].(string); ok && name != "" { - names = append(names, name) + if s := richtext.SanitizeSingleLine(name); s != "" { + names = append(names, s) + } } } } diff --git a/internal/presenter/template.go b/internal/presenter/template.go index b9809574..474ce3d0 100644 --- a/internal/presenter/template.go +++ b/internal/presenter/template.go @@ -34,7 +34,13 @@ func RenderTemplate(tmpl string, data map[string]any) string { if err := t.Execute(&buf, sanitizeNumericValues(data)); err != nil { return "