mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(presets): escape user-supplied catalog name/URL in add/remove output (#3806)
`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) <noreply@anthropic.com>
This commit is contained in:
@@ -717,10 +717,15 @@ def preset_catalog_add(
|
||||
console.print("[red]Error:[/red] Invalid catalog config: 'catalogs' must be a list.")
|
||||
raise typer.Exit(1)
|
||||
|
||||
# Only rendering is escaped — the raw values are what get persisted and
|
||||
# compared below, so a name containing markup still round-trips exactly.
|
||||
safe_name = _escape_markup(str(name))
|
||||
safe_url = _escape_markup(str(url))
|
||||
|
||||
# Check for duplicate name
|
||||
for existing in catalogs:
|
||||
if isinstance(existing, dict) and existing.get("name") == name:
|
||||
console.print(f"[yellow]Warning:[/yellow] A catalog named '{name}' already exists.")
|
||||
console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.")
|
||||
console.print("Use 'specify preset catalog remove' first, or choose a different name.")
|
||||
raise typer.Exit(1)
|
||||
|
||||
@@ -736,10 +741,11 @@ def preset_catalog_add(
|
||||
config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8")
|
||||
|
||||
install_label = "install allowed" if install_allowed else "discovery only"
|
||||
console.print(f"\n[green]✓[/green] Added catalog '[bold]{name}[/bold]' ({install_label})")
|
||||
console.print(f" URL: {url}")
|
||||
console.print(f"\n[green]✓[/green] Added catalog '[bold]{safe_name}[/bold]' ({install_label})")
|
||||
console.print(f" URL: {safe_url}")
|
||||
console.print(f" Priority: {priority}")
|
||||
console.print(f"\nConfig saved to {_display_project_path(project_root, config_path)}")
|
||||
config_label = _escape_markup(str(_display_project_path(project_root, config_path)))
|
||||
console.print(f"\nConfig saved to {config_label}")
|
||||
|
||||
|
||||
@preset_catalog_app.command("remove")
|
||||
@@ -767,17 +773,20 @@ def preset_catalog_remove(
|
||||
if not isinstance(catalogs, list):
|
||||
console.print("[red]Error:[/red] Invalid catalog config: 'catalogs' must be a list.")
|
||||
raise typer.Exit(1)
|
||||
# Rendering only — the raw name drives the comparison below.
|
||||
safe_name = _escape_markup(str(name))
|
||||
|
||||
original_count = len(catalogs)
|
||||
catalogs = [c for c in catalogs if isinstance(c, dict) and c.get("name") != name]
|
||||
|
||||
if len(catalogs) == original_count:
|
||||
console.print(f"[red]Error:[/red] Catalog '{name}' not found.")
|
||||
console.print(f"[red]Error:[/red] Catalog '{safe_name}' not found.")
|
||||
raise typer.Exit(1)
|
||||
|
||||
config["catalogs"] = catalogs
|
||||
config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8")
|
||||
|
||||
console.print(f"[green]✓[/green] Removed catalog '{name}'")
|
||||
console.print(f"[green]✓[/green] Removed catalog '{safe_name}'")
|
||||
if not catalogs:
|
||||
console.print("\n[dim]No catalogs remain in config. Built-in defaults will be used.[/dim]")
|
||||
|
||||
|
||||
@@ -2826,6 +2826,80 @@ class TestPresetCatalogMultiCatalog:
|
||||
assert "https://example.com/[cat].json" in result.output
|
||||
assert "desc [with] brackets" in result.output
|
||||
|
||||
def test_catalog_add_escapes_rich_markup(self, project_dir):
|
||||
"""`preset catalog add` must not parse the name/url as Rich markup.
|
||||
|
||||
An unbalanced closing tag raised MarkupError *after* the entry was
|
||||
already written to preset-catalogs.yml, so the user saw a traceback
|
||||
and no confirmation for a catalog that had in fact been added.
|
||||
"""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
name = "[/red]my-catalog"
|
||||
url = "https://example.com/[bold]c.json"
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(
|
||||
app, ["preset", "catalog", "add", url, "--name", name]
|
||||
)
|
||||
assert result.exit_code == 0, result.output
|
||||
# Rendered verbatim, not swallowed as markup.
|
||||
assert name in result.output
|
||||
assert url in result.output
|
||||
# Only rendering is escaped: the raw values still round-trip to disk.
|
||||
config = yaml.safe_load(
|
||||
(project_dir / ".specify" / "preset-catalogs.yml").read_text(
|
||||
encoding="utf-8"
|
||||
)
|
||||
)
|
||||
assert config["catalogs"][0]["name"] == name
|
||||
assert config["catalogs"][0]["url"] == url
|
||||
|
||||
def test_catalog_remove_escapes_rich_markup(self, project_dir):
|
||||
"""`preset catalog remove` must not parse the name as Rich markup."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
name = "[/red]my-catalog"
|
||||
(project_dir / ".specify" / "preset-catalogs.yml").write_text(
|
||||
yaml.dump({
|
||||
"catalogs": [
|
||||
{
|
||||
"name": name,
|
||||
"url": "https://example.com/c.json",
|
||||
"priority": 1,
|
||||
"install_allowed": False,
|
||||
}
|
||||
]
|
||||
}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "catalog", "remove", name])
|
||||
assert result.exit_code == 0, result.output
|
||||
assert name in result.output
|
||||
|
||||
def test_catalog_remove_escapes_markup_in_not_found_error(self, project_dir):
|
||||
"""The not-found error path renders the name too."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
(project_dir / ".specify" / "preset-catalogs.yml").write_text(
|
||||
yaml.dump({"catalogs": []}), encoding="utf-8"
|
||||
)
|
||||
runner = CliRunner()
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(
|
||||
app, ["preset", "catalog", "remove", "[/red]absent"]
|
||||
)
|
||||
assert result.exit_code == 1
|
||||
assert "[/red]absent" in result.output
|
||||
|
||||
def test_env_var_overrides_catalogs(self, project_dir, monkeypatch):
|
||||
"""Test that SPECKIT_PRESET_CATALOG_URL env var overrides defaults."""
|
||||
monkeypatch.setenv(
|
||||
|
||||
Reference in New Issue
Block a user