From 77070f70cb567d3e7d10b4ddb23b028c1d33182e Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Tue, 18 Aug 2026 21:12:04 -0700 Subject: [PATCH 1/7] fix: refresh GitHub App token on stale request retries --- src/openhound_github/auth.py | 34 +++++++++--- src/openhound_github/helpers.py | 18 ++++++- tests/test_github_app_retry.py | 94 +++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 7 deletions(-) create mode 100644 tests/test_github_app_retry.py diff --git a/src/openhound_github/auth.py b/src/openhound_github/auth.py index c899def..8ef10b1 100644 --- a/src/openhound_github/auth.py +++ b/src/openhound_github/auth.py @@ -172,6 +172,14 @@ def _should_refresh(self) -> bool: refresh_at = self.expires_at - timedelta(seconds=self.refresh_margin_seconds) return datetime.now(timezone.utc) >= refresh_at + def _refresh_token(self) -> None: + logger.info( + f"Refreshing access token for {self.installation.installation_id}" + ) + get_token = self.installation.token + self.access_token = get_token.token + self.expires_at = get_token.expires_at + def token(self, force_refresh: bool = False) -> str | None: if ( not force_refresh @@ -182,15 +190,29 @@ def token(self, force_refresh: bool = False) -> str | None: with self._token_lock: if (force_refresh or self._should_refresh()) or self.access_token is None: - logger.info( - f"Refreshing access token for {self.installation.installation_id}" - ) - get_token = self.installation.token - self.access_token = get_token.token - self.expires_at = get_token.expires_at + self._refresh_token() return self.access_token + def refresh_request(self, request: requests.PreparedRequest) -> requests.PreparedRequest: + """Refresh a rejected prepared request without stampeding token issuance.""" + request_authorization = request.headers.get("Authorization") + + with self._token_lock: + current_authorization = ( + f"Bearer {self.access_token}" if self.access_token is not None else None + ) + if ( + self.access_token is None + or self._should_refresh() + or request_authorization == current_authorization + ): + self._refresh_token() + + request.headers["Authorization"] = f"Bearer {self.access_token}" + + return request + def __call__(self, request: requests.PreparedRequest) -> requests.PreparedRequest: request.headers["Authorization"] = f"Bearer {self.token()}" return request diff --git a/src/openhound_github/helpers.py b/src/openhound_github/helpers.py index 36bd036..593d957 100644 --- a/src/openhound_github/helpers.py +++ b/src/openhound_github/helpers.py @@ -10,6 +10,8 @@ ) from requests import Request +from openhound_github.auth import GitHubAppInstallationAuth + logger = logging.getLogger(__name__) @@ -160,6 +162,21 @@ def retry_policy( headers = response.headers now = int(time.time()) + message = _response_message(response).lower() + + # DLT retries the same prepared request after long Retry-After sleeps. + if ( + response.status_code == 401 + and "bad credentials" in message + and isinstance(auth, GitHubAppInstallationAuth) + and response.request is not None + ): + auth.refresh_request(response.request) + logger.warning( + "GitHub App installation token rejected, retrying request with refreshed token" + ) + return True + if ( response.status_code == 200 and headers.get("x-ratelimit-resource") == "graphql" @@ -178,7 +195,6 @@ def retry_policy( return True return False - message = _response_message(response).lower() if response.status_code not in (403, 429): return False diff --git a/tests/test_github_app_retry.py b/tests/test_github_app_retry.py new file mode 100644 index 0000000..1fe05b2 --- /dev/null +++ b/tests/test_github_app_retry.py @@ -0,0 +1,94 @@ +from datetime import datetime, timedelta, timezone + +import requests +from dlt.sources.helpers.rest_client.auth import BearerTokenAuth + +from openhound_github.auth import GitHubAppInstallationAuth, TokenResponse +from openhound_github.helpers import github_retry_policy + + +class FakeInstallation: + installation_id = "12345" + + def __init__(self, *tokens: str) -> None: + self._tokens = iter(tokens) + self.token_calls = 0 + + @property + def token(self) -> TokenResponse: + self.token_calls += 1 + return TokenResponse( + token=next(self._tokens), + expires_at=datetime.now(timezone.utc) + timedelta(hours=1), + ) + + +def prepared_request(token: str) -> requests.PreparedRequest: + return requests.Request( + "GET", + "https://api.github.com/repos/example/repo", + headers={"Authorization": f"Bearer {token}"}, + ).prepare() + + +def bad_credentials_response( + request: requests.PreparedRequest, +) -> requests.Response: + response = requests.Response() + response.status_code = 401 + response._content = b'{"message":"Bad credentials"}' + response.request = request + return response + + +def test_refresh_request_refreshes_rejected_current_token() -> None: + installation = FakeInstallation("new-token") + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request("old-token") + + auth.refresh_request(request) + + assert request.headers["Authorization"] == "Bearer new-token" + assert auth.access_token == "new-token" + assert installation.token_calls == 1 + + +def test_refresh_request_reuses_token_refreshed_by_another_request() -> None: + installation = FakeInstallation() + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "new-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request("old-token") + + auth.refresh_request(request) + + assert request.headers["Authorization"] == "Bearer new-token" + assert installation.token_calls == 0 + + +def test_retry_policy_repairs_bad_credentials_for_github_app_auth() -> None: + installation = FakeInstallation("new-token") + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request("old-token") + + should_retry = github_retry_policy(auth)(bad_credentials_response(request), None) + + assert should_retry is True + assert request.headers["Authorization"] == "Bearer new-token" + assert installation.token_calls == 1 + + +def test_retry_policy_does_not_repair_bad_credentials_for_bearer_token_auth() -> None: + request = prepared_request("static-token") + + should_retry = github_retry_policy(BearerTokenAuth(token="static-token"))( + bad_credentials_response(request), + None, + ) + + assert should_retry is False + assert request.headers["Authorization"] == "Bearer static-token" From b9d2adcaecac1b0d178c2fa6b418d401c2f99eeb Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 09:43:51 -0700 Subject: [PATCH 2/7] BED-9370: cover concurrent stale app token refreshes Exercise two stale GitHub App requests against the same auth instance while the first token refresh holds the lock. Assert both requests reuse the replacement token and only one installation token is issued so the refresh_request lock behavior is covered directly. --- tests/test_github_app_retry.py | 56 ++++++++++++++++++++++++++++++---- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/tests/test_github_app_retry.py b/tests/test_github_app_retry.py index 1fe05b2..d833434 100644 --- a/tests/test_github_app_retry.py +++ b/tests/test_github_app_retry.py @@ -1,4 +1,6 @@ +from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timedelta, timezone +from threading import Event, Lock import requests from dlt.sources.helpers.rest_client.auth import BearerTokenAuth @@ -23,6 +25,35 @@ def token(self) -> TokenResponse: ) +class BlockingFakeInstallation(FakeInstallation): + def __init__(self, *tokens: str) -> None: + super().__init__(*tokens) + self.token_started = Event() + self.release_token = Event() + + @property + def token(self) -> TokenResponse: + self.token_started.set() + if not self.release_token.wait(timeout=1): + raise TimeoutError("timed out waiting to release fake token refresh") + return super().token + + +class TrackingLock: + def __init__(self) -> None: + self._lock = Lock() + self.waiting = Event() + + def __enter__(self): + if not self._lock.acquire(blocking=False): + self.waiting.set() + self._lock.acquire() + return self + + def __exit__(self, _exc_type, _exc_value, _traceback) -> None: + self._lock.release() + + def prepared_request(token: str) -> requests.PreparedRequest: return requests.Request( "GET", @@ -56,16 +87,29 @@ def test_refresh_request_refreshes_rejected_current_token() -> None: def test_refresh_request_reuses_token_refreshed_by_another_request() -> None: - installation = FakeInstallation() + installation = BlockingFakeInstallation("new-token") auth = GitHubAppInstallationAuth(installation=installation) - auth.access_token = "new-token" + auth.access_token = "old-token" auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) - request = prepared_request("old-token") + auth._token_lock = TrackingLock() + requests_to_refresh = [prepared_request("old-token"), prepared_request("old-token")] - auth.refresh_request(request) + with ThreadPoolExecutor(max_workers=2) as executor: + first_refresh = executor.submit(auth.refresh_request, requests_to_refresh[0]) + assert installation.token_started.wait(timeout=1) - assert request.headers["Authorization"] == "Bearer new-token" - assert installation.token_calls == 0 + second_refresh = executor.submit(auth.refresh_request, requests_to_refresh[1]) + assert auth._token_lock.waiting.wait(timeout=1) + + installation.release_token.set() + first_refresh.result(timeout=1) + second_refresh.result(timeout=1) + + assert all( + request.headers["Authorization"] == "Bearer new-token" + for request in requests_to_refresh + ) + assert installation.token_calls == 1 def test_retry_policy_repairs_bad_credentials_for_github_app_auth() -> None: From 94184b1bea2eeb0645ef263547e6af2a96380236 Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 13:46:04 -0700 Subject: [PATCH 3/7] BED-9370: harden app token retry handling Restrict installation token refresh retries to same-origin API requests that already carried a bearer token, preventing redirected requests from regaining authorization after Requests strips it. Stop retry-driven token minting once a replacement token has already been rejected, while preserving lock-based reuse for concurrent stale requests. Pass API origin context through app auth construction and add regression coverage for cross-origin redirects and repeated bad-credentials responses. --- src/openhound_github/auth.py | 60 +++++++++++++++++++++++++++++---- src/openhound_github/helpers.py | 3 +- src/openhound_github/source.py | 21 ++++++++++-- tests/test_app_auth.py | 5 ++- tests/test_github_app_retry.py | 41 ++++++++++++++++++++-- 5 files changed, 115 insertions(+), 15 deletions(-) diff --git a/src/openhound_github/auth.py b/src/openhound_github/auth.py index 8ef10b1..2cd2e34 100644 --- a/src/openhound_github/auth.py +++ b/src/openhound_github/auth.py @@ -2,6 +2,7 @@ from datetime import datetime, timedelta, timezone from threading import Lock from typing import Iterator +from urllib.parse import urlparse import requests from dlt.common.configuration import configspec @@ -17,6 +18,19 @@ logger = logging.getLogger(__name__) +def _normalized_http_origin(url: str) -> tuple[str, str, int | None]: + parsed = urlparse(url) + scheme = parsed.scheme.lower() + if scheme not in {"http", "https"} or not parsed.hostname: + raise ValueError("GitHub API URI must be an absolute HTTP(S) URL") + + port = parsed.port + if (scheme == "http" and port == 80) or (scheme == "https" and port == 443): + port = None + + return scheme, parsed.hostname.lower(), port + + class AccountConfig(BaseModel): id: int login: str | None = None @@ -63,7 +77,8 @@ def __init__( private_key_path: str, api_uri: str = "https://api.github.com/", ): - self.api_uri = api_uri + _normalized_http_origin(api_uri) + self.api_uri = f"{api_uri.rstrip('/')}/" self.jwt_issuer = jwt_issuer self.private_key_path = private_key_path self.client = RESTClient( @@ -158,11 +173,17 @@ def __init__( self, installation: GithubInstallation, refresh_margin_seconds: int = 300, + api_uri: str | None = None, ): self.installation = installation self.refresh_margin_seconds = refresh_margin_seconds + self.api_uri = api_uri or getattr( + installation, "api_uri", "https://api.github.com/" + ) + self._api_origin = _normalized_http_origin(self.api_uri) self.access_token: str | None = None self.expires_at: datetime | None = None + self._response_refreshed_token: str | None = None self._token_lock = Lock() def _should_refresh(self) -> bool: @@ -172,13 +193,16 @@ def _should_refresh(self) -> bool: refresh_at = self.expires_at - timedelta(seconds=self.refresh_margin_seconds) return datetime.now(timezone.utc) >= refresh_at - def _refresh_token(self) -> None: + def _refresh_token(self, *, response_triggered: bool = False) -> None: logger.info( f"Refreshing access token for {self.installation.installation_id}" ) get_token = self.installation.token self.access_token = get_token.token self.expires_at = get_token.expires_at + self._response_refreshed_token = ( + get_token.token if response_triggered else None + ) def token(self, force_refresh: bool = False) -> str | None: if ( @@ -194,24 +218,46 @@ def token(self, force_refresh: bool = False) -> str | None: return self.access_token - def refresh_request(self, request: requests.PreparedRequest) -> requests.PreparedRequest: - """Refresh a rejected prepared request without stampeding token issuance.""" + def refresh_request(self, request: requests.PreparedRequest) -> bool: + """Repair a rejected same-origin request without stampeding token issuance.""" + try: + request_origin = _normalized_http_origin(request.url or "") + except ValueError: + return False + + if request_origin != self._api_origin: + return False + request_authorization = request.headers.get("Authorization") + if ( + not request_authorization + or not request_authorization.startswith("Bearer ") + or not request_authorization.removeprefix("Bearer ").strip() + ): + return False with self._token_lock: current_authorization = ( f"Bearer {self.access_token}" if self.access_token is not None else None ) + should_refresh = self._should_refresh() + if ( + not should_refresh + and request_authorization == current_authorization + and self.access_token == self._response_refreshed_token + ): + return False + if ( self.access_token is None - or self._should_refresh() + or should_refresh or request_authorization == current_authorization ): - self._refresh_token() + self._refresh_token(response_triggered=True) request.headers["Authorization"] = f"Bearer {self.access_token}" - return request + return True def __call__(self, request: requests.PreparedRequest) -> requests.PreparedRequest: request.headers["Authorization"] = f"Bearer {self.token()}" diff --git a/src/openhound_github/helpers.py b/src/openhound_github/helpers.py index 593d957..55c5295 100644 --- a/src/openhound_github/helpers.py +++ b/src/openhound_github/helpers.py @@ -171,7 +171,8 @@ def retry_policy( and isinstance(auth, GitHubAppInstallationAuth) and response.request is not None ): - auth.refresh_request(response.request) + if not auth.refresh_request(response.request): + return False logger.warning( "GitHub App installation token rejected, retrying request with refreshed token" ) diff --git a/src/openhound_github/source.py b/src/openhound_github/source.py index bd08782..4ec8466 100644 --- a/src/openhound_github/source.py +++ b/src/openhound_github/source.py @@ -172,6 +172,7 @@ def token_client(token: str) -> RESTClient: github_app_session = GithubApp( jwt_issuer=jwt_issuer, private_key_path=credentials.key_path, + api_uri=host, ) for installation in github_app_session.installations: if installation.target_type == "Organization": @@ -179,12 +180,16 @@ def token_client(token: str) -> RESTClient: installation_id=installation.id, jwt_issuer=jwt_issuer, private_key_path=credentials.key_path, + api_uri=host, ) ctx.organizations.append( OrgContext( org_name=installation.account.login, client=client( - GitHubAppInstallationAuth(installation=org_installation) + GitHubAppInstallationAuth( + installation=org_installation, + api_uri=host, + ) ), enterprise_name=credentials.enterprise_name, github_deployment_id=github_deployment_id, @@ -196,9 +201,13 @@ def token_client(token: str) -> RESTClient: installation_id=installation.id, jwt_issuer=jwt_issuer, private_key_path=credentials.key_path, + api_uri=host, ) ctx.client = client( - GitHubAppInstallationAuth(installation=es_installation) + GitHubAppInstallationAuth( + installation=es_installation, + api_uri=host, + ) ) return (*enterprise_resources(ctx), *organization_resources(ctx)) @@ -213,11 +222,17 @@ def token_client(token: str) -> RESTClient: installation_id=credentials.install_id, jwt_issuer=credentials.client_id, private_key_path=credentials.key_path, + api_uri=host, ) ctx.organizations.append( OrgContext( org_name=credentials.org_name, - client=client(GitHubAppInstallationAuth(installation=org_installation)), + client=client( + GitHubAppInstallationAuth( + installation=org_installation, + api_uri=host, + ) + ), github_deployment_id=github_deployment_id, github_web_origin=github_web_origin, ) diff --git a/tests/test_app_auth.py b/tests/test_app_auth.py index ddfb35c..bec15ce 100644 --- a/tests/test_app_auth.py +++ b/tests/test_app_auth.py @@ -104,7 +104,9 @@ def test_enterprise_source_reuses_selected_issuer_for_installation_tokens( captured_issuers: list[str] = [] class FakeGithubApp: - def __init__(self, jwt_issuer: str, private_key_path: str) -> None: + def __init__( + self, jwt_issuer: str, private_key_path: str, api_uri: str + ) -> None: captured_issuers.append(jwt_issuer) self.installations = ( SimpleNamespace( @@ -125,6 +127,7 @@ def __init__( installation_id: int, jwt_issuer: str, private_key_path: str, + api_uri: str, ) -> None: captured_issuers.append(jwt_issuer) diff --git a/tests/test_github_app_retry.py b/tests/test_github_app_retry.py index d833434..00e0598 100644 --- a/tests/test_github_app_retry.py +++ b/tests/test_github_app_retry.py @@ -11,6 +11,7 @@ class FakeInstallation: installation_id = "12345" + api_uri = "https://api.github.com/" def __init__(self, *tokens: str) -> None: self._tokens = iter(tokens) @@ -54,11 +55,15 @@ def __exit__(self, _exc_type, _exc_value, _traceback) -> None: self._lock.release() -def prepared_request(token: str) -> requests.PreparedRequest: +def prepared_request( + token: str | None, + url: str = "https://api.github.com/repos/example/repo", +) -> requests.PreparedRequest: + headers = {"Authorization": f"Bearer {token}"} if token is not None else {} return requests.Request( "GET", - "https://api.github.com/repos/example/repo", - headers={"Authorization": f"Bearer {token}"}, + url, + headers=headers, ).prepare() @@ -112,6 +117,20 @@ def test_refresh_request_reuses_token_refreshed_by_another_request() -> None: assert installation.token_calls == 1 +def test_refresh_request_does_not_restore_authorization_on_cross_origin_request() -> None: + installation = FakeInstallation("new-token") + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request(None, url="https://attacker.example/redirected") + + repaired = auth.refresh_request(request) + + assert repaired is False + assert "Authorization" not in request.headers + assert installation.token_calls == 0 + + def test_retry_policy_repairs_bad_credentials_for_github_app_auth() -> None: installation = FakeInstallation("new-token") auth = GitHubAppInstallationAuth(installation=installation) @@ -126,6 +145,22 @@ def test_retry_policy_repairs_bad_credentials_for_github_app_auth() -> None: assert installation.token_calls == 1 +def test_retry_policy_does_not_retry_replacement_token_bad_credentials() -> None: + installation = FakeInstallation("new-token", "unused-token") + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request("old-token") + retry_policy = github_retry_policy(auth) + + assert retry_policy(bad_credentials_response(request), None) is True + assert request.headers["Authorization"] == "Bearer new-token" + + assert retry_policy(bad_credentials_response(request), None) is False + assert request.headers["Authorization"] == "Bearer new-token" + assert installation.token_calls == 1 + + def test_retry_policy_does_not_repair_bad_credentials_for_bearer_token_auth() -> None: request = prepared_request("static-token") From 41947f3c6099a434b1e91d11d248405d1527f0b4 Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 13:54:21 -0700 Subject: [PATCH 4/7] BED-9370: handle app token refresh failures during retry Catch installation token refresh failures inside refresh_request so the retry policy can return false instead of propagating an exception or mutating the request header. Log the failed refresh without including exception contents and add regression coverage proving the original Authorization header remains unchanged when refresh fails. --- src/openhound_github/auth.py | 10 +++++++++- tests/test_github_app_retry.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/openhound_github/auth.py b/src/openhound_github/auth.py index 2cd2e34..0330aeb 100644 --- a/src/openhound_github/auth.py +++ b/src/openhound_github/auth.py @@ -253,7 +253,15 @@ def refresh_request(self, request: requests.PreparedRequest) -> bool: or should_refresh or request_authorization == current_authorization ): - self._refresh_token(response_triggered=True) + try: + self._refresh_token(response_triggered=True) + except Exception: + logger.warning( + "Failed to refresh GitHub App installation token for " + "installation %s during request retry", + self.installation.installation_id, + ) + return False request.headers["Authorization"] = f"Bearer {self.access_token}" diff --git a/tests/test_github_app_retry.py b/tests/test_github_app_retry.py index 00e0598..cd3f6c4 100644 --- a/tests/test_github_app_retry.py +++ b/tests/test_github_app_retry.py @@ -1,3 +1,4 @@ +import logging from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timedelta, timezone from threading import Event, Lock @@ -40,6 +41,13 @@ def token(self) -> TokenResponse: return super().token +class FailingFakeInstallation(FakeInstallation): + @property + def token(self) -> TokenResponse: + self.token_calls += 1 + raise RuntimeError("sensitive-token-data") + + class TrackingLock: def __init__(self) -> None: self._lock = Lock() @@ -161,6 +169,23 @@ def test_retry_policy_does_not_retry_replacement_token_bad_credentials() -> None assert installation.token_calls == 1 +def test_retry_policy_does_not_retry_when_token_refresh_fails(caplog) -> None: + installation = FailingFakeInstallation() + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + request = prepared_request("old-token") + + with caplog.at_level(logging.WARNING, logger="openhound_github.auth"): + should_retry = github_retry_policy(auth)(bad_credentials_response(request), None) + + assert should_retry is False + assert request.headers["Authorization"] == "Bearer old-token" + assert installation.token_calls == 1 + assert "Failed to refresh GitHub App installation token" in caplog.text + assert "sensitive-token-data" not in caplog.text + + def test_retry_policy_does_not_repair_bad_credentials_for_bearer_token_auth() -> None: request = prepared_request("static-token") From 74c5ace914b645e2ffbcdf928dda75356960819c Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 14:01:56 -0700 Subject: [PATCH 5/7] BED-9370: reject mismatched app auth API origins Validate the selected request API origin against the GithubInstallation API origin during auth initialization so retry matching and token minting cannot be configured for different hosts. Preserve the selected API URI for equivalent origins and add regression coverage for mismatched installation and request origins. --- src/openhound_github/auth.py | 13 +++++++++++-- tests/test_app_auth.py | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/openhound_github/auth.py b/src/openhound_github/auth.py index 0330aeb..2c766d5 100644 --- a/src/openhound_github/auth.py +++ b/src/openhound_github/auth.py @@ -177,10 +177,19 @@ def __init__( ): self.installation = installation self.refresh_margin_seconds = refresh_margin_seconds - self.api_uri = api_uri or getattr( + installation_api_uri = getattr( installation, "api_uri", "https://api.github.com/" ) - self._api_origin = _normalized_http_origin(self.api_uri) + selected_api_uri = api_uri or installation_api_uri + installation_api_origin = _normalized_http_origin(installation_api_uri) + selected_api_origin = _normalized_http_origin(selected_api_uri) + if selected_api_origin != installation_api_origin: + raise ValueError( + "GitHub App auth API URI origin must match installation API URI origin" + ) + + self.api_uri = selected_api_uri + self._api_origin = selected_api_origin self.access_token: str | None = None self.expires_at: datetime | None = None self._response_refreshed_token: str | None = None diff --git a/tests/test_app_auth.py b/tests/test_app_auth.py index bec15ce..761c674 100644 --- a/tests/test_app_auth.py +++ b/tests/test_app_auth.py @@ -9,6 +9,7 @@ from openhound_github import auth from openhound_github.auth import ( AccountConfig, + GitHubAppInstallationAuth, GithubSession, InstallationResponse, resolve_github_app_jwt_issuer, @@ -97,6 +98,19 @@ def test_legacy_installation_response_does_not_require_client_id() -> None: assert installation.app_id == 123456 +def test_github_app_installation_auth_rejects_mismatched_api_origins() -> None: + installation = SimpleNamespace( + installation_id="12345", + api_uri="https://ghe.example/api/v3/", + ) + + with pytest.raises(ValueError, match="must match installation API URI origin"): + GitHubAppInstallationAuth( + installation=installation, + api_uri="https://api.github.com/", + ) + + def test_enterprise_source_reuses_selected_issuer_for_installation_tokens( monkeypatch: pytest.MonkeyPatch, ) -> None: From b6efc7390fa01ea3483d561b33cc320fdb55740d Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 20:33:49 -0700 Subject: [PATCH 6/7] BED-9370: scope app token retry suppression per request Replace the response-refreshed token sentinel with weak request-local tracking so repeated 401s for the same repaired request still fail closed without blocking a later independent request from refreshing the same installation token. This preserves the anti-churn behavior while allowing collection to recover when a subsequently issued request receives its own bad-credentials response.\n\nAlso reject plaintext HTTP API origins for GitHub App JWT and installation-token sessions so custom GitHub Enterprise hosts cannot send app credentials over an unencrypted connection. Add regression coverage for independent request recovery and plaintext host rejection. --- src/openhound_github/auth.py | 25 ++++++++++++++----------- tests/test_app_auth.py | 9 +++++++++ tests/test_github_app_retry.py | 18 ++++++++++++++++++ 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/openhound_github/auth.py b/src/openhound_github/auth.py index 2c766d5..bbc8a10 100644 --- a/src/openhound_github/auth.py +++ b/src/openhound_github/auth.py @@ -3,6 +3,7 @@ from threading import Lock from typing import Iterator from urllib.parse import urlparse +from weakref import WeakKeyDictionary import requests from dlt.common.configuration import configspec @@ -21,11 +22,11 @@ def _normalized_http_origin(url: str) -> tuple[str, str, int | None]: parsed = urlparse(url) scheme = parsed.scheme.lower() - if scheme not in {"http", "https"} or not parsed.hostname: - raise ValueError("GitHub API URI must be an absolute HTTP(S) URL") + if scheme != "https" or not parsed.hostname: + raise ValueError("GitHub API URI must be an absolute HTTPS URL") port = parsed.port - if (scheme == "http" and port == 80) or (scheme == "https" and port == 443): + if scheme == "https" and port == 443: port = None return scheme, parsed.hostname.lower(), port @@ -192,7 +193,9 @@ def __init__( self._api_origin = selected_api_origin self.access_token: str | None = None self.expires_at: datetime | None = None - self._response_refreshed_token: str | None = None + self._response_refreshed_requests: WeakKeyDictionary[ + requests.PreparedRequest, str + ] = WeakKeyDictionary() self._token_lock = Lock() def _should_refresh(self) -> bool: @@ -202,16 +205,13 @@ def _should_refresh(self) -> bool: refresh_at = self.expires_at - timedelta(seconds=self.refresh_margin_seconds) return datetime.now(timezone.utc) >= refresh_at - def _refresh_token(self, *, response_triggered: bool = False) -> None: + def _refresh_token(self) -> None: logger.info( f"Refreshing access token for {self.installation.installation_id}" ) get_token = self.installation.token self.access_token = get_token.token self.expires_at = get_token.expires_at - self._response_refreshed_token = ( - get_token.token if response_triggered else None - ) def token(self, force_refresh: bool = False) -> str | None: if ( @@ -250,10 +250,11 @@ def refresh_request(self, request: requests.PreparedRequest) -> bool: f"Bearer {self.access_token}" if self.access_token is not None else None ) should_refresh = self._should_refresh() + repaired_authorization = self._response_refreshed_requests.get(request) if ( not should_refresh and request_authorization == current_authorization - and self.access_token == self._response_refreshed_token + and request_authorization == repaired_authorization ): return False @@ -263,7 +264,7 @@ def refresh_request(self, request: requests.PreparedRequest) -> bool: or request_authorization == current_authorization ): try: - self._refresh_token(response_triggered=True) + self._refresh_token() except Exception: logger.warning( "Failed to refresh GitHub App installation token for " @@ -272,7 +273,9 @@ def refresh_request(self, request: requests.PreparedRequest) -> bool: ) return False - request.headers["Authorization"] = f"Bearer {self.access_token}" + replacement_authorization = f"Bearer {self.access_token}" + request.headers["Authorization"] = replacement_authorization + self._response_refreshed_requests[request] = replacement_authorization return True diff --git a/tests/test_app_auth.py b/tests/test_app_auth.py index 761c674..236f274 100644 --- a/tests/test_app_auth.py +++ b/tests/test_app_auth.py @@ -111,6 +111,15 @@ def test_github_app_installation_auth_rejects_mismatched_api_origins() -> None: ) +def test_github_session_rejects_plaintext_api_uri() -> None: + with pytest.raises(ValueError, match="absolute HTTPS URL"): + GithubSession( + jwt_issuer="123456", + private_key_path="/tmp/github-app.pem", + api_uri="http://ghe.example/api/v3/", + ) + + def test_enterprise_source_reuses_selected_issuer_for_installation_tokens( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/test_github_app_retry.py b/tests/test_github_app_retry.py index cd3f6c4..3620549 100644 --- a/tests/test_github_app_retry.py +++ b/tests/test_github_app_retry.py @@ -169,6 +169,24 @@ def test_retry_policy_does_not_retry_replacement_token_bad_credentials() -> None assert installation.token_calls == 1 +def test_retry_policy_allows_fresh_request_to_refresh_replacement_token() -> None: + installation = FakeInstallation("new-token", "newer-token") + auth = GitHubAppInstallationAuth(installation=installation) + auth.access_token = "old-token" + auth.expires_at = datetime.now(timezone.utc) + timedelta(hours=1) + retry_policy = github_retry_policy(auth) + recovery_request = prepared_request("old-token") + + assert retry_policy(bad_credentials_response(recovery_request), None) is True + assert recovery_request.headers["Authorization"] == "Bearer new-token" + + independent_request = prepared_request("new-token") + + assert retry_policy(bad_credentials_response(independent_request), None) is True + assert independent_request.headers["Authorization"] == "Bearer newer-token" + assert installation.token_calls == 2 + + def test_retry_policy_does_not_retry_when_token_refresh_fails(caplog) -> None: installation = FailingFakeInstallation() auth = GitHubAppInstallationAuth(installation=installation) From 58133527075853f5f16d8886db9117df9f446d81 Mon Sep 17 00:00:00 2001 From: Jared Atkinson Date: Wed, 19 Aug 2026 21:30:09 -0700 Subject: [PATCH 7/7] BED-9434: harden repository GraphQL collection retries Treat malformed successful GraphQL responses as retryable so transient empty or truncated payloads do not silently terminate repository pagination. Detect GraphQL requests by endpoint path as well as rate-limit headers so the retry policy still applies when GitHub omits GraphQL-specific response metadata. Add repository pagination failure context with the last cursor and emitted repository count, along with focused tests for malformed JSON recovery, multi-page repository emission, and terminal failure logging. Surface the REST repository size on GH_Repository nodes so empty repositories can be distinguished from repositories that lost branch data during collection. --- src/openhound_github/helpers.py | 34 +++++- src/openhound_github/models/repository.py | 4 + .../resources/organization.py | 27 +++- tests/test_helpers.py | 102 ++++++++++++++++ tests/test_repository_rulesets.py | 115 +++++++++++++++--- 5 files changed, 259 insertions(+), 23 deletions(-) create mode 100644 tests/test_helpers.py diff --git a/src/openhound_github/helpers.py b/src/openhound_github/helpers.py index 55c5295..04e7761 100644 --- a/src/openhound_github/helpers.py +++ b/src/openhound_github/helpers.py @@ -1,6 +1,7 @@ import logging import time from typing import Optional +from urllib.parse import urlparse from dlt.common import jsonpath from dlt.sources.helpers import requests @@ -153,6 +154,25 @@ def _has_graphql_errors(response: requests.Response) -> bool: return isinstance(response_data, dict) and bool(response_data.get("errors")) +def _has_invalid_json_body(response: requests.Response) -> bool: + try: + response.json() + except ValueError: + return True + return False + + +def _is_graphql_response(response: requests.Response) -> bool: + if response.headers.get("x-ratelimit-resource") == "graphql": + return True + + request = response.request + if request is None or not request.url: + return False + + return urlparse(request.url).path.rstrip("/").endswith("/graphql") + + def github_retry_policy(auth: AuthConfigBase): def retry_policy( response: Optional[requests.Response], exception: Optional[BaseException] @@ -162,12 +182,11 @@ def retry_policy( headers = response.headers now = int(time.time()) - message = _response_message(response).lower() # DLT retries the same prepared request after long Retry-After sleeps. if ( response.status_code == 401 - and "bad credentials" in message + and "bad credentials" in _response_message(response).lower() and isinstance(auth, GitHubAppInstallationAuth) and response.request is not None ): @@ -180,7 +199,15 @@ def retry_policy( if ( response.status_code == 200 - and headers.get("x-ratelimit-resource") == "graphql" + and _is_graphql_response(response) + and _has_invalid_json_body(response) + ): + logger.warning("GraphQL response body was not valid JSON, retrying request") + return True + + if ( + response.status_code == 200 + and _is_graphql_response(response) and _has_graphql_errors(response) ): if headers.get("Retry-After"): @@ -199,6 +226,7 @@ def retry_policy( if response.status_code not in (403, 429): return False + message = _response_message(response).lower() if ( headers.get("x-ratelimit-remaining") == "0" or "api rate limit exceeded" in message diff --git a/src/openhound_github/models/repository.py b/src/openhound_github/models/repository.py index b35f393..3293b79 100644 --- a/src/openhound_github/models/repository.py +++ b/src/openhound_github/models/repository.py @@ -29,6 +29,7 @@ class GHRepositoryProperties(GHNodeProperties): disabled: Whether the repository is disabled. visibility: The visibility level: `public`, `private`, or `internal`. default_branch: The name of the default branch (e.g., `main`). + size: Repository size in kilobytes as reported by GitHub. open_issues_count: Number of open issues. allow_forking: Whether forking is allowed. web_commit_signoff_required: Whether web-based commits require sign-off. @@ -74,6 +75,7 @@ class GHRepositoryProperties(GHNodeProperties): disabled: bool | None = None visibility: str | None = None default_branch: str | None = None + size: int | None = None open_issues_count: int | None = None allow_forking: bool | None = None web_commit_signoff_required: bool | None = None @@ -195,6 +197,7 @@ class Repository(BaseAsset): disabled: bool | None = None visibility: str | None = None default_branch: str | None = None + size: int | None = None open_issues_count: int | None = None allow_forking: bool | None = None web_commit_signoff_required: bool | None = None @@ -240,6 +243,7 @@ def as_node(self) -> GHNode: disabled=self.disabled, visibility=self.visibility, default_branch=self.default_branch, + size=self.size, open_issues_count=self.open_issues_count, allow_forking=self.allow_forking, web_commit_signoff_required=self.web_commit_signoff_required, diff --git a/src/openhound_github/resources/organization.py b/src/openhound_github/resources/organization.py index 883ed9a..ba57127 100644 --- a/src/openhound_github/resources/organization.py +++ b/src/openhound_github/resources/organization.py @@ -916,6 +916,8 @@ def repositories_graphql(ctx: SourceContext): for org in ctx.organizations: org_name = org.org_name client = org.client + repository_cursor: str | None = None + emitted_repositories = 0 try: paginator = GraphQLCursorPaginator( page_info_path="data.organization.repositories.pageInfo", @@ -939,15 +941,36 @@ def repositories_graphql(ctx: SourceContext): for repo in repos_page["nodes"]: repo_record = {**repo} branch_rulesets = repo_record.pop("branchRulesets", None) or {} + emitted_repositories += 1 yield { **repo_record, "branch_ruleset_count": branch_rulesets.get("totalCount"), "org_login": org_name, } + + request_json = getattr(getattr(page_data, "request", None), "json", None) + if isinstance(request_json, dict): + variables = request_json.get("variables") + if isinstance(variables, dict): + repository_cursor = variables.get("after") except Exception as e: logger.error( - f"Error in resource 'repositories_graphql' processing organization '{org_name}': {e}", - extra={"resource": "repositories_graphql", "phase": "resource_iteration"}, + "Error in resource 'repositories_graphql' processing organization '%s' " + "at repository cursor %r after emitting %d repositories " + "(%s): %s", + org_name, + repository_cursor, + emitted_repositories, + type(e).__name__, + e, + extra={ + "resource": "repositories_graphql", + "phase": "resource_iteration", + "org_name": org_name, + "repository_cursor": repository_cursor, + "emitted_repositories": emitted_repositories, + "error_type": type(e).__name__, + }, ) continue diff --git a/tests/test_helpers.py b/tests/test_helpers.py new file mode 100644 index 0000000..be42929 --- /dev/null +++ b/tests/test_helpers.py @@ -0,0 +1,102 @@ +import json + +import requests +from dlt.sources.helpers.rest_client.auth import BearerTokenAuth +from dlt.sources.helpers.requests.retry import Client +from dlt.sources.helpers.requests.session import Session + +from openhound_github.helpers import github_retry_policy + + +def graphql_response( + *, + headers: dict[str, str] | None = None, + body: dict[str, object] | None = None, + text: str | None = None, + url: str = "https://ghe.example/api/v3/graphql", +) -> requests.Response: + response = requests.Response() + response.status_code = 200 + response.headers.update(headers or {}) + if text is not None: + response._content = text.encode("utf-8") + else: + response._content = json.dumps(body or {}).encode("utf-8") + response.request = requests.Request("POST", url).prepare() + return response + + +def test_retry_policy_recognizes_graphql_endpoint_without_resource_header() -> None: + response = graphql_response( + headers={"Retry-After": "0"}, + body={"errors": [{"message": "temporary GraphQL failure"}]}, + ) + + should_retry = github_retry_policy(BearerTokenAuth(token="static-token"))( + response, + None, + ) + + assert should_retry is True + + +def test_retry_policy_retries_malformed_graphql_json() -> None: + response = graphql_response(text='{"data":{"organization":') + + should_retry = github_retry_policy(BearerTokenAuth(token="static-token"))( + response, + None, + ) + + assert should_retry is True + + +def test_retry_policy_does_not_retry_malformed_non_graphql_json() -> None: + response = graphql_response( + text='{"data":{"organization":', + url="https://ghe.example/api/v3/repos/example/repo", + ) + + should_retry = github_retry_policy(BearerTokenAuth(token="static-token"))( + response, + None, + ) + + assert should_retry is False + + +def test_retry_client_recovers_from_malformed_graphql_json(monkeypatch) -> None: + responses = [ + graphql_response(text='{"data":{"organization":'), + graphql_response(body={"data": {"organization": {"repositories": {}}}}), + ] + requests_seen: list[requests.PreparedRequest] = [] + + def fake_send( + _session: Session, + request: requests.PreparedRequest, + **_kwargs, + ) -> requests.Response: + response = responses[len(requests_seen)] + response.request = request + requests_seen.append(request) + return response + + monkeypatch.setattr(Session, "send", fake_send) + session = Client( + raise_for_status=False, + status_codes=(), + exceptions=(), + request_max_attempts=2, + request_backoff_factor=0, + retry_condition=github_retry_policy(BearerTokenAuth(token="static-token")), + ).session + request = requests.Request( + "POST", + "https://ghe.example/api/v3/graphql", + ).prepare() + + response = session.send(request) + + assert response.json() == {"data": {"organization": {"repositories": {}}}} + assert len(requests_seen) == 2 diff --git a/tests/test_repository_rulesets.py b/tests/test_repository_rulesets.py index 79a0705..34c63e2 100644 --- a/tests/test_repository_rulesets.py +++ b/tests/test_repository_rulesets.py @@ -1,4 +1,6 @@ import duckdb +import logging +from types import SimpleNamespace from unittest.mock import MagicMock from openhound_github.lookup import GithubLookup @@ -15,27 +17,67 @@ def paginate(self, *args, **kwargs): return iter( [ [ + _repository_page_data("R_1", "repo", branch_ruleset_count=2) + ] + ] + ) + + +class _Page(list): + def __init__(self, *args, next_cursor: str | None): + super().__init__(*args) + self.request = SimpleNamespace(json={"variables": {"after": next_cursor}}) + + +def _repository_page_data( + repository_id: str, + repository_name: str, + *, + branch_ruleset_count: int | None = None, +) -> dict: + return { + "organization": { + "repositories": { + "nodes": [ { - "organization": { - "repositories": { - "nodes": [ - { - "id": "R_1", - "name": "repo", - "branchRulesets": {"totalCount": 2}, - "refs": { - "nodes": [], - "pageInfo": { - "endCursor": None, - "hasNextPage": False, - }, - }, - } - ] - } - } + "id": repository_id, + "name": repository_name, + "branchRulesets": {"totalCount": branch_ruleset_count}, + "refs": { + "nodes": [], + "pageInfo": { + "endCursor": None, + "hasNextPage": False, + }, + }, } ] + } + } + } + + +class _FailingSecondPageClient: + def paginate(self, *args, **kwargs): + yield _Page( + [_repository_page_data("R_1", "repo", branch_ruleset_count=2)], + next_cursor="cursor-page-2", + ) + raise ConnectionError("GraphQL page failed after retries") + + +class _TwoPageClient: + def paginate(self, *args, **kwargs): + return iter( + [ + _Page( + [_repository_page_data("R_1", "repo-1", branch_ruleset_count=2)], + next_cursor="cursor-page-2", + ), + _Page( + [_repository_page_data("R_2", "repo-2", branch_ruleset_count=0)], + next_cursor=None, + ), ] ) @@ -47,6 +89,7 @@ def _make_repository() -> Repository: name="repo", full_name="org/repo", private=False, + size=0, owner={ "login": "octocat", "id": 1, @@ -94,6 +137,41 @@ def test_repositories_graphql_flattens_branch_ruleset_count() -> None: ] +def test_repositories_graphql_logs_cursor_and_emitted_count_on_page_failure( + caplog, +) -> None: + client = _FailingSecondPageClient() + ctx = SourceContext( + client=client, + organizations=[OrgContext(client=client, org_name="org")], + ) + + with caplog.at_level(logging.ERROR, logger="openhound_github.resources.organization"): + rows = list(repositories_graphql.__wrapped__(ctx)) + + assert len(rows) == 1 + assert ( + "Error in resource 'repositories_graphql' processing organization 'org' " + "at repository cursor 'cursor-page-2' after emitting 1 repositories " + "(ConnectionError): GraphQL page failed after retries" + ) in caplog.text + + +def test_repositories_graphql_emits_all_repository_pages() -> None: + client = _TwoPageClient() + ctx = SourceContext( + client=client, + organizations=[OrgContext(client=client, org_name="org")], + ) + + rows = list(repositories_graphql.__wrapped__(ctx)) + + assert [(row["id"], row["branch_ruleset_count"]) for row in rows] == [ + ("R_1", 2), + ("R_2", 0), + ] + + def test_repository_node_surfaces_branch_ruleset_presence() -> None: repo = _make_repository() lookup = MagicMock() @@ -105,6 +183,7 @@ def test_repository_node_surfaces_branch_ruleset_presence() -> None: assert node.properties.branch_ruleset_count == 2 assert node.properties.has_branch_rulesets is True + assert node.properties.size == 0 lookup.repository_branch_ruleset_count.assert_called_once_with("R_1")