From 36528c723b52c0a8a4f0f66400f408392bfa3c44 Mon Sep 17 00:00:00 2001 From: Yernat Yestekov <2068106+doublewhy@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:17:26 -0700 Subject: [PATCH 1/4] fix: retry transient pinned tool downloads --- .../issue-1137-ci-release-download-retries.md | 121 +++ docs/requirements/GOV-913/requirement.md | 8 + .../python/tests/test_release_download.py | 920 ++++++++++++++++++ .../python/tests/test_repo_policy_tools.py | 37 +- tools/gitleaks_tool.py | 16 +- tools/osv_scanner_tool.py | 15 +- tools/policy/conftest_tool.py | 16 +- tools/release_download.py | 640 ++++++++++++ tools/vale_tool.py | 10 +- 9 files changed, 1750 insertions(+), 33 deletions(-) create mode 100644 docs/decisions/issue-1137-ci-release-download-retries.md create mode 100644 implementations/python/tests/test_release_download.py create mode 100644 tools/release_download.py diff --git a/docs/decisions/issue-1137-ci-release-download-retries.md b/docs/decisions/issue-1137-ci-release-download-retries.md new file mode 100644 index 00000000..e3f14c3d --- /dev/null +++ b/docs/decisions/issue-1137-ci-release-download-retries.md @@ -0,0 +1,121 @@ +# Issue 1137: Bounded CI Release-Download Retries + +Date: 2026-08-12 + +Issue: #1137. Requirement: GOV-913. + +## Context And Gap + +OpenRAE's required verification lanes acquire pinned Conftest, Vale, Gitleaks, +and OSV Scanner artifacts from their canonical GitHub Releases repositories. +The owning installers validate checksums before executing or extracting those +artifacts. During review of several independent pull requests, GitHub returned +temporary disconnects and HTTP 503 responses while acquiring all four tool +families. A single such response failed the lane before it could inspect the +pull request's code; rerunning the unchanged commit succeeded once the release +service recovered. + +The integrity checks were behaving correctly, but acquisition had no bounded +transient-failure policy. This note defines that missing reliability boundary. +It changes neither the verification graph nor the meaning of a passing gate. + +## Existing-Surface Review + +The review covered the four repository-local installers, their version pins, +checksum sources, archive extraction, cache paths, nox callers, and required CI +jobs. The installers previously used separate direct `urllib` calls. There was +no shared release-acquisition helper to extend. Runtime OCI/module downloads, +vocabulary-source refreshes, Isabelle's separately governed official-mirror +policy, and GitHub Actions artifact transfer are different trust or ownership +boundaries and remain out of scope. + +PR #1121 independently hardens OSV Scanner's cache type, identity, digest, +atomic-install, and size checks. The retry helper composes below that work: it +only returns bounded bytes from the already approved OSV release origin, while +the OSV installer remains the sole owner of cache and checksum validation. + +## Chosen Policy + +All four installers bind their existing `urlopen` seam to one internal helper. +The initial request accepts only exact HTTPS paths under these GitHub Release +families: + +- `errata-ai/vale`; +- `gitleaks/gitleaks`; +- `google/osv-scanner`; and +- `open-policy-agent/conftest`. + +Redirects are handled explicitly instead of delegated to `urllib`. A request +may follow at most three hops, and each transition must be one of the current +GitHub-controlled release paths: the exact `errata-ai/vale` to `vale-cli/vale` +repository relocation with an unchanged release suffix, or an approved GitHub +release URL to an exact `release-assets.githubusercontent.com` production-asset +path. Only that final asset URL may contain GitHub's ephemeral signed query; +the query is never included in a diagnostic, and the asset host cannot redirect +again. Relative, HTTP, cross-origin, credential-bearing, ambiguous, cyclic, +oversized, and other unapproved locations fail without retry. Redirect response +bodies are closed without being read. + +The helper performs at most three attempts. Every connection hop and every +bounded body read receives a finite 60-second socket timeout capped by the +remaining 190-second total deadline. Exponential delays are deterministic and +capped, and no read, redirect hop, sleep, or new attempt may continue after the +deadline. Declared response lengths are checked before and after streaming; +ambiguous framing and oversized bodies fail immediately, while an early EOF is +classified as a retryable incomplete transfer. Each response is bounded to +256 MiB before it is returned to an installer. There is no jitter, alternate +mirror, unpinned version, credential, or redirect-selection fallback. + +Only these failures are retryable: + +- HTTP 408 and 429; +- HTTP 500 through 599; and +- timeouts, remote disconnects, incomplete responses, and connection failures. + +`Retry-After` delta-seconds and HTTP-date values are honored, but an individual +value is capped at ten seconds and can never extend the total deadline. All +other HTTP and URL failures stop after the first attempt. Final diagnostics +include only the approved URL path, attempt count, and stable failure class; +they omit response bodies and exception text. + +The helper buffers a successful bounded response and then returns control to +the owning installer. Consequently, checksum mismatch, signature failure, +archive/type rejection, cache identity failure, and extraction failure happen +outside the retry loop and cannot cause a second download. Required lanes still +fail closed when every transient attempt fails. + +## Alternatives Rejected + +- **Retry each installer independently.** This would create four subtly + different status lists, delays, deadlines, and diagnostics that could drift. +- **Retry every `URLError` or every HTTP failure.** Certificate, DNS-policy, + authorization, missing-asset, and malformed-request failures are not evidence + of a safe transient event. Retrying them obscures configuration or trust + failures. +- **Retry after checksum or extraction failure.** A successful transfer of + invalid bytes is an integrity event, not availability noise. Retrying could + conceal unstable or malicious upstream content. +- **Use a mirror or latest-version fallback.** This would bypass the reviewed + origin, version, and digest boundary. +- **Rely on manual workflow reruns.** Manual reruns provide no deterministic + attempt/time bound and consume reviewer attention without increasing + assurance. + +## Verification And Nonclaims + +Deterministic tests cover every approved origin and redirect transition, +ambiguous URL and response-framing rejection, redirect cycles and hop bounds, +success after transient HTTP and transport failures, incomplete declared +responses, delta and HTTP-date `Retry-After`, read/backoff/deadline caps, exact +exhaustion, non-retryable HTTP and URL failures, response-size rejection, +sanitized diagnostics, and no second request after checksum or archive-type +failure. Local scripted HTTP servers verify that an early EOF never returns +partial bytes, redirect bodies are not drained, and a trickling body cannot +extend the total deadline. A binding test ensures all four installers continue +to share the helper, and clean-cache smoke tests exercise the live current +GitHub redirect chain for all four pinned tools. + +This policy does not make GitHub availability hermetic, validate remote bytes, +or replace the installers' pins, checksums, signatures, archive rules, cache +hardening, or execution gates. It only absorbs a small, explicitly classified +window of transient release-service failure. diff --git a/docs/requirements/GOV-913/requirement.md b/docs/requirements/GOV-913/requirement.md index 4b4a30cd..50738cad 100644 --- a/docs/requirements/GOV-913/requirement.md +++ b/docs/requirements/GOV-913/requirement.md @@ -25,6 +25,14 @@ Requirement inventory expansion. Reusable ecosystem assets need explicit trust a - IMPLEMENTS → SPEC `contracts/schemas/asset-trust/reusable-asset-trust-policy-v1.json` (reusable-asset-trust-policy-v1 published schema) - IMPLEMENTS → ADR `docs/decisions/adrs/adr-071-reusable-asset-trust-and-integrity-policy.md` (ADR-071: Reusable Asset Trust and Integrity Policy) - TESTS → TEST `implementations/python/tests/test_reusable_asset_trust_policy.py` (Reusable-asset trust policy contract tests) +- IMPLEMENTS → GITHUB_ISSUE `1137` (Bounded retries for pinned CI tool downloads) +- IMPLEMENTS → CODE_FILE `tools/release_download.py` (Approved-origin, bounded transient retry policy) +- IMPLEMENTS → CODE_FILE `tools/policy/conftest_tool.py` (Conftest release acquisition adapter) +- IMPLEMENTS → CODE_FILE `tools/vale_tool.py` (Vale release acquisition adapter) +- IMPLEMENTS → CODE_FILE `tools/gitleaks_tool.py` (Gitleaks release acquisition adapter) +- IMPLEMENTS → CODE_FILE `tools/osv_scanner_tool.py` (OSV Scanner release acquisition adapter) +- TESTS → TEST `implementations/python/tests/test_release_download.py` (Retry classification, bounds, and integrity non-retry regressions) +- DOCUMENTS → DOCUMENTATION `docs/decisions/issue-1137-ci-release-download-retries.md` (Release-download retry decision) - IMPLEMENTS → GITHUB_ISSUE `115` (Trust & integrity of reusable assets (GOV-913)) - IMPLEMENTS → GITHUB_ISSUE `1098` (Upgrade vulnerable Click and cryptography locks and gate OSV findings) - IMPLEMENTS → CONFIG `implementations/python/pyproject.toml` (Fixed Click and cryptography dependency floors) diff --git a/implementations/python/tests/test_release_download.py b/implementations/python/tests/test_release_download.py new file mode 100644 index 00000000..e9bdca51 --- /dev/null +++ b/implementations/python/tests/test_release_download.py @@ -0,0 +1,920 @@ +"""Adversarial tests for bounded pinned-tool release acquisition.""" + +from __future__ import annotations + +import io +import queue +import socketserver +import threading +import time +from collections.abc import Callable +from contextlib import suppress +from datetime import UTC, datetime, timedelta +from email.message import Message +from email.utils import format_datetime +from hashlib import sha256 +from http.client import IncompleteRead, RemoteDisconnected +from pathlib import Path +from tarfile import ReadError +from types import ModuleType +from urllib.error import HTTPError, URLError + +import pytest + +import tools.gitleaks_tool as gitleaks_tool +import tools.osv_scanner_tool as osv_scanner_tool +import tools.policy.conftest_tool as conftest_tool +import tools.release_download as release_download +import tools.vale_tool as vale_tool + +RELEASE_URL = "https://github.com/errata-ai/vale/releases/download/v3.15.2/vale.tar.gz" +VALE_CANONICAL_URL = "https://github.com/vale-cli/vale/releases/download/v3.15.2/vale.tar.gz" +RELEASE_ASSET_URL = ( + "https://release-assets.githubusercontent.com/github-production-release-asset/81020247/" + "6307b9db-05dd-4042-9bec-ffb9fe72e4e8?sig=ephemeral-secret" +) + + +class DownloadResponse: + def __init__( + self, + payload: bytes | BaseException, + *, + status: int | None = 200, + headers: object | None = None, + ) -> None: + self.payload = payload + self.offset = 0 + self.status = status + self.headers = {} if headers is None else headers + self.read_timeouts: list[float] = [] + + def __enter__(self) -> DownloadResponse: + return self + + def __exit__(self, *_args: object) -> None: + return None + + def getcode(self) -> int | None: + return self.status + + def read(self, limit: int = -1) -> bytes: + if isinstance(self.payload, BaseException): + raise self.payload + end = len(self.payload) if limit < 0 else self.offset + limit + chunk = self.payload[self.offset : end] + self.offset += len(chunk) + return chunk + + def read1(self, limit: int = -1) -> bytes: + return self.read(limit) + + def set_read_timeout(self, timeout: float) -> None: + self.read_timeouts.append(timeout) + + +class SequenceOpener: + def __init__(self, *results: DownloadResponse | BaseException) -> None: + self.results = list(results) + self.calls: list[tuple[str, float | None]] = [] + + def __call__(self, url: str, *, timeout: float | None = None) -> DownloadResponse: + self.calls.append((url, timeout)) + result = self.results.pop(0) + if isinstance(result, BaseException): + raise result + return result + + +class FakeClock: + def __init__(self, *, wall_time: float = 0.0) -> None: + self.elapsed = 0.0 + self.wall_start = wall_time + self.sleeps: list[float] = [] + + def monotonic(self) -> float: + return self.elapsed + + def wall_time(self) -> float: + return self.wall_start + self.elapsed + + def sleep(self, delay: float) -> None: + self.sleeps.append(delay) + self.elapsed += delay + + +class _ThreadingScriptServer(socketserver.ThreadingTCPServer): + allow_reuse_address = True + daemon_threads = True + block_on_close = False + + def __init__(self) -> None: + self.scripts: queue.Queue[list[tuple[float, bytes]]] = queue.Queue() + self.requests = 0 + super().__init__(("127.0.0.1", 0), _ScriptedResponseHandler) + + +class _ScriptedResponseHandler(socketserver.BaseRequestHandler): + def handle(self) -> None: + self.server.requests += 1 + request = b"" + while b"\r\n\r\n" not in request: + chunk = self.request.recv(4096) + if not chunk: + return + request += chunk + script = self.server.scripts.get(timeout=2) + for delay, payload in script: + if delay: + time.sleep(delay) + with suppress(OSError): + self.request.sendall(payload) + + +class ScriptedReleaseServer: + def __init__(self) -> None: + self.server = _ThreadingScriptServer() + host, port = self.server.server_address + self.base_url = f"http://{host}:{port}" + self.thread = threading.Thread(target=self.server.serve_forever, daemon=True) + + def __enter__(self) -> ScriptedReleaseServer: + self.thread.start() + return self + + def __exit__(self, *_args: object) -> None: + self.server.shutdown() + self.server.server_close() + self.thread.join(timeout=2) + + def enqueue(self, *parts: tuple[float, bytes]) -> None: + self.server.scripts.put(list(parts)) + + +def _local_response( + body: bytes, + *, + status: str = "200 OK", + content_length: int | None = None, + location: str | None = None, +) -> bytes: + length = len(body) if content_length is None else content_length + headers = [f"HTTP/1.1 {status}", f"Content-Length: {length}", "Connection: close"] + if location is not None: + headers.append(f"Location: {location}") + return ("\r\n".join((*headers, "", ""))).encode() + body + + +def _admit_local_server(monkeypatch: pytest.MonkeyPatch, server: ScriptedReleaseServer) -> None: + monkeypatch.setattr(release_download, "_approved_url", lambda url: url.startswith(server.base_url)) + monkeypatch.setattr( + release_download, + "_approved_redirect", + lambda source, target: source.startswith(server.base_url) and target.startswith(server.base_url), + ) + + +def _http_error(status: int, *, retry_after: str | None = None, body: bytes = b"secret body") -> HTTPError: + headers = Message() + if retry_after is not None: + headers["Retry-After"] = retry_after + return HTTPError(RELEASE_URL, status, "sensitive upstream detail", headers, io.BytesIO(body)) + + +def _install_network( + monkeypatch: pytest.MonkeyPatch, + opener: SequenceOpener, + clock: FakeClock | None = None, +) -> FakeClock: + active_clock = FakeClock() if clock is None else clock + monkeypatch.setattr(release_download, "_stdlib_open", opener) + monkeypatch.setattr(release_download, "_monotonic", active_clock.monotonic) + monkeypatch.setattr(release_download, "_wall_time", active_clock.wall_time) + monkeypatch.setattr(release_download, "_sleep", active_clock.sleep) + return active_clock + + +@pytest.mark.parametrize( + "url", + [ + RELEASE_URL, + "https://github.com/gitleaks/gitleaks/releases/download/v8.30.1/checksums.txt", + "https://github.com/google/osv-scanner/releases/download/v2.4.0/osv-scanner_linux_amd64", + "https://github.com/open-policy-agent/conftest/releases/download/v0.65.0/checksums.txt", + ], +) +def test_only_expected_github_release_families_are_approved(url: str) -> None: + assert release_download._approved_url(url) + + +def test_redirect_policy_admits_only_the_current_github_release_chain() -> None: + assert release_download._approved_redirect(RELEASE_URL, VALE_CANONICAL_URL) + assert release_download._approved_redirect(VALE_CANONICAL_URL, RELEASE_ASSET_URL) + assert release_download._approved_redirect( + "https://github.com/google/osv-scanner/releases/download/v2.4.0/osv-scanner_linux_amd64", + RELEASE_ASSET_URL, + ) + assert not release_download._approved_url(RELEASE_ASSET_URL) + assert "ephemeral-secret" not in release_download._display_url(RELEASE_ASSET_URL) + + +@pytest.mark.parametrize( + ("source", "target"), + [ + (RELEASE_URL, "http://release-assets.githubusercontent.com/github-production-release-asset/1/x?sig=x"), + (RELEASE_URL, "https://evil.example/github-production-release-asset/1/x?sig=x"), + ( + RELEASE_URL, + "https://release-assets.githubusercontent.com/github-production-release-asset/1/not-a-uuid?sig=x", + ), + ( + RELEASE_URL, + "https://release-assets.githubusercontent.com/github-production-release-asset/1/" + "6307b9db-05dd-4042-9bec-ffb9fe72e4e8", + ), + (RELEASE_URL, "https://github.com/vale-cli/vale/releases/download/v3.15.2/different.tar.gz"), + (RELEASE_URL, "/relative/release-asset"), + (RELEASE_ASSET_URL, RELEASE_ASSET_URL), + ], +) +def test_redirect_policy_rejects_scheme_origin_path_and_transition_escape(source: str, target: str) -> None: + assert not release_download._approved_redirect(source, target) + + +def test_redirect_policy_rejects_malformed_or_oversized_location_text() -> None: + assert not release_download._approved_url("https://github.com/\x00tool") + assert not release_download._approved_canonical_vale_url("\x00") + assert not release_download._approved_release_asset_url("x" * 20_000) + + +@pytest.mark.parametrize( + "url", + [ + "http://github.com/errata-ai/vale/releases/download/v3/vale", + "https://example.com/errata-ai/vale/releases/download/v3/vale", + "https://github.com:443/errata-ai/vale/releases/download/v3/vale", + "https://github.com:invalid/errata-ai/vale/releases/download/v3/vale", + "https://user@github.com/errata-ai/vale/releases/download/v3/vale", + "https://github.com/errata-ai/vale/releases/download/v3/vale?token=secret", + "https://github.com/errata-ai/vale/releases/download/v3/vale#fragment", + "https://github.com/errata-ai/vale/releases/download/v3\\vale", + "https://github.com/errata-ai/vale/releases/download/v3/%2e%2e/vale", + "https://github.com/errata-ai/vale/releases/download/../vale", + "https://github.com/unknown/tool/releases/download/v1/tool", + ], +) +def test_unapproved_or_ambiguous_urls_fail_before_network( + monkeypatch: pytest.MonkeyPatch, + url: str, +) -> None: + opener = SequenceOpener(AssertionError("unapproved URL reached the network")) + _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match="not an approved pinned-tool origin"): + release_download.retrying_urlopen(url) + + assert opener.calls == [] + + +@pytest.mark.parametrize( + "overrides", + [ + {"max_attempts": 0}, + {"request_timeout_seconds": 0}, + {"total_timeout_seconds": 0}, + {"maximum_response_bytes": 0}, + {"initial_backoff_seconds": -1}, + {"maximum_backoff_seconds": -1}, + {"maximum_retry_after_seconds": -1}, + {"maximum_redirects": -1}, + ], +) +def test_retry_policy_rejects_unbounded_or_negative_configuration(overrides: dict[str, int]) -> None: + with pytest.raises(ValueError): + release_download.ReleaseRetryPolicy(**overrides) + + +def test_success_uses_one_finite_request_and_returns_a_context_response( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener(DownloadResponse(b"reviewed payload")) + clock = _install_network(monkeypatch, opener) + + with release_download.retrying_urlopen(RELEASE_URL) as response: + assert response.read() == b"reviewed payload" + + assert opener.calls == [(RELEASE_URL, 60.0)] + assert clock.sleeps == [] + + +def test_stdlib_open_uses_a_default_redirect_capable_opener(monkeypatch: pytest.MonkeyPatch) -> None: + response = DownloadResponse(b"payload") + observed: dict[str, object] = {} + + class FakeOpener: + def __init__(self, *_handlers: object) -> None: + pass + + def open(self, url: str, *, timeout: float) -> DownloadResponse: + observed.update(url=url, timeout=timeout) + return response + + monkeypatch.setattr(release_download, "build_opener", FakeOpener) + + assert release_download._stdlib_open(RELEASE_URL, timeout=12) is response + assert observed == {"url": RELEASE_URL, "timeout": 12} + + +def test_http_408_429_and_5xx_retry_with_retry_after_then_backoff( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener( + DownloadResponse(b"do not use", status=503, headers={"Retry-After": "2"}), + _http_error(408), + DownloadResponse(b"reviewed payload"), + ) + clock = _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL).read() == b"reviewed payload" + + assert len(opener.calls) == 3 + assert clock.sleeps == [2.0, 2.0] + + +def test_approved_redirect_hops_do_not_read_redirect_bodies(monkeypatch: pytest.MonkeyPatch) -> None: + redirect = DownloadResponse( + b"redirect body must remain unread", + status=302, + headers={"Location": RELEASE_ASSET_URL, "Content-Length": "30"}, + ) + final = DownloadResponse(b"reviewed", headers={"Content-Length": "8"}) + opener = SequenceOpener(redirect, final) + _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL).read() == b"reviewed" + assert len(opener.calls) == 2 + assert redirect.offset == 0 + assert redirect.read_timeouts == [] + + +@pytest.mark.parametrize( + ("response", "diagnostic"), + [ + (DownloadResponse(b"", status=302), "missing or ambiguous Location"), + (DownloadResponse(b"", status=304), "unsupported redirect status 304"), + ( + DownloadResponse(b"", status=302, headers={"Location": "https://evil.example/tool"}), + "unapproved redirect target", + ), + (DownloadResponse(b"", status=302, headers={"Location": RELEASE_URL}), "redirect cycle"), + ], +) +def test_invalid_redirects_fail_once_without_reading_their_bodies( + monkeypatch: pytest.MonkeyPatch, + response: DownloadResponse, + diagnostic: str, +) -> None: + opener = SequenceOpener(response, DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match=diagnostic): + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert response.offset == 0 + assert clock.sleeps == [] + + +def test_duplicate_redirect_location_fails_closed_without_following( + monkeypatch: pytest.MonkeyPatch, +) -> None: + headers = Message() + headers["Location"] = RELEASE_ASSET_URL + headers["Location"] = "https://evil.example/asset" + response = DownloadResponse(b"not read", status=302, headers=headers) + opener = SequenceOpener(response, DownloadResponse(b"not reached")) + _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match="missing or ambiguous Location"): + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert response.offset == 0 + + +def test_redirect_hop_limit_fails_closed_before_following(monkeypatch: pytest.MonkeyPatch) -> None: + redirect = DownloadResponse(b"", status=302, headers={"Location": RELEASE_ASSET_URL}) + opener = SequenceOpener(redirect, DownloadResponse(b"not reached")) + _install_network(monkeypatch, opener) + policy = release_download.ReleaseRetryPolicy(maximum_redirects=0) + + with pytest.raises(release_download.ReleaseDownloadError, match="redirect-hop limit"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert len(opener.calls) == 1 + + +def test_http_date_retry_after_is_honored_and_capped( + monkeypatch: pytest.MonkeyPatch, +) -> None: + now = datetime(2026, 8, 12, tzinfo=UTC) + retry_at = format_datetime(now + timedelta(seconds=120), usegmt=True) + opener = SequenceOpener(_http_error(429, retry_after=retry_at), DownloadResponse(b"ok")) + clock = _install_network(monkeypatch, opener, FakeClock(wall_time=now.timestamp())) + policy = release_download.ReleaseRetryPolicy(maximum_retry_after_seconds=7) + + assert release_download.retrying_urlopen(RELEASE_URL, policy=policy).read() == b"ok" + assert clock.sleeps == [7] + + +def test_retry_after_cannot_consume_or_extend_the_total_deadline( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener(_http_error(503, retry_after="999"), DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + policy = release_download.ReleaseRetryPolicy( + total_timeout_seconds=3, + maximum_retry_after_seconds=10, + ) + + with pytest.raises(release_download.ReleaseDownloadError, match="exhausted 1 bounded attempts"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert len(opener.calls) == 1 + assert clock.sleeps == [] + + +def test_transient_exhaustion_has_an_exact_attempt_bound_and_sanitized_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener( + TimeoutError("token=first"), + TimeoutError("token=second"), + TimeoutError("token=third"), + ) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError) as raised: + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 3 + assert clock.sleeps == [1.0, 2.0] + assert str(raised.value).endswith("exhausted 3 bounded attempts: transport timeout") + assert "token=" not in str(raised.value) + + +@pytest.mark.parametrize( + "failure", + [ + URLError(TimeoutError("timeout detail")), + URLError(RemoteDisconnected("disconnect detail")), + ConnectionResetError("reset detail"), + BrokenPipeError("pipe detail"), + IncompleteRead(b"partial", 20), + ], +) +def test_transport_failures_from_open_or_read_are_retryable( + monkeypatch: pytest.MonkeyPatch, + failure: BaseException, +) -> None: + opener = SequenceOpener(DownloadResponse(failure), DownloadResponse(b"complete")) + clock = _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL).read() == b"complete" + assert len(opener.calls) == 2 + assert clock.sleeps == [1.0] + + +@pytest.mark.parametrize("status", [400, 401, 403, 404, 409, 499, 600]) +def test_non_retryable_http_statuses_fail_once_without_exposing_body( + monkeypatch: pytest.MonkeyPatch, + status: int, +) -> None: + opener = SequenceOpener(_http_error(status, retry_after="1", body=b"credential=do-not-log")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError) as raised: + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert clock.sleeps == [] + assert str(raised.value).endswith(f"failed without retry: HTTP {status}") + assert "credential" not in str(raised.value) + + +def test_non_timeout_url_error_is_not_retried_or_leaked(monkeypatch: pytest.MonkeyPatch) -> None: + opener = SequenceOpener(URLError(OSError("dns response with secret"))) + _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match="non-retryable URL error") as raised: + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert "secret" not in str(raised.value) + + +def test_response_size_failure_is_not_retried(monkeypatch: pytest.MonkeyPatch) -> None: + opener = SequenceOpener(DownloadResponse(b"oversized"), DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + policy = release_download.ReleaseRetryPolicy(maximum_response_bytes=4) + + with pytest.raises(release_download.ReleaseDownloadSizeError, match="4-byte response limit"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert len(opener.calls) == 1 + assert clock.sleeps == [] + + +def test_declared_length_early_eof_is_retried_from_the_original_url( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener( + DownloadResponse(b"partial", headers={"Content-Length": "8"}), + DownloadResponse(b"complete", headers={"Content-Length": "8"}), + ) + clock = _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL).read() == b"complete" + assert len(opener.calls) == 2 + assert clock.sleeps == [1.0] + + +@pytest.mark.parametrize( + ("headers", "diagnostic"), + [ + ({"Content-Length": "not-a-number"}, "invalid Content-Length"), + ({"Content-Length": "9" * 21}, "invalid Content-Length"), + ({"Content-Length": "4, 5"}, "conflicting Content-Length"), + ({"Content-Length": "4", "Transfer-Encoding": "chunked"}, "unsupported transfer framing"), + ({"Transfer-Encoding": "gzip"}, "unsupported transfer framing"), + ], +) +def test_ambiguous_response_framing_is_not_retried( + monkeypatch: pytest.MonkeyPatch, + headers: dict[str, str], + diagnostic: str, +) -> None: + response = DownloadResponse(b"body", headers=headers) + opener = SequenceOpener(response, DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match=diagnostic): + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert response.offset == 0 + assert clock.sleeps == [] + + +def test_duplicate_transfer_encoding_is_rejected_without_body_read( + monkeypatch: pytest.MonkeyPatch, +) -> None: + headers = Message() + headers["Transfer-Encoding"] = "chunked" + headers["Transfer-Encoding"] = "chunked" + response = DownloadResponse(b"body", headers=headers) + opener = SequenceOpener(response, DownloadResponse(b"not reached")) + _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match="unsupported transfer framing"): + release_download.retrying_urlopen(RELEASE_URL) + + assert len(opener.calls) == 1 + assert response.offset == 0 + + +def test_chunked_response_without_content_length_uses_the_bounded_reader( + monkeypatch: pytest.MonkeyPatch, +) -> None: + opener = SequenceOpener(DownloadResponse(b"body", headers={"Transfer-Encoding": "chunked"})) + _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL).read() == b"body" + + +def test_response_without_deadline_capable_transport_fails_closed() -> None: + class NoDeadlineControl: + headers: dict[str, str] = {} + + def read1(self, _size: int) -> bytes: + return b"" + + with pytest.raises(release_download.ReleaseDownloadError, match="cannot enforce the download deadline"): + release_download._read_response_body( + NoDeadlineControl(), + deadline=time.monotonic() + 1, + request_timeout=1, + maximum_bytes=1, + ) + + +def test_bounded_reader_falls_back_to_read_and_rejects_invalid_readers() -> None: + class ReadOnly: + def read(self, _size: int) -> bytes: + return b"ok" + + class NonBytesReader: + def read1(self, _size: int) -> str: + return "not bytes" + + class OverBoundReader: + def read1(self, size: int) -> bytes: + return b"x" * (size + 1) + + assert release_download._read_chunk(ReadOnly(), 2) == b"ok" + with pytest.raises(release_download.ReleaseDownloadError, match="not readable"): + release_download._read_chunk(object(), 2) + with pytest.raises(release_download.ReleaseDownloadError, match="did not return bytes"): + release_download._read_chunk(NonBytesReader(), 2) + with pytest.raises(release_download.ReleaseDownloadError, match="requested read bound"): + release_download._read_chunk(OverBoundReader(), 2) + + +def test_oversized_declared_length_fails_before_body_read(monkeypatch: pytest.MonkeyPatch) -> None: + response = DownloadResponse(b"body", headers={"Content-Length": "5"}) + opener = SequenceOpener(response) + _install_network(monkeypatch, opener) + policy = release_download.ReleaseRetryPolicy(maximum_response_bytes=4) + + with pytest.raises(release_download.ReleaseDownloadSizeError, match="declared 5 bytes"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert response.offset == 0 + + +def test_trickle_body_cannot_extend_the_total_deadline(monkeypatch: pytest.MonkeyPatch) -> None: + clock = FakeClock() + + class TrickleResponse(DownloadResponse): + def read1(self, limit: int = -1) -> bytes: + clock.elapsed += 0.06 + return super().read1(min(limit, 1)) + + opener = SequenceOpener(TrickleResponse(b"ab", headers={"Content-Length": "2"})) + _install_network(monkeypatch, opener, clock) + policy = release_download.ReleaseRetryPolicy(max_attempts=1, total_timeout_seconds=0.1) + + with pytest.raises(release_download.ReleaseDownloadError, match="transport timeout"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert len(opener.calls) == 1 + assert clock.elapsed == pytest.approx(0.12) + + +def test_header_read_cannot_cross_the_total_deadline_and_follow_redirect( + monkeypatch: pytest.MonkeyPatch, +) -> None: + clock = FakeClock() + response = DownloadResponse( + b"not read", + status=302, + headers={"Location": RELEASE_ASSET_URL}, + ) + + class DelayedOpener(SequenceOpener): + def __call__(self, url: str, *, timeout: float | None = None) -> DownloadResponse: + result = super().__call__(url, timeout=timeout) + clock.elapsed += 0.11 + return result + + opener = DelayedOpener(response, DownloadResponse(b"not reached")) + _install_network(monkeypatch, opener, clock) + policy = release_download.ReleaseRetryPolicy( + max_attempts=1, + request_timeout_seconds=1, + total_timeout_seconds=0.1, + ) + + with pytest.raises(release_download.ReleaseDownloadError, match="transport timeout"): + release_download.retrying_urlopen(RELEASE_URL, policy=policy) + + assert len(opener.calls) == 1 + assert response.offset == 0 + + +def test_local_server_early_eof_retries_and_never_returns_partial_bytes( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + server.enqueue((0, _local_response(b"partial", content_length=8))) + server.enqueue((0, _local_response(b"complete", content_length=8))) + policy = release_download.ReleaseRetryPolicy( + max_attempts=2, + request_timeout_seconds=0.5, + total_timeout_seconds=1, + initial_backoff_seconds=0, + maximum_backoff_seconds=0, + ) + + assert release_download.retrying_urlopen(f"{server.base_url}/asset", policy=policy).read() == b"complete" + assert server.server.requests == 2 + + +def test_local_server_redirect_body_is_not_drained_before_next_hop( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + final_url = f"{server.base_url}/final" + redirect_headers = _local_response( + b"", + status="302 Found", + content_length=1_000_000, + location=final_url, + ) + server.enqueue((0, redirect_headers), (1, b"x" * 1024)) + server.enqueue((0, _local_response(b"final"))) + policy = release_download.ReleaseRetryPolicy( + request_timeout_seconds=0.25, + total_timeout_seconds=1, + ) + + result = release_download.retrying_urlopen(f"{server.base_url}/redirect", policy=policy).read() + + assert result == b"final" + assert server.server.requests == 2 + + +def test_local_server_trickle_read_is_cut_off_by_total_deadline( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + server.enqueue( + (0, _local_response(b"a", content_length=3)), + (0.08, b"b"), + (0.08, b"c"), + ) + policy = release_download.ReleaseRetryPolicy( + max_attempts=1, + request_timeout_seconds=1, + total_timeout_seconds=0.12, + ) + + with pytest.raises(release_download.ReleaseDownloadError, match="transport timeout"): + release_download.retrying_urlopen(f"{server.base_url}/trickle", policy=policy) + + assert server.server.requests == 1 + + +def test_invalid_per_call_timeout_fails_before_network(monkeypatch: pytest.MonkeyPatch) -> None: + opener = SequenceOpener(AssertionError("invalid timeout reached the network")) + _install_network(monkeypatch, opener) + + with pytest.raises(ValueError, match="timeout must be positive"): + release_download.retrying_urlopen(RELEASE_URL, timeout=0) + + assert opener.calls == [] + + +def test_retry_after_parser_rejects_ambiguous_values() -> None: + assert release_download._retry_after_seconds({}, now=0) is None + assert release_download._retry_after_seconds(object(), now=0) is None + assert release_download._retry_after_seconds({"Retry-After": object()}, now=0) is None + assert release_download._retry_after_seconds({"Retry-After": "x" * 129}, now=0) is None + assert release_download._retry_after_seconds({"Retry-After": "not-a-date"}, now=0) is None + assert release_download._retry_after_seconds({"Retry-After": "7"}, now=0) == 7 + naive_date = "Wed, 12 Aug 2026 00:00:00" + expected = datetime(2026, 8, 12, tzinfo=UTC).timestamp() + assert release_download._retry_after_seconds({"Retry-After": naive_date}, now=0) == expected + + +def test_response_status_falls_back_to_getcode_or_none() -> None: + class GetCodeOnly: + def getcode(self) -> int: + return 204 + + assert release_download._response_status(GetCodeOnly()) == 204 + assert release_download._response_status(object()) is None + + +def test_unknown_failure_kind_has_a_stable_fallback() -> None: + assert release_download._failure_kind(ValueError("sensitive detail")) == "download failure" + + +def test_expired_total_deadline_makes_no_network_attempt(monkeypatch: pytest.MonkeyPatch) -> None: + opener = SequenceOpener(AssertionError("expired deadline reached the network")) + _install_network(monkeypatch, opener) + moments = iter((0.0, release_download.DEFAULT_RELEASE_RETRY_POLICY.total_timeout_seconds)) + monkeypatch.setattr(release_download, "_monotonic", lambda: next(moments)) + + with pytest.raises(release_download.ReleaseDownloadError, match="exhausted 0 bounded attempts: total timeout"): + release_download.retrying_urlopen(RELEASE_URL) + + assert opener.calls == [] + + +def test_all_checksum_verified_release_installers_share_the_retry_boundary() -> None: + assert conftest_tool.urlopen is release_download.retrying_urlopen + assert vale_tool.urlopen is release_download.retrying_urlopen + assert gitleaks_tool.urlopen is release_download.retrying_urlopen + assert osv_scanner_tool.urlopen is release_download.retrying_urlopen + + +@pytest.mark.parametrize( + ("tool", "ensure", "diagnostic"), + [ + (conftest_tool, conftest_tool.ensure_conftest, "failed to download conftest"), + (vale_tool, vale_tool.ensure_vale, "failed to download Vale"), + (gitleaks_tool, gitleaks_tool.ensure_gitleaks, "failed to download gitleaks"), + (osv_scanner_tool, osv_scanner_tool.ensure_osv_scanner, "failed to download osv-scanner"), + ], +) +@pytest.mark.parametrize( + "detail", + [ + "exhausted 3 bounded attempts: HTTP 503", + "failed without retry: HTTP 404", + ], +) +def test_installers_wrap_retry_failures_with_tool_specific_diagnostics( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + tool: ModuleType, + ensure: Callable[[Path], Path], + diagnostic: str, + detail: str, +) -> None: + calls = 0 + + def fail(_url: str, **_kwargs: object) -> None: + nonlocal calls + calls += 1 + raise release_download.ReleaseDownloadError(detail) + + monkeypatch.setattr(tool, "urlopen", fail) + + with pytest.raises(RuntimeError, match=diagnostic) as raised: + ensure(tmp_path) + + assert calls == 1 + assert detail in str(raised.value) + + +@pytest.mark.parametrize( + ("tool", "ensure", "asset_name", "diagnostic"), + [ + ( + conftest_tool, + conftest_tool.ensure_conftest, + conftest_tool._release_asset_name, + "failed to download conftest from", + ), + ( + gitleaks_tool, + gitleaks_tool.ensure_gitleaks, + gitleaks_tool._release_asset_name, + "failed to download gitleaks from", + ), + ], +) +def test_checksum_metadata_installers_wrap_asset_stage_exhaustion( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + tool: ModuleType, + ensure: Callable[[Path], Path], + asset_name: Callable[[], str], + diagnostic: str, +) -> None: + checksum_metadata = f"{'0' * 64} {asset_name()}\n".encode() + detail = "exhausted 3 bounded attempts: HTTP 503" + opener = SequenceOpener( + DownloadResponse(checksum_metadata), + release_download.ReleaseDownloadError(detail), + ) + monkeypatch.setattr(tool, "urlopen", opener) + + with pytest.raises(RuntimeError, match=diagnostic) as raised: + ensure(tmp_path) + + assert len(opener.calls) == 2 + assert detail in str(raised.value) + + +def test_checksum_mismatch_is_not_retried(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + opener = SequenceOpener(DownloadResponse(b"corrupt archive"), DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(RuntimeError, match="Vale checksum mismatch"): + vale_tool.ensure_vale(tmp_path) + + assert len(opener.calls) == 1 + assert clock.sleeps == [] + + +def test_archive_type_failure_after_valid_digest_is_not_retried( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + payload = b"not a tar archive" + asset_name = vale_tool._release_asset_name() + monkeypatch.setattr(vale_tool, "VALE_ARCHIVE_SHA256", {asset_name: sha256(payload).hexdigest()}) + opener = SequenceOpener(DownloadResponse(payload), DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(ReadError, match="not a gzip file"): + vale_tool.ensure_vale(tmp_path) + + assert len(opener.calls) == 1 + assert clock.sleeps == [] diff --git a/implementations/python/tests/test_repo_policy_tools.py b/implementations/python/tests/test_repo_policy_tools.py index e1c40e0d..28cfa6a9 100644 --- a/implementations/python/tests/test_repo_policy_tools.py +++ b/implementations/python/tests/test_repo_policy_tools.py @@ -2,6 +2,7 @@ import importlib.util import inspect +import io import json import os import shutil @@ -2352,7 +2353,7 @@ def test_osv_scanner_valid_cache_hit_rehashes_without_network( binary.chmod(0o755) monkeypatch.setattr( osv_scanner_tool, - "download_bytes", + "urlopen", lambda _url, **_kwargs: pytest.fail("network used for valid cache"), ) @@ -2376,7 +2377,7 @@ def test_osv_scanner_invalid_file_cache_is_reacquired_atomically( else: binary.write_bytes(payload if cache_kind == "non-executable" else b"tampered") binary.chmod(0o644 if cache_kind == "non-executable" else 0o755) - monkeypatch.setattr(osv_scanner_tool, "download_bytes", lambda _url, **_kwargs: payload) + monkeypatch.setattr(osv_scanner_tool, "urlopen", lambda _url, **_kwargs: io.BytesIO(payload)) installed = osv_scanner_tool.ensure_osv_scanner(tmp_path) @@ -2525,18 +2526,16 @@ def test_osv_scanner_download_has_a_finite_timeout(monkeypatch: pytest.MonkeyPat _pin_fake_osv_download(monkeypatch, payload) observed: dict[str, object] = {} - def download(url: str, **kwargs: object) -> bytes: + def download(url: str, **kwargs: object) -> io.BytesIO: observed.update(url=url, **kwargs) - return payload + return io.BytesIO(payload) - monkeypatch.setattr(osv_scanner_tool, "download_bytes", download) + monkeypatch.setattr(osv_scanner_tool, "urlopen", download) assert osv_scanner_tool.ensure_osv_scanner(tmp_path).read_bytes() == payload assert observed == { "url": "https://github.com/google/osv-scanner/releases/download/v2.4.0/osv-scanner_darwin_arm64", - "description": "osv-scanner", - "timeout_seconds": 60, - "max_bytes": 256 * 1024 * 1024, + "timeout": 60, } @@ -2549,7 +2548,7 @@ def test_osv_scanner_download_rejects_an_untrusted_release_url( monkeypatch.setattr(osv_scanner_tool, "_release_base_url", lambda _version: "file:///tmp") monkeypatch.setattr( osv_scanner_tool, - "download_bytes", + "urlopen", lambda _url, **_kwargs: pytest.fail("unsafe URL reached the network client"), ) @@ -2561,11 +2560,13 @@ def test_osv_scanner_download_timeout_fails_closed(monkeypatch: pytest.MonkeyPat payload = b"reviewed-scanner" _pin_fake_osv_download(monkeypatch, payload) - def timeout(_url: str, **kwargs: object) -> bytes: - assert kwargs["timeout_seconds"] == 60 - raise RuntimeError("failed to download osv-scanner after 5 attempts") + def timeout(_url: str, **kwargs: object) -> io.BytesIO: + assert kwargs["timeout"] == 60 + raise osv_scanner_tool.ReleaseDownloadError( + "release download exhausted 3 bounded attempts" + ) - monkeypatch.setattr(osv_scanner_tool, "download_bytes", timeout) + monkeypatch.setattr(osv_scanner_tool, "urlopen", timeout) with pytest.raises(RuntimeError, match="failed to download osv-scanner"): osv_scanner_tool.ensure_osv_scanner(tmp_path) @@ -2580,7 +2581,7 @@ def test_osv_scanner_unpinned_version_and_download_mismatch_fail_closed( with pytest.raises(RuntimeError, match="no repository-pinned checksum"): osv_scanner_tool.ensure_osv_scanner(tmp_path, version="9.9.9") - monkeypatch.setattr(osv_scanner_tool, "download_bytes", lambda _url, **_kwargs: b"different") + monkeypatch.setattr(osv_scanner_tool, "urlopen", lambda _url, **_kwargs: io.BytesIO(b"different")) with pytest.raises(RuntimeError, match="checksum mismatch"): osv_scanner_tool.ensure_osv_scanner(tmp_path) assert not osv_scanner_tool.osv_scanner_binary_path(tmp_path).exists() @@ -2590,7 +2591,7 @@ def test_osv_scanner_oversized_download_is_rejected(monkeypatch: pytest.MonkeyPa payload = b"four" _pin_fake_osv_download(monkeypatch, payload) monkeypatch.setattr(osv_scanner_tool, "_MAX_BINARY_BYTES", 3) - monkeypatch.setattr(osv_scanner_tool, "download_bytes", lambda _url, **_kwargs: payload) + monkeypatch.setattr(osv_scanner_tool, "urlopen", lambda _url, **_kwargs: io.BytesIO(payload)) with pytest.raises(RuntimeError, match="exceeds the download limit"): osv_scanner_tool.ensure_osv_scanner(tmp_path) @@ -2604,11 +2605,11 @@ def test_osv_scanner_concurrent_acquisition_publishes_only_complete_bytes( _pin_fake_osv_download(monkeypatch, payload) barrier = threading.Barrier(2, timeout=3) - def concurrent_download(_url: str, **_kwargs: object) -> bytes: + def concurrent_download(_url: str, **_kwargs: object) -> io.BytesIO: barrier.wait() - return payload + return io.BytesIO(payload) - monkeypatch.setattr(osv_scanner_tool, "download_bytes", concurrent_download) + monkeypatch.setattr(osv_scanner_tool, "urlopen", concurrent_download) results: list[Path] = [] failures: list[BaseException] = [] diff --git a/tools/gitleaks_tool.py b/tools/gitleaks_tool.py index 41ae7f35..6e128c7b 100644 --- a/tools/gitleaks_tool.py +++ b/tools/gitleaks_tool.py @@ -7,8 +7,10 @@ import tempfile from hashlib import sha256 from pathlib import Path +from urllib.error import URLError -from tools.http_download import download_bytes +from tools.release_download import ReleaseDownloadError +from tools.release_download import retrying_urlopen as urlopen from tools.tool_versions import GITLEAKS_VERSION REPO_ROOT = Path(__file__).resolve().parents[1] @@ -60,7 +62,11 @@ def ensure_gitleaks(repo_root: Path = REPO_ROOT, *, version: str = GITLEAKS_VERS asset_url = f"{base_url}/{asset_name}" checksums_url = f"{base_url}/{_checksums_asset_name(version)}" - checksums_text = download_bytes(checksums_url, description="gitleaks checksums").decode("utf-8") + try: + with urlopen(checksums_url) as response: # noqa: S310 - pinned HTTPS release asset + checksums_text = response.read().decode("utf-8") + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download gitleaks checksums from {checksums_url}: {exc}") from exc expected_checksum = None for line in checksums_text.splitlines(): @@ -71,7 +77,11 @@ def ensure_gitleaks(repo_root: Path = REPO_ROOT, *, version: str = GITLEAKS_VERS if not expected_checksum: raise RuntimeError(f"missing checksum for gitleaks asset {asset_name}") - archive_bytes = download_bytes(asset_url, description="gitleaks") + try: + with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + archive_bytes = response.read() + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download gitleaks from {asset_url}: {exc}") from exc actual_checksum = sha256(archive_bytes).hexdigest() if actual_checksum != expected_checksum: diff --git a/tools/osv_scanner_tool.py b/tools/osv_scanner_tool.py index 1373f04d..1cd83756 100644 --- a/tools/osv_scanner_tool.py +++ b/tools/osv_scanner_tool.py @@ -8,8 +8,10 @@ from enum import StrEnum from hashlib import sha256 from pathlib import Path +from urllib.error import URLError -from tools.http_download import download_bytes +from tools.release_download import ReleaseDownloadError +from tools.release_download import retrying_urlopen as urlopen from tools.tool_versions import OSV_SCANNER_VERSION REPO_ROOT = Path(__file__).resolve().parents[1] @@ -192,12 +194,11 @@ def ensure_osv_scanner(repo_root: Path = REPO_ROOT, *, version: str = OSV_SCANNE if not asset_url.startswith("https://github.com/google/osv-scanner/releases/download/"): raise RuntimeError(f"unsafe osv-scanner release URL: {asset_url}") - binary_bytes = download_bytes( - asset_url, - description="osv-scanner", - timeout_seconds=_DOWNLOAD_TIMEOUT_SECONDS, - max_bytes=_MAX_BINARY_BYTES, - ) + try: + with urlopen(asset_url, timeout=_DOWNLOAD_TIMEOUT_SECONDS) as response: + binary_bytes = response.read() + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download osv-scanner from {asset_url}: {exc}") from exc if len(binary_bytes) > _MAX_BINARY_BYTES: raise RuntimeError(f"osv-scanner asset {asset_name} exceeds the download limit") diff --git a/tools/policy/conftest_tool.py b/tools/policy/conftest_tool.py index c8190ff9..b548814d 100644 --- a/tools/policy/conftest_tool.py +++ b/tools/policy/conftest_tool.py @@ -9,8 +9,10 @@ import tempfile from hashlib import sha256 from pathlib import Path +from urllib.error import URLError -from tools.http_download import download_bytes +from tools.release_download import ReleaseDownloadError +from tools.release_download import retrying_urlopen as urlopen from ..tool_versions import CONTFEST_VERSION from .common import REPO_ROOT, PolicyFailure @@ -56,7 +58,11 @@ def ensure_conftest(repo_root: Path = REPO_ROOT, *, version: str = CONTFEST_VERS asset_url = f"{base_url}/{asset_name}" checksums_url = f"{base_url}/checksums.txt" - checksums_text = download_bytes(checksums_url, description="conftest checksums").decode("utf-8") + try: + with urlopen(checksums_url) as response: # noqa: S310 - pinned HTTPS release asset + checksums_text = response.read().decode("utf-8") + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download conftest checksums from {checksums_url}: {exc}") from exc expected_checksum = None for line in checksums_text.splitlines(): @@ -67,7 +73,11 @@ def ensure_conftest(repo_root: Path = REPO_ROOT, *, version: str = CONTFEST_VERS if not expected_checksum: raise RuntimeError(f"missing checksum for conftest asset {asset_name}") - archive_bytes = download_bytes(asset_url, description="conftest") + try: + with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + archive_bytes = response.read() + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download conftest from {asset_url}: {exc}") from exc actual_checksum = sha256(archive_bytes).hexdigest() if actual_checksum != expected_checksum: diff --git a/tools/release_download.py b/tools/release_download.py new file mode 100644 index 00000000..6c0c69f1 --- /dev/null +++ b/tools/release_download.py @@ -0,0 +1,640 @@ +"""Bounded acquisition for checksum-pinned GitHub Release tooling.""" + +from __future__ import annotations + +import io +import re +import time +from collections.abc import Callable +from dataclasses import dataclass +from datetime import UTC +from email.utils import parsedate_to_datetime +from http.client import IncompleteRead, RemoteDisconnected +from typing import BinaryIO +from urllib.error import HTTPError, URLError +from urllib.parse import SplitResult, urlsplit +from urllib.request import HTTPRedirectHandler, build_opener + +_APPROVED_RELEASE_PATH_PREFIXES = ( + "/errata-ai/vale/releases/download/", + "/gitleaks/gitleaks/releases/download/", + "/google/osv-scanner/releases/download/", + "/open-policy-agent/conftest/releases/download/", +) +_VALE_LEGACY_PATH_PREFIX = "/errata-ai/vale/releases/download/" +_VALE_CANONICAL_PATH_PREFIX = "/vale-cli/vale/releases/download/" +_RELEASE_ASSET_PATH = re.compile( + r"^/github-production-release-asset/[0-9]+/" + r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$", + re.IGNORECASE, +) +_REDIRECT_STATUSES = frozenset({301, 302, 303, 307, 308}) +_MAX_REDIRECT_LOCATION_CHARS = 16 * 1024 +_READ_CHUNK_BYTES = 64 * 1024 + + +class ReleaseDownloadError(RuntimeError): + """A fail-closed release acquisition failure with a stable diagnostic.""" + + +class ReleaseDownloadSizeError(ReleaseDownloadError): + """A release response exceeded its repository-defined byte bound.""" + + +@dataclass(frozen=True) +class ReleaseRetryPolicy: + """Deterministic request, retry, and response bounds for release assets.""" + + max_attempts: int = 3 + request_timeout_seconds: float = 60.0 + total_timeout_seconds: float = 190.0 + initial_backoff_seconds: float = 1.0 + maximum_backoff_seconds: float = 4.0 + maximum_retry_after_seconds: float = 10.0 + maximum_response_bytes: int = 256 * 1024 * 1024 + maximum_redirects: int = 3 + + def __post_init__(self) -> None: + if self.max_attempts < 1: + raise ValueError("max_attempts must be positive") + positive_values = ( + self.request_timeout_seconds, + self.total_timeout_seconds, + self.maximum_response_bytes, + ) + if any(value <= 0 for value in positive_values): + raise ValueError( + "request, total-time, and response bounds must be positive" + ) + delay_values = ( + self.initial_backoff_seconds, + self.maximum_backoff_seconds, + self.maximum_retry_after_seconds, + ) + if any(value < 0 for value in delay_values): + raise ValueError("retry delays must not be negative") + if self.maximum_redirects < 0: + raise ValueError("maximum_redirects must not be negative") + + +DEFAULT_RELEASE_RETRY_POLICY = ReleaseRetryPolicy() + +_monotonic = time.monotonic +_sleep = time.sleep +_wall_time = time.time + + +class _ReturnedHttpError(Exception): + """An HTTP error response returned by a non-standard opener.""" + + def __init__(self, status: int, headers: object) -> None: + self.status = status + self.headers = headers + + +@dataclass(frozen=True) +class _RedirectHop: + location: str + + +class _NoRedirectHandler(HTTPRedirectHandler): + """Return redirect headers without following or draining their bodies.""" + + def _return_response( + self, + _request: object, + response: BinaryIO, + _code: int, + _message: str, + _headers: object, + ) -> BinaryIO: + return response + + http_error_301 = _return_response + http_error_302 = _return_response + http_error_303 = _return_response + http_error_307 = _return_response + http_error_308 = _return_response + + +@dataclass(frozen=True) +class _AttemptFailure: + kind: str + retryable: bool + retry_after_seconds: float | None + + +def _parsed_url_with_port(url: str) -> tuple[SplitResult, int | None] | None: + if ( + not isinstance(url, str) + or len(url) > _MAX_REDIRECT_LOCATION_CHARS + or any(ord(char) < 32 for char in url) + ): + return None + try: + parsed = urlsplit(url) + port = parsed.port + except (TypeError, ValueError): + return None + return parsed, port + + +def _approved_authority(parsed: SplitResult, port: int | None) -> bool: + return not ( + parsed.scheme != "https" + or parsed.hostname != "github.com" + or port is not None + or parsed.username is not None + or parsed.password is not None + or parsed.query + or parsed.fragment + ) + + +def _safe_release_path(path: str) -> bool: + path_parts = path.split("/") + normalized_path = path.lower() + return ( + "\\" not in path + and "%" not in normalized_path + and all(part not in {".", ".."} for part in path_parts) + ) + + +def _approved_release_path(path: str) -> bool: + return _safe_release_path(path) and any( + path.startswith(prefix) for prefix in _APPROVED_RELEASE_PATH_PREFIXES + ) + + +def _approved_url(url: str) -> bool: + parsed_with_port = _parsed_url_with_port(url) + if parsed_with_port is None: + return False + parsed, port = parsed_with_port + return _approved_authority(parsed, port) and _approved_release_path(parsed.path) + + +def _approved_canonical_vale_url(url: str) -> bool: + parsed_with_port = _parsed_url_with_port(url) + if parsed_with_port is None: + return False + parsed, port = parsed_with_port + return ( + _approved_authority(parsed, port) + and _safe_release_path(parsed.path) + and parsed.path.startswith(_VALE_CANONICAL_PATH_PREFIX) + ) + + +def _approved_release_asset_url(url: str) -> bool: + parsed_with_port = _parsed_url_with_port(url) + if parsed_with_port is None: + return False + parsed, port = parsed_with_port + return ( + parsed.scheme == "https" + and parsed.hostname == "release-assets.githubusercontent.com" + and port is None + and parsed.username is None + and parsed.password is None + and not parsed.fragment + and bool(parsed.query) + and _RELEASE_ASSET_PATH.fullmatch(parsed.path) is not None + ) + + +def _approved_vale_relocation(source_url: str, target_url: str) -> bool: + source = urlsplit(source_url) + target = urlsplit(target_url) + if not source.path.startswith(_VALE_LEGACY_PATH_PREFIX): + return False + source_suffix = source.path.removeprefix("/errata-ai/vale") + target_suffix = target.path.removeprefix("/vale-cli/vale") + return _approved_canonical_vale_url(target_url) and source_suffix == target_suffix + + +def _approved_redirect(source_url: str, target_url: str) -> bool: + if _approved_vale_relocation(source_url, target_url): + return True + source_is_github_release = _approved_url( + source_url + ) or _approved_canonical_vale_url(source_url) + return source_is_github_release and _approved_release_asset_url(target_url) + + +def _display_url(url: str) -> str: + parsed = urlsplit(url) + return f"{parsed.scheme}://{parsed.hostname or 'unapproved-host'}{parsed.path}" + + +def _response_status(response: object) -> int | None: + status = getattr(response, "status", None) + if status is None: + getcode = getattr(response, "getcode", None) + status = getcode() if callable(getcode) else None + return status if isinstance(status, int) else None + + +def _header_value(headers: object, name: str) -> str | None: + get = getattr(headers, "get", None) + if not callable(get): + return None + value = get(name) + return value if isinstance(value, str) else None + + +def _http_date_delay(value: str, *, now: float) -> float | None: + try: + parsed = parsedate_to_datetime(value) + except (TypeError, ValueError, OverflowError): + return None + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=UTC) + return max(0.0, parsed.timestamp() - now) + + +def _retry_after_seconds(headers: object, *, now: float) -> float | None: + value = _header_value(headers, "Retry-After") + if value is None or len(value) > 128: + return None + value = value.strip() + if value.isascii() and value.isdecimal(): + return float(value) + return _http_date_delay(value, now=now) + + +def _http_status(exc: BaseException) -> int | None: + if isinstance(exc, HTTPError): + return exc.code + if isinstance(exc, _ReturnedHttpError): + return exc.status + return None + + +def _http_headers(exc: BaseException) -> object | None: + if isinstance(exc, HTTPError): + return exc.headers + if isinstance(exc, _ReturnedHttpError): + return exc.headers + return None + + +def _transport_reason(exc: BaseException) -> BaseException: + if isinstance(exc, URLError) and isinstance(exc.reason, BaseException): + return exc.reason + return exc + + +def _is_retryable(exc: BaseException) -> bool: + status = _http_status(exc) + if status is not None: + return status in {408, 429} or 500 <= status <= 599 + reason = _transport_reason(exc) + return isinstance( + reason, + (TimeoutError, ConnectionError, IncompleteRead), + ) + + +def _failure_kind(exc: BaseException) -> str: + status = _http_status(exc) + if status is not None: + kind = f"HTTP {status}" + else: + reason = _transport_reason(exc) + kind = "download failure" + if isinstance(reason, TimeoutError): + kind = "transport timeout" + elif isinstance(reason, RemoteDisconnected): + kind = "remote disconnect" + elif isinstance(reason, IncompleteRead): + kind = "incomplete response" + elif isinstance(reason, ConnectionError): + kind = "connection failure" + elif isinstance(exc, URLError): + kind = "non-retryable URL error" + return kind + + +def _classified_failure(exc: BaseException) -> _AttemptFailure: + headers = _http_headers(exc) + retry_after = ( + None if headers is None else _retry_after_seconds(headers, now=_wall_time()) + ) + return _AttemptFailure( + kind=_failure_kind(exc), + retryable=_is_retryable(exc), + retry_after_seconds=retry_after, + ) + + +def _retry_delay( + failure: _AttemptFailure, *, failed_attempt: int, policy: ReleaseRetryPolicy +) -> float: + if failure.retry_after_seconds is not None: + return min(failure.retry_after_seconds, policy.maximum_retry_after_seconds) + exponential = policy.initial_backoff_seconds * (2 ** (failed_attempt - 1)) + return min(exponential, policy.maximum_backoff_seconds) + + +def _stdlib_open(url: str, *, timeout: float) -> BinaryIO: + return build_opener(_NoRedirectHandler()).open(url, timeout=timeout) + + +def _remaining_timeout(*, deadline: float, request_timeout: float) -> float: + remaining = deadline - _monotonic() + if remaining <= 0: + raise TimeoutError("release download deadline expired") + return min(request_timeout, remaining) + + +def _header_values(headers: object, name: str) -> tuple[str, ...]: + get_all = getattr(headers, "get_all", None) + if callable(get_all): + values = get_all(name, []) + return tuple(value for value in values if isinstance(value, str)) + value = _header_value(headers, name) + return () if value is None else (value,) + + +def _declared_content_length(headers: object) -> int | None: + tokens = tuple( + token.strip() + for value in _header_values(headers, "Content-Length") + for token in value.split(",") + ) + if not tokens: + return None + if any( + len(token) > 20 or not token.isascii() or not token.isdecimal() + for token in tokens + ): + raise ReleaseDownloadError("release response has an invalid Content-Length") + lengths = {int(token) for token in tokens} + if len(lengths) != 1: + raise ReleaseDownloadError( + "release response has conflicting Content-Length values" + ) + return lengths.pop() + + +def _validate_transfer_encoding(headers: object, declared_length: int | None) -> None: + tokens = tuple( + token.strip().lower() + for value in _header_values(headers, "Transfer-Encoding") + for token in value.split(",") + ) + if not tokens: + return + if tokens != ("chunked",) or declared_length is not None: + raise ReleaseDownloadError( + "release response has an unsupported transfer framing" + ) + + +def _response_timeout_setter(response: object) -> Callable[[float], object] | None: + explicit_setter = getattr(response, "set_read_timeout", None) + if callable(explicit_setter): + return explicit_setter + buffered = getattr(response, "fp", None) + raw = getattr(buffered, "raw", None) + transport = getattr(raw, "_sock", None) + setter = getattr(transport, "settimeout", None) + return setter if callable(setter) else None + + +def _set_response_timeout(response: object, timeout: float) -> None: + setter = _response_timeout_setter(response) + if setter is None: + raise ReleaseDownloadError( + "release response transport cannot enforce the download deadline" + ) + setter(timeout) + + +def _read_chunk(response: object, size: int) -> bytes: + read = getattr(response, "read1", None) + if not callable(read): + read = getattr(response, "read", None) + if not callable(read): + raise ReleaseDownloadError("release response body is not readable") + chunk = read(size) + if not isinstance(chunk, bytes): + raise ReleaseDownloadError("release response body did not return bytes") + if len(chunk) > size: + raise ReleaseDownloadError( + "release response body exceeded its requested read bound" + ) + return chunk + + +def _next_read_size( + *, payload_size: int, declared_length: int | None, maximum_bytes: int +) -> int: + remaining_capacity = maximum_bytes - payload_size + if declared_length is None: + return min(_READ_CHUNK_BYTES, remaining_capacity + 1) + return min(_READ_CHUNK_BYTES, declared_length - payload_size) + + +def _read_response_body( + response: object, + *, + deadline: float, + request_timeout: float, + maximum_bytes: int, +) -> bytes: + headers = getattr(response, "headers", {}) + declared_length = _declared_content_length(headers) + _validate_transfer_encoding(headers, declared_length) + if declared_length is not None and declared_length > maximum_bytes: + raise ReleaseDownloadSizeError( + f"release response declared {declared_length} bytes, above the {maximum_bytes}-byte limit" + ) + + payload = bytearray() + while declared_length is None or len(payload) < declared_length: + read_size = _next_read_size( + payload_size=len(payload), + declared_length=declared_length, + maximum_bytes=maximum_bytes, + ) + timeout = _remaining_timeout(deadline=deadline, request_timeout=request_timeout) + _set_response_timeout(response, timeout) + chunk = _read_chunk(response, read_size) + _remaining_timeout(deadline=deadline, request_timeout=request_timeout) + if not chunk: + break + payload.extend(chunk) + if len(payload) > maximum_bytes: + raise ReleaseDownloadSizeError( + f"release response exceeded the {maximum_bytes}-byte response limit" + ) + + if declared_length is not None and len(payload) != declared_length: + raise IncompleteRead(bytes(payload), declared_length - len(payload)) + return bytes(payload) + + +def _request_hop( + url: str, + *, + deadline: float, + request_timeout: float, + maximum_bytes: int, +) -> bytes | _RedirectHop: + timeout = _remaining_timeout(deadline=deadline, request_timeout=request_timeout) + with _stdlib_open(url, timeout=timeout) as response: + _remaining_timeout(deadline=deadline, request_timeout=request_timeout) + status = _response_status(response) + if status in _REDIRECT_STATUSES: + locations = _header_values(getattr(response, "headers", {}), "Location") + if len(locations) != 1: + raise ReleaseDownloadError( + "release redirect has a missing or ambiguous Location header" + ) + return _RedirectHop(locations[0]) + if status is not None and 300 <= status <= 399: + raise ReleaseDownloadError( + f"release response used unsupported redirect status {status}" + ) + if status is not None and status >= 400: + raise _ReturnedHttpError(status, getattr(response, "headers", {})) + return _read_response_body( + response, + deadline=deadline, + request_timeout=request_timeout, + maximum_bytes=maximum_bytes, + ) + + +def _download_once( + url: str, + *, + deadline: float, + request_timeout: float, + policy: ReleaseRetryPolicy, +) -> bytes: + current_url = url + visited = {url} + redirects_followed = 0 + while True: + outcome = _request_hop( + current_url, + deadline=deadline, + request_timeout=request_timeout, + maximum_bytes=policy.maximum_response_bytes, + ) + if isinstance(outcome, bytes): + return outcome + if redirects_followed >= policy.maximum_redirects: + raise ReleaseDownloadError( + "release download exceeded the approved redirect-hop limit" + ) + target_url = outcome.location + if target_url in visited: + raise ReleaseDownloadError("release download encountered a redirect cycle") + if not _approved_redirect(current_url, target_url): + raise ReleaseDownloadError( + "release download encountered an unapproved redirect target" + ) + visited.add(target_url) + current_url = target_url + redirects_followed += 1 + + +def _download_attempt( + url: str, + *, + deadline: float, + request_timeout: float, + policy: ReleaseRetryPolicy, +) -> bytes | _AttemptFailure: + try: + return _download_once( + url, + deadline=deadline, + request_timeout=request_timeout, + policy=policy, + ) + except ( + URLError, + TimeoutError, + ConnectionError, + IncompleteRead, + _ReturnedHttpError, + ) as exc: + failure = _classified_failure(exc) + if isinstance(exc, HTTPError): + exc.close() + return failure + + +def _wait_for_retry( + failure: _AttemptFailure, + *, + failed_attempt: int, + deadline: float, + policy: ReleaseRetryPolicy, +) -> bool: + delay = _retry_delay(failure, failed_attempt=failed_attempt, policy=policy) + if delay >= deadline - _monotonic(): + return False + _sleep(delay) + return True + + +def retrying_urlopen( + url: str, + *, + timeout: float | None = None, + policy: ReleaseRetryPolicy = DEFAULT_RELEASE_RETRY_POLICY, +) -> BinaryIO: + """Download one approved release URL into a bounded in-memory response. + + Only transient transport failures and HTTP 408, 429, and 5xx responses are + retried. Validation remains in each caller, so digest, archive, signature, + type, and identity failures cannot trigger another network attempt. + """ + + if not _approved_url(url): + raise ReleaseDownloadError( + "release download URL is not an approved pinned-tool origin" + ) + request_timeout = policy.request_timeout_seconds if timeout is None else timeout + if request_timeout <= 0: + raise ValueError("timeout must be positive") + + deadline = _monotonic() + policy.total_timeout_seconds + attempts_made = 0 + last_failure = "total timeout" + for next_attempt in range(1, policy.max_attempts + 1): + remaining = deadline - _monotonic() + if remaining <= 0: + break + attempts_made = next_attempt + outcome = _download_attempt( + url, + deadline=deadline, + request_timeout=request_timeout, + policy=policy, + ) + if isinstance(outcome, bytes): + return io.BytesIO(outcome) + last_failure = outcome.kind + if not outcome.retryable: + raise ReleaseDownloadError( + f"release download from {_display_url(url)} failed without retry: {last_failure}" + ) from None + if attempts_made < policy.max_attempts and not _wait_for_retry( + outcome, + failed_attempt=attempts_made, + deadline=deadline, + policy=policy, + ): + break + + raise ReleaseDownloadError( + f"release download from {_display_url(url)} exhausted {attempts_made} bounded attempts: {last_failure}" + ) from None diff --git a/tools/vale_tool.py b/tools/vale_tool.py index c475d5b8..70bb66b5 100644 --- a/tools/vale_tool.py +++ b/tools/vale_tool.py @@ -7,8 +7,10 @@ import tarfile from hashlib import sha256 from pathlib import Path +from urllib.error import URLError -from tools.http_download import download_bytes +from tools.release_download import ReleaseDownloadError +from tools.release_download import retrying_urlopen as urlopen from tools.tool_versions import VALE_VERSION REPO_ROOT = Path(__file__).resolve().parents[1] @@ -82,7 +84,11 @@ def ensure_vale(repo_root: Path = REPO_ROOT, *, version: str = VALE_VERSION) -> raise RuntimeError(f"no repository-pinned checksum for Vale asset {asset_name}") base_url = _release_base_url(version) asset_url = f"{base_url}/{asset_name}" - archive_bytes = download_bytes(asset_url, description="Vale") + try: + with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + archive_bytes = response.read() + except (URLError, ReleaseDownloadError) as exc: + raise RuntimeError(f"failed to download Vale from {asset_url}: {exc}") from exc actual = sha256(archive_bytes).hexdigest() if actual != expected: raise RuntimeError(f"Vale checksum mismatch for {asset_name}: expected {expected}, got {actual}") From 4f192e185b86c276f1984e486b2974d52ba60ace Mon Sep 17 00:00:00 2001 From: Yernat Yestekov <2068106+doublewhy@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:17:34 -0700 Subject: [PATCH 2/4] fix: enforce release download wall deadline --- .../issue-1137-ci-release-download-retries.md | 26 +- .../python/tests/test_release_download.py | 338 +++++++++++++++++- tools/release_download.py | 319 +++++++++++++---- 3 files changed, 589 insertions(+), 94 deletions(-) diff --git a/docs/decisions/issue-1137-ci-release-download-retries.md b/docs/decisions/issue-1137-ci-release-download-retries.md index e3f14c3d..2cbd2526 100644 --- a/docs/decisions/issue-1137-ci-release-download-retries.md +++ b/docs/decisions/issue-1137-ci-release-download-retries.md @@ -59,12 +59,14 @@ bodies are closed without being read. The helper performs at most three attempts. Every connection hop and every bounded body read receives a finite 60-second socket timeout capped by the remaining 190-second total deadline. Exponential delays are deterministic and -capped, and no read, redirect hop, sleep, or new attempt may continue after the -deadline. Declared response lengths are checked before and after streaming; -ambiguous framing and oversized bodies fail immediately, while an early EOF is -classified as a retryable incomplete transfer. Each response is bounded to -256 MiB before it is returned to an installer. There is no jitter, alternate -mirror, unpinned version, credential, or redirect-selection fallback. +capped, and a deadline-aware socket reader reapplies the remaining bound to +every status-line, header, chunk-framing, and body buffer fill. No read, +redirect hop, sleep, or new attempt may continue after the deadline. Declared +response lengths are checked before and after streaming; ambiguous framing and +oversized bodies fail immediately, while an early EOF is classified as a +retryable incomplete transfer. Each response is bounded to 256 MiB before it +is returned to an installer. There is no jitter, alternate mirror, unpinned +version, credential, or redirect-selection fallback. Only these failures are retryable: @@ -76,7 +78,9 @@ Only these failures are retryable: value is capped at ten seconds and can never extend the total deadline. All other HTTP and URL failures stop after the first attempt. Final diagnostics include only the approved URL path, attempt count, and stable failure class; -they omit response bodies and exception text. +they omit response bodies and exception text. Malformed HTTP status, header, +and framing exceptions are normalized to one non-retryable diagnostic rather +than reflecting upstream text. The helper buffers a successful bounded response and then returns control to the owning installer. Consequently, checksum mismatch, signature failure, @@ -111,9 +115,11 @@ exhaustion, non-retryable HTTP and URL failures, response-size rejection, sanitized diagnostics, and no second request after checksum or archive-type failure. Local scripted HTTP servers verify that an early EOF never returns partial bytes, redirect bodies are not drained, and a trickling body cannot -extend the total deadline. A binding test ensures all four installers continue -to share the helper, and clean-cache smoke tests exercise the live current -GitHub redirect chain for all four pinned tools. +extend the total deadline. The same scripted transport verifies that trickled +redirect headers and chunk trailers cannot extend that deadline and that a +malformed status line cannot leak upstream text. A binding test ensures all +four installers continue to share the helper, and clean-cache smoke tests +exercise the live current GitHub redirect chain for all four pinned tools. This policy does not make GitHub availability hermetic, validate remote bytes, or replace the installers' pins, checksums, signatures, archive rules, cache diff --git a/implementations/python/tests/test_release_download.py b/implementations/python/tests/test_release_download.py index e9bdca51..23e2eb9e 100644 --- a/implementations/python/tests/test_release_download.py +++ b/implementations/python/tests/test_release_download.py @@ -20,7 +20,6 @@ from urllib.error import HTTPError, URLError import pytest - import tools.gitleaks_tool as gitleaks_tool import tools.osv_scanner_tool as osv_scanner_tool import tools.policy.conftest_tool as conftest_tool @@ -78,7 +77,15 @@ def __init__(self, *results: DownloadResponse | BaseException) -> None: self.results = list(results) self.calls: list[tuple[str, float | None]] = [] - def __call__(self, url: str, *, timeout: float | None = None) -> DownloadResponse: + def __call__( + self, + url: str, + *, + timeout: float | None = None, + deadline: float | None = None, + request_timeout: float | None = None, + ) -> DownloadResponse: + del deadline, request_timeout self.calls.append((url, timeout)) result = self.results.pop(0) if isinstance(result, BaseException): @@ -307,13 +314,15 @@ def test_success_uses_one_finite_request_and_returns_a_context_response( assert clock.sleeps == [] -def test_stdlib_open_uses_a_default_redirect_capable_opener(monkeypatch: pytest.MonkeyPatch) -> None: +def test_stdlib_open_installs_deadline_handlers_and_disables_automatic_redirects( + monkeypatch: pytest.MonkeyPatch, +) -> None: response = DownloadResponse(b"payload") observed: dict[str, object] = {} class FakeOpener: - def __init__(self, *_handlers: object) -> None: - pass + def __init__(self, *handlers: object) -> None: + observed["handlers"] = tuple(type(handler).__name__ for handler in handlers) def open(self, url: str, *, timeout: float) -> DownloadResponse: observed.update(url=url, timeout=timeout) @@ -322,7 +331,11 @@ def open(self, url: str, *, timeout: float) -> DownloadResponse: monkeypatch.setattr(release_download, "build_opener", FakeOpener) assert release_download._stdlib_open(RELEASE_URL, timeout=12) is response - assert observed == {"url": RELEASE_URL, "timeout": 12} + assert observed == { + "handlers": ("_NoRedirectHandler", "_DeadlineHTTPHandler", "_DeadlineHTTPSHandler"), + "url": RELEASE_URL, + "timeout": 12, + } def test_http_408_429_and_5xx_retry_with_retry_after_then_backoff( @@ -608,6 +621,226 @@ def read1(self, _size: int) -> bytes: ) +def test_deadline_raw_reader_enforces_transport_contract_and_cleanup( + monkeypatch: pytest.MonkeyPatch, +) -> None: + clock = FakeClock() + monkeypatch.setattr(release_download, "_monotonic", clock.monotonic) + + class Raw: + def __init__(self, result: object = 2) -> None: + self.result = result + self.closed = False + + def readinto(self, buffer: bytearray) -> object: + buffer[:2] = b"ok" + return self.result + + def fileno(self) -> int: + return 17 + + def close(self) -> None: + self.closed = True + + class Transport: + def __init__(self) -> None: + self.timeouts: list[float] = [] + + def settimeout(self, timeout: float) -> None: + self.timeouts.append(timeout) + + raw = Raw() + transport = Transport() + reader = release_download._DeadlineRawReader( + raw, + transport, + deadline=1, + request_timeout=0.5, + ) + buffer = bytearray(2) + + assert reader.readable() + assert reader.readinto(buffer) == 2 + assert buffer == b"ok" + assert reader.fileno() == 17 + assert transport.timeouts == [0.5] + reader.close() + reader.close() + assert raw.closed + + without_timeout = release_download._DeadlineRawReader( + Raw(), + object(), + deadline=1, + request_timeout=0.5, + ) + with pytest.raises(release_download.ReleaseDownloadError, match="cannot enforce"): + without_timeout.readinto(bytearray(2)) + + class NoReader: + def close(self) -> None: + pass + + without_reader = release_download._DeadlineRawReader( + NoReader(), + Transport(), + deadline=1, + request_timeout=0.5, + ) + with pytest.raises(release_download.ReleaseDownloadError, match="not readable"): + without_reader.readinto(bytearray(2)) + + invalid_count = release_download._DeadlineRawReader( + Raw("invalid"), + Transport(), + deadline=1, + request_timeout=0.5, + ) + with pytest.raises(release_download.ReleaseDownloadError, match="invalid byte count"): + invalid_count.readinto(bytearray(2)) + + +def test_deadline_socket_wraps_binary_readers_and_delegates_other_files( + monkeypatch: pytest.MonkeyPatch, +) -> None: + clock = FakeClock() + monkeypatch.setattr(release_download, "_monotonic", clock.monotonic) + + class Raw(io.RawIOBase): + def readable(self) -> bool: + return True + + def readinto(self, _buffer: object) -> int: + return 0 + + class Transport: + marker = "delegated" + + def __init__(self) -> None: + self.calls: list[tuple[str, int | None, dict[str, object]]] = [] + + def makefile( + self, + mode: str, + buffering: int | None = None, + **kwargs: object, + ) -> object: + self.calls.append((mode, buffering, kwargs)) + return Raw() if mode == "rb" else io.BytesIO() + + def settimeout(self, _timeout: float) -> None: + pass + + transport = Transport() + wrapped = release_download._DeadlineSocket( + transport, + deadline=1, + request_timeout=0.5, + ) + + assert wrapped.marker == "delegated" + delegated = wrapped.makefile("wb", 8) + assert isinstance(delegated, io.BytesIO) + unbuffered = wrapped.makefile("rb", 0) + assert isinstance(unbuffered, release_download._DeadlineRawReader) + unbuffered.close() + buffered = wrapped.makefile("rb", 16) + assert isinstance(buffered, io.BufferedReader) + buffered.close() + assert transport.calls == [ + ("wb", 8, {}), + ("rb", 0, {}), + ("rb", 0, {}), + ] + + +def test_deadline_connections_cover_empty_sockets_tunnels_and_https_handler( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class Transport: + def close(self) -> None: + pass + + http_connection = release_download._DeadlineHTTPConnection( + "example.test", + deadline=1, + request_timeout=0.5, + ) + monkeypatch.setattr( + release_download.http_client.HTTPConnection, + "connect", + lambda connection: setattr(connection, "sock", None), + ) + http_connection.connect() + assert http_connection.sock is None + + https_connection = release_download._DeadlineHTTPSConnection( + "example.test", + deadline=1, + request_timeout=0.5, + ) + tunnel_sockets: list[object] = [] + + def observe_tunnel(connection: object) -> None: + tunnel_sockets.append(connection.sock) + + monkeypatch.setattr( + release_download.http_client.HTTPSConnection, + "_tunnel", + observe_tunnel, + ) + https_connection.sock = None + https_connection._tunnel() + transport = Transport() + https_connection.sock = transport + https_connection._tunnel() + assert isinstance(tunnel_sockets[1], release_download._DeadlineSocket) + assert https_connection.sock is transport + + def close_tunnel(connection: object) -> None: + connection.sock = None + + monkeypatch.setattr( + release_download.http_client.HTTPSConnection, + "_tunnel", + close_tunnel, + ) + https_connection.sock = transport + https_connection._tunnel() + assert https_connection.sock is None + + monkeypatch.setattr( + release_download.http_client.HTTPSConnection, + "connect", + lambda connection: setattr(connection, "sock", transport), + ) + https_connection.connect() + assert isinstance(https_connection.sock, release_download._DeadlineSocket) + monkeypatch.setattr( + release_download.http_client.HTTPSConnection, + "connect", + lambda connection: setattr(connection, "sock", None), + ) + https_connection.connect() + assert https_connection.sock is None + + handler = release_download._DeadlineHTTPSHandler(deadline=1, request_timeout=0.5) + response = DownloadResponse(b"") + observed: dict[str, object] = {} + + def fake_do_open(factory: object, request: object, **kwargs: object) -> DownloadResponse: + observed.update(factory=factory, request=request, kwargs=kwargs) + connection = factory("example.test", timeout=0.5) + assert isinstance(connection, release_download._DeadlineHTTPSConnection) + return response + + monkeypatch.setattr(handler, "do_open", fake_do_open) + request = object() + assert handler.https_open(request) is response + assert observed["request"] is request + assert observed["kwargs"] == {"context": handler._context} + + def test_bounded_reader_falls_back_to_read_and_rejects_invalid_readers() -> None: class ReadOnly: def read(self, _size: int) -> bytes: @@ -672,8 +905,20 @@ def test_header_read_cannot_cross_the_total_deadline_and_follow_redirect( ) class DelayedOpener(SequenceOpener): - def __call__(self, url: str, *, timeout: float | None = None) -> DownloadResponse: - result = super().__call__(url, timeout=timeout) + def __call__( + self, + url: str, + *, + timeout: float | None = None, + deadline: float | None = None, + request_timeout: float | None = None, + ) -> DownloadResponse: + result = super().__call__( + url, + timeout=timeout, + deadline=deadline, + request_timeout=request_timeout, + ) clock.elapsed += 0.11 return result @@ -758,6 +1003,83 @@ def test_local_server_trickle_read_is_cut_off_by_total_deadline( assert server.server.requests == 1 +def test_local_server_trickled_redirect_headers_are_cut_off_by_total_deadline( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + final_url = f"{server.base_url}/final" + response = _local_response( + b"", + status="302 Found", + location=final_url, + ) + pieces = [response[index : index + 10] for index in range(0, len(response), 10)] + server.enqueue(*((0 if index == 0 else 0.04, piece) for index, piece in enumerate(pieces))) + policy = release_download.ReleaseRetryPolicy( + max_attempts=1, + request_timeout_seconds=0.08, + total_timeout_seconds=0.12, + ) + + started = time.monotonic() + with pytest.raises(release_download.ReleaseDownloadError, match="transport timeout"): + release_download.retrying_urlopen(f"{server.base_url}/redirect", policy=policy) + elapsed = time.monotonic() - started + + assert elapsed < 0.3 + assert server.server.requests == 1 + + +def test_local_server_trickled_chunk_trailer_is_cut_off_by_total_deadline( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + prefix = b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\nConnection: close\r\n\r\n1\r\na\r\n0\r\n" + trailer = b"X-Review: " + (b"x" * 80) + b"\r\n\r\n" + trailer_pieces = [trailer[index : index + 10] for index in range(0, len(trailer), 10)] + server.enqueue( + (0, prefix), + *((0.04, piece) for piece in trailer_pieces), + ) + policy = release_download.ReleaseRetryPolicy( + max_attempts=1, + request_timeout_seconds=0.08, + total_timeout_seconds=0.12, + ) + + started = time.monotonic() + with pytest.raises(release_download.ReleaseDownloadError, match="transport timeout"): + release_download.retrying_urlopen(f"{server.base_url}/chunked", policy=policy) + elapsed = time.monotonic() - started + + assert elapsed < 0.3 + assert server.server.requests == 1 + + +def test_malformed_status_line_is_sanitized_without_retry( + monkeypatch: pytest.MonkeyPatch, +) -> None: + with ScriptedReleaseServer() as server: + _admit_local_server(monkeypatch, server) + server.enqueue((0, b"credential=do-not-log\r\n\r\n")) + policy = release_download.ReleaseRetryPolicy( + max_attempts=2, + request_timeout_seconds=0.5, + total_timeout_seconds=1, + ) + + with pytest.raises( + release_download.ReleaseDownloadError, + match="invalid HTTP response", + ) as raised: + release_download.retrying_urlopen(f"{server.base_url}/malformed", policy=policy) + + assert "credential" not in str(raised.value) + assert server.server.requests == 1 + + def test_invalid_per_call_timeout_fails_before_network(monkeypatch: pytest.MonkeyPatch) -> None: opener = SequenceOpener(AssertionError("invalid timeout reached the network")) _install_network(monkeypatch, opener) diff --git a/tools/release_download.py b/tools/release_download.py index 6c0c69f1..b39e89eb 100644 --- a/tools/release_download.py +++ b/tools/release_download.py @@ -2,18 +2,21 @@ from __future__ import annotations +import http.client as http_client import io import re +import ssl import time from collections.abc import Callable from dataclasses import dataclass from datetime import UTC from email.utils import parsedate_to_datetime +from functools import partial from http.client import IncompleteRead, RemoteDisconnected from typing import BinaryIO from urllib.error import HTTPError, URLError from urllib.parse import SplitResult, urlsplit -from urllib.request import HTTPRedirectHandler, build_opener +from urllib.request import HTTPHandler, HTTPRedirectHandler, HTTPSHandler, build_opener _APPROVED_RELEASE_PATH_PREFIXES = ( "/errata-ai/vale/releases/download/", @@ -63,9 +66,7 @@ def __post_init__(self) -> None: self.maximum_response_bytes, ) if any(value <= 0 for value in positive_values): - raise ValueError( - "request, total-time, and response bounds must be positive" - ) + raise ValueError("request, total-time, and response bounds must be positive") delay_values = ( self.initial_backoff_seconds, self.maximum_backoff_seconds, @@ -117,6 +118,193 @@ def _return_response( http_error_308 = _return_response +class _DeadlineRawReader(io.RawIOBase): + """Apply the absolute acquisition deadline to every socket-buffer refill.""" + + def __init__( + self, + raw: BinaryIO, + transport: object, + *, + deadline: float, + request_timeout: float, + ) -> None: + self._raw = raw + self._sock = transport + self._deadline = deadline + self._request_timeout = request_timeout + + def readable(self) -> bool: + return True + + def readinto(self, buffer: object) -> int | None: + timeout = _remaining_timeout( + deadline=self._deadline, + request_timeout=self._request_timeout, + ) + setter = getattr(self._sock, "settimeout", None) + if not callable(setter): + raise ReleaseDownloadError("release response transport cannot enforce the download deadline") + setter(timeout) + readinto = getattr(self._raw, "readinto", None) + if not callable(readinto): + raise ReleaseDownloadError("release response transport is not readable") + count = readinto(buffer) + _remaining_timeout( + deadline=self._deadline, + request_timeout=self._request_timeout, + ) + if count is not None and not isinstance(count, int): + raise ReleaseDownloadError("release response transport returned an invalid byte count") + return count + + def fileno(self) -> int: + return self._raw.fileno() + + def close(self) -> None: + if self.closed: + return + try: + self._raw.close() + finally: + super().close() + + +class _DeadlineSocket: + """Delegate socket operations while governing every response-file read.""" + + def __init__( + self, + transport: object, + *, + deadline: float, + request_timeout: float, + ) -> None: + self._transport = transport + self._deadline = deadline + self._request_timeout = request_timeout + + def __getattr__(self, name: str) -> object: + return getattr(self._transport, name) + + def makefile( + self, + mode: str = "r", + buffering: int | None = None, + **kwargs: object, + ) -> BinaryIO: + makefile = self._transport.makefile + if mode != "rb" or kwargs: + return makefile(mode, buffering, **kwargs) + raw = makefile(mode, buffering=0) + deadline_raw = _DeadlineRawReader( + raw, + self._transport, + deadline=self._deadline, + request_timeout=self._request_timeout, + ) + if buffering == 0: + return deadline_raw + buffer_size = io.DEFAULT_BUFFER_SIZE if buffering is None or buffering < 0 else buffering + return io.BufferedReader(deadline_raw, buffer_size) + + +class _DeadlineHTTPConnection(http_client.HTTPConnection): + """HTTP connection whose status, header, and framing reads share one deadline.""" + + def __init__( + self, + host: str, + *, + deadline: float, + request_timeout: float, + **kwargs: object, + ) -> None: + self._download_deadline = deadline + self._download_request_timeout = request_timeout + super().__init__(host, **kwargs) + + def connect(self) -> None: + super().connect() + if self.sock is not None: + self.sock = _DeadlineSocket( + self.sock, + deadline=self._download_deadline, + request_timeout=self._download_request_timeout, + ) + + +class _DeadlineHTTPSConnection(http_client.HTTPSConnection): + """HTTPS connection with deadline-aware proxy and origin response parsing.""" + + def __init__( + self, + host: str, + *, + deadline: float, + request_timeout: float, + **kwargs: object, + ) -> None: + self._download_deadline = deadline + self._download_request_timeout = request_timeout + super().__init__(host, **kwargs) + + def _deadline_socket(self, transport: object) -> _DeadlineSocket: + return _DeadlineSocket( + transport, + deadline=self._download_deadline, + request_timeout=self._download_request_timeout, + ) + + def _tunnel(self) -> None: + transport = self.sock + if transport is None: + super()._tunnel() + return + wrapped = self._deadline_socket(transport) + self.sock = wrapped + try: + super()._tunnel() + finally: + if self.sock is wrapped: + self.sock = transport + + def connect(self) -> None: + super().connect() + if self.sock is not None: + self.sock = self._deadline_socket(self.sock) + + +class _DeadlineHTTPHandler(HTTPHandler): + def __init__(self, *, deadline: float, request_timeout: float) -> None: + super().__init__() + self._deadline = deadline + self._request_timeout = request_timeout + + def http_open(self, request: object) -> BinaryIO: + connection = partial( + _DeadlineHTTPConnection, + deadline=self._deadline, + request_timeout=self._request_timeout, + ) + return self.do_open(connection, request) + + +class _DeadlineHTTPSHandler(HTTPSHandler): + def __init__(self, *, deadline: float, request_timeout: float) -> None: + super().__init__() + self._deadline = deadline + self._request_timeout = request_timeout + + def https_open(self, request: object) -> BinaryIO: + connection = partial( + _DeadlineHTTPSConnection, + deadline=self._deadline, + request_timeout=self._request_timeout, + ) + return self.do_open(connection, request, context=self._context) + + @dataclass(frozen=True) class _AttemptFailure: kind: str @@ -125,11 +313,7 @@ class _AttemptFailure: def _parsed_url_with_port(url: str) -> tuple[SplitResult, int | None] | None: - if ( - not isinstance(url, str) - or len(url) > _MAX_REDIRECT_LOCATION_CHARS - or any(ord(char) < 32 for char in url) - ): + if not isinstance(url, str) or len(url) > _MAX_REDIRECT_LOCATION_CHARS or any(ord(char) < 32 for char in url): return None try: parsed = urlsplit(url) @@ -154,17 +338,11 @@ def _approved_authority(parsed: SplitResult, port: int | None) -> bool: def _safe_release_path(path: str) -> bool: path_parts = path.split("/") normalized_path = path.lower() - return ( - "\\" not in path - and "%" not in normalized_path - and all(part not in {".", ".."} for part in path_parts) - ) + return "\\" not in path and "%" not in normalized_path and all(part not in {".", ".."} for part in path_parts) def _approved_release_path(path: str) -> bool: - return _safe_release_path(path) and any( - path.startswith(prefix) for prefix in _APPROVED_RELEASE_PATH_PREFIXES - ) + return _safe_release_path(path) and any(path.startswith(prefix) for prefix in _APPROVED_RELEASE_PATH_PREFIXES) def _approved_url(url: str) -> bool: @@ -217,9 +395,7 @@ def _approved_vale_relocation(source_url: str, target_url: str) -> bool: def _approved_redirect(source_url: str, target_url: str) -> bool: if _approved_vale_relocation(source_url, target_url): return True - source_is_github_release = _approved_url( - source_url - ) or _approved_canonical_vale_url(source_url) + source_is_github_release = _approved_url(source_url) or _approved_canonical_vale_url(source_url) return source_is_github_release and _approved_release_asset_url(target_url) @@ -293,7 +469,7 @@ def _is_retryable(exc: BaseException) -> bool: reason = _transport_reason(exc) return isinstance( reason, - (TimeoutError, ConnectionError, IncompleteRead), + (TimeoutError, ConnectionError, IncompleteRead, ssl.SSLEOFError), ) @@ -308,7 +484,7 @@ def _failure_kind(exc: BaseException) -> str: kind = "transport timeout" elif isinstance(reason, RemoteDisconnected): kind = "remote disconnect" - elif isinstance(reason, IncompleteRead): + elif isinstance(reason, (IncompleteRead, ssl.SSLEOFError)): kind = "incomplete response" elif isinstance(reason, ConnectionError): kind = "connection failure" @@ -319,9 +495,7 @@ def _failure_kind(exc: BaseException) -> str: def _classified_failure(exc: BaseException) -> _AttemptFailure: headers = _http_headers(exc) - retry_after = ( - None if headers is None else _retry_after_seconds(headers, now=_wall_time()) - ) + retry_after = None if headers is None else _retry_after_seconds(headers, now=_wall_time()) return _AttemptFailure( kind=_failure_kind(exc), retryable=_is_retryable(exc), @@ -329,17 +503,33 @@ def _classified_failure(exc: BaseException) -> _AttemptFailure: ) -def _retry_delay( - failure: _AttemptFailure, *, failed_attempt: int, policy: ReleaseRetryPolicy -) -> float: +def _retry_delay(failure: _AttemptFailure, *, failed_attempt: int, policy: ReleaseRetryPolicy) -> float: if failure.retry_after_seconds is not None: return min(failure.retry_after_seconds, policy.maximum_retry_after_seconds) exponential = policy.initial_backoff_seconds * (2 ** (failed_attempt - 1)) return min(exponential, policy.maximum_backoff_seconds) -def _stdlib_open(url: str, *, timeout: float) -> BinaryIO: - return build_opener(_NoRedirectHandler()).open(url, timeout=timeout) +def _stdlib_open( + url: str, + *, + timeout: float, + deadline: float | None = None, + request_timeout: float | None = None, +) -> BinaryIO: + active_deadline = _monotonic() + timeout if deadline is None else deadline + active_request_timeout = timeout if request_timeout is None else request_timeout + return build_opener( + _NoRedirectHandler(), + _DeadlineHTTPHandler( + deadline=active_deadline, + request_timeout=active_request_timeout, + ), + _DeadlineHTTPSHandler( + deadline=active_deadline, + request_timeout=active_request_timeout, + ), + ).open(url, timeout=timeout) def _remaining_timeout(*, deadline: float, request_timeout: float) -> float: @@ -359,38 +549,25 @@ def _header_values(headers: object, name: str) -> tuple[str, ...]: def _declared_content_length(headers: object) -> int | None: - tokens = tuple( - token.strip() - for value in _header_values(headers, "Content-Length") - for token in value.split(",") - ) + tokens = tuple(token.strip() for value in _header_values(headers, "Content-Length") for token in value.split(",")) if not tokens: return None - if any( - len(token) > 20 or not token.isascii() or not token.isdecimal() - for token in tokens - ): + if any(len(token) > 20 or not token.isascii() or not token.isdecimal() for token in tokens): raise ReleaseDownloadError("release response has an invalid Content-Length") lengths = {int(token) for token in tokens} if len(lengths) != 1: - raise ReleaseDownloadError( - "release response has conflicting Content-Length values" - ) + raise ReleaseDownloadError("release response has conflicting Content-Length values") return lengths.pop() def _validate_transfer_encoding(headers: object, declared_length: int | None) -> None: tokens = tuple( - token.strip().lower() - for value in _header_values(headers, "Transfer-Encoding") - for token in value.split(",") + token.strip().lower() for value in _header_values(headers, "Transfer-Encoding") for token in value.split(",") ) if not tokens: return if tokens != ("chunked",) or declared_length is not None: - raise ReleaseDownloadError( - "release response has an unsupported transfer framing" - ) + raise ReleaseDownloadError("release response has an unsupported transfer framing") def _response_timeout_setter(response: object) -> Callable[[float], object] | None: @@ -407,9 +584,7 @@ def _response_timeout_setter(response: object) -> Callable[[float], object] | No def _set_response_timeout(response: object, timeout: float) -> None: setter = _response_timeout_setter(response) if setter is None: - raise ReleaseDownloadError( - "release response transport cannot enforce the download deadline" - ) + raise ReleaseDownloadError("release response transport cannot enforce the download deadline") setter(timeout) @@ -423,15 +598,11 @@ def _read_chunk(response: object, size: int) -> bytes: if not isinstance(chunk, bytes): raise ReleaseDownloadError("release response body did not return bytes") if len(chunk) > size: - raise ReleaseDownloadError( - "release response body exceeded its requested read bound" - ) + raise ReleaseDownloadError("release response body exceeded its requested read bound") return chunk -def _next_read_size( - *, payload_size: int, declared_length: int | None, maximum_bytes: int -) -> int: +def _next_read_size(*, payload_size: int, declared_length: int | None, maximum_bytes: int) -> int: remaining_capacity = maximum_bytes - payload_size if declared_length is None: return min(_READ_CHUNK_BYTES, remaining_capacity + 1) @@ -468,9 +639,7 @@ def _read_response_body( break payload.extend(chunk) if len(payload) > maximum_bytes: - raise ReleaseDownloadSizeError( - f"release response exceeded the {maximum_bytes}-byte response limit" - ) + raise ReleaseDownloadSizeError(f"release response exceeded the {maximum_bytes}-byte response limit") if declared_length is not None and len(payload) != declared_length: raise IncompleteRead(bytes(payload), declared_length - len(payload)) @@ -485,20 +654,21 @@ def _request_hop( maximum_bytes: int, ) -> bytes | _RedirectHop: timeout = _remaining_timeout(deadline=deadline, request_timeout=request_timeout) - with _stdlib_open(url, timeout=timeout) as response: + with _stdlib_open( + url, + timeout=timeout, + deadline=deadline, + request_timeout=request_timeout, + ) as response: _remaining_timeout(deadline=deadline, request_timeout=request_timeout) status = _response_status(response) if status in _REDIRECT_STATUSES: locations = _header_values(getattr(response, "headers", {}), "Location") if len(locations) != 1: - raise ReleaseDownloadError( - "release redirect has a missing or ambiguous Location header" - ) + raise ReleaseDownloadError("release redirect has a missing or ambiguous Location header") return _RedirectHop(locations[0]) if status is not None and 300 <= status <= 399: - raise ReleaseDownloadError( - f"release response used unsupported redirect status {status}" - ) + raise ReleaseDownloadError(f"release response used unsupported redirect status {status}") if status is not None and status >= 400: raise _ReturnedHttpError(status, getattr(response, "headers", {})) return _read_response_body( @@ -529,16 +699,12 @@ def _download_once( if isinstance(outcome, bytes): return outcome if redirects_followed >= policy.maximum_redirects: - raise ReleaseDownloadError( - "release download exceeded the approved redirect-hop limit" - ) + raise ReleaseDownloadError("release download exceeded the approved redirect-hop limit") target_url = outcome.location if target_url in visited: raise ReleaseDownloadError("release download encountered a redirect cycle") if not _approved_redirect(current_url, target_url): - raise ReleaseDownloadError( - "release download encountered an unapproved redirect target" - ) + raise ReleaseDownloadError("release download encountered an unapproved redirect target") visited.add(target_url) current_url = target_url redirects_followed += 1 @@ -563,12 +729,15 @@ def _download_attempt( TimeoutError, ConnectionError, IncompleteRead, + ssl.SSLEOFError, _ReturnedHttpError, ) as exc: failure = _classified_failure(exc) if isinstance(exc, HTTPError): exc.close() return failure + except (http_client.HTTPException, OSError): + raise ReleaseDownloadError("release transport returned an invalid HTTP response") from None def _wait_for_retry( @@ -599,9 +768,7 @@ def retrying_urlopen( """ if not _approved_url(url): - raise ReleaseDownloadError( - "release download URL is not an approved pinned-tool origin" - ) + raise ReleaseDownloadError("release download URL is not an approved pinned-tool origin") request_timeout = policy.request_timeout_seconds if timeout is None else timeout if request_timeout <= 0: raise ValueError("timeout must be positive") From 5fdc59f882b6b644ea6df6043cf424abeb22e23f Mon Sep 17 00:00:00 2001 From: Yernat Yestekov <2068106+doublewhy@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:31:26 -0700 Subject: [PATCH 3/4] test: isolate release download exception calls --- .../python/tests/test_release_download.py | 14 +++++++++----- .../python/tests/test_repo_policy_tools.py | 4 +--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/implementations/python/tests/test_release_download.py b/implementations/python/tests/test_release_download.py index 23e2eb9e..24848f95 100644 --- a/implementations/python/tests/test_release_download.py +++ b/implementations/python/tests/test_release_download.py @@ -612,10 +612,12 @@ class NoDeadlineControl: def read1(self, _size: int) -> bytes: return b"" + response = NoDeadlineControl() + deadline = time.monotonic() + 1 with pytest.raises(release_download.ReleaseDownloadError, match="cannot enforce the download deadline"): release_download._read_response_body( - NoDeadlineControl(), - deadline=time.monotonic() + 1, + response, + deadline=deadline, request_timeout=1, maximum_bytes=1, ) @@ -854,13 +856,15 @@ class OverBoundReader: def read1(self, size: int) -> bytes: return b"x" * (size + 1) + non_bytes_reader = NonBytesReader() + over_bound_reader = OverBoundReader() assert release_download._read_chunk(ReadOnly(), 2) == b"ok" with pytest.raises(release_download.ReleaseDownloadError, match="not readable"): release_download._read_chunk(object(), 2) with pytest.raises(release_download.ReleaseDownloadError, match="did not return bytes"): - release_download._read_chunk(NonBytesReader(), 2) + release_download._read_chunk(non_bytes_reader, 2) with pytest.raises(release_download.ReleaseDownloadError, match="requested read bound"): - release_download._read_chunk(OverBoundReader(), 2) + release_download._read_chunk(over_bound_reader, 2) def test_oversized_declared_length_fails_before_body_read(monkeypatch: pytest.MonkeyPatch) -> None: @@ -1203,7 +1207,7 @@ def test_checksum_metadata_installers_wrap_asset_stage_exhaustion( detail = "exhausted 3 bounded attempts: HTTP 503" opener = SequenceOpener( DownloadResponse(checksum_metadata), - release_download.ReleaseDownloadError(detail), + DownloadResponse(release_download.ReleaseDownloadError(detail)), ) monkeypatch.setattr(tool, "urlopen", opener) diff --git a/implementations/python/tests/test_repo_policy_tools.py b/implementations/python/tests/test_repo_policy_tools.py index 28cfa6a9..69559834 100644 --- a/implementations/python/tests/test_repo_policy_tools.py +++ b/implementations/python/tests/test_repo_policy_tools.py @@ -2562,9 +2562,7 @@ def test_osv_scanner_download_timeout_fails_closed(monkeypatch: pytest.MonkeyPat def timeout(_url: str, **kwargs: object) -> io.BytesIO: assert kwargs["timeout"] == 60 - raise osv_scanner_tool.ReleaseDownloadError( - "release download exhausted 3 bounded attempts" - ) + raise osv_scanner_tool.ReleaseDownloadError("release download exhausted 3 bounded attempts") monkeypatch.setattr(osv_scanner_tool, "urlopen", timeout) From 4a107f7a05f9304ad7a51e22db2904b4cfd5d1ae Mon Sep 17 00:00:00 2001 From: Yernat Yestekov <2068106+doublewhy@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:21:19 -0700 Subject: [PATCH 4/4] fix: consolidate release download boundary (#1137) --- .../issue-1137-ci-release-download-retries.md | 62 ++++-- docs/requirements/GOV-913/requirement.md | 2 + implementations/python/pyproject.toml | 1 - .../python/tests/test_http_download.py | 181 +++++++++++------- .../python/tests/test_release_download.py | 65 +++++++ tools/gitleaks_tool.py | 9 +- tools/http_download.py | 107 ++++------- tools/osv_scanner_tool.py | 3 +- tools/policy/conftest_tool.py | 9 +- tools/release_download.py | 13 +- tools/vale_tool.py | 5 +- 11 files changed, 287 insertions(+), 170 deletions(-) diff --git a/docs/decisions/issue-1137-ci-release-download-retries.md b/docs/decisions/issue-1137-ci-release-download-retries.md index 2cbd2526..01ad20fe 100644 --- a/docs/decisions/issue-1137-ci-release-download-retries.md +++ b/docs/decisions/issue-1137-ci-release-download-retries.md @@ -9,25 +9,36 @@ Issue: #1137. Requirement: GOV-913. OpenRAE's required verification lanes acquire pinned Conftest, Vale, Gitleaks, and OSV Scanner artifacts from their canonical GitHub Releases repositories. The owning installers validate checksums before executing or extracting those -artifacts. During review of several independent pull requests, GitHub returned +artifacts. When #1137 was captured during review of several independent pull +requests, GitHub returned temporary disconnects and HTTP 503 responses while acquiring all four tool families. A single such response failed the lane before it could inspect the pull request's code; rerunning the unchanged commit succeeded once the release service recovered. -The integrity checks were behaving correctly, but acquisition had no bounded -transient-failure policy. This note defines that missing reliability boundary. -It changes neither the verification graph nor the meaning of a passing gate. +While this remediation was in flight, PR #1113 merged a shared +`tools/http_download.py` helper that makes up to five attempts for a small +HTTP-status set and broad `OSError` failures. That change absorbs common +single-attempt disconnects, but it does not govern automatic redirects, apply +one wall-clock deadline across status, header, framing, and body reads, bound +every response, or prevent chained transport exceptions from reaching +diagnostics. This note therefore defines the hardened acquisition boundary +that replaces that coarse retry implementation. It changes neither the +verification graph nor the meaning of a passing gate. ## Existing-Surface Review The review covered the four repository-local installers, their version pins, -checksum sources, archive extraction, cache paths, nox callers, and required CI -jobs. The installers previously used separate direct `urllib` calls. There was -no shared release-acquisition helper to extend. Runtime OCI/module downloads, -vocabulary-source refreshes, Isabelle's separately governed official-mirror -policy, and GitHub Actions artifact transfer are different trust or ownership -boundaries and remain out of scope. +checksum sources, archive extraction, cache paths, nox callers, required CI +jobs, and the shared `tools/http_download.py` added by #1113. Rather than leave +two retry implementations, `tools/release_download.py` is the sole owner of +retry, redirect, deadline, framing, and response-size policy. The earlier +`download_bytes` entry point remains only as a compatibility facade: it +delegates to the governed implementation and caps legacy attempt, timeout, and +size options at the new policy. Runtime OCI/module downloads, vocabulary-source +refreshes, Isabelle's separately governed official-mirror policy, and GitHub +Actions artifact transfer are different trust or ownership boundaries and +remain out of scope. PR #1121 independently hardens OSV Scanner's cache type, identity, digest, atomic-install, and size checks. The retry helper composes below that work: it @@ -36,7 +47,10 @@ the OSV installer remains the sole owner of cache and checksum validation. ## Chosen Policy -All four installers bind their existing `urlopen` seam to one internal helper. +All four installers bind their release reads to one internal helper. The +compatibility `download_bytes` entry point delegates to the same helper and has +no independent retry or redirect implementation. + The initial request accepts only exact HTTPS paths under these GitHub Release families: @@ -58,9 +72,10 @@ bodies are closed without being read. The helper performs at most three attempts. Every connection hop and every bounded body read receives a finite 60-second socket timeout capped by the -remaining 190-second total deadline. Exponential delays are deterministic and -capped, and a deadline-aware socket reader reapplies the remaining bound to -every status-line, header, chunk-framing, and body buffer fill. No read, +remaining 190-second total deadline. A caller may lower that per-operation +timeout but cannot raise the policy cap. Exponential delays are deterministic +and capped, and a deadline-aware socket reader reapplies the remaining bound +to every status-line, header, chunk-framing, and body buffer fill. No read, redirect hop, sleep, or new attempt may continue after the deadline. Declared response lengths are checked before and after streaming; ambiguous framing and oversized bodies fail immediately, while an early EOF is classified as a @@ -90,6 +105,14 @@ fail closed when every transient attempt fails. ## Alternatives Rejected +- **Keep the #1113 helper unchanged.** Its automatic redirect behavior, broad + transport retry class, per-operation timeout, unbounded default reads, and + chained exception diagnostics do not satisfy the issue's trust and total-time + acceptance criteria. +- **Retain both retry implementations.** A permissive legacy path beside the + governed path would leave future callers with two conflicting trust + boundaries. The compatibility API therefore delegates to the sole hardened + implementation and cannot raise its limits. - **Retry each installer independently.** This would create four subtly different status lists, delays, deadlines, and diagnostics that could drift. - **Retry every `URLError` or every HTTP failure.** Certificate, DNS-policy, @@ -113,10 +136,13 @@ success after transient HTTP and transport failures, incomplete declared responses, delta and HTTP-date `Retry-After`, read/backoff/deadline caps, exact exhaustion, non-retryable HTTP and URL failures, response-size rejection, sanitized diagnostics, and no second request after checksum or archive-type -failure. Local scripted HTTP servers verify that an early EOF never returns -partial bytes, redirect bodies are not drained, and a trickling body cannot -extend the total deadline. The same scripted transport verifies that trickled -redirect headers and chunk trailers cannot extend that deadline and that a +failure. Malformed redirect authorities, including parser-error and Unicode +normalization cases, are rejected through the same sanitized public error +boundary without reflecting their text. Local scripted HTTP servers verify +that an early EOF never returns partial bytes, redirect bodies are not drained, +and a trickling body cannot extend the total deadline. The same scripted +transport verifies that trickled redirect headers and chunk trailers cannot +extend that deadline and that a malformed status line cannot leak upstream text. A binding test ensures all four installers continue to share the helper, and clean-cache smoke tests exercise the live current GitHub redirect chain for all four pinned tools. diff --git a/docs/requirements/GOV-913/requirement.md b/docs/requirements/GOV-913/requirement.md index 50738cad..972b4ef7 100644 --- a/docs/requirements/GOV-913/requirement.md +++ b/docs/requirements/GOV-913/requirement.md @@ -27,11 +27,13 @@ Requirement inventory expansion. Reusable ecosystem assets need explicit trust a - TESTS → TEST `implementations/python/tests/test_reusable_asset_trust_policy.py` (Reusable-asset trust policy contract tests) - IMPLEMENTS → GITHUB_ISSUE `1137` (Bounded retries for pinned CI tool downloads) - IMPLEMENTS → CODE_FILE `tools/release_download.py` (Approved-origin, bounded transient retry policy) +- IMPLEMENTS → CODE_FILE `tools/http_download.py` (Compatibility facade for the governed release boundary) - IMPLEMENTS → CODE_FILE `tools/policy/conftest_tool.py` (Conftest release acquisition adapter) - IMPLEMENTS → CODE_FILE `tools/vale_tool.py` (Vale release acquisition adapter) - IMPLEMENTS → CODE_FILE `tools/gitleaks_tool.py` (Gitleaks release acquisition adapter) - IMPLEMENTS → CODE_FILE `tools/osv_scanner_tool.py` (OSV Scanner release acquisition adapter) - TESTS → TEST `implementations/python/tests/test_release_download.py` (Retry classification, bounds, and integrity non-retry regressions) +- TESTS → TEST `implementations/python/tests/test_http_download.py` (Compatibility delegation and policy-cap regressions) - DOCUMENTS → DOCUMENTATION `docs/decisions/issue-1137-ci-release-download-retries.md` (Release-download retry decision) - IMPLEMENTS → GITHUB_ISSUE `115` (Trust & integrity of reusable assets (GOV-913)) - IMPLEMENTS → GITHUB_ISSUE `1098` (Upgrade vulnerable Click and cryptography locks and gate OSV findings) diff --git a/implementations/python/pyproject.toml b/implementations/python/pyproject.toml index 788f05f5..efb701f4 100644 --- a/implementations/python/pyproject.toml +++ b/implementations/python/pyproject.toml @@ -182,7 +182,6 @@ ignore = [ [tool.ruff.lint.per-file-ignores] "tests/**" = ["S101", "S105", "S106", "S108", "E402", "SIM105", "F841"] -"tools/http_download.py" = ["S310"] # validates absolute HTTPS before opening pinned release assets "packages/raes_contracts/contracts/__init__.py" = ["F403", "F405"] # intentional package re-exports "packages/raes_cli/**" = ["B008"] # typer requires function calls in defaults "packages/raes_runtime/control_plane.py" = ["S112"] # intentional exception suppression diff --git a/implementations/python/tests/test_http_download.py b/implementations/python/tests/test_http_download.py index a292028c..9c1c2be5 100644 --- a/implementations/python/tests/test_http_download.py +++ b/implementations/python/tests/test_http_download.py @@ -1,94 +1,145 @@ -"""Tests for bounded repository-tool downloads.""" +"""Compatibility tests for governed repository-tool downloads.""" from __future__ import annotations -from http.client import RemoteDisconnected -from urllib.error import HTTPError +import io import pytest -from tools.http_download import download_bytes +import tools.http_download as http_download +from tools.release_download import DEFAULT_RELEASE_RETRY_POLICY, ReleaseDownloadError, ReleaseRetryPolicy +RELEASE_URL = "https://github.com/gitleaks/gitleaks/releases/download/v8.30.1/gitleaks_8.30.1_checksums.txt" -class _Response: - def __init__(self, payload: bytes) -> None: - self.payload = payload - def __enter__(self) -> _Response: - return self +def test_download_bytes_delegates_to_the_governed_release_boundary( + monkeypatch: pytest.MonkeyPatch, +) -> None: + observed: dict[str, object] = {} - def __exit__(self, *args: object) -> None: - return None + def governed_open(url: str, *, timeout: float, policy: ReleaseRetryPolicy) -> io.BytesIO: + observed.update(url=url, timeout=timeout, policy=policy) + return io.BytesIO(b"verified asset") - def read(self, size: int = -1) -> bytes: - return self.payload if size < 0 else self.payload[:size] + monkeypatch.setattr(http_download, "retrying_urlopen", governed_open) + assert http_download.download_bytes(RELEASE_URL, description="gitleaks checksums") == b"verified asset" + assert observed["url"] == RELEASE_URL + assert observed["timeout"] == 60 + policy = observed["policy"] + assert isinstance(policy, ReleaseRetryPolicy) + assert policy.max_attempts == DEFAULT_RELEASE_RETRY_POLICY.max_attempts + assert policy.maximum_response_bytes == DEFAULT_RELEASE_RETRY_POLICY.maximum_response_bytes -def test_download_retries_transient_disconnects_with_bounded_backoff() -> None: - calls = 0 - delays: list[float] = [] - def opener(_url: str, *, timeout: float) -> _Response: - nonlocal calls - calls += 1 - assert timeout == 60 - if calls < 3: - raise RemoteDisconnected("transient release-asset disconnect") - return _Response(b"verified asset") +def test_download_bytes_preserves_explicit_attempt_timeout_and_size_options( + monkeypatch: pytest.MonkeyPatch, +) -> None: + observed: dict[str, object] = {} + + def governed_open(url: str, *, timeout: float, policy: ReleaseRetryPolicy) -> io.BytesIO: + observed.update(url=url, timeout=timeout, policy=policy) + return io.BytesIO(b"four") + + monkeypatch.setattr(http_download, "retrying_urlopen", governed_open) assert ( - download_bytes( - "https://example.invalid/pinned-tool", - description="pinned tool", - _opener=opener, - _sleeper=delays.append, + http_download.download_bytes( + RELEASE_URL, + description="gitleaks checksums", + attempts=2, + timeout_seconds=12, + max_bytes=4, ) - == b"verified asset" + == b"four" ) - assert calls == 3 - assert delays == [1.0, 2.0] + assert observed["url"] == RELEASE_URL + assert observed["timeout"] == 12 + policy = observed["policy"] + assert isinstance(policy, ReleaseRetryPolicy) + assert policy.max_attempts == 2 + assert policy.maximum_response_bytes == 4 + +def test_download_bytes_caps_legacy_options_at_the_governed_policy( + monkeypatch: pytest.MonkeyPatch, +) -> None: + observed: dict[str, object] = {} -def test_download_exhausts_a_bounded_retry_window() -> None: - delays: list[float] = [] + def governed_open(url: str, *, timeout: float, policy: ReleaseRetryPolicy) -> io.BytesIO: + observed.update(url=url, timeout=timeout, policy=policy) + return io.BytesIO(b"verified asset") - def opener(_url: str, *, timeout: float) -> _Response: - raise RemoteDisconnected("persistent release-asset disconnect") + monkeypatch.setattr(http_download, "retrying_urlopen", governed_open) - with pytest.raises(RuntimeError, match=r"after 5 attempt\(s\): RemoteDisconnected"): - download_bytes( - "https://example.invalid/pinned-tool", - description="pinned tool", - _opener=opener, - _sleeper=delays.append, + assert ( + http_download.download_bytes( + RELEASE_URL, + description="gitleaks checksums", + attempts=99, + timeout_seconds=999, + max_bytes=DEFAULT_RELEASE_RETRY_POLICY.maximum_response_bytes + 1, ) + == b"verified asset" + ) + assert observed["timeout"] == DEFAULT_RELEASE_RETRY_POLICY.request_timeout_seconds + policy = observed["policy"] + assert isinstance(policy, ReleaseRetryPolicy) + assert policy.max_attempts == DEFAULT_RELEASE_RETRY_POLICY.max_attempts + assert policy.maximum_response_bytes == DEFAULT_RELEASE_RETRY_POLICY.maximum_response_bytes + + +@pytest.mark.parametrize( + "options", + [ + {"attempts": 0}, + {"max_bytes": -1}, + {"timeout_seconds": 0}, + ], +) +def test_download_bytes_rejects_invalid_compatibility_options_before_network( + monkeypatch: pytest.MonkeyPatch, + options: dict[str, int], +) -> None: + monkeypatch.setattr( + http_download, + "retrying_urlopen", + lambda *_args, **_kwargs: pytest.fail("invalid options reached the governed downloader"), + ) - assert delays == [1.0, 2.0, 4.0, 8.0] + with pytest.raises(ValueError): + http_download.download_bytes(RELEASE_URL, description="gitleaks checksums", **options) -def test_download_does_not_retry_non_transient_http_status() -> None: - calls = 0 +def test_download_bytes_preserves_a_zero_byte_caller_bound( + monkeypatch: pytest.MonkeyPatch, +) -> None: + observed: dict[str, object] = {} - def opener(url: str, *, timeout: float) -> _Response: - nonlocal calls - calls += 1 - raise HTTPError(url, 404, "not found", {}, None) + def governed_open(_url: str, *, timeout: float, policy: ReleaseRetryPolicy) -> io.BytesIO: + observed.update(timeout=timeout, policy=policy) + return io.BytesIO(b"x") - with pytest.raises(RuntimeError, match=r"after 1 attempt\(s\): HTTPError"): - download_bytes( - "https://example.invalid/missing-tool", - description="missing tool", - _opener=opener, - _sleeper=lambda _delay: None, - ) - assert calls == 1 + monkeypatch.setattr(http_download, "retrying_urlopen", governed_open) + with pytest.raises(RuntimeError, match="gitleaks checksums exceeds the download limit"): + http_download.download_bytes(RELEASE_URL, description="gitleaks checksums", max_bytes=0) -def test_download_enforces_the_requested_size_bound() -> None: - with pytest.raises(RuntimeError, match="exceeds the download limit"): - download_bytes( - "https://example.invalid/pinned-tool", - description="pinned tool", - max_bytes=3, - _opener=lambda _url, **_kwargs: _Response(b"four"), - _sleeper=lambda _delay: None, - ) + policy = observed["policy"] + assert isinstance(policy, ReleaseRetryPolicy) + assert policy.maximum_response_bytes == 1 + + +def test_download_bytes_wraps_only_the_stable_governed_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + detail = "release download exhausted 3 bounded attempts: HTTP 503" + + def fail(_url: str, **_kwargs: object) -> io.BytesIO: + raise ReleaseDownloadError(detail) + + monkeypatch.setattr(http_download, "retrying_urlopen", fail) + + with pytest.raises(RuntimeError, match="failed to download gitleaks checksums") as raised: + http_download.download_bytes(RELEASE_URL, description="gitleaks checksums") + + assert detail in str(raised.value) diff --git a/implementations/python/tests/test_release_download.py b/implementations/python/tests/test_release_download.py index 24848f95..f72e822e 100644 --- a/implementations/python/tests/test_release_download.py +++ b/implementations/python/tests/test_release_download.py @@ -415,6 +415,30 @@ def test_duplicate_redirect_location_fails_closed_without_following( assert response.offset == 0 +@pytest.mark.parametrize( + "location", + [ + "https://[credential-do-not-log", + "https://evil.example／credential-do-not-log/path", + ], +) +def test_malformed_redirect_authority_is_sanitized_without_retry( + monkeypatch: pytest.MonkeyPatch, + location: str, +) -> None: + response = DownloadResponse(b"not read", status=302, headers={"Location": location}) + opener = SequenceOpener(response, DownloadResponse(b"not reached")) + clock = _install_network(monkeypatch, opener) + + with pytest.raises(release_download.ReleaseDownloadError, match="unapproved redirect target") as raised: + release_download.retrying_urlopen(RELEASE_URL) + + assert "credential-do-not-log" not in str(raised.value) + assert len(opener.calls) == 1 + assert response.offset == 0 + assert clock.sleeps == [] + + def test_redirect_hop_limit_fails_closed_before_following(monkeypatch: pytest.MonkeyPatch) -> None: redirect = DownloadResponse(b"", status=302, headers={"Location": RELEASE_ASSET_URL}) opener = SequenceOpener(redirect, DownloadResponse(b"not reached")) @@ -1094,6 +1118,19 @@ def test_invalid_per_call_timeout_fails_before_network(monkeypatch: pytest.Monke assert opener.calls == [] +@pytest.mark.parametrize(("requested", "expected"), [(12.0, 12.0), (999.0, 60.0)]) +def test_per_call_timeout_can_reduce_but_not_raise_the_policy_cap( + monkeypatch: pytest.MonkeyPatch, + requested: float, + expected: float, +) -> None: + opener = SequenceOpener(DownloadResponse(b"payload")) + _install_network(monkeypatch, opener) + + assert release_download.retrying_urlopen(RELEASE_URL, timeout=requested).read() == b"payload" + assert opener.calls == [(RELEASE_URL, expected)] + + def test_retry_after_parser_rejects_ambiguous_values() -> None: assert release_download._retry_after_seconds({}, now=0) is None assert release_download._retry_after_seconds(object(), now=0) is None @@ -1178,6 +1215,34 @@ def fail(_url: str, **_kwargs: object) -> None: assert detail in str(raised.value) +@pytest.mark.parametrize( + ("tool", "ensure"), + [ + (conftest_tool, conftest_tool.ensure_conftest), + (vale_tool, vale_tool.ensure_vale), + (gitleaks_tool, gitleaks_tool.ensure_gitleaks), + (osv_scanner_tool, osv_scanner_tool.ensure_osv_scanner), + ], +) +def test_installers_only_reclassify_normalized_boundary_failures( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + tool: ModuleType, + ensure: Callable[[Path], Path], +) -> None: + unexpected = URLError("outside the normalized release boundary") + + def fail(_url: str, **_kwargs: object) -> None: + raise unexpected + + monkeypatch.setattr(tool, "urlopen", fail) + + with pytest.raises(URLError) as raised: + ensure(tmp_path) + + assert raised.value is unexpected + + @pytest.mark.parametrize( ("tool", "ensure", "asset_name", "diagnostic"), [ diff --git a/tools/gitleaks_tool.py b/tools/gitleaks_tool.py index 6e128c7b..555d5453 100644 --- a/tools/gitleaks_tool.py +++ b/tools/gitleaks_tool.py @@ -7,7 +7,6 @@ import tempfile from hashlib import sha256 from pathlib import Path -from urllib.error import URLError from tools.release_download import ReleaseDownloadError from tools.release_download import retrying_urlopen as urlopen @@ -63,9 +62,9 @@ def ensure_gitleaks(repo_root: Path = REPO_ROOT, *, version: str = GITLEAKS_VERS checksums_url = f"{base_url}/{_checksums_asset_name(version)}" try: - with urlopen(checksums_url) as response: # noqa: S310 - pinned HTTPS release asset + with urlopen(checksums_url) as response: checksums_text = response.read().decode("utf-8") - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download gitleaks checksums from {checksums_url}: {exc}") from exc expected_checksum = None @@ -78,9 +77,9 @@ def ensure_gitleaks(repo_root: Path = REPO_ROOT, *, version: str = GITLEAKS_VERS raise RuntimeError(f"missing checksum for gitleaks asset {asset_name}") try: - with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + with urlopen(asset_url) as response: archive_bytes = response.read() - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download gitleaks from {asset_url}: {exc}") from exc actual_checksum = sha256(archive_bytes).hexdigest() diff --git a/tools/http_download.py b/tools/http_download.py index d9adcaf3..a411f524 100644 --- a/tools/http_download.py +++ b/tools/http_download.py @@ -1,89 +1,62 @@ -"""Bounded resilient downloads for checksum-pinned repository tools.""" +"""Compatibility entry point for governed pinned-tool release downloads. -from __future__ import annotations - -import time -from collections.abc import Callable -from typing import Protocol -from urllib.error import HTTPError -from urllib.parse import urlsplit -from urllib.request import Request, build_opener - -_RETRY_DELAYS_SECONDS = (1.0, 2.0, 4.0, 8.0) -_RETRYABLE_HTTP_STATUS = frozenset({408, 429, 500, 502, 503, 504}) - - -class _Response(Protocol): - def __enter__(self) -> _Response: ... +The retry, redirect, deadline, framing, and response-size policy has one owner: +``tools.release_download``. This module preserves the ``download_bytes`` API +introduced with the repository-tool installers while delegating every network +request to that boundary. +""" - def __exit__(self, *args: object) -> None: ... - - def read(self, size: int = -1) -> bytes: ... +from __future__ import annotations +from dataclasses import replace -def _open_https(url: str, *, timeout: float) -> _Response: - parsed = urlsplit(url) - if parsed.scheme != "https" or not parsed.hostname: - raise ValueError("repository-tool downloads require an absolute HTTPS URL") - request = Request(url, headers={"User-Agent": "RAES-pinned-tool-installer"}) - return build_opener().open(request, timeout=timeout) +from tools.release_download import ( + DEFAULT_RELEASE_RETRY_POLICY, + ReleaseDownloadError, + ReleaseRetryPolicy, + retrying_urlopen, +) -def _validate_download_options(*, attempts: int, max_bytes: int | None) -> None: +def _compatibility_policy(*, attempts: int, max_bytes: int | None) -> ReleaseRetryPolicy: if attempts < 1: raise ValueError("download attempts must be positive") if max_bytes is not None and max_bytes < 0: raise ValueError("download size bound must be non-negative") - - -def _read_response( - response: _Response, - *, - max_bytes: int | None, - description: str, - url: str, -) -> bytes: - if max_bytes is None: - return response.read() - payload = response.read(max_bytes + 1) - if len(payload) > max_bytes: - raise RuntimeError(f"{description} from {url} exceeds the download limit") - return payload + response_bound = DEFAULT_RELEASE_RETRY_POLICY.maximum_response_bytes + if max_bytes is not None: + # The governed policy requires a positive transport bound. A caller's + # zero-byte bound is enforced immediately after the bounded read. + response_bound = min(response_bound, max(1, max_bytes)) + return replace( + DEFAULT_RELEASE_RETRY_POLICY, + max_attempts=min(attempts, DEFAULT_RELEASE_RETRY_POLICY.max_attempts), + maximum_response_bytes=response_bound, + ) def download_bytes( url: str, *, description: str, - attempts: int = 5, + attempts: int = DEFAULT_RELEASE_RETRY_POLICY.max_attempts, timeout_seconds: float = 60, max_bytes: int | None = None, - _opener: Callable[..., _Response] | None = None, - _sleeper: Callable[[float], None] | None = None, ) -> bytes: - """Download bytes with bounded retries for transient transport failures.""" - - _validate_download_options(attempts=attempts, max_bytes=max_bytes) - opener = _opener or _open_https - sleeper = _sleeper or time.sleep - last_error: BaseException | None = None - for attempt in range(attempts): - try: - with opener(url, timeout=timeout_seconds) as response: - return _read_response(response, max_bytes=max_bytes, description=description, url=url) - except HTTPError as exc: - last_error = exc - if exc.code not in _RETRYABLE_HTTP_STATUS: - break - except OSError as exc: - last_error = exc - if attempt + 1 < attempts: - sleeper(_RETRY_DELAYS_SECONDS[min(attempt, len(_RETRY_DELAYS_SECONDS) - 1)]) - - assert last_error is not None - raise RuntimeError( - f"failed to download {description} from {url} after {attempt + 1} attempt(s): {type(last_error).__name__}" - ) from last_error + """Return bytes acquired through the single governed release boundary.""" + + policy = _compatibility_policy(attempts=attempts, max_bytes=max_bytes) + if timeout_seconds <= 0: + raise ValueError("download timeout must be positive") + request_timeout = min(timeout_seconds, DEFAULT_RELEASE_RETRY_POLICY.request_timeout_seconds) + try: + with retrying_urlopen(url, timeout=request_timeout, policy=policy) as response: + payload = response.read() + except ReleaseDownloadError as exc: + raise RuntimeError(f"failed to download {description}: {exc}") from exc + if max_bytes is not None and len(payload) > max_bytes: + raise RuntimeError(f"{description} exceeds the download limit") + return payload __all__ = ["download_bytes"] diff --git a/tools/osv_scanner_tool.py b/tools/osv_scanner_tool.py index 1cd83756..816ebfd0 100644 --- a/tools/osv_scanner_tool.py +++ b/tools/osv_scanner_tool.py @@ -8,7 +8,6 @@ from enum import StrEnum from hashlib import sha256 from pathlib import Path -from urllib.error import URLError from tools.release_download import ReleaseDownloadError from tools.release_download import retrying_urlopen as urlopen @@ -197,7 +196,7 @@ def ensure_osv_scanner(repo_root: Path = REPO_ROOT, *, version: str = OSV_SCANNE try: with urlopen(asset_url, timeout=_DOWNLOAD_TIMEOUT_SECONDS) as response: binary_bytes = response.read() - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download osv-scanner from {asset_url}: {exc}") from exc if len(binary_bytes) > _MAX_BINARY_BYTES: raise RuntimeError(f"osv-scanner asset {asset_name} exceeds the download limit") diff --git a/tools/policy/conftest_tool.py b/tools/policy/conftest_tool.py index b548814d..d2bf1f26 100644 --- a/tools/policy/conftest_tool.py +++ b/tools/policy/conftest_tool.py @@ -9,7 +9,6 @@ import tempfile from hashlib import sha256 from pathlib import Path -from urllib.error import URLError from tools.release_download import ReleaseDownloadError from tools.release_download import retrying_urlopen as urlopen @@ -59,9 +58,9 @@ def ensure_conftest(repo_root: Path = REPO_ROOT, *, version: str = CONTFEST_VERS checksums_url = f"{base_url}/checksums.txt" try: - with urlopen(checksums_url) as response: # noqa: S310 - pinned HTTPS release asset + with urlopen(checksums_url) as response: checksums_text = response.read().decode("utf-8") - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download conftest checksums from {checksums_url}: {exc}") from exc expected_checksum = None @@ -74,9 +73,9 @@ def ensure_conftest(repo_root: Path = REPO_ROOT, *, version: str = CONTFEST_VERS raise RuntimeError(f"missing checksum for conftest asset {asset_name}") try: - with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + with urlopen(asset_url) as response: archive_bytes = response.read() - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download conftest from {asset_url}: {exc}") from exc actual_checksum = sha256(archive_bytes).hexdigest() diff --git a/tools/release_download.py b/tools/release_download.py index b39e89eb..d4e8a4d9 100644 --- a/tools/release_download.py +++ b/tools/release_download.py @@ -383,8 +383,12 @@ def _approved_release_asset_url(url: str) -> bool: def _approved_vale_relocation(source_url: str, target_url: str) -> bool: - source = urlsplit(source_url) - target = urlsplit(target_url) + parsed_source = _parsed_url_with_port(source_url) + parsed_target = _parsed_url_with_port(target_url) + if parsed_source is None or parsed_target is None: + return False + source, _source_port = parsed_source + target, _target_port = parsed_target if not source.path.startswith(_VALE_LEGACY_PATH_PREFIX): return False source_suffix = source.path.removeprefix("/errata-ai/vale") @@ -769,9 +773,10 @@ def retrying_urlopen( if not _approved_url(url): raise ReleaseDownloadError("release download URL is not an approved pinned-tool origin") - request_timeout = policy.request_timeout_seconds if timeout is None else timeout - if request_timeout <= 0: + requested_timeout = policy.request_timeout_seconds if timeout is None else timeout + if requested_timeout <= 0: raise ValueError("timeout must be positive") + request_timeout = min(requested_timeout, policy.request_timeout_seconds) deadline = _monotonic() + policy.total_timeout_seconds attempts_made = 0 diff --git a/tools/vale_tool.py b/tools/vale_tool.py index 70bb66b5..e84f1c77 100644 --- a/tools/vale_tool.py +++ b/tools/vale_tool.py @@ -7,7 +7,6 @@ import tarfile from hashlib import sha256 from pathlib import Path -from urllib.error import URLError from tools.release_download import ReleaseDownloadError from tools.release_download import retrying_urlopen as urlopen @@ -85,9 +84,9 @@ def ensure_vale(repo_root: Path = REPO_ROOT, *, version: str = VALE_VERSION) -> base_url = _release_base_url(version) asset_url = f"{base_url}/{asset_name}" try: - with urlopen(asset_url) as response: # noqa: S310 - pinned HTTPS release asset + with urlopen(asset_url) as response: archive_bytes = response.read() - except (URLError, ReleaseDownloadError) as exc: + except ReleaseDownloadError as exc: raise RuntimeError(f"failed to download Vale from {asset_url}: {exc}") from exc actual = sha256(archive_bytes).hexdigest() if actual != expected: