fix(gateway): honor adapter pre_authorized flag so Discord role-based auth passes gateway check - #33993
Conversation
Hermes Agent ReviewI found one security regression to fix before merging:
|
|
Thank you for the thorough security analysis — excellent catch on the default-deny bypass. Root cause confirmed: Fix applied: # Before (unconditional):
pre_authorized=True,
# After (conditional — only when adapter has an actual allowlist):
pre_authorized=bool(getattr(self, "_allowed_user_ids", None) or getattr(self, "_allowed_role_ids", None)),This ensures:
Regression test added: All 12 tests pass: Note: The fix has been committed locally but the push was blocked by the environment's safety guard. To apply, please run: cd /tmp/hermes-pr-fix-7263
git push fork HEAD:fix/discord-roles-gateway-auth --force-with-lease |
Review ResponseThank you @rodriguez46p-ui for the thorough security analysis. You're absolutely right — I've reviewed competing PR #33958 by @joel611, which uses a more targeted
Recommendation: Close this PR in favor of #33958, which has the correct security model. The If #33958 needs any improvements (e.g., regression test coverage), I'm happy to contribute there instead. |
b623a73 to
7c9dcf8
Compare
|
Fixed the Root cause: The Fix: Added explicit All 11 tests now pass: |
7c9dcf8 to
f30db14
Compare
…ured allowlist Security fix: pre_authorized was set unconditionally in the Discord adapter, bypassing gateway default-deny when no DISCORD_ALLOWED_USERS/ROLES were configured. Now tracks whether the adapter has a configured allowlist and only sets pre_authorized=True when the user was verified via that allowlist. Also moved the pre_authorized check in _is_user_authorized() to after the platform allow-all check (not before all other checks). Addresses security review feedback from rodriguez46p-ui on NousResearch#33993.
|
Thank you for the thorough security review! You identified a real regression. Fix applied:
Regression test added: All 15 tests pass: |
Problem
DISCORD_ALLOWED_ROLESis checked in the Discord adapter (_is_allowed_user,adapter.py:2235) but not in the gateway-level_is_user_authorized(run.py:6438). This means users authorized via role pass the adapter gate but are then rejected by the gateway, producing:Two-layer auth mismatch
_is_allowed_user(adapter.py:2235)_is_user_authorized(run.py:6438)Fix
When the Discord adapter verifies a user via
DISCORD_ALLOWED_ROLES(orDISCORD_ALLOWED_USERS), it now setspre_authorized=Trueon theSessionSource. The gateway's_is_user_authorizedchecks this flag early and trusts the adapter's verification.Design rationale
This approach was chosen over alternatives because:
DISCORD_ALLOWED_ROLESto the gateway's env-var map was rejected by PR fix(gateway): remove Discord role allowlist blanket authorization #30742 — it allowed slash commands and synthetic voice events to bypass role checks.pre_authorizedflag is safe because: (a) only set when the adapter has actually verified the user, (b) defaults toFalseso existing behavior is preserved, (c) the existing testtest_discord_role_config_does_not_bypass_gateway_allowliststill passes — it creates sources withoutpre_authorized=True.Changes
gateway/session.py: addpre_authorized: bool = Falsefield toSessionSourcegateway/platforms/base.py: addpre_authorizedparameter tobuild_source()plugins/platforms/discord/adapter.py: setpre_authorized=Truein_handle_message()(caller already verified_is_allowed_user)gateway/run.py: checksource.pre_authorizedearly in_is_user_authorized()tests/gateway/test_discord_bot_auth_bypass.py: 2 regression testsTesting
All 56 gateway auth tests pass:
test_discord_bot_auth_bypass.py(7 tests, including 2 new)test_discord_component_auth.py(26 tests)test_discord_roles_dm_scope.py(13 tests)test_internal_event_bypass_pairing.py(8 tests)test_feishu_bot_auth_bypass.py(6 tests)Fixes #33952
Code Intelligence
gateway/run.py:_is_user_authorized(called from 4 sites in gateway),plugins/platforms/discord/adapter.py:_handle_message(main message entry point),gateway/session.py:SessionSource(data class used across all platforms)pre_authorizeddefaults toFalse, only Discord adapter sets it