From e70d7946f077fe85a2bf351df4a86a41f31c3cf0 Mon Sep 17 00:00:00 2001 From: angri450 Date: Mon, 7 Sep 2026 18:54:41 +0800 Subject: [PATCH] fix: security & robustness hardening round (path traversal, cross-agent download, MCP lock, task refs, oauth XSS, memory cache release) Security: - workspace_api_path now rejects any path containing a `..` segment with 403 (previously only lstrip("/")'d, so `../../../etc/passwd` flowed through to the backend and surfaced as an internal 500; data was contained by BackendWorkspace.relative_to but the error path was noisy and defence-in-depth relied on a single check). - is_allowed_host_download_abs_path no longer returns True for *any* `/.octop/agents/` path: the allow branch is narrowed to the current agent's own workspace prefix, so a user with access to one (possibly shared) agent can no longer read another agent's workspace (memory.sqlite etc). - OAuth callback HTML escapes redirect_after / state_id before embedding them in the inline script (user-controlled redirect_after was an unescaped self-XSS vector). Robustness: - MCP tool sync wrapper (`_call`) no longer bypasses the shared per-server lock when a running event loop exists: it runs the locked coroutine on a dedicated thread (avoids deadlock vs run_coroutine_threadsafe when invoked on the loop thread, keeps the loop unblocked). - Background tasks (agent reload worker, deferred bootstrap, connector reload) now keep strong references in a task set with done-callbacks, per the asyncio requirement to hold a reference to every created task. - Memory dashboard cache closes/tears-down evicted and replaced Memory/Bridge instances defensively, and logs the real agent_id on eviction. --- src/octop/api/common/memory_client.py | 29 +++++++++++++++++-- src/octop/api/common/workspace.py | 4 +++ src/octop/api/routers/connectors.py | 21 ++++++++++---- src/octop/api/routers/providers.py | 3 ++ src/octop/infra/agents/manager.py | 11 +++++-- src/octop/infra/connectors/mcp_tool_cache.py | 15 +++++++++- .../infra/gateway/media/backend_files.py | 7 ++++- 7 files changed, 78 insertions(+), 12 deletions(-) diff --git a/src/octop/api/common/memory_client.py b/src/octop/api/common/memory_client.py index 34b2b7b0..57af4af0 100644 --- a/src/octop/api/common/memory_client.py +++ b/src/octop/api/common/memory_client.py @@ -86,18 +86,41 @@ def get_or_open( backend_config=backend_config, ) bridge = Bridge(memory) + # 2026-09-07 修复：替换旧实例（fingerprint 变化）时先释放旧资源 + old = self._entries.get(agent_id) + if old is not None and old[2] != fingerprint: + _close_memory_instances(old[0], old[1]) self._entries[agent_id] = (memory, bridge, fingerprint) while len(self._entries) > self._max_size: - _, evicted = self._entries.popitem(last=False) - logger.debug("memory dashboard cache evicted agent_id=%s", evicted) + evicted_id, evicted_val = self._entries.popitem(last=False) + _close_memory_instances(evicted_val[0], evicted_val[1]) + logger.debug("memory dashboard cache evicted agent_id=%s", evicted_id) return memory, bridge def invalidate(self, agent_id: str | None = None) -> None: with self._lock: if agent_id is None: + for _id, val in self._entries.items(): + _close_memory_instances(val[0], val[1]) self._entries.clear() else: - self._entries.pop(agent_id, None) + val = self._entries.pop(agent_id, None) + if val is not None: + _close_memory_instances(val[0], val[1]) + + +def _close_memory_instances(memory: Any, bridge: Any) -> None: + """防御性释放：harness-memory 无公开 teardown，sqlite 连接随 GC 关闭； + 兜底尝试 close/teardown（若未来版本新增），静默忽略异常。""" + for inst in (bridge, memory): + for name in ("close", "teardown", "aclose"): + fn = getattr(inst, name, None) + if callable(fn): + try: + fn() + except Exception: # noqa: BLE001 - 释放失败不影响主流程 + logger.debug("memory instance %s.%s failed", type(inst).__name__, name, exc_info=True) + break _CACHE = _MemoryCache() diff --git a/src/octop/api/common/workspace.py b/src/octop/api/common/workspace.py index fd3d2309..09547c66 100644 --- a/src/octop/api/common/workspace.py +++ b/src/octop/api/common/workspace.py @@ -94,6 +94,10 @@ def workspace_api_path(raw: str) -> str: text = raw.strip().replace("\\", "/") if not text or text == "/": return "." + # 拒绝路径穿越段（2026-09-07 修复：原只 lstrip("/")，`..` 段透传给后端 + # 触发 500 噪音；对齐 host_dirs/knowledge relpath 的既有拒绝纪律）。 + if any(seg == ".." for seg in text.split("/")): + raise OctopError(ErrorCode.FORBIDDEN, f"path traversal not allowed: {raw!r}") return text.lstrip("/") diff --git a/src/octop/api/routers/connectors.py b/src/octop/api/routers/connectors.py index 6980f2a1..3f15bef6 100644 --- a/src/octop/api/routers/connectors.py +++ b/src/octop/api/routers/connectors.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import html import json import logging import secrets @@ -414,6 +415,10 @@ def _credentials_preview(kind: str, creds: dict[str, Any]) -> dict[str, Any]: return preview +# 2026-09-07 修复：后台任务强引用集合（asyncio 要求保存引用防 GC 回收）。 +_BACKGROUND_TASKS: set[asyncio.Task] = set() + + def _schedule_connector_reload(server: Any, user_id: int, *, all_users: bool = False) -> None: assert server.app_runtime is not None @@ -426,7 +431,9 @@ async def _run() -> None: except Exception: logger.exception("background connector reload failed for user %s", user_id) - asyncio.create_task(_run()) + task = asyncio.create_task(_run(), name=f"connector-reload-user-{user_id}") + _BACKGROUND_TASKS.add(task) + task.add_done_callback(_BACKGROUND_TASKS.discard) def _can_manage_connector(inst: Any, user: Any) -> bool: @@ -1350,18 +1357,22 @@ async def oauth_callback( redirect = row.redirect_after or "/connectors" locale = resolve_request_locale(request) success_message = tr("connector.oauth.callback_success", locale) - html = f""" + # 2026-09-07 修复：redirect_after 为用户可控（OAuth start body），原样拼入 + # JS 字符串可被注入（self-XSS）。转义后再嵌入。 + redirect_esc = html.escape(redirect, quote=True) + state_esc = html.escape(row.state_id, quote=True) + html_doc = f"""

{success_message}

""" - return HTMLResponse(html) + return HTMLResponse(html_doc) @router.get("/connectors/oauth/pending/{state_id}", summary="Poll OAuth result") diff --git a/src/octop/api/routers/providers.py b/src/octop/api/routers/providers.py index c9c49634..090e2e21 100644 --- a/src/octop/api/routers/providers.py +++ b/src/octop/api/routers/providers.py @@ -13,6 +13,9 @@ from octop.api.deps import current_user, get_server, require_permission from octop.infra.agents.providers.model_flags import is_local_runtime_provider + +# 2026-09-07 修复：后台任务强引用集合（asyncio 要求保存引用防 GC 回收）。 +_BACKGROUND_TASKS: set[asyncio.Task] = set() from octop.infra.agents.providers.presets import load_provider_presets from octop.infra.agents.providers.probe import ( fetch_openai_compatible_models, diff --git a/src/octop/infra/agents/manager.py b/src/octop/infra/agents/manager.py index b1d0766f..bc2cc497 100644 --- a/src/octop/infra/agents/manager.py +++ b/src/octop/infra/agents/manager.py @@ -344,6 +344,9 @@ def __init__( self._history_backfills: dict[str, asyncio.Event] = {} self._reload_dirty: set[str] = set() self._reload_worker_running: dict[str, bool] = {} + # 2026-09-07 修复：后台任务强引用集合（asyncio 要求保存引用防 GC 回收—— + # 原 create_task 返回值只存 bool 标记，任务对象可能被回收导致 reload 静默失败）。 + self._background_tasks: set[asyncio.Task] = set() self._bootstrap_graph_refresh_pending: set[str] = set() # Chat user id used to resolve connectors when agent.user_id is NULL (shared agents). self._connector_user_override: dict[str, int] = {} @@ -573,10 +576,12 @@ async def create( self._repos.agent_repo.set_state(agent_id, "starting") row = self._repos.agent_repo.get(agent_id) assert row is not None - asyncio.create_task( + task = asyncio.create_task( self._complete_create_bootstrap(row), name=f"bootstrap-agent-{agent_id}", ) + self._background_tasks.add(task) + task.add_done_callback(self._background_tasks.discard) else: agent = await self._start_agent(row, init_workspace=True) if agent is not None and spec.template_name: @@ -2332,7 +2337,9 @@ def _schedule_reload(self, agent_id: str) -> None: if self._reload_worker_running.get(agent_id): return self._reload_worker_running[agent_id] = True - asyncio.create_task(self._reload_worker(agent_id), name=f"reload-agent-{agent_id}") + task = asyncio.create_task(self._reload_worker(agent_id), name=f"reload-agent-{agent_id}") + self._background_tasks.add(task) + task.add_done_callback(self._background_tasks.discard) async def _reload_worker(self, agent_id: str) -> None: try: diff --git a/src/octop/infra/connectors/mcp_tool_cache.py b/src/octop/infra/connectors/mcp_tool_cache.py index 6ae3cbbb..9fd6c11e 100644 --- a/src/octop/infra/connectors/mcp_tool_cache.py +++ b/src/octop/infra/connectors/mcp_tool_cache.py @@ -5,6 +5,7 @@ import asyncio import hashlib import json +import threading from typing import Any from langchain_core.tools import StructuredTool @@ -79,7 +80,19 @@ async def _run() -> Any: asyncio.get_running_loop() except RuntimeError: return asyncio.run(_run()) - return _tool.invoke(kwargs) + # 2026-09-07 修复：原代码在 running loop 存在时直接 `_tool.invoke(kwargs)` + # ——不持共享锁（共享 MCP 会话并发交错）且阻塞事件循环。 + # 改为独立线程起新 loop 跑带锁协程：持锁序列化 + 不阻塞当前 loop； + # 不用 run_coroutine_threadsafe（调用方恰在 loop 线程时会死锁）。 + box: dict[str, Any] = {} + + def _worker() -> None: + box["value"] = asyncio.run(_run()) + + t = threading.Thread(target=_worker, daemon=True) + t.start() + t.join() + return box["value"] st_kwargs: dict[str, Any] = { "name": name, diff --git a/src/octop/infra/gateway/media/backend_files.py b/src/octop/infra/gateway/media/backend_files.py index f26937ea..4e90bebe 100644 --- a/src/octop/infra/gateway/media/backend_files.py +++ b/src/octop/infra/gateway/media/backend_files.py @@ -357,7 +357,12 @@ def is_allowed_host_download_abs_path(path: str, *, workspace: Path) -> bool: pass if "/.octop/agents/" in norm: - return True + # 2026-09-07 修复：原对任意 /.octop/agents/ 路径放行（不校验归属）， + # 多用户/共享 agent 场景下可跨 agent 读他人工作区。收窄为当前 agent + # 自身 workspace 前缀（workspace 内部已由上方 relative_to 覆盖，此分支 + # 只是兜底同前缀场景）；其他 agent 目录显式拒绝。 + ws_norm = str(workspace.resolve()).replace("\\", "/").lower() + return norm == ws_norm or norm.startswith(ws_norm.rstrip("/") + "/") if is_allowed_host_temp_path(resolved): return True