fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL#3484
fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL#3484Noor-ul-ain001 wants to merge 3 commits into
Conversation
…ed 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` (github#3435) and the bundler adapters (github#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>
There was a problem hiding this comment.
Pull request overview
This PR hardens workflow/step catalog URL validation in src/specify_cli/workflows/catalog.py so malformed authorities that trigger urlparse(...).hostname ValueError are converted into the expected domain-specific errors, preventing uncaught tracebacks in CLI handlers and during post-redirect validation.
Changes:
- Wrap
urlparse()+.hostnameaccess intry/except ValueErrorand raiseWorkflowValidationError/StepValidationErrorwith a clean “malformed URL” message. - Apply the same guarding to the fetch-time (including post-redirect
resp.geturl()) validators so redirects can’t crash the fetch path with a rawValueError. - Add regression tests ensuring malformed authorities raise the appropriate validation errors for both workflow and step catalog validators.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/catalog.py |
Converts malformed-URL ValueError into workflow/step domain errors for both config-time and fetch-time validators, including post-redirect validation. |
tests/test_workflows.py |
Adds regression tests asserting malformed authorities raise WorkflowValidationError / StepValidationError (instead of leaking ValueError). |
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: 2
- Review effort level: Low
| 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( |
| 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( |
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
…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>
|
Thanks for the review. Addressed both Copilot comments — they asked for regression coverage of the two post-redirect validators ( Added
Test-the-test: both fail on the pre-fix source — the raw Also merged the latest |
Summary
The four catalog URL validators in
src/specify_cli/workflows/catalog.pyaccessedurlparse(url).hostnameunguarded:WorkflowCatalog._validate_catalog_url(raisesWorkflowValidationError)StepCatalog._validate_catalog_url(raisesStepValidationError)WorkflowCatalogError/StepCatalogError)A malformed authority — an unterminated IPv6 bracket
https://[::1or a bracketed non-IP hosthttps://[not-an-ip]— makesurlparse/.hostnameraiseValueError.Each validator's contract is to raise its domain error, and the CLI handlers catch only those. So a bad URL leaked a raw
ValueErrortraceback instead of the clean message + exit 1 a bad URL should produce:The two fetch-path validators also run on the post-redirect
resp.geturl(), so a hostile/broken redirect target could crash the fetch the same way.Fix
Guard each
urlparse/.hostnameaccess withtry/except ValueError -> domain error, and readhostnameonce and reuse it for the host check — mirroring the fixes already applied tospecify_cli.catalogs(#3435) and the bundler adapters (#3433). This is the directworkflows/catalog.pytwin of those; the same bug class as the auth-config fix (#3437).Testing
test_validate_url_malformed_raises_validation_errorto both theWorkflowCatalogandStepCatalogtest classes (parametrized: unterminated IPv6 bracket, bracketed non-IP host).specify workflow catalog add "https://[::1"now exits 1 withError: Catalog URL is malformed: ...and no traceback.tests/test_workflows.pypasses except the 11 pre-existing Windows symlink-guard tests (fail identically on a clean base; require elevation).ruff checkclean on the changed files.🤖 Generated with Claude Code