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..e1803b5 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 ( @@ -214,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( @@ -225,7 +228,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() @@ -307,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]) @@ -328,12 +341,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 +361,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 +376,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..0dd4b27 --- /dev/null +++ b/src/agentevals/streaming/exports.py @@ -0,0 +1,41 @@ +"""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 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 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) + + +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..8242d05 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 = "