diff --git a/.security-review/suppressions.json b/.security-review/suppressions.json index e13ea036..47ed2662 100644 --- a/.security-review/suppressions.json +++ b/.security-review/suppressions.json @@ -19,6 +19,45 @@ "suppression_justification": "ACCEPTED LOW residual, metadata-only. fetch-config.sh materializes the .env via `batch-get-secret-value --filters Key=name,Values=open-swe-/`. With a name FILTER, both BatchGetSecretValue (a collection call) and ListSecrets are authorized by AWS against `*`, NOT a per-secret ARN — a prefix-scoped ARN AccessDenies the call (confirmed empirically on i-0af4e03e8bf70e6c3). So the two `*` grants are operation-level, not value-level. Secret VALUES remain strictly gated by the PREFIX-scoped GetSecretValue/DescribeSecret on secret:open-swe-/* (GetSecretValue is checked per-secret even within the batch), so cross-env VALUE isolation is preserved; only NAMES/tags/descriptions are enumerable, within Sea Haven's own single-tenant account 328440206208. Confirmed by GPT-4.1 IAM cross-review (BLOCK: none) and the iac-iam detector (one low residual, no critical/high). Future hardening to eliminate BOTH `*` grants: switch fetch-config.sh to an explicit `--secret-id-list` (no filter), which lets BatchGetSecretValue be prefix-scoped and needs no ListSecrets.", "owner": "adam@seahavenind.com", "added": "2026-06-26" + }, + { + "id": "gitleaks-generic-api-key-89", + "title": "Hardcoded credential flagged in encryption-roundtrip test fixture (CWE-798)", + "file": "tests/test_team_credentials.py", + "line": 89, + "rule": "CWE-798", + "severity": "low", + "status": "false-positive", + "justification": "Test fixture, not a real credential. The value \"secret-api-1234\" is a fake Datadog API key used by test_datadog_roundtrip_and_redaction to assert that the plaintext key is recoverable after an encrypt/decrypt round-trip (and that the stored record holds ciphertext, not the plaintext). It is never a live secret and is scoped to the unit test only.", + "suppression_justification": "Test fixture, not a real credential. The value \"secret-api-1234\" is a fake Datadog API key used by test_datadog_roundtrip_and_redaction to assert that the plaintext key is recoverable after an encrypt/decrypt round-trip. It is never a live secret and is scoped to the unit test only.", + "owner": "adam@seahavenind.com", + "added": "2026-06-29" + }, + { + "id": "gitleaks-generic-api-key-79", + "title": "Hardcoded credential flagged in encryption-roundtrip test fixture (CWE-798)", + "file": "tests/test_team_credentials.py", + "line": 79, + "rule": "CWE-798", + "severity": "low", + "status": "false-positive", + "justification": "Test fixture, not a real credential. Same fake Datadog API key \"secret-api-1234\" passed into connect_datadog by test_datadog_roundtrip_and_redaction. Never a live secret; scoped to the unit test only.", + "suppression_justification": "Test fixture, not a real credential. Same fake Datadog API key \"secret-api-1234\" passed into connect_datadog by test_datadog_roundtrip_and_redaction. Never a live secret; scoped to the unit test only.", + "owner": "adam@seahavenind.com", + "added": "2026-06-29" + }, + { + "id": "gitleaks-generic-api-key-23", + "title": "Hardcoded credential flagged in GitHub-token TTL test fixture (CWE-798)", + "file": "tests/test_github_token_ttl.py", + "line": 23, + "rule": "CWE-798", + "severity": "high", + "status": "false-positive", + "justification": "Test fixture, not a real credential. The literal \"ghp_secret\" is a fake GitHub token used by the cached-token TTL/revocation unit tests to exercise cache_github_token_for_thread / get_github_token expiry and invalidation. It is not a valid 40-char GitHub PAT, is never a live secret, and is scoped to the unit test only. Pre-existing test fixture, not introduced by this change.", + "suppression_justification": "Test fixture, not a real credential. The literal \"ghp_secret\" is a fake GitHub token used by the cached-token TTL/revocation unit tests to exercise cache_github_token_for_thread / get_github_token expiry and invalidation. It is not a valid 40-char GitHub PAT, is never a live secret, and is scoped to the unit test only. Pre-existing test fixture, not introduced by this change.", + "owner": "adam@seahavenind.com", + "added": "2026-06-29" } ] } diff --git a/agent/dashboard/thread_api.py b/agent/dashboard/thread_api.py index cdad0161..e99ac78d 100644 --- a/agent/dashboard/thread_api.py +++ b/agent/dashboard/thread_api.py @@ -1206,6 +1206,8 @@ async def send_dashboard_message( raise HTTPException(404, "thread not found") from exc metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + # AUTHZ-002 (intentional): any org-gated member who can read a surfaced thread + # may also post into it; non-owners are attributed via _attribution_prefix. _assert_thread_readable(metadata) prompt = f"{_attribution_prefix(metadata, login, email)}{body.content.strip()}" diff --git a/agent/reviewer.py b/agent/reviewer.py index 99e28020..c6b8794c 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -59,6 +59,7 @@ from .server import ( DEFAULT_LLM_MAX_TOKENS, DEFAULT_RECURSION_LIMIT, MODEL_CALL_RECURSION_LIMIT, + SandboxRepoMismatchError, _general_purpose_subagent, ensure_sandbox_for_thread, graph_loaded_for_execution, @@ -839,7 +840,9 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: f"GitHub App installation token unavailable for reviewer thread {thread_id}" ) # Cache in-process so reviewer tools and the sandbox proxy can read it this run. - cache_github_token_for_thread(thread_id, github_token, expires_at=expires_at) + cache_github_token_for_thread( + thread_id, github_token, expires_at=expires_at, repo=repo_config + ) github_proxy_token = github_token github_api_token = github_token @@ -849,12 +852,18 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: if repo_config.get("owner") and repo_config.get("name") else None ) - sandbox_backend = await ensure_sandbox_for_thread( - thread_id, - github_proxy_token=github_proxy_token, - github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None, - repo=repo_for_snapshot, - ) + try: + sandbox_backend = await ensure_sandbox_for_thread( + thread_id, + github_proxy_token=github_proxy_token, + github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None, + repo=repo_for_snapshot, + ) + except SandboxRepoMismatchError as exc: + # Repo-binding refusal at the run boundary: log for alarming and surface the + # sanitized terminal error rather than crash-looping the reviewer worker. + logger.error("Refusing reviewer run for thread %s: %s", thread_id, exc) + raise RuntimeError(str(exc)) from exc work_dir = await aresolve_sandbox_work_dir(sandbox_backend) diff --git a/agent/server.py b/agent/server.py index 42e58661..558825c4 100644 --- a/agent/server.py +++ b/agent/server.py @@ -98,6 +98,7 @@ from .utils.github_app import ( get_github_app_installation_token_with_expiry, ) from .utils.github_proxy import record_proxy_token_expiry +from .utils.github_token import repo_cache_key from .utils.model import ( DEFAULT_LLM_REASONING, ModelKwargs, @@ -117,6 +118,7 @@ SANDBOX_POLL_INTERVAL = 1.0 from .utils.sandbox_state import ( SANDBOX_BACKENDS, + get_bound_repo_from_metadata, get_sandbox_id_from_metadata, set_sandbox_backend, unwrap_sandbox_backend, @@ -405,6 +407,24 @@ def graph_loaded_for_execution(config: RunnableConfig) -> bool: ) +class SandboxRepoMismatchError(RuntimeError): + """Raised when a thread_id is presented for a repo it is not bound to. + + A thread is bound to exactly one repo. A different repo presenting a + colliding thread_id (e.g. an attacker-named branch whose first UUID matches + another thread) must never reuse this thread's sandbox or token. + """ + + def __init__(self, thread_id: str, bound_repo: str, current_repo: str) -> None: + self.thread_id = thread_id + self.bound_repo = bound_repo + self.current_repo = current_repo + super().__init__( + f"Thread {thread_id} is bound to repo {bound_repo}, " + f"refusing to serve sandbox for {current_repo}" + ) + + async def ensure_sandbox_for_thread( thread_id: str, *, @@ -435,6 +455,43 @@ async def ensure_sandbox_for_thread( logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id) sandbox_id = await _resolve_creating_sentinel(thread_id) + # Repo-binding guard (TID-COLLIDE-01): a sandbox is never served to a repo + # unless its binding is known and matches. + current_repo = repo_cache_key(repo) + bound_repo = await get_bound_repo_from_metadata(thread_id) + proxy_bound = getattr(sandbox_backend, "bound_repo", None) + effective_bound = bound_repo or (proxy_bound if isinstance(proxy_bound, str) else None) + if current_repo and effective_bound and effective_bound != current_repo: + # Known binding that does not match the current repo: refuse outright so a + # colliding thread_id from a different repo cannot reuse/clobber it. + logger.error( + "Repo mismatch for thread %s: bound=%s current=%s; refusing sandbox reuse", + thread_id, + effective_bound, + current_repo, + ) + raise SandboxRepoMismatchError(thread_id, effective_bound, current_repo) + if ( + current_repo + and not effective_bound + and sandbox_backend is None + and isinstance(sandbox_id, str) + and sandbox_id not in (None, SANDBOX_CREATING) + ): + # Fail CLOSED for unbound-legacy threads (migration window): a thread with a + # persisted sandbox_id but no in-memory cache and no recorded bound_repo + # cannot be confirmed to belong to the current repo, so never + # reconnect-and-serve it. Drop the stale id and recreate a fresh sandbox + # bound to this repo below. + logger.error( + "reconnect-with-missing-binding for thread %s: persisted sandbox %s has no " + "bound_repo; refusing reuse and recreating for repo %s", + thread_id, + sandbox_id, + current_repo, + ) + sandbox_id = None + if sandbox_backend: logger.info("Using cached sandbox backend for thread %s", thread_id) original_sandbox_id = sandbox_backend.id @@ -493,12 +550,15 @@ async def ensure_sandbox_for_thread( sandbox_backend, thread_id, github_proxy_token, github_proxy_repositories, repo ) - sandbox_backend = set_sandbox_backend(thread_id, sandbox_backend) + sandbox_backend = set_sandbox_backend(thread_id, sandbox_backend, repo=current_repo) + metadata_update: dict[str, Any] = {} if sandbox_id != sandbox_backend.id: - await client.threads.update( - thread_id=thread_id, metadata={"sandbox_id": sandbox_backend.id} - ) + metadata_update["sandbox_id"] = sandbox_backend.id + if current_repo and bound_repo != current_repo: + metadata_update["bound_repo"] = current_repo + if metadata_update: + await client.threads.update(thread_id=thread_id, metadata=metadata_update) # Re-apply git identity every run: cached/reconnected sandboxes may have # lost their `--global` config (or had it overwritten), and Vercel preview @@ -629,11 +689,21 @@ async def get_agent(config: RunnableConfig) -> Pregel: ) team_defaults_task = asyncio.create_task(get_team_default_model_pair("agent")) profile_task = asyncio.create_task(load_profile(profile_login)) if profile_login else None - triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather( - triggering_user_identity_task, - sandbox_task, - team_defaults_task, - ) + try: + triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather( + triggering_user_identity_task, + sandbox_task, + team_defaults_task, + ) + except SandboxRepoMismatchError as exc: + # Repo-binding refusal at the run boundary: log for alarming and surface the + # already-sanitized terminal error (no sandbox/token internals) to the caller, + # rather than letting an opaque deep-stack exception crash-loop the worker. + logger.error("Refusing agent run for thread %s: %s", thread_id, exc) + for pending in (triggering_user_identity_task, team_defaults_task, profile_task): + if pending is not None and not pending.done(): + pending.cancel() + raise RuntimeError(str(exc)) from exc profile = await profile_task if profile_task is not None else None del github_token diff --git a/agent/utils/auth.py b/agent/utils/auth.py index b279e730..53ea6c1e 100644 --- a/agent/utils/auth.py +++ b/agent/utils/auth.py @@ -192,11 +192,18 @@ async def get_github_token_for_user(ls_user_id: str, tenant_id: str) -> dict[str return result if auth_url: return {"auth_url": auth_url} - return {"error": f"Unexpected auth result: {response_data}"} + # Log the full upstream body server-side only; the returned error becomes a + # user-facing Slack/Linear comment, so never echo the raw response body. + logger.error( + "GitHub auth returned an unexpected result (no token/url): %s", response_data + ) + return {"error": "GitHub auth returned an unexpected result"} except httpx.HTTPStatusError as e: + # Log the full upstream body server-side only; the returned error becomes a + # user-facing Slack/Linear comment, so never echo the raw response text. logger.error("GitHub auth API HTTP error: %s - %s", e.response.status_code, e.response.text) - return {"error": f"HTTP error: {e.response.status_code} - {e.response.text}"} + return {"error": f"GitHub auth failed (status {e.response.status_code})"} except Exception as e: # noqa: BLE001 logger.error("GitHub auth API call failed: %s: %s", type(e).__name__, str(e)) return {"error": str(e)} @@ -286,10 +293,19 @@ async def leave_failure_comment( raise ValueError(f"Unknown source: {source}") +def _current_repo() -> Any: + """Best-effort read of the run's repo (owner/name) for cache binding.""" + try: + configurable = get_config().get("configurable", {}) + except Exception: + return None + return configurable.get("repo") if isinstance(configurable, dict) else None + + def _cache_resolved_github_token( thread_id: str, token: str, expires_at: str | None = None ) -> tuple[str, str | None]: - cache_github_token_for_thread(thread_id, token, expires_at=expires_at) + cache_github_token_for_thread(thread_id, token, expires_at=expires_at, repo=_current_repo()) return token, expires_at @@ -383,7 +399,13 @@ async def _resolve_dashboard_user_token( async def _resolve_bot_installation_token(thread_id: str) -> tuple[str, str | None]: - """Get a GitHub App installation token and cache it for the thread.""" + """Get a GitHub App installation token and cache it for the thread. + + AUTHZ-003 (accepted): in bot-token-only mode every run shares one GitHub App + installation token, so its blast radius is the whole installation rather than + a single user. This is a documented, accepted prod posture for this + single-tenant deployment, not a defect. + """ bot_token, expires_at = await get_github_app_installation_token_with_expiry() if not bot_token: raise RuntimeError( @@ -444,7 +466,9 @@ async def resolve_github_token(config: RunnableConfig, thread_id: str) -> tuple[ try: if source == "github": - cached_token, cached_expires_at = await get_github_token_from_thread(thread_id) + cached_token, cached_expires_at = await get_github_token_from_thread( + thread_id, expected_repo=configurable.get("repo") + ) if cached_token: return cached_token, cached_expires_at from ..dashboard.user_mappings import email_for_login diff --git a/agent/utils/github_token.py b/agent/utils/github_token.py index 1738bf3c..b44353eb 100644 --- a/agent/utils/github_token.py +++ b/agent/utils/github_token.py @@ -17,22 +17,42 @@ _GITHUB_TOKEN_EXPIRY_SKEW_SECONDS = 60 # Hard cap on how long an entry stays cached regardless of the token's own # expiry, so entries for threads that are never read again don't accumulate. _GITHUB_TOKEN_MAX_TTL = timedelta(hours=24) -# thread_id -> (token, token_expires_at, cached_at) -_GITHUB_TOKEN_CACHE: dict[str, tuple[str, str | None, datetime]] = {} +# thread_id -> (token, token_expires_at, cached_at, bound_repo). ``bound_repo`` +# ("owner/name") binds the entry to the repo it was resolved for so a colliding +# thread_id originating from a different repo cannot reuse another repo's token. +_GITHUB_TOKEN_CACHE: dict[str, tuple[str, str | None, datetime, str | None]] = {} class GitHubAuthError(Exception): """Raised when a GitHub call returns 401, signalling a stale/revoked token.""" +def repo_cache_key(repo: Any) -> str | None: + """Normalize a repo dict/string to a casefolded ``owner/name`` (None if unknown). + + GitHub owner/name are case-insensitive, so the key is casefolded on both the + write (binding) and read (compare) sides to keep ``Org/Repo`` and ``org/repo`` + a single repo and avoid spurious cross-repo mismatches. + """ + if isinstance(repo, str): + cleaned = repo.strip() + return cleaned.casefold() or None + if isinstance(repo, Mapping): + owner = repo.get("owner") + name = repo.get("name") + if isinstance(owner, str) and isinstance(name, str) and owner and name: + return f"{owner}/{name}".casefold() + return None + + def cache_github_token_for_thread( - thread_id: str, token: str, expires_at: str | None = None + thread_id: str, token: str, expires_at: str | None = None, *, repo: Any = None ) -> None: """Cache a GitHub token in process for the current thread.""" if not thread_id or not token: return now = datetime.now(UTC) - _GITHUB_TOKEN_CACHE[thread_id] = (token, expires_at, now) + _GITHUB_TOKEN_CACHE[thread_id] = (token, expires_at, now, repo_cache_key(repo)) _evict_expired(now=now) @@ -78,24 +98,37 @@ def _evict_expired(*, now: datetime | None = None) -> None: current = now or datetime.now(UTC) stale = [ tid - for tid, (_token, expires_at, cached_at) in _GITHUB_TOKEN_CACHE.items() + for tid, (_token, expires_at, cached_at, _repo) in _GITHUB_TOKEN_CACHE.items() if _entry_expired(expires_at, cached_at, now=current) ] for tid in stale: _GITHUB_TOKEN_CACHE.pop(tid, None) -def _cached_token_if_fresh(thread_id: str | None) -> tuple[str | None, str | None]: +def _cached_token_if_fresh( + thread_id: str | None, *, expected_repo: Any = None +) -> tuple[str | None, str | None]: if not thread_id: return None, None cached = _GITHUB_TOKEN_CACHE.get(thread_id) if not cached: return None, None - token, expires_at, cached_at = cached + token, expires_at, cached_at, bound_repo = cached if _entry_expired(expires_at, cached_at, now=datetime.now(UTC)): _GITHUB_TOKEN_CACHE.pop(thread_id, None) logger.info("Cached GitHub token for thread %s has expired; re-resolving", thread_id) return None, None + expected = repo_cache_key(expected_repo) + if expected and bound_repo and expected != bound_repo: + _GITHUB_TOKEN_CACHE.pop(thread_id, None) + logger.warning( + "Cached GitHub token for thread %s is bound to repo %s, not %s; " + "refusing cross-repo reuse", + thread_id, + bound_repo, + expected, + ) + return None, None return token, expires_at @@ -107,16 +140,27 @@ def _thread_id_from_config(run_config: Mapping[str, Any]) -> str | None: return thread_id if isinstance(thread_id, str) and thread_id else None +def _repo_from_config(run_config: Mapping[str, Any]) -> Any: + configurable = run_config.get("configurable", {}) + if not isinstance(configurable, Mapping): + return None + return configurable.get("repo") + + def get_github_token(run_config: Mapping[str, Any] | None = None) -> str | None: """Resolve the current thread's GitHub token from process memory.""" resolved = run_config if run_config is not None else get_config() - token, _expires_at = _cached_token_if_fresh(_thread_id_from_config(resolved)) + token, _expires_at = _cached_token_if_fresh( + _thread_id_from_config(resolved), expected_repo=_repo_from_config(resolved) + ) return token -async def get_github_token_from_thread(thread_id: str) -> tuple[str | None, str | None]: +async def get_github_token_from_thread( + thread_id: str, *, expected_repo: Any = None +) -> tuple[str | None, str | None]: """Resolve the current process's cached GitHub token for a thread.""" - return _cached_token_if_fresh(thread_id) + return _cached_token_if_fresh(thread_id, expected_repo=expected_repo) async def invalidate_cached_github_token(thread_id: str) -> None: diff --git a/agent/utils/sandbox_state.py b/agent/utils/sandbox_state.py index caf7b6b4..70764344 100644 --- a/agent/utils/sandbox_state.py +++ b/agent/utils/sandbox_state.py @@ -19,6 +19,7 @@ from deepagents.backends.protocol import ( ) from langgraph.config import get_config +from .github_token import repo_cache_key from .sandbox import create_sandbox logger = logging.getLogger(__name__) @@ -29,6 +30,9 @@ class SandboxBackendProxy(SandboxBackendProtocol): def __init__(self, backend: SandboxBackendProtocol) -> None: self._backend = backend + # "owner/name" of the repo this sandbox is bound to, used to refuse + # reuse by a different repo presenting a colliding thread_id. + self.bound_repo: str | None = None @property def current(self) -> SandboxBackendProtocol: @@ -131,21 +135,44 @@ def unwrap_sandbox_backend(sandbox_backend: SandboxBackendProtocol) -> SandboxBa def set_sandbox_backend( thread_id: str, sandbox_backend: SandboxBackendProtocol, + *, + repo: str | None = None, ) -> SandboxBackendProxy: if isinstance(sandbox_backend, SandboxBackendProxy): + if repo: + sandbox_backend.bound_repo = repo SANDBOX_BACKENDS[thread_id] = sandbox_backend return sandbox_backend existing = SANDBOX_BACKENDS.get(thread_id) if isinstance(existing, SandboxBackendProxy): existing.replace_backend(sandbox_backend) + if repo: + existing.bound_repo = repo return existing proxy = SandboxBackendProxy(sandbox_backend) + if repo: + proxy.bound_repo = repo SANDBOX_BACKENDS[thread_id] = proxy return proxy +async def get_bound_repo_from_metadata(thread_id: str) -> str | None: + """Fetch the repo (``owner/name``) this thread's sandbox is bound to.""" + try: + config = get_config() + except Exception: + return None + metadata = config.get("metadata", {}) + if not isinstance(metadata, dict): + return None + bound_repo = metadata.get("bound_repo") + # Casefold on read so legacy metadata written before normalization (e.g. + # ``Org/Repo``) still compares equal to the casefolded current repo key. + return repo_cache_key(bound_repo) if isinstance(bound_repo, str) and bound_repo else None + + def clear_sandbox_backend(thread_id: str) -> None: SANDBOX_BACKENDS.pop(thread_id, None) diff --git a/agent/webapp.py b/agent/webapp.py index c7909c6a..f686ba16 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -1339,8 +1339,32 @@ async def process_slack_mention(event_data: dict[str, Any], repo_config: dict[st ) +LINEAR_WEBHOOK_MAX_AGE_SECONDS = 60 + + +def _linear_timestamp_is_fresh(body: bytes) -> bool: + """Reject replays: the signed payload's ``webhookTimestamp`` must be recent. + + Linear includes ``webhookTimestamp`` (Unix milliseconds) inside the signed + body. Fail closed when it is missing or malformed. + """ + try: + ts_ms = json.loads(body)["webhookTimestamp"] + except (json.JSONDecodeError, KeyError, TypeError): + logger.warning("Linear webhook missing/invalid webhookTimestamp — rejecting") + return False + if not isinstance(ts_ms, (int, float)) or isinstance(ts_ms, bool): + logger.warning("Linear webhook webhookTimestamp is not numeric — rejecting") + return False + now_ms = datetime.now(UTC).timestamp() * 1000 + if abs(now_ms - ts_ms) > LINEAR_WEBHOOK_MAX_AGE_SECONDS * 1000: + logger.warning("Linear webhook timestamp outside freshness window — rejecting") + return False + return True + + def verify_linear_signature(body: bytes, signature: str, secret: str) -> bool: - """Verify the Linear webhook signature. + """Verify the Linear webhook signature and replay-freshness window. Args: body: Raw request body bytes @@ -1348,15 +1372,17 @@ def verify_linear_signature(body: bytes, signature: str, secret: str) -> bool: secret: The webhook signing secret Returns: - True if signature is valid, False otherwise + True if the signature is valid AND the signed timestamp is fresh. """ if not secret: logger.warning("LINEAR_WEBHOOK_SECRET is not configured — rejecting webhook request") return False expected = hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest() + if not hmac.compare_digest(expected, signature): + return False - return hmac.compare_digest(expected, signature) + return _linear_timestamp_is_fresh(body) @app.post("/webhooks/linear") @@ -2920,31 +2946,36 @@ async def process_github_autofix_review(payload: dict[str, Any], event_type: str ) -async def _refresh_thread_github_token_after_401(thread_id: str, email: str) -> str | None: +async def _refresh_thread_github_token_after_401( + thread_id: str, email: str, *, repo: dict[str, str] | None = None +) -> str | None: """Invalidate the cached token after a 401 and try to resolve a fresh one.""" logger.warning( "GitHub returned 401 for thread %s; invalidating cached token and re-resolving", thread_id, ) await invalidate_cached_github_token(thread_id) - return await _get_or_resolve_thread_github_token(thread_id, email) + return await _get_or_resolve_thread_github_token(thread_id, email, repo=repo) -async def _get_or_resolve_thread_github_token(thread_id: str, email: str) -> str | None: +async def _get_or_resolve_thread_github_token( + thread_id: str, email: str, *, repo: dict[str, str] | None = None +) -> str | None: """Resolve and cache a GitHub token for a thread when available. In bot-token-only mode, returns a fresh GitHub App installation token - instead of resolving per-user OAuth tokens. + instead of resolving per-user OAuth tokens. ``repo`` (owner/name) binds the + cached entry so a colliding thread_id from a different repo cannot reuse it. """ if is_bot_token_only_mode(): bot_token, expires_at = await get_github_app_installation_token_with_expiry() if bot_token: - cache_github_token_for_thread(thread_id, bot_token, expires_at=expires_at) + cache_github_token_for_thread(thread_id, bot_token, expires_at=expires_at, repo=repo) return bot_token logger.warning("Bot-token-only mode but GitHub App token unavailable") return None - github_token, _expires_at = await get_github_token_from_thread(thread_id) + github_token, _expires_at = await get_github_token_from_thread(thread_id, expected_repo=repo) if github_token: return github_token @@ -2955,7 +2986,10 @@ async def _get_or_resolve_thread_github_token(thread_id: str, email: str) -> str expires_at = auth_result.get("expires_at") cache_github_token_for_thread( - thread_id, github_token, expires_at=expires_at if isinstance(expires_at, str) else None + thread_id, + github_token, + expires_at=expires_at if isinstance(expires_at, str) else None, + repo=repo, ) return github_token @@ -3017,7 +3051,7 @@ async def process_github_pr_comment(payload: dict[str, Any], event_type: str) -> email = await email_for_login(github_login) or "" if email: - github_token = await _get_or_resolve_thread_github_token(thread_id, email) + github_token = await _get_or_resolve_thread_github_token(thread_id, email, repo=repo_config) else: logger.warning("No email mapping for GitHub user '%s', skipping", github_login) return @@ -3037,7 +3071,9 @@ async def process_github_pr_comment(payload: dict[str, Any], event_type: str) -> node_id=node_id, ) except GitHubAuthError: - github_token = await _refresh_thread_github_token_after_401(thread_id, email) + github_token = await _refresh_thread_github_token_after_401( + thread_id, email, repo=repo_config + ) if not github_token: logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id) return @@ -3059,7 +3095,9 @@ async def process_github_pr_comment(payload: dict[str, Any], event_type: str) -> repo_config, pr_number, token=github_token ) except GitHubAuthError: - github_token = await _refresh_thread_github_token_after_401(thread_id, email) + github_token = await _refresh_thread_github_token_after_401( + thread_id, email, repo=repo_config + ) if not github_token: logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id) return @@ -3297,7 +3335,7 @@ async def process_github_issue(payload: dict[str, Any], event_type: str) -> None thread_id = generate_thread_id_from_github_issue(issue_id) existing_thread = await _thread_exists(thread_id) - github_token = await _get_or_resolve_thread_github_token(thread_id, email) + github_token = await _get_or_resolve_thread_github_token(thread_id, email, repo=repo_config) app_token = await get_github_app_installation_token() reaction_token = github_token or app_token comment = payload.get("comment", {}) @@ -3314,7 +3352,9 @@ async def process_github_issue(payload: dict[str, Any], event_type: str) -> None token=reaction_token, ) except GitHubAuthError: - github_token = await _refresh_thread_github_token_after_401(thread_id, email) + github_token = await _refresh_thread_github_token_after_401( + thread_id, email, repo=repo_config + ) reaction_token = github_token or app_token reacted = False if reaction_token: @@ -3348,7 +3388,9 @@ async def process_github_issue(payload: dict[str, Any], event_type: str) -> None repo_config, issue_number, token=github_token or app_token ) except GitHubAuthError: - github_token = await _refresh_thread_github_token_after_401(thread_id, email) + github_token = await _refresh_thread_github_token_after_401( + thread_id, email, repo=repo_config + ) comments = await fetch_issue_comments( repo_config, issue_number, token=github_token or app_token ) diff --git a/infra/lib/constructs/github-deploy-roles.ts b/infra/lib/constructs/github-deploy-roles.ts index fe7533f6..8d3bfeca 100644 --- a/infra/lib/constructs/github-deploy-roles.ts +++ b/infra/lib/constructs/github-deploy-roles.ts @@ -150,10 +150,22 @@ export class GithubDeployRoles extends Construct { resources: [`arn:aws:s3:::open-swe-${envName}-assets/releases/*`], }), ); + // ListBucket is constrained to the releases/ prefix (F-1/IAC-04): the + // publish/rollback scripts only ever list under releases/, so a leaked CI + // token cannot enumerate anything else in the bucket. GetBucketLocation + // carries no s3:prefix, so it stays a separate, unconditioned statement. this.appRole.addToPolicy( new iam.PolicyStatement({ sid: "ListArtifactBucket", - actions: ["s3:ListBucket", "s3:GetBucketLocation"], + actions: ["s3:ListBucket"], + resources: [`arn:aws:s3:::open-swe-${envName}-assets`], + conditions: { StringLike: { "s3:prefix": ["releases/*"] } }, + }), + ); + this.appRole.addToPolicy( + new iam.PolicyStatement({ + sid: "GetArtifactBucketLocation", + actions: ["s3:GetBucketLocation"], resources: [`arn:aws:s3:::open-swe-${envName}-assets`], }), ); diff --git a/infra/lib/constructs/instance-role.ts b/infra/lib/constructs/instance-role.ts index 2acbba39..4a32de9a 100644 --- a/infra/lib/constructs/instance-role.ts +++ b/infra/lib/constructs/instance-role.ts @@ -43,10 +43,23 @@ export class InstanceRole extends Construct { resources: [`arn:aws:s3:::${p}-assets/releases/*`], }), ); + // ListBucket is constrained to the releases/ prefix (F-1/IAC-04) — the box + // only ever lists release artifacts, so a compromised box cannot enumerate + // any other object that might land in the bucket. GetBucketLocation has no + // s3:prefix in its request context, so it stays a separate, unconditioned + // statement (the condition would otherwise AccessDeny it). this.role.addToPolicy( new iam.PolicyStatement({ sid: "ListArtifactBucket", - actions: ["s3:ListBucket", "s3:GetBucketLocation"], + actions: ["s3:ListBucket"], + resources: [`arn:aws:s3:::${p}-assets`], + conditions: { StringLike: { "s3:prefix": ["releases/*"] } }, + }), + ); + this.role.addToPolicy( + new iam.PolicyStatement({ + sid: "GetArtifactBucketLocation", + actions: ["s3:GetBucketLocation"], resources: [`arn:aws:s3:::${p}-assets`], }), ); @@ -85,6 +98,11 @@ export class InstanceRole extends Construct { // holds. The win that DID survive: fetch-config uses `--secret-id-list` (explicit // names, no name filter), so `secretsmanager:ListSecrets` is NOT needed and is // intentionally omitted — the box cannot enumerate secret names account-wide. + // F-2 (accepted residual): because the grant is `*`, a caller naming a secret + // in ANOTHER env's prefix learns whether that name EXISTS (an existence oracle + // via the per-secret AccessDenied-vs-not signal) even though the VALUE stays + // gated by the prefix-scoped GetSecretValue above. Accepted within Sea Haven's + // single-tenant account 328440206208 — cross-env VALUE isolation is preserved. this.role.addToPolicy( new iam.PolicyStatement({ sid: "BatchGetSecretValues", diff --git a/infra/test/s3-list-prefix.test.ts b/infra/test/s3-list-prefix.test.ts new file mode 100644 index 00000000..ba76fcd4 --- /dev/null +++ b/infra/test/s3-list-prefix.test.ts @@ -0,0 +1,71 @@ +import * as cdk from "aws-cdk-lib"; +import { Match, Template } from "aws-cdk-lib/assertions"; +import { OpenSweIamStack } from "../lib/open-swe-iam-stack"; +import { OpenSweStack } from "../lib/open-swe-stack"; + +const ENV = { account: "328440206208", region: "us-east-1" }; + +// F-1 / IAC-04: s3:ListBucket must be constrained to the releases/ prefix so a +// compromised box / leaked CI token cannot enumerate the rest of the bucket. +const RELEASES_PREFIX_CONDITION = { StringLike: { "s3:prefix": ["releases/*"] } }; + +describe("S3 ListBucket prefix scoping (F-1/IAC-04)", () => { + it("instance role ListBucket is constrained to releases/*", () => { + const app = new cdk.App(); + const stack = new OpenSweStack(app, "OpenSweDevStack", { + stackName: "open-swe-dev", + env: ENV, + envName: "dev", + }); + Template.fromStack(stack).hasResourceProperties("AWS::IAM::Policy", { + PolicyDocument: Match.objectLike({ + Statement: Match.arrayWith([ + Match.objectLike({ + Sid: "ListArtifactBucket", + Action: "s3:ListBucket", + Condition: RELEASES_PREFIX_CONDITION, + }), + ]), + }), + }); + }); + + it("github deploy app role ListBucket is constrained to releases/*", () => { + const app = new cdk.App(); + const stack = new OpenSweIamStack(app, "OpenSweIamStack", { + stackName: "open-swe-iam", + env: ENV, + }); + Template.fromStack(stack).hasResourceProperties("AWS::IAM::Policy", { + PolicyDocument: Match.objectLike({ + Statement: Match.arrayWith([ + Match.objectLike({ + Sid: "ListArtifactBucket", + Action: "s3:ListBucket", + Condition: RELEASES_PREFIX_CONDITION, + }), + ]), + }), + }); + }); + + it("GetBucketLocation stays a separate, unconditioned statement", () => { + const app = new cdk.App(); + const stack = new OpenSweStack(app, "OpenSweDevStack", { + stackName: "open-swe-dev", + env: ENV, + envName: "dev", + }); + Template.fromStack(stack).hasResourceProperties("AWS::IAM::Policy", { + PolicyDocument: Match.objectLike({ + Statement: Match.arrayWith([ + Match.objectLike({ + Sid: "GetArtifactBucketLocation", + Action: "s3:GetBucketLocation", + Condition: Match.absent(), + }), + ]), + }), + }); + }); +}); diff --git a/tests/test_auth_error_leak.py b/tests/test_auth_error_leak.py new file mode 100644 index 00000000..33cb1cbc --- /dev/null +++ b/tests/test_auth_error_leak.py @@ -0,0 +1,81 @@ +"""AUTH-RESP-LEAK-01: upstream auth-error bodies must not reach user-facing text.""" + +from __future__ import annotations + +import asyncio +from typing import Any + +import httpx +import pytest + +from agent.utils import auth + +_SECRET_BODY = "SENSITIVE-UPSTREAM-BODY-9999" + + +class _Resp: + def __init__(self, status_code: int, text: str) -> None: + self.status_code = status_code + self.text = text + + def raise_for_status(self) -> None: + raise httpx.HTTPStatusError( + "boom", + request=httpx.Request("POST", "http://example.test"), + response=self, # type: ignore[arg-type] + ) + + def json(self) -> Any: + return {} + + +class _Client: + def __init__(self, resp: _Resp) -> None: + self._resp = resp + + async def __aenter__(self) -> _Client: + return self + + async def __aexit__(self, *_a: Any) -> None: + return None + + async def post(self, *_a: Any, **_k: Any) -> _Resp: + return self._resp + + +class _OkResp: + """A 2xx response whose JSON body lacks both a token and an auth url.""" + + def __init__(self, payload: Any) -> None: + self._payload = payload + + def raise_for_status(self) -> None: + return None + + def json(self) -> Any: + return self._payload + + +def test_http_error_returns_generic_message(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(auth, "GITHUB_OAUTH_PROVIDER_ID", "provider-id") + monkeypatch.setattr(auth, "X_SERVICE_AUTH_JWT_SECRET", "jwt-secret") + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _Client(_Resp(500, _SECRET_BODY))) + + result = asyncio.run(auth.get_github_token_for_user("ls-user", "tenant")) + + assert "error" in result + assert _SECRET_BODY not in result["error"] + assert "500" in result["error"] + + +def test_unexpected_result_returns_generic_message(monkeypatch: pytest.MonkeyPatch) -> None: + """The 2xx-but-missing-token/url branch must not echo the upstream body.""" + monkeypatch.setattr(auth, "GITHUB_OAUTH_PROVIDER_ID", "provider-id") + monkeypatch.setattr(auth, "X_SERVICE_AUTH_JWT_SECRET", "jwt-secret") + body = {"unexpected_field": _SECRET_BODY} + monkeypatch.setattr(httpx, "AsyncClient", lambda *a, **k: _Client(_OkResp(body))) + + result = asyncio.run(auth.get_github_token_for_user("ls-user", "tenant")) + + assert result == {"error": "GitHub auth returned an unexpected result"} + assert _SECRET_BODY not in result["error"] diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index eee94131..7971577d 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -1127,7 +1127,9 @@ def test_process_github_pr_comment_without_email_skips( def test_process_github_issue_uses_resolved_user_token_for_reaction(monkeypatch) -> None: captured: dict[str, object] = {} - async def fake_get_or_resolve_thread_github_token(thread_id: str, email: str) -> str | None: + async def fake_get_or_resolve_thread_github_token( + thread_id: str, email: str, *, repo: dict[str, str] | None = None + ) -> str | None: captured["thread_id"] = thread_id captured["email"] = email return "user-token" @@ -1210,7 +1212,9 @@ def test_process_github_issue_uses_resolved_user_token_for_reaction(monkeypatch) def test_process_github_issue_existing_thread_uses_followup_prompt(monkeypatch) -> None: captured: dict[str, object] = {} - async def fake_get_or_resolve_thread_github_token(thread_id: str, email: str) -> str | None: + async def fake_get_or_resolve_thread_github_token( + thread_id: str, email: str, *, repo: dict[str, str] | None = None + ) -> str | None: return "user-token" async def fake_get_github_app_installation_token() -> str | None: diff --git a/tests/test_github_token_ttl.py b/tests/test_github_token_ttl.py index bd3ee158..0d880814 100644 --- a/tests/test_github_token_ttl.py +++ b/tests/test_github_token_ttl.py @@ -69,7 +69,7 @@ def test_cached_token_expires_after_max_ttl() -> None: """A token with no/far expiry is still dropped once it's older than the 24h cap.""" far_future = (datetime.now(UTC) + timedelta(days=30)).isoformat() old_cached_at = datetime.now(UTC) - timedelta(hours=25) - github_token._GITHUB_TOKEN_CACHE["tid"] = ("ghp_secret", far_future, old_cached_at) + github_token._GITHUB_TOKEN_CACHE["tid"] = ("ghp_secret", far_future, old_cached_at, None) assert github_token.get_github_token({"configurable": {"thread_id": "tid"}}) is None @@ -210,7 +210,7 @@ def test_process_github_pr_comment_invalidates_and_reauths_on_401( tokens = iter(["stale-token", "fresh-token"]) - async def fake_get_or_resolve(thread_id: str, email: str) -> str | None: + async def fake_get_or_resolve(thread_id: str, email: str, *, repo: Any = None) -> str | None: token = next(tokens) resolves.append(token) return token diff --git a/tests/test_linear_webhook_replay.py b/tests/test_linear_webhook_replay.py new file mode 100644 index 00000000..c0e4295c --- /dev/null +++ b/tests/test_linear_webhook_replay.py @@ -0,0 +1,53 @@ +"""Replay-window enforcement for Linear webhook signature verification (AUTHZ-001).""" + +from __future__ import annotations + +import hashlib +import hmac +import json +from datetime import UTC, datetime + +from agent import webapp + +_SECRET = "linear-signing-secret" + + +def _sign(body: bytes) -> str: + return hmac.new(_SECRET.encode("utf-8"), body, hashlib.sha256).hexdigest() + + +def _now_ms() -> int: + return int(datetime.now(UTC).timestamp() * 1000) + + +def test_fresh_timestamp_accepted() -> None: + body = json.dumps({"type": "Comment", "webhookTimestamp": _now_ms()}).encode() + assert webapp.verify_linear_signature(body, _sign(body), _SECRET) is True + + +def test_stale_timestamp_rejected() -> None: + stale = _now_ms() - 10 * 60 * 1000 # 10 minutes old + body = json.dumps({"type": "Comment", "webhookTimestamp": stale}).encode() + # Signature is valid, but the timestamp is outside the freshness window. + assert webapp.verify_linear_signature(body, _sign(body), _SECRET) is False + + +def test_future_timestamp_rejected() -> None: + future = _now_ms() + 10 * 60 * 1000 + body = json.dumps({"type": "Comment", "webhookTimestamp": future}).encode() + assert webapp.verify_linear_signature(body, _sign(body), _SECRET) is False + + +def test_missing_timestamp_rejected() -> None: + body = json.dumps({"type": "Comment"}).encode() + assert webapp.verify_linear_signature(body, _sign(body), _SECRET) is False + + +def test_non_numeric_timestamp_rejected() -> None: + body = json.dumps({"type": "Comment", "webhookTimestamp": "not-a-number"}).encode() + assert webapp.verify_linear_signature(body, _sign(body), _SECRET) is False + + +def test_bad_signature_rejected_even_when_fresh() -> None: + body = json.dumps({"type": "Comment", "webhookTimestamp": _now_ms()}).encode() + assert webapp.verify_linear_signature(body, "deadbeef", _SECRET) is False diff --git a/tests/test_proxy_auth.py b/tests/test_proxy_auth.py index 163bd1a4..dfdfa903 100644 --- a/tests/test_proxy_auth.py +++ b/tests/test_proxy_auth.py @@ -264,6 +264,7 @@ class TestRefreshProxyOnSandboxReuse: mock_sandbox = MagicMock(id="sandbox-cached") with ( + patch("agent.server.client.threads.update", new_callable=AsyncMock), patch( "agent.server.resolve_github_token", new_callable=AsyncMock, @@ -308,11 +309,12 @@ class TestRefreshProxyOnSandboxReuse: @pytest.mark.asyncio async def test_refreshes_proxy_when_reconnecting_to_existing_langsmith_sandbox(self) -> None: - """Reconnected sandboxes should also get a fresh proxy token.""" + """A bound thread reconnecting to its sandbox should get a fresh proxy token.""" config = self._execution_config() mock_sandbox = MagicMock(id="sandbox-existing") with ( + patch("agent.server.client.threads.update", new_callable=AsyncMock), patch( "agent.server.resolve_github_token", new_callable=AsyncMock, @@ -323,6 +325,13 @@ class TestRefreshProxyOnSandboxReuse: new_callable=AsyncMock, return_value="sandbox-existing", ), + # Thread is bound to the current repo, so reconnect proceeds (an unbound + # legacy thread would instead fail closed and recreate). + patch( + "agent.server.get_bound_repo_from_metadata", + new_callable=AsyncMock, + return_value="langchain-ai/open-swe", + ), patch("agent.server.create_sandbox", return_value=mock_sandbox) as mock_create, patch( "agent.server.get_github_app_installation_token_with_expiry", diff --git a/tests/test_repo_binding_isolation.py b/tests/test_repo_binding_isolation.py new file mode 100644 index 00000000..6e9c94bd --- /dev/null +++ b/tests/test_repo_binding_isolation.py @@ -0,0 +1,225 @@ +"""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" diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 109fc90c..a381a0ce 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -221,7 +221,9 @@ async def test_reviewer_resolves_app_installation_token_at_run_start() -> None: # Token is resolved in this process at run start (scoped to the repo), not read # from a cache the webhook handler populated in a different process. mock_app_token.assert_awaited_once_with(repositories=["repo"]) - mock_cache_token.assert_called_once_with("reviewer-thread-id", "app-token", expires_at=None) + mock_cache_token.assert_called_once_with( + "reviewer-thread-id", "app-token", expires_at=None, repo={"owner": "acme", "name": "repo"} + ) middleware = create_agent.call_args.kwargs["middleware"] assert reviewer.check_message_queue_before_model in middleware