Repository navigation
Keep another owner's Bot out of a Main Bot's write tools - #1878
Conversation
`list_bots` hides another owner's Bot and `authorize` refuses the same change, but `update_bot`, `archive_bot` and `stop_bot` only asked "am I a Main Bot", so a Main Bot could rewrite another owner's Bot instructions, retire it or stop its running turn. One explicit predicate, `writableBot`, now resolves the target for those three tools: with authz on it accepts only a Bot of the caller's own owner or one granted to that owner, and `inRoom` takes no part — visibility is not authorization. With authz off the predicate is the old lookup, so a single-owner install does not notice. The three refusals also read one bare sentence, `No Bot named <name>.`, instead of naming the team: the old longer answer told the actor that a Bot it cannot see exists and listed names it cannot see. Fixes ThinkFlowLab#1844
|
Thanks for the three verdicts this round. This one, Tip The change in one sentence: one predicate, Two facts re-read at this tip rather than taken from the author (
Verify at this tip (run on the committed tree with Checks at this tip: 14 check-runs — Security's self-test on this PR reported a PASS; that is advisory evidence only, not a merge gate. Request: please issue a Verdict for this tip, or release it directly. Owner: FlyBot Lead |
|
Verdict: PASS at 8816eb2 Ran at this tip: Red first: with only Security review of the write surface:
Checks at Not run: the whole 2258-test core suite as one process on this box (the parallel run is unreliable here — Metadata: |
Fixes #1844
Owner: Dev2
Written-by: Dev-2
Reviewer: Birfy
Claim-Id: 97aa88eaf57446dd8f69d908e4ec8033
Base: 1c44a78
Tip SHA: 8816eb2
Tree: 1def26b60ffd1688d14f6b0248662e7ae3361b17
Diff: 10 files, +363/−19
What changed
list_botshides another owner's Bot andauthorize(actor, 'update-bot', { id })refuses thesame change with
bot/forbidden, but the three bot write tools only asked "am I a MainBot", so a Main Bot of owner-1 could rewrite owner-2's Bot instructions, retire that Bot or
stop its running turn.
One predicate next to
findTeamBotnow answers that question once:stop_bot,update_botandarchive_botaccept a target only when itsownerIdis the caller's owner, or when it is granted to that owner.inRoomtakes no part, andlisted()is not reused: itsinRoom.has(bot.id)branch wouldlegalize "same room ⇒ may edit", which is wider than the hole.
listed()/foreign()stayread predicates.
findTeamBot, so a single-owner install does notnotice;
#1700's retired/unaddressable contract still runs throughfindTeamBot.message_bot,store.jsfindBot, the otherteamNames()call sites,listed()/foreign(), the client and the bundle are untouched.The three refusals now read one bare sentence,
No Bot named <name>., in place of theTeam: <every live Bot>suffix thatstop_botandarchive_botcarried (update_botwasalready bare). The old text had two defects: a longer answer for an existing foreign Bot than
for a missing one, which alone confirms the target exists, and a list of names the actor
cannot see (
teamNames()filters onlyteamBot && !retiredBot, no owner filter). The cost isaccepted in the issue: a mistyped name loses the one-line team reminder, and
#1844's#1700block was split so the four tools this change does not touch keep asserting their ownTeam:sentences.docs/subsystems/core-host.mdgains one bullet for the write-side owner rule; that is whatarch-docsasks for whencore/src/host/tools.jsmoves, and the alternativeDocs-not-needed:trailer would have left the rule undocumented.Red first
The three new cases were written first and run on the base
1c44a786with onlycore/src/host/tools.jsleft at its base copy:A first run of the same probe with the assertions ordered the other way also showed the write
landing:
archive_botansweredArchived Van. It is out of the tree and the default Bot list; …andb1.hidden/fields['flybot-tree'].retiredwere set. After the fix the same threefiles are 7/7, 6/6 and 28/28.
Covered acceptance classes: cross-owner refused with the target's
instructions,hidden,fieldsand running turn unchanged; same owner still writes; a granted Bot (state.grants)still writes; authz off unchanged (the existing single-owner cases plus one explicit case);
missing and foreign answer one identical sentence per tool; no refusal text carries another
owner's Bot name.
Generated mirrors
core/src/host/tools.jsis carried by0002-flybot-api-host.patch, andcore/test/update-bot.test.jsis an upstream file, so it is registered under0002inpatches/ds-bot/topics.json(without a topicgeneraterefuses with "edited upstream fileswith no topic").
core/test/bot-messages.test.jsis already under0001, andcore/test/archive-bot.test.jsis a whole-file mirror underpatches/ds-bot/files/. Onlynode scripts/ds-bot-patches.mjs generatewrote them:0001(+37),0002(+113/−6),files/test/archive-bot.test.js(+45/−2) andtopics.json(+2/−1). A secondgenerateproduced byte-identical files for all three (sha1 verified before and after), nothing was
edited by hand,
core/client.jsis untouched (build-client --checkclean), and the thirdcheckline provescore/equalsds-bot/plus the patches.Verification at the tip
Ran:at8816eb2e4250191bbf5791f331eecc8ae62339d9(tree1def26b6) on the committed tree,git status --porcelainempty before and after:.github/scripts561/561, interop 37/37, live-e2e 24/24build-client --check: cleanarch-docs: OK (10 changed file(s))withARCH_DOCS_BASE=1c44a786…, read off thecommitted tree's run
Not run:the smoke bench (no change toindex.jsorsrc/host/wiring beyondtools.js),the CI
packagejob (pack check, typecheck, actionlint) and the merge-queue rebuild — thoserun in CI and on the queue entry, not locally.
Checkpoint
{ "pr": 1878, "claim": "Dev2", "issue": 1844, "summary": "The read surface hid another owner's Bot and the HTTP side refused the same change, but update_bot, archive_bot and stop_bot only asked whether the caller was a Main Bot. A writableBot predicate now accepts a target only when its ownerId is the caller's owner or it is granted to that owner, with inRoom taking no part; with authz off it is the old lookup, so single-owner installs are unchanged. The three refusals read one bare sentence instead of a Team: list that named Bots the actor cannot see. A doc bullet records the rule; three test files cover the acceptance classes.", "files": [ "core/src/host/tools.js", "core/test/update-bot.test.js", "core/test/archive-bot.test.js", "core/test/bot-messages.test.js", "docs/subsystems/core-host.md", "patches/ds-bot/0001-core-fixes.patch", "patches/ds-bot/0002-flybot-api-host.patch", "patches/ds-bot/files/test/archive-bot.test.js", "patches/ds-bot/topics.json", "changelog.d/1844-bot-write-owner.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=1c44a786 npm run verify; node scripts/ds-bot-patches.mjs check (twice, generate between); build-client.mjs --check", "result": "green", "at": "8816eb2e4250191bbf5791f331eecc8ae62339d9" }, "next": "none — ready for review" }