mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 16:19:09 +00:00
fix: restore transient workflow-token elevation (revert standing workflows:write)
The standing GitHub-App proxy token (BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS) is ALWAYS-ON, so carrying workflows:write on it made the fork's HITL workflow-push guard the sole control over unapproved workflow pushes. The guard's git-push parser has gaps (obfuscated-expansion push, `gh api` REST contents PUT, fully-qualified cross-branch refspecs); with a permanently workflows-scoped token those gaps become live unapproved-workflow-push exploits (1 critical, 2 high — security review BLOCK on #159). Restore dev's transient-elevation model: - Drop workflows:write from BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS; re-add the WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS constant (base + workflows:write). - Re-introduce _run_with_workflow_token in the guard: it mints the workflows-scoped token via refresh_proxy_token around the approved, guard-normalized fixed_command, then downscopes to RUNTIME then BASE in a finally. Route the approval branch through it. - Restore the dev token/elevation tests. The standing token no longer carries workflows:write, so the three parser bypasses hit GitHub 403 again; an approved push still succeeds because the elevation grants workflows:write only around the normalized command. Keeps all of #159's diff-preview / approval-URL / Slack-card guard additions.
This commit is contained in:
parent
f61a9053ec
commit
179867d120
7 changed files with 156 additions and 12 deletions
|
|
@ -7,6 +7,7 @@ import codecs
|
|||
import hashlib
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import shlex
|
||||
import threading
|
||||
|
|
@ -29,6 +30,12 @@ from ..dashboard.workflow_approval import (
|
|||
)
|
||||
from ..tools.slack_thread_reply import build_workflow_approval_blocks
|
||||
from ..utils.dashboard_links import dashboard_workflow_approval_url
|
||||
from ..utils.github_app import (
|
||||
BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS,
|
||||
RUNTIME_PROXY_TOKEN_PERMISSIONS,
|
||||
WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS,
|
||||
)
|
||||
from ..utils.github_proxy import refresh_proxy_token
|
||||
from ..utils.sandbox_state import SANDBOX_BACKENDS
|
||||
from ..utils.slack import post_slack_thread_reply_with_ts
|
||||
|
||||
|
|
@ -850,6 +857,48 @@ async def _approval_state(request: ToolCallRequest, change: WorkflowPushChange)
|
|||
return "approval_error"
|
||||
|
||||
|
||||
async def _run_with_workflow_token(
|
||||
thread_id: str,
|
||||
request: ToolCallRequest,
|
||||
run: Callable[[], Awaitable[ToolMessage | Command]],
|
||||
) -> ToolMessage | Command:
|
||||
sandbox_type = os.getenv("SANDBOX_TYPE", "langsmith")
|
||||
if sandbox_type != "langsmith":
|
||||
return await run()
|
||||
|
||||
elevated = await refresh_proxy_token(
|
||||
thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS
|
||||
)
|
||||
if not elevated:
|
||||
logger.error(
|
||||
"Workflow push approved for thread %s, but proxy token elevation to workflows:write "
|
||||
"failed; the sandbox cannot push workflow files without an elevated token.",
|
||||
thread_id,
|
||||
)
|
||||
error_message = ToolMessage(
|
||||
content=json.dumps(
|
||||
{
|
||||
"status": "error",
|
||||
"error_type": "WorkflowPushElevationFailed",
|
||||
"error": (
|
||||
"Workflow push approved, but the sandbox could not obtain a "
|
||||
"workflows-scoped token. Please retry the push or check the "
|
||||
"GitHub proxy / token minting configuration."
|
||||
),
|
||||
}
|
||||
),
|
||||
tool_call_id="",
|
||||
status="error",
|
||||
)
|
||||
return _tool_message_for_request(error_message, request)
|
||||
try:
|
||||
return await run()
|
||||
finally:
|
||||
restored = await refresh_proxy_token(thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS)
|
||||
if not restored:
|
||||
await refresh_proxy_token(thread_id, permissions=BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS)
|
||||
|
||||
|
||||
class WorkflowPushGuardMiddleware(AgentMiddleware):
|
||||
"""Require approval before pushing `.github/workflows` changes."""
|
||||
|
||||
|
|
@ -893,9 +942,9 @@ class WorkflowPushGuardMiddleware(AgentMiddleware):
|
|||
return _tool_message_for_request(_blocked_message(change, blocked=True), request)
|
||||
thread_id = _thread_id(request)
|
||||
state = await _approval_state(request, change)
|
||||
if state == "approved":
|
||||
if state == "approved" and thread_id:
|
||||
safe_request = _override_execute_command(request, change.fixed_command)
|
||||
return await handler(safe_request)
|
||||
return await _run_with_workflow_token(thread_id, request, lambda: handler(safe_request))
|
||||
if state == "stale_approval" and thread_id:
|
||||
record, _created = await ensure_workflow_push_pending(
|
||||
thread_id,
|
||||
|
|
|
|||
|
|
@ -31,12 +31,15 @@ BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS: dict[str, str] = {
|
|||
"pull_requests": "write",
|
||||
"issues": "write",
|
||||
"checks": "write",
|
||||
"workflows": "write",
|
||||
}
|
||||
RUNTIME_PROXY_TOKEN_PERMISSIONS: dict[str, str] = {
|
||||
**BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS,
|
||||
"actions": "read",
|
||||
}
|
||||
WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS: dict[str, str] = {
|
||||
**BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS,
|
||||
"workflows": "write",
|
||||
}
|
||||
|
||||
PermissionMap = Mapping[str, str]
|
||||
PermissionKey = tuple[tuple[str, str], ...]
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@
|
|||
{"sha": "e5dbc788", "pr": 1696, "subject": "fix: Harden durable agent runs (#1696)", "disposition": "deferred", "reason": "large durable-run hardening; 3 new modules dev lacks; rewrites fork dispatch/completion", "branch": "durable-dispatch", "local_sha": null, "updated": "2026-07-08T20:14:42Z"}
|
||||
{"sha": "52fe2916", "pr": 1698, "subject": "feat: add PR review link route (#1698)", "disposition": "landed", "reason": "cherry-picked (-x) in #127 (PR review link route)", "branch": "reviewer-misc", "local_sha": null, "updated": "2026-07-08T22:58:21Z"}
|
||||
{"sha": "5f7f5fbd", "pr": 1697, "subject": "fix: Reduce graph import and loader startup latency (#1697)", "disposition": "deferred", "reason": "import-hygiene refactor; cross-cutting, references many deferred upstream-only modules", "branch": "durable-dispatch", "local_sha": null, "updated": "2026-07-08T20:14:42Z"}
|
||||
{"sha": "216cf181", "pr": 1699, "subject": "fix: keep workflow HITL without token downscoping (#1699)", "disposition": "landed", "reason": "FLAG-HUMAN: broadens standing proxy token to workflows:write (auth surface, security-review pending) — cascade fully applied: github_app.py base perms + guard both ported; HITL approval gate preserved", "branch": "plan-approval", "local_sha": null, "updated": "2026-07-09T18:30:00Z"}
|
||||
{"sha": "216cf181", "pr": 1699, "subject": "fix: keep workflow HITL without token downscoping (#1699)", "disposition": "landed", "reason": "DIVERGES-FROM-UPSTREAM: fork deliberately does NOT adopt #1699's standing-token workflows:write broadening. Security review (#159) BLOCKed it — the standing ALWAYS-ON proxy token carrying workflows:write turns the HITL guard's git-push-parser gaps (obfuscated-expansion push, `gh api` REST contents PUT, cross-branch refspecs) into live unapproved-workflow-push exploits. Fork keeps BASE without workflows:write and restores the transient per-approval elevation (_run_with_workflow_token mints WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS around the approved, guard-normalized fixed_command, then downscopes to RUNTIME then BASE): the token scope is the backstop the parser relies on, so a bypass hits GitHub 403. HITL diff-preview/approval-URL/Slack-card additions from #159 retained; token-model divergence only.", "branch": "plan-approval", "local_sha": null, "updated": "2026-07-09T20:00:00Z"}
|
||||
{"sha": "67abf5b0", "pr": 1659, "subject": "fix: surface attributed PR creation failures (#1659)", "disposition": "deferred", "reason": "PR-attribution-failure guard (new mw, safe imports); heavy conflict on diverged open_pull_request.py", "branch": "pr-attribution", "local_sha": null, "updated": "2026-07-08T20:14:42Z"}
|
||||
{"sha": "3dbc0282", "pr": 1676, "subject": "fix: preserve plan redirects after login (#1676)", "disposition": "landed", "reason": "FLAG-HUMAN: follow-on to landed #1668 refining sanitize_redirect_to (open-redirect auth surface); not a dup", "branch": "plan-approval", "local_sha": null, "updated": "2026-07-09T17:10:22Z"}
|
||||
{"sha": "c75cbb1f", "pr": 1677, "subject": "feat: re-add Fable 5 with an admin toggle to disable it (#1677)", "disposition": "deferred", "reason": "FLAG-HUMAN: re-adds Fable 5 via anthropic: — contradicts dev's deliberate hide (#1483) + Bedrock migration (#62); wont-merge candidate", "branch": "fable-admin-toggle", "local_sha": null, "updated": "2026-07-08T20:14:42Z"}
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ Rows key on the **upstream SHA** (stable across local cherry-picks). Deferred ro
|
|||
| `20f63e8c` | #1646 | install missing deps before verification | Landed | already in dev via #81 (upstream-sync); ledger was stale | prompt-tweaks |
|
||||
| `2f237b53` | #1626 | fall back to vision model for image threads | Landed | ported (adapted to Bedrock/Fireworks vision) in #128 | gateway-routing |
|
||||
| `52fe2916` | #1698 | feat: add PR review link route (#1698) | Landed | cherry-picked (-x) in #127 (PR review link route) | reviewer-misc |
|
||||
| `216cf181` | #1699 | fix: keep workflow HITL without token downscoping (#1699) | Landed | FLAG-HUMAN: broadens standing proxy token to workflows:write (auth surface, security-review pending) — cascade fully applied: github_app.py base perms + guard both ported; HITL approval gate preserved | plan-approval |
|
||||
| `216cf181` | #1699 | fix: keep workflow HITL without token downscoping (#1699) | Landed | DIVERGES-FROM-UPSTREAM: fork deliberately does NOT adopt #1699's standing-token workflows:write broadening. Security review (#159) BLOCKed it — the standing ALWAYS-ON proxy token carrying workflows:write turns the HITL guard's git-push-parser gaps (obfuscated-expansion push, `gh api` REST contents PUT, cross-branch refspecs) into live unapproved-workflow-push exploits. Fork keeps BASE without workflows:write and restores the transient per-approval elevation (_run_with_workflow_token mints WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS around the approved, guard-normalized fixed_command, then downscopes to RUNTIME then BASE): the token scope is the backstop the parser relies on, so a bypass hits GitHub 403. HITL diff-preview/approval-URL/Slack-card additions from #159 retained; token-model divergence only. | plan-approval |
|
||||
| `3dbc0282` | #1676 | fix: preserve plan redirects after login (#1676) | Landed | FLAG-HUMAN: follow-on to landed #1668 refining sanitize_redirect_to (open-redirect auth surface); not a dup | plan-approval |
|
||||
| `bb104d93` | #1679 | fix: submit plan comments with cmd enter (#1679) | Landed | applies clean but edits fork-diverged PlanReview.tsx (#130); needs UI/e2e validation — separate PR | plan-approval |
|
||||
| `7f7af715` | #1684 | feat: auto-load scoped AGENTS on reads (#1684) | Landed | ported in #129 (SubdirAgentsReadMiddleware) | subdir-agents |
|
||||
|
|
|
|||
|
|
@ -147,12 +147,11 @@ async def test_installation_token_can_be_scoped_to_repository_ids(
|
|||
assert _FakeAsyncClient.last_post["json"] == {"repository_ids": [123]}
|
||||
|
||||
|
||||
def test_runtime_proxy_token_permissions_include_workflows_and_optional_actions() -> None:
|
||||
assert github_app.BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS["workflows"] == "write"
|
||||
def test_runtime_proxy_token_permissions_include_optional_read_only_actions() -> None:
|
||||
assert "actions" not in github_app.BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS
|
||||
assert github_app.RUNTIME_PROXY_TOKEN_PERMISSIONS["workflows"] == "write"
|
||||
assert github_app.RUNTIME_PROXY_TOKEN_PERMISSIONS["actions"] == "read"
|
||||
assert github_app.RUNTIME_PROXY_TOKEN_PERMISSIONS.get("actions") != "write"
|
||||
assert "actions" not in github_app.WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -210,7 +210,7 @@ class TestCreateSandboxWithProxy:
|
|||
|
||||
@pytest.mark.asyncio
|
||||
async def test_falls_back_when_optional_actions_permission_is_unavailable(self) -> None:
|
||||
"""Sandbox creation should still keep workflow scope without Actions read."""
|
||||
"""Sandbox creation should still work before an install grants Actions read."""
|
||||
with (
|
||||
patch(
|
||||
"agent.server.get_github_app_installation_token_with_expiry",
|
||||
|
|
@ -235,7 +235,6 @@ class TestCreateSandboxWithProxy:
|
|||
assert mock_get_token.await_args_list[1].kwargs["permissions"] == (
|
||||
BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS
|
||||
)
|
||||
assert mock_get_token.await_args_list[1].kwargs["permissions"]["workflows"] == "write"
|
||||
mock_proxy.assert_called_once_with("sandbox-123", "ghs_install")
|
||||
mock_record.assert_called_once_with(
|
||||
"thread-123",
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
from typing import Any
|
||||
|
||||
import pytest
|
||||
|
|
@ -347,15 +348,25 @@ async def test_unapproved_workflow_push_blocks_and_posts_slack(
|
|||
assert posted["blocks"][1]["elements"][0]["value"]
|
||||
|
||||
|
||||
async def test_approved_workflow_push_runs_fixed_command(
|
||||
async def test_approved_workflow_push_elevates_and_restores(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
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)
|
||||
|
||||
pushed_command = ""
|
||||
|
||||
|
|
@ -372,6 +383,40 @@ async def test_approved_workflow_push_runs_fixed_command(
|
|||
pushed_command
|
||||
== "git -C /repo push origin aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:refs/heads/feature"
|
||||
)
|
||||
assert refreshed[0]["workflows"] == "write"
|
||||
assert "workflows" not in refreshed[1]
|
||||
assert refreshed[1]["actions"] == "read"
|
||||
|
||||
|
||||
async def test_workflow_push_restoration_falls_back_when_actions_read_unavailable(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
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 "actions" not in permissions
|
||||
|
||||
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)
|
||||
|
||||
async def handler(_request: Any) -> ToolMessage:
|
||||
return ToolMessage(content="pushed", tool_call_id="call-1")
|
||||
|
||||
await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler)
|
||||
|
||||
assert refreshed[0]["workflows"] == "write"
|
||||
assert refreshed[1]["actions"] == "read"
|
||||
assert refreshed[2] == guard.BASE_RUNTIME_PROXY_TOKEN_PERMISSIONS
|
||||
assert "actions" not in refreshed[2]
|
||||
|
||||
|
||||
async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
|
|
@ -665,7 +710,7 @@ async def test_exact_rejection_checked_before_stale_approval(
|
|||
assert payload["workflow_approval_status"] == "rejected"
|
||||
|
||||
|
||||
async def test_approved_workflow_push_passes_through(
|
||||
async def test_approved_workflow_push_aborts_when_elevation_fails(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
guard.SANDBOX_BACKENDS["thread-1"] = _Backend()
|
||||
|
|
@ -673,7 +718,14 @@ async def test_approved_workflow_push_passes_through(
|
|||
async def fake_approved(thread_id: str, fingerprint: str) -> bool:
|
||||
return True
|
||||
|
||||
refresh_calls: list[dict[str, str]] = []
|
||||
|
||||
async def fake_refresh(*args: Any, **kwargs: Any) -> bool:
|
||||
refresh_calls.append(dict(kwargs.get("permissions", {})))
|
||||
return False
|
||||
|
||||
monkeypatch.setattr(guard, "workflow_push_approved", fake_approved)
|
||||
monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh)
|
||||
|
||||
called = False
|
||||
request = _Request()
|
||||
|
|
@ -685,9 +737,51 @@ async def test_approved_workflow_push_passes_through(
|
|||
|
||||
result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(request, handler)
|
||||
|
||||
assert called is False
|
||||
assert isinstance(result, ToolMessage)
|
||||
assert result.tool_call_id == "call-1"
|
||||
assert result.status == "error"
|
||||
payload = json.loads(str(result.content))
|
||||
assert payload["status"] == "error"
|
||||
assert payload["error_type"] == "WorkflowPushElevationFailed"
|
||||
assert "workflows-scoped token" in payload["error"]
|
||||
assert len(refresh_calls) == 1
|
||||
assert refresh_calls[0].get("workflows") == "write"
|
||||
|
||||
|
||||
async def test_approved_workflow_push_runs_on_non_langsmith_providers(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
guard.SANDBOX_BACKENDS["thread-1"] = _Backend()
|
||||
|
||||
async def fake_approved(thread_id: str, fingerprint: str) -> bool:
|
||||
return True
|
||||
|
||||
refresh_calls: list[dict[str, str]] = []
|
||||
|
||||
async def fake_refresh(*args: Any, **kwargs: Any) -> bool:
|
||||
refresh_calls.append(dict(kwargs.get("permissions", {})))
|
||||
return False
|
||||
|
||||
monkeypatch.setattr(guard, "workflow_push_approved", fake_approved)
|
||||
monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh)
|
||||
monkeypatch.setattr(guard, "os", os)
|
||||
|
||||
called = False
|
||||
|
||||
async def handler(request: Any) -> ToolMessage:
|
||||
nonlocal called
|
||||
called = True
|
||||
return ToolMessage(content="pushed", tool_call_id=request.tool_call["id"])
|
||||
|
||||
with monkeypatch.context() as mp:
|
||||
mp.setenv("SANDBOX_TYPE", "local")
|
||||
result = await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler)
|
||||
|
||||
assert called is True
|
||||
assert isinstance(result, ToolMessage)
|
||||
assert result.content == "pushed"
|
||||
assert refresh_calls == []
|
||||
|
||||
|
||||
def test_parse_git_push_guards_wrapped_and_optioned_forms() -> None:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue