fix: fix openapi chat#8619
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider normalizing the external
username(e.g., strip whitespace and maybe enforce a charset) before buildingopenapi:<key_id>:<username>so that slightly different spellings like" astrbot"don’t create multiple internal identities or edge cases in permission checks. - The
openapi:sender ID prefix and format string ("openapi:{key_id}:{username}") are now duplicated across implementation and tests; pulling this into a shared constant or helper used in tests as well would reduce the risk of format drift in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing the external `username` (e.g., strip whitespace and maybe enforce a charset) before building `openapi:<key_id>:<username>` so that slightly different spellings like `" astrbot"` don’t create multiple internal identities or edge cases in permission checks.
- The `openapi:` sender ID prefix and format string (`"openapi:{key_id}:{username}"`) are now duplicated across implementation and tests; pulling this into a shared constant or helper used in tests as well would reduce the risk of format drift in future changes.
## Individual Comments
### Comment 1
<location path="tests/test_api_key_open_api.py" line_range="247-253" />
<code_context>
json={
"message": "hello",
- "username": "alice_auto_session",
+ "username": "astrbot",
+ "_sender_id": "astrbot",
"enable_streaming": False,
},
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen coverage that client-provided `_sender_id` is ignored/overridden by the HTTP `/api/v1/chat` endpoint
Since `_sender_id="astrbot"` matches the username, the test doesn’t clearly show that a malicious client value is ignored. Please set `_sender_id` (and optionally `_sender_name`) to a clearly different value (e.g. `"evil-admin"`) and assert that the stored/returned `sender_id` is still `openapi:{key_id}:astrbot` and `sender_name` remains `"astrbot"`, proving that client-supplied identity fields cannot override the internal OpenAPI identity.
```suggestion
"/api/v1/chat",
json={
"message": "hello",
"username": "astrbot",
"_sender_id": "evil-admin",
"_sender_name": "evil-admin",
"enable_streaming": False,
},
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces unique sender IDs and names for OpenAPI and WebChat interactions, prefixing OpenAPI sender IDs with 'openapi:{key_id}:' to prevent them from matching default admin IDs. It updates the WebChat adapter, chat routes, and WebSocket handlers to propagate these sender fields, and adds comprehensive tests. However, a high-severity security vulnerability was identified in ChatRoute.chat where sender_id and sender_name are extracted directly from user-supplied post_data without verification. This allows a malicious dashboard user to inject _sender_id and escalate privileges to admin. It is recommended to restrict the use of client-supplied _sender_id and _sender_name to verified OpenAPI requests only.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
已根据bot提出的意见做了两处修改:
关于 Sourcery 的两个 high-level 建议: |
修复 OpenAPI chat 中一个可能导致权限提升的问题。
目前
/api/v1/chat和/api/v1/chat/ws会接收外部系统传入的username,后续 WebChat 消息进入消息管线时,这个username又会被当成事件的 sender id 使用。管理员权限判断依赖event.get_sender_id()是否命中admins_id,而默认配置里包含astrbot。这会导致一个实际风险:如果管理员给外部系统创建了带
chatscope 的 API key,例如接入自建 chat 平台、智能客服或其他第三方入口,外部用户只要把自己的username设置为astrbot,这条消息进入插件、命令和工具权限判断时就可能被当成管理员。这样会绕过原本 API key 只允许 chat 调用的权限边界,也可能影响 admin-only 命令和需要管理员权限的工具调用。这个问题不需要修改全局管理员判断逻辑,更合适的修复点是在 OpenAPI chat 入口把“外部展示用户名”和“内部权限身份”分开。
Modifications / 改动点
OpenAPI chat HTTP 路径现在会为消息生成内部 sender id,格式类似
openapi:<api_key_id>:<username>,不再直接用外部传入的username做权限身份。OpenAPI chat WebSocket 路径现在会在鉴权后保留
api_key_id,并使用同样的内部 sender id 进入 WebChat 队列。WebChatAdapter 支持从 payload 中读取内部
sender_id/sender_name,事件权限判断使用内部 sender id。原有
username仍然保留给会话创建、会话归属、历史展示和配置路由使用,避免破坏现有 OpenAPI chat 调用方式。增加回归测试,覆盖 HTTP、WebSocket、WebChatAdapter 转换,以及最终 waking 阶段的管理员身份判断。
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
结果:
测试覆盖点:
/api/v1/chat中传入username=astrbot时,内部 sender id 会变成openapi:<key_id>:astrbot。_sender_id=astrbot,也会被服务端覆盖。/api/v1/chat/ws入队 payload 使用openapi:<key_id>:astrbot,不会采信客户端传入的sender_id。/api/chat/send即使传入_sender_id=astrbot,也会强制使用当前登录用户作为内部 sender。astrbot会命中管理员,但openapi:<key_id>:astrbot不会命中默认管理员 ID。Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。