From 9fba0520a01e698b637a917da0e82d79fe452625 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 24 Jul 2026 13:55:21 +0500 Subject: [PATCH 1/3] fix(extensions): guard non-numeric catalog downloads in search/info rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `specify extension search` and `specify extension info ` format a catalog entry's `downloads` field with the `:,` thousands separator, guarded only by `is not None`. Catalog payloads are only shape-validated -- individual fields are never type-checked and `_get_merged_extensions` returns raw catalog dicts -- so an entry with a non-numeric `downloads` (e.g. the JSON string "1500", realistic from a community / SPECKIT_CATALOG_URL / project catalog) makes the `:,` format raise `ValueError: Cannot specify ',' with 's'`, aborting the whole command with an uncaught traceback. Group-format `downloads` only when it is actually numeric; otherwise render it as-is. Numeric values (int/float, incl. bool) format identically, so correct catalogs are byte-for-byte unchanged. Every other field in these two renderers is already `str()`-wrapped; this closes the one unguarded field. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/_commands.py | 24 ++++++++-- tests/test_extensions.py | 59 +++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 7ae4f8e9f0..e0d5334dbd 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -790,8 +790,16 @@ def extension_search( # Stats stats = [] - if ext.get('downloads') is not None: - stats.append(f"Downloads: {ext['downloads']:,}") + downloads = ext.get('downloads') + if downloads is not None: + # Catalog fields are untrusted; a non-numeric ``downloads`` + # (e.g. the JSON string "1500") would crash the ``:,`` format + # with "Cannot specify ',' with 's'". Only group-format numbers. + stats.append( + f"Downloads: {downloads:,}" + if isinstance(downloads, (int, float)) + else f"Downloads: {downloads}" + ) if ext.get('stars') is not None: stats.append(f"Stars: {ext['stars']}") if stats: @@ -971,8 +979,16 @@ def _print_extension_info(ext_info: dict, manager): # Statistics stats = [] - if ext_info.get('downloads') is not None: - stats.append(f"Downloads: {ext_info['downloads']:,}") + downloads = ext_info.get('downloads') + if downloads is not None: + # Catalog fields are untrusted; a non-numeric ``downloads`` (e.g. the + # JSON string "1500") would crash the ``:,`` format with "Cannot + # specify ',' with 's'". Only group-format numbers. + stats.append( + f"Downloads: {downloads:,}" + if isinstance(downloads, (int, float)) + else f"Downloads: {downloads}" + ) if ext_info.get('stars') is not None: stats.append(f"Stars: {ext_info['stars']}") if stats: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 61826aad5b..80670fa1ee 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -4034,6 +4034,65 @@ def test_search_all_extensions(self, temp_dir): results = catalog.search() assert len(results) == 2 + def test_info_renders_non_numeric_downloads(self): + """A non-numeric ``downloads`` from an untrusted catalog must not crash + the info renderer with 'Cannot specify ',' with 's''; it renders as-is.""" + from unittest.mock import MagicMock + from specify_cli.extensions._commands import _print_extension_info + + manager = MagicMock() + manager.registry.is_installed.return_value = False + ext_info = { + "name": "Jira", "id": "jira", "version": "1.0.0", + "description": "desc", "downloads": "1500", # string from catalog JSON + } + # Must not raise ValueError. + _print_extension_info(ext_info, manager) + + def test_search_survives_non_numeric_downloads(self, temp_dir): + """`specify extension search` must not abort with a raw ValueError when a + catalog entry's ``downloads`` is a non-numeric string.""" + import yaml as yaml_module + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = temp_dir / "project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + config_path = project_dir / ".specify" / "extension-catalogs.yml" + with open(config_path, "w") as f: + yaml_module.dump( + {"catalogs": [{ + "name": "test-catalog", + "url": ExtensionCatalog.DEFAULT_CATALOG_URL, + "priority": 1, "install_allowed": True, + }]}, f, + ) + + catalog = ExtensionCatalog(project_dir) + catalog_data = { + "schema_version": "1.0", + "extensions": {"jira": { + "name": "Jira", "id": "jira", "version": "1.0.0", + "description": "Jira integration", "author": "x", + "tags": ["jira"], "verified": True, + "downloads": "1500", # non-numeric, straight from catalog JSON + }}, + } + catalog.cache_dir.mkdir(parents=True, exist_ok=True) + catalog.cache_file.write_text(json.dumps(catalog_data)) + catalog.cache_metadata_file.write_text(json.dumps({ + "cached_at": datetime.now(timezone.utc).isoformat(), + "catalog_url": "http://test.com", + })) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "search"], catch_exceptions=True) + assert result.exit_code == 0, result.output + assert "Downloads: 1500" in result.output + def test_search_by_query(self, temp_dir): """Test searching by query text.""" import yaml as yaml_module From 135444f35955052be727e808525e56b64141aa8d Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 09:17:40 +0500 Subject: [PATCH 2/3] fix(extensions): escape the non-numeric downloads fallback for Rich markup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/specify_cli/extensions/_commands.py | 14 +++++++---- tests/test_extensions.py | 32 +++++++++++++++++-------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index e0d5334dbd..1bbae48494 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -794,11 +794,14 @@ def extension_search( if downloads is not None: # Catalog fields are untrusted; a non-numeric ``downloads`` # (e.g. the JSON string "1500") would crash the ``:,`` format - # with "Cannot specify ',' with 's'". Only group-format numbers. + # with "Cannot specify ',' with 's'". Only group-format numbers, + # and escape the fallback: the joined stats are rendered as Rich + # markup, so a value like "[/red]foo" would raise MarkupError + # (matching how every other catalog field here is escaped). stats.append( f"Downloads: {downloads:,}" if isinstance(downloads, (int, float)) - else f"Downloads: {downloads}" + else f"Downloads: {_escape_markup(str(downloads))}" ) if ext.get('stars') is not None: stats.append(f"Stars: {ext['stars']}") @@ -983,11 +986,14 @@ def _print_extension_info(ext_info: dict, manager): if downloads is not None: # Catalog fields are untrusted; a non-numeric ``downloads`` (e.g. the # JSON string "1500") would crash the ``:,`` format with "Cannot - # specify ',' with 's'". Only group-format numbers. + # specify ',' with 's'". Only group-format numbers, and escape the + # fallback: the joined stats are rendered as Rich markup, so a value + # like "[/red]foo" would raise MarkupError (matching how every other + # catalog field here is escaped). stats.append( f"Downloads: {downloads:,}" if isinstance(downloads, (int, float)) - else f"Downloads: {downloads}" + else f"Downloads: {_escape_markup(str(downloads))}" ) if ext_info.get('stars') is not None: stats.append(f"Stars: {ext_info['stars']}") diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 80670fa1ee..582ebad6c0 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -4034,9 +4034,18 @@ def test_search_all_extensions(self, temp_dir): results = catalog.search() assert len(results) == 2 - def test_info_renders_non_numeric_downloads(self): - """A non-numeric ``downloads`` from an untrusted catalog must not crash - the info renderer with 'Cannot specify ',' with 's''; it renders as-is.""" + @pytest.mark.parametrize( + "downloads", + [ + "1500", # plain string: crashed the ``:,`` format + "[/red]foo", # unbalanced closing tag: raises MarkupError unescaped + "[bold]x[/bold]", # balanced tags: would silently restyle the output + ], + ) + def test_info_renders_non_numeric_downloads(self, downloads): + """A non-numeric ``downloads`` from an untrusted catalog must not crash the + info renderer — neither with 'Cannot specify ',' with 's'' (the ``:,`` + format) nor with a Rich MarkupError (the joined stats are markup).""" from unittest.mock import MagicMock from specify_cli.extensions._commands import _print_extension_info @@ -4044,14 +4053,16 @@ def test_info_renders_non_numeric_downloads(self): manager.registry.is_installed.return_value = False ext_info = { "name": "Jira", "id": "jira", "version": "1.0.0", - "description": "desc", "downloads": "1500", # string from catalog JSON + "description": "desc", "downloads": downloads, # from catalog JSON } - # Must not raise ValueError. + # Must not raise ValueError or rich.errors.MarkupError. _print_extension_info(ext_info, manager) - def test_search_survives_non_numeric_downloads(self, temp_dir): - """`specify extension search` must not abort with a raw ValueError when a - catalog entry's ``downloads`` is a non-numeric string.""" + @pytest.mark.parametrize("downloads", ["1500", "[/red]foo"]) + def test_search_survives_non_numeric_downloads(self, temp_dir, downloads): + """`specify extension search` must not abort when a catalog entry's + ``downloads`` is a non-numeric string — not with a raw ValueError from the + ``:,`` format, nor with a Rich MarkupError from unescaped markup.""" import yaml as yaml_module from typer.testing import CliRunner from unittest.mock import patch @@ -4077,7 +4088,7 @@ def test_search_survives_non_numeric_downloads(self, temp_dir): "name": "Jira", "id": "jira", "version": "1.0.0", "description": "Jira integration", "author": "x", "tags": ["jira"], "verified": True, - "downloads": "1500", # non-numeric, straight from catalog JSON + "downloads": downloads, # non-numeric, straight from catalog JSON }}, } catalog.cache_dir.mkdir(parents=True, exist_ok=True) @@ -4091,7 +4102,8 @@ def test_search_survives_non_numeric_downloads(self, temp_dir): with patch.object(Path, "cwd", return_value=project_dir): result = runner.invoke(app, ["extension", "search"], catch_exceptions=True) assert result.exit_code == 0, result.output - assert "Downloads: 1500" in result.output + # Rendered literally (escaped), not interpreted as markup or dropped. + assert f"Downloads: {downloads}" in result.output def test_search_by_query(self, temp_dir): """Test searching by query text.""" From 348d0eed4da0b28a6658a20ee5620d6aadb6d6a7 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 10:25:39 +0500 Subject: [PATCH 3/3] fix(extensions): escape 'stars' too, in the same stats string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the downloads escaping: `stars` is the other catalog-controlled value joined into the same Rich-rendered stats line, and it was still raw -- verified that stars "[/red]x" raises the same MarkupError and aborts `extension info`/`search`. Hardening one of the two adjacent values would have left the reported defect reachable through the sibling field. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/extensions/_commands.py | 14 ++++++++++---- tests/test_extensions.py | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 1bbae48494..5f2bd35b6a 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -803,8 +803,11 @@ def extension_search( if isinstance(downloads, (int, float)) else f"Downloads: {_escape_markup(str(downloads))}" ) - if ext.get('stars') is not None: - stats.append(f"Stars: {ext['stars']}") + stars = ext.get('stars') + if stars is not None: + # Same untrusted-value/Rich-markup hazard as `downloads` above, + # in the same joined string. + stats.append(f"Stars: {_escape_markup(str(stars))}") if stats: console.print(f" [dim]{' | '.join(stats)}[/dim]") @@ -995,8 +998,11 @@ def _print_extension_info(ext_info: dict, manager): if isinstance(downloads, (int, float)) else f"Downloads: {_escape_markup(str(downloads))}" ) - if ext_info.get('stars') is not None: - stats.append(f"Stars: {ext_info['stars']}") + stars = ext_info.get('stars') + if stars is not None: + # Same untrusted-value/Rich-markup hazard as `downloads` above, in the + # same joined string. + stats.append(f"Stars: {_escape_markup(str(stars))}") if stats: console.print(f"[bold]Statistics:[/bold] {' | '.join(stats)}") console.print() diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 582ebad6c0..8236a0f640 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -4058,6 +4058,20 @@ def test_info_renders_non_numeric_downloads(self, downloads): # Must not raise ValueError or rich.errors.MarkupError. _print_extension_info(ext_info, manager) + def test_info_renders_markup_bearing_stars(self): + """``stars`` sits in the same joined stats string as ``downloads`` and is + equally catalog-controlled, so it must be escaped too.""" + from unittest.mock import MagicMock + from specify_cli.extensions._commands import _print_extension_info + + manager = MagicMock() + manager.registry.is_installed.return_value = False + ext_info = { + "name": "Jira", "id": "jira", "version": "1.0.0", + "description": "desc", "stars": "[/red]x", + } + _print_extension_info(ext_info, manager) # must not raise MarkupError + @pytest.mark.parametrize("downloads", ["1500", "[/red]foo"]) def test_search_survives_non_numeric_downloads(self, temp_dir, downloads): """`specify extension search` must not abort when a catalog entry's