Repository navigation
fix(sandbox): document stdio data-dir/socket exposure, doctor warning, writable /dev/null (#1704) - #1738
Merged
Conversation
…ning, make /dev/null writable Under isolation none or sandbox a stdio child runs as the proxy's user: it can read the data directory (mcp_config.json with api_key, config.db with OAuth tokens) and, while enable_socket is on, mint agent tokens over the admin socket without a key. Landlock confines writes only, so hiding the files would not close the gap (#1704, verified by the reporter). - docs: state the limit in the sandbox page (new "What the sandbox does not hide" section with the reporter's verification table), the isolation mode tables and honest limitations, the socket page, and the headless install checklist (enable_socket: false + docker isolation). - doctor: config.StdioSocketExposureFinding names every enabled stdio server resolving to none/sandbox while enable_socket is true, with the fix; wired through the existing DoctorFindings runtime_warnings path. - sandbox: the default spec never granted WRITE_FILE on /dev/null, so a confined `sh -c '... 2>/dev/null'`, Node stdio:"ignore" and Python subprocess.DEVNULL failed with EACCES (confirmed on kernel 6.8, Landlock ABI 4). buildSandboxSpec now grants /dev/null read-write and addPathRule masks directory-only rights for non-directory targets, which Landlock otherwise rejects with EINVAL (and the fail-closed wrapper then aborted the whole confinement). - docs: snap-packaged runtimes cannot run under the sandbox (snap-confine needs privileges no_new_privs forbids). Closes #1704
1 task done
Deploying mcpproxy-docs with
|
| Latest commit: |
9f14004
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://29db65a4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-cool-dhawan-0b193a.mcpproxy-docs.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 39795118329 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1704
Delivers what was promised on the issue: honest docs, a
doctorwarning, and a check of the/dev/nullside observation (which turned out to be a real bug, fixed here).Docs
noneandsandboxisolation do not hide the data directory or the Unix socket from a stdio child: the child runs as the proxy's user, can readmcp_config.json(api_key) andconfig.db(OAuth tokens), and whileenable_socketis on it can mint agent tokens over<data-dir>/mcpproxy.sockwithout a key. Landlock confines writes only (it does block/proc/<pid>/environ).features/sandbox-isolation: new What the sandbox does not hide section with @james-crowley's verification table and wording, plus two new honest-limits bullets (data dir / socket; snap-packaged runtimes).features/docker-isolationand the unpublished rootdocs/docker-isolation.md:sandboxandnonerows of the mode table say so; a new Honest limitations bullet links the table.socket-communication: trust-boundary bullet under API Key Authentication.getting-started/installation: headless recommendation (enable_socket: false+dockerisolation) in the LAN security checklist; the headless example config now setsenable_socket: false.Website build is green and the new anchor resolves from all three linking pages (checked in
website/build).Doctor
config.StdioSocketExposureFindingjoins the existingDoctorFindings→runtime_warningspath. It names every enabled stdio server whose resolved isolation mode isnoneorsandboxwhileenable_socketis true, and states the fix:Skipped: servers resolved to
docker, servers whose command isdockeritself, HTTP/SSE servers, disabled servers. A per-serverenabled: falseopt-out under a globaldockermode is listed. Tests ininternal/config/doctor_findings_test.go.doctorstill exits 0; the finding counts towardtotal_issueslike every other runtime warning./dev/nullundersandbox— confirmed and fixedReproduced on a real kernel (kubic, Ubuntu 24.04, kernel 6.8, Landlock ABI 4) with the production spec:
sh -c '… 2>/dev/null'fails withcannot create /dev/null: Permission denied. The read-only/rule coversREAD_FILEbut notWRITE_FILEon the device.Two changes:
buildSandboxSpecgrants/dev/nullread-write (the single device, not/dev).addPathRulemasks directory-only rights (READ_DIR,MAKE_*,REMOVE_*,REFER) when the target is not a directory. Without this Landlock rejects the rule withEINVAL, and because the wrapper is fail-closed the whole confinement aborted (exit 3) — so a naive/dev/nullgrant would have broken every sandboxed server.What the probe showed with the real
mcpproxy __sandbox_execwrapper on kubic (RO/, RW tmp + caches):sh -c 'echo x 2>/dev/null'spawnSync(…, {stdio: "ignore"})EACCESsubprocess.run(…, stdout=DEVNULL)PermissionErrornpx --version;npx -y @modelcontextprotocol/server-everything stdio→initializeuvx --version;uvx mcp-server-time→initialize(non-snap uv)uvxsnap-confine is packaged without necessary permissionsecho x >/dev/stdoutSo
npx/uvxservers launched fine before; what broke was any server that routes a child's stdio to/dev/null, and shell launch wrappers using2>/dev/null. The snap finding is independent of/dev/null: Landlock requiresno_new_privs, which stops snap-confine from gaining the privileges it needs. Documented as a limitation; the fix is a non-snap runtime install.Tests:
TestBuildSandboxSpec_DevNullWritable(any OS),TestSandboxWrapper_DefaultSpecAllowsDevNull(internal/upstream/core, linux),TestAddPathRule_FileTarget(internal/sandbox, linux). The two linux-tagged tests were watched red then green on kubic via cross-compiled test binaries; macOS cannot run them (Landlock is Linux-only).Verification
golangci-lintwith.github/.golangci.yml: bare,--build-tags server, andGOOS=linuxfor the_linux.gofiles — no findings in changed files (the local v2 reports two pre-existingreflect.Ptrgovet items in untouched test files, identical onmain).go test -race ./internal/config/... ./internal/sandbox/... ./internal/management/...,-tags serverconfig tests,cmd/mcpproxydoctor tests,internal/server+internal/httpapidoctor/bundle tests under the CI skip regex: green../scripts/test-api-e2e.shon a private port (LISTEN_PORT=18790, withmcpproxy-serverbuilt): 70 run, 0 failed.website && npm run build: green.No new dependencies.