Skip to content

Commit f0b7a2b

Browse files
Krishcalinclaude
andcommitted
The trust verdict has to know what was suppressed before it says USABLE
SODCOV-000 read USABLE / INFO on an estate whose only conflict had been removed by a blanket mitigation row carrying no approver, no expiry and no control id. The statement that exists to say how far a segregation result can be believed was the last place in the product still capable of a false all-clear - and it renders above the findings in HTML, PDF and PPTX, and on the console dashboard. That gap was mine: SODCOV-000 was built two commits before MITIG and never connected to it. every conflict hidden by a blanket row before USABLE / INFO after PARTIAL / HIGH Two limits are now stated: that a clean result in those areas reflects a suppression rather than an absence, and that the rows doing the suppressing cannot support an audit conclusion. DERIVED, NOT RECOMPUTED The scanner hands the findings so far to the coverage module, so the verdict reads MITIG-001's own conclusion instead of re-deriving suppression from the mitigation rows. That preserves the rule set when the statement was written: a summary that recalculates can disagree with the detail it summarises, and it does so in front of the reader least able to tell which half to believe. ABSENCE OF SUPPRESSION DATA IS NOT ABSENCE OF SUPPRESSION Run without peer findings, the statement records `suppression_checked: false` and says so in its own description rather than quietly claiming a clean verdict. Claiming one without having looked would be the same defect one level up. The verdict still takes the FLOOR of its measures, so suppression can only make it worse, never better. A GUARD I DID NOT WRITE CAUGHT THE FIRST ATTEMPT, AND ITS FIX WAS BETTER The first version passed `dict(run_ctx, peer_findings=...)` - a hand-rolled context variant, which is exactly the shape tests/test_run_context_is_uniform.py was written to eliminate. Its docstring explains why that matters: when the next field is added to run_ctx, the omissions look like modules choosing not to use it rather than never being offered it. `peer_findings` now goes INTO run_ctx, so every auditor still receives the same object. Full suite green: 4480 passed. Smoke run 410 findings. Six new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 511255e commit f0b7a2b

3 files changed

Lines changed: 146 additions & 3 deletions

File tree

modules/ruleset_coverage.py

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,27 @@ def detail(cid, key, default=None):
585585

586586
limits, verdict, severity = [], "usable", self.SEVERITY_INFO
587587

588+
# WHAT WAS HIDDEN BEFORE THE READER GOT HERE.
589+
#
590+
# This verdict read USABLE / INFO on an estate whose only conflict had
591+
# been removed by a blanket mitigation row carrying no approver, no
592+
# expiry and no control id. The statement that exists to say how far a
593+
# result can be believed was the last place in the product still capable
594+
# of a false all-clear, and it was rendered above the findings in every
595+
# format and on the console dashboard.
596+
#
597+
# Read from the peer findings rather than recomputed from the mitigation
598+
# rows: MITIG-001 already decided what was suppressed, and a summary
599+
# that recalculates can disagree with the detail it summarises.
600+
peers = {f.get("check_id"): f
601+
for f in ((self.run_context or {}).get("peer_findings") or [])
602+
if isinstance(f, dict)}
603+
suppression_checked = bool(peers)
604+
hidden = ((peers.get("MITIG-001", {}).get("details") or {})
605+
.get("risks_hidden_entirely") or 0)
606+
unsupportable = ((peers.get("MITIG-002", {}).get("details") or {})
607+
.get("unsupportable") or 0)
608+
588609
# The floor, in order of how badly each one undermines the result.
589610
if "SODCOV-004" in seen:
590611
verdict = "not measured"
@@ -616,6 +637,23 @@ def detail(cid, key, default=None):
616637
"the ruleset names %.0f%% of the transactions this estate "
617638
"grants" % (tx * 100))
618639

640+
if hidden:
641+
# A conflict removed from the report is not a conflict that was not
642+
# there, and a verdict that cannot see the difference is worthless.
643+
verdict = "partial" if verdict == "usable" else verdict
644+
severity = max(severity, self.SEVERITY_HIGH, key=self._rank)
645+
limits.append(
646+
"%d risk(s) were removed from this report entirely by a "
647+
"mitigating control (MITIG-001), so a clean result in those "
648+
"areas reflects a suppression rather than an absence" % hidden)
649+
if unsupportable:
650+
severity = max(severity, self.SEVERITY_HIGH, key=self._rank)
651+
limits.append(
652+
"%d mitigating control row(s) cannot support an audit "
653+
"conclusion (MITIG-002) - a blanket entry, a missing approver "
654+
"or a missing expiry - yet still suppress conflicts"
655+
% unsupportable)
656+
619657
dead = detail("SODCOV-007", "rules_unfirable") or []
620658
if dead:
621659
severity = max(severity, self.SEVERITY_HIGH, key=self._rank)
@@ -662,7 +700,11 @@ def detail(cid, key, default=None):
662700
"" if not limits else " Stated limits: " + "; ".join(limits) + ".",
663701
"" if limits else
664702
" No limit was found that would qualify it: coverage was "
665-
"measurable, bounded, and broad on every surface examined.")),
703+
"measurable, bounded, and broad on every surface examined."
704+
+ ("" if suppression_checked else
705+
" NOTE: whether any conflict was suppressed by a "
706+
"mitigating control was not examined in this run, so "
707+
"this statement covers ruleset coverage only."))),
666708
affected_items=["limit: " + l for l in limits],
667709
remediation=(
668710
"1. Quote the verdict, not the conflict count, when recording "
@@ -682,8 +724,13 @@ def detail(cid, key, default=None):
682724
details={
683725
"verdict": verdict,
684726
"limits": limits,
685-
"derived_from": sorted(c for c in seen if c.startswith("SODCOV-")
686-
and c != "SODCOV-000"),
727+
"derived_from": sorted(
728+
[c for c in seen
729+
if c.startswith("SODCOV-") and c != "SODCOV-000"]
730+
+ [c for c in peers if c.startswith("MITIG-")]),
731+
# Absence of suppression data is NOT an absence of suppression.
732+
"suppression_checked": suppression_checked,
733+
"risks_hidden_by_mitigation": hidden,
687734
"coverage_state": "complete",
688735
},
689736
scope="aggregate",

sap_scanner.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,17 @@ def main():
558558

559559
if "sodcov" in run_modules:
560560
print("[*] Running SoD Ruleset Coverage (what fraction of this estate the ruleset can see)...")
561+
# SODCOV-000 is the one sentence on how far the SoD result can be
562+
# believed, and a conflict SUPPRESSED by a mitigating control is
563+
# exactly the sort of thing that qualifies it. Access risk analysis
564+
# runs above and reports its suppressions as MITIG-001, so the
565+
# findings so far are handed over rather than recomputed - a
566+
# summary that recalculates can disagree with its own detail.
567+
# Into run_ctx itself, not a hand-rolled variant beside it.
568+
# tests/test_run_context_is_uniform.py exists precisely to stop the
569+
# call shapes diverging again, and passing a different dict here would
570+
# have reintroduced the shape it was written to eliminate.
571+
run_ctx["peer_findings"] = list(all_findings)
561572
auditor = RulesetCoverageAuditor(data, baseline_overrides, run_ctx)
562573
findings = auditor.run_all_checks()
563574
all_findings.extend(findings)

tests/test_ruleset_coverage.py

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -686,3 +686,88 @@ def test_the_limits_are_ordered_worst_first():
686686
limits = one(RulesetCoverageAuditor(data).run_all_checks(),
687687
"SODCOV-000")["details"]["limits"]
688688
assert "grants every transaction" in limits[0]
689+
690+
691+
# ── the verdict has to know what was hidden ────────────────────────────────
692+
#
693+
# SODCOV-000 read USABLE / INFO on an estate whose only conflict had been
694+
# removed by a blanket mitigation row carrying no approver, no expiry and no
695+
# control id. The statement that exists to say how far a result can be believed
696+
# was the last place in the product still capable of a false all-clear — and it
697+
# renders above the findings in every format and on the console dashboard.
698+
699+
from modules.access_risk_analysis import AccessRiskAnalysisAuditor as _ARA2 # noqa: E402
700+
701+
SUPPRESSIBLE = [{"risk_id": "ZS", "name": "vendor create vs pay",
702+
"description": "d", "risk_type": "SOD", "severity": "CRITICAL",
703+
"process": "P2P", "functions": [
704+
{"name": "A", "actions": ["FK01"], "permissions": [
705+
{"object": "F_LFA1_BUK", "field": "ACTVT",
706+
"values": ["02"]}]},
707+
{"name": "B", "actions": ["F110"], "permissions": [
708+
{"object": "F_REGU_BUK", "field": "ACTVT",
709+
"values": ["02"]}]}]}]
710+
711+
HOLDS_IT = [row(low="FK01"), row(low="F110"),
712+
row(obj="F_LFA1_BUK", field="ACTVT", low="02"),
713+
row(obj="F_REGU_BUK", field="ACTVT", low="02")]
714+
715+
716+
def with_peers(mitigations=None):
717+
"""Run ARA first, then the coverage module over the same estate."""
718+
data = {"role_auth_values": HOLDS_IT,
719+
"user_roles": [{"UNAME": "U1", "AGR_NAME": "Z_ROLE"}]}
720+
if mitigations is not None:
721+
data["mitigating_controls"] = mitigations
722+
ara = _ARA2(dict(data))
723+
ara.RULESET = SUPPRESSIBLE
724+
peers = ara.run_all_checks()
725+
cov = RulesetCoverageAuditor(dict(data), {}, {"peer_findings": peers},
726+
ruleset=SUPPRESSIBLE)
727+
return one(cov.run_all_checks(), "SODCOV-000")
728+
729+
730+
BLANKET = [{"USER": "U1", "RISK_ID": "*", "CONTROL_ID": "", "VALID_TO": ""}]
731+
732+
733+
def test_an_unsuppressed_estate_still_reads_usable():
734+
f = with_peers()
735+
assert f["details"]["verdict"] == "usable"
736+
assert f["details"]["suppression_checked"] is True
737+
738+
739+
def test_a_risk_hidden_by_a_mitigation_stops_the_verdict_reading_usable():
740+
"""THE false all-clear. A conflict removed from the report is not a
741+
conflict that was not there."""
742+
f = with_peers(BLANKET)
743+
assert f["details"]["verdict"] == "partial"
744+
assert f["severity"] == "HIGH"
745+
assert f["details"]["risks_hidden_by_mitigation"] == 1
746+
747+
748+
def test_the_limit_says_a_suppression_is_not_an_absence():
749+
limits = " ".join(with_peers(BLANKET)["details"]["limits"])
750+
assert "reflects a suppression rather than an absence" in limits
751+
752+
753+
def test_unsupportable_rows_are_named_as_their_own_limit():
754+
limits = " ".join(with_peers(BLANKET)["details"]["limits"])
755+
assert "cannot support an audit conclusion" in limits
756+
757+
758+
def test_the_verdict_names_the_mitigation_checks_it_was_derived_from():
759+
"""Derived, not recomputed — a summary that recalculates can disagree with
760+
the detail it summarises."""
761+
derived = with_peers(BLANKET)["details"]["derived_from"]
762+
assert "MITIG-001" in derived and "MITIG-002" in derived
763+
764+
765+
def test_without_peer_findings_it_says_suppression_was_not_examined():
766+
"""Absence of suppression data is NOT an absence of suppression. Claiming
767+
a clean verdict without having looked would be the same defect one level
768+
up."""
769+
a = RulesetCoverageAuditor({"role_auth_values": HOLDS_IT},
770+
ruleset=SUPPRESSIBLE)
771+
f = one(a.run_all_checks(), "SODCOV-000")
772+
assert f["details"]["suppression_checked"] is False
773+
assert "not examined in this run" in f["description"]

0 commit comments

Comments
 (0)