mirror of
https://github.com/github/spec-kit.git
synced 2026-08-20 10:47:26 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user