fix(init): report a malformed --extension URL cleanly, not raw urllib text - #4324
fix(init): report a malformed --extension URL cleanly, not raw urllib text#4324Noor-ul-ain001 wants to merge 3 commits into
Conversation
… text
`_install_extension_during_init` (src/specify_cli/commands/init.py) parses
an --extension URL spec with a bare `urlparse(ext_spec)`. An unterminated
or invalid bracketed IPv6 authority (e.g. "https://[not-an-ip]/x.zip")
makes urlparse itself raise ValueError — this became eager in Python 3.14
(previously lazy, raised only on `.hostname` access). Since the call was
unguarded, `specify init --extension <bad-url>` reported the raw urllib
message ("'not-an-ip' does not appear to be an IPv4 or IPv6 address")
instead of an actionable error.
Every sibling URL entry point in this codebase already guards this exact
case with a clean domain error: extensions/__init__.py, presets/__init__.py,
extensions/_commands.py, workflows/catalog.py (the github#3435/github#3484 lineage).
init.py's own `_ext_spec_is_url` classifier next to this function already
catches the same ValueError; this call site was the outlier.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt
There was a problem hiding this comment.
🟡 Changes recommended
Lazy hostname validation remains unguarded, making behavior interpreter-dependent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds clean handling for malformed extension URLs during project initialization.
Changes:
- Converts URL parsing failures into an actionable
ValueError. - Adds regression coverage for malformed bracketed hosts.
File summaries
| File | Description |
|---|---|
src/specify_cli/commands/init.py |
Guards extension URL parsing errors. |
tests/test_init_output_markup.py |
Tests malformed URL reporting. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
URL validation now misclassifies some valid absolute Windows paths as malformed URLs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| # extension/preset/workflow entry point already has (#3435 lineage). | ||
| try: | ||
| parsed = urlparse(ext_spec) | ||
| _ = parsed.hostname |
There was a problem hiding this comment.
Fixed. Moved the local-path check before URL parsing entirely, rather than trying to special-case bracketed authorities: _install_extension_during_init now only calls urlparse()/probes .hostname/.port once the spec has already failed the local-path test. C://[my-ext] (and any other absolute/relative-path-shaped spec) never reaches urlparse at all, so it can't be misreported as a malformed URL on any interpreter -- 3.14's eager parse-time raise included, which is actually where this specific repro fires (not just the lazy .hostname path). Added a regression test using the same drive-letter+bracket construction, asserting the failure is "Directory not found" (local-path branch) not "Malformed extension URL". Confirmed via test-the-test that it fails against the prior commit and passes now (ad762fe).
|
Thanks, and appreciated the disclosure + test. Copilot flagged a cross-version gap worth resolving: the guard only catches the eager |
The URL guard added .hostname/.port probing to catch Python 3.11-3.13's lazy authority validation, but it ran unconditionally before the local-path check. On Windows, a spec like "C://[my-ext]" parses with urlparse() as scheme "c" and netloc "[my-ext]" -- a bracketed authority that fails IPv6-literal validation. On Python 3.14 that failure fires from urlparse() itself, so a valid absolute local path was misreported as "Malformed extension URL" before the local-path branch ever ran. Move the local-path check first so urlparse() only ever sees specs that aren't already local paths. Also broadens the guard's own regression test with a monkeypatched-urlparse case covering the lazy (3.11-3.13) .hostname failure on any interpreter, plus a case proving a bracketed Windows-style local path is handled locally, not misclassified as a URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6
|
@mnriem Both gaps handled:
Pushed as ad762fe. Re-requesting review. |
Summary
_install_extension_during_init(src/specify_cli/commands/init.py) parses an--extensionURL spec with a bareparsed = urlparse(ext_spec).https://[not-an-ip]/x.zip) makesurlparse()itself raiseValueError— this became eager in Python 3.14 (previously lazy, raised only on.hostnameaccess). Since the call was unguarded,specify init --extension <bad-url>surfaced the raw urllib message ('not-an-ip' does not appear to be an IPv4 or IPv6 address) as the tracker's failure text instead of an actionable error.extensions/__init__.py,presets/__init__.py,extensions/_commands.py,workflows/catalog.py(the#3435/#3484lineage — each has an explicit comment about this).init.py's own_ext_spec_is_urlclassifier, defined right next to this function, already catches the sameValueError; this call site was the one outlier that didn't.try/except ValueErrorpattern as every sibling, raising a cleanValueError("Malformed extension URL: ...")that the caller already converts into a tracker error (per this function's own documented contract).Test plan
test_install_extension_during_init_reports_malformed_url_cleanlytotests/test_init_output_markup.py, calling_install_extension_during_initdirectly withhttps://[not-an-ip]/ext.zip.'not-an-ip' does not appear to be an IPv4 or IPv6 address) leaking through — and passes with it.tests/test_init_output_markup.pymodule — 9 passed, 3 skipped (pre-existing bash-requirement skips), no regressions.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt