harden(agent-team): scrub token from HTTP transport errors; fail closed on bad expiry
Two defense-in-depth fixes surfaced by /sh-security-review (both were unverified — no exploit — but cheaply strengthen the credential contract): - dispatcher: wrap the requests.post/get in app_workflow_dispatcher and app_run_locator in try/except that re-raises DispatcherError with the exception TYPE only (`from None`). The no-token-in-a-propagating-exception guarantee is now enforced by code, not by requests' incidental behavior. - github_app: parse expires_at BEFORE caching the token and raise GitHubAppError (scrubbed) on a malformed value, so a parse failure fails closed without leaving a half-written cache (token set, expiry None) behind a bare ValueError. Tests: +3 (transport-error scrub for both HTTP seams; malformed-expiry fail-closed with no half-written cache). Full suite 1526 passing; ruff clean.
This commit is contained in:
parent
61ab1f0cd5
commit
183789a239
4 changed files with 104 additions and 15 deletions
|
|
@ -721,12 +721,20 @@ def app_workflow_dispatcher(
|
||||||
"Authorization": f"Bearer {token_provider.token()}",
|
"Authorization": f"Bearer {token_provider.token()}",
|
||||||
}
|
}
|
||||||
body = {"ref": ref, "inputs": inputs}
|
body = {"ref": ref, "inputs": inputs}
|
||||||
if _http is not None:
|
# Wrap the transport so a requests/transport exception can NEVER carry the
|
||||||
resp = _http.post(url, json=body, headers=headers, timeout=15.0)
|
# Bearer token out unscrubbed: re-raise as a DispatcherError with the
|
||||||
else:
|
# exception TYPE only (mirrors the git seam's secret-hygiene discipline).
|
||||||
import requests # deferred: optional dependency
|
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)
|
status = getattr(resp, "status_code", None)
|
||||||
if status != 204:
|
if status != 204:
|
||||||
raise DispatcherError(f"workflow dispatch failed: status={status}")
|
raise DispatcherError(f"workflow dispatch failed: status={status}")
|
||||||
|
|
@ -791,12 +799,21 @@ def app_run_locator(
|
||||||
}
|
}
|
||||||
|
|
||||||
for attempt in range(_LOCATE_ATTEMPTS):
|
for attempt in range(_LOCATE_ATTEMPTS):
|
||||||
if _http is not None:
|
# Wrap the transport so a requests/transport exception can NEVER carry
|
||||||
resp = _http.get(url, params=params, headers=headers, timeout=15.0)
|
# the Bearer token out unscrubbed (status/type only, like the git seam).
|
||||||
else:
|
try:
|
||||||
import requests # deferred: optional dependency
|
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)
|
# Surface auth/4xx promptly with the STATUS only (NEVER the token)
|
||||||
# instead of treating a 401/403/404 error body as "no runs" and
|
# instead of treating a 401/403/404 error body as "no runs" and
|
||||||
# silently exhausting the ~60s poll window. A genuine 200 with no
|
# silently exhausting the ~60s poll window. A genuine 200 with no
|
||||||
|
|
|
||||||
|
|
@ -170,8 +170,12 @@ class TokenProvider:
|
||||||
_http=self._http,
|
_http=self._http,
|
||||||
_now=self._now,
|
_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_token = result["token"]
|
||||||
self._cached_expiry = _parse_expires_at(result["expires_at"])
|
self._cached_expiry = expiry
|
||||||
return self._cached_token
|
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.
|
"""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
|
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
|
||||||
|
|
|
||||||
|
|
@ -479,6 +479,27 @@ def test_app_workflow_dispatcher_fails_closed_without_token_in_message() -> None
|
||||||
assert "ghs_TESTTOKEN" not in str(excinfo.value)
|
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)
|
# 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 "403" in str(excinfo.value)
|
||||||
assert "ghs_TESTTOKEN" not 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
|
||||||
|
|
|
||||||
|
|
@ -209,6 +209,31 @@ def test_provider_remints_near_expiry(rsa_keypair):
|
||||||
assert http.call_count == 2
|
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 #
|
# Secret hygiene #
|
||||||
# --------------------------------------------------------------------------- #
|
# --------------------------------------------------------------------------- #
|
||||||
|
|
|
||||||
Reference in a new issue