Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
ProviderChatCompletionPaneland the refactoredProviderPageshare a lot of nearly identical provider workbench layout and styling; consider extracting a single reusable layout/composition (or usingProviderChatCompletionPanelinsideProviderPage) to avoid divergence and simplify future changes. - The new
flush_pending_bot_messagehelper is implemented twice (inchat.pyandlive_chat.py) with very similar logic; factoring this into a shared utility or method would reduce duplication and the risk of subtle behavioral drift between streaming and live chat paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ProviderChatCompletionPanel` and the refactored `ProviderPage` share a lot of nearly identical provider workbench layout and styling; consider extracting a single reusable layout/composition (or using `ProviderChatCompletionPanel` inside `ProviderPage`) to avoid divergence and simplify future changes.
- The new `flush_pending_bot_message` helper is implemented twice (in `chat.py` and `live_chat.py`) with very similar logic; factoring this into a shared utility or method would reduce duplication and the risk of subtle behavioral drift between streaming and live chat paths.
## Individual Comments
### Comment 1
<location path="dashboard/src/components/provider/ProviderChatCompletionPanel.vue" line_range="195" />
<code_context>
const emit = defineEmits(['update:modelValue'])
-
-const { tm } = useModuleI18n('features/provider')
-
-// 检测是否为手机端
</code_context>
<issue_to_address>
**issue (bug_risk):** Provider sources are never loaded in this panel, so it will render empty until another caller triggers `loadConfig`.
When this panel is used standalone (e.g. from Chat, ProviderSelector, or the provider dialog), it will show an empty source list unless some other view has already initialized the composable, making behavior order‑dependent.
Trigger `loadConfig()` when the panel mounts (or first becomes visible), for example:
```ts
import { onMounted } from 'vue';
onMounted(() => {
loadConfig();
});
```
If `loadConfig` is expensive, add a "loaded" flag in the composable to skip repeat fetches on subsequent mounts.
</issue_to_address>
### Comment 2
<location path="astrbot/dashboard/routes/live_chat.py" line_range="455" />
<code_context>
+ message_parts_to_save
+ )
+
+ try:
+ extracted_refs = self._extract_web_search_refs(
+ plain_text,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new flush logic by defining `flush_pending_bot_message` as a default no-op within the `try` block and using it directly instead of introducing a separate `pending_bot_message_flusher` variable and `None` checks.
You can simplify the new logic while keeping the behavior by dropping `pending_bot_message_flusher` entirely and making `flush_pending_bot_message` always defined inside the `try` block.
Instead of:
```python
try:
pending_bot_message_flusher = None
...
async def flush_pending_bot_message():
...
pending_bot_message_flusher = flush_pending_bot_message
...
if should_save:
saved_record = await flush_pending_bot_message()
...
finally:
try:
if pending_bot_message_flusher is not None:
await pending_bot_message_flusher()
except Exception as e:
...
```
you can do:
```python
try:
async def flush_pending_bot_message():
return None # default no-op; overridden below
chat_queue = webchat_queue_mgr.get_or_create_queue(session_id)
await chat_queue.put(...)
message_accumulator = BotMessageAccumulator()
agent_stats = {}
refs = {}
async def flush_pending_bot_message():
nonlocal message_accumulator, agent_stats, refs
if not (message_accumulator.has_content() or refs or agent_stats):
return None
message_parts_to_save = message_accumulator.build_message_parts(
include_pending_tool_calls=True
)
plain_text = collect_plain_text_from_message_parts(message_parts_to_save)
try:
extracted_refs = self._extract_web_search_refs(
plain_text,
message_parts_to_save,
)
except Exception as e:
logger.exception(
f"[Live Chat] Failed to extract web search refs: {e}",
exc_info=True,
)
extracted_refs = refs
saved_record = await self._save_bot_message(
session_id,
message_parts_to_save,
agent_stats,
extracted_refs,
llm_checkpoint_id,
)
message_accumulator = BotMessageAccumulator()
agent_stats = {}
refs = {}
return saved_record
while True:
if session.should_interrupt:
session.should_interrupt = False
await flush_pending_bot_message()
break
...
if should_save:
saved_record = await flush_pending_bot_message()
...
finally:
try:
await flush_pending_bot_message()
except Exception as e:
logger.exception(
f"[Live Chat] Failed to persist pending chat message: {e}",
exc_info=True,
)
session.is_processing = False
webchat_queue_mgr.remove_back_queue(message_id)
```
Benefits:
- Removes the `pending_bot_message_flusher` indirection and `None` checks.
- Keeps the safety property that the flush function is always defined, even if an exception occurs before the accumulator is initialized.
- Keeps all existing behavior (flush on interrupt, normal completion, and in `finally`).
Given you mentioned similar logic in `chat.py`, you might later extract this pattern into a small helper (e.g., a `BotMessageFlushContext` class that encapsulates `message_accumulator`, `agent_stats`, `refs`, and `flush()`), but even without that, this change locally reduces complexity without changing semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| }) | ||
|
|
||
| const { tm } = useModuleI18n('features/provider') |
There was a problem hiding this comment.
issue (bug_risk): Provider sources are never loaded in this panel, so it will render empty until another caller triggers loadConfig.
When this panel is used standalone (e.g. from Chat, ProviderSelector, or the provider dialog), it will show an empty source list unless some other view has already initialized the composable, making behavior order‑dependent.
Trigger loadConfig() when the panel mounts (or first becomes visible), for example:
import { onMounted } from 'vue';
onMounted(() => {
loadConfig();
});If loadConfig is expensive, add a "loaded" flag in the composable to skip repeat fetches on subsequent mounts.
| @@ -453,6 +453,7 @@ async def _handle_chat_message( | |||
| llm_checkpoint_id = str(uuid.uuid4()) | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new flush logic by defining flush_pending_bot_message as a default no-op within the try block and using it directly instead of introducing a separate pending_bot_message_flusher variable and None checks.
You can simplify the new logic while keeping the behavior by dropping pending_bot_message_flusher entirely and making flush_pending_bot_message always defined inside the try block.
Instead of:
try:
pending_bot_message_flusher = None
...
async def flush_pending_bot_message():
...
pending_bot_message_flusher = flush_pending_bot_message
...
if should_save:
saved_record = await flush_pending_bot_message()
...
finally:
try:
if pending_bot_message_flusher is not None:
await pending_bot_message_flusher()
except Exception as e:
...you can do:
try:
async def flush_pending_bot_message():
return None # default no-op; overridden below
chat_queue = webchat_queue_mgr.get_or_create_queue(session_id)
await chat_queue.put(...)
message_accumulator = BotMessageAccumulator()
agent_stats = {}
refs = {}
async def flush_pending_bot_message():
nonlocal message_accumulator, agent_stats, refs
if not (message_accumulator.has_content() or refs or agent_stats):
return None
message_parts_to_save = message_accumulator.build_message_parts(
include_pending_tool_calls=True
)
plain_text = collect_plain_text_from_message_parts(message_parts_to_save)
try:
extracted_refs = self._extract_web_search_refs(
plain_text,
message_parts_to_save,
)
except Exception as e:
logger.exception(
f"[Live Chat] Failed to extract web search refs: {e}",
exc_info=True,
)
extracted_refs = refs
saved_record = await self._save_bot_message(
session_id,
message_parts_to_save,
agent_stats,
extracted_refs,
llm_checkpoint_id,
)
message_accumulator = BotMessageAccumulator()
agent_stats = {}
refs = {}
return saved_record
while True:
if session.should_interrupt:
session.should_interrupt = False
await flush_pending_bot_message()
break
...
if should_save:
saved_record = await flush_pending_bot_message()
...
finally:
try:
await flush_pending_bot_message()
except Exception as e:
logger.exception(
f"[Live Chat] Failed to persist pending chat message: {e}",
exc_info=True,
)
session.is_processing = False
webchat_queue_mgr.remove_back_queue(message_id)Benefits:
- Removes the
pending_bot_message_flusherindirection andNonechecks. - Keeps the safety property that the flush function is always defined, even if an exception occurs before the accumulator is initialized.
- Keeps all existing behavior (flush on interrupt, normal completion, and in
finally).
Given you mentioned similar logic in chat.py, you might later extract this pattern into a small helper (e.g., a BotMessageFlushContext class that encapsulates message_accumulator, agent_stats, refs, and flush()), but even without that, this change locally reduces complexity without changing semantics.
There was a problem hiding this comment.
Code Review
This pull request refactors the dashboard's provider configuration UI by introducing the ProviderChatCompletionPanel component and updating the models and sources panels with improved styling and mobile support. It also enhances message persistence in the backend and updates the project's font and icon assets. Feedback focuses on addressing significant code and logic duplication between the new components and existing views, localizing hardcoded Chinese strings, and ensuring the Chat component preserves its state correctly during navigation by avoiding v-if when v-show is intended.
| <div v-if="selectedProviderType === 'chat_completion'" class="provider-workbench"> | ||
| <v-row class="provider-workbench__shell"> | ||
| <v-col cols="12" md="4" lg="3" class="provider-workbench__sources"> | ||
| <ProviderSourcesPanel | ||
| :displayed-provider-sources="displayedProviderSources" | ||
| :selected-provider-source="selectedProviderSource" | ||
| :available-source-types="availableSourceTypes" | ||
| :tm="tm" | ||
| :resolve-source-icon="resolveSourceIcon" | ||
| :get-source-display-name="getSourceDisplayName" | ||
| @add-provider-source="addProviderSource" | ||
| @select-provider-source="selectProviderSource" | ||
| @delete-provider-source="deleteProviderSource" | ||
| /> | ||
| </v-col> | ||
|
|
||
| <v-col cols="12" md="8" lg="9" class="provider-workbench__settings"> | ||
| <v-card class="provider-config-card provider-settings-panel h-100" elevation="0"> | ||
| <div v-if="selectedProviderSource" class="provider-config-header"> | ||
| <div class="provider-config-headline"> | ||
| <div class="provider-config-kicker">{{ tm('providers.settings') }}</div> | ||
| <div class="provider-config-title">{{ selectedProviderSource.id }}</div> | ||
| <div class="provider-config-subtitle"> | ||
| {{ selectedProviderSource.api_base || 'N/A' }} | ||
| </div> | ||
| <div class="provider-workbench__sidebar"> | ||
| <ProviderSourcesPanel | ||
| :displayed-provider-sources="displayedProviderSources" | ||
| :selected-provider-source="selectedProviderSource" | ||
| :available-source-types="availableSourceTypes" | ||
| :tm="tm" | ||
| :resolve-source-icon="resolveSourceIcon" | ||
| :get-source-display-name="getSourceDisplayName" | ||
| @add-provider-source="addProviderSource" | ||
| @select-provider-source="selectProviderSource" | ||
| @delete-provider-source="deleteProviderSource" | ||
| /> | ||
| </div> | ||
|
|
||
| <div class="provider-workbench__divider"></div> | ||
|
|
||
| <div class="provider-workbench__main"> | ||
| <div v-if="selectedProviderSource" class="provider-config-shell"> | ||
| <div class="provider-config-header"> | ||
| <div class="provider-config-headline"> | ||
| <div class="provider-config-title">{{ selectedProviderSource.id }}</div> | ||
| <div class="provider-config-subtitle"> | ||
| {{ selectedProviderSource.api_base || 'N/A' }} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="provider-config-actions"> | ||
| <v-btn | ||
| color="primary" | ||
| prepend-icon="mdi-content-save-outline" | ||
| :loading="savingSource" | ||
| :disabled="!isSourceModified" | ||
| @click="saveProviderSource" | ||
| variant="tonal" | ||
| > | ||
| {{ tm('providerSources.save') }} | ||
| </v-btn> | ||
| </div> | ||
| <div class="provider-config-actions"> | ||
| <v-btn | ||
| color="primary" | ||
| prepend-icon="mdi-content-save-outline" | ||
| :loading="savingSource" | ||
| :disabled="!isSourceModified" | ||
| variant="tonal" | ||
| rounded="xl" | ||
| @click="saveProviderSource" | ||
| > | ||
| {{ tm('providerSources.save') }} | ||
| </v-btn> | ||
| </div> | ||
| </div> | ||
|
|
||
| <v-divider></v-divider> | ||
|
|
||
| <v-card-text class="provider-config-body"> | ||
| <template v-if="selectedProviderSource"> | ||
| <section class="provider-section"> | ||
| <div class="provider-section-head"> | ||
| <div class="provider-section-title">{{ tm('providers.settings') }}</div> | ||
| </div> | ||
| <AstrBotConfig v-if="basicSourceConfig" :iterable="basicSourceConfig" :metadata="providerSourceSchema" | ||
| metadataKey="provider" :is-editing="true" /> | ||
| </section> | ||
|
|
||
| <section v-if="advancedSourceConfig" class="provider-section"> | ||
| <div class="provider-section-head"> | ||
| <div class="provider-section-title">{{ tm('providerSources.advancedConfig') }}</div> | ||
| </div> | ||
| <AstrBotConfig | ||
| :iterable="advancedSourceConfig" | ||
| :metadata="providerSourceSchema" | ||
| metadataKey="provider" | ||
| :is-editing="true" | ||
| /> | ||
| </section> | ||
|
|
||
| <section class="provider-section provider-section--models"> | ||
| <ProviderModelsPanel | ||
| :entries="filteredMergedModelEntries" | ||
| :available-count="availableModels.length" | ||
| v-model:model-search="modelSearch" | ||
| :loading-models="loadingModels" | ||
| :is-source-modified="isSourceModified" | ||
| :supports-image-input="supportsImageInput" | ||
| :supports-audio-input="supportsAudioInput" | ||
| :supports-tool-call="supportsToolCall" | ||
| :supports-reasoning="supportsReasoning" | ||
| :format-context-limit="formatContextLimit" | ||
| :testing-providers="testingProviders" | ||
| :tm="tm" | ||
| @fetch-models="fetchAvailableModels" | ||
| @open-manual-model="openManualModelDialog" | ||
| @open-provider-edit="openProviderEdit" | ||
| @toggle-provider-enable="toggleProviderEnable" | ||
| @test-provider="testProvider" | ||
| @delete-provider="deleteProvider" | ||
| @add-model-provider="addModelProvider" | ||
| /> | ||
| </section> | ||
| </template> | ||
| <div v-else class="provider-empty-state"> | ||
| <v-icon size="48" color="grey-lighten-1">mdi-cursor-default-click</v-icon> | ||
| <p class="mt-2">{{ tm('providerSources.selectHint') }}</p> | ||
| <div class="provider-config-body"> | ||
| <section class="provider-section"> | ||
| <div class="provider-section-head"> | ||
| <div class="provider-section-title">{{ tm('providers.settings') }}</div> | ||
| </div> | ||
| </v-card-text> | ||
| </v-card> | ||
| </v-col> | ||
| </v-row> | ||
| <AstrBotConfig | ||
| v-if="basicSourceConfig" | ||
| :iterable="basicSourceConfig" | ||
| :metadata="providerSourceSchema" | ||
| metadataKey="provider" | ||
| :is-editing="true" | ||
| /> | ||
| </section> | ||
|
|
||
| <v-divider v-if="advancedSourceConfig"></v-divider> | ||
|
|
||
| <section v-if="advancedSourceConfig" class="provider-section"> | ||
| <div class="provider-section-head"> | ||
| <div class="provider-section-title">{{ tm('providerSources.advancedConfig') }}</div> | ||
| </div> | ||
| <AstrBotConfig | ||
| :iterable="advancedSourceConfig" | ||
| :metadata="providerSourceSchema" | ||
| metadataKey="provider" | ||
| :is-editing="true" | ||
| /> | ||
| </section> | ||
|
|
||
| <v-divider></v-divider> | ||
|
|
||
| <section class="provider-section provider-section--models"> | ||
| <ProviderModelsPanel | ||
| :entries="filteredMergedModelEntries" | ||
| :available-count="availableModels.length" | ||
| v-model:model-search="modelSearch" | ||
| :loading-models="loadingModels" | ||
| :is-source-modified="isSourceModified" | ||
| :supports-image-input="supportsImageInput" | ||
| :supports-audio-input="supportsAudioInput" | ||
| :supports-tool-call="supportsToolCall" | ||
| :supports-reasoning="supportsReasoning" | ||
| :format-context-limit="formatContextLimit" | ||
| :testing-providers="testingProviders" | ||
| :tm="tm" | ||
| @fetch-models="fetchAvailableModels" | ||
| @open-manual-model="openManualModelDialog" | ||
| @open-provider-edit="openProviderEdit" | ||
| @toggle-provider-enable="toggleProviderEnable" | ||
| @test-provider="testProvider" | ||
| @delete-provider="deleteProvider" | ||
| @add-model-provider="addModelProvider" | ||
| /> | ||
| </section> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div v-else class="provider-empty-state"> | ||
| <v-icon size="48" color="grey-lighten-1">mdi-cursor-default-click</v-icon> | ||
| <p class="mt-2">{{ tm('providerSources.selectHint') }}</p> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The logic and template for the chat_completion tab are heavily duplicated between this file and the newly created ProviderChatCompletionPanel.vue. This makes maintenance difficult. Please refactor this section to use the ProviderChatCompletionPanel component directly.
<div v-if="selectedProviderType === 'chat_completion'" class="provider-workbench">
<ProviderChatCompletionPanel :show-border="false" />
</div>
| <v-btn variant="text" @click="showManualModelDialog = false">取消</v-btn> | ||
| <v-btn color="primary" @click="confirmManualModel">添加</v-btn> |
There was a problem hiding this comment.
These button labels are hardcoded in Chinese. Please use the existing i18n keys for consistency and localization support.
<v-btn variant="text" @click="showManualModelDialog = false">{{ tm('dialogs.config.cancel') }}</v-btn>
<v-btn color="primary" @click="confirmManualModel">{{ tm('dialogs.config.save') }}</v-btn>
| <v-dialog v-model="showProviderEditDialog" width="800"> | ||
| <v-card :title="providerEditData?.id || tm('dialogs.config.editTitle')"> | ||
| <v-card-text class="py-4"> | ||
| <small style="color: gray;">不建议修改 ID,可能会导致指向该模型的相关配置(如默认模型、插件相关配置等)失效。旧版本 AstrBot 的 “提供商 ID” 是下方的 “ID”。</small> |
| showMessage(res.data.message || '更新失败!', 'error') | ||
| return | ||
| } | ||
| showMessage(res.data.message || "更新成功!") | ||
| showMessage(res.data.message || '更新成功!') | ||
| if (wasUpdating) { | ||
| updatingMode.value = false | ||
| } | ||
| } else { | ||
| const res = await axios.post('/api/config/provider/new', newSelectedProviderConfig.value) | ||
| if (res.data.status === 'error') { | ||
| showMessage(res.data.message || "添加失败!", 'error') | ||
| showMessage(res.data.message || '添加失败!', 'error') | ||
| return | ||
| } | ||
| showMessage(res.data.message || "添加成功!") | ||
| showMessage(res.data.message || '添加成功!') |
| <div><strong>{{ tm('models.tooltips.providerId') }}:</strong> {{ entry.provider.id }}</div> | ||
| <div><strong>{{ tm('models.tooltips.modelId') }}:</strong> {{ entry.provider.model }}</div> |
There was a problem hiding this comment.
In Vuetify 3, it is recommended to wrap multiple elements within a tooltip's content in a single container (like a div) to ensure consistent layout and styling within the tooltip bubble.
<div>
<div><strong>{{ tm('models.tooltips.providerId') }}:</strong> {{ entry.provider.id }}</div>
<div><strong>{{ tm('models.tooltips.modelId') }}:</strong> {{ entry.provider.model }}</div>
</div>
| @@ -1,5 +1,6 @@ | |||
| <template> | |||
| <div | |||
| v-if="props.active" | |||
There was a problem hiding this comment.
Using v-if="props.active" here causes the component's internal state (like message drafts or scroll position) to be destroyed when navigating away from the chat route. If the intention of the changes in FullLayout.vue was to preserve state using v-show, this v-if should be removed or changed to v-show.
| async function saveEditedProvider() { | ||
| if (!providerEditData.value) return | ||
|
|
||
| savingProviders.value.push(providerEditData.value.id) | ||
| try { | ||
| const res = await axios.post('/api/config/provider/update', { | ||
| id: providerEditOriginalId.value || providerEditData.value.id, | ||
| config: providerEditData.value | ||
| }) | ||
|
|
||
| if (res.data.status === 'error') { | ||
| throw new Error(res.data.message) | ||
| } | ||
|
|
||
| showMessage(res.data.message || tm('providerSources.saveSuccess')) | ||
| showProviderEditDialog.value = false | ||
| await loadConfig() | ||
| } catch (err) { | ||
| showMessage(err.response?.data?.message || err.message || tm('providerSources.saveError'), 'error') | ||
| } finally { | ||
| savingProviders.value = savingProviders.value.filter(id => id !== providerEditData.value?.id) | ||
| } | ||
| } |
Mola-maker
left a comment
There was a problem hiding this comment.
🔴 严重:路径遍历防护被削弱(回归风险)
位置:get_file 第 278-308 行、get_attachment 第 310-327 行
get_file 的写法:
file_path = os.path.join(self.attachments_dir, os.path.basename(filename))
real_file_path = os.path.realpath(file_path)
real_imgs_dir = os.path.realpath(self.attachments_dir)
...
if not real_file_path.startswith(real_imgs_dir):
return Response().error("Invalid file path").__dict__三个并发问题:
1. startswith 校验有经典绕过。 如果 real_imgs_dir = "/data/attachments",那么 /data/attachments_evil/xxx 会通过 startswith 校验但并不在目标目录下。正确写法是 Path(real_file_path).is_relative_to(Path(real_imgs_dir))(Python 3.9+),或者拼 os.sep 作为前缀。实际上这个仓库 attachments_dir 是 data/attachments 是确定的,目前没有平级同前缀目录,利用不了,但这是随时会爆的地雷。
2. get_attachment 完全没做路径校验:
attachment = await self.db.get_attachment_by_id(attachment_id)
...
file_path = attachment.path
real_file_path = os.path.realpath(file_path)
return await send_file(real_file_path, mimetype=attachment.mime_type)直接把 attachment.path(来自数据库)喂给 send_file。风险路径:如果 post_file 或其他入口允许把任意 path 写入 attachments 表(比如插件、迁移脚本、未来的批量导入),这里就能读任意文件。虽然 post_file 现在是从 attachments_dir 生成 path,但这依赖"永远只有 post_file 一个入口"这个脆弱假设。纵深防御的话这里必须加一次 is_relative_to(attachments_dir)。
3. post_file 保留原始 file.filename(第 336、349 行):
filename = file.filename or f"{uuid.uuid4()!s}"
...
path = os.path.join(self.attachments_dir, filename)
await file.save(path)这里就是真正的洞。file.filename 完全是客户端可控的。如果客户端上传时把 filename 设为 ../../../data/config/cmd_config.json,os.path.join 在 Unix 下 不会像绝对路径那样跳出(因为不是 / 开头),但 .. 会被 save 和后续的 realpath 解析。实测一下:
>>> os.path.join("/data/attachments", "../../../etc/passwd")
'/data/attachments/../../../etc/passwd'
>>> os.path.realpath(_)
'/etc/passwd'
这意味着攻击者可以任意覆盖文件系统上 AstrBot 进程有写权限的文件(数据库、config、plugins 下的代码)。然后覆盖插件代码就是 RCE。
这里必须加 filename = os.path.basename(filename) 或者直接用 uuid 生成。对比 get_file 里用了 basename,post_file 却没用——很明显是重构时的疏漏。
这一条我认为是 P0 级安全漏洞,严重程度超过历史上 CVE-2025-48957(那个只能读文件,这个能写文件进而 RCE)。
🟡 中等:running_convs 的竞争状态
位置:第 276、828、1269、1390 行
self.running_convs: dict[str, bool] = {}
# track_conversation (第 36)
convs[conv_id] = True
try:
yield
finally:
convs.pop(conv_id, None)webchat_conv_id = session_id(chat 里第 744 行 webchat_conv_id = session_id),也等于 thread 场景下的 thread_id(send_thread_message 第 1413 行传的 session_id = thread.thread_id)。
场景:同一个用户在两个 tab 同时对同一个 session 发消息。
- Tab A 进入
track_conversation→convs[sid] = True - Tab B 进入 →
convs[sid] = True(覆盖,但值一样) - Tab A 提前结束 →
convs.pop(sid)把 Tab B 的标记也抹了 - 此时
get_session返回is_running=False(第 1269 行),前端以为空闲,允许用户再发一条 → 多路请求同时处理,队列错乱
另一个更实际的场景:get_or_create_back_queue(message_id, webchat_conv_id) 按 message_id 建队列(第 758 行,message_id = uuid.uuid4() 第 755 行),但 running_convs 只按 webchat_conv_id 跟踪。也就是说同一个 session 的两个并发请求各有自己的 back_queue,但共用一个 running 标记。前端看到的"是否在跑"信息和后端实际状态不一致。
不是阻塞 bug,但是并发下会出很诡异的状态。
🟡 中等:finally 里 flush 会吞掉异常但不通知客户端
位置:第 945-955 行
except BaseException as e:
logger.exception(f"WebChat stream unexpected error: {e}", exc_info=True)
finally:
try:
await flush_pending_bot_message()
except Exception as e:
logger.exception(
f"Failed to persist pending webchat message: {e}",
exc_info=True,
)
webchat_queue_mgr.remove_back_queue(message_id)两个子问题:
1. except BaseException 抓住了 asyncio.CancelledError,然后静默写日志。在 Python 3.8+ asyncio 的规范里,CancelledError 必须向上传播,否则任务取消语义就坏了(上层 gather/wait_for 的 cancellation 会被卡死)。应该写成:
except asyncio.CancelledError:
raise
except Exception as e:
logger.exception(...)2. 客户端断连后的 flush 发生在 finally 块,此时 saved_record 已经无法通过 SSE 发给前端(连接已断)。这是符合设计的——"断连也要落库"——但前端重连/刷新时如何发现这条消息?依赖下次 get_session 拉取历史。这期间如果前端在断连后立刻刷新了,会看到落库的消息;如果先切了 session 再切回来,没问题;但如果在别的地方继续和同一个 session 对话(比如刚好服务端还在写,前端已经发了新消息进来),新消息可能插到还没落库的那条之前——保存顺序不等于时间顺序。这个风险存在但不严重,因为 platform_history 按 created_at 排序。
🟡 中等:add_plain 在非流式下的覆盖行为
位置:第 152-155 行
if streaming:
self.pending_text += result_text
else:
self.pending_text = result_text非流式分支是覆盖。触发条件:上游在 streaming=False 时连续发两条 msg_type="plain" 且中间没有 flush(即中间没有来一条 tool_call/reasoning/image 等会触发 flush 的消息)。
看 stream 主循环第 916-924 行:
should_save = False
if msg_type == "end":
should_save = ...
elif (streaming and msg_type == "complete") or not streaming:
if chain_type not in ("tool_call", "tool_call_result"):
should_save = Truenot streaming 时每条 plain 消息都会 should_save=True → 走 flush_pending_bot_message → 保存并重置 accumulator。所以单次 save 内不会丢消息,但是:如果一个回合的后端逻辑在非流式模式下本该输出 "段 A 然后段 B"(两次调用 add_plain),中间没 flush(因为两次都是 plain 类型),第一次 pending_text = A,第二次 pending_text = B,A 丢了。
这种情况在当前代码流里是否会发生,取决于上游 webchat adapter 怎么推消息。如果 adapter 对非流式也是"一个回合只推一次 plain",则没事;如果会拆分推送(例如先发工具描述,再发最终回答),就会丢一半。我没有 adapter 代码没法最终判断,但是这个写法就是在给未来埋雷——add_plain 应该永远是追加,或者在 streaming=False 时先 flush 再覆盖。
🟢 小问题
1. _delete_attachments 异常处理吞太宽(第 1155-1175 行):文件删除失败只 warning,不回滚 DB。结果是文件还在磁盘但 DB 记录没了——孤儿文件永远清不掉。
2. get_session 在 session 为 None 时仍然返回 history(第 1236-1280 行):
session = await self.db.get_platform_session_by_id(session_id)
platform_id = session.platform_id if session else "webchat"如果 session 不存在,默认当 webchat 查 history。没做 creator 鉴权——任意登录用户可以拿到任意 session_id 的历史记录(前提是他猜到 session_id)。session_id 是 UUID 不好猜,但这和其他接口(update_session_display_name / delete_webchat_session)对 creator 的严格校验不一致。最小化修复:session 不存在直接返回 404,不要 fallback。
3. new_session 接受任意 platform_id 查询参数(第 1182 行):
platform_id = request.args.get("platform_id", "webchat")没白名单校验。用户可以传任何字符串进来,比如 platform_id=aaa,然后创建一个 platform_id="aaa" 的 session。这在数据库层和下游消息处理里可能造成各种 "unknown platform" 错误。应该校验在已注册的 platform 列表里。
4. _extract_attachment_ids 对 history.content 不做类型校验(第 1142-1153 行):
content = history.content
if not content or "message" not in content:
continue如果 content 是 list 或 str(老数据),"message" not in content 对 str 是子串匹配,对 list 就是元素匹配。会在某些老数据下静默跳过本该处理的记录。应该先 isinstance(content, dict)。
总结和优先级
| 优先级 | 问题 | 位置 |
|---|---|---|
| P0 | post_file 未 basename 化 filename → 任意文件写入 → RCE |
第 336/349 |
| P1 | get_attachment 无路径校验、startswith 前缀绕过 |
第 297/310-327 |
| P2 | except BaseException 吞掉 CancelledError |
第 945 |
| P2 | get_session 未鉴权 |
第 1236 |
| P2 | running_convs 并发竞态 |
第 276/1269 |
| P3 | add_plain 非流式覆盖语义 |
第 152-155 |
| P3 | new_session 未校验 platform_id |
第 1182 |
| P3 | _extract_attachment_ids 类型假设 |
第 1142 |
| P3 |
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
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.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Revamp the provider configuration experience across the dashboard, add an integrated provider workspace to the chat UI, harden streaming message persistence on the backend, and update typography and icon assets.
New Features:
Bug Fixes:
Enhancements: