Skip to content

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

Merged
mnriem merged 3 commits into
github:mainfrom
Noor-ul-ain001:fix/workflow-catalog-malformed-url
Jul 14, 2026
Merged

fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL#3484
mnriem merged 3 commits into
github:mainfrom
Noor-ul-ain001:fix/workflow-catalog-malformed-url

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Summary

The four catalog URL validators in src/specify_cli/workflows/catalog.py accessed urlparse(url).hostname unguarded:

  • WorkflowCatalog._validate_catalog_url (raises WorkflowValidationError)
  • StepCatalog._validate_catalog_url (raises StepValidationError)
  • the nested fetch-path validators (raise WorkflowCatalogError / StepCatalogError)

A malformed authority — 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 its domain error, and the CLI handlers catch only those. So a bad URL leaked a raw ValueError traceback instead of the clean message + exit 1 a bad URL should produce:

$ specify workflow catalog add "https://[::1"
# before: Traceback ... ValueError: Invalid IPv6 URL
# after:  Error: Catalog URL is malformed: https://[::1   (exit 1)

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 / .hostname access with try/except ValueError -> domain error, and read hostname once and reuse it for the host check — mirroring the fixes already applied to specify_cli.catalogs (#3435) and the bundler adapters (#3433). This is the direct workflows/catalog.py twin of those; the same bug class as the auth-config fix (#3437).

Testing

  • Added test_validate_url_malformed_raises_validation_error to both the WorkflowCatalog and StepCatalog test classes (parametrized: unterminated IPv6 bracket, bracketed non-IP host).
  • Verified end-to-end: specify workflow catalog add "https://[::1" now exits 1 with Error: Catalog URL is malformed: ... and no traceback.
  • Full tests/test_workflows.py passes except the 11 pre-existing Windows symlink-guard tests (fail identically on a clean base; require elevation).
  • ruff check clean on the changed files.

🤖 Generated with Claude Code

…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>

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 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() + .hostname access in try/except ValueError and raise WorkflowValidationError / StepValidationError with 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 raw ValueError.
  • 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 thread src/specify_cli/workflows/catalog.py
Comment thread src/specify_cli/workflows/catalog.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

Noor-ul-ain001 and others added 2 commits July 14, 2026 09:05
…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>
@Noor-ul-ain001

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed both Copilot comments — they asked for regression coverage of the two post-redirect validators (_validate_catalog_url(resp.geturl()) / _validate_url(resp.geturl())), which the original tests only exercised at config time.

Added test_fetch_malformed_redirect_target_raises_catalog_error to both TestWorkflowCatalog and TestStepCatalog:

  • Stub open_url to return a response whose geturl() is a malformed authority (https://[::1 for the workflow catalog, https://[not-an-ip]/x for the step catalog), while entry.url is a valid host — so validation only trips on the redirect target, isolating the post-redirect branch.
  • Assert _fetch_single_catalog(...) raises WorkflowCatalogError / StepCatalogError matching malformed. Uses force_refresh=True on a fresh project_dir so no cache masks the error.

Test-the-test: both fail on the pre-fix source — the raw ValueError there is re-wrapped by the broad except as Failed to fetch catalog ...: Invalid IPv6 URL, which does not match malformed — and pass once the guard converts it to a clean malformed-URL refusal. So they genuinely cover the redirect path, not just the code shape.

Also merged the latest upstream/main. pytest tests/test_workflows.py::TestWorkflowCatalog tests/test_workflows.py::TestStepCatalog → 40 passed; ruff check clean.

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.

Review details

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

@mnriem mnriem self-requested a review July 14, 2026 13:10
@mnriem mnriem merged commit e742b80 into github:main Jul 14, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants