fix(channels): gate /sh and privileged tools behind a per-account operators list - #1170
fix(channels): gate /sh and privileged tools behind a per-account operators list#1170penso wants to merge 19 commits into
Conversation
…rators list /sh was reachable by any sender who passed a channel's access gate. On a private instance that is fine; on a Discord guild or any group chat it is arbitrary host command execution, because every member who clears the group policy clears the gate. Two paths existed. On Telegram/Matrix/Signal/Nostr, `is_channel_command()` deliberately does not claim `/sh <cmd>` — only the bare toggles — so `/sh ls -la` fell through to the chat path and the agent runner force-executed it. On Discord/Slack, bare `/sh` enabled command mode, after which `rewrite_for_shell_mode` turned every later message in that chat into `/sh <text>`; command mode is keyed per channel session, so one sender enabling it handed the shell to everyone else in the room. `handle_sh` had no authorization check at all, unlike /approve, /deny and /update. Blocking /sh alone would not close it: exec and the other host-reaching tools stayed in the agent's registry, so a guest could simply ask the agent in prose to run a command or read the owner's credentials, memory, and other sessions. Add `operators`, a per-account list separate from `allowlist`. Access gating answers "may this peer talk to the bot"; operators answers "may this sender run privileged actions". Resolution is fail-closed: `operators` when non-empty, else the DM allowlist (compatibility for single-owner instances), else nobody. An empty operators list matches nobody — unlike an allowlist, where empty means open — and an unidentified sender is never an operator. Enforce at three points so no route reaches the shell: `handle_sh` (including the read-only `status` form, which would otherwise leak whether the shell is live), the per-message command-mode rewrite, and a `/sh <cmd>` block in `dispatch_to_chat` that uses the same predicate the runner executes on. Guests additionally run with a tool policy denying host reach, the owner's private state, `update_channel_settings` (which edits the operator list itself), and persistence/lateral-movement tools; applied on both the text and attachment dispatch paths. /approve, /deny and /update now use the same operator check. Known gap, documented in docs/src/channels.md: OTP self-approval appends to the DM allowlist, so on an account with no explicit `operators` the compatibility fallback promotes OTP-approved senders. Not a regression — they already passed the old allowlist check — but it needs its own tracking of OTP-approved senders.
The file was never declared in any `mod` statement, so it has never compiled into the binary. It is a stale duplicate of `channel_events.rs`: every item in it also exists there, and the only content unique to it was the pre-operators `is_sender_on_allowlist`, which the previous commit replaced with `resolve_sender_role`/`is_sender_authorized`. A dead copy of authorization code that looks live is a trap — an editor hardening channel access could plausibly change it and see no effect. Remove it so `channel_events.rs` is unambiguously the only implementation.
Greptile SummaryThis PR separates channel access from operator privilege and consistently propagates the resulting security context.
Confidence Score: 3/5The PR is not yet safe to merge because an immediate queued replay failure can still leave later sender groups pending indefinitely. The privilege propagation and collect-mode isolation fixes now preserve guest restrictions across asynchronous execution and grouped replay, but queue draining still requeues later groups before replay and does not schedule another drain when that replay fails. Files Needing Attention: crates/chat/src/service/chat_impl/queue_drain.rs
|
| Filename | Overview |
|---|---|
| crates/chat/src/service/chat_impl/tool_policy.rs | Centralizes request-level audience and tool-policy filtering and defines security-context boundaries for queued turns. |
| crates/chat/src/service/chat_impl/send.rs | Applies the resolved registry to asynchronous agent and explicit-shell execution and preserves restrictions when queuing channel requests. |
| crates/chat/src/service/chat_impl/queue_drain.rs | Splits collected messages across authorization contexts, but the previously reported failed-replay drain issue remains outstanding. |
| crates/gateway/src/channel_events/dispatch.rs | Resolves operator/direct-chat trust and stamps guest turns with fail-closed tool and context restrictions. |
| crates/agents/src/tool_registry.rs | Introduces host-owned tool audiences and filtering that prevents public turns from recovering trusted tools. |
| crates/channels/src/operators.rs | Adds exact, case-sensitive, fail-closed per-account operator identity resolution. |
| crates/web/ui/src/pages/channels/modals/EditChannelModal.tsx | Adds configuration controls for managing each channel account's operator list. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Inbound channel message] --> B{Sender listed as operator?}
B -->|No| C[Guest security context]
B -->|Yes| D{Proven direct chat?}
D -->|No| C
D -->|Yes| E[Operator security context]
C --> F[Public tool audience and no private context]
E --> G[Trusted tool audience and privileged commands]
F --> H[Queue grouped by sender security context]
G --> H
H --> I[Replay through chat.send policy resolution]
Reviews (10): Last reviewed commit: "fix(scripts): only treat crate-root test..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The guest tool restriction added in this branch was a no-op. Channels call `chat.send()`, which routes to `send_impl` in `chat_impl/send.rs` — and that path never read `_tool_policy`, starting every run from the shared registry. Only `send_sync` honoured the parameter, so the restriction worked for webhooks (which use send_sync) and silently did nothing on the untrusted channel path it was written for. A non-operator could still reach `exec`, `write_file`, `memory_*`, `sessions_*`, and MCP tools by asking the agent in prose; only the literal `/sh` prefix was blocked. Reported by Greptile on #1170. Extract the parsing and registry filtering into `chat_impl/tool_policy.rs` and call it from both send paths, so there is one implementation instead of two that could drift again. `send_impl` resolves it once up front and hands the filtered registry to both runs it can start: the agent loop and the explicit `/sh` fast path. That fast path resolves `exec` from the registry it is given and errors when absent, so it now fails closed for guests too. Behaviour for trusted callers is unchanged: with no `_tool_policy` the shared registry is passed through by pointer, with no clone and no filtering. Cover it with a structural regression test asserting neither send path hands a run the unfiltered `self.tool_registry`. A behavioural test would need a live provider and database; the guard was verified by reintroducing the original bug and confirming it fails.
The guest tool restriction added in this branch was a no-op. Channels call `chat.send()`, which routes to `send_impl` in `chat_impl/send.rs` — and that path never read `_tool_policy`, starting every run from the shared registry. Only `send_sync` honoured the parameter, so the restriction worked for webhooks (which use send_sync) and silently did nothing on the untrusted channel path it was written for. A non-operator could still reach `exec`, `write_file`, `memory_*`, `sessions_*`, and MCP tools by asking the agent in prose; only the literal `/sh` prefix was blocked. Reported by Greptile on #1170. Extract the parsing and registry filtering into `chat_impl/tool_policy.rs` and call it from both send paths, so there is one implementation instead of two that could drift again. `send_impl` resolves it once up front and hands the filtered registry to both runs it can start: the agent loop and the explicit `/sh` fast path. That fast path resolves `exec` from the registry it is given and errors when absent, so it now fails closed for guests too. Behaviour for trusted callers is unchanged: with no `_tool_policy` the shared registry is passed through by pointer, with no clone and no filtering. Cover it with a structural regression test asserting neither send path hands a run the unfiltered `self.tool_registry`. A behavioural test would need a live provider and database; the guard was verified by reintroducing the original bug and confirming it fails.
|
@greptile review |
The new spec was committed without running biome, so `biome ci` failed in local validation and blocked every downstream CI waiter job. Formatting only, no behavioural change.
|
@greptile review |
`operators round-trip through the edit modal` asserted an element whose exact text is "owner-id", but AllowlistInput renders each tag as a <span> containing the value plus a "×" remove button, so its text content is "owner-id ×" and no element ever matched. The spec was added without being run, so the failure only surfaced in local validation. Scope the assertion to the Operators field's tag list and match on the tag rather than on text equal to the value alone. Scoping also removes the reliance on `.first()` to skip the identical value in the Allowlist field above.
|
@greptile review |
…t privilege Greptile caught a second escalation path on the same mechanism as the first. `MessageQueueMode::Collect` drains the per-session queue by joining the text of every queued message into one turn, then running it with `last.params` — only the *final* message's parameters. In a shared channel session the queue holds messages from different senders, and a guest's params carry the guest `_tool_policy` while an operator's carry none. A guest message followed by an operator message therefore replayed the guest's text with no restriction at all, handing guest-authored instructions `exec`, the owner's memory, sessions, and every MCP integration — the exact access the operator gate exists to withhold. Fixed by splitting the queue at the first `_tool_policy` change: only messages that share an authorization context are merged, and the remainder is put back on the queue for the next drain to replay under its own policy. This keeps the invariant easy to state — a turn never runs with more privilege than the sender of any line in it — and avoids inventing a policy-intersection algebra that `ToolPolicy`'s glob allow-lists cannot express in general. Applied at both Collect sites in send.rs, which were duplicate copies of the drain loop, via one shared helper in tool_policy.rs. Followup mode was already correct: it replays each message with its own params. Requeueing the remainder also fixes a smaller pre-existing bug — when no queued message had text, the whole batch was dropped after being removed from the map.
|
@greptile review |
Both async send paths ended with a copy-pasted drain loop. That duplication is how the Collect privilege bug reached two places at once, and fixing it in both pushed send.rs past the 1,500-line limit. Moves the loop to chat_impl/queue_drain.rs behind a small ReplaySink trait, so the replay rules — Followup replays one message under its own params, Collect merges only messages sharing a `_tool_policy` — exist once and are unit-testable without a live provider or database. send.rs drops from 1520 to 1399 lines. Behaviour is unchanged apart from the fix already committed, plus the requeue of text-less batches that were previously removed from the map and dropped.
|
@greptile review |
Operator checks inherited the conversational allowlist, shared-room turns relied on a denylist that reopened whenever a tool was added or renamed, and several paths — queue replay, interaction callbacks, history reads, webhooks, channel-bound web sessions, and external agents — could still cross the trust boundary. - Operator identity is exact and fail-closed. Only the explicit `operators` list grants privilege; the DM allowlist is no longer consulted, because OTP self-approval mutates it and access to the bot must not imply access to the host. - Untrusted turns get a positive tool allowlist (`calc`, `web_search`, `web_fetch`) instead of a denylist, so a newly added or renamed tool is unavailable until it is deliberately reviewed. - Every normal turn in a shared room is untrusted, including an operator's, because the room's history contains other people's text. Explicit `/sh` is deterministic and stays separately authorized. - Untrusted turns omit owner-private prompt context and only see history from runs that were themselves public. - Queued channel turns execute under the public ceiling regardless of the sender's role at replay time, because authorization can change in between. Shell commands cannot be queued at all. - Interaction callbacks carry their sender, so a button press is attributed to the principal who pressed it rather than to the session. - `chat.send` and `chat.send_sync` strip `channel`, `_channel_reply_target`, and `_native_channel_request`, so an API caller cannot forge channel routing to escape the ceiling. - External agents are unavailable to public or tool-restricted turns, and explicit shell requests go to the deterministic runner instead. BREAKING CHANGE: privileged channel commands are disabled until `operators` is configured. Previously anyone on a channel's DM `allowlist` could run `/sh`, `/approve`, and `/update`; that fallback is gone, and an empty `operators` list now matches nobody — including the owner. After upgrading, add your exact platform sender ID to `operators` for each account. Entries are exact and case-sensitive: globs, usernames, and partial IDs do not grant privilege.
The targeted local-validation selector preserved carriage returns from CRLF Cargo.toml files. Cargo then received package names such as moltis-whatsapp followed by a newline and rejected the command.\n\nStrip carriage returns while extracting package names so targeted validation works across line-ending styles.
|
@greptile review |
…icate The gateway skips the untrusted tool ceiling for a turn it recognizes as an explicit `/sh`, on the promise that the chat layer runs it through the deterministic shell path rather than the agent loop. The two used different predicates, so they disagreed on three forms: input chat layer gateway/runner /sh@mybot uname -a not shell shell /SH uname -a not shell shell /sh foo\nbar shell not shell The first two are the dangerous direction, and `/sh@bot ...` is the ordinary way to address a bot command in a Telegram or Discord group. An operator typing it in a shared room got `explicit_shell = true` at the gateway — so no `_tool_policy` and no `_private_context: false` — and then fell through to an ordinary agent turn with the *unrestricted* registry and owner-private context, over a history containing guest-authored text. That is exactly the prompt-injection path the shared-room ceiling exists to close, and it also meant the command never ran as a shell command at all. Deletes `parse_explicit_shell_command` and uses `moltis_agents::runner::explicit_shell_command` everywhere, restoring the documented invariant that authorization and execution cannot drift. Adds a regression guard that fails if send.rs stops using the shared predicate, and pins the forms both sides must agree on. Also fixes a garbled doc sentence in `resolve_sender_role`.
Two follow-ups to the operator gate. **Privilege was uniform, not judged.** Defaulting every command to operator-only was the right default but was never revisited per command, so `/new` and `/model` ended up as restricted as `/sh`. They are not comparable: `/sh` runs host commands, while `/new` resets the conversation in the room the sender is already in. The rule is now what the command can reach. Public: `/help`, `/new`, `/clear`, `/compact`, `/title`, `/fork`, `/stop`, `/model`, `/mode`, `/fast` — worst case is disrupting the room's own conversation, which any member can already do by talking. Everything else stays operator-only, because it runs on the host (`/sh`, `/update`), acts for the owner (`/approve` and `/deny` resolve the *owner's* pending exec requests, i.e. code execution by proxy), weakens isolation (`/sandbox` can turn the sandbox off), or reads state from outside this chat (`/attach`, `/sessions`, `/context`, `/insights`, `/peek`, `/btw`). New commands still default to operator-only. The public set is enumerated in both the match and its test, so a command cannot become public by accident. **Refusals were dead ends.** Four call sites had four different messages, none of which said what to do. Since the allowlist fallback is gone, the person most likely to hit one is the owner, locked out of their own bot. One shared message now names the action, points at Settings → Channels → Operators and the `operators` config field, and echoes the sender's own platform ID — entries must be exact IDs, so without it the owner has to go and find theirs. That ID is the sender's own already-public platform identifier. Docs get the split, an upgrade warning, and per-command markers.
63d0313 to
566b683
Compare
Approving a sender and handing them the host were the same click's worth of thought but are not the same decision, and `senders.approve` only ever appended to the allowlist. With the allowlist fallback gone, a fresh install had no path to its first operator except finding your own platform ID by hand and pasting it into a different settings page. `channels.senders.approve` now takes `role`. Anything that is not exactly "operator" means guest, so privilege is never granted by a typo or an older client. Granting requires `peer_id`: the allowlist matches usernames and globs, but operators are matched exactly, so approving on a username would be recorded and then silently never match — privilege that quietly does nothing is worse than an error. The UI asks before approving. Operator is pre-selected only while the account has zero operators: the OTP code is never sent to the requester, it is emitted to this UI, so on a fresh install whoever holds it read it off the owner's own screen. Once one operator exists, guest is the default. The choice spells out that operator means running commands on this machine, and the Operators field says the same. This is trust-on-first-use scoped to the one case where the signal is strong, not a standing rule. Also fixes a defaulting bug found by the new e2e: the modal stays mounted and renders null while closed, so a `useSignal` initializer ran once before any channel was loaded and never again. The default is now chosen when the dialog opens.
Shared rooms still exposed reviewed public tools and allowed operators to invoke /sh, while direct-message access alone was not sufficient proof of an owner. This left a complex exception model around terminal-equivalent channel capabilities. Require both an exact operator identity and a conversation proven direct for tools, private context, external agents, privileged commands, and shell access. Apply a deny-all policy to every other channel turn, fail unknown topology closed, preserve explicit public-tool opt-in for webhooks, and keep delayed queue replay and lazy registries from restoring tools.
Title generation and compaction operate on the current conversation and do not grant tools or host access. Keep them available to any sender who already passed the channel access gate while retaining operator direct-chat checks for host-reaching and cross-session actions.
|
@greptileai review |
|
@greptile review |
| if let Err(e) = sink.replay(replay).await { | ||
| warn!(session = %session_key, error = %e, "failed to replay queued messages"); | ||
| } |
There was a problem hiding this comment.
Failed replay strands queued groups
When collect mode splits queued messages across security contexts and replaying the first group returns an immediate error, the later groups have already been requeued but no active run remains to drain them. Those messages stay pending indefinitely until another successful run happens to trigger a drain, so their senders receive neither responses nor errors.
Knowledge Base Used: Sessions, Compaction, and Memory
Follow-up review of the operators branch surfaced one privilege gap, one
correctness landmine, a missing recovery path, and a lint failure.
Telephony was classified as a proven direct chat, so a phone number in an
account's `operators` list granted full host access — tools, private context,
`/sh` — to anyone able to spoof caller ID, which is the same spoof that clears
`phone.inbound_policy`. A call carries no identifier that can authenticate an
operator, so it is now shared like Discord/Teams/Matrix.
`is_shared_chat` was also denylist-shaped: it named the group forms it knew and
treated everything else as direct. WhatsApp gained group-adjacent address kinds
over time (`status@broadcast`, `@newsletter`), and a blank or unexpected chat id
read as a DM on four platforms. It is now an allowlist of shapes known to be
one-to-one, which is the fail-closed direction for a gate that decides who
reaches the host.
`filter_public_history` keeps runs, not messages, and `PersistedMessage::
tool_result()` hardcodes `run_id: None` — so a public run that persisted a pair
through it would drop the result and leave an orphaned assistant `tool_calls`,
which providers reject outright. Unreachable today (only `/sh` uses that
constructor and `/sh` is never public), but one classification change away. The
`/sh` path now uses `tool_result_with_run_id`, and the filter drops any
assistant message whose tool calls did not survive, so its output is valid by
construction rather than by persistence-site discipline.
Every non-gateway request into a channel-bound session is untrusted, which is
correct but was invisible and irreversible: cron jobs, webhooks, and
`sessions_send` silently answered as if they had no tools, and `/attach` had no
inverse, so a working session moved into a chat was degraded in the web UI
forever. The downgrade is now logged, and `sessions.patch { channelBinding:
null }` releases an attached session (surfaced as "Release channel" in the
session header). A chat's own session still cannot be released — its history is
the room's history, and the next inbound message would re-bind it — and
releasing also clears the chat→session override so the channel cannot keep
delivering into a session that no longer declares a binding.
`ApproveSenderModal`'s `role` prop tripped biome's `useValidAriaRole` rule, two
hard lint errors; renamed to `value`.
Also: the `_native_channel_request` trust marker's strip list now lives in
`moltis-chat` beside the code that reads it, so it cannot drift from the RPC
boundary that strips it; the redundant `!explicit_shell` term in the channel
tool ceiling is gone (it made the ceiling depend on a rejection 60 lines away);
and dead code removed — the unreachable web-`/sh`-on-bound-session block in
send.rs, `/btw`'s local scope check now that dispatch gates it, the no-op
memory-tool re-filter, and `glob_match_lower`'s widened visibility.
The new unbind tests pushed session/tests.rs to 1550 lines, past the 1500-line CI limit. Move them to a sibling test module alongside archive_search_tests, following the same #[path] wiring.
… history The first version of the pairing guard only dropped assistant messages whose results had not survived. That left the inverse orphan: a partially answered assistant message is dropped whole, stranding the result that *did* survive, and a tool result with no preceding call is rejected by providers just as a call with no result is. Filter both directions. This cannot cascade: every surviving call had all of its results in the answered set, so the reverse pass only removes results no surviving call asked for.
build_targeted_rust_test_cmd matched any path containing a tests/ component and asked nextest for `--test <basename>`. A tests/ directory nested inside src/ is an ordinary inline module compiled into the lib, not a separate test binary, so the derived command failed with "no test target named channel_binding_tests" even though every test in the file passed. Only .rs files directly under <crate>/tests/ are integration targets. Nested ones fall through to the name-filter branch, which nextest resolves by substring against the full test path. This was already latent for src/session/tests/tests/archive_search_tests.rs; it only surfaced once a file under that directory was changed on a branch.
|
@greptile review |
Summary
Channel senders who passed an access allowlist could previously reach privileged commands and host tools. This change separates access from privilege with an explicit per-account
operatorslist and enforces that boundary across commands, callbacks, queue replay, chat execution, external agents, webhooks, and persisted context.Security behavior is fail-closed:
operatorslist grants operator privileges to nobody./shuses the deterministic shell runner.calc,web_search, andweb_fetch.The channel settings UI, configuration templates, security documentation, Rust tests, and Playwright coverage are updated with the same model.
Validation
Completed
./scripts/local-validate.sh 1170targeted Rust tests: passedcargo fmt --all -- --check: passedjust lint: passedcargo build --workspace --all-targets: passed./scripts/local-validate.sh 1170: passed./scripts/local-validate.sh 1170: passedcd crates/web/ui && npx playwright test e2e/specs/channels-operators.spec.js: 3 passedcd crates/web/ui && npm run build: passedcd crates/web/ui && npx tsc --noEmit: passedjust release-preflight: passed before merging currentmainRemaining
Manual QA
/sh, privileged commands, and host tools are denied./sh uname -aexecutes for that sender.calc,web_search, andweb_fetch.