Skip to content

fix(init): report a malformed --extension URL cleanly, not raw urllib text - #4324

Open
Noor-ul-ain001 wants to merge 3 commits into
github:mainfrom
Noor-ul-ain001:fix/init-extension-url-malformed
Open

Noor-ul-ain001 wants to merge 3 commits into
github:mainfrom
Noor-ul-ain001:fix/init-extension-url-malformed

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Summary

  • _install_extension_during_init (src/specify_cli/commands/init.py) parses an --extension URL spec with a bare parsed = 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> 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.
  • 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 #3435/#3484 lineage — each has an explicit comment about this). init.py's own _ext_spec_is_url classifier, defined right next to this function, already catches the same ValueError; this call site was the one outlier that didn't.
  • Fix: wrap the parse in the same try/except ValueError pattern as every sibling, raising a clean ValueError("Malformed extension URL: ...") that the caller already converts into a tracker error (per this function's own documented contract).

Test plan

  • Added test_install_extension_during_init_reports_malformed_url_cleanly to tests/test_init_output_markup.py, calling _install_extension_during_init directly with https://[not-an-ip]/ext.zip.
  • Verified the test fails without the fix — reproduced the exact raw urllib message ('not-an-ip' does not appear to be an IPv4 or IPv6 address) leaking through — and passes with it.
  • Ran the full tests/test_init_output_markup.py module — 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

@Noor-ul-ain001
Noor-ul-ain001 requested a review from mnriem as a code owner August 25, 2026 15:18
… 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread src/specify_cli/commands/init.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mnriem mnriem added triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate author-awaiting Waiting on author response labels Sep 8, 2026
@mnriem

mnriem commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks, and appreciated the disclosure + test. Copilot flagged a cross-version gap worth resolving: the guard only catches the eager ValueError (Python 3.14), but on the supported 3.11–3.13 the malformed authority is validated lazily at .hostname, so a bad URL may slip through there — and adding a .hostname probe can itself break local-path specs like C://[foo] unless you exclude local-path syntax first. Please handle the lazy path (and add a test that covers 3.11–3.13), then re-request review.

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
@Noor-ul-ain001

Copy link
Copy Markdown
Contributor Author

@mnriem Both gaps handled:

  1. Lazy hostname/port validation (3.11-3.13): the guard now probes .hostname/.port inside the same try/except ValueError as urlparse(), so the failure is caught whether it fires eagerly (3.14, at urlparse() itself) or lazily (3.11-3.13, at attribute access). Added test_install_extension_during_init_lazy_hostname_valueerror_reported_cleanly, which monkeypatches urlparse to raise on .hostname lazily — same pattern already used in tests/test_extensions.py::test_add_from_url_lazy_hostname_valueerror_exits_cleanly — so this is verified on any interpreter, not just whichever Python happens to be running.

  2. Local-path false positive: rather than special-casing bracketed authorities, I moved the local-path check to run before any URL parsing. _install_extension_during_init now only calls urlparse() once a spec has already failed the local-path test, so something like C://[my-ext] (which Python's urlparse treats as scheme "c", netloc "[my-ext]" — an invalid bracketed authority) never reaches urlparse() at all. This also turned out to matter for the 3.14 case specifically: on 3.14 the raise happens at urlparse() itself, not just at .hostname, so checking the path first is required there too, not only on 3.11-3.13. Added a regression test asserting such a spec fails with "Directory not found" (proving the local-path branch handled it), not "Malformed extension URL". Confirmed via test-the-test that both new tests fail against the prior commit and pass now.

Pushed as ad762fe. Re-requesting review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants