Skip to content

set-main lets a member demote another owner's Main Bot and promote their own (owner check covers only body.id) #613

Closed

Description

@Birfy

Symptom

A signed-in member can change the deployment-wide Main Bot list through set-main, which crosses the owner boundary this endpoint is supposed to enforce:

  • demote another owner's Main Bot: set-main { id: <my Bot>, replace: <their Main Bot> } — authz.js only checks body.id;
  • promote their own Bot to Main (it appends to state.mainBotIds), and rt.isMain then grants that Bot the Main-only tools.

Found while reviewing #612 (owner-scoped access for the Agent endpoints and delete answers). Not introduced by #612 and not part of its scope — filed separately, as the reviewer, no code changed.

Evidence (main 7df78baef, checked on the #612 tree which is identical here)

  • core/src/host/authz.js:260 — if (ep === 'set-main') return requireBot(body?.id): only the target Bot is ownership-checked.
  • core/src/host/channel.js:257-262 — body.replace is forwarded with no check.
  • core/src/host/roster.js:491-511 — setMain edits state.mainBotIds (one list for the whole deployment) and, when replace is given, replaces that entry with id. replace must be a current Main Bot (:501-502), which is exactly the other owner's Bot.
  • core/src/host/tools.js — rt.isMain(self) alone gates stop_bot (:236), create_child_bot/create_bot (:266), changing another Bot (:299), creating (:365) and deleting (:449) group chats. So a Bot promoted by its owner's human passes those checks team-wide.

Reproduction

Two members (the fixture TEAM in core/test/room-membership.test.js already builds Alice/Bob with chief-a / spec-a and chief-b / spec-b):

  1. Bob signs in (session token), owns chief-b; state.mainBotIds is ['chief-a'].
  2. Bob calls set-main { id: 'chief-b', main: true } → succeeds (requireBot('chief-b')), state.mainBotIds becomes ['chief-a','chief-b'] — Bob's Bot is now a Main Bot, and Bob's Bot is not shared with Alice.
  3. Bob calls set-main { id: 'chief-b', replace: 'chief-a' } → succeeds, state.mainBotIds becomes ['chief-b']: Alice's Main Bot is demoted, her Bot loses the Main-only tools, and deleteBot for her Bot is no longer refused as a Main Bot.

Both steps pass today. Neither is what #49 promised ("a different member or their bot cannot access them by changing IDs").

Expected

Pick one (product decision — this is the permissions surface):

  1. Owner-scoped replace: in authz.js, for set-main with body.replace, require that the actor also owns replace (requireBot(body.replace)), and in roster.js refuse a replace whose Bot has another owner. Smallest change; also fixes the demote case exactly.
  2. Per-owner Main list: state.mainBotIds per member (schema change, store.js migration; the rule the file already uses for designatedChiefId). Correct long term, much larger.
  3. State the rule: a member's Main Bots are deployment-wide and an ownerless list by design — then set-main must be blocked-shared with more than one member, like add-agent, until (2).

Recommendation: (1) now, (2) when team mode leaves the disabled state.

Tests to add

  • authz.test.js: with two members, set-main { id: ownBot, replace: otherOwnerBot } is bot/forbidden; set-main { id: ownBot, main: true } while another owner has a Main is refused (or becomes bot/blocked-shared, per the chosen option).
  • roster.test.js: setMain with a replace Bot of a different owner throws and leaves state.mainBotIds unchanged.

Notes

  • #612 PASSed without this; keep it out of that PR so the two changes stay reviewable.
  • Related: #49 (owner-scoped access, closed), #50 (rooms), #51 (cross-owner routes). This is the roster-administration half of #49 that the endpoint table missed.

Activity

  1. added
    bugSomething isn't working
    needs-humanThe agent loop stopped; a person must decide (reason in the last comment)
    area:securityAuth, grants, approvals, secrets
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    and removed
    needs-humanThe agent loop stopped; a person must decide (reason in the last comment)
    on Oct 21, 2026
  2. Birfy commented on Oct 21, 2026

    @Birfy
    CollaboratorAuthor

    Triage (2026-10-21): needs-human → agent-ready. Kept bug + area:security, and it now heads the Dev queue: #613 → #609 → #608.

    Why it moves ahead of #609/#608: set-main validates only body.id but forwards body.replace, so a member can demote another owner's Main Bot and promote their own — and rt.isMain alone gates stop_bot, create_bot/create_child_bot, changing another Bot, and creating/deleting group chats. That is privilege escalation on the permissions surface, not a cosmetic gap.

    The product decision the issue left open is settled: option (1) — the narrowest fix.

    • object-level ownership check on body.id and on body.replace (requireBot(body.replace)), plus a refusal in setMain when the replace Bot belongs to another owner;
    • two tests that must go red without the change: (a) a member promoting their own Bot while another owner holds a Main Bot fails, (b) demoting another owner's Main Bot via replace fails and leaves state.mainBotIds unchanged.

    Out of scope: option (2), the per-owner mainBotIds list with a store.js migration — that is a later change once team mode leaves the disabled state, and it is not needed to close the escalation. Keep this out of #612: #612 is PASSed on its own, and the two changes should stay reviewable.

    Gate as usual: tip PASS + CI all green + reverting the source change turns the new tests red. risk:T* is controller-only — none written by hand or transcribed; if absent, the actual tier is in the risk tier (report only) job summary and it does not block.

  3. Birfy commented on Oct 21, 2026

    @Birfy
    CollaboratorAuthor

    Claiming. Branch fix/613-set-main-owner, forked from main 81c708d.

    Fix, narrowest of the three options in the issue (option 1):

    • core/src/host/authz.js — set-main now also requires the actor to own body.replace when it names a different Bot, so taking another owner's Main Bot's place is bot/forbidden before it reaches the roster.
    • core/src/host/roster.js — setMain refuses a replace whose Bot has another owner and leaves state.mainBotIds unchanged, so the invariant holds for every caller, not only the endpoint.

    Order: the failing tests first, then the fix. Evidence goes in the PR.

    Not changing the deployment-wide shape of state.mainBotIds (option 2) and not making set-main blocked-shared (option 3): both are the team-mode decision the issue leaves for later, and the plain promotion of a Bot the caller owns stays as it is.

  4. added
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 21, 2026
  5. self-assigned this
    on Oct 21, 2026
  6. removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 21, 2026
  7. removed
    agent-workingAn agent has claimed this issue (the claim comment names the branch)
    on Oct 22, 2026
  8. added 2 commits that reference this issue on Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:securityAuth, grants, approvals, secretsbugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions