fix(skills): cap archive entry count in safe_extract_skill_archive (#4241)

safe_extract_skill_archive() capped total uncompressed bytes (zip bomb
defence) but had no limit on member count, so a small archive with tens
of thousands of tiny/empty entries extracted with no error. The same
entry-count cap already existed in scan_archive_preflight() (skillscan
orchestrator, 4096 members) with the comment "a huge member count is a
bounded DoS vector even when the total size is small" -- but that scan
only runs when the optional skill_scan.enabled kill switch is on
(default true, but operator-configurable), so disabling it silently
dropped this specific protection while config.example.yaml's comment
implied safe archive extraction alone still covered it.

Move the same 4096 cap into safe_extract_skill_archive itself as an
early-abort before any per-member work, so it applies unconditionally
regardless of skill_scan.enabled. Leaving scan_archive_preflight's own
cap in place as defense in depth (it fires earlier, on preflight, with
a structured finding for reporting).

Related: #2618 requested exactly this hardening; #2619 (closed,
unmerged) implemented a broader version of it, including this same
entry-count cap directly in the extractor.

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

@@ -118,6 +118,7 @@ def safe_extract_skill_archive(
     zip_ref: zipfile.ZipFile,
     dest_path: Path,
     max_total_size: int = 512 * 1024 * 1024,
+    max_entries: int = 4096,
 ) -> None:
     """Safely extract a skill archive with security protections.
 
@@ -125,15 +126,28 @@ 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).
+    - Enforce a hard limit on member count (zip bomb defence by entry count —
+      a huge number of tiny/empty members can be cheap to store yet still
+      slow to extract, independent of total size).
     - Reject executable binaries (ELF/PE/Mach-O) by magic bytes.
 
     Raises:
-        ValueError: If unsafe members, executable binaries, or size limit exceeded.
+        ValueError: If unsafe members, executable binaries, entry count, or size limit exceeded.
     """
     dest_root = dest_path.resolve()
     total_written = 0
 
-    for info in zip_ref.infolist():
+    infos = zip_ref.infolist()
+    if len(infos) > max_entries:
+        # Early-abort before any per-member work below — mirrors the same
+        # early-abort in skillscan/orchestrator.py::scan_archive_preflight
+        # (its comment: "a huge member count is a bounded DoS vector even
+        # when the total size is small"). That scan is optional
+        # (skill_scan.enabled); this check must hold unconditionally since
+        # it lives in the extraction path every install goes through.
+        raise ValueError(f"Skill archive contains too many entries ({len(infos)} > {max_entries}).")
+
+    for info in infos:
         if is_unsafe_zip_member(info):
             raise ValueError(f"Archive contains unsafe member path: {info.filename!r}")
 

backend/tests/test_skills_installer.py

@@ -153,6 +153,25 @@ class TestSafeExtract:
         assert (dest / "normal.txt").exists()
         assert not (dest / "link.txt").exists()
 
+    def test_rejects_too_many_entries(self, tmp_path):
+        """Entry-count cap is independent of total size: 4 tiny files still trips a low max_entries."""
+        zip_path = self._make_zip(tmp_path, {f"file-{i}.txt": "x" for i in range(4)})
+        dest = tmp_path / "out"
+        dest.mkdir()
+        with zipfile.ZipFile(zip_path) as zf:
+            with pytest.raises(ValueError, match="too many entries"):
+                safe_extract_skill_archive(zf, dest, max_entries=3)
+        assert not any(dest.iterdir())
+
+    def test_allows_entries_at_the_cap(self, tmp_path):
+        """The cap is inclusive: exactly max_entries members is not rejected."""
+        zip_path = self._make_zip(tmp_path, {f"file-{i}.txt": "x" for i in range(5)})
+        dest = tmp_path / "out"
+        dest.mkdir()
+        with zipfile.ZipFile(zip_path) as zf:
+            safe_extract_skill_archive(zf, dest, max_entries=5)
+        assert len(list(dest.iterdir())) == 5
+
     def test_normal_archive(self, tmp_path):
         zip_path = self._make_zip(
             tmp_path,
@@ -227,6 +246,86 @@ class TestSafeExtract:
         assert (dest / "my-skill" / "assets" / "blob.bin").exists()
 
 
+# ---------------------------------------------------------------------------
+# Entry-count cap must apply unconditionally, independent of skill_scan.enabled.
+#
+# scan_archive_preflight() (skillscan/orchestrator.py) already caps member
+# count at 4096, but only runs as part of the optional native scanner
+# (skill_scan.enabled, default true). When that scanner is disabled,
+# safe_extract_skill_archive was the only remaining guard on the extraction
+# path, and it only capped total bytes — not entry count. These tests pin
+# the fix: the cap now lives in extraction itself, so it holds regardless of
+# skill_scan.enabled.
+# ---------------------------------------------------------------------------
+
+
+class TestEntryCountCapAppliesRegardlessOfSkillScan:
+    @pytest.fixture(autouse=True)
+    def _allow_security_scan(self, monkeypatch):
+        async def _scan(*args, **kwargs):
+            return ScanResult(decision="allow", reason="ok")
+
+        monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan)
+
+    def _make_storage(self, skills_root: Path, *, skill_scan_enabled: bool):
+        from types import SimpleNamespace
+
+        from deerflow.skills.storage.local_skill_storage import LocalSkillStorage
+
+        return LocalSkillStorage(
+            host_path=str(skills_root),
+            app_config=SimpleNamespace(skill_scan=SimpleNamespace(enabled=skill_scan_enabled)),
+        )
+
+    def _make_many_entry_zip(self, tmp_path: Path, *, entry_count: int, skill_name: str = "test-skill") -> Path:
+        """A real archive with ``entry_count`` tiny members and a small total size —
+        matches the reported shape (50,000 entries, ~5MB total)."""
+        zip_path = tmp_path / f"{skill_name}.skill"
+        with zipfile.ZipFile(zip_path, "w", zipfile.ZIP_DEFLATED) as zf:
+            zf.writestr(f"{skill_name}/SKILL.md", f"---\nname: {skill_name}\ndescription: A test skill\n---\n\n# {skill_name}\n")
+            for i in range(entry_count):
+                zf.writestr(f"{skill_name}/pad-{i:06d}.txt", "")
+        return zip_path
+
+    def test_rejects_high_entry_count_archive_even_with_skill_scan_disabled(self, tmp_path):
+        """The previously-vulnerable configuration: skill_scan disabled, so
+        scan_archive_preflight's member cap never runs. safe_extract_skill_archive
+        must still reject the archive unconditionally, on its own."""
+        zip_path = self._make_many_entry_zip(tmp_path, entry_count=50_000)
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+        storage = self._make_storage(skills_root, skill_scan_enabled=False)
+
+        with pytest.raises(ValueError, match="too many entries"):
+            storage.install_skill_from_archive(zip_path)
+
+        assert not (skills_root / "custom" / "test-skill").exists()
+
+    def test_scan_archive_preflight_independently_flags_the_same_archive(self, tmp_path):
+        """Cross-check tying the two protections together: the pre-existing optional
+        scanner also flags this exact archive by member count when it does run."""
+        from deerflow.skills.skillscan.orchestrator import scan_archive_preflight
+
+        zip_path = self._make_many_entry_zip(tmp_path, entry_count=50_000)
+
+        result = scan_archive_preflight(zip_path)
+
+        assert result["blocked"] is True
+        assert any(finding["rule_id"] == "package-too-many-members" for finding in result["findings"])
+
+    def test_normal_skill_archive_still_installs_with_skill_scan_disabled(self, tmp_path):
+        """Same disabled-scan configuration, but a small, legitimate skill: must still install."""
+        zip_path = self._make_many_entry_zip(tmp_path, entry_count=5, skill_name="small-skill")
+        skills_root = tmp_path / "skills"
+        skills_root.mkdir()
+        storage = self._make_storage(skills_root, skill_scan_enabled=False)
+
+        result = storage.install_skill_from_archive(zip_path)
+
+        assert result["success"] is True
+        assert (skills_root / "custom" / "small-skill" / "SKILL.md").exists()
+
+
 # ---------------------------------------------------------------------------
 # install_skill_from_archive (full integration)
 # ---------------------------------------------------------------------------

config.example.yaml

@@ -1364,8 +1364,10 @@ skills:
 # Native deterministic skill safety scanning. This runs before the LLM skill
 # scanner on skill install/update and agent-managed skill writes.
 skill_scan:
-  # Set false to disable the new deterministic analyzers. Safe archive
-  # extraction and the LLM skill scanner still run.
+  # Set false to disable the new deterministic analyzers (nested-archive,
+  # secret-pattern, and other content-level checks). Safe archive extraction
+  # (path traversal, symlinks, executable-binary, total-size, and entry-count
+  # limits) and the LLM skill scanner still run unconditionally.
   enabled: true
 
 # Note: To restrict which skills are loaded for a specific custom agent,