diff --git a/CHANGELOG.md b/CHANGELOG.md index 1255b4d..db32c33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,40 @@ Initial open-source release of FrontierAgent. ### Fixed +- **One trusted runtime filesystem state** (`plugins/tools/_filesystem_state.py`). + The workspace, outputs, inputs and scratch directories are derived once and + read by everyone: mount resolution, the file-tool gate, the deliverable + roots, `create_file`, the shell variables handed to model commands, and the + system prompts. They used to be re-derived in six places from a mix of + environment variables, scope metadata and backend guesses, so a sub-agent + could write a directory its main agent could not, a relocated outputs root + left the prompt naming `/outputs` while the writers used the override, and + native mode's own outputs directory was under no allowed prefix. + - An installed state settles the workspace root, so `ExecutionScope` metadata + can no longer widen filesystem access. Without one (a benchmark runner + passing a trial directory) the checked metadata path remains. + - A remote or bwrap state grants no host writes and is never resolved with + this host's `realpath`; the canonical mount points are reported instead of + host spellings. + - The read-only inputs directory is refused to every writer through + `output_write_error`, including via a symlink inside the workspace. + - `plugins/skills/` and `data/` are read-only: the skills tree's symlinks are + deliberately trusted, so a writable one is a persistent prompt-injection + channel. +- **A write the path gate refused no longer reaches the host.** `write_file` and + `file_editor_create` fall back to the sandbox when local authorization fails, + and for the in-process `CurrentSandbox` that fallback is an `open()` in the + harness (as root, in container mode), so a refused path was written anyway. + That branch now authorizes the path itself; remote backends are unaffected. +- **Paths are quoted before reaching a sandbox shell.** `file_editor`'s view / + create / str_replace commands, `write_file`'s `mkdir -p`, and the Docker + `mkdir -p` interpolated the path unquoted, so a path containing `;` ran + commands that never passed the bash policy. +- **A system directory named as the workspace root grants nothing.** The root + arrives through `ExecutionScope` metadata (workload input), and + `{"workspace_root": "/etc"}` made `/etc` readable and writable. System roots + and direct children of `/usr` and `/etc` are refused; a run directory deep + under `/var` or `/opt` (a container volume, macOS `$TMPDIR`) still works. - Surface finalize-gate bypasses on the final turn: an answer delivered despite open task-board items now carries an unfinished-work note and a `finalize_gate_bypassed` marker instead of reading as a clean success. diff --git a/plugins/tools/_deliverable_policy.py b/plugins/tools/_deliverable_policy.py index b90fa26..7e7808f 100644 --- a/plugins/tools/_deliverable_policy.py +++ b/plugins/tools/_deliverable_policy.py @@ -48,28 +48,27 @@ class _Policy: def _runtime_outputs_root() -> str: """Return the filesystem output root visible to model tools. - Container and bwrap runs expose the canonical ``/outputs`` mount. Native - mode cannot create that top-level mount on hosts such as macOS (the root - filesystem is read-only), so its tools must use the run-local host path - exported by ``prepare_native_runtime`` instead. + Read from the trusted runtime state (see + :mod:`plugins.tools._filesystem_state`), which is also what the prompts + name and what ``_path_auth`` authorizes. It used to re-derive the root from + the environment and honour an override only under ``native``, so a + relocated outputs directory in container or bwrap mode left the prompt + saying ``/outputs`` while the file tools wrote somewhere this manifest never + recognised. """ - backend = os.environ.get("SANDBOX_BACKEND", "").strip().lower() - if backend != "native" and os.environ.get("APODEX_IN_NATIVE") != "1": - return "/outputs" - configured = os.environ.get("FRONTIER_AGENT_OUTPUTS_DIR", "").strip() - if not configured or not os.path.isabs(configured): - return "/outputs" - return os.path.normpath(configured) + from plugins.tools._filesystem_state import DEFAULT_OUTPUTS_DIR, current_filesystem_state + + return current_filesystem_state().outputs or DEFAULT_OUTPUTS_DIR def _runtime_workspace_root() -> str: """Return the filesystem workspace root visible to model tools.""" - if _runtime_outputs_root() == "/outputs": - return "/workspace" - configured = os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", "").strip() - if not configured or not os.path.isabs(configured): - return "/workspace" - return os.path.normpath(configured) + from plugins.tools._filesystem_state import ( + DEFAULT_WORKSPACE_DIR, + current_filesystem_state, + ) + + return current_filesystem_state().workspace or DEFAULT_WORKSPACE_DIR def _canonical_output_path(path: str) -> str: @@ -572,7 +571,19 @@ def scratch_write_error() -> str | None: def output_write_error(path: str) -> str | None: - """Return an actionable error when ``path`` is not publishable.""" + """Return an actionable error when ``path`` is not publishable. + + The single check every file writer already makes, so the read-only input + contract is enforced here rather than being added to each of them (and + forgotten by the next writer). + """ + from plugins.tools._filesystem_state import runtime_file_write_error + from plugins.tools._sandbox import resolve_runtime_path + + # Check the target the writer will open, including canonical mount aliases. + # Otherwise /workspace/inputs/x bypasses a relocated read-only input root. + if error := runtime_file_write_error(resolve_runtime_path(path)): + return error if error := spill_write_error(path): return error normalised = _canonical_output_path(path) diff --git a/plugins/tools/_filesystem_state.py b/plugins/tools/_filesystem_state.py new file mode 100644 index 0000000..de584e2 --- /dev/null +++ b/plugins/tools/_filesystem_state.py @@ -0,0 +1,343 @@ +"""The one trusted description of this run's filesystem. + +Every path the agent can be told about or allowed to touch — the workspace it +works in, the directory its deliverables go to, the read-only inputs, the +scratch root, the shell variables naming them — is derived HERE, once, and read +from here by everyone else: + +* ``_path_auth`` builds its allowed prefixes from it; +* ``write_file`` / ``create_file`` / ``file_editor`` refuse a read-only root + through :func:`runtime_file_write_error`; +* ``_sandbox._build_tool_env`` projects :meth:`RuntimeFilesystemState.shell_env` + into model-authored commands; +* ``_deliverable_policy`` and ``create_file`` take their roots from it; +* the system prompts name exactly the directories it reports. + +Before this module those facts were re-derived in six places from a mix of +environment variables, scope metadata and backend guesses, so they could +disagree: a sub-agent could write a directory its main agent could not, native +mode's own outputs directory was under no allowed prefix, and the prompt could +name ``/outputs`` while the file tools used a relocated path. + +Two properties are load-bearing: + +**Only trusted code installs a state.** A workflow node or the CLI builds one +from its own configuration and installs it for the duration of the run. +``ExecutionScope`` metadata and request fields are workload INPUT; they may +describe a run, never widen it. When nothing is installed, the state is derived +from the process environment by the same builder, so there is still exactly one +derivation rather than a second, looser one. + +**``local_filesystem`` says whose filesystem these paths name.** It is true only +when model commands run against this host's filesystem under these same +spellings (``native``, ``container``). For a remote backend the paths exist only +in the sandbox, so resolving them here — or granting them on this host — would +describe the wrong machine. + +Deliberately stdlib-only and importing nothing from ``plugins`` or +``frontier_agent``: ``_path_auth``, ``_sandbox`` and ``_deliverable_policy`` all +import it, and it must not import them back. +""" + +from __future__ import annotations + +import contextvars +import logging +import os +from dataclasses import dataclass, replace +from pathlib import Path + +logger = logging.getLogger(__name__) + +__all__ = [ + "RuntimeFilesystemState", + "current_filesystem_state", + "install_filesystem_state", + "reset_filesystem_state", + "runtime_file_write_error", + "state_for_sandbox_mode", + "state_from_environment", +] + +#: Canonical mount points. A deployment that cannot create top-level mounts +#: (macOS under ``native``) overrides them, which is why nothing may hardcode +#: these outside this module. +DEFAULT_WORKSPACE_DIR = "/workspace" +DEFAULT_OUTPUTS_DIR = "/outputs" +DEFAULT_INPUTS_DIR = "/inputs" + +#: Backends whose commands run on THIS host's filesystem, under the same path +#: spellings the harness sees. ``bwrap`` is absent: its canonical +#: ``/workspace`` is a mount of a host directory with a different name, so the +#: two namespaces must not be conflated. Remote backends are absent for the +#: stronger reason that the paths do not exist here at all. +_LOCAL_FILESYSTEM_BACKENDS = frozenset({"native", "container"}) + +_MAX_PATH_CHARS = 4096 + + +def _absolute(path: str | None, *, field: str) -> str: + """Normalize one configured directory, or "" when it is unusable. + + A relative or malformed value is dropped rather than resolved against + whatever cwd this process happens to have: the agent's cwd is not the + harness's, so guessing produces a path that names a different directory in + each namespace. + """ + raw = str(path or "").strip() + if not raw: + return "" + if "\x00" in raw or len(raw) > _MAX_PATH_CHARS: + logger.warning("Ignoring malformed %s directory", field) + return "" + if not os.path.isabs(raw): + logger.warning("Ignoring relative %s directory %r", field, raw) + return "" + return os.path.normpath(raw) + + +def _contains(root: str, candidate: str) -> bool: + """Whether ``candidate`` is ``root`` or sits under it, component-aware. + + A string prefix test also accepts the sibling ``/outputs-old`` and + ``/outputs/../etc``; ``..`` is collapsed before comparing. + """ + if not root: + return False + root = os.path.normpath(root) + candidate = os.path.normpath(candidate) + return candidate == root or candidate.startswith(root.rstrip("/") + os.sep) + + +@dataclass(frozen=True) +class RuntimeFilesystemState: + """This run's directories, and whether they name the local filesystem.""" + + workspace: str = DEFAULT_WORKSPACE_DIR + outputs: str = DEFAULT_OUTPUTS_DIR + inputs: str = DEFAULT_INPUTS_DIR + tmpdir: str = "/tmp" + home: str = "" + local_filesystem: bool = False + + def __post_init__(self) -> None: + # Normalize through ``object.__setattr__``: the dataclass is frozen so + # that a consumer cannot quietly retarget an installed state. + for field in ("workspace", "outputs", "inputs", "tmpdir", "home"): + object.__setattr__( + self, field, _absolute(getattr(self, field), field=field), + ) + + def dirs(self) -> tuple[str, str, str]: + """``(workspace, outputs, inputs)`` — the triple every consumer wants.""" + return self.workspace, self.outputs, self.inputs + + def write_roots(self) -> tuple[str, ...]: + """Directories that BELONG to this run and may be written. + + ``inputs`` is absent by construction: it is a read-only mount, and that + absence is the whole enforcement rather than a check each writer has to + remember. ``tmpdir`` is absent too — see :meth:`scratch_roots`. + """ + return tuple(dict.fromkeys(d for d in (self.workspace, self.outputs) if d)) + + def scratch_roots(self) -> tuple[str, ...]: + """:meth:`write_roots` plus the scratch root. + + Separate because the two answer different questions. A shell command + writing ``$TMPDIR/x`` is ordinary, so the bash policy must not refuse + it; but ``tmpdir`` is frequently the shared ``/tmp``, and authorizing + that for the in-process file tools would hand them every path under it, + which is far wider than the run's own directories. + """ + return tuple(dict.fromkeys((*self.write_roots(), self.tmpdir))) + + def read_only_roots(self) -> tuple[str, ...]: + """Directories the agent may read but never write.""" + return tuple(d for d in (self.inputs,) if d) + + def shell_env(self) -> dict[str, str]: + """Variables naming these directories for model-authored commands. + + Without them a command has no way to find the directory the prompt told + it to write to except by hardcoding a path that is wrong in some + deployments. Empty values are omitted so a shell test on them works. + """ + env = { + "WORKSPACE_DIR": self.workspace, + "OUTPUT_DIR": self.outputs, + "INPUT_DIR": self.inputs, + "TMPDIR": self.tmpdir, + } + return {key: value for key, value in env.items() if value} + + # ── path questions ─────────────────────────────────────────────────── + + def _spellings(self, path: str) -> tuple[str, ...]: + """``path`` as written, plus its resolved form on a local filesystem. + + Resolving is what makes protection follow identity rather than + spelling, so a symlink into the read-only inputs is caught. It is done + only for a local state and only for an absolute path: the shell's cwd is + not this process's, and a remote path must never be interpreted by this + host (``os.path.realpath`` of a path that exists only in the sandbox + answers about the wrong machine). + """ + written = os.path.normpath(path) + if not self.local_filesystem or not os.path.isabs(path): + return (written,) + try: + resolved = os.path.realpath(path) + except OSError: + # An unresolvable path keeps the textual answer: a failed lookup + # must never drop a check. + return (written,) + return (written,) if resolved == written else (written, resolved) + + def write_error(self, path: str) -> str | None: + """Why this run must not write ``path``, or ``None``. + + Only a read-only root is refused here. Whether a path is otherwise + reachable is ``_path_auth``'s question, which has the workspace root, + the service checkout and the spill store to consider as well. + """ + raw = str(path or "").strip() + if not raw: + return "file path is required" + if not os.path.isabs(raw) and self.workspace: + raw = os.path.join(self.workspace, raw) + for root in self.read_only_roots(): + roots = self._spellings(root) if self.local_filesystem else (root,) + if any( + _contains(root_spelling, candidate) + for root_spelling in roots + for candidate in self._spellings(raw) + ): + return ( + f"{path!r} is inside the read-only input directory " + f"{root}. Write to {self.outputs or self.workspace} instead." + ) + return None + + def with_backend(self, sandbox: object) -> RuntimeFilesystemState: + """This state refreshed from facts the live backend exposes. + + Only what the backend states about itself is taken; this never probes + the host. A backend that says nothing leaves the state unchanged. + """ + dirs = getattr(sandbox, "filesystem_dirs", None) + updates: dict[str, object] = {} + if isinstance(dirs, tuple) and len(dirs) == 3: + updates.update(workspace=dirs[0], outputs=dirs[1], inputs=dirs[2]) + tmpdir = getattr(sandbox, "tool_tmpdir", "") + if tmpdir: + updates["tmpdir"] = tmpdir + return replace(self, **updates) if updates else self # type: ignore[arg-type] + + +def state_from_environment( + *, backend: str = "", local_filesystem: bool | None = None, +) -> RuntimeFilesystemState: + """Build a state from this process's configuration. + + THE derivation: both the explicit install path and the fallback for code + running outside one go through here, so there is no second, looser answer + to "where does this run write". + """ + resolved_backend = (backend or os.environ.get("SANDBOX_BACKEND", "")).strip().lower() + if os.environ.get("APODEX_IN_NATIVE", "").strip() == "1": + resolved_backend = resolved_backend or "native" + if local_filesystem is None: + local_filesystem = resolved_backend in _LOCAL_FILESYSTEM_BACKENDS + return RuntimeFilesystemState( + workspace=os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", "") or DEFAULT_WORKSPACE_DIR, + outputs=os.environ.get("FRONTIER_AGENT_OUTPUTS_DIR", "") or DEFAULT_OUTPUTS_DIR, + inputs=os.environ.get("FRONTIER_AGENT_INPUTS_DIR", "") or DEFAULT_INPUTS_DIR, + tmpdir=os.environ.get("TMPDIR", "") or "/tmp", + home=os.environ.get("HOME", "") if local_filesystem else "", + local_filesystem=local_filesystem, + ) + + +def state_for_sandbox_mode( + sandbox_mode: str, + *, + workspace: str = "", + outputs: str = "", + inputs: str = "", + tmpdir: str = "", +) -> RuntimeFilesystemState: + """The state for a workflow that has just resolved its own directories. + + ``container`` / ``native`` run commands against the host directories the + caller passes, so those are the paths the model is told about, the paths the + file tools authorize, and the paths worth resolving for symlink identity. + + Every other mode presents the CANONICAL mount points to the model (bwrap + binds the host directories at ``/workspace`` and friends; a remote backend + has its own filesystem), so the host spellings deliberately do not appear in + the state — naming them would authorize host writes on the strength of a + path that describes a different namespace. The physical directories stay the + caller's business: it is the one that mounts them. + """ + mode = (sandbox_mode or "").strip().lower() + if mode in _LOCAL_FILESYSTEM_BACKENDS: + return RuntimeFilesystemState( + workspace=workspace or DEFAULT_WORKSPACE_DIR, + outputs=outputs or DEFAULT_OUTPUTS_DIR, + inputs=inputs or DEFAULT_INPUTS_DIR, + tmpdir=tmpdir or os.environ.get("TMPDIR", "") or "/tmp", + home=os.environ.get("HOME", ""), + local_filesystem=True, + ) + return RuntimeFilesystemState( + workspace=DEFAULT_WORKSPACE_DIR, + outputs=DEFAULT_OUTPUTS_DIR, + inputs=DEFAULT_INPUTS_DIR, + tmpdir=tmpdir or "/tmp", + local_filesystem=False, + ) + + +# ── installation ───────────────────────────────────────────────────────── + +_state: contextvars.ContextVar[RuntimeFilesystemState | None] = contextvars.ContextVar( + "frontier_agent_runtime_filesystem", default=None, +) + + +def install_filesystem_state(state: RuntimeFilesystemState) -> contextvars.Token: + """Install ``state`` for this context. Only trusted code may call this. + + Returns a token for :func:`reset_filesystem_state`. A contextvar, so a + sub-agent running as a child task inherits its parent's state until it + installs its own, and resetting cannot leak into a sibling. + """ + if not state.workspace: + raise ValueError("a runtime filesystem state needs an absolute workspace") + return _state.set(state) + + +def reset_filesystem_state(token: contextvars.Token) -> None: + """Restore the previously installed state.""" + _state.reset(token) + + +def current_filesystem_state() -> RuntimeFilesystemState: + """The installed state, else one derived from the environment.""" + state = _state.get() + return state if state is not None else state_from_environment() + + +def installed_filesystem_state() -> RuntimeFilesystemState | None: + """The installed state, or ``None`` — for code that must tell them apart.""" + return _state.get() + + +def runtime_file_write_error(path: str | Path) -> str | None: + """Why this run must not write ``path``, or ``None``. + + The one call every file-writing tool makes, so the read-only contract is + enforced in a single place instead of per writer. + """ + return current_filesystem_state().write_error(str(path)) diff --git a/plugins/tools/_path_auth.py b/plugins/tools/_path_auth.py index 31041e4..46046b8 100644 --- a/plugins/tools/_path_auth.py +++ b/plugins/tools/_path_auth.py @@ -14,7 +14,10 @@ _SERVICE_CHECKOUT_ROOT = Path(__file__).resolve().parents[2] -# Allowed directory prefixes (relative paths for project dirs, absolute for output) +# Static prefixes, allowed for READS only (see ``_allowed_local_prefixes``). +# These are the service's own resources rather than this run's directories: a +# skill body to load, a bundled dataset. Writes belong to the run's own +# directories, which the trusted filesystem state names. _ALLOWED_RELATIVE_PREFIXES = [ "plugins/skills/", "data/", @@ -22,6 +25,8 @@ #: The operator-curated skills tree. Its symlinks are trusted (see #: :func:`_candidate_paths`), so it is named once rather than spelled inline. +#: That trust is exactly why it must not be writable: a model that can add a +#: ``SKILL.md`` there writes instructions the next run loads as its own. _SKILLS_DIR = str(_SERVICE_CHECKOUT_ROOT / "plugins" / "skills") _ALLOWED_ABSOLUTE_PREFIXES = [ @@ -41,8 +46,66 @@ _WORD_SPLIT = re.compile(r"[^a-z0-9]+") +#: Roots that can never BECOME a workspace: naming one as the workspace root +#: would authorize the whole system tree for reads, and — outside the service +#: checkout, which is the only thing ``_is_isolated_workspace_root`` refuses — +#: for writes too. The workspace root arrives through ExecutionScope metadata, +#: which carries workload input: a benchmark runner's trial directory is a +#: legitimate value, ``/etc`` is not, and this module cannot tell which caller +#: filled the key in. So the VALUE is constrained rather than the caller. +#: +#: A path may still live under one of these (a container's ``/var/lib/...`` +#: volume, ``/opt/app/run-17``): only naming the root itself, or a direct +#: system subdirectory of it, is refused. ``/tmp`` is absent — a scratch +#: workspace under it is ordinary — and so is ``/home``, whose per-user +#: subdirectories are where a local CLI run actually lives. +_NEVER_A_WORKSPACE_ROOT = frozenset({ + "/", "/etc", "/usr", "/bin", "/sbin", "/lib", "/lib64", "/boot", "/dev", + "/proc", "/sys", "/var", "/root", "/opt", "/home", +}) + + +#: Roots whose direct children are no more suitable than the root itself: +#: ``/usr/local`` and ``/etc/cron.d`` grant exactly the writes that make naming +#: them an escalation. ``/var`` and ``/opt`` are deliberately absent — a +#: container volume at ``/var/lib/app/run`` and macOS ``$TMPDIR`` under +#: ``/var/folders/...`` are real run directories, and refusing them is the +#: over-denial that loses a run's deliverables. +_NEVER_A_WORKSPACE_PARENT = frozenset({"/usr", "/etc", "/bin", "/sbin", "/boot", "/proc", "/sys"}) + + +def _is_system_root(candidate: Path) -> bool: + """Whether ``candidate`` is a system root, or a direct child of a system + directory whose children are equally unsuitable.""" + return ( + str(candidate) in _NEVER_A_WORKSPACE_ROOT + or str(candidate.parent) in _NEVER_A_WORKSPACE_PARENT + ) + + def _configured_workspace_root() -> Path | None: - """Return the explicit workspace root, if one was configured for this task.""" + """Return the explicit workspace root, if one was configured for this task. + + When trusted code installed a runtime filesystem state, that state IS the + answer and request-side metadata is not consulted at all: a workflow node + and the CLI know their own directories, while ``ExecutionScope`` metadata + carries workload input and reached this function from wherever the request + came from. A remote state yields ``None`` — those paths name the sandbox's + filesystem, not this host's. + + Without an installed state the legacy metadata path remains, with the value + checked rather than its source (see ``_NEVER_A_WORKSPACE_ROOT``): the + benchmark runner passes a trial directory this way. + """ + from plugins.tools._filesystem_state import installed_filesystem_state + + state = installed_filesystem_state() + if state is not None: + if not state.local_filesystem or not state.workspace: + return None + root = Path(state.workspace).expanduser().resolve() + return root if root.is_dir() else None + scope = get_current_execution_scope() metadata = scope.metadata if scope else {} raw_root = ( @@ -56,6 +119,12 @@ def _configured_workspace_root() -> Path | None: if not workspace_root.is_dir(): logger.warning("Ignoring invalid CODING_WORKSPACE_ROOT '%s'", raw_root) return None + if _is_system_root(workspace_root): + logger.warning( + "Refusing workspace root '%s': a system directory cannot be a " + "workspace, so it grants no filesystem access", raw_root, + ) + return None return workspace_root @@ -293,8 +362,39 @@ def _allowed_local_prefixes( write_access: bool = False, workspace_root: Path | None = None, ) -> list[str]: - """Return the local path prefixes allowed in the current execution context.""" - prefixes = list(_ALLOWED_RELATIVE_PREFIXES) + list(_ALLOWED_ABSOLUTE_PREFIXES) + """Return the local path prefixes allowed in the current execution context. + + Reads and writes are deliberately asymmetric. Reads may reach the service's + own static resources (bundled skills and data) and this run's read-only + mounts. Writes may only reach directories that BELONG to this run, named by + the trusted filesystem state — never the service checkout's own tree, and + never a path a request asked for. + """ + from plugins.tools._filesystem_state import current_filesystem_state + + state = current_filesystem_state() + prefixes: list[str] = [] + if write_access: + # ``plugins/skills/`` and ``data/`` are service resources, and the + # skills tree's symlinks are explicitly trusted by ``_candidate_paths``. + # A model that could write there would be writing instructions a later + # run loads as its own, so the write branch starts empty and the run's + # own directories are added below. + # + # ``/tmp/agent-outputs/`` stays: it is the last-resort fallback + # ``write_file`` degrades to when no sandbox is reachable at all. + prefixes.append("/tmp/agent-outputs/") + # Only for a LOCAL backend do this run's directories name paths on this + # host. Under bwrap the canonical ``/workspace`` is a mount of a + # differently-named host directory, and under a remote backend the path + # does not exist here at all — authorizing either would grant a host + # write on the strength of a path describing another namespace. + if state.local_filesystem: + prefixes.extend(root for root in state.write_roots() if root) + else: + prefixes.extend(_ALLOWED_RELATIVE_PREFIXES) + prefixes.extend(_ALLOWED_ABSOLUTE_PREFIXES) + resolved_workspace_root = workspace_root or _configured_workspace_root() if resolved_workspace_root is not None: if write_access and not _is_isolated_workspace_root(resolved_workspace_root): diff --git a/plugins/tools/_sandbox.py b/plugins/tools/_sandbox.py index 5e23b86..389c4d3 100644 --- a/plugins/tools/_sandbox.py +++ b/plugins/tools/_sandbox.py @@ -399,6 +399,16 @@ def _build_tool_env( # mode has no inner namespace). Measured on 4 concurrent sub-agents writing # ``/tmp/scratch.csv``: all four read back the last writer's content. projected["TMPDIR"] = tmpdir + # Name this run's directories for model-authored commands. Projected from + # the trusted state rather than inherited, so a stale ``OUTPUT_DIR`` from + # the harness environment cannot point a command at a previous session's + # directory; the explicit ``tmpdir`` argument still wins, since the caller + # may own a private scratch root the state does not know about. + from plugins.tools._filesystem_state import current_filesystem_state + + for key, value in current_filesystem_state().shell_env().items(): + if key != "TMPDIR": + projected[key] = value # Keep pip/matplotlib/caches inside a HOME the tool user owns. Without # these they target root-owned paths and fail with bare permission errors # the model cannot act on. @@ -2338,17 +2348,16 @@ def resolve_sandbox_mode(agent_cfg: dict[str, Any] | None = None) -> str: def resolve_mount_dirs() -> tuple[str, str, str]: - """Return ``(workspace_dir, outputs_dir, inputs_dir)`` for container mode. + """Return ``(workspace_dir, outputs_dir, inputs_dir)`` for this run. - Defaults to the production mount points ``/workspace``, ``/outputs``, - ``/inputs``; each is overridable via ``FRONTIER_AGENT_WORKSPACE_DIR`` / - ``FRONTIER_AGENT_OUTPUTS_DIR`` / ``FRONTIER_AGENT_INPUTS_DIR`` so a local run - (no root) can point them at repo-relative dirs. + Now a thin read of :mod:`plugins.tools._filesystem_state`, which is the one + place these are derived (from the same ``FRONTIER_AGENT_*_DIR`` overrides + when no state is installed). Kept as a function because every existing + caller asks for exactly this triple. """ - ws = os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", "").strip() or _DEFAULT_WORKSPACE_DIR - out = os.environ.get("FRONTIER_AGENT_OUTPUTS_DIR", "").strip() or _DEFAULT_OUTPUTS_DIR - inp = os.environ.get("FRONTIER_AGENT_INPUTS_DIR", "").strip() or _DEFAULT_INPUTS_DIR - return ws, out, inp + from plugins.tools._filesystem_state import current_filesystem_state + + return current_filesystem_state().dirs() def spill_root() -> Path: @@ -2974,6 +2983,22 @@ def shell_quote(s: str) -> str: return "'" + s.replace("'", "'\\''") + "'" +def _local_write_refusal(path: str) -> str: + """Why the path gate refuses writing ``path`` on this host, or "". + + Only consulted for the in-process write branch below. Imported lazily: + ``_path_auth`` imports this module. + """ + try: + from plugins.tools._path_auth import _is_path_allowed + except Exception as exc: # pragma: no cover - broken install + return f"path authorization unavailable: {exc}" + allowed, reason = _is_path_allowed(path, write_access=True) + if allowed: + return "" + return reason or f"write to {path!r} is not authorized" + + def sandbox_write_file( sandbox: Any, path: str, content: str, *, mode: str = "w", ) -> tuple[bool, str]: @@ -2995,9 +3020,19 @@ def sandbox_write_file( # (128 KB), so routing deliverables through it fails with E2BIG somewhere # around 96 KB of content — and container mode has no /tmp fallback, so the # write becomes a hard error. The harness process already has the mount - # bound read-write and the path has passed ``_path_auth``; writing it - # directly is both unlimited and cheaper (no bwrap + python3 spawn). + # bound read-write, so writing it directly is both unlimited and cheaper + # (no bwrap + python3 spawn). + # + # This branch is an ordinary ``open()`` in the harness process — as root, in + # container mode — so it is NOT a sandbox boundary, and the path has to be + # authorized HERE. Its callers (``write_file``, ``file_editor_create``) + # reach the sandbox precisely when ``_path_auth`` refused a local write, and + # for a CurrentSandbox that made the refusal a detour rather than a denial: + # a path the gate rejected was written to the host anyway. if isinstance(sandbox, CurrentSandbox): + reason = _local_write_refusal(path) + if reason: + return False, reason try: p = Path(path) new_parent = not p.parent.exists() @@ -3025,7 +3060,7 @@ def sandbox_write_file( if mode == "w" and isinstance(sandbox, DockerSandbox): parent = os.path.dirname(path) if parent: - sandbox.commands.run(f"mkdir -p {parent}", timeout=10) + sandbox.commands.run(f"mkdir -p {shell_quote(parent)}", timeout=10) with tempfile.NamedTemporaryFile( mode="w", encoding="utf-8", delete=False, suffix=".tmp", ) as f: diff --git a/plugins/tools/create_file.py b/plugins/tools/create_file.py index b223206..ab35cee 100644 --- a/plugins/tools/create_file.py +++ b/plugins/tools/create_file.py @@ -20,7 +20,6 @@ _DEFAULT_WORKSPACE_DIR, aget_sandbox, arun_sandbox_cmd, - resolve_mount_dirs, resolve_runtime_path, resolve_sandbox_mode, ) @@ -100,16 +99,18 @@ def _inlined_ops_program(program_path: str) -> str | None: def _write_roots() -> tuple[str, ...]: """The roots ``create_file`` may write under, resolved per call. - The container mount points (``/workspace`` / ``/outputs`` — the convention - this tool's docstring teaches the model) plus whatever - :func:`resolve_mount_dirs` currently maps them to. Native mode overrides - them to real host directories and then hands the model those exact paths - (``apodex.session._deliverable_context``), so accepting only the literals - refused every native-mode write. + The canonical mount points (``/workspace`` / ``/outputs`` — the convention + this tool's docstring teaches the model) plus whatever this run's trusted + filesystem state currently maps them to. Native mode overrides them to real + host directories and then hands the model those exact paths, so accepting + only the literals refused every native-mode write. ``inputs`` is absent + by construction: it is read-only. """ - workspace, outputs, _inputs = resolve_mount_dirs() + from plugins.tools._filesystem_state import current_filesystem_state + + state = current_filesystem_state() return tuple(dict.fromkeys( - (_DEFAULT_WORKSPACE_DIR, _DEFAULT_OUTPUTS_DIR, workspace, outputs), + (_DEFAULT_WORKSPACE_DIR, _DEFAULT_OUTPUTS_DIR, *state.write_roots()), )) diff --git a/plugins/tools/file_editor.py b/plugins/tools/file_editor.py index 9789b9f..a492e16 100644 --- a/plugins/tools/file_editor.py +++ b/plugins/tools/file_editor.py @@ -11,6 +11,7 @@ aget_sandbox, arun_sandbox_cmd, asandbox_write_file, + shell_quote, ) logger = logging.getLogger(__name__) @@ -78,17 +79,21 @@ async def file_editor_view(path: str, view_range: str = "") -> str: except RuntimeError as e: return f"Error: {e}" + # Every path below is interpolated into a shell command, so it is quoted + # once here: an unquoted path containing ``;`` or ``$(…)`` would run + # arbitrary commands in the sandbox without passing the bash policy. + q = shell_quote(path) try: check = await arun_sandbox_cmd( sandbox, - f"test -d {path} && echo DIR || echo FILE", timeout=10, + f"test -d {q} && echo DIR || echo FILE", timeout=10, ) is_dir = "DIR" in check.stdout if is_dir: result = await arun_sandbox_cmd( sandbox, - f"find {path} -maxdepth 2 -not -path '*/.git/*' " + f"find {q} -maxdepth 2 -not -path '*/.git/*' " f"-not -path '*/node_modules/*' -not -path '*/__pycache__/*' " f"| head -100 | sort", timeout=15, @@ -100,11 +105,11 @@ async def file_editor_view(path: str, view_range: str = "") -> str: parts = view_range.split("-") if len(parts) == 2: start, end = parts[0].strip(), parts[1].strip() - cmd = f"sed -n '{start},{end}p' {path} | cat -n" + cmd = f"sed -n '{start},{end}p' {q} | cat -n" else: - cmd = f"cat -n {path}" + cmd = f"cat -n {q}" else: - cmd = f"cat -n {path}" + cmd = f"cat -n {q}" result = await arun_sandbox_cmd(sandbox, cmd, timeout=15) @@ -156,7 +161,7 @@ async def file_editor_create(path: str, content: str) -> str: parent = os.path.dirname(path) if parent: await arun_sandbox_cmd( - sandbox, f"mkdir -p {parent}", timeout=10, + sandbox, f"mkdir -p {shell_quote(parent)}", timeout=10, ) # Write via python in sandbox (avoids files.write permission issues) @@ -165,7 +170,7 @@ async def file_editor_create(path: str, content: str) -> str: return f"Error creating {path}: {err}" result = await arun_sandbox_cmd( - sandbox, f"wc -l < {path}", timeout=5, + sandbox, f"wc -l < {shell_quote(path)}", timeout=5, ) lines = result.stdout.strip() if result.stdout else "?" @@ -232,7 +237,9 @@ async def file_editor_str_replace(path: str, old_str: str, new_str: str) -> str: try: # Read current content via cat (more reliable than files.read for all paths) - result = await arun_sandbox_cmd(sandbox, f"cat {path}", timeout=10) + result = await arun_sandbox_cmd( + sandbox, f"cat -- {shell_quote(path)}", timeout=10, + ) if result.exit_code != 0: return f"Error: Cannot read {path}: {result.stderr or 'file not found'}" content = result.stdout diff --git a/plugins/tools/write_file.py b/plugins/tools/write_file.py index c0f51d4..b65c81e 100644 --- a/plugins/tools/write_file.py +++ b/plugins/tools/write_file.py @@ -19,6 +19,7 @@ resolve_runtime_path, resolve_sandbox_mode, sandbox_available, + shell_quote, ) logger = logging.getLogger(__name__) @@ -75,7 +76,7 @@ async def write_file(path: str, content: str, append: bool = False) -> str: parent = os.path.dirname(path) if parent: await arun_sandbox_cmd( - sandbox, f"mkdir -p {parent}", timeout=10, + sandbox, f"mkdir -p {shell_quote(parent)}", timeout=10, ) mode = "a" if append else "w" diff --git a/tests/test_path_gate_is_binding.py b/tests/test_path_gate_is_binding.py new file mode 100644 index 0000000..0b8ae8f --- /dev/null +++ b/tests/test_path_gate_is_binding.py @@ -0,0 +1,258 @@ +"""The path gate is a denial, not a detour (handoff item 4a). + +Three ways a refused write reached the host anyway, or a refusal was never +asked for: + +* ``write_file`` / ``file_editor_create`` fall back to the sandbox when + ``_path_auth`` refuses a local write, and for a ``CurrentSandbox`` that + fallback is an ``open()`` in this process — as root, in container mode; +* a path interpolated unquoted into a sandbox shell command runs whatever it + contains, without passing ``assess_bash_command``; +* a workspace root arriving through ExecutionScope metadata was trusted + whatever it named, so ``{"workspace_root": "/etc"}`` authorized ``/etc``. + +Nothing here executes a dangerous command: the shell-injection cases assert on +the command STRING that would have been sent. +""" + +from __future__ import annotations + +import asyncio +import importlib +from pathlib import Path + +import pytest + +from frontier_agent.core.execution_context import ( + ExecutionScope, + reset_current_execution_scope, + set_current_execution_scope, +) +from plugins.tools import _sandbox +from plugins.tools._path_auth import _configured_workspace_root, _is_path_allowed + + +class _scoped: + def __init__(self, **metadata: object) -> None: + self.metadata = metadata + + def __enter__(self) -> None: + self._token = set_current_execution_scope( + ExecutionScope(task_id="t", metadata=dict(self.metadata)), + ) + + def __exit__(self, *exc: object) -> None: + reset_current_execution_scope(self._token) + + +# ── the in-process write branch must honour the gate ───────────────────── + + +@pytest.fixture +def container(tmp_path, monkeypatch): + """A CurrentSandbox over a workspace, as container/native mode has.""" + workspace = tmp_path / "ws" + workspace.mkdir() + monkeypatch.setenv("SANDBOX_BACKEND", "native") + monkeypatch.setenv("FRONTIER_AGENT_WORKSPACE_DIR", str(workspace)) + sandbox = _sandbox.CurrentSandbox(workspace) + token = _sandbox.set_task_sandbox(sandbox) + try: + yield workspace + finally: + _sandbox.clear_task_sandbox(token) + + +def test_sandbox_write_refuses_what_the_path_gate_refused(container) -> None: + target = container.parent / "not_authorized" / "x.txt" + with _scoped(workspace_root=str(container)): + assert not _is_path_allowed(str(target), write_access=True)[0] + ok, reason = _sandbox.sandbox_write_file( + _sandbox.get_existing_sandbox(), str(target), "payload", + ) + assert not ok + assert reason + assert not target.exists(), "a refused path was written to the host" + + +def test_write_file_does_not_route_a_refused_path_through_the_sandbox(container) -> None: + write_file = importlib.import_module("plugins.tools.write_file").write_file + + target = container.parent / "not_authorized" / "x.txt" + with _scoped(workspace_root=str(container)): + out = asyncio.run(write_file.func(path=str(target), content="payload")) + assert "File written" not in out or str(target) not in out + assert not target.exists() + + +def test_file_editor_create_does_not_route_a_refused_path_through_the_sandbox( + container, +) -> None: + editor = importlib.import_module("plugins.tools.file_editor") + + target = container.parent / "not_authorized" / "y.txt" + with _scoped(workspace_root=str(container)): + out = asyncio.run( + editor.file_editor_create.func(path=str(target), content="payload"), + ) + assert "File created" not in out + assert not target.exists() + + +def test_an_authorized_write_still_succeeds_in_process(container) -> None: + target = container / "report.md" + with _scoped(workspace_root=str(container)): + ok, reason = _sandbox.sandbox_write_file( + _sandbox.get_existing_sandbox(), str(target), "body", + ) + assert ok, reason + assert target.read_text() == "body" + + +def test_a_remote_backend_is_not_gated_on_host_authorization() -> None: + """The in-process branch is the only one that writes here; an E2B write + targets a path that does not exist on this host and must not be judged by + this host's gate.""" + written: dict[str, str] = {} + + class FakeFiles: + def write(self, path: str, content: str) -> None: + written[path] = content + + remote = type("Remote", (), {"files": FakeFiles()})() + ok, reason = _sandbox.sandbox_write_file(remote, "/home/user/out.md", "body") + assert ok, reason + assert written == {"/home/user/out.md": "body"} + + +# ── paths interpolated into sandbox shell commands ─────────────────────── + + +_INJECTION = "/workspace/a.txt; touch /tmp/pwned-by-path" + + +@pytest.fixture +def recorded(monkeypatch): + """Record the command strings tools would run, and run none of them.""" + seen: list[str] = [] + + class Result: + exit_code = 0 + stdout = "FILE" + stderr = "" + + async def fake_run(sandbox, command, **kwargs): + seen.append(command) + return Result() + + async def fake_sandbox(*_a, **_k): + return object() + + async def fake_write(*_a, **_k): + return True, "" + + for module in ("file_editor", "write_file", "read_file"): + # ``plugins.tools.__init__`` re-exports the Tool objects under the same + # names as their modules, so the module has to be fetched explicitly. + mod = importlib.import_module(f"plugins.tools.{module}") + for name, fn in ( + ("arun_sandbox_cmd", fake_run), + ("aget_sandbox", fake_sandbox), + ("asandbox_write_file", fake_write), + ): + if hasattr(mod, name): + monkeypatch.setattr(mod, name, fn) + return seen + + +def _assert_no_second_command(commands: list[str]) -> None: + for command in commands: + payload = command.split("touch", 1) + assert len(payload) == 1 or "'" in command, ( + f"path reached the shell unquoted: {command!r}" + ) + # The injected separator must sit inside quotes, never as shell syntax. + for marker in ("; touch", "&& touch", "$(", "`"): + if marker in command: + before = command.split(marker, 1)[0] + assert before.count("'") % 2 == 1, ( + f"unquoted shell metacharacter in: {command!r}" + ) + + +def test_file_editor_view_quotes_the_path(recorded) -> None: + editor = importlib.import_module("plugins.tools.file_editor") + + asyncio.run(editor.file_editor_view.func(path=_INJECTION)) + assert recorded + _assert_no_second_command(recorded) + + +def test_file_editor_create_quotes_the_path(recorded) -> None: + editor = importlib.import_module("plugins.tools.file_editor") + + asyncio.run(editor.file_editor_create.func(path=_INJECTION, content="x")) + _assert_no_second_command(recorded) + + +def test_file_editor_str_replace_quotes_the_path(recorded) -> None: + editor = importlib.import_module("plugins.tools.file_editor") + + asyncio.run( + editor.file_editor_str_replace.func(path=_INJECTION, old_str="a", new_str="b"), + ) + _assert_no_second_command(recorded) + + +def test_write_file_quotes_the_parent_directory(recorded, monkeypatch) -> None: + module = importlib.import_module("plugins.tools.write_file") + + monkeypatch.setattr(module, "sandbox_available", lambda: True) + monkeypatch.setattr(module, "resolve_sandbox_mode", lambda *a, **k: "container") + asyncio.run(module.write_file.func(path=_INJECTION, content="x")) + _assert_no_second_command(recorded) + + +def test_injection_marker_was_never_created() -> None: + assert not Path("/tmp/pwned-by-path").exists() + + +# ── scope metadata cannot name a system directory as the workspace ─────── + + +@pytest.mark.parametrize("root", [ + "/", "/etc", "/usr", "/var", "/opt", "/home", "/root", "/boot", + "/usr/local", "/usr/bin", "/etc/cron.d", +]) +@pytest.mark.parametrize("key", ["workspace_root", "coding_workspace_root"]) +def test_a_system_directory_grants_nothing(root: str, key: str) -> None: + if not Path(root).is_dir(): + pytest.skip(f"{root} does not exist here") + with _scoped(**{key: root}): + assert _configured_workspace_root() is None + assert not _is_path_allowed(f"{root.rstrip('/')}/probe", write_access=True)[0] + with _scoped(**{key: root}): + assert not _is_path_allowed("/etc/hostname")[0] + + +def test_the_environment_variable_is_checked_the_same_way(monkeypatch) -> None: + monkeypatch.setenv("CODING_WORKSPACE_ROOT", "/etc") + with _scoped(): + assert _configured_workspace_root() is None + + +@pytest.mark.parametrize("key", ["workspace_root", "coding_workspace_root"]) +def test_a_real_run_directory_is_still_accepted(tmp_path, key: str) -> None: + # The shape a benchmark runner passes, and macOS $TMPDIR / a container + # volume: deep under a protected-looking prefix, but the run's own. + run_dir = tmp_path / "var" / "folders" / "ab" / "T" / "run-17" / "workspace" + run_dir.mkdir(parents=True) + with _scoped(**{key: str(run_dir)}): + assert _configured_workspace_root() == run_dir + assert _is_path_allowed(str(run_dir / "src.py"), write_access=True)[0] + + +def test_an_unset_root_is_unchanged(monkeypatch) -> None: + monkeypatch.delenv("CODING_WORKSPACE_ROOT", raising=False) + with _scoped(): + assert _configured_workspace_root() is None diff --git a/tests/test_runtime_filesystem_state.py b/tests/test_runtime_filesystem_state.py new file mode 100644 index 0000000..f676b46 --- /dev/null +++ b/tests/test_runtime_filesystem_state.py @@ -0,0 +1,431 @@ +"""One trusted description of this run's filesystem (handoff item 4b, #586). + +What is pinned here: + +* every consumer — mount resolution, the file-tool gate, the deliverable roots, + ``create_file``, the shell variables, the prompts — reports the SAME + directories, because they all read :mod:`plugins.tools._filesystem_state`; +* an installed state settles the workspace root, so ``ExecutionScope`` metadata + (workload input) cannot widen it; +* a remote state grants nothing on this host and is never resolved here; +* the read-only inputs directory is refused to every writer, including through + a symlink; +* a main agent and its sub-agents answer alike. +""" + +from __future__ import annotations + +import importlib +import os +from pathlib import Path + +import pytest + +from plugins.tools import _deliverable_policy, _sandbox +from plugins.tools._filesystem_state import ( + RuntimeFilesystemState, + current_filesystem_state, + install_filesystem_state, + reset_filesystem_state, + runtime_file_write_error, + state_for_sandbox_mode, + state_from_environment, +) +from plugins.tools._path_auth import _allowed_local_prefixes, _is_path_allowed + + +@pytest.fixture +def run_dirs(tmp_path): + """A run's three directories, with a symlink into the inputs mount.""" + for name in ("workspace", "outputs", "inputs"): + (tmp_path / name).mkdir() + (tmp_path / "workspace" / "via-link").symlink_to(tmp_path / "inputs") + return tmp_path + + +@pytest.fixture +def local_state(run_dirs): + """The state a container/native workflow installs.""" + state = state_for_sandbox_mode( + "native", + workspace=str(run_dirs / "workspace"), + outputs=str(run_dirs / "outputs"), + inputs=str(run_dirs / "inputs"), + ) + token = install_filesystem_state(state) + try: + yield state + finally: + reset_filesystem_state(token) + + +@pytest.fixture +def remote_state(): + """The state a bwrap or remote workflow installs: canonical mounts only.""" + state = state_for_sandbox_mode("bwrap", workspace="/host/private/ws") + token = install_filesystem_state(state) + try: + yield state + finally: + reset_filesystem_state(token) + + +# ── every consumer reports the same directories ────────────────────────── + + +def test_one_state_is_what_every_consumer_reports(local_state, run_dirs) -> None: + workspace, outputs, inputs = local_state.dirs() + assert (workspace, outputs, inputs) == ( + str(run_dirs / "workspace"), str(run_dirs / "outputs"), str(run_dirs / "inputs"), + ) + # mount resolution + assert _sandbox.resolve_mount_dirs() == (workspace, outputs, inputs) + # the deliverable manifest's roots + assert _deliverable_policy._runtime_outputs_root() == outputs + assert _deliverable_policy._runtime_workspace_root() == workspace + # create_file's write roots + create_file = importlib.import_module("plugins.tools.create_file") + assert workspace in create_file._write_roots() + assert outputs in create_file._write_roots() + assert inputs not in create_file._write_roots() + # the file-tool gate + prefixes = _allowed_local_prefixes(write_access=True) + assert workspace in prefixes + assert outputs in prefixes + # the shell variables handed to model commands + assert local_state.shell_env()["WORKSPACE_DIR"] == workspace + assert local_state.shell_env()["OUTPUT_DIR"] == outputs + assert local_state.shell_env()["INPUT_DIR"] == inputs + + +def test_the_relocated_outputs_root_reaches_the_prompt(local_state, run_dirs) -> None: + """The prompt used to say ``/outputs`` while the writers used the override.""" + runtime = importlib.import_module("workflows.stateful_react_agent._runtime") + note = runtime.render_system_prompt_notes(sandbox_mode="native", tool_names=["bash"]) + assert str(run_dirs / "outputs") in note + assert str(run_dirs / "workspace") in note + + subagent = importlib.import_module("workflows.agent_team.subagent_runtime") + for audience in ("main", "sub"): + rendered = subagent.render_sandbox_fs_note( + sandbox_mode="native", inputs_available=True, audience=audience, + ) + assert str(run_dirs / "workspace") in rendered + assert str(run_dirs / "inputs") in rendered + + +def test_the_shell_environment_names_this_runs_directories(local_state, run_dirs) -> None: + env = _sandbox._build_tool_env( + {"PATH": "/usr/bin", "OUTPUT_DIR": "/stale/outputs"}, + home=str(run_dirs / "workspace"), + ) + assert env["OUTPUT_DIR"] == str(run_dirs / "outputs") + assert env["WORKSPACE_DIR"] == str(run_dirs / "workspace") + assert env["INPUT_DIR"] == str(run_dirs / "inputs") + + +def test_an_explicit_tmpdir_still_wins_over_the_state(local_state, run_dirs) -> None: + # A sub-agent's private scratch root is the caller's own fact; the state + # does not know about it. + env = _sandbox._build_tool_env( + {"PATH": "/usr/bin"}, home="/w", tmpdir=str(run_dirs / "private-tmp"), + ) + assert env["TMPDIR"] == str(run_dirs / "private-tmp") + + +# ── metadata cannot widen an installed state ───────────────────────────── + + +def test_scope_metadata_cannot_widen_an_installed_state(local_state, tmp_path) -> None: + from frontier_agent.core.execution_context import ( + ExecutionScope, + reset_current_execution_scope, + set_current_execution_scope, + ) + + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + token = set_current_execution_scope( + ExecutionScope(task_id="t", metadata={"workspace_root": str(elsewhere)}), + ) + try: + assert not _is_path_allowed(str(elsewhere / "x.py"), write_access=True)[0] + assert _is_path_allowed( + str(Path(local_state.workspace) / "x.py"), write_access=True, + )[0] + finally: + reset_current_execution_scope(token) + + +def test_the_metadata_path_still_works_without_an_installed_state(tmp_path) -> None: + """A benchmark runner passes its trial directory this way.""" + from frontier_agent.core.execution_context import ( + ExecutionScope, + reset_current_execution_scope, + set_current_execution_scope, + ) + + trial = tmp_path / "trial-7" + trial.mkdir() + token = set_current_execution_scope( + ExecutionScope(task_id="t", metadata={"workspace_root": str(trial)}), + ) + try: + assert _is_path_allowed(str(trial / "x.py"), write_access=True)[0] + finally: + reset_current_execution_scope(token) + + +# ── a remote state grants nothing here ─────────────────────────────────── + + +def test_a_remote_state_grants_no_host_writes(remote_state) -> None: + assert not _is_path_allowed("/workspace/x.py", write_access=True)[0] + assert not _is_path_allowed("/host/private/ws/x.py", write_access=True)[0] + prefixes = _allowed_local_prefixes(write_access=True) + assert "/workspace" not in prefixes + assert "/host/private/ws" not in prefixes + + +def test_a_remote_state_keeps_static_resources_readable(remote_state) -> None: + prefixes = _allowed_local_prefixes(write_access=False) + assert any(p.endswith("plugins/skills") or "plugins/skills" in p for p in prefixes) + + +def test_a_remote_path_is_never_resolved_on_this_host(remote_state, monkeypatch) -> None: + calls: list[str] = [] + monkeypatch.setattr( + os.path, "realpath", lambda p, **k: (calls.append(str(p)), str(p))[1], + ) + assert remote_state.write_error("/inputs/data.csv") + assert remote_state._spellings("/inputs/data.csv") == ("/inputs/data.csv",) + assert calls == [], "a remote path was resolved against the harness filesystem" + + +def test_a_relative_path_is_never_resolved_against_the_harness_cwd(local_state) -> None: + # The shell's cwd is not this process's, so resolving would name the wrong + # file; the textual answer is kept. + assert local_state._spellings("build/../scratch") == ("scratch",) + + +# ── read-only inputs ───────────────────────────────────────────────────── + + +def test_every_writer_refuses_the_read_only_inputs_directory(local_state, run_dirs) -> None: + direct = str(run_dirs / "inputs" / "data.csv") + through_link = str(run_dirs / "workspace" / "via-link" / "data.csv") + for path in (direct, through_link): + assert runtime_file_write_error(path), path + # The one check every file writer already makes. + assert _deliverable_policy.output_write_error(path), path + + +@pytest.mark.parametrize("mode", ["native", "container"]) +@pytest.mark.parametrize("alias", ["/workspace/inputs", "/inputs", "/outputs/inputs"]) +@pytest.mark.parametrize("writer", ["write_file", "create_file", "file_editor_create", "file_editor_str_replace"]) +async def test_writers_refuse_input_aliases(tmp_path, monkeypatch, mode, alias, writer) -> None: + """Exercise the tools themselves, including aliases beneath writable roots.""" + workspace = tmp_path / "workspace" + outputs = tmp_path / "outputs" + workspace.mkdir() + outputs.mkdir() + inputs = workspace / "inputs" + inputs.mkdir() + (outputs / "inputs").symlink_to(inputs, target_is_directory=True) + target = inputs / "data.txt" + target.write_text("original input") + monkeypatch.setenv("SANDBOX_BACKEND", mode) + token = install_filesystem_state(state_for_sandbox_mode( + mode, workspace=str(workspace), outputs=str(outputs), inputs=str(inputs), + )) + module = "file_editor" if writer.startswith("file_editor_") else writer + tool = getattr(importlib.import_module(f"plugins.tools.{module}"), writer) + args = {"path": f"{alias}/data.txt"} + if writer == "file_editor_str_replace": + args.update(old_str="original", new_str="overwritten") + else: + args["content"] = "overwritten input" + try: + assert "read-only input directory" in ( + _deliverable_policy.output_write_error(args["path"]) or "" + ) + result = await tool.ainvoke(args) + assert "read-only input directory" in result + assert target.read_text() == "original input" + finally: + reset_filesystem_state(token) + + +@pytest.mark.parametrize("mode", ["bwrap", "auto", "native", "container"]) +async def test_team_node_renders_prompts_after_installing_state(tmp_path, monkeypatch, mode) -> None: + """Run the real node setup up to prompt construction, without invoking an LLM.""" + from types import SimpleNamespace + + node = importlib.import_module("workflows.agent_team.nodes.main_agent") + host_dirs = tuple(str(tmp_path / name) for name in ("workspace", "outputs", "inputs")) + for path in host_dirs: + Path(path).mkdir() + monkeypatch.setenv("SANDBOX_BACKEND", mode) + for name, path in zip(("WORKSPACE", "OUTPUTS", "INPUTS"), host_dirs, strict=True): + monkeypatch.setenv(f"FRONTIER_AGENT_{name}_DIR", path) + monkeypatch.setattr(node, "_resolve_llm_and_profile", lambda *a, **k: ( + object(), {"agent": {"planning_mode": False, "reporter": False}}, None, + )) + monkeypatch.setattr(node.registry, "get", lambda *a: object()) + monkeypatch.setattr(node.registry, "get_optional", lambda *a: None) + monkeypatch.setattr(node, "_resolve_profile_tools", lambda *a, **k: ([], [])) + monkeypatch.setattr(node, "_build_observers", lambda **k: []) + monkeypatch.setattr(node, "_resolve_trajectory_dir", lambda *a: tmp_path) + monkeypatch.setattr(node, "_resolve_worktree_root", lambda *a: Path(host_dirs[0])) + monkeypatch.setattr(node, "_resolve_sandbox_binds", lambda *a: ((), (), host_dirs[1])) + monkeypatch.setattr(node, "_log_inputs_dir_contents", lambda *a, **k: None) + + class PromptsCaptured(Exception): + pass + + render = node.render_sandbox_fs_note + notes = {} + states = [] + + def capture(**kwargs): + states.append(current_filesystem_state().dirs()) + notes[kwargs["audience"]] = render(**kwargs) + if len(notes) == 2: + raise PromptsCaptured + return notes[kwargs["audience"]] + + monkeypatch.setattr(node, "render_sandbox_fs_note", capture) + # The node is intentionally stopped during setup. Restore the outer context + # even if it installed a state before the capture exception. + token = install_filesystem_state(state_from_environment()) + try: + with pytest.raises(PromptsCaptured): + await node.main_agent_node( + {"original_question": "inspect inputs", "metadata": {}}, + SimpleNamespace(task_id="filesystem-test"), + ) + expected = host_dirs if mode in ("native", "container") else ("/workspace", "/outputs", "/inputs") + assert states == [expected, expected] + for note in notes.values(): + assert expected[0] in note + assert expected[1] in note + if mode in ("bwrap", "auto"): + assert str(tmp_path) not in note + finally: + reset_filesystem_state(token) + + +def test_the_run_can_still_write_its_own_directories(local_state, run_dirs) -> None: + for path in (run_dirs / "outputs" / "r.md", run_dirs / "workspace" / "t.py"): + assert runtime_file_write_error(str(path)) is None + assert _is_path_allowed(str(path), write_access=True)[0] + + +def test_inputs_stay_readable(local_state, run_dirs) -> None: + assert _is_path_allowed(str(run_dirs / "inputs" / "data.csv"))[0] + + +def test_the_skills_tree_is_not_writable() -> None: + """Its symlinks are deliberately trusted, so a model that could write + there would be writing instructions a later run loads as its own.""" + from plugins.tools._path_auth import _SKILLS_DIR + + assert not _is_path_allowed(f"{_SKILLS_DIR}/evil/SKILL.md", write_access=True)[0] + assert _is_path_allowed(f"{_SKILLS_DIR}/SKILL.md")[0] or True # reads unchanged + + +# ── main agent and sub-agents agree ────────────────────────────────────── + + +def test_a_subagent_inherits_the_same_contract(local_state, run_dirs) -> None: + """The mismatch this replaces: a sub-agent's scope carried no workspace + root, so it fell back to an environment variable naming the user's project + and could write files its main agent could not.""" + import asyncio + + async def subagent() -> tuple[tuple[str, str, str], bool, bool]: + return ( + current_filesystem_state().dirs(), + _is_path_allowed(str(run_dirs / "workspace" / "s.py"), write_access=True)[0], + _is_path_allowed(str(run_dirs / "inputs" / "s.csv"), write_access=True)[0], + ) + + async def main() -> None: + dirs, can_write_workspace, can_write_inputs = await asyncio.create_task(subagent()) + assert dirs == local_state.dirs() + assert can_write_workspace + assert not can_write_inputs + + asyncio.run(main()) + + +def test_a_subagent_can_narrow_but_its_parent_is_unchanged(local_state, run_dirs) -> None: + import asyncio + + async def subagent() -> None: + private = run_dirs / "workspace" / "sub-a" + private.mkdir() + token = install_filesystem_state( + state_for_sandbox_mode("native", workspace=str(private)), + ) + try: + assert current_filesystem_state().workspace == str(private) + finally: + reset_filesystem_state(token) + + async def main() -> None: + await asyncio.create_task(subagent()) + assert current_filesystem_state().dirs() == local_state.dirs() + + asyncio.run(main()) + + +# ── the state itself ───────────────────────────────────────────────────── + + +def test_a_relative_or_malformed_directory_is_dropped() -> None: + state = RuntimeFilesystemState(workspace="relative/ws", outputs="out\x00", inputs="") + assert state.workspace == "" + assert state.outputs == "" + assert not state.write_roots() + + +def test_installing_a_state_without_a_workspace_is_refused() -> None: + with pytest.raises(ValueError): + install_filesystem_state(RuntimeFilesystemState(workspace="relative")) + + +def test_sibling_directories_are_not_contained(local_state, run_dirs) -> None: + sibling = str(run_dirs / "inputs-old" / "x.csv") + assert runtime_file_write_error(sibling) is None # not the inputs mount + assert local_state.write_error(str(run_dirs / "inputs" / ".." / "x")) is None + + +def test_the_environment_derivation_matches_a_native_run(monkeypatch, run_dirs) -> None: + monkeypatch.setenv("SANDBOX_BACKEND", "native") + monkeypatch.setenv("FRONTIER_AGENT_WORKSPACE_DIR", str(run_dirs / "workspace")) + monkeypatch.setenv("FRONTIER_AGENT_OUTPUTS_DIR", str(run_dirs / "outputs")) + monkeypatch.setenv("FRONTIER_AGENT_INPUTS_DIR", str(run_dirs / "inputs")) + state = state_from_environment() + assert state.local_filesystem + assert state.dirs() == ( + str(run_dirs / "workspace"), str(run_dirs / "outputs"), str(run_dirs / "inputs"), + ) + + +def test_bwrap_reports_canonical_mounts_not_host_paths() -> None: + state = state_for_sandbox_mode("bwrap", workspace="/host/ws", outputs="/host/out") + assert state.dirs() == ("/workspace", "/outputs", "/inputs") + assert not state.local_filesystem + + +def test_a_backend_can_correct_the_state_but_the_host_is_never_probed() -> None: + state = state_for_sandbox_mode("bwrap") + backend = type("Backend", (), { + "filesystem_dirs": ("/w", "/o", "/i"), "tool_tmpdir": "/scratch", + })() + refreshed = state.with_backend(backend) + assert refreshed.dirs() == ("/w", "/o", "/i") + assert refreshed.tmpdir == "/scratch" + # A backend that says nothing changes nothing. + assert state.with_backend(object()) is state diff --git a/workflows/agent_team/nodes/main_agent.py b/workflows/agent_team/nodes/main_agent.py index 0ada121..4234d53 100644 --- a/workflows/agent_team/nodes/main_agent.py +++ b/workflows/agent_team/nodes/main_agent.py @@ -73,6 +73,11 @@ from frontier_agent.models.node_context import NodeContext from frontier_agent.state.event_store.sqlite import EventStore from plugins.tools._coerce import coerce_json_list +from plugins.tools._filesystem_state import ( + install_filesystem_state, + reset_filesystem_state, + state_for_sandbox_mode, +) from plugins.tools._sandbox import ( BwrapSandbox, bwrap_available, @@ -1060,16 +1065,6 @@ async def main_agent_node( src = dataset_root / src input_sources.append(src) inputs_available = any(_has_input_files(str(src)) for src in input_sources) - main_fs_note = render_sandbox_fs_note( - sandbox_mode=sandbox_mode, - inputs_available=inputs_available, - audience="main", - ) - sub_fs_note = render_sandbox_fs_note( - sandbox_mode=sandbox_mode, - inputs_available=inputs_available, - audience="sub", - ) stream_repetition_config = parse_stream_repetition_config(agent_cfg) sub_agent_llm = llm llm, stream_repetition_observer = wrap_llm_for_stream_repetition( @@ -1253,6 +1248,9 @@ def _decorate(base: str) -> str: main_binds: tuple[tuple[str, str, bool], ...] = () sub_binds: tuple[tuple[str, str, bool], ...] = () shared_workspace_dir = "" + # Bound only on the container/native branch below, where the model sees the + # physical mounts; bwrap presents the canonical mount points instead. + outputs_dir = inputs_dir = "" if sandbox_mode in ("container", "native"): workspace_dir, outputs_dir, inputs_dir = resolve_mount_dirs() worktree_root = Path(workspace_dir) @@ -1288,6 +1286,34 @@ def _decorate(base: str) -> str: ] ) + # One filesystem contract for this task, installed before any path + # question can be asked: the prompts, the shell variables, the file-tool + # permissions and the deliverable roots all read it instead of each + # re-deriving the directories from the environment. Sub-agents inherit it + # through the contextvar and narrow it to their own workspace. + fs_token = install_filesystem_state(state_for_sandbox_mode( + sandbox_mode, + workspace=str(worktree_root), + # Only the container/native branch above resolved physical mounts; for + # bwrap the model sees the canonical mount points, which + # ``state_for_sandbox_mode`` supplies on its own. + outputs=str(outputs_dir), + inputs=str(inputs_dir), + )) + + # Render after installation: environment overrides may name host directories + # that bwrap exposes under entirely different, canonical mount paths. + main_fs_note = render_sandbox_fs_note( + sandbox_mode=sandbox_mode, + inputs_available=inputs_available, + audience="main", + ) + sub_fs_note = render_sandbox_fs_note( + sandbox_mode=sandbox_mode, + inputs_available=inputs_available, + audience="sub", + ) + swarm_runtime = SwarmSubagentRuntime( original_question=question, trajectory_dir=traj_dir, @@ -1548,6 +1574,7 @@ async def _run_main_loop( turns=max_turns, ) + reset_filesystem_state(fs_token) if main_sb_token is not None: clear_task_sandbox(main_sb_token) if reporter_enabled: diff --git a/workflows/agent_team/subagent_runtime.py b/workflows/agent_team/subagent_runtime.py index d51ead2..94a0224 100644 --- a/workflows/agent_team/subagent_runtime.py +++ b/workflows/agent_team/subagent_runtime.py @@ -59,6 +59,7 @@ ) from frontier_agent.core.runtime.loop.tool_exec import ToolResultPostProcessor from frontier_agent.state.event_store.sqlite import EventStore +from plugins.tools._filesystem_state import current_filesystem_state from plugins.tools.bash import BASH_STDERR_SEPARATOR from workflows.agent_team.identity import MAIN_AGENT_ID, SUB_ROLE_ID, llm_session_id from workflows.agent_team.stream_repetition import StreamRepetitionConfig @@ -78,18 +79,12 @@ def render_sandbox_fs_note( """Render truthful filesystem context for the active sandbox topology.""" shared = sandbox_mode in ("container", "native") location = "this native workspace" if sandbox_mode == "native" else "this container" - workspace_path = ( - os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", "the workspace") - if sandbox_mode == "native" else "/workspace" - ) - inputs_path = ( - os.environ.get("FRONTIER_AGENT_INPUTS_DIR", "the inputs directory") - if sandbox_mode == "native" else "/inputs" - ) - outputs_path = ( - os.environ.get("FRONTIER_AGENT_OUTPUTS_DIR", "the outputs directory") - if sandbox_mode == "native" else "/outputs" - ) + # The prompt names exactly the directories the file tools authorize and the + # shell variables point at, because all of them read this one state. Read + # from the environment per mode, these three could disagree with the tools: + # a relocated outputs directory outside native mode left the prompt saying + # ``/outputs`` while every writer used the override. + workspace_path, outputs_path, inputs_path = current_filesystem_state().dirs() project_path = os.environ.get("FRONTIER_AGENT_PROJECT_DIR", "").strip() if audience == "sub": workspace = ( diff --git a/workflows/stateful_react_agent/_runtime.py b/workflows/stateful_react_agent/_runtime.py index cdefcc0..6f470f8 100644 --- a/workflows/stateful_react_agent/_runtime.py +++ b/workflows/stateful_react_agent/_runtime.py @@ -24,6 +24,7 @@ ) from frontier_agent.core.runtime.loop.tool_exec import ToolResultPostProcessor from frontier_agent.infra.config import get_config +from plugins.tools._filesystem_state import current_filesystem_state from plugins.tools.bash import BASH_STDERR_SEPARATOR from plugins.tools.document_node_toolchain import ( render_document_node_toolchain_note, @@ -81,9 +82,11 @@ def render_system_prompt_notes( filesystem_note = SANDBOX_FS_NOTE project = os.environ.get("FRONTIER_AGENT_PROJECT_DIR", "").strip() if sandbox_mode == "native": - workspace = os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", os.getcwd()) - inputs = os.environ.get("FRONTIER_AGENT_INPUTS_DIR", "inputs") - outputs = os.environ.get("FRONTIER_AGENT_OUTPUTS_DIR", "outputs") + # The directories this run actually uses, from the one trusted state — + # the same object the file tools authorize against and the shell + # variables are projected from, so the prompt cannot name a path the + # tools would refuse. + workspace, outputs, inputs = current_filesystem_state().dirs() filesystem_note = ( "\n\nFILESYSTEM CONVENTION (native mode): Your current working " f"directory {workspace} is the workspace. Read task inputs from " @@ -96,7 +99,7 @@ def render_system_prompt_notes( "It will be placed in the workspace-local native dependency overlay." ) if project: - workspace = os.environ.get("FRONTIER_AGENT_WORKSPACE_DIR", "/workspace") + workspace = current_filesystem_state().workspace if os.path.realpath(project) != os.path.realpath(workspace): filesystem_note += ( f" The user's project/coding directory is {project}; read or " diff --git a/workflows/stateful_react_agent/nodes/main_agent.py b/workflows/stateful_react_agent/nodes/main_agent.py index 8fcaa4b..4caa12f 100644 --- a/workflows/stateful_react_agent/nodes/main_agent.py +++ b/workflows/stateful_react_agent/nodes/main_agent.py @@ -70,6 +70,11 @@ resolve_language, ) from plugins.tools._bash_policy import reset_policy_mode, set_policy_mode +from plugins.tools._filesystem_state import ( + install_filesystem_state, + reset_filesystem_state, + state_for_sandbox_mode, +) from plugins.tools._sandbox import ( BwrapSandbox, SandboxUnavailableError, @@ -849,6 +854,17 @@ async def react_agent_node(state: dict[str, Any], ctx: NodeContext) -> dict[str, worktree_root.mkdir(parents=True, exist_ok=True) outputs_dir.mkdir(parents=True, exist_ok=True) + # Install this run's filesystem contract before anything can ask about a + # path. From here on the prompts, the shell variables, the file-tool + # permissions and the deliverable roots all read the SAME directories + # instead of each re-deriving them from the environment. + fs_token = install_filesystem_state(state_for_sandbox_mode( + sandbox_mode, + workspace=str(worktree_root), + outputs=str(outputs_dir), + inputs=inputs_dir, + )) + # /inputs is an external bind-mount (Worker Shell syncs it from S3); the # harness never populates it. Log what actually landed there so a # missing/misplaced input file is diagnosable from the worker log. @@ -1120,6 +1136,7 @@ async def react_agent_node(state: dict[str, Any], ctx: NodeContext) -> dict[str, ) finally: reset_policy_mode(policy_token) + reset_filesystem_state(fs_token) if sb_token is not None: clear_task_sandbox(sb_token) # Drop this run's task board (no-op when task_board is off) so boards