Add Pyodide sandbox code interpreter MCP server - #22
Add Pyodide sandbox code interpreter MCP server#22Ali Keramati (alikera) wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Ports a Pyodide-based sandbox/code-interpreter MCP server into this repo, including a Node.js worker that hosts Pyodide, Python-side orchestration via FastMCP, workspace copy-on-write isolation, and a full integration test suite/documentation.
Changes:
- Adds a new
sandboxMCP server (mcp_sandbox.py) backed by a long-lived Node/Pyodide worker with a JSON-over-stdio protocol. - Implements workspace file listing/search tools plus copy-on-write isolation via NODEFS hooks and a per-session seeded workspace copy.
- Introduces Node worker packaging/CI wiring (npm install, vendored wheels postinstall) and extensive pytest coverage + docs.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs | NPM postinstall script to vendor pure-Python wheels from PyPI into wheels/. |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/sandbox.py | Host-side workspace file listing/search helpers with traversal protections. |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pypi-packages.mjs | Declares PyPI-only packages to preinstall via micropip (vendored when possible). |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/pyodide_worker.mjs | Node worker: loads Pyodide, installs packages, mounts /workspace, applies COW protections, executes code. |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package.json | Declares pyodide dependency and postinstall wheel-vendoring hook. |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json | Locks Node dependencies for the worker. |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/code_interpreter.py | Python supervisor for the Node worker (spawn, handshake, timeouts, serialization, effects log). |
| servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/init.py | Ensures setuptools package discovery of the sandbox subpackage. |
| servers/thinkingbox_tools/thinkingbox_tools/mcp_sandbox.py | FastMCP server defining sandbox tools + session lifecycle and workspace seeding. |
| servers/thinkingbox_tools/tests/test_sandbox_server.py | Integration tests for tool discovery, file ops, isolation/COW behavior, and interpreter semantics. |
| servers/thinkingbox_tools/pyproject.toml | Packages sandbox worker assets as Python package data. |
| servers/servers.yaml | Registers the new sandbox server entry. |
| docs/sandbox_code_interpreter.md | Architecture/design documentation and operational guidance for the sandbox server. |
| .gitignore | Ignores node_modules/ for the new Node worker. |
| .github/workflows/test-thinkingbox-tools.yml | Adds Node setup + npm ci for the sandbox worker prior to pytest. |
Files not reviewed (1)
- servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json: Generated file
Suppressed comments (1)
servers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/scripts/download-wheels.mjs:56
- This wheel download
fetch()also has no timeout, so a stuck transfer can hangnpm install/npm ci. Adding a timeout here keeps postinstall bounded and matches the intended “best-effort vendoring” behavior.
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()));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@microsoft-github-policy-service agree company="Microsoft"
…On Wed, Aug 26, 2026 at 6:35 PM microsoft-github-policy-service[bot] < ***@***.***> wrote:
*microsoft-github-policy-service[bot]* left a comment
(microsoft/thinkingbox-data#22)
<#22 (comment)>
Ali Keramati ***@***.***) <https://github.com/alikera> please read the
following Contributor License Agreement(CLA). If you agree with the CLA,
please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:
- (default - no company specified) I have sole ownership of
intellectual property rights to my Submissions and I am not making
Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
- (when company given) I am making Submissions in the course of work
for my employer (or my employer has intellectual property rights in my
Submissions by contract or applicable law). I have permission from my
employer to make Submissions and enter into this Agreement on behalf of my
employer. By signing below, the defined term “You” includes me and my
employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement Contribution License Agreement
This Contribution License Agreement (*“Agreement”*) is agreed to by the
party signing below (*“You”*),
and conveys certain license rights to Microsoft Corporation and its
affiliates (“Microsoft”) for Your
contributions to Microsoft open source projects. This Agreement is
effective as of the latest signature
date below.
1. *Definitions*.
*“Code”* means the computer software code, whether in human-readable
or machine-executable form,
that is delivered by You to Microsoft under this Agreement.
*“Project”* means any of the projects owned or managed by Microsoft
and offered under a license
approved by the Open Source Initiative (www.opensource.org).
*“Submit”* is the act of uploading, submitting, transmitting, or
distributing code or other content to any
Project, including but not limited to communication on electronic
mailing lists, source code control
systems, and issue tracking systems that are managed by, or on behalf
of, the Project for the purpose of
discussing and improving that Project, but excluding communication
that is conspicuously marked or
otherwise designated in writing by You as “Not a Submission.”
*“Submission”* means the Code and any other copyrightable material
Submitted by You, including any
associated comments and documentation.
2. *Your Submission*. You must agree to the terms of this Agreement
before making a Submission to any
Project. This Agreement covers any and all Submissions that You, now
or in the future (except as
described in Section 4 below), Submit to any Project.
3. *Originality of Work*. You represent that each of Your Submissions
is entirely Your original work.
Should You wish to Submit materials that are not Your original work,
You may Submit them separately
to the Project if You (a) retain all copyright and license information
that was in the materials as You
received them, (b) in the description accompanying Your Submission,
include the phrase “Submission
containing materials of a third party:” followed by the names of the
third party and any licenses or other
restrictions of which You are aware, and (c) follow any other
instructions in the Project’s written
guidelines concerning Submissions.
4. *Your Employer*. References to “employer” in this Agreement include
Your employer or anyone else
for whom You are acting in making Your Submission, e.g. as a
contractor, vendor, or agent. If Your
Submission is made in the course of Your work for an employer or Your
employer has intellectual
property rights in Your Submission by contract or applicable law, You
must secure permission from Your
employer to make the Submission before signing this Agreement. In that
case, the term “You” in this
Agreement will refer to You and the employer collectively. If You
change employers in the future and
desire to Submit additional Submissions for the new employer, then You
agree to sign a new Agreement
and secure permission from the new employer before Submitting those
Submissions.
5. *Licenses*.
- *Copyright License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license in the
Submission to reproduce, prepare derivative works of, publicly
display, publicly perform, and distribute
the Submission and such derivative works, and to sublicense any or all
of the foregoing rights to third
parties.
- *Patent License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license under
Your patent claims that are necessarily infringed by the Submission or
the combination of the
Submission with the Project to which it was Submitted to make, have
made, use, offer to sell, sell and
import or otherwise dispose of the Submission alone or with the
Project.
- *Other Rights Reserved*. Each party reserves all rights not
expressly granted in this Agreement.
No additional licenses or rights whatsoever (including, without
limitation, any implied licenses) are
granted by implication, exhaustion, estoppel or otherwise.
6. *Representations and Warranties*. You represent that You are
legally entitled to grant the above
licenses. You represent that each of Your Submissions is entirely Your
original work (except as You may
have disclosed under Section 3). You represent that You have secured
permission from Your employer to
make the Submission in cases where Your Submission is made in the
course of Your work for Your
employer or Your employer has intellectual property rights in Your
Submission by contract or applicable
law. If You are signing this Agreement on behalf of Your employer, You
represent and warrant that You
have the necessary authority to bind the listed employer to the
obligations contained in this Agreement.
You are not expected to provide support for Your Submission, unless
You choose to do so. UNLESS
REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING, AND EXCEPT FOR THE
WARRANTIES
EXPRESSLY STATED IN SECTIONS 3, 4, AND 6, THE SUBMISSION PROVIDED
UNDER THIS AGREEMENT IS
PROVIDED WITHOUT WARRANTY OF ANY KIND, INCLUDING, BUT NOT LIMITED TO,
ANY WARRANTY OF
NONINFRINGEMENT, MERCHANTABILITY, OR FITNESS FOR A PARTICULAR PURPOSE.
7. *Notice to Microsoft*. You agree to notify Microsoft in writing of
any facts or circumstances of which
You later become aware that would make Your representations in this
Agreement inaccurate in any
respect.
8. *Information about Submissions*. You agree that contributions to
Projects and information about
contributions may be maintained indefinitely and disclosed publicly,
including Your name and other
information that You submit with Your Submission.
9. *Governing Law/Jurisdiction*. This Agreement is governed by the
laws of the State of Washington, and
the parties consent to exclusive jurisdiction and venue in the federal
courts sitting in King County,
Washington, unless no federal subject matter jurisdiction exists, in
which case the parties consent to
exclusive jurisdiction and venue in the Superior Court of King County,
Washington. The parties waive all
defenses of lack of personal jurisdiction and forum non-conveniens.
10. *Entire Agreement/Assignment*. This Agreement is the entire
agreement between the parties, and
supersedes any and all prior agreements, understandings or
communications, written or oral, between
the parties relating to the subject matter hereof. This Agreement may
be assigned by Microsoft.
—
Reply to this email directly, view it on GitHub
<#22?email_source=notifications&email_token=AWEMZKF4MCZJBFZUQ26JMFL5L6F55A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBTGMZDGNJRGU32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5433235157>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWEMZKGCZX7UMPEAFHAZ77D5L6F55AVCNFSNUABGKJSXA33TNF2G64TZHMYTEMJYGMZTGNRVGU5US43TOVSTWNJSGYZDSOBTGI2DNILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Dependency ReviewThe following issues were found:
Vulnerabilitiesservers/thinkingbox_tools/thinkingbox_tools/toolslib/sandbox/package-lock.json
Only included vulnerabilities with severity high or higher. License Issues.github/workflows/test-thinkingbox-tools.yml
OpenSSF Scorecard
Scanned Files
|
Security review: the Pyodide worker is not an isolation boundaryI audited the escape vectors raised in review. They reproduce. Posting findings and a proposed design before making broad changes, as requested. Audit resultsMeasured against the
So agent-supplied Python has host filesystem read/write, process execution, and full environment-variable access. The NODEFS copy-on-write layer is irrelevant to this: it guards This is consistent with Pyodide's own documented position — it provides memory safety via WASM, but deliberately exposes a Python↔JS FFI and is explicitly not a security sandbox for untrusted code. The documentation is currently wrong
That is false as written, and it's the most dangerous thing in this PR — it would lead a reader to route untrusted input here. I'm correcting the threat model in this PR regardless of what we decide about isolation. Why in-process hardening cannot fix thisWorth stating explicitly, because these are the obvious first attempts:
The common failure is that all three try to remove names while leaving live references reachable inside the same process and privilege domain. Proposed design: move the boundary to the OSRun the worker under an owner-approved OS/container boundary, so escaping Pyodide only lands the attacker in an already-empty jail. Preferred — containerized worker:
Fallback where a container runtime is unavailable: fail closed. Defense-in-depth (not load-bearing): Node permission flags, dropping Affected files if we proceed
That is a rewrite of the execution model and a CI-shape change, so I am not doing it unilaterally in this port. It needs an owner decision on runtime (Docker vs Podman vs rootless), base image, and whether eval throughput can absorb per-session container startup. What I am changing in this PR now (bounded)
Keeping this as a draft. |
Note on the Dependency Review "unknown license" noticeThe Dependency Review bot comment on this PR flags:
This is a false positive and is not failing the check. From the same run:
I deliberately did not silence it by pinning to a commit SHA. Every action in this repo uses a floating major tag — Separately, the same bot comment carries a deprecation warning for the Current status on |
|
One thing worth deciding before this merges: the interpreter refusing to start without Happy to drop it (one commit) and keep just the port, the bug fixes, and the docs correction, if the lighter version is preferred. |
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/<path> 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 <susanapa@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
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 <susanapa@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
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>
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>
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>
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>
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>
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>
…grity 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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
7e46ff6 to
4bf1f0b
Compare
|
We have moved away from pyodide and into using python WASM instead. Security and speed were the main concerns and we wanted to only support one backend. There are some downsides to the change, related to support for not pure cython based dependencies, but we can live with that. I don't think pyodide it should be re-introduced, but I would welcome a thorough review like the one done here for python wasm. |
Adds a pyodide-based code interpreter, originally written by Susana Palmaz (@susana-palmaz):
code_interpreter,list_sandbox_filesandsearch_sandbox_files.Security
Pyodide isn't an isolation boundary. Agent code can reach the Node host: host files, process execution, env vars.
docs/sandbox_code_interpreter.mdsets out the threat model.code_interpreternow refuses to run unlessTHINKINGBOX_SANDBOX_ALLOW_UNCONFINED=1is set by whatever launches the server. It's deliberately not inservers.yaml, so the unsafe mode isn't the default. The file tools still work without it.Trusted code only, and no secrets in that environment. Real confinement needs a container and isn't in this PR.
Testing
gpt-4o ran the revenue test end to end and passed: it wrote pandas against a CSV in the workspace, carried the DataFrame across two calls, and got every regional total right to the cent.
Separately drove the tool directly against
support/tau_bench/airline_data(5.2 MB of flights, reservations and users). Aggregations matched host CPython exactly, and a file rewritten from inside the session came back byte-identical. Also round-tripped the document and plotting libraries it advertises.CI: 152 passed, 2 skipped, 6 xfailed. The xfails probe the escapes above and should start passing once the worker is confined.
Fixed while porting
search_sandbox_filescrashed on patterns like'',***,/etc/passwd