security(slack): fail closed for explicit empty channel user allowlists#26032
Closed
bmendonca3 wants to merge 1 commit intoopenclaw:mainfrom
Closed
security(slack): fail closed for explicit empty channel user allowlists#26032bmendonca3 wants to merge 1 commit intoopenclaw:mainfrom
bmendonca3 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Contributor
|
Thanks for the contribution and the security focus here. Closing this as superseded because the relevant auth-boundary hardening has already landed on If you want, we can open a fresh, narrowly-scoped follow-up PR on top of current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
channels.slack.channels.<channel>.users: []was treated as effectively unrestricted in Slack inbound and slash-command auth checks.The root cause was twofold:
allowed=trueinresolveSlackUserAllowedusers.length > 0as "configured"Together, this allowed unauthorized channel users to trigger bot replies and slash command dispatch in channels that were explicitly configured with an empty
userslist.Change Type
Scope
Changed files:
src/slack/monitor/allow-list.tssrc/slack/monitor/message-handler/prepare.tssrc/slack/monitor/slash.tssrc/slack/monitor/allow-list.test.tssrc/slack/monitor/message-handler/prepare.test.tssrc/slack/monitor/slash.test.tsSecurity Impact
High impact authz bypass in Slack channel scope:
channels.slack.channels.<id>.users)usersarrays were interpreted as open access in authorization pathsRepro + Verification
Deterministic repro (pre-fix behavior)
channels.slack.channels.C_LOCKED.allow: truechannels.slack.channels.C_LOCKED.users: []allowFrom: ["*"]).C_LOCKED.Fix verification
Evidence
Dedupe checks:
Targeted tests run:
Passed: 44/44 tests.
Human Verification
channels.slack.channels.<channel>.users: []for a test channel./openclaw ...slash command.usersfrom channel config and confirm normal policy behavior resumes.Compatibility / Migration
Behavior change is intentionally security-hardening:
users: []could act as open in key auth paths.users: []is treated as configured-empty and denies access.If you relied on
users: []as open behavior, remove theusersfield instead of setting it to an empty array.Failure Recovery
If this blocks expected traffic unexpectedly:
users: []key from the affected channel config.userswith intended authorized IDs.Risks and Mitigations
users: []as open.usersbehavior is unchanged.Greptile Summary
Fixed critical authorization bypass in Slack channel user allowlists where explicitly configured empty
users: []arrays were incorrectly treated as unrestricted access.Key Changes:
resolveSlackUserAllowedinsrc/slack/monitor/allow-list.ts:76-96to adddenyWhenConfiguredEmptyparameter that fails closed when an empty array is explicitly configuredchannelUsersAllowlistConfiguredcheck in both message handler (src/slack/monitor/message-handler/prepare.ts:252) and slash command handler (src/slack/monitor/slash.ts:443) to recognize empty arrays as configured (removed&& channelConfig.users.length > 0condition)denyWhenConfiguredEmpty: trueflag to all channel-scoped user authorization checksSecurity Impact:
Before this fix, setting
channels.slack.channels.<channel>.users: []created an authorization bypass allowing any channel member to trigger bot replies and slash commands, despite the explicit configuration suggesting restriction. The fix ensures empty allowlists fail closed as expected.Test Coverage:
Comprehensive regression tests added for both attack vectors (inbound messages and slash commands) that verify the bypass is blocked.
Confidence Score: 5/5
denyWhenConfiguredEmptyflag. Comprehensive regression tests cover both attack vectors (inbound messages and slash commands). The logic is sound:normalizeAllowListLowerconvertsundefinedto[], so the early return at line 83-85 now correctly handles the undefined case (returns true), while empty arrays with the flag set return false at line 88. The change is backward compatible for users who never set theusersfield.Last reviewed commit: 8a4e84f