fix(skill_manage): block cross-user writes on patch/edit/write_file/remove_file
Only delete had a DB owner check via _assert_agent_may_delete. The other four write actions only called ensure_custom_skill_is_editable, which just verifies the directory exists under skills/custom/ — it does not check owner_user_id. Any agent that knew a skill's name could overwrite or strip support files from another user's UI-uploaded skill. Generalise the helper to _assert_agent_may_modify (same three rules: no DB row / built-in owner=None / owner mismatch) and call it from edit, patch, delete, write_file, and remove_file. Hoist user_id + skill_store to the top of the lock block so the four newly-guarded branches stay one-liners and create/delete reuse the same handles. Behaviour preserved when skill_store is None (no DB) — matches the existing delete fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
xuhdandClaude Opus 4.7 committed
1 parent
4416348c51
commit
539a8bcec8
1 file changed
+33
-20
+33
-20
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user