mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(extensions): guard non-numeric catalog downloads in search/info rendering (#3710)
* fix(extensions): guard non-numeric catalog downloads in search/info rendering `specify extension search` and `specify extension info <id>` 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) <noreply@anthropic.com> * 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> * fix(extensions): escape 'stars' too, in the same stats string 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ali jawwad
@@ -790,10 +790,24 @@ def extension_search(
|
||||
|
||||
# Stats
|
||||
stats = []
|
||||
if ext.get('downloads') is not None:
|
||||
stats.append(f"Downloads: {ext['downloads']:,}")
|
||||
if ext.get('stars') is not None:
|
||||
stats.append(f"Stars: {ext['stars']}")
|
||||
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,
|
||||
# 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: {_escape_markup(str(downloads))}"
|
||||
)
|
||||
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]")
|
||||
|
||||
@@ -971,10 +985,24 @@ 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']:,}")
|
||||
if ext_info.get('stars') is not None:
|
||||
stats.append(f"Stars: {ext_info['stars']}")
|
||||
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, 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: {_escape_markup(str(downloads))}"
|
||||
)
|
||||
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()
|
||||
|
||||
@@ -4080,6 +4080,91 @@ class TestExtensionCatalog:
|
||||
results = catalog.search()
|
||||
assert len(results) == 2
|
||||
|
||||
@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
|
||||
|
||||
manager = MagicMock()
|
||||
manager.registry.is_installed.return_value = False
|
||||
ext_info = {
|
||||
"name": "Jira", "id": "jira", "version": "1.0.0",
|
||||
"description": "desc", "downloads": downloads, # from catalog JSON
|
||||
}
|
||||
# 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
|
||||
``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
|
||||
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": downloads, # 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
|
||||
# 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."""
|
||||
import yaml as yaml_module
|
||||
|
||||
Reference in New Issue
Block a user