github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL (#3484)

Browse Source 
* fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL

The four catalog URL validators in `workflows/catalog.py`
(`WorkflowCatalog`/`StepCatalog` `_validate_catalog_url`, and the nested
fetch-path validators) accessed `urlparse(url).hostname` unguarded. A
malformed authority — e.g. an unterminated IPv6 bracket `https://[::1`
or a bracketed non-IP host `https://[not-an-ip]` — makes urlparse /
hostname raise `ValueError`.

Each validator's contract is to raise a domain error
(`WorkflowValidationError` / `StepValidationError` /
`WorkflowCatalogError` / `StepCatalogError`), and the command handlers
catch only those. So `specify workflow catalog add "https://[::1"`
surfaced an uncaught `ValueError` traceback instead of the clean
`Error: Catalog URL is malformed` + exit 1 that a bad URL should give.
The fetch-path validators also run on the post-redirect `resp.geturl()`,
so a hostile redirect target could crash the fetch the same way.

Guard each `urlparse`/`.hostname` access with `try/except ValueError ->
domain error`, mirroring the fixes already applied to
`specify_cli.catalogs` (#3435) and the bundler adapters (#3433). Also
read `hostname` once and reuse it for the host check, matching those
siblings.

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

* test(workflows): cover post-redirect malformed-URL guard (#3484 review)

Copilot review asked for regression tests on the fetch-path validators that
re-check resp.geturl() after redirects — the branch that turns a malformed
redirect target into a domain error instead of a raw ValueError.

- test_fetch_malformed_redirect_target_raises_catalog_error on both
  TestWorkflowCatalog and TestStepCatalog: stub open_url with a response whose
  geturl() is malformed (https://[::1 / https://[not-an-ip]/x) while entry.url
  is valid, so validation only trips on the redirect target, and assert
  _fetch_single_catalog raises WorkflowCatalogError / StepCatalogError with a
  "malformed" message (force_refresh + fresh project_dir so no cache masks it).
- Test-the-test: both fail on pre-fix source (raw ValueError re-wrapped as
  "...Invalid IPv6 URL", no "malformed" match) and pass with the guard.

Also merges latest upstream/main into the branch.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Noor ul ain
2026-07-14 18:11:23 +05:00
committed by GitHub
parent 73093954e2
commit e742b8010a
2 changed files with 204 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
src/specify_cli/workflows/catalog.py
View File

@@ -299,8 +299,20 @@ class WorkflowCatalog:
"""Validate that a catalog URL uses HTTPS (localhost HTTP allowed)."""
from urllib.parse import urlparse
parsed = urlparse(url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
# A malformed authority (e.g. an unterminated IPv6 bracket
# "https://[::1") makes urlparse / hostname access raise ValueError.
# This validator's contract is to raise WorkflowValidationError for a
# bad URL, so surface that rather than leaking a raw ValueError past the
# command handler (which only catches WorkflowValidationError). Mirrors
# specify_cli.catalogs (#3435).
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
raise WorkflowValidationError(
f"Catalog URL is malformed: {url}"
) from None
is_localhost = hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (
parsed.scheme == "http" and is_localhost
):
@@ -308,7 +320,7 @@ class WorkflowCatalog:
f"Catalog URL must use HTTPS (got {parsed.scheme}://). "
"HTTP is only allowed for localhost."
)
if not parsed.hostname:
if not hostname:
raise WorkflowValidationError(
"Catalog URL must be a valid URL with a host."
)
@@ -474,15 +486,26 @@ class WorkflowCatalog:
from specify_cli.authentication.http import open_url as _open_url
def _validate_catalog_url(url: str) -> None:
parsed = urlparse(url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
# A malformed authority (e.g. "https://[::1") makes urlparse /
# hostname access raise ValueError; treat it as a refused fetch
# rather than leaking a raw ValueError (this also validates the
# post-redirect resp.geturl(), so a hostile redirect target cannot
# crash the fetch either).
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
raise WorkflowCatalogError(
f"Refusing to fetch catalog from malformed URL: {url}"
) from None
is_localhost = hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (
parsed.scheme == "http" and is_localhost
):
raise WorkflowCatalogError(
f"Refusing to fetch catalog from non-HTTPS URL: {url}"
)
if not parsed.hostname:
if not hostname:
raise WorkflowCatalogError(
f"Refusing to fetch catalog from URL with no hostname: {url}"
)
@@ -921,8 +944,20 @@ class StepCatalog:
"""Validate that a catalog URL uses HTTPS (localhost HTTP allowed)."""
from urllib.parse import urlparse
parsed = urlparse(url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
# A malformed authority (e.g. an unterminated IPv6 bracket
# "https://[::1") makes urlparse / hostname access raise ValueError.
# This validator's contract is to raise StepValidationError for a bad
# URL, so surface that rather than leaking a raw ValueError past the
# command handler (which only catches StepValidationError). Mirrors
# specify_cli.catalogs (#3435).
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
raise StepValidationError(
f"Catalog URL is malformed: {url}"
) from None
is_localhost = hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (
parsed.scheme == "http" and is_localhost
):
@@ -930,7 +965,7 @@ class StepCatalog:
f"Catalog URL must use HTTPS (got {parsed.scheme}://). "
"HTTP is only allowed for localhost."
)
if not parsed.hostname:
if not hostname:
raise StepValidationError(
"Catalog URL must be a valid URL with a host."
)
@@ -1096,15 +1131,26 @@ class StepCatalog:
from specify_cli.authentication.http import open_url as _open_url
def _validate_url(url: str) -> None:
parsed = urlparse(url)
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
# A malformed authority (e.g. "https://[::1") makes urlparse /
# hostname access raise ValueError; treat it as a refused fetch
# rather than leaking a raw ValueError (this also validates the
# post-redirect resp.geturl(), so a hostile redirect target cannot
# crash the fetch either).
try:
parsed = urlparse(url)
hostname = parsed.hostname
except ValueError:
raise StepCatalogError(
f"Refusing to fetch catalog from malformed URL: {url}"
) from None
is_localhost = hostname in ("localhost", "127.0.0.1", "::1")
if parsed.scheme != "https" and not (
parsed.scheme == "http" and is_localhost
):
raise StepCatalogError(
f"Refusing to fetch catalog from non-HTTPS URL: {url}"
)
if not parsed.hostname:
if not hostname:
raise StepCatalogError(
f"Refusing to fetch catalog from URL with no hostname: {url}"
)

146
tests/test_workflows.py
View File

@@ -5246,6 +5246,83 @@ class TestWorkflowCatalog:
# Should not raise
catalog._validate_catalog_url("http://localhost:8080/catalog.json")
@pytest.mark.parametrize(
"url",
[
"https://[::1", # unterminated IPv6 bracket
"https://[not-an-ip]/x", # bracketed non-IP host
],
)
def test_validate_url_malformed_raises_validation_error(self, project_dir, url):
"""A malformed authority must raise WorkflowValidationError, not leak a
raw ValueError.
``urlparse``/``.hostname`` raise ValueError on a malformed IPv6
authority. The command handler only catches WorkflowValidationError,
so a raw ValueError would surface as an uncaught traceback instead of a
clean 'Error:' message + exit 1. Mirrors specify_cli.catalogs (#3435).
"""
from specify_cli.workflows.catalog import (
WorkflowCatalog,
WorkflowValidationError,
)
catalog = WorkflowCatalog(project_dir)
with pytest.raises(WorkflowValidationError, match="malformed"):
catalog._validate_catalog_url(url)
def test_fetch_malformed_redirect_target_raises_catalog_error(
self, project_dir, monkeypatch
):
"""A malformed post-redirect URL must raise WorkflowCatalogError, not a
raw ValueError.
The fetch path re-validates ``resp.geturl()`` after following redirects,
so a hostile/broken redirect to a malformed authority
(``https://[::1``) hits ``urlparse``/``.hostname`` and raises
``ValueError``. Without the guard that ValueError is re-wrapped by the
broad ``except`` as ``Failed to fetch catalog ...: Invalid IPv6 URL``;
the guard turns it into a clean ``... malformed URL ...`` refusal. The
initial ``entry.url`` is valid so validation only trips on the redirect
target. Mirrors specify_cli.catalogs (#3435).
"""
from specify_cli.workflows.catalog import (
WorkflowCatalog,
WorkflowCatalogEntry,
WorkflowCatalogError,
)
from specify_cli.authentication import http as auth_http
class _FakeResponse:
def __enter__(self):
return self
def __exit__(self, exc_type, exc, tb):
return False
def read(self):
return b"{}"
def geturl(self):
# A redirect landing on a malformed IPv6 authority.
return "https://[::1"
monkeypatch.setattr(
auth_http, "open_url", lambda url, timeout=30: _FakeResponse()
)
catalog = WorkflowCatalog(project_dir)
entry = WorkflowCatalogEntry(
url="https://example.com/catalog.json",
name="test",
priority=1,
install_allowed=True,
)
# A fresh project_dir has no cache to fall back to, so the error
# propagates instead of being masked by a stale-cache read.
with pytest.raises(WorkflowCatalogError, match="malformed"):
catalog._fetch_single_catalog(entry, force_refresh=True)
def test_add_catalog(self, project_dir):
from specify_cli.workflows.catalog import WorkflowCatalog
@@ -5692,6 +5769,75 @@ class TestStepCatalog:
# Should not raise
catalog._validate_catalog_url("http://localhost:8080/step-catalog.json")
@pytest.mark.parametrize(
"url",
[
"https://[::1", # unterminated IPv6 bracket
"https://[not-an-ip]/x", # bracketed non-IP host
],
)
def test_validate_url_malformed_raises_validation_error(self, project_dir, url):
"""A malformed authority must raise StepValidationError, not leak a raw
ValueError past the command handler (which only catches
StepValidationError). Mirrors specify_cli.catalogs (#3435).
"""
from specify_cli.workflows.catalog import StepCatalog, StepValidationError
catalog = StepCatalog(project_dir)
with pytest.raises(StepValidationError, match="malformed"):
catalog._validate_catalog_url(url)
def test_fetch_malformed_redirect_target_raises_catalog_error(
self, project_dir, monkeypatch
):
"""A malformed post-redirect URL must raise StepCatalogError, not a raw
ValueError.
The fetch path re-validates ``resp.geturl()`` after redirects, so a
broken redirect to a bracketed non-IP host (``https://[not-an-ip]/x``)
makes ``urlparse``/``.hostname`` raise ``ValueError``. Without the guard
that leaks out as ``... Invalid IPv6 URL`` re-wrapping; the guard turns
it into a clean ``... malformed URL ...`` refusal. The initial
``entry.url`` is valid so validation only trips on the redirect target.
Mirrors specify_cli.catalogs (#3435).
"""
from specify_cli.workflows.catalog import (
StepCatalog,
StepCatalogEntry,
StepCatalogError,
)
from specify_cli.authentication import http as auth_http
class _FakeResponse:
def __enter__(self):
return self
def __exit__(self, exc_type, exc, tb):
return False
def read(self):
return b"{}"
def geturl(self):
# A redirect landing on a bracketed non-IP authority.
return "https://[not-an-ip]/x"
monkeypatch.setattr(
auth_http, "open_url", lambda url, timeout=30: _FakeResponse()
)
catalog = StepCatalog(project_dir)
entry = StepCatalogEntry(
url="https://example.com/steps.json",
name="test",
priority=1,
install_allowed=True,
)
# A fresh project_dir has no cache to fall back to, so the error
# propagates instead of being masked by a stale-cache read.
with pytest.raises(StepCatalogError, match="malformed"):
catalog._fetch_single_catalog(entry, force_refresh=True)
def test_add_catalog(self, project_dir):
from specify_cli.workflows.catalog import StepCatalog