diff --git a/backend/packages/harness/deerflow/skills/installer.py b/backend/packages/harness/deerflow/skills/installer.py index a6ae66716..5bfcc3860 100644 --- a/backend/packages/harness/deerflow/skills/installer.py +++ b/backend/packages/harness/deerflow/skills/installer.py @@ -228,7 +228,7 @@ def _findings_for_file(findings: list[StaticFinding], rel_path: str) -> list[Sta return [finding for finding in findings if finding.get("file") in {rel_path, None}] -async def _scan_skill_file_or_raise(skill_dir: Path, path: Path, skill_name: str, *, executable: bool, static_findings: list[StaticFinding] | None = None) -> None: +async def _scan_skill_file_or_raise(skill_dir: Path, path: Path, skill_name: str, *, executable: bool, static_findings: list[StaticFinding] | None = None, app_config=None) -> None: rel_path = path.relative_to(skill_dir).as_posix() location = f"{skill_name}/{rel_path}" try: @@ -237,7 +237,7 @@ async def _scan_skill_file_or_raise(skill_dir: Path, path: Path, skill_name: str raise SkillSecurityScanError(f"Security scan failed for skill '{skill_name}': {location} must be valid UTF-8") from e try: - result = await scan_skill_content(content, executable=executable, location=location, static_findings=static_findings or []) + result = await scan_skill_content(content, executable=executable, location=location, app_config=app_config, static_findings=static_findings or []) except Exception as e: raise SkillSecurityScanError(f"Security scan failed for {location}: {e}") from e @@ -289,7 +289,7 @@ async def _scan_skill_archive_contents_or_raise(skill_dir: Path, skill_name: str static_findings = await _scan_static_skill_archive_or_raise(skill_dir, skill_name, app_config=app_config) skill_md = skill_dir / "SKILL.md" - await _scan_skill_file_or_raise(skill_dir, skill_md, skill_name, executable=False, static_findings=_findings_for_file(static_findings, "SKILL.md")) + await _scan_skill_file_or_raise(skill_dir, skill_md, skill_name, executable=False, static_findings=_findings_for_file(static_findings, "SKILL.md"), app_config=app_config) for path in await asyncio.to_thread(_collect_scannable_files, skill_dir): rel_path = path.relative_to(skill_dir) @@ -305,6 +305,7 @@ async def _scan_skill_archive_contents_or_raise(skill_dir: Path, skill_name: str skill_name, executable=True, static_findings=_findings_for_file(static_findings, rel_path_posix), + app_config=app_config, ) elif _should_scan_support_file(rel_path): await _scan_skill_file_or_raise( @@ -313,6 +314,7 @@ async def _scan_skill_archive_contents_or_raise(skill_dir: Path, skill_name: str skill_name, executable=False, static_findings=_findings_for_file(static_findings, rel_path_posix), + app_config=app_config, ) return static_findings diff --git a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py index 8e2a45986..dcd69f619 100644 --- a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py @@ -340,7 +340,7 @@ class UserScopedSkillStorage(LocalSkillStorage): try: skill_dir, skill_name, target = await asyncio.to_thread(self._prepare_skill_archive, path, Path(tmp), custom_dir, archive_path) - await _scan_skill_archive_contents_or_raise(skill_dir, skill_name) + await _scan_skill_archive_contents_or_raise(skill_dir, skill_name, app_config=self._app_config) await asyncio.to_thread(self._commit_skill_install, skill_dir, skill_name, custom_dir, target) logger.info("Skill %r installed to %s for user %s", skill_name, target, self._user_id) diff --git a/backend/tests/test_skills_installer.py b/backend/tests/test_skills_installer.py index f13db4594..a86cae2d4 100644 --- a/backend/tests/test_skills_installer.py +++ b/backend/tests/test_skills_installer.py @@ -455,7 +455,7 @@ class TestInstallSkillFromArchive: skills_root.mkdir() calls = [] - async def _scan(content, *, executable, location, static_findings=None): + async def _scan(content, *, executable, location, app_config=None, static_findings=None): calls.append({"content": content, "executable": executable, "location": location}) return ScanResult(decision="allow", reason="ok") @@ -485,7 +485,7 @@ class TestInstallSkillFromArchive: skills_root.mkdir() calls = [] - async def _scan(content, *, executable, location, static_findings=None): + async def _scan(content, *, executable, location, app_config=None, static_findings=None): calls.append({"content": content, "executable": executable, "location": location}) return ScanResult(decision="allow", reason="ok") @@ -530,7 +530,7 @@ class TestInstallSkillFromArchive: skills_root.mkdir() calls = [] - async def _scan(content, *, executable, location, static_findings=None): + async def _scan(content, *, executable, location, app_config=None, static_findings=None): calls.append({"executable": executable, "location": location}) return ScanResult(decision="allow", reason="ok") diff --git a/backend/tests/test_user_scoped_skill_storage.py b/backend/tests/test_user_scoped_skill_storage.py index 91d6cc91f..f37016a7c 100644 --- a/backend/tests/test_user_scoped_skill_storage.py +++ b/backend/tests/test_user_scoped_skill_storage.py @@ -3,13 +3,18 @@ from __future__ import annotations import stat +import zipfile from pathlib import Path +from types import SimpleNamespace from unittest.mock import patch import pytest from deerflow.config.paths import Paths +from deerflow.skills.installer import SkillSecurityScanError +from deerflow.skills.security_scanner import ScanResult from deerflow.skills.storage import reset_skill_storage, reset_user_skill_storage +from deerflow.skills.storage.local_skill_storage import LocalSkillStorage from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage from deerflow.skills.types import SkillCategory @@ -698,3 +703,132 @@ class TestEnabledSkillsByConfigCacheBounded: assert (id(new_cfg), "user-new") in kept assert (id(configs[2]), "user-2") not in kept, "LRU should have been evicted" assert len(prompt_module._enabled_skills_by_config_cache) == 4 + + +def _skills_root(base: Path) -> Path: + root = base / "skills" + root.mkdir() + (root / "public").mkdir() + (root / "custom").mkdir() + return root + + +def _storage_config(skills_root: Path, *, scan_enabled: bool) -> SimpleNamespace: + """An app_config carrying the ``skill_scan.enabled`` kill switch.""" + return SimpleNamespace( + skills=SimpleNamespace( + get_skills_path=lambda: skills_root, + container_path="/mnt/skills", + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + ), + skill_scan=SimpleNamespace(enabled=scan_enabled), + ) + + +def _malicious_archive(directory: Path, skill_name: str = "evil-skill") -> Path: + """A well-formed archive whose SKILL.md carries CRITICAL private-key material.""" + return _archive(directory, skill_name, extra="-----BEGIN RSA PRIVATE KEY-----\nMIIEowIBAAKCAQEA\n-----END RSA PRIVATE KEY-----\n") + + +def _archive(directory: Path, skill_name: str = "benign-skill", extra: str = "") -> Path: + """A well-formed archive containing one SKILL.md.""" + path = directory / f"{skill_name}.skill" + with zipfile.ZipFile(path, "w") as zf: + zf.writestr( + f"{skill_name}/SKILL.md", + f"---\nname: {skill_name}\ndescription: Demo skill\n---\n\n# {skill_name}\n\n{extra}", + ) + return path + + +def _install_outcome(storage, archive: Path) -> str: + try: + storage.install_skill_from_archive(archive) + except SkillSecurityScanError: + return "blocked" + return "installed" + + +class TestInstallScanConfigParity: + """``ainstall_skill_from_archive`` resolves ``skill_scan.enabled`` from the + storage's own ``app_config`` — the same config the archive preflight it + inherits already reads — not from the process-global ``get_app_config()``. + + The divergence is reachable whenever the storage outlives a config edit: + ``DeerFlowClient`` snapshots ``get_app_config()`` at construction, and the + Gateway hands its per-request ``get_config()`` to + ``get_or_new_user_skill_storage``, while ``get_app_config`` hot-reloads a + later edit of ``config.yaml``. Resolving the content scan from the process + global therefore let the two gates of a single install disagree. + """ + + @pytest.fixture(autouse=True) + def _allow_llm_scan(self, monkeypatch): + """The per-file LLM scan is not gated by the kill switch; only the + static scan is. Stub it so the outcome reflects config resolution.""" + + async def _allow(*args, **kwargs): + return ScanResult(decision="allow", reason="ok") + + monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _allow) + + def _local_storage(self, skills_root: Path, config: SimpleNamespace) -> LocalSkillStorage: + return LocalSkillStorage(host_path=str(skills_root), app_config=config) + + def _user_storage(self, base: Path, skills_root: Path, config: SimpleNamespace) -> UserScopedSkillStorage: + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base)): + with patch("deerflow.config.paths._paths", None): + return UserScopedSkillStorage("test-user", host_path=str(skills_root), app_config=config) + + def test_content_scan_honours_the_storages_own_config(self, tmp_path: Path, monkeypatch) -> None: + """Storage config ON, process global OFF: the archive must still be blocked.""" + monkeypatch.setattr("deerflow.config.get_app_config", lambda: SimpleNamespace(skill_scan=SimpleNamespace(enabled=False))) + + skills_root = _skills_root(tmp_path) + archive = _malicious_archive(tmp_path) + config = _storage_config(skills_root, scan_enabled=True) + + assert _install_outcome(self._local_storage(skills_root, config), archive) == "blocked" + assert _install_outcome(self._user_storage(tmp_path, skills_root, config), archive) == "blocked" + + def test_kill_switch_off_in_the_storages_own_config_skips_both_gates(self, tmp_path: Path, monkeypatch) -> None: + """Storage config OFF, process global ON: both storages accept it. + + This direction pins the same resolution from the other side — an + operator who disables ``skill_scan`` in the config the storage was + handed gets no scan, regardless of what the hot-reloaded global says. + """ + monkeypatch.setattr("deerflow.config.get_app_config", lambda: SimpleNamespace(skill_scan=SimpleNamespace(enabled=True))) + + skills_root = _skills_root(tmp_path) + archive = _malicious_archive(tmp_path) + config = _storage_config(skills_root, scan_enabled=False) + + assert _install_outcome(self._local_storage(skills_root, config), archive) == "installed" + assert _install_outcome(self._user_storage(tmp_path, skills_root, config), archive) == "installed" + + def test_llm_scan_receives_the_storages_own_config(self, tmp_path: Path, monkeypatch) -> None: + """The per-file LLM scan resolves its config the same way. + + ``skill_scan.enabled`` does not gate it — ``scan_skill_content`` always + runs — but it reads ``skill_evolution.moderation_model_name`` and builds + the model from its ``app_config``, so leaving it on the process global + keeps the two halves of one install on two different configs. + """ + seen: list[object] = [] + + async def _record(*args, **kwargs): + seen.append(kwargs.get("app_config")) + return ScanResult(decision="allow", reason="ok") + + monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _record) + + skills_root = _skills_root(tmp_path) + archive = _archive(tmp_path) + config = _storage_config(skills_root, scan_enabled=True) + + self._local_storage(skills_root, config).install_skill_from_archive(archive) + self._user_storage(tmp_path, skills_root, config).install_skill_from_archive(archive) + + assert seen, "the LLM scan should have run for the installed SKILL.md" + assert all(entry is config for entry in seen), "every LLM scan must receive the storage's own app_config"