Handle zero-color terminfo entries with heuristics - #7
Conversation
|
Added TERM name heuristics to get_sgr_support so color support is inferred even when terminfo entries are missing, while falling back safely for legacy terminals. Made COLORTERM detection case-insensitive for truecolor/24-bit declarations, ensuring broader compatibility. Centralized TERM-based heuristics into a helper and applied them when terminfo lookups fail or return unknown results, improving color support detection reliability. Treated unknown or missing tigetnum("colors") results as heuristically detectable terminals instead of returning raw negative values. Added a regression test confirming that get_sgr_support() uses TERM heuristics when terminfo reports an unknown color capability. Treated zero or missing terminfo color capabilities as unknown and now fall back to TERM-based heuristics for SGR support detection. Added a regression test ensuring direct-color terminfo entries reporting zero colors still resolve to truecolor support via heuristics. |
| def _heuristic_colors_from_term(term: str) -> int: | ||
| if "direct" in term: | ||
| return 2**24 | ||
|
|
||
| term_colors = re_search(r"(\d+)[-_]?color", term) | ||
| if term_colors: | ||
| colors = int(term_colors.group(1)) | ||
| if colors >= 256: | ||
| return 256 | ||
| if colors >= 88: | ||
| return 88 | ||
| if colors >= 16: | ||
| return 2**4 | ||
| if colors >= 8: | ||
| return 2**3 | ||
|
|
||
| if "old" in term: | ||
| return -1 | ||
|
|
||
| if term.startswith("xterm"): | ||
| return 2**3 | ||
|
|
||
| return -1 | ||
|
|
||
| try: | ||
| setupterm() | ||
| return tigetnum("colors") | ||
| colors = tigetnum("colors") | ||
| if colors is None or colors < 1: | ||
| return _heuristic_colors_from_term(environ.get("TERM", "").lower()) | ||
| return colors | ||
| except curses_error: | ||
| return -2 | ||
| return _heuristic_colors_from_term(environ.get("TERM", "").lower()) |
There was a problem hiding this comment.
I get the rationale for this, and I kind of like the implementation too, but this worries me. If we can't get known-good information about what colors the terminal supports, trying to guess the right color this way seems like it might introduce a security hole. What if a terminal sets TERM in such a way that the heuristics mistakenly think it supports true color, and the terminal has a vulnerability wherein which actually trying to use true color leads to a buffer overflow? It's much better to fall back to no color at all if the supported colors cannot be detected in a dependable way.
| if environ.get("COLORTERM") in ("truecolor", "24bit"): | ||
| colorterm = environ.get("COLORTERM") | ||
| if colorterm and colorterm.lower() in ("truecolor", "24bit"): |
There was a problem hiding this comment.
I guess this is probably a good idea. It's awfully hard for me to imagine a terminal setting COLORTERM='TRUECOLOR' and not supporting true color.
| class TestGetSgrSupport(unittest.TestCase): | ||
| """ | ||
| Validate SGR color detection fallbacks. | ||
| """ | ||
|
|
||
| @patch("stdisplay.stdisplay.setupterm") | ||
| @patch("stdisplay.stdisplay.tigetnum", return_value=-2) | ||
| def test_heuristic_used_when_colors_unknown( | ||
| self, patched_tigetnum: Any, patched_setupterm: Any | ||
| ) -> None: | ||
| """ | ||
| An unknown terminfo capability (-2) should fall back to TERM heuristics. | ||
| """ | ||
|
|
||
| with patch.dict( | ||
| "stdisplay.stdisplay.environ", | ||
| {"TERM": "xterm-256color", "NO_COLOR": "", "COLORTERM": ""}, | ||
| clear=True, | ||
| ): | ||
| self.assertEqual(get_sgr_support(), 256) | ||
|
|
||
| patched_setupterm.assert_called_once() | ||
| patched_tigetnum.assert_called_once_with("colors") | ||
|
|
||
| @patch("stdisplay.stdisplay.setupterm") | ||
| @patch("stdisplay.stdisplay.tigetnum", return_value=0) | ||
| def test_direct_color_entries_with_zero_colors( | ||
| self, patched_tigetnum: Any, patched_setupterm: Any | ||
| ) -> None: | ||
| """ | ||
| Some direct-color terminfo entries report 0 colors; fall back to heuristics. | ||
| """ | ||
|
|
||
| with patch.dict( | ||
| "stdisplay.stdisplay.environ", | ||
| {"TERM": "xterm-direct", "NO_COLOR": "", "COLORTERM": ""}, | ||
| clear=True, | ||
| ): | ||
| self.assertEqual(get_sgr_support(), 2**24) | ||
|
|
||
| patched_setupterm.assert_called_once() | ||
| patched_tigetnum.assert_called_once_with("colors") |
There was a problem hiding this comment.
Heuristics are a bad idea, so we don't need these.
Rejected, because heuristics in this area may lead to security issues.
Accepted.
All rejected, because heuristics were rejected. The useful part of this PR has been implemented in ArrayBolt3@3c64c17. This PR can be closed. |
coverity: broaden gate to fork + temp push trigger for verification
Address the actionable code-level alerts surfaced by CodeQL on the default branch. Skipped Scorecard meta-alerts (repo policy, not code) and the random-between clear-text-logging alert (false positive: the script's documented purpose is to print the random number to stdout). Fixed: - stdisplay.py: drop tautological `if sgr >= 2**3` so sgr_combo is always initialized (alert #9, py/uninitialized-local-variable). - guimessage.py: open file via `with`, drop dead commented-out code, drop unused local `message` (Kicksecure#21, #7, #8). - str_replace: open file via `with`; stdin path no longer needs a shared file handle (Kicksecure#22, py/file-not-closed). - sttee.py: manage output file lifetimes via `contextlib.ExitStack` instead of manual try/finally close loop (Kicksecure#23). - get-backing-devices-for-mountpoint: replace builtin `exit()` with `sys.exit()` (#10, py/use-of-exit-or-quit). - translations.py: bare `except:` -> `except Exception:`, `== None` -> `is None`, drop unused `import sys` (#17, Kicksecure#24, Kicksecure#25, Kicksecure#26). - leak-tests/{exhaustive_ip_send,simple_ping,tcp_test,udp_test}.py, find_wl_compositor, tor-dormant-check, tor-signal-newnym: drop unused imports (#11, #12, #13, #14, #15, #16, #18). - strip_markup/tests/strip_markup.py and unicode_show/tests/ unicode_show.py: replace `from unittest import mock` with `import unittest.mock as mock` so `unittest` isn't imported both ways (#19, #20).
2 participants
Summary
Testing
Codex Task