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. |
Summary
Testing
Codex Task