Skip to content

Commit d080d5c

Browse files
darion-yaphetOmX
andcommitted
Prevent path and manifest review regressions
Catalog path labels are rendered through Rich markup and downloaded update manifests are trusted long enough to validate extension IDs. Escape displayed project paths before rendering, and reject non-mapping extension.yml payloads before ID validation so bad archives fail with a clear rollback reason. Constraint: Preserve existing catalog config paths and update rollback flow Rejected: Rely on AttributeError from manifest_data.get | produces confusing failure text and masks the invalid archive shape Confidence: high Scope-risk: narrow Directive: Validate parsed YAML shape before field access in archive/update paths Tested: .venv/bin/python -m pytest tests/test_extensions.py -q Co-authored-by: OmX <omx@oh-my-codex.dev>
1 parent 3a57a2d commit d080d5c

2 files changed

Lines changed: 131 additions & 4 deletions

File tree

src/specify_cli/extensions/_commands.py

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,8 @@ def catalog_list():
276276
except ValidationError:
277277
proj_loaded = False
278278
if proj_loaded:
279-
console.print(f"[dim]Config: {_display_project_path(project_root, config_path)}[/dim]")
279+
config_label = _escape_markup(str(_display_project_path(project_root, config_path)))
280+
console.print(f"[dim]Config: {config_label}[/dim]")
280281
else:
281282
try:
282283
user_loaded = user_config_path.exists() and catalog._load_catalog_config(user_config_path) is not None
@@ -354,7 +355,8 @@ def catalog_add(
354355
console.print(f"\n[green]✓[/green] Added catalog '[bold]{safe_name}[/bold]' ({install_label})")
355356
console.print(f" URL: {safe_url}")
356357
console.print(f" Priority: {priority}")
357-
console.print(f"\nConfig saved to {_display_project_path(project_root, config_path)}")
358+
config_label = _escape_markup(str(_display_project_path(project_root, config_path)))
359+
console.print(f"\nConfig saved to {config_label}")
358360

359361

360362
@catalog_app.command("remove")
@@ -1189,17 +1191,23 @@ def extension_update(
11891191
# First try root-level extension.yml
11901192
if "extension.yml" in namelist:
11911193
with zf.open("extension.yml") as f:
1192-
manifest_data = yaml.safe_load(f) or {}
1194+
parsed_manifest = yaml.safe_load(f)
1195+
manifest_data = parsed_manifest if parsed_manifest is not None else {}
11931196
else:
11941197
# Look for extension.yml in a single top-level subdirectory
11951198
# (e.g., "repo-name-branch/extension.yml")
11961199
manifest_paths = [n for n in namelist if n.endswith("/extension.yml") and n.count("/") == 1]
11971200
if len(manifest_paths) == 1:
11981201
with zf.open(manifest_paths[0]) as f:
1199-
manifest_data = yaml.safe_load(f) or {}
1202+
parsed_manifest = yaml.safe_load(f)
1203+
manifest_data = parsed_manifest if parsed_manifest is not None else {}
12001204

12011205
if manifest_data is None:
12021206
raise ValueError("Downloaded extension archive is missing 'extension.yml'")
1207+
if not isinstance(manifest_data, dict):
1208+
raise ValueError(
1209+
"Invalid extension manifest in downloaded archive: expected YAML mapping"
1210+
)
12031211

12041212
zip_extension_id = manifest_data.get("extension", {}).get("id")
12051213
if zip_extension_id != extension_id:

tests/test_extensions.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4700,6 +4700,78 @@ def test_catalog_add_escapes_url_markup(self, tmp_path):
47004700
assert result.exit_code == 0, result.output
47014701
assert f"URL: {url}" in result.output
47024702

4703+
def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path):
4704+
"""Catalog add's saved-path label should render literally under Rich."""
4705+
from typer.testing import CliRunner
4706+
from unittest.mock import patch
4707+
from specify_cli import app
4708+
4709+
project_dir = tmp_path / "test-project"
4710+
project_dir.mkdir()
4711+
(project_dir / ".specify").mkdir()
4712+
4713+
display_path = "project[red]/.specify/extension-catalogs.yml"
4714+
4715+
runner = CliRunner()
4716+
with patch.object(Path, "cwd", return_value=project_dir), \
4717+
patch("specify_cli.extensions._commands._display_project_path", return_value=display_path):
4718+
result = runner.invoke(
4719+
app,
4720+
[
4721+
"extension",
4722+
"catalog",
4723+
"add",
4724+
"https://example.com/catalog.json",
4725+
"--name",
4726+
"community",
4727+
],
4728+
catch_exceptions=True,
4729+
)
4730+
4731+
assert result.exit_code == 0, result.output
4732+
assert f"Config saved to {display_path}" in result.output
4733+
4734+
def test_catalog_list_escapes_config_path_markup(self, tmp_path):
4735+
"""Catalog list's config-path label should render literally under Rich."""
4736+
from typer.testing import CliRunner
4737+
from unittest.mock import patch
4738+
from specify_cli import app
4739+
import yaml
4740+
4741+
project_dir = tmp_path / "test-project"
4742+
project_dir.mkdir()
4743+
specify_dir = project_dir / ".specify"
4744+
specify_dir.mkdir()
4745+
(specify_dir / "extension-catalogs.yml").write_text(
4746+
yaml.safe_dump(
4747+
{
4748+
"catalogs": [
4749+
{
4750+
"name": "community",
4751+
"url": "https://example.com/catalog.json",
4752+
"priority": 10,
4753+
"install_allowed": False,
4754+
}
4755+
]
4756+
}
4757+
),
4758+
encoding="utf-8",
4759+
)
4760+
4761+
display_path = "project[red]/.specify/extension-catalogs.yml"
4762+
4763+
runner = CliRunner()
4764+
with patch.object(Path, "cwd", return_value=project_dir), \
4765+
patch("specify_cli.extensions._commands._display_project_path", return_value=display_path):
4766+
result = runner.invoke(
4767+
app,
4768+
["extension", "catalog", "list"],
4769+
catch_exceptions=True,
4770+
)
4771+
4772+
assert result.exit_code == 0, result.output
4773+
assert f"Config: {display_path}" in result.output
4774+
47034775
def test_catalog_add_escapes_config_read_exception_markup(self, tmp_path):
47044776
"""Catalog config parse errors can include user-controlled file content."""
47054777
import yaml
@@ -5472,6 +5544,53 @@ def test_update_failure_rolls_back_registry_hooks_and_commands(self, tmp_path, m
54725544
for cmd_file in command_files:
54735545
assert cmd_file.exists(), f"Expected command file to be restored after rollback: {cmd_file}"
54745546

5547+
def test_update_rejects_non_mapping_zip_manifest(self, tmp_path, monkeypatch):
5548+
"""Downloaded extension.yml must be a mapping before ID validation."""
5549+
from typer.testing import CliRunner
5550+
from unittest.mock import patch
5551+
from specify_cli import app
5552+
import zipfile
5553+
5554+
fake_home = tmp_path / "home"
5555+
fake_home.mkdir()
5556+
monkeypatch.setattr(Path, "home", lambda: fake_home)
5557+
5558+
runner = CliRunner()
5559+
project_dir = tmp_path / "project"
5560+
project_dir.mkdir()
5561+
(project_dir / ".specify").mkdir()
5562+
(project_dir / ".claude" / "skills").mkdir(parents=True)
5563+
5564+
manager = ExtensionManager(project_dir)
5565+
v1_dir = self._create_extension_source(tmp_path, "1.0.0")
5566+
manager.install_from_directory(v1_dir, "0.1.0")
5567+
original_registry_entry = manager.registry.get("test-ext")
5568+
5569+
zip_path = tmp_path / "bad-manifest.zip"
5570+
with zipfile.ZipFile(zip_path, "w") as zf:
5571+
zf.writestr("extension.yml", "- not\n- a\n- mapping\n")
5572+
5573+
with patch.object(Path, "cwd", return_value=project_dir), \
5574+
patch.object(ExtensionCatalog, "get_extension_info", return_value={
5575+
"id": "test-ext",
5576+
"name": "Test Extension",
5577+
"version": "2.0.0",
5578+
"_install_allowed": True,
5579+
}), \
5580+
patch.object(ExtensionCatalog, "download_extension", return_value=zip_path):
5581+
result = runner.invoke(
5582+
app,
5583+
["extension", "update", "test-ext"],
5584+
input="y\n",
5585+
catch_exceptions=True,
5586+
)
5587+
5588+
assert result.exit_code == 1, result.output
5589+
assert "Invalid extension manifest in downloaded archive" in result.output
5590+
assert "YAML mapping" in result.output
5591+
assert "AttributeError" not in result.output
5592+
assert ExtensionManager(project_dir).registry.get("test-ext") == original_registry_entry
5593+
54755594

54765595
class TestExtensionListCLI:
54775596
"""Test extension list CLI output format."""

0 commit comments

Comments
 (0)