fix(skill-evals): contain fixture reads inside the eval tree

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/<skill>/
@@ -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"

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")

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

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()