From 04e3553a72c337867921f1d0de2fc9e2e91f7013 Mon Sep 17 00:00:00 2001 From: krisztianfekete Date: Fri, 10 Jul 2026 15:06:40 +0200 Subject: [PATCH 1/2] refactor: consolidate filesystem path handling for file endpoints --- src/agentevals/api/app.py | 10 +- src/agentevals/api/routes.py | 26 ++- src/agentevals/api/streaming_routes.py | 38 ++-- src/agentevals/streaming/exports.py | 35 +++ src/agentevals/streaming/ws_server.py | 3 +- tests/test_api.py | 282 +++++++++++++++++++++++-- 6 files changed, 352 insertions(+), 42 deletions(-) create mode 100644 src/agentevals/streaming/exports.py diff --git a/src/agentevals/api/app.py b/src/agentevals/api/app.py index 80a9790..48c56f9 100644 --- a/src/agentevals/api/app.py +++ b/src/agentevals/api/app.py @@ -141,6 +141,7 @@ def create_app( *, trace_manager: StreamingTraceManager | None = None, enable_streaming: bool = False, + static_dir: Path | None = None, ) -> FastAPI: """Create the main agentevals API app.""" app = FastAPI( @@ -216,7 +217,8 @@ async def event_generator(): }, ) - static_dir = Path(__file__).parent.parent / "_static" + if static_dir is None: + static_dir = Path(__file__).parent.parent / "_static" has_ui = static_dir.is_dir() and (static_dir / "index.html").exists() if has_ui and not os.getenv("AGENTEVALS_HEADLESS"): @@ -229,10 +231,12 @@ async def event_generator(): async def root(): return FileResponse(static_dir / "index.html") + static_root = static_dir.resolve() + @app.get("/{path:path}") async def spa_fallback(path: str): - file_path = static_dir / path - if file_path.is_file(): + file_path = (static_dir / path).resolve() + if file_path.is_relative_to(static_root) and file_path.is_file(): return FileResponse(file_path) return FileResponse(static_dir / "index.html") diff --git a/src/agentevals/api/routes.py b/src/agentevals/api/routes.py index eb9e0bc..a958990 100644 --- a/src/agentevals/api/routes.py +++ b/src/agentevals/api/routes.py @@ -119,6 +119,15 @@ def _camel_keys(obj: Any) -> Any: return obj +def _safe_upload_path(temp_dir: str, filename: str, idx: int = 0) -> str: + """Build a save path inside ``temp_dir`` from a client-controlled filename. + + ``basename`` drops any directory components, and the ``idx`` prefix keeps + same-named uploads from clobbering each other. + """ + return os.path.join(temp_dir, f"{idx}_{os.path.basename(filename)}") + + def _load_eval_set_dict(path: str | None) -> dict | None: """Read the uploaded eval set file back into a dict for persistence. @@ -361,7 +370,7 @@ async def validate_eval_set( ): temp_dir = tempfile.mkdtemp() try: - eval_set_path = os.path.join(temp_dir, eval_set_file.filename or "eval_set.json") + eval_set_path = os.path.join(temp_dir, f"evalset_{os.path.basename(eval_set_file.filename or 'eval_set.json')}") with open(eval_set_path, "wb") as f: # noqa: ASYNC230 content = await eval_set_file.read() f.write(content) @@ -433,8 +442,7 @@ async def convert_trace_files( detail=f"Invalid file extension for {original}. Only .json and .jsonl files are allowed.", ) - safe_name = f"{idx}_{os.path.basename(original)}" - trace_path = os.path.join(temp_dir, safe_name) + trace_path = _safe_upload_path(temp_dir, original, idx) with open(trace_path, "wb") as f: # noqa: ASYNC230 content = await trace_file.read() @@ -548,7 +556,7 @@ async def evaluate_traces( raise HTTPException(status_code=400, detail=f"Invalid credentialRefs: {exc}") from exc trace_paths = [] - for trace_file in trace_files: + for idx, trace_file in enumerate(trace_files): if not trace_file.filename: continue @@ -558,7 +566,7 @@ async def evaluate_traces( detail=f"Invalid file extension for {trace_file.filename}. Only .json and .jsonl files are allowed.", ) - trace_path = os.path.join(temp_dir, trace_file.filename) + trace_path = _safe_upload_path(temp_dir, trace_file.filename, idx) with open(trace_path, "wb") as f: # noqa: ASYNC230 content = await trace_file.read() @@ -587,7 +595,7 @@ async def evaluate_traces( detail="Invalid file extension for eval set. Only .json files are allowed.", ) - eval_set_path = os.path.join(temp_dir, eval_set_file.filename) + eval_set_path = os.path.join(temp_dir, f"evalset_{os.path.basename(eval_set_file.filename)}") with open(eval_set_path, "wb") as f: # noqa: ASYNC230 content = await eval_set_file.read() if len(content) > 10 * 1024 * 1024: @@ -669,7 +677,7 @@ async def event_generator(): return trace_paths = [] - for trace_file in trace_files: + for idx, trace_file in enumerate(trace_files): if not trace_file.filename: continue @@ -677,7 +685,7 @@ async def event_generator(): yield f"data: {SSEErrorEvent(error=f'Invalid file extension for {trace_file.filename}').model_dump_json(by_alias=True)}\n\n" return - trace_path = os.path.join(temp_dir, trace_file.filename) + trace_path = _safe_upload_path(temp_dir, trace_file.filename, idx) with open(trace_path, "wb") as f: # noqa: ASYNC230 content = await trace_file.read() @@ -700,7 +708,7 @@ async def event_generator(): yield f"data: {SSEErrorEvent(error='Invalid file extension for eval set').model_dump_json(by_alias=True)}\n\n" return - eval_set_path = os.path.join(temp_dir, eval_set_file.filename) + eval_set_path = os.path.join(temp_dir, f"evalset_{os.path.basename(eval_set_file.filename)}") with open(eval_set_path, "wb") as f: # noqa: ASYNC230 content = await eval_set_file.read() if len(content) > 10 * 1024 * 1024: diff --git a/src/agentevals/api/streaming_routes.py b/src/agentevals/api/streaming_routes.py index 831fcb5..7910533 100644 --- a/src/agentevals/api/streaming_routes.py +++ b/src/agentevals/api/streaming_routes.py @@ -5,6 +5,7 @@ import asyncio import json import logging +import os from typing import TYPE_CHECKING from fastapi import APIRouter, Depends, HTTPException, Request @@ -15,6 +16,7 @@ from ..converter import convert_traces from ..loader.otlp import OtlpJsonLoader from ..runner import RunResult, run_evaluation +from ..streaming.exports import EXPORT_DIR, export_name from ..trace_attrs import OTEL_GENAI_INPUT_MESSAGES, OTEL_GENAI_REQUEST_MODEL from .dependencies import require_trace_manager from .models import ( @@ -225,7 +227,9 @@ async def evaluate_sessions( import tempfile - eval_set_file = tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False, encoding="utf-8") + eval_set_file = tempfile.NamedTemporaryFile( + mode="w", suffix=".json", delete=False, dir=str(EXPORT_DIR), encoding="utf-8" + ) json.dump(eval_set_response.data.eval_set, eval_set_file) eval_set_file.close() @@ -328,12 +332,7 @@ async def prepare_evaluation( manager, ) - import os - import tempfile - - temp_dir = tempfile.gettempdir() - - eval_set_file = os.path.join(temp_dir, f"eval_set_{request.golden_session_id}.json") + eval_set_file = EXPORT_DIR / export_name(request.golden_session_id, prefix="eval_set_", suffix=".json") with open(eval_set_file, "w", encoding="utf-8") as f: # noqa: ASYNC230 json.dump(eval_set_response.data.eval_set, f) @@ -353,7 +352,7 @@ async def prepare_evaluation( return StandardResponse( data=PrepareEvaluationData( - eval_set_url=f"/api/streaming/download/{os.path.basename(eval_set_file)}", + eval_set_url=f"/api/streaming/download/{eval_set_file.name}", trace_urls=[f"/api/streaming/download/{os.path.basename(tf['file_path'])}" for tf in trace_files], num_traces=len(trace_files), ) @@ -368,20 +367,25 @@ async def prepare_evaluation( @streaming_router.get("/download/{filename}") async def download_file(filename: str): - """Download a prepared trace or eval set file.""" - import os - import tempfile + """Download a prepared trace or eval set file from the export directory. + + Only bare filenames are accepted, and the resolved path must stay inside + the export directory. Anything that resolves outside it returns the same + 404 as a missing file. + """ + if filename in {"", ".", ".."} or filename != os.path.basename(filename): + raise HTTPException(status_code=400, detail="Invalid filename") - temp_dir = tempfile.gettempdir() - file_path = os.path.join(temp_dir, filename) + export_root = EXPORT_DIR.resolve() + candidate = (export_root / filename).resolve() - if not os.path.exists(file_path): # noqa: ASYNC240 + if not candidate.is_relative_to(export_root): raise HTTPException(status_code=404, detail="File not found") - if not file_path.startswith(temp_dir): - raise HTTPException(status_code=400, detail="Invalid file path") + if not candidate.is_file(): # noqa: ASYNC240 + raise HTTPException(status_code=404, detail="File not found") - return FileResponse(file_path, media_type="application/json", filename=filename) + return FileResponse(candidate, media_type="application/json", filename=filename) @streaming_router.post("/get-trace", response_model=StandardResponse[GetTraceData]) diff --git a/src/agentevals/streaming/exports.py b/src/agentevals/streaming/exports.py new file mode 100644 index 0000000..4902316 --- /dev/null +++ b/src/agentevals/streaming/exports.py @@ -0,0 +1,35 @@ +"""Process-private directory for download-eligible export files. + +Files served by ``/api/streaming/download`` are confined to this directory +rather than the shared system temp root, so only files this process explicitly +writes here are download-eligible. + +Export names are derived from a per-process HMAC of the session id rather than +the raw id, keeping download URLs independent of client-supplied identifiers. +""" + +from __future__ import annotations + +import hashlib +import hmac +import secrets +import tempfile +from pathlib import Path + +EXPORT_DIR = Path(tempfile.mkdtemp(prefix="agentevals-exports-")) +# mkdtemp already restricts the dir to the owner; set it explicitly so the +# permission is guaranteed even if the creation call is ever changed. +EXPORT_DIR.chmod(0o700) + +_NAME_KEY = secrets.token_bytes(32) + + +def export_name(seed: str, *, prefix: str, suffix: str) -> str: + """Build a stable, opaque export filename for ``seed``. + + The same seed maps to the same name within a process run, so repeated + writes for one session reuse a single file, while the name stays + independent of the seed's value (it derives from a per-process key). + """ + token = hmac.new(_NAME_KEY, seed.encode(), hashlib.sha256).hexdigest()[:32] + return f"{prefix}{token}{suffix}" diff --git a/src/agentevals/streaming/ws_server.py b/src/agentevals/streaming/ws_server.py index 966c599..741b145 100644 --- a/src/agentevals/streaming/ws_server.py +++ b/src/agentevals/streaming/ws_server.py @@ -30,6 +30,7 @@ from ..loader.otlp import OtlpJsonLoader from ..trace_attrs import OTEL_GENAI_INPUT_MESSAGES, OTEL_GENAI_REQUEST_MODEL, OTEL_SERVICE_NAME from ..utils.log_enrichment import enrich_spans_with_logs +from .exports import EXPORT_DIR, export_name from .incremental_processor import IncrementalInvocationExtractor from .session import TraceSession @@ -654,7 +655,7 @@ async def _save_spans_to_temp_file(self, session: TraceSession) -> Path: Returns: Path to the temporary JSONL file containing the spans """ - temp_file = Path(tempfile.gettempdir()) / f"agentevals_{session.session_id}.jsonl" + temp_file = EXPORT_DIR / export_name(session.session_id, prefix="agentevals_", suffix=".jsonl") enriched_spans = enrich_spans_with_logs(session.spans, session.logs, session.session_id) diff --git a/tests/test_api.py b/tests/test_api.py index 59e4f99..8c1ea0c 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -12,6 +12,7 @@ import os import re import tempfile +import urllib.parse import zipfile from datetime import UTC, datetime from unittest.mock import AsyncMock, MagicMock, patch @@ -32,6 +33,7 @@ from agentevals.api.routes import _camel_keys, router from agentevals.api.streaming_routes import streaming_router from agentevals.runner import MetricResult, RunResult, TraceResult +from agentevals.streaming.exports import EXPORT_DIR from agentevals.streaming.session import TraceSession # --------------------------------------------------------------------------- @@ -258,6 +260,38 @@ def _side_effect(*args, **kwargs): return _side_effect +def _capturing_paths(captured: dict): + """Record the on-disk upload paths the route hands to ``run_evaluation``. + + The path traversal fix lives in how the route names the saved file, so the + boundary that proves it is exactly the ``EvalRunConfig`` the evaluator receives. + Existence is captured at call time, before the route's ``finally`` cleans the temp dir. + """ + + def _side_effect(cfg, *args, **kwargs): + captured["trace_files"] = list(cfg.trace_files) + captured["eval_set_file"] = cfg.eval_set_file + captured["existed"] = {p: os.path.exists(p) for p in cfg.trace_files} + return _make_run_result() + + return _side_effect + + +_UI_INDEX = "agentevals-ui-index-sentinel" +_UI_ASSET = "export const marker = 'ui-asset-sentinel';" + + +def _make_ui_client(static_dir, monkeypatch) -> TestClient: + """Build the real app over a throwaway static dir so the SPA fallback is exercised, not copied.""" + from agentevals.api.app import create_app + + monkeypatch.delenv("AGENTEVALS_HEADLESS", raising=False) + (static_dir / "assets").mkdir(parents=True, exist_ok=True) + (static_dir / "index.html").write_text(_UI_INDEX) + (static_dir / "assets" / "app.js").write_text(_UI_ASSET) + return TestClient(create_app(static_dir=static_dir)) + + # --------------------------------------------------------------------------- # Model Serialization # --------------------------------------------------------------------------- @@ -619,6 +653,54 @@ def test_evaluate_unresolvable_credential_returns_400(self, mock_eval, monkeypat assert "Could not resolve credentialRefs" in resp.json()["detail"] mock_eval.assert_not_called() + @patch("agentevals.api.routes.run_evaluation", new_callable=AsyncMock) + def test_evaluate_sanitizes_traversal_trace_filename(self, mock_eval): + captured: dict = {} + mock_eval.side_effect = _capturing_paths(captured) + resp = self.client.post( + "/api/evaluate", + files={"trace_files": ("../../outside.json", io.BytesIO(_make_trace_json()))}, + data={"config": _eval_config_json()}, + ) + _assert_envelope(resp) + saved = captured["trace_files"][0] + assert os.path.basename(saved) == "0_outside.json" + assert ".." not in saved + assert captured["existed"][saved] is True + + @patch("agentevals.api.routes.run_evaluation", new_callable=AsyncMock) + def test_evaluate_sanitizes_traversal_eval_set_filename(self, mock_eval): + captured: dict = {} + mock_eval.side_effect = _capturing_paths(captured) + resp = self.client.post( + "/api/evaluate", + files={ + "trace_files": ("trace.json", io.BytesIO(_make_trace_json())), + "eval_set_file": ("../../outside.json", io.BytesIO(_make_eval_set_json())), + }, + data={"config": _eval_config_json()}, + ) + _assert_envelope(resp) + eval_set = captured["eval_set_file"] + assert os.path.basename(eval_set) == "evalset_outside.json" + assert ".." not in eval_set + + @patch("agentevals.api.routes.run_evaluation", new_callable=AsyncMock) + def test_evaluate_duplicate_filenames_do_not_clobber(self, mock_eval): + captured: dict = {} + mock_eval.side_effect = _capturing_paths(captured) + resp = self.client.post( + "/api/evaluate", + files=[ + ("trace_files", ("same.json", io.BytesIO(_make_trace_json()))), + ("trace_files", ("same.json", io.BytesIO(_make_trace_json()))), + ], + data={"config": _eval_config_json()}, + ) + _assert_envelope(resp) + names = [os.path.basename(p) for p in captured["trace_files"]] + assert names == ["0_same.json", "1_same.json"] + # --------------------------------------------------------------------------- # POST /api/evaluate/stream (SSE) @@ -710,6 +792,27 @@ def test_stream_bad_credential_refs(self): assert '"error"' in resp.text assert "credentialRefs" in resp.text + @patch("agentevals.api.routes.run_evaluation", new_callable=AsyncMock) + @patch("agentevals.api.routes.load_traces") + def test_stream_sanitizes_traversal_filenames(self, mock_load_traces, mock_eval): + mock_load_traces.return_value = [] + captured: dict = {} + mock_eval.side_effect = _capturing_paths(captured) + resp = self.client.post( + "/api/evaluate/stream", + files={ + "trace_files": ("../../outside.json", io.BytesIO(_make_trace_json())), + "eval_set_file": ("../../outside.json", io.BytesIO(_make_eval_set_json())), + }, + data={"config": _eval_config_json()}, + ) + assert '"done"' in resp.text + saved = captured["trace_files"][0] + assert os.path.basename(saved) == "0_outside.json" + assert ".." not in saved + assert os.path.basename(captured["eval_set_file"]) == "evalset_outside.json" + assert ".." not in captured["eval_set_file"] + # --------------------------------------------------------------------------- # POST /api/evaluate/json @@ -1318,36 +1421,136 @@ def test_prepare_skips_incomplete(self, mock_create_eval): # --------------------------------------------------------------------------- +# A benign file placed outside the export directory. Its content token must +# never appear in a download response; if it does, containment leaked. +_OUT_OF_SCOPE_NAME = "out_of_scope_ref.txt" +_OUT_OF_SCOPE_TOKEN = "out-of-scope-marker-a1b2c3" + + class TestStreamingDownload: @classmethod def setup_class(cls): cls.mgr = _make_trace_manager() cls.app = _make_live_app(cls.mgr) - def test_download_missing(self): - client = TestClient(self.app) - resp = client.get("/api/streaming/download/nonexistent_file_abc123.json") - assert resp.status_code == 404 + def test_happy_path_downloads_byte_for_byte(self): + payload = b'{"marker":"in-scope","value":123}\n' + target = EXPORT_DIR / "happy_download.json" + target.write_bytes(payload) + try: + client = TestClient(self.app) + resp = client.get("/api/streaming/download/happy_download.json") + assert resp.status_code == 200 + assert resp.content == payload + finally: + target.unlink(missing_ok=True) - def test_download_success(self): + def test_file_in_shared_temp_root_not_downloadable(self): with tempfile.NamedTemporaryFile( mode="w", suffix=".json", delete=False, dir=tempfile.gettempdir(), encoding="utf-8" ) as f: - f.write('{"test": true}') + f.write(_OUT_OF_SCOPE_TOKEN) fname = os.path.basename(f.name) - try: client = TestClient(self.app) resp = client.get(f"/api/streaming/download/{fname}") - assert resp.status_code == 200 - assert resp.json() == {"test": True} + assert resp.status_code == 404 + assert _OUT_OF_SCOPE_TOKEN not in resp.text finally: os.unlink(os.path.join(tempfile.gettempdir(), fname)) - def test_download_path_traversal(self): + def test_missing_bare_name_returns_404(self): + client = TestClient(self.app) + resp = client.get("/api/streaming/download/definitely_absent_file.json") + assert resp.status_code == 404 + + def test_non_bare_filenames_are_rejected_without_leaking(self): + sentinel = os.path.join(tempfile.gettempdir(), _OUT_OF_SCOPE_NAME) + with open(sentinel, "w", encoding="utf-8") as f: + f.write(_OUT_OF_SCOPE_TOKEN) + + abs_encoded = urllib.parse.quote(sentinel, safe="") + payloads = [ + f"../{_OUT_OF_SCOPE_NAME}", + f"..%2f..%2f{_OUT_OF_SCOPE_NAME}", + abs_encoded, + "..", + ".", + ] + try: + client = TestClient(self.app) + for payload in payloads: + resp = client.get(f"/api/streaming/download/{payload}") + assert resp.status_code in (400, 404), payload + assert _OUT_OF_SCOPE_TOKEN not in resp.text, payload + finally: + os.unlink(sentinel) + + def test_symlink_escape_is_blocked(self): + sentinel = os.path.join(tempfile.gettempdir(), _OUT_OF_SCOPE_NAME) + with open(sentinel, "w", encoding="utf-8") as f: + f.write(_OUT_OF_SCOPE_TOKEN) + + link = EXPORT_DIR / "linked.json" + link.unlink(missing_ok=True) + os.symlink(sentinel, link) + try: + client = TestClient(self.app) + resp = client.get("/api/streaming/download/linked.json") + assert resp.status_code == 404 + assert _OUT_OF_SCOPE_TOKEN not in resp.text + finally: + link.unlink(missing_ok=True) + os.unlink(sentinel) + + def test_containment_is_checked_before_existence(self): + # A symlink resolving outside the export dir must be indistinguishable + # from a plain missing file: same status and same body, so the endpoint + # cannot be used to probe whether out-of-scope paths exist. + sentinel = os.path.join(tempfile.gettempdir(), _OUT_OF_SCOPE_NAME) + with open(sentinel, "w", encoding="utf-8") as f: + f.write(_OUT_OF_SCOPE_TOKEN) + + link = EXPORT_DIR / "probe.json" + link.unlink(missing_ok=True) + os.symlink(sentinel, link) + try: + client = TestClient(self.app) + out_of_scope = client.get("/api/streaming/download/probe.json") + missing = client.get("/api/streaming/download/absent_probe.json") + assert out_of_scope.status_code == missing.status_code == 404 + assert out_of_scope.json() == missing.json() + finally: + link.unlink(missing_ok=True) + os.unlink(sentinel) + + @patch("agentevals.api.streaming_routes._do_create_eval_set", new_callable=AsyncMock) + def test_prepared_files_download_byte_for_byte(self, mock_create_eval): + self.mgr.sessions.clear() + self.mgr.sessions["golden"] = _make_session("golden", "tg") + self.mgr.sessions["s1"] = _make_session("s1", "t1", spans=[{"spanId": "sp1"}]) + + mock_create_eval.return_value = StandardResponse( + data=CreateEvalSetData( + eval_set={"eval_set_id": "e1", "eval_cases": []}, + num_invocations=1, + ) + ) + client = TestClient(self.app) - resp = client.get("/api/streaming/download/..%2F..%2Fetc%2Fpasswd") - assert resp.status_code in (400, 404) + body = _assert_envelope( + client.post( + "/api/streaming/prepare-evaluation", + json={"golden_session_id": "golden", "session_ids": ["s1"]}, + ) + ) + + for url in [body["data"]["evalSetUrl"], *body["data"]["traceUrls"]]: + assert url.startswith("/api/streaming/download/") + on_disk = (EXPORT_DIR / url.rsplit("/", 1)[-1]).read_bytes() + resp = client.get(url) + assert resp.status_code == 200 + assert resp.content == on_disk # --------------------------------------------------------------------------- @@ -1666,3 +1869,58 @@ def test_unknown_shape_returns_load_warning(self): ) assert resp.status_code == 400 assert "Could not detect trace format" in resp.json()["detail"] + + +# --------------------------------------------------------------------------- +# SPA fallback path containment (static file serving) +# --------------------------------------------------------------------------- + + +class TestSpaFallbackContainment: + def test_serves_index_at_root(self, tmp_path, monkeypatch): + client = _make_ui_client(tmp_path, monkeypatch) + resp = client.get("/") + assert resp.status_code == 200 + assert "agentevals-ui-index-sentinel" in resp.text + + def test_serves_real_asset(self, tmp_path, monkeypatch): + client = _make_ui_client(tmp_path, monkeypatch) + assert "ui-asset-sentinel" in client.get("/assets/app.js").text + + def test_unknown_route_falls_back_to_index(self, tmp_path, monkeypatch): + client = _make_ui_client(tmp_path, monkeypatch) + assert "agentevals-ui-index-sentinel" in client.get("/some/client/route").text + + def test_absolute_path_join_does_not_escape(self, tmp_path, monkeypatch): + client = _make_ui_client(tmp_path, monkeypatch) + outside = tmp_path.parent / "outside_root.txt" + outside.write_text("outside-static-root") + try: + resp = client.get("http://testserver/" + str(outside)) + assert "outside-static-root" not in resp.text + assert "agentevals-ui-index-sentinel" in resp.text + finally: + outside.unlink(missing_ok=True) + + def test_encoded_dotdot_does_not_escape(self, tmp_path, monkeypatch): + client = _make_ui_client(tmp_path, monkeypatch) + outside = tmp_path.parent / "outside_root.txt" + outside.write_text("outside-static-root") + try: + resp = client.get("/%2e%2e/outside_root.txt") + assert "outside-static-root" not in resp.text + assert "agentevals-ui-index-sentinel" in resp.text + finally: + outside.unlink(missing_ok=True) + + def test_symlink_target_outside_root_not_followed(self, tmp_path, monkeypatch): + outside = tmp_path.parent / "outside_symlink_target.txt" + outside.write_text("outside-static-root") + client = _make_ui_client(tmp_path, monkeypatch) + (tmp_path / "escape_link").symlink_to(outside) + try: + resp = client.get("/escape_link") + assert "outside-static-root" not in resp.text + assert "agentevals-ui-index-sentinel" in resp.text + finally: + outside.unlink(missing_ok=True) From bc510e1bb1546259f405e2616173df75a29e1937 Mon Sep 17 00:00:00 2001 From: krisztianfekete Date: Fri, 10 Jul 2026 15:23:03 +0200 Subject: [PATCH 2/2] address review comments --- src/agentevals/api/streaming_routes.py | 9 +++++++++ src/agentevals/streaming/exports.py | 8 +++++++- tests/test_api.py | 7 ++----- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/agentevals/api/streaming_routes.py b/src/agentevals/api/streaming_routes.py index 7910533..e1803b5 100644 --- a/src/agentevals/api/streaming_routes.py +++ b/src/agentevals/api/streaming_routes.py @@ -216,6 +216,7 @@ async def evaluate_sessions( if not golden_session: raise HTTPException(status_code=404, detail="Golden session not found") + eval_set_file = None try: eval_set_response = await _do_create_eval_set( CreateEvalSetRequest( @@ -311,6 +312,14 @@ async def eval_one_session(session_id: str, session) -> tuple[SessionEvalResult, except Exception as exc: logger.exception("Failed to evaluate sessions") raise HTTPException(status_code=500, detail=str(exc)) from exc + finally: + # This eval set is consumed only by run_evaluation above and is never + # served for download, so remove it once evaluation completes. + if eval_set_file is not None: + try: + os.unlink(eval_set_file.name) + except OSError: + pass @streaming_router.post("/prepare-evaluation", response_model=StandardResponse[PrepareEvaluationData]) diff --git a/src/agentevals/streaming/exports.py b/src/agentevals/streaming/exports.py index 4902316..0dd4b27 100644 --- a/src/agentevals/streaming/exports.py +++ b/src/agentevals/streaming/exports.py @@ -10,17 +10,23 @@ from __future__ import annotations +import atexit import hashlib import hmac import secrets +import shutil import tempfile from pathlib import Path EXPORT_DIR = Path(tempfile.mkdtemp(prefix="agentevals-exports-")) # mkdtemp already restricts the dir to the owner; set it explicitly so the -# permission is guaranteed even if the creation call is ever changed. +# permission holds even if the creation call is ever changed. EXPORT_DIR.chmod(0o700) +# Best-effort removal on normal interpreter exit; an abrupt kill leaves the +# directory for the OS temp reaper. +atexit.register(shutil.rmtree, EXPORT_DIR, ignore_errors=True) + _NAME_KEY = secrets.token_bytes(32) diff --git a/tests/test_api.py b/tests/test_api.py index 8c1ea0c..8242d05 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1421,8 +1421,6 @@ def test_prepare_skips_incomplete(self, mock_create_eval): # --------------------------------------------------------------------------- -# A benign file placed outside the export directory. Its content token must -# never appear in a download response; if it does, containment leaked. _OUT_OF_SCOPE_NAME = "out_of_scope_ref.txt" _OUT_OF_SCOPE_TOKEN = "out-of-scope-marker-a1b2c3" @@ -1504,9 +1502,8 @@ def test_symlink_escape_is_blocked(self): os.unlink(sentinel) def test_containment_is_checked_before_existence(self): - # A symlink resolving outside the export dir must be indistinguishable - # from a plain missing file: same status and same body, so the endpoint - # cannot be used to probe whether out-of-scope paths exist. + # An out-of-scope path and a missing name must return byte-identical + # responses, so containment is resolved before existence is checked. sentinel = os.path.join(tempfile.gettempdir(), _OUT_OF_SCOPE_NAME) with open(sentinel, "w", encoding="utf-8") as f: f.write(_OUT_OF_SCOPE_TOKEN)