From 1d2184d9a2407de1abc7284f6d62434b7f23a8b8 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 9 Aug 2026 11:17:51 +0500 Subject: [PATCH 1/2] 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( From 3e1f598a8ab71e56327565c473cb4efb5f73f3c9 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Mon, 10 Aug 2026 21:43:12 +0500 Subject: [PATCH 2/2] fix(bundle): escape markup in `bundle list` records and `bundle build` output path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: two raw interpolations the first sweep missed, both on success paths rather than error paths. `bundle_list` rendered `record.bundle_id`, `record.version` and `record.installed_at` unescaped. `InstalledBundleRecord.from_dict` only requires non-empty strings for the first two and applies no charset check to any of them, so a records file that *loads cleanly* still crashed the command that displays it — confirmed as `MarkupError: closing tag '[/red]' at position 12 doesn't match any open tag`. `bundle_build` echoed `result.artifact_path` twice in its success line. Brackets are legal in a directory name, so a bracketed `--output` built the artifact and then misreported it: the work is already on disk when the markup is consumed, so the line names a path that does not exist. Re-scanned every `{...}` interpolation in the module to confirm nothing else remains: the rest are either `BundlerError` messages that funnel through the already-escaped `_fail`, `_format_component` output escaped at its call site (:293), ints, or hardcoded enum `.value`s. Two regression tests. The list case uses the unbalanced-closer form that raises outright. The build case deliberately uses `[bold]` instead: `/` is a path separator on Windows, so `dist[/red]out` becomes the directory `dist[\red]out` and the fixture stops testing what it claims — the silent-swallow form keeps it portable while still asserting the reported path matches what was written. Verified both fail against 1d2184d. tests/contract/test_bundle_cli.py -> 42 passed. tests/contract tests/integration tests/unit -> 364 passed, 6 skipped, 5 failed; the 5 are the pre-existing `*_refuses_symlinked_*` tests needing symlink privileges on Windows, unchanged from main. ruff check passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 10 ++-- tests/contract/test_bundle_cli.py | 59 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index ace3c98979..7ccc6cba31 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -337,9 +337,10 @@ def bundle_list( console.print("\n[bold cyan]Installed bundles:[/bold cyan]\n") for record in records: console.print( - f" [bold]{record.bundle_id}[/bold] v{record.version} " + f" [bold]{_escape_markup(str(record.bundle_id))}[/bold] " + f"v{_escape_markup(str(record.version))} " f"[dim]({len(record.contributed_components)} components, " - f"installed {record.installed_at})[/dim]" + f"installed {_escape_markup(str(record.installed_at))})[/dim]" ) @@ -582,8 +583,9 @@ def bundle_build( return console.print( - f"[green]✓[/green] Built {result.artifact_path.name} " - f"({result.file_count} files) → {result.artifact_path}" + f"[green]✓[/green] Built {_escape_markup(result.artifact_path.name)} " + f"({result.file_count} files) → " + f"{_escape_markup(str(result.artifact_path))}" ) diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 147a227f11..bed2f8964d 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -319,6 +319,65 @@ def test_build_produces_artifact(project: Path): assert len(artifacts) == 1 +def test_build_escapes_markup_in_output_path(project: Path): + """The build success line echoes a caller-supplied ``--output`` path. + + Brackets are legal in a directory name on both POSIX and Windows, so the + artifact is built and *then* misreported: ``[bold]`` is consumed as a style + tag, and the success line names a path that does not exist on disk. + + A closing tag (``[/red]``) would raise MarkupError outright, but ``/`` is a + path separator on Windows, so this uses the silent-swallow form to keep the + fixture portable. + """ + (project / "bundle.yml").write_text( + yaml.safe_dump(valid_manifest_dict()), encoding="utf-8" + ) + (project / "README.md").write_text("# Demo", encoding="utf-8") + out_dir = project / "dist[bold]out" + + result = runner.invoke(app, ["bundle", "build", "--output", str(out_dir)]) + + assert result.exit_code == 0, repr(result.exception) + assert list(out_dir.glob("*.zip")), "the artifact should still be built" + assert "dist[bold]out" in strip_ansi(result.output), ( + "the reported path must match the directory actually written" + ) + + +def test_list_escapes_markup_in_records(project: Path): + """``bundle list`` renders record fields that are never charset-validated. + + ``InstalledBundleRecord.from_dict`` accepts any non-empty string for + ``bundle_id``/``version`` and any string for ``installed_at``, so a records + file that *loads cleanly* could still crash the command that displays it. + """ + (project / ".specify" / "bundle-records.json").write_text( + json.dumps( + { + "schema_version": "1.0", + "bundles": [ + { + "bundle_id": "demo[/red]id", + "version": "1.0.0[/bold]", + "installed_at": "2026-01-01T00:00:00Z[/dim]", + "contributed_components": [], + } + ], + } + ), + encoding="utf-8", + ) + + result = runner.invoke(app, ["bundle", "list"]) + + assert result.exit_code == 0, repr(result.exception) + output = strip_ansi(result.output) + assert "demo[/red]id" in output + assert "1.0.0[/bold]" in output + assert "2026-01-01T00:00:00Z[/dim]" in output + + def _mock_manifest_download(monkeypatch, source_path: Path) -> None: """Mock the HTTPS manifest fetch to return a locally-authored manifest.