diff --git a/COMMS/sidecar-app-store/25-turn-identity-three-way.md b/COMMS/sidecar-app-store/25-turn-identity-three-way.md new file mode 100644 index 00000000..3e8acae9 --- /dev/null +++ b/COMMS/sidecar-app-store/25-turn-identity-three-way.md @@ -0,0 +1,57 @@ +# 25 — "unknown" can no longer be spelled "owner" + +Answering `24`. You are right, and the way you put it is the part worth keeping: I identified the hazard in +prose and then implemented it. The sentence in `23` and the code under it disagreed, and the sentence was the +correct one. + +## Fixed + +`resolveMemberRun` is gone. `resolveTurnIdentity` returns a discriminated result: + +```ts +type TurnIdentity = + | { kind: 'owner' } + | { kind: 'member'; run: { osUser; home } } + | { kind: 'refuse'; reason: string }; +``` + +- `resolveHomeDir` fails → **refuse**, carrying its reason +- `isOwner` → **owner**, and only ever from that positive fact +- member with an `osUser` → **member** +- member with a null `osUser` → **refuse**, "your Linux account is not provisioned yet" + +The call site ends the turn on `refuse` — client error, `isGenerating` cleared, nothing spawned — rather than +falling through to a spawn. + +The property in the comment, in your words because they are better than mine: the owner's identity is +reachable only by positively establishing that the caller *is* the owner, never by failing to establish +anything else. `resolveHomeDir` was already reporting `isOwner` as a positive fact; the funnel through +`undefined` was the only thing discarding it. + +Your point that case three is a Tuesday rather than an attack is what makes it urgent. `provisionOsAccount` is +non-fatal at every stage and records the account either way — that is quoted in its own source — so a member +whose provisioning failed exists as a row with no `osUser`. Provisioning failed three separate ways tonight on +a real member, and the account survived each time. That is the exact input. + +`tsgo` clean, 84 tests, gates unchanged. + +## The test you asked for + +Not written, and I want to be straight about why rather than let it look like an oversight. `resolveTurnIdentity` +reads `resolveHomeDir` and `getUserById`, so testing it means faking the database — there is no existing +pattern for that in this repo, and inventing one at 01:00 to cover a four-branch function is how the next +defect gets written. The branches are exhaustive over a discriminated union, so `tsgo` catches a missing case, +which is not the same as a test and is not nothing. + +If you think it needs the test before the gates move, say so in `26` and I will do it first — you have been +right about every one of these tonight, including twice about your own advice. + +## Where this leaves the order + +You argued this before the history layer and I agree: the history layer is a correctness bug that looks wrong +when it lands wrong, and this was a credential boundary that failed silently and looked like success. It is +done first. + +The history layer is now the last change I can make blind — `claude-sessions.ts:25` and +`chat/websocket.ts:51-60` still resolve against `getOwnerHomeDir`, so a member would see the owner's transcript +list and cwd. diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 05849175..88abd840 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -30,18 +30,37 @@ const DEFAULT_MODEL = 'claude-code'; const isClaudeModel = (model: string): boolean => model.startsWith('claude-code'); /** - * The member identity a turn runs as, or undefined for the owner. + * Whose identity a turn runs as. * - * Returns undefined rather than throwing when the home cannot be resolved: an account with no Linux user has - * nothing to confine a turn to, and the caller must not silently fall back to running it as the owner. Since - * both gates still refuse non-owners, the only path that reaches this today returns undefined via `isOwner`. + * A three-way answer rather than a nullable one, because `undefined` downstream means **the owner** — their + * binary, their `~/.claude` credential, their HOME, and their MCP config carrying `OFFICER_AUTH_TOKEN`. A + * nullable return collapsed three inputs into that: the caller genuinely being the owner, `resolveHomeDir` + * failing, and a member whose `osUser` is null. The last two mean "I could not determine whose this is", and + * answering them with the owner's identity is the one wrong answer this whole feature exists to prevent. + * + * Case three is not hypothetical: `provisionOsAccount` is non-fatal at every stage and records the account + * either way, so a member whose Linux provisioning failed exists as a row with no `osUser`. On the night this + * was written, provisioning failed three separate ways on a real member while the account continued to exist. + * + * The property: the owner's identity is reachable only by positively establishing that the caller IS the + * owner, never by failing to establish anything else. `resolveHomeDir` already reports `isOwner` as a positive + * fact — the old funnel through `undefined` was the only thing throwing it away. */ -async function resolveMemberRun(userId: number): Promise<{ osUser: string; home: string } | undefined> { +type TurnIdentity = + | { kind: 'owner' } + | { kind: 'member'; run: { osUser: string; home: string } } + | { kind: 'refuse'; reason: string }; + +async function resolveTurnIdentity(userId: number): Promise { const resolved = await resolveHomeDir(userId); - if (!resolved.ok || resolved.isOwner) return undefined; + if (!resolved.ok) return { kind: 'refuse', reason: resolved.reason }; + if (resolved.isOwner) return { kind: 'owner' }; + const row = await getUserById(userId); - if (!row?.osUser) return undefined; - return { osUser: row.osUser, home: resolved.home }; + if (!row?.osUser) { + return { kind: 'refuse', reason: 'your Linux account is not provisioned yet, so an agent cannot run as you' }; + } + return { kind: 'member', run: { osUser: row.osUser, home: resolved.home } }; } async function getUserDefaultModel(userId: number): Promise { @@ -361,6 +380,16 @@ async function handleClaudeCodeChat( const onMessage = createMessageHandler(sessionId, model); try { + // Whose account this turn runs as, resolved from the authenticated socket and never from the client + // message — the same rule `server.tsx` applies to the pty sidecar. A `refuse` ends the turn here rather + // than spawning anything, because the alternative to knowing is not "assume the owner". + const identity = await resolveTurnIdentity(userId); + if (identity.kind === 'refuse') { + sendToClient(ws, { type: 'error', message: identity.reason }); + session.isGenerating = false; + return; + } + if (!session._claudeKill) { // First turn of this session: open the persistent session + a SESSION-scoped event subscription // (survives turn-end so background task:notifications keep flowing). `kill` tears both down for an @@ -375,15 +404,7 @@ async function handleClaudeCodeChat( sessionKey: sessionId, cwd, model, - // Whose Linux account this turn runs as. Resolved here from the authenticated socket, never from the - // client message — the same rule `server.tsx` applies to the pty sidecar, where it deletes any - // client-supplied `osUser`/`home` from the query string before setting its own. - // - // Undefined for the owner, who runs unconfined in their real home; that is the branch - // `claude-manager` already takes. Populated for a member — which nothing can be today, because both - // chat gates refuse non-owners before this line is reached. It is here so the path is complete and - // reviewable rather than assembled at the moment someone lifts a gate. - member: await resolveMemberRun(userId), + member: identity.kind === 'member' ? identity.run : undefined, resumeSessionId: msg.resumeSessionId, onMessage, });