Skip to content

Commit 135444f

Browse files
jawwad-aliclaude
andcommitted
fix(extensions): escape the non-numeric downloads fallback for Rich markup
Address review feedback: the fallback interpolated the untrusted catalog value straight into a Rich-rendered string, so guarding the ``:,`` ValueError just traded it for a MarkupError -- a catalog entry with downloads "[/red]foo" still aborted `extension search`/`info`, and balanced tags could restyle the output. Wrap the fallback in _escape_markup(str(...)) at both sites, matching how every other catalog field in these renderers is already escaped. Numeric values keep the identical ``:,`` branch, so correct catalogs are unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9fba052 commit 135444f

2 files changed

Lines changed: 32 additions & 14 deletions

File tree

src/specify_cli/extensions/_commands.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -794,11 +794,14 @@ def extension_search(
794794
if downloads is not None:
795795
# Catalog fields are untrusted; a non-numeric ``downloads``
796796
# (e.g. the JSON string "1500") would crash the ``:,`` format
797-
# with "Cannot specify ',' with 's'". Only group-format numbers.
797+
# with "Cannot specify ',' with 's'". Only group-format numbers,
798+
# and escape the fallback: the joined stats are rendered as Rich
799+
# markup, so a value like "[/red]foo" would raise MarkupError
800+
# (matching how every other catalog field here is escaped).
798801
stats.append(
799802
f"Downloads: {downloads:,}"
800803
if isinstance(downloads, (int, float))
801-
else f"Downloads: {downloads}"
804+
else f"Downloads: {_escape_markup(str(downloads))}"
802805
)
803806
if ext.get('stars') is not None:
804807
stats.append(f"Stars: {ext['stars']}")
@@ -983,11 +986,14 @@ def _print_extension_info(ext_info: dict, manager):
983986
if downloads is not None:
984987
# Catalog fields are untrusted; a non-numeric ``downloads`` (e.g. the
985988
# JSON string "1500") would crash the ``:,`` format with "Cannot
986-
# specify ',' with 's'". Only group-format numbers.
989+
# specify ',' with 's'". Only group-format numbers, and escape the
990+
# fallback: the joined stats are rendered as Rich markup, so a value
991+
# like "[/red]foo" would raise MarkupError (matching how every other
992+
# catalog field here is escaped).
987993
stats.append(
988994
f"Downloads: {downloads:,}"
989995
if isinstance(downloads, (int, float))
990-
else f"Downloads: {downloads}"
996+
else f"Downloads: {_escape_markup(str(downloads))}"
991997
)
992998
if ext_info.get('stars') is not None:
993999
stats.append(f"Stars: {ext_info['stars']}")

tests/test_extensions.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4034,24 +4034,35 @@ def test_search_all_extensions(self, temp_dir):
40344034
results = catalog.search()
40354035
assert len(results) == 2
40364036

4037-
def test_info_renders_non_numeric_downloads(self):
4038-
"""A non-numeric ``downloads`` from an untrusted catalog must not crash
4039-
the info renderer with 'Cannot specify ',' with 's''; it renders as-is."""
4037+
@pytest.mark.parametrize(
4038+
"downloads",
4039+
[
4040+
"1500", # plain string: crashed the ``:,`` format
4041+
"[/red]foo", # unbalanced closing tag: raises MarkupError unescaped
4042+
"[bold]x[/bold]", # balanced tags: would silently restyle the output
4043+
],
4044+
)
4045+
def test_info_renders_non_numeric_downloads(self, downloads):
4046+
"""A non-numeric ``downloads`` from an untrusted catalog must not crash the
4047+
info renderer — neither with 'Cannot specify ',' with 's'' (the ``:,``
4048+
format) nor with a Rich MarkupError (the joined stats are markup)."""
40404049
from unittest.mock import MagicMock
40414050
from specify_cli.extensions._commands import _print_extension_info
40424051

40434052
manager = MagicMock()
40444053
manager.registry.is_installed.return_value = False
40454054
ext_info = {
40464055
"name": "Jira", "id": "jira", "version": "1.0.0",
4047-
"description": "desc", "downloads": "1500", # string from catalog JSON
4056+
"description": "desc", "downloads": downloads, # from catalog JSON
40484057
}
4049-
# Must not raise ValueError.
4058+
# Must not raise ValueError or rich.errors.MarkupError.
40504059
_print_extension_info(ext_info, manager)
40514060

4052-
def test_search_survives_non_numeric_downloads(self, temp_dir):
4053-
"""`specify extension search` must not abort with a raw ValueError when a
4054-
catalog entry's ``downloads`` is a non-numeric string."""
4061+
@pytest.mark.parametrize("downloads", ["1500", "[/red]foo"])
4062+
def test_search_survives_non_numeric_downloads(self, temp_dir, downloads):
4063+
"""`specify extension search` must not abort when a catalog entry's
4064+
``downloads`` is a non-numeric string — not with a raw ValueError from the
4065+
``:,`` format, nor with a Rich MarkupError from unescaped markup."""
40554066
import yaml as yaml_module
40564067
from typer.testing import CliRunner
40574068
from unittest.mock import patch
@@ -4077,7 +4088,7 @@ def test_search_survives_non_numeric_downloads(self, temp_dir):
40774088
"name": "Jira", "id": "jira", "version": "1.0.0",
40784089
"description": "Jira integration", "author": "x",
40794090
"tags": ["jira"], "verified": True,
4080-
"downloads": "1500", # non-numeric, straight from catalog JSON
4091+
"downloads": downloads, # non-numeric, straight from catalog JSON
40814092
}},
40824093
}
40834094
catalog.cache_dir.mkdir(parents=True, exist_ok=True)
@@ -4091,7 +4102,8 @@ def test_search_survives_non_numeric_downloads(self, temp_dir):
40914102
with patch.object(Path, "cwd", return_value=project_dir):
40924103
result = runner.invoke(app, ["extension", "search"], catch_exceptions=True)
40934104
assert result.exit_code == 0, result.output
4094-
assert "Downloads: 1500" in result.output
4105+
# Rendered literally (escaped), not interpreted as markup or dropped.
4106+
assert f"Downloads: {downloads}" in result.output
40954107

40964108
def test_search_by_query(self, temp_dir):
40974109
"""Test searching by query text."""

0 commit comments

Comments
 (0)