From c414e1ccf014f4bf3bcacad23d17271ef544e0dd Mon Sep 17 00:00:00 2001 From: Kiliaan Vanvoorden Date: Tue, 13 Oct 2026 16:04:41 +0000 Subject: [PATCH] fix(server): confine state reads to the .ai-engineering directory CodeQL flagged py/path-injection where jload reads a path built from a request. Every route already limits names to NAME_RE, which has no '/', so no request can reach outside today; CodeQL cannot see a regex check in another function. The server's _jload now resolves the path (symlinks included) and refuses anything outside the state directory, as a second wall and a guard CodeQL can follow. New test covers '..' escapes and a planted symlink. Co-Authored-By: Claude Opus 5.5 --- harness/server.py | 32 +++++++++++++++++++++++--------- tests/test_server.py | 22 ++++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/harness/server.py b/harness/server.py index f8d9983..f4dead8 100644 --- a/harness/server.py +++ b/harness/server.py @@ -10,6 +10,7 @@ import contextlib import io import json +import os import re import secrets import shutil @@ -56,9 +57,18 @@ def _probe() -> dict[str, str | None]: return {t: _version_of(t) for t in TOOLS} -def _jload(p: Path) -> dict: +def _jload(p: Path, state: Path) -> dict: + """Read a state file, refusing any path that resolves outside `state`. + + Every route already limits names to NAME_RE, which has no "/", so no request + can name such a path today. This is the second wall: a future route, or a + symlink planted in the state directory, still cannot read past it.""" + root = os.path.realpath(state) + real = os.path.realpath(p) + if not real.startswith(root + os.sep): + raise HTTPException(400, "path outside the harness state directory") try: - return jload(p) + return jload(real) except SystemExit as e: raise HTTPException(409, str(e.code or "schema error")) @@ -124,6 +134,10 @@ def _agent(cls, v: str) -> str: def create_app(repo: Path, token: str, max_parallel: int = 2) -> FastAPI: # constructed once per app: the Runner owns its loop thread, so no running loop is needed here runner = Runner(repo, runs_db(repo), max_parallel=max_parallel) + state = repo / AE + + def _state_jload(p: Path) -> dict: + return _jload(p, state) @asynccontextmanager async def lifespan(_app: FastAPI): @@ -164,16 +178,16 @@ def verification(): def _jload_or_404(p: Path) -> dict: if not p.exists(): raise HTTPException(404, "not initialized: run `harness init`") - return _jload(p) + return _state_jload(p) def _task_rows() -> list[dict]: d = repo / AE / "tasks" rows = [] for p in sorted(d.glob("*.json")) if d.is_dir() else []: - c = _jload(p) + c = _state_jload(p) ep = repo / AE / "evidence" / f"{c['name']}.json" rows.append({"name": c["name"], "risk": c["risk"], "branch": c["branch"], - "verdict": _jload(ep)["verdict"] if ep.exists() else "unverified"}) + "verdict": _state_jload(ep)["verdict"] if ep.exists() else "unverified"}) return rows @app.get("/api/v1/projects") @@ -207,7 +221,7 @@ def start(body: TaskCreate): rc, out = _run_core(cmd_start, args) if rc: raise HTTPException(400, out.strip() or "task start failed") - return {"output": out, "contract": _jload(repo / AE / "tasks" / f"{body.name}.json")} + return {"output": out, "contract": _state_jload(repo / AE / "tasks" / f"{body.name}.json")} @app.post("/api/tasks/{name}/verify", status_code=202) async def verify(name: str, review: bool = True): @@ -217,11 +231,11 @@ async def verify(name: str, review: bool = True): raise HTTPException(404, f"no such task: {name}") if not review: # review is contract state; the core stays the only writer, so amend via core schema - c = _jload(cf) + c = _state_jload(cf) if c.get("review"): c["review"] = False jsave(cf, c) - c = _jload(cf) + c = _state_jload(cf) run_id = await runner.create(name, "verify", ["verify", name], c["worktree"]) return {"id": run_id, "events": f"/api/runs/{run_id}/events"} @@ -234,7 +248,7 @@ async def agent_run(name: str, body: AgentRun | None = None): agent = body.agent if body else "claude" if not shutil.which(agent): raise HTTPException(409, f"{agent} CLI not found; cannot enqueue an agent run") - c = _jload(cf) + c = _state_jload(cf) run_id = await runner.create(name, "agent", agent_argv(repo, name, agent), c["worktree"]) return {"id": run_id, "events": f"/api/runs/{run_id}/events"} diff --git a/tests/test_server.py b/tests/test_server.py index f72a67f..42dd84b 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -170,3 +170,25 @@ def test_invalid_and_unknown_names(pyrepo: Path): assert c.post("/api/tasks/nope/verify", headers=AUTH).status_code == 404 assert c.post("/api/tasks/nope/run", headers=AUTH, json={"agent": "bogus"}).status_code == 422 assert c.get("/api/policy", headers=AUTH).status_code == 404 # pyrepo not initialized + + +def test_state_reads_are_confined_to_the_state_directory(tmp_path: Path): + # Defense in depth behind NAME_RE: even a path that reached _jload by some + # future route cannot read outside .ai-engineering. + import pytest + from fastapi import HTTPException + + from harness.server import _jload + + state = tmp_path / ".ai-engineering" + (state / "tasks").mkdir(parents=True) + jsave(state / "tasks" / "ok.json", {"name": "ok"}) + (tmp_path / "secret.json").write_text('{"schema_version": 1, "token": "x"}') + assert _jload(state / "tasks" / "ok.json", state)["name"] == "ok" + for escape in (tmp_path / "secret.json", state / ".." / "secret.json", state / "tasks" / ".." / ".." / "secret.json"): + with pytest.raises(HTTPException) as info: + _jload(escape, state) + assert info.value.status_code == 400 + (state / "tasks" / "link.json").symlink_to(tmp_path / "secret.json") + with pytest.raises(HTTPException): + _jload(state / "tasks" / "link.json", state)