fix(security): sanitize terminal-bound text

[benvinegar]

Summary

• Add a shared terminal text sanitizer for untrusted review/pager content
• Strip OSC/CSI/DCS/APC/PM/SOS, C1 controls, BEL, CR, BS, and bare ESC before terminal rendering
• Apply sanitization to diff rows, file labels, notes, expanded context, copy selections, static/plain pager output, and passthrough output
• Add regression coverage for static and dynamic terminal-control injection paths

Test plan

• bun test
• bun run typecheck
• bun run lint
• Live tmux smoke test with malicious OSC/DCS/APC/CR/BS payload fixture

This PR description was generated by Pi using OpenAI GPT-5

[greptile-apps]

Greptile Summary

This PR adds a centralized sanitizeTerminalText/sanitizeTerminalLine utility and applies it pervasively across diff rendering, pager output, file-path labels, agent notes, copy-selection text, and expanded context rows to prevent terminal control-sequence injection from untrusted diff content.

• src/lib/terminalText.ts introduces a four-pass sanitizer (7-bit escape sequences → C1 string sequences → C1 CSI → residual control characters) with an early-exit fast path for clean text; sanitizeTerminalLine is a convenience wrapper that also strips newlines.
• Sanitization is applied at many layers: span-level in renderRows.tsx and expandCollapsedRows.ts, path-level in files.ts and staticDiffPager.ts, note-author/body level in AgentInlineNote.tsx, pager-write level in pager.ts, and passthrough-stdout level in main.tsx.
• Regression tests cover OSC/CSI/DCS/APC/PM/SOS payloads, C1 variants, visual-spoofing controls (CR, BS, bare ESC, BEL), and the static/dynamic pager and copy-selection paths.

Confidence Score: 4/5

The sanitizer logic is sound and all identified injection paths are covered; the only concerns are redundant sanitization in hot render paths and a duplicated helper function.

The four-pass regex sanitizer correctly handles all common terminal injection vectors with a fast-path exit for clean text. Call sites are comprehensive. The structural issue is that sanitizeTerminalLine was embedded inside low-level measureTextWidth and sliceTextByWidth utilities: sliceTextByWidth sanitizes its input once, then calls measureTextWidth(cluster) in a loop for each grapheme, which sanitizes the already-clean cluster again. For render-heavy screens with many Unicode code cells this adds per-character overhead on every repaint. It is not a correctness problem, but it is a latent performance concern in the render hot path.

src/ui/lib/text.ts for the redundant per-cluster re-sanitization inside sliceTextByWidth; src/ui/diff/expandCollapsedRows.ts and src/ui/diff/renderRows.tsx for the duplicated sanitizeSpans/sanitizeRenderSpans helper.

Important Files Changed

Filename	Overview
src/lib/terminalText.ts	New core sanitizer implementing layered regex removal. Logic is correct; module-level global regexes are safe with .replace() but lack a comment explaining the constraint.
src/ui/lib/text.ts	Adds sanitizeTerminalLine to measureTextWidth and sliceTextByWidth. Creates redundant N+1 sanitization calls per non-ASCII string because the inner measureTextWidth(cluster) loop re-sanitizes already-clean clusters.
src/ui/diff/expandCollapsedRows.ts	Adds sanitizeSpans and sanitizeTerminalLine to expanded context rendering. sanitizeSpans is an exact duplicate of sanitizeRenderSpans in renderRows.tsx.
src/ui/diff/renderRows.tsx	Applies sanitization at span level via sanitizeRenderSpans in renderInlineSpans, wrapSpans, spansToPlainText, and spansText. fitText also sanitizes before measurement. Coverage looks complete.
src/core/pager.ts	Sanitizes plain-text pager content before writing directly to stdout and before piping to a spawned pager process. Clean change.
src/ui/staticDiffPager.ts	Sanitizes per-span text in colorText, both paths of fileDisplayPath, the fallback message in warnFallback, and both fallback returns. Defense-in-depth coverage looks solid.
src/ui/lib/files.ts	Sanitizes file paths in sidebarFileName, buildSidebarEntries, and fileLabelParts including the previousPath rename case. Correct guard added for undefined previousPath.
src/ui/components/panes/AgentInlineNote.tsx	Sanitizes note author in inlineNoteTitle and each line in wrapNoteText. The author?.trim() ?? '' null-coalescing preserves original fallback behavior.
src/main.tsx	Wraps passthrough startup output with sanitizeTerminalText before writing to stdout. Minimal, correct change.
src/lib/terminalText.test.ts	New unit tests cover OSC, OSC-ST, CSI, DCS, APC, PM, SOS, C1 variants, visual-spoofing controls, and multiline/tab preservation with a comprehensive control-code regex assertion.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Untrusted Input"] --> B["sanitizeTerminalText / sanitizeTerminalLine\nsrc/lib/terminalText.ts"]
    B --> B1["Pass 1: sevenBitControlStrings\nESC OSC, DCS, SOS, PM, APC, CSI"]
    B1 --> B2["Pass 2: c1ControlStrings\nC1 DCS/SOS/OSC/PM/APC"]
    B2 --> B3["Pass 3: c1Csi\nC1 CSI"]
    B3 --> B4["Pass 4: controlCharacters\nResidual C0/DEL/C1 bytes"]
    B4 --> C["Safe text"]
    subgraph "Call sites"
        D1["pager.ts"]
        D2["main.tsx"]
        D3["staticDiffPager.ts"]
        D4["renderRows.tsx"]
        D5["expandCollapsedRows.ts"]
        D6["files.ts"]
        D7["AgentInlineNote.tsx / agentPopover.ts"]
        D8["text.ts"]
    end
    A --> D1 & D2 & D3 & D4 & D5 & D6 & D7 & D8
    D1 & D2 & D3 & D4 & D5 & D6 & D7 & D8 --> B

Loading

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/ui/lib/text.ts:20-23
**Redundant re-sanitization on every grapheme cluster**

`sliceTextByWidth` sanitizes its input once to produce `safeText`, then iterates `textClusters(safeText)` and calls `measureTextWidth(cluster)` for each cluster. `measureTextWidth` now calls `sanitizeTerminalLine` on its input, so every grapheme cluster from the already-clean `safeText` is sanitized a second time. For a non-ASCII string with N clusters this means N+1 `sanitizeTerminalLine` invocations per `sliceTextByWidth` call. Because `sliceTextByWidth` is called inside render loops, this overhead accumulates on each repaint of a diff with Unicode content. The single `sanitizeTerminalLine` at the top of `sliceTextByWidth` already guarantees clusters are clean; the inner `measureTextWidth` call could use `stringWidth` directly for the cluster width without re-sanitizing.

### Issue 2 of 3
src/ui/diff/expandCollapsedRows.ts:83-87
**Duplicate `sanitizeSpans` / `sanitizeRenderSpans` across two files**

`sanitizeSpans` here and `sanitizeRenderSpans` in `renderRows.tsx` (lines 647-651) are byte-for-byte identical. Having two copies means a future change to the sanitization logic would need to be applied in two places. This helper belongs in `terminalText.ts` as an exported function so both call sites can share it.

### Issue 3 of 3
src/lib/terminalText.ts:9-12
**Module-level `/g` regexes are safe here, but worth an explicit note**

`sevenBitControlStrings`, `c1ControlStrings`, and `c1Csi` are module-level constants with the `/g` flag. `String.prototype.replace` resets `lastIndex` to 0 before iterating matches, so current usage is safe. However, if any caller were to use `.test()` or `.exec()` on these shared regex objects, `lastIndex` would be left in an intermediate state causing every second call to return the wrong result. A brief comment that these regexes must only be used via `.replace()` would prevent a subtle future footgun.

_{Reviews (1): Last reviewed commit: "fix(security): sanitize terminal-bound t..." | Re-trigger Greptile}

[greptile-apps]

Redundant re-sanitization on every grapheme cluster

sliceTextByWidth sanitizes its input once to produce safeText, then iterates textClusters(safeText) and calls measureTextWidth(cluster) for each cluster. measureTextWidth now calls sanitizeTerminalLine on its input, so every grapheme cluster from the already-clean safeText is sanitized a second time. For a non-ASCII string with N clusters this means N+1 sanitizeTerminalLine invocations per sliceTextByWidth call. Because sliceTextByWidth is called inside render loops, this overhead accumulates on each repaint of a diff with Unicode content. The single sanitizeTerminalLine at the top of sliceTextByWidth already guarantees clusters are clean; the inner measureTextWidth call could use stringWidth directly for the cluster width without re-sanitizing.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/ui/lib/text.ts
Line: 20-23

Comment:
**Redundant re-sanitization on every grapheme cluster**

`sliceTextByWidth` sanitizes its input once to produce `safeText`, then iterates `textClusters(safeText)` and calls `measureTextWidth(cluster)` for each cluster. `measureTextWidth` now calls `sanitizeTerminalLine` on its input, so every grapheme cluster from the already-clean `safeText` is sanitized a second time. For a non-ASCII string with N clusters this means N+1 `sanitizeTerminalLine` invocations per `sliceTextByWidth` call. Because `sliceTextByWidth` is called inside render loops, this overhead accumulates on each repaint of a diff with Unicode content. The single `sanitizeTerminalLine` at the top of `sliceTextByWidth` already guarantees clusters are clean; the inner `measureTextWidth` call could use `stringWidth` directly for the cluster width without re-sanitizing.

How can I resolve this? If you propose a fix, please make it concise.

[benvinegar]

Fixed. sliceTextByWidth now measures already-sanitized grapheme clusters with an internal width helper instead of routing every cluster back through measureTextWidth.

This comment was generated by Pi using OpenAI GPT-5

[greptile-apps]

Duplicate sanitizeSpans / sanitizeRenderSpans across two files

sanitizeSpans here and sanitizeRenderSpans in renderRows.tsx (lines 647-651) are byte-for-byte identical. Having two copies means a future change to the sanitization logic would need to be applied in two places. This helper belongs in terminalText.ts as an exported function so both call sites can share it.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/ui/diff/expandCollapsedRows.ts
Line: 83-87

Comment:
**Duplicate `sanitizeSpans` / `sanitizeRenderSpans` across two files**

`sanitizeSpans` here and `sanitizeRenderSpans` in `renderRows.tsx` (lines 647-651) are byte-for-byte identical. Having two copies means a future change to the sanitization logic would need to be applied in two places. This helper belongs in `terminalText.ts` as an exported function so both call sites can share it.

How can I resolve this? If you propose a fix, please make it concise.

[benvinegar]

Fixed. I moved the shared span sanitizer into src/lib/terminalText.ts as sanitizeTerminalSpans and updated both call sites to use it.

This comment was generated by Pi using OpenAI GPT-5

[greptile-apps]

Module-level /g regexes are safe here, but worth an explicit note

sevenBitControlStrings, c1ControlStrings, and c1Csi are module-level constants with the /g flag. String.prototype.replace resets lastIndex to 0 before iterating matches, so current usage is safe. However, if any caller were to use .test() or .exec() on these shared regex objects, lastIndex would be left in an intermediate state causing every second call to return the wrong result. A brief comment that these regexes must only be used via .replace() would prevent a subtle future footgun.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/lib/terminalText.ts
Line: 9-12

Comment:
**Module-level `/g` regexes are safe here, but worth an explicit note**

`sevenBitControlStrings`, `c1ControlStrings`, and `c1Csi` are module-level constants with the `/g` flag. `String.prototype.replace` resets `lastIndex` to 0 before iterating matches, so current usage is safe. However, if any caller were to use `.test()` or `.exec()` on these shared regex objects, `lastIndex` would be left in an intermediate state causing every second call to return the wrong result. A brief comment that these regexes must only be used via `.replace()` would prevent a subtle future footgun.

How can I resolve this? If you propose a fix, please make it concise.

[benvinegar]

Added the note. The global regexes now have a comment explaining they are private and only used with String#replace, avoiding test/exec lastIndex footguns.

This comment was generated by Pi using OpenAI GPT-5