sec: Prevent ANSI Escape Control Code Injection in Input Fields (#3201) - #3280
sec: Prevent ANSI Escape Control Code Injection in Input Fields (#3201)#3280knoxiboy wants to merge 2 commits into
Conversation
github-actions
Bot
added
area:widgets
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesANSI input sanitization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/widgets/src/input/TextInput.ts (1)
142-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAccount for all graphemes left after sanitization.
stripAnsiControl()can turn a hostile string into multiple printable graphemes, butinsertChar()inserts that result as one array element and the following cursor increment advances only once. For example, inserting'\x1b[31mAB\x1b[0m'storesAB, leaves the cursor betweenAandB, and can bypass_maxLength.Split
sanitizedCharinto graphemes, truncate to the available capacity, insert them with spread syntax, and advance_cursorPosby the number actually inserted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/input/TextInput.ts` around lines 142 - 154, Update insertChar around sanitizedChar and the graphemes insertion so the sanitized value is split into individual graphemes before insertion. Respect the remaining _maxLength capacity by truncating the sanitized graphemes, insert the retained graphemes as separate elements, and advance _cursorPos by the number actually inserted rather than once.
🧹 Nitpick comments (1)
packages/widgets/src/input/TextInput.test.ts (1)
448-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the setter and raw mutation paths.
This suite does not exercise the changed
TextInput.valuesetter, nor raw-mode assignment orinsertChar()behavior. Add direct setter assertions for both sanitized and raw values so regressions in Lines 95-100 and the raw branch cannot pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/input/TextInput.test.ts` around lines 448 - 464, The ANSI sanitization tests only cover constructor initialization and miss the changed TextInput.value setter and raw-mode mutation paths. Extend the “ANSI Control Code Security Sanitization” suite with direct value assignments that verify control codes are stripped when raw is false, preserved when raw is true, and that raw-mode insertChar() retains its input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/utils/ansi.ts`:
- Around line 231-234: The ANSI sanitizer regex in stripAnsiControl must
prioritize complete DCS/SS2/SS3 sequences before the single-byte ESC fallback,
so payloads are removed along with their introducers. Reorder the alternatives
accordingly and broaden the CSI final-byte range from [a-zA-Z] to [`@-`~],
preserving the existing handling of other control sequences.
---
Outside diff comments:
In `@packages/widgets/src/input/TextInput.ts`:
- Around line 142-154: Update insertChar around sanitizedChar and the graphemes
insertion so the sanitized value is split into individual graphemes before
insertion. Respect the remaining _maxLength capacity by truncating the sanitized
graphemes, insert the retained graphemes as separate elements, and advance
_cursorPos by the number actually inserted rather than once.
---
Nitpick comments:
In `@packages/widgets/src/input/TextInput.test.ts`:
- Around line 448-464: The ANSI sanitization tests only cover constructor
initialization and miss the changed TextInput.value setter and raw-mode mutation
paths. Extend the “ANSI Control Code Security Sanitization” suite with direct
value assignments that verify control codes are stripped when raw is false,
preserved when raw is true, and that raw-mode insertChar() retains its input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 103e60b7-1a11-4c41-ae1c-461de262dc49
📒 Files selected for processing (5)
packages/core/src/utils/ansi.tspackages/ui/src/MultilineTextInput.test.tspackages/ui/src/MultilineTextInput.tspackages/widgets/src/input/TextInput.test.tspackages/widgets/src/input/TextInput.ts
Karanjot786#3201) - Fix alternative ordering in the regex: move DCS/PM/APC [PX^_] handler BEFORE the catch-all [@-Z\_] so that ESC P/X/^/_ is consumed by the specific handler (stripping the full payload), not by the catch-all (which left payload in output) - Broaden CSI final-byte range from [a-zA-Z] to [@-~] per ECMA-48 to cover sequences like ESC[2~ (Insert), ESC[15~ (F5) with ~ final byte - Add explicit SS2/SS3 (ESC N / ESC O) matching via [NO]. alternative - Fix incorrect test expectations: OSC and DCS should produce '' not partial payload - Add regression tests for DCS/PM/APC payload stripping, SS2/SS3, and CSI ~ Addresses security review comment from coderabbitai
|
No activity on this PR for 14 days. Rebase, resolve conflicts, or comment to keep it open. It closes in 7 days otherwise. |
Description
This PR prevents ANSI escape control code injection in TextInput (@termuijs/widgets) and MultilineTextInput (@termuijs/ui) by sanitizing user input and value assignments using stripAnsiControl when raw: false (default), while preserving formatting when raw: true.
Related Issue
Closes #3201
Which package(s)?
@termuijs/widgets, @termuijs/ui, @termuijs/core
Type of Change
Checklist
eeds-star check blocks your merge otherwise.
GSSoC 2026 Participation
Notes for the Reviewer
Added security sanitization via stripAnsiControl in TextInput and MultilineTextInput while preserving raw: true options and fixing OSC sequence regex matching in @termuijs/core.
Summary by CodeRabbit
Security
raw: trueoption is enabled.Bug Fixes