mirror of
https://github.com/github/spec-kit.git
synced 2026-07-04 04:45:43 +08:00
* Harden preset URL installs against unsafe redirects Preset URL installs already rejected non-HTTPS source URLs, but the authenticated opener follows redirects. Validate the final response URL before writing the ZIP, preserve GitHub release asset URL resolution after the preset command module split, stream the response to disk, and keep catalog config serialization on safe YAML output. Constraint: open_url follows redirects, so source URL validation alone does not constrain the downloaded target Rejected: Keep response.read() for simplicity | large preset downloads should not be buffered entirely in memory Confidence: high Scope-risk: narrow Directive: Keep preset URL policy aligned with workflow installer redirect validation Tested: uvx ruff check src/specify_cli/__init__.py src/specify_cli/presets/__init__.py src/specify_cli/presets/_commands.py tests/test_presets.py Tested: uv run pytest tests/test_presets.py -q Not-tested: Real network redirect integration against a live HTTP server Co-authored-by: OmX <omx@oh-my-codex.dev> * Reject malformed preset download URLs Preset downloads should fail early when a URL lacks a hostname, even if the scheme is HTTPS. The redirect error now describes any disallowed target instead of implying that only non-HTTPS redirects are blocked. * Prevent credentialed preset redirects from downgrading transport Preset URL downloads already checked the final URL after urllib followed redirects, but that was too late for authenticated requests because same-host redirects could preserve Authorization during the redirect itself. The authenticated HTTP helper now supports an opt-in redirect validator, and preset downloads use it to reject disallowed redirect targets before following them. The redirect auth handlers also stop preserving credentials across HTTPS to non-HTTPS downgrades as defense in depth. * test(presets): 修复 URL 解析测试 mock 缺少 redirect_validator 参数 重定向安全加固为 open_url 新增 redirect_validator 参数, 两处 fake_open_url mock 签名未同步导致 TypeError。 补齐参数后全部 3717 个测试通过。 --------- Co-authored-by: OmX <omx@oh-my-codex.dev>
191 lines
7.8 KiB
Python
191 lines
7.8 KiB
Python
"""Tests for GitHub-authenticated HTTP request helpers."""
|
|
|
|
import json
|
|
import os
|
|
from contextlib import contextmanager
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from specify_cli._github_http import (
|
|
build_github_request,
|
|
resolve_github_release_asset_api_url,
|
|
)
|
|
|
|
|
|
class TestBuildGitHubRequest:
|
|
"""Tests for build_github_request() URL validation and auth handling."""
|
|
|
|
# --- URL Validation Tests ---
|
|
|
|
def test_empty_url_raises_value_error(self):
|
|
"""build_github_request() must reject an empty string URL."""
|
|
with pytest.raises(ValueError, match="url must not be empty"):
|
|
build_github_request("")
|
|
|
|
def test_whitespace_url_raises_value_error(self):
|
|
"""build_github_request() must reject a whitespace-only URL."""
|
|
with pytest.raises(ValueError, match="url must not be empty"):
|
|
build_github_request(" ")
|
|
|
|
def test_non_http_url_raises_value_error(self):
|
|
"""build_github_request() must reject URLs without http/https scheme."""
|
|
with pytest.raises(ValueError, match="url must start with http"):
|
|
build_github_request("not-a-url")
|
|
|
|
def test_ftp_url_raises_value_error(self):
|
|
"""build_github_request() must reject ftp:// URLs."""
|
|
with pytest.raises(ValueError, match="url must start with http"):
|
|
build_github_request("ftp://github.com/file.zip")
|
|
|
|
# --- Valid URL Tests ---
|
|
|
|
def test_valid_https_url_returns_request(self):
|
|
"""build_github_request() must return a Request for a valid https URL."""
|
|
req = build_github_request("https://github.com/github/spec-kit")
|
|
assert req.full_url == "https://github.com/github/spec-kit"
|
|
|
|
def test_valid_http_url_returns_request(self):
|
|
"""build_github_request() must accept http:// URLs."""
|
|
req = build_github_request("http://example.com/file")
|
|
assert req.full_url == "http://example.com/file"
|
|
|
|
# --- Auth Header Tests ---
|
|
|
|
def test_github_token_added_for_github_host(self):
|
|
"""Authorization header is set when GITHUB_TOKEN is present."""
|
|
with patch.dict(os.environ, {"GITHUB_TOKEN": "test-token", "GH_TOKEN": ""}):
|
|
req = build_github_request("https://github.com/github/spec-kit")
|
|
assert req.get_header("Authorization") == "Bearer test-token"
|
|
|
|
def test_gh_token_used_as_fallback(self):
|
|
"""GH_TOKEN is used when GITHUB_TOKEN is absent."""
|
|
with patch.dict(os.environ, {"GITHUB_TOKEN": "", "GH_TOKEN": "fallback-token"}):
|
|
req = build_github_request("https://github.com/github/spec-kit")
|
|
assert req.get_header("Authorization") == "Bearer fallback-token"
|
|
|
|
def test_no_auth_header_for_non_github_host(self):
|
|
"""Authorization header must NOT be set for non-GitHub URLs."""
|
|
with patch.dict(os.environ, {"GITHUB_TOKEN": "test-token"}):
|
|
req = build_github_request("https://example.com/file")
|
|
assert req.get_header("Authorization") is None
|
|
|
|
def test_no_auth_header_when_no_token(self):
|
|
"""No Authorization header when no token is set in environment."""
|
|
with patch.dict(os.environ, {}, clear=True):
|
|
req = build_github_request("https://github.com/github/spec-kit")
|
|
assert req.get_header("Authorization") is None
|
|
|
|
def test_missing_hostname_raises_value_error(self):
|
|
"""build_github_request() must reject URLs with valid scheme but no hostname."""
|
|
with pytest.raises(ValueError, match="url must include a hostname"):
|
|
build_github_request("http://")
|
|
|
|
|
|
class TestResolveGitHubReleaseAssetApiUrl:
|
|
"""Tests for resolve_github_release_asset_api_url()."""
|
|
|
|
def _make_open_url_fn(self, release_json):
|
|
"""Create a fake open_url_fn that returns release JSON."""
|
|
@contextmanager
|
|
def fake_open(url, timeout=None, extra_headers=None):
|
|
resp = MagicMock()
|
|
resp.read.return_value = json.dumps(release_json).encode()
|
|
yield resp
|
|
return fake_open
|
|
|
|
def test_returns_none_for_non_github_url(self):
|
|
"""Non-GitHub URLs should return None."""
|
|
result = resolve_github_release_asset_api_url(
|
|
"https://example.com/file.zip", lambda *a, **kw: None
|
|
)
|
|
assert result is None
|
|
|
|
def test_returns_none_for_non_release_github_url(self):
|
|
"""GitHub URLs that aren't release downloads return None."""
|
|
result = resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/archive/refs/tags/v1.zip",
|
|
lambda *a, **kw: None,
|
|
)
|
|
assert result is None
|
|
|
|
def test_passthrough_for_existing_api_asset_url(self):
|
|
"""Already-resolved REST API asset URLs are returned as-is."""
|
|
url = "https://api.github.com/repos/org/repo/releases/assets/12345"
|
|
result = resolve_github_release_asset_api_url(url, lambda *a, **kw: None)
|
|
assert result == url
|
|
|
|
def test_resolves_browser_url_to_api_url(self):
|
|
"""Browser release URL resolves to REST API asset URL."""
|
|
release_json = {
|
|
"assets": [
|
|
{"name": "pack.zip", "url": "https://api.github.com/repos/org/repo/releases/assets/99"}
|
|
]
|
|
}
|
|
result = resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/releases/download/v1.0/pack.zip",
|
|
self._make_open_url_fn(release_json),
|
|
)
|
|
assert result == "https://api.github.com/repos/org/repo/releases/assets/99"
|
|
|
|
def test_returns_none_when_asset_not_found(self):
|
|
"""Returns None when the release exists but asset name doesn't match."""
|
|
release_json = {"assets": [{"name": "other.zip", "url": "https://api.github.com/repos/org/repo/releases/assets/1"}]}
|
|
result = resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/releases/download/v1/missing.zip",
|
|
self._make_open_url_fn(release_json),
|
|
)
|
|
assert result is None
|
|
|
|
def test_returns_none_on_network_error(self):
|
|
"""Returns None when the API request fails."""
|
|
import urllib.error
|
|
|
|
@contextmanager
|
|
def failing_open(url, timeout=None, extra_headers=None):
|
|
raise urllib.error.URLError("network error")
|
|
yield # noqa: unreachable
|
|
|
|
result = resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/releases/download/v1/pack.zip",
|
|
failing_open,
|
|
)
|
|
assert result is None
|
|
|
|
def test_tag_with_special_characters_is_url_encoded(self):
|
|
"""Tags with reserved characters (e.g. '/') are encoded in the API URL."""
|
|
captured_urls = []
|
|
|
|
@contextmanager
|
|
def capturing_open(url, timeout=None, extra_headers=None):
|
|
captured_urls.append(url)
|
|
resp = MagicMock()
|
|
resp.read.return_value = json.dumps({"assets": []}).encode()
|
|
yield resp
|
|
|
|
resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/releases/download/feature%2Fv1/pack.zip",
|
|
capturing_open,
|
|
)
|
|
# The tag "feature/v1" (decoded from %2F) must be re-encoded as "feature%2Fv1"
|
|
assert len(captured_urls) == 1
|
|
assert "releases/tags/feature%2Fv1" in captured_urls[0]
|
|
|
|
def test_tag_with_hash_is_url_encoded(self):
|
|
"""Tags with '#' character are properly encoded."""
|
|
captured_urls = []
|
|
|
|
@contextmanager
|
|
def capturing_open(url, timeout=None, extra_headers=None):
|
|
captured_urls.append(url)
|
|
resp = MagicMock()
|
|
resp.read.return_value = json.dumps({"assets": []}).encode()
|
|
yield resp
|
|
|
|
resolve_github_release_asset_api_url(
|
|
"https://github.com/org/repo/releases/download/v1%23beta/pack.zip",
|
|
capturing_open,
|
|
)
|
|
assert len(captured_urls) == 1
|
|
assert "releases/tags/v1%23beta" in captured_urls[0]
|