diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 4494e15114..65c6cf3ad9 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -428,10 +428,13 @@ def extension_add( try: parsed = urlparse(from_url) + # Keep parsing and hostname extraction in the same guard so any + # ValueError is converted to the CLI's normal invalid-URL error. + hostname = parsed.hostname except ValueError: console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") raise typer.Exit(1) - is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1") + is_localhost = hostname in ("localhost", "127.0.0.1", "::1") if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost): console.print("[red]Error:[/red] URL must use HTTPS for security.") diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index ed8b937600..6dc228e793 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -106,13 +106,28 @@ def preset_add( from urllib.parse import urlparse as _urlparse try: - _parsed = _urlparse(from_url) + # Read .hostname inside the try: a bracketed-but-invalid IPv6 + # authority (e.g. "https://[not-an-ip]/p.zip") parses cleanly + # under urlparse() on Python < 3.14 and only raises ValueError + # lazily on the first .hostname access (eager at urlparse() on + # 3.14+). Guarding it here surfaces the clean "Invalid URL" + # message instead of leaking a raw ValueError traceback. + _urlparse(from_url).hostname except ValueError: console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") raise typer.Exit(1) - def _is_allowed_download_url(parsed_url): - host = parsed_url.hostname + def _is_allowed_download_url(url): + # Parse and read .hostname inside the try: a bracketed-but-invalid + # IPv6 authority (e.g. "https://[not-an-ip]/p.zip") parses cleanly + # under urlparse() on Python < 3.14 and only raises ValueError + # lazily on the first .hostname access (eager at urlparse() on + # 3.14+). A malformed URL is simply not an allowed download URL. + try: + parsed_url = _urlparse(url) + host = parsed_url.hostname + except ValueError: + return False if not host: return False is_loopback = host == "localhost" @@ -125,7 +140,7 @@ def _is_allowed_download_url(parsed_url): return parsed_url.scheme == "https" or (parsed_url.scheme == "http" and is_loopback) def _validate_download_redirect(old_url, new_url): - if not _is_allowed_download_url(_urlparse(new_url)): + if not _is_allowed_download_url(new_url): import urllib.error raise urllib.error.URLError( @@ -133,7 +148,7 @@ def _validate_download_redirect(old_url, new_url): "or HTTP for localhost/loopback" ) - if not _is_allowed_download_url(_parsed): + if not _is_allowed_download_url(from_url): console.print( "[red]Error:[/red] URL must use HTTPS with a hostname, " "or HTTP for localhost/loopback." @@ -167,7 +182,7 @@ def _validate_download_redirect(old_url, new_url): redirect_validator=_validate_download_redirect, ) as response: final_url = response.geturl() if hasattr(response, "geturl") else from_url - if not _is_allowed_download_url(_urlparse(final_url)): + if not _is_allowed_download_url(final_url): console.print( "[red]Error:[/red] Preset URL redirected to a disallowed URL: " f"{final_url}. Redirect targets must use HTTPS with a hostname, " diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 08831c3ae5..cca29413f7 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -6605,6 +6605,79 @@ def test_add_from_malformed_ipv6_url_exits_cleanly(self, tmp_path): plain = strip_ansi(result.output) assert "Invalid URL" in plain + def test_add_from_bracketed_non_ip_url_exits_cleanly(self, tmp_path): + """A bracketed-but-invalid IPv6 host must produce a clean error, not a + ValueError traceback. Unlike an unterminated bracket (which raises eager + at urlparse()), "[not-an-ip]" parses cleanly on Python < 3.14 and only + raises ValueError lazily on the first .hostname access -- the case the + try/except guard around .hostname protects against on CI interpreters. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke( + app, + ["extension", "add", "my-ext", "--from", "https://[not-an-ip]/ext.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + plain = strip_ansi(result.output) + assert "Invalid URL" in plain + + def test_add_from_url_lazy_hostname_valueerror_exits_cleanly(self, tmp_path, monkeypatch): + """Simulate the Python < 3.14 shape explicitly (independent of the running + interpreter): urlparse() succeeds but .hostname raises ValueError lazily. + This is the exact path the fix guards; it leaks a raw ValueError if + .hostname is read outside the try/except. + """ + import urllib.parse + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + real_urlparse = urllib.parse.urlparse + + class _LazyHostnameRaiser: + def __init__(self, parsed): + self._parsed = parsed + + @property + def hostname(self): + raise ValueError("simulated lazy IPv6 hostname failure") + + def __getattr__(self, name): + return getattr(self._parsed, name) + + def _fake_urlparse(url, *args, **kwargs): + return _LazyHostnameRaiser(real_urlparse(url, *args, **kwargs)) + + monkeypatch.setattr(urllib.parse, "urlparse", _fake_urlparse) + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke( + app, + ["extension", "add", "my-ext", "--from", "https://example.com/ext.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Invalid URL" in strip_ansi(result.output) + def test_add_status_escapes_extension_markup(self, tmp_path): """User-controlled extension names must not be parsed as Rich markup.""" from rich.markup import escape as escape_markup diff --git a/tests/test_presets.py b/tests/test_presets.py index aeceaf6a8b..f2cd345e8b 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -5577,6 +5577,76 @@ 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_from_bracketed_non_ip_url_exits_cleanly(self, project_dir): + """A bracketed-but-invalid IPv6 host in --from must exit cleanly. + + Unlike an unterminated bracket (which raises eager at urlparse()), + "[not-an-ip]" parses cleanly on Python < 3.14 and only raises ValueError + lazily on the first .hostname access -- the case the try/except guard + around .hostname protects against on CI interpreters. + """ + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("specify_cli.authentication.http.open_url") as open_url: + result = runner.invoke( + app, + ["preset", "add", "--from", "https://[not-an-ip]/preset.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + output = strip_ansi(result.output) + assert "Invalid URL" in output + open_url.assert_not_called() + + def test_preset_add_from_url_lazy_hostname_valueerror_exits_cleanly(self, project_dir, monkeypatch): + """Simulate the Python < 3.14 shape explicitly (independent of the running + interpreter): urlparse() succeeds but .hostname raises ValueError lazily. + This is the exact path the fix guards; it leaks a raw ValueError if + .hostname is read outside the try/except. + """ + import urllib.parse + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + real_urlparse = urllib.parse.urlparse + + class _LazyHostnameRaiser: + def __init__(self, parsed): + self._parsed = parsed + + @property + def hostname(self): + raise ValueError("simulated lazy IPv6 hostname failure") + + def __getattr__(self, name): + return getattr(self._parsed, name) + + def _fake_urlparse(url, *args, **kwargs): + return _LazyHostnameRaiser(real_urlparse(url, *args, **kwargs)) + + monkeypatch.setattr(urllib.parse, "urlparse", _fake_urlparse) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("specify_cli.authentication.http.open_url") as open_url: + result = runner.invoke( + app, + ["preset", "add", "--from", "https://example.com/preset.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Invalid URL" in strip_ansi(result.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.