From c9043c2574daae8a9e5553150befd82ba4288746 Mon Sep 17 00:00:00 2001 From: lau0708 Date: Tue, 22 Sep 2026 22:55:12 +0800 Subject: [PATCH] fix(skills): resolve the user-scoped install content scan from its own config (#5703) * fix(skills): resolve the user-scoped install scan from its own config UserScopedSkillStorage.ainstall_skill_from_archive re-implements the parent body to redirect the install target into the per-user custom root, and the copy dropped the app_config argument. _scan_skill_archive_contents_or_raise therefore fell back to the process-global get_app_config() for the static scan, while the archive preflight it inherits still read self._app_config -- so the two gates of a single install could consult different configs. 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. With the storage's config enabling skill_scan and the global disabling it, the preflight ran, the content scan silently skipped, and a CRITICAL archive installed. * fix(skills): thread app_config into the per-file LLM scan _scan_skill_archive_contents_or_raise takes app_config and uses it for the static gate, but never forwarded it to _scan_skill_file_or_raise, so the per-file LLM scan fell back to the process-global get_app_config() for skill_evolution.moderation_model_name and the model it constructs. Under the stale-snapshot scenario the content-scan fix addresses, the two halves of one install still read two different configs. Raised in review of #5703 by willem-bd. The gateway's own skill-write routes already pass app_config to scan_skill_content; this brings the install path in line. --- .../harness/deerflow/skills/installer.py | 8 +- .../storage/user_scoped_skill_storage.py | 2 +- backend/tests/test_skills_installer.py | 6 +- .../tests/test_user_scoped_skill_storage.py | 134 ++++++++++++++++++ 4 files changed, 143 insertions(+), 7 deletions(-) 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"