diff --git a/src/skillspector/nodes/analyzers/static_yara.py b/src/skillspector/nodes/analyzers/static_yara.py index 5de066b1a..999d586a4 100644 --- a/src/skillspector/nodes/analyzers/static_yara.py +++ b/src/skillspector/nodes/analyzers/static_yara.py @@ -28,6 +28,7 @@ import math import os import stat +import threading import time from collections.abc import Callable from contextvars import ContextVar @@ -46,6 +47,7 @@ InspectionLedgerEvent, LedgerOutcome, LedgerReason, + LedgerRecordType, analyzer_status_event, ledger_event, ) @@ -172,9 +174,40 @@ def _enforce_rule_load_deadline() -> None: _check_rule_load_budget(budget) -# Module-level cache keyed by a content hash of all rule directories. -_compiled_rules: yara.Rules | None = None -_rules_hash: str | None = None +@dataclass(frozen=True, slots=True) +class _RuleCacheEntry: + """One compiled rule set, the hash it came from, and its own dropped-file count. + + Frozen, and only ever published by replacing :data:`_rule_cache` wholesale, + so the three halves cannot drift apart. They used to be three independent + globals, and the non-populating paths of :func:`_load_rules` wrote the skip + count while leaving the compiled rules and their hash in place. A later + request for that stale hash then hit the cache and returned those rules + paired with the intervening load's count -- zero, when the intervening load + found no rule files at all -- so a rule set that had silently dropped a + detector reported a complete scan, which is the false-clean result #554 is + about. + """ + + rules: yara.Rules + rules_hash: str + skipped_count: int + + +# Module-level cache keyed by a content hash of all rule directories. ``None`` +# means nothing usable is cached; there is deliberately no way to represent a +# half-populated cache, so every non-populating load path simply clears it. +_rule_cache: _RuleCacheEntry | None = None + +# Not cache state: the skip count of whichever load most recently ran, published +# under ``_RULES_LOCK`` so :func:`load_rules_with_skips` can read it inside the +# same transaction that produced it. On a cache hit it is assigned *from the +# cache entry*, so it always describes the rules actually returned. +_rules_skipped_count: int = 0 + +# Reentrant so the load-and-read transaction in :func:`load_rules_with_skips` +# can hold it across its own call to :func:`_load_rules`. +_RULES_LOCK = threading.RLock() def _collect_rule_files(*dirs: Path) -> list[Path]: @@ -334,13 +367,36 @@ def _read_rule_source(rule_file: Path, data: bytes | None = None) -> str: return base64.b64decode("".join(encoded_source.split())).decode("utf-8") +#: Cap on how much of a decode/compile error is echoed into logs. Rule sources +#: are attacker-influenced when ``--yara-rules-dir`` points at untrusted content, +#: and YARA syntax errors can quote the offending source line, so the reason is +#: truncated rather than passed through whole. +MAX_RULE_REJECTION_REASON_CHARS = 200 + + +def _bounded_rejection_reason(exc: Exception) -> str: + """Return a single-line, length-capped description of a rule rejection.""" + reason = " ".join(str(exc).split()) + if len(reason) > MAX_RULE_REJECTION_REASON_CHARS: + reason = f"{reason[:MAX_RULE_REJECTION_REASON_CHARS]}..." + return reason or exc.__class__.__name__ + + def _build_namespace_map( rule_files: list[Path], temp_dir: Path | None = None, *, raw_cache: dict[Path, bytes] | None = None, + namespace_files: dict[str, str] | None = None, ) -> tuple[dict[str, str], int]: - """Build a {namespace: source} dict and count malformed rule files.""" + """Build a {namespace: source} dict and count malformed rule files. + + If ``namespace_files`` is given it is populated with ``{namespace: filename}`` + so a later compile failure can name the file the operator has to fix -- a + namespace has its extension stripped, so it is not a usable filename on its + own. Passed in rather than returned to keep this function's two-value + signature, which existing callers and tests unpack directly. + """ del temp_dir sources: dict[str, str] = {} skipped = 0 @@ -351,17 +407,35 @@ def _build_namespace_map( ns = _rule_namespace(rf) if ns in sources: ns = f"{rf.parent.name}/{ns}" + if namespace_files is not None: + namespace_files[ns] = rf.name try: sources[ns] = _read_rule_source(rf, raw_cache[rf]) except (binascii.Error, UnicodeDecodeError, ValueError) as exc: skipped += 1 - logger.debug("%s: skipping malformed encoded rule %s: %s", ANALYZER_ID, rf, exc) + # WARNING, not DEBUG: a dropped rule silently removes a detector, so + # the operator has to be able to identify and repair the file from a + # default-level run (#554). The filename is named explicitly because + # the ledger event is scoped to the rule set, not to one file. + logger.warning( + "%s: rejected rule file %s (could not decode): %s", + ANALYZER_ID, + rf.name, + _bounded_rejection_reason(exc), + ) return sources, skipped -def _compile_rules(sources: dict[str, str]) -> tuple[yara.Rules | None, int]: +def _compile_rules( + sources: dict[str, str], + *, + namespace_files: dict[str, str] | None = None, +) -> tuple[yara.Rules | None, int]: """Compile YARA rules from a namespace map. Falls back to per-source compilation on error. + ``namespace_files`` maps namespace to filename so a rejection can name the + file the operator has to fix rather than its extension-stripped namespace. + Returns (compiled_rules, skipped_count). """ _enforce_rule_load_deadline() @@ -382,7 +456,14 @@ def _compile_rules(sources: dict[str, str]) -> tuple[yara.Rules | None, int]: good[ns] = source except (yara.SyntaxError, yara.Error) as exc: skipped += 1 - logger.debug("%s: skipping %s: %s", ANALYZER_ID, ns, exc) + # WARNING for the same reason as the decode path above: without it a + # broken detector disappears with no default-level trace (#554). + logger.warning( + "%s: rejected rule file %s (could not compile): %s", + ANALYZER_ID, + (namespace_files or {}).get(ns, ns), + _bounded_rejection_reason(exc), + ) _enforce_rule_load_deadline() compiled = yara.compile(sources=good) if good else None @@ -394,38 +475,118 @@ def _load_rules(extra_dir: Path | None = None) -> yara.Rules | None: """Compile YARA rules from built-in and optional user-supplied directories. Results are cached at module level and reused if directory contents haven't changed. + + Rule files that fail to decode (malformed base64) or fail to compile (YARA + syntax errors) are dropped from the active rule set. The count is recorded + in the module-level ``_rules_skipped_count`` (read via + :func:`rules_skipped_count`) rather than returned here, so this keeps its + original single-value signature and every existing + ``monkeypatch.setattr(static_yara, "_load_rules", ...)`` test double stays + valid; callers that care about the skip count must surface it themselves + or a scan can report ``completed``/SAFE while some of its own detections + never ran (#554). + + A successful load publishes rules, hash and count together as one + :class:`_RuleCacheEntry`, and every path that does not produce usable rules + clears that entry outright. Both halves matter: without the first a cache + hit could answer with another load's count, and without the second the + stale rules would stay reachable under their old hash. + + Callers should prefer :func:`load_rules_with_skips`, which returns both + halves as one value; reading the count separately after this returns is + racy across concurrent scans. """ - global _compiled_rules, _rules_hash # noqa: PLW0603 + global _rule_cache, _rules_skipped_count # noqa: PLW0603 + + with _RULES_LOCK: + # Cleared up front so that a load which raises part way through cannot + # leave a previous load's total readable through + # :func:`rules_skipped_count`. Every return path below assigns its own. + # ``_rule_cache`` is deliberately *not* cleared here: an entry is + # self-consistent, so on an exception it stays a valid answer for its + # own hash rather than forcing a needless recompile. + _rules_skipped_count = 0 + + dirs = [_BUILTIN_RULES_DIR] + if extra_dir and extra_dir.is_dir(): + dirs.append(extra_dir) + elif extra_dir: + logger.warning("%s: user rules directory %s does not exist", ANALYZER_ID, extra_dir) + + rule_files = _collect_rule_files(*dirs) + if not rule_files: + logger.info("%s: no YARA rule files found", ANALYZER_ID) + # Non-populating: discard the entry instead of leaving the previous + # rules cached under their old hash. Keeping them would let the next + # request for that hash return them alongside this load's zero. + _rule_cache = None + return None - dirs = [_BUILTIN_RULES_DIR] - if extra_dir and extra_dir.is_dir(): - dirs.append(extra_dir) - elif extra_dir: - logger.warning("%s: user rules directory %s does not exist", ANALYZER_ID, extra_dir) + raw_cache = _read_rule_bytes_cache(rule_files) + current_hash = _content_hash(rule_files, raw_cache) + cached = _rule_cache + if cached is not None and cached.rules_hash == current_hash: + # The count is taken from the entry, so it describes these rules and + # not whichever load happened to run in between. + _rules_skipped_count = cached.skipped_count + return cached.rules + + namespace_files: dict[str, str] = {} + sources, materialize_skipped = _build_namespace_map( + rule_files, raw_cache=raw_cache, namespace_files=namespace_files + ) + compiled, compile_skipped = _compile_rules(sources, namespace_files=namespace_files) + skipped = materialize_skipped + compile_skipped + _rules_skipped_count = skipped + + if compiled is None: + logger.warning("%s: failed to compile any YARA rules", ANALYZER_ID) + # Non-populating for the same reason as the no-rule-files path above. + _rule_cache = None + return None + + _rule_cache = _RuleCacheEntry( + rules=compiled, + rules_hash=current_hash, + skipped_count=skipped, + ) + loaded = len(sources) - compile_skipped + logger.info("%s: compiled %d YARA rule file(s) (%d skipped)", ANALYZER_ID, loaded, skipped) + return compiled - rule_files = _collect_rule_files(*dirs) - if not rule_files: - logger.info("%s: no YARA rule files found", ANALYZER_ID) - return None - raw_cache = _read_rule_bytes_cache(rule_files) - current_hash = _content_hash(rule_files, raw_cache) - if _compiled_rules is not None and _rules_hash == current_hash: - return _compiled_rules +def load_rules_with_skips(extra_dir: Path | None = None) -> tuple[yara.Rules | None, int]: + """Load rules and return them with their own skip count, as one value. - sources, materialize_skipped = _build_namespace_map(rule_files, raw_cache=raw_cache) - compiled, compile_skipped = _compile_rules(sources) - skipped = materialize_skipped + compile_skipped + The two halves must be obtained in a single locked transaction. Reading the + count separately after :func:`_load_rules` returns lets two concurrent + MCP/graph scans interleave: scan A loads rule set A, scan B loads rule set B + and overwrites the module-level count, then scan A reads B's count. Scan A + would then run rules A while reporting B's skip total -- and if B skipped + nothing, A reports ``completed`` even though one of A's own rules was + dropped, which is exactly the false-clean result #554 is about. - if compiled is None: - logger.warning("%s: failed to compile any YARA rules", ANALYZER_ID) - return None + :func:`_load_rules` is called through the module global so existing + ``monkeypatch.setattr(static_yara, "_load_rules", ...)`` doubles still apply. + """ + with _RULES_LOCK: + rules = _load_rules(extra_dir) + return rules, _rules_skipped_count - _compiled_rules = compiled - _rules_hash = current_hash - loaded = len(sources) - compile_skipped - logger.info("%s: compiled %d YARA rule file(s) (%d skipped)", ANALYZER_ID, loaded, skipped) - return compiled + +def rules_skipped_count() -> int: + """Return how many rule files the rules from the most recent load dropped. + + On a cache hit this is the cached entry's own count, not zero: the whole + point is that the number travels with the rules it describes, so a rule set + that dropped a detector keeps reporting it on every later cache hit. + + Retained for callers that already hold :data:`_RULES_LOCK` or run + single-threaded. Anything reading this straight after :func:`_load_rules` + should use :func:`load_rules_with_skips` instead. + """ + with _RULES_LOCK: + return _rules_skipped_count def _bounded_match_instances( @@ -916,7 +1077,9 @@ def _rule_limit_response( ) deadline_token = _RULE_LOAD_DEADLINE.set(load_budget) try: - rules = _load_rules(extra_dir) + # One transaction: the skip count must describe *these* rules, not + # whatever a concurrent scan loaded in between. + rules, rules_skipped = load_rules_with_skips(extra_dir) except _YaraRuleResourceLimitError as exc: return _rule_limit_response(exc.reason, dict(exc.metrics)) finally: @@ -1072,6 +1235,41 @@ def _rule_limit_response( ) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) + if rules_skipped: + # A rule that fails to compile or decode is dropped from the active + # set with no per-file signal: every scanned component can still + # report COMPLETED, because the rule that would have flagged it + # simply never ran. Surface that as its own ledger event, scoped to + # the rule directory rather than a skill file, so it isn't silently + # absorbed into a clean-looking events list (#554). + events.append( + ledger_event( + # analyzer_id is deliberately omitted. ledger_event derives the + # work identity as ``analyzer_id or f"{record_type}:{phase}"``, + # so passing it would identify this event as + # ``static_yara`` + path -- identical to the planned work item + # for a *scanned component of the same name*. A skill file + # literally named ``yara_rules`` then collides with this event, + # both planned targets resolve to two matching events, and + # reconciliation raises a fatal ``unaccounted_work`` instead of + # the nonfatal partial scan this is meant to record. Falling + # back to ``system:static`` makes the identity disjoint from + # every analyzer work item by construction, so no choice of + # filename can collide -- renaming the synthetic path alone + # would only move the collision to the next unlucky name. + outcome=LedgerOutcome.PARTIAL, + record_type=LedgerRecordType.SYSTEM, + phase="static", + # Not a scanned skill file: a synthetic scope for the rule + # set itself. Ledger paths must be relative POSIX paths, and + # the real rules directory (builtin or --yara-rules-dir) is + # absolute, so it cannot be used here. + path="yara_rules/", + reason=LedgerReason.READ_ERROR, + observed_artifacts=rules_skipped, + limit_artifacts=0, + ) + ) if not events: status = analyzer_status_event( analyzer_id=ANALYZER_ID, diff --git a/tests/nodes/analyzers/test_static_yara.py b/tests/nodes/analyzers/test_static_yara.py index e3fd7cba0..d95d016fa 100644 --- a/tests/nodes/analyzers/test_static_yara.py +++ b/tests/nodes/analyzers/test_static_yara.py @@ -22,7 +22,10 @@ from __future__ import annotations import base64 +import dataclasses import json +import logging +import threading from pathlib import Path from unittest.mock import MagicMock @@ -37,12 +40,17 @@ @pytest.fixture(autouse=True) def _clear_rule_cache(): - """Reset the module-level compiled rules cache between tests.""" - static_yara._compiled_rules = None - static_yara._rules_hash = None + """Reset the module-level compiled rules cache between tests. + + The skip count is part of that cache entry: it is only meaningful alongside + the hash it was produced from, so leaving it set would leak a previous + test's dropped-rule total into the next one. + """ + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 yield - static_yara._compiled_rules = None - static_yara._rules_hash = None + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 def _write_rule( @@ -788,22 +796,22 @@ def test_rules_are_cached(self, tmp_path): tmp_path, "rule_cache", category="malware", severity="HIGH", strings={"a": "CACHETEST"} ) _run("CACHETEST", "f.txt", str(tmp_path)) - first_rules = static_yara._compiled_rules + first_rules = static_yara._rule_cache.rules _run("CACHETEST", "f.txt", str(tmp_path)) - assert static_yara._compiled_rules is first_rules + assert static_yara._rule_cache.rules is first_rules def test_cache_invalidated_on_new_rule(self, tmp_path): _write_rule( tmp_path, "rule_v1", category="malware", severity="HIGH", strings={"a": "V1MARKER"} ) _run("V1MARKER", "f.txt", str(tmp_path)) - first_hash = static_yara._rules_hash + first_hash = static_yara._rule_cache.rules_hash _write_rule( tmp_path, "rule_v2", category="malware", severity="HIGH", strings={"a": "V2MARKER"} ) _run("V2MARKER", "f.txt", str(tmp_path)) - assert static_yara._rules_hash != first_hash + assert static_yara._rule_cache.rules_hash != first_hash # ── Internal helpers ────────────────────────────────────────────────── @@ -953,6 +961,48 @@ def test_build_namespace_map_skips_malformed_encoded_rules(self, tmp_path): assert "invalid" not in ns_map assert skipped == 1 + def test_malformed_rule_is_reported_not_silently_dropped(self, tmp_path, monkeypatch): + """A custom rule that can't compile must not report a clean, SAFE scan (#554). + + Reproduces the issue's own scenario: a valid rule plus a rule with a + YARA syntax error in the same --yara-rules-dir. The good rule must + still fire, but the analyzer status must not be "completed" -- that + claim would be false, since the broken rule never ran against + anything. + """ + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 + monkeypatch.setattr(static_yara, "_BUILTIN_RULES_DIR", tmp_path / "empty_builtin") + (tmp_path / "empty_builtin").mkdir() + + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + (rules_dir / "good.yar").write_text( + 'rule good_rule { meta: category = "malware" ' + 'strings: $a = "ACME_CANARY" condition: $a }' + ) + # Missing closing brace: a real YARA syntax error, not a decode failure. + (rules_dir / "bad.yar").write_text('rule bad_rule { strings: $a = "x" condition: $a') + + result = static_yara.node( + { + "components": ["skill.md"], + "file_cache": {"skill.md": "contains ACME_CANARY"}, + "yara_rules_dir": str(rules_dir), + } + ) + + assert any("good_rule" in f.message for f in result["findings"]), ( + "the valid rule must still fire" + ) + status = result["analyzer_status_events"][0] + assert status["status"] != "completed", "a dropped custom rule must not report a clean scan" + assert any( + event.get("reason_code") == LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in result["inspection_ledger"] + ) + @pytest.mark.parametrize("payload", ["not base64", "not base64 é"]) def test_malformed_extra_encoded_rule_does_not_block_builtin_rules(self, tmp_path, payload): (tmp_path / "bad.yar.b64").write_text(payload) @@ -1322,3 +1372,380 @@ def test_yara_does_not_start_without_one_enforceable_engine_second(self) -> None match.assert_not_called() assert matched.reason == "runtime_limit" assert matched.metrics == {"observed_seconds": 0.0, "limit_seconds": 0.5} + + +class TestRuleSkipAccounting: + """Regressions for the three review findings on the #554 skip-count surface. + + All three share one root shape: the dropped-rule total was reported through + channels not tied to the scan that produced it -- a module global read after + the fact, a ledger work ID shared with component work, and a DEBUG log the + operator never sees at default verbosity. + """ + + @staticmethod + def _isolated_builtin(tmp_path: Path, monkeypatch) -> None: + """Point the built-in rule dir at an empty dir so counts are only ours.""" + builtin = tmp_path / "empty_builtin" + builtin.mkdir(exist_ok=True) + monkeypatch.setattr(static_yara, "_BUILTIN_RULES_DIR", builtin) + + @staticmethod + def _rule_dir(tmp_path: Path, name: str, *, broken: int, good: bool = True) -> Path: + """Build a rule dir with ``broken`` uncompilable rules, optionally one valid one. + + ``good=False`` with ``broken=0`` yields an existing but empty directory, + which is how the "no rule files at all" load path is reached. + """ + rules_dir = tmp_path / name + rules_dir.mkdir(parents=True, exist_ok=True) + marker = f"MARKER_{name.upper()}" + if good: + (rules_dir / "good.yar").write_text( + f'rule good_{name} {{ strings: $a = "{marker}" condition: $a }}' + ) + for index in range(broken): + # Missing closing brace: a real YARA syntax error, not a decode failure. + (rules_dir / f"bad{index}.yar").write_text( + f'rule bad_{name}_{index} {{ strings: $a = "x" condition: $a' + ) + return rules_dir + + def test_skip_count_travels_with_the_rules_it_describes(self, tmp_path, monkeypatch): + """Two loads in sequence must each report their own skip total. + + Deterministic form of the concurrency finding: reading the count as a + separate step after the load is what lets a later load answer for an + earlier one. ``load_rules_with_skips`` returns both halves together, so + the pairing cannot be broken by anything that happens afterwards. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=1) + dir_b = self._rule_dir(tmp_path, "b", broken=0) + + rules_a, skipped_a = static_yara.load_rules_with_skips(dir_a) + rules_b, skipped_b = static_yara.load_rules_with_skips(dir_b) + + assert rules_a is not None + assert rules_b is not None + assert skipped_a == 1, "rule set A dropped one rule and must say so" + assert skipped_b == 0, "rule set B dropped nothing and must not inherit A's count" + + # The separate-read path is what made this unsafe: after B's load the + # module global describes B, so anyone still holding A's rules and + # reading the global now would report a clean scan for A. + assert static_yara.rules_skipped_count() == 0 + + @pytest.mark.parametrize( + ("label", "b_broken", "a_broken"), + [ + # B finds no rule files at all. This path forced the count to zero, + # so A's cache hit reported zero dropped rules: a false-complete scan. + ("no_rule_files", 0, 1), + # B compiles nothing because every one of its rules is rejected. + # Counts are deliberately asymmetric (A drops 2, B drops 1) so an + # inherited count is visible rather than coincidentally equal. + ("all_rejected", 1, 2), + ], + ) + def test_cached_rules_never_report_a_later_loads_skip_count( + self, tmp_path, monkeypatch, label, b_broken, a_broken + ): + """load A -> load a non-populating B -> load A again must still report A's count. + + ``_load_rules`` used to set the skip count and return on both + non-populating paths without replacing *or* clearing the cached rules + and hash. The entry left behind still matched A's hash, so the third + load hit the cache and paired A's rules with B's count -- zero for the + empty/no-files case -- and a rule set that had dropped a detector went + back to reporting a complete scan. + + Deterministic and single-threaded: this is a cache-integrity defect, not + a race, so it reproduces purely from the order of the three loads. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, f"a_{label}", broken=a_broken) + dir_b = self._rule_dir(tmp_path, f"b_{label}", broken=b_broken, good=False) + + rules_a, skipped_a = static_yara.load_rules_with_skips(dir_a) + assert rules_a is not None + assert skipped_a == a_broken + + rules_b, skipped_b = static_yara.load_rules_with_skips(dir_b) + assert rules_b is None, "B must not produce usable rules in this scenario" + + rules_a_again, skipped_a_again = static_yara.load_rules_with_skips(dir_a) + + assert rules_a_again is not None, "A's rules must still be available" + assert skipped_a_again == a_broken, ( + f"rule set A dropped {a_broken} rule(s) but the reload reported " + f"{skipped_a_again}, which is B's count ({skipped_b})" + ) + + @pytest.mark.parametrize( + ("label", "broken", "good"), + [ + ("no_rule_files", 0, False), + ("all_rejected", 1, False), + ], + ) + def test_non_populating_load_leaves_no_cache_entry( + self, tmp_path, monkeypatch, label, broken, good + ): + """A load that yields no usable rules must not leave a populated cache entry. + + Covers the invalidation half directly, so a future change that starts + writing one part of the entry on these paths fails here rather than + only showing up as a wrong skip count three loads later. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, f"seed_{label}", broken=1) + assert static_yara._load_rules(dir_a) is not None + assert static_yara._rule_cache is not None, "the seed load must populate the cache" + + dir_b = self._rule_dir(tmp_path, f"empty_{label}", broken=broken, good=good) + + assert static_yara._load_rules(dir_b) is None + assert static_yara._rule_cache is None, ( + "the previous rules and hash are still cached after a load that " + "produced nothing, so a later request for that hash can be served " + "rules paired with this load's count" + ) + + def test_cache_entry_cannot_be_mutated_in_place(self, tmp_path, monkeypatch): + """The three halves are one immutable value, not three fields to edit. + + Reassigning ``_rule_cache`` wholesale is the only supported way to + publish a change, which is what makes a cache hit unable to mix one + load's rules with another's count. + """ + self._isolated_builtin(tmp_path, monkeypatch) + static_yara._load_rules(self._rule_dir(tmp_path, "frozen", broken=1)) + entry = static_yara._rule_cache + assert entry is not None + assert entry.skipped_count == 1 + + with pytest.raises(dataclasses.FrozenInstanceError): + entry.skipped_count = 0 + + def test_rescan_after_a_non_populating_load_still_reports_the_dropped_rule( + self, tmp_path, monkeypatch + ): + """End to end: the A -> B -> A sequence must not resurrect a clean scan. + + The load-level assertions above pin the count; this pins the user-visible + consequence the issue is actually about. Before the fix the second scan + of A reported ``completed`` with no rule-skip event at all, even though + one of A's own rules had never run. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = self._rule_dir(tmp_path, "scan", broken=1) + empty_dir = self._rule_dir(tmp_path, "empty_scan", broken=0, good=False) + state = { + "components": ["skill.md"], + "file_cache": {"skill.md": "contains MARKER_SCAN"}, + "yara_rules_dir": str(rules_dir), + } + + first = static_yara.node(state) + assert first["analyzer_status_events"][0]["status"] != "completed" + + static_yara.node( + { + "components": ["skill.md"], + "file_cache": {"skill.md": "nothing to match"}, + "yara_rules_dir": str(empty_dir), + } + ) + + second = static_yara.node(state) + + assert any("good_scan" in finding.message for finding in second["findings"]), ( + "the valid rule must still fire on the rescan" + ) + assert second["analyzer_status_events"][0]["status"] != "completed", ( + "a rescan served from cache must not claim a complete scan while one " + "of its own rules is still dropped" + ) + assert any( + event.get("reason_code") is LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in second["inspection_ledger"] + ), "the dropped rule must still be surfaced in the ledger on the rescan" + + def test_load_and_read_is_serialized_against_other_scans(self, tmp_path, monkeypatch): + """The load-and-read pair must be atomic, not merely adjacent. + + Proves the lock is genuinely held across the whole transaction rather + than racing threads and hoping, so the test cannot pass by luck of + timing: mid-transaction, another thread must not be able to acquire the + rules lock at all. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=2) + + lock_was_held: list[bool] = [] + real_load = static_yara._load_rules + + def probing_load(extra_dir=None): + rules = real_load(extra_dir) + acquired_elsewhere: list[bool] = [] + + def try_acquire() -> None: + got = static_yara._RULES_LOCK.acquire(blocking=False) + acquired_elsewhere.append(got) + if got: + static_yara._RULES_LOCK.release() + + probe = threading.Thread(target=try_acquire) + probe.start() + probe.join() + lock_was_held.append(not acquired_elsewhere[0]) + return rules + + monkeypatch.setattr(static_yara, "_load_rules", probing_load) + _, skipped = static_yara.load_rules_with_skips(dir_a) + + assert skipped == 2 + assert lock_was_held == [True], ( + "another scan could enter the load-and-read transaction, so the rules " + "and their skip count are not obtained atomically" + ) + + def test_concurrent_scans_never_report_another_rule_sets_count(self, tmp_path, monkeypatch): + """Under real contention every scan must still see its own total.""" + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=1) + dir_b = self._rule_dir(tmp_path, "b", broken=0) + + mismatches: list[tuple[str, int, int]] = [] + failures: list[BaseException] = [] + observations = 0 + + def scan(label: str, rules_dir: Path, expected: int) -> None: + nonlocal observations + try: + for _ in range(25): + _, skipped = static_yara.load_rules_with_skips(rules_dir) + observations += 1 + if skipped != expected: + mismatches.append((label, expected, skipped)) + except BaseException as exc: # noqa: BLE001 - re-raised in the main thread + failures.append(exc) + + threads = [ + threading.Thread(target=scan, args=("A", dir_a, 1)), + threading.Thread(target=scan, args=("B", dir_b, 0)), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + # An exception inside a worker thread does not fail the test on its own, + # so it is surfaced explicitly -- otherwise this test passes vacuously + # when the scans never actually ran. + assert failures == [], f"a scan thread raised: {failures!r}" + assert observations == 50, f"expected 50 observations, made {observations}" + assert mismatches == [], f"scans observed another rule set's skip count: {mismatches}" + + def test_rule_load_event_does_not_collide_with_a_component_of_the_same_name( + self, tmp_path, monkeypatch + ): + """A skill file named ``yara_rules`` must not collide with the rule-load event. + + The ledger derives a work ID from ``analyzer_id`` plus the normalized + path. The synthetic rule-set scope normalizes to ``yara_rules``, so + attributing the event to ``static_yara`` gave it the same work ID as a + scanned component of that name: both planned targets then resolved to two + matching events and reconciliation raised a fatal ``unaccounted_work`` + instead of recording a nonfatal partial scan. Renaming the synthetic path + alone would only move the collision to the next unlucky filename. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = self._rule_dir(tmp_path, "r", broken=1) + + result = static_yara.node( + { + "components": ["yara_rules"], + "file_cache": {"yara_rules": "contains MARKER_R"}, + "yara_rules_dir": str(rules_dir), + } + ) + + events = result["inspection_ledger"] + work_ids = [event["work_id"] for event in events] + assert len(work_ids) == len(set(work_ids)), ( + "the rule-load event shares a work ID with the scanned component" + ) + + # The planned work the status advertises must be equally distinct, since + # reconciliation requires exactly one event per planned target. + planned = result["analyzer_status_events"][0]["planned_work"] + planned_ids = [target["work_id"] for target in planned] + assert len(planned_ids) == len(set(planned_ids)) + + # The dropped rule is still surfaced, and the scan is partial not clean. + assert result["analyzer_status_events"][0]["status"] != "completed" + assert any( + event.get("reason_code") == LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in events + ) + + @pytest.mark.parametrize( + ("filename", "content", "expected_fragment"), + [ + ("acme.yar", b'rule broken { strings: $a = "x" condition: $a', "could not compile"), + ( + "bom.yar", + b'\xef\xbb\xbfrule bomrule { strings: $a = "y" condition: $a }', + "could not compile", + ), + ( + "bad_utf8.yar", + b'rule u { strings: $a = "\xff\xfe" condition: $a }', + "could not decode", + ), + ], + ) + def test_rejected_rule_is_named_at_default_log_level( + self, tmp_path, monkeypatch, caplog, filename, content, expected_fragment + ): + """Each rejected rule must be reported at WARNING, naming the file (#554). + + A dropped rule removes a detector. At DEBUG the operator gets no signal + at default verbosity, and the ledger event is scoped to the rule set + rather than to one file, so without this the specific file that needs + repairing cannot be identified. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = tmp_path / "rejected" + rules_dir.mkdir() + (rules_dir / filename).write_bytes(content) + + with caplog.at_level(logging.WARNING, logger=static_yara.logger.name): + static_yara._load_rules(rules_dir) + + rejections = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.WARNING and "rejected rule file" in record.getMessage() + ] + assert len(rejections) == 1, f"expected one rejection warning, got {rejections}" + assert filename in rejections[0], f"the warning must name {filename}: {rejections[0]}" + assert expected_fragment in rejections[0] + + def test_rejection_reason_is_length_bounded(self): + """Rule sources can be untrusted, so the echoed reason must be capped.""" + reason = static_yara._bounded_rejection_reason(ValueError("x" * 5_000)) + + assert len(reason) <= static_yara.MAX_RULE_REJECTION_REASON_CHARS + 3 + assert reason.endswith("...") + + def test_rejection_reason_collapses_newlines(self): + """A multi-line YARA error must stay one log line.""" + reason = static_yara._bounded_rejection_reason(ValueError("line one\nline two\r\nthree")) + + assert "\n" not in reason + assert reason == "line one line two three"