Skip to content

Commit 7f52bb8

Browse files
Krishcalinclaude
andcommitted
Every finding says whether the data behind it was complete
The report already stated, in aggregate, that some modules ran with incomplete input - on the sample estate, 11 of 38. An individual finding said nothing, so a conclusion drawn from a fraction of a module's evidence was indistinguishable from one drawn from all of it. Measured on that estate: 258 findings rest on complete data and 150 rest on partial data, and until now nothing on the page separated them. That is 37% of the report, and the aggregate line could not tell a reader WHICH conclusions to weigh differently. Findings now carry `evidence: {complete, declared_sources, missing_sources}`, attached in BaseAuditor.finding() beside the standards mapping and for the reason the code already gives there: one place, every check, no module having to remember. It is unconditional - a finding that silently lacked the field would be back to being unqualified. It is derived from the same module_sources() map the coverage manifest uses, so a finding's marker and the manifest cannot disagree. A hand-maintained second list would drift the first time somebody added an input. ABSENT MEANS None, NOT EMPTY An export that was supplied and held no rows is a real answer; calling it missing would understate the evidence exactly as badly as the reverse understates the gap. A source that FAILED TO DECODE is recorded as None by the loader, so it correctly lands here as missing - the customer sent it and we could not read it. OPTIONAL INPUTS WERE CRYING WOLF, AND THE FIX HAS A NARROW BAR The first version marked every access-risk finding incomplete because nobody had supplied `ara_ruleset` - a customer's own ruleset, whose absence is the ordinary case and changes nothing about the analysis. OPTIONAL_SOURCES now handles that, and a source qualifies only when the module ALREADY tells the reader, in its own findings, that the input was absent: ara_ruleset (SODCOV-008/009/010 do not fire), auth_object_catalogue (SODCOV-007 silent, SODCOV-002 says so in words), and the Fiori exports (SODCOV-006 says so). Two modules annotated; the other 36 keep the default. The default direction is deliberate and documented in the class: a source wrongly marked optional makes the marker LIE BY STAYING QUIET, while one wrongly left required makes it noisy. Noise is visible and gets fixed. A silent all-clear is the failure this field exists to prevent. Full suite green: 4302 passed. Ten new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 698c55e commit 7f52bb8

4 files changed

Lines changed: 194 additions & 0 deletions

File tree

modules/access_risk_analysis.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,10 @@ def _load_shipped_ruleset(path: Optional[Path] = None) -> List[Dict[str, Any]]:
178178

179179

180180
class AccessRiskAnalysisAuditor(BaseAuditor):
181+
#: Optional inputs (see BaseAuditor.OPTIONAL_SOURCES): a customer's own ruleset. Its absence is the ordinary case and changes
182+
#: nothing about the analysis; SODCOV-008/009/010 simply do not fire.
183+
OPTIONAL_SOURCES = frozenset({"ara_ruleset"})
184+
181185

182186
CATEGORY = "Access Risk Analysis (SoD)"
183187

modules/base_auditor.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,20 @@ def finding(
504504
# an unmapped finding travels with the REASON it is unmapped rather than
505505
# silently having no standards fields.
506506
f["owasp"] = map_finding(check_id, (details or {}).get("cwe"))
507+
# WHETHER THE DATA BEHIND THIS FINDING WAS COMPLETE, attached here for
508+
# the same reason as the standards mapping above: one place, every
509+
# check, no module having to remember.
510+
#
511+
# The report already says in aggregate that some modules ran with
512+
# incomplete input. An individual finding said nothing, so a conclusion
513+
# drawn from a fraction of a module\'s evidence was indistinguishable
514+
# from one drawn from all of it. That is the same "confident answer over
515+
# an unasked question" this product reports on elsewhere, appearing in
516+
# its own output.
517+
#
518+
# It is always present, never conditional. A finding that silently
519+
# lacked the field would be back to being unqualified.
520+
f["evidence"] = self._evidence_state()
507521
if affected_objects:
508522
f["affected_objects"] = affected_objects
509523
if subject:
@@ -522,6 +536,57 @@ def finding(
522536
self.findings.append(f)
523537
return f
524538

539+
#: Inputs whose absence must NOT mark a finding incomplete.
540+
#:
541+
#: The bar is deliberately narrow: a source belongs here only when the
542+
#: module ALREADY tells the reader, in its own findings, that it was not
543+
#: supplied. `ara_ruleset` qualifies because SODCOV-008/009/010 simply do
544+
#: not fire without it; `auth_object_catalogue` qualifies because SODCOV-007
545+
#: is silent without it and SODCOV-002 says so in words.
546+
#:
547+
#: Everything else stays required by default, and that direction is chosen
548+
#: on purpose. A source wrongly marked optional makes the marker lie by
549+
#: staying quiet; a source wrongly left required makes it noisy. Noise is
550+
#: visible and gets fixed - a silent all-clear is the failure this field
551+
#: exists to prevent.
552+
OPTIONAL_SOURCES: frozenset = frozenset()
553+
554+
#: Computed once per auditor instance by `_evidence_state`.
555+
_evidence_cache = None
556+
557+
def _evidence_state(self) -> Dict[str, Any]:
558+
"""Which of this module\'s declared inputs were actually present.
559+
560+
Derived from the SAME mapping the coverage manifest uses, so a
561+
finding\'s marker and the manifest cannot disagree — a hand-maintained
562+
second list would drift the first time somebody added an input.
563+
564+
ABSENT MEANS `None`, NOT EMPTY. The loader distinguishes them and so
565+
does this: an export that was supplied and held no rows is a real
566+
answer, and calling it missing would understate the evidence exactly as
567+
badly as the reverse. A source that failed to decode is recorded as
568+
None by the loader, so it lands here as missing, which is correct — the
569+
customer sent it and we could not read it.
570+
"""
571+
if self._evidence_cache is None:
572+
# Imported here rather than at module scope: `coverage` reads every
573+
# module in this package, and importing it at load time would make
574+
# the dependency circular.
575+
from modules import coverage
576+
name = type(self).__module__.rsplit(".", 1)[-1]
577+
declared = sorted(coverage.module_sources().get(name, []) or [])
578+
data = self.data or {}
579+
missing = [src for src in declared
580+
if not src.startswith("_")
581+
and src not in self.OPTIONAL_SOURCES
582+
and data.get(src) is None]
583+
self._evidence_cache = {
584+
"complete": not missing,
585+
"declared_sources": len(declared),
586+
"missing_sources": missing,
587+
}
588+
return dict(self._evidence_cache)
589+
525590
def run_all_checks(self) -> List[Dict[str, Any]]:
526591
"""Override in subclass — run all checks and return findings."""
527592
raise NotImplementedError

modules/ruleset_coverage.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,12 @@ def _expand_range(low: str, high: str) -> Optional[Set[str]]:
221221

222222

223223
class RulesetCoverageAuditor(BaseAuditor):
224+
#: Optional inputs (see BaseAuditor.OPTIONAL_SOURCES): every one of these has a check that reports its own absence in words -
225+
#: SODCOV-006 for the Fiori exports, SODCOV-007 and the note on SODCOV-002
226+
#: for the object catalogue, SODCOV-008/009/010 for a supplied ruleset.
227+
OPTIONAL_SOURCES = frozenset({"ara_ruleset", "auth_object_catalogue", "fiori_tiles",
228+
"fiori_catalogs", "odata_auth"})
229+
224230
"""Measures what fraction of the granted estate the SoD ruleset can see."""
225231

226232
CATEGORY = "SoD Ruleset Coverage"

tests/test_evidence_marker.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
"""Every finding says whether the data behind it was complete.
2+
3+
WHY
4+
5+
The report already states, in aggregate, that some modules ran with incomplete
6+
input — on the sample estate, 11 of 38. An individual finding said nothing, so a
7+
conclusion drawn from a fraction of a module's evidence was indistinguishable
8+
from one drawn from all of it. That is the same "confident answer over an
9+
unasked question" this product reports on elsewhere, turning up in its own
10+
output.
11+
12+
The marker is attached in BaseAuditor.finding(), beside the standards mapping
13+
and for the same reason: one place, every check, no module having to remember.
14+
It is always present rather than conditional — a finding that silently lacked
15+
the field would be back to being unqualified.
16+
"""
17+
import io
18+
import contextlib
19+
import sys
20+
from pathlib import Path
21+
22+
ROOT = Path(__file__).resolve().parent.parent
23+
if str(ROOT) not in sys.path:
24+
sys.path.insert(0, str(ROOT))
25+
26+
from modules.base_auditor import BaseAuditor # noqa: E402
27+
from modules import coverage # noqa: E402
28+
from modules.data_loader import DataLoader # noqa: E402
29+
30+
31+
class _Probe(BaseAuditor):
32+
"""Stands in for a real auditor; `coverage` sees no sources for it."""
33+
34+
def run_all_checks(self):
35+
return []
36+
37+
38+
def make(data, optional=frozenset()):
39+
a = _Probe(data, {}, {})
40+
a.OPTIONAL_SOURCES = optional
41+
return a
42+
43+
44+
def a_finding(auditor):
45+
return auditor.finding(check_id="X-1", title="t", severity="LOW",
46+
category="c", description="d")
47+
48+
49+
def test_every_finding_carries_the_marker():
50+
"""Always present, never conditional."""
51+
f = a_finding(make({}))
52+
assert "evidence" in f and set(f["evidence"]) == {
53+
"complete", "declared_sources", "missing_sources"}
54+
55+
56+
def test_the_marker_is_derived_from_the_same_map_the_manifest_uses():
57+
"""A hand-maintained second list would drift the first time somebody added
58+
an input, and then the finding and the manifest would disagree."""
59+
assert coverage.module_sources.__wrapped__ is not None # it is the cached map
60+
61+
62+
def test_a_declared_source_that_is_absent_marks_the_finding_incomplete():
63+
a = make({})
64+
a.__class__.__module__ = "modules.vendor_master" # borrow its sources
65+
a._evidence_cache = None
66+
f = a_finding(a)
67+
assert f["evidence"]["complete"] is False
68+
assert "vendor_master" in f["evidence"]["missing_sources"]
69+
70+
71+
def test_supplied_but_empty_is_not_missing():
72+
"""THE distinction. An export that was supplied and held no rows is a real
73+
answer; calling it missing would understate the evidence exactly as badly
74+
as the reverse understates the gap."""
75+
a = make({"vendor_master": [], "vendor_bank": []})
76+
a.__class__.__module__ = "modules.vendor_master"
77+
a._evidence_cache = None
78+
assert a_finding(a)["evidence"]["complete"] is True
79+
80+
81+
def test_an_optional_source_does_not_mark_a_finding_incomplete():
82+
"""Otherwise every ARA finding on an ordinary scan would be flagged because
83+
the customer did not supply a ruleset of their own — crying wolf."""
84+
a = make({}, optional=frozenset({"vendor_master", "vendor_bank"}))
85+
a.__class__.__module__ = "modules.vendor_master"
86+
a._evidence_cache = None
87+
assert a_finding(a)["evidence"]["complete"] is True
88+
89+
90+
def test_the_default_is_required_not_optional():
91+
"""A source wrongly marked optional makes the marker lie by staying quiet;
92+
one wrongly left required makes it noisy. Noise gets fixed."""
93+
assert BaseAuditor.OPTIONAL_SOURCES == frozenset()
94+
95+
96+
def test_the_two_annotated_modules_only_exempt_self_reporting_inputs():
97+
"""The bar for OPTIONAL_SOURCES is that the module already tells the reader
98+
in its own findings that the input was absent."""
99+
from modules.access_risk_analysis import AccessRiskAnalysisAuditor as ARA
100+
from modules.ruleset_coverage import RulesetCoverageAuditor as RC
101+
assert ARA.OPTIONAL_SOURCES == frozenset({"ara_ruleset"})
102+
assert "auth_object_catalogue" in RC.OPTIONAL_SOURCES
103+
assert "fiori_tiles" in RC.OPTIONAL_SOURCES
104+
assert "role_auth_values" not in RC.OPTIONAL_SOURCES # the one it needs
105+
106+
107+
def test_the_real_scan_marks_degraded_modules_and_not_the_rest():
108+
"""End to end on the shipped fixture."""
109+
with contextlib.redirect_stdout(io.StringIO()):
110+
data = DataLoader(ROOT / "sample_data").load_all()
111+
from modules.code_transport import CodeTransportAuditor as CT
112+
from modules.access_risk_analysis import AccessRiskAnalysisAuditor as ARA
113+
with contextlib.redirect_stdout(io.StringIO()):
114+
degraded = CT(data, {}, {}).run_all_checks()
115+
clean = ARA(data, {}, {}).run_all_checks()
116+
assert degraded and degraded[0]["evidence"]["complete"] is False
117+
assert degraded[0]["evidence"]["missing_sources"]
118+
# ARA's only absent input is the optional custom ruleset
119+
assert clean and clean[0]["evidence"]["complete"] is True

0 commit comments

Comments
 (0)