andodeki
23fce675de
Option A, as requested. Plus an audit of the whole review against the code. ## The default flips nigig-pay-ui default = [] (leaf stays off; see below) nigig-pay default = ["demo"] nigig-mpesa default = ["demo"] pageflipnav default = ["native", "demo"] `cargo run -p pageflipnav` now drives *334#, shows the PIN field and dispatches. That is the app's primary function and it works out of the box. A release build opts out: cargo build -p pageflipnav --no-default-features --features native This was not one line. A first attempt flipped the four `default =` lines and the opt-out still leaked: the app crates depended on nigig-pay-ui with its own defaults, so `--no-default-features` on pageflipnav was silently re-enabled one level down. Verified with a compile probe rather than cargo tree, which truncates. The inner deps now carry `default-features = false`, and the probe confirms both directions: default -> demo ON, --no-default-features -> demo OFF. ADR 0007's Play-policy note is untouched. The containment requirement of review item 0.1 is not dropped — the flag exists, CI exercises both directions, and a shipped build still cannot dispatch. What changed is which way it points by default, so development and device testing are not fighting it. CI guards inverted to match: they now assert automation is on by default *and* that the packaging opt-out still works. check-no-pin-capture.sh now probes the packaging build, since the default legitimately captures a PIN. ## REVIEWS/IMPLEMENTATION_AUDIT.md Every phase checked against the code, not against the tranche notes. Where they disagreed the code won. Summary: phases 0-5 and 7 done bar 5.2 and Keystore provisioning; phase 6 substantially done with the thread_local session ownership outstanding; phase 8 partly. ## B7 found live while auditing Month navigation had never been examined. Both copies of the transactions widget still stepped months with Duration::days(31) and years with Duration::days(366). Reproduced before touching it: 2025-12-28 -1 month => 2025-11-27 (drifts a day) 2026-03-30 -1 month => 2026-02-27 (drifts; repeated steps skip a month) 2027-06-15 +1 year => 2028-06-15 (366d wrong on a non-leap year) Now uses checked_add_months/checked_sub_months, which clamp to the end of the target month, and checked_add_signed on the day path. An unrepresentable date leaves the view where it was. Neither claimed done nor flagged open — simply never looked at. That is the argument for auditing code rather than notes. ## Validation domain 137 / storage 41 / platform 64 / mpesa 29 / pay-ui 61 pass cargo check: pay-ui, pay, mpesa, core (default and opt-out) pass demo-on-by-default probe, both directions pass no-PIN-capture guard against the packaging build pass Unrelated and still blocking a full APK: nigig-map fails to compile on clean HEAD (12 errors, no field center_lat on ViewportState).
216 lines
9.7 KiB
Markdown
216 lines
9.7 KiB
Markdown
# Nigig Pay — implementation audit against the consolidated review
|
||
|
||
- **Date:** 2026-07-29
|
||
- **Reviewed against:** `REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md`
|
||
- **Method:** every claim below was checked against the code, not against the
|
||
tranche notes in `PAY_CAD_IMPLEMENTATION_STATUS.md`. Where the two disagree,
|
||
the code wins and the disagreement is recorded.
|
||
|
||
## Summary
|
||
|
||
| Phase | Scope | State |
|
||
|---|---|---|
|
||
| 0 | Immediate containment | **Done**, with 0.1 deliberately re-pointed (below) |
|
||
| 1 | Build, ownership, quality gates | **Done** for the owned crates |
|
||
| 2 | Domain layer | **Done** |
|
||
| 3 | Secure repository | **Done** except Android Keystore provisioning |
|
||
| 4 | No blocking I/O in `draw_walk` | **Done and verified** |
|
||
| 5 | Harden platform gateways | **Done** except 5.2, which is not an engineering task |
|
||
| 6 | Truthful UX | **Substantially done**; session ownership outstanding |
|
||
| 7 | Test strategy | **Done** except an adversarial corpus for the UI layer |
|
||
| 8 | Verification and release discipline | **Partly**; simulator and property tests exist, pilot discipline does not |
|
||
|
||
**Test counts:** domain 137, platform 64, storage 41, pay UI 61, M-Pesa
|
||
observation 29. All pass under the isolated runners.
|
||
|
||
---
|
||
|
||
## Phase 0 — containment
|
||
|
||
| Item | State | Evidence |
|
||
|---|---|---|
|
||
| 0.1 dispatch behind a compile-time flag | **Implemented, default re-pointed** | `demo` exists at every level and CI exercises both directions. It is now **on** by default because USSD automation is the product's primary function; a release build uses `--no-default-features`. Containment moved to packaging, not removed. |
|
||
| 0.2 no fail-open biometric | **Done** | `AuthorizationAttempt`: only an explicit success grants dispatch. A displayed prompt does not. |
|
||
| 0.3 remove PIN capture | **Done for packaging builds** | `form_pin` is `#[cfg(feature = "demo")]`; `tools/check-no-pin-capture.sh` proves by compile probe that it does not exist under `--no-default-features`. A default build captures a PIN because it dispatches. |
|
||
| 0.4 SMS/USSD output is evidence, not verification | **Done** | `ObservedEvidence`; no API returns a settled state from a parse. |
|
||
| 0.5 no fake desktop success | **Done** | `MockGateway` is `cfg`-gated and `compile_error!`s in release without an explicit opt-in. |
|
||
| 0.6 stop persisting raw SMS | **Done** | Only a digest is retained; `store.rs` omits `raw_message` from disk. |
|
||
| 0.7 pinned toolchain / recovery branch | **Done** | `rust-toolchain.toml`, `--locked` builds. |
|
||
| 0.8 threat model and risk register | **Done** | `THREAT_MODEL.md`, `PAYMENT_RISK_REGISTER.md`. |
|
||
|
||
### On 0.1, explicitly
|
||
|
||
The review asked for a compile-time flag so that a *production* build cannot
|
||
dispatch. That was implemented as default-off, which had two consequences the
|
||
review did not intend:
|
||
|
||
1. ordinary development and device testing could not use the app's main
|
||
feature without knowing an undocumented flag;
|
||
2. the flag was **unreachable from `pageflipnav`**, the crate that builds the
|
||
APK, until commit `9f0e133`. No APK could enable automation by any means.
|
||
|
||
The flag now defaults on and a packaging build opts out. The safety property
|
||
the review wanted — *a shipped build cannot dispatch* — is still available and
|
||
CI-enforced; it is now a release decision rather than a development obstacle.
|
||
|
||
---
|
||
|
||
## Phase 1 — build, ownership, quality gates
|
||
|
||
Done for `nigig-pay-domain`, `nigig-pay-storage`, `nigig-pay-platform`:
|
||
checked-in lockfiles, `--locked` builds, `cargo fmt --check`,
|
||
`clippy -D warnings`, `cargo-deny`, and a Makepad-import boundary check.
|
||
Ownership is recorded in ADR 0002 and machine-checked.
|
||
|
||
**Not done:** the same gates do not apply to `nigig-pay-ui`, which is linted
|
||
only by `cargo test --lib`. Residual counts there: **10 `unwrap()`**, **6
|
||
`let _ =`**, 0 `dbg!`/`println!`.
|
||
|
||
---
|
||
|
||
## Phase 2 — domain layer
|
||
|
||
Complete. `Money` (exact minor units), the validated state machine,
|
||
`PaymentCoordinator`, correlated operations, separated outcomes, no automatic
|
||
retry on ambiguity, and one `compute_adjusted_amount`.
|
||
|
||
---
|
||
|
||
## Phase 3 — secure repository
|
||
|
||
SQLite with migrations, single writer, `Result` on every operation,
|
||
retention/erasure policy, integrity checks, and online backup/restore.
|
||
SQLCipher works and is tested under a feature.
|
||
|
||
**Not done:** 3.1 key provisioning. The Android Keystore `DatabaseKeyProvider`
|
||
does not exist, so encryption at rest is available but unkeyed by a real
|
||
hierarchy.
|
||
|
||
---
|
||
|
||
## Phase 4 — blocking I/O
|
||
|
||
Done and **verified by compilation**, which earlier tranches could not do.
|
||
`reload()` is out of `draw_walk`, the SMS scan is on a real timer, summaries
|
||
are computed on the event path, and the storage worker owns the only writer.
|
||
|
||
---
|
||
|
||
## Phase 5 — platform gateways
|
||
|
||
`nigig-pay-platform` owns the seam: single-flight correlation, fail-closed
|
||
classification driven by dispatch progress, typed correlated events, a strict
|
||
SMS reader, a mock that cannot ship, and an honest web refusal.
|
||
`#![forbid(unsafe_code)]`; `UssdGateway` is `!Send`/`!Sync`.
|
||
|
||
**Not done — 5.2, the AccessibilityService legal review.** This is a business
|
||
decision about Google Play policy exposure, recorded as blocking in ADR 0007.
|
||
No amount of engineering closes it.
|
||
|
||
---
|
||
|
||
## Phase 6 — truthful UX
|
||
|
||
`PaymentPresentation` makes the exit criterion a type: no `Success` variant is
|
||
reachable from untrusted evidence, and no `Failed` variant from a missing SMS.
|
||
`ConfirmationSummary` makes every required term mandatory and binds consent to
|
||
a digest of the exact terms shown. `PaymentBatch` makes `3/2` progress
|
||
unrepresentable.
|
||
|
||
**Not done:** the `thread_local!` `PayFlowHandler` still owns the USSD session
|
||
lifecycle, the shadow pending-store writes, and the bulk queue. Moving those
|
||
changes who cancels on teardown and who observes an out-of-order callback,
|
||
which needs ADR 0007's device matrix.
|
||
|
||
**Not done:** 6.6 bulk controls — per-item review, limits, pause/resume and
|
||
audit export.
|
||
|
||
---
|
||
|
||
## Phase 7 — tests
|
||
|
||
7.1–7.2, 7.4, 7.6–7.8 done. 7.3 done: UI tests run in CI. 7.5 done for the
|
||
platform SMS reader — spoofing, forged codes, replay, unicode.
|
||
|
||
**Not done:** an adversarial corpus for the UI layer specifically, and no
|
||
consent-safe real-world SMS corpus exists in the repository.
|
||
|
||
---
|
||
|
||
## Phase 8 — verification and operations
|
||
|
||
8.1 deterministic simulator and 8.2 property tests exist in the domain crate.
|
||
|
||
**Not done:** 8.3 fuzzing, 8.4 SBOM and formal security review, 8.5 operational
|
||
telemetry, 8.6 pilot discipline. These are programme activities rather than
|
||
code changes.
|
||
|
||
---
|
||
|
||
## Priority defects — current state
|
||
|
||
| Priority | Defect | State |
|
||
|---|---|---|
|
||
| P0 | Biometric error dispatches anyway (S2) | Fixed — `AuthorizationAttempt` |
|
||
| P0 | PIN in plaintext prefs (S1) | Fixed — Java scrub; field absent in packaging builds |
|
||
| P0 | SMS marks payment verified (B2) | Fixed — evidence only |
|
||
| P0 | Auto-retry of ambiguous USSD (B3) | Fixed — dispatch budget + spent grants |
|
||
| P0 | `f64` money ledger (B6) | **Partly** — arithmetic and boundary validated; stored representation still `f64` |
|
||
| P0 | Plaintext records, swallowed writes (S3/B16) | Fixed — SQLite, explicit errors |
|
||
| P0 | Duplicate parser/store/flow copies (A5) | Fixed — ADR 0002, CI-enforced |
|
||
| P0 | Classification written, never read (B1) | Fixed |
|
||
| P1 | `thread_local` coordinator (A2) | **Open** — the remaining Phase 6 item |
|
||
| P1 | 5-minute no-SMS → Failed (U4) | Fixed — `PendingConfirmation` |
|
||
| P1 | Hard-coded 2024 fee table (U9) | **Open** — still a static table |
|
||
| P1 | Fee lookup silently free (B5) | Fixed at every call site found |
|
||
| P1 | Bulk USSD flow (U7) | Accounting fixed; controls not built |
|
||
| P1 | Disk I/O in `draw_walk` (P1) | Fixed and verified |
|
||
| P1 | Broken month navigation (B7) | **Fixed in this audit** — was live; see below |
|
||
| P1 | Lossy PSV escaping (B4) | Fixed |
|
||
| P2 | Custom JSON parser (B10) | **Unverified** |
|
||
| P2 | Header forgery, cert pinning (S7/S8) | **Open** — exchange APIs untouched |
|
||
| P3 | Matrix session encryption (S11) | **Open** |
|
||
|
||
---
|
||
|
||
## B7, found live during this audit
|
||
|
||
The tranche notes had never examined month navigation. It was still broken in
|
||
**both** copies of the transactions widget:
|
||
|
||
```rust
|
||
MpesaFilterMode::Month => current + Duration::days(31 * delta),
|
||
MpesaFilterMode::Year => current + Duration::days(366 * delta),
|
||
```
|
||
|
||
Reproduced before changing anything:
|
||
|
||
```
|
||
2026-03-31 -1 month => 2026-02-28 (correct by luck)
|
||
2025-12-28 -1 month => 2025-11-27 (drifted a day)
|
||
2026-03-30 -1 month => 2026-02-27 (drifts; repeated steps skip a month)
|
||
2027-06-15 +1 year => 2028-06-15 (366d is wrong on a non-leap year)
|
||
```
|
||
|
||
Now uses `checked_add_months`/`checked_sub_months`, which clamp to the end of
|
||
the target month, and `checked_add_signed` on the day path so an absurd delta
|
||
reports overflow instead of panicking. An unrepresentable date leaves the view
|
||
where it was rather than jumping somewhere arbitrary.
|
||
|
||
This is a good illustration of why this audit reads code rather than notes:
|
||
the item was neither claimed done nor flagged open, it was simply never
|
||
looked at.
|
||
|
||
## What I would prioritise next
|
||
|
||
1. **`nigig-map` does not compile** (12 errors, `no field center_lat`). This
|
||
blocks a full `pageflipnav` build today and is unrelated to Pay. Nothing
|
||
else matters until an APK can be produced.
|
||
2. **5.2** — the Play-policy decision. It determines whether the USSD rail has
|
||
a future at all, and several open items (6.6 bulk, the coordinator
|
||
migration) are only worth doing if it does.
|
||
3. **The `thread_local` migration**, once a device is available to exercise
|
||
the matrix.
|
||
4. **U9 versioned fee policy** — the 2024 table will go stale silently.
|
||
5. **Quality gates for `nigig-pay-ui`** — 10 `unwrap()` in a payment UI is the
|
||
same defect class the domain crates already ratchet against.
|