-
Notifications
You must be signed in to change notification settings - Fork 2
fix(sanitization): close security gaps in ANSI escape sequence stripping #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…SI regex Co-authored-by: GingerGraham <[email protected]>
…omment Co-authored-by: GingerGraham <[email protected]>
Co-authored-by: GingerGraham <[email protected]>
Co-authored-by: GingerGraham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR hardens bash-logger’s ANSI/terminal escape sanitization to prevent additional bypasses that could enable terminal escape injection, and adds targeted tests for the newly covered cases.
Changes:
- Expands CSI stripping to cover DEC private modes and additional CSI parameter bytes.
- Improves OSC stripping by supporting ST (
ESC \) termination and handling embedded escapes / consecutive OSC sequences. - Adds tests for the new CSI/OSC cases and for mixed newline/ANSI sanitization mode behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
logging.sh |
Updates _strip_ansi_codes to recognize more CSI/OSC variants (DEC private modes; ST-terminated OSC via a sed loop). |
tests/test_ansi_injection.sh |
Adds regression tests for DEC private modes, ST-terminated OSC, embedded escapes inside OSC, consecutive OSC sequences, mixed sanitization modes, and extra CSI parameter bytes. |
| # Remove CSI (Control Sequence Introducer) sequences: ESC [ ... letter | ||
| # Includes color codes (\e[...m), cursor movement (\e[H), clearing (\e[2J), etc. | ||
| # Pattern: \e[ followed by zero or more digits/semicolons, followed by a letter | ||
| # Also handles DEC private modes (e.g., \e[?25l, \e[?1049h) and other parameter bytes | ||
| # Pattern: \e[ followed by zero or more parameter bytes ([<=>?!] plus digits/semicolons), | ||
| # followed by a letter or @ | ||
| local step1 | ||
| # Use direct escapes to avoid quoting issues in patterns | ||
| # shellcheck disable=SC1117 | ||
| step1=$(printf '%s' "$input" | sed 's/\x1b\[[0-9;]*[a-zA-Z@]//g') | ||
| step1=$(printf '%s' "$input" | sed 's/\x1b\[[0-9;<?>=!]*[a-zA-Z@]//g') |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSI stripping regex is still not spec-complete: CSI “parameter bytes” include ':' (0x3A) and the final byte can be any in 0x40–0x7E (e.g., sequences ending in '' like "\e[1"). With the current [0-9;<?>=!]*[a-zA-Z@] pattern, valid CSI sequences can remain unstripped, which keeps a terminal escape injection bypass open. Consider widening the parameter-byte and final-byte character classes to cover the full CSI grammar (and optionally intermediate bytes 0x20–0x2F).
| # Pattern: \([^\x1b]\|\x1b[^\\]\)* matches any char except ESC, OR ESC if not followed by \ | ||
| # This allows embedded ESC codes like \e[31m while still stopping at \e\\ terminator | ||
| # The loop ensures multiple consecutive OSC sequences are all removed | ||
| # shellcheck disable=SC1117 | ||
| step2=$(printf '%s' "$step2" | sed ':loop; s/\x1b\]\(\([^\x1b]\|\x1b[^\\]\)*\)\x1b\\//g; t loop') |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ST-terminated OSC sed pattern uses \| alternation, which is a GNU sed BRE extension and will fail on BSD/macOS sed. Since this sanitizer is security-sensitive, a non-portable sed expression can silently leave OSC sequences unstripped on macOS. Please rewrite this using a portable approach (e.g., sed -E with |, or another POSIX-compatible implementation) and ensure the behavior is covered across supported platforms.
| # Pattern: \([^\x1b]\|\x1b[^\\]\)* matches any char except ESC, OR ESC if not followed by \ | |
| # This allows embedded ESC codes like \e[31m while still stopping at \e\\ terminator | |
| # The loop ensures multiple consecutive OSC sequences are all removed | |
| # shellcheck disable=SC1117 | |
| step2=$(printf '%s' "$step2" | sed ':loop; s/\x1b\]\(\([^\x1b]\|\x1b[^\\]\)*\)\x1b\\//g; t loop') | |
| # Pattern (ERE): ([^\x1b]|\x1b[^\\])* matches any char except ESC, OR ESC if not followed by \ | |
| # This allows embedded ESC codes like \e[31m while still stopping at \e\\ terminator | |
| # The loop ensures multiple consecutive OSC sequences are all removed | |
| # shellcheck disable=SC1117 | |
| step2=$(printf '%s' "$step2" | sed -E ':loop; s/\x1b\](([^\x1b]|\x1b[^\\])*)\x1b\\//g; t loop') |
| # Test: Other CSI parameter bytes are handled | ||
| test_other_csi_parameter_bytes_stripped() { | ||
| start_test "CSI sequences with various parameter bytes are stripped" | ||
|
|
||
| # Test various parameter byte prefixes | ||
| local malicious_input=$'A\e[>cB\e[=cC\e[!pD' | ||
| local expected="ABCD" | ||
|
|
||
| LOG_UNSAFE_ALLOW_ANSI_CODES="false" | ||
| local sanitized | ||
| sanitized=$(_strip_ansi_codes "$malicious_input") | ||
|
|
||
| if [[ "$sanitized" == "$expected" ]]; then | ||
| pass_test | ||
| else | ||
| fail_test "Other parameter bytes not stripped: got '$sanitized' expected '$expected'" | ||
| fi |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new tests cover additional CSI parameter bytes, but they still don’t exercise common non-alphabetic CSI final bytes (e.g., sequences ending in ~). Adding at least one case like "ESC [ 1 ~" would prevent regressions where valid CSI sequences are not stripped.
Addresses review feedback on PR #54 identifying three security bypasses in ANSI escape sequence sanitization that could enable terminal escape injection attacks.
Changes
CSI parameter byte handling
\e[?25l,\e[?1049h) and parameter prefixes (<,>,=,!)[0-9;], missing valid CSI sequences with other parameter bytesOSC sequence termination
\e\\) handling alongside existing BEL (\a) support\([^\x1b]\|\x1b[^\\]\)*matches any char except ESC, or ESC if not followed by backslashIndependent sanitization modes
LOG_UNSAFE_ALLOW_NEWLINES=true+LOG_UNSAFE_ALLOW_ANSI_CODES=falseshould preserve newlines while stripping ANSITest coverage
Added 6 tests covering:
\e[?25l)\e]0;title\e\\)\e]0;\e[31mred\e\\)\e[>c)250 tests passing.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.