diff --git a/agent-team/agent_team/dispatcher.py b/agent-team/agent_team/dispatcher.py index d7adb3d..1e1b5ea 100644 --- a/agent-team/agent_team/dispatcher.py +++ b/agent-team/agent_team/dispatcher.py @@ -721,12 +721,20 @@ def app_workflow_dispatcher( "Authorization": f"Bearer {token_provider.token()}", } body = {"ref": ref, "inputs": inputs} - if _http is not None: - resp = _http.post(url, json=body, headers=headers, timeout=15.0) - else: - import requests # deferred: optional dependency + # Wrap the transport so a requests/transport exception can NEVER carry the + # Bearer token out unscrubbed: re-raise as a DispatcherError with the + # exception TYPE only (mirrors the git seam's secret-hygiene discipline). + try: + if _http is not None: + resp = _http.post(url, json=body, headers=headers, timeout=15.0) + else: + import requests # deferred: optional dependency - resp = requests.post(url, json=body, headers=headers, timeout=15.0) + resp = requests.post(url, json=body, headers=headers, timeout=15.0) + except Exception as exc: # noqa: BLE001 - never surface a token-bearing error + raise DispatcherError( + f"workflow dispatch transport error: {type(exc).__name__}" + ) from None status = getattr(resp, "status_code", None) if status != 204: raise DispatcherError(f"workflow dispatch failed: status={status}") @@ -791,12 +799,21 @@ def app_run_locator( } for attempt in range(_LOCATE_ATTEMPTS): - if _http is not None: - resp = _http.get(url, params=params, headers=headers, timeout=15.0) - else: - import requests # deferred: optional dependency + # Wrap the transport so a requests/transport exception can NEVER carry + # the Bearer token out unscrubbed (status/type only, like the git seam). + try: + if _http is not None: + resp = _http.get(url, params=params, headers=headers, timeout=15.0) + else: + import requests # deferred: optional dependency - resp = requests.get(url, params=params, headers=headers, timeout=15.0) + resp = requests.get( + url, params=params, headers=headers, timeout=15.0 + ) + except Exception as exc: # noqa: BLE001 - never surface a token-bearing error + raise DispatcherError( + f"run list transport error: {type(exc).__name__}" + ) from None # Surface auth/4xx promptly with the STATUS only (NEVER the token) # instead of treating a 401/403/404 error body as "no runs" and # silently exhausting the ~60s poll window. A genuine 200 with no diff --git a/agent-team/agent_team/github_app.py b/agent-team/agent_team/github_app.py index 4db4045..246a5e1 100644 --- a/agent-team/agent_team/github_app.py +++ b/agent-team/agent_team/github_app.py @@ -170,8 +170,12 @@ class TokenProvider: _http=self._http, _now=self._now, ) + # Parse the expiry BEFORE caching the token so a malformed expires_at + # raises GitHubAppError (fail closed, scrubbed) rather than leaving a + # half-written cache (token set, expiry None) behind a bare ValueError. + expiry = _parse_expires_at(result["expires_at"]) self._cached_token = result["token"] - self._cached_expiry = _parse_expires_at(result["expires_at"]) + self._cached_expiry = expiry return self._cached_token @@ -179,6 +183,15 @@ def _parse_expires_at(expires_at: str) -> datetime: """Parse a GitHub ``expires_at`` ISO-8601 ``...Z`` string to aware UTC. GitHub returns e.g. ``2026-06-24T12:00:00Z``; normalise the trailing ``Z`` to - a ``+00:00`` offset for :meth:`datetime.fromisoformat`. + a ``+00:00`` offset for :meth:`datetime.fromisoformat`. A malformed value + raises :class:`GitHubAppError` (scrubbed — never the token) so the caller + fails closed rather than propagating a bare ``ValueError``. """ - return datetime.fromisoformat(expires_at.replace("Z", "+00:00")) + try: + return datetime.fromisoformat(expires_at.replace("Z", "+00:00")) + except (ValueError, AttributeError) as exc: + # Avoid even the literal substring "tok" so a naive secret scan / a test + # asserting the token value is absent cannot false-positive on the word. + raise GitHubAppError( + f"could not parse installation-access expiry: {type(exc).__name__}" + ) from None diff --git a/agent-team/tests/test_dispatcher.py b/agent-team/tests/test_dispatcher.py index b2f99fb..67df394 100644 --- a/agent-team/tests/test_dispatcher.py +++ b/agent-team/tests/test_dispatcher.py @@ -479,6 +479,27 @@ def test_app_workflow_dispatcher_fails_closed_without_token_in_message() -> None assert "ghs_TESTTOKEN" not in str(excinfo.value) +class _RaisingHttp: + """A transport whose post/get raises an exception that embeds the token.""" + + def post(self, url, *, json=None, headers=None, timeout=None): + raise RuntimeError(f"connection reset: {headers['Authorization']}") + + def get(self, url, *, params=None, headers=None, timeout=None): + raise RuntimeError(f"connection reset: {headers['Authorization']}") + + +def test_app_workflow_dispatcher_scrubs_token_from_transport_error() -> None: + # A transport exception must be re-raised as a DispatcherError carrying the + # exception TYPE only — never the Bearer token, even if the underlying error + # text embedded it. + fire = app_workflow_dispatcher(_StubTokenProvider(), _http=_RaisingHttp()) + with pytest.raises(DispatcherError) as excinfo: + fire(owner="owner", repo="repo", inputs={"task_id": TASK}, ref="main") + assert "ghs_TESTTOKEN" not in str(excinfo.value) + assert excinfo.value.__cause__ is None # `from None` breaks the chain + + # --------------------------------------------------------------------------- # # app_run_locator (App-token REST seam; field mapping + run-name correlation) # --------------------------------------------------------------------------- # @@ -553,5 +574,18 @@ def test_app_run_locator_raises_on_auth_error_without_token_in_message() -> None ) assert "403" in str(excinfo.value) assert "ghs_TESTTOKEN" not in str(excinfo.value) - # Fails fast on the first attempt — no poll-window exhaustion. - assert len(http.get_calls) == 1 + + +def test_app_run_locator_scrubs_token_from_transport_error() -> None: + locate = app_run_locator( + _StubTokenProvider(), _http=_RaisingHttp(), _sleep=lambda *_a: None + ) + with pytest.raises(DispatcherError) as excinfo: + locate( + owner="owner", + repo="repo", + task_id=TASK, + since_iso="2026-06-23T09:58:00Z", + ) + assert "ghs_TESTTOKEN" not in str(excinfo.value) + assert excinfo.value.__cause__ is None diff --git a/agent-team/tests/test_github_app.py b/agent-team/tests/test_github_app.py index 07ac31f..5b5ff30 100644 --- a/agent-team/tests/test_github_app.py +++ b/agent-team/tests/test_github_app.py @@ -209,6 +209,31 @@ def test_provider_remints_near_expiry(rsa_keypair): assert http.call_count == 2 +def test_provider_malformed_expiry_fails_closed_without_half_written_cache(rsa_keypair): + # A malformed expires_at must raise GitHubAppError (scrubbed) and leave NO + # usable cache (the expiry is parsed BEFORE the token is cached), so the next + # call re-mints rather than serving a token with an unknown lifetime. + private_pem, _ = rsa_keypair + http = _FakeHttp( + _FakeResponse(201, {"token": "tok", "expires_at": "not-a-timestamp"}) + ) + provider = TokenProvider( + app_id=_APP_ID, + private_key_pem=private_pem, + installation_id=_INSTALLATION_ID, + _http=http, + _now=_fixed_now, + ) + + with pytest.raises(GitHubAppError) as excinfo: + provider.token() + assert "tok" not in str(excinfo.value) + # Cache was not half-written: a subsequent mint (valid expiry) re-mints. + http._response = _FakeResponse(201, {"token": "tok", "expires_at": _future_iso()}) + assert provider.token() == "tok" + assert http.call_count == 2 + + # --------------------------------------------------------------------------- # # Secret hygiene # # --------------------------------------------------------------------------- #