fix(skills): close skill install security scan coverage gap (#3924)

* fix(skills): close install-path scan coverage gap

Skill installs only sent scripts/* and document files under references/
and templates/ to the LLM security scanner. Code at the skill root or
under lib/, bin/, src/, etc., and binary files could be installed
without any scan.

- Scan code files anywhere in the skill tree (by extension, plus
  shebang detection for extensionless files) with the executable
  policy: only an explicit allow admits them.
- Reject ELF/PE/Mach-O executable binaries by magic bytes during safe
  archive extraction; non-executable binary assets remain allowed.

Interim hardening ahead of the SkillScan framework (RFC #2634); the
deterministic full-tree scanner from PR #3033 supersedes the per-file
LLM coverage when it lands.

* Apply suggestions from code review

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* test(skills): pin magic coverage and shebang offload boundary

Follow-up to the applied review suggestions: cover every Mach-O magic
variant with tests (plus the fat64 pair and a partial-prefix asset that
must stay installable), name the pure classification helper so the
call-site logic reads as policy, drop the now-unused sync _is_code_file,
and pin that only extensionless files get the shebang sniff.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

backend/packages/harness/deerflow/skills/installer.py

@@ -20,6 +20,21 @@ logger = logging.getLogger(__name__)
 
 _PROMPT_INPUT_DIRS = {"references", "templates"}
 _PROMPT_INPUT_SUFFIXES = frozenset({".json", ".markdown", ".md", ".rst", ".txt", ".yaml", ".yml"})
+_CODE_SUFFIXES = frozenset({".bash", ".cjs", ".js", ".mjs", ".php", ".pl", ".ps1", ".py", ".rb", ".sh", ".ts", ".zsh"})
+# Full magics per variant — a shorter shared prefix would also match
+# non-executable data files.
+_EXECUTABLE_MAGIC_PREFIXES = (
+    b"\x7fELF",  # ELF
+    b"MZ",  # PE/DOS
+    b"\xfe\xed\xfa\xce",  # Mach-O 32-bit big-endian
+    b"\xfe\xed\xfa\xcf",  # Mach-O 64-bit big-endian
+    b"\xce\xfa\xed\xfe",  # Mach-O 32-bit little-endian
+    b"\xcf\xfa\xed\xfe",  # Mach-O 64-bit little-endian
+    b"\xca\xfe\xba\xbe",  # Mach-O fat binary big-endian
+    b"\xbe\xba\xfe\xca",  # Mach-O fat binary little-endian
+    b"\xca\xfe\xba\xbf",  # Mach-O fat64 binary big-endian
+    b"\xbf\xba\xfe\xca",  # Mach-O fat64 binary little-endian
+)
 
 
 class SkillAlreadyExistsError(ValueError):
@@ -54,6 +69,11 @@ def is_symlink_member(info: zipfile.ZipInfo) -> bool:
     return stat.S_ISLNK(mode)
 
 
+def is_executable_binary_prefix(prefix: bytes) -> bool:
+    """Detect ELF, PE, and Mach-O executables by magic bytes."""
+    return prefix.startswith(_EXECUTABLE_MAGIC_PREFIXES)
+
+
 def should_ignore_archive_entry(path: Path) -> bool:
     """Return True for macOS metadata dirs and dotfiles."""
     return path.name.startswith(".") or path.name == "__MACOSX"
@@ -89,9 +109,10 @@ def safe_extract_skill_archive(
     - Reject absolute paths and directory traversal (..).
     - Skip symlink entries instead of materialising them.
     - Enforce a hard limit on total uncompressed size (zip bomb defence).
+    - Reject executable binaries (ELF/PE/Mach-O) by magic bytes.
 
     Raises:
-        ValueError: If unsafe members or size limit exceeded.
+        ValueError: If unsafe members, executable binaries, or size limit exceeded.
     """
     dest_root = dest_path.resolve()
     total_written = 0
@@ -115,7 +136,11 @@ def safe_extract_skill_archive(
             continue
 
         with zip_ref.open(info) as src, member_path.open("wb") as dst:
+            first_chunk = True
             while chunk := src.read(65536):
+                if first_chunk and is_executable_binary_prefix(chunk):
+                    raise ValueError(f"Archive contains executable binary member: {info.filename!r}")
+                first_chunk = False
                 total_written += len(chunk)
                 if total_written > max_total_size:
                     raise ValueError("Skill archive is too large or appears highly compressed.")
@@ -132,6 +157,32 @@ def _should_scan_support_file(rel_path: Path) -> bool:
     return bool(rel_path.parts) and rel_path.parts[0] in _PROMPT_INPUT_DIRS and rel_path.suffix.lower() in _PROMPT_INPUT_SUFFIXES
 
 
+def _has_shebang(path: Path) -> bool:
+    try:
+        with path.open("rb") as f:
+            return f.read(2) == b"#!"
+    except OSError:
+        return False
+
+
+def _is_code_file_by_name(rel_path: Path) -> bool:
+    """Pure name-based code classification: scripts/ members and code suffixes."""
+    if _is_script_support_file(rel_path):
+        return True
+    return rel_path.suffix.lower() in _CODE_SUFFIXES
+
+
+async def _is_code_file(path: Path, rel_path: Path) -> bool:
+    """Classify code files anywhere in the tree for the executable scan policy.
+
+    Name checks are pure and stay on the event loop; only the shebang
+    sniff for extensionless files reads the file and is offloaded.
+    """
+    if _is_code_file_by_name(rel_path):
+        return True
+    return not rel_path.suffix and await asyncio.to_thread(_has_shebang, path)
+
+
 def _move_staged_skill_into_reserved_target(staging_target: Path, target: Path) -> None:
     installed = False
     reserved = False
@@ -190,10 +241,10 @@ async def _scan_skill_archive_contents_or_raise(skill_dir: Path, skill_name: str
             continue
         if path.name == "SKILL.md":
             raise SkillSecurityScanError(f"Security scan failed for skill '{skill_name}': nested SKILL.md is not allowed at {skill_name}/{rel_path.as_posix()}")
-        if not _should_scan_support_file(rel_path):
+        if await _is_code_file(path, rel_path):
-            continue
+            await _scan_skill_file_or_raise(skill_dir, path, skill_name, executable=True)
-
+        elif _should_scan_support_file(rel_path):
-        await _scan_skill_file_or_raise(skill_dir, path, skill_name, executable=_is_script_support_file(rel_path))
+            await _scan_skill_file_or_raise(skill_dir, path, skill_name, executable=False)
 
 
 def _run_async_install(coro):

backend/tests/test_skills_installer.py

@@ -165,6 +165,64 @@ class TestSafeExtract:
         assert (dest / "my-skill" / "SKILL.md").exists()
         assert (dest / "my-skill" / "README.md").exists()
 
+    @pytest.mark.parametrize(
+        "magic",
+        [
+            pytest.param(b"\x7fELF\x02\x01\x01\x00", id="elf"),
+            pytest.param(b"MZ\x90\x00\x03\x00\x00\x00", id="pe"),
+            pytest.param(b"\xfe\xed\xfa\xce\x00\x00\x00\x0c", id="mach-o-32-be"),
+            pytest.param(b"\xfe\xed\xfa\xcf\x00\x00\x00\x0c", id="mach-o-64-be"),
+            pytest.param(b"\xce\xfa\xed\xfe\x0c\x00\x00\x00", id="mach-o-32-le"),
+            pytest.param(b"\xcf\xfa\xed\xfe\x07\x00\x00\x01", id="mach-o-64-le"),
+            pytest.param(b"\xca\xfe\xba\xbe\x00\x00\x00\x02", id="mach-o-fat-be"),
+            pytest.param(b"\xbe\xba\xfe\xca\x02\x00\x00\x00", id="mach-o-fat-le"),
+            pytest.param(b"\xca\xfe\xba\xbf\x00\x00\x00\x02", id="mach-o-fat64-be"),
+            pytest.param(b"\xbf\xba\xfe\xca\x02\x00\x00\x00", id="mach-o-fat64-le"),
+        ],
+    )
+    def test_rejects_executable_binary(self, tmp_path, magic):
+        zip_path = self._make_zip(
+            tmp_path,
+            {
+                "my-skill/SKILL.md": "---\nname: test\ndescription: x\n---\n# Test",
+                "my-skill/bin/tool": magic + b"\x00" * 64,
+            },
+        )
+        dest = tmp_path / "out"
+        dest.mkdir()
+        with zipfile.ZipFile(zip_path) as zf:
+            with pytest.raises(ValueError, match="executable binary"):
+                safe_extract_skill_archive(zf, dest)
+
+    def test_allows_non_executable_binary_assets(self, tmp_path):
+        zip_path = self._make_zip(
+            tmp_path,
+            {
+                "my-skill/SKILL.md": "---\nname: test\ndescription: x\n---\n# Test",
+                "my-skill/assets/logo.png": b"\x89PNG\r\n\x1a\n" + b"\x00" * 32,
+            },
+        )
+        dest = tmp_path / "out"
+        dest.mkdir()
+        with zipfile.ZipFile(zip_path) as zf:
+            safe_extract_skill_archive(zf, dest)
+        assert (dest / "my-skill" / "assets" / "logo.png").exists()
+
+    def test_allows_asset_sharing_a_partial_magic_prefix(self, tmp_path):
+        """Only full 4-byte magics are executable; \\xfe\\xed\\xfa + other byte is data."""
+        zip_path = self._make_zip(
+            tmp_path,
+            {
+                "my-skill/SKILL.md": "---\nname: test\ndescription: x\n---\n# Test",
+                "my-skill/assets/blob.bin": b"\xfe\xed\xfa\x00" + b"\x00" * 32,
+            },
+        )
+        dest = tmp_path / "out"
+        dest.mkdir()
+        with zipfile.ZipFile(zip_path) as zf:
+            safe_extract_skill_archive(zf, dest)
+        assert (dest / "my-skill" / "assets" / "blob.bin").exists()
+
 
 # ---------------------------------------------------------------------------
 # install_skill_from_archive (full integration)
@@ -286,6 +344,93 @@ class TestInstallSkillFromArchive:
         ]
         assert all("secret" not in call["content"] for call in calls)
 
+    def test_scans_code_files_anywhere_in_tree(self, tmp_path, monkeypatch):
+        zip_path = tmp_path / "test-skill.skill"
+        with zipfile.ZipFile(zip_path, "w") as zf:
+            zf.writestr("test-skill/SKILL.md", "---\nname: test-skill\ndescription: A test skill\n---\n\n# test-skill\n")
+            zf.writestr("test-skill/helper.py", "import os\nprint('root code')\n")
+            zf.writestr("test-skill/lib/util.sh", "echo lib\n")
+            zf.writestr("test-skill/bin/tool", "#!/usr/bin/env python3\nprint('extensionless')\n")
+            zf.writestr("test-skill/assets/logo.png", b"\x89PNG\r\n\x1a\n")
+            zf.writestr("test-skill/assets/data.txt", "just data\n")
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+        calls = []
+
+        async def _scan(content, *, executable, location):
+            calls.append({"executable": executable, "location": location})
+            return ScanResult(decision="allow", reason="ok")
+
+        monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan)
+
+        get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path)
+
+        assert {"executable": True, "location": "test-skill/helper.py"} in calls
+        assert {"executable": True, "location": "test-skill/lib/util.sh"} in calls
+        assert {"executable": True, "location": "test-skill/bin/tool"} in calls
+        scanned_locations = {call["location"] for call in calls}
+        assert "test-skill/assets/logo.png" not in scanned_locations
+        assert "test-skill/assets/data.txt" not in scanned_locations
+
+    def test_shebang_sniff_only_reads_extensionless_files(self, tmp_path, monkeypatch):
+        """Suffix/scripts classification is name-based; only extensionless files are opened."""
+        import deerflow.skills.installer as installer_module
+
+        zip_path = tmp_path / "test-skill.skill"
+        with zipfile.ZipFile(zip_path, "w") as zf:
+            zf.writestr("test-skill/SKILL.md", "---\nname: test-skill\ndescription: A test skill\n---\n\n# test-skill\n")
+            zf.writestr("test-skill/helper.py", "print('code')\n")
+            zf.writestr("test-skill/scripts/run.sh", "#!/bin/sh\necho ok\n")
+            zf.writestr("test-skill/bin/tool", "#!/usr/bin/env python3\nprint('extensionless')\n")
+            zf.writestr("test-skill/assets/data.txt", "just data\n")
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+        sniffed = []
+        original_has_shebang = installer_module._has_shebang
+
+        def _tracking_has_shebang(path):
+            sniffed.append(path.name)
+            return original_has_shebang(path)
+
+        monkeypatch.setattr(installer_module, "_has_shebang", _tracking_has_shebang)
+
+        get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path)
+
+        assert sniffed == ["tool"]
+
+    def test_code_file_outside_scripts_warn_prevents_install(self, tmp_path, monkeypatch):
+        zip_path = tmp_path / "test-skill.skill"
+        with zipfile.ZipFile(zip_path, "w") as zf:
+            zf.writestr("test-skill/SKILL.md", "---\nname: test-skill\ndescription: A test skill\n---\n\n# test-skill\n")
+            zf.writestr("test-skill/lib/run.py", "import os\nos.system('whoami')\n")
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+
+        async def _scan(*args, executable, **kwargs):
+            if executable:
+                return ScanResult(decision="warn", reason="code needs review")
+            return ScanResult(decision="allow", reason="ok")
+
+        monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan)
+
+        with pytest.raises(SkillSecurityScanError, match="rejected executable.*code needs review"):
+            get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path)
+
+        assert not (skills_root / "custom" / "test-skill").exists()
+
+    def test_executable_binary_prevents_install(self, tmp_path):
+        zip_path = tmp_path / "test-skill.skill"
+        with zipfile.ZipFile(zip_path, "w") as zf:
+            zf.writestr("test-skill/SKILL.md", "---\nname: test-skill\ndescription: A test skill\n---\n\n# test-skill\n")
+            zf.writestr("test-skill/bin/tool", b"\x7fELF\x02\x01\x01\x00" + b"\x00" * 64)
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+
+        with pytest.raises(ValueError, match="executable binary"):
+            get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path)
+
+        assert not (skills_root / "custom" / "test-skill").exists()
+
     def test_nested_skill_markdown_prevents_install(self, tmp_path):
         zip_path = tmp_path / "test-skill.skill"
         with zipfile.ZipFile(zip_path, "w") as zf: