diff --git a/COMMS/sidecar-app-store/11-session-ownership-partial.md b/COMMS/sidecar-app-store/11-session-ownership-partial.md new file mode 100644 index 00000000..99606c5c --- /dev/null +++ b/COMMS/sidecar-app-store/11-session-ownership-partial.md @@ -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` to + `Record`, 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. diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 1480093f..4bb52ce0 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -61,7 +61,7 @@ type ClaudeCodeOutput = { export async function spawnClaude(params: ClaudeSpawnParams): Promise { const { prompt, sessionKey } = params; - const existingSession = getClaudeSession(sessionKey); + const existingSession = getClaudeSession(sessionKey, params.userId); const claudeArgs = [CLAUDE_BIN, '-p', prompt, '--dangerously-skip-permissions', '--output-format', 'json']; @@ -120,7 +120,7 @@ export async function spawnClaude(params: ClaudeSpawnParams): Promise void; closeInput: () => void; @@ -303,6 +305,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat const session: PersistentSession = { sessionKey, + userId: params.userId, query: undefined as unknown as Query, pushTurn: () => {}, closeInput: () => input.close(), @@ -316,7 +319,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // Strip the nested-session guard vars so the SDK can spawn `claude` (mirrors the old spawn env clean). const { CLAUDECODE: _c, CLAUDE_CODE_ENTRYPOINT: _e, CLAUDE_CODE_SSE_PORT: _s, ...cleanEnv } = process.env; - const resumeId = getClaudeSession(sessionKey) ?? params.resumeSessionId; + const resumeId = getClaudeSession(sessionKey, params.userId) ?? params.resumeSessionId; const subModel = params.model?.split('/')[1]; const q = query({ @@ -450,7 +453,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat for await (const msg of q as AsyncGenerator>) { processMessage(msg, state, { onEvent: emit, - onSessionId: (id: string) => setClaudeSession(sessionKey, id), + onSessionId: (id: string) => setClaudeSession(sessionKey, id, params.userId), }); } } catch (err) { @@ -473,6 +476,15 @@ export async function spawnClaudeStreaming( onEvent: (event: ChatEvent) => void, ): Promise { let session = sessions.get(params.sessionKey); + if (session && session.userId !== params.userId) { + // A live session belongs to whoever started it. Without this, handing over someone else's `sessionKey` + // pushes a turn into their conversation and streams their agent's output back — the chat equivalent of + // resuming another account's shell, which `4d4a253f` refused for the pty sidecar. + // + // Throws rather than silently starting a fresh session under the same key: the caller asked to continue a + // specific conversation, and quietly giving them a different one is its own kind of wrong. + throw new Error('that chat session belongs to another account'); + } if (session) { session.emit = onEvent; // adopt the latest emitter (equivalent across turns; keeps events flowing) } else { @@ -536,7 +548,7 @@ export function listSessions(): LiveClaudeSession[] { sessionKey: session.sessionKey, // The only place this mapping exists. Without it a caller cannot find the transcript, because the // key is officer's handle and the filename is Claude's id. - claudeSessionId: getClaudeSession(session.sessionKey) ?? null, + claudeSessionId: getClaudeSession(session.sessionKey, session.userId) ?? null, isGenerating: session.isGenerating, pendingTasks: session.pendingTasks.size, })); diff --git a/src/servers/sidecar/claude/state.ts b/src/servers/sidecar/claude/state.ts index f2fb317e..2b47093e 100644 --- a/src/servers/sidecar/claude/state.ts +++ b/src/servers/sidecar/claude/state.ts @@ -3,9 +3,22 @@ import { mkdirSync, existsSync, readFileSync, writeFileSync, unlinkSync } from ' const DATA_PATH = process.env.DATA_PATH ?? join(process.cwd(), 'data'); +/** + * A resumable session, and whose it is. + * + * `userId` exists because the map is global to this sidecar and `sessionKey` travels in a client message. The + * pty sidecar learned this first (`4d4a253f`): *"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."* Chat never got the same treatment, because both gates made it unreachable and therefore invisible. + * + * Stored rather than derived: there is nothing in a `sessionKey` or a Claude transcript uuid that says who + * owns it, so ownership has to be written down at the moment it is created. + */ +export type SessionRecord = { userId: number; claudeSessionId: string }; + export type PersistedState = { proxySecret: string; - claudeSessions: Record; // sessionKey → Claude Code session_id + claudeSessions: Record; // sessionKey → whose, and which transcript }; const DEFAULT_STATE: PersistedState = { @@ -41,7 +54,11 @@ function ensureDir() { } } -export function loadState(): PersistedState { +/** + * @param ownerUserId who existing sessions belong to — see the migration below. Omitted by the proxy + * process, which holds no sessions; legacy entries are then dropped rather than attributed to a guess. + */ +export function loadState(ownerUserId?: number): PersistedState { ensureDir(); try { if (!existsSync(stateFile)) { @@ -50,6 +67,7 @@ export function loadState(): PersistedState { } const text = readFileSync(stateFile, 'utf-8'); currentState = { ...DEFAULT_STATE, ...JSON.parse(text) }; + currentState.claudeSessions = migrateSessions(currentState.claudeSessions, ownerUserId); return currentState; } catch { currentState = { ...DEFAULT_STATE }; @@ -57,6 +75,28 @@ export function loadState(): PersistedState { } } +/** + * Entries used to be a bare `sessionKey → transcript uuid` string. Adopt those to the owner. + * + * Safe because it is a statement about the past rather than a guess: until this commit, `api/chat/chat.ts` + * and the chat socket refused every non-owner, so nothing but the owner could ever have created one. The + * alternative — dropping unrecognised entries — would silently lose the owner's resumable history on upgrade, + * and "my old chats stopped resuming" is a bad way to discover a migration. + */ +function migrateSessions(raw: Record, ownerUserId?: number): Record { + const out: Record = {}; + for (const [key, value] of Object.entries(raw ?? {})) { + if (typeof value === 'string') { + if (ownerUserId !== undefined) out[key] = { userId: ownerUserId, claudeSessionId: value }; + } else if (value && typeof value === 'object' && 'claudeSessionId' in value && 'userId' in value) { + out[key] = value as SessionRecord; + } + // Anything else is unreadable and dropped: a malformed entry cannot be attributed to anyone, and + // guessing an owner for it is exactly the mistake this whole change exists to stop. + } + return out; +} + export async function saveState(): Promise { ensureDir(); await Bun.write(stateFile, JSON.stringify(currentState, null, 2)); @@ -83,9 +123,10 @@ export function updateState(patch: Partial): void { * There is nothing to debounce: `onSessionId` fires on every message but with the same id, so the * equality guard collapses it to one write per session, and clearing happens once. */ -export function setClaudeSession(sessionKey: string, sessionId: string): void { - if (currentState.claudeSessions[sessionKey] === sessionId) return; - currentState.claudeSessions[sessionKey] = sessionId; +export function setClaudeSession(sessionKey: string, sessionId: string, userId: number): void { + const existing = currentState.claudeSessions[sessionKey]; + if (existing?.claudeSessionId === sessionId && existing.userId === userId) return; + currentState.claudeSessions[sessionKey] = { userId, claudeSessionId: sessionId }; writeThrough(); } @@ -101,8 +142,17 @@ function writeThrough(): void { }); } -export function getClaudeSession(sessionKey: string): string | undefined { - return currentState.claudeSessions[sessionKey]; +/** + * The transcript for this session key, **only if it belongs to the caller**. + * + * A mismatch returns undefined rather than throwing: to the caller it is simply "no session to resume", which + * is the truthful answer — there is no session of theirs under that key. Throwing would confirm that somebody + * else's exists, which is the one thing a guesser learns from. + */ +export function getClaudeSession(sessionKey: string, userId: number): string | undefined { + const record = currentState.claudeSessions[sessionKey]; + if (!record || record.userId !== userId) return undefined; + return record.claudeSessionId; } /** @@ -122,7 +172,7 @@ export function findSessionKeyByClaudeSession(claudeSessionId: string): string | const keys = Object.keys(currentState.claudeSessions); for (let i = keys.length - 1; i >= 0; i--) { const key = keys[i]!; - if (currentState.claudeSessions[key] === claudeSessionId) return key; + if (currentState.claudeSessions[key]?.claudeSessionId === claudeSessionId) return key; } return undefined; } diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index 097d0e09..deb832ba 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -97,7 +97,7 @@ if (!acquireLock()) { process.exit(1); } -loadState(); +loadState(dbUser.id); // ── MCP config ── diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 03e56e56..94becd93 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -152,7 +152,7 @@ export type SidecarEvent = export type ClaudeState = { proxySecret: string; - claudeSessions: Record; // sessionKey → Claude Code session_id + claudeSessions: Record; // sessionKey → whose, and which transcript }; // ── Param types ──