Skip to content

fix: empty allowed_channels denies all channels (secure by default)#92

Open
chaodu-agent wants to merge 1 commit intoopenabdev:mainfrom
chaodu-agent:fix/empty-allowed-channels-deny-all
Open

fix: empty allowed_channels denies all channels (secure by default)#92
chaodu-agent wants to merge 1 commit intoopenabdev:mainfrom
chaodu-agent:fix/empty-allowed-channels-deny-all

Conversation

@chaodu-agent
Copy link
Copy Markdown
Collaborator

Summary

Implements Option A from #91: empty allowed_channels now means the bot will not respond to any messages (secure by default).

Changes

src/discord.rs

  • Removed self.allowed_channels.is_empty() || from the channel check — empty set now denies all
  • Added startup warning log in ready() when allowed_channels is empty:
    WARN: allowed_channels is empty — bot will NOT respond to any messages. Configure allowed_channels in config.toml.
    

config.toml.example

  • Added comment: # Required: at least one channel ID. Empty list = bot will not respond to any messages.

Before / After

Scenario Before After
allowed_channels = [] ✅ Responds in ALL channels ❌ Responds in NO channels + warning log
allowed_channels = ["123"] ✅ Responds in channel 123 ✅ Responds in channel 123 (unchanged)
Thread in allowed channel ✅ Works ✅ Works (unchanged)

Breaking Change

Users who intentionally left allowed_channels empty to allow all channels will need to explicitly list their channel IDs. This is a one-line config change.

Closes #91

Previously, an empty allowed_channels list would allow the bot to
respond in ALL channels. This changes the behavior so that an empty
list means the bot will not respond to any messages.

Also adds a startup warning log when allowed_channels is empty and
a clarifying comment in config.toml.example.

Closes openabdev#91
Copy link
Copy Markdown
Contributor

@masami-agent masami-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean change, well-scoped 👍 Two things to address:

1. Needs rebase on main

PR #108 (allowed_users) has since merged and modified the same area of discord.rs. This PR will likely have conflicts. The allowed_users check also needs the same treatment — after rebase, please verify the user check logic still works correctly alongside this change.

2. Behavioral consistency with allowed_users

After this PR, the semantics would be:

  • allowed_channels = [] → deny all (this PR)
  • allowed_users = [] → allow all (#108)

This asymmetry could confuse users. Worth discussing: should allowed_users = [] also mean deny all? Or is the current design intentional (channels = required, users = optional filter)?

If intentional, please add a note in the PR description and config.toml.example clarifying the difference. If not, consider aligning the behavior.

Otherwise LGTM — the secure-by-default approach for channels makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Empty allowed_channels should not default to allowing all channels

2 participants