diff --git a/CHANGELOG.md b/CHANGELOG.md index 3950969def..e0e4b1035c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -323,6 +323,15 @@ This release closes that milestone with **181 merged pull requests**. ### Fixed +- **skills:** `skill_manage(action="remove_file")` and `write_file` now work on + binary support files, and reject directories cleanly. A `.skill` archive may + carry `assets/logo.png` (the installer only rejects *executable* binaries), + but both actions read the previous content as UTF-8 text — purely for the + history record — before touching the file, so a binary member raised + `UnicodeDecodeError` and was never removed or overwritten. A bare support + directory such as `assets` also slipped through path validation and raised + `IsADirectoryError`. Non-text content is now recorded as no previous text, + and a directory path is a validation error instead of a crash. ([#5893]) - **projects:** The conversation-files view no longer shows an empty heading for a member thread that has no title yet. A thread's `display_name` is `null` on the wire until title generation has run (or if it never does), but @@ -6110,4 +6119,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5859]: https://github.com/bytedance/deer-flow/pull/5859 [#5879]: https://github.com/bytedance/deer-flow/pull/5879 [#5881]: https://github.com/bytedance/deer-flow/pull/5881 +[#5893]: https://github.com/bytedance/deer-flow/pull/5893 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index de7334c425..bca5d78285 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -328,6 +328,12 @@ ### 修复 +- **技能：** `skill_manage(action="remove_file")` 与 `write_file` 现在可以处理二进制支持文件， + 并会干净地拒绝目录。`.skill` 压缩包可以包含 `assets/logo.png`（安装器只拒绝*可执行*二进制）， + 但这两个操作在改动文件之前会先把原有内容按 UTF-8 文本读出（仅用于历史记录），于是二进制 + 成员会抛出 `UnicodeDecodeError`，文件既不会被删除也不会被覆盖。像 `assets` 这样的裸支持目录 + 也能通过路径校验并抛出 `IsADirectoryError`。现在非文本内容会记录为“无原有文本”，目录路径 + 则作为校验错误返回，而不是崩溃。([#5893]) - **项目：** 会话文件视图不再为尚无标题的成员会话显示空标题。会话的 `display_name` 在标题 生成运行之前（或从未运行时）在接口上为 `null`，但文件分组类型将其声明为必填字符串并原样 渲染，因此在首次回复之前上传的文件会挂在一行空白之下。这类分组现在显示为“未命名”，与 @@ -5198,3 +5204,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5859]: https://github.com/bytedance/deer-flow/pull/5859 [#5879]: https://github.com/bytedance/deer-flow/pull/5879 [#5881]: https://github.com/bytedance/deer-flow/pull/5881 +[#5893]: https://github.com/bytedance/deer-flow/pull/5893 diff --git a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py index 003310a151..8a1378f0d7 100644 --- a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py @@ -111,7 +111,7 @@ class LocalSkillStorage(SkillStorage): if tmp_path is not None: tmp_path.unlink(missing_ok=True) - def remove_custom_skill_file(self, name: str, relative_path: str) -> str: + def remove_custom_skill_file(self, name: str, relative_path: str) -> str | None: removal = ((SkillCategory.CUSTOM, Path(name)),) with self._skill_projection_mutation(remove=removal): return super().remove_custom_skill_file(name, relative_path) diff --git a/backend/packages/harness/deerflow/skills/storage/skill_storage.py b/backend/packages/harness/deerflow/skills/storage/skill_storage.py index 5010a58194..8db36e9ae3 100644 --- a/backend/packages/harness/deerflow/skills/storage/skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/skill_storage.py @@ -17,6 +17,19 @@ logger = logging.getLogger(__name__) _SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") +def read_text_or_none(path: Path) -> str | None: + """Return *path*'s content as UTF-8 text, or ``None`` when it is not text. + + For history records only: a skill's support files may be binary (the + installer rejects executable binaries, not images or other assets), and + recording "no previous text" must never abort the mutation being logged. + """ + try: + return path.read_bytes().decode("utf-8") + except UnicodeDecodeError: + return None + + class SkillStorage(ABC): """Abstract base for skill storage backends. @@ -96,7 +109,13 @@ class SkillStorage(ABC): return len(relative_parent.parts) == 1 and skill_file.parent.resolve().is_dir() def ensure_safe_support_path(self, name: str, relative_path: str) -> Path: - """Validate and return the resolved absolute path for a support file.""" + """Validate and return the resolved absolute path for a support file. + + The path must name a file *inside* one of the support directories: a + bare ``assets`` resolves to its own allowed root and would otherwise + pass the containment check, letting callers try to remove or overwrite + a directory. + """ _ALLOWED_SUPPORT_SUBDIRS = {"references", "templates", "scripts", "assets"} skill_dir = self.get_custom_skill_dir(self.validate_skill_name(name)).resolve() if not relative_path or relative_path.endswith("/"): @@ -109,6 +128,8 @@ class SkillStorage(ABC): top_level = relative.parts[0] if relative.parts else "" if top_level not in _ALLOWED_SUPPORT_SUBDIRS: raise ValueError(f"Supporting files must live under one of: {', '.join(sorted(_ALLOWED_SUPPORT_SUBDIRS))}.") + if len(relative.parts) < 2: + raise ValueError(f"Supporting file path must name a file inside '{top_level}/'.") target = (skill_dir / relative).resolve() allowed_root = (skill_dir / top_level).resolve() try: @@ -181,12 +202,20 @@ class SkillStorage(ABC): Origin: ``deerflow.skills.manager.atomic_write``. """ - def remove_custom_skill_file(self, name: str, relative_path: str) -> str: - """Remove a supporting file and return its previous text content.""" + def remove_custom_skill_file(self, name: str, relative_path: str) -> str | None: + """Remove a supporting file and return its previous text content. + + Returns ``None`` when the file was not UTF-8 text (a binary asset such + as ``assets/logo.png`` from a ``.skill`` archive): the previous content + only feeds the history record, so it must never block the removal. + A directory is rejected rather than removed. + """ target = self.ensure_safe_support_path(name, relative_path) + if target.is_dir(): + raise ValueError(f"Supporting file path '{relative_path}' is a directory, not a file.") if not target.exists(): raise FileNotFoundError(f"Supporting file '{relative_path}' not found for skill '{name}'.") - previous_content = target.read_text(encoding="utf-8") + previous_content = read_text_or_none(target) target.unlink() return previous_content diff --git a/backend/packages/harness/deerflow/tools/skill_manage_tool.py b/backend/packages/harness/deerflow/tools/skill_manage_tool.py index 779de1bc40..2e8b9b16a5 100644 --- a/backend/packages/harness/deerflow/tools/skill_manage_tool.py +++ b/backend/packages/harness/deerflow/tools/skill_manage_tool.py @@ -23,7 +23,7 @@ from deerflow.skills.security_static_scanner import ( enforce_static_scan, ) from deerflow.skills.storage import get_or_new_user_skill_storage -from deerflow.skills.storage.skill_storage import SkillStorage +from deerflow.skills.storage.skill_storage import SkillStorage, read_text_or_none from deerflow.skills.types import SKILL_MD_FILE from deerflow.tools.sync import make_sync_tool_wrapper from deerflow.tools.types import Runtime @@ -223,8 +223,12 @@ async def _skill_manage_impl( 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) + if await _to_thread(target.is_dir): + raise ValueError(f"Supporting file path '{path}' is a directory, not a file.") exists = await _to_thread(target.exists) - prev_content = await _to_thread(target.read_text, encoding="utf-8") if exists else None + # A binary asset (e.g. from a .skill archive) has no previous *text*; + # the history record takes None rather than aborting the write. + prev_content = await _to_thread(read_text_or_none, target) if exists else None executable = "scripts/" in path or path.startswith("scripts/") static_findings = await _scan_static_candidate_or_raise(name, {path: content}, skill_storage) scan = await _scan_or_raise(content, executable=executable, location=f"{name}/{path}", static_findings=static_findings) diff --git a/backend/tests/test_skill_manage_tool.py b/backend/tests/test_skill_manage_tool.py index eb5462fd8b..98dc3e31b9 100644 --- a/backend/tests/test_skill_manage_tool.py +++ b/backend/tests/test_skill_manage_tool.py @@ -252,6 +252,79 @@ def test_skill_manage_remove_file_updates_sandbox_projection_before_return(monke assert not projected_file.exists() +def _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path): + skills_root = tmp_path / "skills" + config = _make_config(skills_root) + monkeypatch.setattr("deerflow.config.get_app_config", lambda: config) + monkeypatch.setattr("deerflow.skills.security_scanner.get_app_config", lambda: config) + from deerflow.config.paths import Paths + + monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + + async def _refresh(user_id: str): + return None + + monkeypatch.setattr(skill_manage_module, "refresh_user_skills_system_prompt_cache_async", _refresh) + monkeypatch.setattr(skill_manage_module, "scan_skill_content", lambda *args, **kwargs: _async_result("allow", "ok")) + runtime = _make_runtime(user_id="default") + anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "create", "demo-skill", _skill_content("demo-skill")) + # A binary support file as a .skill archive install would leave it (the + # installer only rejects *executable* binaries). + skill_dir = tmp_path / "users" / "default" / "skills" / "custom" / "demo-skill" + (skill_dir / "assets" / "nested").mkdir(parents=True) + (skill_dir / "assets" / "logo.png").write_bytes(b"\x89PNG\r\n\x1a\n\xff\xfe\x00") + (skill_dir / "assets" / "nested" / "inner.txt").write_text("inner", encoding="utf-8") + return runtime, skill_dir + + +def _history_records(skill_dir: Path) -> list[dict]: + import json + + history_file = skill_dir.parent.parent / "history" / "demo-skill.jsonl" + if not history_file.exists(): # storage layouts differ; fall back to a search + history_file = next(p for p in skill_dir.parent.parent.rglob("*.jsonl") if "demo-skill" in p.name) + return [json.loads(line) for line in history_file.read_text(encoding="utf-8").splitlines() if line.strip()] + + +def test_skill_manage_remove_file_handles_a_binary_support_file(monkeypatch, tmp_path): + runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path) + + result = anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "remove_file", "demo-skill", None, "assets/logo.png") + + assert result == "Removed 'assets/logo.png' from custom skill 'demo-skill'." + assert not (skill_dir / "assets" / "logo.png").exists() + record = _history_records(skill_dir)[-1] + assert record["action"] == "remove_file" + assert record["file_path"] == "assets/logo.png" + assert record["prev_content"] is None + + +def test_skill_manage_write_file_can_overwrite_a_binary_support_file(monkeypatch, tmp_path): + runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path) + + result = anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "write_file", "demo-skill", "now text", "assets/logo.png") + + assert result == "Wrote 'assets/logo.png' for custom skill 'demo-skill'." + assert (skill_dir / "assets" / "logo.png").read_text(encoding="utf-8") == "now text" + record = _history_records(skill_dir)[-1] + assert record["action"] == "write_file" + assert record["prev_content"] is None + assert record["new_content"] == "now text" + + +@pytest.mark.parametrize("action, content", [("remove_file", None), ("write_file", "x")]) +@pytest.mark.parametrize("path", ["assets", "assets/nested"], ids=["bare-subdir", "nested-dir"]) +def test_skill_manage_rejects_a_support_directory_path(monkeypatch, tmp_path, action, content, path): + runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path) + + with pytest.raises(ValueError, match="file"): + anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, action, "demo-skill", content, path) + + assert (skill_dir / "assets" / "logo.png").exists() + assert (skill_dir / "assets" / "nested" / "inner.txt").read_text(encoding="utf-8") == "inner" + + def test_skill_manage_static_critical_blocks_create_before_llm(monkeypatch, tmp_path): skills_root = tmp_path / "skills" config = _make_config(skills_root) diff --git a/backend/tests/test_skill_storage_support_files.py b/backend/tests/test_skill_storage_support_files.py new file mode 100644 index 0000000000..56a5eb4a86 --- /dev/null +++ b/backend/tests/test_skill_storage_support_files.py @@ -0,0 +1,89 @@ +"""Support-file removal must work for binary members and reject directories. + +``.skill`` archives may carry binary support files (``assets/logo.png``): the +installer only rejects executable binaries by magic bytes. The storage's +``remove_custom_skill_file`` read the file as UTF-8 text *before* unlinking +it, purely to hand the previous content to the history record, so a binary +member raised ``UnicodeDecodeError`` and was never removed, and a support +*directory* (which ``ensure_safe_support_path`` let through because a bare +``assets`` resolves to its own allowed root) raised ``IsADirectoryError``. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from deerflow.skills.storage import get_or_new_skill_storage, reset_skill_storage +from deerflow.skills.storage.skill_storage import read_text_or_none + +PNG_BYTES = b"\x89PNG\r\n\x1a\n\xff\xfe\x00" + + +@pytest.fixture(autouse=True) +def _reset_storages(): + reset_skill_storage() + yield + reset_skill_storage() + + +@pytest.fixture() +def storage(tmp_path: Path): + return get_or_new_skill_storage(skills_path=str(tmp_path)) + + +@pytest.fixture() +def skill_dir(tmp_path: Path, storage) -> Path: + d = tmp_path / "custom" / "demo-skill" + (d / "assets" / "nested").mkdir(parents=True) + (d / "SKILL.md").write_text("---\nname: demo-skill\ndescription: x\n---\nbody\n", encoding="utf-8") + (d / "assets" / "logo.png").write_bytes(PNG_BYTES) + (d / "assets" / "notes.md").write_text("notes", encoding="utf-8") + (d / "assets" / "nested" / "inner.txt").write_text("inner", encoding="utf-8") + return d + + +def test_remove_binary_support_file_removes_it_and_reports_no_text(storage, skill_dir): + previous = storage.remove_custom_skill_file("demo-skill", "assets/logo.png") + + assert previous is None + assert not (skill_dir / "assets" / "logo.png").exists() + + +def test_remove_text_support_file_still_returns_its_content(storage, skill_dir): + previous = storage.remove_custom_skill_file("demo-skill", "assets/notes.md") + + assert previous == "notes" + assert not (skill_dir / "assets" / "notes.md").exists() + + +@pytest.mark.parametrize("path", ["assets", "assets/", "assets/nested"], ids=["bare-subdir", "bare-subdir-slash", "nested-dir"]) +def test_remove_rejects_a_directory_and_leaves_it_in_place(storage, skill_dir, path): + with pytest.raises(ValueError, match="file"): + storage.remove_custom_skill_file("demo-skill", path) + + assert (skill_dir / "assets" / "nested" / "inner.txt").read_text(encoding="utf-8") == "inner" + assert (skill_dir / "assets" / "logo.png").read_bytes() == PNG_BYTES + + +def test_ensure_safe_support_path_requires_a_filename_below_the_subdir(storage, skill_dir): + with pytest.raises(ValueError, match="file"): + storage.ensure_safe_support_path("demo-skill", "assets") + assert storage.ensure_safe_support_path("demo-skill", "assets/logo.png") == (skill_dir / "assets" / "logo.png").resolve() + + +def test_read_text_or_none_decodes_text_and_returns_none_for_binary(tmp_path: Path): + text = tmp_path / "t.md" + text.write_text("héllo", encoding="utf-8") + binary = tmp_path / "b.png" + binary.write_bytes(PNG_BYTES) + + assert read_text_or_none(text) == "héllo" + assert read_text_or_none(binary) is None + + +def test_read_text_or_none_only_masks_undecodable_content(tmp_path: Path): + """``None`` means "not text", never "could not read": IO errors still propagate.""" + with pytest.raises(FileNotFoundError): + read_text_or_none(tmp_path / "missing.md")