security: strip ANSI/OSC escapes from API-controlled output#479
Conversation
github-actions
Bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR mitigates terminal-injection risk by stripping ANSI/OSC escape sequences from API-controlled output before it reaches styled, markdown, presenter, and watch-mode render paths.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Adds source-level sanitization for response summaries, notices, and diagnostics.
- Adds sink-level stripping in styled/markdown renderers and presenter affordance formatting.
- Sanitizes timeline watch TUI fields before rendering/truncation and adds coverage for response option sanitization.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/output/envelope.go |
Strips ANSI/OSC escapes in response options and entity presenter summary/notice output. |
internal/output/render.go |
Applies defense-in-depth stripping in styled and markdown response renderers. |
internal/output/output_test.go |
Adds tests for summary/notice/diagnostic source-level sanitization. |
internal/presenter/format.go |
Sanitizes formatted API-controlled text and person names. |
internal/presenter/render.go |
Sanitizes rendered affordance commands after template interpolation. |
internal/commands/timeline.go |
Sanitizes watch TUI event fields and project/person descriptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
b6d427d to
2eacce7
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
7b1d3ea to
61aea03
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61aea0335f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
61aea03 to
e06c5c2
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
2eacce7 to
447cbf3
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
e06c5c2 to
9103d13
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9103d138ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
447cbf3 to
2734359
Compare
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
9103d13 to
dd63a13
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd63a1322a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
security/02-output-ansi-sanitize
branch
from
dd63a13 to
4566262
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
fc7bafa to
e56fa0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0451d413b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95074c40ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
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.
Assert the exact sanitized string instead of mirroring ansi.Strip, which is a weaker contract than richtext.SanitizeTerminal and ties the expectation to the third-party stripper's behavior.
Stripping keys let a hostile key like "\x1b[31mtitle" collapse onto a legitimate "title", silently replacing or hiding that field in TTY --jq output. Keys changed by sanitization are now visibly escaped via strconv.Quote, so clean keys always keep their slot and no entry is dropped. Also update a stale test comment that referenced ansi.Strip.
The escaped form of a hostile key could still collide with a literal key that mimics escape notation (e.g. a real ESC-prefixed title vs the text "\x1b[31mtitle"), nondeterministically dropping one entry. Clean keys now claim their names first, and escaped keys re-quote with delimiters until unique, in sorted order for deterministic output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0097ae3063
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59baa57217
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
basecamp-cli/internal/commands/timeline.go
Line 348 in 3b4831b
Sanitize watch-mode error text
When basecamp timeline --watch is polling and m.fetchFn returns an SDK/API error, m.err can contain server-controlled text, but this status line renders it directly in the alt-screen TUI. The new sanitization covers event fields and the person/project description, yet a failing API response containing OSC/CSI/C1 controls would still reach the terminal here; sanitize the error string before passing it to errStyle.Render.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef341d4574
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
internal/output/envelope.go:685
- In schema-aware styled output, Summary/Notice rendering is gated on the raw resp.Summary/resp.Notice but the sanitized value can become empty (all-escape/control input). That can currently produce blank header lines and still insert the extra spacer newline. Consider checking the sanitized values and only emitting/spacing when non-empty.
if resp.Summary != "" {
out.WriteString(r.Summary.Render(sanitizeText(resp.Summary, true, false)))
out.WriteString("\n")
}
if resp.Notice != "" {
out.WriteString(r.Hint.Render(sanitizeText(resp.Notice, true, false)))
out.WriteString("\n")
}
if resp.Summary != "" || resp.Notice != "" {
out.WriteString("\n")
}
internal/output/envelope.go:743
- In schema-aware Markdown output, Summary/Notice rendering is gated on the raw resp.Summary/resp.Notice but the sanitized value can become empty (e.g. all-escape/control). That can emit malformed markdown like a blank
##heading or**line. Use the sanitized value for the check and only add the spacer newline when something was actually written.
// Sink-level ANSI stripping (see presentStyledEntity).
if resp.Summary != "" {
out.WriteString("## " + sanitizeText(resp.Summary, true, false) + "\n")
}
if resp.Notice != "" {
out.WriteString("*" + sanitizeText(resp.Notice, true, false) + "*\n")
}
if resp.Summary != "" || resp.Notice != "" {
out.WriteString("\n")
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/presenter/format.go:185
- parseDockItems sanitizes dock item title/name with SanitizeTerminal, which preserves \n/\t. Dock lines are rendered as a line-oriented listing, so API-controlled dock strings containing newlines/tabs can still break the per-item layout or inject extra lines. Use SanitizeSingleLine for title/name so each dock item stays on one line after sanitization.
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
}
disabled := false
Request IDs and string ids print to line-oriented sinks, but SanitizeTerminal preserves newlines/tabs (which inject lines or columns into ids output) and deletes bare CR (which joins adjacent words). A shared sanitizeSingleLine helper normalizes CR/CRLF to newlines first, then strips and collapses all whitespace to single spaces.
Strip ANSI/OSC escapes from API-controlled output
(Threat: T4 terminal injection via API-controlled strings carrying ANSI/OSC sequences.)
API-controlled strings (project/person/entity names, summaries, notices) reached
lipgloss.Render/stdout and the--watchTUI withoutansi.Strip, in render paths the generic table/cell path already protected.Fix
Summary/Notice/Diagnosticare stored verbatim byWithSummary/WithNotice/WithDiagnostic(so machine/JSON output preserves byte fidelity, consistent with error/hint/data) and stripped only where they're emitted to a terminal — the styled and markdown renderers and the quiet-mode stderr diagnostic (render.go,envelope.go).--watchTUI: strip creator/action/title/excerpt + the person/project description (timeline.go); rune-truncation otherwise preserves escape bytes and the alt-screen executes OSC sequences.formatText/formatPerson/formatPeopleand the rendered affordance commands (presenter/format.go,presenter/render.go).Tests
Tests assert JSON
summary/noticeare verbatim and that every terminal sink (styled/markdown/plainRenderError, summary/notice, breadcrumbs, attachment metadata, and the quiet-mode stderrnotice:) neutralizes C1/CSI/OSC escapes.Part 2/6 of the stacked security series. Base:
security/01-api-token-leak.📚 Stack (merge bottom-up)
apito prevent token leak #478 — reject foreign-host URLs inapi(basemain)Each is independent except #482 depends on #481 (shared
root.go). #478 can land first/alone.