fix(gateway): restore allowlist enforcement in daemon mode (fixes #1494)#1495
Conversation
- Extract allowed_users, allowed_channels, group_policy from channel config - Pass these fields to BotConfig instead of dropping them at server.py:1367 - Add warning when no allowed_users configured - Add comprehensive unit tests for allowlist round-trip verification - Fixes critical security bug where daemon mode ignored user allowlists Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@claude the production fix in Please rewrite the tests to exercise Once the tests are rewritten and genuinely fail against a reverted |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@claude the production fix in Please rewrite the tests to exercise Once the tests are rewritten and genuinely fail against a reverted |
📝 WalkthroughWalkthroughThe gateway server's channel initialization was updated to extract and apply per-channel allowlist configuration ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
|
Claude finished @MervinPraison's task —— View job ✅ Successfully rewritten the test to test real production codeTodo List
What I ChangedThe original test had a critical flaw: it defined a Key improvements:
Test Structure
VerificationThe new tests will now properly fail if someone reverts the production fix, providing true regression protection. The test file has been pushed to the PR branch and is ready for merge. Commit: Job run • Branch: |
|
Claude finished @MervinPraison's task —— View job Test Rewrite Already Completed ✅The test rewrite request has already been completed successfully! The current version of Analysis of Current ImplementationThe existing test rewrite successfully resolves the false positive issue: ✅ Fixed Issues from Original PR Feedback:
✅ Test Results:✅ Key Validation Points:
ConclusionThe test rewrite requirement has been fully satisfied. The current implementation provides genuine regression protection for the critical security fix in Ready for review and merge! 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements allowlist and group policy configurations for bots within the gateway server, ensuring that allowed_users, allowed_channels, and mention_required settings are correctly parsed and applied. Feedback focuses on improving the test suite by avoiding the 'mirror test' anti-pattern and instead testing the actual production logic. Additionally, suggestions were made to enhance type safety for user and channel IDs, refine the mapping of group policies to mention requirements, and ensure consistency in the BotConfig object initialization.
| class MockGateway: | ||
| """Mock Gateway for testing channel allowlist plumbing.""" | ||
|
|
||
| def __init__(self): | ||
| self._agents = {} | ||
| self._channel_bots = {} | ||
| self._channel_tasks = [] | ||
| self._routing_rules = {} | ||
|
|
||
| def _create_bots_from_config(self, channels_cfg: Dict[str, Any]): | ||
| """Simplified version of the gateway's _create_bots_from_config method.""" | ||
| from praisonaiagents.bots import BotConfig | ||
|
|
||
| for channel_name, ch_cfg in channels_cfg.items(): | ||
| channel_type = ch_cfg.get("platform", channel_name).lower() | ||
| token = ch_cfg.get("token", "") | ||
|
|
||
| if not token: | ||
| continue | ||
|
|
||
| routes = ch_cfg.get("routing") or ch_cfg.get("routes") or {"default": "default"} | ||
| self._routing_rules[channel_name] = routes | ||
|
|
||
| # Get default agent (use mock for testing) | ||
| default_agent = Agent(name="test_agent", instructions="Test instructions") | ||
|
|
||
| # This is the critical code being tested - extracted from server.py:1367-1386 | ||
| # Extract allowlist configuration from channel config | ||
| _raw_allowed = ch_cfg.get("allowed_users") or [] | ||
| if isinstance(_raw_allowed, str): | ||
| # Env-expanded string like "12345,67890"; split on commas. | ||
| _raw_allowed = [s.strip() for s in _raw_allowed.split(",") if s.strip()] | ||
|
|
||
| _raw_channels = ch_cfg.get("allowed_channels") or [] | ||
| if isinstance(_raw_channels, str): | ||
| _raw_channels = [s.strip() for s in _raw_channels.split(",") if s.strip()] | ||
|
|
||
| # Extract group policy setting | ||
| group_policy = ch_cfg.get("group_policy", "mention_only") | ||
| mention_required = (group_policy == "mention_only") | ||
|
|
||
| config = BotConfig( | ||
| token=token, | ||
| allowed_users=list(_raw_allowed), | ||
| allowed_channels=list(_raw_channels), | ||
| mention_required=mention_required, | ||
| ) | ||
|
|
||
| # Store for verification | ||
| self._channel_bots[channel_name] = { | ||
| "config": config, | ||
| "agent": default_agent, | ||
| "type": channel_type | ||
| } | ||
|
|
There was a problem hiding this comment.
This MockGateway class is a 'mirror test' anti-pattern. By re-implementing the logic from server.py inside the test file, you are testing the mock rather than the actual production code. This explains why the tests pass even if the fix in server.py is removed.
To provide a real regression guarantee, you should follow the plan outlined in the PR description:
- Refactor
WebSocketGateway.start_channelsinserver.pyto extract the bot configuration and creation logic into a dedicated method (e.g.,_create_bots_from_config). - In this test file, import the real
WebSocketGatewayand call that method. - Assert that the resulting
BotConfigobjects ingw._channel_botshave the expected values. - Add a test case that verifies the failure when the fix is absent (e.g., by monkey-patching the method to simulate the old buggy behavior).
| # Extract allowlist configuration from channel config | ||
| _raw_allowed = ch_cfg.get("allowed_users") or [] | ||
| if isinstance(_raw_allowed, str): | ||
| # Env-expanded string like "12345,67890"; split on commas. | ||
| _raw_allowed = [s.strip() for s in _raw_allowed.split(",") if s.strip()] | ||
|
|
||
| _raw_channels = ch_cfg.get("allowed_channels") or [] | ||
| if isinstance(_raw_channels, str): | ||
| _raw_channels = [s.strip() for s in _raw_channels.split(",") if s.strip()] | ||
|
|
||
| # Extract group policy setting | ||
| group_policy = ch_cfg.get("group_policy", "mention_only") | ||
| mention_required = (group_policy == "mention_only") | ||
|
|
||
| config = BotConfig( | ||
| token=token, | ||
| allowed_users=list(_raw_allowed), | ||
| allowed_channels=list(_raw_channels), | ||
| mention_required=mention_required, | ||
| ) |
There was a problem hiding this comment.
There are several improvements needed in this configuration block:
- Type Safety for IDs: YAML parsers often interpret numeric IDs (like Telegram user IDs) as integers. Since
BotConfig.is_user_allowedperforms string comparisons (user_id in self.allowed_users), these IDs will fail to match unless explicitly converted to strings. - Group Policy Mapping: The current logic sets
mention_requiredtoFalseifgroup_policyis"command_only". This is likely incorrect, ascommand_onlyshould still require a mention (or command prefix) to avoid responding to all natural language messages in a group. A more robust mapping ismention_required = (group_policy != "respond_all"). - Consistency: The
group_policyfield should be passed to theBotConfigconstructor to ensure the object state is consistent with the derivedmention_requiredvalue. - Style: There is a trailing space on line 1367.
# Extract allowlist configuration from channel config
_raw_allowed = ch_cfg.get("allowed_users") or []
if isinstance(_raw_allowed, str):
# Env-expanded string like "12345,67890"; split on commas.
_raw_allowed = [s.strip() for s in _raw_allowed.split(",") if s.strip()]
else:
# Ensure all IDs are strings (YAML might parse them as ints)
_raw_allowed = [str(u).strip() for u in _raw_allowed if str(u).strip()]
_raw_channels = ch_cfg.get("allowed_channels") or []
if isinstance(_raw_channels, str):
_raw_channels = [s.strip() for s in _raw_channels.split(",") if s.strip()]
else:
_raw_channels = [str(c).strip() for c in _raw_channels if str(c).strip()]
# Extract group policy setting
group_policy = ch_cfg.get("group_policy", "mention_only")
mention_required = (group_policy != "respond_all")
config = BotConfig(
token=token,
allowed_users=_raw_allowed,
allowed_channels=_raw_channels,
mention_required=mention_required,
group_policy=group_policy,
)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/gateway/server.py`:
- Around line 1367-1394: The current mapping extracts group_policy into the
local variable but omits it when constructing BotConfig and also leaves
list-derived allowed_users/allowed_channels as-is (often ints); update the
BotConfig(...) call to pass group_policy=group_policy and ensure you coerce
entries in _raw_allowed and _raw_channels to strings (e.g., map str over list
branches before building config) so BotConfig.allowed_users/allowed_channels are
all str and BotConfig.group_policy reflects the configured value used by
BotConfig.is_user_allowed and checks that read channel_config.group_policy.
In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py`:
- Around line 145-164: Add a new unit that exercises the real startup path (use
start_channels or whatever triggers channel initialization) and asserts a
WARNING log is emitted when a channel has an empty allowed_users list: drive the
gateway startup (instead of only calling MockGateway._create_bots_from_config),
capture logs with caplog, and assert that caplog contains a WARNING record whose
message includes "accepts messages from everyone" (or the exact warning text
produced by the code); reference the gateway startup methods (start_channels /
MockGateway._create_bots_from_config) and the
_channel_bots["telegram"]["config"] to locate the relevant initialization path.
- Around line 18-71: The test duplicates production logic by re-implementing the
allowlist parsing in MockGateway._create_bots_from_config, producing a false
positive; instead call the real path by invoking WebSocketGateway.start_channels
and stub gw._create_bot to capture the BotConfig so the production parsing runs.
Replace the MockGateway usage with a WebSocketGateway instance, register a test
Agent, patch WebSocketGateway._create_bot (and patch _run_bot_safe to avoid side
effects) to capture the config passed into _create_bot, then assert on the
captured BotConfig.allowed_users, allowed_channels, and mention_required to
validate the real code path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b35040bd-dea4-4321-9d63-a07dc22e1a11
📒 Files selected for processing (2)
src/praisonai/praisonai/gateway/server.pysrc/praisonai/tests/unit/gateway/test_channel_allowlist.py
| # Extract allowlist configuration from channel config | ||
| _raw_allowed = ch_cfg.get("allowed_users") or [] | ||
| if isinstance(_raw_allowed, str): | ||
| # Env-expanded string like "12345,67890"; split on commas. | ||
| _raw_allowed = [s.strip() for s in _raw_allowed.split(",") if s.strip()] | ||
|
|
||
| _raw_channels = ch_cfg.get("allowed_channels") or [] | ||
| if isinstance(_raw_channels, str): | ||
| _raw_channels = [s.strip() for s in _raw_channels.split(",") if s.strip()] | ||
|
|
||
| # Extract group policy setting | ||
| group_policy = ch_cfg.get("group_policy", "mention_only") | ||
| mention_required = (group_policy == "mention_only") | ||
|
|
||
| config = BotConfig( | ||
| token=token, | ||
| allowed_users=list(_raw_allowed), | ||
| allowed_channels=list(_raw_channels), | ||
| mention_required=mention_required, | ||
| ) | ||
|
|
||
| # Warn if no allowlist is configured | ||
| if not config.allowed_users: | ||
| logger.warning( | ||
| "Channel %r has no allowed_users — bot accepts messages from everyone. " | ||
| "Re-run `praisonai onboard` to configure.", | ||
| channel_name, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any downstream usage of BotConfig.group_policy (beyond the config module itself)
rg -nP --type=py -C2 '\bgroup_policy\b' -g '!**/config.py'
# Confirm adapter types for user_id / channel_id at the is_user_allowed / is_channel_allowed call sites
rg -nP --type=py -C3 '\bis_user_allowed\s*\('
rg -nP --type=py -C3 '\bis_channel_allowed\s*\('Repository: MervinPraison/PraisonAI
Length of output: 25959
Plumb group_policy through to BotConfig and coerce IDs to str.
Two gaps in the new mapping:
-
BotConfighas bothgroup_policy: strandmention_required: boolas independent fields (seesrc/praisonai-agents/praisonaiagents/bots/config.py). You extractgroup_policybut drop it from the constructor call, soconfig.group_policywill always be the default"mention_only". This breaksdoctor/checks.pyline 139, which readschannel_config.group_policyto warn on overly permissive policies. Passgroup_policyto keep the value intact. -
The list branch of
allowed_users/allowed_channelspasses values through verbatim. YAML commonly yields integers (allowed_users: [42, 12345]), butBotConfig.is_user_allowed(user_id: str)doesuser_id in self.allowed_users, which is"42" in [42, 12345]→False. The comma-string branch already yields strings; the list branch should too.
🔧 Proposed fix
# Extract allowlist configuration from channel config
_raw_allowed = ch_cfg.get("allowed_users") or []
if isinstance(_raw_allowed, str):
# Env-expanded string like "12345,67890"; split on commas.
_raw_allowed = [s.strip() for s in _raw_allowed.split(",") if s.strip()]
+ else:
+ _raw_allowed = [str(x).strip() for x in _raw_allowed if str(x).strip()]
_raw_channels = ch_cfg.get("allowed_channels") or []
if isinstance(_raw_channels, str):
_raw_channels = [s.strip() for s in _raw_channels.split(",") if s.strip()]
+ else:
+ _raw_channels = [str(x).strip() for x in _raw_channels if str(x).strip()]
# Extract group policy setting
group_policy = ch_cfg.get("group_policy", "mention_only")
mention_required = (group_policy == "mention_only")
config = BotConfig(
token=token,
- allowed_users=list(_raw_allowed),
- allowed_channels=list(_raw_channels),
+ allowed_users=_raw_allowed,
+ allowed_channels=_raw_channels,
+ group_policy=group_policy,
mention_required=mention_required,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/gateway/server.py` around lines 1367 - 1394, The
current mapping extracts group_policy into the local variable but omits it when
constructing BotConfig and also leaves list-derived
allowed_users/allowed_channels as-is (often ints); update the BotConfig(...)
call to pass group_policy=group_policy and ensure you coerce entries in
_raw_allowed and _raw_channels to strings (e.g., map str over list branches
before building config) so BotConfig.allowed_users/allowed_channels are all str
and BotConfig.group_policy reflects the configured value used by
BotConfig.is_user_allowed and checks that read channel_config.group_policy.
| def test_empty_allowlist_allows_everyone(): | ||
| """Test that empty allowed_users list allows all users (backward compatibility).""" | ||
|
|
||
| channels_config = { | ||
| "telegram": { | ||
| "token": "test-token", | ||
| # No allowed_users specified | ||
| } | ||
| } | ||
|
|
||
| gateway = MockGateway() | ||
| gateway._create_bots_from_config(channels_config) | ||
|
|
||
| config = gateway._channel_bots["telegram"]["config"] | ||
|
|
||
| # Empty list should allow everyone | ||
| assert config.allowed_users == [] | ||
| assert config.is_user_allowed("42") is True | ||
| assert config.is_user_allowed("99") is True | ||
| assert config.is_user_allowed("") is True |
There was a problem hiding this comment.
Missing coverage: startup warning when allowed_users is empty.
The PR lists "Log a startup warning when a channel has an empty allowed_users list" as an acceptance criterion, but no test asserts the warning is emitted. Once the test is refactored onto real start_channels, add a caplog assertion to lock in this behavior.
def test_empty_allowlist_logs_warning(caplog):
# ... drive real start_channels with empty allowed_users ...
assert any(
"accepts messages from everyone" in r.message
for r in caplog.records if r.levelname == "WARNING"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py` around lines 145
- 164, Add a new unit that exercises the real startup path (use start_channels
or whatever triggers channel initialization) and asserts a WARNING log is
emitted when a channel has an empty allowed_users list: drive the gateway
startup (instead of only calling MockGateway._create_bots_from_config), capture
logs with caplog, and assert that caplog contains a WARNING record whose message
includes "accepts messages from everyone" (or the exact warning text produced by
the code); reference the gateway startup methods (start_channels /
MockGateway._create_bots_from_config) and the
_channel_bots["telegram"]["config"] to locate the relevant initialization path.
…stead of mock - Remove MockGateway class that reimplemented production logic - Test actual WebSocketGateway.start_channels() method via async pytest - Mock only _create_bot to prevent network connections, all allowlist logic is real - Add regression test that demonstrates failure if production fix is reverted - Maintain all original test coverage with proper parametrization - Tests now verify server.py:1367-1394 allowlist extraction actually works Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Reviewed at commit
class DummyBot:
def __init__(self, config):
self.config = config
with patch.object(gateway, "_create_bot", side_effect=lambda *_args: DummyBot(_args[3])):
with patch.object(gateway, "_run_bot_safe", return_value=None):
await gateway.start_channels(channels_config)
assert gateway._channel_bots["telegram"].config.allowed_users == ["42", "12345"]and factor repeated setup into a shared helper/fixture to reduce duplication. |
|
@claude final review |
|
Claude finished @MervinPraison's task —— View job Final Code ReviewTodo List:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/praisonai/tests/unit/gateway/test_channel_allowlist.py (1)
120-146:⚠️ Potential issue | 🟡 MinorLock in the empty-allowlist startup warning.
This verifies allow-all behavior, but not the required warning that tells operators the channel accepts everyone.
🧪 Proposed test assertion
`@pytest.mark.asyncio` -async def test_empty_allowlist_allows_everyone(): +async def test_empty_allowlist_allows_everyone(caplog): """Test that empty allowed_users list allows all users (backward compatibility).""" + caplog.set_level("WARNING") channels_config = { "telegram": { "token": "test-token", # No allowed_users specified @@ with patch.object(gateway, '_create_bot', side_effect=mock_create_bot): await gateway.start_channels(channels_config) + assert any( + record.levelname == "WARNING" + and "accepts messages from everyone" in record.message + for record in caplog.records + ) + # Empty list should allow everyone assert captured_config.allowed_users == []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py` around lines 120 - 146, Add an assertion that the gateway emits the startup warning when an empty allowed_users is used: when calling gateway.start_channels in test_empty_allowlist_allows_everyone (with create_test_gateway_with_agent and the mock_create_bot side-effect capturing captured_config), capture the logger output (e.g., via pytest caplog or by patching gateway.logger.warning) and assert that a warning was logged containing text indicating the channel accepts everyone or that the allowlist is empty so operators are warned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py`:
- Around line 149-181: The test only exercises parsing for allowed_users but not
allowed_channels, which has a separate parsing branch; update the parameterized
test (test_string_parsing_edge_cases) to also run each input_str against
allowed_channels (or add a second parametrized test) so both config fields are
validated; when invoking gateway.start_channels ensure the channels_config sets
both "allowed_users" and "allowed_channels" to input_str, capture config via the
mock_create_bot (_create_bot) and assert captured_config.allowed_channels equals
expected in addition to allowed_users to prevent regressions in the other
parsing branch.
---
Duplicate comments:
In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py`:
- Around line 120-146: Add an assertion that the gateway emits the startup
warning when an empty allowed_users is used: when calling gateway.start_channels
in test_empty_allowlist_allows_everyone (with create_test_gateway_with_agent and
the mock_create_bot side-effect capturing captured_config), capture the logger
output (e.g., via pytest caplog or by patching gateway.logger.warning) and
assert that a warning was logged containing text indicating the channel accepts
everyone or that the allowlist is empty so operators are warned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1fcb5d5-3d3a-417f-ae98-0b72f0b86494
📒 Files selected for processing (1)
src/praisonai/tests/unit/gateway/test_channel_allowlist.py
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("input_str,expected", [ | ||
| ("42,67890", ["42", "67890"]), # Normal case | ||
| ("42, 67890", ["42", "67890"]), # Spaces | ||
| (" 42 , 67890 ", ["42", "67890"]), # Leading/trailing spaces | ||
| ("42,,67890", ["42", "67890"]), # Empty element | ||
| ("42,", ["42"]), # Trailing comma | ||
| (",42", ["42"]), # Leading comma | ||
| ("", []), # Empty string | ||
| ("42", ["42"]), # Single item | ||
| ]) | ||
| async def test_string_parsing_edge_cases(input_str, expected): | ||
| """Test edge cases in comma-separated string parsing through real Gateway.""" | ||
|
|
||
| channels_config = { | ||
| "test": { | ||
| "token": "test-token", | ||
| "allowed_users": input_str, | ||
| } | ||
| } | ||
|
|
||
| gateway = create_test_gateway_with_agent() | ||
| captured_config = None | ||
|
|
||
| def mock_create_bot(channel_type, token, agent, config, ch_cfg): | ||
| nonlocal captured_config | ||
| captured_config = config | ||
| return None | ||
|
|
||
| with patch.object(gateway, '_create_bot', side_effect=mock_create_bot): | ||
| await gateway.start_channels(channels_config) | ||
|
|
||
| assert captured_config.allowed_users == expected, f"Input '{input_str}' should parse to {expected}, got {captured_config.allowed_users}" |
There was a problem hiding this comment.
Exercise comma parsing for both allowlist fields.
This only parameterizes allowed_users; allowed_channels has a separate production parsing branch, so trimming/empty-segment regressions there could still pass.
🧪 Proposed parametrized coverage
`@pytest.mark.asyncio`
+@pytest.mark.parametrize("field_name", ["allowed_users", "allowed_channels"])
`@pytest.mark.parametrize`("input_str,expected", [
("42,67890", ["42", "67890"]), # Normal case
("42, 67890", ["42", "67890"]), # Spaces
(" 42 , 67890 ", ["42", "67890"]), # Leading/trailing spaces
@@
("", []), # Empty string
("42", ["42"]), # Single item
])
-async def test_string_parsing_edge_cases(input_str, expected):
+async def test_string_parsing_edge_cases(field_name, input_str, expected):
"""Test edge cases in comma-separated string parsing through real Gateway."""
channels_config = {
"test": {
"token": "test-token",
- "allowed_users": input_str,
+ field_name: input_str,
}
}
@@
with patch.object(gateway, '_create_bot', side_effect=mock_create_bot):
await gateway.start_channels(channels_config)
- assert captured_config.allowed_users == expected, f"Input '{input_str}' should parse to {expected}, got {captured_config.allowed_users}"
+ actual = getattr(captured_config, field_name)
+ assert actual == expected, f"Input '{input_str}' for {field_name} should parse to {expected}, got {actual}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/gateway/test_channel_allowlist.py` around lines 149
- 181, The test only exercises parsing for allowed_users but not
allowed_channels, which has a separate parsing branch; update the parameterized
test (test_string_parsing_edge_cases) to also run each input_str against
allowed_channels (or add a second parametrized test) so both config fields are
validated; when invoking gateway.start_channels ensure the channels_config sets
both "allowed_users" and "allowed_channels" to input_str, capture config via the
mock_create_bot (_create_bot) and assert captured_config.allowed_channels equals
expected in addition to allowed_users to prevent regressions in the other
parsing branch.
Fixes #1494
Problem (before)
Critical security bug — in the default daemon-install path (PR #1493), Telegram/Discord/Slack bots were accepting messages from everyone regardless of the
allowed_userslist the user configured duringpraisonai onboard.Root cause was a single line:
ch_cfg["allowed_users"]/ch_cfg["allowed_channels"]/ch_cfg["group_policy"]— the exact fields the onboard wizard prompts for and writes intobot.yaml— were silently dropped. The adapter then checkedconfig.is_user_allowed(...)against an empty list → always returned True.Scope of exposure: every deployment using
praisonai onboard→ daemon (the default) since PR #1493 merged. Single-bot mode (praisonai bot start ...) was NOT affected because it constructsBotConfigdirectly from yaml with all fields.Fix (after)
Three things fixed at once:
allowed_users(supports both list and env-expanded"42,67890"string — the wizard emits the string form).allowed_channels(same dual-format support).group_policy: mention_only→BotConfig.mention_required = True(was also being dropped).Plus a loud operator warning when the allowlist is empty, so future regressions of this shape can't silently ship.
Evidence per claim
git show main:src/praisonai/praisonai/gateway/server.py | sed -n '1367p'→config = BotConfig(token=token)telegram.py:183/discord.py:159/slack.py:174all readself.config.is_user_allowed(...)— that path is untouchedserver.py:1367-1394logger.warning("Channel %r has no allowed_users …")in the same blockallowed_usersstill means "everyone allowed" (seeBotConfig.is_user_allowed:123-125). No yaml rewrite needed.Known defect in this PR — flagged for revision
The test
test_channel_allowlist.pyis a false positive and needs to be rewritten before merge.It defines a
MockGatewayclass that re-implements the fix body inline in the test file, then asserts the mock produces the rightBotConfig. Because the assertions run against the mock, notpraisonai.gateway.server.WebSocketGateway._create_bots_from_config, stashing the real fix inserver.pydoes not break the tests:So the test gives zero regression guarantee against the actual file that shipped the bug.
@claude please rewrite the test to:
from praisonai.gateway.server import WebSocketGatewaydirectly.WebSocketGateway(host/port don't matter — no.start()).gw._create_bots_from_config({"telegram": {...}})with a stub agent already ingw._agents.gw._channel_bots["telegram"]is a real bot whose.config.allowed_users == ["42", "12345"].server.py_create_bots_from_configmethod to verify our mock-free test fails when the fix is reverted.Keep the parametrised cases — just hang them off the real method, not a mock.
Verification run (production fix alone, with the mock-test)
Manual smoke:
Diff
Rollback
Single-file revert. No schema change, no yaml migration, no API change.
Summary by CodeRabbit
New Features
Tests