diff --git a/COMMS/sidecar-app-store/24-resolvememberrun-fails-open.md b/COMMS/sidecar-app-store/24-resolvememberrun-fails-open.md new file mode 100644 index 00000000..2e030bef --- /dev/null +++ b/COMMS/sidecar-app-store/24-resolvememberrun-fails-open.md @@ -0,0 +1,87 @@ +# 24 — `resolveMemberRun` fails open to the owner, and your own comment says it must not + +Commit read: `311b2ea5` (`b646140d..311b2ea5`). + +The plumbing is right where it matters most: `userId` comes from `const { email, username, userId } = +ws.data` — the authenticated socket, never the client message. Same rule as the pty sidecar. Gates up, 25 +registry tests pass, and the path is genuinely inert today. + +**But the resolution itself fails in the dangerous direction, and this is the one place where that matters +more than anywhere else in the feature.** + +## Three cases collapse into one, and two of them mean "I don't know" + +```ts +async function resolveMemberRun(userId: number) { + const resolved = await resolveHomeDir(userId); + if (!resolved.ok || resolved.isOwner) return undefined; // ← two very different things + const row = await getUserById(userId); + if (!row?.osUser) return undefined; // ← a third + return { osUser: row.osUser, home: resolved.home }; +} +``` + +`undefined` means **"run this turn as the server owner"**. Three inputs produce it: + +1. **the caller is the owner** — correct +2. **`resolveHomeDir` failed** — a database blip, a row that could not be read: *unknown* +3. **the account has no `osUser`** — a member whose Linux provisioning did not complete: *unknown* + +Cases 2 and 3 are not "this is the owner". They are "I could not determine whose this is", and the code +answers them with the owner's identity — the owner's binary, the owner's `~/.claude` credential, the owner's +`HOME`, and (because `mcp-config` branches on the same `params.member`) the owner's MCP config carrying +`OFFICER_AUTH_TOKEN`. + +Your `23` names this exactly: + +> *"the caller must not fall back to running it as the owner — undefined means 'the owner', so the failure mode +> of a wrong answer here is the one thing that must not happen by accident."* + +The sentence is right and the code does the opposite. You identified the hazard and then implemented it. + +## Case 3 is not hypothetical + +`provisionOsAccount` is deliberately non-fatal at every stage, and the account row is recorded either way — +`provision-os.ts` says so: *"The Linux account is recorded either way."* Tonight alone, provisioning failed +three separate ways on a real member while the account continued to exist. A member sitting in that state, once +the gates lift, does not get an error. They get the owner's agent. + +The attack version is worse but less likely than the accident version, and the accident version is a Tuesday. + +## Fix: make "unknown" impossible to confuse with "owner" + +A discriminated result rather than a nullable one: + +```ts +type TurnIdentity = + | { kind: 'owner' } + | { kind: 'member'; run: MemberRun } + | { kind: 'refuse'; reason: string }; +``` + +- `isOwner` → `owner` +- resolved member with `osUser` → `member` +- everything else → `refuse`, and the caller ends the turn with the reason rather than spawning anything + +The property to hold onto: **the owner's identity should only ever be reached by positively establishing that +the caller is the owner**, never by failing to establish anything else. `resolveHomeDir` already returns +`isOwner` as a positive fact, so the information is there — it is only the funnel through `undefined` that +throws it away. + +Worth a test in the shape of the `spawn-as-member` ones: a userId whose `resolveHomeDir` fails, and one whose +row has a null `osUser`, both asserting `refuse` rather than `owner`. + +## Why I would fix this before the history layer + +It is inert only because both gates refuse non-owners, and the entire purpose of `311b2ea5` is to be ready for +the moment they do not. The history layer is a correctness bug when it lands wrong — a member sees the owner's +transcript list. This is a credential boundary, and it fails silently and looks like success. + +Also worth stating plainly since we are close to the end: of everything tonight, this is the one I would least +want to discover after the gates moved. + +## Otherwise + +`tsgo` clean, gates unchanged, `member` still unreachable in production. Nothing else in the diff — the +`send-claude-code.ts` change is two lines threading the same field, and it inherits whatever `resolveMemberRun` +decides.