mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(workflows): validate every redirect hop when fetching workflow/step catalogs (#3637)
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 (#3523) and extensions (#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.
This commit is contained in:
@@ -523,8 +523,20 @@ class WorkflowCatalog:
|
||||
|
||||
_validate_catalog_url(entry.url)
|
||||
|
||||
# Validate EVERY redirect hop, not just the final URL: _open_url follows
|
||||
# redirects, so an https:// entry that 30x-redirects through http:// (or
|
||||
# to a non-HTTPS host mid-chain) could otherwise let a network attacker
|
||||
# rewrite the next hop and slip a payload past a final-URL-only check.
|
||||
# redirect_validator runs before each hop; the geturl() check below is
|
||||
# retained as a defense-in-depth backstop. Mirrors the presets/extensions
|
||||
# catalog fix (#3523 / #3524).
|
||||
def _validate_redirect(_old_url: str, new_url: str) -> None:
|
||||
_validate_catalog_url(new_url)
|
||||
|
||||
try:
|
||||
with _open_url(entry.url, timeout=30) as resp:
|
||||
with _open_url(
|
||||
entry.url, timeout=30, redirect_validator=_validate_redirect
|
||||
) as resp:
|
||||
_validate_catalog_url(resp.geturl())
|
||||
data = json.loads(resp.read().decode("utf-8"))
|
||||
except Exception as exc:
|
||||
@@ -1180,8 +1192,20 @@ class StepCatalog:
|
||||
|
||||
_validate_url(entry.url)
|
||||
|
||||
# Validate EVERY redirect hop, not just the final URL: _open_url follows
|
||||
# redirects, so an https:// entry that 30x-redirects through http:// (or
|
||||
# to a non-HTTPS host mid-chain) could otherwise let a network attacker
|
||||
# rewrite the next hop and slip a payload past a final-URL-only check.
|
||||
# redirect_validator runs before each hop; the geturl() check below is
|
||||
# retained as a defense-in-depth backstop. Mirrors the presets/extensions
|
||||
# catalog fix (#3523 / #3524).
|
||||
def _validate_redirect(_old_url: str, new_url: str) -> None:
|
||||
_validate_url(new_url)
|
||||
|
||||
try:
|
||||
with _open_url(entry.url, timeout=30) as resp:
|
||||
with _open_url(
|
||||
entry.url, timeout=30, redirect_validator=_validate_redirect
|
||||
) as resp:
|
||||
_validate_url(resp.geturl())
|
||||
data = json.loads(resp.read().decode("utf-8"))
|
||||
except Exception as exc:
|
||||
|
||||
@@ -6321,7 +6321,9 @@ class TestWorkflowCatalog:
|
||||
return "https://[::1"
|
||||
|
||||
monkeypatch.setattr(
|
||||
auth_http, "open_url", lambda url, timeout=30: _FakeResponse()
|
||||
auth_http,
|
||||
"open_url",
|
||||
lambda url, timeout=30, redirect_validator=None: _FakeResponse(),
|
||||
)
|
||||
|
||||
catalog = WorkflowCatalog(project_dir)
|
||||
@@ -6336,6 +6338,41 @@ class TestWorkflowCatalog:
|
||||
with pytest.raises(WorkflowCatalogError, match="malformed"):
|
||||
catalog._fetch_single_catalog(entry, force_refresh=True)
|
||||
|
||||
def test_fetch_validates_every_redirect_hop(self, project_dir, monkeypatch):
|
||||
"""A redirect_validator is passed to open_url and rejects a non-HTTPS
|
||||
INTERMEDIATE hop — closing the https -> http -> attacker-https chain a
|
||||
terminal-URL-only check would miss. Mirrors presets/extensions
|
||||
(#3523 / #3524)."""
|
||||
from specify_cli.workflows.catalog import (
|
||||
WorkflowCatalog,
|
||||
WorkflowCatalogEntry,
|
||||
WorkflowCatalogError,
|
||||
)
|
||||
from specify_cli.authentication import http as auth_http
|
||||
|
||||
captured = {}
|
||||
|
||||
def fake_open(url, timeout=30, redirect_validator=None):
|
||||
captured["rv"] = redirect_validator
|
||||
# Simulate the hop urllib validates before following the redirect.
|
||||
redirect_validator(
|
||||
"https://good.example/catalog.json", "http://evil.test/hop"
|
||||
)
|
||||
raise AssertionError("redirect_validator should have raised")
|
||||
|
||||
monkeypatch.setattr(auth_http, "open_url", fake_open)
|
||||
|
||||
catalog = WorkflowCatalog(project_dir)
|
||||
entry = WorkflowCatalogEntry(
|
||||
url="https://good.example/catalog.json",
|
||||
name="test",
|
||||
priority=1,
|
||||
install_allowed=True,
|
||||
)
|
||||
with pytest.raises(WorkflowCatalogError, match="HTTPS"):
|
||||
catalog._fetch_single_catalog(entry, force_refresh=True)
|
||||
assert captured["rv"] is not None
|
||||
|
||||
def test_add_catalog(self, project_dir):
|
||||
from specify_cli.workflows.catalog import WorkflowCatalog
|
||||
|
||||
@@ -6880,7 +6917,9 @@ class TestStepCatalog:
|
||||
return "https://[not-an-ip]/x"
|
||||
|
||||
monkeypatch.setattr(
|
||||
auth_http, "open_url", lambda url, timeout=30: _FakeResponse()
|
||||
auth_http,
|
||||
"open_url",
|
||||
lambda url, timeout=30, redirect_validator=None: _FakeResponse(),
|
||||
)
|
||||
|
||||
catalog = StepCatalog(project_dir)
|
||||
@@ -6895,6 +6934,41 @@ class TestStepCatalog:
|
||||
with pytest.raises(StepCatalogError, match="malformed"):
|
||||
catalog._fetch_single_catalog(entry, force_refresh=True)
|
||||
|
||||
def test_fetch_validates_every_redirect_hop(self, project_dir, monkeypatch):
|
||||
"""A redirect_validator is passed to open_url and rejects a non-HTTPS
|
||||
INTERMEDIATE hop — closing the https -> http -> attacker-https chain a
|
||||
terminal-URL-only check would miss. Mirrors presets/extensions
|
||||
(#3523 / #3524)."""
|
||||
from specify_cli.workflows.catalog import (
|
||||
StepCatalog,
|
||||
StepCatalogEntry,
|
||||
StepCatalogError,
|
||||
)
|
||||
from specify_cli.authentication import http as auth_http
|
||||
|
||||
captured = {}
|
||||
|
||||
def fake_open(url, timeout=30, redirect_validator=None):
|
||||
captured["rv"] = redirect_validator
|
||||
# Simulate the hop urllib validates before following the redirect.
|
||||
redirect_validator(
|
||||
"https://good.example/steps.json", "http://evil.test/hop"
|
||||
)
|
||||
raise AssertionError("redirect_validator should have raised")
|
||||
|
||||
monkeypatch.setattr(auth_http, "open_url", fake_open)
|
||||
|
||||
catalog = StepCatalog(project_dir)
|
||||
entry = StepCatalogEntry(
|
||||
url="https://good.example/steps.json",
|
||||
name="test",
|
||||
priority=1,
|
||||
install_allowed=True,
|
||||
)
|
||||
with pytest.raises(StepCatalogError, match="HTTPS"):
|
||||
catalog._fetch_single_catalog(entry, force_refresh=True)
|
||||
assert captured["rv"] is not None
|
||||
|
||||
def test_add_catalog(self, project_dir):
|
||||
from specify_cli.workflows.catalog import StepCatalog
|
||||
|
||||
|
||||
Reference in New Issue
Block a user