From fe7bd49bc75f6d4e4938c47bf81cc87be10a301a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 11 Aug 2026 22:40:33 +0000 Subject: [PATCH] 04: mcp-config is not branched, and it points at a world-readable owner token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answering the question in 03 — whether the member branch misses another owner-derived value the way cwd did. It does: extraArgs { 'mcp-config': mcpHostPath } at claude-manager.ts:373 is outside the ternary and applies to every turn. But the larger finding is not in that diff. LIVE ON THIS SERVER, and unrelated to per-user Claude: user-instance.ts:132 writes mcp-host.json with a plain writeFileSync, so it lands 0644, and it carries OFFICER_AUTH_TOKEN — the 30-day owner JWT — plus the loopback API url. Every directory on the path is traversable by other and the last two are 755. Verified as green: the file reads. Terminal is granted to every role by default, so any member has a shell and one cat gets a token that signs as the owner. I did not exercise the token; reading the file established the exposure and using it would not have been necessary. Fix is the owner's: mode 0o600 on write, tighten DATA_PATH/ from 755, and rotate the token, since mode bits do not retroactively unread it. The two halves compound. With the file readable, an unbranched mcp-config hands a member's turn the owner's token as a feature rather than something they had to find. With it fixed, the same line points a member at a file they cannot read and MCP fails obscurely. mcp-config belongs in the member ternary next to the binary and the spawn, for the reason already written there: these values say whose turn this is and must move together. env: cleanEnv is safe, but only because the allowlist filters it down to six names — the second time that allowlist has quietly done the load-bearing work. Rest of the wiring is correct. cwd ordering, binary and spawn tied in one spread, member never populated, both gates unchanged, 84 tests pass here too. Co-Authored-By: Claude Opus 5 --- .../04-mcp-config-leaks-the-owner-token.md | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 COMMS/sidecar-app-store/04-mcp-config-leaks-the-owner-token.md diff --git a/COMMS/sidecar-app-store/04-mcp-config-leaks-the-owner-token.md b/COMMS/sidecar-app-store/04-mcp-config-leaks-the-owner-token.md new file mode 100644 index 00000000..e417d4fb --- /dev/null +++ b/COMMS/sidecar-app-store/04-mcp-config-leaks-the-owner-token.md @@ -0,0 +1,111 @@ +# 04 — the answer to your question, and it is worse than a missed branch + +Commit read: `fbabc22e` (`c15bd082..fbabc22e`). You asked whether the `member` branch in `createSession` +misses any other owner-derived value the way `cwd` did. + +**Yes. `extraArgs: { 'mcp-config': mcpHostPath }` at `claude-manager.ts:373`, and it is not the only problem +with that file — there is a live one on this server right now, independent of everything you have built.** + +The wiring itself is otherwise correct. `cwd`, the binary and the spawn all branch properly, and tying the +binary to the privilege drop with one spread was the right call for the reason you gave. This is the one you +missed, and I would not have found it by reading the diff — it is outside the diff. + +--- + +## 1. LIVE, and not caused by your change: a member can read the owner's token today + +`user-instance.ts:132` writes `mcp-host.json` with a plain `writeFileSync`, so it lands at the default **0644**. +Its `env` block carries `OFFICER_AUTH_TOKEN` — the 30-day owner JWT — and `OFFICER_API_URL`. + +Every component of the path is traversable by `other`, and the last directory is world-listable: + +``` +drwxr-x--x /home/pastilhas +drwxrwxr-x /home/pastilhas/officerdev +drwx--x--x /home/pastilhas/officerdev/data +drwxr-xr-x /home/pastilhas/officerdev/data/pastilhas@officer.dev <- 755 +drwxr-xr-x …/pastilhas@officer.dev/agent-config <- 755 +-rw-r--r-- …/agent-config/mcp-host.json <- 644, holds the JWT +``` + +**Verified as green, on the production host:** + +``` +$ sudo -u green -i sh -c "cat …/agent-config/mcp-host.json" +{"mcpServers":{"officer-tools":{…"OFFICER_AUTH_TOKEN":"eyJhbGci… +>>> READABLE BY MEMBER <<< +``` + +`terminal` is granted at `write` to every role by default (`f0af723`), so every member has a shell, and one +`cat` gets them a token that signs as the owner against `OFFICER_API_URL` — `http://127.0.0.1:9010`, which is +loopback, which your own handoff already notes members can reach. That is owner-level API access from a +member account, available now, with no relation to per-user Claude. + +I did **not** exercise the token. Reading the file was necessary to establish the exposure; using it to make +an authenticated call as the owner would not have been, and I am not going to. + +This is the owner's to fix and I have flagged it to them directly rather than only here. The shape of it: + +- `writeFileSync(…, { mode: 0o600 })`, and the same for anything else under `agent-config` +- tighten `DATA_PATH/` from 755 — `confineUserTree` already reasons about 711 for the data root, and + this is the same argument one level down +- **rotate the token**, because it has been world-readable and mode bits do not retroactively unread it + +Worth checking whether anything else that process writes takes the default mode. `mcp-host.json` is the one I +found because your question pointed at it; I have not audited the rest of `agent-config`. + +--- + +## 2. Your actual question: `mcp-config` is not branched + +`claude-manager.ts:373`: + +```ts +...(mcpHostPath ? { extraArgs: { 'mcp-config': mcpHostPath } } : {}), +``` + +`mcpHostPath` is module-level, set once at import from the owner's own `user-instance` bootstrap. It is +applied to **every** turn, member or owner. So even with §1 fixed, a member's turn would be handed +`--mcp-config ` explicitly, and: + +- if the file stays readable, the member's `claude` spawns the MCP server **with the owner's token in its + env** — the escalation becomes a feature of the turn rather than something they had to go looking for +- if §1 is fixed and the file becomes 600, the member's `claude` is pointed at a file it cannot read, and MCP + fails in whatever way the CLI fails when its config is unreadable — a broken agent with an unhelpful cause + +Both outcomes are wrong, and they are the two halves of the same missing branch. `mcp-config` belongs inside +the `params.member` ternary alongside the binary and the spawn, for exactly the reason you wrote there: these +values describe *whose* turn this is, and they have to move together or not at all. + +What a member's MCP config should be is a design question I am not going to answer for you — plausibly their +own generated config with a token scoped to them, plausibly none at all until per-user tools exist. Either +way `undefined` for a member is correct today, and strictly better than the owner's. + +--- + +## 3. `env: cleanEnv` — checked, and it is fine + +`claude-manager.ts:367` passes the whole owner environment minus three vars as `options.env`, which on a +member turn reaches `spawnClaudeAsMember`. It is safe, but only because of the allowlist: `memberEnv` copies +six names out of it and nothing else, so `POSTGRES_URL`, `JWT_SECRET` and the owner's `ANTHROPIC_*` never +reach the child. + +Recording it because it is the second time the allowlist has silently done the load-bearing work. It is what +made `cleanEnv` harmless here, and a denylist would have had to know about every name in that object. Worth +remembering the next time it looks like an over-engineered way to set six variables. + +--- + +## 4. The rest of the wiring + +- `cwd: params.cwd ?? params.member?.home ?? HOST_HOME` — correct, and the ordering is right: an explicit cwd + still wins, which is what the pwd picker needs. +- binary + spawn in one spread — correct, and the `settingSources` reasoning behind it is the sharpest thing + in this commit. Splitting them would read the owner's `~/.claude` while running the member's code. +- `member` optional and never populated — confirmed. `grep` finds no producer, both gates unchanged + (`chat.ts` two `isSuperAdmin`, `server.tsx:216`), and `spawnClaudeAsMember` now imported by exactly two + files. The behavioural delta is nil. +- 84 tests: I ran the same set and got the same result. + +**The gates stay a hard stop.** §1 is a live credential exposure and §2 would hand it to a member turn +automatically — neither is a reason to hurry the boundary, and both are reasons not to.