From 3725ba1bfb65046df589bf6346d1c6e92594c932 Mon Sep 17 00:00:00 2001 From: jay <1569941651@qq.com> Date: Tue, 6 Oct 2026 17:55:51 +0800 Subject: [PATCH] fix(skillscan): gate the directory scan file size before reading (#6359) * fix(skillscan): gate the directory scan file size before reading scan_skill_dir() read every candidate file with read_bytes() before any size check; MAX_FILE_BYTES was only recorded as a finding afterwards. Measured: one 300MB file (4.7x the 64MB limit) drives a 600MB peak allocation and a 20s scan, linearly extrapolating to an OOM for larger planted files - reachable via review/analyzer.py on content that never passed the archive preflight gates. Gate with stat() first and bound oversized reads to MAX_FILE_BYTES + 1, mirroring _read_archive_member so both entry points share the same discipline. Regression test plants an oversized file and fails if read_bytes is used on it. Closes #6358 * fix(skillscan): collapse to a single bounded read per file Per review: Path.open + read(MAX_FILE_BYTES + 1) returns the whole file when it is smaller than the limit, identical to read_bytes for regular files - removing the size-conditional branch and closing the stat-to-read race where a file growing past the limit between calls would still be read unbounded. file_size from stat stays the source of truth for the package-oversized-file evidence. --- .../deerflow/skills/skillscan/orchestrator.py | 14 ++++++++-- backend/tests/test_skillscan_native.py | 26 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 4dfde9c01d..e93e8a3c49 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -270,12 +270,22 @@ def scan_skill_dir(skill_dir: Path) -> ScanResult: for path in sorted(candidate for candidate in root.rglob("*") if candidate.is_file()): rel_path = _relative_file(path, root) try: - file_bytes = path.read_bytes() + file_size = path.stat().st_size + except OSError as e: + scanner_errors.append(f"{rel_path}: failed to stat file: {e}") + continue + try: + # One bounded read for every file: files smaller than MAX_FILE_BYTES + # come back whole, oversized ones are truncated and recorded below — + # no unbounded read regardless of growth between stat() and here + # (mirrors _read_archive_member). + with path.open("rb") as handle: + file_bytes = handle.read(MAX_FILE_BYTES + 1) except OSError as e: scanner_errors.append(f"{rel_path}: failed to read file: {e}") continue - findings.extend(_scan_file_package_properties(rel_path, file_bytes, path.stat().st_size)) + findings.extend(_scan_file_package_properties(rel_path, file_bytes, file_size)) text = _decode_text_for_analysis(file_bytes) if text is None: text = _decode_script_lossily(rel_path, file_bytes) diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index f6a32feb0a..76159e001a 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -13,7 +13,7 @@ import pytest from deerflow.skills.package_files import is_executable_binary_prefix from deerflow.skills.security_scanner import scan_skill_content from deerflow.skills.skillscan import StaticScanBlockedError, enforce_static_scan, scan_archive_preflight, scan_skill_dir -from deerflow.skills.skillscan.orchestrator import _PYTHON_CLIENT_SINK_METHODS +from deerflow.skills.skillscan.orchestrator import _PYTHON_CLIENT_SINK_METHODS, MAX_FILE_BYTES _FINDING_FIELDS = {"rule_id", "severity", "file", "line", "message", "remediation", "evidence"} @@ -1812,6 +1812,30 @@ def test_secret_assignment_survives_nul_byte_in_python(tmp_path: Path) -> None: assert finding["file"] == "scripts/sample.py" +def test_scan_dir_bounds_oversized_file_read(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """`scan_skill_dir` must gate the size BEFORE reading: an oversized file is + recorded as a finding and scanned only through the bounded read, never via + `read_bytes` (mirroring `_read_archive_member`'s gate-then-bounded-read).""" + skill_dir = tmp_path / "skill" + _write_skill(skill_dir) + blob = skill_dir / "blob.bin" + with blob.open("wb") as handle: + handle.seek(300 * 1024 * 1024 - 1) + handle.write(b"\0") + + real_read_bytes = Path.read_bytes + + def guarded_read_bytes(self: Path) -> bytes: + if self.stat().st_size > MAX_FILE_BYTES: + raise AssertionError("read_bytes used on an oversized file") + return real_read_bytes(self) + + monkeypatch.setattr(Path, "read_bytes", guarded_read_bytes) + + findings = scan_skill_dir(skill_dir)["findings"] + assert _finding_by_rule(findings, "package-oversized-file") + + def test_bundled_public_skill_scripts_report_no_secret_assignment() -> None: """Bundled skill scripts must not fail the review gate on an unchanged checkout (#4996).