feat(cli,mcp): the runtime token reaches the MCP child on a pipe, not in its environment (TASK-078) - #1780
Merged
Conversation
samxu01
force-pushed
the
kai/mcp-token-pipe
branch
from
September 19, 2026 20:30
ac176cf to
73590c8
Compare
… in its environment (TASK-078)
Sam's ruling on the posture card: take the token out of the environment.
`connectStdioMcp` spawned every declared stdio server with
`{...process.env, ...env}` and the default declaration put the seat's runtime
token in that map, so the token sat in the MCP child's environment where any
same-user process reads it with `ps eww <pid>` or /proc/<pid>/environ.
The token now rides an inherited fd 3 — the channel TASK-070 already uses for the
server list — and the child's environment carries only the pointer
`COMMONLY_TOKEN_FD=3`. `commonly-mcp` reads it from there and never falls back to
the environment when the fd is declared but unreadable. The inherited environment
is stripped of the key even when a declaration does not name it, because
`...process.env` made the daemon's own token a channel into every child.
A server that cannot read the pipe keeps the variable: a pin older than 0.3.11
(measured from the command, with a warning naming it), a server that is not
@commonlyai/mcp at all, or an explicit COMMONLY_TOKEN_CHANNEL=env.
claude and codex start the MCP server themselves, so a pipe we open never
reaches that grandchild; they still hand the token over in their runtime's
environment, and that half is flagged rather than silently assumed to be done.
samxu01
force-pushed
the
kai/mcp-token-pipe
branch
from
September 20, 2026 01:25
73590c8 to
ccaf553
Compare
samxu01
pushed a commit
that referenced
this pull request
Sep 20, 2026
Two slots, and the numbers are the ones free behind #1780's 0.1.61/0.3.11 — re-derive both against main if the queue moves before this is cut. The cli bump is for the three adapter changes and the hook read; the mcp bump is for the credential reader, which resolves fd, then file, then env, and treats a declared source as authoritative. Both files are touched only on the version line.
lilyshen0722
added a commit
that referenced
this pull request
Sep 20, 2026
… not as an inherited environment value (#1801) * feat(mcp): the credential reader resolves fd, then file, then env — by declaration (TASK-083) A declared source is authoritative: a failed read throws and never falls through to the weaker channel. New COMMONLY_TOKEN_FILE=<path> branch, read to EOF and trimmed, refusing an empty credential. Needed because claude and codex cannot be handed a pipe we opened — claude expands ${VAR} from its own environment and codex forwards declared variables from its own — so a path is the strongest channel either of them can carry. 6 new tests; 5 mutations, each reddening a distinct witness, no survivors. * feat(cli): an MCP child gets an allowlist and a launcher credential file (TASK-083) The pi bridge spawned every declared MCP server with {...process.env, ...declared}, so a third-party server inherited the seat token and the model key — measured on 2026-09-19: a @playwright/mcp process held COMMONLY_AGENT_TOKEN, and a pi seat's MCP child held COMMONLY_LITELLM_KEY. The child environment is now the derived allowlist {PATH, HOME, TMPDIR, and the proxy/CA names} plus what the entry itself declares. The credential may now also be named as a PATH (COMMONLY_TOKEN_FILE) instead of carried as a value: the bridge reads that file and pipes the token on fd 3, so the child is handed neither the secret nor the path. A declared file is authoritative (a failed read throws rather than falling through), and an old pinned server that cannot read the pipe still gets the value in its environment, with the warning. New cli/src/lib/credential-file.js writes the per-spawn 0600 file (chmod asserted after the write, since writeFileSync's mode is filtered by umask) inside a 0700 directory. Also corrects the stale doc line claiming the staging seats run 0.3.4; they run 0.3.7 (measured). 14 new tests, including two real-child probes that read their own fd 3 and their own environment; 9 mutations, no survivors. * refactor(cli): one version predicate and one credential-delivery decision (TASK-083) The channel question — does this declared server read a pipe, a file, or only a value in its environment — was about to be answered separately in the pi bridge, the claude adapter and the codex adapter. Three copies of a version threshold drift, and the drift is silent in whichever one nobody is looking at, so the predicate moves to mcp-server-version.js (describeMcpCommand, the pipe-reader release 0.3.11 and the file-reader release 0.3.12) and the decision to mcp-credential-delivery.js. deliverSeatCredential rewrites a DECLARATION, not a value: an entry naming COMMONLY_AGENT_TOKEN comes back naming COMMONLY_TOKEN_FILE, so the token never exists in the runtime's environment for its whole process subtree to inherit. Four honest outcomes: 'path' (moved), 'env' (kept deliberately — somebody else's server, or a checkout that predates the file reader, each with its own warning), 'none' (no credential declared) and 'unavailable' (no launcher file this spawn, so nothing is rewritten rather than pointing at nowhere). 10 new tests. Also moves CREDENTIAL_KEY beside CREDENTIAL_FILE_VAR so the two channels are named in one place. * feat(cli): the claude seat hands its MCP servers a credential file, not a token (TASK-083) claude spawns its MCP servers inside its own process tree, so there is no pipe to inherit: the only two channels are a value in claude's environment, which every child and every hook inherits, or a PATH that the declared server opens itself. The default declaration takes the path. deliverSeatCredential rewrites the config claude actually reads — an entry naming COMMONLY_AGENT_TOKEN comes back naming COMMONLY_TOKEN_FILE — so the token is never in claude's environment to be inherited, logged, or dumped by any other server the seat was granted. WHERE the file lives is part of the contract, measured rather than assumed: sandbox/seatbelt.js admits exactly one path outside the workspace to a confined seat, a subpath rule on the per-spawn --mcp-config directory. A credential file under ~/.commonly would be unreadable by the very child it was written for, so the file is written inside that directory, in a 0700 subdirectory, at 0600, and removed by the spawn's existing finally block. A second handover is deliberately NOT moved, and is now warned about: an entry naming the token in args, url, headers, or inside a larger string gets a literal substitution from claude, where a file path is not a value. The expansion loop already exposed only values the declaration references, and the rewrite is what stops the default declaration referencing the token — so the value survives for those spawns, and only those, with a warning naming the trade. That is the carve-out TASK-082 exists for (the grant broker's bearer header), and the three pre-existing claude environment tests are its witnesses: they pass unchanged. 11 new tests, plus C1-C8 mutations with no survivors (the config rewrite, the file's location, the cleanup, the warning, the expansion guard, the stranger branch, the version threshold, the no-file branch). Full cli suite 43/690. * feat(cli): the codex seat forwards a credential path, never the token (TASK-083) Same decision as the claude half, different mechanism. codex takes its MCP servers as `-c mcp_servers.*` overrides, and its existing anti-leak measure diverted a token-bearing ENV VALUE into `env_vars`, which codex resolves from its own environment — measured on a live codex seat: three MCP children each carrying a 73-char token. The declaration rewrite removes the need for that route entirely: the credential becomes COMMONLY_TOKEN_FILE, a path, which is not token-bearing, so it lands in `env={...}` on the command line where a path is harmless and the child opens the file itself. Substituting the file placeholder needed one line in SUBSTITUTION_KEYS, because the path itself is the thing being handed over. Two things this change made visible, both now handled rather than assumed: - A token wanted as a COMMAND ARGUMENT had no env_vars route and was substituted literally into `-c mcp_servers.<name>.args`, i.e. onto a command line readable by every same-user process — while the doc block in this file promised bearer tokens never ride in argv. That entry is now refused whole, with the reason, rather than half-wired or published. - A declaration that needs the token as a literal (an env value containing it, or a command argument) still gets it forwarded, and now says so: the seat whose environment carries a secret is named instead of assumed fixed. The credential file is written into the per-spawn directory this adapter already creates and removes for --output-last-message, so a child codex spawns can reach it and no second lifecycle has to be right. Updated the existing mcp-override regression test, whose expected argv was `env_vars=["COMMONLY_AGENT_TOKEN"]` — the leak this change closes — and added three tests for the carve-out and the argv refusal. D1-D6 mutations, no survivors. Full cli suite 43/693. * feat(cli): a pi seat's MCP servers get the credential from the launcher file (TASK-083) The pi bridge already had the strongest channel — it reads a credential file and puts the VALUE on the child's fd 3, so the token never reaches a child's environment — but nothing wrote that file for a pi seat, so the declaration had to name the token variable and pi substituted the value into the payload it hands the bridge. Now the launcher writes the file and the declaration names it: the payload carries a PATH, the bridge reads it, and the value still travels only on fd 3. The file is written into this seat's own 0700 home, one per spawn, and removed in a finally when the turn ends — a token file that outlives its turn is a token file that sits in a home for the next one. pi has no enforced sandbox, so the seat home is the narrowest place the bridge can still read; the claude and codex halves cannot use it and write into the per-spawn directory each already hands its runtime instead. Updated the fd-3 payload test, whose expected payload was the token itself, and added two tests: one that witnesses the file DURING the spawn (holding the token, 0600, inside a 0700 directory in the seat home — an after-the-fact read can only prove absence, since the finally is deliberate) and one that a declaration asking for no credential is untouched. E1-E4 mutations, no survivors. Full cli suite 43/695. * feat(cli): a hook reads the credential the way its runtime now carries it (TASK-083) Emptying the token out of a seat runtime's environment has a second consumer nobody asked about: the tool-policy hooks are child processes of that runtime, so they read exactly the environment the token was removed from. Left alone they would find nothing, and because a hook fails OPEN by design (an unwritable ledger must not become an accidental blocker), the symptom would not be an error — it would be a hook that silently stopped deciding anything. So resolveHookToken prefers the launcher file and falls back to the value variable for a seat whose declaration has not migrated. Unlike the MCP reader it does not throw on a declared-but-unreadable file: a hook that dies is a hook the runtime reports as broken, and failing open is the documented posture here. The shipped default declaration stays EXACTLY canonical — it still names the token variable, and each adapter rewrites it per spawn. That is deliberate for deploy skew: a record that named the file directly would be one an older CLI could not serve, and the guard's canonical-shape check is a security property rather than a formality, so it is left alone and the decision is asserted instead of widened. Tests: four for the hook credential (file wins, unreadable falls back, empty falls back, a blank declaration is not consulted, and the pre-existing behaviour) and one asserting the shipped entry remains canonical. F1-F4 and G1 mutations, no survivors. Full cli suite 43/699. * chore(release): cli 0.1.62, @commonlyai/mcp 0.3.12 (TASK-083) Two slots, and the numbers are the ones free behind #1780's 0.1.61/0.3.11 — re-derive both against main if the queue moves before this is cut. The cli bump is for the three adapter changes and the hook read; the mcp bump is for the credential reader, which resolves fd, then file, then env, and treats a declared source as authoritative. Both files are touched only on the version line. * fix(cli): the runtime's own environment is not a credential channel (TASK-083) Vera 70455: four cli tests named the value in the runtime's environment, and the value they found was the runner's own. The declaration rewrite moved our MCP servers onto a launcher file, but every adapter derives its runtime environment from `process.env` -- where the documented bootstrap export lives -- so the credential came back into the runtime and from there into every child below it: claude's MCP children and hooks, codex's MCP children, pi's own bash tool (which spawns with `{ ...process.env }`). One rule, in the module that already owns the delivery decision (`withholdRuntimeCredential`): the PATH of this spawn's file goes into the runtime environment, because a path is not a secret and a hook resolves its credential from it; the VALUE is deleted, unless a carve-out genuinely needs it there. The adapter passes that answer in rather than inferring it -- claude's literal args/url/headers, codex's forwarded env -- so the spawn that still carries a secret is the one that warns about it. The hook path was wired to nothing. `resolveHookToken` (launcher file first, value second) existed, was documented, and was unit-tested, while `agent hooks-forward` -- the command hooks-config actually installs into Claude settings -- read `process.env.COMMONLY_AGENT_TOKEN` directly. On a migrated seat the value is absent, so the forwarder returned `hook_unavailable` and the tool-policy hook silently stopped deciding anything: the fail-open posture turns a dead credential into a silence rather than an error. The command now resolves through `resolveHookToken`, and a test drives that command through `registerAgent` and asserts the Authorization header the backend would see. Tests: the four that failed under an exported variable are now positive controls -- they plant the launcher's value explicitly instead of inheriting whatever the runner happens to have, which is why they passed in one worktree and failed in another. New: a pi witness (value out, path in, provider key kept) and the hooks-forward end-to-end witness. Mutations M1-M7, all run with the variable exported: no survivors, every source restored byte-identical.
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
1 participant
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.
TASK-078. Sam ruled on the posture card (70188/70199, 2026-09-19): take the token out of the environment — inherited pipe. This is the pi half; the claude/codex half cannot use a pipe at all and is explained below rather than silently left out.
What was leaking
connectStdioMcpspawned every declared stdio server with{...process.env, ...env}, and the default declaration puts the seat's runtime token in thatenvmap (default-environment.js:80,COMMONLY_AGENT_TOKEN: '${COMMONLY_AGENT_TOKEN}'substituted inpi.js:75). So the token sat in the MCP child's environment, where any same-user process reads it back withps eww <pid>or/proc/<pid>/environ— the boundary does not hold on a shared host.What replaces it
The token rides an inherited pipe on fd 3 — the same channel TASK-070 already uses for the server list — and the child's environment carries only a pointer,
COMMONLY_TOKEN_FD=3, which is not a secret.commonly-mcpreads the credential from that fd when the pointer is declared.The declared fd is authoritative. A read that fails does NOT fall back to the environment: silently using the weaker channel after being told to use the stronger one is precisely the class of bug this removes. The failure names the fd.
The inherited environment is stripped even when the declaration asks for nothing.
{...process.env, ...env}meant the daemon's own token reached a child that never asked for it; a declaration that does not name the token gets none.The fd is read to EOF and never closed — on macOS a
closeSyncafter a failed read aborts the process (kqueue.c), which TASK-070 already paid for.Who still gets the environment variable, and why
The pipe is only available where WE spawn the server.
describeMcpCommanddecides from the declaration, and the fallback is a property of the server, not of this change's mood:@commonlyai/mcp@<>= 0.3.11>, or a local checkout of it at that version@commonlyai/mcp@latest, or unpinned.github/workflows/npm-publish.ymlfires oncommonly-mcp/**)@commonlyai/mcp@< 0.3.11(a pin — the sprint seats run a staging checkout of 0.3.4)@commonlyai/mcpat allCOMMONLY_TOKEN_CHANNEL=envin the entryThe first draft of this change piped any server that declared the key. That is a silent contract change to a stranger's server, so the identification step exists, and mutation M7 is what keeps it honest.
The claude/codex half is NOT done here, deliberately
Those two runtimes start the MCP server themselves: claude expands
${COMMONLY_AGENT_TOKEN}inside its own process environment (claude.js:280, where the comment records that this replaced materializing the token in a transient JSON file) and codex hands it over inmcp_servers.*.env_vars(codex.js:175). A pipe opened by the daemon never reaches that grandchild, so "the env var dropped per runtime" can only be true for pi. Those two need a launcher-based channel (a per-spawn 0600 file, or a local socket the launcher reads) or the scoped per-spawn token from option B — a decision I am not making inside a PR. Flagged in the pod (70204) with wren.Evidence
COMMONLY_TOKEN_FD, and its ownCOMMONLY_AGENT_TOKEN— because an argument passed to an injected fake is not proof that a process could not read the secret. 6 new mcp tests.Versions
cli0.1.61 (0.1.60 is #1778's, 0.1.59 the TASK-052 baseline's) and@commonlyai/mcp0.3.11 — the reader must be published for the pipe to be readable, andnpm-publish.ymlpublishes it on merge. If a daemon updates inside the publish window, the child fails loudly (No runtime token: either COMMONLY_AGENT_TOKEN must be set, or COMMONLY_TOKEN_FD must name a pipe carrying one), not silently;COMMONLY_TOKEN_CHANNEL=envon the entry is the immediate escape.