fix(cli): guard lazy .hostname ValueError in extension/preset add --from (#3651)

* fix(cli): guard lazy .hostname ValueError in extension/preset add --from

`extension add --from <url>` and `preset add --from <url>` 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) <noreply@anthropic.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix(cli): address Copilot review on --from URL guard comments/tests

Copilot's review on #3651 flagged two accuracy problems:

1. The guard comments asserted a specific (and incorrect) CPython version
   history -- that "https://[not-an-ip]/..." parses cleanly under urlparse()
   on Python < 3.14 and only raises ValueError lazily on the first .hostname
   access. In fact the eager bracketed-host check (gh-103848, CVE-2024-11168)
   was backported to the 3.11 branch and shipped in 3.11.4, so on every
   interpreter spec-kit supports (>=3.11) that URL is rejected eagerly at
   urlparse(). Reworded the three source comments to state the guard as a
   defensive policy (parsing OR the .hostname read can raise ValueError, guard
   both) without asserting version history.

2. The two monkeypatched lazy-.hostname tests were described as reproducing
   "the exact production path" / "the Python < 3.14 shape". They are synthetic
   defensive cases. Relabeled them as synthetic defensive coverage that does
   not reproduce any specific CPython behavior, and dropped the version-history
   claims from the bracketed-non-IP test docstrings.

The second-round suggestion (_is_allowed_download_url(final_url) instead of
_is_allowed_download_url(_urlparse(final_url))) was already applied in the
original commit.

Behavior unchanged; comments/docstrings only. URL-guard tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: 3 people

src/specify_cli/extensions/_commands.py

@@ -428,10 +428,17 @@ def extension_add(
 
         try:
             parsed = urlparse(from_url)
+            # Read .hostname inside the try: parsing a malformed authority -- or
+            # accessing .hostname on one, e.g. an invalid bracketed IPv6 host like
+            # "https://[not-an-ip]/x.zip" -- can raise ValueError. Keeping both the
+            # parse and the .hostname read inside the guard surfaces a clean
+            # "Invalid URL" message 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.")

tests/test_extensions.py

@@ -6607,6 +6607,80 @@ 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. "https://[not-an-ip]/ext.zip" is a malformed
+        authority that raises ValueError during URL validation; the try/except
+        guard around parsing and the .hostname read must turn that into a clean
+        "Invalid URL" message.
+        """
+        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):
+        """Synthetic defensive coverage: monkeypatch urlparse() to return an
+        object whose .hostname raises ValueError lazily. This does not reproduce
+        any specific CPython behavior -- it just exercises the case where the
+        ValueError surfaces on the .hostname read rather than at parse time, so a
+        raw ValueError would leak if .hostname were 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

tests/test_presets.py

@@ -5605,6 +5605,58 @@ 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.
+
+        "https://[not-an-ip]/preset.zip" is a malformed authority that raises
+        ValueError during URL validation; the try/except guard around parsing
+        and the .hostname read must turn that into a clean "Invalid URL" message.
+        """
+        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_out_of_range_port_exits_cleanly(self, project_dir):
+        """An out-of-range port raises ValueError lazily on .port access.
+
+        The up-front guard reads ``_parsed.port`` (urllib validates the port
+        range/syntax there) inside its try/except, so "https://example.com:99999/
+        preset.zip" must produce a clean "Invalid URL" message rather than
+        leaking a raw ValueError traceback past the CLI.
+        """
+        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://example.com:99999/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.