From 4c25828af15d9df1f0e3f1045d944410224da05e Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sun, 18 Oct 2026 00:54:13 +0200 Subject: [PATCH] feat(sandbox-lint): accept only allowlisted sandbox exclusions An excluded command runs outside the sandbox, and recognising the shapes of commands that run their arguments can never be complete (find -exec, git -c, make, docker). Exclusions are few and named by the framework, so accept only those, or a narrowing of one, plus whatever a project lists in its overrides after review. Generated-by: Claude Opus 5 --- tools/sandbox-lint/README.md | 21 ++++++- .../sandbox-lint/src/sandbox_lint/__init__.py | 60 +++++++++++++++++++ tools/sandbox-lint/tests/test_layered.py | 40 +++++++++++++ .../specs/agent-isolation-sandbox.md | 7 +++ 4 files changed, 125 insertions(+), 3 deletions(-) diff --git a/tools/sandbox-lint/README.md b/tools/sandbox-lint/README.md index 557935f38..89dc78643 100644 --- a/tools/sandbox-lint/README.md +++ b/tools/sandbox-lint/README.md @@ -176,6 +176,18 @@ uv run --project tools/sandbox-lint sandbox-lint --any-harness /path/to/magpie fail; every other `uv` subcommand (`sync`, `lock`, `pip`, `build`) counts too, since each can run a project's build backend. No bare `Bash` allow; no `permissions.defaultMode: bypassPermissions`, and no `*` in `sandbox.network.allowedDomains`. + - Every `sandbox.excludedCommands` entry, in any layer, is on the + exclusion allowlist (`EXCLUDED_COMMAND_ALLOWLIST`: `gh *`, `roborev *`, + the four vetted-ops forms, the adversarial-review runner and + `~/.claude/scripts/magpie-run-evals.sh *`) or narrows one of them + (`gh auth *`, `gh`). The shape check above cannot recognise every + command that runs its arguments (`find -exec`, `git -c`, `make`), and an + excluded command runs outside the sandbox, so exclusions are an + allowlist, not a denylist. A project that needs another exclusion lists + it, one per line (`#` comments), in + `.apache-magpie-overrides/sandbox-lint/excluded-commands.txt` under its + root; such an entry still has to pass the shape check, so it cannot add + a shell or an unpinned runner. 3. **Baseline self-check.** The same invariants are applied to `expected.json` itself, so a PR cannot weaken the baseline in lockstep with the live settings without the lint catching the @@ -291,9 +303,12 @@ if the two diverge. The wholesale-weakening check (`_runs_anything`) recognises *shapes* of rule that run arbitrary code: shells and interpreters, globbed command words, and package runners not pinned to a path. It cannot prove an arbitrary rule safe. -A tool that executes its arguments and is not on the list would pass, so -review new `excludedCommands` and `Bash(...)` allows on their merits. The check -is a backstop, not a substitute for that review. +For `excludedCommands` the allowlist closes that gap, except for what a +project adds to its own extension file. For `Bash(...)` allows there is no +allowlist (approving a tool such as `zizmor *` without a prompt is +legitimate), so a tool that executes its arguments and is not on the list +would pass. Review new `Bash(...)` allows and extension entries on their +merits; the check is a backstop, not a substitute for that review. A pinned runner is only as safe as the files it runs, and the check looks at the pin's source path alone. The writable set it uses is `allowWrite`, the diff --git a/tools/sandbox-lint/src/sandbox_lint/__init__.py b/tools/sandbox-lint/src/sandbox_lint/__init__.py index 88dda5c15..14b557782 100644 --- a/tools/sandbox-lint/src/sandbox_lint/__init__.py +++ b/tools/sandbox-lint/src/sandbox_lint/__init__.py @@ -59,6 +59,9 @@ # Which MCP tools each family calls. In the framework checkout every family is # in use, so the floor's MCP allows must be exactly what this derives. FAMILY_MCP_REGISTRY = Path("plugins/magpie-setup/skills/setup/family-mcp-allows.json") +# A project's own additions to EXCLUDED_COMMAND_ALLOWLIST, one entry per line, +# under its project root. +PROJECT_EXCLUSION_ALLOWLIST = Path(".apache-magpie-overrides/sandbox-lint/excluded-commands.txt") # The layer actually in force on this machine. It may add personal entries # (and MCP allows for families not adopted), but layered over the floor it must # still pass every invariant and may not override a value the floor sets. @@ -196,6 +199,23 @@ def registry_errors(floor: dict[str, Any], registry: dict[str, Any]) -> list[str "awk", "gawk", "nawk", "mawk", "tclsh", "lua", "Rscript", } ) # fmt: skip +# The only commands that may run outside the sandbox. Recognising commands that +# run anything (below) cannot be complete: `find -exec`, `git -c`, `make` and +# many more run their arguments. An exclusion therefore has to be one of these, +# or a narrowing of one (`gh auth *` under `gh *`); a project adds its own in +# PROJECT_EXCLUSION_ALLOWLIST, where they still pass the runs-anything check. +EXCLUDED_COMMAND_ALLOWLIST = frozenset( + { + "gh *", + "roborev *", + "uv run --project ~/.claude/magpie/vetted-ops vetted-op-read *", + "uv run --project ~/.claude/magpie/vetted-ops vetted-op-tracker *", + "uvx --from ~/.claude/magpie/vetted-ops vetted-op-read *", + "uvx --from ~/.claude/magpie/vetted-ops vetted-op-tracker *", + "uvx --from ~/.claude/magpie/adversarial-review adversarial-review *", + "~/.claude/scripts/magpie-run-evals.sh *", + } +) # Interpreters, matched with any version suffix (`python3.12`, `node22`). _VERSIONED_INTERPRETER = re.compile(r"(python|pypy|node|ruby|perl|php)[0-9.]*") _ENV_ASSIGNMENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=") @@ -248,6 +268,36 @@ def _is_outside_pin(path: str, writable: tuple[str, ...] = ()) -> bool: return not _is_writable(os.path.normpath(os.path.expanduser(path)), writable) +def _read_project_exclusions(project_root: Path | None) -> tuple[frozenset[str], str | None]: + """The project's own allowed exclusions, and an error when its file exists + but cannot be read.""" + if project_root is None: + return frozenset(), None + path = project_root / PROJECT_EXCLUSION_ALLOWLIST + try: + lines = path.read_text(encoding="utf-8").splitlines() + except FileNotFoundError: + return frozenset(), None + except (OSError, UnicodeDecodeError) as exc: + return frozenset(), f"{PROJECT_EXCLUSION_ALLOWLIST}: cannot be read ({exc})" + return frozenset(s for s in (line.strip() for line in lines) if s and not s.startswith("#")), None + + +def _is_allowed_exclusion(entry: str, allowed: frozenset[str]) -> bool: + """``entry`` is an allowed exclusion or a narrowing of one: the same + command with more of its arguments named (``gh auth *`` under ``gh *``).""" + text = entry.strip() + # Claude Code's legacy prefix form `cmd:*` means the same as `cmd *`. + if text.endswith(":*"): + text = text[:-2] + " *" + for pattern in allowed: + if text == pattern: + return True + if pattern.endswith(" *") and (text == pattern[:-2] or text.startswith(pattern[:-1])): + return True + return False + + def _has_glob(word: str) -> bool: return any(c in word for c in "*?[") @@ -539,7 +589,17 @@ def check_invariants(settings: dict[str, Any], project_root: Path | None = None) # before the end also matches options spliced in at that position, and an # excluded command runs outside the sandbox once approved (the # adversarial-review exclusion once globbed the plugin version this way). + project_exclusions, project_exclusions_error = _read_project_exclusions(project_root) + if project_exclusions_error: + errors.append(project_exclusions_error) + allowed_exclusions = EXCLUDED_COMMAND_ALLOWLIST | project_exclusions for entry in sandbox.get("excludedCommands", []) or []: + if isinstance(entry, str) and not _is_allowed_exclusion(entry, allowed_exclusions): + errors.append( + f"sandbox.excludedCommands: {entry!r} is not on the exclusion allowlist; an excluded " + "command runs outside the sandbox, so only the framework's own exclusions are accepted. " + f"Add it to {PROJECT_EXCLUSION_ALLOWLIST} once reviewed" + ) if isinstance(entry, str) and _runs_anything(entry, writable): errors.append( f"sandbox.excludedCommands: {entry!r} would run arbitrary commands outside the sandbox" diff --git a/tools/sandbox-lint/tests/test_layered.py b/tools/sandbox-lint/tests/test_layered.py index 0da5cf0c7..061874162 100644 --- a/tools/sandbox-lint/tests/test_layered.py +++ b/tools/sandbox-lint/tests/test_layered.py @@ -372,3 +372,43 @@ def test_a_working_dir_inside_the_checkout_fails_the_invariants(work_dir: str) - settings = merge_layers(merge_layers(_load(FLOOR), _load(LOCAL_TEMPLATE)), addition) errors = check_invariants(settings, project_root=REPO_ROOT) assert any(e.startswith(f"permissions.additionalDirectories: {work_dir!r}") for e in errors), errors + + +def _with_exclusions(*entries: str) -> dict[str, Any]: + return merge_layers( + merge_layers(_load(FLOOR), _load(LOCAL_TEMPLATE)), {"sandbox": {"excludedCommands": list(entries)}} + ) + + +@pytest.mark.parametrize("entry", ["roborev *", "gh auth *", "gh", "gh auth status", "gh:*", "roborev:*"]) +def test_allowlisted_exclusions_and_their_narrowings_pass(entry: str) -> None: + assert check_invariants(_with_exclusions(entry), project_root=REPO_ROOT) == [] + + +@pytest.mark.parametrize( + "entry", ["find *", "git *", "make *", "docker *", "zizmor *", "ghx *", "roborevx *"] +) +def test_an_exclusion_off_the_allowlist_fails(entry: str) -> None: + errors = check_invariants(_with_exclusions(entry), project_root=REPO_ROOT) + assert any(f"{entry!r} is not on the exclusion allowlist" in e for e in errors), errors + + +def test_a_project_can_extend_the_allowlist(tmp_path: Path) -> None: + from sandbox_lint import PROJECT_EXCLUSION_ALLOWLIST + + extension = tmp_path / PROJECT_EXCLUSION_ALLOWLIST + extension.parent.mkdir(parents=True) + extension.write_text("# reviewed in #123\nzizmor *\n\nbash *\n") + errors = check_invariants(_with_exclusions("zizmor *", "bash *"), project_root=tmp_path) + assert not any("'zizmor *'" in e for e in errors), errors + # An extension still has to pass the runs-anything check. + assert any("'bash *' would run arbitrary commands" in e for e in errors), errors + assert not any("'bash *' is not on the exclusion allowlist" in e for e in errors), errors + + +def test_an_unreadable_extension_is_reported(tmp_path: Path) -> None: + from sandbox_lint import PROJECT_EXCLUSION_ALLOWLIST + + (tmp_path / PROJECT_EXCLUSION_ALLOWLIST).mkdir(parents=True) + errors = check_invariants(_with_exclusions(), project_root=tmp_path) + assert any(e.startswith(f"{PROJECT_EXCLUSION_ALLOWLIST}: cannot be read") for e in errors), errors diff --git a/tools/spec-loop/specs/agent-isolation-sandbox.md b/tools/spec-loop/specs/agent-isolation-sandbox.md index f56fd14cd..3a1ae4b44 100644 --- a/tools/spec-loop/specs/agent-isolation-sandbox.md +++ b/tools/spec-loop/specs/agent-isolation-sandbox.md @@ -347,6 +347,13 @@ The reference model is four layers, layered: - `~/.claude/scripts/magpie-run-evals.sh` (optional), so a suite can be graded by `claude -p` from inside the sandbox. + That list is closed: sandbox-lint accepts an exclusion only when it is + one of these entries or a narrowing of one (`EXCLUDED_COMMAND_ALLOWLIST`), + because no check can recognise every command that runs its arguments. + An adopter adds its own in + `.apache-magpie-overrides/sandbox-lint/excluded-commands.txt`, and those + still have to pass the runs-anything check. + An exclusion runs whatever the command executes **outside** the sandbox, so the executed code must not be writable by the thing being sandboxed. The non-`gh` entries above therefore address code