diff --git a/plugins/home/src/reme_home/auto_bash.py b/plugins/home/src/reme_home/auto_bash.py index a464103d..0c647800 100644 --- a/plugins/home/src/reme_home/auto_bash.py +++ b/plugins/home/src/reme_home/auto_bash.py @@ -29,6 +29,9 @@ _READ_COMMANDS = { "git": { "allow": ["status", "diff", "log", "show", "rev-parse", "ls-files"], "denied": [ + # Help dispatch can execute repository-configured man viewers. + "--help", + "-h", "--output", "--ext-diff", "--textconv", diff --git a/plugins/home/tests/test_auto_bash.py b/plugins/home/tests/test_auto_bash.py index 9ad9f466..cbd7817c 100644 --- a/plugins/home/tests/test_auto_bash.py +++ b/plugins/home/tests/test_auto_bash.py @@ -518,6 +518,55 @@ async def test_git_pretty_config_cannot_launch_gpg(tmp_path): assert not marker.exists() +@pytest.mark.asyncio +@pytest.mark.parametrize("subcommand", ["status", "diff", "log", "show", "rev-parse", "ls-files"]) +@pytest.mark.parametrize("flag", ["--help", "--hel", "--he", "--h", "-h"]) +async def test_git_help_flags_are_denied(tmp_path, monkeypatch, subcommand, flag): + """All allowlisted Git commands reject help before starting a process.""" + + async def unexpected_process(*_args, **_kwargs): + pytest.fail("Git help must be denied before execution") + + monkeypatch.setattr(asyncio, "create_subprocess_exec", unexpected_process) + tool = AutoBash(cwd=tmp_path) + command = f"git {subcommand} {flag}" + + assert not await tool.check_read_only({"command": command}) + chunks = [chunk async for chunk in tool.call(command)] + assert len(chunks) == 1 + assert chunks[0].state == ToolResultState.ERROR + assert "Permission denied" in chunks[0].content[0].text + + +@pytest.mark.asyncio +@pytest.mark.parametrize("subcommand", ["rev-parse", "ls-files"]) +async def test_git_help_cannot_run_configured_man_viewer(tmp_path, monkeypatch, subcommand): + """Help flags must not dispatch repository-configured commands on the host.""" + monkeypatch.delenv("GIT_MAN_VIEWER", raising=False) + + def git(*args): + return subprocess.run(["git", *args], cwd=tmp_path, check=True, capture_output=True) + + git("init", "-q") + marker = tmp_path / "denied.txt" + git("config", "man.viewer", "reme-test") + git("config", "man.reme-test.cmd", f'echo ran > "{marker}"') + # Prove that this fixture would execute the helper without the guard. + git(subcommand, "--help") + assert marker.exists() + marker.unlink() + tool = AutoBash(cwd=tmp_path) + command = f"git {subcommand} --help" + + chunks = [chunk async for chunk in tool.call(command)] + + assert not marker.exists() + assert not await tool.check_read_only({"command": command}) + assert len(chunks) == 1 + assert chunks[0].state == ToolResultState.ERROR + assert "Permission denied" in chunks[0].content[0].text + + @pytest.mark.asyncio @pytest.mark.parametrize("helper", ["fsmonitor", "diff", "textconv", "clean"]) async def test_git_configured_helpers_cannot_write(tmp_path, helper): diff --git a/reme/utils/file_sandbox/file_sandbox_backend.py b/reme/utils/file_sandbox/file_sandbox_backend.py index 220d0e4c..5dca6b46 100644 --- a/reme/utils/file_sandbox/file_sandbox_backend.py +++ b/reme/utils/file_sandbox/file_sandbox_backend.py @@ -82,8 +82,16 @@ class FileSandboxBackend(LocalProcessBackend): timeout: float | None = None, ) -> ExecResult: """Execute an argv list with a timeout inside the platform sandbox.""" - process = await self._start_process(command, cwd or self._cwd) - return await self._communicate(process, timeout=timeout) + return await self._run_job(command, cwd or self._cwd, data=None, timeout=timeout) + + async def _run_job(self, command: list[str], cwd: str, *, data: bytes | None, timeout: float | None) -> 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) async def read_file(self, path: str) -> bytes: """Read through a sandboxed command so file rules still apply.""" @@ -97,11 +105,11 @@ class FileSandboxBackend(LocalProcessBackend): if not isinstance(data, bytes): raise TypeError("data must be bytes") script = 'mkdir -p -- "$(dirname -- "$1")" && cat > "$1"' - process = await self._start_process( + result = await self._run_job( ["/bin/sh", "-c", script, "sh", path], self._cwd, - stdin=asyncio.subprocess.PIPE, + data=data, + timeout=None, ) - result = await self._communicate(process, data=data) if not result.ok(): raise OSError(result.stderr.decode(errors="replace")) diff --git a/reme/utils/file_sandbox/path_seatbelt_backend.py b/reme/utils/file_sandbox/path_seatbelt_backend.py index ee6a5c67..84d2b286 100644 --- a/reme/utils/file_sandbox/path_seatbelt_backend.py +++ b/reme/utils/file_sandbox/path_seatbelt_backend.py @@ -208,12 +208,7 @@ class PathSeatbeltBackend(FileSandboxBackend): RuntimeWarning, stacklevel=2, ) - 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 super()._run_job(command, cwd, data=data, timeout=timeout) control = Path(tempfile.mkdtemp(prefix="reme-seatbelt-", dir=os.path.realpath(tempfile.gettempdir()))) sandbox_tmp = control / "sandbox-tmp" sandbox_tmp.mkdir() @@ -304,26 +299,13 @@ class PathSeatbeltBackend(FileSandboxBackend): stderr=bytes(stderr), ) finally: - for reader in readers: - reader.cancel() - await asyncio.gather(*readers, return_exceptions=True) - for fd in output_fds: - os.close(fd) - shutil.rmtree(control) - async def exec_shell( - self, - command: list[str], - *, - cwd: str | None = None, - timeout: float | None = None, - ) -> ExecResult: - return await self._run_job(command, cwd or self._cwd, data=None, timeout=timeout) + async def release(): + for reader in readers: + reader.cancel() + await asyncio.gather(*readers, return_exceptions=True) + for fd in output_fds: + os.close(fd) + shutil.rmtree(control) - async def write_file(self, path: str, data: bytes) -> None: - if not isinstance(data, bytes): - raise TypeError("data must be bytes") - script = 'mkdir -p -- "$(dirname -- "$1")" && cat > "$1"' - result = await self._run_job(["/bin/sh", "-c", script, "sh", path], self._cwd, data=data, timeout=None) - if not result.ok(): - raise OSError(result.stderr.decode(errors="replace")) + await finish_cleanup(release()) diff --git a/reme/utils/local_backend.py b/reme/utils/local_backend.py index d8b48cb3..465ab44a 100644 --- a/reme/utils/local_backend.py +++ b/reme/utils/local_backend.py @@ -14,14 +14,21 @@ OUTPUT_LIMIT_BYTES = 1024 * 1024 async def finish_cleanup(operation: Awaitable): - """Finish rollback after cancellation, deferring additional cancellation requests.""" + """Finish cleanup before propagating any cancellation received while waiting.""" task = asyncio.ensure_future(operation) + cancelled = False while True: try: - return await asyncio.shield(task) + result = await asyncio.shield(task) + break except asyncio.CancelledError: + cancelled = True if task.done(): - return task.result() + result = task.result() + break + if cancelled: + raise asyncio.CancelledError + return result class OutputLimitExceeded(RuntimeError): @@ -117,7 +124,7 @@ class LocalProcessBackend(LocalBackend): return await task finally: task.cancel() - await asyncio.gather(task, return_exceptions=True) + await finish_cleanup(asyncio.gather(task, return_exceptions=True)) @staticmethod async def _collect_output(process, data, stdout, stderr, exceeded): @@ -150,7 +157,7 @@ class LocalProcessBackend(LocalBackend): finally: for task in tasks: task.cancel() - await asyncio.gather(*tasks, return_exceptions=True) + await finish_cleanup(asyncio.gather(*tasks, return_exceptions=True)) @staticmethod async def _execute(operation: Awaitable, *, timeout: float | None, cleanup: Callable[[bool], Awaitable[None]]): @@ -162,9 +169,13 @@ class LocalProcessBackend(LocalBackend): completed = True return result finally: - task.cancel() - await asyncio.gather(task, return_exceptions=True) - await cleanup(completed) + + async def finalize(): + task.cancel() + await asyncio.gather(task, return_exceptions=True) + await cleanup(completed) + + await finish_cleanup(finalize()) async def _communicate( self, diff --git a/tests/unit/test_file_sandbox_backend.py b/tests/unit/test_file_sandbox_backend.py index ad09c0ad..941cb97c 100644 --- a/tests/unit/test_file_sandbox_backend.py +++ b/tests/unit/test_file_sandbox_backend.py @@ -4,6 +4,7 @@ import asyncio import os import plistlib import sys +from contextlib import nullcontext from pathlib import Path import pytest @@ -110,6 +111,39 @@ def test_seatbelt_excludes_read_only_child_from_writable_parent(tmp_path): assert f'(allow file-write* (subpath "{output}"))' in profile +@pytest.mark.asyncio +@pytest.mark.parametrize("backend_class", [PathBubbleBackend, PathSeatbeltBackend]) +@pytest.mark.parametrize("data", [b"", b"hello\x00\n\xff"]) +async def test_sandbox_file_io_round_trip(tmp_path, monkeypatch, backend_class, data): + """The common file helpers preserve binary stdin, literal paths and write errors.""" + from unittest.mock import AsyncMock + + backend = backend_class(cwd=tmp_path, read_only_paths=[], writable_paths=[str(tmp_path)]) + # Exercise shared file I/O with real pipes; OS isolation is checked separately. + monkeypatch.setattr( + backend, + "_start_process", + LocalProcessBackend()._start_process, # pylint: disable=protected-access + ) + 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)) + warning = ( + pytest.warns(RuntimeWarning, match="No macOS GUI launchd domain") + if backend_class is PathSeatbeltBackend + else nullcontext() + ) + with warning: + path = str(tmp_path / "nested directory" / "literal ' ; name.bin") + await backend.write_file(path, data) + assert await backend.read_file(path) == data + with pytest.raises(TypeError, match="data must be bytes"): + await backend.write_file(path, "invalid") + with pytest.raises(OSError): + await backend.write_file(str(tmp_path), data) + + @pytest.mark.asyncio @pytest.mark.parametrize("sandbox", [False, True]) @pytest.mark.parametrize("cancel", [False, True]) @@ -221,7 +255,7 @@ async def test_python_job_selects_seatbelt_on_macos(tmp_path, monkeypatch): @pytest.mark.asyncio -@pytest.mark.parametrize("mode", ["success", "failure", "timeout", "cancel", "error", "kickstart_error"]) +@pytest.mark.parametrize("mode", ["success", "failure", "timeout", "cancel", "error", "kickstart_error", "write"]) async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): """Every launchd outcome unregisters the job and removes its control files.""" from unittest.mock import AsyncMock, Mock @@ -241,6 +275,10 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): calls.append(args[0]) if args[0] == "bootstrap": control = Path(args[-1]).parent + if mode == "write": + with open(args[-1], "rb") as stream: + job = plistlib.load(stream) + assert Path(job["StandardInPath"]).read_bytes() == b"binary\x00\xff" if args[0] == "kickstart" and mode == "kickstart_error": raise RuntimeError("kickstart failed") if args[0] == "bootout": @@ -259,7 +297,12 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): monkeypatch.setattr(backend, "_launchctl", launchctl) monkeypatch.setattr(backend, "_wait_status", wait_status) - task = asyncio.create_task(backend.exec_shell(["true"], timeout=0.01 if mode == "timeout" else None)) + 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) + ) + task = asyncio.create_task(operation) if mode == "cancel": await asyncio.wait_for(started.wait(), timeout=1) task.cancel() @@ -270,7 +313,9 @@ async def test_seatbelt_launchd_lifecycle(tmp_path, monkeypatch, mode): await asyncio.wait_for(task, timeout=1) else: result = await asyncio.wait_for(task, timeout=1) - if mode == "timeout": + if mode == "write": + assert result is None + elif mode == "timeout": assert result == ExecResult(exit_code=-1, stdout=b"", stderr=b"timed out") else: assert result.exit_code == (7 if mode == "failure" else 0) diff --git a/tests/unit/test_process_output_limits.py b/tests/unit/test_process_output_limits.py index 47242925..b14f4f6f 100644 --- a/tests/unit/test_process_output_limits.py +++ b/tests/unit/test_process_output_limits.py @@ -14,38 +14,21 @@ import pytest from reme.components.application_context import ApplicationContext from reme.steps.common.python_execute import PythonExecuteStep -from reme.utils.file_sandbox.path_bubble_backend import PathBubbleBackend from reme.utils.file_sandbox.path_seatbelt_backend import PathSeatbeltBackend from reme.utils.local_backend import LocalProcessBackend, OUTPUT_LIMIT_BYTES @pytest.mark.asyncio -@pytest.mark.parametrize("backend_name", ["local", "bubble", "seatbelt"]) @pytest.mark.parametrize("stream", ["stdout", "stderr", "both"]) @pytest.mark.parametrize("extra", [0, 1]) -async def test_pipe_output_boundary(tmp_path, monkeypatch, backend_name, stream, extra): +async def test_pipe_output_boundary(tmp_path, stream, extra): """Each stream has its own budget; crossing either budget fails execution.""" - local = LocalProcessBackend() - backend = local - if backend_name != "local": - backend_class = PathBubbleBackend if backend_name == "bubble" else PathSeatbeltBackend - backend = backend_class(cwd=tmp_path, read_only_paths=[], writable_paths=[]) - # Exercise the shared collector with a real process, without requiring - # kernel sandbox support or changing the host launchd domain. - monkeypatch.setattr(backend, "_start_process", local._start_process) - if backend_name == "seatbelt": - monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.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)) + backend = LocalProcessBackend() size = OUTPUT_LIMIT_BYTES + extra code = "import os;" + ";".join( f"os.write({fd}, b'x' * {size})" for fd in (1, 2) if stream in ("both", "stdout" if fd == 1 else "stderr") ) - if backend_name == "seatbelt": - with pytest.warns(RuntimeWarning, match="No macOS GUI launchd domain"): - result = await backend.exec_shell([sys.executable, "-c", code], timeout=5) - else: - result = await backend.exec_shell([sys.executable, "-c", code], timeout=5) + result = await backend.exec_shell([sys.executable, "-c", code], cwd=str(tmp_path), timeout=5) assert len(result.stdout) <= OUTPUT_LIMIT_BYTES assert len(result.stderr) <= OUTPUT_LIMIT_BYTES if extra: diff --git a/tests/unit/test_sandbox_startup_cancellation.py b/tests/unit/test_sandbox_startup_cancellation.py index 88783383..fe65ed61 100644 --- a/tests/unit/test_sandbox_startup_cancellation.py +++ b/tests/unit/test_sandbox_startup_cancellation.py @@ -1,9 +1,10 @@ -"""Cancellation during startup must not abandon OS-owned sandbox resources.""" +"""Cancellation during startup or shutdown must not abandon sandbox resources.""" # pylint: disable=protected-access import asyncio import errno +import os import sys from pathlib import Path from unittest.mock import AsyncMock @@ -12,6 +13,167 @@ import pytest from reme.utils.file_sandbox.path_bubble_backend import PathBubbleBackend from reme.utils.file_sandbox.path_seatbelt_backend import LaunchctlError, PathSeatbeltBackend +from reme.utils.local_backend import LocalProcessBackend + + +@pytest.mark.asyncio +async def test_repeated_cancellation_finishes_fifo_resource_release(tmp_path, monkeypatch): + """Even setup failure must drain reader finalizers before closing control resources.""" + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.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=True)) + started = asyncio.Event() + stopping = asyncio.Event() + reply = asyncio.Event() + fds = set() + stopped = set() + control = None + + async def read_fifo(fd, *_args): + fds.add(fd) + if len(fds) == 2: + started.set() + try: + await asyncio.Event().wait() + finally: + stopping.set() + await reply.wait() + stopped.add(fd) + + async def bootstrap(_domain, plist, _service): + nonlocal control + control = plist.parent + await started.wait() + raise RuntimeError("bootstrap failed") + + monkeypatch.setattr(backend, "_read_fifo", read_fifo) + monkeypatch.setattr(backend, "_bootstrap", bootstrap) + task = asyncio.create_task(backend.exec_shell(["true"])) + try: + await asyncio.wait_for(stopping.wait(), timeout=1) + for _ in range(2): + task.cancel() + await asyncio.sleep(0) + await asyncio.sleep(0) + assert not task.done() + assert control.exists() + reply.set() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(task, timeout=1) + assert stopped == fds + assert not control.exists() + for fd in fds: + with pytest.raises(OSError) as exc: + os.fstat(fd) + assert exc.value.errno == errno.EBADF + finally: + reply.set() + await asyncio.gather(task, return_exceptions=True) + + +@pytest.mark.asyncio +async def test_repeated_cancellation_waits_for_operation_and_backend_cleanup(): + """Repeated cancellation cannot interrupt the operation's asynchronous finalizer.""" + entered = asyncio.Event() + stopping = asyncio.Event() + stopped = asyncio.Event() + reply = asyncio.Event() + released = [] + + async def operation(): + try: + entered.set() + await asyncio.Event().wait() + finally: + stopping.set() + await reply.wait() + stopped.set() + + async def cleanup(completed): + assert stopped.is_set() + released.append(completed) + + task = asyncio.create_task(LocalProcessBackend._execute(operation(), timeout=None, cleanup=cleanup)) + try: + await asyncio.wait_for(entered.wait(), timeout=1) + task.cancel() + await asyncio.wait_for(stopping.wait(), timeout=1) + task.cancel() + await asyncio.sleep(0) + await asyncio.sleep(0) + assert not task.done() + reply.set() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(task, timeout=1) + assert released == [False] + finally: + reply.set() + await asyncio.gather(task, return_exceptions=True) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("outcome", ["cancel", "success", "timeout"]) +async def test_execution_cleanup_unregisters_despite_repeated_cancellation(tmp_path, monkeypatch, outcome): + """Keep launchd control files until bootout finishes even after repeated cancellation.""" + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + monkeypatch.setattr("reme.utils.file_sandbox.path_seatbelt_backend.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=True)) + monkeypatch.setattr(backend, "_kill_process", lambda _process: None) + running = asyncio.Event() + bootout_entered = asyncio.Event() + bootout_reply = asyncio.Event() + services = set() + control = None + + async def launchctl(*args): + nonlocal control + if args[0] == "bootstrap": + control = Path(args[-1]).parent + services.add("sandbox") + elif args[0] == "kickstart": + return "12345" + elif args[0] == "bootout": + assert control.exists() + bootout_entered.set() + await bootout_reply.wait() + assert control.exists() + services.clear() + else: + pytest.fail(f"Unexpected launchctl call: {args[0]}") + return "" + + async def wait_status(*_args): + running.set() + if outcome == "success": + return 0 + await asyncio.Event().wait() + return 0 + + monkeypatch.setattr(backend, "_launchctl", launchctl) + monkeypatch.setattr(backend, "_wait_status", wait_status) + task = asyncio.create_task(backend.exec_shell(["true"], timeout=0.05 if outcome == "timeout" else None)) + try: + await asyncio.wait_for(running.wait(), timeout=1) + if outcome == "cancel": + task.cancel() + await asyncio.wait_for(bootout_entered.wait(), timeout=1) + for _ in range(1 if outcome == "cancel" else 2): + task.cancel() + await asyncio.sleep(0) + await asyncio.sleep(0) + assert not task.done() + assert services + assert control.exists() + bootout_reply.set() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(task, timeout=1) + assert not services + assert not control.exists() + finally: + bootout_reply.set() + await asyncio.gather(task, return_exceptions=True) @pytest.mark.asyncio