From 1d2184d9a2407de1abc7284f6d62434b7f23a8b8 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 9 Aug 2026 11:17:51 +0500 Subject: [PATCH] fix(bundle): escape Rich markup in bundle CLI error and status output `specify bundle`'s `_fail` helper interpolated its message straight into `err_console.print`, which has Rich markup enabled. Every caller passes `str(exc)` from a `BundlerError`, and those messages embed untrusted data -- including the command's own argument -- so a `[...]` in it was parsed as a style tag. Balanced tags were silently swallowed; an unbalanced closer raised `MarkupError`, which replaced the error message with a traceback and left the output completely empty. Three commands crashed on user input alone, with no project state required: specify bundle catalog add 'ssh://ex[/red]ample.com/c.json' specify bundle catalog remove 'no[/red]such' specify bundle update 'no[/red]such' `bundle validate` had the same failure on both branches: its errors echo `requires.speckit_version`, and its warnings echo component ids, which are not charset-validated -- so a structurally *valid* manifest crashed on the success path too. Fixed centrally in `_fail`, plus the remaining raw interpolations: the `validate` warning/error/success lines, the install overlap and plan warnings, the install/update/remove/catalog-add confirmations, the `catalog list` id/url, and the `bundle init` project path. Regression tests cover the four crashing error paths (parametrized) and both `validate` branches; all six fail without this change. Assisted-by: Claude Opus 4.8 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 50 +++++++++++++------- tests/contract/test_bundle_cli.py | 52 +++++++++++++++++++++ 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 10df8aca14..ace3c98979 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -45,7 +45,12 @@ def _fail(message: str) -> None: """Print an actionable error to stderr and exit non-zero.""" # Use the stderr console so the error never lands on stdout, which under # ``--json`` carries the machine-readable payload and must stay parseable. - err_console.print(f"[red]Error:[/red] {message}", style=None) + # Escape the message: every caller passes ``str(exc)`` from a BundlerError + # that interpolates untrusted data (a CLI argument, a catalog url, a + # bundle.yml field), so a '[...]' in it would be parsed as a Rich style tag + # -- silently swallowing the text, or raising MarkupError on an unbalanced + # closer and replacing the whole message with a traceback. + err_console.print(f"[red]Error:[/red] {_escape_markup(message)}", style=None) raise typer.Exit(code=1) @@ -394,13 +399,13 @@ def bundle_install( ) console.print( f"[cyan]No Spec Kit project here; initializing with integration " - f"'{init_integration}'…[/cyan]" + f"'{_escape_markup(str(init_integration))}'…[/cyan]" ) _run_init(init_integration, script_type=_default_script_type(), offline=offline) project_root = require_project_root() for overlap in _bundle_overlaps(project_root, manifest, offline=offline): - console.print(f"[yellow]![/yellow] {overlap}") + console.print(f"[yellow]![/yellow] {_escape_markup(str(overlap))}") # For an already-initialized project, the project's recorded active # integration is authoritative — an explicit --integration must not be @@ -415,7 +420,7 @@ def bundle_install( integration_explicit=bool(integration) and detected is None, ) for warning in plan.warnings: - console.print(f"[yellow]![/yellow] {warning}") + console.print(f"[yellow]![/yellow] {_escape_markup(str(warning))}") result = install_bundle( project_root, @@ -428,7 +433,7 @@ def bundle_install( return console.print( - f"[green]✓[/green] Installed '{result.bundle_id}' " + f"[green]✓[/green] Installed '{_escape_markup(str(result.bundle_id))}' " f"({len(result.installed)} added, {len(result.skipped)} already present)." ) @@ -480,7 +485,10 @@ def bundle_update( integration_explicit=bool(integration) and detected is None, ) install_bundle(project_root, plan, installer, manifest=manifest, refresh=True) - console.print(f"[green]✓[/green] Updated '{target}' to v{plan.version}.") + console.print( + f"[green]✓[/green] Updated '{_escape_markup(str(target))}' " + f"to v{_escape_markup(str(plan.version))}." + ) except BundlerError as exc: _fail(str(exc)) return @@ -502,7 +510,7 @@ def bundle_remove( return console.print( - f"[green]✓[/green] Removed '{result.bundle_id}' " + f"[green]✓[/green] Removed '{_escape_markup(str(result.bundle_id))}' " f"({len(result.uninstalled)} uninstalled, {len(result.skipped)} kept for other bundles)." ) @@ -542,13 +550,16 @@ def bundle_validate( return for warning in report.warnings: - console.print(f"[yellow]![/yellow] {warning}") + console.print(f"[yellow]![/yellow] {_escape_markup(str(warning))}") if not report.ok: console.print("[red]Manifest is invalid:[/red]") for error in report.errors: - console.print(f" [red]-[/red] {error}") + console.print(f" [red]-[/red] {_escape_markup(str(error))}") raise typer.Exit(code=1) - console.print(f"[green]✓[/green] {manifest.bundle.id} is well-formed and valid.") + console.print( + f"[green]✓[/green] {_escape_markup(str(manifest.bundle.id))} " + "is well-formed and valid." + ) @bundle_app.command("build") @@ -591,7 +602,7 @@ def bundle_init( init_integration = _resolve_init_integration(integration, None) console.print( f"[cyan]Initializing a Spec Kit project with integration " - f"'{init_integration}'…[/cyan]" + f"'{_escape_markup(str(init_integration))}'…[/cyan]" ) _run_init(init_integration, script_type=_default_script_type(), offline=offline) project_root = require_project_root() @@ -599,7 +610,10 @@ def bundle_init( _fail(str(exc)) return - console.print(f"[green]✓[/green] Spec Kit project ready at {project_root}.") + console.print( + f"[green]✓[/green] Spec Kit project ready at " + f"{_escape_markup(str(project_root))}." + ) if bundle: bundle_install(bundle, integration=integration, offline=offline) @@ -623,10 +637,11 @@ def catalog_list() -> None: only_builtin = all(s.scope == Scope.BUILTIN for s in sources) for source in sources: console.print( - f" [bold]{source.id}[/bold] priority={source.priority} " + f" [bold]{_escape_markup(str(source.id))}[/bold] " + f"priority={source.priority} " f"policy={source.install_policy.value} scope={source.scope.value}" ) - console.print(f" [dim]{source.url}[/dim]") + console.print(f" [dim]{_escape_markup(str(source.url))}[/dim]") if only_builtin: console.print("\n[dim]Using the built-in default stack.[/dim]") @@ -651,7 +666,7 @@ def catalog_add( return console.print( - f"[green]✓[/green] Added catalog '{source.id}' " + f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " f"(priority {source.priority}, {source.install_policy.value})." ) @@ -670,7 +685,10 @@ def catalog_remove( _fail(str(exc)) return - console.print(f"[green]✓[/green] Removed catalog source '{removed}'.") + console.print( + f"[green]✓[/green] Removed catalog source " + f"'{_escape_markup(str(removed))}'." + ) # ZIP magic-byte signatures used to detect .zip payloads from REST API asset diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 830a22c5dc..147a227f11 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -217,6 +217,31 @@ def test_catalog_remove_builtin_is_refused(project: Path): assert "built-in" in result.output +# Every ``bundle`` error path funnels through ``_fail(str(exc))``, and the +# BundlerError messages interpolate untrusted data -- including the command's +# own argument. An unbalanced closer used to raise MarkupError instead of the +# error, leaving the user with a traceback and no message at all. +@pytest.mark.parametrize( + "argv, expected", + [ + ( + ["bundle", "catalog", "add", "ssh://ex[/red]ample.com/c.json"], + "ssh://ex[/red]ample.com/c.json", + ), + (["bundle", "catalog", "remove", "no[/red]such"], "no[/red]such"), + (["bundle", "update", "no[/red]such"], "no[/red]such"), + (["bundle", "remove", "no[/red]such"], "no[/red]such"), + ], +) +def test_error_paths_escape_rich_markup(project: Path, argv: list, expected: str): + result = runner.invoke(app, argv) + + assert result.exit_code == 1 + # A MarkupError would surface here as an exception rather than a clean exit. + assert isinstance(result.exception, SystemExit) + assert expected in strip_ansi(result.output) + + def test_validate_reports_invalid_manifest(project: Path): data = valid_manifest_dict() del data["bundle"]["license"] @@ -237,6 +262,33 @@ def test_validate_accepts_valid_manifest(project: Path): assert "valid" in result.output +def test_validate_escapes_manifest_markup_in_errors(project: Path): + data = valid_manifest_dict() + # An invalid constraint is echoed back inside the validation error. + data["requires"] = {"speckit_version": ">=1.0[/bold]"} + (project / "bundle.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + + result = runner.invoke(app, ["bundle", "validate", "--offline"]) + + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert ">=1.0[/bold]" in strip_ansi(result.output) + + +def test_validate_escapes_manifest_markup_in_warnings(project: Path): + data = valid_manifest_dict() + # Step ids are not charset-validated, and the unresolved-reference warning + # echoes them -- so an otherwise *valid* manifest crashed just as readily as + # an invalid one, on the success path. + data["provides"]["steps"] = [{"id": "step[/bold]a"}] + (project / "bundle.yml").write_text(yaml.safe_dump(data), encoding="utf-8") + + result = runner.invoke(app, ["bundle", "validate", "--offline"]) + + assert result.exit_code == 0, repr(result.exception) + assert "step[/bold]a" in strip_ansi(result.output) + + def test_validate_rejects_broken_reference(project: Path): # Synthetic component ids resolve to nothing in any catalog → hard failure. (project / "bundle.yml").write_text(