Skip to content

fix(presets): escape user-supplied catalog name/URL in preset catalog add/remove output - #3806

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix-preset-catalog-markup
Open

fix(presets): escape user-supplied catalog name/URL in preset catalog add/remove output#3806
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix-preset-catalog-markup

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

preset_catalog_add and preset_catalog_remove interpolate the raw --name and URL straight into console.print(), so Rich parses user input as markup. Two distinct failure modes, both reproduced on current main (2e44ed6):

1. Silent misreporting — the markup is consumed, so the reported name isn't the persisted name:

name='[bold red]pwned[/]'
  add -> exit 0
    "Added catalog 'pwned' (discovery only)"      <-- reports 'pwned'

The YAML records [bold red]pwned[/]. A user who then runs preset catalog remove pwned — the only name they were shown — gets "not found".

2. Unhandled MarkupError, and it fires after the config is written:

name='safe'  url='https://example.com/[/bold]c.json'
  add -> exit 1
    UNHANDLED MarkupError: closing tag '[/bold]' at position 27 doesn't match any open tag
  rm  -> exit 0
    "Removed catalog 'safe'"                      <-- so it HAD been added

The traceback tells the user the command failed. The subsequent successful remove proves it succeeded. remove crashes the same way on a bracketed name, including on its not-found error path.

Why this is a gap rather than a design choice

presets/_commands.py already imports _escape_markup (L16) and already escapes exactly these fields in preset catalog list (L589-592), an invariant pinned by the existing test_catalog_list_escapes_rich_markup in the same test class. add/remove were simply missed. This also mirrors the escaping just merged for the integrations twin in #3772.

Fix

~10 changed lines in one file: safe_name/safe_url locals used at every render site (duplicate-name warning, the added/removed confirmations, the URL line, the not-found error, and the Config saved to label).

Only rendering changes. The raw name still drives existing.get("name") == name and still goes into preset-catalogs.yml. One of the new tests asserts that round-trip explicitly:

assert config["catalogs"][0]["name"] == name   # raw, with brackets
assert config["catalogs"][0]["url"] == url

After the fix, all three repro cases exit 0 and print the name verbatim ([bold red]pwned[/], not pwned).

Test placement

The three new tests go inside the existing TestPresetCatalogMultiCatalog class, directly after test_catalog_list_escapes_rich_markup — the precedent test for this exact invariant.

I chose that deliberately over adding a class at EOF: my own open #3773 appends class TestPresetCatalogRichMarkup to this file and #3769 appends there too. A duplicate class name in one module silently shadows the earlier definition — it would have dropped #3773's tests from the run with no error. Placing these mid-file also keeps this PR from colliding with either append.

Verification

  • Fail-before / pass-after: all 3 new tests fail on unpatched src (assert '[/red]absent' in '' where output is empty because of the MarkupError) and pass with the fix.
  • tests/test_presets.py + tests/integrations/test_cli.py → failure set is byte-identical to clean main (7+7, all Windows symlink-privilege tests); passed count 464 → 467, exactly my 3 new tests.
  • Two existing tests do assert on this output and both still pass — test_catalog_config_output_uses_posix_paths (asserts Config saved to .specify/preset-catalogs.yml) and test_project_scoped_commands_require_specify_directory, which invokes preset catalog add/remove. rich.markup.escape is a no-op on bracket-free paths.
  • uvx [email protected] check → clean

Note

PresetCatalog._validate_catalog_url is a standalone class rather than a CatalogStackBase subclass, so nothing here changes validation behaviour — this PR is display-only.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main; every output above is from runs I did.

`preset catalog add` and `preset catalog remove` interpolate the raw
`--name` and URL into `console.print()`, so Rich parses them as markup.
Two failure modes:

* Silent misreporting — a name like `[bold red]pwned[/]` is printed as
  `pwned`, so the confirmed name is not the persisted name and a later
  `remove` with the reported name fails.
* Unhandled MarkupError — an unbalanced closing tag raises, and because
  the crash happens *after* preset-catalogs.yml is written, the user gets
  a traceback for a catalog that was in fact added.

This file already imports `_escape_markup` and escapes name/description/
url in `preset catalog list` (whose invariant `test_catalog_list_escapes_
rich_markup` already pins); `add`/`remove` were the remaining gaps.

Only rendering changes: the raw values are still what get persisted and
what the duplicate-name comparison uses.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

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

Escapes user-controlled preset catalog names, URLs, and paths before Rich rendering while preserving raw persisted values.

Changes:

  • Escapes all affected add/remove output paths.
  • Adds regression tests for successful and not-found flows.
Show a summary per file
File Description
src/specify_cli/presets/_commands.py Safely renders catalog values.
tests/test_presets.py Tests markup escaping and persistence.

Review details

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Medium

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