fix(slack): authorize bot/workflow senders before the no-user-id guard (salvage #52403) - #313
Merged
Conversation
|
Review Complete Risk: 🟡 Medium (38/100) — no findings · 195 LOC across 4 files The PR cleanly reorders bot auth bypass before the user-id guard and adds SLACK_ALLOW_BOTS support, but leaves orphaned test code and a stale config bridge that will cause CI failures if not cleaned up. Files Reviewed (4 files) |
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
Salvage of NousResearch#52403 by @cypctlinux (rebased onto current
mainwith a conflict resolution). Lets allowed Slack bot/workflow senders route by authorizing them before the no-user-id guard.The bug
gateway/authz_mixin.pyhas a{PLATFORM}_ALLOW_BOTSbypass, but onmainit (a) didn't list Slack, and (b) ran the bot bypass ~83 lines after theif not user_id: return Falseguard. Slack Workflow-Builder / app posts arrive assubtype=bot_messagewith nouserfield →user_id=None, so they hit that guard and were rejected before the bypass could ever run. Net effect:SLACK_ALLOW_BOTS=mentions|allhad no effect for exactly the workflow/app posts it was meant to admit.The fix
platform_allow_bots_map+if getattr(source, "is_bot")bypass to run before the no-user-id guard (so auser=Nonebot event can still be admitted), and addPlatform.SLACK: "SLACK_ALLOW_BOTS"to the map.is_bot = bool(event.get("bot_id")) or subtype == "bot_message") so the bypass applies.user_id=Noneauthorized undermentions/all, denied when unset/none, humans unaffected.The bypass still requires
is_botANDSLACK_ALLOW_BOTS ∈ {mentions, all}; unmapped platforms and non-bot traffic fall through to the guard exactly as before — moving it earlier changes when a would-be-authorized bot is admitted, never whether.Rebase conflict resolution (this salvage)
mainhad drifted since the PR was cut: it addedPlatform.TELEGRAM: "TELEGRAM_ALLOW_BOTS"to the old (late-position) map. The PR's relocated map only had DISCORD/FEISHU/SLACK. Resolved by deleting the old block and re-addingPlatform.TELEGRAMto the relocated map, so no platform is dropped — the map now has DISCORD/FEISHU/TELEGRAM/SLACK. Also added an AUTHOR_MAP entry (t.chen@aftership.com→ @cypctlinux).Chosen over competing PRs (issue: Slack bot mentions don't route)
This was a 5-PR cluster. NousResearch#52403 is the correct base because it relocates the bypass before the guard — the actual
user=Nonefailure mode. Two near-duplicates are superseded:gateway/platforms/slack.pyandgateway/run.pyhelpers that no longer exist onmain(dead path).Distinct/complementary PRs left open: NousResearch#52390 (Block-Kit-only mention detection) and NousResearch#51627 (peer bot-routing loops).
Review
Ran hermes-agent-dev + hermes-pr-review Phase 2c — 0 Critical / 0 Warnings. Verified the relocation opens no new hole, Telegram preserved, comment now literally accurate, consistent double-gate with the adapter's own bot filter. Locally: 53 tests pass (31 Slack bot-auth + 22 Telegram/Discord/Feishu bot-auth — the relocation doesn't regress other platforms).
Tests
Supersedes NousResearch#52403 (and NousResearch#56286 / NousResearch#40883). Full credit to @cypctlinux.
Mirror-of: NousResearch#56316
NousResearch#56316