From 1e14b287b112a5fdc7daa06c0417c7f203557851 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Fri, 19 Jun 2026 14:09:58 +0100 Subject: [PATCH] fix(agent): inline the path-containment guard at the write/unlink sink (#1083, CodeQL) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #950 normpath+startswith guard cleared the construction site but CodeQL re-flagged the _persist/_delete sinks: it treats _pending_path's arg→return as tainted and does not propagate a CWE-022 barrier across a callee return. Inline the guard co-located with each write/replace/unlink sink (exactly how #950's guard sits with its rmtree/exists sink), and drop the now-unused _pending_path. Refs #1083 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/services/result_callback.py | 48 +++++++++---------- tests/unit/test_1083_result_callback.py | 23 ++++----- 2 files changed, 32 insertions(+), 39 deletions(-) diff --git a/docker/base-image/agent_server/services/result_callback.py b/docker/base-image/agent_server/services/result_callback.py index fc2024b70..a31109a46 100644 --- a/docker/base-image/agent_server/services/result_callback.py +++ b/docker/base-image/agent_server/services/result_callback.py @@ -75,7 +75,9 @@ def _callbacks_configured() -> bool: # malformed/hostile value never reaches the path build or the callback URL. (The # ``temp-…`` fallback id carries a ``.`` but only exists when execution_id is # absent, in which case try_spawn_async already falls back to sync.) -# Suspenders: resolve() + is_relative_to() containment in _pending_path below. +# Suspenders: the #950 normpath+startswith containment guard, co-located with the +# write/unlink sinks in _persist/_delete below (CodeQL only honours a CWE-022 +# barrier in the same function as the sink, not across a callee's return). _SAFE_EXECUTION_ID = re.compile(r"\A[A-Za-z0-9_-]{1,128}\Z") @@ -83,39 +85,37 @@ def _is_safe_execution_id(execution_id: Any) -> bool: return bool(isinstance(execution_id, str) and _SAFE_EXECUTION_ID.match(execution_id)) -def _pending_path(execution_id: str) -> Path: - """Resolve the on-disk pending-result path, guaranteeing containment. - - Path-containment guard (the #950 CWE-022 pattern CodeQL recognizes): - os.path.normpath collapses any ``..`` in the joined path, an inline - startswith prefix-check confirms the result is under _PENDING_DIR, and the - *normalized* value is flowed downstream so the path reaching write/replace/ - unlink is provably contained. Raises ValueError on escape — the best-effort - _persist/_delete callers catch and log it; the regex guard in - try_spawn_async remains the belt that rejects such ids before they get here. - """ - base = os.path.normpath(str(_PENDING_DIR)) - candidate = os.path.normpath(os.path.join(base, f"{execution_id}.json")) - if candidate != base and not candidate.startswith(base + os.sep): - raise ValueError(f"pending-result path escapes {base}: {execution_id!r}") - return Path(candidate) - - def _persist(execution_id: str, record: Dict) -> None: """Atomically write the pending record (tmp + rename) so a crash mid-write - never leaves a half-JSON file the sweep would choke on.""" + never leaves a half-JSON file the sweep would choke on. + + The #950 path-containment guard is inlined here, co-located with the + write/replace sink, so a hostile execution_id is provably confined to + _PENDING_DIR (CWE-022). Best-effort: a containment failure is swallowed. + """ try: _PENDING_DIR.mkdir(parents=True, exist_ok=True) - tmp = _pending_path(execution_id).with_suffix(".json.tmp") - tmp.write_text(json.dumps(record)) - tmp.replace(_pending_path(execution_id)) + base = os.path.normpath(str(_PENDING_DIR)) + dest = os.path.normpath(os.path.join(base, f"{execution_id}.json")) + if dest != base and not dest.startswith(base + os.sep): + raise ValueError(f"pending-result path escapes {base}: {execution_id!r}") + tmp = f"{dest}.tmp" + Path(tmp).write_text(json.dumps(record)) + Path(tmp).replace(dest) except Exception: # noqa: BLE001 — persistence is best-effort logger.debug("[#1083] could not persist pending result %s", execution_id, exc_info=True) def _delete(execution_id: str) -> None: + """Remove a pending record. The #950 containment guard is inlined here, + co-located with the unlink sink, so a hostile id can't unlink outside + _PENDING_DIR (CWE-022). Best-effort: a containment failure is a no-op.""" try: - _pending_path(execution_id).unlink(missing_ok=True) + base = os.path.normpath(str(_PENDING_DIR)) + dest = os.path.normpath(os.path.join(base, f"{execution_id}.json")) + if dest != base and not dest.startswith(base + os.sep): + return + Path(dest).unlink(missing_ok=True) except Exception: # noqa: BLE001 logger.debug("[#1083] could not delete pending result %s", execution_id, exc_info=True) diff --git a/tests/unit/test_1083_result_callback.py b/tests/unit/test_1083_result_callback.py index f704cf383..fb7d7e4cd 100644 --- a/tests/unit/test_1083_result_callback.py +++ b/tests/unit/test_1083_result_callback.py @@ -174,23 +174,16 @@ def test_persist_and_delete(self, tmp_path, monkeypatch): rc._delete("exec-9") assert not (tmp_path / "exec-9.json").exists() - def test_pending_path_rejects_traversal(self, tmp_path, monkeypatch): - # #950 containment pattern (CodeQL-recognized): normpath + startswith - # prefix-check raises on any id that escapes _PENDING_DIR. + def test_persist_delete_reject_traversal(self, tmp_path, monkeypatch): + # #950 containment (normpath + startswith), inlined co-located with the + # write/unlink sinks: a traversal id is rejected best-effort — nothing is + # written outside _PENDING_DIR and neither call raises. monkeypatch.setattr(rc, "_PENDING_DIR", tmp_path) - assert rc._pending_path("exec-ok") == (tmp_path / "exec-ok.json") - with pytest.raises(ValueError): - rc._pending_path("../../etc/passwd") - with pytest.raises(ValueError): - rc._pending_path("../escape") - - def test_persist_delete_swallow_traversal(self, tmp_path, monkeypatch): - # _persist/_delete stay best-effort: a hostile id is logged, never raised, - # and nothing is written outside the pending dir. - monkeypatch.setattr(rc, "_PENDING_DIR", tmp_path) - rc._persist("../escape", {"agent_name": "a", "envelope": {}}) # must not raise - rc._delete("../escape") # must not raise + for bad in ("../escape", "../../etc/passwd"): + rc._persist(bad, {"agent_name": "a", "envelope": {}}) # must not raise + rc._delete(bad) # must not raise assert not (tmp_path.parent / "escape.json").exists() + assert not (tmp_path / "escape.json").exists() def test_resend_delivers_and_deletes(self, tmp_path, monkeypatch): monkeypatch.setattr(rc, "_PENDING_DIR", tmp_path)