From 3d4c90f1160c807e5d5f068255a4baf1cd4da478 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:17:09 -0500 Subject: [PATCH 1/6] feat(identity): add bounded GitHub OIDC transport --- src/odoo_forge/identity_github/transport.py | 99 +++++++++++++ tests/identity_github/test_transport.py | 150 ++++++++++++++++++++ 2 files changed, 249 insertions(+) create mode 100644 src/odoo_forge/identity_github/transport.py create mode 100644 tests/identity_github/test_transport.py diff --git a/src/odoo_forge/identity_github/transport.py b/src/odoo_forge/identity_github/transport.py new file mode 100644 index 0000000..98705bc --- /dev/null +++ b/src/odoo_forge/identity_github/transport.py @@ -0,0 +1,99 @@ +"""Bounded, injectable transport for GitHub OIDC metadata and JWKS.""" + +from __future__ import annotations + +import json +import urllib.request +from typing import Protocol, cast, runtime_checkable +from urllib.parse import urlsplit + +DEFAULT_TIMEOUT_SECONDS = 10.0 +MAX_RESPONSE_BYTES = 1_048_576 +_OPENID_CONFIGURATION_PATH = "/.well-known/openid-configuration" +_JSON_HEADERS = {"Accept": "application/json"} + + +@runtime_checkable +class GitHubOidcTransport(Protocol): + def get_metadata(self, issuer: str) -> dict[str, object]: + """Retrieve the issuer's OpenID configuration.""" + ... + + def get_jwks(self, jwks_uri: str) -> dict[str, object]: + """Retrieve the issuer's JSON Web Key Set.""" + ... + + +class GitHubOidcHttpsTransport: + """Retrieve GitHub OIDC JSON documents using bounded HTTPS requests.""" + + def __init__( + self, + *, + timeout: float = DEFAULT_TIMEOUT_SECONDS, + max_response_bytes: int = MAX_RESPONSE_BYTES, + ) -> None: + if timeout <= 0: + raise ValueError("timeout must be greater than zero") + if max_response_bytes <= 0: + raise ValueError("response size limit must be greater than zero") + self._timeout = timeout + self._max_response_bytes = max_response_bytes + + def get_metadata(self, issuer: str) -> dict[str, object]: + """Retrieve the standard OpenID configuration for an HTTPS issuer.""" + issuer = self._validate_https_url(issuer) + return self._get_json(f"{issuer.rstrip('/')}{_OPENID_CONFIGURATION_PATH}") + + def get_jwks(self, jwks_uri: str) -> dict[str, object]: + """Retrieve a JSON Web Key Set from an HTTPS URL.""" + return self._get_json(self._validate_https_url(jwks_uri)) + + def _get_json(self, url: str) -> dict[str, object]: + return self._decode_json(self._read_response(url)) + + def _read_response(self, url: str) -> bytes: + request = urllib.request.Request( + url, + method="GET", + headers=_JSON_HEADERS, + ) + try: + with urllib.request.urlopen(request, timeout=self._timeout) as response: # noqa: S310 + body = response.read(self._max_response_bytes + 1) + except Exception as exc: + raise RuntimeError("GitHub OIDC transport request failed") from exc + if not isinstance(body, bytes): + raise RuntimeError("GitHub OIDC transport returned an invalid response") + if len(body) > self._max_response_bytes: + raise RuntimeError("GitHub OIDC transport response exceeds size limit") + return body + + @staticmethod + def _decode_json(body: bytes) -> dict[str, object]: + try: + payload = json.loads(body) + except (json.JSONDecodeError, UnicodeDecodeError) as exc: + raise RuntimeError("GitHub OIDC transport returned malformed JSON") from exc + if not isinstance(payload, dict): + raise RuntimeError("GitHub OIDC transport returned malformed JSON") + return cast(dict[str, object], payload) + + @staticmethod + def _validate_https_url(url: str) -> str: + try: + parsed = urlsplit(url) + except ValueError as exc: + raise ValueError("GitHub OIDC transport requires an HTTPS URL") from exc + if ( + parsed.scheme != "https" + or not parsed.hostname + or parsed.username is not None + or parsed.password is not None + or parsed.fragment + ): + raise ValueError("GitHub OIDC transport requires an HTTPS URL") + return url + + +__all__ = ["GitHubOidcHttpsTransport", "GitHubOidcTransport"] diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py new file mode 100644 index 0000000..da16d59 --- /dev/null +++ b/tests/identity_github/test_transport.py @@ -0,0 +1,150 @@ +import json +import urllib.error +import urllib.request +from collections.abc import Callable + +import pytest + +from odoo_forge.identity_github.transport import ( + DEFAULT_TIMEOUT_SECONDS, + MAX_RESPONSE_BYTES, + GitHubOidcHttpsTransport, + GitHubOidcTransport, +) + + +class _FakeTransport: + def get_metadata(self, issuer: str) -> dict[str, object]: + return {"issuer": issuer} + + def get_jwks(self, jwks_uri: str) -> dict[str, object]: + return {"jwks_uri": jwks_uri} + + +class _Response: + def __init__(self, body: bytes, reads: list[int]) -> None: + self._body = body + self._reads = reads + + def __enter__(self) -> "_Response": + return self + + def __exit__(self, *args: object) -> None: + return None + + def read(self, amount: int = -1) -> bytes: + self._reads.append(amount) + return self._body + + +def _urlopen_returning( + body: bytes, + calls: list[tuple[str, float]], + reads: list[int], +) -> Callable[..., _Response]: + def urlopen(request: urllib.request.Request, timeout: float) -> _Response: + calls.append((request.full_url, timeout)) + return _Response(body, reads) + + return urlopen + + +def test_transport_protocol_is_runtime_checkable_and_satisfied_structurally() -> None: + assert isinstance(_FakeTransport(), GitHubOidcTransport) + + +def test_non_https_urls_are_rejected_without_network(monkeypatch: pytest.MonkeyPatch) -> None: + def fail_if_called(*args: object, **kwargs: object) -> None: + raise AssertionError("network must not be called") + + monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + transport = GitHubOidcHttpsTransport() + + with pytest.raises(ValueError, match="HTTPS"): + transport.get_metadata("http://issuer.example") + with pytest.raises(ValueError, match="HTTPS"): + transport.get_jwks("file:///tmp/keys.json") + + +def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[tuple[str, float]] = [] + reads: list[int] = [] + monkeypatch.setattr( + urllib.request, + "urlopen", + _urlopen_returning(b'{"issuer":"https://issuer.example"}', calls, reads), + ) + transport = GitHubOidcHttpsTransport(timeout=2.5) + + result = transport.get_metadata("https://issuer.example") + + assert result == {"issuer": "https://issuer.example"} + assert calls == [ + ( + "https://issuer.example/.well-known/openid-configuration", + 2.5, + ) + ] + assert reads == [MAX_RESPONSE_BYTES + 1] + + +def test_oversized_response_is_rejected_before_json_is_accepted( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[tuple[str, float]] = [] + reads: list[int] = [] + body = b"{" + b"x" * MAX_RESPONSE_BYTES + b"}" + monkeypatch.setattr( + urllib.request, + "urlopen", + _urlopen_returning(body, calls, reads), + ) + + with pytest.raises(RuntimeError, match="response exceeds size limit"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + + +def test_malformed_or_non_object_json_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: + calls: list[tuple[str, float]] = [] + reads: list[int] = [] + monkeypatch.setattr( + urllib.request, + "urlopen", + _urlopen_returning(b"not-json", calls, reads), + ) + + with pytest.raises(RuntimeError, match="malformed JSON"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + + monkeypatch.setattr( + urllib.request, + "urlopen", + _urlopen_returning(json.dumps(["not", "an", "object"]).encode(), calls, reads), + ) + with pytest.raises(RuntimeError, match="malformed JSON"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + + +def test_network_failures_are_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: + def fail(*args: object, **kwargs: object) -> None: + raise urllib.error.URLError("token=do-not-leak at https://private.example") + + monkeypatch.setattr(urllib.request, "urlopen", fail) + + with pytest.raises(RuntimeError, match="request failed") as error: + GitHubOidcHttpsTransport().get_jwks("https://private.example/keys") + + message = str(error.value) + assert "do-not-leak" not in message + assert "private.example" not in message + + +def test_constructor_rejects_unbounded_timeout_configuration() -> None: + with pytest.raises(ValueError, match="timeout"): + GitHubOidcHttpsTransport(timeout=0) + with pytest.raises(ValueError, match="response size"): + GitHubOidcHttpsTransport(max_response_bytes=0) + + assert DEFAULT_TIMEOUT_SECONDS > 0 From 1a0a5ee50efae8f988f107a74dd020f008cdabc1 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:24:38 -0500 Subject: [PATCH 2/6] fix(identity): harden OIDC transport boundaries --- src/odoo_forge/identity_github/transport.py | 17 +++++-- tests/identity_github/test_transport.py | 56 ++++++++++++++++++--- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/odoo_forge/identity_github/transport.py b/src/odoo_forge/identity_github/transport.py index 98705bc..2e740ea 100644 --- a/src/odoo_forge/identity_github/transport.py +++ b/src/odoo_forge/identity_github/transport.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import math import urllib.request from typing import Protocol, cast, runtime_checkable from urllib.parse import urlsplit @@ -33,8 +34,12 @@ def __init__( timeout: float = DEFAULT_TIMEOUT_SECONDS, max_response_bytes: int = MAX_RESPONSE_BYTES, ) -> None: - if timeout <= 0: + if isinstance(timeout, bool) or not isinstance(timeout, (int, float)): + raise ValueError("timeout must be a finite number greater than zero") + if not math.isfinite(timeout) or timeout <= 0: raise ValueError("timeout must be greater than zero") + if isinstance(max_response_bytes, bool) or not isinstance(max_response_bytes, int): + raise ValueError("response size limit must be a positive integer") if max_response_bytes <= 0: raise ValueError("response size limit must be greater than zero") self._timeout = timeout @@ -42,7 +47,7 @@ def __init__( def get_metadata(self, issuer: str) -> dict[str, object]: """Retrieve the standard OpenID configuration for an HTTPS issuer.""" - issuer = self._validate_https_url(issuer) + issuer = self._validate_https_url(issuer, allow_query=False) return self._get_json(f"{issuer.rstrip('/')}{_OPENID_CONFIGURATION_PATH}") def get_jwks(self, jwks_uri: str) -> dict[str, object]: @@ -60,9 +65,10 @@ def _read_response(self, url: str) -> bytes: ) try: with urllib.request.urlopen(request, timeout=self._timeout) as response: # noqa: S310 + self._validate_https_url(response.geturl()) body = response.read(self._max_response_bytes + 1) - except Exception as exc: - raise RuntimeError("GitHub OIDC transport request failed") from exc + except Exception: + raise RuntimeError("GitHub OIDC transport request failed") from None if not isinstance(body, bytes): raise RuntimeError("GitHub OIDC transport returned an invalid response") if len(body) > self._max_response_bytes: @@ -80,7 +86,7 @@ def _decode_json(body: bytes) -> dict[str, object]: return cast(dict[str, object], payload) @staticmethod - def _validate_https_url(url: str) -> str: + def _validate_https_url(url: str, *, allow_query: bool = True) -> str: try: parsed = urlsplit(url) except ValueError as exc: @@ -91,6 +97,7 @@ def _validate_https_url(url: str) -> str: or parsed.username is not None or parsed.password is not None or parsed.fragment + or (not allow_query and parsed.query) ): raise ValueError("GitHub OIDC transport requires an HTTPS URL") return url diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py index da16d59..3ebf6e5 100644 --- a/tests/identity_github/test_transport.py +++ b/tests/identity_github/test_transport.py @@ -1,4 +1,5 @@ import json +import traceback import urllib.error import urllib.request from collections.abc import Callable @@ -22,9 +23,10 @@ def get_jwks(self, jwks_uri: str) -> dict[str, object]: class _Response: - def __init__(self, body: bytes, reads: list[int]) -> None: + def __init__(self, body: bytes, reads: list[int], url: str) -> None: self._body = body self._reads = reads + self._url = url def __enter__(self) -> "_Response": return self @@ -36,15 +38,20 @@ def read(self, amount: int = -1) -> bytes: self._reads.append(amount) return self._body + def geturl(self) -> str: + return self._url + def _urlopen_returning( body: bytes, calls: list[tuple[str, float]], reads: list[int], + *, + final_url: str | None = None, ) -> Callable[..., _Response]: def urlopen(request: urllib.request.Request, timeout: float) -> _Response: calls.append((request.full_url, timeout)) - return _Response(body, reads) + return _Response(body, reads, final_url or request.full_url) return urlopen @@ -66,6 +73,18 @@ def fail_if_called(*args: object, **kwargs: object) -> None: transport.get_jwks("file:///tmp/keys.json") +def test_metadata_issuer_with_query_is_rejected_without_network( + monkeypatch: pytest.MonkeyPatch, +) -> None: + def fail_if_called(*args: object, **kwargs: object) -> None: + raise AssertionError("network must not be called") + + monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + + with pytest.raises(ValueError, match="HTTPS"): + GitHubOidcHttpsTransport().get_metadata("https://issuer.example?token=secret") + + def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -106,6 +125,23 @@ def test_oversized_response_is_rejected_before_json_is_accepted( GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") +def test_https_request_rejects_non_https_redirect_target( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[tuple[str, float]] = [] + reads: list[int] = [] + monkeypatch.setattr( + urllib.request, + "urlopen", + _urlopen_returning(b'{"keys":[]}', calls, reads, final_url="http://issuer.example/keys"), + ) + + with pytest.raises(RuntimeError, match="request failed"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + + assert reads == [] + + def test_malformed_or_non_object_json_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] @@ -134,17 +170,23 @@ def fail(*args: object, **kwargs: object) -> None: monkeypatch.setattr(urllib.request, "urlopen", fail) with pytest.raises(RuntimeError, match="request failed") as error: - GitHubOidcHttpsTransport().get_jwks("https://private.example/keys") + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") message = str(error.value) assert "do-not-leak" not in message assert "private.example" not in message + formatted = "".join(traceback.format_exception(error.type, error.value, error.tb)) + assert "do-not-leak" not in formatted + assert "private.example" not in formatted def test_constructor_rejects_unbounded_timeout_configuration() -> None: - with pytest.raises(ValueError, match="timeout"): - GitHubOidcHttpsTransport(timeout=0) - with pytest.raises(ValueError, match="response size"): - GitHubOidcHttpsTransport(max_response_bytes=0) + for timeout in (0, float("nan"), float("inf"), float("-inf"), True): + with pytest.raises(ValueError, match="timeout"): + GitHubOidcHttpsTransport(timeout=timeout) + + for max_response_bytes in (0, 1.5, True): + with pytest.raises(ValueError, match="response size"): + GitHubOidcHttpsTransport(max_response_bytes=max_response_bytes) # type: ignore[arg-type] assert DEFAULT_TIMEOUT_SECONDS > 0 From ce65ad316337a65f6923c367f78d07e031f0a1e5 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Mon, 10 Aug 2026 06:06:11 -0500 Subject: [PATCH 3/6] refactor(identity): isolate GitHub OIDC adapter --- pyproject.toml | 2 +- src/odoo_forge_identity_github/__init__.py | 5 +++++ .../transport.py | 2 +- tests/identity_github/test_transport.py | 15 ++++++++++++++- 4 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 src/odoo_forge_identity_github/__init__.py rename src/{odoo_forge/identity_github => odoo_forge_identity_github}/transport.py (98%) diff --git a/pyproject.toml b/pyproject.toml index a2a2f7d..6856f29 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,7 +49,7 @@ requires = ["hatchling"] build-backend = "hatchling.build" [tool.hatch.build.targets.wheel] -packages = ["src/odoo_forge", "src/odoo_forge_cli", "src/odoo_forge_git", "src/odoo_forge_workspace", "src/odoo_forge_docker", "src/odoo_forge_registry", "src/odoo_forge_postgres_docker", "src/odoo_forge_catalog", "src/odoo_forge_pipeline_github", "src/odoo_forge_instances_postgres", "src/odoo_forge_server"] +packages = ["src/odoo_forge", "src/odoo_forge_cli", "src/odoo_forge_git", "src/odoo_forge_workspace", "src/odoo_forge_docker", "src/odoo_forge_registry", "src/odoo_forge_postgres_docker", "src/odoo_forge_catalog", "src/odoo_forge_pipeline_github", "src/odoo_forge_instances_postgres", "src/odoo_forge_server", "src/odoo_forge_identity_github"] [dependency-groups] dev = [ diff --git a/src/odoo_forge_identity_github/__init__.py b/src/odoo_forge_identity_github/__init__.py new file mode 100644 index 0000000..f6277e1 --- /dev/null +++ b/src/odoo_forge_identity_github/__init__.py @@ -0,0 +1,5 @@ +"""GitHub identity adapter.""" + +from odoo_forge_identity_github.transport import GitHubOidcHttpsTransport, GitHubOidcTransport + +__all__ = ["GitHubOidcHttpsTransport", "GitHubOidcTransport"] diff --git a/src/odoo_forge/identity_github/transport.py b/src/odoo_forge_identity_github/transport.py similarity index 98% rename from src/odoo_forge/identity_github/transport.py rename to src/odoo_forge_identity_github/transport.py index 2e740ea..1de6356 100644 --- a/src/odoo_forge/identity_github/transport.py +++ b/src/odoo_forge_identity_github/transport.py @@ -97,7 +97,7 @@ def _validate_https_url(url: str, *, allow_query: bool = True) -> str: or parsed.username is not None or parsed.password is not None or parsed.fragment - or (not allow_query and parsed.query) + or (not allow_query and ("?" in url or "#" in url)) ): raise ValueError("GitHub OIDC transport requires an HTTPS URL") return url diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py index 3ebf6e5..66ad33c 100644 --- a/tests/identity_github/test_transport.py +++ b/tests/identity_github/test_transport.py @@ -6,7 +6,7 @@ import pytest -from odoo_forge.identity_github.transport import ( +from odoo_forge_identity_github.transport import ( DEFAULT_TIMEOUT_SECONDS, MAX_RESPONSE_BYTES, GitHubOidcHttpsTransport, @@ -85,6 +85,19 @@ def fail_if_called(*args: object, **kwargs: object) -> None: GitHubOidcHttpsTransport().get_metadata("https://issuer.example?token=secret") +@pytest.mark.parametrize("issuer", ["https://issuer.example?", "https://issuer.example#"]) +def test_metadata_issuer_with_empty_delimiter_is_rejected_without_network( + monkeypatch: pytest.MonkeyPatch, issuer: str +) -> None: + def fail_if_called(*args: object, **kwargs: object) -> None: + raise AssertionError("network must not be called") + + monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + + with pytest.raises(ValueError, match="HTTPS"): + GitHubOidcHttpsTransport().get_metadata(issuer) + + def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( monkeypatch: pytest.MonkeyPatch, ) -> None: From 3b857859511c32f34ab04d08f73c6b55f1e13f71 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Mon, 10 Aug 2026 06:21:07 -0500 Subject: [PATCH 4/6] fix(identity): validate OIDC redirect hops --- src/odoo_forge_identity_github/transport.py | 24 ++++- tests/identity_github/test_transport.py | 111 ++++++++++++++++---- 2 files changed, 108 insertions(+), 27 deletions(-) diff --git a/src/odoo_forge_identity_github/transport.py b/src/odoo_forge_identity_github/transport.py index 1de6356..f2aeda5 100644 --- a/src/odoo_forge_identity_github/transport.py +++ b/src/odoo_forge_identity_github/transport.py @@ -2,10 +2,11 @@ from __future__ import annotations +import http.client import json import math import urllib.request -from typing import Protocol, cast, runtime_checkable +from typing import IO, Protocol, cast, runtime_checkable from urllib.parse import urlsplit DEFAULT_TIMEOUT_SECONDS = 10.0 @@ -44,6 +45,7 @@ def __init__( raise ValueError("response size limit must be greater than zero") self._timeout = timeout self._max_response_bytes = max_response_bytes + self._opener = urllib.request.build_opener(_HttpsRedirectHandler()) def get_metadata(self, issuer: str) -> dict[str, object]: """Retrieve the standard OpenID configuration for an HTTPS issuer.""" @@ -64,7 +66,7 @@ def _read_response(self, url: str) -> bytes: headers=_JSON_HEADERS, ) try: - with urllib.request.urlopen(request, timeout=self._timeout) as response: # noqa: S310 + with self._opener.open(request, timeout=self._timeout) as response: # noqa: S310 self._validate_https_url(response.geturl()) body = response.read(self._max_response_bytes + 1) except Exception: @@ -96,11 +98,25 @@ def _validate_https_url(url: str, *, allow_query: bool = True) -> str: or not parsed.hostname or parsed.username is not None or parsed.password is not None - or parsed.fragment - or (not allow_query and ("?" in url or "#" in url)) + or "#" in url + or (not allow_query and "?" in url) ): raise ValueError("GitHub OIDC transport requires an HTTPS URL") return url +class _HttpsRedirectHandler(urllib.request.HTTPRedirectHandler): + def redirect_request( + self, + req: urllib.request.Request, + fp: IO[bytes], + code: int, + msg: str, + headers: http.client.HTTPMessage, + newurl: str, + ) -> urllib.request.Request | None: + GitHubOidcHttpsTransport._validate_https_url(newurl) + return super().redirect_request(req, fp, code, msg, headers, newurl) + + __all__ = ["GitHubOidcHttpsTransport", "GitHubOidcTransport"] diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py index 66ad33c..b16dac7 100644 --- a/tests/identity_github/test_transport.py +++ b/tests/identity_github/test_transport.py @@ -1,3 +1,5 @@ +import http.client +import io import json import traceback import urllib.error @@ -42,18 +44,30 @@ def geturl(self) -> str: return self._url +def _build_opener_failing_if_opened(*handlers: object) -> object: + class _Opener: + def open(self, *args: object, **kwargs: object) -> None: + raise AssertionError("network must not be called") + + return _Opener() + + def _urlopen_returning( body: bytes, calls: list[tuple[str, float]], reads: list[int], *, final_url: str | None = None, -) -> Callable[..., _Response]: - def urlopen(request: urllib.request.Request, timeout: float) -> _Response: - calls.append((request.full_url, timeout)) - return _Response(body, reads, final_url or request.full_url) +) -> Callable[..., object]: + class _Opener: + def open(self, request: urllib.request.Request, timeout: float) -> _Response: + calls.append((request.full_url, timeout)) + return _Response(body, reads, final_url or request.full_url) - return urlopen + def build_opener(*handlers: object) -> _Opener: + return _Opener() + + return build_opener def test_transport_protocol_is_runtime_checkable_and_satisfied_structurally() -> None: @@ -61,10 +75,7 @@ def test_transport_protocol_is_runtime_checkable_and_satisfied_structurally() -> def test_non_https_urls_are_rejected_without_network(monkeypatch: pytest.MonkeyPatch) -> None: - def fail_if_called(*args: object, **kwargs: object) -> None: - raise AssertionError("network must not be called") - - monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) transport = GitHubOidcHttpsTransport() with pytest.raises(ValueError, match="HTTPS"): @@ -76,10 +87,7 @@ def fail_if_called(*args: object, **kwargs: object) -> None: def test_metadata_issuer_with_query_is_rejected_without_network( monkeypatch: pytest.MonkeyPatch, ) -> None: - def fail_if_called(*args: object, **kwargs: object) -> None: - raise AssertionError("network must not be called") - - monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) with pytest.raises(ValueError, match="HTTPS"): GitHubOidcHttpsTransport().get_metadata("https://issuer.example?token=secret") @@ -89,10 +97,7 @@ def fail_if_called(*args: object, **kwargs: object) -> None: def test_metadata_issuer_with_empty_delimiter_is_rejected_without_network( monkeypatch: pytest.MonkeyPatch, issuer: str ) -> None: - def fail_if_called(*args: object, **kwargs: object) -> None: - raise AssertionError("network must not be called") - - monkeypatch.setattr(urllib.request, "urlopen", fail_if_called) + monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) with pytest.raises(ValueError, match="HTTPS"): GitHubOidcHttpsTransport().get_metadata(issuer) @@ -105,7 +110,7 @@ def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( reads: list[int] = [] monkeypatch.setattr( urllib.request, - "urlopen", + "build_opener", _urlopen_returning(b'{"issuer":"https://issuer.example"}', calls, reads), ) transport = GitHubOidcHttpsTransport(timeout=2.5) @@ -130,7 +135,7 @@ def test_oversized_response_is_rejected_before_json_is_accepted( body = b"{" + b"x" * MAX_RESPONSE_BYTES + b"}" monkeypatch.setattr( urllib.request, - "urlopen", + "build_opener", _urlopen_returning(body, calls, reads), ) @@ -145,7 +150,7 @@ def test_https_request_rejects_non_https_redirect_target( reads: list[int] = [] monkeypatch.setattr( urllib.request, - "urlopen", + "build_opener", _urlopen_returning(b'{"keys":[]}', calls, reads, final_url="http://issuer.example/keys"), ) @@ -155,12 +160,68 @@ def test_https_request_rejects_non_https_redirect_target( assert reads == [] +def test_each_redirect_hop_is_validated_before_request( + monkeypatch: pytest.MonkeyPatch, +) -> None: + requested: list[str] = [] + + class _RedirectingOpener: + def __init__(self, handler: urllib.request.HTTPRedirectHandler) -> None: + self._handler = handler + + def open(self, request: urllib.request.Request, timeout: float) -> _Response: + requested.append(request.full_url) + response = io.BytesIO() + headers = http.client.HTTPMessage() + next_request = self._handler.redirect_request( + request, + response, + 302, + "Found", + headers, + "https://issuer.example/second", + ) + assert next_request is not None + requested.append(next_request.full_url) + self._handler.redirect_request( + next_request, + response, + 302, + "Found", + headers, + "http://issuer.example/keys", + ) + raise AssertionError("insecure redirect must be rejected") + + def build_opener(handler: urllib.request.HTTPRedirectHandler) -> _RedirectingOpener: + return _RedirectingOpener(handler) + + monkeypatch.setattr(urllib.request, "build_opener", build_opener) + + with pytest.raises(RuntimeError, match="request failed"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/first") + + assert requested == [ + "https://issuer.example/first", + "https://issuer.example/second", + ] + + +def test_jwks_url_with_empty_fragment_is_rejected_without_network( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) + + with pytest.raises(ValueError, match="HTTPS"): + GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys#") + + def test_malformed_or_non_object_json_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] monkeypatch.setattr( urllib.request, - "urlopen", + "build_opener", _urlopen_returning(b"not-json", calls, reads), ) @@ -169,7 +230,7 @@ def test_malformed_or_non_object_json_is_rejected(monkeypatch: pytest.MonkeyPatc monkeypatch.setattr( urllib.request, - "urlopen", + "build_opener", _urlopen_returning(json.dumps(["not", "an", "object"]).encode(), calls, reads), ) with pytest.raises(RuntimeError, match="malformed JSON"): @@ -180,7 +241,11 @@ def test_network_failures_are_sanitized(monkeypatch: pytest.MonkeyPatch) -> None def fail(*args: object, **kwargs: object) -> None: raise urllib.error.URLError("token=do-not-leak at https://private.example") - monkeypatch.setattr(urllib.request, "urlopen", fail) + class _FailingOpener: + def open(self, *args: object, **kwargs: object) -> None: + fail(*args, **kwargs) + + monkeypatch.setattr(urllib.request, "build_opener", lambda *handlers: _FailingOpener()) with pytest.raises(RuntimeError, match="request failed") as error: GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") From 7f2f7043d70822592a129f357d78dc89f3c058ee Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Mon, 10 Aug 2026 06:28:08 -0500 Subject: [PATCH 5/6] refactor(identity): inject bounded HTTP opener --- src/odoo_forge_identity_github/__init__.py | 16 +- src/odoo_forge_identity_github/transport.py | 47 +++++- tests/identity_github/test_transport.py | 167 ++++++++------------ 3 files changed, 126 insertions(+), 104 deletions(-) diff --git a/src/odoo_forge_identity_github/__init__.py b/src/odoo_forge_identity_github/__init__.py index f6277e1..4a88dad 100644 --- a/src/odoo_forge_identity_github/__init__.py +++ b/src/odoo_forge_identity_github/__init__.py @@ -1,5 +1,17 @@ """GitHub identity adapter.""" -from odoo_forge_identity_github.transport import GitHubOidcHttpsTransport, GitHubOidcTransport +from odoo_forge_identity_github.transport import ( + BoundedHttpOpener, + BoundedHttpResponse, + GitHubOidcHttpsTransport, + GitHubOidcTransport, + create_github_oidc_https_transport, +) -__all__ = ["GitHubOidcHttpsTransport", "GitHubOidcTransport"] +__all__ = [ + "BoundedHttpOpener", + "BoundedHttpResponse", + "GitHubOidcHttpsTransport", + "GitHubOidcTransport", + "create_github_oidc_https_transport", +] diff --git a/src/odoo_forge_identity_github/transport.py b/src/odoo_forge_identity_github/transport.py index f2aeda5..802ba6f 100644 --- a/src/odoo_forge_identity_github/transport.py +++ b/src/odoo_forge_identity_github/transport.py @@ -6,7 +6,8 @@ import json import math import urllib.request -from typing import IO, Protocol, cast, runtime_checkable +from types import TracebackType +from typing import IO, Protocol, Self, cast, runtime_checkable from urllib.parse import urlsplit DEFAULT_TIMEOUT_SECONDS = 10.0 @@ -26,12 +27,32 @@ def get_jwks(self, jwks_uri: str) -> dict[str, object]: ... +class BoundedHttpResponse(Protocol): + def __enter__(self) -> Self: ... + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType | None, + ) -> None: ... + + def geturl(self) -> str: ... + + def read(self, amount: int = -1) -> bytes: ... + + +class BoundedHttpOpener(Protocol): + def open(self, request: urllib.request.Request, *, timeout: float) -> BoundedHttpResponse: ... + + class GitHubOidcHttpsTransport: """Retrieve GitHub OIDC JSON documents using bounded HTTPS requests.""" def __init__( self, *, + opener: BoundedHttpOpener, timeout: float = DEFAULT_TIMEOUT_SECONDS, max_response_bytes: int = MAX_RESPONSE_BYTES, ) -> None: @@ -45,7 +66,7 @@ def __init__( raise ValueError("response size limit must be greater than zero") self._timeout = timeout self._max_response_bytes = max_response_bytes - self._opener = urllib.request.build_opener(_HttpsRedirectHandler()) + self._opener = opener def get_metadata(self, issuer: str) -> dict[str, object]: """Retrieve the standard OpenID configuration for an HTTPS issuer.""" @@ -119,4 +140,24 @@ def redirect_request( return super().redirect_request(req, fp, code, msg, headers, newurl) -__all__ = ["GitHubOidcHttpsTransport", "GitHubOidcTransport"] +def create_github_oidc_https_transport( + *, + timeout: float = DEFAULT_TIMEOUT_SECONDS, + max_response_bytes: int = MAX_RESPONSE_BYTES, +) -> GitHubOidcHttpsTransport: + """Compose the production GitHub OIDC transport with urllib.""" + opener = cast(BoundedHttpOpener, urllib.request.build_opener(_HttpsRedirectHandler())) + return GitHubOidcHttpsTransport( + opener=opener, + timeout=timeout, + max_response_bytes=max_response_bytes, + ) + + +__all__ = [ + "BoundedHttpOpener", + "BoundedHttpResponse", + "GitHubOidcHttpsTransport", + "GitHubOidcTransport", + "create_github_oidc_https_transport", +] diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py index b16dac7..2ae7942 100644 --- a/tests/identity_github/test_transport.py +++ b/tests/identity_github/test_transport.py @@ -4,15 +4,17 @@ import traceback import urllib.error import urllib.request -from collections.abc import Callable import pytest from odoo_forge_identity_github.transport import ( DEFAULT_TIMEOUT_SECONDS, MAX_RESPONSE_BYTES, + BoundedHttpResponse, GitHubOidcHttpsTransport, GitHubOidcTransport, + _HttpsRedirectHandler, + create_github_oidc_https_transport, ) @@ -44,39 +46,36 @@ def geturl(self) -> str: return self._url -def _build_opener_failing_if_opened(*handlers: object) -> object: - class _Opener: - def open(self, *args: object, **kwargs: object) -> None: - raise AssertionError("network must not be called") +class _FailIfOpened: + def open(self, request: urllib.request.Request, *, timeout: float) -> BoundedHttpResponse: + raise AssertionError("network must not be called") - return _Opener() +class _ReturningOpener: + def __init__( + self, + body: bytes, + calls: list[tuple[str, float]], + reads: list[int], + *, + final_url: str | None = None, + ) -> None: + self._body = body + self._calls = calls + self._reads = reads + self._final_url = final_url -def _urlopen_returning( - body: bytes, - calls: list[tuple[str, float]], - reads: list[int], - *, - final_url: str | None = None, -) -> Callable[..., object]: - class _Opener: - def open(self, request: urllib.request.Request, timeout: float) -> _Response: - calls.append((request.full_url, timeout)) - return _Response(body, reads, final_url or request.full_url) - - def build_opener(*handlers: object) -> _Opener: - return _Opener() - - return build_opener + def open(self, request: urllib.request.Request, *, timeout: float) -> _Response: + self._calls.append((request.full_url, timeout)) + return _Response(self._body, self._reads, self._final_url or request.full_url) def test_transport_protocol_is_runtime_checkable_and_satisfied_structurally() -> None: assert isinstance(_FakeTransport(), GitHubOidcTransport) -def test_non_https_urls_are_rejected_without_network(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) - transport = GitHubOidcHttpsTransport() +def test_non_https_urls_are_rejected_without_network() -> None: + transport = GitHubOidcHttpsTransport(opener=_FailIfOpened()) with pytest.raises(ValueError, match="HTTPS"): transport.get_metadata("http://issuer.example") @@ -84,36 +83,28 @@ def test_non_https_urls_are_rejected_without_network(monkeypatch: pytest.MonkeyP transport.get_jwks("file:///tmp/keys.json") -def test_metadata_issuer_with_query_is_rejected_without_network( - monkeypatch: pytest.MonkeyPatch, -) -> None: - monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) - +def test_metadata_issuer_with_query_is_rejected_without_network() -> None: with pytest.raises(ValueError, match="HTTPS"): - GitHubOidcHttpsTransport().get_metadata("https://issuer.example?token=secret") + GitHubOidcHttpsTransport(opener=_FailIfOpened()).get_metadata( + "https://issuer.example?token=secret" + ) @pytest.mark.parametrize("issuer", ["https://issuer.example?", "https://issuer.example#"]) def test_metadata_issuer_with_empty_delimiter_is_rejected_without_network( - monkeypatch: pytest.MonkeyPatch, issuer: str + issuer: str, ) -> None: - monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) - with pytest.raises(ValueError, match="HTTPS"): - GitHubOidcHttpsTransport().get_metadata(issuer) + GitHubOidcHttpsTransport(opener=_FailIfOpened()).get_metadata(issuer) -def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( - monkeypatch: pytest.MonkeyPatch, -) -> None: +def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound() -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] - monkeypatch.setattr( - urllib.request, - "build_opener", - _urlopen_returning(b'{"issuer":"https://issuer.example"}', calls, reads), + transport = GitHubOidcHttpsTransport( + opener=_ReturningOpener(b'{"issuer":"https://issuer.example"}', calls, reads), + timeout=2.5, ) - transport = GitHubOidcHttpsTransport(timeout=2.5) result = transport.get_metadata("https://issuer.example") @@ -127,42 +118,28 @@ def test_https_requests_use_timeout_and_read_one_byte_beyond_response_bound( assert reads == [MAX_RESPONSE_BYTES + 1] -def test_oversized_response_is_rejected_before_json_is_accepted( - monkeypatch: pytest.MonkeyPatch, -) -> None: +def test_oversized_response_is_rejected_before_json_is_accepted() -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] body = b"{" + b"x" * MAX_RESPONSE_BYTES + b"}" - monkeypatch.setattr( - urllib.request, - "build_opener", - _urlopen_returning(body, calls, reads), - ) - with pytest.raises(RuntimeError, match="response exceeds size limit"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + GitHubOidcHttpsTransport(opener=_ReturningOpener(body, calls, reads)).get_jwks( + "https://issuer.example/keys" + ) -def test_https_request_rejects_non_https_redirect_target( - monkeypatch: pytest.MonkeyPatch, -) -> None: +def test_https_request_rejects_non_https_redirect_target() -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] - monkeypatch.setattr( - urllib.request, - "build_opener", - _urlopen_returning(b'{"keys":[]}', calls, reads, final_url="http://issuer.example/keys"), - ) + opener = _ReturningOpener(b'{"keys":[]}', calls, reads, final_url="http://issuer.example/keys") with pytest.raises(RuntimeError, match="request failed"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + GitHubOidcHttpsTransport(opener=opener).get_jwks("https://issuer.example/keys") assert reads == [] -def test_each_redirect_hop_is_validated_before_request( - monkeypatch: pytest.MonkeyPatch, -) -> None: +def test_each_redirect_hop_is_validated_before_request() -> None: requested: list[str] = [] class _RedirectingOpener: @@ -193,13 +170,10 @@ def open(self, request: urllib.request.Request, timeout: float) -> _Response: ) raise AssertionError("insecure redirect must be rejected") - def build_opener(handler: urllib.request.HTTPRedirectHandler) -> _RedirectingOpener: - return _RedirectingOpener(handler) - - monkeypatch.setattr(urllib.request, "build_opener", build_opener) - with pytest.raises(RuntimeError, match="request failed"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/first") + GitHubOidcHttpsTransport(opener=_RedirectingOpener(_HttpsRedirectHandler())).get_jwks( + "https://issuer.example/first" + ) assert requested == [ "https://issuer.example/first", @@ -207,48 +181,36 @@ def build_opener(handler: urllib.request.HTTPRedirectHandler) -> _RedirectingOpe ] -def test_jwks_url_with_empty_fragment_is_rejected_without_network( - monkeypatch: pytest.MonkeyPatch, -) -> None: - monkeypatch.setattr(urllib.request, "build_opener", _build_opener_failing_if_opened) - +def test_jwks_url_with_empty_fragment_is_rejected_without_network() -> None: with pytest.raises(ValueError, match="HTTPS"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys#") + GitHubOidcHttpsTransport(opener=_FailIfOpened()).get_jwks("https://issuer.example/keys#") -def test_malformed_or_non_object_json_is_rejected(monkeypatch: pytest.MonkeyPatch) -> None: +def test_malformed_or_non_object_json_is_rejected() -> None: calls: list[tuple[str, float]] = [] reads: list[int] = [] - monkeypatch.setattr( - urllib.request, - "build_opener", - _urlopen_returning(b"not-json", calls, reads), - ) - with pytest.raises(RuntimeError, match="malformed JSON"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + GitHubOidcHttpsTransport(opener=_ReturningOpener(b"not-json", calls, reads)).get_jwks( + "https://issuer.example/keys" + ) - monkeypatch.setattr( - urllib.request, - "build_opener", - _urlopen_returning(json.dumps(["not", "an", "object"]).encode(), calls, reads), - ) with pytest.raises(RuntimeError, match="malformed JSON"): - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + GitHubOidcHttpsTransport( + opener=_ReturningOpener(json.dumps(["not", "an", "object"]).encode(), calls, reads) + ).get_jwks("https://issuer.example/keys") -def test_network_failures_are_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: +def test_network_failures_are_sanitized() -> None: def fail(*args: object, **kwargs: object) -> None: raise urllib.error.URLError("token=do-not-leak at https://private.example") class _FailingOpener: - def open(self, *args: object, **kwargs: object) -> None: - fail(*args, **kwargs) - - monkeypatch.setattr(urllib.request, "build_opener", lambda *handlers: _FailingOpener()) + def open(self, request: urllib.request.Request, *, timeout: float) -> BoundedHttpResponse: + fail(request, timeout=timeout) + raise AssertionError("unreachable") with pytest.raises(RuntimeError, match="request failed") as error: - GitHubOidcHttpsTransport().get_jwks("https://issuer.example/keys") + GitHubOidcHttpsTransport(opener=_FailingOpener()).get_jwks("https://issuer.example/keys") message = str(error.value) assert "do-not-leak" not in message @@ -261,10 +223,17 @@ def open(self, *args: object, **kwargs: object) -> None: def test_constructor_rejects_unbounded_timeout_configuration() -> None: for timeout in (0, float("nan"), float("inf"), float("-inf"), True): with pytest.raises(ValueError, match="timeout"): - GitHubOidcHttpsTransport(timeout=timeout) + GitHubOidcHttpsTransport(opener=_FailIfOpened(), timeout=timeout) for max_response_bytes in (0, 1.5, True): with pytest.raises(ValueError, match="response size"): - GitHubOidcHttpsTransport(max_response_bytes=max_response_bytes) # type: ignore[arg-type] + GitHubOidcHttpsTransport( + opener=_FailIfOpened(), + max_response_bytes=max_response_bytes, # type: ignore[arg-type] + ) assert DEFAULT_TIMEOUT_SECONDS > 0 + + +def test_composition_factory_builds_a_usable_urllib_transport() -> None: + assert isinstance(create_github_oidc_https_transport(), GitHubOidcHttpsTransport) From 3bfb1d1daafd4aacc6b5a5d6ffed644b1ed3cde1 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Mon, 10 Aug 2026 06:33:37 -0500 Subject: [PATCH 6/6] test(identity): cover redirects through public API --- tests/identity_github/test_transport.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/identity_github/test_transport.py b/tests/identity_github/test_transport.py index 2ae7942..ad9583f 100644 --- a/tests/identity_github/test_transport.py +++ b/tests/identity_github/test_transport.py @@ -13,7 +13,6 @@ BoundedHttpResponse, GitHubOidcHttpsTransport, GitHubOidcTransport, - _HttpsRedirectHandler, create_github_oidc_https_transport, ) @@ -139,7 +138,9 @@ def test_https_request_rejects_non_https_redirect_target() -> None: assert reads == [] -def test_each_redirect_hop_is_validated_before_request() -> None: +def test_each_redirect_hop_is_validated_before_request( + monkeypatch: pytest.MonkeyPatch, +) -> None: requested: list[str] = [] class _RedirectingOpener: @@ -170,10 +171,16 @@ def open(self, request: urllib.request.Request, timeout: float) -> _Response: ) raise AssertionError("insecure redirect must be rejected") + def build_opener( + handler: urllib.request.BaseHandler, + ) -> _RedirectingOpener: + assert isinstance(handler, urllib.request.HTTPRedirectHandler) + return _RedirectingOpener(handler) + + monkeypatch.setattr(urllib.request, "build_opener", build_opener) + with pytest.raises(RuntimeError, match="request failed"): - GitHubOidcHttpsTransport(opener=_RedirectingOpener(_HttpsRedirectHandler())).get_jwks( - "https://issuer.example/first" - ) + create_github_oidc_https_transport().get_jwks("https://issuer.example/first") assert requested == [ "https://issuer.example/first",