From a774cea0e9609bb2be919efa810cdcaa5d41a055 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Wed, 17 Jun 2026 20:52:57 +0800 Subject: [PATCH 01/11] fix(extensions): close path traversal and make `list --available` query the catalog - extension add --from: sanitize the extension label before building the download filename so "../" path separators can no longer escape the downloads dir and overwrite arbitrary files - extension list --available/--all: actually query the catalog and list uninstalled extensions (filtering out installed IDs), instead of only printing a static install hint that contradicted the CLI help and docs --- src/specify_cli/extensions/_commands.py | 44 +++++++++++++++++++++---- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 1e78ee8116..d2d7ce78ce 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -386,19 +386,24 @@ def extension_list( all_extensions: bool = typer.Option(False, "--all", help="Show both installed and available"), ): """List installed extensions.""" - from . import ExtensionManager + from . import ExtensionManager, ExtensionCatalog, ExtensionError project_root = _require_specify_project() manager = ExtensionManager(project_root) installed = manager.list_installed() - if not installed and not (available or all_extensions): + # Default (no flags) lists installed; --all also lists installed. + # --available alone lists only catalog extensions, not installed. + show_installed = all_extensions or not available + show_available = available or all_extensions + + if not installed and not show_available: console.print("[yellow]No extensions installed.[/yellow]") console.print("\nInstall an extension with:") console.print(" specify extension add ") return - if installed: + if show_installed and installed: console.print("\n[bold cyan]Installed Extensions:[/bold cyan]\n") for ext in installed: @@ -411,9 +416,36 @@ def extension_list( console.print(f" Commands: {ext['command_count']} | Hooks: {ext['hook_count']} | Priority: {ext['priority']} | Status: {'Enabled' if ext['enabled'] else 'Disabled'}") console.print() - if available or all_extensions: - console.print("\nInstall an extension:") - console.print(" [cyan]specify extension add [/cyan]") + if show_available: + # Query the catalog and show extensions that are not already installed. + catalog = ExtensionCatalog(project_root) + installed_ids = {ext["id"] for ext in installed} + + try: + results = catalog.search() + except ExtensionError as e: + console.print(f"\n[red]Error:[/red] Could not query extension catalog: {e}") + console.print("[dim]The catalog may be temporarily unavailable. Try again later.[/dim]") + raise typer.Exit(1) + + available_exts = [ext for ext in results if ext.get("id") not in installed_ids] + + console.print("\n[bold cyan]Available Extensions:[/bold cyan]\n") + if not available_exts: + console.print(" [dim]No additional extensions available in the catalog.[/dim]") + else: + for ext in available_exts: + verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" + console.print(f" [bold]{ext['name']}[/bold] (v{ext['version']}){verified_badge}") + console.print(f" [dim]{ext['id']}[/dim]") + console.print(f" {ext.get('description', '')}") + install_allowed = ext.get("_install_allowed", True) + if install_allowed: + console.print(f" [cyan]Install:[/cyan] specify extension add {ext['id']}") + else: + catalog_name = ext.get("_catalog_name", "") + console.print(f" [yellow]Discovery only — not installable from '{catalog_name}'[/yellow]") + console.print() @catalog_app.command("list") From 593c74ead870c4b104dbf8c98105364a136273e5 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Thu, 18 Jun 2026 12:01:54 +0800 Subject: [PATCH 02/11] test(extensions): cover list --available catalog query and add --from path traversal Add regression coverage for the two behaviors wired up in the preceding fix: - list --available/--all: queries the catalog, filters installed IDs, marks discovery-only entries, reports an empty catalog, and exits 1 on catalog failure. - add --from : a label containing path separators is sanitized so the download cannot escape the downloads cache dir. Both suites were verified red against the pre-fix behavior and green after. --- tests/test_extension_list_available.py | 135 +++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 tests/test_extension_list_available.py diff --git a/tests/test_extension_list_available.py b/tests/test_extension_list_available.py new file mode 100644 index 0000000000..911a6d2881 --- /dev/null +++ b/tests/test_extension_list_available.py @@ -0,0 +1,135 @@ +"""Behavior tests for `specify extension list --available/--all`. + +These flags were documented from the original extension system (#1551) as +"Show available extensions from catalog", but the implementation was a static +hint that never queried the catalog. This suite covers the wired-up behavior: +the catalog is queried, already-installed IDs are filtered out, and a clear +error is surfaced when the catalog is unavailable. +""" + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.extensions import ExtensionManager, ExtensionCatalog, ExtensionError + +runner = CliRunner() + + +@pytest.fixture +def project_dir(tmp_path): + """Create a minimal spec-kit project directory.""" + proj_dir = tmp_path / "project" + proj_dir.mkdir() + (proj_dir / ".specify").mkdir() + (proj_dir / ".specify" / "config.toml").write_text("ai = 'claude'") + return proj_dir + + +def _catalog_entry(ext_id, name, version="1.0.0", verified=False, install_allowed=True, catalog_name="default"): + return { + "id": ext_id, + "name": name, + "version": version, + "description": f"{name} description", + "verified": verified, + "_install_allowed": install_allowed, + "_catalog_name": catalog_name, + } + + +def test_list_available_queries_catalog_and_filters_installed(project_dir, monkeypatch): + """--available must query the catalog and drop already-installed IDs.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{"id": "already-installed"}]) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + _catalog_entry("already-installed", "Already Installed"), + _catalog_entry("fresh-ext", "Fresh Ext", verified=True), + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "Available Extensions:" in result.output + # Uninstalled catalog extension is shown... + assert "fresh-ext" in result.output + assert "✓ Verified" in result.output + assert "specify extension add fresh-ext" in result.output + # ...and the installed one is filtered out. + assert "already-installed" not in result.output + + +def test_list_available_marks_discovery_only_entries(project_dir, monkeypatch): + """Entries whose catalog disallows install render a discovery-only note.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + _catalog_entry("locked-ext", "Locked Ext", install_allowed=False, catalog_name="curated"), + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "Discovery only" in result.output + assert "curated" in result.output + assert "specify extension add locked-ext" not in result.output + + +def test_list_available_empty_catalog_message(project_dir, monkeypatch): + """An empty (post-filter) catalog reports no additional extensions.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: []) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "Available Extensions:" in result.output + assert "No additional extensions available" in result.output + + +def test_list_available_catalog_error_exits(project_dir, monkeypatch): + """A catalog failure surfaces a clear error and exits non-zero.""" + monkeypatch.chdir(project_dir) + + def _boom(self): + raise ExtensionError("catalog unreachable") + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", _boom) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 1 + assert "Could not query extension catalog" in result.output + assert "catalog unreachable" in result.output + + +def test_list_all_shows_installed_and_available(project_dir, monkeypatch): + """--all lists installed extensions and available catalog extensions.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{ + "id": "my-ext", + "name": "My Ext", + "version": "2.0.0", + "description": "installed one", + "command_count": 1, + "hook_count": 0, + "priority": 10, + "enabled": True, + }]) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + _catalog_entry("other-ext", "Other Ext"), + ]) + + result = runner.invoke(app, ["extension", "list", "--all"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "Installed Extensions:" in result.output + assert "My Ext" in result.output + assert "Available Extensions:" in result.output + assert "other-ext" in result.output From ced753e2a95bb9eb510d2a106f6472ae1998b211 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Tue, 23 Jun 2026 09:34:06 +0800 Subject: [PATCH 03/11] fix(extensions): escape catalog markup and always render installed section - Escape untrusted catalog fields (name, version, id, description, catalog name) and the catalog-query error before embedding them in Rich markup, preventing markup injection in `extension list --available/--all` output - Always print the "Installed Extensions:" header when the installed section is shown, with a "No extensions installed." note when empty, so `--all` output is no longer missing the section on an empty project - Update the command docstring to reflect --available/--all catalog listing so `--help` is accurate --- src/specify_cli/extensions/_commands.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index d2d7ce78ce..132c64104f 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -385,7 +385,7 @@ def extension_list( available: bool = typer.Option(False, "--available", help="Show available extensions from catalog"), all_extensions: bool = typer.Option(False, "--all", help="Show both installed and available"), ): - """List installed extensions.""" + """List installed extensions, and available catalog extensions with --available/--all.""" from . import ExtensionManager, ExtensionCatalog, ExtensionError project_root = _require_specify_project() @@ -403,9 +403,12 @@ def extension_list( console.print(" specify extension add ") return - if show_installed and installed: + if show_installed: console.print("\n[bold cyan]Installed Extensions:[/bold cyan]\n") + if not installed: + console.print(" [dim]No extensions installed.[/dim]") + console.print() for ext in installed: status_icon = "✓" if ext["enabled"] else "✗" status_color = "green" if ext["enabled"] else "red" @@ -424,7 +427,7 @@ def extension_list( try: results = catalog.search() except ExtensionError as e: - console.print(f"\n[red]Error:[/red] Could not query extension catalog: {e}") + console.print(f"\n[red]Error:[/red] Could not query extension catalog: {_escape_markup(str(e))}") console.print("[dim]The catalog may be temporarily unavailable. Try again later.[/dim]") raise typer.Exit(1) @@ -435,15 +438,18 @@ def extension_list( console.print(" [dim]No additional extensions available in the catalog.[/dim]") else: for ext in available_exts: + # Catalog fields are untrusted (remote/community catalogs); escape + # before embedding in Rich markup to prevent markup injection. + safe_id = _escape_markup(str(ext.get("id", ""))) verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" - console.print(f" [bold]{ext['name']}[/bold] (v{ext['version']}){verified_badge}") - console.print(f" [dim]{ext['id']}[/dim]") - console.print(f" {ext.get('description', '')}") + console.print(f" [bold]{_escape_markup(str(ext['name']))}[/bold] (v{_escape_markup(str(ext['version']))}){verified_badge}") + console.print(f" [dim]{safe_id}[/dim]") + console.print(f" {_escape_markup(str(ext.get('description', '')))}") install_allowed = ext.get("_install_allowed", True) if install_allowed: - console.print(f" [cyan]Install:[/cyan] specify extension add {ext['id']}") + console.print(f" [cyan]Install:[/cyan] specify extension add {safe_id}") else: - catalog_name = ext.get("_catalog_name", "") + catalog_name = _escape_markup(str(ext.get("_catalog_name", ""))) console.print(f" [yellow]Discovery only — not installable from '{catalog_name}'[/yellow]") console.print() From 75c4b67e855b403b6c65e19bb46603b180d4c51c Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Tue, 23 Jun 2026 21:54:40 +0800 Subject: [PATCH 04/11] fix(extensions): harden catalog-driven commands against malformed entries Catalog entries are untrusted (remote/community catalogs) and only guaranteed to carry an injected `id`; every other field comes straight from `**ext_data`. Hard subscripts and unguarded `.get()` chains on that data could crash `search`, `info`, `add`, and `update`, and a couple of catalog-controlled values were printed without Rich-markup escaping. - Use `.get()` fallbacks for name/version/description across search/info/add resolution and the download message - Guard `requires`/`provides` with isinstance(dict) before `.get()`, and skip non-dict tool entries - Catch KeyError alongside InvalidVersion in `update` version parsing - Escape catalog-controlled `stars` before printing - Correct the add --from path-traversal test docstring to describe the real mitigation (generated tempfile in the downloads dir, not label sanitization) - Add regression tests for malformed catalog entries in list/search/info --- src/specify_cli/extensions/_commands.py | 123 +++++++++++---------- tests/test_extension_catalog_robustness.py | 101 +++++++++++++++++ tests/test_extension_list_available.py | 22 ++++ 3 files changed, 186 insertions(+), 60 deletions(-) create mode 100644 tests/test_extension_catalog_robustness.py diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 132c64104f..12450ce6b2 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -11,6 +11,7 @@ import errno import hashlib import os +import re import shutil import stat import tempfile @@ -49,6 +50,29 @@ extension_app.add_typer(catalog_app, name="catalog") +def _catalog_str(ext: dict, key: str, fallback: str = "") -> str: + """Return a non-blank catalog string field, or a safe fallback.""" + value = ext.get(key) + if isinstance(value, str): + value = value.strip() + if value: + return value + return fallback + + +def _catalog_id(ext: dict) -> str: + """Return an installable catalog ID, or an empty string when invalid.""" + extension_id = _catalog_str(ext, "id") + return extension_id if re.fullmatch(r"[a-z0-9-]+", extension_id) else "" + + +def _catalog_number(value) -> str: + """Return a formatted numeric catalog value, omitting malformed input.""" + if isinstance(value, bool) or not isinstance(value, (int, float)): + return "" + return f"{value:,}" + + # Root helpers re-fetched at call time so test monkeypatching of # `specify_cli.` keeps working after the move. def _require_specify_project(*args, **kwargs): @@ -334,18 +358,16 @@ def _resolve_catalog_extension( # First try by ID ext_info = catalog.get_extension_info(argument) if ext_info: - return (ext_info, None) - - # Try by display name - search using argument as query, then filter for exact match. - # Coerce name defensively: catalog JSON is user-editable, so a hand-authored - # non-string/missing name must not crash the match (the ambiguous-match display - # below already str()-coerces name for the same reason). - search_results = catalog.search() - argument_lower = argument.lower() + if _catalog_id(ext_info): + return (ext_info, None) + return (None, None) + + # Resolve display names only from entries the CLI can safely install. + search_results = catalog.search(query=argument) name_matches = [ ext for ext in search_results - if str(ext.get("name", "")).lower() == argument_lower + if _catalog_id(ext) and _catalog_str(ext, "name").lower() == argument.lower() ] if len(name_matches) == 1: @@ -442,7 +464,9 @@ def extension_list( # before embedding in Rich markup to prevent markup injection. safe_id = _escape_markup(str(ext.get("id", ""))) verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" - console.print(f" [bold]{_escape_markup(str(ext['name']))}[/bold] (v{_escape_markup(str(ext['version']))}){verified_badge}") + safe_name = _escape_markup(str(ext.get("name", "(unnamed)"))) + safe_version = _escape_markup(str(ext.get("version", "?"))) + console.print(f" [bold]{safe_name}[/bold] (v{safe_version}){verified_badge}") console.print(f" [dim]{safe_id}[/dim]") console.print(f" {_escape_markup(str(ext.get('description', '')))}") install_allowed = ext.get("_install_allowed", True) @@ -1057,7 +1081,9 @@ def extension_add( # Download extension archive (use the resolved catalog ID). extension_id = ext_info['id'] - console.print(f"Downloading {_escape_markup(str(ext_info['name']))} v{_escape_markup(str(ext_info.get('version', 'unknown')))}...") + dl_name = _escape_markup(_catalog_str(ext_info, "name", extension_id)) + dl_version = _escape_markup(_catalog_str(ext_info, "version", "unknown")) + console.print(f"Downloading {dl_name} v{dl_version}...") archive_path = catalog.download_extension(extension_id) try: @@ -1247,8 +1273,8 @@ def extension_search( for ext in results: # Extension header verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" - console.print(f"[bold]{_escape_markup(str(ext['name']))}[/bold] (v{_escape_markup(str(ext['version']))}){verified_badge}") - console.print(f" {_escape_markup(str(ext['description']))}") + console.print(f"[bold]{_escape_markup(str(ext.get('name', '(unnamed)')))}[/bold] (v{_escape_markup(str(ext.get('version', '?')))}){verified_badge}") + console.print(f" {_escape_markup(str(ext.get('description', '')))}") # Metadata console.print(f"\n [dim]Author:[/dim] {_escape_markup(str(ext.get('author', 'Unknown')))}") @@ -1268,24 +1294,11 @@ def extension_search( # Stats stats = [] - 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))}") + downloads = _catalog_number(ext.get("downloads")) + if downloads: + stats.append(f"Downloads: {downloads}") + if ext.get('stars') is not None: + stats.append(f"Stars: {_escape_markup(str(ext['stars']))}") if stats: console.print(f" [dim]{' | '.join(stats)}[/dim]") @@ -1427,12 +1440,12 @@ def _print_extension_info(ext_info: dict, manager): # Header verified_badge = " [green]✓ Verified[/green]" if ext_info.get("verified") else "" - console.print(f"\n[bold]{_escape_markup(str(ext_info['name']))}[/bold] (v{_escape_markup(str(ext_info['version']))}){verified_badge}") + console.print(f"\n[bold]{_escape_markup(str(ext_info.get('name', '(unnamed)')))}[/bold] (v{_escape_markup(str(ext_info.get('version', '?')))}){verified_badge}") console.print(f"ID: {_escape_markup(str(ext_info['id']))}") console.print() # Description - console.print(f"{_escape_markup(str(ext_info['description']))}") + console.print(f"{_escape_markup(str(ext_info.get('description', '')))}") console.print() # Author and License @@ -1453,23 +1466,26 @@ def _print_extension_info(ext_info: dict, manager): console.print() # Requirements - if ext_info.get('requires'): + reqs = ext_info.get('requires') + if isinstance(reqs, dict) and reqs: console.print("[bold]Requirements:[/bold]") - reqs = ext_info['requires'] if reqs.get('speckit_version'): console.print(f" • Spec Kit: {_escape_markup(str(reqs['speckit_version']))}") - if reqs.get('tools'): - for tool in reqs['tools']: - tool_name = _escape_markup(str(tool['name'])) + tools = reqs.get('tools') + if isinstance(tools, list): + for tool in tools: + if not isinstance(tool, dict): + continue + tool_name = _escape_markup(str(tool.get('name', '(unnamed)'))) tool_version = _escape_markup(str(tool.get('version', 'any'))) required = " (required)" if tool.get('required') else " (optional)" console.print(f" • {tool_name}: {tool_version}{required}") console.print() # Provides - if ext_info.get('provides'): + provides = ext_info.get('provides') + if isinstance(provides, dict) and provides: console.print("[bold]Provides:[/bold]") - provides = ext_info['provides'] if provides.get('commands'): console.print(f" • Commands: {_escape_markup(str(provides['commands']))}") if provides.get('hooks'): @@ -1485,24 +1501,11 @@ def _print_extension_info(ext_info: dict, manager): # Statistics stats = [] - 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))}") + downloads = _catalog_number(ext_info.get("downloads")) + if downloads: + stats.append(f"Downloads: {downloads}") + if ext_info.get('stars') is not None: + stats.append(f"Stars: {_escape_markup(str(ext_info['stars']))}") if stats: console.print(f"[bold]Statistics:[/bold] {' | '.join(stats)}") console.print() @@ -1610,8 +1613,8 @@ def extension_update( continue try: - catalog_version = pkg_version.Version(ext_info["version"]) - except pkg_version.InvalidVersion: + catalog_version = pkg_version.Version(str(ext_info["version"])) + except (pkg_version.InvalidVersion, KeyError): console.print( f"⚠ {safe_ext_id}: Invalid catalog version '{_escape_markup(str(ext_info.get('version')))}' (skipping)" ) diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py new file mode 100644 index 0000000000..ed3e6d08a0 --- /dev/null +++ b/tests/test_extension_catalog_robustness.py @@ -0,0 +1,101 @@ +"""Robustness tests for catalog-driven extension commands. + +Catalog entries are untrusted (remote/community catalogs) and only guaranteed +to be dicts with an injected ``id`` — ``_get_merged_extensions`` does not +validate any other field. ``extension search`` and ``extension info`` must +therefore tolerate entries missing ``name``/``version``/``description`` (no +KeyError), entries whose ``requires``/``provides`` are non-dicts (no +AttributeError), and must escape catalog-controlled values like ``stars`` +before printing them as Rich markup. +""" + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.extensions import ExtensionManager, ExtensionCatalog + +runner = CliRunner() + + +@pytest.fixture +def project_dir(tmp_path): + proj_dir = tmp_path / "project" + proj_dir.mkdir() + (proj_dir / ".specify").mkdir() + (proj_dir / ".specify" / "config.toml").write_text("ai = 'claude'") + return proj_dir + + +def test_search_tolerates_entry_missing_name_version_description(project_dir, monkeypatch): + """A malformed catalog entry must not crash `extension search`.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [{"id": "broken-ext"}], # only id present + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "broken-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + + +def test_search_escapes_markup_in_stars(project_dir, monkeypatch): + """Catalog-controlled `stars` must be escaped, not parsed as Rich markup.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [{ + "id": "starry", + "name": "Starry", + "version": "1.0.0", + "description": "d", + "stars": "[red]999[/red]", + }], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + # Escaped markup is rendered literally rather than swallowed by Rich. + assert "[red]999[/red]" in result.output + + +def test_info_tolerates_missing_fields_and_non_dict_sections(project_dir, monkeypatch): + """`extension info` must survive a malformed catalog entry. + + Missing name/version/description → placeholders (no KeyError); requires as a + list and provides as a string → skipped, not `.get()`-crashed. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "broken-ext", + "requires": ["not", "a", "dict"], + "provides": "junk", + "stars": "[bold]42[/bold]", + }, + ) + + result = runner.invoke(app, ["extension", "info", "broken-ext"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "broken-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + # Non-dict requires/provides are skipped, not crashed on. + assert "Requirements:" not in result.output + assert "Provides:" not in result.output + # stars is escaped. + assert "[bold]42[/bold]" in result.output diff --git a/tests/test_extension_list_available.py b/tests/test_extension_list_available.py index 911a6d2881..011d48ed90 100644 --- a/tests/test_extension_list_available.py +++ b/tests/test_extension_list_available.py @@ -108,6 +108,28 @@ def _boom(self): assert "catalog unreachable" in result.output +def test_list_available_tolerates_entry_missing_name_or_version(project_dir, monkeypatch): + """A malformed catalog entry missing name/version must not crash listing. + + Catalog entries are untrusted (remote/community catalogs) and only + guaranteed to be dicts with an injected ``id``. A missing ``name`` or + ``version`` must degrade to a placeholder, not raise KeyError. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + {"id": "broken-ext"}, # no name, no version, no description + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "broken-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + + def test_list_all_shows_installed_and_available(project_dir, monkeypatch): """--all lists installed extensions and available catalog extensions.""" monkeypatch.chdir(project_dir) From f0d76e09c00c4c3b46e9ac23686dfc47c822a228 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Wed, 24 Jun 2026 01:14:05 +0800 Subject: [PATCH 05/11] Prevent catalog path traversal during extension downloads Remote extension catalogs are untrusted, and catalog entry IDs feed into the cached ZIP filename used by extension installation. The download path now validates IDs against the manifest ID rule before catalog lookup and verifies the final ZIP path stays under the downloads directory before writing. The issue-template agent list is also synchronized with runtime integrations so consistency checks pass for newly registered agents. Constraint: Remote catalog metadata controls extension IDs and versions before local manifest validation can run Rejected: Sanitize path separators in the filename | silently rewriting remote IDs could install an extension under an unexpected identity Confidence: high Scope-risk: narrow Directive: Keep catalog-sourced identifiers validated before using them in filesystem paths Tested: uv run pytest tests/test_extension_catalog_robustness.py tests/test_extension_add_path_traversal.py tests/test_extension_update_hardening.py tests/test_extension_list_available.py -q Tested: uv run pytest tests/test_agent_config_consistency.py -q Tested: uv run python -m compileall -q src/specify_cli/extensions tests/test_extension_catalog_robustness.py Tested: git diff --check Not-tested: Full uv run pytest Assisted-by: Codex (model: GPT-5, autonomous) Co-authored-by: OmX --- src/specify_cli/extensions/__init__.py | 11 +++++++- tests/test_extension_catalog_robustness.py | 29 +++++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 9fa44d3809..ee07457b26 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -60,6 +60,7 @@ } ) EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$") +EXTENSION_ID_PATTERN = re.compile(r"^[a-z0-9-]+$") VALID_EFFECTS = frozenset({"read-only", "read-write"}) @@ -306,7 +307,7 @@ def _validate(self): ) # Validate extension ID format - if not re.match(r"^[a-z0-9-]+$", ext["id"]): + if not EXTENSION_ID_PATTERN.match(ext["id"]): raise ValidationError( f"Invalid extension ID '{ext['id']}': " "must be lowercase alphanumeric with hyphens only" @@ -4079,6 +4080,14 @@ def download_extension( """ import urllib.error + if not isinstance(extension_id, str) or not EXTENSION_ID_PATTERN.match( + extension_id + ): + raise ExtensionError( + f"Invalid extension ID '{extension_id}': " + "must be lowercase alphanumeric with hyphens only" + ) + # Get extension info from catalog ext_info = self.get_extension_info(extension_id) if not ext_info: diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index ed3e6d08a0..e6b238e17c 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -13,7 +13,7 @@ from typer.testing import CliRunner from specify_cli import app -from specify_cli.extensions import ExtensionManager, ExtensionCatalog +from specify_cli.extensions import ExtensionError, ExtensionManager, ExtensionCatalog runner = CliRunner() @@ -99,3 +99,30 @@ def test_info_tolerates_missing_fields_and_non_dict_sections(project_dir, monkey assert "Provides:" not in result.output # stars is escaped. assert "[bold]42[/bold]" in result.output + + +def test_download_rejects_catalog_id_path_traversal(project_dir, monkeypatch): + """Catalog-controlled IDs must not become path-traversing ZIP filenames.""" + monkeypatch.chdir(project_dir) + + catalog = ExtensionCatalog(project_dir) + malicious_id = "../escape" + + monkeypatch.setattr( + catalog, + "_get_merged_extensions", + lambda: [{ + "id": malicious_id, + "name": "Evil", + "version": "1.0.0", + "download_url": "https://example.com/evil.zip", + }], + ) + + def fail_open_url(*args, **kwargs): + raise AssertionError("download should not be attempted for an unsafe id") + + monkeypatch.setattr(catalog, "_open_url", fail_open_url) + + with pytest.raises(ExtensionError, match="Invalid extension ID"): + catalog.download_extension(malicious_id) From cd9cfab4945c5aa6ebc5dd2cf8040802ac23f93d Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Fri, 3 Jul 2026 16:33:09 +0800 Subject: [PATCH 06/11] fix(extensions): coalesce null catalog fields and sanitize download version Address remaining Copilot review on PR #3051: - list --available/--all and search now skip catalog entries without a valid id and coalesce JSON null name/version/description to placeholders instead of rendering the literal "None". - extension info gets the same null coalescing for name/version/ description/author/license via a shared _catalog_str helper. - download_extension sanitizes the catalog-provided version into a filename-safe token so a version with separators cannot produce a nested path or write failure, rather than relying on the post-hoc resolve() containment check alone. - Fix a stale mock in test_add_from_url_sanitizes_traversal_label (missing extra_headers kwarg, non-ZIP body) so the suite is green. New tests cover null-field rendering, id-skipping, the version token sanitizer, and the download filename path. --- src/specify_cli/extensions/__init__.py | 23 ++- src/specify_cli/extensions/_commands.py | 47 ++++-- tests/test_extension_catalog_robustness.py | 178 +++++++++++++++++++++ tests/test_extension_list_available.py | 51 ++++++ 4 files changed, 283 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index ee07457b26..1d79c4e027 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -62,6 +62,27 @@ EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$") EXTENSION_ID_PATTERN = re.compile(r"^[a-z0-9-]+$") +# Characters allowed verbatim in a catalog-provided version when it is used to +# build a download filename. Anything else (path separators, "..", control +# chars, whitespace) is collapsed to "-" so an untrusted version cannot inject +# a nested path or an unwritable filename — we sanitize rather than rely on the +# post-hoc Path.resolve() containment check as the primary guard. +_UNSAFE_VERSION_TOKEN_PATTERN = re.compile(r"[^A-Za-z0-9._-]+") + + +def _safe_version_token(version: Any, fallback: str = "unknown") -> str: + """Reduce a catalog ``version`` to a filename-safe token. + + Non-strings (JSON null, numbers) and blank values degrade to ``fallback``. + Path separators and traversal tokens are stripped so the result can never + span directories or escape the download target. + """ + if not isinstance(version, str): + return fallback + token = _UNSAFE_VERSION_TOKEN_PATTERN.sub("-", version).strip("-.") + return token or fallback + + VALID_EFFECTS = frozenset({"read-only", "read-write"}) DEFAULT_HOOK_PRIORITY = 10 @@ -4139,7 +4160,7 @@ def download_extension( if target_dir is None: target_dir = self.cache_dir / "downloads" target_dir = Path(target_dir) - version = ext_info.get("version", "unknown") + version = _safe_version_token(ext_info.get("version")) declared_format = archive_format_from_name(download_url) build_safe_download_path( target_dir, diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 12450ce6b2..47341a26a6 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -462,18 +462,23 @@ def extension_list( for ext in available_exts: # Catalog fields are untrusted (remote/community catalogs); escape # before embedding in Rich markup to prevent markup injection. - safe_id = _escape_markup(str(ext.get("id", ""))) + # A missing/blank id means the entry cannot be installed, so skip + # it entirely rather than printing a bogus install hint. + extension_id = _catalog_id(ext) + if not extension_id: + continue + safe_id = _escape_markup(extension_id) verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" - safe_name = _escape_markup(str(ext.get("name", "(unnamed)"))) - safe_version = _escape_markup(str(ext.get("version", "?"))) + safe_name = _escape_markup(_catalog_str(ext, "name", "(unnamed)")) + safe_version = _escape_markup(_catalog_str(ext, "version", "?")) console.print(f" [bold]{safe_name}[/bold] (v{safe_version}){verified_badge}") console.print(f" [dim]{safe_id}[/dim]") - console.print(f" {_escape_markup(str(ext.get('description', '')))}") + console.print(f" {_escape_markup(_catalog_str(ext, 'description'))}") install_allowed = ext.get("_install_allowed", True) if install_allowed: console.print(f" [cyan]Install:[/cyan] specify extension add {safe_id}") else: - catalog_name = _escape_markup(str(ext.get("_catalog_name", ""))) + catalog_name = _escape_markup(_catalog_str(ext, "_catalog_name")) console.print(f" [yellow]Discovery only — not installable from '{catalog_name}'[/yellow]") console.print() @@ -1271,16 +1276,28 @@ def extension_search( console.print(f"\n[green]Found {len(results)} extension(s):[/green]\n") for ext in results: + # Catalog entries are untrusted; a missing/blank id cannot be + # installed or referenced, so skip it rather than emit a bogus + # install hint or crash on ext['id']. + extension_id = _catalog_id(ext) + if not extension_id: + continue + # Extension header verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" - console.print(f"[bold]{_escape_markup(str(ext.get('name', '(unnamed)')))}[/bold] (v{_escape_markup(str(ext.get('version', '?')))}){verified_badge}") - console.print(f" {_escape_markup(str(ext.get('description', '')))}") + console.print(f"[bold]{_escape_markup(_catalog_str(ext, 'name', '(unnamed)'))}[/bold] (v{_escape_markup(_catalog_str(ext, 'version', '?'))}){verified_badge}") + console.print(f" {_escape_markup(_catalog_str(ext, 'description'))}") # Metadata - console.print(f"\n [dim]Author:[/dim] {_escape_markup(str(ext.get('author', 'Unknown')))}") + console.print(f"\n [dim]Author:[/dim] {_escape_markup(_catalog_str(ext, 'author', 'Unknown'))}") ext_tags = ext.get('tags', []) - if isinstance(ext_tags, list) and ext_tags: - tags_str = ", ".join(str(t) for t in ext_tags) + tags = ( + [tag.strip() for tag in ext_tags if isinstance(tag, str) and tag.strip()] + if isinstance(ext_tags, list) + else [] + ) + if tags: + tags_str = ", ".join(tags) console.print(f" [dim]Tags:[/dim] {_escape_markup(tags_str)}") # Source catalog @@ -1307,7 +1324,7 @@ def extension_search( console.print(f" [dim]Repository:[/dim] {_escape_markup(str(ext['repository']))}") # Install command (show warning if not installable) - safe_id = _escape_markup(str(ext['id'])) + safe_id = _escape_markup(extension_id) if install_allowed: console.print(f"\n [cyan]Install:[/cyan] specify extension add {safe_id}") else: @@ -1440,17 +1457,17 @@ def _print_extension_info(ext_info: dict, manager): # Header verified_badge = " [green]✓ Verified[/green]" if ext_info.get("verified") else "" - console.print(f"\n[bold]{_escape_markup(str(ext_info.get('name', '(unnamed)')))}[/bold] (v{_escape_markup(str(ext_info.get('version', '?')))}){verified_badge}") + console.print(f"\n[bold]{_escape_markup(_catalog_str(ext_info, 'name', '(unnamed)'))}[/bold] (v{_escape_markup(_catalog_str(ext_info, 'version', '?'))}){verified_badge}") console.print(f"ID: {_escape_markup(str(ext_info['id']))}") console.print() # Description - console.print(f"{_escape_markup(str(ext_info.get('description', '')))}") + console.print(f"{_escape_markup(_catalog_str(ext_info, 'description'))}") console.print() # Author and License - console.print(f"[dim]Author:[/dim] {_escape_markup(str(ext_info.get('author', 'Unknown')))}") - console.print(f"[dim]License:[/dim] {_escape_markup(str(ext_info.get('license', 'Unknown')))}") + console.print(f"[dim]Author:[/dim] {_escape_markup(_catalog_str(ext_info, 'author', 'Unknown'))}") + console.print(f"[dim]License:[/dim] {_escape_markup(_catalog_str(ext_info, 'license', 'Unknown'))}") # Category and Effect if ext_info.get('category'): diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index e6b238e17c..2196b3eb45 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -45,6 +45,66 @@ def test_search_tolerates_entry_missing_name_version_description(project_dir, mo assert "(v?)" in result.output +def test_search_json_null_fields_render_placeholders_not_none(project_dir, monkeypatch): + """Explicit JSON null fields must fall back, not render the literal "None". + + ``dict.get(key, default)`` only substitutes on an absent key; an explicit + ``null`` value reaches ``str()`` and would print "None". Name/version/ + description/author must degrade to placeholders instead. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [{ + "id": "null-ext", + "name": None, + "version": None, + "description": None, + "author": None, + }], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "null-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + assert "Unknown" in result.output # author fallback + # The literal string "None" must never leak into rendered output. + assert "None" not in result.output + + +@pytest.mark.parametrize("bad_id", [None, "", " "]) +def test_search_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id): + """Entries with a missing/blank/null id are skipped entirely. + + Such an id cannot be installed (``download_extension()`` refuses it) and + previously crashed on ``ext['id']`` when absent. Skip the whole entry + rather than emit a bogus/dangling install hint. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [ + {"id": bad_id, "name": "Ghost Ext", "version": "1.0.0", "description": "d"}, + {"id": "real-ext", "name": "Real Ext", "version": "1.0.0", "description": "d"}, + ], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "real-ext" in result.output + assert "specify extension add real-ext" in result.output + # The id-less entry is dropped: no header, no dangling install hint. + assert "Ghost Ext" not in result.output + + def test_search_escapes_markup_in_stars(project_dir, monkeypatch): """Catalog-controlled `stars` must be escaped, not parsed as Rich markup.""" monkeypatch.chdir(project_dir) @@ -126,3 +186,121 @@ def fail_open_url(*args, **kwargs): with pytest.raises(ExtensionError, match="Invalid extension ID"): catalog.download_extension(malicious_id) + + +def test_info_json_null_fields_render_placeholders_not_none(project_dir, monkeypatch): + """`extension info` must coalesce JSON null fields, not print "None". + + ``get_extension_info`` returns the raw merged catalog entry unmodified, so + an explicit ``null`` name/version/description/author/license reaches the + renderer. These must degrade to their placeholders, mirroring the fix + already applied to `list --available` and `search`. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "null-ext", + "name": None, + "version": None, + "description": None, + "author": None, + "license": None, + }, + ) + + result = runner.invoke(app, ["extension", "info", "null-ext"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "null-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + # The literal string "None" must never leak into rendered output. + assert "None" not in result.output + + +@pytest.mark.parametrize( + "raw, expected", + [ + ("1.2.3", "1.2.3"), # normal version untouched + ("1.0/../x", "1.0-..-x"), # separators collapse to a single segment + ("../../etc", "etc"), # leading traversal stripped + ("a b\\c/d", "a-b-c-d"), # spaces and both slash kinds collapse + ("..", "unknown"), # pure traversal degrades to fallback + (" ", "unknown"), # blank degrades to fallback + (None, "unknown"), # JSON null degrades to fallback + (123, "unknown"), # non-string degrades to fallback + ], +) +def test_safe_version_token_strips_separators(raw, expected): + """Version tokens used in ZIP filenames must never carry path separators.""" + from specify_cli.extensions import _safe_version_token + + token = _safe_version_token(raw) + assert token == expected + assert "/" not in token and "\\" not in token + + +def test_download_sanitizes_catalog_version_into_safe_filename(project_dir, monkeypatch): + """A catalog version with separators must not yield a nested/escaping path. + + The pre-fix code used the version verbatim, so ``1.0/evil`` produced a + nested path that stayed inside the target dir (passing the ``resolve()`` + guard) yet failed to write. Sanitizing the version keeps the ZIP a single + filename directly under the target directory. + """ + monkeypatch.chdir(project_dir) + + catalog = ExtensionCatalog(project_dir) + target_dir = project_dir / "downloads" + target_dir.mkdir() + + monkeypatch.setattr( + catalog, + "_get_merged_extensions", + lambda: [{ + "id": "good-ext", + "name": "Good", + "version": "1.0/evil", + "download_url": "https://example.com/good.zip", + }], + ) + + import io + import zipfile + + archive_buffer = io.BytesIO() + with zipfile.ZipFile(archive_buffer, "w") as archive: + archive.writestr("extension.yaml", "name: good\n") + archive_bytes = archive_buffer.getvalue() + + class _FakeResponse: + def __init__(self): + self._body = io.BytesIO(archive_bytes) + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def read(self, size=-1): + return self._body.read(size) + + monkeypatch.setattr(catalog, "_open_url", lambda *a, **k: _FakeResponse()) + monkeypatch.setattr(catalog, "_resolve_github_release_asset_api_url", lambda url: None) + # Skip integrity check (no sha256 in this fixture entry). + import specify_cli.extensions as ext_module + monkeypatch.setattr(ext_module, "verify_archive_sha256", lambda *a, **k: None) + + zip_path = catalog.download_extension("good-ext", target_dir=target_dir) + + # The ZIP is a single file directly under target_dir — no nested dirs, + # no separators leaked from the version. + assert zip_path.parent == target_dir + assert "/" not in zip_path.name and "\\" not in zip_path.name + assert zip_path.name == "good-ext-1.0-evil.zip" + assert zip_path.read_bytes() == archive_bytes diff --git a/tests/test_extension_list_available.py b/tests/test_extension_list_available.py index 011d48ed90..8b0d1e6668 100644 --- a/tests/test_extension_list_available.py +++ b/tests/test_extension_list_available.py @@ -130,6 +130,57 @@ def test_list_available_tolerates_entry_missing_name_or_version(project_dir, mon assert "(v?)" in result.output +def test_list_available_json_null_fields_render_placeholders_not_none(project_dir, monkeypatch): + """Explicit JSON null fields must fall back, not render the literal "None". + + ``dict.get(key, default)`` only substitutes when the key is absent; an + explicit ``null`` value reaches ``str()`` and would print "None". Untrusted + catalog JSON can carry nulls, so name/version/description must degrade to + their placeholders instead. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + {"id": "null-ext", "name": None, "version": None, "description": None}, + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "null-ext" in result.output + assert "(unnamed)" in result.output + assert "(v?)" in result.output + # The literal string "None" must never leak into the rendered output. + assert "None" not in result.output + + +@pytest.mark.parametrize("bad_id", [None, "", " "]) +def test_list_available_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id): + """Entries with a missing/blank/null id are skipped entirely. + + Such an id cannot be installed (``download_extension()`` would refuse it), + so printing an install hint like ``specify extension add`` with no id — or + with ``None`` — only misleads the user. Skip the whole entry. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + {"id": bad_id, "name": "Ghost Ext", "version": "1.0.0"}, + _catalog_entry("real-ext", "Real Ext"), + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + # The valid entry still renders with its install hint... + assert "real-ext" in result.output + assert "specify extension add real-ext" in result.output + # ...but the id-less entry is dropped: no name, no dangling install hint. + assert "Ghost Ext" not in result.output + + def test_list_all_shows_installed_and_available(project_dir, monkeypatch): """--all lists installed extensions and available catalog extensions.""" monkeypatch.chdir(project_dir) From f755e5cd7a50d11d0ee3af2fbe4ffe83db870987 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Tue, 7 Jul 2026 14:10:07 +0800 Subject: [PATCH 07/11] fix(extensions): use _catalog_str/_catalog_id for catalog display fallbacks - list --available: filter with _catalog_id() before the empty check so an all-invalid-id catalog shows the "no additional extensions" fallback - search: count/iterate only valid-id entries and render _catalog_name via _catalog_str() so an explicit JSON null no longer prints "Catalog: None" - add: render download status name/version via _catalog_str() so null fields fall back to the resolved id / "unknown" instead of the literal "None" - add regression tests for each path --- src/specify_cli/extensions/_commands.py | 29 +++++-- tests/test_extension_catalog_robustness.py | 95 ++++++++++++++++++++++ tests/test_extension_list_available.py | 27 ++++++ 3 files changed, 142 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 47341a26a6..00816f5b90 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -453,7 +453,15 @@ def extension_list( console.print("[dim]The catalog may be temporarily unavailable. Try again later.[/dim]") raise typer.Exit(1) - available_exts = [ext for ext in results if ext.get("id") not in installed_ids] + # Use _catalog_id() (not raw ext.get("id")) so entries with a + # missing/blank/null id are excluded up front. Otherwise they inflate + # available_exts, get skipped by the print loop, and leave the section + # header with no rows and no "No additional extensions" fallback. + available_exts = [ + ext + for ext in results + if (ext_id := _catalog_id(ext)) and ext_id not in installed_ids + ] console.print("\n[bold cyan]Available Extensions:[/bold cyan]\n") if not available_exts: @@ -1273,16 +1281,18 @@ def extension_search( console.print(" • specify extension search (show all)") raise typer.Exit(0) + # Catalog entries are untrusted; a missing/blank id cannot be installed + # or referenced. Filter those out before counting so the "Found N" + # count matches what actually renders (the loop below would skip them). + results = [ext for ext in results if _catalog_id(ext)] + if not results: + console.print("\n[yellow]No extensions found matching criteria[/yellow]") + raise typer.Exit(0) + console.print(f"\n[green]Found {len(results)} extension(s):[/green]\n") for ext in results: - # Catalog entries are untrusted; a missing/blank id cannot be - # installed or referenced, so skip it rather than emit a bogus - # install hint or crash on ext['id']. extension_id = _catalog_id(ext) - if not extension_id: - continue - # Extension header verified_badge = " [green]✓ Verified[/green]" if ext.get("verified") else "" console.print(f"[bold]{_escape_markup(_catalog_str(ext, 'name', '(unnamed)'))}[/bold] (v{_escape_markup(_catalog_str(ext, 'version', '?'))}){verified_badge}") @@ -1300,8 +1310,9 @@ def extension_search( tags_str = ", ".join(tags) console.print(f" [dim]Tags:[/dim] {_escape_markup(tags_str)}") - # Source catalog - catalog_name = _escape_markup(str(ext.get("_catalog_name", ""))) + # Source catalog. Use _catalog_str so an explicit JSON null or blank + # _catalog_name falls back to "" instead of rendering "None". + catalog_name = _escape_markup(_catalog_str(ext, "_catalog_name")) install_allowed = ext.get("_install_allowed", True) if catalog_name: if install_allowed: diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index 2196b3eb45..fd303645d2 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -105,6 +105,62 @@ def test_search_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id) assert "Ghost Ext" not in result.output +def test_search_count_excludes_invalid_id_entries(project_dir, monkeypatch): + """The "Found N" count must match what actually renders. + + Counting raw results before filtering invalid-id entries misreports the + total (e.g. "Found 3" while only one entry prints). The count must be taken + after dropping entries without a valid id. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [ + {"id": None, "name": "Ghost One", "version": "1.0.0", "description": "d"}, + {"id": "", "name": "Ghost Two", "version": "1.0.0", "description": "d"}, + {"id": "real-ext", "name": "Real Ext", "version": "1.0.0", "description": "d"}, + ], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "Found 1 extension(s)" in result.output + assert "real-ext" in result.output + assert "Ghost" not in result.output + + +def test_search_catalog_name_null_does_not_render_none(project_dir, monkeypatch): + """An explicit null _catalog_name must not print "Catalog: None". + + ``ext.get("_catalog_name", "")`` only substitutes on an absent key; an + explicit ``null`` reaches ``str()`` → "None", which is truthy and prints a + bogus "Catalog: None" line. Use _catalog_str so null/blank fall back to "". + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [{ + "id": "real-ext", + "name": "Real Ext", + "version": "1.0.0", + "description": "d", + "_catalog_name": None, + }], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "real-ext" in result.output + assert "Catalog: None" not in result.output + assert "None" not in result.output + + def test_search_escapes_markup_in_stars(project_dir, monkeypatch): """Catalog-controlled `stars` must be escaped, not parsed as Rich markup.""" monkeypatch.chdir(project_dir) @@ -128,6 +184,45 @@ def test_search_escapes_markup_in_stars(project_dir, monkeypatch): assert "[red]999[/red]" in result.output +def test_add_download_status_null_name_version_no_none(project_dir, monkeypatch): + """The catalog download status line must not render the literal "None". + + When a catalog entry sets name/version to explicit JSON null, ``.get()`` + returns None and reaches ``str()`` → "None". The status line must use + _catalog_str so null/blank fall back to the resolved id / "unknown". + """ + monkeypatch.chdir(project_dir) + + import specify_cli.extensions._commands as cmds + + # Force the catalog branch (no bundled match) and resolve to a null-field entry. + monkeypatch.setattr(cmds, "_locate_bundled_extension", lambda *a, **k: None) + monkeypatch.setattr( + cmds, + "_resolve_catalog_extension", + lambda extension, catalog, action: ( + {"id": "null-ext", "name": None, "version": None}, + None, + ), + ) + + # Stop the flow right after the status line prints. + def _boom(self, ext_id, *args, **kwargs): + raise RuntimeError("stop after status line") + + monkeypatch.setattr(ExtensionCatalog, "download_extension", _boom) + + result = runner.invoke( + app, ["extension", "add", "null-ext"], obj={"project_root": project_dir} + ) + + assert "Downloading" in result.output + # Falls back to the resolved id and "unknown", never the literal "None". + assert "null-ext" in result.output + assert "vunknown" in result.output + assert "None" not in result.output + + def test_info_tolerates_missing_fields_and_non_dict_sections(project_dir, monkeypatch): """`extension info` must survive a malformed catalog entry. diff --git a/tests/test_extension_list_available.py b/tests/test_extension_list_available.py index 8b0d1e6668..3132d800cf 100644 --- a/tests/test_extension_list_available.py +++ b/tests/test_extension_list_available.py @@ -181,6 +181,33 @@ def test_list_available_skips_entries_without_valid_id(project_dir, monkeypatch, assert "Ghost Ext" not in result.output +def test_list_available_all_invalid_ids_reports_empty(project_dir, monkeypatch): + """When every catalog entry has an invalid id, report no additional extensions. + + The emptiness check and the print loop must use the same notion of a valid + id (``_catalog_id()``). If emptiness were judged by raw ``ext.get("id")``, + entries with a blank/null id would inflate the list, get skipped by the + loop, and leave the "Available Extensions" header with no rows and no + "No additional extensions" fallback. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self: [ + {"id": None, "name": "Ghost One"}, + {"id": "", "name": "Ghost Two"}, + {"id": " ", "name": "Ghost Three"}, + ]) + + result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) + + assert result.exit_code == 0 + assert "Available Extensions:" in result.output + assert "No additional extensions available" in result.output + # None of the id-less entries leak into the output. + assert "Ghost" not in result.output + + def test_list_all_shows_installed_and_available(project_dir, monkeypatch): """--all lists installed extensions and available catalog extensions.""" monkeypatch.chdir(project_dir) From 41fd158984aeedc99bf207dc853ff2f110e7cc1a Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Wed, 8 Jul 2026 10:52:22 +0800 Subject: [PATCH 08/11] fix(extensions): harden catalog search against malformed fields Remote/community catalog entries may contain null values or malformed tag data. Search filtering runs before CLI rendering fallbacks, so it must normalize unsafe field shapes before matching query, author, and tag filters. Coerce non-string search fields to empty text, ignore non-list tags, and drop non-string tag entries during matching. Add a regression test that exercises the real ExtensionCatalog.search path by stubbing _get_merged_extensions instead of replacing search itself. --- src/specify_cli/extensions/__init__.py | 38 +++++++++----------- tests/test_extension_catalog_robustness.py | 40 ++++++++++++++++++++++ 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 1d79c4e027..6f15575852 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -4021,42 +4021,38 @@ def search( results = [] + def _search_text(value: Any) -> str: + return value if isinstance(value, str) else "" + + def _search_tags(value: Any) -> List[str]: + if not isinstance(value, list): + return [] + return [tag for tag in value if isinstance(tag, str)] + for ext_data in all_extensions: ext_id = ext_data["id"] + tags = _search_tags(ext_data.get("tags", [])) # Apply filters if verified_only and not ext_data.get("verified", False): continue - if author: - author_val = ext_data.get("author", "") - if not isinstance(author_val, str): - author_val = str(author_val) if author_val is not None else "" - if author_val.lower() != author.lower(): - continue + if author and _search_text(ext_data.get("author")).lower() != author.lower(): + continue - if tag: - raw_tags = ext_data.get("tags", []) - tags_list = raw_tags if isinstance(raw_tags, list) else [] - if tag.lower() not in [ - t.lower() for t in tags_list if isinstance(t, str) - ]: - continue + if tag and tag.lower() not in [t.lower() for t in tags]: + continue if query: # Search in name, description, and tags query_lower = query.lower() - raw_tags = ext_data.get("tags", []) - tags_list = raw_tags if isinstance(raw_tags, list) else [] - name_val = ext_data.get("name", "") - desc_val = ext_data.get("description", "") searchable_text = " ".join( [ - str(name_val) if name_val else "", - str(desc_val) if desc_val else "", - ext_id, + _search_text(ext_data.get("name")), + _search_text(ext_data.get("description")), + _search_text(ext_id), ] - + [t for t in tags_list if isinstance(t, str)] + + tags ).lower() if query_lower not in searchable_text: diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index fd303645d2..1a4a2afaa4 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -77,6 +77,46 @@ def test_search_json_null_fields_render_placeholders_not_none(project_dir, monke assert "None" not in result.output +def test_catalog_search_tolerates_null_fields_and_malformed_tags(project_dir, monkeypatch): + """Real catalog search must tolerate malformed remote catalog fields.""" + catalog = ExtensionCatalog(project_dir) + + good_entry = { + "id": "good-ext", + "name": "Good Extension", + "description": "Matches query text", + "author": "Jane", + "tags": ["tools", None, 123], + } + scalar_tags_entry = { + "id": "scalar-tags", + "name": "Scalar Tags", + "description": "Malformed tags should not be searched", + "author": "Jane", + "tags": "tools", + } + + monkeypatch.setattr( + catalog, + "_get_merged_extensions", + lambda: [ + { + "id": "null-fields", + "name": None, + "description": None, + "author": None, + "tags": None, + }, + scalar_tags_entry, + good_entry, + ], + ) + + assert catalog.search(query="query text") == [good_entry] + assert catalog.search(author="Jane") == [scalar_tags_entry, good_entry] + assert catalog.search(tag="tools") == [good_entry] + + @pytest.mark.parametrize("bad_id", [None, "", " "]) def test_search_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id): """Entries with a missing/blank/null id are skipped entirely. From 24ee04ec9bb5ae10cedd70d04952b38c9a2c800c Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Thu, 9 Jul 2026 22:42:26 +0800 Subject: [PATCH 09/11] fix(extensions): prevent malformed tag crashes Catalog entries are untrusted, so the extension command renderer now accepts only list-based string tags before joining values. This keeps scalar tags from raising TypeError and avoids rendering strings as character lists. --- src/specify_cli/extensions/_commands.py | 34 +++++---- tests/test_command_template_py_scripts.py | 3 +- tests/test_extension_catalog_robustness.py | 80 ++++++++++++++++++++-- tests/test_extension_list_available.py | 13 ++-- 4 files changed, 105 insertions(+), 25 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 00816f5b90..82ebfa992b 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -11,7 +11,6 @@ import errno import hashlib import os -import re import shutil import stat import tempfile @@ -25,8 +24,8 @@ from rich.panel import Panel from rich.table import Table -from .._console import console from .._assets import get_speckit_version +from .._console import console from .._download_security import ( archive_format_from_name, detect_archive_format, @@ -35,6 +34,7 @@ safe_extract_archive, ) from .._init_options import is_ai_skills_enabled +from . import EXTENSION_ID_PATTERN extension_app = typer.Typer( name="extension", @@ -61,9 +61,22 @@ def _catalog_str(ext: dict, key: str, fallback: str = "") -> str: def _catalog_id(ext: dict) -> str: - """Return an installable catalog ID, or an empty string when invalid.""" + """Return a usable extension id, or "" when the catalog entry lacks a + valid one. An empty result means callers should skip the install hint (and + ideally the entry) rather than emit a command ``download_extension()`` will + refuse.""" extension_id = _catalog_str(ext, "id") - return extension_id if re.fullmatch(r"[a-z0-9-]+", extension_id) else "" + if extension_id and EXTENSION_ID_PATTERN.fullmatch(extension_id): + return extension_id + return "" + + +def _catalog_tags(ext: dict) -> list[str]: + """Return renderable catalog tags from an untrusted catalog entry.""" + tags = ext.get("tags") + if not isinstance(tags, list): + return [] + return [tag.strip() for tag in tags if isinstance(tag, str) and tag.strip()] def _catalog_number(value) -> str: @@ -1300,12 +1313,7 @@ def extension_search( # Metadata console.print(f"\n [dim]Author:[/dim] {_escape_markup(_catalog_str(ext, 'author', 'Unknown'))}") - ext_tags = ext.get('tags', []) - tags = ( - [tag.strip() for tag in ext_tags if isinstance(tag, str) and tag.strip()] - if isinstance(ext_tags, list) - else [] - ) + tags = _catalog_tags(ext) if tags: tags_str = ", ".join(tags) console.print(f" [dim]Tags:[/dim] {_escape_markup(tags_str)}") @@ -1521,9 +1529,9 @@ def _print_extension_info(ext_info: dict, manager): console.print() # Tags - info_tags = ext_info.get('tags', []) - if isinstance(info_tags, list) and info_tags: - tags_str = ", ".join(str(t) for t in info_tags) + tags = _catalog_tags(ext_info) + if tags: + tags_str = ", ".join(tags) console.print(f"[bold]Tags:[/bold] {_escape_markup(tags_str)}") console.print() diff --git a/tests/test_command_template_py_scripts.py b/tests/test_command_template_py_scripts.py index a634f1f2f0..1a47b8cd28 100644 --- a/tests/test_command_template_py_scripts.py +++ b/tests/test_command_template_py_scripts.py @@ -53,7 +53,8 @@ def _pin_interpreter(monkeypatch): # Pin the probe to True so the interpreter token stays ``python3`` on all # platforms. monkeypatch.setattr( - "specify_cli.integrations.base.IntegrationBase._interpreter_runs", + IntegrationBase, + "_interpreter_runs", staticmethod(lambda path: True), ) diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index 1a4a2afaa4..2928f1e978 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -77,6 +77,52 @@ def test_search_json_null_fields_render_placeholders_not_none(project_dir, monke assert "None" not in result.output +def test_search_renders_only_string_list_tags(project_dir, monkeypatch): + """`extension search` must treat catalog tags as untrusted data. + + Non-list tags are ignored entirely; list tags render only string members. + This prevents scalar tags from crashing or printing as comma-separated + characters. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [ + { + "id": "int-tags", + "name": "Int Tags", + "version": "1.0.0", + "description": "d", + "tags": 123, + }, + { + "id": "string-tags", + "name": "String Tags", + "version": "1.0.0", + "description": "d", + "tags": "abc", + }, + { + "id": "mixed-tags", + "name": "Mixed Tags", + "version": "1.0.0", + "description": "d", + "tags": ["safe", None, 123, "ok"], + }, + ], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert result.output.count("Tags:") == 1 + assert "safe, ok" in result.output + assert "a, b, c" not in result.output + assert "123" not in result.output + + def test_catalog_search_tolerates_null_fields_and_malformed_tags(project_dir, monkeypatch): """Real catalog search must tolerate malformed remote catalog fields.""" catalog = ExtensionCatalog(project_dir) @@ -117,13 +163,13 @@ def test_catalog_search_tolerates_null_fields_and_malformed_tags(project_dir, mo assert catalog.search(tag="tools") == [good_entry] -@pytest.mark.parametrize("bad_id", [None, "", " "]) +@pytest.mark.parametrize("bad_id", [None, "", " ", "../escape", "foo/bar", "Bad_ID"]) def test_search_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id): - """Entries with a missing/blank/null id are skipped entirely. + """Entries with a missing/blank/null/invalid-format id are skipped entirely. Such an id cannot be installed (``download_extension()`` refuses it) and - previously crashed on ``ext['id']`` when absent. Skip the whole entry - rather than emit a bogus/dangling install hint. + missing ids previously crashed on ``ext['id']``. Skip the whole entry + rather than emit a bogus install hint. """ monkeypatch.chdir(project_dir) @@ -141,7 +187,7 @@ def test_search_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id) assert result.exit_code == 0, result.output assert "real-ext" in result.output assert "specify extension add real-ext" in result.output - # The id-less entry is dropped: no header, no dangling install hint. + # The invalid-id entry is dropped: no header, no bogus install hint. assert "Ghost Ext" not in result.output @@ -296,6 +342,30 @@ def test_info_tolerates_missing_fields_and_non_dict_sections(project_dir, monkey assert "[bold]42[/bold]" in result.output +def test_info_renders_only_string_list_tags(project_dir, monkeypatch): + """`extension info` uses the same untrusted tag rendering as search.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "mixed-tags", + "name": "Mixed Tags", + "version": "1.0.0", + "description": "d", + "tags": ["safe", None, 123, "ok"], + }, + ) + + result = runner.invoke(app, ["extension", "info", "mixed-tags"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "Tags: safe, ok" in result.output + assert "123" not in result.output + + def test_download_rejects_catalog_id_path_traversal(project_dir, monkeypatch): """Catalog-controlled IDs must not become path-traversing ZIP filenames.""" monkeypatch.chdir(project_dir) diff --git a/tests/test_extension_list_available.py b/tests/test_extension_list_available.py index 3132d800cf..5f45fcd1c1 100644 --- a/tests/test_extension_list_available.py +++ b/tests/test_extension_list_available.py @@ -155,13 +155,13 @@ def test_list_available_json_null_fields_render_placeholders_not_none(project_di assert "None" not in result.output -@pytest.mark.parametrize("bad_id", [None, "", " "]) +@pytest.mark.parametrize("bad_id", [None, "", " ", "../escape", "foo/bar", "Bad_ID"]) def test_list_available_skips_entries_without_valid_id(project_dir, monkeypatch, bad_id): - """Entries with a missing/blank/null id are skipped entirely. + """Entries with a missing/blank/null/invalid-format id are skipped entirely. Such an id cannot be installed (``download_extension()`` would refuse it), - so printing an install hint like ``specify extension add`` with no id — or - with ``None`` — only misleads the user. Skip the whole entry. + so printing an install hint like ``specify extension add`` with an invalid + id only misleads the user. Skip the whole entry. """ monkeypatch.chdir(project_dir) @@ -177,7 +177,7 @@ def test_list_available_skips_entries_without_valid_id(project_dir, monkeypatch, # The valid entry still renders with its install hint... assert "real-ext" in result.output assert "specify extension add real-ext" in result.output - # ...but the id-less entry is dropped: no name, no dangling install hint. + # ...but the invalid-id entry is dropped: no name, no bogus install hint. assert "Ghost Ext" not in result.output @@ -197,6 +197,7 @@ def test_list_available_all_invalid_ids_reports_empty(project_dir, monkeypatch): {"id": None, "name": "Ghost One"}, {"id": "", "name": "Ghost Two"}, {"id": " ", "name": "Ghost Three"}, + {"id": "../escape", "name": "Ghost Four"}, ]) result = runner.invoke(app, ["extension", "list", "--available"], obj={"project_root": project_dir}) @@ -204,7 +205,7 @@ def test_list_available_all_invalid_ids_reports_empty(project_dir, monkeypatch): assert result.exit_code == 0 assert "Available Extensions:" in result.output assert "No additional extensions available" in result.output - # None of the id-less entries leak into the output. + # None of the invalid-id entries leak into the output. assert "Ghost" not in result.output From d48b8cc670e28cdd4a9812c5e6ca98c957db258a Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Mon, 13 Jul 2026 20:29:29 +0800 Subject: [PATCH 10/11] fix(extensions): enforce trusted catalog identity boundaries Catalog entries can originate from remote/community sources and cached state, so install and info resolution must not coerce malformed fields into usable names or IDs. This keeps display-name lookup aligned with rendering, requires whole-string ID validation, and prevents default download cache writes through symlinked project paths. --- src/specify_cli/extensions/__init__.py | 68 +++++++++++++++++----- src/specify_cli/extensions/_commands.py | 8 +-- tests/test_extension_catalog_robustness.py | 52 +++++++++++++++++ tests/test_extensions.py | 64 ++++++++++++++++++++ 4 files changed, 174 insertions(+), 18 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 6f15575852..79d0d716cc 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -328,7 +328,7 @@ def _validate(self): ) # Validate extension ID format - if not EXTENSION_ID_PATTERN.match(ext["id"]): + if not EXTENSION_ID_PATTERN.fullmatch(ext["id"]): raise ValidationError( f"Invalid extension ID '{ext['id']}': " "must be lowercase alphanumeric with hyphens only" @@ -3485,6 +3485,54 @@ def __init__(self, project_root: Path): self.cache_file = self.cache_dir / "catalog.json" self.cache_metadata_file = self.cache_dir / "catalog-metadata.json" + def _ensure_default_download_cache_dir(self) -> Path: + """Create the default download cache without following symlink parents.""" + root = self.project_root.resolve() + target_dir = self.cache_dir / "downloads" + try: + rel = target_dir.relative_to(self.project_root) + except ValueError: + raise ExtensionError("Default extension download cache escapes project root") from None + + current = self.project_root + for part in rel.parts: + current = current / part + try: + label = current.relative_to(self.project_root).as_posix() + except ValueError: + label = str(current) + + if current.is_symlink(): + raise ExtensionError( + f"Refusing to use symlinked extension download cache path: {label}" + ) + if current.exists(): + if not current.is_dir(): + raise ExtensionError( + f"Extension download cache path is not a directory: {label}" + ) + try: + current.resolve().relative_to(root) + except (OSError, ValueError): + raise ExtensionError( + f"Extension download cache path escapes project root: {label}" + ) from None + continue + + current.mkdir() + if current.is_symlink(): + raise ExtensionError( + f"Refusing to use symlinked extension download cache path: {label}" + ) + try: + current.resolve().relative_to(root) + except (OSError, ValueError): + raise ExtensionError( + f"Extension download cache path escapes project root: {label}" + ) from None + + return target_dir + def _make_request(self, url: str): """Build a urllib Request, adding auth headers when a provider matches. @@ -4097,7 +4145,7 @@ def download_extension( """ import urllib.error - if not isinstance(extension_id, str) or not EXTENSION_ID_PATTERN.match( + if not isinstance(extension_id, str) or not EXTENSION_ID_PATTERN.fullmatch( extension_id ): raise ExtensionError( @@ -4154,19 +4202,11 @@ def download_extension( # Determine target path if target_dir is None: - target_dir = self.cache_dir / "downloads" - target_dir = Path(target_dir) + target_dir = self._ensure_default_download_cache_dir() + else: + target_dir = Path(target_dir) + target_dir.mkdir(parents=True, exist_ok=True) version = _safe_version_token(ext_info.get("version")) - declared_format = archive_format_from_name(download_url) - build_safe_download_path( - target_dir, - extension_id, - version, - error_type=ExtensionError, - label="extension", - suffix=archive_suffix(declared_format or "tar.gz"), - ) - target_dir.mkdir(parents=True, exist_ok=True) original_download_url = download_url extra_headers = None diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 82ebfa992b..7cf431c5f7 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -398,10 +398,10 @@ def _resolve_catalog_extension( table.add_column("Catalog", style="dim") for ext in name_matches: table.add_row( - _escape_markup(str(ext.get("id", ""))), - _escape_markup(str(ext.get("name", ""))), - _escape_markup(str(ext.get("version", ""))), - _escape_markup(str(ext.get("_catalog_name", ""))), + _escape_markup(_catalog_id(ext)), + _escape_markup(_catalog_str(ext, "name", "(unnamed)")), + _escape_markup(_catalog_str(ext, "version", "?")), + _escape_markup(_catalog_str(ext, "_catalog_name")), ) console.print(table) console.print("\nPlease rerun using the extension ID:") diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index 2928f1e978..7d9a7b0bd6 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -393,6 +393,58 @@ def fail_open_url(*args, **kwargs): catalog.download_extension(malicious_id) +@pytest.mark.parametrize( + ("argument", "catalog_entry"), + [ + ( + "none", + { + "id": "null-name", + "name": None, + "version": "1.0.0", + "description": "JSON null name", + }, + ), + ( + "123", + { + "id": "numeric-name", + "name": 123, + "version": "1.0.0", + "description": "Numeric name", + }, + ), + ( + "Hidden Name", + { + "id": "../bad", + "name": "Hidden Name", + "version": "1.0.0", + "description": "Invalid ID", + }, + ), + ], +) +def test_info_display_name_resolution_ignores_malformed_catalog_entries( + project_dir, monkeypatch, argument, catalog_entry +): + """Display-name lookup must follow the same untrusted-entry rules as rendering. + + JSON null / numeric names are unnamed, and invalid IDs are not installable + or addressable even when their display name matches exactly. + """ + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: None) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self, **kwargs: [catalog_entry]) + + result = runner.invoke(app, ["extension", "info", argument], obj={"project_root": project_dir}) + + assert result.exit_code == 1, result.output + assert f"Extension '{argument}' not found" in result.output + + def test_info_json_null_fields_render_placeholders_not_none(project_dir, monkeypatch): """`extension info` must coalesce JSON null fields, not print "None". diff --git a/tests/test_extensions.py b/tests/test_extensions.py index d668019087..c7789fe6d0 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -398,6 +398,21 @@ def test_invalid_extension_id(self, temp_dir, valid_manifest_data): with pytest.raises(ValidationError, match="Invalid extension ID"): ExtensionManifest(manifest_path) + def test_extension_id_with_trailing_newline_is_invalid(self, temp_dir, valid_manifest_data): + """A trailing newline must not pass the extension ID regex anchor.""" + import yaml + + valid_manifest_data["extension"]["id"] = "valid-id\n" + valid_manifest_data["provides"]["commands"] = [] + valid_manifest_data["hooks"] = {} + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, "w") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="Invalid extension ID"): + ExtensionManifest(manifest_path) + def test_invalid_version(self, temp_dir, valid_manifest_data): """Test manifest with invalid semantic version.""" import yaml @@ -8307,6 +8322,55 @@ def test_download_extension_raises_no_url_for_non_bundled(self, temp_dir): with pytest.raises(ExtensionError, match="has no download URL"): catalog.download_extension("some-ext") + def test_download_extension_rejects_trailing_newline_id(self, temp_dir): + """download_extension must require the whole extension ID to match.""" + project_dir = temp_dir / "project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + catalog = ExtensionCatalog(project_dir) + + with pytest.raises(ExtensionError, match="Invalid extension ID"): + catalog.download_extension("valid-id\n") + + def test_download_extension_rejects_symlinked_default_cache(self, temp_dir): + """The default download cache must not follow symlinked project paths.""" + from unittest.mock import patch + + if not can_create_symlink(temp_dir): + pytest.skip("symlinks are not available in this environment") + + project_dir = temp_dir / "project" + project_dir.mkdir() + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True) + outside_cache = temp_dir / "outside-cache" + outside_cache.mkdir() + + try: + os.symlink(outside_cache, extensions_dir / ".cache") + except OSError: + pytest.skip("directory symlinks are not available in this environment") + + catalog = ExtensionCatalog(project_dir) + ext_info = { + "name": "Test Extension", + "id": "test-ext", + "version": "1.0.0", + "description": "Test", + "download_url": "https://example.com/test-ext.zip", + } + + def fail_open_url(*args, **kwargs): + raise AssertionError("download should not be attempted with symlinked cache") + + with patch.object(catalog, "get_extension_info", return_value=ext_info), \ + patch.object(catalog, "_open_url", side_effect=fail_open_url): + with pytest.raises(ExtensionError, match="symlinked extension download cache"): + catalog.download_extension("test-ext") + + assert not (outside_cache / "downloads" / "test-ext-1.0.0.zip").exists() + class TestExtensionUpdateCLI: """CLI integration tests for extension update command.""" From 5826de1a160aab711bc52ee6c742206cfee6cc9e Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Fri, 7 Aug 2026 14:13:50 +0800 Subject: [PATCH 11/11] fix --- tests/test_extension_catalog_robustness.py | 85 ++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/tests/test_extension_catalog_robustness.py b/tests/test_extension_catalog_robustness.py index 7d9a7b0bd6..20c8f8907c 100644 --- a/tests/test_extension_catalog_robustness.py +++ b/tests/test_extension_catalog_robustness.py @@ -80,9 +80,9 @@ def test_search_json_null_fields_render_placeholders_not_none(project_dir, monke def test_search_renders_only_string_list_tags(project_dir, monkeypatch): """`extension search` must treat catalog tags as untrusted data. - Non-list tags are ignored entirely; list tags render only string members. - This prevents scalar tags from crashing or printing as comma-separated - characters. + Non-list tags are ignored entirely; list tags render only non-blank string + members. This prevents scalar tags from crashing or printing as + comma-separated characters. """ monkeypatch.chdir(project_dir) @@ -109,7 +109,7 @@ def test_search_renders_only_string_list_tags(project_dir, monkeypatch): "name": "Mixed Tags", "version": "1.0.0", "description": "d", - "tags": ["safe", None, 123, "ok"], + "tags": ["safe", None, 123, " ", "ok"], }, ], ) @@ -270,6 +270,31 @@ def test_search_escapes_markup_in_stars(project_dir, monkeypatch): assert "[red]999[/red]" in result.output +def test_search_omits_malformed_download_count(project_dir, monkeypatch): + """A nonnumeric catalog downloads value must not crash search rendering.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr( + ExtensionCatalog, + "search", + lambda self, **kwargs: [{ + "id": "bad-downloads", + "name": "Bad Downloads", + "version": "1.0.0", + "description": "d", + "downloads": "many", + "stars": "[red]999[/red]", + }], + ) + + result = runner.invoke(app, ["extension", "search"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "bad-downloads" in result.output + assert "Downloads:" not in result.output + assert "[red]999[/red]" in result.output + + def test_add_download_status_null_name_version_no_none(project_dir, monkeypatch): """The catalog download status line must not render the literal "None". @@ -342,6 +367,32 @@ def test_info_tolerates_missing_fields_and_non_dict_sections(project_dir, monkey assert "[bold]42[/bold]" in result.output +def test_info_omits_malformed_download_count(project_dir, monkeypatch): + """A nonnumeric catalog downloads value must not crash info rendering.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "bad-downloads", + "name": "Bad Downloads", + "version": "1.0.0", + "description": "d", + "downloads": "many", + "stars": "[bold]42[/bold]", + }, + ) + + result = runner.invoke(app, ["extension", "info", "bad-downloads"], obj={"project_root": project_dir}) + + assert result.exit_code == 0, result.output + assert "bad-downloads" in result.output + assert "Downloads:" not in result.output + assert "[bold]42[/bold]" in result.output + + def test_info_renders_only_string_list_tags(project_dir, monkeypatch): """`extension info` uses the same untrusted tag rendering as search.""" monkeypatch.chdir(project_dir) @@ -355,7 +406,7 @@ def test_info_renders_only_string_list_tags(project_dir, monkeypatch): "name": "Mixed Tags", "version": "1.0.0", "description": "d", - "tags": ["safe", None, 123, "ok"], + "tags": ["safe", None, 123, " ", "ok"], }, ) @@ -445,6 +496,30 @@ def test_info_display_name_resolution_ignores_malformed_catalog_entries( assert f"Extension '{argument}' not found" in result.output +def test_info_direct_id_resolution_rejects_invalid_catalog_id(project_dir, monkeypatch): + """Direct catalog lookup must not bypass the shared catalog-id validation.""" + monkeypatch.chdir(project_dir) + + monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: []) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "../bad", + "name": "Hidden Name", + "version": "1.0.0", + "description": "Invalid ID", + }, + ) + monkeypatch.setattr(ExtensionCatalog, "search", lambda self, **kwargs: []) + + result = runner.invoke(app, ["extension", "info", "../bad"], obj={"project_root": project_dir}) + + assert result.exit_code == 1, result.output + assert "Extension '../bad' not found" in result.output + assert "Hidden Name" not in result.output + + def test_info_json_null_fields_render_placeholders_not_none(project_dir, monkeypatch): """`extension info` must coalesce JSON null fields, not print "None".