From ea93adec36cb406adb80c26a908ea7f0c8a79528 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 12 Jul 2026 21:34:28 +0500 Subject: [PATCH 1/2] fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/specify_cli/workflows/catalog.py | 70 +++++++++++++++++++++++----- tests/test_workflows.py | 43 +++++++++++++++++ 2 files changed, 101 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 97bf58a04e..0908b89b94 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -157,8 +157,20 @@ def _validate_catalog_url(self, url: str) -> None: """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 ): @@ -166,7 +178,7 @@ def _validate_catalog_url(self, url: str) -> None: 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." ) @@ -332,15 +344,26 @@ def _fetch_single_catalog( 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}" ) @@ -774,8 +797,20 @@ def _validate_catalog_url(self, url: str) -> None: """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 ): @@ -783,7 +818,7 @@ def _validate_catalog_url(self, url: str) -> None: 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." ) @@ -949,15 +984,26 @@ def _fetch_single_catalog( 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}" ) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d7cff20f6d..ee456deeb7 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4805,6 +4805,31 @@ def test_validate_url_localhost_http_allowed(self, project_dir): # 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_add_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -5251,6 +5276,24 @@ def test_validate_url_localhost_http_allowed(self, project_dir): # 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_add_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog From 5941491bc97db429e6d0bb7dfa73e733903924c2 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 14 Jul 2026 09:10:17 +0500 Subject: [PATCH 2/2] test(workflows): cover post-redirect malformed-URL guard (#3484 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/test_workflows.py | 103 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index b8122b8bf2..e9d4b33a51 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -5097,6 +5097,58 @@ def test_validate_url_malformed_raises_validation_error(self, project_dir, url): 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 @@ -5561,6 +5613,57 @@ def test_validate_url_malformed_raises_validation_error(self, project_dir, url): 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