diff --git a/astrbot/core/computer/booters/local.py b/astrbot/core/computer/booters/local.py index 66ad3b26d..fb2329aeb 100644 --- a/astrbot/core/computer/booters/local.py +++ b/astrbot/core/computer/booters/local.py @@ -1,19 +1,21 @@ from __future__ import annotations import asyncio -import hashlib import locale import os import shutil import signal import subprocess import sys +import tempfile +import threading import time import uuid +from _thread import LockType from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path -from typing import Any, cast +from typing import Any, BinaryIO, cast if sys.version_info < (3, 14): import python_ripgrep @@ -140,7 +142,8 @@ class _LocalShellSession: creator_is_admin: bool sandboxed: bool process: SandboxProcess | asyncio.subprocess.Process - output_path: Path + output_file: BinaryIO + output_lock: LockType started_at: float output_event: asyncio.Event reader_task: asyncio.Task[None] @@ -314,19 +317,18 @@ class LocalShellComponent(ShellComponent): working_dir = Path(cwd).resolve() if cwd else Path(get_astrbot_root()).resolve() session_id = f"sh_{uuid.uuid4().hex[:16]}" - owner_digest = hashlib.sha256(owner_id.encode("utf-8")).hexdigest()[:16] - output_dir = Path(get_astrbot_system_tmp_path()) / "shell" / owner_digest + output_dir = Path(get_astrbot_system_tmp_path()) output_dir.mkdir(parents=True, exist_ok=True) - output_path = output_dir / f"{session_id}.log" - output_path.touch() - # Configuration invalidation must also see processes still being spawned. async with self._sessions_lock: if not permission_check(): - output_path.unlink(missing_ok=True) raise PermissionError( "Local shell permissions changed; retry the command." ) + # Shared temporary roots are writable by sandboxed processes. Keep + # output on an anonymous handle to prevent redirecting host I/O. + output_file = tempfile.TemporaryFile(mode="w+b", dir=output_dir) + output_lock = threading.Lock() try: if sandboxed: process = await create_process_sandbox().spawn_shell( @@ -376,8 +378,8 @@ class LocalShellComponent(ShellComponent): stderr=asyncio.subprocess.STDOUT, **process_kwargs, ) - except Exception: - output_path.unlink(missing_ok=True) + except BaseException: + output_file.close() raise output_event = asyncio.Event() @@ -386,24 +388,25 @@ class LocalShellComponent(ShellComponent): if process.stdout is None: return output_size = 0 - with output_path.open("ab") as output_file: - while chunk := await process.stdout.read(8192): - if sandboxed: - remaining = _LOCAL_SANDBOX_MAX_OUTPUT_BYTES - output_size - if remaining <= 0: - session.output_limited = True - process.terminate() - return - if len(chunk) > remaining: - chunk = chunk[:remaining] - session.output_limited = True - output_file.write(chunk) - output_file.flush() - output_size += len(chunk) - output_event.set() - if session.output_limited: + while chunk := await process.stdout.read(8192): + if sandboxed: + remaining = _LOCAL_SANDBOX_MAX_OUTPUT_BYTES - output_size + if remaining <= 0: + session.output_limited = True process.terminate() return + if len(chunk) > remaining: + chunk = chunk[:remaining] + session.output_limited = True + with output_lock: + output_file.seek(0, os.SEEK_END) + output_file.write(chunk) + output_file.flush() + output_size += len(chunk) + output_event.set() + if session.output_limited: + process.terminate() + return reader_task = asyncio.create_task( _capture_output(), @@ -421,7 +424,8 @@ class LocalShellComponent(ShellComponent): creator_is_admin=creator_is_admin, sandboxed=sandboxed, process=process, - output_path=output_path, + output_file=output_file, + output_lock=output_lock, started_at=time.time(), output_event=output_event, reader_task=reader_task, @@ -528,8 +532,8 @@ class LocalShellComponent(ShellComponent): ) ) try: - output_size = session.output_path.stat().st_size - except OSError: + output_size = os.fstat(session.output_file.fileno()).st_size + except (OSError, ValueError): output_size = session.cursor items.append( { @@ -588,14 +592,13 @@ class LocalShellComponent(ShellComponent): raise ValueError("`cursor` must be greater than or equal to 0.") def _read_output() -> tuple[bytes, int, int]: - try: - output_size = session.output_path.stat().st_size - except FileNotFoundError: - return b"", read_cursor, read_cursor - normalized_cursor = min(read_cursor, output_size) - with session.output_path.open("rb") as output_file: - output_file.seek(normalized_cursor) - raw_output = output_file.read(max_output_chars) + with session.output_lock: + if session.output_file.closed: + return b"", read_cursor, read_cursor + output_size = os.fstat(session.output_file.fileno()).st_size + normalized_cursor = min(read_cursor, output_size) + session.output_file.seek(normalized_cursor) + raw_output = session.output_file.read(max_output_chars) return ( raw_output, normalized_cursor + len(raw_output), @@ -956,11 +959,8 @@ class LocalShellComponent(ShellComponent): await timeout_task except asyncio.CancelledError: pass - session.output_path.unlink(missing_ok=True) - try: - session.output_path.parent.rmdir() - except OSError: - pass + with session.output_lock: + session.output_file.close() @dataclass diff --git a/astrbot/core/tools/message_tools.py b/astrbot/core/tools/message_tools.py index eb34c3122..344e0d3c7 100644 --- a/astrbot/core/tools/message_tools.py +++ b/astrbot/core/tools/message_tools.py @@ -179,6 +179,12 @@ class SendMessageToUserTool(FunctionTool[AstrAgentContext]): f"Blocked path: {local_candidate}." ) + # Local runtime has no separate sandbox: the workspace and local-file + # branches above already enforced the caller's permissions, so probing + # the host shell here would bypass them and expose host paths. + if is_local_runtime(context): + raise FileNotFoundError(f"{component_type} path does not exist: {path}") + try: sb = await get_booter( context.context.context, diff --git a/tests/test_local_sandbox_access.py b/tests/test_local_sandbox_access.py index 376b9775b..ad9bbab6e 100644 --- a/tests/test_local_sandbox_access.py +++ b/tests/test_local_sandbox_access.py @@ -25,7 +25,6 @@ from astrbot.core.computer.process_sandbox import ( from astrbot.core.tools.computer_tools import fs, python, shell from astrbot.dashboard.services import stat_service - requires_local_sandbox = pytest.mark.skipif( not ( (sys.platform.startswith("linux") and shutil.which("bwrap")) @@ -35,6 +34,50 @@ requires_local_sandbox = pytest.mark.skipif( ) +@requires_local_sandbox +@pytest.mark.asyncio +async def test_managed_shell_output_cannot_be_redirected_outside_sandbox( + monkeypatch, tmp_path +): + """Keep host output reads on the original handle after shared-path attacks.""" + workspace = tmp_path / "workspace" + shared_temp = tmp_path / "shared-temp" + workspace.mkdir() + shared_temp.mkdir() + secret = tmp_path / "host-only.txt" + secret.write_text("host-only marker", encoding="utf-8") + monkeypatch.setattr(local, "get_astrbot_system_tmp_path", lambda: str(shared_temp)) + code = f""" +from pathlib import Path + +secret = Path({str(secret)!r}) +assert not secret.exists(), "The host file must be outside the sandbox." +for log in Path({str(shared_temp)!r}).rglob("sh_*.log"): + log.unlink() + log.symlink_to(secret) +print("original process output") +""" + component = local.LocalShellComponent() + try: + result = await component.exec_managed( + shlex.join(["python", "-c", code]), + owner_id="test:GroupMessage:output", + creator_id="member", + creator_is_admin=False, + sandboxed=True, + permission_check=lambda: True, + cwd=str(workspace), + writable_roots=(shared_temp,), + timeout=10, + ) + assert result["exit_code"] == 0, result + assert result["stdout"] == "original process output\n" + assert result["session_closed"] is True + assert secret.read_text(encoding="utf-8") == "host-only marker" + finally: + await component.shutdown_sessions() + + @requires_local_sandbox def test_runtime_probe_launches_native_sandbox(monkeypatch, tmp_path): """Verify the dashboard startup check against each platform's real backend.""" @@ -96,7 +139,9 @@ def test_local_sandbox_protects_venv_inside_writable_roots( attachments = tmp_path / "attachments" workspace.mkdir() attachments.mkdir() - parent = {"workspace": workspace, "writable_root": attachments}.get(location, tmp_path) + parent = {"workspace": workspace, "writable_root": attachments}.get( + location, tmp_path + ) prefix = parent / ".venv" base_python = Path(sys._base_executable) subprocess.run( @@ -166,7 +211,9 @@ print("virtualenv is read-only") [str(executable), "-c", code], SandboxSpec( workspace=workspace, - writable_roots=(package.parent if location == "writable_packages" else attachments,), + writable_roots=( + package.parent if location == "writable_packages" else attachments, + ), ), timeout=15, ) diff --git a/tests/test_local_shell_component.py b/tests/test_local_shell_component.py index 324fc2e14..a002cd525 100644 --- a/tests/test_local_shell_component.py +++ b/tests/test_local_shell_component.py @@ -1268,6 +1268,7 @@ async def test_managed_shell_cleans_children_after_leader_exits(): session.permission_check = None await asyncio.wait_for(shell.shutdown_sessions(invalid_only=True), 8) assert session.reader_task.done() + assert session.output_file.closed assert not shell._sessions finally: await shell.shutdown_sessions() @@ -1510,6 +1511,7 @@ async def test_managed_shell_hard_timeout_terminates_session(): timeout=1, yield_time_ms=0, ) + session = shell._sessions[result["session_id"]] try: timed_out = await shell.poll_session( @@ -1523,6 +1525,7 @@ async def test_managed_shell_hard_timeout_terminates_session(): assert timed_out["status"] == "timed_out" assert timed_out["exit_code"] is not None assert timed_out["session_closed"] is True + assert session.output_file.closed finally: await shell.shutdown_sessions() @@ -1540,10 +1543,12 @@ async def test_managed_shell_keeps_completed_session_until_output_is_drained(): yield_time_ms=5_000, max_output_chars=10_000, ) + session = shell._sessions[result["session_id"]] try: assert result["status"] == "completed" assert result["has_more"] is True + assert not session.output_file.closed output = result["stdout"] while result["has_more"]: result = await shell.poll_session( @@ -1557,6 +1562,7 @@ async def test_managed_shell_keeps_completed_session_until_output_is_drained(): assert output.splitlines() == ["x" * 25000] assert result["session_closed"] is True + assert session.output_file.closed assert await shell.list_sessions( owner_id="owner-a", requester_id="user-a", diff --git a/tests/unit/test_message_tools.py b/tests/unit/test_message_tools.py index 419b546e6..70fda2ccf 100644 --- a/tests/unit/test_message_tools.py +++ b/tests/unit/test_message_tools.py @@ -291,7 +291,9 @@ async def test_send_message_empty_messages_returns_error(): async def test_send_message_missing_image_path_stops_before_send(tmp_path, monkeypatch): """Missing image paths fail before sending any message components.""" tool = SendMessageToUserTool() - ctx = _make_context() + # Sandbox runtime so the booter is still consulted for missing paths; + # local runtime now rejects them before any booter call. + ctx = _make_context(runtime="sandbox") missing_image_path = tmp_path / "missing.png" async def mock_get_booter(*args, **kwargs): @@ -525,3 +527,55 @@ async def test_send_message_downloads_trailing_slash_sandbox_file_with_basename( sent_chain = ctx.context.context.send_message.await_args.args[1] sent_file = sent_chain.chain[0] assert sent_file.name == "export" + + +@pytest.mark.asyncio +async def test_send_message_local_runtime_skips_sandbox_file_probe( + tmp_path, monkeypatch +): + """Local runtime must resolve send-file paths only via permission-checked branches. + + Falling through to the booter branch would probe the host shell without + the caller's filesystem permissions. + """ + workspace = tmp_path / "workspace" + workspace.mkdir() + secret = tmp_path / "host-only.txt" + secret.write_text("host-only marker", encoding="utf-8") + allowed = workspace / "allowed.txt" + allowed.write_text("workspace file", encoding="utf-8") + + async def mock_workspace_root_for_context(_context): + return workspace + + async def mock_get_booter(*args, **kwargs): + del args, kwargs + raise AssertionError("local runtime must not query a sandbox booter") + + monkeypatch.setattr( + "astrbot.core.tools.message_tools.workspace_root_for_context", + mock_workspace_root_for_context, + ) + monkeypatch.setattr( + "astrbot.core.tools.message_tools.get_booter", + mock_get_booter, + ) + + tool = SendMessageToUserTool() + ctx = _make_context( + role="member", + local_permissions={ + "member": { + "allow_execution": False, + "allow_network": False, + "filesystem_scope": "workspace", + } + }, + ) + + resolved, downloaded = await tool._resolve_path_from_sandbox(ctx, "allowed.txt") + assert resolved == str(allowed.resolve()) + assert downloaded is False + + with pytest.raises(FileNotFoundError): + await tool._resolve_path_from_sandbox(ctx, "host-only.txt")