mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-05 08:22:12 +00:00
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.
This commit is contained in:
parent
9444fd7677
commit
a33aaec495
18 changed files with 787 additions and 55 deletions
|
|
@ -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-<env>/`. 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-<env>/* (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.",
|
"suppression_justification": "ACCEPTED LOW residual, metadata-only. fetch-config.sh materializes the .env via `batch-get-secret-value --filters Key=name,Values=open-swe-<env>/`. 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-<env>/* (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",
|
"owner": "adam@seahavenind.com",
|
||||||
"added": "2026-06-26"
|
"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"
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1206,6 +1206,8 @@ async def send_dashboard_message(
|
||||||
raise HTTPException(404, "thread not found") from exc
|
raise HTTPException(404, "thread not found") from exc
|
||||||
|
|
||||||
metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {}
|
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)
|
_assert_thread_readable(metadata)
|
||||||
|
|
||||||
prompt = f"{_attribution_prefix(metadata, login, email)}{body.content.strip()}"
|
prompt = f"{_attribution_prefix(metadata, login, email)}{body.content.strip()}"
|
||||||
|
|
|
||||||
|
|
@ -59,6 +59,7 @@ from .server import (
|
||||||
DEFAULT_LLM_MAX_TOKENS,
|
DEFAULT_LLM_MAX_TOKENS,
|
||||||
DEFAULT_RECURSION_LIMIT,
|
DEFAULT_RECURSION_LIMIT,
|
||||||
MODEL_CALL_RECURSION_LIMIT,
|
MODEL_CALL_RECURSION_LIMIT,
|
||||||
|
SandboxRepoMismatchError,
|
||||||
_general_purpose_subagent,
|
_general_purpose_subagent,
|
||||||
ensure_sandbox_for_thread,
|
ensure_sandbox_for_thread,
|
||||||
graph_loaded_for_execution,
|
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}"
|
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 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_proxy_token = github_token
|
||||||
github_api_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")
|
if repo_config.get("owner") and repo_config.get("name")
|
||||||
else None
|
else None
|
||||||
)
|
)
|
||||||
sandbox_backend = await ensure_sandbox_for_thread(
|
try:
|
||||||
thread_id,
|
sandbox_backend = await ensure_sandbox_for_thread(
|
||||||
github_proxy_token=github_proxy_token,
|
thread_id,
|
||||||
github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None,
|
github_proxy_token=github_proxy_token,
|
||||||
repo=repo_for_snapshot,
|
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)
|
work_dir = await aresolve_sandbox_work_dir(sandbox_backend)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -98,6 +98,7 @@ from .utils.github_app import (
|
||||||
get_github_app_installation_token_with_expiry,
|
get_github_app_installation_token_with_expiry,
|
||||||
)
|
)
|
||||||
from .utils.github_proxy import record_proxy_token_expiry
|
from .utils.github_proxy import record_proxy_token_expiry
|
||||||
|
from .utils.github_token import repo_cache_key
|
||||||
from .utils.model import (
|
from .utils.model import (
|
||||||
DEFAULT_LLM_REASONING,
|
DEFAULT_LLM_REASONING,
|
||||||
ModelKwargs,
|
ModelKwargs,
|
||||||
|
|
@ -117,6 +118,7 @@ SANDBOX_POLL_INTERVAL = 1.0
|
||||||
|
|
||||||
from .utils.sandbox_state import (
|
from .utils.sandbox_state import (
|
||||||
SANDBOX_BACKENDS,
|
SANDBOX_BACKENDS,
|
||||||
|
get_bound_repo_from_metadata,
|
||||||
get_sandbox_id_from_metadata,
|
get_sandbox_id_from_metadata,
|
||||||
set_sandbox_backend,
|
set_sandbox_backend,
|
||||||
unwrap_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(
|
async def ensure_sandbox_for_thread(
|
||||||
thread_id: str,
|
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)
|
logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id)
|
||||||
sandbox_id = await _resolve_creating_sentinel(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:
|
if sandbox_backend:
|
||||||
logger.info("Using cached sandbox backend for thread %s", thread_id)
|
logger.info("Using cached sandbox backend for thread %s", thread_id)
|
||||||
original_sandbox_id = sandbox_backend.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, 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:
|
if sandbox_id != sandbox_backend.id:
|
||||||
await client.threads.update(
|
metadata_update["sandbox_id"] = sandbox_backend.id
|
||||||
thread_id=thread_id, metadata={"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
|
# Re-apply git identity every run: cached/reconnected sandboxes may have
|
||||||
# lost their `--global` config (or had it overwritten), and Vercel preview
|
# 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"))
|
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
|
profile_task = asyncio.create_task(load_profile(profile_login)) if profile_login else None
|
||||||
triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather(
|
try:
|
||||||
triggering_user_identity_task,
|
triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather(
|
||||||
sandbox_task,
|
triggering_user_identity_task,
|
||||||
team_defaults_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
|
profile = await profile_task if profile_task is not None else None
|
||||||
del github_token
|
del github_token
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -192,11 +192,18 @@ async def get_github_token_for_user(ls_user_id: str, tenant_id: str) -> dict[str
|
||||||
return result
|
return result
|
||||||
if auth_url:
|
if auth_url:
|
||||||
return {"auth_url": 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:
|
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)
|
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
|
except Exception as e: # noqa: BLE001
|
||||||
logger.error("GitHub auth API call failed: %s: %s", type(e).__name__, str(e))
|
logger.error("GitHub auth API call failed: %s: %s", type(e).__name__, str(e))
|
||||||
return {"error": str(e)}
|
return {"error": str(e)}
|
||||||
|
|
@ -286,10 +293,19 @@ async def leave_failure_comment(
|
||||||
raise ValueError(f"Unknown source: {source}")
|
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(
|
def _cache_resolved_github_token(
|
||||||
thread_id: str, token: str, expires_at: str | None = None
|
thread_id: str, token: str, expires_at: str | None = None
|
||||||
) -> tuple[str, str | 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
|
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]:
|
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()
|
bot_token, expires_at = await get_github_app_installation_token_with_expiry()
|
||||||
if not bot_token:
|
if not bot_token:
|
||||||
raise RuntimeError(
|
raise RuntimeError(
|
||||||
|
|
@ -444,7 +466,9 @@ async def resolve_github_token(config: RunnableConfig, thread_id: str) -> tuple[
|
||||||
|
|
||||||
try:
|
try:
|
||||||
if source == "github":
|
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:
|
if cached_token:
|
||||||
return cached_token, cached_expires_at
|
return cached_token, cached_expires_at
|
||||||
from ..dashboard.user_mappings import email_for_login
|
from ..dashboard.user_mappings import email_for_login
|
||||||
|
|
|
||||||
|
|
@ -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
|
# 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.
|
# expiry, so entries for threads that are never read again don't accumulate.
|
||||||
_GITHUB_TOKEN_MAX_TTL = timedelta(hours=24)
|
_GITHUB_TOKEN_MAX_TTL = timedelta(hours=24)
|
||||||
# thread_id -> (token, token_expires_at, cached_at)
|
# thread_id -> (token, token_expires_at, cached_at, bound_repo). ``bound_repo``
|
||||||
_GITHUB_TOKEN_CACHE: dict[str, tuple[str, str | None, datetime]] = {}
|
# ("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):
|
class GitHubAuthError(Exception):
|
||||||
"""Raised when a GitHub call returns 401, signalling a stale/revoked token."""
|
"""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(
|
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:
|
) -> None:
|
||||||
"""Cache a GitHub token in process for the current thread."""
|
"""Cache a GitHub token in process for the current thread."""
|
||||||
if not thread_id or not token:
|
if not thread_id or not token:
|
||||||
return
|
return
|
||||||
now = datetime.now(UTC)
|
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)
|
_evict_expired(now=now)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -78,24 +98,37 @@ def _evict_expired(*, now: datetime | None = None) -> None:
|
||||||
current = now or datetime.now(UTC)
|
current = now or datetime.now(UTC)
|
||||||
stale = [
|
stale = [
|
||||||
tid
|
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)
|
if _entry_expired(expires_at, cached_at, now=current)
|
||||||
]
|
]
|
||||||
for tid in stale:
|
for tid in stale:
|
||||||
_GITHUB_TOKEN_CACHE.pop(tid, None)
|
_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:
|
if not thread_id:
|
||||||
return None, None
|
return None, None
|
||||||
cached = _GITHUB_TOKEN_CACHE.get(thread_id)
|
cached = _GITHUB_TOKEN_CACHE.get(thread_id)
|
||||||
if not cached:
|
if not cached:
|
||||||
return None, None
|
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)):
|
if _entry_expired(expires_at, cached_at, now=datetime.now(UTC)):
|
||||||
_GITHUB_TOKEN_CACHE.pop(thread_id, None)
|
_GITHUB_TOKEN_CACHE.pop(thread_id, None)
|
||||||
logger.info("Cached GitHub token for thread %s has expired; re-resolving", thread_id)
|
logger.info("Cached GitHub token for thread %s has expired; re-resolving", thread_id)
|
||||||
return None, None
|
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
|
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
|
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:
|
def get_github_token(run_config: Mapping[str, Any] | None = None) -> str | None:
|
||||||
"""Resolve the current thread's GitHub token from process memory."""
|
"""Resolve the current thread's GitHub token from process memory."""
|
||||||
resolved = run_config if run_config is not None else get_config()
|
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
|
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."""
|
"""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:
|
async def invalidate_cached_github_token(thread_id: str) -> None:
|
||||||
|
|
|
||||||
|
|
@ -19,6 +19,7 @@ from deepagents.backends.protocol import (
|
||||||
)
|
)
|
||||||
from langgraph.config import get_config
|
from langgraph.config import get_config
|
||||||
|
|
||||||
|
from .github_token import repo_cache_key
|
||||||
from .sandbox import create_sandbox
|
from .sandbox import create_sandbox
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
@ -29,6 +30,9 @@ class SandboxBackendProxy(SandboxBackendProtocol):
|
||||||
|
|
||||||
def __init__(self, backend: SandboxBackendProtocol) -> None:
|
def __init__(self, backend: SandboxBackendProtocol) -> None:
|
||||||
self._backend = backend
|
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
|
@property
|
||||||
def current(self) -> SandboxBackendProtocol:
|
def current(self) -> SandboxBackendProtocol:
|
||||||
|
|
@ -131,21 +135,44 @@ def unwrap_sandbox_backend(sandbox_backend: SandboxBackendProtocol) -> SandboxBa
|
||||||
def set_sandbox_backend(
|
def set_sandbox_backend(
|
||||||
thread_id: str,
|
thread_id: str,
|
||||||
sandbox_backend: SandboxBackendProtocol,
|
sandbox_backend: SandboxBackendProtocol,
|
||||||
|
*,
|
||||||
|
repo: str | None = None,
|
||||||
) -> SandboxBackendProxy:
|
) -> SandboxBackendProxy:
|
||||||
if isinstance(sandbox_backend, SandboxBackendProxy):
|
if isinstance(sandbox_backend, SandboxBackendProxy):
|
||||||
|
if repo:
|
||||||
|
sandbox_backend.bound_repo = repo
|
||||||
SANDBOX_BACKENDS[thread_id] = sandbox_backend
|
SANDBOX_BACKENDS[thread_id] = sandbox_backend
|
||||||
return sandbox_backend
|
return sandbox_backend
|
||||||
|
|
||||||
existing = SANDBOX_BACKENDS.get(thread_id)
|
existing = SANDBOX_BACKENDS.get(thread_id)
|
||||||
if isinstance(existing, SandboxBackendProxy):
|
if isinstance(existing, SandboxBackendProxy):
|
||||||
existing.replace_backend(sandbox_backend)
|
existing.replace_backend(sandbox_backend)
|
||||||
|
if repo:
|
||||||
|
existing.bound_repo = repo
|
||||||
return existing
|
return existing
|
||||||
|
|
||||||
proxy = SandboxBackendProxy(sandbox_backend)
|
proxy = SandboxBackendProxy(sandbox_backend)
|
||||||
|
if repo:
|
||||||
|
proxy.bound_repo = repo
|
||||||
SANDBOX_BACKENDS[thread_id] = proxy
|
SANDBOX_BACKENDS[thread_id] = proxy
|
||||||
return 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:
|
def clear_sandbox_backend(thread_id: str) -> None:
|
||||||
SANDBOX_BACKENDS.pop(thread_id, None)
|
SANDBOX_BACKENDS.pop(thread_id, None)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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:
|
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:
|
Args:
|
||||||
body: Raw request body bytes
|
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
|
secret: The webhook signing secret
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
True if signature is valid, False otherwise
|
True if the signature is valid AND the signed timestamp is fresh.
|
||||||
"""
|
"""
|
||||||
if not secret:
|
if not secret:
|
||||||
logger.warning("LINEAR_WEBHOOK_SECRET is not configured — rejecting webhook request")
|
logger.warning("LINEAR_WEBHOOK_SECRET is not configured — rejecting webhook request")
|
||||||
return False
|
return False
|
||||||
|
|
||||||
expected = hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest()
|
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")
|
@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."""
|
"""Invalidate the cached token after a 401 and try to resolve a fresh one."""
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"GitHub returned 401 for thread %s; invalidating cached token and re-resolving",
|
"GitHub returned 401 for thread %s; invalidating cached token and re-resolving",
|
||||||
thread_id,
|
thread_id,
|
||||||
)
|
)
|
||||||
await invalidate_cached_github_token(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.
|
"""Resolve and cache a GitHub token for a thread when available.
|
||||||
|
|
||||||
In bot-token-only mode, returns a fresh GitHub App installation token
|
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():
|
if is_bot_token_only_mode():
|
||||||
bot_token, expires_at = await get_github_app_installation_token_with_expiry()
|
bot_token, expires_at = await get_github_app_installation_token_with_expiry()
|
||||||
if bot_token:
|
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
|
return bot_token
|
||||||
logger.warning("Bot-token-only mode but GitHub App token unavailable")
|
logger.warning("Bot-token-only mode but GitHub App token unavailable")
|
||||||
return None
|
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:
|
if github_token:
|
||||||
return 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")
|
expires_at = auth_result.get("expires_at")
|
||||||
cache_github_token_for_thread(
|
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
|
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 ""
|
email = await email_for_login(github_login) or ""
|
||||||
if email:
|
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:
|
else:
|
||||||
logger.warning("No email mapping for GitHub user '%s', skipping", github_login)
|
logger.warning("No email mapping for GitHub user '%s', skipping", github_login)
|
||||||
return
|
return
|
||||||
|
|
@ -3037,7 +3071,9 @@ async def process_github_pr_comment(payload: dict[str, Any], event_type: str) ->
|
||||||
node_id=node_id,
|
node_id=node_id,
|
||||||
)
|
)
|
||||||
except GitHubAuthError:
|
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:
|
if not github_token:
|
||||||
logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id)
|
logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id)
|
||||||
return
|
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
|
repo_config, pr_number, token=github_token
|
||||||
)
|
)
|
||||||
except GitHubAuthError:
|
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:
|
if not github_token:
|
||||||
logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id)
|
logger.warning("Re-auth failed for thread %s after 401; skipping", thread_id)
|
||||||
return
|
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)
|
thread_id = generate_thread_id_from_github_issue(issue_id)
|
||||||
existing_thread = await _thread_exists(thread_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()
|
app_token = await get_github_app_installation_token()
|
||||||
reaction_token = github_token or app_token
|
reaction_token = github_token or app_token
|
||||||
comment = payload.get("comment", {})
|
comment = payload.get("comment", {})
|
||||||
|
|
@ -3314,7 +3352,9 @@ async def process_github_issue(payload: dict[str, Any], event_type: str) -> None
|
||||||
token=reaction_token,
|
token=reaction_token,
|
||||||
)
|
)
|
||||||
except GitHubAuthError:
|
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
|
reaction_token = github_token or app_token
|
||||||
reacted = False
|
reacted = False
|
||||||
if reaction_token:
|
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
|
repo_config, issue_number, token=github_token or app_token
|
||||||
)
|
)
|
||||||
except GitHubAuthError:
|
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(
|
comments = await fetch_issue_comments(
|
||||||
repo_config, issue_number, token=github_token or app_token
|
repo_config, issue_number, token=github_token or app_token
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -150,10 +150,22 @@ export class GithubDeployRoles extends Construct {
|
||||||
resources: [`arn:aws:s3:::open-swe-${envName}-assets/releases/*`],
|
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(
|
this.appRole.addToPolicy(
|
||||||
new iam.PolicyStatement({
|
new iam.PolicyStatement({
|
||||||
sid: "ListArtifactBucket",
|
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`],
|
resources: [`arn:aws:s3:::open-swe-${envName}-assets`],
|
||||||
}),
|
}),
|
||||||
);
|
);
|
||||||
|
|
|
||||||
|
|
@ -43,10 +43,23 @@ export class InstanceRole extends Construct {
|
||||||
resources: [`arn:aws:s3:::${p}-assets/releases/*`],
|
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(
|
this.role.addToPolicy(
|
||||||
new iam.PolicyStatement({
|
new iam.PolicyStatement({
|
||||||
sid: "ListArtifactBucket",
|
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`],
|
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
|
// 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
|
// names, no name filter), so `secretsmanager:ListSecrets` is NOT needed and is
|
||||||
// intentionally omitted — the box cannot enumerate secret names account-wide.
|
// 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(
|
this.role.addToPolicy(
|
||||||
new iam.PolicyStatement({
|
new iam.PolicyStatement({
|
||||||
sid: "BatchGetSecretValues",
|
sid: "BatchGetSecretValues",
|
||||||
|
|
|
||||||
71
infra/test/s3-list-prefix.test.ts
Normal file
71
infra/test/s3-list-prefix.test.ts
Normal file
|
|
@ -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(),
|
||||||
|
}),
|
||||||
|
]),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
81
tests/test_auth_error_leak.py
Normal file
81
tests/test_auth_error_leak.py
Normal file
|
|
@ -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"]
|
||||||
|
|
@ -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:
|
def test_process_github_issue_uses_resolved_user_token_for_reaction(monkeypatch) -> None:
|
||||||
captured: dict[str, object] = {}
|
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["thread_id"] = thread_id
|
||||||
captured["email"] = email
|
captured["email"] = email
|
||||||
return "user-token"
|
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:
|
def test_process_github_issue_existing_thread_uses_followup_prompt(monkeypatch) -> None:
|
||||||
captured: dict[str, object] = {}
|
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"
|
return "user-token"
|
||||||
|
|
||||||
async def fake_get_github_app_installation_token() -> str | None:
|
async def fake_get_github_app_installation_token() -> str | None:
|
||||||
|
|
|
||||||
|
|
@ -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."""
|
"""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()
|
far_future = (datetime.now(UTC) + timedelta(days=30)).isoformat()
|
||||||
old_cached_at = datetime.now(UTC) - timedelta(hours=25)
|
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
|
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"])
|
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)
|
token = next(tokens)
|
||||||
resolves.append(token)
|
resolves.append(token)
|
||||||
return token
|
return token
|
||||||
|
|
|
||||||
53
tests/test_linear_webhook_replay.py
Normal file
53
tests/test_linear_webhook_replay.py
Normal file
|
|
@ -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
|
||||||
|
|
@ -264,6 +264,7 @@ class TestRefreshProxyOnSandboxReuse:
|
||||||
mock_sandbox = MagicMock(id="sandbox-cached")
|
mock_sandbox = MagicMock(id="sandbox-cached")
|
||||||
|
|
||||||
with (
|
with (
|
||||||
|
patch("agent.server.client.threads.update", new_callable=AsyncMock),
|
||||||
patch(
|
patch(
|
||||||
"agent.server.resolve_github_token",
|
"agent.server.resolve_github_token",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -308,11 +309,12 @@ class TestRefreshProxyOnSandboxReuse:
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_refreshes_proxy_when_reconnecting_to_existing_langsmith_sandbox(self) -> None:
|
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()
|
config = self._execution_config()
|
||||||
mock_sandbox = MagicMock(id="sandbox-existing")
|
mock_sandbox = MagicMock(id="sandbox-existing")
|
||||||
|
|
||||||
with (
|
with (
|
||||||
|
patch("agent.server.client.threads.update", new_callable=AsyncMock),
|
||||||
patch(
|
patch(
|
||||||
"agent.server.resolve_github_token",
|
"agent.server.resolve_github_token",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -323,6 +325,13 @@ class TestRefreshProxyOnSandboxReuse:
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value="sandbox-existing",
|
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.create_sandbox", return_value=mock_sandbox) as mock_create,
|
||||||
patch(
|
patch(
|
||||||
"agent.server.get_github_app_installation_token_with_expiry",
|
"agent.server.get_github_app_installation_token_with_expiry",
|
||||||
|
|
|
||||||
225
tests/test_repo_binding_isolation.py
Normal file
225
tests/test_repo_binding_isolation.py
Normal file
|
|
@ -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"
|
||||||
|
|
@ -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
|
# 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.
|
# from a cache the webhook handler populated in a different process.
|
||||||
mock_app_token.assert_awaited_once_with(repositories=["repo"])
|
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"]
|
middleware = create_agent.call_args.kwargs["middleware"]
|
||||||
assert reviewer.check_message_queue_before_model in middleware
|
assert reviewer.check_message_queue_before_model in middleware
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue