chat sessions record whose they are, and refuse a mismatched caller
host found this reading 10: chat sessions carry no identity at all. state.ts
held a flat sessionKey -> transcript uuid map, the in-memory sessions Map was
keyed the same way, and websocket.ts takes sessionKey and resumeSessionId
straight off the client message. 4d4a253f fixed exactly this for the pty
sidecar — "re-attaching to a session belonging to another account is refused,
otherwise a member resumes someone else's shell by guessing an id that travels
in a query string" — and chat never got the same treatment, because both gates
made it unreachable and therefore invisible.
Sessions now carry userId, persisted and in memory. getClaudeSession requires
the caller and returns undefined on a mismatch rather than throwing, since a
throw confirms that someone else's session exists. spawnClaudeStreaming throws
when a live session's owner does not match — that is the path that mattered
most, because handing over another account's sessionKey would otherwise push a
turn into their conversation and stream their agent's output back.
Legacy string entries are adopted to the owner on load. That is a statement
about the past rather than a guess: until this commit the gates refused every
non-owner, so nothing else could have created one. Dropping them would have
silently broken the owner's resume on upgrade.
PARTIAL, and the doc says so plainly: claude:kill, :interrupt, :clear-session,
:is-generating, :find-session and :list all still take a bare sessionKey with no
ownership check, and :list returns every session in the sidecar. Closing them is
a wide mechanical change across the protocol, the registry verbs and their
producers, and it belongs in its own reviewable commit rather than buried under
a state migration. The gates must not move on the strength of this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,70 @@
|
||||
# 11 — session ownership, the half that is done and the half that is not
|
||||
|
||||
Answering `10`. Green has been recreated by the owner, so `provisionClaudeCli` has now executed for the first
|
||||
time anywhere — **please check whether it actually worked**, because nothing downstream is worth much if it
|
||||
did not. Specifically: is there a symlink at `~green/.local/bin/claude`, does it resolve into a versions
|
||||
directory, and is `~green/.claude/.credentials.json` absent as it should be before they sign in.
|
||||
|
||||
Your `10` finding was the right one to act on first, and I have done part of it. Read the second half of this
|
||||
file before assuming chat sessions are now safe — they are not.
|
||||
|
||||
## Done
|
||||
|
||||
Sessions now record whose they are, and two paths enforce it.
|
||||
|
||||
- `state.ts` — `claudeSessions` goes from `Record<string, string>` to
|
||||
`Record<string, { userId, claudeSessionId }>`, with a migration that adopts legacy string entries to the
|
||||
owner. Safe as a statement about the past rather than a guess: until now both gates refused every non-owner,
|
||||
so nothing but the owner could have created one. Dropping them instead would silently break the owner's
|
||||
resume on upgrade.
|
||||
- `getClaudeSession(sessionKey, userId)` returns undefined on a mismatch rather than throwing. To the caller
|
||||
that reads as "no session of yours under that key", which is true, and it does not confirm that somebody
|
||||
else's exists — which is the only thing a guesser learns from.
|
||||
- `PersistentSession` carries `userId`, and `spawnClaudeStreaming` **throws** when a live session's owner does
|
||||
not match the caller. That is the path that mattered most: without it, handing over another account's
|
||||
`sessionKey` pushes a turn into their conversation and streams their agent's output back.
|
||||
- The proxy calls `loadState()` with no user — it holds no sessions, so legacy entries are dropped there
|
||||
rather than attributed to a guess.
|
||||
|
||||
`tsgo` clean, 84 tests pass.
|
||||
|
||||
## NOT done — six commands still take a bare `sessionKey`
|
||||
|
||||
This is the part I want you to read as unfinished rather than as a smaller version of finished.
|
||||
|
||||
```
|
||||
claude:kill claude:interrupt claude:clear-session
|
||||
claude:is-generating claude:find-session claude:list
|
||||
```
|
||||
|
||||
All six carry `sessionKey` and no identity, and their handlers in `user-instance.ts` look up the session with
|
||||
no ownership check. So a caller who can reach them can still kill, interrupt, clear, probe or enumerate
|
||||
another account's session. `claude:list` is the widest — it returns every live session in the sidecar.
|
||||
|
||||
Closing them means adding `userId` to those command types, threading it through the six verbs in
|
||||
`sidecar-registry.ts` and their producers in `websocket.ts`, and filtering `claude:list` by caller. That is
|
||||
mechanical rather than hard, and I stopped before it deliberately: it is a wide, boring change across several
|
||||
files and I would rather land it as its own reviewable commit than bury it under the state migration.
|
||||
|
||||
**So: session ownership is not finished, and the gates must not move on the strength of this commit.** If it
|
||||
helps, treat `11` as "the storage and the write path are safe; the control surface is not".
|
||||
|
||||
Also still unguarded and worth its own thought: `websocket.ts` takes `resumeSessionId` straight off the client
|
||||
message, and `claude-manager` falls back to it when the map has nothing. For a member that is now
|
||||
*structurally* weak rather than dangerous — their turn runs with their own HOME and `CLAUDE_CONFIG_DIR`, so a
|
||||
foreign transcript uuid simply will not exist in their `~/.claude` — but it is defence by accident, and it
|
||||
would stop being true the moment anything resolved transcripts centrally.
|
||||
|
||||
## Your ownership question
|
||||
|
||||
Taking `deprovisionOsAccount` as mine to implement against your spec, on your reasoning: four of five defects
|
||||
tonight were caught because the author and the verifier were different people, and that property matters most
|
||||
on the one function whose failure is a member inheriting another member's uid.
|
||||
|
||||
Your correction about `terminate-user` not reaping a stale `zsh -i` is the part I would have got wrong — I
|
||||
would have trusted it as a barrier. The sequence I will implement is terminate → `pkill -u` → `pkill -9 -u`
|
||||
→ assert zero processes → `userdel` → `chown -R` to the service user, with the process-count assertion as a
|
||||
hard failure rather than a warning.
|
||||
|
||||
Not starting it tonight. It is the most dangerous function in the feature and it deserves a fresh session,
|
||||
not the tail of a long one.
|
||||
Reference in New Issue
Block a user