Skip to content

Commit 4566262

Browse files
committed
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.
1 parent 2734359 commit 4566262

9 files changed

Lines changed: 337 additions & 31 deletions

File tree

internal/commands/timeline.go

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
tea "charm.land/bubbletea/v2"
1515
"charm.land/lipgloss/v2"
1616
"github.com/basecamp/basecamp-sdk/go/pkg/basecamp"
17+
"github.com/charmbracelet/x/ansi"
1718
"github.com/spf13/cobra"
1819

1920
"github.com/basecamp/basecamp-cli/internal/appctx"
@@ -395,19 +396,26 @@ func formatEvent(e basecamp.TimelineEvent) string {
395396
timeStr = "--:--"
396397
}
397398

399+
// API-controlled fields are ANSI-stripped before rendering: rune truncation
400+
// preserves escape bytes, and the alt-screen watch TUI would otherwise
401+
// execute embedded OSC/ANSI sequences (terminal injection).
402+
// Strip before the empty check so a name that's only escape sequences
403+
// still falls back to the placeholder rather than rendering blank.
398404
creatorName := "Someone"
399-
if e.Creator != nil && e.Creator.Name != "" {
400-
creatorName = e.Creator.Name
405+
if e.Creator != nil {
406+
if name := ansi.Strip(e.Creator.Name); name != "" {
407+
creatorName = name
408+
}
401409
}
402410

403-
action := e.Action
411+
action := ansi.Strip(e.Action)
404412
if action == "" {
405413
action = "updated"
406414
}
407415

408-
title := e.Title
416+
title := ansi.Strip(e.Title)
409417
if title == "" {
410-
title = e.SummaryExcerpt
418+
title = ansi.Strip(e.SummaryExcerpt)
411419
}
412420
// Truncate at rune boundary for proper Unicode handling
413421
if len([]rune(title)) > 40 {
@@ -507,7 +515,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string,
507515
if err != nil {
508516
return output.ErrUsage("Invalid person ID")
509517
}
510-
description = fmt.Sprintf("activity for %s", personName)
518+
description = fmt.Sprintf("activity for %s", ansi.Strip(personName))
511519
fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) {
512520
result, err := app.Account().Timeline().PersonProgress(ctx, personID, opts)
513521
if err != nil {
@@ -525,7 +533,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string,
525533
if err != nil {
526534
return output.ErrUsage("Invalid project ID")
527535
}
528-
description = fmt.Sprintf("activity in %s", projectName)
536+
description = fmt.Sprintf("activity in %s", ansi.Strip(projectName))
529537
fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) {
530538
r, err := app.Account().Timeline().ProjectTimeline(ctx, projectIDInt, opts)
531539
if err != nil {

internal/output/envelope.go

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"os"
88
"strings"
99

10+
"github.com/charmbracelet/x/ansi"
1011
"github.com/itchyny/gojq"
1112

1213
clioutput "github.com/basecamp/cli/output"
@@ -421,22 +422,26 @@ func (w *Writer) writeLiteralMarkdown(v any) error {
421422
type ResponseOption func(*Response)
422423

423424
// WithSummary adds a summary to the response.
425+
// Summaries frequently interpolate API-controlled strings (project/person/
426+
// entity names), so ANSI/OSC escape sequences are stripped at the source to
427+
// prevent terminal injection in every styled/markdown sink.
424428
func WithSummary(s string) ResponseOption {
425-
return func(r *Response) { r.Summary = s }
429+
return func(r *Response) { r.Summary = ansi.Strip(s) }
426430
}
427431

428432
// WithNotice adds an informational notice to the response.
429433
// Use this for non-error messages like truncation warnings.
434+
// Like WithSummary, the value is ANSI-stripped at the source.
430435
func WithNotice(s string) ResponseOption {
431-
return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false }
436+
return func(r *Response) { r.Notice = ansi.Strip(s); r.noticeDiagnostic = false }
432437
}
433438

434439
// WithDiagnostic sets a notice that is also emitted to stderr in quiet mode.
435440
// Use this for degraded-operation warnings (e.g. unresolved mentions) that
436441
// automation consumers need to detect. Truncation and other informational
437442
// notices should use WithNotice instead.
438443
func WithDiagnostic(s string) ResponseOption {
439-
return func(r *Response) { r.Notice = s; r.noticeDiagnostic = true }
444+
return func(r *Response) { r.Notice = ansi.Strip(s); r.noticeDiagnostic = true }
440445
}
441446

442447
// WithBreadcrumbs adds breadcrumbs to the response.
@@ -530,13 +535,16 @@ func (w *Writer) presentStyledEntity(resp *Response) bool {
530535
var out strings.Builder
531536
r := NewRenderer(w.opts.Writer, true)
532537

538+
// ansi.Strip defends against terminal injection from API-controlled
539+
// summary/notice content (already stripped at the WithSummary/WithNotice
540+
// source; repeated here as defense-in-depth at the render sink).
533541
if resp.Summary != "" {
534-
out.WriteString(r.Summary.Render(resp.Summary))
542+
out.WriteString(r.Summary.Render(ansi.Strip(resp.Summary)))
535543
out.WriteString("\n")
536544
}
537545

538546
if resp.Notice != "" {
539-
out.WriteString(r.Hint.Render(resp.Notice))
547+
out.WriteString(r.Hint.Render(ansi.Strip(resp.Notice)))
540548
out.WriteString("\n")
541549
}
542550

@@ -589,12 +597,13 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool {
589597
var out strings.Builder
590598
mr := NewMarkdownRenderer(w.opts.Writer)
591599

600+
// Defense-in-depth ANSI stripping (see presentStyledEntity).
592601
if resp.Summary != "" {
593-
out.WriteString("## " + resp.Summary + "\n")
602+
out.WriteString("## " + ansi.Strip(resp.Summary) + "\n")
594603
}
595604

596605
if resp.Notice != "" {
597-
out.WriteString("*" + resp.Notice + "*\n")
606+
out.WriteString("*" + ansi.Strip(resp.Notice) + "*\n")
598607
}
599608

600609
if resp.Summary != "" || resp.Notice != "" {

internal/output/output_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3565,3 +3565,34 @@ func TestPluralNoun(t *testing.T) {
35653565
assert.Equal(t, tt.expected, PluralNoun(tt.input), "PluralNoun(%q)", tt.input)
35663566
}
35673567
}
3568+
3569+
// TestWithSummaryStripsANSI verifies that API-controlled summary/notice content
3570+
// is sanitized of terminal escape sequences at the source, preventing terminal
3571+
// injection in every styled/markdown sink.
3572+
func TestWithSummaryStripsANSI(t *testing.T) {
3573+
// OSC 8 hyperlink + CSI color sequence wrapping a hostile payload.
3574+
payload := "\x1b]8;;http://evil\x07click\x1b]8;;\x07\x1b[31mpwn\x1b[0m"
3575+
3576+
t.Run("WithSummary", func(t *testing.T) {
3577+
r := &Response{}
3578+
WithSummary(payload)(r)
3579+
assert.Equal(t, ansi.Strip(payload), r.Summary)
3580+
assert.NotContains(t, r.Summary, "\x1b")
3581+
})
3582+
3583+
t.Run("WithNotice", func(t *testing.T) {
3584+
r := &Response{}
3585+
WithNotice(payload)(r)
3586+
assert.Equal(t, ansi.Strip(payload), r.Notice)
3587+
assert.NotContains(t, r.Notice, "\x1b")
3588+
assert.False(t, r.noticeDiagnostic)
3589+
})
3590+
3591+
t.Run("WithDiagnostic", func(t *testing.T) {
3592+
r := &Response{}
3593+
WithDiagnostic(payload)(r)
3594+
assert.Equal(t, ansi.Strip(payload), r.Notice)
3595+
assert.NotContains(t, r.Notice, "\x1b")
3596+
assert.True(t, r.noticeDiagnostic)
3597+
})
3598+
}

internal/output/render.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,15 +112,17 @@ func terminalInfo(w io.Writer) (width int, isTTY bool) {
112112
func (r *Renderer) RenderResponse(w io.Writer, resp *Response) error {
113113
var b strings.Builder
114114

115-
// Summary line
115+
// Summary line. ansi.Strip guards against terminal injection from
116+
// API-controlled summary/notice content (defense-in-depth: also stripped
117+
// at the WithSummary/WithNotice source).
116118
if resp.Summary != "" {
117-
b.WriteString(r.Summary.Render(resp.Summary))
119+
b.WriteString(r.Summary.Render(ansi.Strip(resp.Summary)))
118120
b.WriteString("\n")
119121
}
120122

121123
// Notice (e.g., truncation warning)
122124
if resp.Notice != "" {
123-
b.WriteString(r.Hint.Render(resp.Notice))
125+
b.WriteString(r.Hint.Render(ansi.Strip(resp.Notice)))
124126
b.WriteString("\n")
125127
}
126128

@@ -1116,14 +1118,15 @@ func NewMarkdownRenderer(w io.Writer) *MarkdownRenderer {
11161118
func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error {
11171119
var b strings.Builder
11181120

1119-
// Summary as heading
1121+
// Summary as heading. ansi.Strip guards against terminal injection
1122+
// (defense-in-depth; also stripped at the WithSummary/WithNotice source).
11201123
if resp.Summary != "" {
1121-
b.WriteString("## " + resp.Summary + "\n")
1124+
b.WriteString("## " + ansi.Strip(resp.Summary) + "\n")
11221125
}
11231126

11241127
// Notice (e.g., truncation warning)
11251128
if resp.Notice != "" {
1126-
b.WriteString("*" + resp.Notice + "*\n")
1129+
b.WriteString("*" + ansi.Strip(resp.Notice) + "*\n")
11271130
}
11281131

11291132
if resp.Summary != "" || resp.Notice != "" {

internal/presenter/format.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"strings"
99
"time"
1010

11+
"github.com/charmbracelet/x/ansi"
12+
1113
"github.com/basecamp/basecamp-cli/internal/richtext"
1214
)
1315

@@ -61,7 +63,7 @@ func formatDate(val any, locale Locale) string {
6163
if t, err := time.Parse("2006-01-02", str); err == nil {
6264
return locale.FormatDate(t)
6365
}
64-
return str
66+
return ansi.Strip(str)
6567
}
6668

6769
// formatRelativeTime formats a timestamp as relative time (e.g. "2 hours ago").
@@ -77,7 +79,7 @@ func formatRelativeTime(val any, locale Locale) string {
7779
// Try date-only
7880
t, err = time.Parse("2006-01-02", str)
7981
if err != nil {
80-
return str
82+
return ansi.Strip(str)
8183
}
8284
}
8385

@@ -129,7 +131,9 @@ func formatPeople(val any) string {
129131
for _, item := range arr {
130132
if m, ok := item.(map[string]any); ok {
131133
if name, ok := m["name"].(string); ok {
132-
names = append(names, name)
134+
if stripped := ansi.Strip(name); stripped != "" {
135+
names = append(names, stripped)
136+
}
133137
}
134138
}
135139
}
@@ -168,7 +172,9 @@ func parseDockItems(val any) []dockItem {
168172
result := make([]dockItem, len(items))
169173
for i, m := range items {
170174
title, _ := m["title"].(string)
175+
title = ansi.Strip(title)
171176
name, _ := m["name"].(string)
177+
name = ansi.Strip(name)
172178
if title == "" {
173179
title = name
174180
}
@@ -263,7 +269,7 @@ func dockPosition(m map[string]any) int {
263269
func formatPerson(val any) string {
264270
if m, ok := val.(map[string]any); ok {
265271
if name, ok := m["name"].(string); ok {
266-
return name
272+
return ansi.Strip(name)
267273
}
268274
}
269275
return ""
@@ -295,8 +301,14 @@ func formatText(val any) string {
295301
case nil:
296302
return ""
297303
case string:
304+
// Strip terminal escape sequences from API-controlled strings before
305+
// they reach a styled/markdown sink (terminal injection defense).
306+
v = ansi.Strip(v)
298307
if richtext.IsHTML(v) {
299-
return richtext.HTMLToMarkdown(v)
308+
// Defense-in-depth: strip the generated markdown too before it can
309+
// reach a styled/markdown sink. The input is already stripped above
310+
// and HTMLToMarkdown never emits ESC, so this is belt-and-suspenders.
311+
return ansi.Strip(richtext.HTMLToMarkdown(v))
300312
}
301313
return v
302314
case bool:

internal/presenter/format_test.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package presenter
22

33
import (
44
"encoding/json"
5+
"strings"
56
"testing"
67
)
78

@@ -133,3 +134,82 @@ func TestFormatDockTitleFallsBackToName(t *testing.T) {
133134
t.Errorf("formatDock(title fallback) = %q, want %q", got, want)
134135
}
135136
}
137+
138+
func TestFormatDockStripsANSIFromTitleAndName(t *testing.T) {
139+
dock := []any{
140+
map[string]any{
141+
"title": "\x1b]8;;http://evil.example\x07To-dos\x1b]8;;\x07",
142+
"name": "\x1b[31mtodoset\x1b[0m",
143+
"enabled": true,
144+
"id": float64(1),
145+
},
146+
}
147+
148+
got := formatDock(dock)
149+
want := "1 To-dos (todoset)"
150+
if got != want {
151+
t.Errorf("formatDock(ANSI in title/name) = %q, want %q", got, want)
152+
}
153+
}
154+
155+
func TestFormatDockTitleFallbackStripsANSIFromName(t *testing.T) {
156+
dock := []any{
157+
map[string]any{
158+
"name": "\x1b[31mtodoset\x1b[0m",
159+
"enabled": true,
160+
"id": float64(1),
161+
},
162+
}
163+
164+
got := formatDock(dock)
165+
want := "1 todoset"
166+
if got != want {
167+
t.Errorf("formatDock(ANSI in name fallback) = %q, want %q", got, want)
168+
}
169+
}
170+
171+
func TestFormatPeopleStripsANSI(t *testing.T) {
172+
people := []any{
173+
map[string]any{"name": "\x1b[31mAlice\x1b[0m"},
174+
map[string]any{"name": "Bob"},
175+
}
176+
177+
got := formatPeople(people)
178+
want := "Alice, Bob"
179+
if got != want {
180+
t.Errorf("formatPeople(ANSI names) = %q, want %q", got, want)
181+
}
182+
}
183+
184+
func TestFormatDateStripsANSIFromUnparseableInput(t *testing.T) {
185+
locale := NewLocale("")
186+
for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} {
187+
got := formatDate(in, locale)
188+
if strings.ContainsRune(got, 0x1b) {
189+
t.Errorf("formatDate(%q) = %q, want no escape byte", in, got)
190+
}
191+
}
192+
}
193+
194+
func TestFormatRelativeTimeStripsANSIFromUnparseableInput(t *testing.T) {
195+
locale := NewLocale("")
196+
for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} {
197+
got := formatRelativeTime(in, locale)
198+
if strings.ContainsRune(got, 0x1b) {
199+
t.Errorf("formatRelativeTime(%q) = %q, want no escape byte", in, got)
200+
}
201+
}
202+
}
203+
204+
func TestFormatPeopleDropsAllEscapeName(t *testing.T) {
205+
people := []any{
206+
map[string]any{"name": "Alice"},
207+
map[string]any{"name": "\x1b[0m\x1b]8;;\x07"},
208+
}
209+
210+
got := formatPeople(people)
211+
want := "Alice"
212+
if got != want {
213+
t.Errorf("formatPeople(all-escape name dropped) = %q, want %q", got, want)
214+
}
215+
}

0 commit comments

Comments
 (0)