diff --git a/offline-backend-20260512/backend/packages/harness/deerflow/tools/skill_manage_tool.py b/offline-backend-20260512/backend/packages/harness/deerflow/tools/skill_manage_tool.py index 13d9526a..61cd417a 100644 --- a/offline-backend-20260512/backend/packages/harness/deerflow/tools/skill_manage_tool.py +++ b/offline-backend-20260512/backend/packages/harness/deerflow/tools/skill_manage_tool.py @@ -134,27 +134,31 @@ def _default_agent_store() -> Any | None: return None -async def _assert_agent_may_delete(store: SkillStore, name: str, user_id: str) -> dict[str, Any]: - """Pre-flight ownership check before agent-delete touches the on-disk file. +async def _assert_agent_may_modify(store: SkillStore, name: str, user_id: str) -> dict[str, Any]: + """Pre-flight ownership check before an agent-driven write touches a custom skill. - LocalSkillStorage doesn't know about users — it deletes anything that - matches by name. Without this guard, a malicious or hallucinating agent - could destroy another user's skill just by knowing the name. We refuse - unless the DB confirms ``user_id`` owns the skill outright. + LocalSkillStorage operates by name only — patch/edit/write_file/remove_file + and delete all resolve a name to a directory without knowing who owns it. + Without this guard a malicious or hallucinating agent could overwrite or + destroy another user's skill (including UI-uploaded skills) just by + knowing the name. We refuse unless the DB confirms ``user_id`` owns the + skill outright. - Returns the DB record for use in the subsequent cascade step. Raises - ``ValueError`` (mapped to a tool error visible to the LLM) on refusal. + Returns the DB record so callers that need owner info (e.g. delete + cascade) can reuse it. Raises ``ValueError`` (mapped to a tool error + visible to the LLM) on refusal. """ record = await store.get_any(name) if record is None: # No DB row — could be a legacy/orphan skill. Refuse rather than - # delete blindly; admin can clean up via the Web UI. + # touch blindly; users should adopt it via /api/skills/reconcile-custom + # or an admin can clean it up via the Web UI. raise ValueError( - f"Refused: skill '{name}' has no ownership record. Ask a human to delete it via the Web UI." + f"Refused: skill '{name}' has no ownership record. Ask a human to adopt or manage it via the Web UI." ) owner = record.get("owner_user_id") if owner is None: - raise ValueError(f"Refused: skill '{name}' is built-in and cannot be deleted by an agent.") + raise ValueError(f"Refused: skill '{name}' is built-in and cannot be modified by an agent.") if owner != user_id: raise ValueError(f"Refused: skill '{name}' is owned by another user.") return record @@ -200,6 +204,10 @@ async def _skill_manage_impl( lock = _get_lock(name) thread_id = _get_thread_id(runtime) skill_storage = get_or_new_skill_storage() + # Resolved up-front so ownership checks in the write-action branches stay + # one-liners. Cheap when the DB is unconfigured (returns None). + user_id = get_effective_user_id() + skill_store = _default_skill_store() async with lock: if action == "create": @@ -224,14 +232,15 @@ async def _skill_manage_impl( # surfaces it (custom skills without a DB row are filtered out by # ``GET /api/skills``). Resolve the user_id *here* on the main # async stack — contextvars don't propagate into ``to_thread``. - store = _default_skill_store() - if store is not None: - await _ensure_skill_db_registered(store, name, get_effective_user_id()) + if skill_store is not None: + await _ensure_skill_db_registered(skill_store, name, user_id) await refresh_skills_system_prompt_cache_async() return f"Created custom skill '{name}'." if action == "edit": await _to_thread(skill_storage.ensure_custom_skill_is_editable, name) + if skill_store is not None: + await _assert_agent_may_modify(skill_store, name, user_id) if content is None: raise ValueError("content is required for edit.") await _to_thread(skill_storage.validate_skill_markdown_content, name, content) @@ -250,6 +259,8 @@ async def _skill_manage_impl( if action == "patch": await _to_thread(skill_storage.ensure_custom_skill_is_editable, name) + if skill_store is not None: + await _assert_agent_may_modify(skill_store, name, user_id) if find is None or replace is None: raise ValueError("find and replace are required for patch.") skill_file = skill_storage.get_custom_skill_file(name) @@ -277,11 +288,9 @@ async def _skill_manage_impl( # Ownership pre-check: LocalSkillStorage deletes purely by name, # so without this an agent could blow away another user's skill # just by knowing the name. Raise BEFORE touching the filesystem. - user_id = get_effective_user_id() - store = _default_skill_store() record: dict[str, Any] | None = None - if store is not None: - record = await _assert_agent_may_delete(store, name, user_id) + if skill_store is not None: + record = await _assert_agent_may_modify(skill_store, name, user_id) await _to_thread( skill_storage.delete_custom_skill, @@ -306,9 +315,9 @@ async def _skill_manage_impl( await _to_thread(forget, name) except Exception: logger.debug("Failed to forget usage entry for %r", name, exc_info=True) - if store is not None: + if skill_store is not None: try: - await store.delete(name, user_id) + await skill_store.delete(name, user_id) except Exception: logger.debug("Failed to delete DB row for %r", name, exc_info=True) agent_store = _default_agent_store() @@ -330,6 +339,8 @@ async def _skill_manage_impl( if action == "write_file": await _to_thread(skill_storage.ensure_custom_skill_is_editable, name) + if skill_store is not None: + await _assert_agent_may_modify(skill_store, name, user_id) if path is None or content is None: raise ValueError("path and content are required for write_file.") target = await _to_thread(skill_storage.ensure_safe_support_path, name, path) @@ -347,6 +358,8 @@ async def _skill_manage_impl( if action == "remove_file": await _to_thread(skill_storage.ensure_custom_skill_is_editable, name) + if skill_store is not None: + await _assert_agent_may_modify(skill_store, name, user_id) if path is None: raise ValueError("path is required for remove_file.") target = await _to_thread(skill_storage.ensure_safe_support_path, name, path)