04: mcp-config is not branched, and it points at a world-readable owner token
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/<email> 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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/<email>` 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 <the owner's path>` 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.
|
||||
Reference in New Issue
Block a user