From f0d18f0b3063900dbd0490cdc30d4b579fcda0a9 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 21 Sep 2026 13:43:40 +0200 Subject: [PATCH] fix(skill-evals): contain fixture reads inside the eval tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `magpie-run-evals.sh` is excluded from the sandbox, so everything the eval runner reads, it reads unsandboxed. Fixtures are deliberately left agent-writable, on the stated grounds that a fixture can only route *repository* text to the model — text the session could send anyway. Nothing enforced the "repository" half of that. There are two ways out of the tree, both reachable by a sandboxed agent writing a fixture: - A symlink. `report.md -> ~/.ssh/id_rsa` is followed by `read_text()` and its contents go into the prompt piped to `claude -p`. The wrapper's guard constrains how its argument is spelled, not where it leads, and `-d` follows a directory symlink just as happily. - A path string. `step-config.json` supplies `skill_md`, which is joined to the repository root and read with no containment, so enough `..` walks out with no symlink involved at all. Either turns a fixture read into a read of any file the host can see, including the credential paths the sandbox denies the session directly — which is the one thing the exclusion was not supposed to buy. Resolve and contain instead: the wrapper resolves its target against `tools/skill-evals/evals`, and the runner resolves every fixture path it reads. Containment is by resolved path rather than a ban on symlinks, so shared fixtures factored out inside the tree keep working. Found by an adversarial review pass over #1311. Generated-by: Claude Opus 5 --- tools/skill-evals/magpie-run-evals.sh | 32 +++++++ tools/skill-evals/src/skill_evals/runner.py | 61 +++++++++--- .../tests/test_run_evals_wrapper.py | 95 +++++++++++++++++++ tools/skill-evals/tests/test_runner.py | 88 +++++++++++++++++ 4 files changed, 265 insertions(+), 11 deletions(-) create mode 100644 tools/skill-evals/tests/test_run_evals_wrapper.py diff --git a/tools/skill-evals/magpie-run-evals.sh b/tools/skill-evals/magpie-run-evals.sh index 7956b7ffe..4c842ad63 100755 --- a/tools/skill-evals/magpie-run-evals.sh +++ b/tools/skill-evals/magpie-run-evals.sh @@ -66,6 +66,16 @@ # — but cannot run code outside the sandbox. It can route repository # text to the model, which the session's own API access already allows. # +# That last sentence holds only while a fixture read cannot leave the +# repository, and nothing about a fixture being "data" guarantees it: a +# symlink, or a `skill_md` path with enough `..` in it, turns a fixture +# read into a read of any file the host can see — including the +# credential paths the sandbox denies the session directly. So it is +# enforced rather than assumed. The gate below resolves the target, and +# `skill_evals.runner` resolves every fixture path it reads; each refuses +# anything landing outside the tree. Containment is by resolved path +# rather than a ban on symlinks, so shared fixtures still work. +# # Usage, from the repository root: # # ~/.claude/scripts/magpie-run-evals.sh tools/skill-evals/evals// @@ -129,5 +139,27 @@ esac exit 2 } +# The case above constrains how the argument is spelled, not where it +# leads. A symlink under evals/ satisfies it and still points anywhere, +# and `-d` follows one — so an argument that passes the spelling check +# can hand the runner a directory outside the repository, which this +# script would then read *unsandboxed*. Resolve both sides and require +# containment. The runner enforces the same rule per fixture read, since +# a file symlink inside a real case escapes without the target ever +# leaving the tree. +evals_root="$(cd -- "$EVALS_REL" 2>/dev/null && pwd -P)" || { + echo "magpie-run-evals.sh: $EVALS_REL not found — run from the repository root" >&2 + exit 2 +} +target_real="$(cd -- "$target" && pwd -P)" +case "$target_real" in + "$evals_root" | "$evals_root"/*) ;; + *) + echo "magpie-run-evals.sh: $target resolves outside $evals_root" >&2 + echo " (to $target_real) — refusing to run it unsandboxed" >&2 + exit 2 + ;; +esac + export PYTHONPATH="$pythonpath" exec python3 -m skill_evals.runner --cli "claude -p" -- "$target" diff --git a/tools/skill-evals/src/skill_evals/runner.py b/tools/skill-evals/src/skill_evals/runner.py index 0f21059e5..b7d0b6b3d 100644 --- a/tools/skill-evals/src/skill_evals/runner.py +++ b/tools/skill-evals/src/skill_evals/runner.py @@ -184,24 +184,28 @@ def load_step_config(fixtures_dir: Path) -> tuple[str, str]: Raises FileNotFoundError if neither file is present. """ user_tmpl_path = fixtures_dir / "user-prompt-template.md" - user_prompt_template = user_tmpl_path.read_text() if user_tmpl_path.exists() else USER_PROMPT_TEMPLATE + user_prompt_template = ( + read_contained(user_tmpl_path, fixtures_dir) if user_tmpl_path.exists() else USER_PROMPT_TEMPLATE + ) # 1. step-config.json → live extraction from SKILL.md config_path = fixtures_dir / "step-config.json" if config_path.exists(): - config = json.loads(config_path.read_text()) + config = json.loads(read_contained(config_path, fixtures_dir)) repo_root = find_repo_root(fixtures_dir) - skill_md_path = repo_root / config["skill_md"] + # `skill_md` comes out of the fixture, so it is contained against the + # repository rather than trusted as written. + skill_md_path = resolve_contained(repo_root / config["skill_md"], repo_root) section = extract_skill_section(skill_md_path, config["step_heading"]) output_spec_path = fixtures_dir / "output-spec.md" if output_spec_path.exists(): - section += "\n\n" + output_spec_path.read_text() + section += "\n\n" + read_contained(output_spec_path, fixtures_dir) return section, user_prompt_template # 2. system-prompt.md → manually maintained (triage steps) sys_prompt_path = fixtures_dir / "system-prompt.md" if sys_prompt_path.exists(): - return sys_prompt_path.read_text(), user_prompt_template + return read_contained(sys_prompt_path, fixtures_dir), user_prompt_template raise FileNotFoundError( f"{fixtures_dir} has neither step-config.json nor system-prompt.md. " @@ -214,6 +218,39 @@ def load_step_config(fixtures_dir: Path) -> tuple[str, str]: # --------------------------------------------------------------------------- +def read_contained(path: Path, root: Path) -> str: + """Read ``path``, refusing anything whose real path escapes ``root``. + + ``magpie-run-evals.sh`` is excluded from the sandbox, so the reads below + are not subject to the sandbox's deny list. Fixtures are deliberately + left agent-writable, on the grounds that a fixture can only route + *repository* text to the model — text the session could send anyway. A + symlink is what breaks that scoping: it turns a fixture read into a read + of any file the host can see, including the credential paths the + sandboxed session is explicitly denied. Containment is by resolved path + rather than a blanket refusal of symlinks, so shared fixtures factored + out inside the eval tree keep working. + """ + return resolve_contained(path, root).read_text() + + +def resolve_contained(path: Path, root: Path) -> Path: + """Resolve ``path``, refusing anything whose real path escapes ``root``. + + Two ways a fixture reaches outside the tree, and this closes both: a + symlink whose target is elsewhere, and a path *string* a fixture + supplies (``step-config.json`` → ``skill_md``) with enough ``..`` in it + to walk out of the repository. + """ + resolved = path.resolve() + root_resolved = root.resolve() + if not resolved.is_relative_to(root_resolved): + raise ValueError( + f"{path.name}: resolves outside {root_resolved} (to {resolved}); refusing to read it" + ) + return resolved + + def load_case(case_dir: Path) -> tuple[list[dict], dict, str, str, dict]: """Return (corpus, roster, report_text, trusted_context, expected). @@ -232,11 +269,13 @@ def _resolve(name: str) -> Path: roster_path = _resolve("reporter-roster.json") trusted_context_path = _resolve("trusted-context.md") - corpus = json.loads(corpus_path.read_text()) if corpus_path.exists() else [] - roster = json.loads(roster_path.read_text()) if roster_path.exists() else {} - report = (case_dir / "report.md").read_text() - trusted_context = trusted_context_path.read_text() if trusted_context_path.exists() else "" - expected = json.loads((case_dir / "expected.json").read_text()) + corpus = json.loads(read_contained(corpus_path, fixtures_dir)) if corpus_path.exists() else [] + roster = json.loads(read_contained(roster_path, fixtures_dir)) if roster_path.exists() else {} + report = read_contained(case_dir / "report.md", fixtures_dir) + trusted_context = ( + read_contained(trusted_context_path, fixtures_dir) if trusted_context_path.exists() else "" + ) + expected = json.loads(read_contained(case_dir / "expected.json", fixtures_dir)) return corpus, roster, report, trusted_context, expected @@ -249,7 +288,7 @@ def load_case_tags(case_dir: Path) -> set[str]: meta_path = case_dir / "case-meta.json" if not meta_path.exists(): return set() - meta = json.loads(meta_path.read_text()) + meta = json.loads(read_contained(meta_path, case_dir.parent)) tags = meta.get("tags", []) if not isinstance(tags, list) or not all(isinstance(tag, str) for tag in tags): raise ValueError(f"{meta_path} must contain a string-list 'tags' field") diff --git a/tools/skill-evals/tests/test_run_evals_wrapper.py b/tools/skill-evals/tests/test_run_evals_wrapper.py new file mode 100644 index 000000000..1b92df5ea --- /dev/null +++ b/tools/skill-evals/tests/test_run_evals_wrapper.py @@ -0,0 +1,95 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for the argument gate in magpie-run-evals.sh. + +`sandbox.excludedCommands` names this wrapper, so whatever it runs, runs +outside the sandbox. The gate is what keeps that exclusion narrow, and it +has to constrain where the argument *leads*, not only how it is spelled. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +WRAPPER = Path(__file__).resolve().parent.parent / "magpie-run-evals.sh" +EVALS_REL = "tools/skill-evals/evals" + + +def _run(cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["bash", str(WRAPPER), *args], + cwd=cwd, + capture_output=True, + text=True, + timeout=60, + ) + + +def _fake_repo(tmp_path: Path) -> Path: + root = tmp_path / "repo" + (root / EVALS_REL).mkdir(parents=True) + return root + + +def test_rejects_a_target_symlinked_outside_the_evals_tree(tmp_path: Path) -> None: + """A symlink satisfies the spelling check and still points anywhere. + + `-d` follows it, so without resolution the runner is handed a directory + outside the repository and reads it unsandboxed. + """ + root = _fake_repo(tmp_path) + outside = tmp_path / "secrets" + outside.mkdir() + (outside / "id_rsa").write_text("PRIVATE KEY MATERIAL") + (root / EVALS_REL / "sneaky").symlink_to(outside) + + result = _run(root, f"{EVALS_REL}/sneaky") + + assert result.returncode == 2, result.stdout + result.stderr + assert "PRIVATE KEY MATERIAL" not in result.stdout + + +def test_rejects_a_target_outside_the_evals_tree(tmp_path: Path) -> None: + """The existing spelling check, pinned so the new gate cannot loosen it.""" + root = _fake_repo(tmp_path) + result = _run(root, "/etc") + assert result.returncode == 2 + + +def test_rejects_more_than_one_argument(tmp_path: Path) -> None: + """The exclusion rests on the one-argument, no-flags shape.""" + root = _fake_repo(tmp_path) + result = _run(root, f"{EVALS_REL}", "--verbose") + assert result.returncode == 2 + + +def test_lets_a_real_path_inside_the_evals_tree_through(tmp_path: Path) -> None: + """The gate must not cost the normal case. + + A genuine directory under `evals/` reaches the runner, which then exits + on its own terms (no cases here) rather than on the gate's — so this + pins "allowed through" without invoking `claude -p`. + """ + root = _fake_repo(tmp_path) + (root / EVALS_REL / "some-skill").mkdir() + + result = _run(root, f"{EVALS_REL}/some-skill") + + assert result.returncode != 2, result.stdout + result.stderr + assert "resolves outside" not in result.stderr diff --git a/tools/skill-evals/tests/test_runner.py b/tools/skill-evals/tests/test_runner.py index 63c389de6..6f4c39b30 100644 --- a/tools/skill-evals/tests/test_runner.py +++ b/tools/skill-evals/tests/test_runner.py @@ -440,6 +440,94 @@ def test_load_case_loads_optional_trusted_context(tmp_path: Path): assert trusted_context == "Policy from the trusted base." +# --------------------------------------------------------------------------- +# fixture containment +# --------------------------------------------------------------------------- + + +def test_load_case_refuses_a_fixture_symlinked_outside_the_eval_tree(tmp_path: Path): + """A fixture read must not escape the eval tree. + + `magpie-run-evals.sh` is excluded from the sandbox, so the reads this + runner performs are not subject to the sandbox's deny list. Fixtures are + left agent-writable on the stated grounds that doing so can only route + *repository* text to the model, which the session could send anyway. A + symlink breaks exactly that scoping: it turns a fixture read into a read + of any file the host can see -- `~/.ssh/id_rsa`, a token file -- which + the sandboxed session itself is denied. + """ + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "id_rsa" + secret.write_text("PRIVATE KEY MATERIAL") + + fixtures_dir = _make_fixtures_dir(tmp_path / "step") + case_dir = _make_case(fixtures_dir, "case-1") + report = case_dir / "report.md" + report.unlink() + report.symlink_to(secret) + + with pytest.raises(ValueError) as excinfo: + load_case(case_dir) + message = str(excinfo.value) + assert "report.md" in message + assert "PRIVATE KEY MATERIAL" not in message + + +def test_load_case_allows_a_symlink_that_stays_inside_the_eval_tree(tmp_path: Path): + """Containment is about where the link points, not that it is a link. + + Shared fixtures are a normal thing to factor out, so the rule has to be + resolved-path containment rather than a blanket refusal of symlinks. + """ + fixtures_dir = _make_fixtures_dir(tmp_path / "step") + shared = fixtures_dir / "shared-report.md" + shared.write_text("shared report text") + case_dir = _make_case(fixtures_dir, "case-1") + report = case_dir / "report.md" + report.unlink() + report.symlink_to(shared) + + _, _, report_text, _, _ = load_case(case_dir) + assert report_text == "shared report text" + + +def test_load_step_config_refuses_a_prompt_symlinked_outside_the_eval_tree(tmp_path: Path): + """Same containment rule for the step-level fixtures, not just case files.""" + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "token" + secret.write_text("ghp_SECRET_TOKEN_VALUE") + + fixtures_dir = _make_fixtures_dir(tmp_path / "step") + (fixtures_dir / "system-prompt.md").symlink_to(secret) + + with pytest.raises(ValueError) as excinfo: + load_step_config(fixtures_dir) + assert "ghp_SECRET_TOKEN_VALUE" not in str(excinfo.value) + + +def test_load_step_config_refuses_a_skill_md_path_escaping_the_repo(tmp_path: Path): + """`skill_md` is a fixture-controlled path string joined to the repo root. + + It needs no symlink to escape -- a relative path with enough `..` in it + walks straight out of the repository, into any file the unsandboxed + wrapper can read. + """ + repo = _make_repo(tmp_path) + outside = tmp_path.parent / "outside-skill.md" + outside.write_text("## Step\n\nsecret content\n") + + fixtures_dir = _make_fixtures_dir( + repo / "tools" / "skill-evals" / "evals" / "s" / "step", + step_config={"skill_md": f"../{outside.name}", "step_heading": "## Step"}, + ) + + with pytest.raises(ValueError) as excinfo: + load_step_config(fixtures_dir) + assert "secret content" not in str(excinfo.value) + + def test_load_case_tags_missing_meta_returns_empty_set(tmp_path: Path): fixtures_dir = tmp_path / "fixtures" fixtures_dir.mkdir()