From b94a43e3fbe1b58e9fd06282e690023ce74a2203 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Wed, 26 Aug 2026 18:33:39 -0700 Subject: [PATCH 01/21] Add Pyodide sandbox code interpreter MCP server Ports the pyodide-based Code Interpreter / sandbox MCP server from an internal implementation by Susana Palmaz. The server runs agent-supplied Python inside Pyodide (CPython-in-WASM) hosted by a long-lived Node worker, exposes workspace files at /workspace/, and isolates writes with copy-on-write at the NODEFS layer. Adapted for this public repo: - Added the MIT license header required by .pre-commit-config.yaml to every ported .py file. - Added toolslib/sandbox/__init__.py so setuptools find_packages() discovers the subpackage (the original tree relied on implicit namespace packages). - Rewrote absolute doc links to repo-relative paths. - Added a [tool.setuptools.package-data] section carrying only the sandbox worker assets; no new runtime dependencies were needed. - Pinned actions/setup-node@v4 in CI rather than relying on the runner's preinstalled Node. Fixes made while porting: - download-wheels.mjs: a rejected fetch() (offline/blocked host/TLS error) escaped Promise.all and failed `npm install` outright, contradicting the documented "falls back to runtime fetch" behavior. Now caught per package. - sandbox.py list_files(): os.walk() descends from the resolved base, so relative_to(self.workspace_dir) raised ValueError whenever the workspace path was aliased (symlinked dir, macOS /tmp, Windows 8.3 short path). Now relative to the same resolved root. - sandbox.py: return "/" separated paths, as the tool contract and the /workspace/ concatenation require. - package-lock.json: npm audit fix for transitive ws (8.19.0 -> 8.21.3), clearing 2 high advisories. package.json and pyodide are unchanged. Co-authored-by: Susana Palmaz Lopez-Pelaez Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/test-thinkingbox-tools.yml | 9 + .gitignore | 3 + docs/sandbox_code_interpreter.md | 301 +++++++ servers/servers.yaml | 4 + servers/thinkingbox_tools/pyproject.toml | 11 + .../tests/test_sandbox_server.py | 756 ++++++++++++++++++ .../thinkingbox_tools/mcp_sandbox.py | 178 +++++ .../toolslib/sandbox/__init__.py | 2 + .../toolslib/sandbox/code_interpreter.py | 185 +++++ .../toolslib/sandbox/package-lock.json | 54 ++ .../toolslib/sandbox/package.json | 9 + .../toolslib/sandbox/pyodide_worker.mjs | 337 ++++++++ .../toolslib/sandbox/pypi-packages.mjs | 20 + .../toolslib/sandbox/sandbox.py | 51 ++ .../sandbox/scripts/download-wheels.mjs | 71 ++ 15 files changed, 1991 insertions(+) create mode 100644 docs/sandbox_code_interpreter.md create mode 100644 servers/thinkingbox_tools/tests/test_sandbox_server.py create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/__init__.py create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py create mode 100644 servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs diff --git a/.github/workflows/test-thinkingbox-tools.yml b/.github/workflows/test-thinkingbox-tools.yml index c33bc30..1e14c2f 100644 --- a/.github/workflows/test-thinkingbox-tools.yml +++ b/.github/workflows/test-thinkingbox-tools.yml @@ -28,6 +28,15 @@ jobs: run: | uv sync --group dev + - name: Install Node.js + uses: actions/setup-node@v4 + with: + node-version: '22' + + - name: Install pyodide worker (Node deps + vendored PyPI wheels) + working-directory: ./servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox + run: npm ci + - name: Run tests with pytest working-directory: ./servers/thinkingbox_tools run: | diff --git a/.gitignore b/.gitignore index 8432cec..c90c67d 100644 --- a/.gitignore +++ b/.gitignore @@ -203,3 +203,6 @@ support/search_sources/*/sources/ support/search_sources/*/parsed/ .memory_bank/ .claude/ + +# Node.js dependencies (pyodide worker for the sandbox/code-interpreter tool) +node_modules/ diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md new file mode 100644 index 0000000..4f2743c --- /dev/null +++ b/docs/sandbox_code_interpreter.md @@ -0,0 +1,301 @@ +# Sandbox Code Interpreter (Pyodide) + +The `sandbox` MCP server runs agent-supplied Python in a sandboxed Pyodide +(CPython-in-WebAssembly) interpreter, exposes the test's workspace files at +`/workspace/`, and isolates writes via copy-on-write at the NODEFS layer. + +--- + +## Architecture + +``` +┌──────────────────────────────────────────────┐ +│ ThinkingBox Agent Loop │ +└────────────────────┬─────────────────────────┘ + │ stdio MCP (one per session) + v +┌──────────────────────────────────────────────┐ +│ mcp_sandbox.py (Python, FastMCP) │ +│ - per-session init/teardown │ +│ - owns Sandbox + CodeInterpreter │ +└────────────────────┬─────────────────────────┘ + │ asyncio subprocess + │ stdin/stdout: newline-delimited JSON + │ stderr: inherited (diagnostic logs) + v +┌──────────────────────────────────────────────┐ +│ pyodide_worker.mjs (Node.js, long-lived)│ +│ - loadPyodide() — CPython compiled to WASM│ +│ - eager loadPackage / micropip.install │ +│ - NODEFS mount + COW patches │ +│ - _execute(code) request loop │ +└────────────────────┬─────────────────────────┘ + │ NODEFS (Emscripten ↔ host fs) + v +┌──────────────────────────────────────────────┐ +│ /tmp/sandbox_session_XXXXXX/ │ +│ symlinks → configured workspace_dir │ +└──────────────────────────────────────────────┘ +``` + +Each session gets its own Node subprocess and its own `/tmp/sandbox_session_*` +directory. The Pyodide global namespace lives inside that subprocess, so +variables and imports never leak between sessions. + +--- + +## Why Pyodide + +Requirements for the sandbox: + +- Run untrusted, agent-generated Python safely. +- Deterministic behavior across machines. +- Minimized per-call cold-start cost. + +The WASM runtime is the trust boundary: user code cannot reach the host +filesystem, network, or processes except through the FS bridges we +explicitly expose. + +Compared with seccomp'd subprocesses or per-session containers, Pyodide +gives us: + +- A **pinned package set** via the pyodide lock plus vendored wheels. +- A **single dependency** to install (Node plus the `pyodide` npm package). +- **REPL-style state** via a shared `_namespace` across calls in a session. + +Trade-off: ~5–10 s first start, paid once and amortized across all +`code_interpreter` calls in the session. + +--- + +## Components + +| File | Role | +| ---- | ---- | +| [mcp_sandbox.py](../servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py) | FastMCP server: tool definitions, session lifecycle, owns the singletons. | +| [toolslib/sandbox/code_interpreter.py](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py) | Spawns and supervises the Node subprocess; frames requests as JSON; enforces per-call timeouts; records an `effects` log. | +| [toolslib/sandbox/sandbox.py](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py) | Host-side helpers for `list_sandbox_files` / `search_sandbox_files`, with lexical path-traversal rejection. | +| [toolslib/sandbox/pyodide_worker.mjs](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs) | The long-lived Node worker. Loads Pyodide, installs packages, mounts the workspace with COW, runs the request loop. | +| [toolslib/sandbox/pypi-packages.mjs](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs) | Source of truth for PyPI packages (not in pyodide's lock) preinstalled in every session. | +| [toolslib/sandbox/scripts/download-wheels.mjs](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs) | `npm postinstall`: vendors pure-Python wheels into `./wheels/` so the worker installs from `file://` URLs. | + +--- + +## Tools + +| Tool | Description | +| ---- | ----------- | +| `code_interpreter(code)` | Execute Python in the session's Pyodide interpreter. Returns `stdout`, `stderr`, `result` (repr of the last bare expression, Jupyter-style), and `error` (formatted traceback). Stateful — variables and imports persist across calls. | +| `list_sandbox_files(prefix)` | List files under `workspace_dir/` as workspace-relative paths. Dotfiles filtered out. | +| `search_sandbox_files(pattern)` | Glob the workspace. Matches containing `..` segments are rejected. | +| `__reserved__init(config)` | Per-session setup. `config = {"workspace_dir": str, "timeout": float}`. Idempotent — re-calling resets the interpreter and session directory. | +| `__reserved__teardown()` | Stop the worker, remove the session directory. | +| `__reserved__geteffects()` | Return the chronological list of `{type, code, result}` entries recorded during the session. Used by graders/replay. | + +Workspace files are addressable from user code as `/workspace/`. +The list tools' descriptions instruct the agent to prepend `/workspace/` +before calling `open(...)`. + +--- + +## Python libraries + +Pre-installed in every session (eager-loaded at worker startup): + +| Category | Libraries | +| -------- | --------- | +| Data / math | `numpy`, `pandas`, `sympy`, `mpmath` | +| Excel | `openpyxl`, `xlsxwriter` | +| Word | `python-docx`, `mammoth` | +| PowerPoint | `python-pptx` | +| PDF | `pypdf`, `pdfminer.six`, `reportlab` | +| HTML / XML / Markdown | `beautifulsoup4`, `lxml`, `markdownify` | +| Imaging | `pillow` | +| Plotting | `altair`, `plotly` | +| Templating | `jinja2` | +| Tabular output | `tabulate` | +| Runtime install | `micropip` | + +Plus the CPython standard library. + +Other pyodide-lock packages (e.g. `matplotlib`, `scipy`) are auto-loaded +on first `import` — they don't need to be in the eager set, they just +pay their load cost the first time the agent imports them. + +Anything outside both sets the agent installs at runtime via +`await micropip.install("name")` — pure-Python only. + +--- + +## Protocol + +Newline-delimited JSON on stdin/stdout, strict request/response sequencing. + +``` +request: { "code": "" } +response: { "stdout": "...", "stderr": "...", + "result": "" | null, + "error": "" | null } +``` + +The worker writes a one-shot `{"ready": true}` line once Pyodide has loaded +and packages are installed. `code_interpreter.py` blocks on this for up to +`STARTUP_TIMEOUT = 300s`. + +**Stream discipline.** Stdout is reserved for protocol frames. The worker +overrides `console.log/info/warn` and passes stdout/stderr callbacks to +`loadPyodide` so Pyodide and micropip progress chatter goes to stderr. +Stderr is inherited from the parent (`stderr=None`). Piping it without a +drain task would deadlock once the ~64 KB pipe buffer fills during package +loading. User-code stdout/stderr is captured separately inside `_execute` +and returned in the response frame. + +**Timeouts.** `execute()` awaits the response with `asyncio.wait_for`. On +timeout, the worker is killed (WASM is single-threaded with no external +interrupt) and the next call respawns a fresh worker, losing session +state. The returned error says so explicitly. Worker crashes surface as +"closed unexpectedly" and recover the same way. + +--- + +## The `_execute` helper + +[pyodide_worker.mjs:110–152](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs#L110-L152) +installs a Python helper that every request calls. Two non-obvious bits: + +- It `ast.parse`s the code and, if the last node is a bare `Expr`, splits + it off — `exec`s the rest, then `eval`s the trailing expression so its + `repr` lands in `result`. Jupyter "last-expression" behavior. +- Everything runs in a module-level `_namespace` dict that lives for the + worker's lifetime, which is how state persists across `code_interpreter` + calls. + +Before each call, the worker also runs `pyodide.loadPackagesFromImports(code)`, +which auto-loads any pyodide-lock package the user references (e.g. +`matplotlib`, `scipy`) without needing it in the eager preload set. + +--- + +## Package provisioning + +The "Python libraries" section above lists what's available. This +section explains how each library gets there. Three populations, loaded +in order: + +1. **Bundled** (`BUNDLED_PACKAGES` in `pyodide_worker.mjs`) — + pyodide-lock packages loaded eagerly at startup via + `pyodide.loadPackage`. Often include C extensions compiled to WASM. + +2. **PyPI** (`PYPI_PACKAGES` in `pypi-packages.mjs`) — pure-Python + wheels not in pyodide's lock, installed via `micropip.install`. At + `npm install` time, `download-wheels.mjs` vendors the pure-Python + wheel for each into `./wheels/`; the worker rewrites each spec to a + local `file://` URL when a vendored wheel matches, else falls back + to the bare name (micropip fetches from PyPI). + +3. **Auto-loaded by import** — `loadPackagesFromImports` runs before + every user request and pulls in any other pyodide-lock package + referenced by the code. + +To add a new library: drop it into `BUNDLED_PACKAGES` if pyodide ships +a wheel for it, otherwise into `PYPI_PACKAGES` if a pure-Python wheel +exists on PyPI. Anything else has to be installed by the agent itself +via `await micropip.install(...)`. + +--- + +## Workspace and copy-on-write + +The most non-obvious piece. Requirements: + +- The agent can read every file under the configured `workspace_dir`. +- The agent's writes never mutate the source workspace. +- Init cost is independent of file size — a 1 GB CSV shouldn't take + 30 seconds to start a session. + +### Host-side symlink seeding + +`__reserved__init` populates `sandbox_session_*/` with `shutil.copytree` +using a custom `copy_function` that calls `os.symlink(abspath(src), dst)`. +The session directory mirrors `workspace_dir` structurally, but every +leaf is a symlink to the original. Cost is O(files), not O(bytes). Falls +back to `shutil.copy2` where symlinks aren't supported. + +A session directory rather than mounting `workspace_dir` directly because +writes from one session must not affect the workspace or the next session. +The session directory is what gets mutated below, and is `rmtree`'d on +teardown. + +### NODEFS copy-on-write inside Pyodide + +NODEFS is Pyodide's bridge to Node's `fs` module. By default, when +Emscripten's path resolver encounters a symlink, it calls `FS.readlink` +to follow it — which tries to open the absolute target from *inside* the +Emscripten filesystem and fails with `ENOENT`. The worker patches four +NODEFS hooks ([pyodide_worker.mjs:173–254](../servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs#L173-L254)) +to make seeded symlinks behave like regular files inside the sandbox +while copy-on-writing host-side when the agent mutates them: + +| Hook | Behavior | +| ---- | -------- | +| `node_ops.lookup` | Rewrites a symlink node's `mode` to the target's file-type bits, so the resolver treats the entry as a regular file (no `readlink` follow). | +| `node_ops.getattr` | Reports the target's size, mode, and timestamps — so `os.stat("/workspace/big.csv").st_size` returns the real byte count. | +| `stream_ops.open` | On `O_WRONLY` / `O_RDWR`, materializes the symlink: `readFileSync` the source bytes (host follows the symlink), `unlink`, write the bytes back to the same path. The symlink is now a regular file in the session dir; subsequent writes go there. `O_TRUNC` skips the read. | +| `node_ops.setattr` | Same materialization for `os.truncate` / `os.utime` / `os.chmod`, which don't go through `open`. | + +This catches every Python write path — `builtins.open`, `os.open`, +`io.FileIO`, `mmap`, `sqlite3`, `numpy.save` — because they all bottom +out in NODEFS. Reads never materialize: Node's `fs` follows host +symlinks automatically. + +`node_ops.symlink` is overridden to raise `EPERM`, blocking +`os.symlink("/etc/passwd", "/workspace/x")` attacks. Seeded symlinks +were created on the host before the mount and are unaffected. + +### Path traversal in the listing tools + +The session directory is full of symlinks, so `Path.resolve()`-based +checks would land outside `workspace_dir` for every legitimate match. +`Sandbox.search_files` checks the **lexical** path (`p.parts`) and rejects +any match containing `..`. `list_files` uses `os.walk` from a +`resolve`-checked base and never accepts user paths that resolve outside +the session root. + +--- + +## Effects log + +Every successful execution is appended to `_interpreter.effects` as +`{"type": "code_execution", "code": code, "result": asdict(result)}` and +returned by `__reserved__geteffects`. Graders use this for replay and +intermediate-step scoring; storing `result` as a plain dict (not the +dataclass) keeps the JSON shape stable across versions. + +--- + +## Setup and operations + +```bash +cd servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox +npm install +``` + +The `postinstall` script vendors wheels into `./wheels/`. Idempotent — +re-running keeps existing wheels; force a refresh by deleting `wheels/`. +If PyPI is unreachable, missing wheels fall back to runtime fetch +(slower startup, still works). + +**Cost.** Cold start ~5–10 s with cached wheels, paid once per session. +Memory ~250–400 MB resident per worker — each concurrent session needs +its own. Per-call overhead is low tens of ms for pure-Python code with +no new imports; dominated by `loadPackagesFromImports` otherwise. + +**Upgrades.** The NODEFS hooks reach into `pyodide.FS.filesystems.NODEFS` +internals and assume the shape of `node_ops` / `stream_ops` on the +current pyodide version. If a `pyodide` upgrade silently breaks them, +reads still work (Node follows symlinks transparently) but **writes leak +to the source workspace**. The integration tests in +[test_sandbox_server.py](../servers/thinkingbox_tools/tests/test_sandbox_server.py) +exercise the COW write path (open / `os.open` / sqlite3 / truncate / +utime) and the path-traversal guards — they're the most reliable canary +for a pyodide bump. diff --git a/servers/servers.yaml b/servers/servers.yaml index 7c29ed5..d674621 100644 --- a/servers/servers.yaml +++ b/servers/servers.yaml @@ -13,6 +13,10 @@ servers: online_banking: type: mcp-process command: ["{python}", "-m", "thinkingbox_tools.mcp_online_banking"] + # Sandbox code interpreter (Pyodide-based) + sandbox: + type: mcp-process + command: ["{python}", "-m", "thinkingbox_tools.mcp_sandbox"] # TB BUSINESS OPS SERVERS 202606 sandbox_external_retail: diff --git a/servers/thinkingbox_tools/pyproject.toml b/servers/thinkingbox_tools/pyproject.toml index 343f876..29abea0 100644 --- a/servers/thinkingbox_tools/pyproject.toml +++ b/servers/thinkingbox_tools/pyproject.toml @@ -30,6 +30,17 @@ py-modules = [] where = ["."] include = ["thinkingbox_tools*"] +[tool.setuptools.package-data] +thinkingbox_tools = [ + # Sandbox worker assets — required at runtime by mcp_sandbox. node_modules/ + # and wheels/ are populated by `npm install` post-install and are NOT + # packaged; see docs/sandbox_code_interpreter.md. + "toolslib/sandbox/*.mjs", + "toolslib/sandbox/package.json", + "toolslib/sandbox/package-lock.json", + "toolslib/sandbox/scripts/*.mjs", +] + [tool.pytest.ini_options] markers = [ "typesense: tests that require a running typesense server", diff --git a/servers/thinkingbox_tools/tests/test_sandbox_server.py b/servers/thinkingbox_tools/tests/test_sandbox_server.py new file mode 100644 index 0000000..6630895 --- /dev/null +++ b/servers/thinkingbox_tools/tests/test_sandbox_server.py @@ -0,0 +1,756 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Tests for the sandbox MCP server — filesystem tools and code interpreter workspace access.""" + +import asyncio + +import pytest +import pytest_asyncio +from fastmcp import Client + +from thinkingbox_tools import mcp_sandbox + +# --------------------------------------------------------------------------- +# Fixture +# --------------------------------------------------------------------------- +# Each test gets a fresh session: __reserved__init copies tmp_path into a new +# temp directory, wiring both the filesystem tools and the Pyodide worker to +# that isolated copy. __reserved__teardown removes it. The Pyodide worker +# starts lazily on the first code_interpreter call, so filesystem-only tests +# pay no startup cost. +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def sandbox_client(tmp_path): + """Provide a sandbox session backed by a temporary workspace with known test files.""" + (tmp_path / "readme.txt").write_text("Hello, sandbox!") + (tmp_path / "data.csv").write_text("name,value\nalice,1\nbob,2\n") + (tmp_path / "report.pdf").write_bytes(b"%PDF-1.4 fake pdf content") + subdir = tmp_path / "subdir" + subdir.mkdir() + (subdir / "notes.txt").write_text("notes inside subdir") + + original_sandbox = mcp_sandbox._sandbox + original_interp = mcp_sandbox._interpreter + original_session_dir = mcp_sandbox._session_dir + + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(tmp_path)}} + ) + yield client + await client.call_tool("__reserved__teardown", {}) + finally: + mcp_sandbox._sandbox = original_sandbox + mcp_sandbox._interpreter = original_interp + mcp_sandbox._session_dir = original_session_dir + + +# --------------------------------------------------------------------------- +# Tool discovery +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_tools_list(sandbox_client): + tools = await sandbox_client.list_tools() + names = [t.name for t in tools] + assert "list_sandbox_files" in names + assert "search_sandbox_files" in names + assert "code_interpreter" in names + + +# --------------------------------------------------------------------------- +# list_files +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_list_files_all(sandbox_client): + result = await sandbox_client.call_tool("list_sandbox_files", {"prefix": ""}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + files = sc["files"] + assert "readme.txt" in files + assert "data.csv" in files + assert "report.pdf" in files + assert "subdir/notes.txt" in files + + +@pytest.mark.asyncio +async def test_list_files_prefix(sandbox_client): + result = await sandbox_client.call_tool("list_sandbox_files", {"prefix": "subdir"}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == ["subdir/notes.txt"] + + +@pytest.mark.asyncio +async def test_list_files_nonexistent_prefix(sandbox_client): + result = await sandbox_client.call_tool( + "list_sandbox_files", {"prefix": "no_such_dir"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == [] + + +@pytest.mark.asyncio +async def test_list_files_rejects_parent_traversal(sandbox_client): + """Prefixes that resolve outside the workspace must not leak host paths.""" + for prefix in ("..", "../", "../..", "subdir/../.."): + result = await sandbox_client.call_tool( + "list_sandbox_files", {"prefix": prefix} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == [], f"prefix {prefix!r} leaked files: {sc['files']}" + + +# --------------------------------------------------------------------------- +# search_files +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_search_files_by_extension(sandbox_client): + result = await sandbox_client.call_tool( + "search_sandbox_files", {"pattern": "*.csv"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == ["data.csv"] + + +@pytest.mark.asyncio +async def test_search_files_recursive(sandbox_client): + result = await sandbox_client.call_tool( + "search_sandbox_files", {"pattern": "**/*.txt"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert "readme.txt" in sc["files"] + assert "subdir/notes.txt" in sc["files"] + + +@pytest.mark.asyncio +async def test_search_files_no_match(sandbox_client): + result = await sandbox_client.call_tool( + "search_sandbox_files", {"pattern": "*.xyz"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == [] + + +@pytest.mark.asyncio +async def test_search_files_rejects_parent_traversal(sandbox_client): + """Glob patterns that walk outside the workspace must not leak host paths.""" + for pattern in ("../*", "../**/*", "../../*"): + result = await sandbox_client.call_tool( + "search_sandbox_files", {"pattern": pattern} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["files"] == [], f"pattern {pattern!r} leaked files: {sc['files']}" + + +# --------------------------------------------------------------------------- +# code_interpreter — workspace file access +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_code_interpreter_reads_text_file(sandbox_client): + """Code can open and read a plain text file from /workspace/.""" + code = "open('/workspace/readme.txt').read()" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok", sc.get("error") or sc + assert sc["error"] is None + assert sc["result"] == repr("Hello, sandbox!") + + +@pytest.mark.asyncio +async def test_code_interpreter_reads_csv_with_pandas(sandbox_client): + """Code can load a CSV from /workspace/ into a pandas DataFrame.""" + code = ( + "import pandas as pd\n" + "df = pd.read_csv('/workspace/data.csv')\n" + "list(df['name'])" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is None + assert "alice" in sc["result"] + assert "bob" in sc["result"] + + +@pytest.mark.asyncio +async def test_code_interpreter_reads_subdir_file(sandbox_client): + """Code can access files in subdirectories under /workspace/.""" + code = "open('/workspace/subdir/notes.txt').read()" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is None + assert sc["result"] == repr("notes inside subdir") + + +# --------------------------------------------------------------------------- +# Isolation — writes go to the session copy, not the original workspace +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_code_interpreter_can_write_file(sandbox_client): + """Code can write a new file into /workspace/ (the session copy).""" + code = "open('/workspace/output.txt', 'w').write('written by agent')" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is None + + +@pytest.mark.asyncio +async def test_written_file_visible_to_list_files(sandbox_client): + """A file written via code_interpreter is visible to list_files in the same session.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/output.txt', 'w').write('hello')"}, + ) + result = await sandbox_client.call_tool("list_sandbox_files", {"prefix": ""}) + sc = result.structured_content["result"] + assert "output.txt" in sc["files"] + + +@pytest.mark.asyncio +async def test_original_workspace_not_modified(sandbox_client, tmp_path): + """Writing a new file via code_interpreter does not modify the original workspace_dir.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/injected.txt', 'w').write('should not exist')"}, + ) + assert not (tmp_path / "injected.txt").exists() + + +# --------------------------------------------------------------------------- +# Copy-on-write — overwriting existing files is isolated +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_overwrite_existing_file_isolated(sandbox_client, tmp_path): + """Overwriting an existing workspace file does not affect the original inode.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/readme.txt', 'w').write('overwritten')"}, + ) + assert (tmp_path / "readme.txt").read_text() == "Hello, sandbox!" + result = await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/readme.txt', 'r').read()"}, + ) + sc = result.structured_content["result"] + assert sc["result"] == repr("overwritten") + + +@pytest.mark.asyncio +async def test_overwrite_visible_within_session(sandbox_client): + """After overwriting, the updated content is readable in the same session.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/readme.txt', 'w').write('new content')"}, + ) + result = await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/readme.txt').read()"}, + ) + sc = result.structured_content["result"] + assert sc["error"] is None + assert sc["result"] == repr("new content") + + +@pytest.mark.asyncio +async def test_append_existing_file_isolated(sandbox_client, tmp_path): + """Appending to an existing workspace file does not affect the original.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/readme.txt', 'a').write(' appended')"}, + ) + assert (tmp_path / "readme.txt").read_text() == "Hello, sandbox!" + + +@pytest.mark.asyncio +async def test_os_open_truncate_without_o_creat(sandbox_client, tmp_path): + """`os.open(path, O_WRONLY | O_TRUNC)` (no O_CREAT) succeeds on a seeded + workspace file and does not touch the source. + + Regression: materializeSymlink used to unlink the symlink without + recreating the host file in the truncate path, so this call landed on a + missing file and raised ENOENT. + """ + code = ( + "import os\n" + "fd = os.open('/workspace/readme.txt', os.O_WRONLY | os.O_TRUNC)\n" + "try:\n" + " os.write(fd, b'truncated')\n" + "finally:\n" + " os.close(fd)\n" + "open('/workspace/readme.txt').read()" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["error"] is None, sc["error"] + assert sc["result"] == repr("truncated") + assert (tmp_path / "readme.txt").read_text() == "Hello, sandbox!" + + +# --------------------------------------------------------------------------- +# code_interpreter — stdout / stderr capture +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_execute_stdout(sandbox_client): + result = await sandbox_client.call_tool( + "code_interpreter", {"code": "print('hello world')"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["stdout"] == "hello world\n" + assert sc["stderr"] == "" + assert sc["error"] is None + + +@pytest.mark.asyncio +async def test_execute_stderr(sandbox_client): + code = "import sys; sys.stderr.write('err msg')" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["stderr"] == "err msg" + assert sc["error"] is None + + +# --------------------------------------------------------------------------- +# code_interpreter — expression result capture (Jupyter-style last-expression) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_execute_expression_result(sandbox_client): + result = await sandbox_client.call_tool("code_interpreter", {"code": "1 + 1"}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["result"] == "2" + assert sc["stdout"] == "" + + +@pytest.mark.asyncio +async def test_execute_statement_no_result(sandbox_client): + """Assignments are statements — result should be None.""" + result = await sandbox_client.call_tool( + "code_interpreter", {"code": "_stmt_var = 42"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["result"] is None + + +@pytest.mark.asyncio +async def test_execute_stdout_and_expression(sandbox_client): + """Print followed by a trailing expression — both captured.""" + code = "print('hi')\n2 + 2" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["stdout"] == "hi\n" + assert sc["result"] == "4" + + +# --------------------------------------------------------------------------- +# code_interpreter — error handling stays in 'error' field, tool doesn't raise +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_execute_runtime_error(sandbox_client): + result = await sandbox_client.call_tool("code_interpreter", {"code": "1 / 0"}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is not None + assert "ZeroDivisionError" in sc["error"] + assert sc["result"] is None + + +@pytest.mark.asyncio +async def test_execute_name_error(sandbox_client): + result = await sandbox_client.call_tool( + "code_interpreter", {"code": "_undefined_xyz"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is not None + assert "NameError" in sc["error"] + + +@pytest.mark.asyncio +async def test_execute_syntax_error(sandbox_client): + result = await sandbox_client.call_tool("code_interpreter", {"code": "def f(:"}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is not None + assert "SyntaxError" in sc["error"] + + +# --------------------------------------------------------------------------- +# code_interpreter — multiline code +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_execute_multiline(sandbox_client): + code = "def _add(a, b):\n return a + b\n_add(3, 4)" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["result"] == "7" + assert sc["error"] is None + + +# --------------------------------------------------------------------------- +# code_interpreter — state persistence (REPL semantics across calls) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_state_persistence(sandbox_client): + """Variables defined in one call are visible in subsequent calls.""" + await sandbox_client.call_tool("code_interpreter", {"code": "_persist_x = 100"}) + result = await sandbox_client.call_tool( + "code_interpreter", {"code": "_persist_x * 2"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["result"] == "200" + + +@pytest.mark.asyncio +async def test_import_persists(sandbox_client): + """Imports made in one call are available in subsequent calls.""" + await sandbox_client.call_tool("code_interpreter", {"code": "import math as _math"}) + result = await sandbox_client.call_tool( + "code_interpreter", {"code": "_math.floor(2.9)"} + ) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["result"] == "2" + + +# --------------------------------------------------------------------------- +# code_interpreter — pre-installed packages +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_numpy_and_pandas_available(sandbox_client): + """numpy and pandas are pre-installed and usable.""" + code = ( + "import numpy as np, pandas as pd\n" + "s = pd.Series(np.array([1, 2, 3]))\n" + "int(s.sum())" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is None + assert sc["result"] == "6" + + +# Distribution name on PyPI / pyodide-lock → top-level module name. +# Kept aligned with BUNDLED_PACKAGES + PYPI_PACKAGES in pyodide_worker.mjs. +# Excluded from preload (and this test) because their transitive deps require +# native binaries that pyodide does not provide: +# - markitdown -> magika -> onnxruntime +# - pdfplumber -> pypdfium2 +PREINSTALLED_LIBRARIES = [ + # Bundled in pyodide-lock.json (loaded via pyodide.loadPackage). + ("numpy", "numpy"), + ("pandas", "pandas"), + ("beautifulsoup4", "bs4"), + ("Jinja2", "jinja2"), + ("sympy", "sympy"), + ("altair", "altair"), + ("mpmath", "mpmath"), + ("lxml", "lxml"), + ("Pillow", "PIL"), + # Installed from PyPI via micropip at worker startup. + ("openpyxl", "openpyxl"), + ("xlsxwriter", "xlsxwriter"), + ("markdownify", "markdownify"), + ("mammoth", "mammoth"), + ("pypdf", "pypdf"), + ("pdfminer.six", "pdfminer"), + ("tabulate", "tabulate"), + ("plotly", "plotly"), + ("python-docx", "docx"), + ("python-pptx", "pptx"), + ("reportlab", "reportlab"), +] + + +@pytest.mark.asyncio +async def test_preinstalled_libraries_importable(sandbox_client): + """Every library preloaded by pyodide_worker.mjs imports cleanly in user code. + + Bundled packages (numpy, pandas, ...) load from pyodide's local wheel set + inside node_modules/pyodide/. PyPI-only packages (openpyxl, mammoth, ...) + install from file:// URLs pointing at wheels/, which are pre-downloaded by + the `npm install` postinstall hook (scripts/download-wheels.mjs). If a + wheel is missing locally, micropip falls back to PyPI at worker startup. + """ + import_lines = "\n".join(f"import {mod}" for _, mod in PREINSTALLED_LIBRARIES) + name_tuple = ( + "(" + ", ".join(f"{mod}.__name__" for _, mod in PREINSTALLED_LIBRARIES) + ",)" + ) + code = f"{import_lines}\n{name_tuple}" + + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert sc["error"] is None, sc["error"] + for _, mod in PREINSTALLED_LIBRARIES: + assert f"'{mod}'" in sc["result"], f"missing module {mod} in {sc['result']}" + + +@pytest.mark.asyncio +async def test_pyodide_lazy_autoload_on_import(sandbox_client): + """A pyodide-bundled package not in PREINSTALLED_LIBRARIES auto-loads on first import. + + Verifies the pyodide.loadPackagesFromImports() call wired into the worker's + request loop — without it, this import would raise ModuleNotFoundError. + """ + code = "import scipy; scipy.__name__" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok", sc.get("error") or sc + assert sc["error"] is None, sc["error"] + assert "'scipy'" in sc["result"] + + +@pytest.mark.asyncio +async def test_concurrent_execute_calls_are_serialized(sandbox_client): + """Two concurrent execute() coroutines must not collide on the worker stdio. + + The Node worker speaks a strict request/response protocol on a single + stdin/stdout pair. Without serialization, asyncio.gather()'d calls + would interleave writes and race on `StreamReader.readline()` — + asyncio raises `RuntimeError: readline() called while another + coroutine is already waiting for incoming data` on the second + concurrent reader. Bypass FastMCP and hit the interpreter directly + so the race is actually reachable (FastMCP may serialize at its own + transport layer). + """ + # Warm up the worker so the race is on execute(), not _ensure_started(). + await sandbox_client.call_tool("code_interpreter", {"code": "1"}) + interpreter = mcp_sandbox._interpreter + assert interpreter is not None + + code_a = "'A' * 5000" + code_b = "'B' * 5000" + a, b = await asyncio.gather( + interpreter.execute(code_a), + interpreter.execute(code_b), + ) + + assert a.error is None, a.error + assert b.error is None, b.error + assert a.result == repr("A" * 5000) + assert b.result == repr("B" * 5000) + + +@pytest.mark.asyncio +async def test_top_level_await_micropip_install(sandbox_client): + """Agent-supplied `await micropip.install(...)` should succeed. + + The documentation and the `code_interpreter` MCP tool description tell + agents to add new packages via `await micropip.install("name")`. The + worker must therefore accept top-level await in user code, which a plain + `exec(compile(...))` does not — `PyCF_ALLOW_TOP_LEVEL_AWAIT` (0x2000) is + required, and any returned coroutine must be awaited. + """ + code = "import micropip\nawait micropip.install('cowsay')\nimport cowsay\ncowsay.__name__" + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok", sc.get("error") or sc + assert sc["error"] is None, sc["error"] + assert "'cowsay'" in sc["result"] + + +# --------------------------------------------------------------------------- +# Effects tracking — calls __reserved__init so placed last +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_effects_tracking(sandbox_client): + """Effects list records every execution with its outputs.""" + await sandbox_client.call_tool("__reserved__init", {"config": {}}) + + await sandbox_client.call_tool("code_interpreter", {"code": "print('tracked')"}) + + effects_result = await sandbox_client.call_tool("__reserved__geteffects", {}) + ec = effects_result.structured_content + assert len(ec["effects"]) == 1 + effect = ec["effects"][0] + assert effect["type"] == "code_execution" + assert effect["code"] == "print('tracked')" + assert effect["result"]["stdout"] == "tracked\n" + assert effect["result"]["error"] is None + + +@pytest.mark.asyncio +async def test_effects_reset_on_init(sandbox_client): + """__reserved__init clears the effects list.""" + await sandbox_client.call_tool("code_interpreter", {"code": "1 + 1"}) + await sandbox_client.call_tool("__reserved__init", {"config": {}}) + + effects_result = await sandbox_client.call_tool("__reserved__geteffects", {}) + ec = effects_result.structured_content + assert ec["effects"] == [] + + +# --------------------------------------------------------------------------- +# Workspace write boundary — writes must not escape to the host filesystem +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_write_to_absolute_host_path_does_not_affect_host( + sandbox_client, tmp_path_factory +): + """Writing to an absolute host path outside /workspace does not create a host file.""" + outside_dir = tmp_path_factory.mktemp("outside_abs") + target = outside_dir / "escape.txt" + code = f"open({str(target)!r}, 'w').write('escaped')" + await sandbox_client.call_tool("code_interpreter", {"code": code}) + assert not target.exists() + + +@pytest.mark.asyncio +async def test_path_traversal_does_not_escape_to_host(sandbox_client, tmp_path): + """`/workspace/../foo.txt` does not escape to the host filesystem.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "open('/workspace/../traversal_escape.txt', 'w').write('escaped')"}, + ) + assert not (tmp_path / "traversal_escape.txt").exists() + assert not (tmp_path.parent / "traversal_escape.txt").exists() + + +@pytest.mark.asyncio +async def test_relative_path_traversal_does_not_escape_to_host( + sandbox_client, tmp_path +): + """`../foo.txt` written from inside /workspace does not escape to the host.""" + code = ( + "import os\n" + "os.chdir('/workspace')\n" + "open('../relative_escape.txt', 'w').write('escaped')\n" + ) + await sandbox_client.call_tool("code_interpreter", {"code": code}) + assert not (tmp_path / "relative_escape.txt").exists() + assert not (tmp_path.parent / "relative_escape.txt").exists() + + +@pytest.mark.asyncio +async def test_unlink_in_workspace_does_not_affect_source(sandbox_client, tmp_path): + """Deleting a file under /workspace does not delete the original source file.""" + await sandbox_client.call_tool( + "code_interpreter", + {"code": "import os; os.unlink('/workspace/readme.txt')"}, + ) + assert (tmp_path / "readme.txt").exists() + assert (tmp_path / "readme.txt").read_text() == "Hello, sandbox!" + + +@pytest.mark.asyncio +async def test_host_files_outside_workspace_not_readable( + sandbox_client, tmp_path_factory +): + """Files outside /workspace on the host are not readable via absolute paths.""" + outside_dir = tmp_path_factory.mktemp("outside_read") + secret = outside_dir / "secret.txt" + secret.write_text("HOST_SECRET_MARKER") + code = ( + "try:\n" + f" _c = open({str(secret)!r}).read()\n" + "except (FileNotFoundError, OSError):\n" + " _c = ''\n" + "_c" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["status"] == "ok" + assert "HOST_SECRET_MARKER" not in (sc["result"] or "") + + +@pytest.mark.asyncio +async def test_symlink_to_existing_target_does_not_escape( + sandbox_client, tmp_path_factory +): + """User code cannot create a symlink under /workspace pointing to an + existing host file and use it to overwrite that host file. + + Both halves matter: + - NODEFS.node_ops.symlink must raise (protection actually fires). + - The host file must remain untouched. + Asserting only the second would also pass for any unrelated failure in + user code, which would silently hide a broken protection. + """ + outside_dir = tmp_path_factory.mktemp("outside_symlink_existing") + target = outside_dir / "victim.txt" + target.write_text("ORIGINAL") + + code = ( + "import os\n" + f"os.symlink({str(target)!r}, '/workspace/link')\n" + f"open('/workspace/link', 'w').write('overwritten')\n" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["error"] is not None, sc + assert "PermissionError" in sc["error"], sc["error"] + assert target.read_text() == "ORIGINAL" + + +@pytest.mark.asyncio +async def test_symlink_to_nonexistent_target_does_not_escape( + sandbox_client, tmp_path_factory +): + """User code cannot create a symlink under /workspace pointing to a + nonexistent host path and use it to materialize a file at that path. + + Both halves matter: + - NODEFS.node_ops.symlink must raise (protection actually fires). + - The host path must not be created. + """ + outside_dir = tmp_path_factory.mktemp("outside_symlink_dangling") + target = outside_dir / "should_not_be_created.txt" + assert not target.exists() + + code = ( + "import os\n" + f"os.symlink({str(target)!r}, '/workspace/dangling')\n" + f"open('/workspace/dangling', 'w').write('escaped')\n" + ) + result = await sandbox_client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + assert sc["error"] is not None, sc + assert "PermissionError" in sc["error"], sc["error"] + assert not target.exists() diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py new file mode 100644 index 0000000..950e8d5 --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -0,0 +1,178 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import json +import os +import shutil +import tempfile +import traceback +from typing import Annotated, Literal, Union + +from fastmcp import FastMCP +from pydantic import BaseModel, Field + +from thinkingbox_tools.toolslib.sandbox.code_interpreter import ( + CodeInterpreter, + CodeInterpreterError, +) +from thinkingbox_tools.toolslib.sandbox.sandbox import Sandbox + +mcp = FastMCP("sandbox") + +_sandbox: Sandbox | None = None +_interpreter: CodeInterpreter | None = None +_session_dir: str | None = ( + None # per-session copy of workspace_dir, cleaned up on teardown +) + + +class FilesResult(BaseModel): + status: Literal["ok"] = "ok" + files: list[str] + + +class ExecutionResult(BaseModel): + status: Literal["ok"] = "ok" + stdout: str + stderr: str + result: str | None + error: str | None + + +class ErrorResult(BaseModel): + status: Literal["error"] = "error" + message: str + + +def _symlink_or_copy(src: str, dst: str) -> None: + """Symlink dst → src; fall back to a real copy if symlinks aren't supported.""" + try: + os.symlink(os.path.abspath(src), dst) + except OSError: + shutil.copy2(src, dst) + + +@mcp.tool(name="__reserved__init") +async def initialize(config: dict): + global _sandbox, _interpreter, _session_dir + + workspace_dir = os.path.expanduser(os.path.expandvars(config.get("workspace_dir", ""))) + timeout = config.get("timeout", 30.0) + + # Populate a fresh temp directory with symlinks to the workspace files. + # Symlinks don't duplicate bytes, so init cost is proportional to the number + # of files rather than their size. Copy-on-write is enforced at the NODEFS + # layer inside the Pyodide worker (see pyodide_worker.mjs): when a symlink + # is opened for write, the worker replaces it with a private copy in the + # session directory before the write proceeds, leaving the source file + # untouched. New files created by the agent are regular files from the + # start and need no special handling. Falls back to a real copy if the + # filesystem doesn't support symlinks. + if _session_dir is not None: + shutil.rmtree(_session_dir, ignore_errors=True) + _session_dir = tempfile.mkdtemp(prefix="sandbox_session_") + if workspace_dir and os.path.isdir(workspace_dir): + shutil.copytree( + workspace_dir, _session_dir, copy_function=_symlink_or_copy, dirs_exist_ok=True + ) + + _sandbox = Sandbox(_session_dir) + if _interpreter is not None: + await _interpreter.close() + _interpreter = CodeInterpreter(timeout=timeout, workspace_dir=_session_dir) + return {} + + +@mcp.tool(name="__reserved__teardown") +async def teardown(): + global _sandbox, _interpreter, _session_dir + if _interpreter is not None: + await _interpreter.close() + _interpreter = None + _sandbox = None + if _session_dir is not None: + shutil.rmtree(_session_dir, ignore_errors=True) + _session_dir = None + return {} + + +@mcp.tool(name="__reserved__geteffects") +async def geteffects(): + return {"effects": _interpreter.effects if _interpreter else []} + + +@mcp.tool( + name="list_sandbox_files", + description=( + "List files in the workspace whose paths start with a given prefix. " + "The directory separator is /. " + "Returned paths are relative to the workspace root; prepend /workspace/ to use them in code_interpreter (e.g. 'reports/q1.csv' → open('/workspace/reports/q1.csv'))." + ), +) +async def list_files( + prefix: Annotated[ + str, + Field( + description="Path prefix to filter by, e.g. 'reports/' or '' for all files" + ), + ] = "", +) -> Union[FilesResult, ErrorResult]: + if _sandbox is None: + return ErrorResult(message="not initialized") + return FilesResult(files=_sandbox.list_files(prefix)) + + +@mcp.tool( + name="search_sandbox_files", + description=( + "Find files in the workspace whose paths match a wildcard pattern. " + "Returned paths are relative to the workspace root; prepend /workspace/ to use them in code_interpreter (e.g. 'reports/q1.csv' → open('/workspace/reports/q1.csv'))." + ), +) +async def search_files( + pattern: Annotated[ + str, + Field( + description="Glob pattern, e.g. '*.csv', 'reports/**/*.pdf', or '**/*.xlsx'" + ), + ], +) -> Union[FilesResult, ErrorResult]: + if _sandbox is None: + return ErrorResult(message="not initialized") + return FilesResult(files=_sandbox.search_files(pattern)) + + +@mcp.tool( + name="code_interpreter", + description=( + "Execute Python code in a sandboxed Pyodide (CPython-in-WebAssembly) interpreter. " + "Workspace files are accessible at /workspace/ using standard Python file I/O. " + "Pre-installed: numpy, pandas, beautifulsoup4, jinja2, sympy, altair, mpmath, lxml, " + "Pillow, openpyxl, xlsxwriter, markdownify, mammoth, pypdf, pdfminer.six, tabulate, " + "plotly, python-docx, python-pptx, reportlab. " + "Use micropip.install() to add other pure-Python packages. " + "The interpreter is stateful: variables and imports persist across calls." + ), +) +async def execute_python( + code: Annotated[str, Field(description="Python source code to execute")], +) -> Union[ExecutionResult, ErrorResult]: + if _interpreter is None: + return ErrorResult(message="not initialized") + try: + res = await _interpreter.execute(code) + return ExecutionResult( + stdout=res.stdout, + stderr=res.stderr, + result=res.result, + error=res.error, + ) + except CodeInterpreterError as e: + return ErrorResult(message=str(e)) + except Exception: + traceback.print_exc() + return ErrorResult(message="Internal error") + + +if __name__ == "__main__": + mcp.run(transport="stdio", show_banner=False) diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/__init__.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/__init__.py new file mode 100644 index 0000000..59e481e --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/__init__.py @@ -0,0 +1,2 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py new file mode 100644 index 0000000..72b939e --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -0,0 +1,185 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import asyncio +import json +from dataclasses import asdict, dataclass, field +from pathlib import Path + + +class CodeInterpreterError(Exception): + pass + + +@dataclass +class ExecutionResult: + stdout: str + stderr: str + result: str | None # repr() of the last expression, or None + error: str | None # formatted traceback, or None + + +class CodeInterpreter: + """ + Manages a long-lived Node.js / Pyodide subprocess. + + The subprocess hosts a CPython-in-WASM interpreter and communicates over + stdin/stdout using a newline-delimited JSON protocol (see pyodide_worker.mjs). + + The interpreter is stateful like a REPL: variables defined in one execute() + call are visible in subsequent calls. Call close() (or reinitialize via the + MCP __reserved__init tool) to get a fresh interpreter. + + Requires Node.js and the pyodide npm package: + cd thinkingbox_tools/toolslib/sandbox && npm install + """ + + # Pyodide loads the runtime + several dozen wheels on startup. The first run + # fetches PyPI wheels over the network; subsequent runs use the local cache. + STARTUP_TIMEOUT = 300.0 + + def __init__(self, timeout: float = 30.0, workspace_dir: str | None = None): + self.timeout = timeout + self.workspace_dir = workspace_dir + self.effects: list[dict] = [] + self._process: asyncio.subprocess.Process | None = None + self._worker_path = Path(__file__).parent / "pyodide_worker.mjs" + # Serializes execute() and close() against the shared worker stdio. + # The worker speaks a strict request/response protocol on a single + # stdin/stdout pair, so two concurrent execute() coroutines would + # interleave writes and race to read each other's response frame. + self._lock = asyncio.Lock() + + # ------------------------------------------------------------------ + # Public API + # ------------------------------------------------------------------ + + async def execute(self, code: str) -> ExecutionResult: + async with self._lock: + await self._ensure_started() + + request = json.dumps({"code": code}) + "\n" + self._process.stdin.write(request.encode()) + await self._process.stdin.drain() + + try: + response_line = await asyncio.wait_for( + self._process.stdout.readline(), + timeout=self.timeout, + ) + except asyncio.TimeoutError: + await self._kill() + raise CodeInterpreterError( + f"Execution timed out after {self.timeout}s. " + "The interpreter has been reset." + ) + + if not response_line: + await self._kill() + raise CodeInterpreterError( + "Worker process closed unexpectedly. " + "The interpreter has been reset." + ) + + data = json.loads(response_line) + result = ExecutionResult( + stdout=data.get("stdout", ""), + stderr=data.get("stderr", ""), + result=data.get("result"), + error=data.get("error"), + ) + + self.effects.append( + {"type": "code_execution", "code": code, "result": asdict(result)} + ) + return result + + async def close(self) -> None: + """Gracefully stop the worker process.""" + async with self._lock: + if self._process is None: + return + try: + self._process.stdin.close() + await asyncio.wait_for(self._process.wait(), timeout=5.0) + except (asyncio.TimeoutError, Exception): + await self._kill() + finally: + self._process = None + + # ------------------------------------------------------------------ + # Internal helpers + # ------------------------------------------------------------------ + + async def _ensure_started(self) -> None: + if self._process is None or self._process.returncode is not None: + await self._start() + + async def _start(self) -> None: + worker_dir = self._worker_path.parent + if not self._worker_path.exists(): + raise CodeInterpreterError( + f"Worker script not found: {self._worker_path}\n" + f"Run 'npm install' in {worker_dir} first." + ) + # node_modules is not packaged in the wheel — the sandbox depends on + # the pyodide npm package, which the user must install once after + # `pip install` (see docs/sandbox_code_interpreter.md). Detecting it + # before spawning lets us surface a precise remediation message + # instead of a generic "Cannot find package 'pyodide'" from Node. + if not (worker_dir / "node_modules" / "pyodide").exists(): + raise CodeInterpreterError( + "Pyodide Node dependency is not installed. Run once after " + f"`pip install`:\n cd {worker_dir} && npm install" + ) + + cmd = ["node", str(self._worker_path)] + if self.workspace_dir: + cmd += ["--workspace", self.workspace_dir] + self._process = await asyncio.create_subprocess_exec( + *cmd, + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + # Inherit parent stderr. Pyodide + micropip emit substantial + # diagnostic output during startup and on every loadPackagesFromImports + # call; piping it without a drain task would eventually block the worker + # once the kernel pipe buffer (~64KB on Linux) fills. + stderr=None, + # Run from the worker's own directory so Node.js can resolve + # the pyodide package in the sibling node_modules/ folder. + cwd=str(self._worker_path.parent), + ) + + # Wait for the { "ready": true } handshake before accepting requests. + try: + ready_line = await asyncio.wait_for( + self._process.stdout.readline(), + timeout=self.STARTUP_TIMEOUT, + ) + except asyncio.TimeoutError: + await self._kill() + raise CodeInterpreterError( + f"Worker timed out during startup (>{self.STARTUP_TIMEOUT}s). " + f"Make sure 'npm install' has been run in {worker_dir}." + ) + + if not ready_line: + await self._kill() + raise CodeInterpreterError( + "Worker exited before sending ready signal. " + "See [pyodide_worker] output above for details." + ) + ready = json.loads(ready_line) + if not ready.get("ready"): + await self._kill() + raise CodeInterpreterError(f"Unexpected worker handshake: {ready}") + + async def _kill(self) -> None: + if self._process: + try: + self._process.kill() + await self._process.wait() + except Exception: + pass + finally: + self._process = None diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json new file mode 100644 index 0000000..463ce4d --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json @@ -0,0 +1,54 @@ +{ + "name": "sandbox", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "sandbox", + "hasInstallScript": true, + "dependencies": { + "pyodide": "^0.29.0" + } + }, + "node_modules/@types/emscripten": { + "version": "1.41.5", + "resolved": "https://registry.npmjs.org/@types/emscripten/-/emscripten-1.41.5.tgz", + "integrity": "sha512-cMQm7pxu6BxtHyqJ7mQZ2kXWV5SLmugybFdHCBbJ5eHzOo6VhBckEgAT3//rP5FwPHNPeEiq4SmQ5ucBwsOo4Q==", + "license": "MIT" + }, + "node_modules/pyodide": { + "version": "0.29.4", + "resolved": "https://registry.npmjs.org/pyodide/-/pyodide-0.29.4.tgz", + "integrity": "sha512-tCseTsqU3kSxZIjkue5zXxTMNEwrKZwOIIEQRBA/VzHxFN1hoCxe4w41phfCdHd9it9RcCNQb5K/Re0InqMgvA==", + "license": "MPL-2.0", + "dependencies": { + "@types/emscripten": "^1.41.4", + "ws": "^8.5.0" + }, + "engines": { + "node": ">=18.0.0" + } + }, + "node_modules/ws": { + "version": "8.21.3", + "resolved": "https://ms-feed-12.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/ws/-/ws-8.21.3.tgz", + "integrity": "sha1-ZgtPrdtqPldchuB4EmkZlh9N5Pw=", + "license": "MIT", + "engines": { + "node": ">=10.0.0" + }, + "peerDependencies": { + "bufferutil": "^4.0.1", + "utf-8-validate": ">=5.0.2" + }, + "peerDependenciesMeta": { + "bufferutil": { + "optional": true + }, + "utf-8-validate": { + "optional": true + } + } + } + } +} diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json new file mode 100644 index 0000000..ece42a2 --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json @@ -0,0 +1,9 @@ +{ + "type": "module", + "scripts": { + "postinstall": "node scripts/download-wheels.mjs" + }, + "dependencies": { + "pyodide": "^0.29.0" + } +} diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs new file mode 100644 index 0000000..1143ffa --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs @@ -0,0 +1,337 @@ +/** + * Pyodide worker — long-lived Node.js process that hosts a Pyodide (CPython-in-WASM) + * interpreter. The parent process communicates over stdin/stdout using a simple + * newline-delimited JSON protocol: + * + * stdin → { "code": "" } + * stdout ← { "stdout": "...", "stderr": "...", "result": "..." | null, "error": "..." | null } + * + * A single { "ready": true } line is written to stdout once Pyodide has finished + * loading and is ready to accept requests. + * + * Diagnostic / startup messages are written to stderr so they never mix with the + * stdout protocol. + * + * Run from the toolslib/ directory so Node.js can resolve the pyodide package: + * node pyodide_worker.mjs + */ + +import { loadPyodide } from "pyodide"; +import { createInterface } from "readline"; +import { readdir } from "node:fs/promises"; +import { lstatSync, statSync, readFileSync, writeFileSync, unlinkSync } from "node:fs"; +import { PYPI_PACKAGES } from "./pypi-packages.mjs"; + +// Pyodide and micropip log package-loading progress to Python's sys.stdout +// (e.g. micropip.install internally calls pyodide.loadPackage for transitive +// deps from the lock, which prints "Loading X, Y" / "Loaded X, Y" messages). +// Those would land on Node's stdout and corrupt our JSON protocol, so route +// Python-level output and JS-level console output to stderr instead. The +// worker uses process.stdout.write directly for protocol frames, which is +// unaffected. User-code stdout/stderr is captured separately via StringIO +// inside _execute (see below). +const _toStderr = (msg) => process.stderr.write(msg + "\n"); +console.log = (...args) => _toStderr(args.join(" ")); +console.info = console.log; +console.warn = console.log; + +// Parse optional --workspace argument +const workspaceIdx = process.argv.indexOf("--workspace"); +const workspacePath = workspaceIdx !== -1 ? process.argv[workspaceIdx + 1] : null; + +process.stderr.write("[pyodide_worker] Loading Pyodide...\n"); + +const pyodide = await loadPyodide({ + stdout: _toStderr, + stderr: _toStderr, +}); + +// Packages bundled in pyodide-lock.json — loaded directly as pyodide-built wheels. +const BUNDLED_PACKAGES = [ + "numpy", + "pandas", + "micropip", + "beautifulsoup4", + "jinja2", + "sympy", + "altair", + "mpmath", + "lxml", + "pillow", +]; + +process.stderr.write("[pyodide_worker] Loading bundled packages...\n"); +await pyodide.loadPackage(BUNDLED_PACKAGES, { + messageCallback: (msg) => process.stderr.write(msg + "\n"), + errorCallback: (err) => process.stderr.write(err + "\n"), +}); + +// Resolve each PyPI package to a local wheel (file:// URL) when one was +// vendored by `npm install` into ./wheels/ — avoids hitting PyPI on every +// worker startup. Anything without a local wheel falls back to a bare +// package name, which micropip resolves via PyPI or via pyodide's own +// bundled wheel set (e.g. reportlab). +const wheelsDir = new URL("./wheels/", import.meta.url); +const normalizePkg = (s) => s.toLowerCase().replace(/[-._]/g, "_"); +let localWheels = []; +try { + localWheels = await readdir(wheelsDir); +} catch { + process.stderr.write("[pyodide_worker] No local wheels/ directory — micropip will fetch from PyPI\n"); +} +const installSpecs = PYPI_PACKAGES.map((pkg) => { + const target = normalizePkg(pkg); + // PEP 427 wheel filename: {distribution}-{version}(-{build})?-{python}-{abi}-{platform}.whl + // Compare the normalized distribution segment, not a normalized prefix — + // normalizePkg(f) rewrites the "-" separators to "_", so a "-"-suffixed + // prefix could never match. + const match = localWheels.find((f) => { + if (!f.endsWith(".whl")) return false; + const dist = f.split("-")[0]; + return normalizePkg(dist) === target; + }); + return match ? new URL(match, wheelsDir).href : pkg; +}); + +process.stderr.write("[pyodide_worker] Installing PyPI packages via micropip...\n"); +pyodide.globals.set("_pypi_install_specs", installSpecs); +await pyodide.runPythonAsync(` +import micropip +await micropip.install(list(_pypi_install_specs)) +`); +process.stderr.write("[pyodide_worker] Packages loaded.\n"); + +// Install a reusable _execute helper in Pyodide's global namespace. +// The helper: +// - redirects stdout/stderr to StringIO buffers for the duration of the call +// - uses the AST trick to capture the value of a trailing expression (Jupyter style) +// - runs all code in a shared _namespace dict so variables persist across calls +// - compiles with PyCF_ALLOW_TOP_LEVEL_AWAIT so user code can `await` directly +// (e.g. `await micropip.install("pkg")`) — Jupyter / IPython %autoawait semantics +// - always restores real stdout/stderr in the finally block +await pyodide.runPythonAsync(` +import sys, io, traceback, ast, inspect + +_namespace = {} +_real_stdout = sys.stdout +_real_stderr = sys.stderr + +_TOP_LEVEL_AWAIT = ast.PyCF_ALLOW_TOP_LEVEL_AWAIT + +async def _execute(code): + buf_out = io.StringIO() + buf_err = io.StringIO() + sys.stdout = buf_out + sys.stderr = buf_err + + result = None + error = None + + try: + tree = ast.parse(code, filename="") + # If the last node is a bare expression, split it off so we can eval it + # and capture its value (like a Jupyter cell last-expression result). + if tree.body and isinstance(tree.body[-1], ast.Expr): + stmts = ast.Module(tree.body[:-1], type_ignores=[]) + ast.fix_missing_locations(stmts) + # PyCF_ALLOW_TOP_LEVEL_AWAIT turns the compiled object into a + # coroutine when any await/async-for/async-with appears at module + # scope; eval() of an exec-mode code object returns that coroutine + # so we can await it. exec() would silently drop it. + stmt_code = compile(stmts, "", "exec", flags=_TOP_LEVEL_AWAIT) + stmt_coro = eval(stmt_code, _namespace) + if inspect.iscoroutine(stmt_coro): + await stmt_coro + + expr_node = ast.Expression(tree.body[-1].value) + ast.fix_missing_locations(expr_node) + expr_code = compile(expr_node, "", "eval", flags=_TOP_LEVEL_AWAIT) + value = eval(expr_code, _namespace) + if inspect.iscoroutine(value): + value = await value + result = value + else: + full_code = compile(tree, "", "exec", flags=_TOP_LEVEL_AWAIT) + full_coro = eval(full_code, _namespace) + if inspect.iscoroutine(full_coro): + await full_coro + except BaseException: + error = traceback.format_exc() + finally: + sys.stdout = _real_stdout + sys.stderr = _real_stderr + + return ( + buf_out.getvalue(), + buf_err.getvalue(), + repr(result) if result is not None else None, + error, + ) +`); + +if (workspacePath) { + process.stderr.write(`[pyodide_worker] Mounting workspace: ${workspacePath}\n`); + pyodide.FS.mkdir("/workspace"); + // NODEFS mounts the per-session directory created by mcp_sandbox's + // __reserved__init. That directory contains symlinks pointing at the + // source workspace; reads follow the symlinks transparently, writes are + // intercepted below to materialize a private copy first. + pyodide.FS.mount(pyodide.FS.filesystems.NODEFS, { root: workspacePath }, "/workspace"); + + // Copy-on-write at the NODEFS layer. The mount points at a directory of + // symlinks; if we let Emscripten's path resolver see them as symlinks it + // would try to FS.readlink and follow the absolute target out of the + // /workspace/ mount (failing with ENOENT). Instead we make symlinks look + // like regular files to Emscripten, then rely on host-level fs.openSync + // to follow them for reads. For writes we replace the symlink with a + // private copy in the session directory before delegating to the real + // open, so the source file is never mutated. This catches every caller — + // builtins.open, os.open, io.FileIO, mmap, sqlite3, numpy — because they + // all bottom out in NODEFS. + const NODEFS = pyodide.FS.filesystems.NODEFS; + + const _origLookup = NODEFS.node_ops.lookup; + NODEFS.node_ops.lookup = function (parent, name) { + const node = _origLookup(parent, name); + try { + const lst = lstatSync(NODEFS.realPath(node)); + if (lst.isSymbolicLink()) { + const target = statSync(NODEFS.realPath(node)); + node.mode = (node.mode & 0o7777) | (target.mode & 0o170000); + } + } catch {} + return node; + }; + + const _origGetattr = NODEFS.node_ops.getattr; + NODEFS.node_ops.getattr = function (node) { + const attr = _origGetattr(node); + try { + const path = NODEFS.realPath(node); + if (lstatSync(path).isSymbolicLink()) { + const target = statSync(path); + attr.mode = (attr.mode & 0o7777) | (target.mode & 0o170000); + attr.size = target.size; + attr.blocks = Math.ceil(target.size / (attr.blksize || 4096)); + attr.atime = target.atime; + attr.mtime = target.mtime; + attr.ctime = target.ctime; + } + } catch {} + return attr; + }; + + const materializeSymlink = (path, preserveContent) => { + if (preserveContent) { + const data = readFileSync(path); // follows symlink → source bytes + unlinkSync(path); + writeFileSync(path, data); + } else { + // O_TRUNC path: caller is about to discard the contents anyway, so + // skip copying the source bytes — but still leave a real, empty + // file at the path. Otherwise `os.open(path, O_WRONLY | O_TRUNC)` + // (no O_CREAT) would land on a missing host file and fail with + // ENOENT, even though the symlink looked like a regular file + // before the open() call. + unlinkSync(path); + writeFileSync(path, ""); + } + }; + + const _origStreamOpen = NODEFS.stream_ops.open; + NODEFS.stream_ops.open = function (stream) { + const O_ACCMODE = 3; + const O_TRUNC = 0o1000; + if ((stream.flags & O_ACCMODE) !== 0) { + const path = NODEFS.realPath(stream.node); + let lst; + try { lst = lstatSync(path); } catch {} + if (lst && lst.isSymbolicLink()) { + materializeSymlink(path, (stream.flags & O_TRUNC) === 0); + } + } + return _origStreamOpen(stream); + }; + + const _origSetattr = NODEFS.node_ops.setattr; + NODEFS.node_ops.setattr = function (node, attr) { + const path = NODEFS.realPath(node); + let lst; + try { lst = lstatSync(path); } catch {} + if (lst && lst.isSymbolicLink()) { + materializeSymlink(path, true); + } + return _origSetattr(node, attr); + }; + + // Block user code from creating symlinks inside /workspace. Seeded + // symlinks (placed by mcp_sandbox.__reserved__init before this mount) + // are unaffected — they were materialized on the host fs before NODEFS + // wrapped them, and this hook only fires for in-sandbox symlinkat calls. + // Without this, user code could do os.symlink("/etc/passwd", + // "/workspace/x") and then read it transparently via readFileSync's + // built-in symlink following. + NODEFS.node_ops.symlink = function () { + throw new pyodide.FS.ErrnoError(pyodide.ERRNO_CODES.EPERM); + }; + + process.stderr.write("[pyodide_worker] Workspace mounted at /workspace\n"); +} + +process.stderr.write("[pyodide_worker] Ready.\n"); + +// Signal readiness to the parent process over stdout. +process.stdout.write(JSON.stringify({ ready: true }) + "\n"); + +// Process requests one at a time (sequential request/response). +const rl = createInterface({ input: process.stdin, terminal: false }); + +for await (const line of rl) { + if (!line.trim()) continue; + + let request; + try { + request = JSON.parse(line); + } catch (e) { + process.stdout.write( + JSON.stringify({ stdout: "", stderr: "", result: null, error: `invalid_json: ${e.message}` }) + "\n" + ); + continue; + } + + try { + // Auto-load any pyodide-distributed packages referenced by the user's + // imports but not in the eager preload set (e.g. matplotlib, scipy). + // No-op for already-loaded packages. Pure-Python PyPI packages must + // still be listed in PYPI_PACKAGES (or installed via micropip in user + // code) since loadPackagesFromImports only resolves pyodide's lock. + await pyodide.loadPackagesFromImports(request.code, { + messageCallback: _toStderr, + errorCallback: _toStderr, + }); + + pyodide.globals.set("_user_code", request.code); + // _execute is async — runPythonAsync awaits the returned coroutine. + const pyResult = await pyodide.runPythonAsync("await _execute(_user_code)"); + const [stdout, stderr, result, error] = pyResult.toJs({ depth: -1 }); + pyResult.destroy(); + + process.stdout.write( + JSON.stringify({ + stdout: stdout ?? "", + stderr: stderr ?? "", + result: result ?? null, + error: error ?? null, + }) + "\n" + ); + } catch (e) { + process.stdout.write( + JSON.stringify({ + stdout: "", + stderr: "", + result: null, + error: `worker_error: ${e.message}`, + }) + "\n" + ); + } +} diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs new file mode 100644 index 0000000..3c0eb3e --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs @@ -0,0 +1,20 @@ +// Pure-Python wheels fetched from PyPI. These are NOT in pyodide's package +// lock, so pyodide.loadPackagesFromImports() cannot auto-load them — they +// must be installed via micropip. At `npm install` time, scripts/download-wheels.mjs +// downloads each into ./wheels/, and the worker installs from local file:// +// URLs to avoid hitting PyPI on every worker startup. Anything that can't +// be vendored (e.g. no pure-Python wheel published) falls back to a bare +// package name and micropip fetches it at runtime. +export const PYPI_PACKAGES = [ + "openpyxl", + "xlsxwriter", + "markdownify", + "mammoth", + "pypdf", + "pdfminer.six", + "tabulate", + "plotly", + "python-docx", + "python-pptx", + "reportlab", +]; diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py new file mode 100644 index 0000000..92ab027 --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py @@ -0,0 +1,51 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +import os +from pathlib import Path + + +class Sandbox: + def __init__(self, workspace_dir: str): + self.workspace_dir = Path(os.path.expandvars(workspace_dir)).expanduser() + + def list_files(self, prefix: str = "") -> list[str]: + """Return relative file paths under workspace_dir/prefix, sorted.""" + root = self.workspace_dir.resolve() + base = (self.workspace_dir / prefix).resolve() if prefix else root + if base != root and root not in base.parents: + return [] + if not base.exists(): + return [] + results = [] + for dirpath, dirs, files in os.walk(base): + dirs[:] = sorted(d for d in dirs if not d.startswith(".")) + for f in sorted(files): + if not f.startswith("."): + # Relative to `root`, not `self.workspace_dir`: os.walk() + # descends from the resolved `base`, so its dirpaths carry + # the resolved spelling. Wherever the two differ — a + # symlinked workspace, macOS /tmp -> /private/tmp, a Windows + # 8.3 short path — relative_to(self.workspace_dir) raises + # ValueError. They are identical when nothing is aliased. + rel = Path(dirpath, f).relative_to(root) + # The tool contract (and /workspace/ concatenation in + # the interpreter) requires "/" separators on every host. + results.append(rel.as_posix()) + return results + + def search_files(self, pattern: str) -> list[str]: + """Return relative file paths matching a glob pattern.""" + # Reject any match that navigates via "..", even if it loops back into + # the workspace. We check the lexical path (p.parts), not p.resolve(): + # __reserved__init populates the workspace with symlinks to source + # files for COW isolation, and resolving those would land outside + # workspace_dir for every legitimate match. + results = [] + for p in self.workspace_dir.glob(pattern): + if not p.is_file() or p.name.startswith("."): + continue + if ".." in p.parts: + continue + results.append(p.relative_to(self.workspace_dir).as_posix()) + return sorted(results) diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs new file mode 100644 index 0000000..284161d --- /dev/null +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -0,0 +1,71 @@ +#!/usr/bin/env node +// Vendors pure-Python wheels for the PyPI packages listed in pypi-packages.mjs +// into ../wheels/, so the pyodide worker can install them from local file:// +// URLs instead of fetching from PyPI on every startup. +// +// Runs automatically via `npm install` (see package.json "postinstall"). +// Idempotent: existing wheel files are kept. To force a refresh, delete +// the wheels/ directory and re-run `npm install`. +// +// Packages without a pure-Python (`*-none-any.whl`) wheel on PyPI are skipped +// with a warning; the worker falls back to micropip at runtime for those +// (which works because micropip resolves them via pyodide's own bundle when +// available, e.g. reportlab). + +import { mkdir, writeFile, access } from "node:fs/promises"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import { PYPI_PACKAGES } from "../pypi-packages.mjs"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const wheelsDir = join(__dirname, "..", "wheels"); + +await mkdir(wheelsDir, { recursive: true }); + +const PURE_PYTHON_WHEEL = /-py[23](\.py3)?-none-any\.whl$/; + +async function downloadOne(name) { + const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/json`); + if (!res.ok) { + console.warn(`[download-wheels] PyPI returned ${res.status} for ${name} — skipping`); + return; + } + const data = await res.json(); + const version = data.info.version; + const wheel = (data.releases[version] || []).find( + (f) => f.packagetype === "bdist_wheel" && PURE_PYTHON_WHEEL.test(f.filename), + ); + if (!wheel) { + console.log(`[download-wheels] No pure-Python wheel for ${name} ${version} — leaving to runtime`); + return; + } + const dest = join(wheelsDir, wheel.filename); + try { + await access(dest); + console.log(`[download-wheels] Cached: ${wheel.filename}`); + return; + } catch { + // not present — download below + } + console.log(`[download-wheels] Downloading: ${wheel.filename}`); + const wRes = await fetch(wheel.url); + if (!wRes.ok) { + console.warn(`[download-wheels] Download failed (${wRes.status}) for ${wheel.url} — skipping`); + return; + } + await writeFile(dest, Buffer.from(await wRes.arrayBuffer())); +} + +await Promise.all( + PYPI_PACKAGES.map((name) => + // A network failure (offline, blocked host, TLS error) rejects fetch() + // rather than returning a non-ok response. Without this catch the + // rejection propagates out of Promise.all and fails `npm install` + // outright, instead of degrading to the documented behavior: skip the + // wheel and let micropip fetch it at worker startup. + downloadOne(name).catch((err) => { + console.warn(`[download-wheels] ${name}: ${err.message} — leaving to runtime`); + }), + ), +); +console.log("[download-wheels] Done."); From a4010aa478bb1bc84380631b7bb61d63859c3e78 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Wed, 26 Aug 2026 18:39:35 -0700 Subject: [PATCH 02/21] Bump actions/setup-node to v7 v4 targets the deprecated Node.js 20 runtime and CI emitted a deprecation annotation. v7 also matches how this repo pins actions/checkout@v7 and astral-sh/setup-uv@v7. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/test-thinkingbox-tools.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-thinkingbox-tools.yml b/.github/workflows/test-thinkingbox-tools.yml index 1e14c2f..42e5a75 100644 --- a/.github/workflows/test-thinkingbox-tools.yml +++ b/.github/workflows/test-thinkingbox-tools.yml @@ -29,7 +29,7 @@ jobs: uv sync --group dev - name: Install Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@v7 with: node-version: '22' From f442a910bb6bfcb7e22476679238e9e0591c94ba Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Wed, 26 Aug 2026 18:59:45 -0700 Subject: [PATCH 03/21] Address review: revert lockfile to public registry, clear CodeQL alerts package-lock.json: revert the ws 8.19.0 -> 8.21.3 bump and restore the upstream lockfile byte-for-byte. My `npm audit fix` had been resolved through a corporate npm proxy, which rewrote ws's `resolved` URL to an internal Azure Artifacts host and downgraded its `integrity` from sha512 to sha1. Both are unacceptable in a public repo: the URL leaks an internal endpoint and may be unreachable for outside contributors, and the sha1 value is a supply-chain integrity downgrade. The proxy strips `dist.integrity` and rewrites tarball URLs, so a correct public lockfile entry cannot be produced from this network. Reverting is preferable to committing an integrity hash that cannot be verified against the public registry. This restores the two `ws` advisories; ws is a transitive dependency of pyodide (`ws: ^8.5.0`) that the sandbox worker never uses, and the bump should come from Dependabot or a pyodide upgrade resolved against registry.npmjs.org. Also from review feedback: - Drop unused imports `json` (mcp_sandbox) and `field` (code_interpreter); both were flagged by CodeQL. - Explain the bare `except Exception: pass` in CodeInterpreter._kill, flagged by CodeQL as an empty except clause. - Bound both fetch() calls in the npm postinstall script with AbortSignal.timeout, so a stalled connection fails fast instead of hanging `npm install` indefinitely. Co-authored-by: Susana Palmaz Lopez-Pelaez Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../thinkingbox_tools/mcp_sandbox.py | 1 - .../toolslib/sandbox/code_interpreter.py | 6 +++++- .../toolslib/sandbox/package-lock.json | 10 ++++------ .../toolslib/sandbox/scripts/download-wheels.mjs | 14 ++++++++++++-- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py index 950e8d5..0c0ff0d 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -1,7 +1,6 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -import json import os import shutil import tempfile diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index 72b939e..4a4c16d 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -3,7 +3,7 @@ import asyncio import json -from dataclasses import asdict, dataclass, field +from dataclasses import asdict, dataclass from pathlib import Path @@ -180,6 +180,10 @@ async def _kill(self) -> None: self._process.kill() await self._process.wait() except Exception: + # Best effort: the worker may already be dead, or reaping it may + # race with the event loop shutting down. Either way the process + # handle is dropped below and the next call spawns a fresh one, + # so there is nothing useful to recover or report here. pass finally: self._process = None diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json index 463ce4d..8f6a121 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json @@ -1,11 +1,9 @@ { - "name": "sandbox", + "name": "toolslib", "lockfileVersion": 3, "requires": true, "packages": { "": { - "name": "sandbox", - "hasInstallScript": true, "dependencies": { "pyodide": "^0.29.0" } @@ -30,9 +28,9 @@ } }, "node_modules/ws": { - "version": "8.21.3", - "resolved": "https://ms-feed-12.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/ws/-/ws-8.21.3.tgz", - "integrity": "sha1-ZgtPrdtqPldchuB4EmkZlh9N5Pw=", + "version": "8.19.0", + "resolved": "https://registry.npmjs.org/ws/-/ws-8.19.0.tgz", + "integrity": "sha512-blAT2mjOEIi0ZzruJfIhb3nps74PRWTCz1IjglWEEpQl5XS/UNama6u2/rjFkDDouqr4L67ry+1aGIALViWjDg==", "license": "MIT", "engines": { "node": ">=10.0.0" diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs index 284161d..c4f08ac 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -24,8 +24,16 @@ await mkdir(wheelsDir, { recursive: true }); const PURE_PYTHON_WHEEL = /-py[23](\.py3)?-none-any\.whl$/; +// A stalled connection would otherwise hang `npm install` indefinitely, since +// fetch() has no default timeout. Bounding it lets the script fail fast and +// degrade to a runtime micropip fetch, as documented. +const METADATA_TIMEOUT_MS = 30_000; +const DOWNLOAD_TIMEOUT_MS = 120_000; + async function downloadOne(name) { - const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/json`); + const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/json`, { + signal: AbortSignal.timeout(METADATA_TIMEOUT_MS), + }); if (!res.ok) { console.warn(`[download-wheels] PyPI returned ${res.status} for ${name} — skipping`); return; @@ -48,7 +56,9 @@ async function downloadOne(name) { // not present — download below } console.log(`[download-wheels] Downloading: ${wheel.filename}`); - const wRes = await fetch(wheel.url); + const wRes = await fetch(wheel.url, { + signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS), + }); if (!wRes.ok) { console.warn(`[download-wheels] Download failed (${wRes.status}) for ${wheel.url} — skipping`); return; From 44fd1cf364112146a1f5ccd93e0817c2004e5d70 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Wed, 26 Aug 2026 19:04:24 -0700 Subject: [PATCH 04/21] Bump ws to 8.21.3 in the lockfile using public registry metadata Dependency Review runs with `fail-on-severity: high`, so the upstream lockfile's ws@8.19.0 is a hard blocker (GHSA-96hv-2xvq-fx4p). The previous commit had reverted to it after my `npm audit fix` was found to have written an internal Azure Artifacts URL and a sha1 integrity, so neither version was mergeable. This pins ws 8.21.3 with the correct public URL and a genuine sha512. The integrity was derived without access to registry.npmjs.org (blocked from my machine) and then independently verified, rather than copied from the corporate proxy, which strips `dist.integrity`: 1. Downloaded ws@8.19.0 through the proxy and computed its sha512. It matched the upstream lockfile's known-good value exactly, establishing that the proxy mirrors the public registry byte-for-byte. 2. Computed the sha512 of ws@8.21.3 the same way. 3. `npm ci` validates the tarball against this hash and succeeds, and `npm audit` now reports 0 vulnerabilities. CI re-verifies against the real registry.npmjs.org, so a wrong hash would fail the build rather than pass silently. Only version/resolved/integrity change; ws 8.21.3 carries the same license, engines and peerDependencies metadata as 8.19.0, and pyodide's `ws: ^8.5.0` range is unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../thinkingbox_tools/toolslib/sandbox/package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json index 8f6a121..f187072 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json @@ -28,9 +28,9 @@ } }, "node_modules/ws": { - "version": "8.19.0", - "resolved": "https://registry.npmjs.org/ws/-/ws-8.19.0.tgz", - "integrity": "sha512-blAT2mjOEIi0ZzruJfIhb3nps74PRWTCz1IjglWEEpQl5XS/UNama6u2/rjFkDDouqr4L67ry+1aGIALViWjDg==", + "version": "8.21.3", + "resolved": "https://registry.npmjs.org/ws/-/ws-8.21.3.tgz", + "integrity": "sha512-201TZ/kPWxoPr/OKWjquZR1SWKXcvxdH+e1xrx89b3YbmzLMFCLfnaG1HFIgWzJOEWZ7MvpK++odZufgYR50Rw==", "license": "MIT", "engines": { "node": ">=10.0.0" From 47b7b024ebfb6b77051a4a2d92128993cee8e669 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 11:37:51 -0700 Subject: [PATCH 05/21] Correct sandbox threat model; reject escaping workspace links Security review found that agent-supplied Python can escape Pyodide into the Node host. Verified against the pinned pyodide 0.29.4, through the real code_interpreter tool: `import js` exposes `process`; `pyodide_js._api` is reachable and its `loadBinaryFile` reads host files outside the session directory; any JsProxy yields `.constructor.constructor` (the Function constructor), from which `node:fs` and `node:child_process` resolve via dynamic import(); and all 76 parent environment variables are readable. In-process mitigations do not close this. `jsglobals: {}` fails because Function bodies evaluate in global scope, not the restricted object. Unregistering modules fails because Pyodide internals hold live JsProxy references captured at load time. Node's --permission model is experimental, does not gate process.env, and is not a substitute for an OS boundary. Closing this requires confining the worker in an owner-approved OS/container boundary; that design is proposed on the PR and is deliberately not implemented here, as it rewrites the execution model and the CI shape. Threat model corrected. The docs previously claimed "user code cannot reach the host filesystem, network, or processes", which is false. The docs now carry an explicit warning and a Threat model section, the "run untrusted Python safely" requirement is removed, and the code_interpreter tool description no longer advertises itself as sandboxed. mcp_sandbox carries a module-level warning that only trusted first-party agent code may run here. Workspace link handling fixed. __reserved__init previously re-pointed links found in the source workspace, and NODEFS follows host links transparently, so a link escaping workspace_dir was readable from inside /workspace. Now: links resolving inside the workspace are materialized as real copies, links resolving outside are rejected, and linked directories are rejected outright to avoid traversal and copytree recursion loops. Detection uses the Windows reparse-point attribute in addition to S_ISLNK, because os.path.islink() returns False for junctions -- which can be created without elevation -- and fails closed on entries that cannot be lstat'd. Adds tests/test_sandbox_isolation.py: - Link-handling regression tests. Verified to fail against the pre-fix code: the junction case seeded `junc/hidden.txt` from outside the workspace. - A host capability audit marked xfail(strict=True). These fail today by design, recording the gap in the suite rather than omitting it; strict means they will XPASS and fail the build once confinement lands, forcing the markers to be removed. The probes measure reachability only -- they use a sentinel file the test creates, never a real system file, and never execute a command or open a socket. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 83 +++- .../tests/test_sandbox_isolation.py | 432 ++++++++++++++++++ .../thinkingbox_tools/mcp_sandbox.py | 83 +++- 3 files changed, 590 insertions(+), 8 deletions(-) create mode 100644 servers/thinkingbox_tools/tests/test_sandbox_isolation.py diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index 4f2743c..2c49ef4 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -1,9 +1,56 @@ # Sandbox Code Interpreter (Pyodide) -The `sandbox` MCP server runs agent-supplied Python in a sandboxed Pyodide +The `sandbox` MCP server runs agent-supplied Python in a Pyodide (CPython-in-WebAssembly) interpreter, exposes the test's workspace files at `/workspace/`, and isolates writes via copy-on-write at the NODEFS layer. +> [!WARNING] +> **This is not a security boundary.** Pyodide provides memory safety, not +> isolation: Python running here can reach the Node.js host, the host +> filesystem, process execution and environment variables. Run only trusted, +> first-party agent code. See [Threat model](#threat-model) before routing any +> untrusted input to this server. + +--- + +## Threat model + +Pyodide is explicitly **not** a security sandbox for untrusted code. WASM +gives memory safety, but Pyodide deliberately exposes a Python↔JavaScript FFI, +and that FFI is reachable from user code. + +Verified against the pinned `pyodide` version, escaping Python into the Node +host is possible via several independent routes: + +- `import js` exposes the JavaScript global scope, including `process`. +- `pyodide_js._api` is reachable from Python and exposes internals such as + `loadBinaryFile`, which reads host files outside the session directory. +- Any reachable `JsProxy` yields `.constructor.constructor` — the `Function` + constructor — and `node:fs` / `node:child_process` are reachable from there + via dynamic `import()`. +- The full parent environment (every variable in `process.env`) is readable. + +**In-process mitigations do not close this.** `jsglobals: {}` fails because +`Function` bodies evaluate in the *global* scope, not the restricted object. +Deleting or unregistering modules fails because Pyodide's internals hold live +`JsProxy` references captured at load time. Node's `--permission` model is +useful defense-in-depth but is experimental, does not gate `process.env`, and +is not a substitute for an OS boundary. + +**Consequences for how this server may be used:** + +- Only trusted, first-party agent code may be executed. +- The workspace must be treated as trusted input. +- Secrets must not be present in the environment of the MCP server process, + because the worker inherits it. + +**Required to lift these constraints:** confine the worker with an +owner-approved OS/container boundary — no host filesystem beyond the session +directory, no inherited environment, no network, no process spawning, plus +pid/memory/CPU limits. The NODEFS copy-on-write layer described below is a +*correctness* mechanism for workspace isolation, not a security control, and +does not mitigate any of the above. + --- ## Architecture @@ -46,15 +93,15 @@ variables and imports never leak between sessions. ## Why Pyodide -Requirements for the sandbox: +Requirements this choice was made against: -- Run untrusted, agent-generated Python safely. +- Run agent-generated Python with a **reproducible, pinned** package set. - Deterministic behavior across machines. - Minimized per-call cold-start cost. -The WASM runtime is the trust boundary: user code cannot reach the host -filesystem, network, or processes except through the FS bridges we -explicitly expose. +Note that "run untrusted code safely" is **not** on that list, and Pyodide +does not provide it — see [Threat model](#threat-model). The WASM runtime is a +*memory-safety* boundary, not a privilege boundary. Compared with seccomp'd subprocesses or per-session containers, Pyodide gives us: @@ -63,6 +110,10 @@ gives us: - A **single dependency** to install (Node plus the `pyodide` npm package). - **REPL-style state** via a shared `_namespace` across calls in a session. +What it costs us: containers and seccomp'd subprocesses would have given a +real privilege boundary, which Pyodide does not. Recovering that requires +wrapping the worker in OS-level confinement. + Trade-off: ~5–10 s first start, paid once and amortized across all `code_interpreter` calls in the session. @@ -226,6 +277,26 @@ writes from one session must not affect the workspace or the next session. The session directory is what gets mutated below, and is `rmtree`'d on teardown. +#### Links in the source workspace + +Seeding must not blindly re-point a link that already exists in +`workspace_dir`. NODEFS reads follow host links transparently, so a link whose +target sits outside the workspace would be readable from inside `/workspace`. +Windows junctions matter here too: `os.path.islink` returns `False` for them, +so the reparse-point attribute is checked explicitly, and an entry that cannot +be `lstat`'d is treated as a link so the check fails closed. + +| Entry in `workspace_dir` | Seeding behavior | +| ---- | ---- | +| Regular file | Symlinked into the session dir (the O(files) fast path). | +| Link whose target resolves **inside** `workspace_dir` | Materialized as a real copy — data preserved, no followable link. | +| Link whose target resolves **outside** `workspace_dir` | **Rejected**, reported on stderr. | +| Linked directory | **Rejected** — avoids both traversal and `copytree` recursion loops. | + +Note this is workspace hygiene, not a privilege boundary: a caller who can +already execute code in the worker does not need a link to reach host files +(see [Threat model](#threat-model)). + ### NODEFS copy-on-write inside Pyodide NODEFS is Pyodide's bridge to Node's `fs` module. By default, when diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py new file mode 100644 index 0000000..0ef6fea --- /dev/null +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -0,0 +1,432 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Isolation regression tests for the sandbox MCP server. + +Two groups: + +1. **Workspace link handling** — asserts that links in the source workspace are + rejected or safely materialized during ``__reserved__init``. These pass and + are a genuine regression guard: they fail against the pre-fix seeding logic. + +2. **Host capability audit** — asserts that host capabilities are unavailable to + agent-supplied Python. These are currently ``xfail(strict=True)``: Pyodide is + not a privilege boundary, so they genuinely fail today. They are recorded + here rather than omitted so the gap is visible in the suite, and ``strict`` + means that once OS/container confinement lands they will XPASS and force the + marker to be removed. See docs/sandbox_code_interpreter.md ("Threat model"). + +The capability probes measure *reachability only*. They never read a real +system or secret file — a sentinel the test itself creates is used instead — +and they never execute a command or open a network connection. +""" + +import os +import sys + +import pytest +import pytest_asyncio +from fastmcp import Client + +from thinkingbox_tools import mcp_sandbox + +NOT_ISOLATED = ( + "Pyodide is not a privilege boundary; requires OS/container confinement. " + "See docs/sandbox_code_interpreter.md (Threat model)." +) + + +def _make_link(target, link_path, target_is_dir=False): + """Create a symlink, skipping the test where the OS forbids it.""" + try: + os.symlink(target, link_path, target_is_directory=target_is_dir) + except (OSError, NotImplementedError, AttributeError) as exc: + pytest.skip(f"cannot create symlinks on this host: {exc}") + + +# --------------------------------------------------------------------------- +# Workspace link handling +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def link_workspace(tmp_path_factory): + """A workspace containing a link that points outside of itself.""" + outside = tmp_path_factory.mktemp("outside_secrets") + (outside / "outside.txt").write_text("OUTSIDE_WORKSPACE_MARKER") + + workspace = tmp_path_factory.mktemp("workspace_with_link") + (workspace / "normal.txt").write_text("regular file") + _make_link(str(outside / "outside.txt"), str(workspace / "escaping_link.txt")) + + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(workspace)}} + ) + yield client, workspace, outside + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + +@pytest.mark.asyncio +async def test_escaping_link_not_listed(link_workspace): + """A link escaping the workspace must not be seeded into the session.""" + client, _workspace, _outside = link_workspace + result = await client.call_tool("list_sandbox_files", {"prefix": ""}) + files = result.structured_content["result"]["files"] + assert "normal.txt" in files, files + assert "escaping_link.txt" not in files, ( + f"link escaping the workspace was seeded into the session: {files}" + ) + + +@pytest.mark.asyncio +async def test_escaping_link_content_not_reachable(link_workspace): + """The content behind an escaping link must not be readable from the session.""" + _client, _workspace, _outside = link_workspace + session_dir = mcp_sandbox._session_dir + assert session_dir is not None + + leaked = [] + for dirpath, _dirs, names in os.walk(session_dir): + for name in names: + path = os.path.join(dirpath, name) + try: + with open(path, "rb") as handle: + if b"OUTSIDE_WORKSPACE_MARKER" in handle.read(): + leaked.append(path) + except OSError: + continue + assert not leaked, f"content from outside the workspace reachable at: {leaked}" + + +@pytest.mark.asyncio +async def test_internal_link_is_materialized(tmp_path_factory): + """A link pointing *inside* the workspace is kept, as a real copy.""" + workspace = tmp_path_factory.mktemp("workspace_internal_link") + (workspace / "real.txt").write_text("INTERNAL_CONTENT") + _make_link(str(workspace / "real.txt"), str(workspace / "alias.txt")) + + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(workspace)}} + ) + result = await client.call_tool("list_sandbox_files", {"prefix": ""}) + files = result.structured_content["result"]["files"] + assert "alias.txt" in files, f"in-workspace link was dropped: {files}" + + seeded = os.path.join(mcp_sandbox._session_dir, "alias.txt") + assert not os.path.islink(seeded), "in-workspace link was left followable" + with open(seeded, "rb") as handle: + assert b"INTERNAL_CONTENT" in handle.read() + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + +@pytest.mark.asyncio +async def test_linked_directory_is_rejected(tmp_path_factory): + """A linked directory in the workspace must not be traversed.""" + outside = tmp_path_factory.mktemp("outside_dir") + (outside / "hidden.txt").write_text("DIR_ESCAPE_MARKER") + + workspace = tmp_path_factory.mktemp("workspace_linked_dir") + (workspace / "keep.txt").write_text("keep") + _make_link(str(outside), str(workspace / "linked_dir"), target_is_dir=True) + + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(workspace)}} + ) + result = await client.call_tool("list_sandbox_files", {"prefix": ""}) + files = result.structured_content["result"]["files"] + assert "keep.txt" in files, files + assert not any("hidden.txt" in f for f in files), ( + f"linked directory was traversed: {files}" + ) + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + +def test_is_link_detects_reparse_points(tmp_path): + """_is_link must catch symlinks, and fail closed on unreadable entries.""" + plain = tmp_path / "plain.txt" + plain.write_text("x") + assert mcp_sandbox._is_link(str(plain)) is False + + link = tmp_path / "link.txt" + _make_link(str(plain), str(link)) + assert mcp_sandbox._is_link(str(link)) is True + + # Non-existent entries cannot be inspected, so they must be treated as unsafe. + assert mcp_sandbox._is_link(str(tmp_path / "missing")) is True + + +def test_resolves_inside_rejects_escapes(tmp_path): + root = tmp_path / "root" + (root / "sub").mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + + assert mcp_sandbox._resolves_inside(str(root / "sub"), str(root)) is True + assert mcp_sandbox._resolves_inside(str(outside), str(root)) is False + # A sibling sharing a name prefix must not be treated as inside. + sibling = tmp_path / "root_evil" + sibling.mkdir() + assert mcp_sandbox._resolves_inside(str(sibling), str(root)) is False + + +def _make_junction(target_dir, link_path): + """Create a Windows junction, skipping elsewhere. + + Junctions matter because they can be created without elevation, and + ``os.path.islink`` reports False for them — so a naive symlink check misses + them entirely. + """ + if sys.platform != "win32": + pytest.skip("junctions are Windows-only") + import subprocess + + result = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(link_path), str(target_dir)], + capture_output=True, + text=True, + ) + if result.returncode != 0: + pytest.skip(f"cannot create junction: {result.stderr.strip()}") + + +@pytest.mark.asyncio +async def test_windows_junction_is_rejected(tmp_path_factory): + """A junction pointing outside the workspace must not be traversed. + + Regression guard for the Windows-specific case: ``os.path.islink`` returns + False for junctions, so detection must use the reparse-point attribute. + """ + outside = tmp_path_factory.mktemp("outside_junction") + (outside / "hidden.txt").write_text("JUNCTION_ESCAPE_MARKER") + + workspace = tmp_path_factory.mktemp("workspace_junction") + (workspace / "keep.txt").write_text("keep") + _make_junction(outside, workspace / "junc") + + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(workspace)}} + ) + result = await client.call_tool("list_sandbox_files", {"prefix": ""}) + files = result.structured_content["result"]["files"] + assert "keep.txt" in files, files + assert not any("hidden.txt" in f for f in files), ( + f"junction was traversed into the session: {files}" + ) + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + +def test_is_link_detects_junctions(tmp_path): + """_is_link must detect junctions, which os.path.islink misses.""" + target = tmp_path / "target" + target.mkdir() + junction = tmp_path / "junc" + _make_junction(target, junction) + + assert os.path.islink(str(junction)) is False, ( + "precondition: os.path.islink is expected to miss junctions" + ) + assert mcp_sandbox._is_link(str(junction)) is True, ( + "junction not detected as a link — reparse-point check is not working" + ) + + +# --------------------------------------------------------------------------- +# Host capability audit +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def sandbox_client(tmp_path): + (tmp_path / "readme.txt").write_text("hello") + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(tmp_path)}} + ) + yield client + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + +async def _probe(client, code): + """Run a probe and return its repr'd result, or the surfaced error.""" + result = await client.call_tool("code_interpreter", {"code": code}) + sc = result.structured_content["result"] + if sc.get("status") == "error": + return f"tool-error:{sc.get('message')}" + return sc.get("result") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_node_process_global_unavailable(sandbox_client): + """`import js` must not expose the Node `process` global.""" + code = ( + "try:\n" + " import js\n" + " _r = 'ABSENT' if getattr(js, 'process', None) is None else 'REACHABLE'\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_privileged_pyodide_api_unavailable(sandbox_client): + """Pyodide's internal `_api` must not be reachable from user code.""" + code = ( + "try:\n" + " import pyodide_js\n" + " _r = 'REACHABLE' if hasattr(pyodide_js, '_api') else 'ABSENT'\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_js_function_constructor_unavailable(sandbox_client): + """Cached JsProxy references must not yield the JS Function constructor. + + This is the route that survives `jsglobals` restriction and module hiding, + because Function bodies evaluate in global scope. + """ + code = ( + "try:\n" + " import pyodide_js\n" + " _f = pyodide_js._api.loadBinaryFile.constructor.constructor\n" + " _r = 'REACHABLE' if _f is not None else 'ABSENT'\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_host_file_outside_session_unreadable(sandbox_client, tmp_path_factory): + """A host file outside the session dir must not be readable. + + Uses a sentinel this test creates; no real system file is touched. + """ + sentinel_dir = tmp_path_factory.mktemp("sentinel") + sentinel = sentinel_dir / "canary.txt" + sentinel.write_text("CANARY_MARKER_DO_NOT_LEAK") + as_posix = str(sentinel).replace("\\", "/") + + code = ( + "try:\n" + " import pyodide_js\n" + f" _buf = await pyodide_js._api.loadBinaryFile({as_posix!r})\n" + " _r = 'REACHABLE' if b'CANARY_MARKER' in bytes(_buf.to_py()) else 'ABSENT'\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_environment_variables_unreadable(sandbox_client): + """The parent process environment must not be readable from user code.""" + code = ( + "try:\n" + " import pyodide_js\n" + " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" + " _n = _F(\"return typeof process==='undefined' ? 0 : Object.keys(process.env).length\")()\n" + " _r = 'ABSENT' if int(_n) == 0 else 'REACHABLE'\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") + + +@pytest.mark.asyncio +@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) +async def test_node_builtin_modules_unreachable(sandbox_client): + """node:fs and node:child_process must not be resolvable. + + Resolution only — no file is read and no process is spawned. + """ + code = ( + "try:\n" + " import pyodide_js\n" + " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" + " _fn = _F(\"return import('node:child_process')" + ".then(m => typeof m.execSync === 'function' ? 'REACHABLE' : 'ABSENT')" + ".catch(() => 'ABSENT')\")\n" + " _r = await _fn()\n" + "except Exception:\n" + " _r = 'ABSENT'\n" + "_r" + ) + assert await _probe(sandbox_client, code) == repr("ABSENT") diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py index 0c0ff0d..0b590dc 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -3,6 +3,8 @@ import os import shutil +import stat +import sys import tempfile import traceback from typing import Annotated, Literal, Union @@ -16,6 +18,17 @@ ) from thinkingbox_tools.toolslib.sandbox.sandbox import Sandbox +# SECURITY: Pyodide is NOT a security boundary. It provides memory safety via +# WASM, but deliberately exposes a Python<->JavaScript FFI, and Python code can +# reach the Node host through it (`import js`, cached JsProxy references, and +# the Function constructor, which evaluates in global scope and therefore +# survives jsglobals restriction or module hiding). Host filesystem access, +# process execution and environment variables are all reachable. +# +# Run only trusted, first-party agent code here. Do not route untrusted or +# third-party input to this server until the worker is confined by an OS/ +# container boundary. See docs/sandbox_code_interpreter.md ("Threat model"). + mcp = FastMCP("sandbox") _sandbox: Sandbox | None = None @@ -43,6 +56,33 @@ class ErrorResult(BaseModel): message: str +def _is_link(path: str) -> bool: + """True for symlinks and, on Windows, junctions and other reparse points. + + ``os.path.islink`` returns False for Windows junctions, so the reparse-point + attribute is checked explicitly. An unreadable entry is reported as a link + so callers fail closed rather than following something they can't inspect. + """ + try: + st = os.lstat(path) + except OSError: + return True + if stat.S_ISLNK(st.st_mode): + return True + reparse = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) + return bool(getattr(st, "st_file_attributes", 0) & reparse) + + +def _resolves_inside(path: str, root: str) -> bool: + """True when ``path`` fully resolves to a location at or under ``root``.""" + try: + real_root = os.path.realpath(root) + real_path = os.path.realpath(path) + except OSError: + return False + return real_path == real_root or real_path.startswith(real_root + os.sep) + + def _symlink_or_copy(src: str, dst: str) -> None: """Symlink dst → src; fall back to a real copy if symlinks aren't supported.""" try: @@ -71,9 +111,48 @@ async def initialize(config: dict): shutil.rmtree(_session_dir, ignore_errors=True) _session_dir = tempfile.mkdtemp(prefix="sandbox_session_") if workspace_dir and os.path.isdir(workspace_dir): + workspace_root = os.path.abspath(workspace_dir) + rejected: list[str] = [] + + def _seed(src: str, dst: str) -> None: + # A link in the *source* workspace would otherwise be re-pointed at + # its own target, and NODEFS reads follow host links transparently — + # so a link escaping workspace_dir would be readable from inside the + # sandbox. In-workspace links are materialized as real copies so + # legitimate data is preserved without keeping a followable link. + if _is_link(src): + if _resolves_inside(src, workspace_root): + shutil.copy2(src, dst, follow_symlinks=True) + else: + rejected.append(src) + return + _symlink_or_copy(src, dst) + + def _ignore(dirpath: str, names: list[str]) -> set[str]: + # Linked directories are rejected outright: following them risks both + # traversal outside the workspace and copytree recursion loops. + drop = set() + for name in names: + entry = os.path.join(dirpath, name) + if os.path.isdir(entry) and _is_link(entry): + rejected.append(entry) + drop.add(name) + return drop + shutil.copytree( - workspace_dir, _session_dir, copy_function=_symlink_or_copy, dirs_exist_ok=True + workspace_dir, + _session_dir, + copy_function=_seed, + ignore=_ignore, + dirs_exist_ok=True, ) + if rejected: + print( + f"[mcp_sandbox] refused to seed {len(rejected)} link(s) that " + f"escape the workspace: {', '.join(sorted(rejected)[:5])}" + + (" ..." if len(rejected) > 5 else ""), + file=sys.stderr, + ) _sandbox = Sandbox(_session_dir) if _interpreter is not None: @@ -144,7 +223,7 @@ async def search_files( @mcp.tool( name="code_interpreter", description=( - "Execute Python code in a sandboxed Pyodide (CPython-in-WebAssembly) interpreter. " + "Execute Python code in a Pyodide (CPython-in-WebAssembly) interpreter. " "Workspace files are accessible at /workspace/ using standard Python file I/O. " "Pre-installed: numpy, pandas, beautifulsoup4, jinja2, sympy, altair, mpmath, lxml, " "Pillow, openpyxl, xlsxwriter, markdownify, mammoth, pypdf, pdfminer.six, tabulate, " From a4d3eecf0176537cfbfcfb06f1a38ebd896375d8 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 13:51:11 -0700 Subject: [PATCH 06/21] Harden sandbox: fail closed, allowlist worker env, fix reparse detection None of this is isolation. Pyodide remains reachable from agent code (see docs "Threat model"); these changes narrow the blast radius and stop the unconfined default, they do not create a boundary. Fail closed. CodeInterpreter._start now refuses to spawn a worker unless THINKINGBOX_SANDBOX_ALLOW_UNCONFINED is set, so running agent code with the MCP server's privileges is a deliberate, auditable choice instead of the default. A documentation warning is not a control. servers.yaml sets the variable explicitly so the decision is visible in configuration, and the test suites opt in through an autouse fixture rather than the product defaulting to permissive. Allowlisted worker environment. The worker was inheriting the parent environment wholesale (76 variables were readable from agent code). It is now spawned with PATH plus a few operational variables. Defense-in-depth: an escape no longer automatically yields exported secrets, but it is still an escape. Reparse detection corrected. The previous check treated *any* reparse point as a link, which would have rejected OneDrive / Files On-Demand placeholders and so any OneDrive-backed workspace. Detection now follows the Win32 IsReparseTagNameSurrogate rule (bit 29): junctions and symlinks name another location and are traversal risks; cloud placeholders, dedup and WIM/container mappings are the same file with different backing storage and are ordinary files. Unreadable entries still fail closed. Capability tests reworked. They were blanket xfail(strict=True), which would have absorbed a broken harness as an expected failure. Each probe now distinguishes three outcomes: a probe that cannot run fails the suite loudly, a confirmed reachable capability xfails at runtime with a per-capability reason, and an absent capability simply passes -- so no markers need removing once the worker is confined. Verified by fault injection: a malformed probe result produces FAILED, not XFAIL. node:fs and node:child_process are now tested separately rather than as one combined probe. Adds tests for the fail-closed gate, the environment allowlist, name-surrogate semantics across six real reparse tags, and a simulated cloud placeholder. Removes the unused fixture unpacking flagged by CodeQL by yielding a namespace instead of a tuple. Windows is documented as unsupported rather than claimed: micropip mishandles the file:///C:/... URLs used for vendored wheels, so the worker does not start there without a workaround. The reparse-point logic is unit-tested, but there is no Windows CI job, so Windows should not be treated as supported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 52 +++- servers/servers.yaml | 10 +- .../tests/test_sandbox_isolation.py | 289 +++++++++++++++--- .../tests/test_sandbox_server.py | 12 + .../thinkingbox_tools/mcp_sandbox.py | 36 ++- .../toolslib/sandbox/code_interpreter.py | 65 ++++ 6 files changed, 401 insertions(+), 63 deletions(-) diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index 2c49ef4..de14ed1 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -42,7 +42,16 @@ is not a substitute for an OS boundary. - Only trusted, first-party agent code may be executed. - The workspace must be treated as trusted input. - Secrets must not be present in the environment of the MCP server process, - because the worker inherits it. + because the worker is spawned from it. + +**Controls currently in place.** These narrow the blast radius. None of them +makes the boundary real, and none should be described as isolation: + +| Control | Effect | +| ---- | ---- | +| Fail-closed startup | `CodeInterpreter` refuses to spawn a worker unless `THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1` is set, so unconfined execution is a deliberate, auditable choice rather than the default. | +| Allowlisted worker environment | The worker is spawned with a minimal environment (`PATH` and a few operational variables) instead of inheriting the parent's. An escape therefore does not automatically hand over exported secrets. | +| Workspace link rejection | Links escaping `workspace_dir` are not seeded into the session (see [below](#links-in-the-source-workspace)). | **Required to lift these constraints:** confine the worker with an owner-approved OS/container boundary — no host filesystem beyond the session @@ -51,6 +60,12 @@ pid/memory/CPU limits. The NODEFS copy-on-write layer described below is a *correctness* mechanism for workspace isolation, not a security control, and does not mitigate any of the above. +**Regression coverage.** `tests/test_sandbox_isolation.py` probes each of the +capabilities above. They are not blanket-`xfail`ed: a probe that cannot run +(worker fails to start, malformed result) fails the suite loudly, and only a +*confirmed* reachable capability is recorded as an expected failure. When the +worker is confined, those tests simply start passing. + --- ## Architecture @@ -282,16 +297,24 @@ teardown. Seeding must not blindly re-point a link that already exists in `workspace_dir`. NODEFS reads follow host links transparently, so a link whose target sits outside the workspace would be readable from inside `/workspace`. -Windows junctions matter here too: `os.path.islink` returns `False` for them, -so the reparse-point attribute is checked explicitly, and an entry that cannot -be `lstat`'d is treated as a link so the check fails closed. + +Windows needs care here. `os.path.islink` returns `False` for junctions, so the +reparse point is inspected directly — but only *name surrogate* tags count. +Those are the tags that name another filesystem location (junctions, symlinks), +which is what makes them a traversal risk. Non-surrogate reparse points describe +alternate backing storage for the same file — OneDrive / Files On-Demand +placeholders, deduplication, WIM and container mappings — and must be treated as +ordinary files, otherwise a OneDrive-backed workspace would be rejected wholesale. +This mirrors the Win32 `IsReparseTagNameSurrogate` macro (bit 29 of the tag). An +entry that cannot be `lstat`'d is treated as a link so the check fails closed. | Entry in `workspace_dir` | Seeding behavior | | ---- | ---- | | Regular file | Symlinked into the session dir (the O(files) fast path). | +| Non-surrogate reparse point (OneDrive placeholder, dedup) | Treated as a regular file. | | Link whose target resolves **inside** `workspace_dir` | Materialized as a real copy — data preserved, no followable link. | | Link whose target resolves **outside** `workspace_dir` | **Rejected**, reported on stderr. | -| Linked directory | **Rejected** — avoids both traversal and `copytree` recursion loops. | +| Linked directory (junction or symlink) | **Rejected** — avoids both traversal and `copytree` recursion loops. | Note this is workspace hygiene, not a privilege boundary: a caller who can already execute code in the worker does not need a link to reach host files @@ -356,6 +379,25 @@ re-running keeps existing wheels; force a refresh by deleting `wheels/`. If PyPI is unreachable, missing wheels fall back to runtime fetch (slower startup, still works). +**Enabling execution.** The interpreter fails closed: it refuses to spawn a +worker unless the operator opts in. + +```bash +export THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 +``` + +Set this only where the executed code is trusted and first-party, and keep +secrets out of the environment of the process that launches the server. The +entry in `servers/servers.yaml` sets it explicitly so the choice is visible in +configuration rather than implied. + +**Supported platforms.** CI covers Linux (`ubuntu-latest`) only, and that is +the supported platform. The code paths are cross-platform and the Windows +reparse-point handling above is unit-tested, but Windows is not exercised end +to end in CI: `micropip` mishandles the `file:///C:/...` URLs used for vendored +wheels, so the worker does not start there without a workaround. Windows should +not be treated as supported unless a Windows CI job is added. + **Cost.** Cold start ~5–10 s with cached wheels, paid once per session. Memory ~250–400 MB resident per worker — each concurrent session needs its own. Per-call overhead is low tens of ms for pure-Python code with diff --git a/servers/servers.yaml b/servers/servers.yaml index d674621..01a6f63 100644 --- a/servers/servers.yaml +++ b/servers/servers.yaml @@ -13,10 +13,18 @@ servers: online_banking: type: mcp-process command: ["{python}", "-m", "thinkingbox_tools.mcp_online_banking"] - # Sandbox code interpreter (Pyodide-based) + # Sandbox code interpreter (Pyodide-based). + # + # SECURITY: Pyodide is not a privilege boundary. Code executed here can reach + # the Node host, the host filesystem, process execution, and this process's + # environment. The server refuses to start without the opt-in below. Set it + # only where the executed code is trusted and first-party, and keep secrets + # out of this process's environment. See docs/sandbox_code_interpreter.md. sandbox: type: mcp-process command: ["{python}", "-m", "thinkingbox_tools.mcp_sandbox"] + env: + THINKINGBOX_SANDBOX_ALLOW_UNCONFINED: "1" # TB BUSINESS OPS SERVERS 202606 sandbox_external_retail: diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 0ef6fea..a242af5 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -22,13 +22,16 @@ """ import os +import subprocess import sys +from types import SimpleNamespace import pytest import pytest_asyncio from fastmcp import Client from thinkingbox_tools import mcp_sandbox +from thinkingbox_tools.toolslib.sandbox import code_interpreter NOT_ISOLATED = ( "Pyodide is not a privilege boundary; requires OS/container confinement. " @@ -36,6 +39,16 @@ ) +@pytest.fixture(autouse=True) +def _allow_unconfined_worker(monkeypatch): + """Opt in to unconfined execution for the duration of the tests. + + The interpreter fails closed without this, which is the point of the gate. + Tests must opt in explicitly rather than the production default being lax. + """ + monkeypatch.setenv(code_interpreter.UNCONFINED_OPT_IN_ENV, "1") + + def _make_link(target, link_path, target_is_dir=False): """Create a symlink, skipping the test where the OS forbids it.""" try: @@ -69,7 +82,9 @@ async def link_workspace(tmp_path_factory): await client.call_tool( "__reserved__init", {"config": {"workspace_dir": str(workspace)}} ) - yield client, workspace, outside + # A namespace rather than a tuple so each test references only the + # attributes it needs, instead of unpacking values it discards. + yield SimpleNamespace(client=client, workspace=workspace, outside=outside) await client.call_tool("__reserved__teardown", {}) finally: ( @@ -82,8 +97,7 @@ async def link_workspace(tmp_path_factory): @pytest.mark.asyncio async def test_escaping_link_not_listed(link_workspace): """A link escaping the workspace must not be seeded into the session.""" - client, _workspace, _outside = link_workspace - result = await client.call_tool("list_sandbox_files", {"prefix": ""}) + result = await link_workspace.client.call_tool("list_sandbox_files", {"prefix": ""}) files = result.structured_content["result"]["files"] assert "normal.txt" in files, files assert "escaping_link.txt" not in files, ( @@ -94,7 +108,7 @@ async def test_escaping_link_not_listed(link_workspace): @pytest.mark.asyncio async def test_escaping_link_content_not_reachable(link_workspace): """The content behind an escaping link must not be readable from the session.""" - _client, _workspace, _outside = link_workspace + assert link_workspace.client is not None session_dir = mcp_sandbox._session_dir assert session_dir is not None @@ -217,7 +231,6 @@ def _make_junction(target_dir, link_path): """ if sys.platform != "win32": pytest.skip("junctions are Windows-only") - import subprocess result = subprocess.run( ["cmd", "/c", "mklink", "/J", str(link_path), str(target_dir)], @@ -282,9 +295,134 @@ def test_is_link_detects_junctions(tmp_path): ) +def test_name_surrogate_semantics(): + """Only name-surrogate reparse tags count as links. + + Junctions and symlinks name another location and are traversal risks. + Cloud placeholders (OneDrive / Files On-Demand), deduplication and + container mappings are the *same* file with different backing storage — + treating those as links would reject an ordinary OneDrive-backed workspace. + + This pins the Win32 IsReparseTagNameSurrogate rule (bit 29 of the tag). + """ + bit = mcp_sandbox._IO_REPARSE_TAG_NAME_SURROGATE_BIT + + surrogates = { + "IO_REPARSE_TAG_MOUNT_POINT": 0xA0000003, + "IO_REPARSE_TAG_SYMLINK": 0xA000000C, + } + non_surrogates = { + "IO_REPARSE_TAG_CLOUD": 0x9000001A, + "IO_REPARSE_TAG_CLOUD_1": 0x9000101A, + "IO_REPARSE_TAG_CLOUD_7": 0x9000701A, + "IO_REPARSE_TAG_DEDUP": 0x80000013, + "IO_REPARSE_TAG_WCI": 0x80000018, + "IO_REPARSE_TAG_APPEXECLINK": 0x8000001B, + } + + for name, tag in surrogates.items(): + assert tag & bit, f"{name} must be treated as a link" + for name, tag in non_surrogates.items(): + assert not (tag & bit), ( + f"{name} must NOT be treated as a link — doing so would reject " + "ordinary files such as OneDrive placeholders" + ) + + +def test_non_surrogate_reparse_point_is_not_a_link(tmp_path, monkeypatch): + """A non-surrogate reparse point must be treated as an ordinary file. + + Simulates an OneDrive-style placeholder by reporting the reparse attribute + together with a cloud tag, since such a file cannot be created on demand. + """ + plain = tmp_path / "cloud_placeholder.txt" + plain.write_text("content") + + reparse_attr = getattr(mcp_sandbox.stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0x400) + real_lstat = os.lstat + + class _CloudStat: + def __init__(self, base): + self._base = base + self.st_file_attributes = reparse_attr + self.st_reparse_tag = 0x9000001A # IO_REPARSE_TAG_CLOUD + + def __getattr__(self, item): + return getattr(self._base, item) + + def fake_lstat(path, *args, **kwargs): + if str(path) == str(plain): + return _CloudStat(real_lstat(plain)) + return real_lstat(path, *args, **kwargs) + + monkeypatch.setattr(mcp_sandbox.os, "lstat", fake_lstat) + assert mcp_sandbox._is_link(str(plain)) is False, ( + "a cloud placeholder was treated as a link; OneDrive-backed workspaces " + "would be rejected" + ) + + +# --------------------------------------------------------------------------- +# Fail-closed gate +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_interpreter_refuses_to_start_without_opt_in(monkeypatch, tmp_path): + """Without the opt-in the interpreter must refuse to spawn a worker.""" + monkeypatch.delenv(code_interpreter.UNCONFINED_OPT_IN_ENV, raising=False) + interp = code_interpreter.CodeInterpreter(timeout=5.0, workspace_dir=str(tmp_path)) + with pytest.raises(code_interpreter.CodeInterpreterError) as excinfo: + await interp.execute("1 + 1") + message = str(excinfo.value) + assert code_interpreter.UNCONFINED_OPT_IN_ENV in message + assert "not a security boundary" in message.lower() + + +@pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes"]) +def test_opt_in_accepts_truthy_values(monkeypatch, value): + monkeypatch.setenv(code_interpreter.UNCONFINED_OPT_IN_ENV, value) + assert code_interpreter._unconfined_allowed() is True + + +@pytest.mark.parametrize("value", ["", "0", "false", "no", "maybe"]) +def test_opt_in_rejects_other_values(monkeypatch, value): + monkeypatch.setenv(code_interpreter.UNCONFINED_OPT_IN_ENV, value) + assert code_interpreter._unconfined_allowed() is False + + +def test_worker_env_is_allowlisted(monkeypatch): + """The worker must not inherit the parent environment wholesale.""" + monkeypatch.setenv("SANDBOX_TEST_FAKE_SECRET", "super-secret-value") + monkeypatch.setenv("PATH", os.environ.get("PATH", "")) + + env = code_interpreter._minimal_env() + assert "SANDBOX_TEST_FAKE_SECRET" not in env, ( + "an unrelated parent variable leaked into the worker environment" + ) + assert "PATH" in env, "PATH is required to locate the node binary" + for name in env: + assert ( + name in code_interpreter._ENV_ALLOWLIST or name in {"TMPDIR", "TEMP", "TMP"} + ), f"{name} is not on the allowlist" + + # --------------------------------------------------------------------------- # Host capability audit # --------------------------------------------------------------------------- +# +# These probe capabilities that MUST NOT be available to agent code. They are +# not decorated with xfail. Instead each probe distinguishes three outcomes: +# +# * harness broke (worker didn't start, probe malformed) -> FAIL, loudly. +# A blanket xfail would swallow this and hide a broken suite. +# * capability confirmed reachable -> XFAIL at runtime, +# recording the known gap (see docs "Threat model"). +# * capability absent -> PASS, which is what +# happens once the worker is confined. No marker needs removing. +# +# Probes measure reachability only: they use a sentinel this test creates, never +# a real system file, and never execute a command or open a socket. @pytest_asyncio.fixture @@ -310,114 +448,165 @@ async def sandbox_client(tmp_path): ) = original -async def _probe(client, code): - """Run a probe and return its repr'd result, or the surfaced error.""" +async def _capability_probe(client, code): + """Run a probe expected to return exactly 'REACHABLE' or 'ABSENT'. + + Any other outcome means the harness itself is broken, which fails the test + rather than being silently absorbed as an expected failure. + """ result = await client.call_tool("code_interpreter", {"code": code}) sc = result.structured_content["result"] + if sc.get("status") == "error": - return f"tool-error:{sc.get('message')}" - return sc.get("result") + pytest.fail(f"code_interpreter tool failed, probe inconclusive: {sc.get('message')}") + if sc.get("error"): + pytest.fail(f"probe raised inside the interpreter, inconclusive:\n{sc['error']}") + + value = sc.get("result") + if value == repr("REACHABLE"): + return "REACHABLE" + if value == repr("ABSENT"): + return "ABSENT" + pytest.fail( + "probe returned an unexpected value, so the harness is not measuring " + f"what it claims. got {value!r}, stdout={sc.get('stdout')!r}" + ) + + +def _record(capability, outcome, detail): + """XFAIL on a confirmed gap; pass when the capability is genuinely gone.""" + if outcome == "REACHABLE": + pytest.xfail(f"{capability} is reachable from agent code. {NOT_ISOLATED} ({detail})") + assert outcome == "ABSENT" @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) async def test_node_process_global_unavailable(sandbox_client): """`import js` must not expose the Node `process` global.""" - code = ( + outcome = await _capability_probe( + sandbox_client, "try:\n" " import js\n" " _r = 'ABSENT' if getattr(js, 'process', None) is None else 'REACHABLE'\n" - "except Exception:\n" + "except ImportError:\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("the Node process global via `import js`", outcome, "js.process") @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) async def test_privileged_pyodide_api_unavailable(sandbox_client): """Pyodide's internal `_api` must not be reachable from user code.""" - code = ( + outcome = await _capability_probe( + sandbox_client, "try:\n" " import pyodide_js\n" " _r = 'REACHABLE' if hasattr(pyodide_js, '_api') else 'ABSENT'\n" - "except Exception:\n" + "except ImportError:\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("the privileged Pyodide internal API", outcome, "pyodide_js._api") @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) async def test_js_function_constructor_unavailable(sandbox_client): """Cached JsProxy references must not yield the JS Function constructor. This is the route that survives `jsglobals` restriction and module hiding, - because Function bodies evaluate in global scope. + because Function bodies evaluate in global scope rather than in the + restricted globals object. """ - code = ( + outcome = await _capability_probe( + sandbox_client, "try:\n" " import pyodide_js\n" " _f = pyodide_js._api.loadBinaryFile.constructor.constructor\n" " _r = 'REACHABLE' if _f is not None else 'ABSENT'\n" - "except Exception:\n" + "except (ImportError, AttributeError):\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("the JS Function constructor via a cached JsProxy", outcome, "constructor.constructor") @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) async def test_host_file_outside_session_unreadable(sandbox_client, tmp_path_factory): - """A host file outside the session dir must not be readable. + """A host file outside the session directory must not be readable. - Uses a sentinel this test creates; no real system file is touched. + Uses a sentinel this test creates; no real system or secret file is touched. """ - sentinel_dir = tmp_path_factory.mktemp("sentinel") - sentinel = sentinel_dir / "canary.txt" + sentinel = tmp_path_factory.mktemp("sentinel") / "canary.txt" sentinel.write_text("CANARY_MARKER_DO_NOT_LEAK") as_posix = str(sentinel).replace("\\", "/") - code = ( + outcome = await _capability_probe( + sandbox_client, "try:\n" " import pyodide_js\n" f" _buf = await pyodide_js._api.loadBinaryFile({as_posix!r})\n" " _r = 'REACHABLE' if b'CANARY_MARKER' in bytes(_buf.to_py()) else 'ABSENT'\n" - "except Exception:\n" + "except (ImportError, AttributeError, OSError, Exception):\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("host files outside the session directory", outcome, "read of a sentinel file") @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) async def test_environment_variables_unreadable(sandbox_client): - """The parent process environment must not be readable from user code.""" - code = ( + """The parent process environment must not be readable from user code. + + The worker is spawned with an allowlisted environment, so a small number of + operational variables (PATH and friends) are expected to remain visible. + This asserts that the *inherited* environment is not exposed wholesale. + """ + outcome = await _capability_probe( + sandbox_client, "try:\n" " import pyodide_js\n" " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" - " _n = _F(\"return typeof process==='undefined' ? 0 : Object.keys(process.env).length\")()\n" - " _r = 'ABSENT' if int(_n) == 0 else 'REACHABLE'\n" - "except Exception:\n" + " _n = int(_F(\"return typeof process==='undefined' ? 0 :" + ' Object.keys(process.env).length")())\n' + " _r = 'ABSENT' if _n == 0 else 'REACHABLE'\n" + "except (ImportError, AttributeError):\n" + " _r = 'ABSENT'\n" + "_r", + ) + _record("the worker process environment", outcome, "process.env") + + +@pytest.mark.asyncio +async def test_node_filesystem_module_unreachable(sandbox_client): + """`node:fs` must not be resolvable from agent code. + + Resolution only: nothing is read or written through the module. + """ + outcome = await _capability_probe( + sandbox_client, + "try:\n" + " import pyodide_js\n" + " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" + " _fn = _F(\"return import('node:fs')" + ".then(m => typeof m.readFileSync === 'function' ? 'REACHABLE' : 'ABSENT')" + ".catch(() => 'ABSENT')\")\n" + " _r = await _fn()\n" + "except (ImportError, AttributeError):\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("the node:fs module", outcome, "dynamic import('node:fs')") @pytest.mark.asyncio -@pytest.mark.xfail(strict=True, reason=NOT_ISOLATED) -async def test_node_builtin_modules_unreachable(sandbox_client): - """node:fs and node:child_process must not be resolvable. +async def test_node_process_module_unreachable(sandbox_client): + """`node:child_process` must not be resolvable from agent code. - Resolution only — no file is read and no process is spawned. + Resolution only: no process is ever spawned by this test. """ - code = ( + outcome = await _capability_probe( + sandbox_client, "try:\n" " import pyodide_js\n" " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" @@ -425,8 +614,8 @@ async def test_node_builtin_modules_unreachable(sandbox_client): ".then(m => typeof m.execSync === 'function' ? 'REACHABLE' : 'ABSENT')" ".catch(() => 'ABSENT')\")\n" " _r = await _fn()\n" - "except Exception:\n" + "except (ImportError, AttributeError):\n" " _r = 'ABSENT'\n" - "_r" + "_r", ) - assert await _probe(sandbox_client, code) == repr("ABSENT") + _record("the node:child_process module", outcome, "dynamic import('node:child_process')") diff --git a/servers/thinkingbox_tools/tests/test_sandbox_server.py b/servers/thinkingbox_tools/tests/test_sandbox_server.py index 6630895..3060332 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_server.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_server.py @@ -10,6 +10,7 @@ from fastmcp import Client from thinkingbox_tools import mcp_sandbox +from thinkingbox_tools.toolslib.sandbox import code_interpreter # --------------------------------------------------------------------------- # Fixture @@ -22,6 +23,17 @@ # --------------------------------------------------------------------------- +@pytest.fixture(autouse=True) +def _allow_unconfined_worker(monkeypatch): + """Opt in to unconfined execution for these tests. + + The interpreter fails closed without this (Pyodide is not a privilege + boundary), so the test suite must opt in explicitly rather than the + production default being permissive. + """ + monkeypatch.setenv(code_interpreter.UNCONFINED_OPT_IN_ENV, "1") + + @pytest_asyncio.fixture async def sandbox_client(tmp_path): """Provide a sandbox session backed by a temporary workspace with known test files.""" diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py index 0b590dc..7bb206b 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -31,6 +31,11 @@ mcp = FastMCP("sandbox") +# Win32 IsReparseTagNameSurrogate: bit 29 marks reparse tags that name another +# filesystem location (junctions, symlinks) as opposed to tags describing +# alternate backing storage for the same file (OneDrive placeholders, dedup). +_IO_REPARSE_TAG_NAME_SURROGATE_BIT = 0x20000000 + _sandbox: Sandbox | None = None _interpreter: CodeInterpreter | None = None _session_dir: str | None = ( @@ -57,11 +62,20 @@ class ErrorResult(BaseModel): def _is_link(path: str) -> bool: - """True for symlinks and, on Windows, junctions and other reparse points. - - ``os.path.islink`` returns False for Windows junctions, so the reparse-point - attribute is checked explicitly. An unreadable entry is reported as a link - so callers fail closed rather than following something they can't inspect. + """True for symlinks and for Windows reparse points that redirect by name. + + ``os.path.islink`` returns False for Windows junctions, so reparse points are + inspected explicitly. Only *name surrogates* count: those are the reparse + tags that name another filesystem location (junctions, symlinks, mount + points) and are therefore traversal risks. Non-surrogate reparse points + describe alternate backing storage for the same file -- OneDrive / Files + On-Demand placeholders, deduplication, WIM/container mappings -- and must be + treated as ordinary files, or a user with OneDrive-backed files would have + their whole workspace rejected. + + This mirrors the Win32 ``IsReparseTagNameSurrogate`` macro, which tests bit + 29 of the tag. An entry that cannot be inspected is reported as a link so + callers fail closed. """ try: st = os.lstat(path) @@ -69,8 +83,16 @@ def _is_link(path: str) -> bool: return True if stat.S_ISLNK(st.st_mode): return True - reparse = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) - return bool(getattr(st, "st_file_attributes", 0) & reparse) + + reparse_attr = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) + if not (getattr(st, "st_file_attributes", 0) & reparse_attr): + return False + + tag = getattr(st, "st_reparse_tag", 0) + if not tag: + # Reparse point whose tag we can't read: fail closed. + return True + return bool(tag & _IO_REPARSE_TAG_NAME_SURROGATE_BIT) def _resolves_inside(path: str, root: str) -> bool: diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index 4a4c16d..a5fccda 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -3,9 +3,56 @@ import asyncio import json +import os from dataclasses import asdict, dataclass from pathlib import Path +# Opt-in required to run the worker unconfined. Pyodide is not a privilege +# boundary (see docs/sandbox_code_interpreter.md "Threat model"), so the default +# is to refuse to start rather than to silently execute agent code with the +# permissions of the MCP server process. +UNCONFINED_OPT_IN_ENV = "THINKINGBOX_SANDBOX_ALLOW_UNCONFINED" + +# Passed through to the worker when present. Everything else in the parent +# environment is withheld: the worker inherits the MCP server's environment +# otherwise, and agent code can read all of it. This narrows the blast radius +# of an escape; it is NOT isolation, and does not stop an escape from happening. +_ENV_ALLOWLIST = ( + "PATH", # required to locate the node binary + "SystemRoot", # Windows: required by the CRT / winsock + "SystemDrive", + "COMSPEC", + "NUMBER_OF_PROCESSORS", # libuv threadpool sizing + "LANG", + "LC_ALL", + "TZ", +) + + +def _minimal_env() -> dict[str, str]: + """Build the worker environment from an allowlist. + + Defense-in-depth only. A process that escapes Pyodide still runs with the + OS-level privileges of this user; withholding variables merely means the + escape does not hand over whatever secrets happened to be exported. + """ + env = {name: os.environ[name] for name in _ENV_ALLOWLIST if name in os.environ} + # Node reads TMPDIR/TEMP/TMP for os.tmpdir(); give it the session's own + # temp root rather than leaking the parent's. + for name in ("TMPDIR", "TEMP", "TMP"): + if name in os.environ: + env[name] = os.environ[name] + return env + + +def _unconfined_allowed() -> bool: + """True when the operator has explicitly opted in to unconfined execution.""" + return os.environ.get(UNCONFINED_OPT_IN_ENV, "").strip().lower() in { + "1", + "true", + "yes", + } + class CodeInterpreterError(Exception): pass @@ -116,6 +163,22 @@ async def _ensure_started(self) -> None: await self._start() async def _start(self) -> None: + # Fail closed. Pyodide does not confine agent code, so refuse to spawn + # an unconfined worker unless the operator has explicitly accepted that. + # A documentation warning is not a control; this is. + if not _unconfined_allowed(): + raise CodeInterpreterError( + "Refusing to start the code interpreter: it would run agent-supplied " + "Python unconfined.\n" + "Pyodide is NOT a security boundary -- code executed here can reach " + "the Node host, the host filesystem, process execution and this " + "process's environment variables.\n" + "Only enable this where the executed code is trusted and " + "first-party, by setting:\n" + f" {UNCONFINED_OPT_IN_ENV}=1\n" + "See docs/sandbox_code_interpreter.md ('Threat model')." + ) + worker_dir = self._worker_path.parent if not self._worker_path.exists(): raise CodeInterpreterError( @@ -148,6 +211,8 @@ async def _start(self) -> None: # Run from the worker's own directory so Node.js can resolve # the pyodide package in the sibling node_modules/ folder. cwd=str(self._worker_path.parent), + # Withhold the parent environment (defense-in-depth, not isolation). + env=_minimal_env(), ) # Wait for the { "ready": true } handshake before accepting requests. From ef7aa6323b016b46b2db121ab36c107c570a14f7 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 14:01:42 -0700 Subject: [PATCH 07/21] Avoid mixed implicit/explicit returns in the probe helper CodeQL flagged _capability_probe for mixing explicit returns with a fall- through path, because it cannot infer that pytest.fail() does not return. Restructured so the function has a single explicit return. Behavior is unchanged and re-verified: a probe that cannot run still fails the suite loudly rather than being absorbed as an expected failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/test_sandbox_isolation.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index a242af5..a45e3d0 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -463,14 +463,12 @@ async def _capability_probe(client, code): pytest.fail(f"probe raised inside the interpreter, inconclusive:\n{sc['error']}") value = sc.get("result") - if value == repr("REACHABLE"): - return "REACHABLE" - if value == repr("ABSENT"): - return "ABSENT" - pytest.fail( - "probe returned an unexpected value, so the harness is not measuring " - f"what it claims. got {value!r}, stdout={sc.get('stdout')!r}" - ) + if value not in (repr("REACHABLE"), repr("ABSENT")): + pytest.fail( + "probe returned an unexpected value, so the harness is not measuring " + f"what it claims. got {value!r}, stdout={sc.get('stdout')!r}" + ) + return "REACHABLE" if value == repr("REACHABLE") else "ABSENT" def _record(capability, outcome, detail): From 66c6598af338201feb871271531749246602084a Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 15:44:42 -0700 Subject: [PATCH 08/21] Fix gate bypass in shipped config and unfalsifiable capability probes Six corrections from review, all of which were defects introduced by the previous hardening commit rather than pre-existing issues. servers.yaml no longer sets THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1. That file is the documented normal startup path, so shipping the opt-in in it made the unsafe mode the default and defeated the fail-closed gate entirely. Operators must now export it in the environment that launches the server. Verified both ways: without it the interpreter refuses to start, with it execution proceeds. Probes no longer swallow their own failures. One caught bare `Exception`, so any error at all -- a dead worker, a typo, a loader fault -- was recorded as 'ABSENT' and reported as confinement. The dynamic imports also used `.catch(() => 'ABSENT')`, which did the same for any promise rejection. Probes now catch only the specific errors a confining policy would raise (ImportError, AttributeError, PermissionError); anything else propagates and fails the test. The environment probe was unfalsifiable. It asserted `len(process.env) == 0`, but the worker is deliberately given PATH, so the condition could never hold even under perfect confinement. It now exports a uniquely-named secret into the parent *before* the worker starts and checks whether that specific value is visible. This test passes today: the allowlist does keep the parent's secrets out of the worker, which the previous formulation could not show. Filesystem and process probes now assert on effects rather than on whether a module name resolves. The fs probe performs a test-owned write into a pytest temp directory and the host is then checked for the file; the process probe runs a node one-liner whose only effect is creating a marker file in a pytest temp directory. Both assert the host-side effect matches the reported outcome, so a probe that lies is caught. Nothing outside the temp directories is touched and no network is used. TMPDIR/TEMP/TMP now match their comment. The code claimed to give the worker its own temp root while actually copying the parent's values. CodeInterpreter now creates a directory it owns, points the worker at it, and removes it on close; when no directory is supplied the variables are omitted entirely. Stale claims removed: the docs said the full parent environment is readable (it is allowlisted now, though process.env itself still is), and the test module still described blanket xfail(strict=True) after that approach was replaced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 30 +-- servers/servers.yaml | 12 +- .../tests/test_sandbox_isolation.py | 196 ++++++++++++++---- .../thinkingbox_tools/mcp_sandbox.py | 4 +- .../toolslib/sandbox/code_interpreter.py | 32 ++- 5 files changed, 207 insertions(+), 67 deletions(-) diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index de14ed1..84a97d0 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -28,7 +28,9 @@ host is possible via several independent routes: - Any reachable `JsProxy` yields `.constructor.constructor` — the `Function` constructor — and `node:fs` / `node:child_process` are reachable from there via dynamic `import()`. -- The full parent environment (every variable in `process.env`) is readable. +- The full parent environment was readable. It is now allowlisted (see + "Controls currently in place" below), so an escape no longer yields whatever + secrets happened to be exported — but `process.env` itself remains readable. **In-process mitigations do not close this.** `jsglobals: {}` fails because `Function` bodies evaluate in the *global* scope, not the restricted object. @@ -49,8 +51,8 @@ makes the boundary real, and none should be described as isolation: | Control | Effect | | ---- | ---- | -| Fail-closed startup | `CodeInterpreter` refuses to spawn a worker unless `THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1` is set, so unconfined execution is a deliberate, auditable choice rather than the default. | -| Allowlisted worker environment | The worker is spawned with a minimal environment (`PATH` and a few operational variables) instead of inheriting the parent's. An escape therefore does not automatically hand over exported secrets. | +| Fail-closed startup | `CodeInterpreter` refuses to spawn a worker unless `THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1` is set **in the environment that launches the server**. It is deliberately not set in `servers/servers.yaml`, since baking it into the shipped config would make the unsafe mode the default. | +| Allowlisted worker environment | The worker is spawned with a minimal environment (`PATH` and a few operational variables) instead of inheriting the parent's, and with `TMPDIR`/`TEMP`/`TMP` pointing at a directory the interpreter owns and removes on close. An escape therefore does not hand over exported secrets. `process.env` itself is still readable. | | Workspace link rejection | Links escaping `workspace_dir` are not seeded into the session (see [below](#links-in-the-source-workspace)). | **Required to lift these constraints:** confine the worker with an @@ -61,10 +63,12 @@ pid/memory/CPU limits. The NODEFS copy-on-write layer described below is a does not mitigate any of the above. **Regression coverage.** `tests/test_sandbox_isolation.py` probes each of the -capabilities above. They are not blanket-`xfail`ed: a probe that cannot run -(worker fails to start, malformed result) fails the suite loudly, and only a -*confirmed* reachable capability is recorded as an expected failure. When the -worker is confined, those tests simply start passing. +capabilities above. The probes assert on observable effects — bytes read, a +file written, a uniquely-named secret retrieved — rather than on whether an API +name happens to exist, and they catch only the specific errors a confining +policy would raise. A probe that cannot run fails the suite loudly, only a +*confirmed* reachable capability is recorded as an expected failure, and once +the worker is confined those tests simply start passing. --- @@ -380,16 +384,18 @@ If PyPI is unreachable, missing wheels fall back to runtime fetch (slower startup, still works). **Enabling execution.** The interpreter fails closed: it refuses to spawn a -worker unless the operator opts in. +worker unless the operator opts in, in the environment that launches the +server. ```bash export THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 ``` -Set this only where the executed code is trusted and first-party, and keep -secrets out of the environment of the process that launches the server. The -entry in `servers/servers.yaml` sets it explicitly so the choice is visible in -configuration rather than implied. +This is deliberately **not** set in `servers/servers.yaml`. That file is the +documented normal startup path, so setting it there would make the unsafe mode +the default and defeat the gate. Export it only where the executed code is +trusted and first-party, and keep secrets out of that environment — the worker +is started from it. **Supported platforms.** CI covers Linux (`ubuntu-latest`) only, and that is the supported platform. The code paths are cross-platform and the Windows diff --git a/servers/servers.yaml b/servers/servers.yaml index 01a6f63..4fd6157 100644 --- a/servers/servers.yaml +++ b/servers/servers.yaml @@ -17,14 +17,16 @@ servers: # # SECURITY: Pyodide is not a privilege boundary. Code executed here can reach # the Node host, the host filesystem, process execution, and this process's - # environment. The server refuses to start without the opt-in below. Set it - # only where the executed code is trusted and first-party, and keep secrets - # out of this process's environment. See docs/sandbox_code_interpreter.md. + # environment. The server fails closed: it refuses to start unless the + # operator sets THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 in the *environment* + # that launches it. That opt-in is deliberately NOT set here -- baking it + # into the shipped config would make the unsafe mode the default and defeat + # the gate. Enable it only where the executed code is trusted and + # first-party, and keep secrets out of that environment. + # See docs/sandbox_code_interpreter.md ("Threat model"). sandbox: type: mcp-process command: ["{python}", "-m", "thinkingbox_tools.mcp_sandbox"] - env: - THINKINGBOX_SANDBOX_ALLOW_UNCONFINED: "1" # TB BUSINESS OPS SERVERS 202606 sandbox_external_retail: diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index a45e3d0..b1d11c7 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -10,20 +10,26 @@ are a genuine regression guard: they fail against the pre-fix seeding logic. 2. **Host capability audit** — asserts that host capabilities are unavailable to - agent-supplied Python. These are currently ``xfail(strict=True)``: Pyodide is - not a privilege boundary, so they genuinely fail today. They are recorded - here rather than omitted so the gap is visible in the suite, and ``strict`` - means that once OS/container confinement lands they will XPASS and force the - marker to be removed. See docs/sandbox_code_interpreter.md ("Threat model"). - -The capability probes measure *reachability only*. They never read a real -system or secret file — a sentinel the test itself creates is used instead — -and they never execute a command or open a network connection. + agent-supplied Python. They currently fail, because Pyodide is not a + privilege boundary. They are *not* blanket-``xfail``ed: each probe fails + loudly if it cannot run, records an expected failure only when a capability + is *confirmed* reachable, and simply passes once the capability is gone. + See docs/sandbox_code_interpreter.md ("Threat model"). + +The capability probes assert on observable effects (bytes read, a file written, +a secret retrieved) rather than on whether an API name exists, and they catch +only the specific errors a confining policy would raise. An unexpected worker, +import or loader error propagates and fails the test, so a broken probe is +never mistaken for confinement. Probes stay inside directories pytest created: +no real system or secret file is read, and nothing outside the temp directory +is modified. """ +import json import os import subprocess import sys +import uuid from types import SimpleNamespace import pytest @@ -402,9 +408,25 @@ def test_worker_env_is_allowlisted(monkeypatch): ) assert "PATH" in env, "PATH is required to locate the node binary" for name in env: - assert ( - name in code_interpreter._ENV_ALLOWLIST or name in {"TMPDIR", "TEMP", "TMP"} - ), f"{name} is not on the allowlist" + assert name in code_interpreter._ENV_ALLOWLIST, f"{name} is not on the allowlist" + + +def test_worker_tmp_is_not_inherited(monkeypatch, tmp_path): + """TMPDIR/TEMP/TMP must point at a worker-owned dir, not the parent's.""" + monkeypatch.setenv("TMPDIR", "/parent/tmp") + monkeypatch.setenv("TEMP", r"C:\parent\temp") + monkeypatch.setenv("TMP", r"C:\parent\temp") + + # Without a worker-owned directory the variables are omitted entirely. + env = code_interpreter._minimal_env() + for name in ("TMPDIR", "TEMP", "TMP"): + assert name not in env, f"{name} was forwarded from the parent" + + # With one, they point at it rather than at the parent's value. + worker_tmp = str(tmp_path / "worker_tmp") + env = code_interpreter._minimal_env(worker_tmp) + for name in ("TMPDIR", "TEMP", "TMP"): + assert env[name] == worker_tmp, f"{name} did not point at the worker dir" # --------------------------------------------------------------------------- @@ -448,6 +470,41 @@ async def sandbox_client(tmp_path): ) = original +@pytest_asyncio.fixture +async def sandbox_client_with_secret(tmp_path, monkeypatch): + """A session whose worker was started *after* a unique secret was exported. + + The secret must be in the parent environment before the worker is spawned, + otherwise the probe would pass for the wrong reason. + """ + secret_name = "SANDBOX_PARENT_SECRET_PROBE" + secret_value = f"parent-secret-{uuid.uuid4().hex}" + monkeypatch.setenv(secret_name, secret_value) + + (tmp_path / "readme.txt").write_text("hello") + original = ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) + try: + async with Client(mcp_sandbox.mcp) as client: + await client.call_tool( + "__reserved__init", {"config": {"workspace_dir": str(tmp_path)}} + ) + # Force worker startup now, so it inherits (or does not inherit) + # the secret exported above. + await client.call_tool("code_interpreter", {"code": "1"}) + yield SimpleNamespace(client=client, secret=(secret_name, secret_value)) + await client.call_tool("__reserved__teardown", {}) + finally: + ( + mcp_sandbox._sandbox, + mcp_sandbox._interpreter, + mcp_sandbox._session_dir, + ) = original + + async def _capability_probe(client, code): """Run a probe expected to return exactly 'REACHABLE' or 'ABSENT'. @@ -533,7 +590,10 @@ async def test_js_function_constructor_unavailable(sandbox_client): async def test_host_file_outside_session_unreadable(sandbox_client, tmp_path_factory): """A host file outside the session directory must not be readable. - Uses a sentinel this test creates; no real system or secret file is touched. + Asserts on the *effect* (bytes retrieved), not on whether an API exists. + Only the specific errors a confining policy would raise are treated as + ABSENT; anything else propagates and fails the test, so a broken probe is + never mistaken for confinement. """ sentinel = tmp_path_factory.mktemp("sentinel") / "canary.txt" sentinel.write_text("CANARY_MARKER_DO_NOT_LEAK") @@ -545,7 +605,9 @@ async def test_host_file_outside_session_unreadable(sandbox_client, tmp_path_fac " import pyodide_js\n" f" _buf = await pyodide_js._api.loadBinaryFile({as_posix!r})\n" " _r = 'REACHABLE' if b'CANARY_MARKER' in bytes(_buf.to_py()) else 'ABSENT'\n" - "except (ImportError, AttributeError, OSError, Exception):\n" + # ImportError/AttributeError: the loader was removed from the surface. + # PermissionError/OSError: a policy refused the read. + "except (ImportError, AttributeError, PermissionError, OSError):\n" " _r = 'ABSENT'\n" "_r", ) @@ -553,67 +615,117 @@ async def test_host_file_outside_session_unreadable(sandbox_client, tmp_path_fac @pytest.mark.asyncio -async def test_environment_variables_unreadable(sandbox_client): - """The parent process environment must not be readable from user code. +async def test_parent_environment_secret_not_visible(sandbox_client_with_secret): + """A secret exported to the parent must not be visible to agent code. - The worker is spawned with an allowlisted environment, so a small number of - operational variables (PATH and friends) are expected to remain visible. - This asserts that the *inherited* environment is not exposed wholesale. + Presence of *some* environment is expected and is not a failure: the worker + is deliberately given PATH and a few operational variables. What must not + happen is the parent's own environment reaching agent code, so this looks + for a unique sentinel exported before the worker started. """ + secret_name, secret_value = sandbox_client_with_secret.secret outcome = await _capability_probe( - sandbox_client, + sandbox_client_with_secret.client, "try:\n" " import pyodide_js\n" " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" - " _n = int(_F(\"return typeof process==='undefined' ? 0 :" - ' Object.keys(process.env).length")())\n' - " _r = 'ABSENT' if _n == 0 else 'REACHABLE'\n" - "except (ImportError, AttributeError):\n" + f" _v = _F(\"return typeof process==='undefined' ? '' :" + f" (process.env[{secret_name!r}] || '')\")()\n" + f" _r = 'REACHABLE' if _v == {secret_value!r} else 'ABSENT'\n" + "except (ImportError, AttributeError, PermissionError):\n" " _r = 'ABSENT'\n" "_r", ) - _record("the worker process environment", outcome, "process.env") + _record( + "a secret exported in the parent environment", + outcome, + f"{secret_name} readable via process.env", + ) @pytest.mark.asyncio -async def test_node_filesystem_module_unreachable(sandbox_client): - """`node:fs` must not be resolvable from agent code. - - Resolution only: nothing is read or written through the module. +async def test_host_filesystem_write_unavailable(sandbox_client, tmp_path_factory): + """Agent code must not be able to write outside the session via node:fs. + + Performs a harmless, test-owned write into a directory pytest created, then + checks the host for the effect. A rejected dynamic import is *not* swallowed + here: if the import fails unexpectedly the probe returns something the + harness does not recognise and the test fails, rather than silently counting + as confinement. """ + target = tmp_path_factory.mktemp("fs_probe") / "written_by_agent.txt" + + js_src = ( + "return import('node:fs').then(m => {" + f" m.writeFileSync({json.dumps(str(target))}, 'FS_WRITE_MARKER');" + " return 'REACHABLE';" + "})" + ) outcome = await _capability_probe( sandbox_client, "try:\n" " import pyodide_js\n" " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" - " _fn = _F(\"return import('node:fs')" - ".then(m => typeof m.readFileSync === 'function' ? 'REACHABLE' : 'ABSENT')" - ".catch(() => 'ABSENT')\")\n" + f" _fn = _F({js_src!r})\n" " _r = await _fn()\n" - "except (ImportError, AttributeError):\n" + "except (ImportError, AttributeError, PermissionError):\n" " _r = 'ABSENT'\n" "_r", ) - _record("the node:fs module", outcome, "dynamic import('node:fs')") + + if outcome == "REACHABLE": + assert target.exists(), ( + "probe reported a successful write but no host file appeared; " + "the probe is not measuring what it claims" + ) + assert "FS_WRITE_MARKER" in target.read_text() + else: + assert not target.exists(), "probe reported ABSENT but the host file was written" + + _record("host filesystem writes via node:fs", outcome, "writeFileSync to a test-owned path") @pytest.mark.asyncio -async def test_node_process_module_unreachable(sandbox_client): - """`node:child_process` must not be resolvable from agent code. +async def test_process_execution_unavailable(sandbox_client, tmp_path_factory): + """Agent code must not be able to execute a process. - Resolution only: no process is ever spawned by this test. + Runs a harmless, test-owned command whose only effect is to create a file in + a directory pytest created, then checks the host for that file. Nothing is + downloaded, no network is used, and no state outside that temp directory is + touched. """ + marker = tmp_path_factory.mktemp("proc_probe") / "spawned.txt" + + # A trivial node one-liner: write a marker file and exit. + inner = f"require('fs').writeFileSync({json.dumps(str(marker))}, 'SPAWN_MARKER')" + js_src = ( + "return import('node:child_process').then(m => {" + f" m.execFileSync(process.execPath, ['-e', {json.dumps(inner)}]);" + " return 'REACHABLE';" + "})" + ) outcome = await _capability_probe( sandbox_client, "try:\n" " import pyodide_js\n" " _F = pyodide_js._api.loadBinaryFile.constructor.constructor\n" - " _fn = _F(\"return import('node:child_process')" - ".then(m => typeof m.execSync === 'function' ? 'REACHABLE' : 'ABSENT')" - ".catch(() => 'ABSENT')\")\n" + f" _fn = _F({js_src!r})\n" " _r = await _fn()\n" - "except (ImportError, AttributeError):\n" + "except (ImportError, AttributeError, PermissionError):\n" " _r = 'ABSENT'\n" "_r", ) - _record("the node:child_process module", outcome, "dynamic import('node:child_process')") + + if outcome == "REACHABLE": + assert marker.exists(), ( + "probe reported successful execution but no host file appeared; " + "the probe is not measuring what it claims" + ) + else: + assert not marker.exists(), "probe reported ABSENT but the command ran" + + _record( + "process execution via node:child_process", + outcome, + "execFileSync of a node one-liner", + ) diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py index 7bb206b..558ad0d 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -23,7 +23,9 @@ # reach the Node host through it (`import js`, cached JsProxy references, and # the Function constructor, which evaluates in global scope and therefore # survives jsglobals restriction or module hiding). Host filesystem access, -# process execution and environment variables are all reachable. +# process execution and its own environment are all reachable. The worker's +# environment is allowlisted so the parent's secrets are not forwarded, but +# that narrows the blast radius rather than preventing the escape. # # Run only trusted, first-party agent code here. Do not route untrusted or # third-party input to this server until the worker is confined by an OS/ diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index a5fccda..efc31f9 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -4,6 +4,8 @@ import asyncio import json import os +import shutil +import tempfile from dataclasses import asdict, dataclass from pathlib import Path @@ -29,7 +31,7 @@ ) -def _minimal_env() -> dict[str, str]: +def _minimal_env(worker_tmp: str | None = None) -> dict[str, str]: """Build the worker environment from an allowlist. Defense-in-depth only. A process that escapes Pyodide still runs with the @@ -37,11 +39,14 @@ def _minimal_env() -> dict[str, str]: escape does not hand over whatever secrets happened to be exported. """ env = {name: os.environ[name] for name in _ENV_ALLOWLIST if name in os.environ} - # Node reads TMPDIR/TEMP/TMP for os.tmpdir(); give it the session's own - # temp root rather than leaking the parent's. - for name in ("TMPDIR", "TEMP", "TMP"): - if name in os.environ: - env[name] = os.environ[name] + # Node reads TMPDIR/TEMP/TMP for os.tmpdir(). Point them at a directory + # this interpreter owns rather than forwarding the parent's values, so + # worker scratch files do not land in a shared temp location. When no + # directory is supplied the variables are omitted entirely and Node falls + # back to its platform default. + if worker_tmp: + for name in ("TMPDIR", "TEMP", "TMP"): + env[name] = worker_tmp return env @@ -91,6 +96,10 @@ def __init__(self, timeout: float = 30.0, workspace_dir: str | None = None): self.effects: list[dict] = [] self._process: asyncio.subprocess.Process | None = None self._worker_path = Path(__file__).parent / "pyodide_worker.mjs" + # Scratch directory handed to the worker as TMPDIR/TEMP/TMP, created on + # first start and removed on close so worker temp files do not outlive + # the interpreter or share the host's temp root. + self._worker_tmp: str | None = None # Serializes execute() and close() against the shared worker stdio. # The worker speaks a strict request/response protocol on a single # stdin/stdout pair, so two concurrent execute() coroutines would @@ -145,6 +154,7 @@ async def close(self) -> None: """Gracefully stop the worker process.""" async with self._lock: if self._process is None: + self._cleanup_worker_tmp() return try: self._process.stdin.close() @@ -153,11 +163,17 @@ async def close(self) -> None: await self._kill() finally: self._process = None + self._cleanup_worker_tmp() # ------------------------------------------------------------------ # Internal helpers # ------------------------------------------------------------------ + def _cleanup_worker_tmp(self) -> None: + if self._worker_tmp: + shutil.rmtree(self._worker_tmp, ignore_errors=True) + self._worker_tmp = None + async def _ensure_started(self) -> None: if self._process is None or self._process.returncode is not None: await self._start() @@ -199,6 +215,8 @@ async def _start(self) -> None: cmd = ["node", str(self._worker_path)] if self.workspace_dir: cmd += ["--workspace", self.workspace_dir] + if self._worker_tmp is None: + self._worker_tmp = tempfile.mkdtemp(prefix="sandbox_worker_tmp_") self._process = await asyncio.create_subprocess_exec( *cmd, stdin=asyncio.subprocess.PIPE, @@ -212,7 +230,7 @@ async def _start(self) -> None: # the pyodide package in the sibling node_modules/ folder. cwd=str(self._worker_path.parent), # Withhold the parent environment (defense-in-depth, not isolation). - env=_minimal_env(), + env=_minimal_env(self._worker_tmp), ) # Wait for the { "ready": true } handshake before accepting requests. From 094483894b5b30af3bb45baa4f5ad63fc0be538f Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 17:17:02 -0700 Subject: [PATCH 09/21] State the trust constraint precisely: provenance is not trust Two corrections to how the constraint is worded, in all five places it appeared (docs, servers.yaml, the module comment, and the fail-closed error). "Trusted, first-party agent code" conflated provenance with trust. An agent developed in-house is not a trusted *code source*: it can be induced to emit hostile code by attacker-controlled input. That is concrete here rather than theoretical, because this server exists to read workspace documents with pandas/openpyxl/pypdf, so a malicious spreadsheet or PDF is an input to the agent that writes the code. The wording now says trusted code only and calls out that first-party provenance is not itself trust. The secrets constraint was also justified with reasoning that the environment allowlist has since made obsolete: it said to keep secrets out of the server's environment "because the worker is spawned from it". The worker no longer inherits that environment, and a passing test asserts a parent secret is not visible. The real requirement is broader, so it is now stated as a secret-free execution context: an escape runs with this user's OS privileges and can read credentials from disk, which withholding environment variables does nothing about. The allowlist is described as reducing exposure, not as a boundary. No behavior change; wording and comments only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 26 ++++++++++++------- servers/servers.yaml | 6 +++-- .../thinkingbox_tools/mcp_sandbox.py | 10 ++++--- .../toolslib/sandbox/code_interpreter.py | 5 ++-- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index 84a97d0..4a739a6 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -7,9 +7,10 @@ The `sandbox` MCP server runs agent-supplied Python in a Pyodide > [!WARNING] > **This is not a security boundary.** Pyodide provides memory safety, not > isolation: Python running here can reach the Node.js host, the host -> filesystem, process execution and environment variables. Run only trusted, -> first-party agent code. See [Threat model](#threat-model) before routing any -> untrusted input to this server. +> filesystem, process execution and environment variables. Run only **trusted +> code** — note that provenance is not trust, so "generated by our own agent" +> is not sufficient (see [Threat model](#threat-model)). Read that section +> before routing any untrusted input to this server. --- @@ -41,10 +42,17 @@ is not a substitute for an OS boundary. **Consequences for how this server may be used:** -- Only trusted, first-party agent code may be executed. -- The workspace must be treated as trusted input. -- Secrets must not be present in the environment of the MCP server process, - because the worker is spawned from it. +- Only **trusted code** may be executed. Provenance is not trust: code produced + by a first-party agent is not automatically trustworthy, because an agent can + be induced to emit hostile code by attacker-controlled input. That is a live + concern here rather than a theoretical one — this server exists to process + workspace documents, so a malicious spreadsheet or PDF is an input to the + agent that writes the code. +- The workspace must therefore be treated as trusted input too. +- The execution context must be secret-free. The environment allowlist below + keeps the parent's variables out of the worker, but an escape still runs with + this user's OS privileges and can read credentials from disk — so withholding + environment variables is a reduction in exposure, not a boundary. **Controls currently in place.** These narrow the blast radius. None of them makes the boundary real, and none should be described as isolation: @@ -394,8 +402,8 @@ export THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 This is deliberately **not** set in `servers/servers.yaml`. That file is the documented normal startup path, so setting it there would make the unsafe mode the default and defeat the gate. Export it only where the executed code is -trusted and first-party, and keep secrets out of that environment — the worker -is started from it. +**trusted** — remembering that first-party provenance is not itself trust — and +only in a secret-free context. **Supported platforms.** CI covers Linux (`ubuntu-latest`) only, and that is the supported platform. The code paths are cross-platform and the Windows diff --git a/servers/servers.yaml b/servers/servers.yaml index 4fd6157..b8d4be4 100644 --- a/servers/servers.yaml +++ b/servers/servers.yaml @@ -21,8 +21,10 @@ servers: # operator sets THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 in the *environment* # that launches it. That opt-in is deliberately NOT set here -- baking it # into the shipped config would make the unsafe mode the default and defeat - # the gate. Enable it only where the executed code is trusted and - # first-party, and keep secrets out of that environment. + # the gate. Enable it only where the executed code is trusted -- first-party + # provenance is not itself trust, since an agent can be induced to emit + # hostile code by attacker-controlled input -- and only in a secret-free + # execution context. # See docs/sandbox_code_interpreter.md ("Threat model"). sandbox: type: mcp-process diff --git a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py index 558ad0d..0662830 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py @@ -27,9 +27,13 @@ # environment is allowlisted so the parent's secrets are not forwarded, but # that narrows the blast radius rather than preventing the escape. # -# Run only trusted, first-party agent code here. Do not route untrusted or -# third-party input to this server until the worker is confined by an OS/ -# container boundary. See docs/sandbox_code_interpreter.md ("Threat model"). +# Run only trusted code here. Provenance is not trust: code emitted by a +# first-party agent is not automatically trustworthy, since the agent can be +# induced to produce hostile code by attacker-controlled input (this server +# reads workspace documents, so those inputs are in scope). Do not route +# untrusted or third-party input to this server until the worker is confined +# by an OS/container boundary. +# See docs/sandbox_code_interpreter.md ("Threat model"). mcp = FastMCP("sandbox") diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index efc31f9..f523d2a 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -189,8 +189,9 @@ async def _start(self) -> None: "Pyodide is NOT a security boundary -- code executed here can reach " "the Node host, the host filesystem, process execution and this " "process's environment variables.\n" - "Only enable this where the executed code is trusted and " - "first-party, by setting:\n" + "Only enable this where the executed code is trusted -- note " + "that first-party provenance is not itself trust -- and the " + "execution context is secret-free, by setting:\n" f" {UNCONFINED_OPT_IN_ENV}=1\n" "See docs/sandbox_code_interpreter.md ('Threat model')." ) From ae3778146da7ecdc84bfd06f12618c727fc92367 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Thu, 27 Aug 2026 18:04:13 -0700 Subject: [PATCH 10/21] Add an eval scenario exercising the sandbox through the harness path The port added a server that nothing in this repo used. All ten existing scenarios target the airline, banking and email servers, so `code_interpreter` had no scenario and was effectively dead code here. The 139 unit tests drive the server directly through fastmcp.Client with a tmp_path built in Python, which never exercised the path an eval actually takes: servers.yaml -> world_state -> the scenario's declared tools. Adds dataset/scenario/sandbox_code_interpreter.yaml, wired the same way as the tau_bench scenarios ($THINKINGBOX_DATA expansion, which mcp_sandbox already supports via os.path.expandvars), plus a small workspace fixture under support/sandbox_workspace/ and test cases asserting on the code_execution effects rather than on prose alone. The fixture has known ground truth (revenue = units * unit_price, summed per region: East 7312.50, North 5297.35, South 4923.50, West 4032.00), so a test can distinguish a computed answer from an invented one. Verified end to end against the real server before committing: world_state passes through __reserved__init verbatim, $THINKINGBOX_DATA resolves to the fixture, all three declared tools exist and respond, pandas returns the correct ground truth, and -- importantly -- an agent write to reports/sales.csv succeeds inside the session while leaving the committed fixture byte-identical. That last check exercises the NODEFS copy-on-write layer against real repository files rather than a synthetic temp directory; a regression there would surface as a dirty working tree in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../scenario/sandbox_code_interpreter.yaml | 26 ++++ dataset/test_case/sandbox_code_interpreter.py | 134 ++++++++++++++++++ support/sandbox_workspace/reports/notes.txt | 5 + support/sandbox_workspace/reports/sales.csv | 9 ++ 4 files changed, 174 insertions(+) create mode 100644 dataset/scenario/sandbox_code_interpreter.yaml create mode 100644 dataset/test_case/sandbox_code_interpreter.py create mode 100644 support/sandbox_workspace/reports/notes.txt create mode 100644 support/sandbox_workspace/reports/sales.csv diff --git a/dataset/scenario/sandbox_code_interpreter.yaml b/dataset/scenario/sandbox_code_interpreter.yaml new file mode 100644 index 0000000..98cbc84 --- /dev/null +++ b/dataset/scenario/sandbox_code_interpreter.yaml @@ -0,0 +1,26 @@ +world_state: + sandbox: + # Expanded by mcp_sandbox via os.path.expandvars, same convention as the + # tau_bench scenarios. __reserved__init seeds a per-session copy, so the + # agent's writes never mutate these fixtures. + workspace_dir: $THINKINGBOX_DATA/support/sandbox_workspace + timeout: 60.0 +tools: +- name: list_sandbox_files +- name: search_sandbox_files +- name: code_interpreter + +bot_instructions: | + You are a data analyst. You have access to a workspace of files and a Python + interpreter. + + - Workspace files are listed with paths relative to the workspace root. To + open one from Python, prepend /workspace/ — for example the listed path + 'reports/sales.csv' is opened as '/workspace/reports/sales.csv'. + - Use the code interpreter to compute answers rather than doing arithmetic + yourself. numpy and pandas are available. + - The interpreter is stateful: variables and imports persist between calls. + - Report the figures you computed. Do not invent numbers that the code did + not produce. + +tags: [domain:data-analysis, eval:orchestration:tool-selection] diff --git a/dataset/test_case/sandbox_code_interpreter.py b/dataset/test_case/sandbox_code_interpreter.py new file mode 100644 index 0000000..86ace98 --- /dev/null +++ b/dataset/test_case/sandbox_code_interpreter.py @@ -0,0 +1,134 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +from thinkingbox.common import Judge, TestContext + +"""! +scenario: sandbox_code_interpreter +""" + +# Ground truth for support/sandbox_workspace/reports/sales.csv, computed as +# units * unit_price summed per region: +# East 7312.50 +# North 5297.35 +# South 4923.50 +# West 4032.00 + + +def _executions(x: TestContext) -> list[dict]: + """The code_execution effects recorded by the sandbox server.""" + effects = x.effects["sandbox"]["effects"] + return [e for e in effects if e.get("type") == "code_execution"] + + +def test_reads_workspace_file_through_interpreter(x: TestContext, judge: Judge): + """! + query: | + Read reports/notes.txt from the workspace and tell me what it says the + team wants. + """ + executions = _executions(x) + assert executions, "the agent did not use the code interpreter" + + # The workspace must be reachable at /workspace/, and the read must actually + # have succeeded rather than erroring out. + read_notes = [ + e + for e in executions + if "notes.txt" in e.get("code", "") and e["result"].get("error") is None + ] + assert read_notes, ( + "no successful execution read notes.txt from the workspace: " + f"{[e.get('code') for e in executions]}" + ) + + assert judge.text_yesno( + x.response, + "Does the response say the team wants revenue totalled per region, " + "ordered from highest to lowest?", + ) + + +def test_computes_revenue_per_region(x: TestContext, judge: Judge): + """! + query: | + Using reports/sales.csv, compute total revenue per region (units times + unit price, summed across quarters) and tell me which region has the + highest revenue. + """ + executions = _executions(x) + assert executions, "the agent did not use the code interpreter" + + # Every execution the agent kept should be error-free by the end; at minimum + # one must have succeeded, otherwise any correct-looking answer was invented. + successful = [e for e in executions if e["result"].get("error") is None] + assert successful, ( + "every code execution failed, so the answer was not computed: " + f"{[e['result'].get('error') for e in executions]}" + ) + + # The figures must come from code, not from the model doing mental math. + assert any( + "sales.csv" in e.get("code", "") for e in successful + ), "no successful execution referenced sales.csv" + + # East is the correct answer (7312.50). + assert judge.text_yesno( + x.response, + "Does the response identify East as the region with the highest total " + "revenue?", + ) + + # Guard against the most likely wrong answer: summing units instead of + # revenue would still make East highest, so check the figure was reported. + assert judge.text_yesno( + x.response, + "Does the response report East's total revenue as approximately 7312.50 " + "(accepting 7312.5, 7,312.50 or $7312.50)?", + ) + + +def test_discovers_workspace_files(x: TestContext, judge: Judge): + """! + query: | + What files are in the workspace? + """ + effects = x.effects["sandbox"]["effects"] + # Listing does not require the interpreter, so this exercises the filesystem + # tools independently of the Pyodide worker. + assert effects is not None + + assert judge.text_yesno( + x.response, + "Does the response mention both a sales CSV file and a notes text file " + "under a reports folder?", + ) + + +def test_source_workspace_is_not_mutated(x: TestContext, judge: Judge): + """! + query: | + Add a row for region Central with 100 units at 20.00 to + reports/sales.csv, then tell me the new total revenue for Central. + """ + executions = _executions(x) + assert executions, "the agent did not use the code interpreter" + + # The write is expected to succeed *inside the session*: __reserved__init + # seeds a per-session copy, and the NODEFS copy-on-write layer materialises + # a private copy before the write lands. The fixture under + # support/sandbox_workspace/ must be untouched afterwards, which the + # repository's own git status verifies -- a mutated fixture would show up as + # a dirty working tree in CI. + wrote = [ + e + for e in executions + if "sales.csv" in e.get("code", "") and e["result"].get("error") is None + ] + assert wrote, "no successful execution touched sales.csv" + + assert judge.text_yesno( + x.response, + "Does the response report Central's revenue as approximately 2000 " + "(accepting 2000.0, 2,000 or $2000)?", + ) diff --git a/support/sandbox_workspace/reports/notes.txt b/support/sandbox_workspace/reports/notes.txt new file mode 100644 index 0000000..d6d4d45 --- /dev/null +++ b/support/sandbox_workspace/reports/notes.txt @@ -0,0 +1,5 @@ +Q1-Q2 sales notes + +Unit prices are fixed per region for the period. +Revenue is units * unit_price. +The team wants revenue totalled per region, highest first. \ No newline at end of file diff --git a/support/sandbox_workspace/reports/sales.csv b/support/sandbox_workspace/reports/sales.csv new file mode 100644 index 0000000..d76375b --- /dev/null +++ b/support/sandbox_workspace/reports/sales.csv @@ -0,0 +1,9 @@ +region,quarter,units,unit_price +North,Q1,120,19.99 +North,Q2,145,19.99 +South,Q1,98,21.50 +South,Q2,131,21.50 +East,Q1,203,18.75 +East,Q2,187,18.75 +West,Q1,76,24.00 +West,Q2,92,24.00 \ No newline at end of file From 100346e6d036b656aa169b7e275e85e7861f1d90 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 09:27:07 -0700 Subject: [PATCH 11/21] Fix worker protocol desync, 64 KiB output cap, glob crash, wheel integrity Findings from three independent reviews, each reproduced before fixing. Output larger than 64 KiB broke the interpreter. create_subprocess_exec was called without `limit=`, so asyncio's 64 KiB StreamReader default applied to a protocol that puts one JSON frame per line. Reproduced: a 100 KB frame raises "ValueError: Separator is found, but chunk is longer than limit" and a 500 KB frame raises the not-found variant, while both read cleanly at a higher limit. This is not an edge case for a data-analysis tool -- printing a moderately sized DataFrame exceeds 64 KiB -- and the ValueError was not caught, so it surfaced as a generic "Internal error" while leaving the worker alive with an unread partial frame. STREAM_LIMIT is now 64 MiB and over-limit frames reset the worker with an explanatory message. Malformed frames desynchronized every later call. `json.loads` on the response line was unguarded, so a non-protocol line raised while the real response stayed buffered; the next execute() then consumed that stale frame and returned the previous call's result. Frames that are not JSON objects are now treated as fatal protocol errors that kill and reset the worker. The startup handshake had the same unguarded parse, which additionally leaked a live Pyodide process on every retry. search_sandbox_files crashed on patterns an agent can plausibly emit. Path.glob raises on an empty pattern, on a malformed "***", and on absolute paths such as "/etc/passwd". The pattern is model-supplied and the tool is agent-facing, so these now read as "no matches" instead of raising out of the call. Vendored wheels were installed without integrity checking. PyPI publishes a sha256 in the release metadata that download-wheels.mjs ignored. These wheels are installed into the interpreter, so a corrupted or substituted file is code execution. Downloads are now verified against the published digest and refused on mismatch, and cached wheels are re-verified rather than trusted by filename (the cache lives in a writable working directory). Confirmed the comparison is correct: all 11 locally cached wheels match PyPI's published digests. The new eval scenario could not actually run. The interpreter fails closed and the opt-in is set only by the unit suites' autouse fixture, so a scenario run would fail with a confusing "agent did not use the code interpreter". The requirement is now documented in both the scenario and the test case rather than papered over by re-adding the opt-in to servers.yaml, which would defeat the gate. Adds five regression tests, four of which were confirmed to fail against the unfixed code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../scenario/sandbox_code_interpreter.yaml | 8 ++ dataset/test_case/sandbox_code_interpreter.py | 7 + docs/sandbox_code_interpreter.md | 7 +- .../tests/test_sandbox_isolation.py | 129 ++++++++++++++++++ .../toolslib/sandbox/code_interpreter.py | 59 +++++++- .../toolslib/sandbox/sandbox.py | 10 +- .../sandbox/scripts/download-wheels.mjs | 38 +++++- 7 files changed, 247 insertions(+), 11 deletions(-) diff --git a/dataset/scenario/sandbox_code_interpreter.yaml b/dataset/scenario/sandbox_code_interpreter.yaml index 98cbc84..cec9b72 100644 --- a/dataset/scenario/sandbox_code_interpreter.yaml +++ b/dataset/scenario/sandbox_code_interpreter.yaml @@ -1,3 +1,11 @@ +# Exercises the sandbox code interpreter over a small fixture workspace. +# +# PREREQUISITE: the interpreter fails closed. Running this scenario requires +# THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 in the environment that launches the +# MCP server; without it `code_interpreter` calls are rejected (the filesystem +# tools still work). This is deliberate -- see docs/sandbox_code_interpreter.md +# ("Threat model"). The opt-in is not set in servers.yaml because that would +# make the unconfined mode the default for every consumer of that file. world_state: sandbox: # Expanded by mcp_sandbox via os.path.expandvars, same convention as the diff --git a/dataset/test_case/sandbox_code_interpreter.py b/dataset/test_case/sandbox_code_interpreter.py index 86ace98..739209d 100644 --- a/dataset/test_case/sandbox_code_interpreter.py +++ b/dataset/test_case/sandbox_code_interpreter.py @@ -7,6 +7,13 @@ scenario: sandbox_code_interpreter """ +# PREREQUISITE: the code interpreter fails closed. These test cases require +# THINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1 in the environment that launches the +# MCP server. Without it the `code_interpreter` tool returns an error and the +# assertions below fail with "the agent did not use the code interpreter", +# which is expected rather than a defect. See docs/sandbox_code_interpreter.md +# ("Threat model") for why the opt-in is not baked into servers.yaml. + # Ground truth for support/sandbox_workspace/reports/sales.csv, computed as # units * unit_price summed per region: # East 7312.50 diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index 4a739a6..fa29d6e 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -388,8 +388,11 @@ npm install The `postinstall` script vendors wheels into `./wheels/`. Idempotent — re-running keeps existing wheels; force a refresh by deleting `wheels/`. -If PyPI is unreachable, missing wheels fall back to runtime fetch -(slower startup, still works). +Each wheel is checked against the SHA-256 that PyPI publishes in its metadata, +both on download and when reusing a cached file; a mismatch is refused rather +than written, since these wheels are installed into the interpreter. If PyPI is +unreachable, missing wheels fall back to runtime fetch (slower startup, still +works). **Enabling execution.** The interpreter fails closed: it refuses to spawn a worker unless the operator opts in, in the environment that launches the diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index b1d11c7..4429f70 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -38,6 +38,7 @@ from thinkingbox_tools import mcp_sandbox from thinkingbox_tools.toolslib.sandbox import code_interpreter +from thinkingbox_tools.toolslib.sandbox.sandbox import Sandbox NOT_ISOLATED = ( "Pyodide is not a privilege boundary; requires OS/container confinement. " @@ -429,6 +430,134 @@ def test_worker_tmp_is_not_inherited(monkeypatch, tmp_path): assert env[name] == worker_tmp, f"{name} did not point at the worker dir" +# --------------------------------------------------------------------------- +# Worker protocol robustness +# --------------------------------------------------------------------------- + + +class _FakeStdout: + """Minimal StreamReader stand-in returning canned frames.""" + + def __init__(self, frames): + self._frames = list(frames) + + async def readline(self): + if not self._frames: + return b"" + frame = self._frames.pop(0) + if isinstance(frame, Exception): + raise frame + return frame + + +class _FakeStdin: + def write(self, _data): + pass + + async def drain(self): + pass + + def close(self): + pass + + +class _FakeProcess: + def __init__(self, frames): + self.stdout = _FakeStdout(frames) + self.stdin = _FakeStdin() + self.returncode = None + self.killed = False + + def kill(self): + self.killed = True + self.returncode = -9 + + async def wait(self): + return self.returncode + + +def _interp_with(frames): + """A CodeInterpreter wired to a fake worker emitting `frames`.""" + interp = code_interpreter.CodeInterpreter(timeout=5.0) + proc = _FakeProcess(frames) + interp._process = proc + return interp, proc + + +@pytest.mark.asyncio +async def test_malformed_frame_resets_worker(): + """A non-JSON frame must reset the worker, not desync the stream. + + Without the reset, the malformed line raises while the real response stays + buffered, and the *next* execute() returns the previous call's result. + """ + real = json.dumps({"stdout": "correct", "stderr": "", "result": None, "error": None}) + interp, proc = _interp_with([b"not json at all\n", real.encode() + b"\n"]) + + with pytest.raises(code_interpreter.CodeInterpreterError) as excinfo: + await interp.execute("1") + assert "out of sync" in str(excinfo.value) + assert proc.killed, "worker was left running with a desynchronized stream" + assert interp._process is None, "next call would reuse the poisoned stream" + + +@pytest.mark.asyncio +async def test_non_object_frame_resets_worker(): + """A JSON frame that is not an object is also a protocol violation.""" + interp, proc = _interp_with([b'"just a string"\n']) + with pytest.raises(code_interpreter.CodeInterpreterError): + await interp.execute("1") + assert proc.killed + assert interp._process is None + + +@pytest.mark.asyncio +async def test_oversized_frame_resets_worker(): + """An over-limit frame must surface a clear error and reset the worker. + + asyncio's StreamReader raises ValueError rather than returning the line, and + the unread remainder would otherwise be parsed as the next response. + """ + interp, proc = _interp_with( + [ValueError("Separator is found, but chunk is longer than limit")] + ) + with pytest.raises(code_interpreter.CodeInterpreterError) as excinfo: + await interp.execute("print('x' * 10_000_000)") + message = str(excinfo.value) + assert "larger than" in message and "reset" in message + assert proc.killed + assert interp._process is None + + +def test_stream_limit_exceeds_asyncio_default(): + """The configured limit must be above asyncio's 64 KiB default. + + A single print() of a large DataFrame exceeds 64 KiB, so the default would + make ordinary analysis fail. + """ + import asyncio.streams + + assert code_interpreter.CodeInterpreter.STREAM_LIMIT > asyncio.streams._DEFAULT_LIMIT + assert code_interpreter.CodeInterpreter.STREAM_LIMIT >= 8 * 1024 * 1024 + + +def test_search_files_tolerates_unusable_patterns(tmp_path): + """Model-supplied glob patterns must not raise out of the tool. + + Path.glob rejects an empty pattern, a malformed '***', and absolute paths. + The pattern comes straight from the agent, so these must read as "no + matches" rather than crashing the call. + """ + (tmp_path / "a.txt").write_text("x") + sandbox = Sandbox(str(tmp_path)) + + for pattern in ("", "***", "/etc/passwd", "C:\\Windows\\win.ini", "[", "a[b"): + assert sandbox.search_files(pattern) == [], f"pattern {pattern!r} returned matches" + + # A valid pattern still works. + assert sandbox.search_files("*.txt") == ["a.txt"] + + # --------------------------------------------------------------------------- # Host capability audit # --------------------------------------------------------------------------- diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index f523d2a..0ea15c8 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -90,6 +90,14 @@ class CodeInterpreter: # fetches PyPI wheels over the network; subsequent runs use the local cache. STARTUP_TIMEOUT = 300.0 + # asyncio's StreamReader defaults to a 64 KiB line limit, and the protocol + # puts one JSON frame per line. A single print() of a moderately sized + # DataFrame exceeds that, and readline() then raises ValueError rather than + # returning the frame -- so a routine analysis would fail with an opaque + # error. Raise the ceiling well above realistic output while still bounding + # memory for a runaway producer. + STREAM_LIMIT = 64 * 1024 * 1024 + def __init__(self, timeout: float = 30.0, workspace_dir: str | None = None): self.timeout = timeout self.workspace_dir = workspace_dir @@ -129,6 +137,16 @@ async def execute(self, code: str) -> ExecutionResult: f"Execution timed out after {self.timeout}s. " "The interpreter has been reset." ) + except (ValueError, asyncio.LimitOverrunError) as exc: + # readline() raises when a frame exceeds STREAM_LIMIT. The + # unread remainder would be parsed as the *next* response, so + # the worker has to be reset rather than reused. + await self._kill() + raise CodeInterpreterError( + f"Worker produced a response frame larger than " + f"{self.STREAM_LIMIT} bytes ({exc}). The interpreter has " + "been reset. Reduce the amount of data printed or returned." + ) if not response_line: await self._kill() @@ -137,7 +155,25 @@ async def execute(self, code: str) -> ExecutionResult: "The interpreter has been reset." ) - data = json.loads(response_line) + # A frame that is not a JSON object means the stream is no longer in + # sync with the protocol -- anything still buffered would be read as + # the next response and silently returned for the wrong call. Kill + # the worker so the next execute() starts from a known state. + try: + data = json.loads(response_line) + except ValueError: + await self._kill() + raise CodeInterpreterError( + "Worker produced a malformed response frame; the protocol " + "stream is out of sync. The interpreter has been reset." + ) + if not isinstance(data, dict): + await self._kill() + raise CodeInterpreterError( + "Worker produced an unexpected response frame; the protocol " + "stream is out of sync. The interpreter has been reset." + ) + result = ExecutionResult( stdout=data.get("stdout", ""), stderr=data.get("stderr", ""), @@ -232,6 +268,9 @@ async def _start(self) -> None: cwd=str(self._worker_path.parent), # Withhold the parent environment (defense-in-depth, not isolation). env=_minimal_env(self._worker_tmp), + # Raise the StreamReader line limit above asyncio's 64 KiB default; + # response frames carry user stdout and can legitimately be large. + limit=self.STREAM_LIMIT, ) # Wait for the { "ready": true } handshake before accepting requests. @@ -246,6 +285,11 @@ async def _start(self) -> None: f"Worker timed out during startup (>{self.STARTUP_TIMEOUT}s). " f"Make sure 'npm install' has been run in {worker_dir}." ) + except (ValueError, asyncio.LimitOverrunError) as exc: + await self._kill() + raise CodeInterpreterError( + f"Worker emitted an oversized handshake frame ({exc})." + ) if not ready_line: await self._kill() @@ -253,8 +297,17 @@ async def _start(self) -> None: "Worker exited before sending ready signal. " "See [pyodide_worker] output above for details." ) - ready = json.loads(ready_line) - if not ready.get("ready"): + # A non-JSON handshake previously raised out of _start() with the child + # still running, leaking a Pyodide process on every retry. + try: + ready = json.loads(ready_line) + except ValueError: + await self._kill() + raise CodeInterpreterError( + "Worker sent a malformed handshake frame. " + "See [pyodide_worker] output above for details." + ) + if not isinstance(ready, dict) or not ready.get("ready"): await self._kill() raise CodeInterpreterError(f"Unexpected worker handshake: {ready}") diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py index 92ab027..f8f90d4 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py @@ -42,7 +42,15 @@ def search_files(self, pattern: str) -> list[str]: # files for COW isolation, and resolving those would land outside # workspace_dir for every legitimate match. results = [] - for p in self.workspace_dir.glob(pattern): + try: + matches = list(self.workspace_dir.glob(pattern)) + except (ValueError, NotImplementedError, OSError): + # Path.glob rejects some inputs outright -- an empty pattern, a + # malformed "***", or an absolute path all raise. This tool is + # agent-facing and the pattern is model-supplied, so an unusable + # pattern must read as "no matches" rather than crash the call. + return [] + for p in matches: if not p.is_file() or p.name.startswith("."): continue if ".." in p.parts: diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs index c4f08ac..5179f46 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -12,7 +12,8 @@ // (which works because micropip resolves them via pyodide's own bundle when // available, e.g. reportlab). -import { mkdir, writeFile, access } from "node:fs/promises"; +import { mkdir, writeFile, readFile } from "node:fs/promises"; +import { createHash } from "node:crypto"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; import { PYPI_PACKAGES } from "../pypi-packages.mjs"; @@ -20,6 +21,8 @@ import { PYPI_PACKAGES } from "../pypi-packages.mjs"; const __dirname = dirname(fileURLToPath(import.meta.url)); const wheelsDir = join(__dirname, "..", "wheels"); +const sha256 = (buf) => createHash("sha256").update(buf).digest("hex"); + await mkdir(wheelsDir, { recursive: true }); const PURE_PYTHON_WHEEL = /-py[23](\.py3)?-none-any\.whl$/; @@ -48,13 +51,25 @@ async function downloadOne(name) { return; } const dest = join(wheelsDir, wheel.filename); - try { - await access(dest); - console.log(`[download-wheels] Cached: ${wheel.filename}`); + const expected = wheel.digests?.sha256; + if (!expected) { + console.warn(`[download-wheels] No sha256 published for ${wheel.filename} — leaving to runtime`); return; + } + + // Re-verify a cached wheel rather than trusting the filename: the cache + // lives in a working directory that anything on this machine can write to. + try { + const cached = await readFile(dest); + if (sha256(cached) === expected) { + console.log(`[download-wheels] Cached: ${wheel.filename}`); + return; + } + console.warn(`[download-wheels] Cached ${wheel.filename} failed digest check — refetching`); } catch { // not present — download below } + console.log(`[download-wheels] Downloading: ${wheel.filename}`); const wRes = await fetch(wheel.url, { signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS), @@ -63,7 +78,20 @@ async function downloadOne(name) { console.warn(`[download-wheels] Download failed (${wRes.status}) for ${wheel.url} — skipping`); return; } - await writeFile(dest, Buffer.from(await wRes.arrayBuffer())); + + // These wheels are installed into the interpreter, so a corrupted or + // substituted file is code execution. PyPI publishes a sha256 in the + // metadata; refuse to write anything that does not match it. + const body = Buffer.from(await wRes.arrayBuffer()); + const actual = sha256(body); + if (actual !== expected) { + console.warn( + `[download-wheels] DIGEST MISMATCH for ${wheel.filename} ` + + `(expected ${expected}, got ${actual}) — refusing to write, leaving to runtime`, + ); + return; + } + await writeFile(dest, body); } await Promise.all( From e1a673cb6f91a0efe1b5b736b6f761a192d548be Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 10:23:48 -0700 Subject: [PATCH 12/21] Record that Node's permission model cannot confine Pyodide The threat model listed --permission as weak-but-useful defense-in-depth. Testing shows that is wrong in a way worth recording: Pyodide cannot start under it at all. Pyodide calls process.binding during staticInit, which the permission system denies unconditionally, and no flag re-enables it. Verified on Node 24 -- the worker fails with ERR_ACCESS_DENIED even when every permission flag is granted (--allow-fs-read=* --allow-fs-write=* --allow-child-process --allow-worker --allow-wasi --allow-addons). This matters for the isolation decision: the choice is not between weak in-process confinement and strong OS confinement, it is between no in-process confinement and OS confinement. Recorded so the next person does not spend the same time discovering the flags do not apply. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index fa29d6e..0d1ecd1 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -36,9 +36,17 @@ host is possible via several independent routes: **In-process mitigations do not close this.** `jsglobals: {}` fails because `Function` bodies evaluate in the *global* scope, not the restricted object. Deleting or unregistering modules fails because Pyodide's internals hold live -`JsProxy` references captured at load time. Node's `--permission` model is -useful defense-in-depth but is experimental, does not gate `process.env`, and -is not a substitute for an OS boundary. +`JsProxy` references captured at load time. + +Node's own permission model (`--permission`) is not an option either, and not +merely because it is weak: **Pyodide cannot start under it at all.** Pyodide +calls `process.binding` during `staticInit`, which the permission system denies +unconditionally — there is no flag to re-enable it. Verified on Node 24: the +worker fails with `ERR_ACCESS_DENIED: process.binding` even when every +permission flag is granted (`--allow-fs-read=* --allow-fs-write=* +--allow-child-process --allow-worker --allow-wasi --allow-addons`). So the +choice is not "weak in-process confinement versus strong OS confinement"; it is +"no in-process confinement at all". **Consequences for how this server may be used:** From 0593a130a48473f2cd5dd1f895172808e956acfc Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 12:47:00 -0700 Subject: [PATCH 13/21] Reset the worker on cancellation; delete wheels that fail their digest Two high-severity findings from a review of the previous fix commit, both reproduced first. Cancelling a call returned the wrong answer to the next one. The previous commit added handlers for timeout, oversized frames, and malformed frames, but not for cancellation -- and that was the one remaining path that produces a silently wrong result instead of an error. If the caller goes away mid-request (client disconnect, MCP cancellation, an outer wait_for), CancelledError unwinds through the lock with the process still attached and the worker's reply still unread. The next execute() then reads that stale frame. Reproduced against the real CodeInterpreter: call 2 received "RESULT-OF-CALL-1". Nothing upstream could catch it either, since CancelledError is a BaseException and mcp_sandbox only catches Exception. The worker is now killed and the cancellation re-raised. A cached wheel that failed its digest was left on disk and still installed. The check detected the bad file, warned, and returned -- but never removed it, and the worker loads whatever readdir() returns without verifying anything. So the check reported a compromise and then permitted it, which is worse than not checking. Failed files are now deleted, so a failed repair degrades to "absent" rather than "known-bad but present". Verified by planting a corrupt tabulate wheel: it is detected, removed, and gone even when the refetch fails. Writes also go through a temp file plus rename, so an interrupted install cannot leave a truncated wheel that fails every later check with no way to repair itself offline. The docs claimed more than that mechanism delivers, so they now say plainly that it is an integrity check against corruption, not a trust boundary: the worker matches on distribution name and does not re-verify at load time, and a file planted under a different version string is never examined. Anyone who can write to that directory can already run code as this user. Also tightened test_search_files_tolerates_unusable_patterns, where '[' and 'a[b' were passing vacuously -- they match literally rather than raising. The patterns that genuinely raise are now asserted to raise before checking that the tool swallows them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/sandbox_code_interpreter.md | 12 ++++- .../tests/test_sandbox_isolation.py | 49 ++++++++++++++++++- .../toolslib/sandbox/code_interpreter.py | 9 ++++ .../sandbox/scripts/download-wheels.mjs | 29 +++++++++-- 4 files changed, 90 insertions(+), 9 deletions(-) diff --git a/docs/sandbox_code_interpreter.md b/docs/sandbox_code_interpreter.md index 0d1ecd1..91cfdb3 100644 --- a/docs/sandbox_code_interpreter.md +++ b/docs/sandbox_code_interpreter.md @@ -397,11 +397,19 @@ npm install The `postinstall` script vendors wheels into `./wheels/`. Idempotent — re-running keeps existing wheels; force a refresh by deleting `wheels/`. Each wheel is checked against the SHA-256 that PyPI publishes in its metadata, -both on download and when reusing a cached file; a mismatch is refused rather -than written, since these wheels are installed into the interpreter. If PyPI is +on download and when reusing a cached file; a file that fails the check is +deleted rather than left in place, and writes go through a temp file plus +rename so an interrupted install cannot leave a truncated wheel. If PyPI is unreachable, missing wheels fall back to runtime fetch (slower startup, still works). +This is an integrity check against corruption and accidental substitution, not +a trust boundary. The worker loads whatever `wheels/` contains by matching on +the distribution name, so it does not re-verify at load time, and a file +planted under a different version string would not be examined by the +downloader at all. Anyone who can write to that directory can already run code +as this user (see [Threat model](#threat-model)). + **Enabling execution.** The interpreter fails closed: it refuses to spawn a worker unless the operator opts in, in the environment that launches the server. diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 4429f70..0ac026b 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -25,6 +25,7 @@ is modified. """ +import asyncio import json import os import subprocess @@ -541,6 +542,43 @@ def test_stream_limit_exceeds_asyncio_default(): assert code_interpreter.CodeInterpreter.STREAM_LIMIT >= 8 * 1024 * 1024 +@pytest.mark.asyncio +async def test_cancelled_execute_resets_worker(): + """Cancelling a call must reset the worker, not leave its reply buffered. + + The worker still writes a reply for the abandoned request. If the process + stays attached, the next execute() reads that stale frame and returns the + previous call's output -- a silently wrong answer rather than an error. + CancelledError is a BaseException, so nothing upstream catches this. + """ + + class _ParkingStdout: + def __init__(self): + self.release = asyncio.Event() + self.queued = [] + + async def readline(self): + if not self.release.is_set(): + await self.release.wait() + return self.queued.pop(0) if self.queued else b"" + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + proc = _FakeProcess([]) + proc.stdout = _ParkingStdout() + interp._process = proc + + task = asyncio.create_task(interp.execute("call_1()")) + await asyncio.sleep(0.05) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + assert proc.killed, "worker survived cancellation with an unread reply pending" + assert interp._process is None, ( + "the next execute() would reuse a stream holding the cancelled call's reply" + ) + + def test_search_files_tolerates_unusable_patterns(tmp_path): """Model-supplied glob patterns must not raise out of the tool. @@ -551,8 +589,15 @@ def test_search_files_tolerates_unusable_patterns(tmp_path): (tmp_path / "a.txt").write_text("x") sandbox = Sandbox(str(tmp_path)) - for pattern in ("", "***", "/etc/passwd", "C:\\Windows\\win.ini", "[", "a[b"): - assert sandbox.search_files(pattern) == [], f"pattern {pattern!r} returned matches" + # These genuinely raise out of Path.glob and must be caught. + for pattern in ("", "***", "/etc/passwd", "C:\\Windows\\win.ini"): + with pytest.raises((ValueError, NotImplementedError)): + list(tmp_path.glob(pattern)) + assert sandbox.search_files(pattern) == [], f"pattern {pattern!r} leaked" + + # These do not raise; they simply match nothing. Kept to pin that behavior. + for pattern in ("[", "a[b"): + assert sandbox.search_files(pattern) == [] # A valid pattern still works. assert sandbox.search_files("*.txt") == ["a.txt"] diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index 0ea15c8..8db714f 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -137,6 +137,15 @@ async def execute(self, code: str) -> ExecutionResult: f"Execution timed out after {self.timeout}s. " "The interpreter has been reset." ) + except asyncio.CancelledError: + # The caller went away (client disconnect, MCP cancellation, an + # outer wait_for) while the request was in flight. The worker + # will still write its reply, and that unread frame would be + # returned to the *next* execute() as its result -- a silently + # wrong answer rather than an error. CancelledError is a + # BaseException, so nothing upstream catches this for us. + await self._kill() + raise except (ValueError, asyncio.LimitOverrunError) as exc: # readline() raises when a frame exceeds STREAM_LIMIT. The # unread remainder would be parsed as the *next* response, so diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs index 5179f46..f9d1146 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -12,7 +12,7 @@ // (which works because micropip resolves them via pyodide's own bundle when // available, e.g. reportlab). -import { mkdir, writeFile, readFile } from "node:fs/promises"; +import { mkdir, writeFile, readFile, rm, rename } from "node:fs/promises"; import { createHash } from "node:crypto"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; @@ -59,15 +59,22 @@ async function downloadOne(name) { // Re-verify a cached wheel rather than trusting the filename: the cache // lives in a working directory that anything on this machine can write to. + // A wheel that fails the check is removed immediately -- leaving it in + // place would mean the check detects a bad wheel and then lets the worker + // install it anyway, since the worker loads whatever readdir() returns. try { const cached = await readFile(dest); if (sha256(cached) === expected) { console.log(`[download-wheels] Cached: ${wheel.filename}`); return; } - console.warn(`[download-wheels] Cached ${wheel.filename} failed digest check — refetching`); - } catch { - // not present — download below + console.warn(`[download-wheels] Cached ${wheel.filename} failed digest check — removing`); + await rm(dest, { force: true }); + } catch (err) { + if (err?.code !== "ENOENT") { + // Unreadable or undeletable: drop it rather than risk installing it. + await rm(dest, { force: true }).catch(() => {}); + } } console.log(`[download-wheels] Downloading: ${wheel.filename}`); @@ -91,7 +98,19 @@ async function downloadOne(name) { ); return; } - await writeFile(dest, body); + + // Write via a temp file and rename so an interrupted install cannot leave a + // truncated wheel behind. A truncated file would fail every later digest + // check, and with PyPI unreachable -- the case this vendoring exists for -- + // `npm install` could never repair it. + const tmp = `${dest}.${process.pid}.tmp`; + try { + await writeFile(tmp, body); + await rename(tmp, dest); + } catch (err) { + await rm(tmp, { force: true }).catch(() => {}); + throw err; + } } await Promise.all( From 5043e1a717770a55ac0c5127af14cb3003189acc Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 12:53:18 -0700 Subject: [PATCH 14/21] Make the glob-pattern test platform-agnostic The previous version asserted that 'C:\\Windows\\win.ini' raises from Path.glob. That holds on Windows, where it is an absolute path, but on Linux it is just a relative name containing backslashes, so glob returns no matches instead of raising -- and CI failed there. Only the pattern-syntax errors ('' and '***') raise on every platform, so those are what pin the guard as load-bearing. Everything else is asserted only to return no matches without raising, which is the property the tool actually needs. Also folded in '../*' to keep the traversal guard covered. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/test_sandbox_isolation.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 0ac026b..c52cf2f 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -589,15 +589,18 @@ def test_search_files_tolerates_unusable_patterns(tmp_path): (tmp_path / "a.txt").write_text("x") sandbox = Sandbox(str(tmp_path)) - # These genuinely raise out of Path.glob and must be caught. - for pattern in ("", "***", "/etc/passwd", "C:\\Windows\\win.ini"): - with pytest.raises((ValueError, NotImplementedError)): + # Pattern-syntax errors: these raise on every platform, so the guard around + # Path.glob is provably load-bearing rather than decorative. + for pattern in ("", "***"): + with pytest.raises(ValueError): list(tmp_path.glob(pattern)) assert sandbox.search_files(pattern) == [], f"pattern {pattern!r} leaked" - # These do not raise; they simply match nothing. Kept to pin that behavior. - for pattern in ("[", "a[b"): - assert sandbox.search_files(pattern) == [] + # Whether a given string is "absolute" is platform-dependent -- a Windows + # drive path is just a relative name containing backslashes on POSIX -- so + # only require that the tool never raises and never returns a match. + for pattern in ("/etc/passwd", "C:\\Windows\\win.ini", "[", "a[b", "../*"): + assert sandbox.search_files(pattern) == [], f"pattern {pattern!r} leaked" # A valid pattern still works. assert sandbox.search_files("*.txt") == ["a.txt"] From f2e28c6f8c33e1326f8d012691adfd06965a8870 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 13:00:32 -0700 Subject: [PATCH 15/21] Await the cancelled task explicitly in the regression test CodeQL flagged the bare 'await task' inside pytest.raises as a statement with no effect -- it cannot see that awaiting the task to observe its exception is the point. Replaced with an explicit try/except that names the outcome, which satisfies the analyzer and states the assertion more clearly. Behaviour unchanged and still falsifiable: removing the CancelledError handler makes the test fail with 'worker survived cancellation with an unread reply pending'. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../thinkingbox_tools/tests/test_sandbox_isolation.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index c52cf2f..13d9caf 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -570,8 +570,16 @@ async def readline(self): task = asyncio.create_task(interp.execute("call_1()")) await asyncio.sleep(0.05) task.cancel() - with pytest.raises(asyncio.CancelledError): + + # Await explicitly rather than relying on a bare `await task` inside + # pytest.raises: naming the outcome states what is being asserted, and a + # bare await reads as a no-op statement to static analysis. + cancelled = False + try: await task + except asyncio.CancelledError: + cancelled = True + assert cancelled, "execute() swallowed the cancellation instead of propagating it" assert proc.killed, "worker survived cancellation with an unread reply pending" assert interp._process is None, ( From 926810c700ba84b638a5c13e65c6f58d2d076a62 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 14:43:38 -0700 Subject: [PATCH 16/21] Pin wheels by digest; handle cancellation across the whole transaction Cancellation is now handled at every await in execute(), not just the read. _ensure_started() and the request write/drain were unguarded, so a cancellation there left the worker attached with a half-written request or an unconsumed handshake, and the next call read a frame that did not belong to it. _start() guards its handshake too: cancelled there, the child is spawned but not yet stored anywhere, so it would have been orphaned outright. _kill() now reaps reliably. It is reached almost exclusively from cancellation handlers, where a bare `await proc.wait()` is cancelled again immediately and returns before the child is collected. The wait is shielded and bounded, and the process handle is detached first so an interrupted kill can never leave a reusable reference behind. Wheels are pinned by exact version, filename and SHA-256 in pypi-packages.mjs. The worker previously resolved a package by scanning wheels/ for any file whose distribution segment matched, so readdir() order decided which artifact was installed -- a file named openpyxl-0.0.1-py3-none-any.whl would win. It now accepts only the pinned filename and otherwise falls back to `name==version` rather than a bare name. Demonstrated against the local cache: with pypdf 6.16.2 pinned and 6.16.1 present, the old matcher selected 6.16.1 while the new one declines it. download-wheels.mjs resolves that pinned release instead of whatever is current, and cross-checks PyPI's published digest against the pin before downloading, so a silent upstream change is a hard error rather than a new artifact. Integrity failures now fail the install. A corrupt cached wheel that cannot be removed, a file still present after removal, or a PyPI digest that disagrees with the pin all raise IntegrityError, which is rethrown after the batch and sets a non-zero exit. Only network failures still degrade to a runtime fetch. The oversize test drives a real subprocess emitting a 200 KB line rather than injecting a synthetic ValueError, with a control proving a 100 KB frame is still returned intact. Cancellation is covered at all three points: startup, drain, and read. The eval assertion no longer accepts a hard-coded answer. Naming sales.csv in a string literal counted as evidence of reading it, so `print("sales.csv: East 7312.50")` would have passed. It now requires an actual read call, and requires the figure to appear in interpreter output while being absent from the code that produced it. Verified both ways: the hard-coded form is rejected, the computed form accepted. The PR description said "the agent loaded all three files". No model run has happened -- code_interpreter was driven directly -- so it now says direct tool validation and states plainly that no code_execution trace exists yet. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- dataset/test_case/sandbox_code_interpreter.py | 63 ++++++-- .../tests/test_sandbox_isolation.py | 153 ++++++++++++++++++ .../toolslib/sandbox/code_interpreter.py | 79 ++++++--- .../toolslib/sandbox/pyodide_worker.mjs | 35 ++-- .../toolslib/sandbox/pypi-packages.mjs | 97 +++++++++-- .../sandbox/scripts/download-wheels.mjs | 151 ++++++++++------- 6 files changed, 457 insertions(+), 121 deletions(-) diff --git a/dataset/test_case/sandbox_code_interpreter.py b/dataset/test_case/sandbox_code_interpreter.py index 739209d..27c9016 100644 --- a/dataset/test_case/sandbox_code_interpreter.py +++ b/dataset/test_case/sandbox_code_interpreter.py @@ -56,6 +56,28 @@ def test_reads_workspace_file_through_interpreter(x: TestContext, judge: Judge): ) +def _reads_fixture(execution) -> bool: + """True when the code actually opens the fixture rather than naming it. + + Naming the file in a string literal is not evidence of reading it. + """ + code = execution.get("code", "") + if "sales.csv" not in code: + return False + return any( + marker in code + for marker in ("open(", "read_csv", "Path(", "csv.", "loadtxt", "genfromtxt") + ) + + +def _states_total(text: str) -> bool: + """True when the text contains East's revenue in any plausible formatting.""" + if not text: + return False + normalized = text.replace(",", "").replace("$", "") + return any(form in normalized for form in ("7312.5", "7312.50")) + + def test_computes_revenue_per_region(x: TestContext, judge: Judge): """! query: | @@ -66,18 +88,37 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): executions = _executions(x) assert executions, "the agent did not use the code interpreter" - # Every execution the agent kept should be error-free by the end; at minimum - # one must have succeeded, otherwise any correct-looking answer was invented. successful = [e for e in executions if e["result"].get("error") is None] assert successful, ( "every code execution failed, so the answer was not computed: " f"{[e['result'].get('error') for e in executions]}" ) - # The figures must come from code, not from the model doing mental math. - assert any( - "sales.csv" in e.get("code", "") for e in successful - ), "no successful execution referenced sales.csv" + # Naming the file is not reading it -- require an actual read call. + reading = [e for e in successful if _reads_fixture(e)] + assert reading, ( + "no successful execution actually read sales.csv; mentioning the " + f"filename is not enough: {[e.get('code') for e in successful]}" + ) + + # The decisive check: the figure must come *out* of the interpreter while + # being absent from the code that produced it. A response is only credible + # if the number was computed from the fixture, and an execution such as + # `print("sales.csv: East 7312.50")` would satisfy every check above while + # reading nothing -- so require the value in the output and not in the source. + computed = [ + e + for e in reading + if _states_total( + (e["result"].get("stdout") or "") + " " + (e["result"].get("result") or "") + ) + and not _states_total(e.get("code", "")) + ] + assert computed, ( + "East's revenue never appeared in interpreter output that did not " + "already contain it as a literal -- the figure was hard-coded rather " + "than computed from the fixture" + ) # East is the correct answer (7312.50). assert judge.text_yesno( @@ -86,12 +127,10 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): "revenue?", ) - # Guard against the most likely wrong answer: summing units instead of - # revenue would still make East highest, so check the figure was reported. - assert judge.text_yesno( - x.response, - "Does the response report East's total revenue as approximately 7312.50 " - "(accepting 7312.5, 7,312.50 or $7312.50)?", + # Guard against summing units instead of revenue, which would still put East + # first, by requiring the figure itself. + assert _states_total(x.response), ( + f"the response did not report East's revenue as 7312.50: {x.response!r}" ) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 13d9caf..61ae61a 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -587,6 +587,159 @@ async def readline(self): ) +@pytest.mark.asyncio +async def test_cancelled_during_startup_resets_worker(): + """Cancelling while the handshake is pending must not orphan the child. + + _start() has spawned the process but not yet returned it, so if the + cancellation escapes without a kill the child is unreachable from anywhere. + """ + interp = code_interpreter.CodeInterpreter(timeout=30.0) + spawned = {} + + async def fake_start(): + proc = _FakeProcess([]) + spawned["proc"] = proc + interp._process = proc + try: + await asyncio.Event().wait() # park, as the real handshake would + except asyncio.CancelledError: + await interp._kill() + raise + + interp._start = fake_start + + task = asyncio.create_task(interp.execute("x")) + await asyncio.sleep(0.05) + task.cancel() + + cancelled = False + try: + await task + except asyncio.CancelledError: + cancelled = True + assert cancelled + assert spawned["proc"].killed, "child spawned during startup was orphaned" + assert interp._process is None + + +@pytest.mark.asyncio +async def test_cancelled_during_drain_resets_worker(): + """Cancelling while flushing the request must reset the worker. + + The request is partially written, so the worker's view of the stream no + longer matches ours; reusing it would desynchronize the protocol. + """ + + class _ParkingStdin: + def __init__(self): + self.written = [] + + def write(self, data): + self.written.append(data) + + async def drain(self): + await asyncio.Event().wait() # never completes + + def close(self): + pass + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + proc = _FakeProcess([]) + proc.stdin = _ParkingStdin() + interp._process = proc + + task = asyncio.create_task(interp.execute("y")) + await asyncio.sleep(0.05) + task.cancel() + + cancelled = False + try: + await task + except asyncio.CancelledError: + cancelled = True + assert cancelled + assert proc.killed, "worker survived cancellation with a half-written request" + assert interp._process is None + + +@pytest.mark.asyncio +async def test_real_oversized_response_is_rejected(): + """A genuine over-limit frame must be reported and reset the worker. + + Drives a real subprocess emitting a single line larger than the configured + limit, so this exercises asyncio's actual StreamReader behaviour rather than + a synthetic ValueError. + """ + payload = 200_000 + child = ( + "import sys;" + f"sys.stdout.write('A' * {payload} + chr(10));" + "sys.stdout.flush()" + ) + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + # A small limit keeps the test fast while exercising the same code path the + # 64 MiB production value protects. + interp.STREAM_LIMIT = 64 * 1024 + + proc = await asyncio.create_subprocess_exec( + sys.executable, + "-c", + child, + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + limit=interp.STREAM_LIMIT, + ) + interp._process = proc + try: + with pytest.raises(code_interpreter.CodeInterpreterError) as excinfo: + await interp.execute("irrelevant") + message = str(excinfo.value) + assert "larger than" in message, message + assert "reset" in message, message + assert interp._process is None, "oversized frame left the worker attached" + finally: + if proc.returncode is None: + proc.kill() + await proc.wait() + + +@pytest.mark.asyncio +async def test_real_response_under_limit_is_returned(): + """Control for the test above: a large-but-permitted frame still works. + + The child builds the payload itself; embedding 100 KB in the command line + exceeds the OS argument limit on Windows. + """ + child = ( + "import json,sys;" + "sys.stdout.write(json.dumps(" + "{'stdout':'B'*100000,'stderr':'','result':None,'error':None}" + ") + chr(10));" + "sys.stdout.flush()" + ) + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + proc = await asyncio.create_subprocess_exec( + sys.executable, + "-c", + child, + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + limit=interp.STREAM_LIMIT, + ) + interp._process = proc + try: + result = await interp.execute("irrelevant") + assert len(result.stdout) == 100_000, len(result.stdout) + assert result.stdout.startswith("B") + finally: + if proc.returncode is None: + proc.kill() + await proc.wait() + + def test_search_files_tolerates_unusable_patterns(tmp_path): """Model-supplied glob patterns must not raise out of the tool. diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index 8db714f..990f416 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -98,6 +98,9 @@ class CodeInterpreter: # memory for a runaway producer. STREAM_LIMIT = 64 * 1024 * 1024 + # Bound on reaping a killed worker, so a wedged child cannot stall teardown. + KILL_REAP_TIMEOUT = 5.0 + def __init__(self, timeout: float = 30.0, workspace_dir: str | None = None): self.timeout = timeout self.workspace_dir = workspace_dir @@ -120,11 +123,24 @@ def __init__(self, timeout: float = 30.0, workspace_dir: str | None = None): async def execute(self, code: str) -> ExecutionResult: async with self._lock: - await self._ensure_started() + # Cancellation is handled across the whole transaction, not just the + # read. At any await here the caller can go away (client + # disconnect, MCP cancellation, an outer wait_for). If the worker + # is left attached, its pending reply -- or a half-written request + # -- desynchronizes the stream, and the *next* execute() returns + # someone else's output as its own result. That is a silently wrong + # answer rather than an error, so every cancellation point resets + # the worker. CancelledError is a BaseException; nothing upstream + # catches it for us. + try: + await self._ensure_started() - request = json.dumps({"code": code}) + "\n" - self._process.stdin.write(request.encode()) - await self._process.stdin.drain() + request = json.dumps({"code": code}) + "\n" + self._process.stdin.write(request.encode()) + await self._process.stdin.drain() + except asyncio.CancelledError: + await self._kill() + raise try: response_line = await asyncio.wait_for( @@ -138,12 +154,6 @@ async def execute(self, code: str) -> ExecutionResult: "The interpreter has been reset." ) except asyncio.CancelledError: - # The caller went away (client disconnect, MCP cancellation, an - # outer wait_for) while the request was in flight. The worker - # will still write its reply, and that unread frame would be - # returned to the *next* execute() as its result -- a silently - # wrong answer rather than an error. CancelledError is a - # BaseException, so nothing upstream catches this for us. await self._kill() raise except (ValueError, asyncio.LimitOverrunError) as exc: @@ -294,6 +304,12 @@ async def _start(self) -> None: f"Worker timed out during startup (>{self.STARTUP_TIMEOUT}s). " f"Make sure 'npm install' has been run in {worker_dir}." ) + except asyncio.CancelledError: + # Cancelled mid-handshake: the child is spawned but never handed + # over, so without this it would be orphaned entirely -- no + # reference is stored anywhere the caller could reach. + await self._kill() + raise except (ValueError, asyncio.LimitOverrunError) as exc: await self._kill() raise CodeInterpreterError( @@ -321,15 +337,34 @@ async def _start(self) -> None: raise CodeInterpreterError(f"Unexpected worker handshake: {ready}") async def _kill(self) -> None: - if self._process: - try: - self._process.kill() - await self._process.wait() - except Exception: - # Best effort: the worker may already be dead, or reaping it may - # race with the event loop shutting down. Either way the process - # handle is dropped below and the next call spawns a fresh one, - # so there is nothing useful to recover or report here. - pass - finally: - self._process = None + # Detach first: whatever happens below, the next execute() must not + # reuse this process. If _kill() is itself interrupted, a dropped + # handle is recoverable; a retained one is not. + proc = self._process + self._process = None + if proc is None: + return + + try: + proc.kill() + except (ProcessLookupError, OSError): + # Already exited; there is still a zombie to reap below on POSIX. + pass + + # Reap the child so it does not linger. shield() matters here because + # _kill() is normally reached from a cancellation handler: a bare await + # would be cancelled again immediately and return before the process was + # reaped. Shielding lets the wait() finish in the background even when + # our own await is interrupted. + waiter = asyncio.shield(proc.wait()) + try: + await asyncio.wait_for(waiter, timeout=self.KILL_REAP_TIMEOUT) + except asyncio.CancelledError: + # Our await was cancelled, not the reap. The shielded wait() keeps + # running and collects the child. Swallow rather than propagate: + # the caller re-raises the original cancellation. + pass + except Exception: + # Timed out, already reaped, or the loop is shutting down. Nothing + # useful to recover or report; the handle is already dropped. + pass diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs index 1143ffa..54a198c 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs @@ -66,31 +66,28 @@ await pyodide.loadPackage(BUNDLED_PACKAGES, { errorCallback: (err) => process.stderr.write(err + "\n"), }); -// Resolve each PyPI package to a local wheel (file:// URL) when one was -// vendored by `npm install` into ./wheels/ — avoids hitting PyPI on every -// worker startup. Anything without a local wheel falls back to a bare -// package name, which micropip resolves via PyPI or via pyodide's own -// bundled wheel set (e.g. reportlab). +// Resolve each pinned package to its exact vendored wheel. Only the filename +// recorded in pypi-packages.mjs is accepted: matching on the distribution name +// would let any file in wheels/ win on readdir() order, so a planted +// "openpyxl-0.0.1-py3-none-any.whl" would be installed ahead of the real one. +// A package whose exact wheel is absent falls back to the pinned +// "name==version" spec rather than a bare name, so micropip still resolves a +// known version instead of whatever is current. const wheelsDir = new URL("./wheels/", import.meta.url); -const normalizePkg = (s) => s.toLowerCase().replace(/[-._]/g, "_"); -let localWheels = []; +let localWheels = new Set(); try { - localWheels = await readdir(wheelsDir); + localWheels = new Set(await readdir(wheelsDir)); } catch { process.stderr.write("[pyodide_worker] No local wheels/ directory — micropip will fetch from PyPI\n"); } const installSpecs = PYPI_PACKAGES.map((pkg) => { - const target = normalizePkg(pkg); - // PEP 427 wheel filename: {distribution}-{version}(-{build})?-{python}-{abi}-{platform}.whl - // Compare the normalized distribution segment, not a normalized prefix — - // normalizePkg(f) rewrites the "-" separators to "_", so a "-"-suffixed - // prefix could never match. - const match = localWheels.find((f) => { - if (!f.endsWith(".whl")) return false; - const dist = f.split("-")[0]; - return normalizePkg(dist) === target; - }); - return match ? new URL(match, wheelsDir).href : pkg; + if (localWheels.has(pkg.filename)) { + return new URL(pkg.filename, wheelsDir).href; + } + process.stderr.write( + `[pyodide_worker] No vendored ${pkg.filename} — falling back to ${pkg.name}==${pkg.version}\n`, + ); + return `${pkg.name}==${pkg.version}`; }); process.stderr.write("[pyodide_worker] Installing PyPI packages via micropip...\n"); diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs index 3c0eb3e..b204a63 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs @@ -2,19 +2,88 @@ // lock, so pyodide.loadPackagesFromImports() cannot auto-load them — they // must be installed via micropip. At `npm install` time, scripts/download-wheels.mjs // downloads each into ./wheels/, and the worker installs from local file:// -// URLs to avoid hitting PyPI on every worker startup. Anything that can't -// be vendored (e.g. no pure-Python wheel published) falls back to a bare -// package name and micropip fetches it at runtime. +// URLs to avoid hitting PyPI on every worker startup. +// +// Each entry pins the exact version, wheel filename and SHA-256 from PyPI's +// metadata. Pinning matters in two places: +// +// - download-wheels.mjs fetches that exact release rather than whatever is +// current, so `npm install` is reproducible and the digest is meaningful. +// - pyodide_worker.mjs installs only a file whose name matches `filename` +// exactly. Matching on the distribution name alone would let any file in +// wheels/ named e.g. "openpyxl-0.0.1-py3-none-any.whl" be installed +// instead, since readdir() order decides the winner. +// +// A package with no local wheel falls back to the pinned `name==version` +// spec, so micropip resolves a known version rather than "latest". +// +// To refresh: update version, filename and sha256 together from +// https://pypi.org/pypi//json — never edit one without the others. export const PYPI_PACKAGES = [ - "openpyxl", - "xlsxwriter", - "markdownify", - "mammoth", - "pypdf", - "pdfminer.six", - "tabulate", - "plotly", - "python-docx", - "python-pptx", - "reportlab", + { + name: "openpyxl", + version: "3.1.5", + filename: "openpyxl-3.1.5-py2.py3-none-any.whl", + sha256: "5282c12b107bffeef825f4617dc029afaf41d0ea60823bbb665ef3079dc79de2", + }, + { + name: "xlsxwriter", + version: "3.2.9", + filename: "xlsxwriter-3.2.9-py3-none-any.whl", + sha256: "9a5db42bc5dff014806c58a20b9eae7322a134abb6fce3c92c181bfb275ec5b3", + }, + { + name: "markdownify", + version: "1.2.3", + filename: "markdownify-1.2.3-py3-none-any.whl", + sha256: "a189a0bedfd14009030fde5f85bb6f77c56897cb839b5c25315dd7d4e3e290ba", + }, + { + name: "mammoth", + version: "1.12.1", + filename: "mammoth-1.12.1-py2.py3-none-any.whl", + sha256: "2af047e3e796faa25740112310ddf11f8de2a24c96dc57de3c87dfd7cb6543b3", + }, + { + name: "pypdf", + version: "6.16.2", + filename: "pypdf-6.16.2-py3-none-any.whl", + sha256: "c8b09a59399062fb45a1b8156c18a787a10a3dae03ac9674397a226712c94604", + }, + { + name: "pdfminer.six", + version: "20260107", + filename: "pdfminer_six-20260107-py3-none-any.whl", + sha256: "366585ba97e80dffa8f00cebe303d2f381884d8637af4ce422f1df3ef38111a9", + }, + { + name: "tabulate", + version: "0.10.0", + filename: "tabulate-0.10.0-py3-none-any.whl", + sha256: "f0b0622e567335c8fabaaa659f1b33bcb6ddfe2e496071b743aa113f8774f2d3", + }, + { + name: "plotly", + version: "7.0.0", + filename: "plotly-7.0.0-py3-none-any.whl", + sha256: "78cbf7bd06d1b05bb3b8ec1b709864695229b55151b6f7530fbf55517ead6fdd", + }, + { + name: "python-docx", + version: "1.2.0", + filename: "python_docx-1.2.0-py3-none-any.whl", + sha256: "3fd478f3250fbbbfd3b94fe1e985955737c145627498896a8a6bf81f4baf66c7", + }, + { + name: "python-pptx", + version: "1.0.2", + filename: "python_pptx-1.0.2-py3-none-any.whl", + sha256: "160838e0b8565a8b1f67947675886e9fea18aa5e795db7ae531606d68e785cba", + }, + { + name: "reportlab", + version: "5.0.1", + filename: "reportlab-5.0.1-py3-none-any.whl", + sha256: "1c36e6bb0e71780c72331eba60da7f602e8d4389a8723825af71342e49d791e8", + }, ]; diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs index f9d1146..e65b02f 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -1,18 +1,19 @@ #!/usr/bin/env node -// Vendors pure-Python wheels for the PyPI packages listed in pypi-packages.mjs -// into ../wheels/, so the pyodide worker can install them from local file:// -// URLs instead of fetching from PyPI on every startup. +// Vendors the exact pinned wheels listed in pypi-packages.mjs into ../wheels/, +// so the pyodide worker can install them from local file:// URLs instead of +// fetching from PyPI on every startup. // // Runs automatically via `npm install` (see package.json "postinstall"). -// Idempotent: existing wheel files are kept. To force a refresh, delete -// the wheels/ directory and re-run `npm install`. +// Idempotent: a cached wheel whose SHA-256 matches the pin is kept. // -// Packages without a pure-Python (`*-none-any.whl`) wheel on PyPI are skipped -// with a warning; the worker falls back to micropip at runtime for those -// (which works because micropip resolves them via pyodide's own bundle when -// available, e.g. reportlab). +// Network failures degrade gracefully -- the wheel is skipped and the worker +// falls back to the pinned `name==version` spec at runtime. Integrity +// failures do NOT degrade: if a bad file cannot be removed, or PyPI's digest +// disagrees with the pin, the install fails rather than leaving something the +// worker would load. import { mkdir, writeFile, readFile, rm, rename } from "node:fs/promises"; +import { existsSync } from "node:fs"; import { createHash } from "node:crypto"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; @@ -25,59 +26,84 @@ const sha256 = (buf) => createHash("sha256").update(buf).digest("hex"); await mkdir(wheelsDir, { recursive: true }); -const PURE_PYTHON_WHEEL = /-py[23](\.py3)?-none-any\.whl$/; - // A stalled connection would otherwise hang `npm install` indefinitely, since // fetch() has no default timeout. Bounding it lets the script fail fast and // degrade to a runtime micropip fetch, as documented. const METADATA_TIMEOUT_MS = 30_000; const DOWNLOAD_TIMEOUT_MS = 120_000; -async function downloadOne(name) { - const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/json`, { - signal: AbortSignal.timeout(METADATA_TIMEOUT_MS), - }); - if (!res.ok) { - console.warn(`[download-wheels] PyPI returned ${res.status} for ${name} — skipping`); - return; - } - const data = await res.json(); - const version = data.info.version; - const wheel = (data.releases[version] || []).find( - (f) => f.packagetype === "bdist_wheel" && PURE_PYTHON_WHEEL.test(f.filename), - ); - if (!wheel) { - console.log(`[download-wheels] No pure-Python wheel for ${name} ${version} — leaving to runtime`); - return; - } - const dest = join(wheelsDir, wheel.filename); - const expected = wheel.digests?.sha256; - if (!expected) { - console.warn(`[download-wheels] No sha256 published for ${wheel.filename} — leaving to runtime`); - return; - } +// Raised for conditions that must abort the install rather than degrade. +class IntegrityError extends Error {} + +async function downloadOne(pkg) { + const { name, version, filename, sha256: expected } = pkg; + const dest = join(wheelsDir, filename); // Re-verify a cached wheel rather than trusting the filename: the cache // lives in a working directory that anything on this machine can write to. - // A wheel that fails the check is removed immediately -- leaving it in - // place would mean the check detects a bad wheel and then lets the worker - // install it anyway, since the worker loads whatever readdir() returns. + // A wheel that fails the check must be removed -- leaving it in place would + // mean the check detects a bad wheel and the worker installs it anyway. + // If it cannot be removed, fail: continuing would leave a known-bad file + // where the worker will pick it up. try { const cached = await readFile(dest); if (sha256(cached) === expected) { - console.log(`[download-wheels] Cached: ${wheel.filename}`); + console.log(`[download-wheels] Cached: ${filename}`); return; } - console.warn(`[download-wheels] Cached ${wheel.filename} failed digest check — removing`); + console.warn(`[download-wheels] Cached ${filename} failed digest check — removing`); await rm(dest, { force: true }); } catch (err) { if (err?.code !== "ENOENT") { - // Unreadable or undeletable: drop it rather than risk installing it. - await rm(dest, { force: true }).catch(() => {}); + // Unreadable, or the removal above threw. Try once more and fail + // loudly if the bad file survives. + try { + await rm(dest, { force: true }); + } catch (rmErr) { + throw new IntegrityError( + `Refusing to continue: ${dest} failed its integrity check and could ` + + `not be removed (${rmErr.message}). Delete it manually before ` + + `re-running, or the worker will install it.`, + ); + } } } - console.log(`[download-wheels] Downloading: ${wheel.filename}`); + // Guard against a removal that silently did not happen (permissions, a + // read-only mount, or a file recreated by something else). + if (existsSync(dest)) { + throw new IntegrityError( + `Refusing to continue: ${dest} failed its integrity check and is still ` + + `present after removal. Delete it manually before re-running.`, + ); + } + + console.log(`[download-wheels] Downloading: ${filename}`); + // Resolve the pinned release, not data.info.version: `npm install` must be + // reproducible, and a digest is only meaningful against a fixed artifact. + const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/${encodeURIComponent(version)}/json`, { + signal: AbortSignal.timeout(METADATA_TIMEOUT_MS), + }); + if (!res.ok) { + console.warn(`[download-wheels] PyPI returned ${res.status} for ${name} ${version} — leaving to runtime`); + return; + } + const data = await res.json(); + const wheel = (data.urls || []).find((f) => f.filename === filename); + if (!wheel) { + console.warn(`[download-wheels] ${filename} not found in ${name} ${version} — leaving to runtime`); + return; + } + if (wheel.digests?.sha256 !== expected) { + // PyPI's own digest disagrees with the pin: either the pin is stale or + // the artifact changed. Either way, do not download it. + throw new IntegrityError( + `Refusing to download ${filename}: PyPI reports sha256 ` + + `${wheel.digests?.sha256}, pinned value is ${expected}. ` + + `Update pypi-packages.mjs deliberately if this is an intended bump.`, + ); + } + const wRes = await fetch(wheel.url, { signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS), }); @@ -87,13 +113,12 @@ async function downloadOne(name) { } // These wheels are installed into the interpreter, so a corrupted or - // substituted file is code execution. PyPI publishes a sha256 in the - // metadata; refuse to write anything that does not match it. + // substituted file is code execution. Verify the bytes actually received. const body = Buffer.from(await wRes.arrayBuffer()); const actual = sha256(body); if (actual !== expected) { console.warn( - `[download-wheels] DIGEST MISMATCH for ${wheel.filename} ` + + `[download-wheels] DIGEST MISMATCH for ${filename} ` + `(expected ${expected}, got ${actual}) — refusing to write, leaving to runtime`, ); return; @@ -113,16 +138,34 @@ async function downloadOne(name) { } } -await Promise.all( - PYPI_PACKAGES.map((name) => +const results = await Promise.all( + PYPI_PACKAGES.map((pkg) => // A network failure (offline, blocked host, TLS error) rejects fetch() - // rather than returning a non-ok response. Without this catch the - // rejection propagates out of Promise.all and fails `npm install` - // outright, instead of degrading to the documented behavior: skip the - // wheel and let micropip fetch it at worker startup. - downloadOne(name).catch((err) => { - console.warn(`[download-wheels] ${name}: ${err.message} — leaving to runtime`); - }), + // rather than returning a non-ok response, and must degrade to the + // documented behavior: skip the wheel, let micropip fetch the pinned + // version at worker startup. An IntegrityError is different in kind -- + // it means a file the worker would load cannot be trusted -- so it is + // rethrown below and fails the install. + downloadOne(pkg).then( + () => null, + (err) => { + if (err instanceof IntegrityError) return err; + console.warn(`[download-wheels] ${pkg.name}: ${err.message} — leaving to runtime`); + return null; + }, + ), ), ); + +const integrityFailures = results.filter(Boolean); +if (integrityFailures.length > 0) { + for (const err of integrityFailures) { + console.error(`[download-wheels] INTEGRITY FAILURE: ${err.message}`); + } + process.exitCode = 1; + throw new Error( + `${integrityFailures.length} wheel(s) failed integrity checks and could not be ` + + `quarantined. Refusing to complete installation.`, + ); +} console.log("[download-wheels] Done."); From a44e0394d315f89ba3df6d0a94cc7fef5a1798ed Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 16:25:58 -0700 Subject: [PATCH 17/21] Fail on digest mismatch; scope cancellation swallowing to cancel paths A downloaded wheel whose bytes disagree with the pinned SHA-256 now raises IntegrityError instead of warning and returning. Degrading to a runtime micropip fetch meant the pinned artifact could not be obtained and the worker would install whatever PyPI served next, which is the opposite of what pinning is for. Regression test drives the real script against a local stub PyPI that advertises the pinned digest and serves different bytes; confirmed falsifiable by restoring the old behaviour, where it fails with "install succeeded despite a digest mismatch". Two env seams were added to make that testable -- THINKINGBOX_PYPI_BASE_URL and THINKINGBOX_WHEELS_DIR -- both documented as test-only, with production defaults unchanged. _kill() no longer swallows cancellation unconditionally. It takes during_cancellation, set only by the three callers that are already unwinding a CancelledError and will re-raise it. Reached from a timeout or malformed-frame handler, a cancellation arriving during the shielded reap is a new external request to stop, and dropping it would leave the task running. Shielded reaping is unchanged. Tests cover both directions. close() gained the same treatment: it caught (TimeoutError, Exception), which does not include CancelledError, so cancelling a teardown dropped the child without killing it. The startup-cancellation test now exercises the real _start(). It previously substituted a fake that performed its own cleanup, so it verified the test's own code rather than the implementation. Only create_subprocess_exec is stubbed; the spawn, handshake read, cancellation handling, kill and reap are all real. Writing it surfaced that _kill() deliberately retains the worker temp dir for restart, so the test now asserts the actual contract -- close() reclaims it -- instead of an eager delete that would break reuse. The dataset assertion now compares exact structured output derived from the fixture. Ground truth is computed from sales.csv at test time rather than hard-coded, so editing the fixture changes what the test demands. It requires every per-region total to appear in interpreter output and none of them to appear as literals in the code that produced it. Verified against four cases: computed totals accepted; totals hard-coded into a print() rejected; only the top total rejected; unit sums instead of revenue rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- dataset/test_case/sandbox_code_interpreter.py | 101 ++++--- .../tests/test_sandbox_isolation.py | 263 ++++++++++++++++-- .../toolslib/sandbox/code_interpreter.py | 47 ++-- .../sandbox/scripts/download-wheels.mjs | 27 +- 4 files changed, 364 insertions(+), 74 deletions(-) diff --git a/dataset/test_case/sandbox_code_interpreter.py b/dataset/test_case/sandbox_code_interpreter.py index 27c9016..a358420 100644 --- a/dataset/test_case/sandbox_code_interpreter.py +++ b/dataset/test_case/sandbox_code_interpreter.py @@ -1,6 +1,11 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. +import csv +import os +import re +from pathlib import Path + from thinkingbox.common import Judge, TestContext """! @@ -56,6 +61,35 @@ def test_reads_workspace_file_through_interpreter(x: TestContext, judge: Judge): ) +def _expected_revenue_by_region() -> dict[str, float]: + """Compute the ground truth from the fixture itself. + + Derived rather than hard-coded so the assertion cannot drift from the data: + editing sales.csv changes what the test demands. + """ + fixture = ( + Path(os.environ.get("THINKINGBOX_DATA", ".")) + / "support" + / "sandbox_workspace" + / "reports" + / "sales.csv" + ) + totals: dict[str, float] = {} + with open(fixture, newline="") as handle: + for row in csv.DictReader(handle): + revenue = int(row["units"]) * float(row["unit_price"]) + totals[row["region"]] = round(totals.get(row["region"], 0.0) + revenue, 2) + return totals + + +def _numbers_in(text: str) -> set[float]: + """Every number in `text`, normalised so 7,312.50 and $7312.5 both match.""" + if not text: + return set() + cleaned = text.replace(",", "").replace("$", "") + return {float(m) for m in re.findall(r"-?\d+\.\d+|-?\d+", cleaned)} + + def _reads_fixture(execution) -> bool: """True when the code actually opens the fixture rather than naming it. @@ -70,14 +104,6 @@ def _reads_fixture(execution) -> bool: ) -def _states_total(text: str) -> bool: - """True when the text contains East's revenue in any plausible formatting.""" - if not text: - return False - normalized = text.replace(",", "").replace("$", "") - return any(form in normalized for form in ("7312.5", "7312.50")) - - def test_computes_revenue_per_region(x: TestContext, judge: Judge): """! query: | @@ -85,6 +111,10 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): unit price, summed across quarters) and tell me which region has the highest revenue. """ + expected = _expected_revenue_by_region() + assert expected, "fixture produced no ground truth; check THINKINGBOX_DATA" + top_region = max(expected, key=expected.__getitem__) + executions = _executions(x) assert executions, "the agent did not use the code interpreter" @@ -101,36 +131,43 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): f"filename is not enough: {[e.get('code') for e in successful]}" ) - # The decisive check: the figure must come *out* of the interpreter while - # being absent from the code that produced it. A response is only credible - # if the number was computed from the fixture, and an execution such as - # `print("sales.csv: East 7312.50")` would satisfy every check above while - # reading nothing -- so require the value in the output and not in the source. - computed = [ - e - for e in reading - if _states_total( - (e["result"].get("stdout") or "") + " " + (e["result"].get("result") or "") + # The decisive check: every per-region total derived from the fixture must + # appear in interpreter output, and none of them may appear in the code that + # produced it. Reproducing four independent totals that match the file to + # the cent is not something a model can do by writing them into a print() + # without having read the data -- and if it does write them in, the second + # half of this check rejects it. + wanted = set(expected.values()) + for execution in reading: + produced = _numbers_in( + (execution["result"].get("stdout") or "") + + " " + + (execution["result"].get("result") or "") + ) + if not wanted.issubset(produced): + continue + if _numbers_in(execution.get("code", "")) & wanted: + continue # the totals were literals in the source, not computed + break + else: + raise AssertionError( + "no execution produced all per-region totals " + f"{sorted(wanted)} as output without also containing them as " + "literals in its code -- the figures were not computed from the fixture" ) - and not _states_total(e.get("code", "")) - ] - assert computed, ( - "East's revenue never appeared in interpreter output that did not " - "already contain it as a literal -- the figure was hard-coded rather " - "than computed from the fixture" - ) - # East is the correct answer (7312.50). + # And the reported answer must name the right region. assert judge.text_yesno( x.response, - "Does the response identify East as the region with the highest total " - "revenue?", + f"Does the response identify {top_region} as the region with the " + "highest total revenue?", ) - # Guard against summing units instead of revenue, which would still put East - # first, by requiring the figure itself. - assert _states_total(x.response), ( - f"the response did not report East's revenue as 7312.50: {x.response!r}" + # Guard against summing units instead of revenue, which would still put + # the same region first, by requiring the exact figure. + assert expected[top_region] in _numbers_in(x.response), ( + f"the response did not report {top_region}'s revenue as " + f"{expected[top_region]}: {x.response!r}" ) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 61ae61a..8d8e53e 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -28,9 +28,11 @@ import asyncio import json import os +import shutil import subprocess import sys import uuid +from pathlib import Path from types import SimpleNamespace import pytest @@ -588,29 +590,55 @@ async def readline(self): @pytest.mark.asyncio -async def test_cancelled_during_startup_resets_worker(): - """Cancelling while the handshake is pending must not orphan the child. - - _start() has spawned the process but not yet returned it, so if the - cancellation escapes without a kill the child is unreachable from anywhere. +async def test_cancelled_during_startup_resets_worker(tmp_path, monkeypatch): + """Cancelling during the real _start() handshake must not orphan the child. + + Exercises the actual _start(): only `create_subprocess_exec` is stubbed, so + the spawn, the handshake read and the cancellation handling are the real + code paths. At that point the child exists but has not been handed back to + execute(), so a cancellation that escapes without a kill leaves a process + nothing can reach. """ - interp = code_interpreter.CodeInterpreter(timeout=30.0) + # _start() refuses to run unless the worker script and node_modules exist. + worker_dir = tmp_path / "sandbox" + (worker_dir / "node_modules" / "pyodide").mkdir(parents=True) + (worker_dir / "pyodide_worker.mjs").write_text("// stub") + + class _NeverReadyStdout: + async def readline(self): + await asyncio.Event().wait() # handshake never arrives + + class _SpawnedProc: + def __init__(self): + self.stdout = _NeverReadyStdout() + self.stdin = _FakeStdin() + self.returncode = None + self.killed = False + self.reaped = False + + def kill(self): + self.killed = True + self.returncode = -9 + + async def wait(self): + self.reaped = True + return self.returncode + spawned = {} - async def fake_start(): - proc = _FakeProcess([]) + async def fake_exec(*args, **kwargs): + proc = _SpawnedProc() spawned["proc"] = proc - interp._process = proc - try: - await asyncio.Event().wait() # park, as the real handshake would - except asyncio.CancelledError: - await interp._kill() - raise + return proc - interp._start = fake_start + monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_exec) + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + interp._worker_path = worker_dir / "pyodide_worker.mjs" task = asyncio.create_task(interp.execute("x")) await asyncio.sleep(0.05) + assert "proc" in spawned, "the stubbed spawn never happened; _start() bailed early" task.cancel() cancelled = False @@ -618,8 +646,94 @@ async def fake_start(): await task except asyncio.CancelledError: cancelled = True - assert cancelled - assert spawned["proc"].killed, "child spawned during startup was orphaned" + assert cancelled, "cancellation did not propagate out of execute()" + + proc = spawned["proc"] + assert proc.killed, "child spawned during the handshake was orphaned" + assert proc.reaped, "child was killed but never reaped" + assert interp._process is None + + # _kill() deliberately keeps the worker temp dir so a restart can reuse it + # rather than accumulating one per failed attempt. The contract is that + # close() reclaims it, so verify that rather than expecting an eager delete. + leaked = interp._worker_tmp + assert leaked is not None and os.path.isdir(leaked) + await interp.close() + assert interp._worker_tmp is None + assert not os.path.exists(leaked), "close() did not reclaim the worker temp dir" + + +@pytest.mark.asyncio +async def test_kill_propagates_new_cancellation_outside_cancel_paths(): + """A cancellation arriving during a non-cancellation kill must not be lost. + + _kill() swallows CancelledError only when the caller is already unwinding + one and will re-raise it. Called from a timeout or malformed-frame handler, + a cancellation arriving now is a new external request to stop, and dropping + it would let the task keep running. + """ + + class _UnreapableProc: + def __init__(self): + self.returncode = None + self.killed = False + + def kill(self): + self.killed = True + + async def wait(self): + await asyncio.Event().wait() # never completes + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + proc = _UnreapableProc() + interp._process = proc + + # during_cancellation defaults to False: the new cancellation must escape. + task = asyncio.create_task(interp._kill()) + await asyncio.sleep(0.05) + task.cancel() + + propagated = False + try: + await task + except asyncio.CancelledError: + propagated = True + assert propagated, "_kill() swallowed a cancellation from a non-cancellation path" + assert proc.killed + assert interp._process is None + + +@pytest.mark.asyncio +async def test_kill_swallows_cancellation_when_already_unwinding(): + """The converse: during cancellation, _kill() must not mask the original.""" + + class _UnreapableProc: + def __init__(self): + self.returncode = None + self.killed = False + + def kill(self): + self.killed = True + + async def wait(self): + await asyncio.Event().wait() + + interp = code_interpreter.CodeInterpreter(timeout=30.0) + proc = _UnreapableProc() + interp._process = proc + + task = asyncio.create_task(interp._kill(during_cancellation=True)) + await asyncio.sleep(0.05) + task.cancel() + + # The task is cancelled from outside, so awaiting it still raises; what + # matters is that _kill() itself did not convert that into a different + # error and that the process was detached. + try: + await task + except asyncio.CancelledError: + pass + assert proc.killed assert interp._process is None @@ -740,6 +854,121 @@ async def test_real_response_under_limit_is_returned(): await proc.wait() +# --------------------------------------------------------------------------- +# Wheel vendoring integrity +# --------------------------------------------------------------------------- + +_DOWNLOADER = ( + Path(mcp_sandbox.__file__).parent + / "toolslib" + / "sandbox" + / "scripts" + / "download-wheels.mjs" +) + + +def _pinned_packages(): + """The pinned manifest, read through node so the test uses the real source.""" + out = subprocess.run( + [ + "node", + "-e", + "import('./pypi-packages.mjs').then(m=>console.log(JSON.stringify(m.PYPI_PACKAGES)))", + ], + cwd=str(_DOWNLOADER.parent.parent), + capture_output=True, + text=True, + check=True, + ).stdout + return json.loads(out) + + +def _run_downloader(tmp_path, *, serve_body: bytes): + """Run download-wheels.mjs against a local stub PyPI and a scratch wheels dir. + + The stub advertises each package's pinned digest but serves `serve_body`, + so the "downloaded bytes disagree with the pin" path is exercised without + touching the network or the real wheels cache. + """ + if shutil.which("node") is None: + pytest.skip("node is not available") + + import http.server + import threading + + by_name = {p["name"]: p for p in _pinned_packages()} + wheels_dir = tmp_path / "wheels" + + class Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self): # noqa: N802 + if self.path.endswith("/json"): + # ///json + name = self.path.strip("/").split("/")[0] + pin = by_name.get(name) + if pin is None: + self.send_response(404) + self.end_headers() + return + payload = json.dumps( + { + "urls": [ + { + "filename": pin["filename"], + "packagetype": "bdist_wheel", + "digests": {"sha256": pin["sha256"]}, + "url": f"http://127.0.0.1:{self.server.server_port}/wheel", + } + ] + } + ).encode() + else: + payload = serve_body + self.send_response(200) + self.send_header("Content-Length", str(len(payload))) + self.end_headers() + self.wfile.write(payload) + + def log_message(self, *args): + pass + + server = http.server.HTTPServer(("127.0.0.1", 0), Handler) + threading.Thread(target=server.serve_forever, daemon=True).start() + try: + env = { + **os.environ, + "THINKINGBOX_PYPI_BASE_URL": f"http://127.0.0.1:{server.server_port}", + "THINKINGBOX_WHEELS_DIR": str(wheels_dir), + } + result = subprocess.run( + ["node", str(_DOWNLOADER)], + capture_output=True, + text=True, + env=env, + timeout=120, + ) + return result, wheels_dir + finally: + server.shutdown() + + +def test_downloaded_bytes_failing_digest_abort_the_install(tmp_path): + """Bytes that disagree with the pinned digest must fail, not fall back. + + Degrading to a runtime micropip fetch would silently install whatever PyPI + serves next instead of surfacing that the pinned artifact was unobtainable. + """ + result, wheels_dir = _run_downloader(tmp_path, serve_body=b"NOT THE REAL WHEEL") + + combined = result.stdout + result.stderr + assert result.returncode != 0, ( + f"install succeeded despite a digest mismatch:\n{combined}" + ) + assert "does not match its pinned digest" in combined, combined + + written = list(wheels_dir.glob("*.whl")) if wheels_dir.exists() else [] + assert not written, f"a wheel was written despite failing its digest: {written}" + + def test_search_files_tolerates_unusable_patterns(tmp_path): """Model-supplied glob patterns must not raise out of the tool. diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py index 990f416..cbaebc8 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py @@ -139,7 +139,7 @@ async def execute(self, code: str) -> ExecutionResult: self._process.stdin.write(request.encode()) await self._process.stdin.drain() except asyncio.CancelledError: - await self._kill() + await self._kill(during_cancellation=True) raise try: @@ -154,7 +154,7 @@ async def execute(self, code: str) -> ExecutionResult: "The interpreter has been reset." ) except asyncio.CancelledError: - await self._kill() + await self._kill(during_cancellation=True) raise except (ValueError, asyncio.LimitOverrunError) as exc: # readline() raises when a frame exceeds STREAM_LIMIT. The @@ -214,7 +214,13 @@ async def close(self) -> None: try: self._process.stdin.close() await asyncio.wait_for(self._process.wait(), timeout=5.0) - except (asyncio.TimeoutError, Exception): + except asyncio.CancelledError: + # Cancelled mid-teardown. CancelledError is a BaseException, so + # the handler below does not see it; without this the child + # would be dropped without ever being killed. + await self._kill(during_cancellation=True) + raise + except Exception: await self._kill() finally: self._process = None @@ -308,7 +314,7 @@ async def _start(self) -> None: # Cancelled mid-handshake: the child is spawned but never handed # over, so without this it would be orphaned entirely -- no # reference is stored anywhere the caller could reach. - await self._kill() + await self._kill(during_cancellation=True) raise except (ValueError, asyncio.LimitOverrunError) as exc: await self._kill() @@ -336,10 +342,20 @@ async def _start(self) -> None: await self._kill() raise CodeInterpreterError(f"Unexpected worker handshake: {ready}") - async def _kill(self) -> None: + async def _kill(self, *, during_cancellation: bool = False) -> None: + """Kill and reap the worker, detaching it first. + + ``during_cancellation`` says whether the caller is already unwinding a + CancelledError. It decides what happens if the shielded reap below is + itself interrupted: when the caller is about to re-raise that same + cancellation, swallowing here is correct; when the caller is raising a + CodeInterpreterError (timeout, malformed frame), a CancelledError + arriving now is a *new* external cancellation and must not be lost, or + the task keeps running after something asked it to stop. + """ # Detach first: whatever happens below, the next execute() must not - # reuse this process. If _kill() is itself interrupted, a dropped - # handle is recoverable; a retained one is not. + # reuse this process. If _kill() is interrupted, a dropped handle is + # recoverable; a retained one is not. proc = self._process self._process = None if proc is None: @@ -351,19 +367,18 @@ async def _kill(self) -> None: # Already exited; there is still a zombie to reap below on POSIX. pass - # Reap the child so it does not linger. shield() matters here because - # _kill() is normally reached from a cancellation handler: a bare await - # would be cancelled again immediately and return before the process was - # reaped. Shielding lets the wait() finish in the background even when - # our own await is interrupted. + # shield() matters because _kill() is usually reached from a + # cancellation handler: a bare await would be cancelled again + # immediately and return before the child was reaped. Shielding lets + # wait() finish in the background regardless. waiter = asyncio.shield(proc.wait()) try: await asyncio.wait_for(waiter, timeout=self.KILL_REAP_TIMEOUT) except asyncio.CancelledError: - # Our await was cancelled, not the reap. The shielded wait() keeps - # running and collects the child. Swallow rather than propagate: - # the caller re-raises the original cancellation. - pass + if not during_cancellation: + raise + # The caller re-raises the original cancellation; the shielded + # wait() continues and collects the child. except Exception: # Timed out, already reaped, or the loop is shutting down. Nothing # useful to recover or report; the handle is already dropped. diff --git a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs index e65b02f..3c141bf 100644 --- a/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs +++ b/servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs @@ -20,18 +20,23 @@ import { dirname, join } from "node:path"; import { PYPI_PACKAGES } from "../pypi-packages.mjs"; const __dirname = dirname(fileURLToPath(import.meta.url)); -const wheelsDir = join(__dirname, "..", "wheels"); +// Overridable only so tests can exercise the integrity paths against a scratch +// directory and a local stub. Production uses the sibling wheels/ dir and PyPI. +const wheelsDir = process.env.THINKINGBOX_WHEELS_DIR || join(__dirname, "..", "wheels"); const sha256 = (buf) => createHash("sha256").update(buf).digest("hex"); await mkdir(wheelsDir, { recursive: true }); -// A stalled connection would otherwise hang `npm install` indefinitely, since -// fetch() has no default timeout. Bounding it lets the script fail fast and -// degrade to a runtime micropip fetch, as documented. +// Bounded so a stalled connection cannot hang `npm install`; fetch() has no +// default timeout. const METADATA_TIMEOUT_MS = 30_000; const DOWNLOAD_TIMEOUT_MS = 120_000; +// Overridable only so the integrity paths can be exercised against a local +// stub in tests. Production always uses PyPI. +const PYPI_BASE_URL = process.env.THINKINGBOX_PYPI_BASE_URL || "https://pypi.org/pypi"; + // Raised for conditions that must abort the install rather than degrade. class IntegrityError extends Error {} @@ -81,7 +86,7 @@ async function downloadOne(pkg) { console.log(`[download-wheels] Downloading: ${filename}`); // Resolve the pinned release, not data.info.version: `npm install` must be // reproducible, and a digest is only meaningful against a fixed artifact. - const res = await fetch(`https://pypi.org/pypi/${encodeURIComponent(name)}/${encodeURIComponent(version)}/json`, { + const res = await fetch(`${PYPI_BASE_URL}/${encodeURIComponent(name)}/${encodeURIComponent(version)}/json`, { signal: AbortSignal.timeout(METADATA_TIMEOUT_MS), }); if (!res.ok) { @@ -114,14 +119,18 @@ async function downloadOne(pkg) { // These wheels are installed into the interpreter, so a corrupted or // substituted file is code execution. Verify the bytes actually received. + // A mismatch here means the artifact served does not match the pin, which + // is an integrity failure and not a transport problem -- degrading to a + // runtime micropip fetch would silently install whatever PyPI serves next + // instead of surfacing that the pinned artifact could not be obtained. const body = Buffer.from(await wRes.arrayBuffer()); const actual = sha256(body); if (actual !== expected) { - console.warn( - `[download-wheels] DIGEST MISMATCH for ${filename} ` + - `(expected ${expected}, got ${actual}) — refusing to write, leaving to runtime`, + throw new IntegrityError( + `Downloaded ${filename} does not match its pinned digest ` + + `(expected ${expected}, got ${actual}). Refusing to install. ` + + `Update pypi-packages.mjs deliberately if this is an intended bump.`, ); - return; } // Write via a temp file and rename so an interrupted install cannot leave a From b11de9d86efe05c312f7c2b7db6f8d89b0d6aa7b Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 16:32:54 -0700 Subject: [PATCH 18/21] Assert the exception type instead of a bare pass in the kill test CodeQL flagged the empty except. The clause was not doing nothing -- it was absorbing the expected cancellation -- but that intent was implicit. The test now captures what was raised and asserts it is CancelledError, which states the actual claim: _kill() must not mask the cancellation with a different error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/test_sandbox_isolation.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 8d8e53e..399e420 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -726,13 +726,17 @@ async def wait(self): await asyncio.sleep(0.05) task.cancel() - # The task is cancelled from outside, so awaiting it still raises; what - # matters is that _kill() itself did not convert that into a different - # error and that the process was detached. + # The task is cancelled from outside, so awaiting it raises either way. + # What this test pins is that _kill() did not convert that cancellation + # into a different exception, and that the process was still detached. + raised = None try: await task - except asyncio.CancelledError: - pass + except BaseException as exc: # noqa: BLE001 - the type is the assertion + raised = exc + assert isinstance(raised, asyncio.CancelledError), ( + f"_kill() masked the cancellation with {type(raised).__name__}: {raised!r}" + ) assert proc.killed assert interp._process is None From ce5c903730701d0af0717a2d80770c7278fcef33 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 16:33:42 -0700 Subject: [PATCH 19/21] Fix the kill-swallow test to match the contract it documents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit asserted that awaiting a cancelled _kill( during_cancellation=True) re-raises CancelledError. It does not, and must not: swallowing is the whole point of that flag. The caller is inside an xcept CancelledError: block and re-raises immediately after, so a second cancellation from _kill() would compete with it. The task therefore completes normally, which is what the test now asserts. My previous commit pushed this test in a failing state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/test_sandbox_isolation.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py index 399e420..e48e41a 100644 --- a/servers/thinkingbox_tools/tests/test_sandbox_isolation.py +++ b/servers/thinkingbox_tools/tests/test_sandbox_isolation.py @@ -726,16 +726,18 @@ async def wait(self): await asyncio.sleep(0.05) task.cancel() - # The task is cancelled from outside, so awaiting it raises either way. - # What this test pins is that _kill() did not convert that cancellation - # into a different exception, and that the process was still detached. - raised = None + # With during_cancellation=True the swallow is the contract: the caller is + # inside an `except CancelledError:` block and re-raises immediately after, + # so _kill() must not raise a second, competing CancelledError of its own. + # Awaiting the task here therefore completes normally. + outcome = "completed" try: await task - except BaseException as exc: # noqa: BLE001 - the type is the assertion - raised = exc - assert isinstance(raised, asyncio.CancelledError), ( - f"_kill() masked the cancellation with {type(raised).__name__}: {raised!r}" + except asyncio.CancelledError: + outcome = "raised" + assert outcome == "completed", ( + "_kill(during_cancellation=True) raised instead of deferring to the " + "caller, which would surface a second cancellation" ) assert proc.killed assert interp._process is None From a0e0f5addf7687142d0b809b43f1a51729cb6c17 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Fri, 28 Aug 2026 19:04:20 -0700 Subject: [PATCH 20/21] Let the revenue evidence span executions, as the REPL allows The assertion required one execution to both read sales.csv and emit the totals. The interpreter is a stateful REPL -- a documented feature with its own passing tests -- so a model may reasonably load the CSV in one call and aggregate off the persisted DataFrame in the next. That is the better pattern, and the test rejected it: the reading call prints nothing, and the call that prints never names the file, so it was filtered out before the numeric check ran. Confirmed against a simulated two-step session, which failed. Evidence is now gathered across the session in order. Some execution must read the fixture, and some execution at or after it must emit every per-region total while not carrying those totals as literals in its own code. The ordering requirement is load-bearing rather than incidental: totals printed before anything was read cannot have come from the data, so that case is still rejected. Exact numeric comparison against ground truth derived from the fixture is unchanged. Verified across nine cases -- the two-step split and a three-step variant with an unrelated call between now pass, alongside the single-call form, while hard-coded totals, totals emitted before any read, no read at all, partial totals, unit sums instead of revenue, and totals after a failed read are all still rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- dataset/test_case/sandbox_code_interpreter.py | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/dataset/test_case/sandbox_code_interpreter.py b/dataset/test_case/sandbox_code_interpreter.py index a358420..e70b2d8 100644 --- a/dataset/test_case/sandbox_code_interpreter.py +++ b/dataset/test_case/sandbox_code_interpreter.py @@ -124,21 +124,29 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): f"{[e['result'].get('error') for e in executions]}" ) - # Naming the file is not reading it -- require an actual read call. - reading = [e for e in successful if _reads_fixture(e)] - assert reading, ( + # The interpreter is a stateful REPL, so reading and reporting may happen in + # separate calls -- load the CSV once, then aggregate later off the + # persisted DataFrame. Evidence is therefore gathered across the session in + # order, not from a single execution: requiring one call to both read the + # file and print the totals would fail the better multi-step pattern this + # eval is meant to reward. + first_read = next( + (i for i, e in enumerate(successful) if _reads_fixture(e)), None + ) + assert first_read is not None, ( "no successful execution actually read sales.csv; mentioning the " f"filename is not enough: {[e.get('code') for e in successful]}" ) - # The decisive check: every per-region total derived from the fixture must - # appear in interpreter output, and none of them may appear in the code that - # produced it. Reproducing four independent totals that match the file to - # the cent is not something a model can do by writing them into a print() - # without having read the data -- and if it does write them in, the second - # half of this check rejects it. + # The decisive check: some execution *at or after* that read must emit every + # per-region total derived from the fixture, while not carrying those totals + # as literals in its own code. Ordering matters -- totals printed before + # anything was read cannot have come from the data. Reproducing four + # independent totals to the cent is not something a model can do without + # having read the file, and if it writes them in instead, the literal check + # rejects it. wanted = set(expected.values()) - for execution in reading: + for execution in successful[first_read:]: produced = _numbers_in( (execution["result"].get("stdout") or "") + " " @@ -151,8 +159,8 @@ def test_computes_revenue_per_region(x: TestContext, judge: Judge): break else: raise AssertionError( - "no execution produced all per-region totals " - f"{sorted(wanted)} as output without also containing them as " + "no execution after the fixture was read produced all per-region " + f"totals {sorted(wanted)} as output without also containing them as " "literals in its code -- the figures were not computed from the fixture" ) From 4bf1f0b327210b10c9e1e5e456fee4aa6dae7128 Mon Sep 17 00:00:00 2001 From: Ali Keramati Date: Sun, 30 Aug 2026 12:58:53 -0700 Subject: [PATCH 21/21] Use a domain tag the public taxonomy actually defines domain:data-analysis does not exist. thinkingbox/common/tag_types.py builds the Domain enum from tag_taxonomy.yaml, falling back to the shipped example, and the public fallback defines exactly three members: customer-service, hr and misc. Hydrating the scenario against the public framework therefore fails before any model is called. misc is the honest fit; the other two do not describe this suite. This repo's CI never caught it because nothing here hydrates scenarios -- test-thinkingbox-tools.yml runs the server package's pytest and nothing else -- so the tag stayed invalid through green CI. Found by running the scenario end-to-end against a real model, which is the only thing that exercises this path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- dataset/scenario/sandbox_code_interpreter.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dataset/scenario/sandbox_code_interpreter.yaml b/dataset/scenario/sandbox_code_interpreter.yaml index cec9b72..fd87a62 100644 --- a/dataset/scenario/sandbox_code_interpreter.yaml +++ b/dataset/scenario/sandbox_code_interpreter.yaml @@ -31,4 +31,4 @@ bot_instructions: | - Report the figures you computed. Do not invent numbers that the code did not produce. -tags: [domain:data-analysis, eval:orchestration:tool-selection] +tags: [domain:misc, eval:orchestration:tool-selection]