fix(open-swe): harden proxy-token restore and mint error handling

Two low-severity follow-ups from the security review of the #1701 port.

Restore the recorded baseline scope after a workflow-push elevation instead
of a hardcoded RUNTIME. An install granted workflows:write but not actions:read
resolves its standing token to core; hardcoding RUNTIME on restore requested the
ungranted actions:read, 422'd, and fired a false "SECURITY: failed to downscope"
error on every approved workflow push before the core fallback recovered. The
guard now captures the run's recorded scope before elevating (via the new
get_recorded_proxy_permissions) and restores exactly that, falling back to the
guaranteed core scope only when the baseline restore fails.

Classify installation-token mint failures. get_github_app_installation_token_
with_expiry now treats HTTP 422 (a permission the installation hasn't granted)
as the ladder's expected descend signal and keeps it at debug, while a non-422
failure (network/5xx/timeout) is surfaced at WARNING even when errors are
otherwise suppressed — so a transient blip no longer silently downscopes a whole
run under a debug-only trace. The reduced-scope warning no longer asserts a
missing grant as the sole cause.
This commit is contained in:
Adam Moussa 2026-07-13 13:40:39 -04:00
parent df1f4a0e5d
commit 45567e007f
No known key found for this signature in database
6 changed files with 168 additions and 15 deletions

View file

@ -35,7 +35,7 @@ from ..utils.github_app import (
RUNTIME_PROXY_TOKEN_PERMISSIONS, RUNTIME_PROXY_TOKEN_PERMISSIONS,
WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS, WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS,
) )
from ..utils.github_proxy import refresh_proxy_token from ..utils.github_proxy import get_recorded_proxy_permissions, refresh_proxy_token
from ..utils.sandbox_state import SANDBOX_BACKENDS from ..utils.sandbox_state import SANDBOX_BACKENDS
from ..utils.slack import post_slack_thread_reply_with_ts from ..utils.slack import post_slack_thread_reply_with_ts
@ -876,6 +876,13 @@ async def _run_with_workflow_token(
if sandbox_type != "langsmith": if sandbox_type != "langsmith":
return await run() return await run()
# Capture the run's standing scope *before* elevating so we restore exactly
# what the install resolved to (RUNTIME, or CORE when actions:read is absent)
# rather than a hardcoded RUNTIME that 422s on installs lacking actions:read.
# The standing ladder never carries workflows:write, so this is always a real
# downscope. Fall back to RUNTIME if nothing was recorded yet.
baseline_scope = get_recorded_proxy_permissions(thread_id) or RUNTIME_PROXY_TOKEN_PERMISSIONS
elevated = await refresh_proxy_token( elevated = await refresh_proxy_token(
thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS
) )
@ -904,21 +911,22 @@ async def _run_with_workflow_token(
try: try:
return await run() return await run()
finally: finally:
restored = await refresh_proxy_token(thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS) restored = await refresh_proxy_token(thread_id, permissions=baseline_scope)
if not restored: if not restored and baseline_scope != CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS:
logger.error( logger.error(
"SECURITY: failed to downscope proxy token for thread %s after an approved " "SECURITY: failed to restore baseline proxy scope for thread %s after an "
"workflow push; retrying without actions:read.", "approved workflow push; retrying at the guaranteed core scope.",
thread_id, thread_id,
) )
if not await refresh_proxy_token( restored = await refresh_proxy_token(
thread_id, permissions=CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS thread_id, permissions=CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS
): )
logger.error( if not restored:
"SECURITY: proxy token downscope fully failed for thread %s; the sandbox " logger.error(
"may retain workflows:write for the remainder of this run.", "SECURITY: proxy token downscope fully failed for thread %s; the sandbox "
thread_id, "may retain workflows:write for the remainder of this run.",
) thread_id,
)
class WorkflowPushGuardMiddleware(AgentMiddleware): class WorkflowPushGuardMiddleware(AgentMiddleware):

View file

@ -227,8 +227,8 @@ async def _resolve_proxy_token(
continue continue
if index: if index:
logger.warning( logger.warning(
"GitHub proxy token minted with reduced scope %s; installation is " "GitHub proxy token minted with reduced scope %s; a higher-privilege "
"missing higher-privilege grants", "scope was unavailable (missing grant or transient mint failure)",
sorted(scope), sorted(scope),
) )
return token, expires_at, scope return token, expires_at, scope

View file

@ -189,9 +189,32 @@ async def get_github_app_installation_token_with_expiry(
if isinstance(token, str) and token and parsed is not None: if isinstance(token, str) and token and parsed is not None:
_TOKEN_CACHE[key] = (token, expires_at, parsed - _TOKEN_CACHE_MARGIN) _TOKEN_CACHE[key] = (token, expires_at, parsed - _TOKEN_CACHE_MARGIN)
return token, expires_at return token, expires_at
except httpx.HTTPStatusError as exc:
status = exc.response.status_code if exc.response is not None else None
if status == 422:
# Expected when the installation hasn't granted a requested permission:
# the ladder caller descends to a smaller scope. Not a transient error,
# so keep it at debug even on the terminal rung.
logger.debug(
"GitHub App token mint rejected for requested scope (HTTP 422)", exc_info=True
)
elif log_errors:
logger.exception("Failed to get GitHub App installation token")
else:
# A non-422 failure is NOT a missing grant; surface it so a transient
# error doesn't silently downscope a run that would otherwise qualify.
logger.warning(
"GitHub App token mint failed (HTTP %s); scope may degrade transiently",
status,
exc_info=True,
)
return None, None
except Exception: except Exception:
if log_errors: if log_errors:
logger.exception("Failed to get GitHub App installation token") logger.exception("Failed to get GitHub App installation token")
else: else:
logger.debug("Failed to get GitHub App installation token", exc_info=True) logger.warning(
"GitHub App token mint failed unexpectedly; scope may degrade transiently",
exc_info=True,
)
return None, None return None, None

View file

@ -91,6 +91,23 @@ def clear_proxy_token_expiry(thread_id: str | None) -> None:
_PROXY_TOKEN_EXPIRY.pop(thread_id, None) _PROXY_TOKEN_EXPIRY.pop(thread_id, None)
def get_recorded_proxy_permissions(thread_id: str | None) -> dict[str, str] | None:
"""The permission scope last minted for ``thread_id``'s proxy token, if any.
Lets a caller that temporarily elevates the proxy scope restore the exact
baseline the run resolved to (e.g. an install granted workflows:write but not
actions:read resolves to core), instead of guessing a fixed scope that may
422 on restore.
"""
if not thread_id:
return None
record = _PROXY_TOKEN_EXPIRY.get(thread_id)
if record is None:
return None
*_, permission_key = _unpack_proxy_token_record(record)
return dict(permission_key) if permission_key else None
def _unpack_proxy_token_record(record: tuple[Any, ...]) -> ProxyTokenRecord: def _unpack_proxy_token_record(record: tuple[Any, ...]) -> ProxyTokenRecord:
expires_at, recorded_at, repositories, *rest = record expires_at, recorded_at, repositories, *rest = record
permissions = rest[0] if rest else () permissions = rest[0] if rest else ()

View file

@ -1,8 +1,10 @@
from __future__ import annotations from __future__ import annotations
import logging
from datetime import UTC, datetime, timedelta from datetime import UTC, datetime, timedelta
from typing import Any from typing import Any
import httpx
import pytest import pytest
from agent.utils import github_app from agent.utils import github_app
@ -183,6 +185,68 @@ def test_proxy_token_ladder_descends_to_core() -> None:
) )
class _HTTPStatusErrorClient:
"""Raises an ``HTTPStatusError`` with a configurable status on mint."""
status = 500
def __init__(self, **kwargs: Any) -> None:
pass
async def __aenter__(self) -> _HTTPStatusErrorClient:
return self
async def __aexit__(self, exc_type: object, exc: object, tb: object) -> None:
return None
async def post(self, url: str, **kwargs: Any) -> Any:
request = httpx.Request("POST", url)
response = httpx.Response(type(self).status, request=request)
raise httpx.HTTPStatusError("mint failed", request=request, response=response)
@pytest.mark.asyncio
async def test_missing_grant_422_descends_quietly(
monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
) -> None:
"""A 422 (ungranted permission) is the ladder's expected descend signal, so it
must not be logged as a transient failure even on a non-terminal rung."""
class Client(_HTTPStatusErrorClient):
status = 422
_configure(monkeypatch, Client)
with caplog.at_level(logging.DEBUG, logger="agent.utils.github_app"):
token, _ = await github_app.get_github_app_installation_token_with_expiry(
permissions={"actions": "read"}, log_errors=False
)
assert token is None
assert not any(r.levelno >= logging.WARNING for r in caplog.records)
@pytest.mark.asyncio
async def test_transient_mint_error_warns_even_when_errors_suppressed(
monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
) -> None:
"""A non-422 failure is not a missing grant; it must surface at WARNING even on
a non-terminal rung so a blip doesn't silently downscope a whole run."""
class Client(_HTTPStatusErrorClient):
status = 503
_configure(monkeypatch, Client)
with caplog.at_level(logging.DEBUG, logger="agent.utils.github_app"):
token, _ = await github_app.get_github_app_installation_token_with_expiry(
permissions={"actions": "read"}, log_errors=False
)
assert token is None
assert any(r.levelno == logging.WARNING for r in caplog.records)
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_installation_token_includes_permissions(monkeypatch: pytest.MonkeyPatch) -> None: async def test_installation_token_includes_permissions(monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.setattr(github_app, "GITHUB_APP_ID", "1") monkeypatch.setattr(github_app, "GITHUB_APP_ID", "1")

View file

@ -420,6 +420,47 @@ async def test_workflow_push_restoration_falls_back_when_actions_read_unavailabl
assert "workflows" not in refreshed[2] assert "workflows" not in refreshed[2]
async def test_workflow_push_restores_recorded_baseline_scope(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""Restore targets the run's recorded baseline scope (here: core, because the
install never granted actions:read) — not a hardcoded RUNTIME that would 422."""
guard.SANDBOX_BACKENDS["thread-1"] = _Backend()
refreshed: list[dict[str, str]] = []
async def fake_approved(thread_id: str, fingerprint: str) -> bool:
return True
async def fake_refresh(thread_id: str | None, *, permissions: dict[str, str]) -> bool:
refreshed.append(dict(permissions))
return True
async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None:
return None
monkeypatch.setattr(guard, "workflow_push_approved", fake_approved)
monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval)
monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh)
monkeypatch.setattr(
guard,
"get_recorded_proxy_permissions",
lambda _thread_id: dict(guard.CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS),
)
async def handler(_request: Any) -> ToolMessage:
return ToolMessage(content="pushed", tool_call_id="call-1")
await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler)
# Elevate to workflows:write, then restore straight to the recorded core scope:
# no spurious RUNTIME attempt (which would 422 for this install) and no false
# "failed to downscope" error.
assert refreshed[0]["workflows"] == "write"
assert refreshed[1] == guard.CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS
assert "workflows" not in refreshed[1]
assert len(refreshed) == 2
async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None: async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None:
# Head contains workflow files, but they are identical to the trusted remote base, # Head contains workflow files, but they are identical to the trusted remote base,
# so no approval is required for a code-only change. # so no approval is required for a code-only change.