github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: harden bounded reads and redirect validation (#3671)

Browse Source 
Assisted-by: OpenAI Codex (model: GPT-5, autonomous)
This commit is contained in:
Pascal THUET
2026-07-22 22:18:10 +02:00
committed by GitHub
parent c0f4cee25a
commit 3a7a8758f7
10 changed files with 557 additions and 143 deletions
Download Patch File Download Diff File Expand all files Collapse all files

195
tests/test_authentication.py
View File

@@ -516,6 +516,23 @@ class TestAzureDevOpsAuth:
with patch("specify_cli.authentication.azure_devops.subprocess.run", side_effect=boom):
assert AzureDevOpsAuth().resolve_token(entry) is None
@pytest.mark.parametrize("payload", [[], {"accessToken": None}, {"accessToken": 123}])
def test_resolve_token_azure_cli_unexpected_json_shape_returns_none(
self, payload
):
from unittest.mock import MagicMock, patch
entry = AuthConfigEntry(
hosts=("dev.azure.com",), provider="azure-devops", auth="azure-cli",
)
result = MagicMock(returncode=0, stdout=json.dumps(payload))
with patch(
"specify_cli.authentication.azure_devops.subprocess.run",
return_value=result,
):
assert AzureDevOpsAuth().resolve_token(entry) is None
def test_resolve_token_azure_ad_success(self, monkeypatch):
"""azure-ad acquires token via OAuth2 client credentials."""
from unittest.mock import patch, MagicMock
@@ -559,7 +576,13 @@ class TestAzureDevOpsAuth:
with patch("urllib.request.build_opener", return_value=mock_opener):
assert AzureDevOpsAuth().resolve_token(entry) is None
def test_resolve_token_azure_ad_rejects_https_redirect(self, monkeypatch):
@pytest.mark.parametrize(
("status", "reason"),
[(307, "Temporary Redirect"), (308, "Permanent Redirect")],
)
def test_resolve_token_azure_ad_rejects_https_redirect(
self, monkeypatch, status, reason
):
"""The client-secret POST must never be redirected to another host."""
import urllib.error
from unittest.mock import MagicMock, patch
@@ -577,17 +600,72 @@ class TestAzureDevOpsAuth:
assert AzureDevOpsAuth().resolve_token(entry) is None
redirect_handler = build_opener.call_args.args[0]
request = Request("https://login.microsoftonline.com/tid/oauth2/v2.0/token")
request = Request(
"https://login.microsoftonline.com/tid/oauth2/v2.0/token",
data=b"grant_type=client_credentials&client_secret=secret-value",
headers={"Content-Type": "application/x-www-form-urlencoded"},
)
assert request.get_method() == "POST"
assert b"client_secret=secret-value" in request.data
with pytest.raises(urllib.error.URLError, match="must not be redirected"):
redirect_handler.redirect_request(
request,
io.BytesIO(b""),
307,
"Temporary Redirect",
status,
reason,
{},
"https://evil.example/token",
)
def test_resolve_token_azure_ad_oversized_response_returns_none(
self, monkeypatch
):
"""Oversized token metadata is rejected before JSON parsing."""
from unittest.mock import MagicMock, patch
from specify_cli._download_security import MAX_JSON_METADATA_BYTES
monkeypatch.setenv("MY_SECRET", "secret-value")
entry = AuthConfigEntry(
hosts=("dev.azure.com",), provider="azure-devops", auth="azure-ad",
tenant_id="tid", client_id="cid", client_secret_env="MY_SECRET",
)
mock_resp = MagicMock()
mock_resp.read.side_effect = io.BytesIO(
b"x" * (MAX_JSON_METADATA_BYTES + 1)
).read
mock_resp.__enter__ = lambda s: s
mock_resp.__exit__ = MagicMock(return_value=False)
mock_opener = MagicMock()
mock_opener.open.return_value = mock_resp
with patch("urllib.request.build_opener", return_value=mock_opener), patch(
"specify_cli.authentication.azure_devops._json.loads",
side_effect=AssertionError("oversized body must not be parsed"),
):
assert AzureDevOpsAuth().resolve_token(entry) is None
@pytest.mark.parametrize("payload", [[], {"access_token": None}, {"access_token": 123}])
def test_resolve_token_azure_ad_unexpected_json_shape_returns_none(
self, monkeypatch, payload
):
from unittest.mock import MagicMock, patch
monkeypatch.setenv("MY_SECRET", "secret-value")
entry = AuthConfigEntry(
hosts=("dev.azure.com",), provider="azure-devops", auth="azure-ad",
tenant_id="tid", client_id="cid", client_secret_env="MY_SECRET",
)
mock_resp = MagicMock()
mock_resp.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read
mock_resp.__enter__ = lambda s: s
mock_resp.__exit__ = MagicMock(return_value=False)
mock_opener = MagicMock()
mock_opener.open.return_value = mock_resp
with patch("urllib.request.build_opener", return_value=mock_opener):
assert AzureDevOpsAuth().resolve_token(entry) is None
def test_resolve_token_azure_ad_invalid_utf8_returns_none(self, monkeypatch):
"""azure-ad returns None when the token response is not valid UTF-8."""
from unittest.mock import MagicMock, patch
@@ -960,7 +1038,41 @@ class TestRedirectStripping:
handler.redirect_request(req, io.BytesIO(b""), 302, "Found", {},
"http://evil.example.com/archive.zip")
def test_redirect_rejects_remote_to_http_loopback(self):
@pytest.mark.parametrize(
"target",
[
"http://127.0.0.1/internal",
"https://localhost/internal",
"https://localhost./internal",
"https://service.localhost/internal",
"https://service.localhost./internal",
"https://127.0.0.2/internal",
"https://127.1/internal",
"https://2130706433/internal",
"https://0x7f000001/internal",
"https://017700000001/internal",
"https://0177.0.0.1/internal",
"https://[::1]/internal",
"https://[::1%25lo0]/internal",
"https://[::ffff:127.0.0.1]/internal",
"https://127%2e0%2e0%2e1/internal",
"https://%31%32%37.0.0.1/internal",
"https://127%2E1/internal",
"https://local%68ost/internal",
"https://[::ffff:127%2e0.0.1]/internal",
"https://[::ffff:7f00%3a1]/internal",
"https://[::ffff%3a127.0.0.1]/internal",
"https://ℓocalhost/internal",
"https://ｌｏｃａｌｈｏｓｔ/internal",
"https://127。0。0。1/internal",
"https://0.0.0.0/internal",
"https://0/internal",
"https://00.00.00.00/internal",
"https://[::]/internal",
"https://[::ffff:0.0.0.0]/internal",
],
)
def test_redirect_rejects_remote_to_loopback(self, target):
"""A remote response must not redirect a download into loopback."""
import io
import urllib.error
@@ -978,10 +1090,27 @@ class TestRedirectStripping:
302,
"Found",
{},
"http://127.0.0.1/internal",
target,
)
def test_redirect_allows_loopback_to_http_loopback(self):
@pytest.mark.parametrize(
("source", "target"),
[
(
"http://localhost:8000/archive.zip",
"http://127.0.0.1:8001/archive.zip",
),
(
"http://127.0.0.2:8000/archive.zip",
"http://127.255.255.254:8001/archive.zip",
),
(
"https://[0:0:0:0:0:0:0:1]/archive.zip",
"http://[::1]:8001/archive.zip",
),
],
)
def test_redirect_allows_loopback_to_http_loopback(self, source, target):
"""Local development may continue redirecting between loopback URLs."""
import io
from urllib.request import Request
@@ -989,18 +1118,66 @@ class TestRedirectStripping:
from specify_cli.authentication.http import _StripAuthOnRedirect
handler = _StripAuthOnRedirect(())
request = Request("http://localhost:8000/archive.zip")
request = Request(source)
redirected = handler.redirect_request(
request,
io.BytesIO(b""),
302,
"Found",
{},
"http://127.0.0.1:8001/archive.zip",
target,
)
assert redirected is not None
def test_multi_hop_remote_to_loopback_chain_is_rejected_at_first_hop(self):
import io
import urllib.error
from urllib.request import Request
from specify_cli.authentication.http import _StripAuthOnRedirect
handler = _StripAuthOnRedirect(())
request = Request("https://example.com/archive.zip")
with pytest.raises(urllib.error.URLError, match="unsafe redirect"):
handler.redirect_request(
request,
io.BytesIO(b""),
302,
"Found",
{},
"https://localhost:4443/hop",
)
@pytest.mark.parametrize(
"target",
[
"https://example.com:notaport/archive.zip",
"https://example.com:+443/archive.zip",
"https://example.com:65536/archive.zip",
],
)
def test_malformed_redirect_port_raises_urlerror(self, target):
import io
import urllib.error
from urllib.request import Request
from specify_cli.authentication.http import _StripAuthOnRedirect
handler = _StripAuthOnRedirect(())
request = Request("https://example.com/archive.zip")
with pytest.raises(urllib.error.URLError, match="malformed redirect URL"):
handler.redirect_request(
request,
io.BytesIO(b""),
302,
"Found",
{},
target,
)
def test_strict_redirect_error_describes_target_and_allowed_localhost(self):
from specify_cli.authentication.http import _StripAuthOnRedirect
from urllib.request import Request

163
tests/test_download_security.py
View File

@@ -2,10 +2,13 @@
from __future__ import annotations
import weakref
import pytest
from specify_cli._download_security import (
is_https_or_localhost_http,
is_loopback_url,
read_response_limited,
)
@@ -16,20 +19,93 @@ from specify_cli._download_security import (
("https://example.com/preset.zip", True),
("http://localhost:8000/preset.zip", True),
("http://127.0.0.1/preset.zip", True),
("http://127.0.0.2/preset.zip", True),
("http://127.255.255.254/preset.zip", True),
("http://[::1]/preset.zip", True),
("http://[0:0:0:0:0:0:0:1]/preset.zip", True),
("http://[::ffff:127.0.0.2]/preset.zip", True),
("http://[::1%25lo0]/preset.zip", True),
# Non-loopback HTTP is rejected.
("http://example.com/preset.zip", False),
# Loopback allowance is an exact-string match: 127.0.0.2 is not covered.
("http://127.0.0.2/preset.zip", False),
("http://192.0.2.1/preset.zip", False),
("http://[fe80::1]/preset.zip", False),
("http://[fe80::1%25lo0]/preset.zip", False),
("http://0.0.0.0/preset.zip", False),
("http://0/preset.zip", False),
("http://[::]/preset.zip", False),
("http://[::ffff:0.0.0.0]/preset.zip", False),
# Ambiguous/platform-dependent spellings may never authorize HTTP.
("http://127.1/preset.zip", False),
("http://2130706433/preset.zip", False),
("http://0x7f000001/preset.zip", False),
("http://017700000001/preset.zip", False),
("http://0177.0.0.1/preset.zip", False),
("http://00177.0.0.1/preset.zip", False),
("http://localhost./preset.zip", False),
("http://ℓocalhost/preset.zip", False),
("http://127。0。0。1/preset.zip", False),
# A hostname is always required, even for HTTPS.
("https:///preset.zip", False),
("https://", False),
# Invalid ports must be rejected before urllib opens the URL.
("https://example.com:notaport/preset.zip", False),
("https://example.com:+443/preset.zip", False),
("https://example.com:65536/preset.zip", False),
# urllib decodes escapes in the authority before connecting; reject
# encoded reg-names so validation and connection cannot disagree.
("https://127%2e0%2e0%2e1/preset.zip", False),
("https://%31%32%37.0.0.1/preset.zip", False),
("https://local%68ost/preset.zip", False),
("https://example.com%3a443/preset.zip", False),
("https://[::1%lo0]/preset.zip", False),
("https://[::ffff:127%2e0.0.1]/preset.zip", False),
("https://[::ffff:7f00%3a1]/preset.zip", False),
("https://[::ffff%3a127.0.0.1]/preset.zip", False),
],
)
def test_is_https_or_localhost_http(url, allowed):
assert is_https_or_localhost_http(url) is allowed
@pytest.mark.parametrize(
"url",
[
"https://localhost/internal",
"https://127.0.0.2/internal",
"https://[::1]/internal",
"https://[::1%25lo0]/internal",
"https://[::ffff:127.0.0.2]/internal",
],
)
def test_is_loopback_url_recognizes_effective_loopback_literals(url):
assert is_loopback_url(url) is True
@pytest.mark.parametrize(
"url",
[
"https://localhost./internal",
"https://service.localhost/internal",
"https://service.localhost./internal",
"https://127.1/internal",
"https://2130706433/internal",
"https://0x7f000001/internal",
"https://017700000001/internal",
"https://0177.0.0.1/internal",
"https://ℓocalhost/internal",
"https://127。0。0。1/internal",
"https://127%2e0%2e0%2e1/internal",
"https://0.0.0.0/internal",
"https://0/internal",
"https://00.00.00.00/internal",
"https://[::]/internal",
"https://[::ffff:0.0.0.0]/internal",
],
)
def test_is_loopback_url_does_not_authorize_ambiguous_spellings(url):
assert is_loopback_url(url) is False
class _Response:
"""Faithful stream stand-in: read() advances a cursor and returns b"" at EOF."""
@@ -50,6 +126,41 @@ class _Response:
return out
class _RecordingResponse(_Response):
def __init__(self, data: bytes, *, chunk: int | None = None):
super().__init__(data, chunk=chunk)
self.requested_sizes: list[int] = []
def read(self, size: int = -1) -> bytes:
self.requested_sizes.append(size)
return super().read(size)
class _TrackedChunk(bytearray):
pass
class _OneByteResponse:
"""Return distinct weak-referenceable chunks to detect retained fragments."""
def __init__(self, count: int):
self.remaining = count
self.refs: list[weakref.ReferenceType[_TrackedChunk]] = []
self.peak_live = 0
def read(self, _size: int = -1) -> bytes | _TrackedChunk:
if self.remaining == 0:
return b""
self.remaining -= 1
chunk = _TrackedChunk(b"x")
self.refs.append(weakref.ref(chunk))
self.peak_live = max(
self.peak_live,
sum(ref() is not None for ref in self.refs),
)
return chunk
def test_read_response_limited_rejects_oversized_download():
with pytest.raises(ValueError, match="exceeds maximum size"):
read_response_limited(_Response(b"abcde"), max_bytes=4)
@@ -66,3 +177,51 @@ def test_read_response_limited_enforces_bound_under_short_reads():
response = _Response(b"x" * 100, chunk=8)
with pytest.raises(ValueError, match="exceeds maximum size"):
read_response_limited(response, max_bytes=16)
def test_read_response_limited_does_not_retain_short_read_fragments():
response = _OneByteResponse(64)
assert read_response_limited(response, max_bytes=64) == b"x" * 64
assert response.peak_live <= 2
def test_read_response_limited_caps_underlying_reads_at_64_kib():
response = _RecordingResponse(b"x" * (64 * 1024 + 1))
with pytest.raises(ValueError, match="exceeds maximum size"):
read_response_limited(response, max_bytes=64 * 1024)
assert max(response.requested_sizes) <= 64 * 1024
@pytest.mark.parametrize("value", [None, "1", 1.5, True])
def test_read_response_limited_rejects_non_integer_limits(value):
with pytest.raises(TypeError, match="integer"):
read_response_limited(_Response(b""), max_bytes=value)
def test_read_response_limited_rejects_negative_limit_without_reading():
response = _RecordingResponse(b"")
with pytest.raises(ValueError, match="non-negative"):
read_response_limited(response, max_bytes=-1)
assert response.requested_sizes == []
def test_read_response_limited_allows_empty_response_at_zero_limit():
assert read_response_limited(_Response(b""), max_bytes=0) == b""
class _CustomLimitError(Exception):
pass
def test_read_response_limited_rejects_first_byte_at_zero_limit():
with pytest.raises(_CustomLimitError, match="exceeds maximum size"):
read_response_limited(
_Response(b"x"),
max_bytes=0,
error_type=_CustomLimitError,
)

1
tests/test_self_upgrade_guidance.py
View File

@@ -6,6 +6,7 @@ from specify_cli import app
from tests.self_upgrade_helpers import (
mock_urlopen_response,
route_opener_open_through_urlopen, # noqa: F401 (autouse fixture)
runner,
strip_ansi,
)

11
tests/test_upgrade.py
View File

@@ -2,11 +2,12 @@
Network isolation contract (SC-004 / FR-014): every test that exercises
`specify self check` or `_fetch_latest_release_tag()` MUST mock the outbound
urllib path it expects (`urlopen` for unauthenticated requests, `build_opener`
for authenticated requests) so no real outbound call ever reaches api.github.com.
Tests for non-network `self upgrade` behavior should keep that contract explicit
with local mocks. Run this module under `pytest-socket` (if installed) with
`--disable-socket` as an extra safety net.
urllib path so no real call reaches api.github.com. Production always uses an
isolated `build_opener`; this module's autouse fixture routes its `open()` back
through the locally mocked `urlopen`. Tests for non-network `self upgrade`
behavior should keep that contract explicit with local mocks. Run this module
under `pytest-socket` (if installed) with `--disable-socket` as an extra safety
net.
"""
import urllib.error

7
tests/test_workflows.py
View File

@@ -11744,6 +11744,10 @@ steps:
_reject_insecure_download_redirect(
"https://example.com/wf.yml", "http://localhost:8000/wf.yml"
)
with pytest.raises(urllib.error.URLError):
_reject_insecure_download_redirect(
"https://example.com/wf.yml", "https://127.0.0.2/wf.yml"
)
# Allowed: HTTPS anywhere, or loopback HTTP that stays on loopback HTTP.
_reject_insecure_download_redirect(
"https://example.com/wf.yml", "https://cdn.example.com/wf.yml"
@@ -11754,6 +11758,9 @@ steps:
_reject_insecure_download_redirect(
"http://127.0.0.1/source.yml", "http://127.0.0.1/wf.yml"
)
_reject_insecure_download_redirect(
"http://127.0.0.2/source.yml", "http://127.255.255.254/wf.yml"
)
def test_add_from_url_passes_redirect_validator(self, project_dir, monkeypatch):
from unittest.mock import patch