fix(home): block Git help helpers and harden sandbox cleanup

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",

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):

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"))

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())

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,

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)

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:

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