review: reject path traversal + bound per-item timeout to tool deadline

Two Greptile findings on #791:

- P1 security: batch_view_files prefixed `workspace/` but didn't guard `..`, so
  `workspace/../../etc/passwd` was cat'd verbatim as `/workspace/../../etc/passwd`
  and read container files outside the target. Added `_safe_workspace_path()`:
  normalizes the path and rejects anything that resolves outside `/workspace`
  (per-item error, never handed to cat); the safe entries in the batch still read.

- P1: a full valid batch could exceed the enclosing @function_tool deadline and
  be cancelled before returning isolated results. Worst case is
  ceil(_MAX_ITEMS/_MAX_CONCURRENCY)=7 waves of the per-item timeout; the old 60s
  per item => ~420s > the 180s/300s tool deadlines. Lowered per-item timeouts
  (view 20s, exec 40s) so 7 waves stay under the tool deadline, and named all
  four constants so the invariant is explicit (+ a test asserting it holds).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

strix/tools/batch/tools.py

@@ -16,6 +16,8 @@ from __future__ import annotations
 
 import asyncio
 import logging
+import os
+from pathlib import PurePosixPath
 from typing import Any
 
 from agents import RunContextWrapper, function_tool
@@ -25,7 +27,16 @@ logger = logging.getLogger(__name__)
 
 _MAX_CONCURRENCY = 8          # bounded fan-out — don't swamp the sandbox
 _MAX_ITEMS = 50               # cap batch size (a 200-file batch is a smell)
-_DEFAULT_TIMEOUT = 60
+
+# Per-item timeouts are bounded so a full, valid batch cannot exceed the
+# enclosing @function_tool deadline (below) and get cancelled before returning
+# its per-item results. Worst case is ceil(_MAX_ITEMS / _MAX_CONCURRENCY) waves
+# of per-item timeouts: ceil(50/8) = 7. So keep 7 * per_item < tool deadline:
+#   view: 7 * 20s = 140s < 180s ;  exec: 7 * 40s = 280s < 300s.
+_VIEW_ITEM_TIMEOUT = 20
+_EXEC_ITEM_TIMEOUT = 40
+_VIEW_TOOL_TIMEOUT = 180
+_EXEC_TOOL_TIMEOUT = 300
 
 
 def _session(ctx: RunContextWrapper) -> Any:
@@ -33,6 +44,26 @@ def _session(ctx: RunContextWrapper) -> Any:
     return inner.get("sandbox_session")
 
 
+def _safe_workspace_path(path: str) -> str | None:
+    """Resolve a caller path to an absolute /workspace path, or None if it would
+    escape the workspace root.
+
+    The path is passed to ``cat`` verbatim, so a ``..`` hop (e.g.
+    ``workspace/../../etc/passwd``) would read container files outside the
+    target. Normalise and require the result to stay under ``/workspace``.
+    """
+    rel = path.lstrip("/")
+    rel = rel[len("workspace/") :] if rel.startswith("workspace/") else rel
+    # PurePosixPath: sandbox paths are always posix regardless of host OS.
+    normalized = PurePosixPath("/workspace") / rel
+    resolved = PurePosixPath(os.path.normpath(str(normalized)))
+    if resolved != PurePosixPath("/workspace") and PurePosixPath(
+        "/workspace"
+    ) not in resolved.parents:
+        return None
+    return str(resolved)
+
+
 async def _gather_bounded(coros: list[Any]) -> list[Any]:
     sem = asyncio.Semaphore(_MAX_CONCURRENCY)
 
@@ -54,9 +85,10 @@ async def _view_files_impl(session: Any, paths: list[str]) -> dict[str, Any]:
                 "error": f"batch too large ({len(paths)} > {_MAX_ITEMS}); split it"}
 
     async def _read(path: str) -> dict[str, Any]:
-        rel = path.lstrip("/")
-        target = rel if rel.startswith("workspace/") else f"workspace/{rel}"
-        result = await session.exec("cat", f"/{target}", timeout=_DEFAULT_TIMEOUT)
+        safe = _safe_workspace_path(path)
+        if safe is None:
+            return {"path": path, "error": "path escapes /workspace (rejected)"}
+        result = await session.exec("cat", safe, timeout=_VIEW_ITEM_TIMEOUT)
         if getattr(result, "exit_code", 1) != 0:
             return {"path": path, "error": (getattr(result, "stderr", "") or
                                             "read failed").strip()[:200]}
@@ -73,7 +105,7 @@ async def _view_files_impl(session: Any, paths: list[str]) -> dict[str, Any]:
     return {"success": True, "results": results}
 
 
-@function_tool(timeout=180, strict_mode=False)
+@function_tool(timeout=_VIEW_TOOL_TIMEOUT, strict_mode=False)
 async def batch_view_files(
     ctx: RunContextWrapper,
     paths: list[str],
@@ -101,7 +133,7 @@ async def _exec_impl(session: Any, commands: list[str]) -> dict[str, Any]:
                 "error": f"batch too large ({len(commands)} > {_MAX_ITEMS}); split it"}
 
     async def _run(cmd: str) -> dict[str, Any]:
-        result = await session.exec("sh", "-c", cmd, timeout=_DEFAULT_TIMEOUT)
+        result = await session.exec("sh", "-c", cmd, timeout=_EXEC_ITEM_TIMEOUT)
         return {
             "command": cmd,
             "exit_code": getattr(result, "exit_code", None),
@@ -118,7 +150,7 @@ async def _exec_impl(session: Any, commands: list[str]) -> dict[str, Any]:
     return {"success": True, "results": results}
 
 
-@function_tool(timeout=300, strict_mode=False)
+@function_tool(timeout=_EXEC_TOOL_TIMEOUT, strict_mode=False)
 async def batch_terminal_execute(
     ctx: RunContextWrapper,
     commands: list[str],

tests/tools/test_batch_tools.py

@@ -7,6 +7,7 @@ the reads/execs actually fan out concurrently (bounded).
 from __future__ import annotations
 
 import asyncio
+import math
 
 from strix.tools.batch import tools as bt
 
@@ -79,6 +80,43 @@ def test_view_files_no_session():
     assert res["success"] is False and "no sandbox session" in res["error"]
 
 
+def test_view_files_rejects_path_traversal():
+    # A '..' hop must not escape /workspace — the path is cat'd verbatim, so
+    # workspace/../../etc/passwd would read container files otherwise.
+    catted: list[str] = []
+
+    def handler(args):
+        catted.append(args[-1])
+        return _Result(stdout="x")
+
+    res = _view(_FakeSession(handler), ["../../etc/passwd", "workspace/../secret", "ok.py"])
+    # traversal entries are rejected with an error, never handed to cat
+    assert res["results"][0]["error"].startswith("path escapes")
+    assert res["results"][1]["error"].startswith("path escapes")
+    assert "content" in res["results"][2]  # the safe one still reads
+    assert all("/workspace/../" not in c and "/etc/passwd" not in c for c in catted)
+
+
+def test_view_files_normalizes_workspace_prefix():
+    # Both bare and workspace-prefixed forms resolve to the same /workspace path.
+    seen: list[str] = []
+
+    def handler(args):
+        seen.append(args[-1])
+        return _Result(stdout="x")
+
+    _view(_FakeSession(handler), ["src/a.py", "workspace/src/a.py", "/src/a.py"])
+    assert seen == ["/workspace/src/a.py"] * 3
+
+
+def test_batch_deadline_fits_tool_timeout():
+    # Worst-case wave count * per-item timeout must stay under the enclosing
+    # @function_tool deadline, or a valid batch is cancelled before returning.
+    waves = math.ceil(bt._MAX_ITEMS / bt._MAX_CONCURRENCY)
+    assert waves * bt._VIEW_ITEM_TIMEOUT < bt._VIEW_TOOL_TIMEOUT
+    assert waves * bt._EXEC_ITEM_TIMEOUT < bt._EXEC_TOOL_TIMEOUT
+
+
 def test_view_files_empty_is_ok():
     res = _view(_FakeSession(lambda _a: _Result()), [])
     assert res["success"] is True and res["results"] == []