Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 56 additions & 16 deletions internal/commands/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"encoding/json"
"fmt"
"net/url"
"regexp"
"strings"

Expand Down Expand Up @@ -47,7 +48,10 @@ func newAPIGetCmd() *cobra.Command {
return err
}

path := parsePath(args[0])
path, err := parsePath(args[0], app.Config.BaseURL)
if err != nil {
return err
}
resp, err := app.Account().Get(cmd.Context(), path)
if err != nil {
return convertSDKError(err)
Expand Down Expand Up @@ -85,7 +89,10 @@ func newAPIPostCmd() *cobra.Command {
return err
}

path := parsePath(args[0])
path, err := parsePath(args[0], app.Config.BaseURL)
if err != nil {
return err
}

// Parse JSON data
var body any
Expand Down Expand Up @@ -134,7 +141,10 @@ func newAPIPutCmd() *cobra.Command {
return err
}

path := parsePath(args[0])
path, err := parsePath(args[0], app.Config.BaseURL)
if err != nil {
return err
}

// Parse JSON data
var body any
Expand Down Expand Up @@ -176,7 +186,10 @@ func newAPIDeleteCmd() *cobra.Command {
return err
}

path := parsePath(args[0])
path, err := parsePath(args[0], app.Config.BaseURL)
if err != nil {
return err
}
resp, err := app.Account().Delete(cmd.Context(), path)
if err != nil {
return err
Expand All @@ -201,20 +214,47 @@ 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 /<account-id>/ segment in an API
// path so it can be dropped — the SDK re-prefixes the configured account.
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 /<account-id> 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 stray leading slash 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 string) (string, error) {
candidate := strings.TrimPrefix(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 == "" || !strings.EqualFold(u.Host, base.Host) {
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 /<account-id>.
path := u.EscapedPath()
if m := accountSegmentPattern.FindStringSubmatch(path); m != nil {
path = m[1]
}
if u.RawQuery != "" {
path += "?" + u.RawQuery
}
return path, nil
}

// Strip leading slashthe SDK prefixes the account path.
input = strings.TrimPrefix(input, "/")

return input
// Relative path — return without the leading slash; the SDK prefixes the
// account path (keeping it here would double-slash and, on Windows,
// MSYS/Git Bash converts /path to C:\...\path).
return candidate, nil
}

// apiSummary generates a summary from the API response.
Expand Down
42 changes: 41 additions & 1 deletion internal/commands/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/stretchr/testify/require"
)

const testBaseURL = "https://3.basecampapi.com"

func TestParsePath(t *testing.T) {
tests := []struct {
input string
Expand All @@ -17,13 +19,51 @@ 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"},
// Same-host absolute URLs: extract the path, dropping the account segment.
{"https://3.basecampapi.com/999/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"},
// Query strings are preserved.
{"https://3.basecampapi.com/999/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/999/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)
require.NoError(t, err)
assert.Equal(t, tt.want, 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
// and a leading-slash smuggling attempt.
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
}

for _, input := range bad {
t.Run(input, func(t *testing.T) {
got, err := parsePath(input, testBaseURL)
require.Error(t, err)
assert.Empty(t, got)
})
}
}
Expand Down
22 changes: 15 additions & 7 deletions internal/commands/timeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
23 changes: 16 additions & 7 deletions internal/output/envelope.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"strings"

"github.com/charmbracelet/x/ansi"
"github.com/itchyny/gojq"

clioutput "github.com/basecamp/cli/output"
Expand Down Expand Up @@ -421,22 +422,26 @@ 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.
// Use this for degraded-operation warnings (e.g. unresolved mentions) that
// 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.
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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 != "" {
Expand Down
31 changes: 31 additions & 0 deletions internal/output/output_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
15 changes: 9 additions & 6 deletions internal/output/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -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 != "" {
Expand Down
Loading