diff --git a/packages/terminal-bench/agent/omp_local.py b/packages/terminal-bench/agent/omp_local.py index b647221f1..ea73160cc 100644 --- a/packages/terminal-bench/agent/omp_local.py +++ b/packages/terminal-bench/agent/omp_local.py @@ -466,6 +466,10 @@ class OmpLocal(BaseInstalledAgent): parts.append(f"--thinking {shlex.quote(self._thinking)}") if self._extra_args: parts.append(self._extra_args) + # POSIX positional separator: some task prompts start with "-" (e.g. a + # markdown bullet, as in pytorch-model-recovery). Without this, omp parses + # the prompt as an unknown flag and exits 2. `--` forces positional mode. + parts.append("--") parts.append(shlex.quote(instruction)) # No pipes/stdbuf (absent in minimal images): redirect raw JSONL to the # mounted agent log dir; populate_context_post_run parses it on the host. diff --git a/python/robomp/src/proxy/server.py b/python/robomp/src/proxy/server.py index 96e7629db..794a18922 100644 --- a/python/robomp/src/proxy/server.py +++ b/python/robomp/src/proxy/server.py @@ -207,41 +207,68 @@ def _read_origin_url(repo_dir: Path, slot_uid: int | None = None) -> str: return proc.stdout.strip() -def _assert_origin_safe_for_repo(repo_dir: Path, expected_repo: str, slot_uid: int | None = None) -> None: - """Refuse the push if the worktree's `origin` would leak the PAT. +def _pat_safe_remote(url: str, expected_repo: str) -> bool: + """Whether git may carry the bot PAT to `url` for `expected_repo`. The PAT is injected via `--config-env http.extraHeader=…` (see `git_ops._run_git`); git ONLY forwards that header on HTTP(S) requests. So: - • If `origin` is HTTPS/HTTP, it MUST resolve to - `github.com/` exactly — anything else and we'd be + • An HTTP(S) remote MUST resolve to `github.com/` + exactly, with no embedded credentials — anything else and we'd be handing the bot's token to an attacker-controlled host. • Other schemes (ssh, file, git://, …) can't carry the PAT header, - so we let them through; the legitimate test path uses local file - remotes. - - Without this guard, an agent with shell access in the workspace could - `git remote set-url origin https://evil.example/x.git` and the proxy - would happily push (with the PAT) to that remote. + so they're safe by construction; the legitimate test path uses + local file remotes. """ - url = _read_origin_url(repo_dir, slot_uid=slot_uid) parsed = urlparse(url) scheme = (parsed.scheme or "").lower() if scheme not in ("http", "https"): - return # PAT header is never sent over non-http(s); safe by construction + return True + if parsed.username or parsed.password: + return False host = (parsed.hostname or "").lower() # Strip optional leading slash, trailing slash, and `.git` suffix. path = parsed.path.strip("/") if path.endswith(".git"): path = path[:-4] - if host != "github.com" or path.lower() != expected_repo.lower(): + return host == "github.com" and path.lower() == expected_repo.lower() + + +def _assert_origin_safe_for_repo(repo_dir: Path, expected_repo: str, slot_uid: int | None = None) -> None: + """Refuse a token-bearing git op if the repo's `origin` would leak the PAT. + + Without this guard, an agent with shell access in the workspace (or with + group write to the shared pool clone) could + `git remote set-url origin https://evil.example/x.git` and the proxy + would happily fetch/push (with the PAT) to that remote. + """ + url = _read_origin_url(repo_dir, slot_uid=slot_uid) + if not _pat_safe_remote(url, expected_repo): log.warning( - "gh-proxy: refusing push — origin does not match repo", - extra={"expected_repo": expected_repo, "origin_host": host}, + "gh-proxy: refusing git op — origin does not match repo", + extra={"expected_repo": expected_repo}, ) raise HTTPException( 400, - f"origin url does not match repo {expected_repo!r}; refusing to push", + f"origin url does not match repo {expected_repo!r}; refusing token-bearing git op", + ) + + +def _assert_clone_url_safe(clone_url: str, expected_repo: str) -> None: + """Refuse a clone whose caller-supplied `clone_url` would leak the PAT. + + The pool has no `origin` yet, so there is nothing on disk to validate — + guard the request body directly with the same policy as + `_assert_origin_safe_for_repo`. + """ + if not _pat_safe_remote(clone_url, expected_repo): + log.warning( + "gh-proxy: refusing clone — clone_url does not match repo", + extra={"expected_repo": expected_repo}, + ) + raise HTTPException( + 400, + f"clone_url does not match repo {expected_repo!r}; refusing to clone", ) @@ -610,6 +637,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: repo = _require_str(data.get("repo"), "repo") clone_url = _require_str(data.get("clone_url"), "clone_url") default_branch = _require_str(data.get("default_branch"), "default_branch") + _assert_clone_url_safe(clone_url, repo) target = _pool_dir(settings, repo) try: await _run_git_op( @@ -628,6 +656,9 @@ def create_proxy_app(settings: Settings) -> FastAPI: data = await _json_body(request) repo = _require_str(data.get("repo"), "repo") target = _pool_dir(settings, repo) + # Block attacker-controlled `origin` from being a PAT exfil channel + # before any subprocess injects the token header. + await asyncio.to_thread(_assert_origin_safe_for_repo, target, repo) try: await _run_git_op(git_fetch_prune, target, token=_resolve_token(settings)) except GitCommandError as exc: @@ -640,6 +671,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: repo = _require_str(data.get("repo"), "repo") ref = _require_str(data.get("ref"), "ref") target = _pool_dir(settings, repo) + await asyncio.to_thread(_assert_origin_safe_for_repo, target, repo) # fetch_ref is intentionally best-effort; never surfaces a 5xx. await _run_git_op(git_fetch_ref, target, ref, token=_resolve_token(settings)) return JSONResponse({"pool_dir": str(target)}) @@ -650,6 +682,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: repo = _require_str(data.get("repo"), "repo") pr_number = _require_int(data.get("pr_number"), "pr_number") target = _pool_dir(settings, repo) + await asyncio.to_thread(_assert_origin_safe_for_repo, target, repo) try: await _run_git_op(git_fetch_pr_head, target, pr_number, token=_resolve_token(settings)) except GitCommandError as exc: diff --git a/python/robomp/tests/test_proxy_server.py b/python/robomp/tests/test_proxy_server.py index 0b8fb2878..d4ba5665a 100644 --- a/python/robomp/tests/test_proxy_server.py +++ b/python/robomp/tests/test_proxy_server.py @@ -103,6 +103,14 @@ def _stage_workspace(cfg: Settings, upstream: Path, repo: str, number: int, bran return repo_dir, proc.stdout.strip() +def _stage_pool(cfg: Settings, upstream: Path, repo: str = "octo/widget") -> Path: + """Pre-stage the shared pool clone that the fetch endpoints operate on.""" + pool_dir = Path(cfg.workspace_root) / "_pool" / repo.replace("/", "__") + pool_dir.parent.mkdir(parents=True, exist_ok=True) + _git(["clone", "--filter=blob:none", str(upstream), str(pool_dir)], Path(cfg.workspace_root)) + return pool_dir + + def _bare_has_branch(bare: Path, branch: str) -> bool: proc = subprocess.run( ["git", "-C", str(bare), "branch", "--list", branch], @@ -1012,3 +1020,58 @@ async def test_git_push_rejects_origin_with_wrong_repo(proxy_settings: Settings, ) assert resp.status_code == 400, resp.text assert not _bare_has_branch(upstream_repo, branch) + + +# ============================================================================ +# Finding 6 — fetch + clone refuse attacker-controlled origin (PAT exfil guard) +# ============================================================================ + + +@pytest.mark.parametrize( + ("endpoint", "body"), + [ + ("/gh/v1/git/fetch", b'{"repo":"octo/widget"}'), + ("/gh/v1/git/fetch_ref", b'{"repo":"octo/widget","ref":"refs/heads/main"}'), + ("/gh/v1/git/fetch_pr_head", b'{"repo":"octo/widget","pr_number":1}'), + ], +) +async def test_git_fetch_endpoints_reject_attacker_origin( + proxy_settings: Settings, upstream_repo: Path, endpoint: str, body: bytes +) -> None: + """An agent with group write to the shared pool can rewrite its `origin`; + every token-bearing fetch MUST refuse with 400 before git carries the PAT + to that host.""" + pool_dir = _stage_pool(proxy_settings, upstream_repo) + _git(["-C", str(pool_dir), "remote", "set-url", "origin", "https://evil.example.com/octo/widget.git"], pool_dir) + + app = _build_app(proxy_settings) + async with await _async_client(app) as client: + resp = await client.post( + endpoint, + content=body, + headers={**_signed("POST", endpoint, body), "Content-Type": "application/json"}, + ) + assert resp.status_code == 400, resp.text + + +@pytest.mark.parametrize( + "clone_url", + [ + "https://evil.example.com/octo/widget.git", # wrong host + "https://github.com/attacker/other.git", # wrong repo + "https://user:pass@github.com/octo/widget.git", # embedded credentials + ], +) +async def test_git_clone_rejects_unsafe_url(proxy_settings: Settings, clone_url: str) -> None: + """`clone_url` is caller-supplied; an HTTP(S) URL that doesn't resolve to + github.com/ MUST be refused before git carries the PAT to it.""" + app = _build_app(proxy_settings) + body = b'{"repo":"octo/widget","clone_url":"' + clone_url.encode() + b'","default_branch":"main"}' + async with await _async_client(app) as client: + resp = await client.post( + "/gh/v1/git/clone", + content=body, + headers={**_signed("POST", "/gh/v1/git/clone", body), "Content-Type": "application/json"}, + ) + assert resp.status_code == 400, resp.text + assert not (Path(proxy_settings.workspace_root) / "_pool" / "octo__widget").exists()