From c7ba9818801ddfd31671c1256e46ee539b44231c Mon Sep 17 00:00:00 2001 From: Akshaya Shanbhogue Date: Mon, 14 Sep 2026 12:47:30 -0700 Subject: [PATCH] fix: close path-traversal gap and harness scoring nits from full code review Ran the full 8-axis coder-eval-code-review-full pass and fixed the mechanically-safe, high-confidence findings: - orchestration/batch.py::clear_rerun_artifacts joined an unvalidated task_id onto the run dir and rmtree'd it with no containment check, letting a crafted task_id delete outside the run directory. Extracted regrade.py's containment check into a shared path_utils.is_within and applied it here too, with a regression test. - criteria/llm_judge.py's route-dispatch match had no exhaustiveness arm (confirmed py/uninitialized-local-variable CodeQL alert); added the same case _: raise AssertionError(...) pattern already used in models/routing.py::resolve_route. - reports.py's per-criterion completion_rate published a bare 0.0 for a zero-row suite instead of following the nothing_was_measured convention pass_rate/error_share already use; now omitted so a suite_thresholds gate fails closed with a visible actual_value=None. - batch.py's cost-pricing-coverage check used an untyped getattr(criterion, "model", None) probe; replaced with isinstance narrowing against LLMJudgeCriterion, the only union member with that field. - Removed a stale optimize/ line from CLAUDE.md's directory tree. Larger findings (docker image-tag collisions under parallel builds, CE039 discipline gaps across 5 criteria checkers, generate_run_id collision risk, UiPath credentials forwarded by default, a 3443-line orchestrator.py) are documented in tmp/code-review-20260914-124404/ for dedicated follow-up rather than bundled here. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 1 - src/coder_eval/criteria/llm_judge.py | 5 ++++ src/coder_eval/orchestration/batch.py | 11 +++++-- src/coder_eval/orchestration/regrade.py | 12 ++------ src/coder_eval/path_utils.py | 15 ++++++++++ src/coder_eval/reports.py | 8 ++++- tests/test_preservation_mode.py | 40 +++++++++++++++++++++++++ tests/test_suite_rollup.py | 5 +++- 8 files changed, 81 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 857fb4470..0d0d367ec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,7 +34,6 @@ coder_eval/ ├── argv_match.py # Structured argv matcher. STDLIB-ONLY (CE057): this file is copied into the recorder dir as a SIDECAR beside every response-serving shim, which imports it as a sibling, so `cli_called` and a `record_cli` response rule dispatch on ONE semantic ├── telemetry.py # App Insights / OpenTelemetry emission (CoderEval.Task.End et al.) ├── isolation/ # driver: docker — docker_runner.py builds, runs and reaps one container per task -├── optimize/ # Prompt/config optimization helpers ├── utils.py # Version info helpers │ ├── agents/ diff --git a/src/coder_eval/criteria/llm_judge.py b/src/coder_eval/criteria/llm_judge.py index 42edad250..dfb6987dc 100644 --- a/src/coder_eval/criteria/llm_judge.py +++ b/src/coder_eval/criteria/llm_judge.py @@ -263,6 +263,11 @@ async def _invoke_tool_channel( # Handled by the unconfigured-arm guard in _check_impl_async before # dispatch; defensive only. return None, "llm_judge: no usable API route", "(no route)", None + case _: + # ApiRoute covers exactly Bedrock/Direct/LiteLLM above; this arm is + # unreachable but makes the match exhaustive (CodeQL: py/uninitialized-local-variable + # on verdict/err/response_usage below) — mirrors models/routing.py::resolve_route. + raise AssertionError(f"unhandled ApiRoute: {route!r}") if verdict is not None: return verdict, None, verdict.model_dump_json(), response_usage diff --git a/src/coder_eval/orchestration/batch.py b/src/coder_eval/orchestration/batch.py index 2dc2379d0..65b2319b8 100644 --- a/src/coder_eval/orchestration/batch.py +++ b/src/coder_eval/orchestration/batch.py @@ -21,6 +21,7 @@ AgentKind, EvaluationResult, FinalStatus, + LLMJudgeCriterion, PreservationMode, ResolvedTask, RunSummary, @@ -28,7 +29,7 @@ TaskDefinition, TaskResult, ) -from ..path_utils import TASK_JSON_FILENAME, format_task_log_id +from ..path_utils import TASK_JSON_FILENAME, format_task_log_id, is_within from ..pricing import unpriced_models from ..reports_experiment import eval_result_to_task_dict from ..streaming.callbacks import StreamCallback @@ -49,7 +50,7 @@ def _run_models(resolved_tasks: list[ResolvedTask]) -> Iterator[str | None]: for rt in resolved_tasks: yield rt.task.agent.model if rt.task.agent else None for criterion in rt.task.success_criteria: - yield getattr(criterion, "model", None) + yield criterion.model if isinstance(criterion, LLMJudgeCriterion) else None def check_pricing_coverage(resolved_tasks: list[ResolvedTask]) -> list[str]: @@ -452,7 +453,11 @@ def clear_rerun_artifacts(to_run: list[ResolvedTask]) -> int: cleared = 0 for rt in to_run: artifacts = rt.run_dir / "artifacts" / rt.task.task_id - if artifacts.exists(): + # task_id is an unvalidated string (dataset rows are "/"), so + # a crafted value like "../../../home/victim" joins to a real directory + # exists() happily confirms — mirror regrade.py's containment check + # before ever deleting anything derived from it. + if artifacts.exists() and is_within(artifacts, rt.run_dir): shutil.rmtree(artifacts, ignore_errors=True) cleared += 1 return cleared diff --git a/src/coder_eval/orchestration/regrade.py b/src/coder_eval/orchestration/regrade.py index ae4b19aa5..bfe2a9d81 100644 --- a/src/coder_eval/orchestration/regrade.py +++ b/src/coder_eval/orchestration/regrade.py @@ -41,6 +41,7 @@ GRADE_LOG_FILENAME, PRE_GRADE_JSON_FILENAME, TASK_JSON_FILENAME, + is_within, write_text_atomic, ) from coder_eval.sandbox import Sandbox @@ -498,7 +499,7 @@ def default_workspace(run_dir: Path, prior: EvaluationResult) -> Path: def _contained(candidate: Path, description: str) -> Path: # One chokepoint, one root. `run_dir` is the operator-supplied path; a # candidate is only ever derived from the untrusted record. - if not _is_within(candidate, run_dir): + if not is_within(candidate, run_dir): raise RegradeError( f"{description} resolves outside the run directory ({run_dir}). " + "Pass --workspace explicitly to grade a directory outside the run." @@ -550,15 +551,6 @@ def _contained(candidate: Path, description: str) -> Path: ) -def _is_within(candidate: Path, root: Path) -> bool: - """True when ``candidate`` resolves inside ``root``.""" - try: - candidate.resolve().relative_to(root.resolve()) - except ValueError: - return False - return True - - def verify_reference_unchanged(prior: EvaluationResult, task: TaskDefinition, task_file: Path | None) -> None: """Refuse to grade when the reference tree changed since the run. diff --git a/src/coder_eval/path_utils.py b/src/coder_eval/path_utils.py index 46c97f54e..711fb9037 100644 --- a/src/coder_eval/path_utils.py +++ b/src/coder_eval/path_utils.py @@ -158,6 +158,21 @@ def rmtree_restrictive(root: Path) -> None: logger.warning("Directory %s could not be fully removed", root) +def is_within(candidate: Path, root: Path) -> bool: + """True when ``candidate`` resolves inside ``root``. + + Shared containment check for any path joined from an untrusted, task- or + record-authored field (a ``task_id``, a recorded ``sandbox_path``) before + it is read, written, or deleted — ``"../../etc"`` joins to a real path + that ``is_dir()``/``exists()`` happily confirms. + """ + try: + candidate.resolve().relative_to(root.resolve()) + except ValueError: + return False + return True + + def ignore_patterns_and_symlinks(patterns: list[str]) -> Callable[[str, list[str]], set[str]]: """``copytree`` ``ignore`` callable that drops pattern matches AND every symlink. diff --git a/src/coder_eval/reports.py b/src/coder_eval/reports.py index 4981b9793..bd5304cd5 100644 --- a/src/coder_eval/reports.py +++ b/src/coder_eval/reports.py @@ -850,7 +850,13 @@ def _attach_row_accounting(agg: CriterionAggregate, rows_total: int, rows_aggreg """ excluded = rows_total - rows_aggregated metrics = dict(agg.metrics) - metrics["completion_rate"] = (rows_aggregated / rows_total) if rows_total else 0.0 + # Omit rather than publish 0.0 when there is no denominator: a suite_thresholds + # gate on this metric then fails closed with actual_value=None (see + # _evaluate_thresholds), distinguishable from "measured and completely failed" — + # the same nondescript-zero shape pass_rate/error_share guard against via + # nothing_was_measured. + if rows_total: + metrics["completion_rate"] = rows_aggregated / rows_total return agg.model_copy(update={"rows_total": rows_total, "rows_excluded": excluded, "metrics": metrics}) diff --git a/tests/test_preservation_mode.py b/tests/test_preservation_mode.py index b7e1e4806..b5e616346 100644 --- a/tests/test_preservation_mode.py +++ b/tests/test_preservation_mode.py @@ -141,6 +141,46 @@ def _rt(task_id: str) -> ResolvedTask: assert not (stale.run_dir / "artifacts" / "stale").exists() +def test_clear_rerun_artifacts_refuses_path_traversal(tmp_path): + """A crafted task_id ('../../escape') must not delete outside the run dir. + + task_id is an unvalidated string (dataset rows are "/"), so + "../../../../victim" joins to a real directory `exists()` happily confirms. + Mirrors the containment check orchestration/regrade.py applies to the same + recorded-task_id shape. + """ + from coder_eval.models import ResolvedTask, TaskDefinition + from coder_eval.orchestration.batch import clear_rerun_artifacts + + task_id = "../../escaped" + task = TaskDefinition( + task_id=task_id, + description="d", + initial_prompt="p", + agent={"type": "claude-code"}, + sandbox={"driver": "docker"}, + success_criteria=[{"type": "file_exists", "path": "x.txt", "description": "x"}], + ) + run_dir = tmp_path / "run" / "default" / "victim" / "00" + run_dir.mkdir(parents=True) + victim = tmp_path / "run" / "default" / "escaped" + victim.mkdir(parents=True) + (victim / "do-not-delete.txt").write_text("real data outside the run dir") + + rt = ResolvedTask( + task=task, + task_file=tmp_path / "t.yaml", + run_dir=run_dir, + variant_id="default", + original_task_id=task_id, + ) + + cleared = clear_rerun_artifacts([rt]) + + assert cleared == 0 + assert (victim / "do-not-delete.txt").exists() + + @pytest.mark.asyncio async def test_run_batch_dispatches_resolved_mode_for_tempdir(tmp_path): """run_batch must hand the driver-derived mode (tempdir→MOVE_ON_WRITE) to the Orchestrator.""" diff --git a/tests/test_suite_rollup.py b/tests/test_suite_rollup.py index 7adb9c7b8..17e8aadb1 100644 --- a/tests/test_suite_rollup.py +++ b/tests/test_suite_rollup.py @@ -589,7 +589,10 @@ def test_attach_row_accounting_zero_total_no_div_error(self) -> None: out = _attach_row_accounting(agg, rows_total=0, rows_aggregated=0) assert out.rows_total == 0 assert out.rows_excluded == 0 - assert out.metrics["completion_rate"] == 0.0 + # No denominator: completion_rate is omitted rather than published as + # 0.0 (indistinguishable from "measured and completely failed") — a + # suite_thresholds gate on it then fails closed via actual_value=None. + assert "completion_rate" not in out.metrics def test_render_includes_denominator_line_when_excluded(self) -> None: agg = CriterionAggregate(