fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL#3484
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
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>
Contributor
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
Comment on lines
346
to
+356
| 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( |
Comment on lines
986
to
+996
| 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
requested changes
Jul 13, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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