fix(workflows): validate every redirect hop when fetching workflow/step catalogs#3637
Merged
mnriem merged 1 commit intoJul 22, 2026
Conversation
…ep catalogs WorkflowCatalog._fetch_single_catalog and StepCatalog._fetch_single_catalog opened the catalog URL with open_url(entry.url, timeout=30) and validated only the final resp.geturl(). open_url follows redirects, so an https:// catalog entry that 30x-redirects through a non-HTTPS host mid-chain could let a network attacker rewrite the next hop and slip a payload past the terminal-URL-only check. The payload then drives step/workflow catalog data. Pass a redirect_validator that runs the existing HTTPS/hostname check before every redirect hop, keeping the final geturl() check as a defense-in-depth backstop. This brings both workflow catalog loaders to parity with the presets (github#3523) and extensions (github#3524) catalog fetchers. Tests: add per-hop redirect-validation tests for both WorkflowCatalog and StepCatalog (a non-HTTPS intermediate hop is rejected); both fail before the fix ("NoneType object is not callable" — no validator passed). Update the two existing malformed-redirect tests whose open_url stub lacked the redirect_validator kwarg.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-hop redirect validation to workflow and step catalog fetching, closing an HTTPS security gap.
Changes:
- Validates every redirect target before following it.
- Retains final URL validation as defense in depth.
- Adds regression tests for insecure intermediate redirects.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/catalog.py |
Adds redirect validators to both catalog loaders. |
tests/test_workflows.py |
Tests intermediate-hop rejection and updates mocks. |
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: 0
- Review effort level: Medium
Collaborator
|
Thank you! |
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.
WorkflowCatalog._fetch_single_catalogandStepCatalog._fetch_single_catalogopened the catalog URL withopen_url(entry.url, timeout=30)and validated only the finalresp.geturl().open_urlfollows redirects, so anhttps://catalog entry that 30x-redirects through a non-HTTPS host mid-chain could let a network attacker rewrite the next hop and slip a payload past the terminal-URL-only check. That payload then drives the workflow/step catalog data used by discovery and install.This is the same security-parity gap already closed for the presets (#3523) and extensions (#3524) catalog fetchers — both now validate every redirect hop, not just the final URL. The two workflow catalog loaders were missed.
Fix
Pass a
redirect_validatorthat runs the existing HTTPS/hostname check (_validate_catalog_url/_validate_url) before every redirect hop, in both loaders. The finalgeturl()check is kept as a defense-in-depth backstop.Tests
test_fetch_validates_every_redirect_hopfor bothTestWorkflowCatalogandTestStepCatalog: a non-HTTPS intermediate hop is rejected with an HTTPS refusal. Both fail before the fix ('NoneType' object is not callable— no validator was passed, so the redirect is followed unchecked) and pass after.test_fetch_malformed_redirect_target_raises_catalog_errorstubs whoseopen_urllambda lacked theredirect_validatorkwarg.All catalog-related workflow tests pass (78 passed, 1 skipped).