op-connect: an unrun PEP screen is not a clearance #286
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/openpay-screening-unrun-is-not-clear"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The defect
ConnectScreenertakes its PEP and adverse-media screeners asOption<Screener>, documented so that "operators who only have a sanctions feed can still use this crate (PEP / AM are then no-ops returning empty hit-vectors)". Both screens are gated onif let Some(..), so withpep: Noneneitherpep_hitsnorany_reviewis ever touched, and the fall-through returnedDecision::Clear— whose own doc comment read "No hits anywhere; safe to proceed."The module doc states that at each
BeneficialOwnerssubmission the platform must "screen every person against a PEP watchlist ... per FATF Recommendation 12 and EU AMLD5 Art. 20". So the crate emitted safe to proceed for a screen it is required to run and did not run.ScreeningResulthad no field recording which screens ran, so the persisted row was indistinguishable from one where a real PEP feed was queried and returned nothing. A beneficial owner who is a sitting head of state, onboarded under a sanctions-only configuration, produced the same record as a genuine three-screen clearance — the required enhanced due diligence never triggers and the gap is invisible on review.The crate's own test
clear_when_lists_emptybuilt exactly that configuration and assertedClear, pinning the behaviour.This is the "unknown conflated with a value" family, and here the value it was conflated with is the best possible one.
The fix
Unknown becomes a distinct value rather than a clearance:
ScreenCoverage { pep, adverse_media }records which optional screens ran and travels with the hits. Sanctions is deliberately absent from it: it cannot be absent from the run, becauseConnectScreener::newtakes it by value, so a bool could never befalse.#[serde(default)]makes a stored row that does not state its coverage default to neither ran — a record that does not say cannot assert it.Decision::Incompletereports a mandated screen that did not run. Precedence isBlock>Review>Incomplete>Clear: a sanctions hit outranks everything, and a hit needing adjudication outranks a missing screen because the gap is reported independently incoverage.Decision::may_proceed()is true only forClear, so a caller cannot readIncompleteas proceedable by forgetting a match arm.The mandated/optional line is taken from the crate's own doc rather than invented: items 1–3 are "must", item 4 is "Optionally". A missing adverse-media screen is therefore recorded but still clears; a missing PEP screen does not.
Tests
clear_when_lists_emptyis replaced by a discriminating pair with identical empty hit-vectors and opposite decisions — which is the distinction the old single test could not express:pep_hitsan_unrun_pep_screen_is_not_a_clearancenew(sanctions, None, None)Incompletea_pep_screen_that_ran_and_found_nothing_is_a_clearancenew(sanctions, Some(empty), Some(empty))ClearPlus
a_missing_optional_screen_still_clears_but_is_recorded(adverse media absent → recorded, still clears) anda_pep_hit_routes_to_review(a hit still reviews rather than blocks, with full coverage).Falsification
Each test was shown to fail against a mutated implementation, so none is decoration:
else { Clear })an_unrun_pep_screen_is_not_a_clearanceFAILEDmandated_screens_ran()→truean_unrun_pep_screen_is_not_a_clearanceFAILEDcoverage.pep→falseM3 is the check that the positive tests have teeth; M1 is the regression test for the defect itself.
Verification
Run on the build host (aarch64), matching what
standards-build.ymlruns foropenpay:cargo check --workspace --all-targets— cleancargo test --workspace— exit status 0, 0 failed suites, 117 suites with passing testscargo clippy -p op-connect --all-targets --no-deps -- -D warnings— clean (not a CI gate for openpay; run anyway)Scope
No wire or vector impact:
op-connectships no conformance vectors (onlyop-rails-a2a,op-iso20022andop-emvdo), andgrepfinds no serializedScreeningResultanywhere in the tree.ConnectScreener/ScreeningResult/Decisionhave no consumer outside the crate beyond thelib.rsre-export, and nothing in the repo matches onDecision, so the new variant breaks no match.Noted, not fixed here
Two adjacent findings in the same subsystem, left for separate changes because they are a different kind of change:
Error::ScreeningBlockedis defined and never constructed.OnboardingProvider::submit_step's doc promises it ("if screening fired on a step") and this module's doc claims theScreeningResult"gates theStepResult", butNativeProvider'sStepPayload::BeneficialOwnersarm validates FinCEN CDD and merges the roster without ever screening.tests/end_to_end.rswalks a full onboarding toActivewith no screen performed. Wiring it is not a small change:submit_stepisasyncand holdsstd::sync::MutexGuards, which cannot be held across the.awaitofscreen()underasync_trait'sSendbound, so it needs a two-phase lock (read, drop, screen, re-lock) plus a decision on requirement semantics forReview/Incompleteand on not persisting a partial mutation whenBlockreturnsErr. That belongs in its own PR.annotate_pep_flagsclobbersis_peptofalsewhen its index returns no hits, discarding a flag that may have been set from another source. Arguably its documented job ("re-running the PEP screen and toggling"), so recorded rather than changed.🤖 Generated with Claude Code
`ConnectScreener` takes its PEP and adverse-media screeners as `Option<Screener>`, documented so that "operators who only have a sanctions feed can still use this crate (PEP / AM are then no-ops returning empty hit-vectors)". Both screens are gated on `if let Some(..)`, so with `pep: None` neither `pep_hits` nor `any_review` is ever touched, and the fall-through returned `Decision::Clear` — whose own doc comment read "No hits anywhere; safe to proceed." The module doc states that at each BeneficialOwners submission the platform *must* "screen every person against a PEP watchlist ... per FATF Recommendation 12 and EU AMLD5 Art. 20". So the crate emitted "safe to proceed" for a screen it is required to run and did not run. `ScreeningResult` recorded no field saying which screens had run, so the persisted row was indistinguishable from one where a real PEP feed was queried and returned nothing: a sitting head of state onboarded under a sanctions-only configuration produced the same record as a genuine three-screen clearance, and the gap was invisible on review. The crate's own test `clear_when_lists_empty` built exactly that configuration and asserted `Clear`, pinning the behaviour. Unknown is now a distinct value rather than the best possible one: - `ScreenCoverage { pep, adverse_media }` records which optional screens ran, and travels with the hits. Sanctions is absent from it because it cannot be absent from the run — `new` takes it by value, so a bool could never be false. `#[serde(default)]` makes a stored row that does not state its coverage default to "neither ran": a record that does not say cannot assert it. - `Decision::Incomplete` reports a mandated screen that did not run. Precedence is `Block` > `Review` > `Incomplete` > `Clear`; a hit needing adjudication outranks a missing screen because the gap is reported independently in `coverage`. - `Decision::may_proceed()` is true only for `Clear`, so a caller cannot read `Incomplete` as proceedable by forgetting a match arm. The mandated/optional line is drawn from the crate's own doc: items 1-3 are "must", item 4 is "Optionally", so a missing adverse-media screen is recorded but still clears, and a missing PEP screen does not. `clear_when_lists_empty` is replaced by a discriminating pair with identical empty hit-vectors and opposite decisions — unconfigured PEP yields `Incomplete`, a configured-but-empty PEP index yields `Clear` — which is the distinction the old single test could not express. Co-Authored-By: Claude Opus 5 <[email protected]>