From 38e7ffd190d90494a46213b0e6923b48ac6b0c8c Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 22 Jul 2026 20:45:24 +0500 Subject: [PATCH 1/2] fix(cli): guard lazy .hostname ValueError in extension/preset add --from `extension add --from ` and `preset add --from ` validated the URL by reading `parsed.hostname` OUTSIDE their `try/except ValueError` guards. A bracketed-but-invalid IPv6 authority (e.g. "https://[not-an-ip]/x.zip") parses cleanly under urlparse() on Python < 3.14 and only raises ValueError lazily on the first .hostname access. On the interpreters spec-kit supports (>=3.11) that raw ValueError leaked past the CLI, printing an uncaught traceback instead of the clean "Invalid URL" error. (The raise moved eager into urlparse() only in 3.14.) Same bug class as the catalog/download fixes #3433/#3435/#3437/#3577. - extensions/_commands.py: read parsed.hostname inside the existing try and reuse it for the localhost check. - presets/_commands.py: guard the up-front `urlparse(from_url).hostname` read (preserves the "Invalid URL" message), and harden the nested `_is_allowed_download_url` to take a URL string and parse+read .hostname inside its own try/except -> returns False on malformed input. This also covers the redirect-validator and final-URL (post-redirect) checks, where the URL is server-controlled. Regression tests for each command: a bracketed-non-IP URL, plus a monkeypatched lazy-.hostname raiser that reproduces the pre-3.14 shape independently of the running interpreter (fails with a raw ValueError before the fix, verified via test-the-test). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/_commands.py | 9 ++- src/specify_cli/presets/_commands.py | 27 +++++++-- tests/test_extensions.py | 73 +++++++++++++++++++++++++ tests/test_presets.py | 70 ++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 4494e15114..f54589defd 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -428,10 +428,17 @@ def extension_add( try: parsed = urlparse(from_url) + # Read .hostname inside the try: a bracketed-but-invalid IPv6 + # authority (e.g. "https://[not-an-ip]/x.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+). Reading + # it here keeps that ValueError inside the guard instead of leaking a + # raw traceback past the CLI. Reuse the value below. + 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. From fde898511f9455d8e2178a368fa16326bfee324f Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Wed, 22 Jul 2026 22:49:52 +0500 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/_commands.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index f54589defd..65c6cf3ad9 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -428,12 +428,8 @@ def extension_add( try: parsed = urlparse(from_url) - # Read .hostname inside the try: a bracketed-but-invalid IPv6 - # authority (e.g. "https://[not-an-ip]/x.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+). Reading - # it here keeps that ValueError inside the guard instead of leaking a - # raw traceback past the CLI. Reuse the value below. + # 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)}")