From 4f54179ea46927e553e89f38ca583e8412add1fc Mon Sep 17 00:00:00 2001 From: Alpamys Date: Wed, 20 May 2026 00:12:05 +0500 Subject: [PATCH] fix(security): backfill Rich markup escape in legacy adapters commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #174. Picked up after PR #175 (dreamer0129) went quiet — the list/info path was clean, but compare() escaped only inside the highlight branch, leaving a shared crafted base_model = "[link=evil] click[/]" un-escaped on equal-value rows. Fix follows the "escape always at the value layer, decoration wraps after" pattern mirroring v0.57.0 `adapters diff` / `info`: - list_adapters: wrap base / lora_r / peft_type / rel_path with rich.markup.escape() before table.add_row(); also escape adapter_path in the JSONDecodeError fallback. - info: wrap base_model / peft_type / task_type / lora_r / lora_alpha / lora_dropout / modules_str inside the Rich Panel f-string; also escape adapter_path.name in the Panel title. - compare: escape val1_str / val2_str unconditionally; [yellow] highlight wraps already-escaped values when they differ. Equal-value rows now also escape (was the v0.57.0 known-limitation gap). +4 regression tests in tests/test_adapters.py::TestAdaptersMarkupEscape: - test_list_escapes_crafted_base_model — asserts no ANSI hyperlink sequence (\x1b]8;) leaks from a crafted [link=http://evil/...] payload. - test_info_escapes_crafted_base_model — same assertion for Panel. - test_compare_escapes_equal_crafted_values — the specific regression for the PR #175 review gap (identical crafted values on both sides must NOT smuggle live markup through the equal-branch). - test_compare_escapes_differing_crafted_values — highlight branch also escapes. Closes v0.57.0 Known Limitation (9). Verified locally: - ruff check soup_cli/commands/adapters.py tests/test_adapters.py -> clean - pytest tests/test_adapters.py --no-cov -> 20 passed --- soup_cli/commands/adapters.py | 43 +++++++++++++++------ tests/test_adapters.py | 71 +++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/soup_cli/commands/adapters.py b/soup_cli/commands/adapters.py index 95df359..31aaea6 100644 --- a/soup_cli/commands/adapters.py +++ b/soup_cli/commands/adapters.py @@ -94,9 +94,18 @@ def list_adapters( rel_path = adapter_path.relative_to(dir_path) except ValueError: rel_path = adapter_path - table.add_row(str(rel_path), base, lora_r, peft_type, size) + # Escape every adapter-config-sourced value — a crafted + # base_model_name_or_path like "[link=evil]click[/]" would + # otherwise render as live Rich markup in the terminal. + table.add_row( + escape(str(rel_path)), + escape(str(base)), + escape(lora_r), + escape(str(peft_type)), + size, + ) except (json.JSONDecodeError, OSError): - table.add_row(str(adapter_path), "[red]error[/]", "-", "-", "-") + table.add_row(escape(str(adapter_path)), "[red]error[/]", "-", "-", "-") console.print(table) console.print(f"\n[dim]Found {len(adapters)} adapter(s).[/]") @@ -134,17 +143,22 @@ def info( else: modules_str = str(target_modules) + # Escape every adapter-config-sourced value before embedding into + # Rich markup. A crafted base_model_name_or_path like + # "[link=http://evil]click[/]" would otherwise render as a live + # clickable link in the terminal. info_text = ( - f"Base model: [bold]{base_model}[/]\n" - f"PEFT type: [bold]{peft_type}[/]\n" - f"Task: [bold]{task_type}[/]\n" - f"LoRA rank: [bold]{lora_r}[/], alpha: [bold]{lora_alpha}[/], " - f"dropout: [bold]{lora_dropout}[/]\n" - f"Targets: [bold]{modules_str}[/]\n" + f"Base model: [bold]{escape(str(base_model))}[/]\n" + f"PEFT type: [bold]{escape(str(peft_type))}[/]\n" + f"Task: [bold]{escape(str(task_type))}[/]\n" + f"LoRA rank: [bold]{escape(str(lora_r))}[/], " + f"alpha: [bold]{escape(str(lora_alpha))}[/], " + f"dropout: [bold]{escape(str(lora_dropout))}[/]\n" + f"Targets: [bold]{escape(modules_str)}[/]\n" f"Size on disk: [bold]{size}[/]" ) - console.print(Panel(info_text, title=f"Adapter Info -- {adapter_path.name}")) + console.print(Panel(info_text, title=f"Adapter Info -- {escape(adapter_path.name)}")) @app.command() @@ -194,10 +208,15 @@ def compare( if isinstance(val2, list): val2 = ", ".join(str(item) for item in val2) - val1_str = str(val1) - val2_str = str(val2) + # Escape always at the value layer; decoration wraps after. + # Mirrors v0.57.0 `adapters diff` / `info` policy — equal-value + # rows must NOT skip escape just because the highlight branch + # doesn't fire (otherwise a shared crafted value like + # "[link=evil]click[/]" still injects live markup). + val1_str = escape(str(val1)) + val2_str = escape(str(val2)) - # Highlight differences + # Highlight differences (already-escaped values wrap in yellow). if val1_str != val2_str: val1_str = f"[yellow]{val1_str}[/]" val2_str = f"[yellow]{val2_str}[/]" diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 56886ce..808ce8d 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -156,6 +156,77 @@ class TestAdaptersCompare: assert result.exit_code != 0 +class TestAdaptersMarkupEscape: + """Regression tests for Rich-markup escape across list/info/compare. + + Closes issue #174 (v0.57.0 Known Limitation 9) — a crafted + `adapter_config.json` value like `base_model_name_or_path: "[link=evil] + click[/]"` previously rendered as live markup in the terminal. + Every adapter-config-sourced value must pass through `rich.markup.escape` + before embedding in a Rich f-string. + """ + + CRAFTED_MARKUP = "[link=http://evil.example/]click-me[/]" + CRAFTED_BOLD = "[red bold]injected[/]" + + def test_list_escapes_crafted_base_model(self, tmp_path): + """list: crafted base_model renders as literal `[link=...]` text.""" + _create_adapter(tmp_path / "adapter_a", base_model=self.CRAFTED_MARKUP) + result = runner.invoke(app, ["adapters", "list", str(tmp_path)]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + # Literal `[link=` substring must survive in output (escaped form + # uses \[link= in Rich-rendered ANSI; absence of an actual ANSI + # link sequence is the security signal). + assert "click-me" in result.output + assert "\x1b]8;" not in result.output, ( + "Rich rendered crafted [link=...] as a real ANSI hyperlink " + "— escape() is not being applied to base_model_name_or_path." + ) + + def test_info_escapes_crafted_base_model(self, tmp_path): + """info: crafted base_model in Panel renders as literal text.""" + adapter = tmp_path / "adapter_info" + _create_adapter(adapter, base_model=self.CRAFTED_MARKUP) + result = runner.invoke(app, ["adapters", "info", str(adapter)]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + assert "click-me" in result.output + assert "\x1b]8;" not in result.output, ( + "Rich rendered crafted [link=...] as a real ANSI hyperlink " + "in `adapters info` — escape() missing on base_model." + ) + + def test_compare_escapes_equal_crafted_values(self, tmp_path): + """compare: shared crafted value MUST escape even when highlight branch skips. + + Regression for the security gap surfaced in PR #175 review: + the original patch only escaped inside the `val1_str != val2_str` + branch, leaving equal-value rows un-escaped. + """ + a = tmp_path / "adapter_left" + b = tmp_path / "adapter_right" + _create_adapter(a, base_model=self.CRAFTED_BOLD) + _create_adapter(b, base_model=self.CRAFTED_BOLD) # IDENTICAL crafted value + result = runner.invoke(app, ["adapters", "compare", str(a), str(b)]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + # Literal "injected" text must appear (the [red bold] tag must + # NOT be interpreted as markup styling). + assert "injected" in result.output + # A live `[red bold]` rendered tag would emit ANSI color codes + # for the inner text alone; assert the raw bracketed token + # literal is present (escape() preserves it). + assert "[red bold]" in result.output or r"\[red bold]" in result.output + + def test_compare_escapes_differing_crafted_values(self, tmp_path): + """compare: crafted values that DIFFER (highlight branch fires) also escape.""" + a = tmp_path / "adapter_left" + b = tmp_path / "adapter_right" + _create_adapter(a, base_model=self.CRAFTED_BOLD) + _create_adapter(b, base_model="meta-llama/Llama-3.1-8B") # benign + result = runner.invoke(app, ["adapters", "compare", str(a), str(b)]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + assert "injected" in result.output + + class TestAdapterDiscovery: """Test adapter discovery helper function."""