Skip to content

fix(extensions): guard non-numeric catalog downloads in search/info rendering#3710

Open
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/extension-downloads-non-numeric-guard
Open

fix(extensions): guard non-numeric catalog downloads in search/info rendering#3710
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/extension-downloads-non-numeric-guard

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

specify extension search and specify extension info <id> render a catalog entry's downloads field with the :, thousands separator:

stats.append(f"Downloads: {ext['downloads']:,}")   # extension_search
...
stats.append(f"Downloads: {ext_info['downloads']:,}")  # _print_extension_info

guarded only by is not None. But :, is only valid for int/float — on a string it raises ValueError: Cannot specify ',' with 's'.

Catalog payloads are only shape-validated (_validate_catalog_payload checks that extensions is a dict); individual entry fields are never type-checked, and _get_merged_extensions returns the raw catalog dicts. So a catalog entry with "downloads": "1500" (a quoted number — realistic from a community catalog, a SPECKIT_CATALOG_URL catalog, a project/user catalog, or a poisoned cache) makes the whole command abort with an uncaught traceback. Reproduced end-to-end: with such a cache, both extension search and extension info exit non-zero with ValueError("Cannot specify ',' with 's'.").

Fix

Group-format downloads only when it is actually numeric; otherwise render it as-is:

downloads = ext.get('downloads')
if downloads is not None:
    stats.append(
        f"Downloads: {downloads:,}"
        if isinstance(downloads, (int, float))
        else f"Downloads: {downloads}"
    )

(and the equivalent in _print_extension_info). Numeric values (int/float, including bool) take the identical :, branch, so correct catalogs render byte-for-byte the same — only the previously-crashing case changes to print the value. This mirrors the module's existing defensive treatment: every other field in these two renderers is already str()-wrapped.

Verification

  • test_info_renders_non_numeric_downloads (direct) and test_search_survives_non_numeric_downloads (end-to-end via CliRunner): both fail before with the exact ValueError, pass after (exit_code == 0, output shows Downloads: 1500).
  • TestExtensionCatalog: 64 passed. ruff clean.

AI-assisted: authored with Claude Code. I reproduced the crash on main through the real CLI (catalog cache with a string downloads) before writing the guard, and confirmed numeric output is unchanged.

…endering

`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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Guards extension catalog rendering against non-numeric download counts.

Changes:

  • Formats numeric downloads with separators and renders other values directly.
  • Adds coverage for search and info rendering.
Show a summary per file
File Description
src/specify_cli/extensions/_commands.py Adds defensive download rendering.
tests/test_extensions.py Tests string-valued downloads.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/specify_cli/extensions/_commands.py:991

  • The info renderer also passes the untrusted fallback directly to Rich. For example, downloads="[/red]foo" still produces an uncaught MarkupError, while valid markup can inject styling or links. Escape the string fallback so arbitrary non-numeric catalog values actually render safely.
        stats.append(
            f"Downloads: {downloads:,}"
            if isinstance(downloads, (int, float))
            else f"Downloads: {downloads}"
        )
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread src/specify_cli/extensions/_commands.py
…arkup

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>
@jawwad-ali

Copy link
Copy Markdown
Contributor Author

You're right — fixed in 135444f. Guarding the :, ValueError just traded it for a MarkupError: I confirmed Console().print("Downloads: [/red]foo") raises closing tag '[/red]' doesn't match any open tag, so extension search/info still aborted on a hostile catalog value.

Both sites now escape the fallback — f"Downloads: {_escape_markup(str(downloads))}" — matching how every other catalog field in these renderers is already handled. The numeric branch keeps the identical :, format, so correct catalogs render byte-for-byte the same.

Tests parametrized over "1500", "[/red]foo" (unbalanced → MarkupError) and "[bold]x[/bold]" (balanced → would silently restyle); the two markup cases fail before this commit and pass after.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants