diff --git a/.github/workflows/skill-eval.yml b/.github/workflows/skill-eval.yml index c33a01e..b6603da 100644 --- a/.github/workflows/skill-eval.yml +++ b/.github/workflows/skill-eval.yml @@ -7,6 +7,7 @@ on: - '*/evals/evals.json' - '*/scripts/**' - 'eval_runner/**' + - '.github/workflows/skill-eval.yml' - 'schemas/comparison-report-v1.schema.json' - 'schemas/release-eval-v1.schema.json' push: @@ -17,6 +18,7 @@ on: - '*/evals/evals.json' - '*/scripts/**' - 'eval_runner/**' + - '.github/workflows/skill-eval.yml' - 'schemas/comparison-report-v1.schema.json' - 'schemas/release-eval-v1.schema.json' @@ -28,11 +30,11 @@ jobs: runs-on: ubuntu-latest steps: - name: Check out repository - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 with: python-version: '3.12' - name: Install dependencies @@ -49,11 +51,11 @@ jobs: needs: paired-eval-tests steps: - name: Check out repository - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 with: python-version: '3.12' - name: Install dependencies @@ -93,7 +95,7 @@ jobs: exit $exit_code - name: Upload paired eval artifacts if: always() && steps.changed.outputs.manifests != '' - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: paired-eval-artifacts path: eval-output-paired/ @@ -105,11 +107,11 @@ jobs: needs: paired-eval-tests steps: - name: Check out repository - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 with: python-version: '3.12' - name: Install dependencies @@ -117,8 +119,13 @@ jobs: - name: Check model endpoint id: endpoint env: - EVAL_BASE_URL: ${{ vars.EVAL_BASE_URL || 'http://gpuslut01:8080' }} + EVAL_BASE_URL: ${{ vars.EVAL_BASE_URL }} + EVAL_MODEL: ${{ vars.EVAL_MODEL }} run: | + if [ -z "$EVAL_BASE_URL" ] || [ -z "$EVAL_MODEL" ]; then + echo "available=false" >> "$GITHUB_OUTPUT" + exit 0 + fi if curl -sf --connect-timeout 5 "$EVAL_BASE_URL/v1/models" > /dev/null 2>&1; then echo "available=true" >> "$GITHUB_OUTPUT" else @@ -148,8 +155,8 @@ jobs: if: steps.endpoint.outputs.available == 'true' && steps.changed.outputs.manifests != '' env: MANIFESTS: ${{ steps.changed.outputs.manifests }} - EVAL_MODEL: ${{ vars.EVAL_MODEL || 'google_gemma-4-26B-A4B-it-IQ4_XS.gguf' }} - EVAL_BASE_URL: ${{ vars.EVAL_BASE_URL || 'http://gpuslut01:8080' }} + EVAL_MODEL: ${{ vars.EVAL_MODEL }} + EVAL_BASE_URL: ${{ vars.EVAL_BASE_URL }} run: | exit_code=0 for manifest in $MANIFESTS; do @@ -159,6 +166,7 @@ jobs: --adapter openai \ --base-url "$EVAL_BASE_URL" \ --model "$EVAL_MODEL" \ + --model-label configured-model \ --max-tokens 4096 \ --no-thinking \ --timeout 300 \ @@ -168,7 +176,7 @@ jobs: exit $exit_code - name: Upload model eval artifacts if: always() && steps.endpoint.outputs.available == 'true' - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: paired-eval-model-artifacts path: eval-output-model/ diff --git a/eval_runner/comparison.py b/eval_runner/comparison.py index 0797bfd..1d3fd08 100644 --- a/eval_runner/comparison.py +++ b/eval_runner/comparison.py @@ -9,6 +9,7 @@ from pathlib import Path from typing import Any from .grader import GradeResult +from .path_safety import contained_path, validate_case_id COMPARISON_SCHEMA_VERSION = 1 @@ -73,8 +74,9 @@ def build_comparison_report( def write_comparison_report(report: dict[str, Any], output_dir: Path) -> Path: output_dir.mkdir(parents=True, exist_ok=True) case_id = report.get("case_id", "unknown") + validate_case_id(case_id) report_id = report.get("report_id", "unknown")[:8] - path = output_dir / f"{case_id}--{report_id}.comparison.json" + path = contained_path(output_dir, f"{case_id}--{report_id}.comparison.json") path.write_text(json.dumps(report, indent=2) + "\n", encoding="utf-8") return path diff --git a/eval_runner/manifest.py b/eval_runner/manifest.py index 3f2f825..1d1d144 100644 --- a/eval_runner/manifest.py +++ b/eval_runner/manifest.py @@ -2,7 +2,6 @@ from __future__ import annotations -import hashlib import json import subprocess import uuid @@ -11,6 +10,7 @@ from pathlib import Path from typing import Any from .models import AdapterInput, AdapterOutput, EvalCase +from .path_safety import contained_path, hash_contained_file, validate_case_id MANIFEST_SCHEMA_VERSION = 1 @@ -53,13 +53,9 @@ def build_manifest( artifact_digests: dict[str, str] = {} for artifact_rel in adapter_output.artifacts: - artifact_path = adapter_input.output_dir / artifact_rel - if artifact_path.is_file(): - artifact_digests[artifact_rel] = hashlib.sha256( - artifact_path.read_bytes() - ).hexdigest()[:16] - else: - artifact_digests[artifact_rel] = "missing" + artifact_digests[artifact_rel] = ( + hash_contained_file(adapter_input.output_dir, artifact_rel) or "missing" + ) failures: list[dict[str, str]] = [] if adapter_output.error: @@ -70,7 +66,7 @@ def build_manifest( "trial_id": str(uuid.uuid4()), "candidate": { "skill_name": _skill_name(skill_path), - "skill_path": str(skill_path), + "skill_path": skill_path.name, "tree_hash": _git_tree_hash(skill_path), }, "case": { @@ -114,7 +110,8 @@ def write_manifest(manifest: dict[str, Any], output_dir: Path) -> Path: output_dir.mkdir(parents=True, exist_ok=True) trial_id = manifest.get("trial_id", "unknown") case_id = manifest.get("case", {}).get("case_id", "unknown") + validate_case_id(case_id) filename = f"{case_id}--{trial_id[:8]}.manifest.json" - path = output_dir / filename + path = contained_path(output_dir, filename) path.write_text(json.dumps(manifest, indent=2) + "\n", encoding="utf-8") return path diff --git a/eval_runner/models.py b/eval_runner/models.py index fbccd4d..b9ddc5e 100644 --- a/eval_runner/models.py +++ b/eval_runner/models.py @@ -8,6 +8,8 @@ from enum import Enum from pathlib import Path from typing import Any +from .path_safety import hash_contained_file, validate_case_id, validate_relative_path + class ExitStatus(str, Enum): COMPLETED = "completed" @@ -25,6 +27,11 @@ class EvalCase: files: list[str] = field(default_factory=list) case_set: str = "dev" + def __post_init__(self) -> None: + validate_case_id(self.id) + for relative_path in self.files: + validate_relative_path(relative_path) + @property def prompt_hash(self) -> str: return hashlib.sha256(self.prompt.encode()).hexdigest()[:16] @@ -32,11 +39,7 @@ class EvalCase: def fixture_hashes(self, skill_root: Path) -> dict[str, str]: hashes: dict[str, str] = {} for rel in self.files: - target = skill_root / rel - if target.is_file(): - hashes[rel] = hashlib.sha256(target.read_bytes()).hexdigest()[:16] - else: - hashes[rel] = "missing" + hashes[rel] = hash_contained_file(skill_root, rel) or "missing" return hashes diff --git a/eval_runner/paired.py b/eval_runner/paired.py index 81e0c44..ecbbb3d 100644 --- a/eval_runner/paired.py +++ b/eval_runner/paired.py @@ -19,6 +19,7 @@ from .comparison import build_comparison_report, format_comparison_summary, writ from .grader import grade_output from .manifest import build_manifest, write_manifest from .models import AdapterInput, EvalCase +from .path_safety import contained_path from .sandbox import cleanup_sandbox, stage_paired_sandboxes @@ -28,18 +29,19 @@ def run_paired_trial( skill_path: Path, output_dir: Path, model: str, + model_label: str | None = None, ) -> dict[str, Any]: """Run one case in candidate and baseline conditions, grade, and compare.""" candidate_sandbox, baseline_sandbox = stage_paired_sandboxes(skill_path) try: - candidate_output_dir = output_dir / "candidate" / case.id - baseline_output_dir = output_dir / "baseline" / case.id + candidate_output_dir = contained_path(output_dir, "candidate", case.id) + baseline_output_dir = contained_path(output_dir, "baseline", case.id) candidate_input = AdapterInput( skill_path=candidate_sandbox, case=case, - work_dir=output_dir / "work" / "candidate" / case.id, + work_dir=contained_path(output_dir, "work", "candidate", case.id), output_dir=candidate_output_dir, model=model, permissions={"skill_readonly": True, "grader_visible": False}, @@ -49,7 +51,7 @@ def run_paired_trial( baseline_input = AdapterInput( skill_path=baseline_sandbox, case=case, - work_dir=output_dir / "work" / "baseline" / case.id, + work_dir=contained_path(output_dir, "work", "baseline", case.id), output_dir=baseline_output_dir, model=model, permissions={"skill_readonly": False, "grader_visible": False}, @@ -64,13 +66,14 @@ def run_paired_trial( baseline_result = adapter.execute(baseline_input) b_finished = datetime.now(timezone.utc) + reported_model = model_label or model candidate_manifest = build_manifest( adapter_name=adapter.name, adapter_version=adapter.version, harness_name=adapter.name, harness_version=adapter.version, - model_provider="unspecified" if not model else model.split("/")[0], - model_id=model or "unspecified", + model_provider="unspecified" if not reported_model else reported_model.split("/")[0], + model_id=reported_model or "unspecified", adapter_input=candidate_input, adapter_output=candidate_result, started_at=c_started, @@ -82,16 +85,17 @@ def run_paired_trial( adapter_version=adapter.version, harness_name=adapter.name, harness_version=adapter.version, - model_provider="unspecified" if not model else model.split("/")[0], - model_id=model or "unspecified", + model_provider="unspecified" if not reported_model else reported_model.split("/")[0], + model_id=reported_model or "unspecified", adapter_input=baseline_input, adapter_output=baseline_result, started_at=b_started, finished_at=b_finished, ) - write_manifest(candidate_manifest, output_dir / "manifests") - write_manifest(baseline_manifest, output_dir / "manifests") + manifests_dir = contained_path(output_dir, "manifests") + write_manifest(candidate_manifest, manifests_dir) + write_manifest(baseline_manifest, manifests_dir) candidate_grade = grade_output(case.id, case.assertions, candidate_result) baseline_grade = grade_output(case.id, case.assertions, baseline_result) @@ -105,13 +109,15 @@ def run_paired_trial( baseline_manifest=baseline_manifest, ) - write_comparison_report(report, output_dir / "reports") + write_comparison_report(report, contained_path(output_dir, "reports")) return report finally: - cleanup_sandbox(candidate_sandbox) - cleanup_sandbox(baseline_sandbox) + try: + cleanup_sandbox(candidate_sandbox) + finally: + cleanup_sandbox(baseline_sandbox) def run_paired_evaluation( @@ -120,11 +126,12 @@ def run_paired_evaluation( skill_path: Path, output_dir: Path, model: str, + model_label: str | None = None, ) -> list[dict[str, Any]]: """Run all cases as paired trials and return comparison reports.""" reports = [] for case in cases: - report = run_paired_trial(adapter, case, skill_path, output_dir, model) + report = run_paired_trial(adapter, case, skill_path, output_dir, model, model_label) reports.append(report) return reports @@ -144,6 +151,11 @@ def main() -> int: parser.add_argument("--adapter", choices=["fake", "cli", "openai"], default="fake") parser.add_argument("--output-dir", type=Path, default=Path("eval-output-paired")) parser.add_argument("--model", default="") + parser.add_argument( + "--model-label", + default=None, + help="logical model label recorded in artifacts (defaults to --model)", + ) parser.add_argument("--case", dest="case_id", default=None) parser.add_argument("--command", default=None) parser.add_argument("--prompt-mode", default="stdin", choices=["stdin", "arg"]) @@ -221,7 +233,14 @@ def main() -> int: print(f"output: {output_dir}") print() - reports = run_paired_evaluation(adapter, cases, skill_path, output_dir, args.model) + reports = run_paired_evaluation( + adapter, + cases, + skill_path, + output_dir, + args.model, + args.model_label, + ) improvements = 0 regressions = 0 diff --git a/eval_runner/path_safety.py b/eval_runner/path_safety.py new file mode 100644 index 0000000..26be7e7 --- /dev/null +++ b/eval_runner/path_safety.py @@ -0,0 +1,109 @@ +"""Validation and containment helpers for repository-controlled eval paths.""" + +from __future__ import annotations + +import errno +import hashlib +import os +import re +import stat +from pathlib import Path, PurePosixPath + +_CASE_ID_RE = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*", re.ASCII) + + +def validate_case_id(case_id: str) -> str: + """Return a safe case ID or raise ValueError for unsafe input.""" + if ( + not isinstance(case_id, str) + or len(case_id) > 64 + or _CASE_ID_RE.fullmatch(case_id) is None + ): + raise ValueError( + "invalid eval case ID: expected 1-64 lowercase ASCII letters or digits " + "separated by single hyphens" + ) + return case_id + + +def validate_relative_path(path_text: str) -> str: + """Return a schema-compatible relative POSIX path or raise ValueError.""" + if not isinstance(path_text, str) or not path_text or not path_text.strip(): + raise ValueError("invalid relative path: expected a non-empty path") + if any(ord(character) < 32 or ord(character) == 127 for character in path_text): + raise ValueError("invalid relative path: control characters are forbidden") + if "\\" in path_text or path_text.endswith("/"): + raise ValueError("invalid relative path: backslashes and trailing slashes are forbidden") + + lexical_parts = path_text.split("/") + if any(part in {"", ".", ".."} for part in lexical_parts): + raise ValueError("invalid relative path: '.', '..', and empty components are forbidden") + + pure = PurePosixPath(path_text) + if pure.is_absolute() or not pure.parts: + raise ValueError("invalid relative path: expected a relative POSIX path") + return path_text + + +def hash_contained_file(root: Path, relative_path: str) -> str | None: + """Hash a regular file beneath root without following symlinks. + + Returns ``None`` when the path is absent or not a regular file. Every path + component is opened descriptor-relatively with ``O_NOFOLLOW`` so a + validation-to-read symlink swap cannot redirect the read outside ``root``. + """ + validate_relative_path(relative_path) + root_path = root.resolve(strict=True) + parts = PurePosixPath(relative_path).parts + opened_fds: list[int] = [] + + try: + current_fd = os.open( + root_path, + os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, + ) + opened_fds.append(current_fd) + + for component in parts[:-1]: + current_fd = os.open( + component, + os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, + dir_fd=current_fd, + ) + opened_fds.append(current_fd) + + file_fd = os.open( + parts[-1], + os.O_RDONLY | os.O_NONBLOCK | os.O_NOFOLLOW, + dir_fd=current_fd, + ) + opened_fds.append(file_fd) + if not stat.S_ISREG(os.fstat(file_fd).st_mode): + return None + + digest = hashlib.sha256() + while chunk := os.read(file_fd, 64 * 1024): + digest.update(chunk) + return digest.hexdigest()[:16] + except FileNotFoundError: + return None + except OSError as exc: + if exc.errno in {errno.ELOOP, errno.ENOTDIR}: + raise ValueError( + f"unsafe symlink component in contained path: {relative_path}" + ) from exc + raise + finally: + for descriptor in reversed(opened_fds): + os.close(descriptor) + + +def contained_path(root: Path, *parts: str) -> Path: + """Resolve a child path and require it to remain beneath *root*.""" + resolved_root = root.resolve() + candidate = resolved_root.joinpath(*parts).resolve() + try: + candidate.relative_to(resolved_root) + except ValueError as exc: + raise ValueError(f"output path escapes designated root: {candidate}") from exc + return candidate diff --git a/eval_runner/runner.py b/eval_runner/runner.py index 852792a..c0e83ef 100644 --- a/eval_runner/runner.py +++ b/eval_runner/runner.py @@ -13,6 +13,7 @@ from .cli_adapter import CliSubprocessAdapter from .fake_adapter import FakeAdapter from .manifest import build_manifest, write_manifest from .models import AdapterInput, EvalCase +from .path_safety import contained_path def load_cases(manifest_path: Path) -> list[EvalCase]: @@ -64,8 +65,8 @@ def run_trial( output_dir: Path, model: str, ) -> Path: - work_dir = output_dir / "work" / case.id - case_output_dir = output_dir / "trials" / case.id + work_dir = contained_path(output_dir, "work", case.id) + case_output_dir = contained_path(output_dir, "trials", case.id) adapter_input = AdapterInput( skill_path=skill_path, @@ -93,7 +94,7 @@ def run_trial( finished_at=finished_at, ) - return write_manifest(manifest, output_dir / "manifests") + return write_manifest(manifest, contained_path(output_dir, "manifests")) def main() -> int: diff --git a/eval_runner/sandbox.py b/eval_runner/sandbox.py index a820a14..eb3b3c6 100644 --- a/eval_runner/sandbox.py +++ b/eval_runner/sandbox.py @@ -20,6 +20,30 @@ EXCLUDED_SUFFIXES = {".pyc"} PRODUCTION_SURFACE = {"SKILL.md", "README.md", "references", "templates", "scripts", "assets"} +def _require_contained(path: Path, root: Path) -> None: + try: + path.resolve().relative_to(root.resolve()) + except ValueError as exc: + raise ValueError(f"sandbox source escapes skill root: {path}") from exc + + +def _reject_symlinks(path: Path, root: Path) -> None: + """Fail closed if *path* or anything beneath it is a symlink.""" + if path.is_symlink(): + raise ValueError(f"sandbox source contains symlink: {path}") + _require_contained(path, root) + if not path.is_dir(): + return + for current_root, dirs, files in os.walk(path, followlinks=False): + current = Path(current_root) + _require_contained(current, root) + for name in [*dirs, *files]: + child = current / name + if child.is_symlink(): + raise ValueError(f"sandbox source contains symlink: {child}") + _require_contained(child, root) + + def _is_excluded(path: Path, skill_root: Path) -> bool: rel = path.relative_to(skill_root) parts = rel.parts @@ -43,20 +67,35 @@ def stage_skill_sandbox(skill_path: Path, *, readonly: bool = True) -> Path: Returns the staged skill directory path. Caller is responsible for cleanup (typically via tempfile.TemporaryDirectory context). """ - staging_root = Path(tempfile.mkdtemp(prefix="eval-sandbox-")) - staged = staging_root / skill_path.name - staged.mkdir() - + if skill_path.is_symlink(): + raise ValueError(f"skill path must not be a symlink: {skill_path}") + skill_root = skill_path.resolve() + items: list[Path] = [] for item in skill_path.iterdir(): if _is_excluded(item, skill_path): continue if item.name not in PRODUCTION_SURFACE and item.is_dir(): continue + _reject_symlinks(item, skill_root) + items.append(item) + + staging_root = Path(tempfile.mkdtemp(prefix="eval-sandbox-")) + staged = staging_root / skill_path.name + staged.mkdir() + + for item in items: dest = staged / item.name if item.is_dir(): - shutil.copytree(item, dest, ignore=shutil.ignore_patterns(*EXCLUDED_DIRS)) + shutil.copytree( + item, + dest, + ignore=shutil.ignore_patterns(*EXCLUDED_DIRS), + symlinks=True, + ) else: - shutil.copy2(item, dest) + shutil.copy2(item, dest, follow_symlinks=False) + + _reject_symlinks(staged, staging_root) if readonly: for root, dirs, files in os.walk(staged): @@ -83,20 +122,42 @@ def stage_paired_sandboxes(skill_path: Path) -> tuple[Path, Path]: Returns (candidate_path, baseline_path). """ candidate = stage_skill_sandbox(skill_path, readonly=True) - baseline = stage_baseline_sandbox(skill_path) + try: + baseline = stage_baseline_sandbox(skill_path) + except Exception: + cleanup_sandbox(candidate) + raise return candidate, baseline def cleanup_sandbox(staged_path: Path) -> None: - """Remove a staged sandbox, restoring write permissions first.""" - if not staged_path.exists(): + """Remove a staged sandbox without following replacement symlinks.""" + staging_root = staged_path.parent + open_flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW + try: + root_fd = os.open(staging_root, open_flags) + except FileNotFoundError: return - for root, dirs, files in os.walk(staged_path): - for d in dirs: - p = Path(root) / d - p.chmod(p.stat().st_mode | stat.S_IWUSR) - for f in files: - p = Path(root) / f - p.chmod(p.stat().st_mode | stat.S_IWUSR) - staged_path.chmod(staged_path.stat().st_mode | stat.S_IWUSR) - shutil.rmtree(staged_path.parent, ignore_errors=True) + except OSError: + # A replacement symlink must be unlinked, never traversed. If the path + # changed to anything else, fail closed and leave it for inspection. + if staging_root.is_symlink(): + staging_root.unlink() + return + raise + + try: + for _root, _dirs, _files, dir_fd in os.fwalk( + ".", + topdown=True, + follow_symlinks=False, + dir_fd=root_fd, + ): + current_mode = stat.S_IMODE(os.fstat(dir_fd).st_mode) + os.fchmod(dir_fd, current_mode | stat.S_IWUSR) + finally: + os.close(root_fd) + + # rmtree uses descriptor-relative operations on supported Unix platforms + # and refuses to descend through symlinks introduced after the fwalk. + shutil.rmtree(staging_root) diff --git a/eval_runner/tests/test_paired.py b/eval_runner/tests/test_paired.py index f64d651..c534105 100644 --- a/eval_runner/tests/test_paired.py +++ b/eval_runner/tests/test_paired.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import re import sys import tempfile from pathlib import Path @@ -12,7 +13,11 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent)) from eval_runner.models import AdapterOutput, EvalCase, ExitStatus, ToolEvent from eval_runner.grader import AssertionVerdict, grade_output from eval_runner.sandbox import cleanup_sandbox, stage_paired_sandboxes, stage_skill_sandbox -from eval_runner.comparison import build_comparison_report, format_comparison_summary +from eval_runner.comparison import ( + build_comparison_report, + format_comparison_summary, + write_comparison_report, +) from eval_runner.paired import run_paired_trial from eval_runner.fake_adapter import FakeAdapter @@ -243,6 +248,155 @@ def test_paired_trial_candidate_cannot_read_evals(): cleanup_sandbox(staged) +def test_sandbox_rejects_top_level_symlink(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + outside = tmp_path / "outside.md" + outside.write_text("private") + (skill / "linked.md").symlink_to(outside) + + try: + stage_skill_sandbox(skill, readonly=False) + except ValueError as exc: + assert "symlink" in str(exc) + else: + raise AssertionError("top-level symlink was staged") + + +def test_sandbox_rejects_nested_symlink(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + outside = tmp_path / "outside.md" + outside.write_text("private") + (skill / "references" / "linked.md").symlink_to(outside) + + try: + stage_skill_sandbox(skill, readonly=False) + except ValueError as exc: + assert "symlink" in str(exc) + else: + raise AssertionError("nested symlink was staged") + + +def test_paired_trial_uses_generic_model_label_in_artifacts(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + report = run_paired_trial( + FakeAdapter(), + _make_case(), + _make_skill_dir(tmp_path), + tmp_path / "output", + "private-runtime-model", + "configured-model", + ) + assert report["candidate"]["manifest"]["model"]["model_id"] == "configured-model" + assert report["baseline"]["manifest"]["model"]["model_id"] == "configured-model" + + +def test_comparison_writer_rejects_unsafe_case_id(): + with tempfile.TemporaryDirectory() as tmp: + try: + write_comparison_report( + {"case_id": "../../escape", "report_id": "report"}, + Path(tmp) / "reports", + ) + except ValueError: + pass + else: + raise AssertionError("comparison writer accepted an unsafe case ID") + + +def test_paired_trial_rejects_symlinked_output_subdirectory(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + output_dir = tmp_path / "output" + output_dir.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (output_dir / "candidate").symlink_to(outside, target_is_directory=True) + + try: + run_paired_trial( + FakeAdapter(), + _make_case(), + _make_skill_dir(tmp_path), + output_dir, + "fake-model", + ) + except ValueError as exc: + assert "escapes designated root" in str(exc) + else: + raise AssertionError("symlinked output subdirectory escaped containment") + + +def test_cleanup_does_not_follow_replaced_sandbox_symlink(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + staged = stage_skill_sandbox(_make_skill_dir(tmp_path)) + staging_root = staged.parent + moved = staged.with_name("moved-skill") + staged.rename(moved) + + outside = tmp_path / "outside" + outside.mkdir() + victim = outside / "victim.txt" + victim.write_text("do not touch") + original_mode = victim.stat().st_mode + staged.symlink_to(outside, target_is_directory=True) + + cleanup_sandbox(staged) + + assert victim.read_text() == "do not touch" + assert victim.stat().st_mode == original_mode + assert not staging_root.exists() + + +def test_cleanup_does_not_follow_replaced_nested_symlink(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + references = skill / "references" + (references / "cleanup-reference.md").write_text("reference") + staged = stage_skill_sandbox(skill) + staging_root = staged.parent + + staged_references = staged / "references" + staged.chmod(0o700) + staged_references.chmod(0o700) + staged_references.rename(staged / "moved-references") + outside = tmp_path / "outside" + outside.mkdir() + victim = outside / "victim.txt" + victim.write_text("do not touch") + original_mode = outside.stat().st_mode + staged_references.symlink_to(outside, target_is_directory=True) + + cleanup_sandbox(staged) + + assert victim.read_text() == "do not touch" + assert outside.stat().st_mode == original_mode + assert not staging_root.exists() + + +def test_workflow_uses_variables_without_deployment_defaults_and_pins_actions(): + workflow = ( + Path(__file__).resolve().parent.parent.parent + / ".github" + / "workflows" + / "skill-eval.yml" + ).read_text() + assert "vars.EVAL_BASE_URL ||" not in workflow + assert "vars.EVAL_MODEL ||" not in workflow + assert "http://" not in workflow + assert ".gguf" not in workflow + assert "--model-label configured-model" in workflow + action_refs = re.findall(r"uses: actions/[^@]+@([^ #\n]+)", workflow) + assert action_refs + assert all(re.fullmatch(r"[0-9a-f]{40}", ref) for ref in action_refs) + + if __name__ == "__main__": test_sandbox_excludes_eval_and_tests() test_sandbox_readonly() @@ -255,4 +409,12 @@ if __name__ == "__main__": test_comparison_report_validates_against_schema() test_paired_trial_end_to_end() test_paired_trial_candidate_cannot_read_evals() + test_sandbox_rejects_top_level_symlink() + test_sandbox_rejects_nested_symlink() + test_paired_trial_uses_generic_model_label_in_artifacts() + test_comparison_writer_rejects_unsafe_case_id() + test_paired_trial_rejects_symlinked_output_subdirectory() + test_cleanup_does_not_follow_replaced_sandbox_symlink() + test_cleanup_does_not_follow_replaced_nested_symlink() + test_workflow_uses_variables_without_deployment_defaults_and_pins_actions() print("All paired evaluation tests passed.") diff --git a/eval_runner/tests/test_runner.py b/eval_runner/tests/test_runner.py index 8d41604..2e206fa 100644 --- a/eval_runner/tests/test_runner.py +++ b/eval_runner/tests/test_runner.py @@ -12,6 +12,9 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent)) from eval_runner import models as models from eval_runner import fake_adapter as fake_adapter_mod from eval_runner import manifest as manifest_mod +from eval_runner import path_safety as path_safety_mod +from eval_runner.path_safety import contained_path +from eval_runner.runner import load_cases EvalCase = models.EvalCase AdapterInput = models.AdapterInput @@ -125,6 +128,8 @@ def test_manifest_serialization(): assert manifest["schema_version"] == 1 assert manifest["candidate"]["skill_name"] == "test-skill" + assert manifest["candidate"]["skill_path"] == "test-skill" + assert not Path(manifest["candidate"]["skill_path"]).is_absolute() assert manifest["case"]["case_id"] == "test-case-01" assert manifest["status"] == "completed" assert manifest["adapter"]["name"] == "fake" @@ -193,10 +198,260 @@ def test_eval_case_prompt_hash_deterministic(): assert len(case.prompt_hash) == 16 +def test_eval_case_rejects_unsafe_ids(): + invalid_ids = [ + "", + "../escape", + "/absolute", + "folder/case", + "folder\\case", + ".leading", + "Uppercase", + "has_underscore", + "has.dot", + "leading-", + "-trailing", + "two--hyphens", + "case\n", + "x" * 65, + ] + for case_id in invalid_ids: + try: + EvalCase( + id=case_id, + prompt="prompt", + expected_output="output", + assertions=[], + ) + except ValueError: + pass + else: + raise AssertionError(f"unsafe case ID accepted: {case_id!r}") + + assert EvalCase( + id="x" * 64, + prompt="prompt", + expected_output="output", + assertions=[], + ).id == "x" * 64 + + +def test_eval_case_rejects_unsafe_fixture_paths(): + invalid_paths = [ + "../secret", + "/absolute", + "folder/../secret", + "folder//file", + "folder\\file", + "folder/", + "folder/file\n", + "folder\nfile", + ] + for fixture_path in invalid_paths: + try: + EvalCase( + id="fixture-case", + prompt="prompt", + expected_output="output", + assertions=[], + files=[fixture_path], + ) + except ValueError: + pass + else: + raise AssertionError(f"unsafe fixture path accepted: {fixture_path!r}") + + +def test_manifest_rejects_unsafe_artifact_paths(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + output_dir = tmp_path / "output" + output_dir.mkdir() + adapter_input = AdapterInput( + case=_make_case(), + work_dir=tmp_path / "work", + output_dir=output_dir, + skill_path=skill, + ) + adapter_output = AdapterOutput( + exit_status=ExitStatus.COMPLETED, + artifacts=["../secret"], + ) + now = datetime.now(timezone.utc) + try: + build_manifest( + adapter_name="fake", + adapter_version="1", + harness_name="fake", + harness_version="1", + adapter_input=adapter_input, + adapter_output=adapter_output, + model_provider="fake", + model_id="fake", + started_at=now, + finished_at=now, + ) + except ValueError: + pass + else: + raise AssertionError("unsafe artifact path was hashed") + + +def test_hashing_rejects_symlink_escapes(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + outside = tmp_path / "secret.txt" + outside.write_text("secret") + (skill / "fixture.txt").symlink_to(outside) + + case = EvalCase( + id="fixture-case", + prompt="prompt", + expected_output="output", + assertions=[], + files=["fixture.txt"], + ) + try: + case.fixture_hashes(skill) + except ValueError: + pass + else: + raise AssertionError("fixture symlink escape was hashed") + + output_dir = tmp_path / "output" + output_dir.mkdir() + (output_dir / "artifact.txt").symlink_to(outside) + adapter_input = AdapterInput( + case=_make_case(), + work_dir=tmp_path / "work", + output_dir=output_dir, + skill_path=skill, + ) + adapter_output = AdapterOutput( + exit_status=ExitStatus.COMPLETED, + artifacts=["artifact.txt"], + ) + now = datetime.now(timezone.utc) + try: + build_manifest( + adapter_name="fake", + adapter_version="1", + harness_name="fake", + harness_version="1", + adapter_input=adapter_input, + adapter_output=adapter_output, + model_provider="fake", + model_id="fake", + started_at=now, + finished_at=now, + ) + except ValueError: + pass + else: + raise AssertionError("artifact symlink escape was hashed") + + +def test_hashing_rejects_validation_to_open_symlink_swap(): + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + skill = _make_skill_dir(tmp_path) + fixture = skill / "fixture.txt" + fixture.write_text("safe") + outside = tmp_path / "secret.txt" + outside.write_text("secret") + case = EvalCase( + id="fixture-case", + prompt="prompt", + expected_output="output", + assertions=[], + files=["fixture.txt"], + ) + + original_open = path_safety_mod.os.open + swapped = False + + def swapping_open(path, flags, mode=0o777, *, dir_fd=None): + nonlocal swapped + if path == "fixture.txt" and dir_fd is not None and not swapped: + fixture.unlink() + fixture.symlink_to(outside) + swapped = True + return original_open(path, flags, mode, dir_fd=dir_fd) + + path_safety_mod.os.open = swapping_open + try: + try: + case.fixture_hashes(skill) + except ValueError: + pass + else: + raise AssertionError("symlink swap escaped descriptor-relative hashing") + finally: + path_safety_mod.os.open = original_open + assert swapped + + +def test_load_cases_rejects_unsafe_id(): + with tempfile.TemporaryDirectory() as tmp: + manifest_path = Path(tmp) / "evals.json" + manifest_path.write_text( + json.dumps( + { + "evals": [ + { + "id": "../../escape", + "prompt": "prompt", + "expected_output": "output", + } + ] + } + ) + ) + try: + load_cases(manifest_path) + except ValueError as exc: + assert "invalid eval case ID" in str(exc) + else: + raise AssertionError("unsafe manifest case ID was accepted") + + +def test_contained_path_rejects_escape(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) / "output" + for parts in [("..", "escape"), (str(Path(tmp).parent / "absolute"),)]: + try: + contained_path(root, *parts) + except ValueError: + pass + else: + raise AssertionError(f"escaping output path accepted: {parts!r}") + + +def test_manifest_writer_rejects_unsafe_case_id(): + with tempfile.TemporaryDirectory() as tmp: + manifest = {"trial_id": "trial", "case": {"case_id": "../../escape"}} + try: + write_manifest(manifest, Path(tmp) / "manifests") + except ValueError: + pass + else: + raise AssertionError("manifest writer accepted an unsafe case ID") + + if __name__ == "__main__": test_fake_adapter_returns_completed() test_fake_adapter_missing_evidence() test_manifest_serialization() test_manifest_validates_against_schema() test_eval_case_prompt_hash_deterministic() + test_eval_case_rejects_unsafe_ids() + test_eval_case_rejects_unsafe_fixture_paths() + test_manifest_rejects_unsafe_artifact_paths() + test_hashing_rejects_symlink_escapes() + test_hashing_rejects_validation_to_open_symlink_swap() + test_load_cases_rejects_unsafe_id() + test_contained_path_rejects_escape() + test_manifest_writer_rejects_unsafe_case_id() print("All tests passed.") diff --git a/schemas/evals-v1.schema.json b/schemas/evals-v1.schema.json index f1852d2..5229ab6 100644 --- a/schemas/evals-v1.schema.json +++ b/schemas/evals-v1.schema.json @@ -18,7 +18,7 @@ "relative_path": { "type": "string", "minLength": 1, - "pattern": "^(?=.*\\S)(?!/)(?!\\./)(?!.*//)(?!.*(?:^|/)\\.(?:/|$))(?!.*(?:^|/)\\.\\.(?:/|$))(?!.*\\\\)(?!.*\\/$).+$" + "pattern": "^(?![\\s\\S]*[\\u0000-\\u001F\\u007F])(?=.*\\S)(?!/)(?!\\./)(?!.*//)(?!.*(?:^|/)\\.(?:/|$))(?!.*(?:^|/)\\.\\.(?:/|$))(?!.*\\\\)(?!.*\\/$).+$" }, "eval_case": { "type": "object", @@ -29,7 +29,7 @@ "type": "string", "minLength": 1, "maxLength": 64, - "pattern": "^[a-z0-9]+(?:-[a-z0-9]+)*$" + "pattern": "^(?![\\s\\S]*[\\u0000-\\u001F\\u007F])[a-z0-9]+(?:-[a-z0-9]+)*$" }, "prompt": {"type": "string", "minLength": 1, "pattern": "\\S"}, "expected_output": {"type": "string", "minLength": 1, "pattern": "\\S"},