Skip to content

Commit fec5a68

Browse files
Noor-ul-ain001claude
Noor-ul-ain001
and
claude
committed
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) <[email protected]>
1 parent fde8985 commit fec5a68

4 files changed

Lines changed: 32 additions & 29 deletions

File tree

‎src/specify_cli/extensions/_commands.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -428,8 +428,12 @@ def extension_add(
428428

429429
try:
430430
parsed = urlparse(from_url)
431-
# Keep parsing and hostname extraction in the same guard so any
432-
# ValueError is converted to the CLI's normal invalid-URL error.
431+
# Read .hostname inside the try: parsing a malformed authority -- or
432+
# accessing .hostname on one, e.g. an invalid bracketed IPv6 host like
433+
# "https://[not-an-ip]/x.zip" -- can raise ValueError. Keeping both the
434+
# parse and the .hostname read inside the guard surfaces a clean
435+
# "Invalid URL" message instead of leaking a raw traceback past the
436+
# CLI. Reuse the value below.
433437
hostname = parsed.hostname
434438
except ValueError:
435439
console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}")

‎src/specify_cli/presets/_commands.py‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -106,23 +106,21 @@ def preset_add(
106106
from urllib.parse import urlparse as _urlparse
107107

108108
try:
109-
# Read .hostname inside the try: a bracketed-but-invalid IPv6
110-
# authority (e.g. "https://[not-an-ip]/p.zip") parses cleanly
111-
# under urlparse() on Python < 3.14 and only raises ValueError
112-
# lazily on the first .hostname access (eager at urlparse() on
113-
# 3.14+). Guarding it here surfaces the clean "Invalid URL"
114-
# message instead of leaking a raw ValueError traceback.
109+
# Read .hostname inside the try: parsing a malformed authority --
110+
# or accessing .hostname on one, e.g. an invalid bracketed IPv6
111+
# host like "https://[not-an-ip]/p.zip" -- can raise ValueError.
112+
# Guarding both here surfaces the clean "Invalid URL" message
113+
# instead of leaking a raw ValueError traceback.
115114
_urlparse(from_url).hostname
116115
except ValueError:
117116
console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}")
118117
raise typer.Exit(1)
119118

120119
def _is_allowed_download_url(url):
121-
# Parse and read .hostname inside the try: a bracketed-but-invalid
122-
# IPv6 authority (e.g. "https://[not-an-ip]/p.zip") parses cleanly
123-
# under urlparse() on Python < 3.14 and only raises ValueError
124-
# lazily on the first .hostname access (eager at urlparse() on
125-
# 3.14+). A malformed URL is simply not an allowed download URL.
120+
# Parse and read .hostname inside the try: parsing a malformed
121+
# authority -- or accessing .hostname on one, e.g. an invalid
122+
# bracketed IPv6 host like "https://[not-an-ip]/p.zip" -- can raise
123+
# ValueError. A malformed URL is simply not an allowed download URL.
126124
try:
127125
parsed_url = _urlparse(url)
128126
host = parsed_url.hostname

‎tests/test_extensions.py‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6607,10 +6607,10 @@ def test_add_from_malformed_ipv6_url_exits_cleanly(self, tmp_path):
66076607

66086608
def test_add_from_bracketed_non_ip_url_exits_cleanly(self, tmp_path):
66096609
"""A bracketed-but-invalid IPv6 host must produce a clean error, not a
6610-
ValueError traceback. Unlike an unterminated bracket (which raises eager
6611-
at urlparse()), "[not-an-ip]" parses cleanly on Python < 3.14 and only
6612-
raises ValueError lazily on the first .hostname access -- the case the
6613-
try/except guard around .hostname protects against on CI interpreters.
6610+
ValueError traceback. "https://[not-an-ip]/ext.zip" is a malformed
6611+
authority that raises ValueError during URL validation; the try/except
6612+
guard around parsing and the .hostname read must turn that into a clean
6613+
"Invalid URL" message.
66146614
"""
66156615
from typer.testing import CliRunner
66166616
from unittest.mock import patch
@@ -6634,10 +6634,11 @@ def test_add_from_bracketed_non_ip_url_exits_cleanly(self, tmp_path):
66346634
assert "Invalid URL" in plain
66356635

66366636
def test_add_from_url_lazy_hostname_valueerror_exits_cleanly(self, tmp_path, monkeypatch):
6637-
"""Simulate the Python < 3.14 shape explicitly (independent of the running
6638-
interpreter): urlparse() succeeds but .hostname raises ValueError lazily.
6639-
This is the exact path the fix guards; it leaks a raw ValueError if
6640-
.hostname is read outside the try/except.
6637+
"""Synthetic defensive coverage: monkeypatch urlparse() to return an
6638+
object whose .hostname raises ValueError lazily. This does not reproduce
6639+
any specific CPython behavior -- it just exercises the case where the
6640+
ValueError surfaces on the .hostname read rather than at parse time, so a
6641+
raw ValueError would leak if .hostname were read outside the try/except.
66416642
"""
66426643
import urllib.parse
66436644
from typer.testing import CliRunner

‎tests/test_presets.py‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5580,10 +5580,9 @@ def test_preset_add_from_malformed_ipv6_url_exits_cleanly(self, project_dir):
55805580
def test_preset_add_from_bracketed_non_ip_url_exits_cleanly(self, project_dir):
55815581
"""A bracketed-but-invalid IPv6 host in --from must exit cleanly.
55825582
5583-
Unlike an unterminated bracket (which raises eager at urlparse()),
5584-
"[not-an-ip]" parses cleanly on Python < 3.14 and only raises ValueError
5585-
lazily on the first .hostname access -- the case the try/except guard
5586-
around .hostname protects against on CI interpreters.
5583+
"https://[not-an-ip]/preset.zip" is a malformed authority that raises
5584+
ValueError during URL validation; the try/except guard around parsing
5585+
and the .hostname read must turn that into a clean "Invalid URL" message.
55875586
"""
55885587
from typer.testing import CliRunner
55895588
from unittest.mock import patch
@@ -5605,10 +5604,11 @@ def test_preset_add_from_bracketed_non_ip_url_exits_cleanly(self, project_dir):
56055604
open_url.assert_not_called()
56065605

56075606
def test_preset_add_from_url_lazy_hostname_valueerror_exits_cleanly(self, project_dir, monkeypatch):
5608-
"""Simulate the Python < 3.14 shape explicitly (independent of the running
5609-
interpreter): urlparse() succeeds but .hostname raises ValueError lazily.
5610-
This is the exact path the fix guards; it leaks a raw ValueError if
5611-
.hostname is read outside the try/except.
5607+
"""Synthetic defensive coverage: monkeypatch urlparse() to return an
5608+
object whose .hostname raises ValueError lazily. This does not reproduce
5609+
any specific CPython behavior -- it just exercises the case where the
5610+
ValueError surfaces on the .hostname read rather than at parse time, so a
5611+
raw ValueError would leak if .hostname were read outside the try/except.
56125612
"""
56135613
import urllib.parse
56145614
from typer.testing import CliRunner

0 commit comments

Comments
 (0)