diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 05aa35f7fb..fec109bd15 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2631,8 +2631,20 @@ def download_extension( # Validate download URL requires HTTPS (prevent man-in-the-middle attacks) from urllib.parse import urlparse - parsed = urlparse(download_url) - is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1") + # A malformed authority (e.g. an unterminated IPv6 bracket + # "https://[::1") makes urlparse / hostname access raise ValueError. + # The download_url comes from catalog payload data, so surface a clean + # ExtensionError rather than leaking a raw ValueError past the command + # handler (which only catches ExtensionError). Mirrors catalogs (#3435) + # and workflows/catalog.py (#3484). + try: + parsed = urlparse(download_url) + hostname = parsed.hostname + except ValueError: + raise ExtensionError( + f"Extension download URL is malformed: {download_url}" + ) from None + is_localhost = hostname in ("localhost", "127.0.0.1", "::1") if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost): raise ExtensionError( f"Extension download URL must use HTTPS: {download_url}" diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 98bba08fd4..a5943de5ad 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2713,8 +2713,20 @@ def download_pack( from urllib.parse import urlparse - parsed = urlparse(download_url) - is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1") + # A malformed authority (e.g. an unterminated IPv6 bracket + # "https://[::1") makes urlparse / hostname access raise ValueError. + # The download_url comes from catalog payload data, so surface a clean + # PresetError rather than leaking a raw ValueError past the command + # handler (which only catches PresetError). Mirrors catalogs (#3435) + # and workflows/catalog.py (#3484). + try: + parsed = urlparse(download_url) + hostname = parsed.hostname + except ValueError: + raise PresetError( + f"Preset download URL is malformed: {download_url}" + ) from None + is_localhost = hostname in ("localhost", "127.0.0.1", "::1") if parsed.scheme != "https" and not ( parsed.scheme == "http" and is_localhost ): diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 44402831e8..ed8b937600 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -13,6 +13,7 @@ import typer import yaml +from rich.markup import escape as _escape_markup from .._console import console @@ -107,8 +108,6 @@ def preset_add( try: _parsed = _urlparse(from_url) except ValueError: - from rich.markup import escape as _escape_markup - console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") raise typer.Exit(1) @@ -141,9 +140,7 @@ def _validate_download_redirect(old_url, new_url): ) raise typer.Exit(1) - from rich.markup import escape as _esc - - console.print(f"Installing preset from [cyan]{_esc(from_url)}[/cyan]...") + console.print(f"Installing preset from [cyan]{_escape_markup(from_url)}[/cyan]...") import urllib.error import tempfile import shutil @@ -183,7 +180,7 @@ def _validate_download_redirect(old_url, new_url): except TypeError: output.write(response.read()) except urllib.error.URLError as e: - console.print(f"[red]Error:[/red] Failed to download: {e}") + console.print(f"[red]Error:[/red] Failed to download: {_escape_markup(str(e))}") raise typer.Exit(1) manifest = manager.install_from_zip(zip_path, speckit_version, priority) @@ -240,13 +237,13 @@ def _validate_download_redirect(old_url, new_url): raise typer.Exit(1) except PresetCompatibilityError as e: - console.print(f"[red]Compatibility Error:[/red] {e}") + console.print(f"[red]Compatibility Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) except PresetValidationError as e: - console.print(f"[red]Validation Error:[/red] {e}") + console.print(f"[red]Validation Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) except PresetError as e: - console.print(f"[red]Error:[/red] {e}") + console.print(f"[red]Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) @@ -288,7 +285,7 @@ def preset_search( try: results = catalog.search(query=query, tag=tag, author=author) except PresetError as e: - console.print(f"[red]Error:[/red] {e}") + console.print(f"[red]Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) if not results: @@ -582,7 +579,7 @@ def preset_catalog_list(): try: active_catalogs = catalog.get_active_catalogs() except PresetValidationError as e: - console.print(f"[red]Error:[/red] {e}") + console.print(f"[red]Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) console.print("\n[bold cyan]Active Preset Catalogs:[/bold cyan]\n") @@ -647,7 +644,7 @@ def preset_catalog_add( try: tmp_catalog._validate_catalog_url(url) except PresetValidationError as e: - console.print(f"[red]Error:[/red] {e}") + console.print(f"[red]Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) config_path = specify_dir / "preset-catalogs.yml" @@ -658,7 +655,7 @@ def preset_catalog_add( config = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} except Exception as e: config_label = _display_project_path(project_root, config_path) - console.print(f"[red]Error:[/red] Failed to read {config_label}: {e}") + console.print(f"[red]Error:[/red] Failed to read {_escape_markup(str(config_label))}: {_escape_markup(str(e))}") raise typer.Exit(1) else: config = {} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index d2f4af8264..7faf3eaec0 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -4195,6 +4195,26 @@ def test_download_extension_rejects_sha256_mismatch(self, temp_dir): with pytest.raises(ExtensionError, match="[Ii]ntegrity"): catalog.download_extension("test-ext", target_dir=temp_dir) + def test_download_extension_malformed_url_raises_extension_error(self, temp_dir): + """A catalog ``download_url`` with a malformed authority (e.g. an + unterminated IPv6 bracket) surfaces a clean ``ExtensionError`` rather + than leaking a raw ``ValueError`` from ``urlparse``/``.hostname`` past + the command handler (which only catches ``ExtensionError``). + """ + from unittest.mock import patch + + catalog = self._make_catalog(temp_dir) + for bad_url in ("https://[::1", "https://[not-an-ip]/x"): + ext_info = { + "id": "test-ext", + "name": "Test Extension", + "version": "1.0.0", + "download_url": bad_url, + } + with patch.object(catalog, "get_extension_info", return_value=ext_info): + with pytest.raises(ExtensionError, match="malformed"): + catalog.download_extension("test-ext", target_dir=temp_dir) + def test_download_extension_without_sha256_still_succeeds(self, temp_dir): """Entries without ``sha256`` keep working (backwards compatible).""" from unittest.mock import patch diff --git a/tests/test_presets.py b/tests/test_presets.py index 797835a88c..793fa30418 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -2291,6 +2291,28 @@ def test_download_pack_rejects_sha256_mismatch(self, project_dir): with pytest.raises(PresetError, match="[Ii]ntegrity"): catalog.download_pack("test-pack", target_dir=project_dir) + def test_download_pack_malformed_url_raises_preset_error(self, project_dir): + """A catalog ``download_url`` with a malformed authority (e.g. an + unterminated IPv6 bracket) surfaces a clean ``PresetError`` rather than + leaking a raw ``ValueError`` from ``urlparse``/``.hostname`` past the + command handler (which only catches ``PresetError``). Mirrors the + extensions coverage. + """ + from unittest.mock import patch + + catalog = PresetCatalog(project_dir) + for bad_url in ("https://[::1", "https://[not-an-ip]/x"): + pack_info = { + "id": "test-pack", + "name": "Test Pack", + "version": "1.0.0", + "download_url": bad_url, + "_install_allowed": True, + } + with patch.object(catalog, "get_pack_info", return_value=pack_info): + with pytest.raises(PresetError, match="malformed"): + catalog.download_pack("test-pack", target_dir=project_dir) + def test_download_pack_without_sha256_skips_verification(self, project_dir): """A catalog entry with no ``sha256`` keeps working: verification is opt-in, so the backwards-compatible path (``pack_info.get("sha256")`` @@ -5418,6 +5440,82 @@ def test_preset_add_from_malformed_ipv6_url_exits_cleanly(self, project_dir): assert "Invalid URL" in output open_url.assert_not_called() + def test_preset_add_bracketed_host_download_url_exits_cleanly(self, project_dir): + """A catalog download_url with a bracketed non-IP host must render cleanly. + + ``download_pack`` raises ``PresetError`` whose message embeds the raw URL + (e.g. ``https://[not-an-ip]/x``). The ``preset_add`` handler must escape + that message before printing so Rich does not interpret ``[not-an-ip]`` + as a markup tag and crash while rendering the error. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + bad_url = "https://[not-an-ip]/x" + catalog_data = { + "test-pack": { + "name": "Test Pack", + "version": "1.0.0", + "download_url": bad_url, + } + } + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object(PresetCatalog, "_get_merged_packs", return_value=catalog_data): + result = runner.invoke( + app, + ["preset", "add", "test-pack"], + catch_exceptions=True, + ) + + assert result.exit_code == 1, result.output + assert result.exception is None or isinstance(result.exception, SystemExit) + output = strip_ansi(result.output) + assert "Error:" in output + # The malformed URL surfaces verbatim rather than crashing the renderer. + assert bad_url in output + + @pytest.mark.parametrize( + ("exc_type", "label"), + [ + (PresetCompatibilityError, "Compatibility Error"), + (PresetValidationError, "Validation Error"), + (PresetError, "Error"), + ], + ) + def test_preset_add_exception_handlers_escape_markup(self, project_dir, exc_type, label): + """Preset install exceptions can include catalog-controlled values. + + The message must be escaped so Rich does not treat bracketed content as + markup and raise while rendering the error. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + dev_dir = project_dir / "dev-pack" + dev_dir.mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object( + PresetManager, + "install_from_directory", + side_effect=exc_type("bad [red]preset[/red]"), + ): + result = runner.invoke( + app, + ["preset", "add", "--dev", str(dev_dir)], + catch_exceptions=True, + ) + + assert result.exit_code == 1, result.output + assert result.exception is None or isinstance(result.exception, SystemExit) + assert f"{label}:" in result.output + assert "bad [red]preset[/red]" in result.output + def test_preset_add_from_url_redirect_error_describes_disallowed_url(self, project_dir, monkeypatch, capsys): """Redirect rejection message covers hostless HTTPS, not only non-HTTPS URLs.""" import typer