Skip to content

fix(auth): return no matches, not raw ValueError, for a malformed URL#3437

Merged
mnriem merged 1 commit into
github:mainfrom
Quratulain-bilal:fix/auth-url-malformed
Jul 10, 2026
Merged

fix(auth): return no matches, not raw ValueError, for a malformed URL#3437
mnriem merged 1 commit into
github:mainfrom
Quratulain-bilal:fix/auth-url-malformed

Conversation

@Quratulain-bilal

@Quratulain-bilal Quratulain-bilal commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

fixes #3436

find_entries_for_url did (urlparse(url).hostname or "").lower() unguarded. a malformed authority (unterminated ipv6 bracket https://[::1, bracketed non-ip host https://[not-an-ip]) makes urlparse/hostname raise ValueError, so a raw ValueError leaked instead of the empty list the function already returns for a host-less url. this is the first step of the shared http client (build_request / open_url call it before any url validation), so it can surface out of catalog/extension/preset fetches.

fix: wrap the parse + hostname access; on ValueError return no matches, exactly like the host-less case just below it (no auth entry can match a url with no usable host anyway).

added a regression test (test_malformed_url_returns_empty) over an unterminated bracket and a bracketed non-ip host. i confirmed it fails on the pre-fix code by stashing the source and re-running (raw ValueError leaks); the auth suite passes with the fix.

same bug class as #3210 and the recent bundler/catalog validator fixes.

@Quratulain-bilal
fix(auth): return no matches, not raw ValueError, for a malformed URL
3d7955f 
find_entries_for_url did (urlparse(url).hostname or "").lower() unguarded. a
malformed authority (e.g. an unterminated ipv6 bracket "https://[::1") makes
urlparse/hostname raise ValueError, so instead of the empty list the function
already returns for a host-less url, a raw ValueError leaked out of the shared
http client (build_request / open_url call this before any url validation).

no auth entry can match such a url, so treat it like the host-less case and
return no matches. added a regression test over an unterminated bracket and a
bracketed non-ip host; confirmed it fails on the pre-fix code.
@Quratulain-bilal
Quratulain-bilal requested a review from mnriem as a code owner July 9, 2026 20:58
@mnriem
mnriem requested a review from Copilot July 10, 2026 13:02
Copilot AI reviewed Jul 10, 2026
View reviewed changes

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.

Pull request overview

This PR fixes a crash in find_entries_for_url where malformed URL authorities could cause urlparse(...).hostname to raise ValueError, leaking an exception from the shared HTTP client path. The fix makes malformed-host URLs behave like host-less URLs for auth matching (returning no matches).

Changes:

  • Catch ValueError when extracting/lowercasing the parsed hostname and return [] for malformed authorities.
  • Add a regression test covering an unterminated IPv6 bracket and a bracketed non-IP host.
Show a summary per file
File Description
src/specify_cli/authentication/config.py Wrap hostname extraction in try/except ValueError and return no matches on malformed URLs.
tests/test_authentication.py Add parametrized regression test ensuring malformed URLs return [] rather than raising.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Low

@mnriem
mnriem merged commit 74d03a2 into github:main Jul 10, 2026
12 checks passed
@mnriem

mnriem commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

kanfil added a commit to tikalk/agentic-sdlc-spec-kit that referenced this pull request Jul 11, 2026
@kanfil
chore: merge upstream 0.12.11, bump to 0.12.11+adlc1
7736089 
Upstream merge (30 commits, 3 releases 0.12.9-0.12.11):
- invoke_separator parse-success fix (github#3304)
- Windows Store python3 stub skip + _interpreter_runs() probe (github#3385)
- SKILL.md frontmatter control char escape via yaml_quote() (github#3399)
- chained expression filters left-to-right refactor (github#3339)
- refresh_shared_templates preserves recovered files (github#3378)
- Goose yaml skill placeholder resolution (github#3374)
- bundled version pin enforcement (github#3377)
- integration test home isolation (github#3144)
- py: script type in command templates (github#3403)
- configurable shell step timeout (github#3404)
- find plans in nested spec directories (github#3405)
- plan.md phase numbering fix (github#3416)
- PowerShell -Number 0 honor via ContainsKey (github#3412)
- workflow.yml non-string scalar validation (github#3421)
- plan-template.md self-referencing path fix (github#3417)
- pre-commit config + trailing whitespace cleanup (github#3430)
- malformed URL error handling (github#3433/github#3435/github#3437)
- agent-context nested plan.md discovery (github#3301)
- community catalog additions (EARS, Figma) (github#3407/github#3408)

9 conflicts resolved: pyproject.toml, integrations/base.py, agents.py,
forge/__init__.py, hermes/__init__.py, create-new-feature-branch.ps1,
test_git_extension.py, test_base.py, test_integration_devin.py.

Template-to-preset alignment: added py: script lines to 6 preset commands,
removed stale Phase 1 agent context line from plan preset, fixed phase
numbering. Pre-merge fix: wrapped bare make_typer import with fallback.

Assisted-by: opencode (model: glm-5.2, autonomous)
kanfil added a commit to tikalk/agentic-sdlc-spec-kit that referenced this pull request Jul 11, 2026
@kanfil
chore: merge upstream 0.12.11, bump to 0.12.11+adlc1
a3aa3d2 
Upstream merge (30 commits, 3 releases 0.12.9-0.12.11):
- invoke_separator parse-success fix (github#3304)
- Windows Store python3 stub skip + _interpreter_runs() probe (github#3385)
- SKILL.md frontmatter control char escape via yaml_quote() (github#3399)
- chained expression filters left-to-right refactor (github#3339)
- refresh_shared_templates preserves recovered files (github#3378)
- Goose yaml skill placeholder resolution (github#3374)
- bundled version pin enforcement (github#3377)
- integration test home isolation (github#3144)
- py: script type in command templates (github#3403)
- configurable shell step timeout (github#3404)
- find plans in nested spec directories (github#3405)
- plan.md phase numbering fix (github#3416)
- PowerShell -Number 0 honor via ContainsKey (github#3412)
- workflow.yml non-string scalar validation (github#3421)
- plan-template.md self-referencing path fix (github#3417)
- pre-commit config + trailing whitespace cleanup (github#3430)
- malformed URL error handling (github#3433/github#3435/github#3437)
- agent-context nested plan.md discovery (github#3301)
- community catalog additions (EARS, Figma) (github#3407/github#3408)

9 conflicts resolved: pyproject.toml, integrations/base.py, agents.py,
forge/__init__.py, hermes/__init__.py, create-new-feature-branch.ps1,
test_git_extension.py, test_base.py, test_integration_devin.py.

Template-to-preset alignment: added py: script lines to 6 preset commands,
removed stale Phase 1 agent context line from plan preset, fixed phase
numbering. Pre-merge fix: wrapped bare make_typer import with fallback.

Assisted-by: opencode (model: glm-5.2, autonomous)
@Noor-ul-ain001 Noor-ul-ain001 mentioned this pull request Jul 12, 2026
Merged
@Noor-ul-ain001 Noor-ul-ain001 mentioned this pull request Jul 22, 2026
Merged
mnriem pushed a commit that referenced this pull request Jul 23, 2026
@Noor-ul-ain001 @claude
fix(cli): guard lazy .hostname ValueError in extension/preset add --f…
cce47f6 
…rom (#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@mnriem mnriem Awaiting requested review from mnriem mnriem is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

auth find_entries_for_url leaks raw ValueError on a malformed URL

3 participants

@Quratulain-bilal @mnriem