fix(grok): validate agent_id before using it as a usage-dir path segment

CodeQL flagged a high-severity py/path-injection: agent_id flowed from
request-facing call sites into _grok_usage_dir() and on to read_text(), so a
value containing '..' or a separator could traverse the filesystem. Validate
agent_id against the slug/uuid allowlist ([A-Za-z0-9_-]+) at the single
chokepoint (_grok_usage_dir feeds both the mount and the finalize read);
anything else raises. Rejects traversal; accepts every real agent slug.
This commit is contained in:
Renn F
2026-06-19 08:03:30 +02:00
parent 7713daf57f
commit a35b640d03
2 changed files with 37 additions and 3 deletions
+21 -3
View File
@@ -16,6 +16,7 @@ import asyncio
import contextlib import contextlib
import json import json
import os import os
import re
import shutil import shutil
import tempfile import tempfile
from dataclasses import dataclass from dataclasses import dataclass
@@ -877,6 +878,20 @@ class AgentOrchestrator:
img, f"{docker_dir}/{dockerfile}", build_context img, f"{docker_dir}/{dockerfile}", build_context
) )
@staticmethod
def _safe_agent_path_segment(agent_id: str) -> str:
"""Return ``agent_id`` if it is safe as a single path segment, else raise.
``agent_id`` reaches the grok usage dir from request-facing call sites, so
it must not be able to traverse the path. Allow only the slug / uuid
charset the orchestrator actually assigns (alphanumerics, hyphen,
underscore); a value with a separator, ``..``, or any other character is
rejected rather than stripped.
"""
if re.fullmatch(r"[A-Za-z0-9_-]+", agent_id):
return agent_id
raise ValueError(f"unsafe agent id for a filesystem path: {agent_id!r}")
@staticmethod @staticmethod
def _grok_usage_dir(agent_id: str) -> Path: def _grok_usage_dir(agent_id: str) -> Path:
"""Per-agent grok usage dir, branched compose-vs-local. """Per-agent grok usage dir, branched compose-vs-local.
@@ -885,11 +900,14 @@ class AgentOrchestrator:
(``_ensure_grok_usage_dir``) and the finalize read side (``_ensure_grok_usage_dir``) and the finalize read side
(``_grok_usage_json``) so they can never drift: in compose the orchestrator (``_grok_usage_json``) so they can never drift: in compose the orchestrator
sees the mounted host dir at ``GROK_USAGE_DATA_DIR``; in local mode the sees the mounted host dir at ``GROK_USAGE_DATA_DIR``; in local mode the
container's usage.json lands under the shared tempdir. container's usage.json lands under the shared tempdir. ``agent_id`` is
validated as a single safe path segment first (path-injection barrier for
both the mount and the finalize read).
""" """
safe_agent_id = AgentOrchestrator._safe_agent_path_segment(agent_id)
if PROJECT_HOST_PATH: if PROJECT_HOST_PATH:
return Path(GROK_USAGE_DATA_DIR) / agent_id return Path(GROK_USAGE_DATA_DIR) / safe_agent_id
return Path(tempfile.gettempdir()) / "roboco-grok-usage" / agent_id return Path(tempfile.gettempdir()) / "roboco-grok-usage" / safe_agent_id
def _ensure_grok_usage_dir(self, agent_id: str) -> None: def _ensure_grok_usage_dir(self, agent_id: str) -> None:
"""Pre-create the agent's grok usage dir (world-writable) before the mount. """Pre-create the agent's grok usage dir (world-writable) before the mount.
@@ -94,6 +94,22 @@ def test_grok_usage_dir_branches_compose_vs_local(
) )
@pytest.mark.parametrize(
"bad",
["..", ".", "../etc", "a/b", "a\\b", "", "be-dev-1/../x", "x\x00y"],
)
def test_grok_usage_dir_rejects_path_traversal(bad: str) -> None:
# agent_id reaches the usage dir from request-facing call sites; a value that
# could traverse the path must be rejected, not used to build a Path.
with pytest.raises(ValueError, match="unsafe agent id"):
AgentOrchestrator._grok_usage_dir(bad)
def test_safe_agent_path_segment_accepts_real_slugs() -> None:
for slug in ("be-dev-1", "pr-reviewer-1", "main-pm", "intake", "secretary"):
assert AgentOrchestrator._safe_agent_path_segment(slug) == slug
def test_grok_usage_json_reads_the_real_local_dir( def test_grok_usage_json_reads_the_real_local_dir(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None: ) -> None: