diff --git a/.github/workflows/ci-macos-sandbox.yml b/.github/workflows/ci-macos-sandbox.yml new file mode 100644 index 00000000..c587d31a --- /dev/null +++ b/.github/workflows/ci-macos-sandbox.yml @@ -0,0 +1,93 @@ +name: CI / macOS sandbox + +on: + push: + branches: [main] + pull_request: + branches: [main] + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + seatbelt: + name: Seatbelt - ${{ matrix.toolchain }} + runs-on: macos-15 + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + toolchain: [clt, xcode] + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Configure selected developer tools + shell: bash + env: + TOOLCHAIN: ${{ matrix.toolchain }} + run: | + set -euo pipefail + if [[ "$TOOLCHAIN" == xcode ]]; then + # Hosted images commonly use a symlink named Xcode.app. Test the + # real standard installation supported by the sandbox's contract. + selected=$(/usr/bin/env -u DEVELOPER_DIR /usr/bin/xcode-select -p) + app=$(python3 -c 'import os, sys; print(os.path.realpath(sys.argv[1]))' "${selected%/Contents/Developer}") + [[ "$app" == /Applications/*.app ]] + [[ -x "$app/Contents/Developer/usr/bin/git" ]] + if [[ -L /Applications/Xcode.app ]]; then + sudo unlink /Applications/Xcode.app + fi + if [[ "$app" != /Applications/Xcode.app ]]; then + # Fail rather than overwrite any existing real installation. + [[ ! -e /Applications/Xcode.app ]] + sudo mv "$app" /Applications/Xcode.app + fi + developer=/Applications/Xcode.app/Contents/Developer + [[ -f /Applications/Xcode.app/Contents/Frameworks/libxcodebuildLoader.dylib ]] + [[ -f /Applications/Xcode.app/Contents/version.plist ]] + else + developer=/Library/Developer/CommandLineTools + fi + sudo /usr/bin/xcode-select --switch "$developer" + echo "DEVELOPER_DIR=$developer" >> "$GITHUB_ENV" + /usr/bin/env -i /usr/bin/xcode-select -p + /usr/bin/env -i /usr/bin/git --version + "$developer/usr/bin/git" --version + # Require real Seatbelt and a GUI launchd domain for process cleanup + # tests; missing infrastructure must fail this job, not skip tests. + /usr/bin/sandbox-exec -p '(version 1)(allow default)' /usr/bin/true + /bin/launchctl print "gui/$(id -u)" > /dev/null + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.13' + cache: 'pip' + + - name: Install dependencies + run: | + python -m pip install --upgrade pip setuptools wheel + python -m pip install -e reme_studio -e '.[dev,as]' + python -m pip install 'safecmd>=0.1.16,<0.2' shfmt-py + python -m pip install --no-deps -e plugins/home + + - name: Run sandbox and executor regressions + env: + FASTMCP_MCP_CAMELCASE_COMPAT: 'false' + run: | + python -m pytest \ + tests/unit/test_file_sandbox_backend.py \ + tests/unit/test_sandbox_startup_cancellation.py \ + tests/unit/test_local_backend.py \ + tests/unit/test_path_bubble_backend.py \ + plugins/home/tests/test_auto_bash.py \ + tests/integration/test_seatbelt_backend.py \ + plugins/home/tests/test_auto_bash_seatbelt.py \ + -v --tb=long --junitxml=macos-sandbox-results.xml diff --git a/benchmark/beam/README.md b/benchmark/beam/README.md index 287616b0..e8425956 100644 --- a/benchmark/beam/README.md +++ b/benchmark/beam/README.md @@ -31,6 +31,13 @@ Custom application config paths still work through `reme.config` and can use `ex This directory continues to own the runner, evaluation settings, dataset and outputs. Model credentials use the environment variables declared by the shared benchmark configuration. +The shared preset keeps `python_execute` private (`enable_serve: false`); in-process benchmark jobs +remain callable. Python always uses a sandbox with `workspace_dir` read-only by default, no network, +and a minimal environment. Linux requires Bubblewrap and, for the default memory cap, a writable +delegated cgroup v2; macOS requires Seatbelt. Unavailable isolation fails the call. Custom path grants +must be configured on the Python Step, not passed as job arguments. Read-only workspace files, +including any `.env` files, remain readable: keep credentials outside the workspace. + ## 1. Get the Dataset BEAM is a public repository, cloned into `benchmark/beam/dataset/`: diff --git a/benchmark/longmemeval/README.md b/benchmark/longmemeval/README.md index 09185db7..7ddda3cb 100644 --- a/benchmark/longmemeval/README.md +++ b/benchmark/longmemeval/README.md @@ -28,6 +28,10 @@ Custom application config paths still work through `reme.config` and can use `ex This directory continues to own the runner, evaluation settings, dataset and outputs. Model credentials use the environment variables declared by the shared benchmark configuration. +The shared preset keeps `python_execute` private (`enable_serve: false`); in-process benchmark jobs +remain callable. Python sandbox requirements and credential placement are described in the +[BEAM benchmark guide](../beam/README.md). + ## 1. Get the Dataset ReMe uses only the **cleaned-S** split, hosted on HuggingFace: diff --git a/plugins/home/README.md b/plugins/home/README.md index c3470af3..b76f07f1 100644 --- a/plugins/home/README.md +++ b/plugins/home/README.md @@ -9,16 +9,33 @@ reme plugins install plugins/home ``` Enable it in an application config with `plugins: [home]`. The plugin then -contributes the `auto_bash` job, which can be run with -`reme start config= job=auto_bash command=pwd`. +contributes the `auto_bash` job. Start `reme start config=` +in one terminal, then call `reme auto_bash command=pwd` from another. -The job defaults to read-only command validation and a 60-second timeout. Set -`is_read_only=false` to disable command validation. Supplying `read_only_paths` -or `writable_paths` enables a file sandbox (Bubblewrap on Linux, Seatbelt on -macOS), with sandbox paths identical to their host paths. On Linux, the -`memory_limit_mib` cgroup limit defaults to 4096 MiB; 0 disables it. Supplying -the limit requires a path policy. macOS has no memory limit and warns when a -nonzero value is supplied. +The job always uses a file sandbox (Bubblewrap on Linux, Seatbelt on macOS), +with `workspace_dir` read-only by default, network access blocked, and a minimal +child environment. Missing or unavailable isolation fails without host execution. +Files inside the workspace remain readable, including any `.env` files placed there. +Keep credentials outside the workspace. + +The job also defaults to read-only command validation and a 60-second timeout. +`is_read_only=false` disables command validation while keeping the sandbox active. +Configure `read_only_paths` and `writable_paths` only on the Step; request-level +path grants are rejected. Existing path arguments must move to Step configuration. +For example, a trusted Step can grant workspace writes: + +```yaml +steps: + - backend: auto_bash_step + read_only_paths: [] + writable_paths: ["/absolute/path/to/workspace"] +``` + +On Linux, Bubblewrap and a writable delegated cgroup v2 are required. The +`memory_limit_mib` cgroup limit defaults to 4096 MiB; 0 disables it. macOS has +no memory limit and warns when a nonzero value is supplied. Direct `AutoBash` +instances also default to a sandbox with their working directory read-only; +trusted callers may inject a different backend. Standard output and standard error each have a 1 MiB (1,048,576-byte) limit on all backends. Exceeding either limit terminates the command and returns a diff --git a/plugins/home/pyproject.toml b/plugins/home/pyproject.toml index 77f8c174..0da629b4 100644 --- a/plugins/home/pyproject.toml +++ b/plugins/home/pyproject.toml @@ -5,7 +5,7 @@ description = "Local command execution jobs for ReMe." readme = "README.md" requires-python = ">=3.11" dependencies = [ - "reme-ai[as]>=0.4.2.1", + "reme-ai[as]>=0.4.2.2", "safecmd>=0.1.16,<0.2", ] diff --git a/plugins/home/src/reme_home/auto_bash.py b/plugins/home/src/reme_home/auto_bash.py index 0f4a2d25..3f03d452 100644 --- a/plugins/home/src/reme_home/auto_bash.py +++ b/plugins/home/src/reme_home/auto_bash.py @@ -4,6 +4,7 @@ import asyncio import os import shlex from functools import cache +from pathlib import Path from agentscope.message import TextBlock, ToolResultState from agentscope.permission import PermissionBehavior, PermissionContext, PermissionDecision @@ -12,7 +13,7 @@ from agentscope.tool import Bash, ToolChunk from reme.steps.base_step import BaseStep from reme.steps.common.file_sandbox import file_sandbox_for_step -from reme.utils.local_backend import LocalProcessBackend +from reme.utils.file_sandbox import create_file_sandbox_backend _READ_COMMANDS = { ":": {}, @@ -153,7 +154,7 @@ class AutoBash(Bash): Shell expansions, function definitions and for loops are rejected because their runtime arguments or environment can differ from the parsed tokens. Git external filters require unrestricted mode; other Git helpers are - disabled. Use path injection for OS file isolation. + disabled. The default backend isolates the working directory read-only. """ name = "AutoBash" @@ -172,7 +173,13 @@ class AutoBash(Bash): def __init__(self, *, is_read_only: bool = True, **kwargs): if kwargs.get("backend") is None: - kwargs["backend"] = LocalProcessBackend() + workspace = Path(kwargs.get("cwd") or Path.cwd()).expanduser().resolve() + kwargs["cwd"] = workspace + kwargs["backend"] = create_file_sandbox_backend( + cwd=workspace, + read_only_paths=[], + writable_paths=[], + ) super().__init__(**kwargs) self.is_read_only = is_read_only @@ -247,7 +254,7 @@ class AutoBash(Bash): class AutoBashStep(BaseStep): - """Expose AgentScope Bash with configurable read-only mode as a ReMe job.""" + """Run sandboxed Bash with a read-only workspace and optional command validation.""" async def execute(self): """Run the command with the selected permission policy and backend.""" @@ -263,7 +270,7 @@ class AutoBashStep(BaseStep): if not isinstance(is_read_only, bool): raise ValueError("is_read_only must be a boolean") backend = file_sandbox_for_step(self) - except (TypeError, ValueError, RuntimeError) as exc: + except (TypeError, ValueError, OSError, RuntimeError) as exc: self.context.response.success = False self.context.response.answer = str(exc) return self.context.response diff --git a/plugins/home/src/reme_home/plugin.yaml b/plugins/home/src/reme_home/plugin.yaml index 77ad1584..5ef67f39 100644 --- a/plugins/home/src/reme_home/plugin.yaml +++ b/plugins/home/src/reme_home/plugin.yaml @@ -5,7 +5,7 @@ application_defaults: jobs: auto_bash: backend: base - description: "Run a shell command in the workspace; read-only mode is enabled by default" + description: "Run a sandboxed shell command with a read-only workspace and command validation by default" parameters: type: object properties: @@ -22,18 +22,10 @@ application_defaults: default: 60000 minimum: 1 maximum: 600000 - read_only_paths: - type: array - items: { type: string } - description: "Read-only host paths; providing either path list enables the platform file sandbox" - writable_paths: - type: array - items: { type: string } - description: "Writable host paths; sandbox paths match host paths" memory_limit_mib: type: integer minimum: 0 - description: "Linux cgroup memory limit in MiB (default 4096; 0 disables it); requires a path policy; macOS warns and ignores nonzero values" + description: "Linux cgroup memory limit in MiB (default 4096; 0 disables it); macOS warns and ignores nonzero values" required: - command steps: diff --git a/plugins/home/tests/test_auto_bash.py b/plugins/home/tests/test_auto_bash.py index c8f7ddfc..a10f518d 100644 --- a/plugins/home/tests/test_auto_bash.py +++ b/plugins/home/tests/test_auto_bash.py @@ -19,6 +19,7 @@ from reme_home.auto_bash import AutoBash, AutoBashStep from reme.utils.local_backend import LocalProcessBackend from reme.utils.file_sandbox.path_bubble_backend import PathBubbleBackend +from reme.utils.file_sandbox.path_seatbelt_backend import PathSeatbeltBackend from reme.components.application_context import ApplicationContext from reme.config.config_parser import _load_config from reme.enumeration import ComponentEnum @@ -31,6 +32,16 @@ def linux_backend_selection(monkeypatch): monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "linux") +@pytest.fixture(autouse=True) +def sandbox_process_boundary(monkeypatch): + """Use real local pipes for command tests; policy tests inspect the selected sandbox.""" + + async def local_exec(_self, command, *, cwd=None, timeout=None): + return await LocalProcessBackend().exec_shell(command, cwd=cwd, timeout=timeout) + + monkeypatch.setattr(PathBubbleBackend, "exec_shell", local_exec) + + @pytest.fixture(autouse=True) def isolated_git_config(tmp_path, monkeypatch): """Keep Git tests independent of the developer's global and system config.""" @@ -38,6 +49,29 @@ def isolated_git_config(tmp_path, monkeypatch): monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") +@pytest.mark.asyncio +@pytest.mark.parametrize("platform", ["linux", "darwin"]) +async def test_direct_auto_bash_defaults_to_read_only_sandbox(tmp_path, monkeypatch, platform): + """Direct tool construction has the same default grants as the job.""" + monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", platform) + tool = AutoBash(cwd=tmp_path, is_read_only=False) + backend = tool._backend # pylint: disable=protected-access + if platform == "linux": + assert isinstance(backend, PathBubbleBackend) + argv = backend._bwrap_argv(["/bin/sh", "-c", "echo ok"], str(tmp_path)) # pylint: disable=protected-access + assert argv[argv.index(str(tmp_path)) - 1] == "--ro-bind" + assert "--bind" not in argv + else: + assert isinstance(backend, PathSeatbeltBackend) + profile = backend._profile(str(tmp_path / "temporary")) # pylint: disable=protected-access + assert f'(allow file-read* (subpath "{tmp_path}"))' in profile + assert f'(allow file-write* (subpath "{tmp_path}"))' not in profile + + monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "unsupported") + with pytest.raises(RuntimeError, match="No file sandbox backend"): + AutoBash(cwd=tmp_path, is_read_only=False) + + @pytest.mark.asyncio async def test_auto_bash_runs_in_workspace(tmp_path): """Approved commands run relative to the configured workspace.""" @@ -212,8 +246,12 @@ async def test_step_returns_permission_error(tmp_path): @pytest.mark.asyncio async def test_step_can_run_unrestricted_commands(tmp_path): - """Job input can disable the guard for commands that write files.""" - step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) + """Trusted Step grants allow writes when command validation is disabled.""" + step = AutoBashStep( + app_context=ApplicationContext(workspace_dir=str(tmp_path)), + read_only_paths=[], + writable_paths=[str(tmp_path)], + ) response = await step(command="echo hello > written.txt", is_read_only=False) @@ -222,34 +260,66 @@ async def test_step_can_run_unrestricted_commands(tmp_path): @pytest.mark.asyncio -async def test_unsupported_platform_runs_without_paths_but_rejects_path_policy(tmp_path, monkeypatch): - """AutoBash never drops an explicitly requested sandbox on unsupported systems.""" +async def test_unsupported_platform_never_runs_on_host(tmp_path, monkeypatch): + """AutoBash requires isolation even without an explicit path policy.""" monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "unsupported") step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - plain = await step(command="echo host", is_read_only=False) - assert plain.success is True - assert plain.answer.strip() == "host" - - restricted = await step(command="echo blocked", is_read_only=False, read_only_paths=[]) - assert restricted.success is False - assert restricted.answer == "No file sandbox backend for unsupported" + result = await step(command="touch host-marker", is_read_only=False) + assert result.success is False + assert result.answer == "No file sandbox backend for unsupported" + assert not (tmp_path / "host-marker").exists() @pytest.mark.asyncio -@pytest.mark.parametrize("limit", [0, 1, -1]) -async def test_step_rejects_memory_limit_without_path_policy(tmp_path, limit): - """An explicitly supplied limit cannot be ignored by host execution.""" +@pytest.mark.parametrize("limit", [None, 0, 1, -1]) +@pytest.mark.parametrize("is_read_only", [False, True]) +async def test_step_defaults_to_read_only_workspace(tmp_path, monkeypatch, limit, is_read_only): + """Sandbox grants and limits apply independently of command validation.""" + seen = [] + + async def inspect_exec(backend, command, *, cwd=None, timeout=None): + assert timeout == 60 + seen.append(backend._bwrap_argv(command, cwd)) # pylint: disable=protected-access + assert backend._memory_limit_mib == (4096 if limit is None else limit) # pylint: disable=protected-access + return ExecResult(exit_code=0, stdout=b"ok", stderr=b"") + + monkeypatch.setattr(PathBubbleBackend, "exec_shell", inspect_exec) step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(command="echo should-not-run", is_read_only=False, memory_limit_mib=limit) + response = await step(command="echo ok", is_read_only=is_read_only, memory_limit_mib=limit) + if limit == -1: + assert response.success is False + assert response.answer == "memory_limit_mib must be a non-negative integer" + assert not seen + else: + assert response.success is True + argv = seen[0] + mount = argv.index(str(tmp_path)) + assert argv[mount - 1] == "--ro-bind" + assert "--bind" not in argv + assert "--clearenv" in argv + assert "--unshare-all" in argv + + +@pytest.mark.asyncio +@pytest.mark.parametrize("name", ["read_only_paths", "writable_paths"]) +@pytest.mark.parametrize("value", [None, [], ["/"]]) +async def test_step_rejects_request_path_grants(tmp_path, name, value): + """Request payloads cannot widen or replace trusted sandbox grants.""" + step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) + response = await step(command="echo blocked", **{name: value}) assert response.success is False - assert response.answer == "memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox" + assert response.answer == f"{name} must be configured on the Step, not supplied by the caller" @pytest.mark.asyncio async def test_step_configuration_can_disable_read_only(tmp_path): """Step configuration applies when the job does not supply the flag.""" - step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path)), is_read_only=False) + step = AutoBashStep( + app_context=ApplicationContext(workspace_dir=str(tmp_path)), + is_read_only=False, + writable_paths=[str(tmp_path)], + ) response = await step(command="touch written.txt") @@ -341,14 +411,20 @@ async def test_auto_bash_uses_path_backend_when_paths_supplied(tmp_path, monkeyp return ExecResult(exit_code=0, stdout=b"ok", stderr=b"") monkeypatch.setattr(PathBubbleBackend, "exec_shell", fake_exec) - step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(command="printf ok", is_read_only=False, read_only_paths=[], writable_paths=[str(tmp_path)]) + step = AutoBashStep( + app_context=ApplicationContext(workspace_dir=str(tmp_path)), + read_only_paths=[], + writable_paths=[str(tmp_path)], + ) + response = await step(command="printf ok", is_read_only=False) assert response.success is True assert response.answer == "ok" assert seen["command"] == ["/bin/sh", "-c", "printf ok"] assert seen["cwd"] == str(tmp_path) assert seen["timeout"] == 60 + argv = seen["backend"]._bwrap_argv(seen["command"], seen["cwd"]) # pylint: disable=protected-access + assert argv[argv.index(str(tmp_path)) - 1] == "--bind" @pytest.mark.asyncio @@ -358,12 +434,13 @@ async def test_sandbox_start_failure_does_not_run_on_host(tmp_path, monkeypatch) async def unavailable(*_args, **_kwargs): raise RuntimeError("bwrap unavailable") - monkeypatch.setattr(PathBubbleBackend, "_start_process", unavailable) + monkeypatch.setattr(PathBubbleBackend, "exec_shell", unavailable) step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - result = await step(command="printf host", is_read_only=False, read_only_paths=[]) + result = await step(command="touch host-marker", is_read_only=False) assert result.success is False assert "bwrap unavailable" in result.answer + assert not (tmp_path / "host-marker").exists() def test_home_manifest_declares_step_and_job(): @@ -377,8 +454,8 @@ def test_home_manifest_declares_step_and_job(): assert AutoBash.input_schema["properties"]["timeout"]["default"] == 60000 assert job["parameters"]["properties"]["timeout"]["default"] == 60000 assert job["parameters"]["properties"]["is_read_only"]["default"] is True - assert job["parameters"]["properties"]["read_only_paths"]["type"] == "array" - assert job["parameters"]["properties"]["writable_paths"]["type"] == "array" + assert "read_only_paths" not in job["parameters"]["properties"] + assert "writable_paths" not in job["parameters"]["properties"] assert job["parameters"]["properties"]["memory_limit_mib"]["minimum"] == 0 diff --git a/plugins/home/tests/test_auto_bash_seatbelt.py b/plugins/home/tests/test_auto_bash_seatbelt.py new file mode 100644 index 00000000..50594ef2 --- /dev/null +++ b/plugins/home/tests/test_auto_bash_seatbelt.py @@ -0,0 +1,95 @@ +"""Real macOS execution, deliberately separate from the mocked command tests.""" + +import os +import shlex +import subprocess +import sys + +import pytest +from reme_home.auto_bash import AutoBash, AutoBashStep + +from reme.components.application_context import ApplicationContext +from reme.steps.common.python_execute import PythonExecuteStep +from reme.utils.file_sandbox.path_seatbelt_backend import PathSeatbeltBackend + +pytestmark = pytest.mark.skipif(sys.platform != "darwin", reason="Seatbelt requires macOS") + + +@pytest.mark.asyncio +async def test_auto_bash_git_preserves_sandbox(tmp_path, monkeypatch): + """Apple Git works under the default sandbox without granting external access.""" + root = tmp_path.resolve() + workspace = root / "workspace" + workspace.mkdir() + # Use the Apple dispatcher rather than a developer's Homebrew Git. + subprocess.run(["/usr/bin/git", "init", "-q", str(workspace)], check=True) + (workspace / "note.txt").write_text("hello", encoding="utf-8") + outside = root / "outside.txt" + outside.write_text("private", encoding="utf-8") + monkeypatch.setenv("REME_SANDBOX_TEST_SECRET", "must-not-inherit") + context = ApplicationContext(workspace_dir=str(workspace)) + step = AutoBashStep(app_context=context) + + # The default PATH may select Homebrew Git. Explicitly exercise Apple's + # dispatcher argv too, which the backend resolves to trusted developer tools. + backend = AutoBash(cwd=workspace)._backend # pylint: disable=protected-access + assert isinstance(backend, PathSeatbeltBackend) + for command, expected in (("status", "?? note.txt"), ("rev-parse", str(workspace))): + flag = "--short" if command == "status" else "--show-toplevel" + result = await backend.exec_shell(["/usr/bin/git", command, flag], timeout=10) + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert result.stderr == b"" + assert expected in result.stdout.decode() + + for read_only in (True, False): + status = await step(command="git status --short", is_read_only=read_only) + assert status.success, status.answer + assert "?? note.txt" in status.answer + toplevel = await step(command="git rev-parse --show-toplevel", is_read_only=read_only) + assert toplevel.success, toplevel.answer + assert str(workspace) in toplevel.answer + + denied = await step(command=f"cat {shlex.quote(str(outside))}") + assert not denied.success + denied = await step(command=f"echo changed > {shlex.quote(str(outside))}", is_read_only=False) + assert not denied.success + assert outside.read_text(encoding="utf-8") == "private" + denied = await step(command="echo changed > note.txt", is_read_only=False) + assert not denied.success + assert (workspace / "note.txt").read_text(encoding="utf-8") == "hello" + denied = await step(command="true", is_read_only=False, writable_paths=[str(root)]) + assert not denied.success + assert "must be configured on the Step" in denied.answer + + # Both public executors share the backend affected by the runtime grants. + code = ( + "import os; from pathlib import Path; " + "assert 'REME_SANDBOX_TEST_SECRET' not in os.environ; " + f"Path({str(outside)!r}).read_text()" + ) + python = PythonExecuteStep(app_context=context) + denied = await python(code=code) + assert not denied.success + assert "PermissionError" in denied.metadata["stderr"] + clean = await step( + command=shlex.join([sys.executable, "-c", "import os; print(os.getenv('REME_SANDBOX_TEST_SECRET', 'clean'))"]), + is_read_only=False, + ) + assert clean.success, clean.answer + assert clean.answer.strip() == "clean" + + # Make the trace destination writable so absence of output proves that + # environment cleanup works independently of filesystem denials. + trace = workspace / "trace" + trace.mkdir() + marker = trace / "git-trace" + monkeypatch.setenv("GIT_TRACE", str(marker)) + step = AutoBashStep(app_context=context, writable_paths=[str(trace)]) + for read_only in (True, False): + result = await step(command="git rev-parse --show-toplevel", is_read_only=read_only) + assert result.success, result.answer + assert not marker.exists() + assert os.environ["GIT_TRACE"] == str(marker) + assert os.environ["REME_SANDBOX_TEST_SECRET"] == "must-not-inherit" + subprocess.run(["/usr/bin/git", "rev-parse", "--show-toplevel"], cwd=workspace, check=True, capture_output=True) + assert marker.stat().st_size > 0 diff --git a/reme/config/benchmark.yaml b/reme/config/benchmark.yaml index 743475ed..79a67ab4 100644 --- a/reme/config/benchmark.yaml +++ b/reme/config/benchmark.yaml @@ -132,7 +132,10 @@ jobs: python_execute: backend: base - description: "Execute Python code and return printed stdout." + enable_serve: false + # Local-only execution; the workspace is read-only unless Step config grants writes. + # Readable workspace files (including .env) are visible to executed code. + description: "Execute sandboxed Python with a read-only workspace and return printed stdout." parameters: type: object properties: @@ -141,11 +144,11 @@ jobs: description: "Python code to execute. Print the final result to stdout." python_timeout: type: number - description: "Execution timeout in seconds; defaults to 60." + description: "Positive execution timeout in seconds; defaults to 60, capped at 86400 or the shorter Step max_python_timeout." memory_limit_mib: type: integer minimum: 0 - description: "Linux memory limit in MiB (default 4096; 0 disables it); requires a path policy; macOS ignores nonzero values" + description: "Linux sandbox memory limit in MiB (default 4096; 0 disables it); macOS ignores nonzero values" required: - code steps: diff --git a/reme/steps/common/file_sandbox.py b/reme/steps/common/file_sandbox.py index fd516899..450cf44a 100644 --- a/reme/steps/common/file_sandbox.py +++ b/reme/steps/common/file_sandbox.py @@ -1,23 +1,23 @@ -"""Resolve an invocation's optional file sandbox from injected job parameters.""" +"""Resolve executor sandbox policies.""" from ...utils.file_sandbox import create_file_sandbox_backend from ...utils.file_sandbox.file_sandbox_backend import FileSandboxBackend from ..base_step import BaseStep -def file_sandbox_for_step(step: BaseStep) -> FileSandboxBackend | None: - """Keep path and memory policy selection identical across executor steps.""" +def file_sandbox_for_step(step: BaseStep) -> FileSandboxBackend: + """Default to a read-only workspace; callers cannot supply path grants.""" assert step.context is not None - read_only_paths = step.context.get("read_only_paths", step.kwargs.get("read_only_paths")) - writable_paths = step.context.get("writable_paths", step.kwargs.get("writable_paths")) + for name in ("read_only_paths", "writable_paths"): + if name in step.context: + raise ValueError(f"{name} must be configured on the Step, not supplied by the caller") + workspace = step.workspace_path.resolve() + read_only_paths = step.kwargs.get("read_only_paths", []) + writable_paths = step.kwargs.get("writable_paths", []) memory_limit_mib = step.context.get("memory_limit_mib", step.kwargs.get("memory_limit_mib")) - if read_only_paths is None and writable_paths is None: - if memory_limit_mib is not None: - raise ValueError("memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox") - return None return create_file_sandbox_backend( - cwd=step.workspace_path, - read_only_paths=[] if read_only_paths is None else read_only_paths, - writable_paths=[] if writable_paths is None else writable_paths, + cwd=workspace, + read_only_paths=read_only_paths, + writable_paths=writable_paths, memory_limit_mib=memory_limit_mib, ) diff --git a/reme/steps/common/python_execute.py b/reme/steps/common/python_execute.py index 14ac974c..7d613524 100644 --- a/reme/steps/common/python_execute.py +++ b/reme/steps/common/python_execute.py @@ -1,27 +1,38 @@ """Execute Python code and return printed stdout.""" +import math import sys from typing import Any -from ...utils.local_backend import LocalProcessBackend, ProcessTimeout +from ...utils.local_backend import ProcessTimeout from ..base_step import BaseStep from ...components import R from .file_sandbox import file_sandbox_for_step DEFAULT_TIMEOUT = 60.0 +MAX_TIMEOUT = 86400.0 @R.register("python_execute_step") class PythonExecuteStep(BaseStep): - """Run Python code and return stdout, limiting each output stream to 1 MiB.""" + """Run sandboxed Python with a read-only workspace and bounded output. + + Timeouts default to 60 seconds and are capped at one day. A trusted Step + configuration can set max_python_timeout to a shorter limit; request data + cannot override that limit. Positive infinity is capped; NaN is rejected. + """ async def execute(self): assert self.context is not None code = self.context.get("code", "") + maximum, maximum_error = self._parse_timeout(self.kwargs.get("max_python_timeout", MAX_TIMEOUT)) timeout, timeout_error = self._parse_timeout( self.context.get("python_timeout", self.context.get("timeout", DEFAULT_TIMEOUT)), + maximum=maximum, ) + if maximum_error: + timeout_error = maximum_error.replace("python_timeout", "max_python_timeout") if not isinstance(code, str) or not code.strip(): self.context.response.success = False self.context.response.answer = "code is required" @@ -34,11 +45,11 @@ class PythonExecuteStep(BaseStep): timed_out = False try: backend = file_sandbox_for_step(self) - result = await (backend or LocalProcessBackend()).exec_shell( + result = await backend.exec_shell( [sys.executable, "-c", code], - cwd=str(self.workspace_path), + cwd=str(self.workspace_path.resolve()), timeout=timeout, - **({"raise_errors": True} if backend is None else {}), + raise_errors=True, ) except ProcessTimeout as exc: result, timed_out = exc.result, True @@ -46,7 +57,6 @@ class PythonExecuteStep(BaseStep): self.context.response.success = False self.context.response.answer = str(exc) return self.context.response - timed_out = timed_out or (result.exit_code == -1 and result.stderr == b"timed out") stdout = result.stdout.decode(errors="replace") stderr = result.stderr.decode(errors="replace") if timed_out: @@ -65,11 +75,19 @@ class PythonExecuteStep(BaseStep): return self.context.response @staticmethod - def _parse_timeout(raw: Any) -> tuple[float, str]: + def _parse_timeout(raw: Any, *, maximum: float = MAX_TIMEOUT) -> tuple[float, str]: try: + if isinstance(raw, bool): + raise ValueError("boolean timeout") timeout = float(raw) + except OverflowError: + # JSON integers can exceed float's range. They still have a known + # sign, so large positive integers follow the normal cap policy. + if isinstance(raw, int) and raw > 0: + return maximum, "" + return DEFAULT_TIMEOUT, "python_timeout must be a positive number" except (TypeError, ValueError): return DEFAULT_TIMEOUT, "python_timeout must be a positive number" - if timeout <= 0: + if math.isnan(timeout) or timeout <= 0: return DEFAULT_TIMEOUT, "python_timeout must be a positive number" - return timeout, "" + return min(timeout, maximum), "" diff --git a/reme/utils/file_sandbox/file_sandbox_backend.py b/reme/utils/file_sandbox/file_sandbox_backend.py index 7926eafd..4dc50ce3 100644 --- a/reme/utils/file_sandbox/file_sandbox_backend.py +++ b/reme/utils/file_sandbox/file_sandbox_backend.py @@ -111,18 +111,27 @@ class FileSandboxBackend(LocalProcessBackend): *, cwd: str | None = None, timeout: float | None = None, + raise_errors: bool = False, ) -> ExecResult: - """Execute an argv list with a timeout inside the platform sandbox.""" - return await self._run_job(command, cwd or self._cwd, data=None, timeout=timeout) + """Execute inside the sandbox; raise_errors preserves output in ProcessTimeout.""" + return await self._run_job(command, cwd or self._cwd, data=None, timeout=timeout, raise_errors=raise_errors) - async def _run_job(self, command: list[str], cwd: str, *, data: bytes | None, timeout: float | None) -> ExecResult: + async def _run_job( + self, + command: list[str], + cwd: str, + *, + data: bytes | None, + timeout: float | None, + raise_errors: bool = False, + ) -> ExecResult: """Run with optional stdin; platform backends may own a different job lifecycle.""" process = await self._start_process( command, cwd, stdin=asyncio.subprocess.PIPE if data is not None else asyncio.subprocess.DEVNULL, ) - return await self._communicate(process, data=data, timeout=timeout) + return await self._communicate(process, data=data, timeout=timeout, raise_errors=raise_errors) async def read_file(self, path: str) -> bytes: """Read through a sandboxed command so file rules still apply.""" diff --git a/reme/utils/file_sandbox/path_seatbelt_backend.py b/reme/utils/file_sandbox/path_seatbelt_backend.py index f2b9ee5a..5fd420ca 100644 --- a/reme/utils/file_sandbox/path_seatbelt_backend.py +++ b/reme/utils/file_sandbox/path_seatbelt_backend.py @@ -21,6 +21,7 @@ from ..async_utils import finish_cleanup from ..local_backend import ( LocalProcessBackend, OutputLimitExceeded, + ProcessTimeout, capture_output, output_limit_result, ) @@ -37,6 +38,8 @@ class LaunchctlError(RuntimeError): class PathSeatbeltBackend(FileSandboxBackend): """Run each command under a deny-by-default Seatbelt profile.""" + _DEVELOPER_ROOTS = ("/Library/Developer/CommandLineTools", "/Applications/Xcode.app/Contents/Developer") + def __init__( self, *, @@ -73,6 +76,28 @@ class PathSeatbeltBackend(FileSandboxBackend): """Allow only configured files plus the system runtime needed to launch tools.""" system_roots = ("/System", "/usr", "/bin", "/sbin", "/private/etc", "/dev") read_paths = [path for path in system_roots if os.path.exists(path)] + # Apple Git dispatches into the installed developer tools. Only trust + # standard runtime locations, never symlinks to arbitrary user data. + read_paths += [path for path in self._DEVELOPER_ROOTS if os.path.isdir(path) and os.path.realpath(path) == path] + # The Apple dispatcher also loads runtime files next to Developer. + # Keep these grants separate from Contents and reject redirected paths. + xcode_runtime = ( + "/Applications/Xcode.app/Contents/Info.plist", + "/Applications/Xcode.app/Contents/version.plist", + "/Applications/Xcode.app/Contents/Frameworks", + "/Applications/Xcode.app/Contents/SharedFrameworks", + ) + read_paths += [path for path in xcode_runtime if os.path.exists(path) and os.path.realpath(path) == path] + # Selectors may be absent: xcode-select must be allowed to probe them + # before falling back to the standard installation. Keep these literal. + runtime_files = ( + "/etc", + "/var", + "/var/select/developer_dir", + "/private/var/select/developer_dir", + "/var/db/xcode_select_link", + "/private/var/db/xcode_select_link", + ) read_paths += self._read_only_paths + self._writable_paths + [tempdir] lines = [ "(version 1)", @@ -84,13 +109,14 @@ class PathSeatbeltBackend(FileSandboxBackend): '(allow file-read* (literal "/") (literal "/private/var/select/sh"))', ] ancestors = set() - for path in read_paths: + for path in read_paths + list(runtime_files): parent = os.path.dirname(path) while parent != "/": ancestors.add(parent) parent = os.path.dirname(parent) lines.extend(f"(allow file-read-metadata (literal {json.dumps(path)}))" for path in sorted(ancestors)) lines.extend(self._runtime_link_rules()) + lines.extend(f"(allow file-read* (literal {json.dumps(path)}))" for path in runtime_files) lines.extend(f"(allow file-read* {self._path_rule(path)})" for path in read_paths) # Shell redirection to the null device is safe and is accepted by # AutoBash's read-only policy; do not grant writes to the rest of /dev. @@ -123,14 +149,60 @@ class PathSeatbeltBackend(FileSandboxBackend): lines.append(f"(deny file-write-create {rule})") return "\n".join(lines) + def _environment(self, tempdir: str, developer_bin: str | None = None) -> dict[str, str]: + """Expose runtime settings only, never service credentials or Python hooks.""" + paths = [str(Path(sys.prefix) / "bin"), "/usr/local/bin"] + if developer_bin is not None: + paths.append(developer_bin) + paths.extend(("/usr/bin", "/bin")) + return { + "HOME": self._cwd, + "TMPDIR": tempdir, + "PATH": ":".join(paths), + } + + async def _prepare_command(self, command: list[str], tempdir: str) -> tuple[list[str], dict[str, str]]: + """Resolve Apple's Git shim before sandboxing, without running user commands on the host.""" + # xcode-select only reports the system selection. In particular, do not + # inherit DEVELOPER_DIR, PATH or loader hooks from the service process. + backend = LocalProcessBackend() + try: + process = await backend._spawn_process( # pylint: disable=protected-access + ["/usr/bin/xcode-select", "--print-path"], + cwd="/", + env={}, + stdin=asyncio.subprocess.DEVNULL, + ) + result = await backend._communicate(process, timeout=5) # pylint: disable=protected-access + except OSError: + return command, self._environment(tempdir) + developer = result.stdout.decode(errors="replace").strip() if result.ok() else "" + git = os.path.join(developer, "usr", "bin", "git") + # Match the runtime grants: selection never authorizes an arbitrary + # directory or a standard installation redirected to user-owned data. + if ( + developer not in self._DEVELOPER_ROOTS + or os.path.realpath(git) != git + or not os.path.isfile(git) + or not os.access(git, os.X_OK) + ): + return command, self._environment(tempdir) + environment = self._environment(tempdir, os.path.dirname(git)) + # Shells find the real Git through PATH; explicit dispatcher argv must + # also bypass xcrun's loader and per-user cache outside sandbox TMPDIR. + if command[0] == "/usr/bin/git": + command = [git, *command[1:]] + return command, environment + async def _start_process(self, command: list[str], cwd: str, *, stdin=asyncio.subprocess.DEVNULL): """Start Seatbelt directly when the GUI launchd domain is unavailable.""" tempdir = tempfile.mkdtemp(prefix="reme-seatbelt-", dir=os.path.realpath(tempfile.gettempdir())) try: + command, environment = await self._prepare_command(command, tempdir) process = await self._spawn_process( ["/usr/bin/sandbox-exec", "-p", self._profile(tempdir), *command], cwd=cwd, - env={**os.environ, "HOME": self._cwd, "TMPDIR": tempdir}, + env=environment, stdin=stdin, ) except BaseException: @@ -202,7 +274,15 @@ class PathSeatbeltBackend(FileSandboxBackend): capture_output(buffer, chunk, name, exceeded) await asyncio.sleep(0) - async def _run_job(self, command: list[str], cwd: str, *, data: bytes | None, timeout: float | None) -> ExecResult: + async def _run_job( + self, + command: list[str], + cwd: str, + *, + data: bytes | None, + timeout: float | None, + raise_errors: bool = False, + ) -> ExecResult: if sys.platform != "darwin": raise RuntimeError("PathSeatbeltBackend requires macOS") if shutil.which("sandbox-exec") is None: @@ -217,7 +297,7 @@ class PathSeatbeltBackend(FileSandboxBackend): RuntimeWarning, stacklevel=2, ) - return await super()._run_job(command, cwd, data=data, timeout=timeout) + return await super()._run_job(command, cwd, data=data, timeout=timeout, raise_errors=raise_errors) control = Path(tempfile.mkdtemp(prefix="reme-seatbelt-", dir=os.path.realpath(tempfile.gettempdir()))) sandbox_tmp = control / "sandbox-tmp" sandbox_tmp.mkdir() @@ -249,6 +329,7 @@ class PathSeatbeltBackend(FileSandboxBackend): pass try: + command, environment = await self._prepare_command(command, str(sandbox_tmp)) for path, buffer, name in ((stdout_path, stdout, "stdout"), (stderr_path, stderr, "stderr")): os.mkfifo(path, 0o600) # Holding both ends open prevents a transient EOF before launchd @@ -269,13 +350,18 @@ class PathSeatbeltBackend(FileSandboxBackend): "-c", script, str(status_path), + # launchd can also supply its own environment. Clear it at + # the payload boundary, rather than merely adding safe keys. + "/usr/bin/env", + "-i", + *(f"{key}={value}" for key, value in environment.items()), "/usr/bin/sandbox-exec", "-p", self._profile(str(sandbox_tmp)), *command, ], "WorkingDirectory": cwd, - "EnvironmentVariables": {**os.environ, "HOME": self._cwd, "TMPDIR": str(sandbox_tmp)}, + "EnvironmentVariables": environment, "StandardInPath": str(stdin_path) if data is not None else "/dev/null", "StandardOutPath": str(stdout_path), "StandardErrorPath": str(stderr_path), @@ -297,6 +383,13 @@ class PathSeatbeltBackend(FileSandboxBackend): try: exit_code = await self._execute(run(), timeout=timeout, cleanup=cleanup) except TimeoutError: + if raise_errors: + # launchd does not expose a subprocess returncode. Use the + # wrapper's status when available, otherwise retain -1. + exit_code = int(status_path.read_text(encoding="ascii")) if status_path.exists() else -1 + raise ProcessTimeout( + ExecResult(exit_code=exit_code, stdout=bytes(stdout), stderr=bytes(stderr)), + ) from None return ExecResult(exit_code=-1, stdout=b"", stderr=b"timed out") except OutputLimitExceeded as exc: return output_limit_result(bytes(stdout), bytes(stderr), str(exc)) diff --git a/reme/utils/local_backend.py b/reme/utils/local_backend.py index 4feed857..8e67573b 100644 --- a/reme/utils/local_backend.py +++ b/reme/utils/local_backend.py @@ -193,8 +193,9 @@ class LocalProcessBackend(LocalBackend): return output_limit_result(bytes(stdout_buffer), bytes(stderr_buffer), str(exc)) except TimeoutError: if raise_errors: - stdout, stderr = communication.result() if not communication.cancelled() else (b"", b"") - raise ProcessTimeout(ExecResult(exit_code=process.returncode, stdout=stdout, stderr=stderr)) from None + raise ProcessTimeout( + ExecResult(exit_code=process.returncode, stdout=bytes(stdout_buffer), stderr=bytes(stderr_buffer)), + ) from None return ExecResult(exit_code=-1, stdout=b"", stderr=b"timed out") return ExecResult(exit_code=process.returncode, stdout=stdout, stderr=stderr) diff --git a/tests/integration/test_home_wheel.py b/tests/integration/test_home_wheel.py new file mode 100644 index 00000000..eb87d742 --- /dev/null +++ b/tests/integration/test_home_wheel.py @@ -0,0 +1,97 @@ +"""Offline installed-wheel smoke test; set REME_TEST_WHEEL_DIR to built release wheels.""" + +import os +import subprocess +import sys +import venv +from pathlib import Path + +import pytest + + +@pytest.mark.skipif(sys.platform != "darwin", reason="Real sandbox smoke test requires macOS") +def test_home_wheel_with_minimum_core(tmp_path): + """Built home and minimum-core wheels enforce policy outside the source tree.""" + directory = os.environ.get("REME_TEST_WHEEL_DIR") + if not directory: + pytest.skip("Set REME_TEST_WHEEL_DIR to a directory containing built core and home wheels") + wheels = Path(directory).resolve() + core = list(wheels.glob("reme_ai-0.4.2.2-*.whl")) + home = list(wheels.glob("reme_home-*.whl")) + assert len(core) == len(home) == 1, "Provide exactly one minimum-core wheel and one home wheel" + environment = tmp_path / "venv" + # Reuse third-party test dependencies, but install both packages into a fresh + # venv. No network or dependency upgrades are needed for this artifact test. + venv.EnvBuilder(with_pip=True, system_site_packages=True).create(environment) + interpreter = str(environment / "bin" / "python") + env = {key: value for key, value in os.environ.items() if key != "PYTHONPATH"} + subprocess.run( + [ + interpreter, + "-m", + "pip", + "install", + "--no-index", + "--no-deps", + "--force-reinstall", + str(core[0]), + str(home[0]), + ], + cwd=tmp_path, + env=env, + check=True, + capture_output=True, + text=True, + timeout=60, + ) + script = """ +import asyncio +import os +import shlex +import sys +from importlib.metadata import distribution, version +from pathlib import Path +from packaging.requirements import Requirement +import reme +import reme_home +from reme.components.application_context import ApplicationContext +from reme_home.auto_bash import AutoBashStep + +assert version('reme-ai') == '0.4.2.2' +for module in (reme, reme_home): + assert Path(module.__file__).is_relative_to(sys.prefix), module.__file__ +dist = distribution('reme-home') +requirement = next(Requirement(item) for item in dist.requires if Requirement(item).name == 'reme-ai') +assert requirement.extras == {'as'} +assert '0.4.2.1' not in requirement.specifier +assert '0.4.2.2' in requirement.specifier +next(point for point in dist.entry_points if point.group == 'reme.plugins' and point.name == 'home').load() + +async def main(): + workspace = Path('workspace').resolve() + workspace.mkdir() + outside = Path('outside.txt').resolve() + outside.write_text('private') + step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(workspace))) + denied = await step(command='true', is_read_only=False, writable_paths=[str(workspace.parent)]) + assert not denied.success and 'must be configured on the Step' in denied.answer + denied = await step(command=f'echo changed > {shlex.quote(str(outside))}', is_read_only=False) + assert not denied.success, denied.answer + assert outside.read_text() == 'private' + os.environ['REME_WHEEL_TEST_SECRET'] = 'must-not-inherit' + command = shlex.join([sys.executable, '-c', "import os; print(os.getenv('REME_WHEEL_TEST_SECRET', 'clean'))"]) + result = await step(command=command, is_read_only=False) + assert result.success and result.answer.strip() == 'clean', result.answer + +asyncio.run(main()) +""" + result = subprocess.run( + [interpreter, "-c", script], + cwd=tmp_path, + env=env, + capture_output=True, + text=True, + timeout=60, + check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr diff --git a/tests/integration/test_seatbelt_backend.py b/tests/integration/test_seatbelt_backend.py index 8a66379b..792be347 100644 --- a/tests/integration/test_seatbelt_backend.py +++ b/tests/integration/test_seatbelt_backend.py @@ -2,6 +2,8 @@ import asyncio import os +import shlex +import subprocess import sys import sysconfig import tempfile @@ -17,6 +19,64 @@ from reme.utils.file_sandbox.path_seatbelt_backend import PathSeatbeltBackend pytestmark = pytest.mark.skipif(sys.platform != "darwin", reason="Seatbelt requires macOS") +@pytest.mark.asyncio +@pytest.mark.parametrize("direct", [False, True], ids=["auto-launch", "direct"]) +async def test_seatbelt_apple_git_preserves_isolation(tmp_path, monkeypatch, direct): + """Resolve Apple's Git to real tools, with isolated files and temporary storage.""" + root = tmp_path.resolve() + workspace = root / "workspace" + workspace.mkdir() + subprocess.run(["/usr/bin/git", "init", "-q", str(workspace)], check=True, capture_output=True) + (workspace / "note.txt").write_text("hello", encoding="utf-8") + outside = root / "outside.txt" + outside.write_text("private", encoding="utf-8") + monkeypatch.setenv("REME_SANDBOX_TEST_SECRET", "must-not-inherit") + monkeypatch.setenv("DEVELOPER_DIR", str(root / "untrusted-developer")) + backend = PathSeatbeltBackend(cwd=workspace, read_only_paths=[], writable_paths=[]) + if direct: + + async def missing(_domain): + return False + + monkeypatch.setattr(backend, "_has_gui_launchd", missing) + + for args, expected in ((["status", "--short"], "?? note.txt"), (["rev-parse", "--show-toplevel"], str(workspace))): + for command in (["/usr/bin/git", *args], ["git", *args], ["/bin/sh", "-c", shlex.join(["git", *args])]): + result = await backend.exec_shell(command, timeout=10) + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert result.stderr == b"" + assert expected in result.stdout.decode() + for args in (["--version"], ["--exec-path"], ["hash-object", "note.txt"]): + result = await backend.exec_shell(["/usr/bin/git", *args], timeout=10) + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert result.stderr == b"" + assert result.stdout.strip() + + code = f""" +import os +from pathlib import Path +assert os.environ['HOME'] == {str(workspace)!r} +assert 'REME_SANDBOX_TEST_SECRET' not in os.environ +assert 'DEVELOPER_DIR' not in os.environ +temporary = Path(os.environ['TMPDIR']) +(temporary / 'cache').write_text('temporary') +for path, operation in [({str(outside)!r}, 'read'), ({str(outside)!r}, 'write'), ('note.txt', 'write')]: + try: + Path(path).read_text() if operation == 'read' else Path(path).write_text('denied') + except PermissionError: + pass + else: + raise AssertionError(f'{{operation}} unexpectedly allowed: {{path}}') +print(temporary) +""" + result = await backend.exec_shell([sys.executable, "-c", code], timeout=10) + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert not Path(result.stdout.decode().strip()).exists() + assert outside.read_text(encoding="utf-8") == "private" + assert (workspace / "note.txt").read_text(encoding="utf-8") == "hello" + assert os.environ["REME_SANDBOX_TEST_SECRET"] == "must-not-inherit" + + @pytest.mark.asyncio @pytest.mark.parametrize( "command", diff --git a/tests/unit/test_common_steps.py b/tests/unit/test_common_steps.py index b63c5f69..02c8a870 100644 --- a/tests/unit/test_common_steps.py +++ b/tests/unit/test_common_steps.py @@ -17,11 +17,21 @@ from reme.steps.common.health_check import _file_graph_status from reme.steps.common.llm_demo import LLMDemoStep from reme.steps.common.python_execute import PythonExecuteStep from reme.steps.index import traverse as traverse_mod +from reme.utils.local_backend import LocalProcessBackend warnings.filterwarnings("ignore", category=DeprecationWarning, module="jieba") warnings.filterwarnings("ignore", category=DeprecationWarning, module="pkg_resources") +@pytest.fixture(autouse=True) +def python_process_backend(monkeypatch): + """Test executor responses with real pipes; sandbox policy is tested separately.""" + monkeypatch.setattr( + "reme.steps.common.python_execute.file_sandbox_for_step", + lambda _step: LocalProcessBackend(), + ) + + def _node( path: str, links: list[tuple[str, str | None]] | None = None, diff --git a/tests/unit/test_file_sandbox_backend.py b/tests/unit/test_file_sandbox_backend.py index 0b46bde6..c261e7ea 100644 --- a/tests/unit/test_file_sandbox_backend.py +++ b/tests/unit/test_file_sandbox_backend.py @@ -11,7 +11,7 @@ from agentscope.tool import ExecResult from reme.components.application_context import ApplicationContext from reme.steps.common.python_execute import PythonExecuteStep -from reme.utils.local_backend import LocalProcessBackend +from reme.utils.local_backend import LocalProcessBackend, ProcessTimeout from reme.utils.file_sandbox import create_file_sandbox_backend from reme.utils.file_sandbox.path_bubble_backend import PathBubbleBackend from reme.utils.file_sandbox.path_seatbelt_backend import PathSeatbeltBackend @@ -38,18 +38,16 @@ def test_platform_selection_and_memory_defaults(tmp_path, monkeypatch): @pytest.mark.asyncio -async def test_unsupported_platform_requires_backend_only_for_path_policy(tmp_path, monkeypatch): - """An unsupported platform runs ordinary Python but rejects a requested sandbox.""" +async def test_unsupported_platform_never_runs_python_on_host(tmp_path, monkeypatch): + """Default execution fails closed when the platform has no sandbox.""" monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "unsupported") step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - plain = await step(code="print('host')") - assert plain.success is True - assert plain.answer == "host\n" - - restricted = await step(code="print('blocked')", read_only_paths=[]) + marker = tmp_path / "host-execution" + restricted = await step(code=f"from pathlib import Path; Path({str(marker)!r}).touch()") assert restricted.success is False assert restricted.answer == "No file sandbox backend for unsupported" + assert not marker.exists() def test_seatbelt_profile_limits_file_access(tmp_path): @@ -67,6 +65,178 @@ def test_seatbelt_profile_limits_file_access(tmp_path): assert "network*" not in profile +@pytest.mark.parametrize("redirected", [False, True]) +@pytest.mark.parametrize( + "runtime", + ["/Library/Developer/CommandLineTools", "/Applications/Xcode.app/Contents/Developer"], +) +def test_seatbelt_developer_runtime_grants_are_read_only(tmp_path, monkeypatch, redirected, runtime): + """Standard developer tools are readable, but symlinks cannot expose other trees.""" + original_isdir = os.path.isdir + original_realpath = os.path.realpath + monkeypatch.setattr(os.path, "isdir", lambda path: path == runtime or original_isdir(path)) + monkeypatch.setattr( + os.path, + "realpath", + lambda path: (str(tmp_path) if redirected else runtime) if path == runtime else original_realpath(path), + ) + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + profile = backend._profile(str(tmp_path / "temporary")) # pylint: disable=protected-access + + assert (f'(allow file-read* (subpath "{runtime}"))' in profile) is not redirected + assert f'(allow file-write* (subpath "{runtime}"))' not in profile + for path in ("/Library", "/var", "/private/var"): + assert f'(allow file-read* (subpath "{path}"))' not in profile + assert f'(allow file-write* (subpath "{path}"))' not in profile + for path in ("/etc", "/var", "/var/select/developer_dir", "/private/var/db/xcode_select_link"): + assert f'(allow file-read* (literal "{path}"))' in profile + assert '(allow file-read-metadata (literal "/private/var/select"))' in profile + + +@pytest.mark.parametrize("redirected", [False, True]) +def test_seatbelt_developer_runtime_grants_are_narrow(tmp_path, monkeypatch, redirected): + """Xcode's sibling runtime is readable, without trusting symlinked installations.""" + directories = { + "/Library/Developer/CommandLineTools", + "/Applications/Xcode.app/Contents/Developer", + "/Applications/Xcode.app/Contents/Frameworks", + "/Applications/Xcode.app/Contents/SharedFrameworks", + } + runtime = directories | { + "/Applications/Xcode.app/Contents/Info.plist", + "/Applications/Xcode.app/Contents/version.plist", + } + exists, isdir, realpath = os.path.exists, os.path.isdir, os.path.realpath + monkeypatch.setattr(os.path, "exists", lambda path: path in runtime or exists(path)) + monkeypatch.setattr(os.path, "isdir", lambda path: path in directories or isdir(path)) + # Model both canonical and redirected installations independently of the + # host's Xcode.app symlink (common on GitHub's macOS runners). + monkeypatch.setattr( + os.path, + "realpath", + lambda path: (str(tmp_path / "untrusted") if redirected else path) if path in runtime else realpath(path), + ) + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + profile = backend._profile(str(tmp_path / "temporary")) # pylint: disable=protected-access + + for path in runtime: + rule = f'(subpath "{path}")' if path in directories else f'(literal "{path}")' + assert (f"(allow file-read* {rule})" in profile) is not redirected + assert f"(allow file-write* {rule})" not in profile + for parent in ("/Applications", "/Applications/Xcode.app/Contents", "/Library/Developer", "/private/var/db"): + assert f'(allow file-read* (subpath "{parent}"))' not in profile + assert f'(allow file-write* (subpath "{parent}"))' not in profile + assert '(allow file-read* (literal "/private/var/db/xcode_select_link"))' in profile + + +def test_seatbelt_environment_drops_host_values(tmp_path, monkeypatch): + """The environment is an allowlist, including for future secret and hook names.""" + values = { + "REME_SANDBOX_TEST_SECRET": "secret", + "PYTHONPATH": str(tmp_path), + "PYTHONSTARTUP": str(tmp_path / "hook.py"), + "DYLD_INSERT_LIBRARIES": str(tmp_path / "hook.dylib"), + "DEVELOPER_DIR": str(tmp_path / "developer"), + "PATH": str(tmp_path / "untrusted"), + "GIT_TRACE_FUTURE": str(tmp_path / "trace"), + } + for name, value in values.items(): + monkeypatch.setenv(name, value) + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + environment = backend._environment(str(tmp_path / "temporary")) # pylint: disable=protected-access + assert environment == { + "HOME": str(tmp_path), + "TMPDIR": str(tmp_path / "temporary"), + "PATH": f"{sys.prefix}/bin:/usr/local/bin:/usr/bin:/bin", + } + assert all(os.environ[name] == value for name, value in values.items()) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "developer,usable", + [ + ("/Library/Developer/CommandLineTools", True), + ("/Applications/Xcode.app/Contents/Developer", True), + ("/Applications/Other.app/Contents/Developer", False), + ("/tmp/developer", False), + ("relative/developer", False), + ("", False), + ], +) +@pytest.mark.parametrize("redirected", [False, True]) +async def test_seatbelt_resolves_only_trusted_selected_git(tmp_path, monkeypatch, developer, usable, redirected): + """Host selection is sanitized, and only an unredirected runtime can replace Apple's shim.""" + from unittest.mock import AsyncMock + + monkeypatch.setenv("DEVELOPER_DIR", "/untrusted/developer") + monkeypatch.setenv("DYLD_INSERT_LIBRARIES", "/untrusted/loader") + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + process = object() + spawn = AsyncMock(return_value=process) + communicate = AsyncMock(return_value=ExecResult(exit_code=0, stdout=f"{developer}\n".encode(), stderr=b"")) + monkeypatch.setattr(LocalProcessBackend, "_spawn_process", spawn) + monkeypatch.setattr(LocalProcessBackend, "_communicate", communicate) + git = f"{developer}/usr/bin/git" + realpath = os.path.realpath + monkeypatch.setattr(os.path, "realpath", lambda path: "/untrusted/git" if redirected and path == git else path) + monkeypatch.setattr(os.path, "isfile", lambda path: path == git) + monkeypatch.setattr(os, "access", lambda path, _mode: path == git) + temporary = str(tmp_path / "temporary") + command = ["/usr/bin/git", "status", "--short"] + + prepared, environment = await backend._prepare_command(command, temporary) # pylint: disable=protected-access + + spawn.assert_awaited_once_with( + ["/usr/bin/xcode-select", "--print-path"], + cwd="/", + env={}, + stdin=asyncio.subprocess.DEVNULL, + ) + communicate.assert_awaited_once_with(process, timeout=5) + selected = usable and not redirected + assert prepared == ([git, "status", "--short"] if selected else command) + expected_bin = f"{developer}/usr/bin:" if selected else "" + assert environment == { + "HOME": str(tmp_path), + "TMPDIR": temporary, + "PATH": f"{sys.prefix}/bin:/usr/local/bin:{expected_bin}/usr/bin:/bin", + } + assert command == ["/usr/bin/git", "status", "--short"] + # A shell's script and quoting remain intact; it finds Git through PATH. + shell = ["/bin/sh", "-c", "git rev-parse --show-toplevel"] + prepared, _ = await backend._prepare_command(shell, temporary) # pylint: disable=protected-access + assert prepared == shell + monkeypatch.setattr(os.path, "realpath", realpath) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("failure", ["missing", "timeout", "failed", "not-executable", "not-file"]) +async def test_seatbelt_git_selection_failure_preserves_sandbox(tmp_path, monkeypatch, failure): + """Unavailable tools never turn into arbitrary host execution or broader path grants.""" + from unittest.mock import AsyncMock + + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + spawn = AsyncMock(side_effect=FileNotFoundError() if failure == "missing" else None) + developer = "/Library/Developer/CommandLineTools" + result = ExecResult( + exit_code=0 if failure in {"not-executable", "not-file"} else -1, + stdout=f"{developer}\n".encode(), + stderr=b"timed out" if failure == "timeout" else b"", + ) + monkeypatch.setattr(LocalProcessBackend, "_spawn_process", spawn) + monkeypatch.setattr(LocalProcessBackend, "_communicate", AsyncMock(return_value=result)) + monkeypatch.setattr(os.path, "isfile", lambda _: failure != "not-file") + monkeypatch.setattr(os, "access", lambda *_: failure != "not-executable") + command = ["/usr/bin/git", "status", "--short"] + temporary = str(tmp_path / "temporary") + + prepared, environment = await backend._prepare_command(command, temporary) # pylint: disable=protected-access + + assert prepared == command + assert environment == backend._environment(temporary) # pylint: disable=protected-access + + def test_sandbox_reads_virtual_environment_base_runtime(tmp_path, monkeypatch): """A virtual environment grants its real base interpreter without widening writes.""" workspace = tmp_path / "workspace" @@ -157,6 +327,8 @@ async def test_sandbox_file_io_round_trip(tmp_path, monkeypatch, data): async def test_seatbelt_launch_failure_cleans_up_without_host_fallback(tmp_path, monkeypatch): """A failed sandbox launcher never starts the requested command directly.""" monkeypatch.setattr(sys, "platform", "darwin") + monkeypatch.setenv("REME_TEST_SECRET", "must-not-be-inherited") + monkeypatch.setenv("PYTHONPATH", "/untrusted/python") monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.shutil.which", lambda _: "/usr/bin/sandbox-exec") seen = {} @@ -183,30 +355,110 @@ async def test_seatbelt_launch_failure_cleans_up_without_host_fallback(tmp_path, assert seen["arguments"][0] == "bootstrap" assert seen["job"]["AbandonProcessGroup"] is False assert seen["job"]["ProgramArguments"][-2:] == ["/bin/echo", "unsafe"] + assert "REME_TEST_SECRET" not in seen["job"]["EnvironmentVariables"] + assert "PYTHONPATH" not in seen["job"]["EnvironmentVariables"] + argv = seen["job"]["ProgramArguments"] + assert argv[argv.index("/usr/bin/env") + 1] == "-i" + assert not any("must-not-be-inherited" in argument for argument in argv) assert not Path(seen["arguments"][-1]).exists() @pytest.mark.asyncio async def test_python_job_selects_seatbelt_on_macos(tmp_path, monkeypatch): - """Path injection reaches the macOS backend with no implicit memory cap.""" + """Default execution selects macOS isolation with a read-only workspace.""" monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "darwin") seen = {} - async def fake_exec(self, command, *, cwd=None, timeout=None): + async def fake_run_job(self, command, cwd, *, data, timeout, raise_errors=False): + assert data is None + assert raise_errors is True seen.update(backend=self, command=command, cwd=cwd, timeout=timeout) return ExecResult(exit_code=0, stdout=b"ok\n", stderr=b"") - monkeypatch.setattr(PathSeatbeltBackend, "exec_shell", fake_exec) + monkeypatch.setattr(PathSeatbeltBackend, "_run_job", fake_run_job) step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(code="print('ok')", read_only_paths=[]) + response = await step(code="print('ok')") assert response.success is True assert isinstance(seen["backend"], PathSeatbeltBackend) assert seen["command"] == [sys.executable, "-c", "print('ok')"] + assert seen["cwd"] == str(tmp_path) + assert seen["timeout"] == 60 + profile = seen["backend"]._profile(str(tmp_path / "temporary")) # pylint: disable=protected-access + assert f'(allow file-read* (subpath "{tmp_path}"))' in profile + assert f'(allow file-write* (subpath "{tmp_path}"))' not in profile @pytest.mark.asyncio -@pytest.mark.parametrize("mode", ["success", "failure", "timeout", "cancel", "error", "kickstart_error", "write"]) -async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): +@pytest.mark.parametrize("platform", ["linux", "darwin"]) +@pytest.mark.parametrize("grant", ["default", "workspace", "output"]) +async def test_python_workspace_grants(tmp_path, monkeypatch, platform, grant): + """Backend completion keeps defaults read-only and accepts explicit write grants.""" + monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", platform) + output = tmp_path / "output" + output.mkdir() + writable = [] if grant == "default" else [str(tmp_path if grant == "workspace" else output)] + backend_class = PathBubbleBackend if platform == "linux" else PathSeatbeltBackend + + async def inspect_exec(backend, _command, **kwargs): + assert kwargs["raise_errors"] is True + assert backend._writable_paths == writable # pylint: disable=protected-access + read_only = backend._read_only_paths # pylint: disable=protected-access + assert (str(tmp_path) in read_only) is (grant != "workspace") + return ExecResult(exit_code=0, stdout=b"ok\n", stderr=b"") + + monkeypatch.setattr(backend_class, "exec_shell", inspect_exec) + step = PythonExecuteStep( + app_context=ApplicationContext(workspace_dir=str(tmp_path)), + **({"writable_paths": writable} if writable else {}), + ) + response = await step(code="print('ok')") + assert response.success is True + assert response.answer == "ok\n" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("platform", ["linux", "darwin"]) +@pytest.mark.parametrize("name", ["read_only_paths", "writable_paths"]) +async def test_python_rejects_request_path_grants(tmp_path, monkeypatch, platform, name): + """Neither platform accepts request-level path authorization.""" + monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", platform) + step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) + response = await step(code="print('blocked')", **{name: [str(tmp_path)]}) + assert response.success is False + assert response.answer == f"{name} must be configured on the Step, not supplied by the caller" + + +@pytest.mark.asyncio +async def test_seatbelt_direct_execution_does_not_inherit_service_environment(tmp_path, monkeypatch): + """The non-launchd path strips credentials and interpreter injection settings too.""" + monkeypatch.setenv("REME_TEST_SECRET", "must-not-be-inherited") + monkeypatch.setenv("PYTHONPATH", "/untrusted/python") + monkeypatch.setenv("DYLD_INSERT_LIBRARIES", "/untrusted/library") + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + seen = {} + + async def spawn(_command, **kwargs): + seen.update(kwargs) + raise RuntimeError("stop before spawning") + + monkeypatch.setattr(backend, "_spawn_process", spawn) + with pytest.raises(RuntimeError, match="stop before spawning"): + await backend._start_process( # pylint: disable=protected-access + [sys.executable, "-c", "print('ok')"], + str(tmp_path), + ) + assert set(seen["env"]) == {"HOME", "TMPDIR", "PATH"} + assert seen["env"]["HOME"] == str(tmp_path) + assert not Path(seen["env"]["TMPDIR"]).exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("raise_errors", [False, True]) +@pytest.mark.parametrize( + "mode", + ["success", "failure", "timeout", "timeout_status", "cancel", "error", "kickstart_error", "write"], +) +async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode, raise_errors): """Every launchd outcome unregisters the job and removes its control files.""" from unittest.mock import AsyncMock, Mock @@ -235,13 +487,15 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): # Output is collected after launchd has reaped the process group. (control / "stdout").write_bytes(b"output") (control / "stderr").write_bytes(b"diagnostic") + if mode == "timeout_status": + (control / "status").write_text("137\n", encoding="ascii") return "12345" async def wait_status(*_args): started.set() if mode == "error": raise RuntimeError("status failed") - if mode in ("timeout", "cancel"): + if mode in ("timeout", "timeout_status", "cancel"): await asyncio.Event().wait() return 7 if mode == "failure" else 0 @@ -250,7 +504,11 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): operation = ( backend.write_file(str(tmp_path / "note.bin"), b"binary\x00\xff") if mode == "write" - else backend.exec_shell(["true"], timeout=0.01 if mode == "timeout" else None) + else backend.exec_shell( + ["true"], + timeout=0.01 if mode.startswith("timeout") else None, + raise_errors=raise_errors, + ) ) task = asyncio.create_task(operation) if mode == "cancel": @@ -261,23 +519,49 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): elif mode in ("error", "kickstart_error"): with pytest.raises(RuntimeError, match="failed"): await asyncio.wait_for(task, timeout=1) + elif mode.startswith("timeout") and raise_errors: + with pytest.raises(ProcessTimeout) as caught: + await asyncio.wait_for(task, timeout=1) + assert caught.value.result == ExecResult( + exit_code=137 if mode == "timeout_status" else -1, + stdout=b"output", + stderr=b"diagnostic", + ) else: result = await asyncio.wait_for(task, timeout=1) if mode == "write": assert result is None - elif mode == "timeout": + elif mode.startswith("timeout"): assert result == ExecResult(exit_code=-1, stdout=b"", stderr=b"timed out") else: assert result.exit_code == (7 if mode == "failure" else 0) assert result.stdout == b"output" and result.stderr == b"diagnostic" assert calls == ["bootstrap", "kickstart", "bootout"] assert not control.exists() - assert killed.call_count == int(mode in ("timeout", "cancel", "error")) + assert killed.call_count == int(mode in ("timeout", "timeout_status", "cancel", "error")) @pytest.mark.asyncio -async def test_python_timeout_preserves_exit_status_and_stderr(tmp_path): - """The shared executor retains the existing unsandboxed Python metadata.""" +@pytest.mark.parametrize("backend_class", [PathBubbleBackend, PathSeatbeltBackend]) +async def test_python_timeout_preserves_exit_status_and_stderr(tmp_path, monkeypatch, backend_class): + """Real pipes exercise sandbox timeout propagation without OS isolation requirements.""" + from unittest.mock import AsyncMock + + backend = backend_class(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + + async def start(command, cwd, *, stdin): + return await LocalProcessBackend()._start_process(command, cwd, stdin=stdin) # pylint: disable=protected-access + + monkeypatch.setattr(backend, "_start_process", start) + if backend_class is PathSeatbeltBackend: + monkeypatch.setattr(sys, "platform", "darwin") + monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.shutil.which", lambda _: "sandbox-exec") + monkeypatch.setattr(backend, "_has_gui_launchd", AsyncMock(return_value=False)) + monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.warnings.warn", lambda *a, **kw: None) + monkeypatch.setattr( + "reme.steps.common.python_execute.file_sandbox_for_step", + lambda _step: backend, + ) step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) response = await step( code="import sys, time; print('diagnostic', file=sys.stderr, flush=True); time.sleep(60)", diff --git a/tests/unit/test_local_backend.py b/tests/unit/test_local_backend.py index d6cb10c9..84892dcc 100644 --- a/tests/unit/test_local_backend.py +++ b/tests/unit/test_local_backend.py @@ -2,12 +2,40 @@ import asyncio import os +from types import SimpleNamespace import anyio import psutil import pytest -from reme.utils.local_backend import LocalProcessBackend, finish_cleanup +from reme.utils.local_backend import LocalProcessBackend, ProcessTimeout, finish_cleanup + + +@pytest.mark.asyncio +async def test_timeout_retains_output_when_pipe_draining_is_cancelled(monkeypatch): + """Bounded cleanup must retain bytes captured before an inherited pipe stalls.""" + from unittest.mock import AsyncMock, Mock + + backend = LocalProcessBackend() + process = SimpleNamespace(returncode=-9, _transport=Mock(), wait=AsyncMock(return_value=-9)) + + async def collect(_process, _data, stdout, stderr, _exceeded): + stdout.extend(b"partial output") + stderr.extend(b"diagnostic") + await asyncio.Event().wait() + + monkeypatch.setattr(backend, "_collect_output", collect) + monkeypatch.setattr(backend, "_kill_process", lambda _process: None) + monkeypatch.setattr("reme.utils.local_backend._CLEANUP_TIMEOUT", 0.01) + with pytest.raises(ProcessTimeout) as caught: + await asyncio.wait_for( + backend._communicate(process, timeout=0.02, raise_errors=True), # pylint: disable=protected-access + timeout=1, + ) + assert caught.value.result.exit_code == -9 + assert caught.value.result.stdout == b"partial output" + assert caught.value.result.stderr == b"diagnostic" + process._transport.close.assert_called_once() # pylint: disable=protected-access @pytest.mark.asyncio diff --git a/tests/unit/test_package_versions.py b/tests/unit/test_package_versions.py index 559e62a9..ac8530ae 100644 --- a/tests/unit/test_package_versions.py +++ b/tests/unit/test_package_versions.py @@ -27,15 +27,16 @@ bump_version = _load_script("bump_version") package_studio = _load_script("package_studio") -def test_home_requires_first_sandbox_release() -> None: - """Home must reject published ReMe wheels that predate its sandbox imports.""" +def test_home_requires_complete_sandbox_fix() -> None: + """Home must reject cores without request path rejection and environment isolation.""" config = tomllib.loads((REPOSITORY / "plugins" / "home" / "pyproject.toml").read_text(encoding="utf-8")) requirement = next(Requirement(value) for value in config["project"]["dependencies"] if value.startswith("reme-ai")) assert requirement.extras == {"as"} assert Version("0.4.1.14") not in requirement.specifier assert Version("0.4.2.0") not in requirement.specifier - assert Version("0.4.2.1") in requirement.specifier + assert Version("0.4.2.1") not in requirement.specifier + assert Version("0.4.2.2") in requirement.specifier assert Version(bump_version.read_version()) in requirement.specifier diff --git a/tests/unit/test_path_bubble_backend.py b/tests/unit/test_path_bubble_backend.py index 03695788..db3a33de 100644 --- a/tests/unit/test_path_bubble_backend.py +++ b/tests/unit/test_path_bubble_backend.py @@ -14,6 +14,8 @@ from agentscope.tool import ExecResult from reme.utils.file_sandbox.path_bubble_backend import PathBubbleBackend from reme.components.application_context import ApplicationContext +from reme.components.job import BaseJob +from reme.components.service import HttpService, MCPService from reme.steps.common.python_execute import PythonExecuteStep @@ -274,17 +276,19 @@ def test_path_bubble_backend_rejects_replaced_mount_source(tmp_path): @pytest.mark.asyncio @pytest.mark.parametrize("memory_options", [{}, {"memory_limit_mib": 0}]) -async def test_python_execute_uses_path_backend_when_paths_supplied(tmp_path, monkeypatch, memory_options): - """Python execution uses the same backend and timeout units.""" +async def test_python_execute_defaults_to_read_only_sandbox(tmp_path, monkeypatch, memory_options): + """Default Python execution has no host write grants and uses the sandbox.""" seen = {} - async def fake_exec(self, command, *, cwd=None, timeout=None): + async def fake_run_job(self, command, cwd, *, data, timeout, raise_errors=False): + assert data is None + assert raise_errors is True seen.update(command=command, cwd=cwd, timeout=timeout, backend=self) return ExecResult(exit_code=0, stdout=b"ok\n", stderr=b"") - monkeypatch.setattr(PathBubbleBackend, "exec_shell", fake_exec) + monkeypatch.setattr(PathBubbleBackend, "_run_job", fake_run_job) step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(code="print('ok')", read_only_paths=[], writable_paths=[str(tmp_path)], **memory_options) + response = await step(code="print('ok')", **memory_options) assert response.success is True assert response.answer == "ok\n" @@ -293,23 +297,27 @@ async def test_python_execute_uses_path_backend_when_paths_supplied(tmp_path, mo assert seen["timeout"] == 60 expected_limit = memory_options.get("memory_limit_mib", 4096) assert seen["backend"]._memory_limit_mib == expected_limit # pylint: disable=protected-access + argv = seen["backend"]._bwrap_argv(seen["command"], str(tmp_path)) # pylint: disable=protected-access + assert ("--ro-bind", str(tmp_path), str(tmp_path)) in _mounts(argv) + assert not any(mode == "--bind" for mode, _source, _target in _mounts(argv)) + assert "--clearenv" in argv and "--unshare-all" in argv @pytest.mark.asyncio -@pytest.mark.parametrize("limit", [0, 1, -1]) -async def test_python_execute_rejects_memory_limit_without_path_policy(tmp_path, limit): - """A requested memory policy must never be silently ignored.""" +@pytest.mark.parametrize("policy", [{"writable_paths": ["/tmp"]}, {"read_only_paths": []}, {"writable_paths": None}]) +async def test_python_execute_rejects_caller_path_grants(tmp_path, policy): + """A caller cannot widen or disable the server's sandbox path policy.""" step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(code="print('should not run')", memory_limit_mib=limit) + response = await step(code="print('should not run')", **policy) assert response.success is False - assert response.answer == "memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox" + assert "must be configured on the Step, not supplied by the caller" in response.answer @pytest.mark.asyncio async def test_step_rejects_invalid_mount_parameters(tmp_path): """Malformed path injection cannot silently disable sandboxing.""" - step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(code="print('unsafe')", writable_paths="/tmp") + step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path)), writable_paths="/tmp") + response = await step(code="print('unsafe')") assert response.success is False assert "writable_paths must be a list" in response.answer @@ -324,7 +332,7 @@ async def test_sandbox_start_failure_does_not_run_on_host(tmp_path, monkeypatch) monkeypatch.setattr(PathBubbleBackend, "_start_process", unavailable) app_context = ApplicationContext(workspace_dir=str(tmp_path)) python_step = PythonExecuteStep(app_context=app_context) - python_result = await python_step(code="print('host')", read_only_paths=[]) + python_result = await python_step(code="print('host')") assert python_result.success is False assert "bwrap unavailable" in python_result.answer @@ -333,7 +341,39 @@ async def test_sandbox_start_failure_does_not_run_on_host(tmp_path, monkeypatch) def test_job_hides_injected_path_policy(): """Path policy reaches the Step through context without becoming a tool argument.""" config_path = Path(__file__).parents[2] / "reme" / "config" / "benchmark.yaml" - parameters = yaml.safe_load(config_path.read_text(encoding="utf-8"))["jobs"]["python_execute"]["parameters"] + job = yaml.safe_load(config_path.read_text(encoding="utf-8"))["jobs"]["python_execute"] + assert job["enable_serve"] is False + parameters = job["parameters"] assert "read_only_paths" not in parameters["properties"] assert "writable_paths" not in parameters["properties"] assert "default" not in parameters["properties"]["memory_limit_mib"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("service_class", [HttpService, MCPService]) +async def test_benchmark_python_job_is_private_but_locally_callable(tmp_path, monkeypatch, service_class): + """Disabling serving preserves the benchmark's in-process job execution.""" + from unittest.mock import Mock + + config_path = Path(__file__).parents[2] / "reme" / "config" / "benchmark.yaml" + config = yaml.safe_load(config_path.read_text(encoding="utf-8"))["jobs"]["python_execute"] + context = ApplicationContext(workspace_dir=str(tmp_path)) + job = BaseJob(name="python_execute", app_context=context, **config) + context.jobs["python_execute"] = job + service = service_class() + service.add_job = Mock(return_value=True) + service.add_jobs(SimpleNamespace(context=context)) + service.add_job.assert_not_called() + + monkeypatch.setattr( + PathBubbleBackend, + "exec_shell", + AsyncMock(return_value=ExecResult(exit_code=0, stdout=b"local\n", stderr=b"")), + ) + await job.start() + try: + response = await job(code="print('local')") + assert response.success is True + assert response.answer == "local\n" + finally: + await job.close() diff --git a/tests/unit/test_process_output_limits.py b/tests/unit/test_process_output_limits.py index b14f4f6f..989dce42 100644 --- a/tests/unit/test_process_output_limits.py +++ b/tests/unit/test_process_output_limits.py @@ -60,8 +60,12 @@ async def test_continuous_output_stops_process(tmp_path, fd): @pytest.mark.asyncio -async def test_python_step_reports_output_overflow(tmp_path): +async def test_python_step_reports_output_overflow(tmp_path, monkeypatch): """Python callers receive bounded partial output and failure metadata.""" + monkeypatch.setattr( + "reme.steps.common.python_execute.file_sandbox_for_step", + lambda _step: LocalProcessBackend(), + ) step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) response = await step(code=f"import os;os.write(1, b'x' * {OUTPUT_LIMIT_BYTES + 1})") assert response.success is False diff --git a/tests/unit/test_python_execute_timeout.py b/tests/unit/test_python_execute_timeout.py new file mode 100644 index 00000000..88df253b --- /dev/null +++ b/tests/unit/test_python_execute_timeout.py @@ -0,0 +1,96 @@ +"""Timeout bounds at the Python executor and HTTP request boundaries.""" + +from unittest.mock import AsyncMock + +import pytest +from agentscope.tool import ExecResult +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from reme.components.application_context import ApplicationContext +from reme.components.job.base_job import BaseJob +from reme.components.service.http_service import HttpService +from reme.steps.common.python_execute import PythonExecuteStep + + +@pytest.fixture(name="backend") +def mock_backend(monkeypatch): + """Capture deadlines without executing untrusted code or waiting for them.""" + instance = AsyncMock() + instance.exec_shell.return_value = ExecResult(exit_code=0, stdout=b"ok\n", stderr=b"") + monkeypatch.setattr("reme.steps.common.python_execute.file_sandbox_for_step", lambda _step: instance) + return instance + + +@pytest.mark.asyncio +@pytest.mark.parametrize("key", ["python_timeout", "timeout"]) +@pytest.mark.parametrize( + ("requested", "expected"), + [ + (0.25, 0.25), + ("12.5", 12.5), + (86400, 86400), + (86401, 86400), + ("inf", 86400), + (float("inf"), 86400), + ("1e9999", 86400), + (10**400, 86400), + ], +) +async def test_timeout_is_capped_before_execution(backend, key, requested, expected): + """Both supported request keys use a finite deadline and report the effective value.""" + response = await PythonExecuteStep()(code="pass", **{key: requested}) + assert response.success + assert backend.exec_shell.await_args.kwargs["timeout"] == expected + assert response.metadata["python_timeout"] == expected + + +@pytest.mark.asyncio +@pytest.mark.parametrize("requested", ["nan", float("nan"), "-inf", 0, -1, -(10**400), True, None, "bad"]) +async def test_invalid_timeout_never_executes(backend, requested): + """Invalid inputs fail before constructing or invoking an execution backend.""" + response = await PythonExecuteStep()(code="pass", python_timeout=requested) + assert not response.success + assert response.answer == "python_timeout must be a positive number" + backend.exec_shell.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("maximum", "expected"), [(30, 30), (120, 60), (100000, 60)]) +async def test_default_timeout_respects_server_cap(backend, maximum, expected): + """An omitted timeout retains 60 seconds unless the server requires less.""" + response = await PythonExecuteStep(max_python_timeout=maximum)(code="pass") + assert response.success + assert backend.exec_shell.await_args.kwargs["timeout"] == expected + assert response.metadata["python_timeout"] == expected + + +@pytest.mark.asyncio +@pytest.mark.parametrize("maximum", [0, -1, "nan", "-inf", None, True]) +async def test_invalid_server_cap_fails_closed(backend, maximum): + """A misconfigured server limit cannot silently become unlimited.""" + response = await PythonExecuteStep(max_python_timeout=maximum)(code="pass") + assert not response.success + assert response.answer == "max_python_timeout must be a positive number" + backend.exec_shell.assert_not_awaited() + + +@pytest.mark.parametrize(("maximum", "expected"), [(86400, 86400), (30, 30)]) +@pytest.mark.parametrize("key", ["python_timeout", "timeout"]) +def test_http_request_cannot_raise_server_timeout_cap(tmp_path, backend, maximum, expected, key): + """Exercise the real HTTP job route, including extra fields that bypass job schemas.""" + context = ApplicationContext(workspace_dir=str(tmp_path)) + job = BaseJob(name="python_review", app_context=context) + job.step_specs = [(PythonExecuteStep, {"app_context": context, "max_python_timeout": maximum})] + service = HttpService() + service.service = FastAPI() + service._add_json_job(job) # pylint: disable=protected-access + with TestClient(service.service) as client: + response = client.post( + "/python_review", + json={"code": "pass", key: "inf", "max_python_timeout": "inf"}, + ) + assert response.status_code == 200 + assert response.json()["success"] + assert response.json()["metadata"]["python_timeout"] == expected + assert backend.exec_shell.await_args.kwargs["timeout"] == expected