Skip to content

security(slack): fail closed for explicit empty channel user allowlists#26032

Closed
bmendonca3 wants to merge 1 commit intoopenclaw:mainfrom
bmendonca3:bm/appsec-20260225-h
Closed

security(slack): fail closed for explicit empty channel user allowlists#26032
bmendonca3 wants to merge 1 commit intoopenclaw:mainfrom
bmendonca3:bm/appsec-20260225-h

Conversation

@bmendonca3
Copy link
Copy Markdown
Contributor

@bmendonca3 bmendonca3 commented Feb 25, 2026

Summary

channels.slack.channels.<channel>.users: [] was treated as effectively unrestricted in Slack inbound and slash-command auth checks.

The root cause was twofold:

  • empty allowlists defaulted to allowed=true in resolveSlackUserAllowed
  • channel user gating treated only users.length > 0 as "configured"

Together, this allowed unauthorized channel users to trigger bot replies and slash command dispatch in channels that were explicitly configured with an empty users list.

Change Type

  • Security bug fix (auth/authz boundary hardening)
  • Regression tests

Scope

  • Slack monitor allowlist evaluation
  • Slack message inbound authorization
  • Slack slash-command authorization

Changed files:

  • src/slack/monitor/allow-list.ts
  • src/slack/monitor/message-handler/prepare.ts
  • src/slack/monitor/slash.ts
  • src/slack/monitor/allow-list.test.ts
  • src/slack/monitor/message-handler/prepare.test.ts
  • src/slack/monitor/slash.test.ts

Security Impact

High impact authz bypass in Slack channel scope:

  • Trust boundary: channel-level user allowlist (channels.slack.channels.<id>.users)
  • Bypass: explicit empty users arrays were interpreted as open access in authorization paths
  • Practical impact: unauthorized channel members could trigger model execution and command paths despite explicit channel-user configuration

Repro + Verification

Deterministic repro (pre-fix behavior)

  1. Configure Slack channel policy with an explicit empty user list:
    • channels.slack.channels.C_LOCKED.allow: true
    • channels.slack.channels.C_LOCKED.users: []
  2. Keep owner/global allowlist permissive (e.g. allowFrom: ["*"]).
  3. Send a message or slash command from a non-owner account in C_LOCKED.
  4. Observe unauthorized message/command was accepted.

Fix verification

  • Explicitly configured empty channel user lists now fail closed in both inbound message and slash-command paths.
  • Added regression tests that cover the bypass and lock in fail-closed behavior.

Evidence

Dedupe checks:

  • Existing low-severity issue covers global empty Slack allowlist defaults, not channel-scoped explicit-empty user allowlists: #13161
  • Existing related PR in a different provider/surface (Signal), not Slack channel users: #26029

Targeted tests run:

pnpm vitest run --maxWorkers=1 src/slack/monitor/allow-list.test.ts src/slack/monitor/message-handler/prepare.test.ts src/slack/monitor/slash.test.ts

Passed: 44/44 tests.

Human Verification

  1. Set channels.slack.channels.<channel>.users: [] for a test channel.
  2. From an unlisted/non-owner Slack user, send a channel message and /openclaw ... slash command.
  3. Confirm both are blocked.
  4. Remove users from channel config and confirm normal policy behavior resumes.

Compatibility / Migration

Behavior change is intentionally security-hardening:

  • Before: explicit users: [] could act as open in key auth paths.
  • After: explicit users: [] is treated as configured-empty and denies access.

If you relied on users: [] as open behavior, remove the users field instead of setting it to an empty array.

Failure Recovery

If this blocks expected traffic unexpectedly:

  1. Remove the explicit users: [] key from the affected channel config.
  2. Or populate users with intended authorized IDs.
  3. Restart/reload Slack monitor.

Risks and Mitigations

  • Risk: behavior change for deployments that intentionally used users: [] as open.
  • Mitigation: change is narrowly scoped to explicitly configured channel user lists; undefined/missing users behavior is unchanged.
  • Mitigation: regression tests added for both inbound messages and slash commands.

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:

  • Modified resolveSlackUserAllowed in src/slack/monitor/allow-list.ts:76-96 to add denyWhenConfiguredEmpty parameter that fails closed when an empty array is explicitly configured
  • Updated channelUsersAllowlistConfigured check 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 > 0 condition)
  • Applied denyWhenConfiguredEmpty: true flag to all channel-scoped user authorization checks

Security 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

  • This PR is safe to merge - it closes a critical authorization bypass with precise, well-tested changes
  • The fix is surgically precise, addressing exactly the described vulnerability with minimal code changes. The implementation correctly distinguishes between undefined allowlists (unrestricted) and explicitly empty allowlists (fail closed) via the denyWhenConfiguredEmpty flag. Comprehensive regression tests cover both attack vectors (inbound messages and slash commands). The logic is sound: normalizeAllowListLower converts undefined to [], 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 the users field.
  • No files require special attention

Last reviewed commit: 8a4e84f

@Takhoffman
Copy link
Copy Markdown
Contributor

Thanks for the contribution and the security focus here.

Closing this as superseded because the relevant auth-boundary hardening has already landed on main through newer changes, and merging this branch now would add duplicate or stale behavior.

If you want, we can open a fresh, narrowly-scoped follow-up PR on top of current main for any remaining gap.

@Takhoffman Takhoffman closed this Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: slack Channel integration: slack close:superseded PR close reason size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants