From a1eb3971412ee269f1f89bfb5cc9e158b682f86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=93=88=E5=9F=BA=E7=B1=B3?= <140241684+BlueX888@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:28:20 +0800 Subject: [PATCH] fix(skills): treat only top-level scripts/ paths as executable in skill_manage (#7088) * fix(skills): treat only top-level scripts/ paths as executable in skill_manage write_file classified a support file as executable content with a substring test ("scripts/" in path), so any support path merely containing "scripts/" (e.g. references/scripts/overview.md or templates/my-scripts/setup.md) was moderated as executable: that scan requires an exact "allow" decision and hard-blocks when the moderation model is unavailable. Executable content is now exactly a file under the skill's top-level scripts/ directory, matching the scripts/ member rule in skills/package_files.py and the same Path semantics the storage layer already used to validate the support path. * refactor(skills): share the scripts/ member rule with skill_manage The write_file executable gate re-derived the top-level scripts/ member rule locally. Use package_files.is_scripts_member instead so the gate, installer, and SkillScan stay in lockstep. Extend the regression test to pin scripts/sub/run.sh and ./scripts/run.sh as executable content, plus the shared helper's normalization. * fix(skills): moderate nested code files as executable content in skill_manage The write_file executable gate classified only top-level members as code, so runnable support files elsewhere in the package (references/scripts/helper.py, templates/my-scripts/run.sh, assets/my-scripts/tool.js) and extensionless shebang files fell back to static moderation: a warn decision, or an unavailable moderation model with security_fail_closed=false, wrote them to disk. The deterministic package scan does not enforce the allow-only model verdict, and this write path publishes through write_custom_skill without running the mutations CandidateScanner, so no later check restores executable moderation. Classify with the complete shared code-file rule from deerflow/skills/package_files.py (scripts members, code suffixes, and extensionless shebang files) instead of the scripts-member rule alone, and keep Markdown/data support files on the static tier. Extend the regression suite with the code-suffix/ shebang cases plus the warn and unavailable-provider outcomes. --- .../harness/deerflow/skills/package_files.py | 9 +- .../deerflow/tools/skill_manage_tool.py | 11 +- ...skill_manage_write_file_executable_gate.py | 349 ++++++++++++++++++ 3 files changed, 366 insertions(+), 3 deletions(-) create mode 100644 backend/tests/test_skill_manage_write_file_executable_gate.py diff --git a/backend/packages/harness/deerflow/skills/package_files.py b/backend/packages/harness/deerflow/skills/package_files.py index 9dc7261c9d..3d8726bfb0 100644 --- a/backend/packages/harness/deerflow/skills/package_files.py +++ b/backend/packages/harness/deerflow/skills/package_files.py @@ -40,10 +40,15 @@ def _posix(path: str | PurePath) -> PurePosixPath: return PurePosixPath(str(path).replace("\\", "/")) +def is_scripts_member(path: str | PurePath) -> bool: + """Return whether a package-relative path is a member of the top-level ``scripts/`` directory.""" + posix = _posix(path) + return bool(posix.parts) and posix.parts[0] == "scripts" + + def is_code_path(path: str | PurePath) -> bool: """Return whether a package-relative path is code by name: a ``scripts/`` member or a code suffix.""" - posix = _posix(path) - return (bool(posix.parts) and posix.parts[0] == "scripts") or posix.suffix.lower() in CODE_SUFFIXES + return is_scripts_member(path) or _posix(path).suffix.lower() in CODE_SUFFIXES def is_code_file(path: str | PurePath, head: bytes) -> bool: diff --git a/backend/packages/harness/deerflow/tools/skill_manage_tool.py b/backend/packages/harness/deerflow/tools/skill_manage_tool.py index 83f2809518..0c34782d2c 100644 --- a/backend/packages/harness/deerflow/tools/skill_manage_tool.py +++ b/backend/packages/harness/deerflow/tools/skill_manage_tool.py @@ -19,6 +19,7 @@ from langchain.tools import tool from deerflow.agents.lead_agent.prompt import SkillCacheResetPublishError, publish_skills_cache_reset, refresh_user_skills_system_prompt_cache_async from deerflow.runtime.user_context import resolve_runtime_user_id +from deerflow.skills.package_files import is_code_file from deerflow.skills.security_scanner import scan_skill_content from deerflow.skills.security_static_scanner import ( StaticFinding, @@ -307,7 +308,15 @@ async def _skill_manage_impl( from deerflow.skills.mutations.guard import read_optional_text prev_content = await _to_thread(read_optional_text, skill_storage, target) - executable = "scripts/" in path or path.startswith("scripts/") + # Executable content is whatever the shared package-file rule classifies + # as code: a file under the top-level ``scripts/`` directory, a code-suffix + # file anywhere in the package, or an extensionless file that starts with + # a shebang. Classify with that shared rule instead of re-deriving it; a + # substring test here also caught static support files such as + # references/scripts/overview.md and templates/my-scripts/setup.md, while + # the ``scripts/``-member rule alone would downgrade runnable files such as + # references/scripts/helper.py or assets/my-scripts/tool.js to static. + executable = is_code_file(path, content[:8].encode("utf-8", "surrogatepass")) 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) scan["static_findings"] = static_findings diff --git a/backend/tests/test_skill_manage_write_file_executable_gate.py b/backend/tests/test_skill_manage_write_file_executable_gate.py new file mode 100644 index 0000000000..391f0d0873 --- /dev/null +++ b/backend/tests/test_skill_manage_write_file_executable_gate.py @@ -0,0 +1,349 @@ +"""Regression coverage for the write_file executable-content gate in skill_manage. + +Executable content is whatever the shared ``deerflow/skills/package_files.py`` +rule classifies as code: a file under the *top-level* ``scripts/`` directory of a +skill package, a code-suffixed file anywhere in the package, or an extensionless +file that starts with a shebang (``is_code_file``). Every other support file is +static and must be moderated as such: an ``executable=True`` scan requires an +exact "allow" decision and hard-blocks when the moderation model is unavailable. + +Both misclassifications are regressions, so both directions are pinned here: + +* over-classifying (the original bug) moderates static support files such as + ``references/scripts/overview.md`` as executable and over-blocks legitimate + writes; and +* under-classifying (the ``scripts/``-member-only rule) leaves runnable files + such as ``references/scripts/helper.py``, ``templates/my-scripts/run.sh`` and + ``assets/my-scripts/tool.js`` on the static tier, so a ``warn`` decision -- or + an unavailable moderation model under ``security_fail_closed: false`` -- writes + code to disk. +""" + +import importlib +from pathlib import Path +from types import SimpleNamespace + +import anyio +import pytest + +skill_manage_module = importlib.import_module("deerflow.tools.skill_manage_tool") + +#: Paths the shared code-file rule classifies as executable content, with content +#: that exercises the three clauses: a code suffix outside ``scripts/``, a +#: ``scripts/``-looking directory that is not the package root, and a shebang on +#: an extensionless file. +CODE_SUPPORT_FILES = [ + ("references/scripts/helper.py", "print('hello')\n", "nested-scripts-python"), + ("templates/my-scripts/run.sh", "#!/bin/bash\necho hello\n", "dir-name-ending-in-scripts"), + ("assets/my-scripts/tool.js", "console.log('hello');\n", "nested-js"), + ("references/scripts/hook", "#!/bin/sh\necho hello\n", "extensionless-shebang"), +] + +#: Paths that stay on the static tier: Markdown/data support files, plus an +#: extensionless file without a shebang. +STATIC_SUPPORT_FILES = [ + ("references/scripts/overview.md", "# Overview\n\nNotes about the helper scripts.\n", "nested-scripts-markdown"), + ("templates/my-scripts/setup.md", "# Setup\n\nSteps.\n", "dir-name-ending-in-scripts-markdown"), + ("references/schema.json", '{"type": "object"}\n', "nested-data"), + ("references/scripts/notes", "plain notes, no shebang\n", "extensionless-no-shebang"), +] + + +def _skill_content(name: str, description: str = "Demo skill") -> str: + return f"---\nname: {name}\ndescription: {description}\n---\n\n# {name}\n" + + +async def _async_result(decision: str, reason: str): + from deerflow.skills.security_scanner import ScanResult + + return ScanResult(decision=decision, reason=reason) + + +def _make_config(skills_root: Path, *, security_fail_closed: bool = True): + return SimpleNamespace( + skills=SimpleNamespace( + get_skills_path=lambda: skills_root, + container_path="/mnt/skills", + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + ), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None, security_fail_closed=security_fail_closed), + ) + + +def _make_runtime(*, thread_id: str = "thread-1", user_id: str = "default"): + return SimpleNamespace( + context={"thread_id": thread_id, "user_id": user_id}, + config={"configurable": {"thread_id": thread_id, "user_id": user_id}}, + ) + + +async def _allow_scan(content, *, executable, location, **kwargs): + return await _async_result("allow", "ok") + + +def _setup_skill(monkeypatch, tmp_path, *, security_fail_closed: bool = True, scanner=None): + """Point the skill storage at ``tmp_path`` and create the demo skill. + + The ``create`` call itself is moderated, so ``scanner`` (default: always + allow) is installed before it runs; tests that need the real moderation + policy for the ``write_file`` call replace it afterwards. + """ + skills_root = tmp_path / "skills" + config = _make_config(skills_root, security_fail_closed=security_fail_closed) + 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", scanner or _allow_scan) + + runtime = _make_runtime(user_id="default") + anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "create", "demo-skill", _skill_content("demo-skill")) + skill_dir = tmp_path / "users" / "default" / "skills" / "custom" / "demo-skill" + return runtime, skill_dir + + +def _setup_with_scan_recorder(monkeypatch, tmp_path, *, decision: str = "allow"): + """Create the demo skill; return (runtime, scan_calls, skill_dir). + + ``scan_calls`` records the moderation kwargs of every ``scan_skill_content`` + call made after the initial create, so tests can pin the ``executable`` + classification of a write_file path and the decision the gate is handed. + """ + scan_calls = [] + + async def _scan(content, *, executable, location, **kwargs): + scan_calls.append({"executable": executable, "location": location}) + return await _async_result(decision, f"stubbed {decision}") + + runtime, skill_dir = _setup_skill(monkeypatch, tmp_path, scanner=_scan) + scan_calls.clear() + return runtime, scan_calls, skill_dir + + +def _use_real_moderation_and_make_it_unavailable(monkeypatch, tmp_path, *, security_fail_closed: bool): + """Create the demo skill, then hand the gate the real scanner with no model.""" + runtime, skill_dir = _setup_skill(monkeypatch, tmp_path, security_fail_closed=security_fail_closed) + + from deerflow.skills.security_scanner import scan_skill_content + + monkeypatch.setattr(skill_manage_module, "scan_skill_content", scan_skill_content) + monkeypatch.setattr("deerflow.skills.security_scanner.create_chat_model", lambda **kwargs: (_ for _ in ()).throw(RuntimeError("moderation provider unavailable"))) + return runtime, skill_dir + + +@pytest.mark.parametrize( + "path", + ["references/scripts/overview.md", "templates/my-scripts/setup.md"], + ids=["nested-scripts-dir", "dir-name-ending-in-scripts"], +) +def test_write_file_support_path_containing_scripts_substring_is_scanned_as_static(monkeypatch, tmp_path, path): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path) + content = "# Overview\n\nNotes about the helper scripts.\n" + + result = anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert f"Wrote '{path}'" in result + assert (skill_dir / Path(path)).read_text(encoding="utf-8") == content + assert scan_calls == [{"executable": False, "location": f"demo-skill/{path}"}], f"support file '{path}' must be moderated as static content (executable=False); got {scan_calls}" + + +@pytest.mark.parametrize( + "path", + ["scripts/run.sh", "scripts/sub/run.sh", "./scripts/run.sh"], + ids=["top-level-script", "nested-under-scripts", "dot-prefixed"], +) +def test_write_file_top_level_scripts_file_is_scanned_as_executable(monkeypatch, tmp_path, path): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path) + content = "#!/bin/bash\necho hello\n" + + anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert (skill_dir / Path(path)).read_text(encoding="utf-8") == content + assert scan_calls == [{"executable": True, "location": f"demo-skill/{path}"}], f"file '{path}' under the top-level scripts/ directory must be moderated as executable content; got {scan_calls}" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in CODE_SUPPORT_FILES], + ids=[case for _, _, case in CODE_SUPPORT_FILES], +) +def test_write_file_code_support_file_outside_top_level_scripts_is_scanned_as_executable(monkeypatch, tmp_path, path, content): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path) + + result = anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert f"Wrote '{path}'" in result + assert scan_calls == [{"executable": True, "location": f"demo-skill/{path}"}], f"code file '{path}' must be moderated as executable content by the shared code-file rule; got {scan_calls}" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in STATIC_SUPPORT_FILES], + ids=[case for _, _, case in STATIC_SUPPORT_FILES], +) +def test_write_file_non_code_support_file_stays_static(monkeypatch, tmp_path, path, content): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path) + + anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert (skill_dir / Path(path)).read_text(encoding="utf-8") == content + assert scan_calls == [{"executable": False, "location": f"demo-skill/{path}"}], f"non-code support file '{path}' must stay on the static tier; got {scan_calls}" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in CODE_SUPPORT_FILES], + ids=[case for _, _, case in CODE_SUPPORT_FILES], +) +def test_write_file_rejects_code_support_file_when_moderation_warns(monkeypatch, tmp_path, path, content): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path, decision="warn") + + with pytest.raises(ValueError, match="Security scan rejected executable content"): + anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert not (skill_dir / Path(path)).exists(), f"a warn decision must not write code file '{path}'" + assert scan_calls == [{"executable": True, "location": f"demo-skill/{path}"}], f"code file '{path}' must reach the gate as executable content; got {scan_calls}" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in STATIC_SUPPORT_FILES], + ids=[case for _, _, case in STATIC_SUPPORT_FILES], +) +def test_write_file_writes_support_file_when_moderation_warns(monkeypatch, tmp_path, path, content): + runtime, scan_calls, skill_dir = _setup_with_scan_recorder(monkeypatch, tmp_path, decision="warn") + + result = anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert f"Wrote '{path}'" in result + assert (skill_dir / Path(path)).read_text(encoding="utf-8") == content + assert scan_calls == [{"executable": False, "location": f"demo-skill/{path}"}], f"static support file '{path}' must stay writable on a warn decision; got {scan_calls}" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in CODE_SUPPORT_FILES], + ids=[case for _, _, case in CODE_SUPPORT_FILES], +) +def test_write_file_rejects_code_support_file_when_moderation_unavailable_and_fail_open(monkeypatch, tmp_path, path, content): + # ``security_fail_closed: false`` only relaxes *non-executable* content; code + # stays blocked because an ``executable=True`` scan never fails open. + runtime, skill_dir = _use_real_moderation_and_make_it_unavailable(monkeypatch, tmp_path, security_fail_closed=False) + + with pytest.raises(ValueError, match="Security scan blocked the write"): + anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert not (skill_dir / Path(path)).exists(), f"an unavailable moderation model must not write code file '{path}'" + + +@pytest.mark.parametrize( + ("path", "content"), + [(path, content) for path, content, _ in STATIC_SUPPORT_FILES], + ids=[case for _, _, case in STATIC_SUPPORT_FILES], +) +def test_write_file_writes_support_file_when_moderation_unavailable_and_fail_open(monkeypatch, tmp_path, path, content): + runtime, skill_dir = _use_real_moderation_and_make_it_unavailable(monkeypatch, tmp_path, security_fail_closed=False) + + result = anyio.run( + skill_manage_module.skill_manage_tool.coroutine, + runtime, + "write_file", + "demo-skill", + content, + path, + ) + + assert f"Wrote '{path}'" in result + assert (skill_dir / Path(path)).read_text(encoding="utf-8") == content + + +@pytest.mark.parametrize( + ("path", "expected"), + [ + ("scripts/run.sh", True), + ("scripts/sub/run.sh", True), + ("scripts\\run.sh", True), + # The shared predicate treats a bare ``scripts`` as a member too; the + # storage layer rejects it as a support path before classification. + ("scripts", True), + ("references/scripts/overview.md", False), + ("/scripts/run.sh", False), + ], +) +def test_is_scripts_member_uses_the_shared_posix_rule(path, expected): + from deerflow.skills.package_files import is_scripts_member + + assert is_scripts_member(path) is expected + + +@pytest.mark.parametrize( + ("path", "head", "expected"), + [ + ("references/scripts/helper.py", b"print('hello')\n", True), + ("assets/my-scripts/tool.js", b"console.log(1);\n", True), + ("references/scripts/hook", b"#!/bin/sh\n", True), + ("references/scripts/hook", b"plain notes\n", False), + ("references/scripts/overview.md", b"# Overview\n", False), + ("scripts/run.sh", b"echo hello\n", True), + ], +) +def test_is_code_file_covers_code_suffixes_and_shebangs_anywhere_in_the_package(path, head, expected): + from deerflow.skills.package_files import is_code_file + + assert is_code_file(path, head) is expected