fix(docker): publish on loopback by default; make a mounted state dir writable - #1974
Conversation
… writable Closes #1964. - README's Docker recipes dropped v1's `127.0.0.1:` prefix, so `-p 6274:6274` published the Inspector on every host interface. The container's `HOST=0.0.0.0` governs the container's interfaces, not the host's, so `DANGEROUSLY_BIND_ALL_INTERFACES` never covered this. Verified against a real container: from a LAN peer, `GET /` served the injected `MCP_INSPECTOR_API_TOKEN`, and that token then unlocked `/api/*` with no `Origin` header (server.ts:241 allows origin-less requests). Restore the prefix on both recipes, on the port-remap examples, and in the web README. - Adding a server failed inside the container the moment you mounted a volume to keep it. Docker seeds a named volume's ownership from the image's directory at the mount point, and creates it `root:root` when that directory is absent — so the non-root `node` user got `EACCES ... open '/home/node/.mcp-inspector/mcp.json.tmp-*'`. Create the dir in the image, owned by `node`, and document the volume recipe (plus the bind-mount `--user` caveat) — without a volume the catalog lives in the writable layer and `--rm` throws it away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
There was a problem hiding this comment.
Pull request overview
Improves Docker security defaults and persistent state handling.
Changes:
- Publishes the web port on loopback by default.
- Pre-creates a writable runtime-state directory.
- Documents persistent volumes and port remapping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
README.md |
Updates Docker security and persistence guidance. |
Dockerfile |
Creates the state directory with node ownership. |
clients/web/README.md |
Uses loopback-only Docker port publishing. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…n claim - A named volume created by an image predating the ownership fix keeps its root ownership if it is non-empty, since Docker only applies the image directory's ownership to an *empty* volume. Verified both halves: an empty pre-existing volume repairs itself on the first run of the fixed image, while one holding a file stays root-owned and still EACCESes. Document the distinction and the one-shot `chown` repair (verified to restore writes). - "pair it with a known MCP_INSPECTOR_API_TOKEN" was bad advice for a wider publication: the same paragraph notes `GET /` discloses the token, so a custom one is harvested exactly as a generated one is. Point at a real access-control boundary instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a
|
Round 1 review addressed in 5be2b3e — mirroring here since inline replies get collapsed once the threads go outdated. 1. Pre-existing volumes from an older image (README.md:339) — correct, and the fix was incomplete. I tested both halves to find the boundary:
README now documents the distinction plus a one-shot repair (verified to restore writes): docker run --rm -u 0 --entrypoint chown \
-v mcp-inspector-data:/data ghcr.io/modelcontextprotocol/inspector \
-R node:node /data2. "pair it with a known Both were actionable; nothing skipped. |
Closes #1964
Two Docker problems, both of which only show up in a real container.
1. The recipe published 6274 on every host interface
README.mdrecommended-p 6274:6274with no address prefix, which publishes on all host interfaces. v1's own recipe used-p 127.0.0.1:6274:6274; v2 dropped the prefix.The container's
HOST=0.0.0.0is a separate concern — it governs the container's interfaces, not the host's — so theDANGEROUSLY_BIND_ALL_INTERFACESopt-in that guards a wildcard bind outside a container never covered this case.I confirmed the whole chain against a running container built from this branch, reaching it at the host's LAN address (
192.168.2.6) rather than loopback. Output below is verbatim; image tags and container names are trimmed to the relevant part:So from any host on the network:
GET /hands out the injected API token, and that token then unlocks/api/*— a backend that spawns processes — because a request with noOriginheader skips the origin allow-list entirely (core/mcp/remote/node/server.ts:241).With the prefix restored:
Fixed in both
README.mdrecipes, both port-remap examples, and the one inclients/web/README.md.2. You couldn't keep the servers you added
Raised separately while testing this. Two distinct causes:
Without a volume, the catalog is written to
$HOME/.mcp-inspector/mcp.json=/home/node/.mcp-inspector/mcp.json, inside the container's writable layer — so--rmdiscards it and every run starts empty. That's expected once you know it, but nothing said so.With a volume, it was outright broken. Docker seeds a named volume's ownership from the image's directory at the mount point, and when that directory doesn't exist it creates it
root:root. The image runs as the non-rootnodeuser (uid 1000), so every add failed:The
Dockerfilenow creates that directory up front owned bynode, so a mounted volume inheritsnode:node. The README documents the volume recipe and the bind-mount--usercaveat (a bind mount keeps its host ownership, so it still needs--useror achownon Linux).I chose
mkdir+chownover aVOLUMEinstruction deliberately —VOLUMEwould create an anonymous volume on every plaindocker run, which is a surprise of its own.Proof
Before — unfixed image, named volume mounted, adding a server fails with the
EACCESsurfaced in the modal:After — same volume mount on the fixed image, the add succeeds (new server highlighted):
After — container destroyed with
docker rm -fand recreated against the same volume; the saved server is still there:Testing
npm run ci—validate,verify:build-gate,smoke, and the Storybook run all pass. Thecoveragestep exits 1 on my machine with all 4881 tests passing and 2 unhandled rejections at teardown (SdkError: Connection closedfromInspectorClient.disconnectinsrc/test/integration/mcp/inspectorClient.test.ts). That is pre-existing and unrelated to this PR —git diff origin/v2/main..HEADtouches onlyDockerfile,README.md, andclients/web/README.md, and GitHub CI is green ona09f2dcf, this branch's exact base. Reproduced twice locally; flagging it rather than papering over it.npm run ci, so theDockerfilechange was verified by building and running it directly.