From 19654e983a6e54b17469bf0011085a9a8004f6ff Mon Sep 17 00:00:00 2001 From: Thomas Christory Date: Sun, 25 Oct 2026 00:12:56 +0200 Subject: [PATCH 1/2] fix(output,tui): neutralize control sequences and markup in server text (#206) Rich's escape() only stops [..] markup, and Rich/Textual strip just BEL, BS, VT, FF and CR, so ESC sequences and bidi overrides in NetBox data reached the terminal (retitle/clear it, spoof text). Move the sanitizer to nsc/output/sanitize.py (printable, safe_markup, safe_text) and apply it to table cells and headers (headers were not escaped at all), the --explain panel, error panels, and the TUI: list and detail cells, record picker labels, error notifications, and diff views. TUI cells are now Text, not str: DataTable/Label parsed str as markup, so a value like `[red]x[/]` was styled and lost its brackets. Claude-Session: https://claude.ai/code/session_01RD8NgCVtp3yYFd6NSZpbUb --- CHANGELOG.md | 17 +++++-- nsc/output/errors.py | 34 ++++---------- nsc/output/explain.py | 22 ++++----- nsc/output/sanitize.py | 35 ++++++++++++++ nsc/output/table.py | 18 +++---- nsc/tui/errors.py | 7 +++ nsc/tui/screens/detail.py | 5 +- nsc/tui/screens/record_picker.py | 3 +- nsc/tui/view.py | 8 ++-- nsc/tui/widgets/bulk_diff.py | 11 ++--- nsc/tui/widgets/bulk_summary.py | 6 ++- nsc/tui/widgets/diff.py | 9 ++-- tests/output/test_sanitize.py | 59 +++++++++++++++++++++++ tests/tui/test_detail_screen.py | 5 +- tests/tui/test_sanitize_tui.py | 80 ++++++++++++++++++++++++++++++++ tests/tui/test_view.py | 17 +++++-- 16 files changed, 260 insertions(+), 76 deletions(-) create mode 100644 nsc/output/sanitize.py create mode 100644 tests/output/test_sanitize.py create mode 100644 tests/tui/test_sanitize_tui.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a37c99..a8845e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,10 +16,18 @@ All notable changes to netbox-super-cli are tracked here. Format follows [Keep a - **Long error messages wrap inside the terminal error panel** instead of being cut off. For example, the conflict error's closing `re-run with --acknowledge-conflicts` hint used to disappear. -- **Server text in terminal error panels can't send terminal control - sequences.** The error message, endpoint, operation id and conflict values - were printed raw, so an ESC sequence in an object name could retitle or clear - the terminal. Control and bidi characters now show as visible `\x..` escapes. +- **Server data can't send terminal control sequences or Rich/Textual markup** + ([#206]). Rich's markup escaping doesn't strip ESC, so an object name containing + an escape sequence could retitle or clear your terminal or spoof text. Control + and bidi characters now show as visible `\x..`/`\u....` escapes in: + - table output, including column headers, which weren't escaped at all; + - the `--explain` panel; + - error panels; + - the TUI: list and detail cells, record picker, error notifications and + diff views. + + In the TUI, values were also parsed as markup, so a value like `[red]x[/]` + was styled and lost its brackets. They now render literally. - **Plugin endpoints get one command group per resource** ([#202]). NetBox tags every plugin endpoint `plugins`, so all of a plugin's endpoints were merged into one `nsc plugins ` resource whose verbs overwrote each other. For @@ -221,6 +229,7 @@ notices, and a fix for `nsc login` reporting the wrong user. [#192]: https://github.com/thomaschristory/netbox-super-cli/issues/192 [#196]: https://github.com/thomaschristory/netbox-super-cli/issues/196 [#202]: https://github.com/thomaschristory/netbox-super-cli/issues/202 +[#206]: https://github.com/thomaschristory/netbox-super-cli/issues/206 [#171]: https://github.com/thomaschristory/netbox-super-cli/issues/171 [#179]: https://github.com/thomaschristory/netbox-super-cli/issues/179 [#173]: https://github.com/thomaschristory/netbox-super-cli/issues/173 diff --git a/nsc/output/errors.py b/nsc/output/errors.py index 36a7200..b25d596 100644 --- a/nsc/output/errors.py +++ b/nsc/output/errors.py @@ -7,21 +7,19 @@ from __future__ import annotations import json -import unicodedata from enum import Enum, StrEnum from typing import Any, Literal, TextIO from pydantic import BaseModel, ConfigDict, Field from rich import box from rich.console import Group, RenderableType -from rich.markup import escape from rich.panel import Panel from rich.table import Table -from rich.text import Text from nsc.config.models import OutputFormat from nsc.model.command_model import HttpMethod from nsc.output._console import make_console +from nsc.output.sanitize import safe_markup, safe_text class ErrorType(StrEnum): @@ -109,50 +107,35 @@ def render_to_rich_stderr(env: ErrorEnvelope, *, stream: TextIO, color: bool = F # which cut off e.g. the "re-run with --acknowledge-conflicts" hint. console = make_console(stream, color=color) # env.error/endpoint/operation_id/audit_log_path/details carry server- and - # user-sourced text: `_safe` neutralizes Rich markup and terminal control + # user-sourced text: `safe_markup` neutralizes Rich markup and terminal control # sequences (an object name must not retitle or clear the terminal). body_lines = [ - f"[bold red]{env.type.value}[/]: {_safe(env.error)}", + f"[bold red]{env.type.value}[/]: {safe_markup(env.error)}", ] if env.endpoint: - body_lines.append(f"endpoint: {_safe(env.endpoint)}") + body_lines.append(f"endpoint: {safe_markup(env.endpoint)}") if env.method is not None: body_lines.append(f"method: {env.method.value}") if env.status_code is not None: body_lines.append(f"status: {env.status_code}") if env.operation_id: - body_lines.append(f"op: {_safe(env.operation_id)}") + body_lines.append(f"op: {safe_markup(env.operation_id)}") if env.attempt_n is not None: body_lines.append(f"attempt: {env.attempt_n}") if env.audit_log_path: - body_lines.append(f"audit: {_safe(env.audit_log_path)}") + body_lines.append(f"audit: {safe_markup(env.audit_log_path)}") details = dict(env.details) conflicts = _conflict_table(details.get("conflicts")) if conflicts is not None: del details["conflicts"] if details: - body_lines.append(f"details: {_safe(str(details))}") + body_lines.append(f"details: {safe_markup(str(details))}") body: RenderableType = "\n".join(body_lines) if conflicts is not None: body = Group(body, conflicts) console.print(Panel(body, title="nsc error", border_style="red")) -def _printable(text: str) -> str: - """Show control and format characters (ESC, BEL, bidi overrides, ...) as - visible escapes so server text cannot drive the terminal.""" - return "".join( - ch - if ch in "\n\t" or not unicodedata.category(ch).startswith("C") - else ch.encode("unicode_escape").decode("ascii") - for ch in text - ) - - -def _safe(text: str) -> str: - return escape(_printable(text)) - - _CONFLICT_SIDES = ("original", "branch", "main") @@ -173,9 +156,8 @@ def _conflict_table(raw: Any) -> Table | None: names = [str(f) for f in fields] if isinstance(fields, list) and fields else [""] for i, name in enumerate(names): values = [_conflict_value(sides.get(side), name) for side in _CONFLICT_SIDES] - # Text cells are never parsed as markup; `_printable` handles control codes. cells = (obj if i == 0 else "", name, *values) - table.add_row(*(Text(_printable(c)) for c in cells)) + table.add_row(*(safe_text(c) for c in cells)) return table diff --git a/nsc/output/explain.py b/nsc/output/explain.py index c6dfc4c..011e7f7 100644 --- a/nsc/output/explain.py +++ b/nsc/output/explain.py @@ -11,7 +11,6 @@ from typing import Any, Literal, TextIO from pydantic import BaseModel, ConfigDict, Field -from rich.markup import escape from rich.panel import Panel from nsc.cli.writes.apply import ResolvedRequest @@ -20,6 +19,7 @@ from nsc.cli.writes.preflight import PreflightResult from nsc.model.command_model import Operation from nsc.output._console import make_console +from nsc.output.sanitize import safe_markup DECISION_CAP = 200 @@ -155,25 +155,25 @@ def render_to_json(trace: ExplainTrace) -> str: def render_to_rich_stdout(trace: ExplainTrace, *, stream: TextIO, color: bool = False) -> None: console = make_console(stream, color=color, soft_wrap=True) # Reasoning strings and decision values carry user/schema/server text that - # may contain [..]; escape it so it is not parsed as Rich markup. + # may contain [..] or control characters; `safe_markup` neutralizes both. body = [ - f"[bold cyan]operation:[/] {escape(trace.operation_id)}", + f"[bold cyan]operation:[/] {safe_markup(trace.operation_id)}", ] if trace.operation_summary: - body.append(f"summary: {escape(trace.operation_summary)}") - body.append(f"method: {escape(trace.method_reasoning)}") - body.append(f"url: {escape(trace.url_reasoning)}") + body.append(f"summary: {safe_markup(trace.operation_summary)}") + body.append(f"method: {safe_markup(trace.method_reasoning)}") + body.append(f"url: {safe_markup(trace.url_reasoning)}") if trace.bulk_reasoning: - body.append(f"bulk: {escape(trace.bulk_reasoning)}") + body.append(f"bulk: {safe_markup(trace.bulk_reasoning)}") for r in trace.requests: - body.append(f" → {r.method.value} {escape(r.url)}") + body.append(f" → {r.method.value} {safe_markup(r.url)}") if r.body is not None: - body.append(f" body: {escape(str(r.body))}") + body.append(f" body: {safe_markup(str(r.body))}") if trace.preflight is not None and not trace.preflight.ok: body.append("preflight: [red]FAIL[/]") for issue in trace.preflight.issues: loc = f"records[{issue.record_index}].{issue.field_path}" - body.append(escape(f" [{issue.kind}] {loc}: {issue.message}")) + body.append(safe_markup(f" [{issue.kind}] {loc}: {issue.message}")) elif trace.preflight is not None: body.append("preflight: [green]OK[/]") if trace.decisions: @@ -181,7 +181,7 @@ def render_to_rich_stdout(trace: ExplainTrace, *, stream: TextIO, color: bool = for d in trace.decisions: extra = f" — {d.note}" if d.note else "" body.append( - escape( + safe_markup( f" {d.field_path} [{d.source}] {d.raw_value!r} → {d.resolved_value!r}{extra}" ) ) diff --git a/nsc/output/sanitize.py b/nsc/output/sanitize.py new file mode 100644 index 0000000..b8871c4 --- /dev/null +++ b/nsc/output/sanitize.py @@ -0,0 +1,35 @@ +"""Make server-sourced text safe to put on a terminal. + +Rich's `escape()` stops `[..]` markup, and Rich/Textual strip only BEL, BS, VT, +FF and CR. ESC, other C0/C1 controls and bidi overrides still reach the +terminal, so a NetBox object name could retitle or clear it, or spoof text +(issue #206). +""" + +from __future__ import annotations + +import unicodedata + +from rich.markup import escape +from rich.text import Text + + +def printable(text: str) -> str: + """Show control and format characters as visible `\\x..`/`\\u....` escapes.""" + return "".join( + ch + if ch in "\n\t" or not unicodedata.category(ch).startswith("C") + else ch.encode("unicode_escape").decode("ascii") + for ch in text + ) + + +def safe_markup(text: str) -> str: + """`text` for interpolation into a Rich markup string.""" + return escape(printable(text)) + + +def safe_text(text: str, style: str = "") -> Text: + """A Rich `Text` (never parsed as markup) for widgets that would otherwise + parse a plain `str` as markup, e.g. Textual's `DataTable` and `Label`.""" + return Text(printable(text), style=style) diff --git a/nsc/output/table.py b/nsc/output/table.py index 0fcdf59..9447f80 100644 --- a/nsc/output/table.py +++ b/nsc/output/table.py @@ -5,12 +5,12 @@ import sys from typing import Any, TextIO -from rich.markup import escape from rich.table import Table from nsc.output._console import make_console from nsc.output.colors import ColoredValue from nsc.output.flatten import flatten +from nsc.output.sanitize import safe_markup _STATUS_COLORS: dict[str, str] = { "active": "green", @@ -49,7 +49,7 @@ def render( table = Table(show_header=True, header_style="bold") for col in fieldnames: - table.add_column(header_labels.get(col, col) if header_labels else col) + table.add_column(safe_markup(header_labels.get(col, col) if header_labels else col)) for r in flat_records: table.add_row(*[_format_cell(r.get(col, ""), color=color) for col in fieldnames]) @@ -65,7 +65,7 @@ def _gather_fieldnames(records: list[dict[str, Any]]) -> list[str]: def _colored_markup(value: ColoredValue) -> str: - inner = escape(value.text) + inner = safe_markup(value.text) if value.color is None: return inner return f"[#{value.color}]{inner}[/]" @@ -75,9 +75,9 @@ def _format_colored(value: ColoredValue | list[ColoredValue], *, color: bool) -> # Object colors only ever reach here when color is on (gated in render), but # escape unconditionally so a bare ColoredValue can't inject markup. if isinstance(value, ColoredValue): - return _colored_markup(value) if color else escape(value.text) + return _colored_markup(value) if color else safe_markup(value.text) if not color: - return escape(", ".join(v.text for v in value)) + return safe_markup(", ".join(v.text for v in value)) return ", ".join(_colored_markup(v) for v in value) @@ -99,14 +99,14 @@ def _format_cell(value: Any, *, color: bool = False) -> str: text = str(value) # Rich parses markup in table cells regardless of the no-color setting, so - # arbitrary cell values must be escaped on every return path. + # arbitrary cell values must go through `safe_markup` on every return path. if not color: - return escape(text) + return safe_markup(text) if not text: return "[dim]-[/]" style = _STATUS_COLORS.get(text.lower()) if style: - return f"[{style}]{escape(text)}[/]" - return escape(text) + return f"[{style}]{safe_markup(text)}[/]" + return safe_markup(text) diff --git a/nsc/tui/errors.py b/nsc/tui/errors.py index 52f4406..112cfe4 100644 --- a/nsc/tui/errors.py +++ b/nsc/tui/errors.py @@ -10,9 +10,16 @@ import json from nsc.http.errors import NetBoxAPIError, NetBoxClientError +from nsc.output.sanitize import safe_markup def api_error_message(exc: Exception) -> str: + """The message as Textual markup: `notify` parses markup, and the text is + server data, so it is passed through `safe_markup` (#206).""" + return safe_markup(_message(exc)) + + +def _message(exc: Exception) -> str: if isinstance(exc, NetBoxAPIError): detail = _format_body(exc.body_snippet) return f"API {exc.status_code} — {detail}" if detail else f"API {exc.status_code}" diff --git a/nsc/tui/screens/detail.py b/nsc/tui/screens/detail.py index 9474133..5cf01b7 100644 --- a/nsc/tui/screens/detail.py +++ b/nsc/tui/screens/detail.py @@ -22,6 +22,7 @@ from nsc.http.errors import NetBoxAPIError, NetBoxClientError from nsc.model.command_model import CommandModel, Resource from nsc.output.flatten import flatten +from nsc.output.sanitize import safe_text from nsc.tui._bindings import textual_bindings from nsc.tui.errors import api_error_message from nsc.tui.fk import is_fk_value, resolve_fk_target @@ -121,7 +122,9 @@ def _refresh_rows(self) -> None: for row in self._rows: dirty = row.name in patch label = f"* {row.name}" if dirty else row.name - table.add_row(label, self._value_display(row, flat)) + value = self._value_display(row, flat) + # DataTable parses a plain `str` as markup; keys and values are server data. + table.add_row(safe_text(label), value if isinstance(value, Text) else safe_text(value)) if table.row_count: table.move_cursor(row=min(cursor, table.row_count - 1)) diff --git a/nsc/tui/screens/record_picker.py b/nsc/tui/screens/record_picker.py index 46d5040..a5f19ee 100644 --- a/nsc/tui/screens/record_picker.py +++ b/nsc/tui/screens/record_picker.py @@ -14,6 +14,7 @@ from nsc.http.errors import NetBoxAPIError, NetBoxClientError from nsc.model.command_model import Operation +from nsc.output.sanitize import safe_text from nsc.tui.errors import api_error_message _PAGE_LIMIT = 50 @@ -88,7 +89,7 @@ async def _populate(self, records: list[dict[str, Any]]) -> None: await lv.clear() highlight = 0 for idx, record in enumerate(records): - item = ListItem(Label(_label(record))) + item = ListItem(Label(safe_text(_label(record)))) item.data = record # type: ignore[attr-defined] lv.append(item) if self._current_id is not None and record.get("id") == self._current_id: diff --git a/nsc/tui/view.py b/nsc/tui/view.py index 732b778..d1dc097 100644 --- a/nsc/tui/view.py +++ b/nsc/tui/view.py @@ -9,6 +9,7 @@ from nsc.model.command_model import Operation from nsc.output.colors import ColoredValue from nsc.output.flatten import flatten +from nsc.output.sanitize import safe_text _FALLBACK = ["id"] @@ -60,9 +61,7 @@ def build_rows( def _colored_text(value: ColoredValue) -> Text: - if value.color is None: - return Text(value.text) - return Text(value.text, style=f"#{value.color}") + return safe_text(value.text, style=f"#{value.color}" if value.color else "") def render_cell(value: Any) -> str | Text: @@ -83,4 +82,5 @@ def render_cell(value: Any) -> str | Text: out.append(", ") out.append_text(_colored_text(item)) return out - return str(value) + # A plain `str` cell is parsed as markup by DataTable (#206). + return safe_text(str(value)) diff --git a/nsc/tui/widgets/bulk_diff.py b/nsc/tui/widgets/bulk_diff.py index 011a27b..be18ba9 100644 --- a/nsc/tui/widgets/bulk_diff.py +++ b/nsc/tui/widgets/bulk_diff.py @@ -11,8 +11,7 @@ from typing import ClassVar -from rich.markup import escape - +from nsc.output.sanitize import safe_markup from nsc.tui.bulk import RecordChange from nsc.tui.widgets._modal import ConfirmModalBase @@ -29,14 +28,14 @@ def render_text(self) -> str: if not self._changes: lines.append("[dim]No changes.[/dim]") for change in self._changes: - lines.append(f"record #{escape(str(change.record_id))}") + lines.append(f"record #{safe_markup(str(change.record_id))}") if not change.rows: lines.append(" [dim]unchanged[/dim]") continue for row in change.rows: - field = escape(row.field) - old = escape(row.old_display) - new = escape(row.new_display) + field = safe_markup(row.field) + old = safe_markup(row.old_display) + new = safe_markup(row.new_display) lines.append(f" {field}: {old} -> {new}") lines.append("") lines.append("[dim]Enter/y confirm · Esc/n cancel[/dim]") diff --git a/nsc/tui/widgets/bulk_summary.py b/nsc/tui/widgets/bulk_summary.py index 41a57f1..3cfb7e4 100644 --- a/nsc/tui/widgets/bulk_summary.py +++ b/nsc/tui/widgets/bulk_summary.py @@ -10,13 +10,13 @@ from typing import ClassVar -from rich.markup import escape from textual.app import ComposeResult from textual.binding import BindingType from textual.containers import VerticalScroll from textual.screen import ModalScreen from textual.widgets import Static +from nsc.output.sanitize import safe_markup from nsc.tui.bulk import BulkResult @@ -43,7 +43,9 @@ def render_text(self) -> str: lines.append("") lines.append("[b]Failures[/b]") for failure in result.failures: - lines.append(f" #{escape(str(failure.record_id))}: {escape(failure.error)}") + lines.append( + f" #{safe_markup(str(failure.record_id))}: {safe_markup(failure.error)}" + ) lines.append("") lines.append("[dim]Enter/Esc close[/dim]") return "\n".join(lines) diff --git a/nsc/tui/widgets/diff.py b/nsc/tui/widgets/diff.py index 216c61f..91c3513 100644 --- a/nsc/tui/widgets/diff.py +++ b/nsc/tui/widgets/diff.py @@ -9,8 +9,7 @@ from typing import ClassVar -from rich.markup import escape - +from nsc.output.sanitize import safe_markup from nsc.tui.forms import DiffRow from nsc.tui.widgets._modal import ConfirmModalBase @@ -27,9 +26,9 @@ def render_text(self) -> str: if not self._rows: lines.append("[dim]No changes.[/dim]") for row in self._rows: - field = escape(row.field) - old = escape(row.old_display) - new = escape(row.new_display) + field = safe_markup(row.field) + old = safe_markup(row.old_display) + new = safe_markup(row.new_display) lines.append(f" {field}: {old} -> {new}") lines.append("") lines.append("[dim]Enter/y confirm · Esc/n cancel[/dim]") diff --git a/tests/output/test_sanitize.py b/tests/output/test_sanitize.py new file mode 100644 index 0000000..7929c3f --- /dev/null +++ b/tests/output/test_sanitize.py @@ -0,0 +1,59 @@ +"""Server text shown on a terminal can't carry control sequences or markup (#206).""" + +from __future__ import annotations + +import io + +from nsc.output import table +from nsc.output.sanitize import printable, safe_markup + +EVIL = "x\x1b]0;pwned\x07\x1b[2J\u202e" + + +def test_printable_escapes_control_and_format_characters() -> None: + assert printable(EVIL) == "x\\x1b]0;pwned\\x07\\x1b[2J\\u202e" + + +def test_printable_keeps_newlines_tabs_and_ordinary_unicode() -> None: + text = "café ☃ 東京\tcol\nnext" + assert printable(text) == text + + +def test_safe_markup_also_escapes_rich_markup() -> None: + assert safe_markup("[bold]x\x1b[/]") == "\\[bold]x\\x1b\\[/]" + + +def _table(data: object, *, color: bool) -> str: + buf = io.StringIO() + table.render(data, stream=buf, color=color) # type: ignore[arg-type] + return buf.getvalue() + + +def test_table_cells_and_headers_are_sanitized() -> None: + for color in (False, True): + out = _table([{"name": EVIL, f"k{EVIL}": "v"}], color=color) + assert "\x1b]0;pwned" not in out + assert "\x07" not in out + assert "\u202e" not in out + assert "pwned" in out + + +def test_table_headers_are_not_parsed_as_markup() -> None: + out = _table([{"[red]key[/]": "v"}], color=False) + assert "[red]key[/]" in out + + +def test_explain_panel_is_sanitized() -> None: + from nsc.output.explain import ExplainTrace, render_to_rich_stdout # noqa: PLC0415 + + trace = ExplainTrace( + operation_id=f"op{EVIL}", + operation_summary=f"summary{EVIL}", + method_reasoning=f"m{EVIL}", + url_reasoning=f"u{EVIL}", + ) + buf = io.StringIO() + render_to_rich_stdout(trace, stream=buf, color=False) + out = buf.getvalue() + assert "\x1b" not in out + assert "\u202e" not in out diff --git a/tests/tui/test_detail_screen.py b/tests/tui/test_detail_screen.py index fc0871a..5cf31fd 100644 --- a/tests/tui/test_detail_screen.py +++ b/tests/tui/test_detail_screen.py @@ -202,8 +202,9 @@ async def test_staged_sensitive_field_renders_masked_in_table() -> None: table = screen._table auth_row = next(i for i, r in enumerate(screen._rows) if r.name == "auth_key") cell = table.get_cell_at(Coordinate(auth_row, 1)) - assert cell == "****" - assert "topsecret" not in str(cell) + assert isinstance(cell, Text) + assert cell.plain == "****" + assert "topsecret" not in cell.plain @pytest.mark.asyncio diff --git a/tests/tui/test_sanitize_tui.py b/tests/tui/test_sanitize_tui.py new file mode 100644 index 0000000..d69264d --- /dev/null +++ b/tests/tui/test_sanitize_tui.py @@ -0,0 +1,80 @@ +"""TUI cells and messages don't render server text as markup or control codes (#206).""" + +from __future__ import annotations + +from typing import Any + +import pytest +from rich.text import Text +from textual.app import App, ComposeResult +from textual.widgets import DataTable, Label, ListView, Static + +from nsc.http.errors import NetBoxAPIError +from nsc.output.colors import ColoredValue +from nsc.tui.errors import api_error_message +from nsc.tui.screens.detail import DetailScreen +from nsc.tui.view import render_cell +from tests.tui.test_detail_screen import _model, _SpyClient +from tests.tui.test_record_picker import _FakeClient, _PickerApp + +EVIL = "[bold red]x[/] \x1b]0;pwned\x07" +SHOWN = "[bold red]x[/] \\x1b]0;pwned\\x07" + + +def _plain(cell: object) -> str: + # A plain `str` would be parsed as markup by DataTable/Label; Textual's own + # `Content` (from Label.render) is already literal. + assert not isinstance(cell, str), cell + return str(cell.plain) # type: ignore[attr-defined] + + +def test_render_cell_returns_sanitized_text_not_markup() -> None: + assert _plain(render_cell(EVIL)) == SHOWN + assert _plain(render_cell(ColoredValue(text=EVIL, color="ff0000"))) == SHOWN + assert render_cell(None) == "" + + +def test_api_error_message_is_safe_for_markup_notifications() -> None: + exc = NetBoxAPIError(status_code=500, url="https://nb/api/x/", body_snippet=EVIL, headers={}) + message = api_error_message(exc) + assert "\x1b" not in message + assert Text.from_markup(message).plain == f"API 500 — {SHOWN}" + + +class _EvilDetailApp(App[None]): + def __init__(self) -> None: + super().__init__() + self.client = _SpyClient() + + def compose(self) -> ComposeResult: + yield Static("") + + async def on_mount(self) -> None: + model = _model() + resource = model.tags["dcim"].resources["devices"] + record: dict[str, Any] = {"id": 7, "name": EVIL, f"k{EVIL}": EVIL} + screen = DetailScreen(model, self.client, "dcim", "devices", resource, record) + await self.push_screen(screen) + + +@pytest.mark.asyncio +async def test_detail_cells_are_sanitized_text() -> None: + app = _EvilDetailApp() + async with app.run_test() as pilot: + await pilot.pause() + table = app.screen.query_one("#fields", DataTable) + rows = [table.get_row_at(i) for i in range(table.row_count)] + cells = {_plain(label): _plain(value) for label, value in rows} + assert cells["name"] == SHOWN + assert cells[f"k{SHOWN}"] == SHOWN + + +@pytest.mark.asyncio +async def test_record_picker_labels_are_sanitized_text() -> None: + app = _PickerApp(_FakeClient([{"id": 1, "display": EVIL}])) + async with app.run_test() as pilot: + await pilot.pause() + await app.workers.wait_for_complete() + await pilot.pause() + label = app.screen.query_one(ListView).query_one(Label) + assert _plain(label.render()) == SHOWN diff --git a/tests/tui/test_view.py b/tests/tui/test_view.py index e4f1350..e2ec858 100644 --- a/tests/tui/test_view.py +++ b/tests/tui/test_view.py @@ -39,16 +39,23 @@ def test_choose_columns_prefers_configured_then_default_then_sample() -> None: assert choose_columns(_op(None), None, None) == ["id"] +def _plain(rows: list[list[str | Text]]) -> list[list[str]]: + return [[c.plain if isinstance(c, Text) else c for c in row] for row in rows] + + def test_build_rows_flattens_to_selected_columns_as_strings() -> None: records = [{"id": 1, "site": {"display": "HQ"}}, {"id": 2, "site": None}] rows = build_rows(records, ["id", "site.display"]) - assert rows == [["1", "HQ"], ["2", ""]] + assert _plain(rows) == [["1", "HQ"], ["2", ""]] -def test_build_rows_without_object_colors_returns_plain_str() -> None: +def test_build_rows_without_object_colors_returns_unstyled_text() -> None: records = [{"role": {"display": "Router", "color": "4caf50"}}] rows = build_rows(records, ["role"]) - assert rows == [["Router"]] + assert _plain(rows) == [["Router"]] + cell = rows[0][0] + assert isinstance(cell, Text) + assert not cell.style def test_build_rows_object_colors_produces_styled_text() -> None: @@ -97,7 +104,7 @@ def test_build_rows_object_colors_mixed_tag_list_joins_cleanly() -> None: assert "#ff0000" in styles -def test_build_rows_object_colors_object_without_color_is_plain_str() -> None: +def test_build_rows_object_colors_object_without_color_is_unstyled() -> None: records = [{"role": {"display": "Router"}}] rows = build_rows(records, ["role"], object_colors=True) - assert rows == [["Router"]] + assert _plain(rows) == [["Router"]] From 6d58c2fef8120bd5852d734900e0ed14da7836ea Mon Sep 17 00:00:00 2001 From: Thomas Christory Date: Sun, 25 Oct 2026 00:19:26 +0200 Subject: [PATCH 2/2] fix(tui): sanitize global search, FK buttons and column chooser labels Review of #207 found more widgets that parse a str label as markup and take server text: global-search tree leaves, FK buttons in the edit, bulk-edit and filter forms (initial label and after picking), and the column chooser (whose literal "[x]" mark was itself parsed as markup). Claude-Session: https://claude.ai/code/session_01RD8NgCVtp3yYFd6NSZpbUb --- CHANGELOG.md | 4 ++-- nsc/tui/screens/bulk_edit_form.py | 9 ++++---- nsc/tui/screens/columns.py | 3 ++- nsc/tui/screens/edit_form.py | 5 +++-- nsc/tui/screens/filter.py | 6 ++++-- nsc/tui/screens/global_search.py | 3 ++- tests/tui/test_sanitize_tui.py | 35 ++++++++++++++++++++++++++++++- 7 files changed, 52 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8845e9..22fed17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,8 +23,8 @@ All notable changes to netbox-super-cli are tracked here. Format follows [Keep a - table output, including column headers, which weren't escaped at all; - the `--explain` panel; - error panels; - - the TUI: list and detail cells, record picker, error notifications and - diff views. + - the TUI: list and detail cells, global search results, record picker, FK + buttons, column chooser, error notifications and diff views. In the TUI, values were also parsed as markup, so a value like `[red]x[/]` was styled and lost its brackets. They now render literally. diff --git a/nsc/tui/screens/bulk_edit_form.py b/nsc/tui/screens/bulk_edit_form.py index 5de9e3c..2e4c5c0 100644 --- a/nsc/tui/screens/bulk_edit_form.py +++ b/nsc/tui/screens/bulk_edit_form.py @@ -30,6 +30,7 @@ from textual.widgets.selection_list import Selection from nsc.model.command_model import CommandModel, Operation +from nsc.output.sanitize import safe_text from nsc.savedfilters.custom_fields import CustomFieldDef from nsc.savedfilters.tags import TagDef from nsc.tui._bindings import textual_bindings @@ -199,7 +200,7 @@ def _compose_fk(self, name: str) -> ComposeResult: yield Label(target.hint, classes="bulk-fk-hint") return yield Button( - f"{name}: {self._fk_seed_label(name)}", + safe_text(f"{name}: {self._fk_seed_label(name)}"), id=f"fk-{encode_field_id(name)}", classes="bulk-fk", ) @@ -352,9 +353,9 @@ def _stage(result: tuple[int, str] | None) -> None: if result is not None: self._values[name] = result[0] self._fk_labels[name] = result[1] - self.query_one( - f"#fk-{encode_field_id(name)}", Button - ).label = f"{name}: {result[1]}" + self.query_one(f"#fk-{encode_field_id(name)}", Button).label = safe_text( + f"{name}: {result[1]}" + ) self.app.push_screen(RecordPicker(self._client, target.list_op, target.current_id), _stage) diff --git a/nsc/tui/screens/columns.py b/nsc/tui/screens/columns.py index f7e4a48..9673ac1 100644 --- a/nsc/tui/screens/columns.py +++ b/nsc/tui/screens/columns.py @@ -10,6 +10,7 @@ from textual.screen import ModalScreen from textual.widgets import Label, ListItem, ListView +from nsc.output.sanitize import safe_text from nsc.tui.columns import ColumnSelection _HINT = "space toggle · shift+↑/↓ reorder · ↑/↓ move · Enter apply · Esc cancel" @@ -49,7 +50,7 @@ def _rebuild(self, index: int) -> None: listing.clear() for name in self.selection.items: mark = "x" if self.selection.is_visible(name) else " " - listing.append(ListItem(Label(f"[{mark}] {self._labels.get(name, name)}"))) + listing.append(ListItem(Label(safe_text(f"[{mark}] {self._labels.get(name, name)}")))) if self.selection.items: listing.index = max(0, min(index, len(self.selection.items) - 1)) diff --git a/nsc/tui/screens/edit_form.py b/nsc/tui/screens/edit_form.py index eb64c8e..6586adf 100644 --- a/nsc/tui/screens/edit_form.py +++ b/nsc/tui/screens/edit_form.py @@ -18,6 +18,7 @@ from nsc.http.errors import NetBoxAPIError, NetBoxClientError from nsc.model.command_model import CommandModel, Operation +from nsc.output.sanitize import safe_text from nsc.savedfilters.custom_fields import CustomFieldDef from nsc.savedfilters.tags import TagDef from nsc.tui._bindings import textual_bindings @@ -193,7 +194,7 @@ def _compose_fk(self, name: str, value: Any) -> ComposeResult: if isinstance(nested, dict) else ("" if value is None else str(value)) ) - yield Button(f"{name}: {current}", id=f"fk-{name}", classes="edit-fk") + yield Button(safe_text(f"{name}: {current}"), id=f"fk-{name}", classes="edit-fk") @staticmethod def _field_name(ident: str | None) -> str | None: @@ -287,7 +288,7 @@ def _stage(result: tuple[int, str] | None) -> None: if result is not None: self.staged[name] = result[0] self._fk_labels[name] = result[1] - self.query_one(f"#fk-{name}", Button).label = f"{name}: {result[1]}" + self.query_one(f"#fk-{name}", Button).label = safe_text(f"{name}: {result[1]}") self.app.push_screen(RecordPicker(self._client, target.list_op, target.current_id), _stage) diff --git a/nsc/tui/screens/filter.py b/nsc/tui/screens/filter.py index 27cbff1..4705e50 100644 --- a/nsc/tui/screens/filter.py +++ b/nsc/tui/screens/filter.py @@ -14,6 +14,7 @@ import asyncio from typing import Any, ClassVar +from rich.text import Text from textual.app import ComposeResult from textual.binding import Binding, BindingType from textual.containers import Horizontal, Vertical, VerticalScroll @@ -21,6 +22,7 @@ from textual.widgets import Button, Input, Label, ListItem, ListView, Select from nsc.model.command_model import CommandModel, Operation, Parameter +from nsc.output.sanitize import safe_text from nsc.tui.filters import FilterState, common_filters, parse_raw, searchable_filters from nsc.tui.fk import resolve_fk_target @@ -129,10 +131,10 @@ def _common_field(self, param: Parameter) -> ComposeResult: else: yield Input(value=current, id=f"f-{param.name}") - def _fk_button_text(self, name: str) -> str: + def _fk_button_text(self, name: str) -> Text: key = self._apply_key(name) display = self._fk_display.get(key) or self.state.as_params().get(key, "") - return f"{self._label(name)}: {display or _ANY}" + return safe_text(f"{self._label(name)}: {display or _ANY}") @staticmethod def _field_name(ident: str | None) -> str | None: diff --git a/nsc/tui/screens/global_search.py b/nsc/tui/screens/global_search.py index 5475254..faa9e54 100644 --- a/nsc/tui/screens/global_search.py +++ b/nsc/tui/screens/global_search.py @@ -21,6 +21,7 @@ from nsc.http.errors import NetBoxAPIError, NetBoxClientError from nsc.model.command_model import CommandModel +from nsc.output.sanitize import safe_text from nsc.tui.catalog import ResourceRef from nsc.tui.search import global_search_targets, search_target from nsc.tui.widgets.nav_tree import NavTree @@ -140,7 +141,7 @@ def _add_group(self, ref: ResourceRef, rows: list[dict[str, Any]]) -> None: tree = self.query_one("#search-tree", Tree) group = tree.root.add(f"{ref.resource_name} ({len(rows)})", expand=True) for record in rows: - group.add_leaf(_record_label(record), data=_Hit(ref, record)) + group.add_leaf(safe_text(_record_label(record)), data=_Hit(ref, record)) tree.cursor_line = max(tree.cursor_line, 0) def on_tree_node_selected(self, event: Tree.NodeSelected[Any]) -> None: diff --git a/tests/tui/test_sanitize_tui.py b/tests/tui/test_sanitize_tui.py index d69264d..b42159a 100644 --- a/tests/tui/test_sanitize_tui.py +++ b/tests/tui/test_sanitize_tui.py @@ -7,14 +7,17 @@ import pytest from rich.text import Text from textual.app import App, ComposeResult -from textual.widgets import DataTable, Label, ListView, Static +from textual.widgets import DataTable, Label, ListView, Static, Tree from nsc.http.errors import NetBoxAPIError from nsc.output.colors import ColoredValue from nsc.tui.errors import api_error_message from nsc.tui.screens.detail import DetailScreen +from nsc.tui.screens.global_search import GlobalSearchScreen from nsc.tui.view import render_cell +from tests.tui.test_column_chooser import _ChooserApp from tests.tui.test_detail_screen import _model, _SpyClient +from tests.tui.test_global_search import _SearchApp from tests.tui.test_record_picker import _FakeClient, _PickerApp EVIL = "[bold red]x[/] \x1b]0;pwned\x07" @@ -78,3 +81,33 @@ async def test_record_picker_labels_are_sanitized_text() -> None: await pilot.pause() label = app.screen.query_one(ListView).query_one(Label) assert _plain(label.render()) == SHOWN + + +@pytest.mark.asyncio +async def test_global_search_leaves_are_sanitized() -> None: + class _EvilClient: + def paginate(self, path, params=None, *, limit=None): # type: ignore[no-untyped-def] + if "devices" in path: + yield {"id": 7, "name": EVIL} + + app = _SearchApp() + app.client = _EvilClient() # type: ignore[assignment] + async with app.run_test() as pilot: + await pilot.pause() + screen = app.screen + assert isinstance(screen, GlobalSearchScreen) + screen._client = app.client + await screen._run_search("x") + await pilot.pause() + tree = screen.query_one("#search-tree", Tree) + leaf = tree.root.children[0].children[0] + assert _plain(leaf.label) == SHOWN + + +@pytest.mark.asyncio +async def test_column_chooser_labels_render_literally() -> None: + app = _ChooserApp(available=["id", "cf"], visible=["id"], labels={"cf": EVIL}) + async with app.run_test() as pilot: + await pilot.pause() + rendered = [_plain(lbl.render()) for lbl in app.screen.query_one(ListView).query(Label)] + assert rendered == ["[x] id", f"[ ] {SHOWN}"]