Repository navigation
Fix owner-scoped access for Agent endpoints and delete answers - #612
Conversation
Two defects in the ThinkFlowLab#49 access foundation, both reachable as soon as someone has signed in: - The Agent-session endpoints read body.botId/body.ownerBotId, but the browser sends agentId, so agent-session / agent-adopt / agent-archive / agent-rename / remove-agent were denied with bot/forbidden before they ran. The DSH Agent is one per deployment and carries no owner, like add-agent, so those endpoints now follow the same shared-resource rule: free to a signed-in member while alone, blocked with two members. - delete-bot, set-main, remove-agent and delete-room answered with the unfiltered view() (no actor), leaking every owner's Bots, rooms and secret names to a signed-in member; they now pass the actor like the state and update-room answers.
Birfy
added
bug
|
Triage (2026-10-21): |
Birfy
removed
agent-pr
|
Labels added (type + area only): Ready for review — requested reviewer: Reviewer. Review it as a security-surface change:
|
|
Triage (2026-10-21):
In the Reviewer queue per the Chief, with the review strength a security-surface change needs: the PR must show a minimal reproduction that turns red without the source change. Merge note: external contributor + permissions surface — the Chief's go-ahead applies before it is enqueued. |
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Verdict: PASS at 31342ec — tier T3 (controller's paths rule: core/src/host/authz.js, core/src/host/channel.js)
结论：PASS，按外部 PR 规矩等用户确认，未经确认不排队。 这是安全面（权限判定）改动，已同步报 Chief 呈用户。
Tip SHA: 31342ec06c6cc85e3165047cad2843a3b3b88f9b
CI at tip (run 39587837221): all 8 checks success
Tier: T3 — computed by the repository's own risk-gate judge (core/src/host/* is a T3 path)
Reviewer environment: now on danger-full-access; the suites below ran locally at this tip.
Reproduced both directions locally instead of trusting the body:
$ cd core && TZ=UTC DSH_HARNESS=… node --import ./test/harness-resolve.mjs \
--test test/authz.test.js test/room-membership.test.js
# tests 32 / pass 32 / fail 0
$ # restore main's src/host/authz.js and src/host/channel.js, keep the new tests
not ok 4 - the one deployment Agent stays usable to a signed-in member while alone, and blocks with two
not ok 31 - channel: delete-bot answers with the actor's scoped view, not the whole team
# tests 32 / pass 30 / fail 2
Exactly the two new tests go red. That matches the two defects in the body.
Security review of the change itself:
- The five Agent endpoints are genuinely ownerless.
channel.jsat this tip has exactly six Agent endpoints —add-agent,agent-session,agent-adopt,agent-archive,agent-rename,remove-agent— and all six are now inSHARED_BLOCKED. Moving them there is the same ruleadd-agentalready had, so it grants no new authority: a signed-in sole member gets what an enrolment-free host had, and with two or more members every one of them fails closed withbot/blocked-shared. Any futureagent-*endpoint not in the set falls through to the default deny, which is the safe direction. - The four leaked answers are fixed, with none left behind. I grepped
channel.jsat this tip: there is noreturn ok(view())left — the only fourview(returns areview(actor)(delete-bot,set-main,remove-agent,delete-room). So the "a signed-in member received every other owner's Bots, rooms and secret names" hole is closed in the file that produced it. - The new
room-membershiptest is a real scoping assertion, not a smoke test: Bob deletes his own Bot and the answer must not contain Alice's private Bot.
One consequence worth putting in front of the owner before merging, not a defect: with more than one member, the deployment's Agent and its Sessions become unmanageable by anyone (bot/blocked-shared for all six endpoints). That is the existing add-agent rule applied consistently, but it means a multi-member team cannot rename, archive or remove Agent sessions at all. If that is not wanted, it is a separate issue, not a change to this PR.
Non-blocking: the body's Ran at the tip: npm run verify (green) is a claim I could not re-run end to end here (the macOS-only smoke step needs setsid), but the two suites that carry this change are green at the tip both in CI and locally.
Review — PASSTip SHA: 31342ec Both defects are real and both fixes are the right shape:
Two notes, neither blocking:
Before enqueueing, add the missing |
Birfy
added
agent-pr
|
Merge-queue note: the controller did not write a Enqueuing at tip |
risk:T*, but risk-tier.yml is report-only and never writes labels
#609
…#670) Fixes ThinkFlowLab#609 Tip SHA: 4c8d98c ## Reproduce The rule text says `risk:T*` is set by the controller from the diff, never by hand, and the mechanism does not hold up: `github-actions[bot]` labelled ThinkFlowLab#574 at 07:28:43Z, while ThinkFlowLab#606, ThinkFlowLab#610, ThinkFlowLab#611, ThinkFlowLab#612, ThinkFlowLab#616, ThinkFlowLab#617, ThinkFlowLab#618 and ThinkFlowLab#619 carry no `risk:T*` at all even though `risk tier (report only)` succeeded on every one of them. The owner decision on the issue is ②: keep the workflow read-only, never label — and make the computed tier auditable instead of remembered. At this tip, `node .github/scripts/risk-gate.mjs --pr <n>` ends its report with risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3 so a review can quote the tier the rules produced instead of restating a number. ## What changed - `.github/scripts/risk-gate.mjs`: `judge()` records every changed path (and a rename's old path) with the tier `tierOf()` gave it, and `summaryLine()` renders the final line of the report from that same result — highest tier first, paths capped at 10 with a `+N more` tail. `render()` ends the job summary with it, after the prose, so the two can never disagree. - `.github/scripts/risk-gate.test.mjs`: four cases — the line's exact shape for a T3 PR, the ordering and the cap, the no-changed-files case, and "the line is the last line of the report". - `AGENTS.md` gains a bullet under the label section: quote the summary line, the workflow never writes a label, the controller copies it on its poll, no review transcribes a tier. - `.github/labels.json`: every `risk:T*` description keeps `advisory` and `the merge gate recomputes it` and now also names the controller as the writer (all ≤ 100 characters). - `docs/controller.md`: the tier stays auditable without the label — quote the summary line. - `.github/workflows/risk-tier.yml`: header comment only — no permission added, no step changed. It keeps `contents: read` and `pull-requests: read` and still never labels, comments or merges. ## What I ran - `TZ=UTC node --test .github/scripts/*.test.mjs` at this tip (with `git` on `PATH`) → **144/144**. - `node --test .github/scripts/risk-gate.test.mjs` → **18/18**, including the four new cases. - `node .github/scripts/risk-gate.mjs` behaviour checked by hand through the harness result: `judge([AGENTS.md, plugins/social/src/social.js])` → `risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3`; no files → `risk-tier: T0; basis: none (no changed files)`. - `changelog.mjs check` not run: a change users cannot see needs no fragment (changelog.d/README). ## Red without the source change - Removing the `basis.push([name, t])` line from `judge()` (the tests untouched) → `risk-gate.test.mjs` **16/18, 2 red**, exactly `summaryLine: one machine-readable line…` and `summaryLine: highest tier first…`. - Removing `summaryLine(result)` from the end of `render()` instead → **17/18, 1 red**, exactly `render: the machine-readable line is the last line of the report`. Restored → 18/18. ## Not run - The workflow itself (it needs Actions): the change to `risk-tier.yml` is a comment, and the existing `the workflow stays report only` test still passes with it. - `npm run verify` end to end: no `core/node_modules` in this sandbox (the same on `main`). - The `risk-tier` job on this PR runs the base copy of the script, so its summary will only show the new line after merge — that is the workflow's normal base-copy behaviour, not a gap here. ## Risk T3 by the repository's own rules (`.github/**`, `AGENTS.md`): a reporting script plus text. It adds no permission, no network call, no label, comment or merge, and cannot block anything — the job is still `continue-on-error: true` and not a required check. The only behaviour it changes is the last line of that job's summary. The one thing a reader must not conclude is that the label now arrives on time: it does not, and the docs keep saying why (ThinkFlowLab#629).
…kFlowLab#611 merge dropped from core-host.md (ThinkFlowLab#671) (ThinkFlowLab#672) Fixes ThinkFlowLab#671 — restores the two paragraphs that the `ThinkFlowLab#611` merge silently dropped from `docs/subsystems/core-host.md`. Documentation only: this touches no code, no test, no changelog entry, and does not ride along with `ThinkFlowLab#664`. ## Reproduce Measure the file itself (`GET /contents/docs/subsystems/core-host.md?ref=<ref>`), counting `stripMarkers`, `deployment-wide`, `hiddenByGone`, `remoteChats.sync`: | ref | `stripMarkers` | `deployment-wide` | `hiddenByGone` | bytes | | --- | --- | --- | --- | --- | | `4f6e3e54` (before ThinkFlowLab#611) | 1 | 3 | 0 | 75046 | | `16dc44b2` (ThinkFlowLab#611 merge) | 0 | 1 | 1 | 74379 | | `8005fc9d` (main, base of this PR) | 0 | 1 | 1 | 74379 | `main`'s copy is byte-identical to `16dc44b2`'s, so the loss happened in that one merge and nothing after it touched the file. `ThinkFlowLab#611`'s branch carried an older copy of the whole file; the three-way merge (`git merge-file -p head base main`) exits 0 with no conflict, so GitHub had nothing to warn about and the drop was invisible in review. The number falls because the older copy simply does not contain the `ThinkFlowLab#574` sentence or the `ThinkFlowLab#613` paragraph. ## What changed - `docs/subsystems/core-host.md`, **+2 / −1**, exactly two places, both copied verbatim from `4f6e3e54`: - the `ThinkFlowLab#613` bullet *"The Main Bot list is deployment-wide (ThinkFlowLab#613): `set-main` requires the actor to own `body.id` … and the deployment-wide list itself are unchanged."*, restored as its own bullet under `### Team state and access` right after the merged overview bullet (which keeps main's newer `ThinkFlowLab#612` / `ThinkFlowLab#620`-era wording untouched); - the tail of the `ThinkFlowLab#574` sentence in `### Untrusted fences in every shared room`: `stripUntrusted`'s whitespace-inside-the-tag pattern, `stripMarkers` for one tag, and `core/test/fence-property.test.js` as the fuzz guard. - Everything already on main stays, including `ThinkFlowLab#611`'s own `remoteChats.sync` / `hiddenByGone` paragraph (count still 1) and every later edit. - No other file is in the diff: `git diff --shortstat origin/main...HEAD` → `1 file changed, 2 insertions(+), 1 deletion(-)`. ## What I ran Tip SHA: `5c610391c81e2464a010893020045fcef7f874e5` (Node 22.23.3, `TZ=UTC`, base `8005fc9d`): - Acceptance check on the file itself — `stripMarkers` 1, `deployment-wide` 3, `hiddenByGone` 1, `remoteChats.sync` 1 → **PASS** (script saved as a run-only check, not committed; the numbers are the issue's acceptance line). - `node core/scripts/check-arch-docs.mjs` → `arch-docs: OK (1 changed file(s))`. - `node core/scripts/lint.mjs` (`DSH_HARNESS` oxlint) → **0 errors**, 43 pre-existing warnings, `arch-docs: OK (1 changed file(s))`. - `node scripts/ds-bot-patches.mjs check` → pin `ds-bot/` = `Fei2411/DeepSeek-Bot@2b10c70` (tree `5834c00`, v0.1.5), 10 patches apply cleanly, `core/` = `ds-bot/` + patches. - `node core/scripts/build-client.mjs --check` → exit 0. - `git diff origin/main...HEAD -- docs/subsystems/core-host.md` read line by line: one added bullet, one replaced line, nothing else. ## Red without the source change With `docs/subsystems/core-host.md` back at `origin/main` (`8005fc9d`) and the same acceptance check: - `{"stripMarkers":0,"deployment-wide":1,"hiddenByGone":1,"remoteChats.sync":1}` → **FAIL** (`exit 1`), asking for 1 / 3 / 1 / 1. That is the reported regression, reproduced from the file alone. ## Not run - The full test suites, the three plugin suites and the smoke bench: the diff is one documentation file, no code path reads `core-host.md`, and this box has no `pnpm` (`dev/bench.sh: 62: pnpm: not found`). Green `tests-ok` at the tip is the gate; if a full local run is wanted, it can be added at the next tip. - No `changelog.d/` entry: nothing user-visible changes, and the entry would describe a documentation restore. ## Risk - **T0 by diff** (one file under `docs/`); worth noting the content is a record of host behavior, not executable. Nothing touches authz, secrets, saved state or the client bundle. - Out of scope, flagged for Triage: the same `ThinkFlowLab#611` merge also flattened one more `4f6e3e54` sentence — the `ThinkFlowLab#608` standing-approval clause (*"or for a turn that carries no verdict at all in the Bot's own chat — … (ThinkFlowLab#608)"*, plus the `the standing approval for <kind> was not used` log sentence). It is a third loss of the same accident, not part of this issue's acceptance line, so I did not touch it here; the issue says not to mix documentation restores. It needs its own issue if the owner wants it back. - The `ThinkFlowLab#613` bullet is restored verbatim, so it describes `set-main` / `roster.setMain` as of `4f6e3e54`; a later behavior change would need a separate edit rather than a rewrite here.
Fix owner-scoped access for Agent endpoints and delete answers
Two defects in the owner-scoped access foundation (#49), both live as soon as any member has signed in.
First, the Agent endpoints were unusable.
authz.jsdecidedagent-session/agent-adopt/agent-archive/agent-rename/remove-agentfrombody.botId ?? body.ownerBotId, but the browser sendsagentId(and a workspace/session/title), never a Bot id, so every one of them was denied withbot/forbiddenbefore it ran. The DSH Agent is one per deployment and carries no owner, exactly likeadd-agent, so those endpoints now follow the same shared-resource rule: reachable by a signed-in member while the team is alone,bot/blocked-sharedwith more than one member.Second, four answers leaked the whole team.
delete-bot,set-main,remove-agentanddelete-roomreturnedok(view())with no actor, andownedViewreturns the unfiltered view when it gets no actor even while enforcement is on, so a signed-in member received every other owner's Bots, rooms and secret names. They now pass the actor, like thestateandupdate-roomanswers already did.core/test/authz.test.jsasserts the five Agent endpoints are allowed for a signed-in sole member andbot/blocked-sharedwith two (fails without the authz change:bot/forbidden).core/test/room-membership.test.jshas Bob delete his own Bot and asserts the answer keeps Alice's private Bot out (fails without the channel change).docs/subsystems/core-host.mdrecords both.Ran at the tip:
npm run verify(green,TZ=UTC). Not run: smoke (this macOS box has nosetsid, sodev/bench.shcannot startdsh web), Desktop, real-model steward.