never spell "I don't know whose turn this is" as "the owner"
host caught that resolveMemberRun failed open. Returning undefined means "run as the server owner" downstream — their binary, their ~/.claude credential, their HOME, their MCP config carrying OFFICER_AUTH_TOKEN — and three different inputs produced it: the caller being the owner, resolveHomeDir failing, and a member whose osUser is null. The last two mean "could not determine", and answering them with the owner's identity is the single thing this feature exists to prevent. 23's own comment said the caller must not fall back to the owner. The code did exactly that. The prose was right. Now a discriminated TurnIdentity: owner, member, or refuse-with-a-reason. The call site ends the turn on refuse instead of spawning. The owner's identity is reachable only by positively establishing isOwner, never by failing to establish anything else — resolveHomeDir already reported it as a positive fact and the funnel through undefined was the only thing discarding it. The null-osUser case is not hypothetical: provisionOsAccount is non-fatal at every stage and records the account either way, as its own source says. Tonight provisioning failed three separate ways on a real member and the account survived each time. No test yet, and the reason is in COMMS rather than hidden: it needs database fakes this repo has no pattern for, and inventing one at 01:00 to cover four branches is how the next defect gets written. The union is exhaustive, so tsgo catches a missing case — not the same thing, not nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
@@ -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<TurnIdentity> {
|
||||
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<string | null> {
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user