open-swe/tests/test_repo_binding_isolation.py
Adam Moussa a33aaec495
fix: resolve security-review findings (sandbox isolation, IAM list scope, webhook replay, info-leak) (#54)
* fix: enforce a replay window on Linear webhooks (AUTHZ-001)

verify_linear_signature accepted any correctly-signed body with no freshness
check, so a captured request could be replayed indefinitely. Parse the
signed webhookTimestamp (Unix ms) and reject requests outside a 60s window,
failing closed when the field is missing or malformed — mirroring the Slack
verifier.

* fix: stop leaking upstream auth-error bodies into user comments

get_github_token_for_user folded the raw upstream response text into the
error string that becomes a Slack/Linear comment (AUTH-RESP-LEAK-01). Log the
full body server-side only and return a generic "GitHub auth failed (status
<code>)". Also document the accepted shared-installation-token blast radius on
the bot-token-only path (AUTHZ-003).

* fix: bind sandbox and token caches to repo to prevent thread-id collision

A PR head-branch name is attacker-controllable and get_thread_id_from_branch
derives a thread_id from its first UUID with no repo binding (TID-COLLIDE-01).
The in-memory sandbox cache and the per-thread GitHub-token cache were keyed on
thread_id alone, and a cached sandbox was reused after only an echo-ping, so a
different repo's webhook could bind to another thread's sandbox or token.

Without changing the persistent thread-id scheme:
- Persist the bound repo (owner/name) in thread metadata on sandbox creation and
  refuse to reuse a sandbox whose bound repo does not match the current event
  (SandboxRepoMismatchError); the in-memory proxy also carries the binding.
- Bind the GitHub-token cache entries to their repo and evict on a cross-repo
  read so a colliding thread_id cannot be served another repo's token.
- Thread repo through the reviewer and the webhook token resolvers.

* fix: scope s3:ListBucket to the releases/ prefix (F-1/IAC-04)

The instance role and the GitHub deploy app role granted s3:ListBucket on the
whole assets bucket. Every caller (deploy.sh, the publish/rollback scripts)
only ever lists under releases/, so add a StringLike s3:prefix=releases/*
condition. GetBucketLocation has no s3:prefix in its request context, so it
moves to its own unconditioned statement. Also document the accepted F-2
cross-env existence-oracle residual on BatchGetSecretValue.

* chore: suppress test-fixture credential false positive; document AUTHZ-002

Add a machine-level suppression for the fake Datadog key in the
test_team_credentials encryption-roundtrip fixture (CWE-798, not a real
credential). Clarify that the within-org thread-write path is intentional by
design (AUTHZ-002) — comment only, no behavior change.

* fix: casefold repo-binding keys to avoid spurious cross-repo mismatch

GitHub owner/name are case-insensitive. Casefold the owner/name key on both the
write (binding) and read (compare) sides — repo_cache_key and the metadata
bound_repo read — so Org/Repo and org/repo resolve to one repo and a legitimate
same-repo run cannot raise a spurious SandboxRepoMismatchError (Gap 2).

* fix: stop leaking upstream auth body in unexpected-result branch

The 2xx-but-missing-token/url branch echoed the parsed upstream response body
into the user-facing error. Return a generic message and log response_data
server-side only, mirroring the existing HTTPStatusError fix (Gap 4).

* fix: fail closed for unbound-legacy sandboxes and catch repo mismatch

Gap 1: a thread with a persisted sandbox_id but no in-memory cache and no
recorded bound_repo (a pre-binding legacy thread, post-deploy) previously
reconnected-and-served the sandbox to the current repo, then rebound it. Now
fail closed: drop the stale id and recreate a fresh sandbox bound to this repo,
logging a reconnect-with-missing-binding event. A sandbox is never served to a
repo unless its binding is known and matches; new threads bind on first run
unchanged.

Gap 3: catch SandboxRepoMismatchError at the agent and reviewer run entrypoints,
log it for alarming, and surface a clean sanitized error instead of letting an
opaque deep-stack exception crash-loop the worker.

* chore: suppress test-fixture credential false positive in token-TTL tests

Add a machine-level suppression for the fake "ghp_secret" GitHub token used by
the cached-token TTL/revocation unit tests (CWE-798). Not a real credential and
not a valid PAT; scoped to the unit test only.
2026-06-29 12:21:19 -04:00

225 lines
8.5 KiB
Python

"""Repo-binding isolation for the sandbox and GitHub-token caches.
Covers TID-COLLIDE-01: a second repo presenting a colliding thread_id must not
reuse the first repo's in-process sandbox or cached GitHub token.
"""
from __future__ import annotations
from datetime import UTC, datetime, timedelta
from typing import Any
import pytest
from agent import server
from agent.utils import github_token
@pytest.fixture(autouse=True)
def _clear_caches() -> None:
github_token._GITHUB_TOKEN_CACHE.clear()
server.SANDBOX_BACKENDS.clear()
# --- token cache repo binding ------------------------------------------------
def test_cached_token_not_reused_across_repos() -> None:
"""A token bound to repo A is refused for a colliding thread_id from repo B."""
future = (datetime.now(UTC) + timedelta(hours=1)).isoformat()
github_token.cache_github_token_for_thread(
"tid", "ghp_repoA", expires_at=future, repo={"owner": "acme", "name": "alpha"}
)
# Same thread_id, different repo → must NOT serve repo A's token.
cfg_b = {"configurable": {"thread_id": "tid", "repo": {"owner": "evil", "name": "beta"}}}
assert github_token.get_github_token(cfg_b) is None
# And the poisoned entry is evicted.
assert "tid" not in github_token._GITHUB_TOKEN_CACHE
def test_cached_token_reused_for_same_repo() -> None:
future = (datetime.now(UTC) + timedelta(hours=1)).isoformat()
github_token.cache_github_token_for_thread(
"tid", "ghp_repoA", expires_at=future, repo={"owner": "acme", "name": "alpha"}
)
cfg_a = {"configurable": {"thread_id": "tid", "repo": {"owner": "acme", "name": "alpha"}}}
assert github_token.get_github_token(cfg_a) == "ghp_repoA"
def test_unbound_token_served_when_repo_unknown() -> None:
"""Legacy entries with no bound repo still resolve when no repo is supplied."""
github_token.cache_github_token_for_thread("tid", "ghp_legacy")
assert github_token.get_github_token({"configurable": {"thread_id": "tid"}}) == "ghp_legacy"
def test_cached_token_reused_for_same_repo_case_insensitive() -> None:
"""``Org/Repo`` and ``org/repo`` are the same repo: no spurious refusal."""
future = (datetime.now(UTC) + timedelta(hours=1)).isoformat()
github_token.cache_github_token_for_thread(
"tid", "ghp_repoA", expires_at=future, repo={"owner": "Acme", "name": "Alpha"}
)
cfg = {"configurable": {"thread_id": "tid", "repo": {"owner": "acme", "name": "alpha"}}}
assert github_token.get_github_token(cfg) == "ghp_repoA"
def test_repo_cache_key_normalizes() -> None:
assert github_token.repo_cache_key({"owner": "o", "name": "r"}) == "o/r"
assert github_token.repo_cache_key("o/r") == "o/r"
assert github_token.repo_cache_key({"owner": "o"}) is None
assert github_token.repo_cache_key(None) is None
# Casefolded so different casing of the same repo collapses to one key.
assert github_token.repo_cache_key({"owner": "Org", "name": "Repo"}) == "org/repo"
assert github_token.repo_cache_key("Org/Repo") == "org/repo"
# --- sandbox repo binding ----------------------------------------------------
class _FakeBackend:
def __init__(self, sandbox_id: str) -> None:
self.id = sandbox_id
self.bound_repo: str | None = None
def execute(self, *_a: Any, **_k: Any) -> Any: # pragma: no cover - must not run
raise AssertionError("colliding-repo sandbox must not be pinged/reused")
@pytest.mark.asyncio
async def test_ensure_sandbox_refuses_colliding_repo(monkeypatch: pytest.MonkeyPatch) -> None:
async def fake_sandbox_id(_tid: str) -> str:
return "sb-A"
async def fake_bound_repo(_tid: str) -> str:
return "acme/alpha"
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
backend = _FakeBackend("sb-A")
backend.bound_repo = "acme/alpha"
server.SANDBOX_BACKENDS["tid"] = backend # type: ignore[assignment]
with pytest.raises(server.SandboxRepoMismatchError):
await server.ensure_sandbox_for_thread("tid", repo={"owner": "evil", "name": "beta"})
@pytest.mark.asyncio
async def test_ensure_sandbox_allows_matching_repo(monkeypatch: pytest.MonkeyPatch) -> None:
async def fake_sandbox_id(_tid: str) -> str:
return "sb-A"
async def fake_bound_repo(_tid: str) -> str:
return "acme/alpha"
calls: dict[str, int] = {"git": 0}
backend = _FakeBackend("sb-A")
backend.bound_repo = "acme/alpha"
async def fake_check(b: Any, *_a: Any, **_k: Any) -> Any:
return b
async def fake_refresh(b: Any, *_a: Any, **_k: Any) -> Any:
return b
async def fake_git(_b: Any) -> None:
calls["git"] += 1
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
monkeypatch.setattr(server, "check_or_recreate_sandbox", fake_check)
monkeypatch.setattr(server, "_refresh_github_proxy_or_recreate", fake_refresh)
monkeypatch.setattr(server, "set_sandbox_backend", lambda _tid, b, **_k: b)
monkeypatch.setattr(server, "_configure_git_identity", fake_git)
server.SANDBOX_BACKENDS["tid"] = backend # type: ignore[assignment]
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "acme", "name": "alpha"})
assert result is backend
assert calls["git"] == 1
@pytest.mark.asyncio
async def test_ensure_sandbox_reuses_same_repo_case_insensitive(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""A bound sandbox is reused when the current repo differs only by casing."""
async def fake_sandbox_id(_tid: str) -> str:
return "sb-A"
async def fake_bound_repo(_tid: str) -> str:
return "acme/alpha"
backend = _FakeBackend("sb-A")
backend.bound_repo = "acme/alpha"
async def fake_check(b: Any, *_a: Any, **_k: Any) -> Any:
return b
async def fake_refresh(b: Any, *_a: Any, **_k: Any) -> Any:
return b
async def fake_git(_b: Any) -> None:
return None
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
monkeypatch.setattr(server, "check_or_recreate_sandbox", fake_check)
monkeypatch.setattr(server, "_refresh_github_proxy_or_recreate", fake_refresh)
monkeypatch.setattr(server, "set_sandbox_backend", lambda _tid, b, **_k: b)
monkeypatch.setattr(server, "_configure_git_identity", fake_git)
server.SANDBOX_BACKENDS["tid"] = backend # type: ignore[assignment]
# Different casing of the same repo must not raise and must reuse the sandbox.
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "ACME", "name": "Alpha"})
assert result is backend
@pytest.mark.asyncio
async def test_legacy_unbound_sandbox_recreated_not_reused(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""A legacy thread (sandbox_id present, bound_repo absent) must fail closed.
With no recorded binding, the existing sandbox cannot be confirmed to belong
to the current repo, so it is never reconnected-and-served: a fresh sandbox is
created and bound to the requesting repo instead.
"""
async def fake_sandbox_id(_tid: str) -> str:
return "sb-legacy"
async def fake_bound_repo(_tid: str) -> None:
return None
def fake_reconnect(*_a: Any, **_k: Any) -> Any: # pragma: no cover - must not run
raise AssertionError("must not reconnect to a legacy unbound sandbox")
fresh = _FakeBackend("sb-fresh")
async def fake_create_with_proxy(*_a: Any, **_k: Any) -> Any:
return fresh
async def fake_git(_b: Any) -> None:
return None
class _Threads:
async def update(self, **_k: Any) -> None:
return None
async def get(self, *_a: Any, **_k: Any) -> dict[str, Any]:
return {}
class _Client:
threads = _Threads()
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
monkeypatch.setattr(server, "create_sandbox", fake_reconnect)
monkeypatch.setattr(server, "_create_sandbox_with_proxy", fake_create_with_proxy)
monkeypatch.setattr(server, "_configure_git_identity", fake_git)
monkeypatch.setattr(server, "client", _Client())
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "evil", "name": "beta"})
assert result.id == "sb-fresh"
assert server.SANDBOX_BACKENDS["tid"].bound_repo == "evil/beta"