fix(qq): harden onebot access control - #66
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de02b7d108
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !qqUserAllowed(cfg, userID, groupID) { | ||
| if qqShouldReplyAccessDenied(incoming) { | ||
| msgType, target := qqAccessDeniedReplyDestination(userID, groupID) | ||
| c.sendQQReply(ctx, cfg, msgType, target, qqAccessDeniedReplyText) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This new allowlist-only gate lets unbound users through whenever allow_all_users is true (the default when no allowlists are set), because private QQ messages now proceed without any channel_identity_links check. In that state, any QQ user who can DM the bot can execute commands like /new and create runs on the channel, which is a security regression from the previous bootstrap-then-bind behavior.
Useful? React with 👍 / 👎.
de02b7d to
66a5b30
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66a5b3060a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !qqUserAllowed(cfg, userID, groupID) { | ||
| if qqShouldReplyAccessDenied(incoming) { | ||
| msgType, target := qqAccessDeniedReplyDestination(userID, groupID) | ||
| c.sendQQReply(ctx, cfg, msgType, target, qqAccessDeniedReplyText) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This branch sends qqAccessDeniedReplyText and returns before the inbound ledger/receipt deduplication logic runs, so a retried/replayed OneBot event with the same message ID will emit the denial reply again. In practice, disallowed users can receive duplicate denial messages whenever delivery is retried (for example after reconnects), because the message is never recorded as processed on the denial path.
Useful? React with 👍 / 👎.
| setAllowedUserIDs(nextIDs) | ||
| setAllowedUserInput('') | ||
| setSaved(false) | ||
| await persistQQSettings(nextIDs, mergeListValues(allowedGroupIDs, allowedGroupInput)) |
There was a problem hiding this comment.
When adding an allowed QQ user, this call also persists mergeListValues(allowedGroupIDs, allowedGroupInput), which includes whatever is currently typed in the group input even if the user never clicked “add” for groups. That means partial or unintended group IDs can be silently committed just by adding a user (and the symmetric path does the same in reverse), causing unexpected access-control changes.
Useful? React with 👍 / 👎.
qqqqqf-q
left a comment
qqqqqf-q
left a comment
There was a problem hiding this comment.
不合。核心问题：自搓权限轮子。
阻塞
私聊 bind 访问控制被移除，替换为自建的 allowlist 权限体系
原有的 telegramLinkBootstrapAllowed + HasLink 检查被完整删除，替换为基于 QQ 号的 allowlist 分级。这是一套独立于现有 bind 体系的新权限模型，没有提前沟通就上了。
现有的 channel binding 体系已经提供了身份关联和权限控制的基础设施，应该在此基础上扩展，而不是另起炉灶。qqChannelIdentityIsOwner 和 qqShouldReplyAccessDenied 这些函数的存在本身就说明在绕过现有架构。
要求
- 不要自建权限模型，复用现有的 bind 访问控制体系
- 如果现有体系不满足 QQ 场景的需求，先讨论方案再动手
非阻塞（代码质量本身没问题的部分）
以下改动如果从权限重构中拆出来，可以单独提 PR：
- allowlist 归一化（去重、trim、多分隔符支持）— 这个有价值
- NapCat WS endpoint 自动解析 — 独立功能，可以单独合
- 前端 add/remove 立即持久化 — UX 改进，可以单独合
2 participants
Summary:
Tests: