From d974ee63af7c6805789b464fd9d37e91d1f464f7 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Wed, 21 Oct 2026 19:44:25 +0800 Subject: [PATCH] Scope set-main's replace to the caller's own Main Bot (#613) set-main checked only body.id, so a member could hand another owner's Main Bot's place to their own Bot: the swap both promoted theirs to the deployment-wide Main list and demoted the other owner's Main Bot. The endpoint now checks the Bot named in replace, and setMain refuses a replace across owners so the state invariant holds for every caller. Tests: core/test/authz.test.js and core/test/roster.test.js, one per half; docs/subsystems/core-host.md and a changelog fragment go with it. --- changelog.d/613-main-bot-owner.security.md | 1 + core/src/host/authz.js | 11 +++++++- core/src/host/roster.js | 4 +++ core/test/authz.test.js | 25 ++++++++++++++++ core/test/roster.test.js | 31 ++++++++++++++++++++ docs/subsystems/core-host.md | 1 + patches/ds-bot/0002-flybot-api-host.patch | 11 ++++++++ patches/ds-bot/0009-identity-schema.patch | 33 +++++++++++++++++++++- patches/ds-bot/files/src/host/authz.js | 11 +++++++- patches/ds-bot/files/test/authz.test.js | 25 ++++++++++++++++ 10 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 changelog.d/613-main-bot-owner.security.md diff --git a/changelog.d/613-main-bot-owner.security.md b/changelog.d/613-main-bot-owner.security.md new file mode 100644 index 000000000..68e54ad3d --- /dev/null +++ b/changelog.d/613-main-bot-owner.security.md @@ -0,0 +1 @@ +`set-main` no longer lets a member take another owner's Main Bot's place: the endpoint checks the Bot named in `replace` too, and `setMain` refuses a `replace` across owners, so a member cannot demote another owner's Main Bot or promote their own Bot over it ([#613](https://github.com/ThinkFlowLab/FlyBot/issues/613)). diff --git a/core/src/host/authz.js b/core/src/host/authz.js index 298b55ffa..e75c860f1 100644 --- a/core/src/host/authz.js +++ b/core/src/host/authz.js @@ -257,7 +257,16 @@ export function install(rt) { return requireSession(body?.sessionId) } if (ep === 'usage') return body?.botId === undefined ? ALLOW : requireBot(body.botId) - if (ep === 'set-main') return requireBot(body?.id) + if (ep === 'set-main') { + // `replace` names the Main Bot whose place the target takes, and the Main list is + // deployment-wide: checking only `body.id` let a member swap another owner's Main Bot + // for their own and demote it (#613). + if (typeof body?.replace === 'string' && body.replace !== '' && body.replace !== body?.id) { + const replaceGate = requireBot(body.replace) + if (!replaceGate.ok) return replaceGate + } + return requireBot(body?.id) + } if (ep === 'set-flags') { const id = body?.id ?? body?.botId if (botOwnedBy(actor, id) || roomOwnedBy(actor, id) || roomMemberOf(actor, id)) return ALLOW diff --git a/core/src/host/roster.js b/core/src/host/roster.js index 970bae576..eaefbf547 100644 --- a/core/src/host/roster.js +++ b/core/src/host/roster.js @@ -500,6 +500,10 @@ export function install(rt) { } else if (replace !== undefined && replace !== id) { const at = others.indexOf(replace) if (at === -1) throw new Error('Only a Main Bot can be replaced') + // The Main list is deployment-wide, so a swap is only within one owner: taking another + // member's Main Bot's place demotes it for them (#613). authz.js refuses the endpoint for + // the same reason; this keeps the state invariant for every caller. + if ((rt.botOf(replace)?.ownerId ?? null) !== (bot.ownerId ?? null)) throw new Error('Only a Bot of the same owner can take a Main Bot’s place') next = others.with(at, id) } else if (primary) { next = [id, ...others] diff --git a/core/test/authz.test.js b/core/test/authz.test.js index 8af99073a..f67b1130d 100644 --- a/core/test/authz.test.js +++ b/core/test/authz.test.js @@ -433,3 +433,28 @@ test('a group\'s question card is its Bot owner\'s to see and answer, not every assert.deepEqual(Object.keys(rt.ownedView(full, actorA).questions), ['rx']) assert.deepEqual(Object.keys(rt.ownedView(full, actorB).questions), []) }) + +// #613: the Main Bot list is deployment-wide, so `replace` crosses the owner boundary unless the +// actor owns both the Bot it promotes and the Main Bot whose place it takes. +test('#613: a member cannot take another owner’s Main Bot’s place', async (t) => { + const { rt } = runtime(t, { ...TEAM, mainBotIds: ['m1'] }) + await rt.load() + const sessionA = await redeemOwner(rt) + const sessionB = await redeemOwner(rt) + assert.equal(sessionB.memberId, 'owner-2') + rt.state.bots.a1 = { id: 'a1', sessionId: 'a1', name: 'A', instructions: '', createdAt: 2, ownerId: 'owner-1' } + rt.state.bots.b1 = { id: 'b1', sessionId: 'b1', name: 'B', instructions: '', createdAt: 3, ownerId: 'owner-2' } + const actorA = await rt.authenticate(sessionA.sessionToken) + const actorB = await rt.authenticate(sessionB.sessionToken) + + // Bob's own Bot may not take Alice's Main Bot's place: promoting it that way demotes hers. + const escalation = rt.authorize(actorB, 'set-main', { id: 'b1', main: true, replace: 'm1' }) + assert.equal(escalation.ok, false) + assert.equal(escalation.code, 'bot/forbidden') + // Demoting hers by naming it as the target must not work either. + assert.equal(rt.authorize(actorB, 'set-main', { id: 'm1', main: false }).ok, false) + // A swap inside one owner's team stays allowed. + assert.equal(rt.authorize(actorA, 'set-main', { id: 'a1', main: true, replace: 'm1' }).ok, true) + // And Bob's own Bot keeps the plain promotion the deployment-wide list gives it. + assert.equal(rt.authorize(actorB, 'set-main', { id: 'b1', main: true }).ok, true) +}) diff --git a/core/test/roster.test.js b/core/test/roster.test.js index 6054eebf2..1a1f4e6b2 100644 --- a/core/test/roster.test.js +++ b/core/test/roster.test.js @@ -593,3 +593,34 @@ test('a failed Main Bot create does not leave a dangling designated Chief', asyn assert.equal(saved.members['owner-1'].designatedChiefId, null) assert.equal(Object.keys(saved.bots || {}).length, 0) }) + +// #613: `setMain` is the layer that writes `mainBotIds`, so the same-owner rule is enforced here +// too, not only at the endpoint: a `replace` across owners would demote another member's Bot. +test('#613: a Main Bot’s place is only taken by a Bot of the same owner', async (t) => { + const { call, stop, home } = start({ + version: 2, + revision: 1, + members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'chief-a' }, 'owner-2': { id: 'owner-2', createdAt: 2, designatedChiefId: 'chief-b' } }, + rooms: {}, + exchanges: [], + exchangeTurns: {}, + questions: {}, + prefs: {}, + mainBotIds: ['chief-a'], + bots: { + 'chief-a': { id: 'chief-a', name: 'Alice Chief', ownerId: 'owner-1' }, + 'chief-b': { id: 'chief-b', name: 'Bob Chief', ownerId: 'owner-2' }, + 'helper-b': { id: 'helper-b', name: 'Bob Helper', ownerId: 'owner-2' }, + }, + }) + t.after(stop) + const refused = await call('set-main', { id: 'chief-b', main: true, replace: 'chief-a' }) + assert.equal(refused.ok, false) + assert.match(refused.error.message, /same owner/) + assert.deepEqual(JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')).mainBotIds, ['chief-a']) + // A swap within Bob's own team still works. + await call('set-main', { id: 'chief-b', main: true }) + const swapped = await call('set-main', { id: 'helper-b', main: true, replace: 'chief-b' }) + assert.equal(swapped.ok, true) + assert.deepEqual(swapped.value.mainBotIds, ['chief-a', 'helper-b']) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 84299369f..d3ad5d67d 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -68,6 +68,7 @@ external backend or task-specific session mapping; see the ### Team state and access - The saved team (`state.json`) is versioned. Version 2 (#47) carries the identity schema — `members` records, a per-Bot `ownerId`, the owner's `designatedChiefId`. A legacy version-1 file is migrated on load with a one-time `state.json.v1` backup; an existing backup counts only when it is a readable file holding a parseable version-1 state, and anything else (or any malformed ownership) fails closed (read-only). An older binary refuses a newer version read-only. This is schema foundation only — team mode stays disabled, see [team](team.md). On top of it, member auth (#48) adds host module `src/host/member-auth.js`, installed after `store` in `MODULES` (`core/index.js`): `invitations` and `authSessions` maps live on state (no `STATE_VERSION` bump), the channel resolves the actor from the `x-flybot-member-session` header, and endpoints are `auth-invite` / `auth-redeem` / `auth-revoke` / `auth-whoami` / `auth-remove-member` / `auth-revoke-invite` with `body.actorId` ignored; `auth-invite` alone also opens for an anonymous caller on a host with no sessions yet (bootstrap first invite), `auth-revoke-invite` kills an unredeemed invitation (redeeming it afterwards fails with `revoked`), `removeMember` refuses a member that still owns a Bot (`has_bots`), and the auth mutations are serialized behind an in-process lock so concurrent `removeMember` calls cannot both pass the last-owner check. Sign-in surface only — still not team mode; details in [team](team.md). On top of it, owner-scoped access (#49) adds host module `src/host/authz.js`, installed after member-auth: when any `authSessions` exist, the channel `authorize()` default-denies cross-owner Bot/room/session access, blocks shared non-isolatable resources (secrets, connectors, folder picks, browser bridge, update-install, diagnostics, set-prefs, and the DSH Agent with its Sessions — `add-agent`, `agent-session`, `agent-adopt`, `agent-archive`, `agent-rename`, `remove-agent`, which name an ownerless `agentId` and are one per deployment) with `bot/blocked-shared` when more than one member exists, stamps `create-bot` with `actor.memberId`, and filters `view()`/`live()` to the actor — every answer that returns a view, `delete-bot`, `set-main`, `remove-agent` and `delete-room` included, passes the actor so it stays scoped; with no sessions the single-human behavior is unchanged. Session targets resolve through the store's owner maps — a Bot id via `botOf`, current chats and group Sessions and earlier parts via `selfOf`/`groupOwners`/`pastOwners` (a shared room Session also resolves for its current participants, #50) — and unresolvable ids deny. `approval-answer` gates on the approval's own Bot or room (resolved by approval id), never on caller-supplied session fields. `flybot/*` plugin endpoints (src/host/hooks.js) reach any signed-in actor for reads; plugin writes follow the shared rule. `view()`/`live()` narrowing (`rt.ownedView`/`rt.ownedLive` in authz.js) keeps per-session entries (`questions`, `exchangeTurns`) the actor owns, and deployment-wide surfaces (agents, secrets, connectors, memory reviews) only while a single member exists — the same rule as the shared writes; rooms the actor belongs to and the Bots in them (#50) stay visible. `create-room` refuses a request with neither a member Bot nor any participant. On top of it, room membership (#50) adds host module `src/host/room-membership.js`, installed after groups: a room may carry typed `participants` (`human` / local `bot` / granted remote `bot` with `handle` + owner fields, roles `admin` / `member`, `joinedSeq` / `leftSeq`); log lines carry `authorId` + `authorKind`; history is the membership window (current members and the owner of an invited Bot see `seq >=` their interval; leave withdraws access; rejoin starts a new interval); local cross-owner rooms accept each side's *granted* Bots (#90: default deny, Chief included); a friend's Bot from `social/remote-bots` (`cleanRemote`) may be added and ungranted Bots stay private, and a room with a friend's Bot holds only granted local Bots (a Bot that joined by room code, `viaCode`, only for that room); `view()` labels remotes with an owner badge and shows an invited owner **Shared room from \**. Face-to-face rooms (#83) reuse the same `participants` / membership-window / owner-badge presentation with `origin: 'face-to-face'`: room-only remotes may sit in the room without a person-grant or a friendship, `presentRoom` treats the viewer as a member (not Shared room from), leave skips the two-Bot floor, and `fanoutRoomLine` goes to `rt.faceRoomFanout` instead of friend `flybotSend`. Still not team mode — see [team](team.md). Per-Bot grants (#90) add host module `src/host/reach.js`, installed after room-membership: `state.grants[botId]` is the list of independent grants (one Bot → one grantee, optionally room-scoped); default deny, Chief included; a Bot whose owner is not a member has none. Grants are set with `rt.setGrants`, exposed as `BotView.granted`, `flybot.bot.hasGrant` / `hasAnyGrant`, `flybot.grants` and the `bot.grants` event. `reach()` is the owner policy: a friend's Bot may reach only local Bots granted to that sender, only a granted Bot (or one code-joined in that room) may message a friend's Bot, another owner's Bot is reached only if both sides have matching grants or both are members of the room the message belongs to. It is not a middleware: hooks.js runs it before the `bot.send` chain, again on approval release, and on every deliver with a sender or remote origin (resumed, retried, deferred, re-sent), so no verdict or `bypass` can skip it and a queued message is judged by what is true when it is delivered. Revoking a grant, or replacing its Chief, removes it from rooms that have friends' Bots or several owners at once (`rt.evictUnshared`). Approval binding (#52) completes the held-send half of this: `state.flybot.approvals` records carry `requestId`, a sha256 `version` over the request content, server-derived `ownerId` and `origin` (`via`/`hop`/audience) — claims in the message (`meta`, text) never confer authority. `flybot/decide` is allowed through authz for any signed-in actor and the per-record check happens in hooks (`decide` requires the sender Bot's owner's session, `bot/forbidden` otherwise, `bot/unauthorized` without a session); `release()` revalidates before dispatch and marks the record `stale` when the request changed, the sender or target Bot is gone (also invalidated eagerly on `deleteBot`), or the Bot changed owners; a demoted Chief's sender-side holds are invalidated eagerly on `setMain` (not inside `staleReason`), and `release()` re-checks immediately before dispatch so an in-flight approve cannot go out after a concurrent invalidation. Unknown `invalidateFor` field values throw. `flybot/approvals` lists only the actor's own holds while enforcement is on; decided stale records keep their attribution in state but appear in no member's list. +- The Main Bot list is deployment-wide (#613): `set-main` requires the actor to own `body.id` and, when `body.replace` names the Main Bot whose place the target takes, that Bot too, so a member cannot swap another owner's Main Bot for their own or demote it (both are `bot/forbidden`); `roster.setMain` keeps the same invariant for every caller, refusing a `replace` across owners and leaving `state.mainBotIds` unchanged. A swap inside one owner's team, the plain promotion of a Bot the caller owns, and the deployment-wide list itself are unchanged. - Context sharing (#53) is specified in [team](team.md), **Context sharing foundation**: the own-chat brief is share-gated (`share-brief` / `briefShared`), a memory review reads only its owner as the user (`isUserLine`) and labels another human as `peer` (not `bot`), and keeps cross-owner entries out of the team scope (`teamAllowedFrom`), `list_bots` lists only the caller's owner's Bots (a granted child: that child's `createdBy` subtree), Bots granted to the same owner, and Bots in the current room — another owner's folder and instructions stay private — and a `dsh-session:` chip in actor-authored `send` / `room-post` / `answer-question` / `schedule-create` text must resolve for the author (`sessionOwnedBy`; `schedule-create` authz keys off `body.target`). Bot-authored group posts and Bot-to-Bot / own-chat followups drop chips so they cannot tunnel a private chat into another Session. Host modules: `src/host/groups.js`, `src/host/memory.js`, `src/host/tools.js`, `src/host/channel.js`, `src/host/room-membership.js`, `src/host/authz.js`. Team mode stays disabled. diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index 2c3007d7f..21b32d0c2 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -370,6 +370,17 @@ diff --git a/src/host/roster.js b/src/host/roster.js rt.roomInbox.delete(id) const failed = await saveForDelete() // Armed scheduled tasks refuse a plain archive and would keep delivering into +@@ -409,6 +409,10 @@ export function install(rt) { + } else if (replace !== undefined && replace !== id) { + const at = others.indexOf(replace) + if (at === -1) throw new Error('Only a Main Bot can be replaced') ++ // The Main list is deployment-wide, so a swap is only within one owner: taking another ++ // member's Main Bot's place demotes it for them (#613). authz.js refuses the endpoint for ++ // the same reason; this keeps the state invariant for every caller. ++ if ((rt.botOf(replace)?.ownerId ?? null) !== (bot.ownerId ?? null)) throw new Error('Only a Bot of the same owner can take a Main Bot’s place') + next = others.with(at, id) + } else if (primary) { + next = [id, ...others] @@ -422,6 +422,7 @@ export function install(rt) { if (rt.botOf(other)?.kickoff?.main) delete rt.botOf(other).kickoff } diff --git a/patches/ds-bot/0009-identity-schema.patch b/patches/ds-bot/0009-identity-schema.patch index de6ab57dd..81164e707 100644 --- a/patches/ds-bot/0009-identity-schema.patch +++ b/patches/ds-bot/0009-identity-schema.patch @@ -53,7 +53,7 @@ diff --git a/test/roster.test.js b/test/roster.test.js test('a new team starts with a DeepSeek-blue whale as its Main Bot', async (t) => { const { call, stop } = start() t.after(stop) -@@ -526,3 +526,34 @@ test('a write that fails at save time archives the Session it made', async (t) = +@@ -526,3 +526,65 @@ test('a write that fails at save time archives the Session it made', async (t) = const saved = JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')) assert.equal(Object.values(saved.bots).some(bot => bot.name === 'Phantom'), false) }) @@ -88,3 +88,34 @@ diff --git a/test/roster.test.js b/test/roster.test.js + assert.equal(saved.members['owner-1'].designatedChiefId, null) + assert.equal(Object.keys(saved.bots || {}).length, 0) +}) ++ ++// #613: `setMain` is the layer that writes `mainBotIds`, so the same-owner rule is enforced here ++// too, not only at the endpoint: a `replace` across owners would demote another member's Bot. ++test('#613: a Main Bot’s place is only taken by a Bot of the same owner', async (t) => { ++ const { call, stop, home } = start({ ++ version: 2, ++ revision: 1, ++ members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'chief-a' }, 'owner-2': { id: 'owner-2', createdAt: 2, designatedChiefId: 'chief-b' } }, ++ rooms: {}, ++ exchanges: [], ++ exchangeTurns: {}, ++ questions: {}, ++ prefs: {}, ++ mainBotIds: ['chief-a'], ++ bots: { ++ 'chief-a': { id: 'chief-a', name: 'Alice Chief', ownerId: 'owner-1' }, ++ 'chief-b': { id: 'chief-b', name: 'Bob Chief', ownerId: 'owner-2' }, ++ 'helper-b': { id: 'helper-b', name: 'Bob Helper', ownerId: 'owner-2' }, ++ }, ++ }) ++ t.after(stop) ++ const refused = await call('set-main', { id: 'chief-b', main: true, replace: 'chief-a' }) ++ assert.equal(refused.ok, false) ++ assert.match(refused.error.message, /same owner/) ++ assert.deepEqual(JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')).mainBotIds, ['chief-a']) ++ // A swap within Bob's own team still works. ++ await call('set-main', { id: 'chief-b', main: true }) ++ const swapped = await call('set-main', { id: 'helper-b', main: true, replace: 'chief-b' }) ++ assert.equal(swapped.ok, true) ++ assert.deepEqual(swapped.value.mainBotIds, ['chief-a', 'helper-b']) ++}) diff --git a/patches/ds-bot/files/src/host/authz.js b/patches/ds-bot/files/src/host/authz.js index 298b55ffa..e75c860f1 100644 --- a/patches/ds-bot/files/src/host/authz.js +++ b/patches/ds-bot/files/src/host/authz.js @@ -257,7 +257,16 @@ export function install(rt) { return requireSession(body?.sessionId) } if (ep === 'usage') return body?.botId === undefined ? ALLOW : requireBot(body.botId) - if (ep === 'set-main') return requireBot(body?.id) + if (ep === 'set-main') { + // `replace` names the Main Bot whose place the target takes, and the Main list is + // deployment-wide: checking only `body.id` let a member swap another owner's Main Bot + // for their own and demote it (#613). + if (typeof body?.replace === 'string' && body.replace !== '' && body.replace !== body?.id) { + const replaceGate = requireBot(body.replace) + if (!replaceGate.ok) return replaceGate + } + return requireBot(body?.id) + } if (ep === 'set-flags') { const id = body?.id ?? body?.botId if (botOwnedBy(actor, id) || roomOwnedBy(actor, id) || roomMemberOf(actor, id)) return ALLOW diff --git a/patches/ds-bot/files/test/authz.test.js b/patches/ds-bot/files/test/authz.test.js index 8af99073a..f67b1130d 100644 --- a/patches/ds-bot/files/test/authz.test.js +++ b/patches/ds-bot/files/test/authz.test.js @@ -433,3 +433,28 @@ test('a group\'s question card is its Bot owner\'s to see and answer, not every assert.deepEqual(Object.keys(rt.ownedView(full, actorA).questions), ['rx']) assert.deepEqual(Object.keys(rt.ownedView(full, actorB).questions), []) }) + +// #613: the Main Bot list is deployment-wide, so `replace` crosses the owner boundary unless the +// actor owns both the Bot it promotes and the Main Bot whose place it takes. +test('#613: a member cannot take another owner’s Main Bot’s place', async (t) => { + const { rt } = runtime(t, { ...TEAM, mainBotIds: ['m1'] }) + await rt.load() + const sessionA = await redeemOwner(rt) + const sessionB = await redeemOwner(rt) + assert.equal(sessionB.memberId, 'owner-2') + rt.state.bots.a1 = { id: 'a1', sessionId: 'a1', name: 'A', instructions: '', createdAt: 2, ownerId: 'owner-1' } + rt.state.bots.b1 = { id: 'b1', sessionId: 'b1', name: 'B', instructions: '', createdAt: 3, ownerId: 'owner-2' } + const actorA = await rt.authenticate(sessionA.sessionToken) + const actorB = await rt.authenticate(sessionB.sessionToken) + + // Bob's own Bot may not take Alice's Main Bot's place: promoting it that way demotes hers. + const escalation = rt.authorize(actorB, 'set-main', { id: 'b1', main: true, replace: 'm1' }) + assert.equal(escalation.ok, false) + assert.equal(escalation.code, 'bot/forbidden') + // Demoting hers by naming it as the target must not work either. + assert.equal(rt.authorize(actorB, 'set-main', { id: 'm1', main: false }).ok, false) + // A swap inside one owner's team stays allowed. + assert.equal(rt.authorize(actorA, 'set-main', { id: 'a1', main: true, replace: 'm1' }).ok, true) + // And Bob's own Bot keeps the plain promotion the deployment-wide list gives it. + assert.equal(rt.authorize(actorB, 'set-main', { id: 'b1', main: true }).ok, true) +})