diff --git a/routes/document/document_helpers.py b/routes/document/document_helpers.py index a0c2d08eb..3c0f6d45f 100644 --- a/routes/document/document_helpers.py +++ b/routes/document/document_helpers.py @@ -13,6 +13,7 @@ from pydantic import BaseModel from core.database import Document, DocumentVersion from core.database import Session as DbSession from src.auth_helpers import _auth_disabled +from src.path_confinement import is_inside from src.upload_handler import UploadHandler logger = logging.getLogger(__name__) @@ -136,12 +137,7 @@ _PDF_RENDER_SCALE = 2.0 def _upload_path_inside(upload_dir: str, path: str) -> bool: - base = os.path.realpath(upload_dir) - p = os.path.realpath(path) - try: - return os.path.commonpath([base, p]) == base - except Exception: - return False + return is_inside(upload_dir, path) def _resolve_user_upload_path( diff --git a/routes/email/email_routes.py b/routes/email/email_routes.py index 9422221dc..e5b0d3224 100644 --- a/routes/email/email_routes.py +++ b/routes/email/email_routes.py @@ -39,6 +39,7 @@ from email.mime.multipart import MIMEMultipart from fastapi import APIRouter, Query, UploadFile, File, BackgroundTasks, HTTPException, Depends, Request from fastapi.responses import FileResponse, StreamingResponse from src.constants import DATA_DIR +from src.path_confinement import confine from src.llm_core import llm_call_async from src.upload_limits import read_upload_limited, EMAIL_COMPOSE_UPLOAD_MAX_BYTES @@ -4199,9 +4200,12 @@ def setup_email_routes(): return {"error": f"Attachment index {index} not found"} from pathlib import Path as _Path - target_root = os.path.abspath(str(target_dir)) - filepath_str = os.path.abspath(str(filepath)) - if os.path.commonpath([target_root, filepath_str]) != target_root: + # realpath, not abspath: abspath only folds `..`, so a symlink + # written into the extraction directory would have passed this + # check and then been read through. + try: + filepath_str = confine(str(target_dir), str(filepath)) + except (ValueError, OSError): logger.warning("Rejected attachment path outside extraction dir: %s", filepath) return {"error": "Invalid attachment path"} filepath = _Path(filepath_str) diff --git a/routes/gallery/gallery_routes.py b/routes/gallery/gallery_routes.py index 63de56975..276ac90a5 100644 --- a/routes/gallery/gallery_routes.py +++ b/routes/gallery/gallery_routes.py @@ -21,6 +21,7 @@ from src.upload_limits import ( GALLERY_TRANSFORM_UPLOAD_MAX_BYTES, ) from src.constants import GENERATED_IMAGES_DIR +from src.path_confinement import confine from src.optional_deps import patch_realesrgan_torchvision_compat from routes.gallery.gallery_helpers import ( @@ -235,12 +236,9 @@ def _gallery_image_path(filename: str) -> Path: raise HTTPException(400, "Unsafe gallery filename") safe_name = _sanitize_gallery_filename(filename) original = str(filename or "") - root = GALLERY_IMAGE_DIR.resolve() - path = (GALLERY_IMAGE_DIR / safe_name).resolve() try: - if os.path.commonpath([str(root), str(path)]) != str(root): - raise ValueError - except Exception: + path = Path(confine(GALLERY_IMAGE_DIR, safe_name, allow_root=False)) + except (ValueError, OSError): raise HTTPException(400, "Unsafe gallery filename") if safe_name != original: raise HTTPException(400, "Unsafe gallery filename") diff --git a/routes/personal_routes.py b/routes/personal_routes.py index 3cf6c1d9d..278a95931 100644 --- a/routes/personal_routes.py +++ b/routes/personal_routes.py @@ -13,6 +13,7 @@ from core.constants import BASE_DIR, PERSONAL_DIR, PERSONAL_UPLOADS_DIR from src.rag_singleton import get_rag_manager from src.auth_helpers import require_privilege, require_user from core.middleware import require_admin +from src.path_confinement import confine from src.upload_handler import secure_filename from src.upload_limits import PERSONAL_UPLOAD_MAX_BYTES @@ -23,10 +24,7 @@ logger = logging.getLogger(__name__) def _personal_upload_dir_for_owner(owner: str | None, *, create: bool = True) -> str: """Return the per-owner upload directory used for direct RAG uploads.""" owner_segment = secure_filename((owner or "local").strip())[:80] or "local" - upload_dir = os.path.abspath(os.path.join(UPLOADS_DIR, owner_segment)) - base_abs = os.path.abspath(UPLOADS_DIR) - if os.path.commonpath([upload_dir, base_abs]) != base_abs: - raise ValueError("Unsafe upload owner path") + upload_dir = confine(UPLOADS_DIR, owner_segment, allow_root=False) if create: os.makedirs(upload_dir, exist_ok=True) return upload_dir @@ -41,10 +39,7 @@ def _unique_personal_upload_path(upload_dir: str, original_name: str | None) -> stem, ext = os.path.splitext(safe_name) stem = (stem or "upload")[:80] filename = f"{stem}-{uuid.uuid4().hex[:10]}{ext.lower()}" - file_path = os.path.abspath(os.path.join(upload_dir, filename)) - upload_abs = os.path.abspath(upload_dir) - if os.path.commonpath([file_path, upload_abs]) != upload_abs: - raise ValueError("Unsafe upload filename") + file_path = confine(upload_dir, filename, allow_root=False) return file_path, filename, safe_name @@ -167,19 +162,10 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available): if not directory: raise HTTPException(400, "Directory path is required") - # realpath (not abspath) so a symlink inside PERSONAL_DIR that points - # outside it is resolved before the commonpath confinement check below; - # abspath only normalises `..` and would let such a symlink escape. - base_abs = os.path.realpath(PERSONAL_DIR) - candidate = directory if os.path.isabs(directory) else os.path.join(base_abs, directory) - resolved = os.path.realpath(candidate) try: - in_base = os.path.commonpath([resolved, base_abs]) == base_abs - except ValueError: - in_base = False - if not in_base: + return confine(PERSONAL_DIR, directory) + except (ValueError, OSError): raise HTTPException(403, "Directory must be inside personal documents") - return resolved @router.get("") def api_personal_list(owner: str = Depends(require_user), _admin: None = Depends(require_admin)): @@ -425,17 +411,17 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available): # Scope to the per-owner subdir, not the shared uploads root, so one # admin can't delete another user's personal files by path. deleted_from_disk = False + # allow_root=False: the per-owner upload directory itself is + # never a deletion target, only files under it. try: - abs_target = os.path.realpath(filepath) - base_abs = os.path.realpath(_personal_upload_dir_for_owner(owner, create=False)) - in_uploads = ( - abs_target == base_abs - or os.path.commonpath([abs_target, base_abs]) == base_abs + abs_target = confine( + _personal_upload_dir_for_owner(owner, create=False), + filepath, + allow_root=False, ) - except ValueError: - # commonpath raises on mixed drives / non-comparable paths - in_uploads = False - if in_uploads and abs_target != base_abs: + except (ValueError, OSError): + abs_target = "" + if abs_target: try: os.remove(abs_target) deleted_from_disk = True diff --git a/routes/upload_routes.py b/routes/upload_routes.py index 93b91cf6d..0978f2217 100644 --- a/routes/upload_routes.py +++ b/routes/upload_routes.py @@ -24,6 +24,7 @@ from core.database import ( from src.auth_helpers import effective_user from src.attachment_refs import attachment_refs_from_metadata from src.constants import GENERATED_IMAGES_DIR +from src.path_confinement import is_inside from src.upload_handler import ( UploadCleanupSafetyError, count_recent_uploads, @@ -152,10 +153,7 @@ def setup_upload_routes(upload_handler): return os.path.realpath(getattr(upload_handler, "upload_dir", UPLOAD_DIR)) def _path_inside_upload_dir(path: str) -> bool: - try: - return os.path.commonpath([_upload_root(), os.path.realpath(path)]) == _upload_root() - except Exception: - return False + return is_inside(_upload_root(), path) def _resolve_upload_path(file_id: str) -> str: from src.constants import UPLOAD_DIR diff --git a/services/memory/skills.py b/services/memory/skills.py index a8882d5ad..713ae2199 100644 --- a/services/memory/skills.py +++ b/services/memory/skills.py @@ -25,6 +25,8 @@ import os import time from typing import Dict, Iterable, List, Optional +from src.path_confinement import confine + from .skill_format import Skill, slugify logger = logging.getLogger(__name__) @@ -644,9 +646,14 @@ class SkillsManager: or (sk.source == "builtin" and not (sk.owner or "")) ): continue - base = os.path.realpath(os.path.dirname(path)) - target = os.path.realpath(os.path.join(base, ref_path)) - if os.path.commonpath([base, target]) != base or target == os.path.dirname(path): + # allow_root=False refuses the skill directory itself. The old + # guard compared a realpath-ed target against a raw dirname, so on + # a host where the skills tree is reached through a symlink (macOS + # /tmp -> /private/tmp) the two sides never matched and the guard + # could not fire. + try: + target = confine(os.path.dirname(path), ref_path, allow_root=False) + except (ValueError, OSError): return None if not os.path.isfile(target): return None diff --git a/src/agent_tools/filesystem_tools.py b/src/agent_tools/filesystem_tools.py index be0cd9f90..89fd30975 100644 --- a/src/agent_tools/filesystem_tools.py +++ b/src/agent_tools/filesystem_tools.py @@ -9,6 +9,7 @@ import tempfile from typing import Optional, Dict, Any, Tuple, List from src.constants import MAX_READ_CHARS, MAX_DIFF_LINES, MAX_OUTPUT_CHARS +from src.path_confinement import is_inside _CODENAV_SKIP_DIRS = frozenset({ ".git", ".hg", ".svn", "node_modules", "venv", ".venv", "__pycache__", @@ -675,13 +676,7 @@ class GlobTool: # confinement that _resolve_search_root applies to the root. # An escaping literal falls through to the walk, which only ever # yields paths under base. - nbase = os.path.normcase(rbase) - try: - inside = cand == rbase or os.path.commonpath( - [os.path.normcase(cand), nbase] - ) == nbase - except ValueError: - inside = False + inside = is_inside(rbase, cand) # A literal that names a deny-listed sensitive file (.env, # .ssh/id_rsa, …) falls through to the walk, which skips it — # otherwise glob would surface secret paths that read_file / diff --git a/src/agent_tools/subprocess_tools.py b/src/agent_tools/subprocess_tools.py index 7537c82b5..559cc037e 100644 --- a/src/agent_tools/subprocess_tools.py +++ b/src/agent_tools/subprocess_tools.py @@ -1,5 +1,6 @@ import asyncio import ast +import logging import os import re import shlex @@ -15,9 +16,12 @@ from urllib.parse import urlparse import httpx -from src.constants import MAX_OUTPUT_CHARS +from src import containment +from src.constants import AGENT_ISOLATED_TMP_DIRNAME, MAX_OUTPUT_CHARS, WORKSPACE_MOUNT from src.agent_runtime.journal import mark_operation_started +logger = logging.getLogger(__name__) + # Agent shell calls must fail fast enough for the loop to recover and choose a # better tool. A one-hour default can pin an entire benchmark worker on an # accidental recursive scan, even though ordinary artifact commands complete @@ -227,11 +231,221 @@ def _replace_workspace_alias(content: str, cwd: str) -> str: ) +#: Roots the namespace argv mounts itself. A host path under one of these is +#: already reachable inside the namespace, so it needs no bind and must not get +#: a ``--dir`` chain: mkdir inside a read-only bind fails and takes the whole +#: namespace with it. +_NAMESPACE_MOUNTED_ROOTS = ("/usr", "/home", "/mnt") + +#: Destinations a bind must never overlay. Replacing the private root, the +#: private /tmp or the workspace mount with a host directory undoes the +#: namespace from inside the argv that builds it. +_NAMESPACE_RESERVED_DESTS = frozenset({ + "/", "/tmp", "/var", "/opt", "/etc", WORKSPACE_MOUNT, + "/root", "/run", "/proc", "/dev", "/sys", *_NAMESPACE_MOUNTED_ROOTS, +}) + + +def _namespace_visible_without_bind(path: str) -> bool: + """True when ``path`` is already reachable through a root the argv mounts.""" + return any( + path == root or path.startswith(root + os.sep) + for root in _NAMESPACE_MOUNTED_ROOTS + ) + + +def _namespace_dir_chain(path: str) -> list[str]: + """``--dir`` args for every ancestor of ``path`` the argv has to create. + + bwrap mounts into a tmpfs root, so a bind destination's parents have to + exist before the bind. Returns nothing when the parents already exist by + virtue of a mount the argv made — creating a directory inside a read-only + bind is an error, not a no-op. + """ + if _namespace_visible_without_bind(path): + return [] + parents: list[str] = [] + parent = os.path.dirname(path) + while parent not in ("/", "", "/tmp", "/etc", WORKSPACE_MOUNT, *_NAMESPACE_MOUNTED_ROOTS): + parents.append(parent) + parent = os.path.dirname(parent) + args: list[str] = [] + for directory in reversed(parents): + args.extend(("--dir", directory)) + return args + + +def _isolated_tmp_dir(cwd: str) -> str: + """The workspace-local stand-in for the host ``/tmp``. + + Creation is best-effort: the source tree is read-only in Docker and a + workspace can be mounted read-only, and a command that mentions ``/tmp/`` + must not die with an OSError traceback because a scratch directory could + not be made. The rewrite still points at the workspace, so a command that + really needs to write there fails on its own terms, inside the boundary, + with its own error message. + """ + path = os.path.join(cwd, AGENT_ISOLATED_TMP_DIRNAME) + try: + os.makedirs(path, exist_ok=True) + except OSError: + pass + return path + + +def _execution_boundary( + cwd: str, *, wall_clock_s: int = DEFAULT_BASH_TIMEOUT, +) -> "containment.ContainmentProbe": + """What this host can actually enforce for an agent command in ``cwd``. + + The single place the shell and Python tools ask. Both used to decide for + themselves, by testing whether a namespace wrapper came back non-None, and + both then fell through to a regex if it had not — so "was that command + confined" had no answer and no field in the result. Routing the question + through :mod:`src.containment` means one mechanism table, one answer, and a + ``containment`` block in the tool result either way. + + ``network`` is left inherited on purpose: ``--unshare-net`` was measured to + cut the loopback sidecars this product depends on (ChromaDB on 8100), and + the Dockerfile installs ``nmap``/``iproute2``/``dnsutils`` because + Docker-hosted agents are expected to do LAN work. It is a reported + dimension here, not an enforced one. + """ + try: + return containment.probe( + containment.agent_spec( + workspace=cwd, + env={}, + wall_clock_s=wall_clock_s, + max_output_bytes=MAX_OUTPUT_CHARS, + ) + ) + except ValueError as exc: + # A workspace that is not a usable directory is a caller bug to + # containment, which raises rather than reporting. Here it must not + # take out the tool, and it is still a containment failure: nothing can + # be confined to a directory that is not there. Fail closed. The reason + # goes in the message rather than a traceback -- this is a known shape, + # not an unexpected exception. + logger.warning( + "execution boundary: cannot probe containment for workspace %r (%s); " + "treating every required dimension as unenforced", + cwd, exc, + ) + return containment.ContainmentProbe( + mechanism="none", + enforced=frozenset(), + degraded=(), + unenforced_required=tuple(sorted(containment.DEFAULT_REQUIRED)), + mode=containment.CONTAINMENT_MODE, + ) + + +#: What the fallback actually is, named so it cannot be mistaken for a +#: mechanism. ``_replace_workspace_alias`` rewrites the literal token +#: ``/workspace`` to the real path in the command string; a command that never +#: mentions ``/workspace`` is untouched by it and runs on the host unrestricted. +ALIAS_REWRITE_MECHANISM = "workspace_alias_rewrite" + +#: Guards the one-per-process fallback warning below. Module state, because the +#: fact it reports is a property of the host rather than of a command. +_ALIAS_FALLBACK_LOGGED = False + + +def _filesystem_boundary_block(mechanism: str, mode: str, *, confined: bool) -> dict: + """The ``containment`` block for a spawn these tools still build themselves. + + Reports the **filesystem dimension only**, deliberately. The probe knows + this host could also give a process group and a real wall clock, but + BashTool and PythonTool still assemble their own ``create_subprocess_*`` + call and pass neither ``start_new_session`` nor a group-wide kill, so + listing those dimensions here would be the false claim + :mod:`src.containment` calls worse than an honest absence. They arrive when + this spawn path moves onto :func:`containment.run`, not before. + """ + return { + "mechanism": mechanism, + "mode": mode, + "enforced": [containment.FILESYSTEM] if confined else [], + "unenforced_required": [] if confined else [containment.FILESYSTEM], + "contained": confined, + "executed": True, + # Names the scope of the claim, so "process_tree is absent from + # enforced" reads as "not reported here" rather than "not enforced". + "reported_dimensions": [containment.FILESYSTEM], + } + + +def _contained_command( + content: str, + cwd: str, + *, + chdir: str = WORKSPACE_MOUNT, + interpreter_prefix: str | None = None, +) -> tuple[str, dict, bool]: + """Resolve ``content`` into the strongest form this host can run. + + Returns ``(command, containment_block, confined)``. The caller spawns + ``command``, copies ``containment_block`` into its result verbatim, and + refuses instead when ``confined`` is false under enforcing mode. + + This replaces ``namespaced or _replace_workspace_alias(...)``, the line this + ticket exists to delete. The two branches it chose between are not + comparable — one is a mount namespace, the other is a regex — and choosing + the second silently means an uncontained host execution reads in the + transcript exactly like a contained one. The fallback still happens under + report-only mode, which is what ships; the difference is that it is now + recorded in the result. + + :raises containment.ContainmentUnavailable: filesystem containment could + not be established and the mode is enforcing. The command is not run. + """ + probe = _execution_boundary(cwd) + wrapped = _wrap_workspace_namespace( + content, cwd, chdir=chdir, interpreter_prefix=interpreter_prefix, + ) + # The probe's filesystem answer and the wrapper's None/not-None answer rest + # on the same condition (`not IS_WINDOWS and which("bwrap")`), so they agree + # by construction. `wrapped` is still what decides, because it is what + # actually runs: a probe that said yes to a wrapper that declined would be + # the same false claim in the other direction. + if wrapped is not None: + return wrapped, _filesystem_boundary_block( + probe.mechanism, probe.mode, confined=True, + ), True + if probe.mode == containment.MODE_ENFORCING: + raise containment.ContainmentUnavailable( + frozenset({containment.FILESYSTEM}), ALIAS_REWRITE_MECHANISM, + ) + # Once per process, not once per command. The host's ability to establish a + # namespace does not change between calls, so a per-call warning would + # drown the log on every macOS install while adding nothing — and the + # per-call fact is already in the result block, which is where a reader + # looking at one command will look. + global _ALIAS_FALLBACK_LOGGED + if not _ALIAS_FALLBACK_LOGGED: + _ALIAS_FALLBACK_LOGGED = True + logger.warning( + "execution boundary: no filesystem containment is available on this " + "host (mechanism %r); agent commands fall back to the %s, which is " + "a path rewrite and not a boundary. Reported per command in the " + "result's containment block.", + probe.mechanism, ALIAS_REWRITE_MECHANISM, + ) + return ( + _replace_workspace_alias(content, cwd), + _filesystem_boundary_block( + ALIAS_REWRITE_MECHANISM, probe.mode, confined=False, + ), + False, + ) + + def _wrap_workspace_namespace( content: str, cwd: str, *, - chdir: str = "/workspace", + chdir: str = WORKSPACE_MOUNT, interpreter_prefix: str | None = None, ) -> str | None: """Run a shell command with the active workspace mounted at /workspace. @@ -251,12 +465,31 @@ def _wrap_workspace_namespace( "--symlink", "usr/lib64", "/lib64", "--symlink", "usr/bin", "/sbin", "--dir", "/etc", "--ro-bind", "/etc", "/etc", - "--dir", "/home", "--bind", "/home", "/home", - "--dir", "/mnt", "--bind", "/mnt", "/mnt", + # Read-only, not read-write. These two binds exist so a command can + # *read* host material it legitimately needs — a dataset under /mnt, a + # dotfile under /home. Binding them writable gave back most of what + # the namespace was for: a Linux host with working bubblewrap running + # this argv reaches outside the workspace and writes to the user's home + # directory, measured rather than inferred. The workspace bind below is + # the one writable path, which is what "workspace confinement" means. + "--dir", "/home", "--ro-bind", "/home", "/home", + "--dir", "/mnt", "--ro-bind", "/mnt", "/mnt", "--dir", "/tmp", "--tmpfs", "/tmp", "--dev-bind", "/dev", "/dev", "--proc", "/proc", - "--dir", "/workspace", "--bind", cwd, "/workspace", + "--dir", WORKSPACE_MOUNT, "--bind", cwd, WORKSPACE_MOUNT, ] + # The workspace stays writable at its real host path as well as at + # /workspace. A command can carry the absolute host path: BashTool's own + # /tmp redirect rewrites `/tmp/` to `/.tmp/` before the + # namespace is built, so the command reaching bwrap already names the real + # path. Before /home and /mnt became read-only those writes landed only + # because the workspace happened to sit under one of them. Binding the + # workspace itself is the narrow version of what that accident provided: + # the same directory by either name, and nothing else writable. + real_cwd = os.path.realpath(cwd) + if real_cwd not in _NAMESPACE_RESERVED_DESTS and len(real_cwd.split(os.sep)) >= 3: + args.extend(_namespace_dir_chain(real_cwd)) + args.extend(("--bind", real_cwd, real_cwd)) # setup-python installs interpreters under /opt, and local CI virtualenvs # can live under /tmp. Those paths are hidden by the private root/tmpfs. # Expose only the active interpreter environment, read-only, so Python @@ -264,15 +497,7 @@ def _wrap_workspace_namespace( if interpreter_prefix: prefix = os.path.abspath(interpreter_prefix) resolved_prefix = os.path.realpath(prefix) - mounted_roots = ("/usr", "/home", "/mnt") - reserved_roots = { - "/", "/tmp", "/var", "/opt", "/etc", "/workspace", - "/root", "/run", "/proc", "/dev", "/sys", *mounted_roots, - } - already_visible = any( - prefix == root or prefix.startswith(root + os.sep) - for root in mounted_roots - ) + already_visible = _namespace_visible_without_bind(prefix) # A prefix is trusted only when it names a specific interpreter tree. # In particular, never overlay the private root, tmpfs, or workspace # with a broad host directory. Reject symlinked prefixes too: bwrap @@ -289,18 +514,12 @@ def _wrap_workspace_namespace( if ( not already_visible and prefix == resolved_prefix - and prefix not in reserved_roots + and prefix not in _NAMESPACE_RESERVED_DESTS and len(prefix.split(os.sep)) >= 3 and os.path.isdir(prefix) and has_environment_layout ): - parents = [] - parent = os.path.dirname(prefix) - while parent not in ("/", "/tmp", "/etc", "/workspace", *mounted_roots): - parents.append(parent) - parent = os.path.dirname(parent) - for directory in reversed(parents): - args.extend(("--dir", directory)) + args.extend(_namespace_dir_chain(prefix)) args.extend(("--ro-bind", prefix, prefix)) args.extend(("--chdir", chdir, "/bin/bash", "-lc", content)) return shlex.join(args) @@ -635,12 +854,13 @@ class BashTool: ), "exit_code": 1, } - isolated_tmp = os.path.join(agent_cwd(), ".tmp") if "/tmp/" in content: - os.makedirs(isolated_tmp, exist_ok=True) + isolated_tmp = _isolated_tmp_dir(agent_cwd()) content = content.replace("/tmp/", isolated_tmp.rstrip("/") + "/") - namespaced = _wrap_workspace_namespace(content, agent_cwd()) - content = namespaced or _replace_workspace_alias(content, agent_cwd()) + try: + content, boundary, _confined = _contained_command(content, agent_cwd()) + except containment.ContainmentUnavailable as exc: + return containment.unavailable_tool_result(exc, tool="bash") progress_cb = ctx.get("progress_cb") _subproc_env = ctx.get("subproc_env") session_id = ctx.get("session_id") @@ -660,6 +880,7 @@ class BashTool: "stdout": _truncate(stdout, MAX_OUTPUT_CHARS), "stderr": _truncate(stderr, MAX_OUTPUT_CHARS), "tmux_session": _tmux_session_name(str(session_id)), + "containment": boundary, } output = stdout.rstrip() err = stderr.rstrip() @@ -669,6 +890,7 @@ class BashTool: "output": _truncate(output, MAX_OUTPUT_CHARS) or "(no output)", "exit_code": rc or 0, "tmux_session": _tmux_session_name(str(session_id)), + "containment": boundary, } try: @@ -690,7 +912,7 @@ class BashTool: cwd=agent_cwd(), ) except RuntimeError as exc: - return {"error": str(exc), "exit_code": 1} + return {"error": str(exc), "exit_code": 1, "containment": boundary} mark_operation_started('subprocess', pid=proc.pid) stdout, stderr, rc, timed_out = await _run_subprocess_streaming( proc, @@ -698,13 +920,13 @@ class BashTool: progress_cb=progress_cb, ) if timed_out: - return {"error": f"bash: timed out after {DEFAULT_BASH_TIMEOUT}s — process killed", "exit_code": 124, "stdout": _truncate(stdout, MAX_OUTPUT_CHARS), "stderr": _truncate(stderr, MAX_OUTPUT_CHARS)} + return {"error": f"bash: timed out after {DEFAULT_BASH_TIMEOUT}s — process killed", "exit_code": 124, "stdout": _truncate(stdout, MAX_OUTPUT_CHARS), "stderr": _truncate(stderr, MAX_OUTPUT_CHARS), "containment": boundary} output = stdout.rstrip() err = stderr.rstrip() if err: output = (output + "\nSTDERR: " + err).strip() if output else "STDERR: " + err output = _truncate(output, MAX_OUTPUT_CHARS) - return {"output": output or "(no output)", "exit_code": rc or 0} + return {"output": output or "(no output)", "exit_code": rc or 0, "containment": boundary} class HostShellTool: async def execute(self, content: str, ctx: dict) -> dict: @@ -959,12 +1181,11 @@ class PythonTool: # every invocation would make that stable contract appear as # ``/workspace`` instead. needs_virtual_namespace = bool( - "/workspace" in content + WORKSPACE_MOUNT in content or re.search(r"\b(?:runpy\.run_path|exec\s*\(|importlib\.)", content) ) - isolated_tmp = os.path.join(agent_cwd(), ".tmp") if "/tmp/" in content: - os.makedirs(isolated_tmp, exist_ok=True) + isolated_tmp = _isolated_tmp_dir(agent_cwd()) content = content.replace("/tmp/", isolated_tmp.rstrip("/") + "/") progress_cb = ctx.get("progress_cb") _subproc_env = ctx.get("subproc_env") @@ -987,12 +1208,39 @@ class PythonTool: _wrap_workspace_namespace( python_command, agent_cwd(), - chdir="/workspace", + chdir=WORKSPACE_MOUNT, interpreter_prefix=sys.prefix, ) if needs_virtual_namespace else None ) + # The boundary this call actually got, reported either way. Note what + # the gate above means: code that does not mention /workspace and is + # not dynamic gets NO namespace, on every platform including a Linux + # host with working bubblewrap. That is deliberate -- it keeps + # os.getcwd() the real workspace -- but it is also a filesystem + # containment gap wider than the macOS one, and until now nothing said + # so. Reporting it is in scope here; closing it is not: it changes the + # Linux Python path for every call and cannot be verified on a host + # without bwrap. It is the reason enforcing mode cannot be switched on + # yet, because enforcing it as written would refuse ordinary Python on + # a correctly configured host. + boundary_probe = _execution_boundary( + agent_cwd(), wall_clock_s=DEFAULT_PYTHON_TIMEOUT, + ) + confined = namespaced is not None + if not confined and boundary_probe.mode == containment.MODE_ENFORCING: + return containment.unavailable_tool_result( + containment.ContainmentUnavailable( + frozenset({containment.FILESYSTEM}), ALIAS_REWRITE_MECHANISM, + ), + tool="python", + ) + boundary = _filesystem_boundary_block( + boundary_probe.mechanism if confined else ALIAS_REWRITE_MECHANISM, + boundary_probe.mode, + confined=confined, + ) if namespaced: proc = await asyncio.create_subprocess_exec( "/bin/bash", "-lc", namespaced, @@ -1024,7 +1272,7 @@ class PythonTool: progress_cb=progress_cb, ) if timed_out: - return {"error": f"python: timed out after {DEFAULT_PYTHON_TIMEOUT}s — process killed", "exit_code": 124, "stdout": _truncate(stdout, MAX_OUTPUT_CHARS), "stderr": _truncate(stderr, MAX_OUTPUT_CHARS)} + return {"error": f"python: timed out after {DEFAULT_PYTHON_TIMEOUT}s — process killed", "exit_code": 124, "stdout": _truncate(stdout, MAX_OUTPUT_CHARS), "stderr": _truncate(stderr, MAX_OUTPUT_CHARS), "containment": boundary} child_failure = _python_child_runtime_failure(stdout, stderr, rc) if child_failure: return { @@ -1035,10 +1283,11 @@ class PythonTool: ), "exit_code": 1, "stderr": _truncate(stderr, MAX_OUTPUT_CHARS), + "containment": boundary, } output = stdout.rstrip() err = stderr.rstrip() if err: output = (output + "\nSTDERR: " + err).strip() if output else "STDERR: " + err output = _truncate(output, MAX_OUTPUT_CHARS) - return {"output": output or "(no output)", "exit_code": rc or 0} + return {"output": output or "(no output)", "exit_code": rc or 0, "containment": boundary} diff --git a/src/app_helpers.py b/src/app_helpers.py index 1b915a5a2..5dd772848 100644 --- a/src/app_helpers.py +++ b/src/app_helpers.py @@ -7,6 +7,8 @@ from fastapi import HTTPException from fastapi.responses import HTMLResponse from starlette.requests import Request +from src.path_confinement import is_inside + logger = logging.getLogger(__name__) def read_if_exists(path: str) -> str: @@ -51,11 +53,4 @@ def serve_html_with_nonce(request: Request, file_path: str) -> HTMLResponse: def inside_base_dir(base_dir: str, path: str) -> bool: """Check if path is inside base directory.""" - if not isinstance(base_dir, str) or not isinstance(path, str): - return False - base = os.path.realpath(base_dir) - p = os.path.realpath(path) - try: - return os.path.commonpath([base, p]) == base - except Exception: - return False + return is_inside(base_dir, path) diff --git a/src/constants.py b/src/constants.py index 40f859f3e..d11283646 100644 --- a/src/constants.py +++ b/src/constants.py @@ -155,6 +155,17 @@ SCHOLARLY_LOOKUP_TOTAL_BUDGET = 20.0 CLEANUP_ENABLED = os.getenv("CLEANUP_ENABLED", "True").lower() == "true" CLEANUP_INTERVAL_HOURS = int(os.getenv("CLEANUP_INTERVAL_HOURS", "24")) +# Agent workspace +# The stable virtual root the tool contract promises an agent, independent of +# where the workspace physically lives. Both the mount namespace and the +# path resolvers map it to the active workspace, so it is the one absolute path +# a contained command may assume. +WORKSPACE_MOUNT = "/workspace" +# Scratch directory inside the workspace that agent shell commands get in place +# of the host /tmp. A dirname rather than a path: the workspace is dynamic, so +# the full path is only knowable per turn. +AGENT_ISOLATED_TMP_DIRNAME = ".tmp" + # Auth policy PASSWORD_MIN_LENGTH = 8 diff --git a/src/containment.py b/src/containment.py index bd84cc7ff..3584cdee2 100644 --- a/src/containment.py +++ b/src/containment.py @@ -67,7 +67,11 @@ from core.atomic_io import atomic_write_json from core.platform_compat import IS_WINDOWS, find_bash, pid_alive from src import process_ownership -from src.constants import CONTAINMENT_STATE_FILE, MAX_OUTPUT_CHARS +from src.constants import ( + CONTAINMENT_STATE_FILE, + MAX_OUTPUT_CHARS, + WORKSPACE_MOUNT, +) logger = logging.getLogger(__name__) @@ -118,13 +122,12 @@ _DEATH_POLL_S = 0.05 # private /tmp or the workspace itself with a host directory would undo the # namespace from inside the argv that builds it. _RESERVED_BIND_DESTS = frozenset({ - "/", "/tmp", "/proc", "/dev", "/sys", "/workspace", + "/", "/tmp", "/proc", "/dev", "/sys", WORKSPACE_MOUNT, }) -#: Where the workspace is mounted inside a namespace. The public tool contract -#: already promises this path, so it is the one path a contained command may -#: assume. -WORKSPACE_MOUNT = "/workspace" +# WORKSPACE_MOUNT is re-exported from src.constants: where the workspace is +# mounted inside a namespace is a property of the tool contract, not of this +# module, and two definitions of it would be two contracts. class ContainmentUnavailable(RuntimeError): @@ -604,6 +607,59 @@ def _select(spec: ContainmentSpec) -> tuple[Optional[Mechanism], frozenset[str]] return best, best_provided +@dataclass(frozen=True) +class ContainmentProbe: + """What a spec *would* get on this host. No grant, no record, no process. + + For a spawn path that has not yet been rewritten to run through + :func:`run` and still builds its own ``create_subprocess_*`` call. Such a + caller still has to decide — refuse, or run and say so — and that decision + has to come from the same mechanism table :func:`acquire` consults, or the + tree grows a second opinion about what this host can enforce. + + Calling :func:`acquire` for the answer is the wrong shape: it writes a + durable grant record, and a record whose pid is never filled in and whose + :func:`release` never runs is an entry a restart reaper will keep finding. + """ + + mechanism: str + enforced: frozenset[str] + degraded: tuple[str, ...] + unenforced_required: tuple[str, ...] + mode: str + + @property + def contained(self) -> bool: + return not self.unenforced_required + + @property + def refuses(self) -> bool: + """True when this spec cannot run at all under the current mode.""" + return bool(self.unenforced_required) and self.mode == MODE_ENFORCING + + +def probe(spec: ContainmentSpec) -> ContainmentProbe: + """Answer what this host can establish for ``spec``, without acquiring it. + + Same selection, same mechanism table and same arithmetic as + :func:`acquire`; it just stops before the side effects. The command is not + an input here either. + + :raises ValueError: the spec is malformed (a caller bug, in either mode). + """ + spec = _validate_spec(spec) + mechanism, provided = _select(spec) + enforced = provided & spec.requested + missing_required = frozenset(spec.required) - enforced + return ContainmentProbe( + mechanism=mechanism.name if mechanism else "none", + enforced=enforced, + degraded=tuple(sorted(spec.requested - enforced - spec.required)), + unenforced_required=tuple(sorted(missing_required)), + mode=CONTAINMENT_MODE, + ) + + def acquire(spec: ContainmentSpec, *, owner: str) -> ContainmentGrant: """Establish containment, or refuse. diff --git a/src/generated_images.py b/src/generated_images.py index d40022d60..8c312cd99 100644 --- a/src/generated_images.py +++ b/src/generated_images.py @@ -1,10 +1,10 @@ -import os import re from pathlib import Path from fastapi import HTTPException from src.constants import GENERATED_IMAGES_DIR +from src.path_confinement import confine GENERATED_IMAGE_DIR = Path(GENERATED_IMAGES_DIR) @@ -20,12 +20,9 @@ GENERATED_IMAGE_HEADERS = { def resolve_generated_image_path(filename: str) -> Path: if not isinstance(filename, str) or not GENERATED_IMAGE_RE.fullmatch(filename): raise HTTPException(status_code=400, detail="Invalid filename") - root = GENERATED_IMAGE_DIR.resolve() - path = (GENERATED_IMAGE_DIR / filename).resolve() try: - if os.path.commonpath([str(root), str(path)]) != str(root): - raise ValueError - except Exception: + path = Path(confine(GENERATED_IMAGE_DIR, filename, allow_root=False)) + except (ValueError, OSError): raise HTTPException(status_code=400, detail="Invalid filename") if not path.exists(): raise HTTPException(status_code=404, detail="Image not found") diff --git a/src/path_confinement.py b/src/path_confinement.py new file mode 100644 index 000000000..9143e7088 --- /dev/null +++ b/src/path_confinement.py @@ -0,0 +1,188 @@ +"""The filesystem confinement boundary. One implementation, every call site. + +"Is this path inside that root" is asked in twenty places in this tree, and +twenty times it is answered by a locally written ``realpath`` + +``os.path.commonpath`` pair. Each one is defensible on its own. Together they +are the problem: the boundary has no single definition, so a site that gets a +detail wrong is wrong *alone*, and a site added tomorrow starts from whichever +neighbour its author happened to copy. + +The details that differ between those copies, and what this module settles: + +**Both sides get canonicalized.** Comparing a ``realpath``-ed candidate against +a root that was only ``abspath``-ed is the bug class that has already cost this +project real time: on macOS ``/tmp`` is a symlink to ``/private/tmp``, so the +two sides disagree about a path neither of them is wrong about. It reads as an +escape and refuses a legitimate access. Canonicalizing one side is worse than +canonicalizing neither. + +**``commonpath``, never ``startswith``.** ``/a/bc`` begins with ``/a/b`` and is +not inside it. + +**Case folding is the filesystem's business, not the comparison's.** +``os.path.normcase`` lowercases on Windows and is the identity everywhere else +— including macOS, whose default filesystem is case-insensitive while its +``realpath`` preserves case. So normcase alone does not make the comparison +agree with the filesystem on macOS, and :func:`is_inside` does not pretend +otherwise: it answers about the canonical path, which is the question a +confinement check should be asking. Where a caller needs to match the +filesystem's own folding it must compare real paths of real files, not strings. + +**A relative candidate joins the root, never the process cwd.** ``abspath`` of a +relative path silently uses ``os.getcwd()``, which is whatever the server +happens to be running in. A confinement helper that does that is resolving +against the wrong base before it even starts comparing. + +**NUL and newline are rejected, not caught.** Several of the copies wrap the +whole comparison in ``except Exception: return False``, which turns a malformed +path into "outside" — the safe answer, reached by accident. Here it is a +``ValueError`` with a reason. + +**``commonpath`` raising means outside.** It raises across Windows drive letters +and for mixed absolute/relative inputs. Both mean the candidate is not under the +root, so the refusal is deliberate rather than incidental. + +What this module does *not* do: decide whether a path is sensitive (``.ssh``, +``id_rsa``, …). That is a separate deny list applied inside an allowed root, and +it lives with the callers that own it — ``src/tool_execution`` for the agent +tools. Confinement answers "inside the root"; it does not answer "allowed". + +Relationship to :mod:`src.containment`: that module is the boundary for *where a +process runs*; this one is the boundary for *which paths a path check accepts*. +A contained process is restricted by a mount namespace, which this module cannot +express and does not try to; an in-process read of a model-supplied path is +restricted by this module, which a namespace does not see. +""" + +from __future__ import annotations + +import os + +__all__ = [ + "PathEscape", + "canonical_root", + "confine", + "is_inside", +] + + +class PathEscape(ValueError): + """A candidate path does not resolve inside the root it was checked against. + + A subclass of :class:`ValueError` so the call sites this replaces — which + raise ``ValueError`` and are caught as such by their callers and their + tests — keep behaving the way they did. + """ + + def __init__(self, root: str, candidate: str, reason: str = "") -> None: + self.root = str(root) + self.candidate = str(candidate) + self.reason = str(reason or "outside the allowed root") + super().__init__( + f"path {self.candidate!r} is {self.reason} ({self.root})" + ) + + +def _reject_unusable(value: str, *, label: str) -> str: + """Normalize a path argument to ``str``, refusing the unusable shapes. + + ``\\x00`` is refused here because the OS layer raises on it much later and + from somewhere unhelpful, and because a broad ``except Exception`` around + the comparison would otherwise record it as an ordinary escape. Newlines + are refused for the same reason the workspace-mount parser refuses them: + a path carrying one has been built by splitting something that was not a + path list. + """ + if value is None: + raise ValueError(f"{label} is required") + if isinstance(value, os.PathLike): + value = os.fspath(value) + if not isinstance(value, str): + raise ValueError(f"{label} must be a path, got {type(value).__name__}") + text = value.strip() + if not text: + raise ValueError(f"{label} is required") + if "\x00" in text: + raise ValueError(f"{label} must not contain NUL") + if "\n" in text or "\r" in text: + raise ValueError(f"{label} must not contain a newline") + return text + + +def canonical_root(root) -> str: + """The canonical form of a confinement root. + + Exposed because a caller that holds a root across several checks should + canonicalize it once, and because a caller comparing two paths itself needs + the same canonical form this module compares against — a realpath-ed value + tested against a raw one is the asymmetry this module exists to remove. + """ + text = _reject_unusable(root, label="root") + return os.path.realpath(os.path.expanduser(text)) + + +def _canonical_candidate(root: str, candidate) -> str: + """Canonicalize ``candidate``, resolving a relative path under ``root``. + + ``realpath`` is deliberately the non-strict kind: a final component that + does not exist yet is normalized rather than refused, because a write target + is a legitimate thing to confine. Everything that *does* exist is resolved, + so a symlink anywhere in the chain — including the final component — is + followed before the comparison rather than after the open. + """ + text = _reject_unusable(candidate, label="path") + expanded = os.path.expanduser(text) + if not os.path.isabs(expanded): + expanded = os.path.join(root, expanded) + return os.path.realpath(expanded) + + +def is_inside(root, candidate, *, allow_root: bool = True) -> bool: + """True when ``candidate`` resolves inside ``root``. + + The boolean form, for call sites whose contract is a predicate. A malformed + argument is ``False`` here rather than a raise, because a predicate that + raises is the reason those call sites wrapped themselves in + ``except Exception`` in the first place. Use :func:`confine` where the + caller wants the resolved path and a reason for the refusal. + + ``allow_root=False`` excludes the root itself, for a caller whose operation + is only meaningful on something *under* the root — deleting a file, say, + where the root is the directory it must not be. + """ + try: + confine(root, candidate, allow_root=allow_root) + return True + except (ValueError, OSError): + return False + + +def confine(root, candidate, *, allow_root: bool = True) -> str: + """Resolve ``candidate`` inside ``root``, or raise. + + Returns the canonical absolute path, which is what the caller should then + open: resolving and then opening the *original* string re-introduces the + symlink race the resolution just closed. + + :raises ValueError: either argument is unusable as a path. + :raises PathEscape: the candidate resolves outside the root. + """ + base = canonical_root(root) + resolved = _canonical_candidate(base, candidate) + + if resolved == base: + if allow_root: + return resolved + raise PathEscape(base, candidate, "the root itself, not a path inside it") + + # normcase folds case on Windows and is the identity elsewhere; it is + # applied to both sides or to neither, which is the whole point. + try: + common = os.path.commonpath([os.path.normcase(resolved), os.path.normcase(base)]) + except ValueError: + # Different Windows drives, or mixed absolute/relative. Both mean the + # candidate is not under the root. + raise PathEscape(base, candidate) from None + if common != os.path.normcase(base): + raise PathEscape(base, candidate) + return resolved diff --git a/src/session_image_cleanup.py b/src/session_image_cleanup.py index fcedaf56b..3583b3a57 100644 --- a/src/session_image_cleanup.py +++ b/src/session_image_cleanup.py @@ -4,11 +4,11 @@ from __future__ import annotations import json import logging -import os import re from pathlib import Path from src.constants import GENERATED_IMAGES_DIR +from src.path_confinement import confine logger = logging.getLogger(__name__) @@ -26,14 +26,10 @@ def _generated_image_path_for_cleanup(filename: str) -> Path | None: name = Path(filename).name if name != filename or name in {".", ".."}: return None - root = Path(GENERATED_IMAGES_DIR).resolve() - path = (root / name).resolve() try: - if os.path.commonpath([str(root), str(path)]) != str(root): - return None - except Exception: + return Path(confine(GENERATED_IMAGES_DIR, name, allow_root=False)) + except (ValueError, OSError): return None - return path def _image_filename_from_url(url: str) -> str: diff --git a/src/tool_execution.py b/src/tool_execution.py index e72594cba..a5d1b7118 100644 --- a/src/tool_execution.py +++ b/src/tool_execution.py @@ -34,7 +34,14 @@ from src.tool_capabilities import ToolRunSecurityContext, blocked_tool_result from src.tool_approvals import ExactToolApproval from src.tool_policy import ToolPolicy from src.client_tool_contract import TUI_ROUTED_BRIDGE_TOOL_NAMES -from src.constants import MAX_OUTPUT_CHARS, MAX_READ_CHARS, MAX_DIFF_LINES, DATA_DIR +from src.constants import ( + DATA_DIR, + MAX_DIFF_LINES, + MAX_OUTPUT_CHARS, + MAX_READ_CHARS, + WORKSPACE_MOUNT, +) +from src.path_confinement import canonical_root, confine, is_inside from src.tool_utils import _truncate, get_mcp_manager @@ -814,13 +821,7 @@ def _resolve_tool_path(raw_path: str) -> str: ) for root in _tool_path_roots(): - if resolved == root: - return resolved - try: - common = os.path.commonpath([resolved, root]) - except ValueError: - continue - if common == root: + if is_inside(root, resolved): return resolved raise ValueError( f"path '{raw_path}' is outside the allowed roots" @@ -838,33 +839,27 @@ def _resolve_tool_path_in_workspace(workspace: str, raw_path: str) -> str: """ if raw_path is None or not str(raw_path).strip(): raise ValueError("path is required") - base = os.path.realpath(workspace) + base = canonical_root(workspace) expanded = os.path.expanduser(str(raw_path).strip()) # `/workspace` is the stable user-facing agent root in tasks and docs. # Native/manual installs may bind the request to another physical folder; # resolve the alias inside that active workspace rather than rejecting it. - if expanded == "/workspace": + if expanded == WORKSPACE_MOUNT: expanded = base - elif expanded.startswith("/workspace/"): - expanded = os.path.join(base, expanded.removeprefix("/workspace/")) - candidate = expanded if os.path.isabs(expanded) else os.path.join(base, expanded) - resolved = os.path.realpath(candidate) + elif expanded.startswith(WORKSPACE_MOUNT + "/"): + expanded = os.path.join(base, expanded.removeprefix(WORKSPACE_MOUNT + "/")) + try: + resolved = confine(base, expanded) + except (ValueError, OSError): + raise ValueError(f"path '{raw_path}' is outside the workspace ({workspace})") + # Confinement says "inside the root"; the deny list says "allowed". They + # are separate questions and this one stays here, with the policy that + # owns it. if _is_sensitive_path(resolved): raise ValueError( f"path '{raw_path}' is inside a sensitive directory " f"(e.g. .ssh, .gnupg) or matches a sensitive filename" ) - if resolved != base: - # normcase so containment holds on case-insensitive filesystems - # (Windows, default macOS): it lowercases on Windows and is a no-op on - # POSIX. commonpath raises ValueError across Windows drives (C: vs D:) - # or mixed abs/rel — both mean "outside", so the except rejects them. - nbase = os.path.normcase(base) - try: - if os.path.commonpath([os.path.normcase(resolved), nbase]) != nbase: - raise ValueError - except ValueError: - raise ValueError(f"path '{raw_path}' is outside the workspace ({workspace})") return resolved diff --git a/src/upload_handler.py b/src/upload_handler.py index abde96707..ce87120d0 100644 --- a/src/upload_handler.py +++ b/src/upload_handler.py @@ -13,6 +13,7 @@ from datetime import datetime, timedelta from typing import Dict, Any, Optional from fastapi import HTTPException, UploadFile +from src.path_confinement import is_inside from src.upload_limits import format_byte_limit, get_chat_upload_max_bytes @@ -256,12 +257,7 @@ class UploadHandler: def inside_base_dir(self, path: str) -> bool: """Check if path is inside base directory""" - base = os.path.realpath(self.base_dir) - p = os.path.realpath(path) - try: - return os.path.commonpath([base, p]) == base - except Exception: - return False + return is_inside(self.base_dir, path) def get_upload_dir(self): """Get date-based upload directory""" @@ -684,12 +680,7 @@ class UploadHandler: def _inside_upload_dir(self, path: str) -> bool: """Check if path is inside the upload directory.""" - base = os.path.normcase(os.path.realpath(self.upload_dir)) - p = os.path.normcase(os.path.realpath(path)) - try: - return os.path.commonpath([base, p]) == base - except Exception: - return False + return is_inside(self.upload_dir, path) def _atomic_write_json( self, diff --git a/tests/test_agent_bash_windows.py b/tests/test_agent_bash_windows.py index 0145b4b28..90e548b5c 100644 --- a/tests/test_agent_bash_windows.py +++ b/tests/test_agent_bash_windows.py @@ -100,7 +100,12 @@ async def test_windows_bash_does_not_use_a_stray_tmux_executable(monkeypatch): {"subproc_env": {}, "session_id": "chat-1"}, ) - assert result == {"output": "ok", "exit_code": 0} + assert result["output"] == "ok" + assert result["exit_code"] == 0 + # Every bash result now carries the execution boundary it actually got. + # Asserting dict equality here would make that field impossible to add + # without touching a test about tmux, so the shape is asserted instead. + assert result["containment"]["reported_dimensions"] == ["filesystem"] assert captured["command"] == "pwd" assert captured["kwargs"]["cwd"] == workspace diff --git a/tests/test_execution_filesystem_boundary.py b/tests/test_execution_filesystem_boundary.py new file mode 100644 index 000000000..0c6ef1b4a --- /dev/null +++ b/tests/test_execution_filesystem_boundary.py @@ -0,0 +1,304 @@ +"""The filesystem execution boundary: what the namespace binds, and what a +spawn says when there is no namespace to bind it with. + +Two separate defects, both on the agent shell/Python path. + +**The writable /home bind.** The workspace namespace bound ``/home`` and +``/mnt`` read-write. On the one platform where the namespace engages at all, +a command inside it reached outside the workspace and wrote to the user's home +directory. That was measured on a Linux host with working bubblewrap by +running this repo's own argv, so it is not a source read. Binding the user's +whole home directory into a workspace-confinement namespace gives back most of +what the namespace was for. + +**The silent downgrade.** ``namespaced or _replace_workspace_alias(...)`` chose +between a mount namespace and a regex, with nothing in the tool result saying +which one ran. The fallback is a naming convenience — it rewrites the literal +token ``/workspace`` in the command string — so a command that never mentions +``/workspace`` is untouched by it and runs on the host unrestricted. That is +every agent shell command on macOS, which is a platform this project is +maintained and run on. + +The argv tests assert the argv rather than running it: bubblewrap does not +exist on macOS, and it does not work in Docker either without +``--privileged`` (default and ``seccomp=unconfined`` both give +"Creating new namespace failed", ``--cap-add=SYS_ADMIN`` gives +"pivot_root: Operation not permitted"). An argv assertion is what can honestly +be checked on this host; the execution evidence for the defect itself came from +a Linux host. +""" +import os +import shlex + +import pytest + +from src import containment +from src.agent_tools import subprocess_tools +from src.constants import WORKSPACE_MOUNT + + +@pytest.fixture +def workspace(tmp_path): + (tmp_path / "artifact.txt").write_text("x", encoding="utf-8") + return str(tmp_path) + + +def _argv(workspace, **kwargs): + """The namespace argv, with bubblewrap forced present. + + `shutil.which` is patched rather than skipped so the argv is asserted on + every platform the suite runs on — the bind flags are the finding, and they + are wrong independently of whether this host can execute them. + """ + import shutil as _shutil + + original = _shutil.which + try: + _shutil.which = lambda name, *a, **kw: ( + "/usr/bin/bwrap" if name == "bwrap" else original(name, *a, **kw) + ) + wrapped = subprocess_tools._wrap_workspace_namespace("true", workspace, **kwargs) + finally: + _shutil.which = original + assert wrapped is not None, "forced bwrap should produce a namespace argv" + return shlex.split(wrapped) + + +def _bind_mode(argv, dest): + """The bind flag immediately preceding ``src dest`` in the argv, or None.""" + for index in range(len(argv) - 2): + if argv[index + 2] == dest and argv[index].startswith("--"): + return argv[index] + return None + + +# ── what the namespace binds ──────────────────────────────────────────────── +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_home_and_mnt_are_read_only(workspace): + argv = _argv(workspace) + assert _bind_mode(argv, "/home") == "--ro-bind" + assert _bind_mode(argv, "/mnt") == "--ro-bind" + + +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_the_workspace_is_the_writable_bind(workspace): + argv = _argv(workspace) + assert _bind_mode(argv, WORKSPACE_MOUNT) == "--bind" + assert argv[argv.index("--bind")] == "--bind" + + +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_the_workspace_stays_writable_at_its_real_host_path_too(workspace): + """A command can carry the absolute host path, not only /workspace. + + BashTool's /tmp redirect rewrites `/tmp/` to `/.tmp/` before + the namespace is built, so the command bwrap receives already names the real + path. Those writes used to land because the workspace happened to sit under + the writable `/home` bind. With /home read-only they need the workspace's + own bind, or making /home read-only silently breaks every command that uses + a real host path. + """ + real = os.path.realpath(workspace) + argv = _argv(workspace) + assert _bind_mode(argv, real) == "--bind", ( + f"expected a writable bind of {real}; argv was {argv}" + ) + + +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_no_dir_chain_is_created_inside_a_read_only_bind(monkeypatch, tmp_path): + """mkdir inside a read-only mount fails and takes the namespace with it. + + A workspace under `/home` or `/mnt` already has its parents, because the + argv mounted those roots. Emitting `--dir /home/someone` for it would be an + error, not a no-op. + """ + assert subprocess_tools._namespace_dir_chain("/home/someone/ws") == [] + assert subprocess_tools._namespace_dir_chain("/mnt/data/ws") == [] + assert subprocess_tools._namespace_dir_chain("/usr/share/ws") == [] + # Somewhere the argv does not mount: the parents have to be created in the + # private tmpfs root. + assert subprocess_tools._namespace_dir_chain("/srv/agents/ws") == [ + "--dir", "/srv", "--dir", "/srv/agents", + ] + + +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_reserved_destinations_are_never_bound_over(workspace, monkeypatch): + """Overlaying the private root, tmpfs or the workspace mount with a host + directory undoes the namespace from inside the argv that builds it.""" + for reserved in ("/", "/tmp", WORKSPACE_MOUNT, "/proc", "/etc", "/usr"): + assert reserved in subprocess_tools._NAMESPACE_RESERVED_DESTS + + +@pytest.mark.skipif(os.name == "nt", reason="bwrap argv is POSIX-only") +def test_a_workspace_at_a_reserved_destination_gets_no_extra_bind(monkeypatch): + """`/tmp` as the workspace must not produce `--bind /tmp /tmp` after the + argv has already put a private tmpfs there.""" + monkeypatch.setattr(os.path, "realpath", lambda path: "/tmp") + argv = _argv("/tmp") + # `--tmpfs /tmp` takes a destination only, so it is a two-arg pair. + pairs = list(zip(argv, argv[1:])) + assert ("--tmpfs", "/tmp") in pairs + assert _bind_mode(argv, "/tmp") is None, ( + f"a host bind of /tmp would undo the private tmpfs; argv was {argv}" + ) + + +# ── the silent downgrade ──────────────────────────────────────────────────── +def test_fallback_reports_that_filesystem_containment_did_not_hold( + workspace, monkeypatch, +): + """The fallback still runs under report-only — but it is now recorded. + + Before this, the only difference between a contained run and a host run was + whether a regex had rewritten a token, and nothing in the result said so. + """ + monkeypatch.setattr( + subprocess_tools, "_wrap_workspace_namespace", + lambda *args, **kwargs: None, + ) + monkeypatch.setattr(containment, "CONTAINMENT_MODE", containment.MODE_REPORT_ONLY) + command, block, confined = subprocess_tools._contained_command( + "echo hi", workspace, + ) + assert confined is False + assert command == "echo hi" + assert block["contained"] is False + assert block["executed"] is True + assert block["unenforced_required"] == [containment.FILESYSTEM] + assert block["mechanism"] == subprocess_tools.ALIAS_REWRITE_MECHANISM + assert block["mode"] == containment.MODE_REPORT_ONLY + + +def test_the_fallback_mechanism_is_not_named_like_a_mechanism(workspace, monkeypatch): + """A string rewrite reported as "bubblewrap" or "none" is the same silence + with extra steps. It gets its own name so a reader cannot mistake it.""" + monkeypatch.setattr( + subprocess_tools, "_wrap_workspace_namespace", + lambda *args, **kwargs: None, + ) + _command, block, _confined = subprocess_tools._contained_command("echo hi", workspace) + assert block["mechanism"] == "workspace_alias_rewrite" + assert block["mechanism"] not in {name.name for name in containment.MECHANISMS} + + +def test_enforcing_mode_refuses_instead_of_falling_back(workspace, monkeypatch): + """Fail closed. Containment required and unavailable means not executed.""" + monkeypatch.setattr( + subprocess_tools, "_wrap_workspace_namespace", + lambda *args, **kwargs: None, + ) + monkeypatch.setattr(containment, "CONTAINMENT_MODE", containment.MODE_ENFORCING) + with pytest.raises(containment.ContainmentUnavailable) as caught: + subprocess_tools._contained_command("echo hi", workspace) + assert caught.value.missing == frozenset({containment.FILESYSTEM}) + + +def test_a_namespaced_command_reports_the_mechanism_that_established_it( + workspace, monkeypatch, +): + monkeypatch.setattr( + subprocess_tools, "_wrap_workspace_namespace", + lambda *args, **kwargs: "bwrap --whatever true", + ) + monkeypatch.setattr( + containment, "probe", + lambda spec: containment.ContainmentProbe( + mechanism="bubblewrap", + enforced=frozenset({containment.FILESYSTEM, containment.PROCESS_TREE}), + degraded=(), + unenforced_required=(), + mode=containment.MODE_REPORT_ONLY, + ), + ) + command, block, confined = subprocess_tools._contained_command("true", workspace) + assert confined is True + assert command == "bwrap --whatever true" + assert block["mechanism"] == "bubblewrap" + assert block["contained"] is True + assert block["enforced"] == [containment.FILESYSTEM] + + +def test_the_reported_block_claims_only_the_filesystem_dimension(workspace, monkeypatch): + """These tools still build their own create_subprocess_* call and pass + neither start_new_session nor a group-wide kill, so listing process_tree or + wall_clock here would be a false claim. The block names its own scope.""" + monkeypatch.setattr( + subprocess_tools, "_wrap_workspace_namespace", + lambda *args, **kwargs: "bwrap --whatever true", + ) + _command, block, _confined = subprocess_tools._contained_command("true", workspace) + assert block["reported_dimensions"] == [containment.FILESYSTEM] + assert containment.PROCESS_TREE not in block["enforced"] + assert containment.WALL_CLOCK not in block["enforced"] + + +# ── containment.probe: the single answer both tools ask for ───────────────── +def test_probe_answers_without_writing_a_grant_record(workspace, monkeypatch, tmp_path): + """A grant record whose pid is never filled in and whose release never runs + is an entry a restart reaper keeps finding, which is why the decision does + not go through acquire().""" + store = tmp_path / "grants.json" + monkeypatch.setattr(containment, "CONTAINMENT_STATE_FILE", str(store), raising=False) + monkeypatch.setattr(containment, "_store_path", lambda: store) + probe = containment.probe( + containment.agent_spec(workspace=workspace, env={}, wall_clock_s=5) + ) + assert probe.mechanism + assert not store.exists() + assert containment.active_grants() == [] + + +def test_probe_and_acquire_agree_on_what_this_host_enforces(workspace, monkeypatch, tmp_path): + """One mechanism table, one answer. A second opinion about what this host + can enforce is the thing the probe exists to prevent.""" + store = tmp_path / "grants.json" + monkeypatch.setattr(containment, "_store_path", lambda: store) + spec = containment.agent_spec(workspace=workspace, env={}, wall_clock_s=5) + probe = containment.probe(spec) + grant = containment.acquire(spec, owner="test") + assert probe.mechanism == grant.mechanism + assert probe.enforced == grant.enforced + assert probe.unenforced_required == grant.unenforced_required + assert probe.contained == grant.contained + + +def test_probe_refuses_only_under_enforcing_mode(workspace, monkeypatch): + spec = containment.agent_spec(workspace=workspace, env={}, wall_clock_s=5) + monkeypatch.setattr(containment, "CONTAINMENT_MODE", containment.MODE_REPORT_ONLY) + assert containment.probe(spec).refuses is False + monkeypatch.setattr(containment, "CONTAINMENT_MODE", containment.MODE_ENFORCING) + probe = containment.probe(spec) + # Only meaningful where something required is actually missing; on a host + # with bubblewrap nothing is. + assert probe.refuses == bool(probe.unenforced_required) + + +def test_probe_rejects_a_malformed_spec_in_either_mode(tmp_path): + missing = tmp_path / "not-a-directory" + with pytest.raises(ValueError, match="not a directory"): + containment.probe( + containment.agent_spec(workspace=str(missing), env={}, wall_clock_s=5) + ) + + +# ── the isolated /tmp stand-in ────────────────────────────────────────────── +def test_isolated_tmp_is_created_inside_the_workspace(workspace): + path = subprocess_tools._isolated_tmp_dir(workspace) + assert os.path.isdir(path) + assert os.path.realpath(path).startswith(os.path.realpath(workspace)) + + +def test_isolated_tmp_degrades_instead_of_raising_on_an_unwritable_workspace( + workspace, monkeypatch, +): + """The source tree is read-only in Docker and a workspace can be mounted + read-only. A command that merely mentions `/tmp/` must not die with an + OSError traceback because a scratch directory could not be made.""" + def _refuse(*args, **kwargs): + raise OSError(30, "Read-only file system") + + monkeypatch.setattr(os, "makedirs", _refuse) + path = subprocess_tools._isolated_tmp_dir(workspace) + assert path == os.path.join(workspace, ".tmp") diff --git a/tests/test_path_confinement_boundary.py b/tests/test_path_confinement_boundary.py new file mode 100644 index 000000000..efea39f22 --- /dev/null +++ b/tests/test_path_confinement_boundary.py @@ -0,0 +1,225 @@ +"""The single filesystem confinement boundary. + +Nine test files in this suite each prove one call site confines correctly, and +each call site had its own ``realpath``/``commonpath`` pair to prove it about. +This file covers the one implementation they now all go through, so a property +is asserted once instead of nine times and inconsistently. + +Each test names the detail the scattered copies disagreed on. The macOS tests +are the ones with history: ``/tmp`` is a symlink to ``/private/tmp`` there, and +comparing a canonicalized candidate against a root that was not canonicalized +has already produced a false failure in this suite. +""" +import os +import sys + +import pytest + +from src.path_confinement import ( + PathEscape, + canonical_root, + confine, + is_inside, +) + + +@pytest.fixture +def root(tmp_path): + """A real directory, canonicalized the way a caller's root should be. + + tmp_path is under ``/private/var/...`` on macOS via a ``/var`` symlink, so + this fixture is itself an instance of the aliasing the module exists to + handle — which is why it is used as-is rather than pre-resolved. + """ + (tmp_path / "inside.txt").write_text("x", encoding="utf-8") + (tmp_path / "sub").mkdir() + return str(tmp_path) + + +# ── the basic shape ───────────────────────────────────────────────────────── +def test_path_under_the_root_resolves(root): + assert confine(root, "inside.txt") == os.path.join(canonical_root(root), "inside.txt") + + +def test_relative_candidate_joins_the_root_not_the_process_cwd(root, tmp_path, monkeypatch): + """abspath() of a relative path silently uses os.getcwd(). + + A confinement helper that does that resolves against whatever directory the + server happens to be running in, which is the wrong base before the + comparison even starts. + """ + elsewhere = tmp_path.parent / "elsewhere" + elsewhere.mkdir() + monkeypatch.chdir(elsewhere) + assert confine(root, "inside.txt") == os.path.join(canonical_root(root), "inside.txt") + + +def test_the_root_itself_is_inside_by_default(root): + assert confine(root, root) == canonical_root(root) + + +def test_allow_root_false_excludes_the_root(root): + """For an operation only meaningful on something *under* the root.""" + with pytest.raises(PathEscape): + confine(root, root, allow_root=False) + assert confine(root, "sub", allow_root=False) + + +def test_a_target_that_does_not_exist_yet_is_confined_not_refused(root): + """A write target is a legitimate thing to confine. + + realpath is the non-strict kind: it resolves what exists and normalizes the + rest, so a new file under the root passes while a new file above it does + not. + """ + assert confine(root, "not-created-yet.txt").startswith(canonical_root(root)) + with pytest.raises(PathEscape): + confine(root, "../not-created-yet.txt") + + +# ── traversal ─────────────────────────────────────────────────────────────── +def test_dotdot_escape_is_refused(root): + with pytest.raises(PathEscape): + confine(root, "../outside.txt") + with pytest.raises(PathEscape): + confine(root, "sub/../../outside.txt") + + +def test_absolute_candidate_outside_the_root_is_refused(root): + with pytest.raises(PathEscape): + confine(root, os.path.dirname(canonical_root(root))) + + +def test_sibling_with_a_shared_prefix_is_not_inside(tmp_path): + """`/a/bc` begins with `/a/b` and is not inside it. + + This is why the boundary uses commonpath and not startswith. A copy written + with startswith accepts the sibling. + """ + (tmp_path / "b").mkdir() + (tmp_path / "bc").mkdir() + (tmp_path / "bc" / "f.txt").write_text("x", encoding="utf-8") + assert not is_inside(tmp_path / "b", tmp_path / "bc" / "f.txt") + assert is_inside(tmp_path / "bc", tmp_path / "bc" / "f.txt") + + +# ── symlinks ──────────────────────────────────────────────────────────────── +@pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX symlinks") +def test_symlink_as_the_final_component_is_followed_before_the_check(root, tmp_path): + outside = tmp_path.parent / "outside-secret.txt" + outside.write_text("secret", encoding="utf-8") + os.symlink(outside, os.path.join(root, "link.txt")) + with pytest.raises(PathEscape): + confine(root, "link.txt") + + +@pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX symlinks") +def test_symlinked_intermediate_directory_is_followed_before_the_check(root, tmp_path): + outside_dir = tmp_path.parent / "outside-dir" + outside_dir.mkdir() + (outside_dir / "f.txt").write_text("secret", encoding="utf-8") + os.symlink(outside_dir, os.path.join(root, "hop")) + with pytest.raises(PathEscape): + confine(root, "hop/f.txt") + + +@pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX symlinks") +def test_symlink_pointing_back_inside_the_root_is_allowed(root): + os.symlink(os.path.join(root, "inside.txt"), os.path.join(root, "loop.txt")) + assert confine(root, "loop.txt") == os.path.join(canonical_root(root), "inside.txt") + + +# ── the aliasing class that has already cost real time ────────────────────── +@pytest.mark.skipif( + not os.path.islink("/tmp"), reason="needs a platform where /tmp is a symlink", +) +def test_root_reached_through_a_symlink_still_contains_its_own_files(tmp_path): + """macOS: /tmp is a symlink to /private/tmp. + + A root given as `/tmp/x` and a candidate that canonicalizes to + `/private/tmp/x/f` describe the same file. Canonicalizing one side and not + the other reads as an escape and refuses a legitimate access — the false + false failure this suite has already recorded. Canonicalizing *neither* + side would agree, which is why the rule is both or nothing. + """ + unresolved_root = os.path.join("/tmp", os.path.basename(str(tmp_path))) + os.makedirs(unresolved_root, exist_ok=True) + try: + target = os.path.join(unresolved_root, "f.txt") + with open(target, "w", encoding="utf-8") as handle: + handle.write("x") + assert os.path.realpath(unresolved_root) != unresolved_root, ( + "fixture assumption: /tmp should not canonicalize to itself here" + ) + # Either spelling of the root, either spelling of the candidate. + assert is_inside(unresolved_root, target) + assert is_inside(unresolved_root, os.path.realpath(target)) + assert is_inside(os.path.realpath(unresolved_root), target) + finally: + try: + os.remove(os.path.join(unresolved_root, "f.txt")) + os.rmdir(unresolved_root) + except OSError: + pass + + +def test_canonical_root_is_idempotent(root): + once = canonical_root(root) + assert canonical_root(once) == once + + +# ── malformed input is a reason, not an accidental "outside" ──────────────── +@pytest.mark.parametrize("bad", ["", " ", None]) +def test_empty_candidate_is_a_value_error_not_an_escape(root, bad): + with pytest.raises(ValueError) as caught: + confine(root, bad) + assert not isinstance(caught.value, PathEscape) + assert "required" in str(caught.value) + + +def test_nul_is_refused_with_a_reason(root): + with pytest.raises(ValueError, match="NUL"): + confine(root, "a\x00b") + + +def test_newline_is_refused_with_a_reason(root): + with pytest.raises(ValueError, match="newline"): + confine(root, "a\nb") + + +def test_is_inside_is_false_for_malformed_input_rather_than_raising(root): + """The predicate form never raises; that is why callers wrapped the old + copies in `except Exception` and reached "outside" by accident.""" + for bad in ("", None, "a\x00b", "a\nb", 17, object()): + assert is_inside(root, bad) is False + assert is_inside(None, "x") is False + + +def test_path_escape_is_a_value_error(root): + """The call sites this replaces raised ValueError and their callers catch + it as such, so the subclass relationship is part of the contract.""" + assert issubclass(PathEscape, ValueError) + with pytest.raises(ValueError): + confine(root, "../elsewhere") + + +def test_path_escape_names_the_root_and_the_candidate(root): + with pytest.raises(PathEscape) as caught: + confine(root, "../elsewhere") + message = str(caught.value) + assert "../elsewhere" in message + assert canonical_root(root) in message + + +# ── the deny list is somebody else's job ──────────────────────────────────── +def test_confinement_does_not_decide_whether_a_path_is_sensitive(root): + """`.ssh` inside the root is inside the root. + + Confinement answers "inside"; the sensitive-file deny list answers + "allowed", and it stays with src.tool_execution, which owns that policy. + Folding the two together here is how a boundary acquires a second job and + then disagrees with itself. + """ + secret = os.path.join(root, ".ssh") + os.makedirs(secret, exist_ok=True) + assert is_inside(root, os.path.join(secret, "id_rsa")) diff --git a/tests/test_personal_dir_symlink_escape.py b/tests/test_personal_dir_symlink_escape.py index 064e12c58..10b543da1 100644 --- a/tests/test_personal_dir_symlink_escape.py +++ b/tests/test_personal_dir_symlink_escape.py @@ -6,14 +6,20 @@ so a symlink placed inside PERSONAL_DIR pointing outside it passes the os.path.commonpath confinement check and lets index_personal_documents read files outside the root. os.path.realpath resolves the symlink before the check. -_resolve_allowed_personal_dir is a closure inside setup_personal_routes, so the -source-level test pins the fix and the behavioural test proves the underlying -confinement principle. +_resolve_allowed_personal_dir is a closure inside setup_personal_routes, so it +cannot be imported and called directly. The resolution now happens in +src.path_confinement, so the behavioural test runs against that boundary and +the source-level test is reduced to the one thing still worth pinning here: +this closure must not grow its own abspath-based check again. """ import ast import os from pathlib import Path +import pytest + +from src.path_confinement import PathEscape, confine + SRC = Path(__file__).resolve().parent.parent / "routes" / "personal_routes.py" @@ -25,16 +31,49 @@ def _function_source(src_text, name): raise AssertionError(f"{name} not found in {SRC}") -def test_confinement_uses_realpath_not_abspath(): +def test_confinement_does_not_rely_on_abspath(): + """The resolver must not reach a confinement verdict through abspath. + + Originally this asserted the presence of the literal ``os.path.realpath``. + The resolution now happens inside ``src.path_confinement.confine``, which + is the point — one boundary instead of a copy per call site — so the + literal is gone while the behaviour is unchanged. What is still worth + pinning at the source level is the negative: this closure must not grow its + own abspath-based check again. + """ body = _function_source(SRC.read_text(), "_resolve_allowed_personal_dir") - assert "os.path.realpath" in body, ( - "_resolve_allowed_personal_dir must use os.path.realpath so a symlink " - "inside PERSONAL_DIR cannot escape the confinement check" - ) assert "os.path.abspath" not in body, ( "os.path.abspath does not resolve symlinks; the confinement check must " "not rely on it" ) + assert "confine(" in body, ( + "the resolver must go through the shared confinement boundary rather " + "than reimplementing one" + ) + + +def test_shared_boundary_refuses_a_symlink_out_of_the_base(tmp_path): + """The behaviour the source assertion used to stand in for. + + A symlink inside the base pointing outside it is refused, and the file it + points at is not reachable through it. This is asserted against the + boundary the resolver now calls, so it covers every call site that shares + it rather than this one closure. + """ + base = tmp_path / "personal" + base.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_text("nope", encoding="utf-8") + os.symlink(outside, base / "escape") + + with pytest.raises(PathEscape): + confine(base, "escape") + with pytest.raises(PathEscape): + confine(base, "escape/secret.txt") + # A real directory inside the base is still reachable. + (base / "real").mkdir() + assert confine(base, "real") == os.path.join(os.path.realpath(base), "real") def test_realpath_catches_symlink_escape(tmp_path): diff --git a/tests/test_workspace_artifact_tool_floor.py b/tests/test_workspace_artifact_tool_floor.py index 3de10155e..897ce541e 100644 --- a/tests/test_workspace_artifact_tool_floor.py +++ b/tests/test_workspace_artifact_tool_floor.py @@ -2260,14 +2260,20 @@ def test_workspace_namespace_rejects_broad_or_symlinked_python_prefixes(monkeypa monkeypatch.setattr(subprocess_tools.shutil, "which", lambda name: "/usr/bin/bwrap") linked_root = tmp_path / "linked-root" linked_root.symlink_to("/", target_is_directory=True) + # Compared against the argv with no interpreter prefix at all: an unsafe + # prefix must add *nothing*. Asserting the absence of a literal + # `--ro-bind ` instead would also fire on a base mount the + # argv makes for its own reasons -- /home and /mnt are read-only binds + # there -- which says nothing about whether the prefix was rejected. + baseline = shlex.split( + subprocess_tools._wrap_workspace_namespace("echo ok", str(tmp_path)) + ) for unsafe_prefix in ("/", "/tmp", "/var", "/home", str(linked_root)): command = subprocess_tools._wrap_workspace_namespace( "echo ok", str(tmp_path), interpreter_prefix=unsafe_prefix, ) args = shlex.split(command) - assert ["--ro-bind", unsafe_prefix, unsafe_prefix] not in [ - args[index:index + 3] for index in range(len(args) - 2) - ] + assert args == baseline, f"prefix {unsafe_prefix} changed the namespace argv" assert ["--tmpfs", "/tmp"] in [ args[index:index + 2] for index in range(len(args) - 1) ] diff --git a/website/configuration-reference.md b/website/configuration-reference.md index a87f574c7..a9f8a29cb 100644 --- a/website/configuration-reference.md +++ b/website/configuration-reference.md @@ -75,7 +75,7 @@ The source tree reads **108** `ODYSSEUS_*` variables: 78 an operator may want to | `ODYSSEUS_MAX_VISUAL_EVIDENCE_FRAMES` | `'3'` | `src/agent_loop.py:15361` | How many video frames one tool result may contribute. Clamped to 1-8. | | `ODYSSEUS_MAX_VISUAL_EVIDENCE_IMAGES` | `'1'` | `src/agent_loop.py:15329` | How many images one tool result may contribute to the model turn. Clamped to 1-8. | | `ODYSSEUS_MCP_ALLOWED_COMMANDS` | `''` | `src/agent_tools/admin_tools.py:140` | Security-relevant. Comma-separated allowlist of MCP launcher basenames the agent may start. Empty by default, and the deny list still wins. | -| `ODYSSEUS_PYTHON_TOOL_SITE_PACKAGES` | `''` | `src/agent_tools/subprocess_tools.py:927` | Security-relevant. Absolute package roots, separated by the platform path separator, exposed to the sandboxed Python tool. Empty exposes none. | +| `ODYSSEUS_PYTHON_TOOL_SITE_PACKAGES` | `''` | `src/agent_tools/subprocess_tools.py:1149` | Security-relevant. Absolute package roots, separated by the platform path separator, exposed to the sandboxed Python tool. Empty exposes none. | | `ODYSSEUS_SCRIPT_HOST` | `'localhost'` | `src/builtin_actions.py:919` | Default host for the run-script action. `localhost`, `127.0.0.1`, `local` and empty run locally; any other value runs over SSH. | | `ODYSSEUS_TOOL_APPROVAL_GATE` | `'0'` | `src/tool_capabilities.py:645` | Security-relevant. Truthy makes tool calls pass through the approval gate. Off by default. | @@ -151,15 +151,15 @@ The source tree reads **108** `ODYSSEUS_*` variables: 78 an operator may want to | Variable | Default | Read in | What it does | |---|---|---|---| -| `ODYSSEUS_SKILL_SEMANTIC_RETRIEVAL` | `'1'` | `services/memory/skills.py:789` | On by default. Set 0, false, no or off to fall back to keyword-only skill retrieval when no vector store is reachable. | -| `ODYSSEUS_SKILL_SEMANTIC_THRESHOLD` | `'0.4'` | `services/memory/skills.py:800` | Minimum semantic score a skill needs to be retrieved. A non-numeric value falls back to the default. | +| `ODYSSEUS_SKILL_SEMANTIC_RETRIEVAL` | `'1'` | `services/memory/skills.py:796` | On by default. Set 0, false, no or off to fall back to keyword-only skill retrieval when no vector store is reachable. | +| `ODYSSEUS_SKILL_SEMANTIC_THRESHOLD` | `'0.4'` | `services/memory/skills.py:807` | Minimum semantic score a skill needs to be retrieved. A non-numeric value falls back to the default. | ### Speech and vision models | Variable | Default | Read in | What it does | |---|---|---|---| -| `ODYSSEUS_GROUNDING_MODEL` | `'google/owlvit-base-patch32'` | `routes/gallery/gallery_routes.py:95` | Object-grounding model id the gallery loads for text-driven selection. | -| `ODYSSEUS_SAM_MODEL` | `'facebook/sam-vit-base'` | `routes/gallery/gallery_routes.py:59` | Segmentation model id the gallery loads for subject selection. | +| `ODYSSEUS_GROUNDING_MODEL` | `'google/owlvit-base-patch32'` | `routes/gallery/gallery_routes.py:96` | Object-grounding model id the gallery loads for text-driven selection. | +| `ODYSSEUS_SAM_MODEL` | `'facebook/sam-vit-base'` | `routes/gallery/gallery_routes.py:60` | Segmentation model id the gallery loads for subject selection. | | `ODYSSEUS_STT_MODEL` | *unset* | `src/agent_tools/media_tools.py:2184` | Default speech-to-text model for media transcription when the tool call does not name one. | | `ODYSSEUS_TTS_CACHE_MAX_BYTES` | `500 * 1024 * 1024` | `services/tts/tts_service.py:47` | Cap on the synthesized-speech cache. A non-numeric value falls back to the default. | @@ -167,7 +167,7 @@ The source tree reads **108** `ODYSSEUS_*` variables: 78 an operator may want to | Variable | Default | Read in | What it does | |---|---|---|---| -| `ODYSSEUS_INTERNAL_BASE` | *unset* | `src/constants.py:179` | Base URL the in-app tool layer uses for loopback HTTP calls. Set it when the app is not reachable at the port it thinks it is bound to. | +| `ODYSSEUS_INTERNAL_BASE` | *unset* | `src/constants.py:190` | Base URL the in-app tool layer uses for loopback HTTP calls. Set it when the app is not reachable at the port it thinks it is bound to. | | `ODYSSEUS_INTERNAL_TOKEN` | *unset* | `core/middleware.py:20` | Security-relevant. Token that lets the in-app tool layer reach admin-gated routes over loopback. Unset generates a fresh per-process token, which is what you want unless something outside the process needs the same value. | ### Integrations (Claude, Codex)