mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-06 06:52:20 +02:00
fix(runtime): enforce workspace confinement in one place
"Is this path inside that root" is asked in twenty places in this tree and answered twenty times by a locally written realpath/commonpath pair. Nine test files exist because nine call sites each needed their own proof. Each one is defensible alone; together they are the defect, because the boundary has no single definition and a site that gets a detail wrong is wrong by itself. src/path_confinement.py is that definition, and it settles the details the copies disagreed on. Both sides get canonicalized: comparing a realpath-ed candidate against a root that was only abspath-ed is the macOS /tmp -> /private/tmp mismatch that has already produced a false failure here, and canonicalizing one side is worse than canonicalizing neither. commonpath rather than startswith, because /a/bc begins with /a/b and is not inside it. A relative candidate joins the root rather than os.getcwd(), which is whatever directory the server happens to be running in. NUL and newline are refused with a reason instead of caught by a bare `except Exception` and reported as an ordinary escape. Eighteen call sites go through it now. It deliberately does not decide whether a path is sensitive -- that deny list answers "allowed" rather than "inside", and it stays with src/tool_execution, which owns it. The one commonpath left in the tree, in src/workspace_paths.py, stays: that function translates a host path into a container path, so canonicalizing either side would change the relative path it computes and break the mapping. It is not a confinement check. Two of those sites were weaker than the rest and are fixed rather than moved. The email attachment check used abspath, which folds `..` but does not resolve symlinks, so a symlink written into the extraction directory passed it and was then read through. The skill-reference guard compared a realpath-ed target against a raw dirname, so on a host where the skills tree is reached through a symlink the two sides never matched and the guard could not fire. The execution boundary had two separate holes. The workspace namespace bound /home and /mnt read-write. On the one platform where that namespace engages at all, a command inside it reaches outside the workspace and writes to the user's home directory -- measured by running this argv on a Linux host with working bubblewrap, not inferred from the source. Binding the user's whole home directory into a workspace-confinement namespace gives back most of what the namespace was for. Both are read-only now. The workspace is also bound writable at its real host path, not only at /workspace: BashTool's own /tmp redirect rewrites `/tmp/` to `<agent_cwd()>/.tmp/` before the namespace is built, so the command bwrap receives already names the real path, and those writes previously landed only because the workspace happened to sit under the writable /home. `namespaced or _replace_workspace_alias(...)` chose between a mount namespace and a regex with nothing in the result saying which one ran. The fallback 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 -- which is every agent shell command on macOS. Both tools now ask containment.probe() instead of each deciding for itself, and every bash and python result carries a containment block naming the mechanism and stating whether the filesystem dimension actually held. Under enforcing mode the command is not run and the result says so. That block reports the filesystem dimension only, and says so in a reported_dimensions field. The probe knows this host could also give a process group and a real wall clock, but these two tools still assemble their own create_subprocess_* call and pass neither, so listing those dimensions would be exactly the false claim src/containment.py calls worse than an honest absence. probe() is new on src/containment.py: the same mechanism table and the same arithmetic as acquire(), stopping before the side effects. acquire() is the wrong shape for a decision -- it writes a durable grant record, and a record whose pid is never filled in and whose release() never runs is an entry a restart reaper keeps finding. CONTAINMENT_MODE stays report_only. Flipping it refuses every agent shell command on macOS and on any Linux host without bubblewrap, which is a product decision rather than a code one. Smaller things in the same area: the /tmp redirect's makedirs was unguarded, so a read-only workspace turned a command that merely mentioned `/tmp/` into an OSError traceback instead of a tool error; it degrades now. WORKSPACE_MOUNT moved to src/constants.py so the namespace and the path resolvers read one definition of the contract rather than two. The ".tmp" dirname got a constant, since it appeared in both tool paths. One generated artifact moved with it: website/configuration-reference.md pins the source line where each ODYSSEUS_* variable is read, and three of those shifted. Regenerated with scripts/generate_env_reference.py; the diff is line numbers only. Three existing tests changed. test_workspace_artifact_tool_floor asserted that an unsafe interpreter prefix produces no `--ro-bind <prefix> <prefix>`, which now fires on /home because /home is legitimately a read-only base mount. Asserting the absence of a literal flag string cannot distinguish "the prefix was rejected" from "the argv mounted that root itself", so it compares the argv against the no-prefix baseline instead: an unsafe prefix must add nothing. The Windows bash test asserted dict equality on the whole result, which makes adding a field to every bash result impossible without touching a test about tmux; it asserts the shape now. The personal-dir symlink test grepped the resolver's source for the literal "os.path.realpath", which is gone because the resolution moved into the shared boundary -- it keeps the negative assertion that the closure must not grow its own abspath check again, and the behavioural half now runs against the boundary, where it covers every call site instead of one closure. Not verified: the bubblewrap argv is asserted, not executed. There is no bwrap on macOS, and in Docker it needs --privileged to work at all -- default and seccomp=unconfined both fail with "Creating new namespace failed", and --cap-add=SYS_ADMIN fails at pivot_root. The Python tool's needs_virtual_namespace gate means ordinary Python code gets no namespace even on a Linux host that could provide one; that is reported now but deliberately not changed, because it alters the Linux Python path on every call and cannot be checked from here.
This commit is contained in:
Léo
@@ -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
|
||||
|
||||
|
||||
@@ -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 `<agent_cwd()>/.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")
|
||||
@@ -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"))
|
||||
@@ -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):
|
||||
|
||||
@@ -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 <prefix> <prefix>` 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)
|
||||
]
|
||||
|
||||
Reference in New Issue
Block a user