refactor: centralize channel ingress access#79092
Conversation
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: no. high-confidence failing reproduction remains from this source review. The previously flagged Teams and Matrix cases match current docs/source behavior by source comparison, but I did not run live callbacks or local tests. Real behavior proof Next step before merge Security Review detailsBest possible solution: Keep the shared ingress direction under maintainer review, require fresh redacted current-head proof, and land only after the remaining checks and owner review are satisfied. Do we have a high-confidence way to reproduce the issue? No high-confidence failing reproduction remains from this source review. The previously flagged Teams and Matrix cases match current docs/source behavior by source comparison, but I did not run live callbacks or local tests. Is this the best way to solve the issue? Unclear as a final merge decision. The centralized ingress direction matches the architecture guidance, but this XL auth refactor still needs maintainer review and fresh current-head real behavior proof before landing. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against aac9ebd4f3ef. Re-review progress:
|
982e23a to
6055c49
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cc9487255
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| effectiveRoomUsers, | ||
| }), | ||
| commandOwner: commandAllowFrom, | ||
| commandGroup: effectiveRoomUsers.length > 0 ? effectiveRoomUsers : effectiveGroupAllowFrom, |
There was a problem hiding this comment.
Merge room and global command allowlists
When both roomUsers and groupAllowFrom are configured for a room, commandGroup is set to effectiveRoomUsers only, so command authorization no longer accepts users who are allowed solely through the global group allowlist. This changes prior OR-style behavior (room users OR global group allowlist) and can block valid control/text commands in existing Matrix setups that rely on both lists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc378bd7b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| effectiveRoomUsers, | ||
| }), | ||
| commandOwner: commandAllowFrom, | ||
| commandGroup: effectiveRoomUsers.length > 0 ? effectiveRoomUsers : effectiveGroupAllowFrom, |
There was a problem hiding this comment.
Merge room and global command allowlists
For room traffic, commandGroup currently picks effectiveRoomUsers whenever that list is non-empty and drops effectiveGroupAllowFrom, which changes command auth from (roomUsers OR groupAllowFrom) to roomUsers only in mixed configurations. This blocks valid control/text commands from users who are allowed via the global group allowlist but not present in the room-specific list. Fresh evidence on this commit: with roomUsers=['@room-only:example.org'], groupAllowFrom=['@admin:example.org'], and sender @admin:example.org, commandAuthorizers[2].allowed is true but resolveMatrixMonitorCommandAccess(...).commandAuthorized is false because only the room list is fed into commandGroup.
Useful? React with 👍 / 👎.
bc378bd to
0a7b5ac
Compare
0a7b5ac to
08db2c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08db2c33a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const commandGroupAllowFrom = | ||
| state.effectiveRoomUsers.length > 0 ? state.effectiveRoomUsers : state.effectiveGroupAllowFrom; |
There was a problem hiding this comment.
Merge room and global command allowlists for Matrix commands
When a Matrix room has both roomUsers and groupAllowFrom configured, command authorization currently uses only effectiveRoomUsers and discards effectiveGroupAllowFrom, which changes command auth from an OR condition to room-users-only and blocks valid control/text commands from globally allowlisted senders. Fresh evidence on this commit: commandGroupAllowFrom is assigned with state.effectiveRoomUsers.length > 0 ? state.effectiveRoomUsers : state.effectiveGroupAllowFrom, so any non-empty room user list suppresses the global group allowlist entirely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1129a2afa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| groupPolicy: senderGroupPolicy, | ||
| groupPolicy, | ||
| policy: { | ||
| groupAllowFromFallbackToAllowFrom: true, |
There was a problem hiding this comment.
Preserve Teams group allowlist isolation from DM allowlist
Switching groupAllowFromFallbackToAllowFrom to true makes group sender authorization inherit channels.msteams.allowFrom whenever groupAllowFrom is unset/empty, which broadens access beyond prior behavior. In practice, a user listed only for DMs can now be accepted in group/channel conversations under groupPolicy: "allowlist", whereas the previous implementation (false) treated empty group sender lists as deny/empty-allowlist. This is a regression in authorization scope and can unintentionally expose group conversations.
Useful? React with 👍 / 👎.
d49a568 to
cfe5616
Compare
3a0d830 to
dce57e3
Compare
dce57e3 to
0c55491
Compare
Summary
This PR centralizes message-channel ingress authorization behind an experimental
openclaw/plugin-sdk/channel-ingressAPI, then migrates bundled channel hot paths onto the shared decision graph while preserving older plugin SDK compatibility seams.AccessFactsprojection for route, sender, command, event, and mention-activation gates.Behavior Fixed
cfg.accessGroupsinto DM and group ingress state.allowFromfallback where that was the legacy contract.origin-subjectevent auth matches adapter-normalized identity values, not opaque subject slot ids or raw unnormalized values.API Design
Core receives selected facts and policy slices, not whole config, stores, clients, or network hooks. Raw sender ids and raw allowlist entries exist only in resolver input and adapter match material. Serialized state, decisions, diagnostics, snapshots, and
AccessFactsuse opaque ids, counts, reason codes, and redacted match ids. The new SDK subpath remains experimental while bundled migrations settle; existing SDK helpers remain source-compatible.Review Guide
The branch is split for review by layer:
refactor: add channel ingress kernelrefactor: expose channel ingress sdkrefactor: migrate bundled channel ingressfix: mark discord system events untrustedfix: fail closed slack command ingressThe important split: commit 1 is generic core only, commit 2 is SDK/docs/compatibility surface, commit 3 is bundled plugin adoption, commit 4 is the OpenGrep hardening fix for Discord system events, commit 5 is the Slack command-auth regression fix.
Compatibility / Migration
Real Behavior Proof
allowFromfallback, origin-subject event auth, QQBot slash/native command auth, Discord system-event trust classification, Slack block-action command auth, and bundled plugin command/sender gate separation.origin/main, final pushed head9cc94872559dec61ef264ce4912debbbc3a042f5; remote Linux proof via Crabbox/Blacksmith Testbox leasestbx_01kr2tv446sw5ntj53n3kajxmf,tbx_01kr2y2gt7hwer215nj5cvba32.enqueueSystemEventcallsites found by OpenGrep now passtrusted: falsefor channel-derived system text. Slack command ingress now reportscommandAuthorized: falsewhen no command gate exists because a sender gate blocked first.