Skip to content

Commit 3a57a2d

Browse files
darion-yaphetOmX
darion-yaphet
and
OmX
committed
Prevent markup injection from exception text
Rich markup remains enabled for styled CLI messages, so exception text and config path labels must be escaped before rendering. YAML parser errors, URL validation failures, download errors, and extension validation errors can include user-controlled catalog or manifest values. Constraint: Preserve existing exception handling and user-facing error paths Rejected: Disable Rich markup for these messages | existing output intentionally uses markup for labels and styling Confidence: high Scope-risk: narrow Directive: Escape user-controlled exception text before interpolating into Rich-rendered strings Tested: .venv/bin/python -m pytest tests/test_extensions.py -q Co-authored-by: OmX <omx@oh-my-codex.dev>
1 parent eb3d72c commit 3a57a2d

2 files changed

Lines changed: 157 additions & 12 deletions

File tree

‎src/specify_cli/extensions/_commands.py‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,14 @@ def _load_catalog_command_config(project_root: Path, config_path: Path) -> dict:
6565
try:
6666
config = yaml.safe_load(config_path.read_text(encoding="utf-8"))
6767
except Exception as e:
68-
config_label = _display_project_path(project_root, config_path)
69-
console.print(f"[red]Error:[/red] Failed to read {config_label}: {e}")
68+
config_label = _escape_markup(str(_display_project_path(project_root, config_path)))
69+
console.print(f"[red]Error:[/red] Failed to read {config_label}: {_escape_markup(str(e))}")
7070
raise typer.Exit(1)
7171

7272
if config is None:
7373
return {}
7474
if not isinstance(config, dict):
75-
config_label = _display_project_path(project_root, config_path)
75+
config_label = _escape_markup(str(_display_project_path(project_root, config_path)))
7676
console.print(
7777
f"[red]Error:[/red] Invalid catalog config {config_label}: "
7878
"expected a YAML mapping at the root."
@@ -249,7 +249,7 @@ def catalog_list():
249249
try:
250250
active_catalogs = catalog.get_active_catalogs()
251251
except ValidationError as e:
252-
console.print(f"[red]Error:[/red] {e}")
252+
console.print(f"[red]Error:[/red] {_escape_markup(str(e))}")
253253
raise typer.Exit(1)
254254

255255
console.print("\n[bold cyan]Active Extension Catalogs:[/bold cyan]\n")
@@ -313,7 +313,7 @@ def catalog_add(
313313
try:
314314
tmp_catalog._validate_catalog_url(url)
315315
except ValidationError as e:
316-
console.print(f"[red]Error:[/red] {e}")
316+
console.print(f"[red]Error:[/red] {_escape_markup(str(e))}")
317317
raise typer.Exit(1)
318318

319319
config_path = specify_dir / "extension-catalogs.yml"
@@ -505,7 +505,10 @@ def extension_add(
505505
# Install from downloaded ZIP
506506
manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority, force=force)
507507
except urllib.error.URLError as e:
508-
console.print(f"[red]Error:[/red] Failed to download from {safe_url}: {e}")
508+
console.print(
509+
f"[red]Error:[/red] Failed to download from {safe_url}: "
510+
f"{_escape_markup(str(e))}"
511+
)
509512
raise typer.Exit(1)
510513
finally:
511514
# Clean up downloaded ZIP
@@ -615,13 +618,13 @@ def extension_add(
615618
console.print(f" Check: .specify/extensions/{_escape_markup(str(manifest.id))}/")
616619

617620
except ValidationError as e:
618-
console.print(f"\n[red]Validation Error:[/red] {e}")
621+
console.print(f"\n[red]Validation Error:[/red] {_escape_markup(str(e))}")
619622
raise typer.Exit(1)
620623
except CompatibilityError as e:
621-
console.print(f"\n[red]Compatibility Error:[/red] {e}")
624+
console.print(f"\n[red]Compatibility Error:[/red] {_escape_markup(str(e))}")
622625
raise typer.Exit(1)
623626
except ExtensionError as e:
624-
console.print(f"\n[red]Error:[/red] {e}")
627+
console.print(f"\n[red]Error:[/red] {_escape_markup(str(e))}")
625628
raise typer.Exit(1)
626629

627630

@@ -768,7 +771,7 @@ def extension_search(
768771
console.print()
769772

770773
except ExtensionError as e:
771-
console.print(f"\n[red]Error:[/red] {e}")
774+
console.print(f"\n[red]Error:[/red] {_escape_markup(str(e))}")
772775
console.print("\nTip: The catalog may be temporarily unavailable. Try again later.")
773776
raise typer.Exit(1)
774777

@@ -1396,10 +1399,10 @@ def extension_update(
13961399
raise typer.Exit(1)
13971400

13981401
except ValidationError as e:
1399-
console.print(f"\n[red]Validation Error:[/red] {e}")
1402+
console.print(f"\n[red]Validation Error:[/red] {_escape_markup(str(e))}")
14001403
raise typer.Exit(1)
14011404
except ExtensionError as e:
1402-
console.print(f"\n[red]Error:[/red] {e}")
1405+
console.print(f"\n[red]Error:[/red] {_escape_markup(str(e))}")
14031406
raise typer.Exit(1)
14041407

14051408

‎tests/test_extensions.py‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4700,6 +4700,75 @@ 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_read_exception_markup(self, tmp_path):
4704+
"""Catalog config parse errors can include user-controlled file content."""
4705+
import yaml
4706+
from typer.testing import CliRunner
4707+
from unittest.mock import patch
4708+
from specify_cli import app
4709+
4710+
project_dir = tmp_path / "test-project"
4711+
project_dir.mkdir()
4712+
specify_dir = project_dir / ".specify"
4713+
specify_dir.mkdir()
4714+
(specify_dir / "extension-catalogs.yml").write_text("[red]bad[/red]", encoding="utf-8")
4715+
4716+
runner = CliRunner()
4717+
with patch.object(Path, "cwd", return_value=project_dir), \
4718+
patch(
4719+
"specify_cli.extensions._commands.yaml.safe_load",
4720+
side_effect=yaml.YAMLError("bad [red]catalog[/red] yaml"),
4721+
):
4722+
result = runner.invoke(
4723+
app,
4724+
[
4725+
"extension",
4726+
"catalog",
4727+
"add",
4728+
"https://example.com/catalog.json",
4729+
"--name",
4730+
"community",
4731+
],
4732+
catch_exceptions=True,
4733+
)
4734+
4735+
assert result.exit_code == 1, result.output
4736+
assert "bad [red]catalog[/red]" in result.output
4737+
assert "yaml" in result.output
4738+
4739+
def test_catalog_add_escapes_url_validation_exception_markup(self, tmp_path):
4740+
"""URL validation errors may include user-controlled URL text."""
4741+
from typer.testing import CliRunner
4742+
from unittest.mock import patch
4743+
from specify_cli import app
4744+
4745+
project_dir = tmp_path / "test-project"
4746+
project_dir.mkdir()
4747+
(project_dir / ".specify").mkdir()
4748+
4749+
runner = CliRunner()
4750+
with patch.object(Path, "cwd", return_value=project_dir), \
4751+
patch.object(
4752+
ExtensionCatalog,
4753+
"_validate_catalog_url",
4754+
side_effect=ValidationError("bad [red]url[/red]"),
4755+
):
4756+
result = runner.invoke(
4757+
app,
4758+
[
4759+
"extension",
4760+
"catalog",
4761+
"add",
4762+
"https://example.com/[red]catalog[/red].json",
4763+
"--name",
4764+
"community",
4765+
],
4766+
catch_exceptions=True,
4767+
)
4768+
4769+
assert result.exit_code == 1, result.output
4770+
assert "bad [red]url[/red]" in result.output
4771+
47034772
def test_add_dev_links_copilot_agent_when_supported(
47044773
self, extension_dir, project_dir, temp_dir
47054774
):
@@ -5014,6 +5083,79 @@ def test_add_from_url_cancel_exits_cleanly(self, tmp_path):
50145083
assert result.exit_code == 0
50155084
assert "Cancelled" in result.output
50165085

5086+
def test_add_from_url_escapes_download_exception_markup(self, tmp_path):
5087+
"""Download errors can include user-controlled URL text."""
5088+
import urllib.error
5089+
from typer.testing import CliRunner
5090+
from unittest.mock import patch
5091+
from specify_cli import app
5092+
5093+
project_dir = tmp_path / "test-project"
5094+
project_dir.mkdir()
5095+
(project_dir / ".specify").mkdir()
5096+
5097+
runner = CliRunner()
5098+
with patch.object(Path, "cwd", return_value=project_dir), \
5099+
patch("typer.confirm", return_value=True), \
5100+
patch(
5101+
"specify_cli.authentication.http.open_url",
5102+
side_effect=urllib.error.URLError("bad [red]download[/red]"),
5103+
):
5104+
result = runner.invoke(
5105+
app,
5106+
[
5107+
"extension",
5108+
"add",
5109+
"my-ext",
5110+
"--from",
5111+
"https://example.com/[red]ext[/red].zip",
5112+
],
5113+
catch_exceptions=True,
5114+
)
5115+
5116+
assert result.exit_code == 1, result.output
5117+
assert "https://example.com/[red]ext[/red].zip" in result.output
5118+
assert "bad [red]download[/red]" in result.output
5119+
5120+
@pytest.mark.parametrize(
5121+
("exc_type", "label"),
5122+
[
5123+
(ValidationError, "Validation Error"),
5124+
(CompatibilityError, "Compatibility Error"),
5125+
(ExtensionError, "Error"),
5126+
],
5127+
)
5128+
def test_add_exception_handlers_escape_markup(self, tmp_path, exc_type, label):
5129+
"""Extension install exceptions can include manifest-controlled values."""
5130+
from typer.testing import CliRunner
5131+
from unittest.mock import patch
5132+
from specify_cli import app
5133+
5134+
project_dir = tmp_path / "test-project"
5135+
project_dir.mkdir()
5136+
(project_dir / ".specify").mkdir()
5137+
5138+
ext_dir = tmp_path / "ext"
5139+
ext_dir.mkdir()
5140+
(ext_dir / "extension.yml").write_text("extension:\n id: test\n", encoding="utf-8")
5141+
5142+
runner = CliRunner()
5143+
with patch.object(Path, "cwd", return_value=project_dir), \
5144+
patch.object(
5145+
ExtensionManager,
5146+
"install_from_directory",
5147+
side_effect=exc_type("bad [red]extension[/red]"),
5148+
):
5149+
result = runner.invoke(
5150+
app,
5151+
["extension", "add", str(ext_dir), "--dev"],
5152+
catch_exceptions=True,
5153+
)
5154+
5155+
assert result.exit_code == 1, result.output
5156+
assert f"{label}:" in result.output
5157+
assert "bad [red]extension[/red]" in result.output
5158+
50175159
def test_add_from_url_uses_cache_tempfile_for_untrusted_extension_name(self, tmp_path):
50185160
"""The extension argument must not control the downloaded ZIP path."""
50195161
import io

0 commit comments

Comments
 (0)