From 10057f5d4e8f3781b9275fb79de4666b34f055d3 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 29 May 2026 22:55:22 -0700 Subject: [PATCH 1/6] Reject foreign-host URLs in api command to prevent token leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parsePath now validates absolute URLs against the configured base URL host: a path is accepted only if it's relative or an absolute Basecamp URL on the same host (path extracted, account segment dropped — the SDK re-prefixes the configured account). Absolute URLs on any other host are rejected before any network call, so the SDK never attaches the Authorization: Bearer header to a foreign host. A stray leading slash ('/https://evil/…') and mixed-case schemes ('HTTPS://…') are normalized first so neither can smuggle an absolute URL past the host check. Error propagated through get/post/put/delete; api_test.go covers same-host extraction, query preservation, and foreign-host rejection. --- internal/commands/api.go | 115 +++++++++++++++++++++++++++++----- internal/commands/api_test.go | 92 ++++++++++++++++++++++++++- 2 files changed, 190 insertions(+), 17 deletions(-) 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) }) } } From 18469ebf8049613b5b43d6abf9172cd5dc045fea Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 29 May 2026 22:55:28 -0700 Subject: [PATCH 2/6] Strip ANSI/OSC sequences from API-controlled output API-controlled strings (names, summaries, notices, headlines, affordance commands) reached styled/markdown sinks and the --watch TUI without ANSI stripping, allowing terminal/OSC injection. Sanitize WithSummary/WithNotice/ WithDiagnostic at the source, the timeline watch TUI fields (creator name stripped before the empty check so an all-escape name still falls back to a placeholder), and presenter format helpers, with defense-in-depth at the render sinks. Stripping is centralized in RenderTemplate + RenderHeadline so schema headlines ({{.name}}/{{.subject}}/{{.content}}) and affordance commands are all covered, including the identity-label headline fallback. --- internal/commands/timeline.go | 22 +- internal/output/envelope.go | 96 +++++++-- internal/output/output_test.go | 308 +++++++++++++++++++++++++++ internal/output/render.go | 96 ++++----- internal/presenter/format.go | 22 +- internal/presenter/format_test.go | 148 +++++++++++++ internal/presenter/presenter_test.go | 176 +++++++++++++++ internal/presenter/render.go | 18 +- internal/presenter/template.go | 16 +- internal/richtext/sanitize.go | 33 +++ internal/richtext/sanitize_test.go | 32 +++ 11 files changed, 890 insertions(+), 77 deletions(-) create mode 100644 internal/richtext/sanitize.go create mode 100644 internal/richtext/sanitize_test.go diff --git a/internal/commands/timeline.go b/internal/commands/timeline.go index 99d2a903..38bb0195 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.SanitizeTerminal(e.Creator.Name); name != "" { + creatorName = name + } } - action := e.Action + action := richtext.SanitizeTerminal(e.Action) if action == "" { action = "updated" } - title := e.Title + title := richtext.SanitizeTerminal(e.Title) if title == "" { - title = e.SummaryExcerpt + title = richtext.SanitizeTerminal(e.SummaryExcerpt) } // Truncate at rune boundary for proper Unicode handling if len([]rune(title)) > 40 { @@ -507,7 +515,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 = fmt.Sprintf("activity for %s", richtext.SanitizeTerminal(personName)) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { result, err := app.Account().Timeline().PersonProgress(ctx, personID, opts) if err != nil { @@ -525,7 +533,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 = fmt.Sprintf("activity in %s", richtext.SanitizeTerminal(projectName)) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { r, err := app.Account().Timeline().ProjectTimeline(ctx, projectIDInt, opts) if err != nil { diff --git a/internal/output/envelope.go b/internal/output/envelope.go index 1bf01cd8..10c99f96 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -13,6 +13,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. @@ -282,6 +283,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 +298,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 +326,34 @@ 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: + out := make(map[string]any, len(val)) + for k, elem := range val { + out[richtext.SanitizeTerminal(k)] = sanitizeJSONValue(elem) + } + return out + default: + return v + } +} + +// 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 +364,12 @@ func isTTY(w io.Writer) bool { return false } +// writeJSON emits the full envelope as JSON. Strings are deliberately NOT +// sanitized here: JSON is the machine-consumption contract (FormatAuto only +// selects JSON when stdout is not a TTY, and forced --json/--agent output is +// meant for pipes and files), so stripping control characters would silently +// corrupt data fidelity for programmatic consumers. Terminal-facing sinks +// (styled, markdown, --ids, and --jq when stdout is a TTY) sanitize instead. func (w *Writer) writeJSON(v any) error { toEncode := v if resp, ok := v.(*Response); ok { @@ -350,17 +403,27 @@ 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 removes terminal escape sequences and control characters +// from string values before they reach a raw print sink. Non-string values +// (json.Number ids, etc.) pass through unchanged. +func stripIfString(v any) any { + if s, ok := v.(string); ok { + return richtext.SanitizeTerminal(s) + } + return v +} + func (w *Writer) writeCount(v any) error { resp, ok := v.(*Response) if !ok { @@ -427,14 +490,19 @@ func (w *Writer) writeLiteralMarkdown(v any) error { type ResponseOption func(*Response) // WithSummary adds a summary to the response. +// Summaries frequently interpolate API-controlled strings (project/person/ +// entity names), so terminal escape sequences and control characters are +// stripped at the source to prevent terminal injection in every +// styled/markdown sink. func WithSummary(s string) ResponseOption { - return func(r *Response) { r.Summary = s } + return func(r *Response) { r.Summary = richtext.SanitizeTerminal(s) } } // WithNotice adds an informational notice to the response. // Use this for non-error messages like truncation warnings. +// Like WithSummary, the value is sanitized at the source. func WithNotice(s string) ResponseOption { - return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false } + return func(r *Response) { r.Notice = richtext.SanitizeTerminal(s); r.noticeDiagnostic = false } } // WithDiagnostic sets a notice that is also emitted to stderr in quiet mode. @@ -442,7 +510,7 @@ func WithNotice(s string) ResponseOption { // automation consumers need to detect. Truncation and other informational // notices should use WithNotice instead. func WithDiagnostic(s string) ResponseOption { - return func(r *Response) { r.Notice = s; r.noticeDiagnostic = true } + return func(r *Response) { r.Notice = richtext.SanitizeTerminal(s); r.noticeDiagnostic = true } } // WithBreadcrumbs adds breadcrumbs to the response. @@ -536,13 +604,16 @@ func (w *Writer) presentStyledEntity(resp *Response) bool { var out strings.Builder r := NewRenderer(w.opts.Writer, true) + // SanitizeTerminal defends against terminal injection from API-controlled + // summary/notice content (already sanitized at the WithSummary/WithNotice + // source; repeated here as defense-in-depth at the render sink). if resp.Summary != "" { - out.WriteString(r.Summary.Render(resp.Summary)) + out.WriteString(r.Summary.Render(richtext.SanitizeTerminal(resp.Summary))) out.WriteString("\n") } if resp.Notice != "" { - out.WriteString(r.Hint.Render(resp.Notice)) + out.WriteString(r.Hint.Render(richtext.SanitizeTerminal(resp.Notice))) out.WriteString("\n") } @@ -595,12 +666,13 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool { var out strings.Builder mr := NewMarkdownRenderer(w.opts.Writer) + // Defense-in-depth ANSI stripping (see presentStyledEntity). if resp.Summary != "" { - out.WriteString("## " + resp.Summary + "\n") + out.WriteString("## " + richtext.SanitizeTerminal(resp.Summary) + "\n") } if resp.Notice != "" { - out.WriteString("*" + resp.Notice + "*\n") + out.WriteString("*" + richtext.SanitizeTerminal(resp.Notice) + "*\n") } if resp.Summary != "" || resp.Notice != "" { diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 834c2b9d..08702f6e 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,26 @@ 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") +} + func TestWriterCountFormat(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -2341,6 +2362,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 +2387,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 // ============================================================================= @@ -3146,6 +3241,160 @@ 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. + 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]["title"], "sanitized 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"]) +} + +// 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") +} + func TestWriterJQFilterSelect(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -3648,3 +3897,62 @@ func TestPluralNoun(t *testing.T) { assert.Equal(t, tt.expected, PluralNoun(tt.input), "PluralNoun(%q)", tt.input) } } + +// TestWithSummaryStripsANSI verifies that API-controlled summary/notice content +// is sanitized of terminal escape sequences at the source, preventing terminal +// injection in every styled/markdown sink. +func TestWithSummaryStripsANSI(t *testing.T) { + // OSC 8 hyperlink + CSI color sequence wrapping a hostile payload. + payload := "\x1b]8;;http://evil\x07click\x1b]8;;\x07\x1b[31mpwn\x1b[0m" + + t.Run("WithSummary", func(t *testing.T) { + r := &Response{} + WithSummary(payload)(r) + assert.Equal(t, ansi.Strip(payload), r.Summary) + assert.NotContains(t, r.Summary, "\x1b") + }) + + t.Run("WithNotice", func(t *testing.T) { + r := &Response{} + WithNotice(payload)(r) + assert.Equal(t, ansi.Strip(payload), r.Notice) + assert.NotContains(t, r.Notice, "\x1b") + assert.False(t, r.noticeDiagnostic) + }) + + t.Run("WithDiagnostic", func(t *testing.T) { + r := &Response{} + WithDiagnostic(payload)(r) + assert.Equal(t, ansi.Strip(payload), r.Notice) + assert.NotContains(t, r.Notice, "\x1b") + assert.True(t, r.noticeDiagnostic) + }) +} + +// TestWithSummaryStripsC1Controls verifies that UTF-8-encoded Unicode C1 +// controls (U+0080-U+009F) and DEL are stripped too: they survive ansi.Strip +// but are executed directly by C1-honoring terminals. +func TestWithSummaryStripsC1Controls(t *testing.T) { + // C1 CSI color + C1 OSC title-set + DEL wrapping a hostile payload. + payload := "\u009b31mpwn\u009b0m \u009d0;evil\a pe\u007fek" + want := "31mpwn0m 0;evil peek" + + t.Run("WithSummary", func(t *testing.T) { + r := &Response{} + WithSummary(payload)(r) + assert.Equal(t, want, r.Summary) + }) + + t.Run("WithNotice", func(t *testing.T) { + r := &Response{} + WithNotice(payload)(r) + assert.Equal(t, want, r.Notice) + }) + + t.Run("WithDiagnostic", func(t *testing.T) { + r := &Response{} + WithDiagnostic(payload)(r) + assert.Equal(t, want, r.Notice) + assert.True(t, r.noticeDiagnostic) + }) +} diff --git a/internal/output/render.go b/internal/output/render.go index 4f903a83..75e37027 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,15 +112,17 @@ 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 + // Summary line. SanitizeTerminal guards against terminal injection from + // API-controlled summary/notice content (defense-in-depth: also sanitized + // at the WithSummary/WithNotice source). if resp.Summary != "" { - b.WriteString(r.Summary.Render(resp.Summary)) + b.WriteString(r.Summary.Render(richtext.SanitizeTerminal(resp.Summary))) b.WriteString("\n") } // Notice (e.g., truncation warning) if resp.Notice != "" { - b.WriteString(r.Hint.Render(resp.Notice)) + b.WriteString(r.Hint.Render(richtext.SanitizeTerminal(resp.Notice))) b.WriteString("\n") } @@ -258,20 +259,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) + // SanitizeTerminal strips escape sequences plus C0/C1/DEL controls, + // keeping only newline and tab, which Fields then collapses to spaces. + requestID = richtext.SanitizeTerminal(requestID) return strings.Join(strings.Fields(requestID), " ") } +// sanitizeRichText prepares an API-controlled rich-text string for terminal +// rendering: carriage returns are normalized to newlines (SanitizeTerminal +// would otherwise delete them, joining adjacent words), HTML is converted to +// markdown, and the result is stripped of escape sequences and control +// characters. Sanitization runs after the HTML conversion because entity +// decoding (e.g. ›) can reintroduce C1 controls that were not present +// in the raw input. +func sanitizeRichText(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + s = strings.ReplaceAll(s, "\r", "\n") + if richtext.IsHTML(s) { + s = richtext.HTMLToMarkdown(s) + } + return richtext.SanitizeTerminal(s) +} + func escapeMarkdownText(s string) string { replacer := strings.NewReplacer( `\`, `\\`, @@ -390,7 +399,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 +408,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 +775,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" @@ -795,11 +804,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 +819,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(sanitizeRichText(content)) } func (r *Renderer) renderCommentsSection(b *strings.Builder, comments []map[string]any) { @@ -999,10 +1002,7 @@ func formatCell(val any) string { case nil: return "" case string: - v = ansi.Strip(v) - if richtext.IsHTML(v) { - v = richtext.HTMLToMarkdown(v) - } + v = sanitizeRichText(v) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1035,7 +1035,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 +1056,27 @@ 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)) } 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 +1103,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 = sanitizeRichText(v) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1195,14 +1192,15 @@ func NewMarkdownRenderer(w io.Writer) *MarkdownRenderer { func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { var b strings.Builder - // Summary as heading + // Summary as heading. SanitizeTerminal guards against terminal injection + // (defense-in-depth; also sanitized at the WithSummary/WithNotice source). if resp.Summary != "" { - b.WriteString("## " + resp.Summary + "\n") + b.WriteString("## " + richtext.SanitizeTerminal(resp.Summary) + "\n") } // Notice (e.g., truncation warning) if resp.Notice != "" { - b.WriteString("*" + resp.Notice + "*\n") + b.WriteString("*" + richtext.SanitizeTerminal(resp.Notice) + "*\n") } if resp.Summary != "" || resp.Notice != "" { @@ -1275,13 +1273,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..721e541a 100644 --- a/internal/presenter/format.go +++ b/internal/presenter/format.go @@ -52,6 +52,9 @@ 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. + str = richtext.SanitizeTerminal(str) // Try ISO8601 full timestamp if t, err := time.Parse(time.RFC3339, str); err == nil { @@ -71,6 +74,9 @@ 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. + str = richtext.SanitizeTerminal(str) t, err := time.Parse(time.RFC3339, str) if err != nil { @@ -129,7 +135,9 @@ 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) + if stripped := richtext.SanitizeTerminal(name); stripped != "" { + names = append(names, stripped) + } } } } @@ -168,7 +176,9 @@ func parseDockItems(val any) []dockItem { result := make([]dockItem, len(items)) for i, m := range items { title, _ := m["title"].(string) + title = richtext.SanitizeTerminal(title) name, _ := m["name"].(string) + name = richtext.SanitizeTerminal(name) if title == "" { title = name } @@ -263,7 +273,7 @@ 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 + return richtext.SanitizeTerminal(name) } } return "" @@ -295,8 +305,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..2059d2aa 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,149 @@ 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) + } +} diff --git a/internal/presenter/presenter_test.go b/internal/presenter/presenter_test.go index 8b256f7e..45be7ecc 100644 --- a/internal/presenter/presenter_test.go +++ b/internal/presenter/presenter_test.go @@ -1617,6 +1617,158 @@ 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 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 ansi.Strip 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 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 +1883,30 @@ 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) + } +} + +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..2bd3b3eb 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,11 @@ 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. Strip + // terminal escapes before the empty-check so an all-escape-sequence + // name falls back to "Other" instead of emitting a blank "## ". + heading := richtext.SanitizeTerminal(g.name) if heading == "" { heading = "Other" } @@ -559,7 +565,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 +655,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.SanitizeTerminal(name); s != "" { + names = append(names, s) + } } } } diff --git a/internal/presenter/template.go b/internal/presenter/template.go index b9809574..65db3119 100644 --- a/internal/presenter/template.go +++ b/internal/presenter/template.go @@ -34,7 +34,11 @@ func RenderTemplate(tmpl string, data map[string]any) string { if err := t.Execute(&buf, sanitizeNumericValues(data)); err != nil { return "