diff --git a/internal/commands/timeline.go b/internal/commands/timeline.go index 99d2a903..cc2df748 100644 --- a/internal/commands/timeline.go +++ b/internal/commands/timeline.go @@ -14,6 +14,7 @@ import ( tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/charmbracelet/x/ansi" "github.com/spf13/cobra" "github.com/basecamp/basecamp-cli/internal/appctx" @@ -395,19 +396,26 @@ func formatEvent(e basecamp.TimelineEvent) string { timeStr = "--:--" } + // API-controlled fields are ANSI-stripped 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 := ansi.Strip(e.Creator.Name); name != "" { + creatorName = name + } } - action := e.Action + action := ansi.Strip(e.Action) if action == "" { action = "updated" } - title := e.Title + title := ansi.Strip(e.Title) if title == "" { - title = e.SummaryExcerpt + title = ansi.Strip(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", ansi.Strip(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", ansi.Strip(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 e70469f6..4bcfcacf 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -7,6 +7,7 @@ import ( "os" "strings" + "github.com/charmbracelet/x/ansi" "github.com/itchyny/gojq" clioutput "github.com/basecamp/cli/output" @@ -421,14 +422,18 @@ 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 ANSI/OSC escape sequences 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 = ansi.Strip(s) } } // WithNotice adds an informational notice to the response. // Use this for non-error messages like truncation warnings. +// Like WithSummary, the value is ANSI-stripped at the source. func WithNotice(s string) ResponseOption { - return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false } + return func(r *Response) { r.Notice = ansi.Strip(s); r.noticeDiagnostic = false } } // WithDiagnostic sets a notice that is also emitted to stderr in quiet mode. @@ -436,7 +441,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 = ansi.Strip(s); r.noticeDiagnostic = true } } // WithBreadcrumbs adds breadcrumbs to the response. @@ -530,13 +535,16 @@ func (w *Writer) presentStyledEntity(resp *Response) bool { var out strings.Builder r := NewRenderer(w.opts.Writer, true) + // ansi.Strip defends against terminal injection from API-controlled + // summary/notice content (already stripped 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(ansi.Strip(resp.Summary))) out.WriteString("\n") } if resp.Notice != "" { - out.WriteString(r.Hint.Render(resp.Notice)) + out.WriteString(r.Hint.Render(ansi.Strip(resp.Notice))) out.WriteString("\n") } @@ -589,12 +597,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("## " + ansi.Strip(resp.Summary) + "\n") } if resp.Notice != "" { - out.WriteString("*" + resp.Notice + "*\n") + out.WriteString("*" + ansi.Strip(resp.Notice) + "*\n") } if resp.Summary != "" || resp.Notice != "" { diff --git a/internal/output/output_test.go b/internal/output/output_test.go index e4c5adc1..8ac43523 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -3565,3 +3565,34 @@ 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) + }) +} diff --git a/internal/output/render.go b/internal/output/render.go index d45a2d5b..faa4a699 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -112,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. ansi.Strip guards against terminal injection from + // API-controlled summary/notice content (defense-in-depth: also stripped + // at the WithSummary/WithNotice source). if resp.Summary != "" { - b.WriteString(r.Summary.Render(resp.Summary)) + b.WriteString(r.Summary.Render(ansi.Strip(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(ansi.Strip(resp.Notice))) b.WriteString("\n") } @@ -1116,14 +1118,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. ansi.Strip guards against terminal injection + // (defense-in-depth; also stripped at the WithSummary/WithNotice source). if resp.Summary != "" { - b.WriteString("## " + resp.Summary + "\n") + b.WriteString("## " + ansi.Strip(resp.Summary) + "\n") } // Notice (e.g., truncation warning) if resp.Notice != "" { - b.WriteString("*" + resp.Notice + "*\n") + b.WriteString("*" + ansi.Strip(resp.Notice) + "*\n") } if resp.Summary != "" || resp.Notice != "" { diff --git a/internal/presenter/format.go b/internal/presenter/format.go index 55dc9282..97f40d6d 100644 --- a/internal/presenter/format.go +++ b/internal/presenter/format.go @@ -8,6 +8,8 @@ import ( "strings" "time" + "github.com/charmbracelet/x/ansi" + "github.com/basecamp/basecamp-cli/internal/richtext" ) @@ -61,7 +63,7 @@ func formatDate(val any, locale Locale) string { if t, err := time.Parse("2006-01-02", str); err == nil { return locale.FormatDate(t) } - return str + return ansi.Strip(str) } // formatRelativeTime formats a timestamp as relative time (e.g. "2 hours ago"). @@ -77,7 +79,7 @@ func formatRelativeTime(val any, locale Locale) string { // Try date-only t, err = time.Parse("2006-01-02", str) if err != nil { - return str + return ansi.Strip(str) } } @@ -129,7 +131,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 := ansi.Strip(name); stripped != "" { + names = append(names, stripped) + } } } } @@ -168,7 +172,9 @@ func parseDockItems(val any) []dockItem { result := make([]dockItem, len(items)) for i, m := range items { title, _ := m["title"].(string) + title = ansi.Strip(title) name, _ := m["name"].(string) + name = ansi.Strip(name) if title == "" { title = name } @@ -263,7 +269,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 ansi.Strip(name) } } return "" @@ -295,8 +301,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 = ansi.Strip(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 ansi.Strip(richtext.HTMLToMarkdown(v)) } return v case bool: diff --git a/internal/presenter/format_test.go b/internal/presenter/format_test.go index 416bb55a..c55b6770 100644 --- a/internal/presenter/format_test.go +++ b/internal/presenter/format_test.go @@ -2,6 +2,7 @@ package presenter import ( "encoding/json" + "strings" "testing" ) @@ -133,3 +134,82 @@ 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) + } +} + +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) + } + } +} + +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 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 588fb60c..fc1d5f2b 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..c86fcdf3 100644 --- a/internal/presenter/render.go +++ b/internal/presenter/render.go @@ -7,6 +7,7 @@ import ( "strings" "charm.land/lipgloss/v2" + "github.com/charmbracelet/x/ansi" "github.com/basecamp/basecamp-cli/internal/richtext" "github.com/basecamp/basecamp-cli/internal/tui" @@ -335,6 +336,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 +551,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 := ansi.Strip(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, ansi.Strip(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 := ansi.Strip(name); s != "" { + names = append(names, s) + } } } } diff --git a/internal/presenter/template.go b/internal/presenter/template.go index b9809574..9e24523d 100644 --- a/internal/presenter/template.go +++ b/internal/presenter/template.go @@ -7,6 +7,8 @@ import ( "regexp" "text/template" + "github.com/charmbracelet/x/ansi" + "github.com/basecamp/basecamp-cli/internal/richtext" ) @@ -34,7 +36,11 @@ func RenderTemplate(tmpl string, data map[string]any) string { if err := t.Execute(&buf, sanitizeNumericValues(data)); err != nil { return "