fix: sanitize ANSI/control sequences in markdown render path#116
fix: sanitize ANSI/control sequences in markdown render path#116Mingye-Lu wants to merge 2 commits into
Conversation
Untrusted text (e.g. scraped page content echoed back by the model) could carry raw ESC bytes that reached the terminal unfiltered: text_to_ansi() wrote span.content directly into the ANSI stream, and the existing strip_ansi() helper was never called from the streaming markdown render path (MarkdownStreamState -> markdown_to_ansi -> render_lines), only from tool_format.rs previews and the TUI. Fix by scrubbing the raw markdown string in render_lines() before it reaches the parser, rather than sanitizing per pulldown-cmark Event: the tokenizer can split an escape sequence's bytes across two adjacent Text/Html events (e.g. a stray '[' inside an OSC/CSI payload gets tokenized as markdown link syntax), so per-event sanitization misses sequences split at token boundaries. Since render_lines() is the single entry point used by both the streaming state machine and one-shot rendering, every caller is protected automatically. Strips all C0 control characters except tab/newline, and now recognizes CSI, OSC, DCS, SOS, PM, and APC escape sequences (not just CSI) via a shared skip_escape_sequence() helper also used by the existing strip_ansi(), so its other callers (tool_format.rs bash previews, the TUI) get the same broader coverage.
|
@codex review |
![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: 1ddb5b41fc
ℹ️ 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".
| let sanitized = sanitize_untrusted_text(markdown); | ||
|
|
||
| let mut writer = MdWriter::default(); | ||
| for event in Parser::new_ext(markdown, opts) { | ||
| for event in Parser::new_ext(&sanitized, opts) { |
There was a problem hiding this comment.
Because the filter only scrubs the raw markdown before pulldown_cmark runs, inputs such as [31mRED bypass it: the parser decodes the numeric character reference into an ESC Event::Text after this point, and text_to_ansi later writes span contents verbatim. In the streamed assistant/TUI render path, untrusted page text can therefore still inject terminal controls via entities unless decoded text events or rendered spans are sanitized as well.
Useful? React with 👍 / 👎.
| while let Some(next) = chars.next() { | ||
| if next == '\u{07}' { | ||
| break; | ||
| } | ||
| if next == '\u{1b}' && chars.peek() == Some(&'\\') { | ||
| chars.next(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
When an OSC/DCS/SOS/PM/APC sequence uses the 8-bit ST terminator (U+009C) instead of ESC \\, this loop never breaks and consumes everything through EOF. In markdown rendering or strip_ansi previews, content like before ESC]0;title U+009C after renders only before, so valid terminal strings can hide all following output rather than just being removed.
Useful? React with 👍 / 👎.
…equences - Push text through sanitize_untrusted_text() in push_text() to catch HTML-entity-decoded ANSI/C0 sequences that bypass the pre-parse filter (e.g.,  decoded by pulldown-cmark into ESC in Event::Text). - Accept C1 ST terminator (U+009C) as a valid string-escape terminator in skip_escape_sequence(), preventing unbounded consumption when OSC/DCS/SOS/PM/APC sequences use 8-bit terminators instead of ESC \. Closes #116
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
text_to_ansiwrote streamed markdown text directly into the terminal ANSIstream with no filtering, and the existing
strip_ansihelper was neverinvoked from that path (
MarkdownStreamState->markdown_to_ansi->render_lines), only from bash-output previews and the TUI. Untrustedscraped page text echoed back by the model could therefore inject raw
terminal escape sequences.
render_lines()beforeit reaches the
pulldown-cmarkparser (not per-Event, since thetokenizer can split a single escape sequence's bytes across two adjacent
events — e.g. a stray
[inside an OSC/CSI payload gets tokenized asmarkdown link syntax, defeating per-event filtering).
render_lines()is the single entry point for both the streaming statemachine and one-shot rendering, so every caller (including the assistant's
own streamed output path) is protected automatically without an opt-in
strip_ansi()call.CSI, OSC, DCS, SOS, PM, and APC escape sequences (not just CSI) via a
shared
skip_escape_sequence()helper also reused bystrip_ansi(), soits existing callers get the same broader coverage.
Test plan
cargo fmt -p rendercargo test -p render(81 passed, including newrender_lines_strips_injected_escape_sequences_from_textandstrip_ansi_removes_osc_sequencestests)cargo clippy -p render --all-targets -- -D warnings(clean)