Skip to content

fix: fix openapi chat#8619

Open
Sisyphbaous-DT-Project wants to merge 2 commits into
AstrBotDevs:masterfrom
Sisyphbaous-DT-Project:fix/openapi-chat-sender-identity
Open

fix: fix openapi chat#8619
Sisyphbaous-DT-Project wants to merge 2 commits into
AstrBotDevs:masterfrom
Sisyphbaous-DT-Project:fix/openapi-chat-sender-identity

Conversation

@Sisyphbaous-DT-Project
Copy link
Copy Markdown
Contributor

@Sisyphbaous-DT-Project Sisyphbaous-DT-Project commented Jun 6, 2026

修复 OpenAPI chat 中一个可能导致权限提升的问题。

目前 /api/v1/chat/api/v1/chat/ws 会接收外部系统传入的 username,后续 WebChat 消息进入消息管线时,这个 username 又会被当成事件的 sender id 使用。管理员权限判断依赖 event.get_sender_id() 是否命中 admins_id,而默认配置里包含 astrbot

这会导致一个实际风险:如果管理员给外部系统创建了带 chat scope 的 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 / 运行截图或测试结果

/tmp/astrbot-p1-review-venv/bin/python -m pytest -q --capture=no tests/test_api_key_open_api.py

结果:

15 passed, 2 warnings in 8.57s

测试覆盖点:

  • /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
  • 普通 dashboard /api/chat/send 即使传入 _sender_id=astrbot,也会强制使用当前登录用户作为内部 sender。
  • WebChatAdapter 会把内部 sender id 用作事件 sender id,同时保持原有 session id 结构。
  • WakingCheckStage 中,原始 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.txt and pyproject.toml.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txtpyproject.toml 文件相应位置。

  • 😮 My changes do not introduce malicious code.
    / 我的更改没有引入恶意代码。

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend area:webui The bug / feature is about webui(dashboard) of astrbot. labels Jun 6, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread tests/test_api_key_open_api.py
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread astrbot/dashboard/routes/chat.py Outdated
@Sisyphbaous-DT-Project
Copy link
Copy Markdown
Contributor Author

Sisyphbaous-DT-Project commented Jun 6, 2026

已根据bot提出的意见做了两处修改:

  • 针对 Sourcery 的测试建议:HTTP 回归测试现在会传入明显不同的客户端伪造值 _sender_id="evil-admin"_sender_name="evil-admin",并继续断言服务端内部 sender 是 openapi:{key_id}:astrbotsender_name 仍为展示用户名 astrbot
  • 针对 Gemini 指出的 dashboard 注入风险:ChatRoute.chat() 现在只有在请求来自 /api/v1/ 且存在 OpenAPI 鉴权中间件写入的 g.api_key_id 时,才会读取 _sender_id/_sender_name。普通 dashboard /api/chat/send 会强制使用当前登录的 g.username,不会信任客户端 JSON 里的内部 sender 字段。已补 test_dashboard_chat_send_ignores_internal_sender_fields 覆盖。

关于 Sourcery 的两个 high-level 建议:username_resolve_open_username() 里已经做了 strip(),这次暂不增加字符集限制,避免破坏已有外部系统使用的 username;openapi:<key_id>:<username> 格式目前集中在 OpenAPI route helper 中生成,测试里显式断言该格式是为了锁定外部可观察行为,暂不额外抽公共测试常量。

@Soulter Soulter changed the title fix: 修复 OpenAPI chat 可冒充管理员身份 fix: fix openapi chat Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend area:webui The bug / feature is about webui(dashboard) of astrbot. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant