# CMS audit — running progress Working notes for an audit that spans more than one sitting. The finished account lives in [CMS-FINAL-AUDIT.md](CMS-FINAL-AUDIT.md); this file exists so the work can be picked up mid-flight without re-deriving what is already known. Started 2026-09-03. Branch `main`, working tree already carrying uncommitted document-verification and branding work (73 paths) — none of it reverted. ## Method Evidence over documentation. Every finding below was reproduced against the code or the database before it was written down, and every fix has a test that failed before it and passes after. ## Phase status | Phase | Area | Status | |---|---|---| | 0 | Inventory | DONE | | 1 | Architecture | DONE | | 2 | Database | IN PROGRESS | | 3 | Clinic isolation | IN PROGRESS | | 4 | Authn / authz | TODO | | 9 | Laboratory | TODO | | 10 | Pharmacy | TODO | | 11 | Billing / finance | DONE | | 13 | Documents / PDF | TODO | | 15 | Audit trail | TODO | | 16 | Security | TODO | | 17 | File storage | TODO | | 19 | Performance | TODO | | 24 | Concurrency | IN PROGRESS | ## Fixed ### CRITICAL — settlement decided from a stale invoice `PaymentService::take()`, `::refund()`, `::reverse()` and `CreditNoteService::write()` each read the invoice's balance, paid total or credited total from the model the request had loaded, decided against that snapshot, and only then wrote. Two requests holding the same snapshot both decided yes. Reproduced deterministically in `ConcurrentSettlementTest`: - 100.00 invoice, two cashiers, 100.00 each → **both accepted**, paid 200.00, balance −100.00. - 50.00 invoice, three 30.00 payments → **all three accepted**, 90.00 taken. - Full refund requested twice → second refund row **committed**, then the settlement write threw on the unsigned `paid_total` column, leaving the payments ledger and the invoice permanently disagreeing about money that had left the drawer. Root cause: no row lock, and the guard outside the write's transaction. The rest of the system already locks correctly — `FefoAllocator`, `lockDoctorDay`, `SequenceGenerator`, `CashSessionService` — so this was a gap, not a pattern. Fix: `InvoiceServiceInterface::settleUnderLock()`. It opens a transaction, takes `lockForUpdate()` on the invoice by primary key, hands the *locked* row to the mutation, refreshes the settlement figures inside the same transaction, and brings the caller's copy up to date. Every guard moved inside it. Also found while fixing: `setRawAttributes($attrs, sync: true)` on a model whose money casts had already been read syncs the original *before* clearing the cast cache, writing the stale amounts straight back over the new ones. Hence `refresh()` in `settleUnderLock`, with the reason recorded at the call site. Tests: `app/Modules/Billing/Tests/Feature/ConcurrentSettlementTest.php` (6). Verified: 49 billing tests pass (Concurrent + PaymentAndTill + Invoicing). ## Confirmed sound (no change needed) - `Money` — integer minor units throughout, exact-rational `multiplyRatio`, largest-remainder `allocate` correct in both directions. No float arithmetic reaches a financial column. - `InvoiceCalculator` — discount before tax, invoice discount allocated across eligible lines so the parts sum back exactly; non-discountable lines excluded. - Schema — all money is `BIGINT` minor units + a currency column; foreign keys are `RESTRICT`; `payments` carries a per-branch unique idempotency key. - Pharmacy `FefoAllocator` — locks in expiry order and refuses to run outside a transaction, which is both the FEFO rule and deadlock avoidance. - Appointments — `lockDoctorDay()` serialises concurrent bookings. - Deployment config — `APP_DEBUG=false`, `APP_ENV=production`, session encrypted and secure-only, no pending migrations. ## Next Authorization negative tests, laboratory workflow, file-download authorization, audit-trail completeness, then the second independent pass. --- ### HIGH — suspending an account revoked nothing `AuthServiceProvider` put the account-status check in a `Gate::after` callback. Laravel merges an after callback's answer with `$result ??= $afterResult`: it can fill in a decision nobody made, and can never overturn one. So every ability a policy granted — and every ability the Super Admin bypass granted — sailed straight past it. Reproduced in `InactiveAccountGateTest`: a suspended clinician kept an ability their role granted; a suspended **Super Admin** kept everything; a suspended administrator kept `settings.update`. Sign-in does refuse an inactive account, so the blast radius is a session open at the moment of suspension — and, indefinitely, **an API token issued before it**, since `auth:sanctum` authenticates by bearer token and never revisits the sign-in rules. Suspending a compromised account did not lock it out. Fix, in two places because two engines answer authorization here: - `Gate::before` now asks the status first and returns `false`, ahead of the Super Admin bypass — a bypass that outranks suspension cannot be switched off. - `User::hasPermissionTo()` refuses too. The permission package registers its *own* `Gate::before` when the gate is first resolved, so it always runs ahead of ours and answers bare permission names itself; it calls this method. Tests: `tests/Feature/InactiveAccountGateTest.php` (4). 81 Auth tests still pass. ### HIGH — the whole PHI directory had an HTTP route `config/filesystems.php` left the `local` disk on Laravel's default `serve => true`, and pointed its root at `storage/app/private` — the same directory as the `private` disk. That registers `GET /storage/{path}` and `PUT /storage/{path}` across all of it: patient documents, portal uploads, the stored report PDFs whose digest makes a forgery detectable, and `keys/document-signing.key` — the ECDSA private key those PDFs are signed with. Signature-gated, so not reachable by a stranger, and not remotely exploitable without `APP_KEY`. But it answered with no policy and no record-access entry — the exact guarantee written on the `private` disk two lines below — and the PUT half would let a filed report, or the signing key, be overwritten. Nothing in the application names the `local` disk. The routes bought nothing. Fix: `serve => false`, plus `visibility => 'private'`. Tests: `tests/Feature/PrivateStorageIsNotServedTest.php` (2). Verified live: `GET /storage/keys/document-signing.key` → 404. ### HIGH — a retracted lab result could never be replaced `retract()` sends the analyte back to Pending because "it still needs reporting". `enter()` then refused every attempt to report it: `currentResult()` selects on `superseded_by_id IS NULL`, nothing supersedes a withdrawal, so the withdrawn result stayed current and verified — *"This result has already been released. Amend it instead."* The worklist asked for a result it would not accept. The only way out was to amend a result that had been withdrawn, which says the wrong thing about the record. The existing test (`it sends a retracted result back to be reported again`) asserted the analyte was Pending and stopped there — the appearance of the rule, not the rule. Fix: a withdrawn result no longer blocks a replacement, and the replacement supersedes it, at `version + 1`. That reuses the versioning already in place: the withdrawn row drops into the history the screens already render, exactly one row stays current, and `orderByDesc('version')` is no longer a tie between two rows that both said version 1 — which is how a withdrawn value could be shown as the current one. Tests: `app/Modules/Laboratory/Tests/Feature/RetractedResultTest.php` (5). Verified: 323 Laboratory tests pass. ### Test corrected, not weakened `WorklistScreenTest::it shows the accession number once a sample has been drawn` asserted a column the clinic had asked to be removed from the worklist. Rewritten to assert the rule underneath — the row asks for a sample, then stops asking and names the specimen — and strengthened with a check that the accession number is still on the order screen, so removing it from a list cannot quietly become removing it altogether. ### LOW — a dead method that said the opposite of the truth `ResultStatus::isSuperseded()` reported `Amended` as superseded. An amendment is the row that *replaces* one. Called from nowhere; removed, with the reason left in its place. --- ### An error of my own, and the guard that came out of it The first full-suite run reported **844 failed, 554 passed**, every failure a `Duplicate entry 'MAIN' for key branches_code_unique` pointing at `PrescriptionListScreenTest` — a file that was entirely fine. The cause was a test I had just written. `tests/Pest.php` binds `RefreshDatabase` to `app/Modules` and deliberately **not** to `tests/`, because several suites there never open a connection. Every suite under `tests/` that does touch the database opts in by hand with `uses(RefreshDatabase::class)`. All twelve of the existing ones do. `InactiveAccountGateTest` did not — so the branch, users and patient it created outlived it, and the next module test to create its own primary branch died on the unique key, taking everything after it down too. The convention was sound; I broke it; the failure then blamed somebody else's file, which is the worst property a failure can have. Fixed the test, and then closed the trap: `tests/Architecture/DatabaseRefreshTest` fails, by name, on any suite outside the modules that touches the database without refreshing it. **The first version of that guard was itself wrong.** It looked for the string `RefreshDatabase` anywhere in the file, which the surviving `use` import satisfies — so when I deliberately broke the test to check the guard, the guard reported everything fine. It now requires the `uses()` call, and has been verified in both directions: passing when the convention is honoured, failing and naming the file when it is not. Two lessons worth keeping: - A full-suite baseline should have been taken **before** the first change, not after several. Without it, an hour went into deciding whether 844 failures were mine or pre-existing. - Editing source while a suite runs makes its result unreadable. The second run was started only after every edit was finished. --- ### HIGH — the invoice arithmetic tests had not run since the enum moved `tests/Unit/InvoiceCalculatorTest.php` imports `App\Modules\Billing\Enums\DiscountType`. That class does not exist: the enum lives at `App\Foundation\Billing\DiscountType`, moved there so a form request could consult it without reaching into a module. The import was never updated, so all five tests that use it died on *"Class not found"* — including `it foots exactly across many awkward lines`, `it never discounts more than the bill is worth` and `it discounts before taxing`. They are the tests for the most safety-critical arithmetic in the product, and they have been erroring rather than running. Committed in the initial commit and untouched by this audit, so it predates it. Nothing hid it — a full run always reported it. It survived because a red suite had stopped being read, which is the same reason Pint was failing on five files and PHPStan on nine. Fix: one import. All fourteen tests in the file now pass, which confirms by execution what this audit had only confirmed by reading — the calculator itself was correct throughout. ### And one more of mine The permission filter added to the appointments listing was written as a Blade comment `{{-- … --}}` *inside* an `@php` block, where it is compiled as PHP and is a parse error. It took down `BladeCompilesTest`, eleven `DayListScreenTest` cases and one `AjaxFilteringTest` case. Rewritten as a PHP block comment. Worth noting that the architecture test caught it immediately and named the compiled file, which is exactly what that test is for. --- ## Final verification | Gate | Result | |---|---| | `pest` (whole suite) | **1,405 passed, 0 failed** (4,198 assertions, 23m) | | `pint --test` | passed | | `phpstan analyse` (level 6) | no errors | | `deptrac analyse` | 0 violations | | `audit:verify --from=147` (live) | chain intact across 3,245 entries | | Live smoke | `/lab/orders` 302, `/dashboard` 302, `/verify` 200, `/book-appointment` 200 | | Live PHI route | `GET /storage/keys/document-signing.key` → 404 | Baseline for comparison: on arrival the suite errored on five tests in `InvoiceCalculatorTest`, Pint failed on five committed files, and PHPStan reported nine errors. --- ## Postscript, 2026-09-07 — a total lockout, and the test that agreed with it The live site began answering every signed-in request with `403 · YOUR ACCOUNT IS NOT ACTIVE`. Every account, web and API, including the administrator who would have had to sign in to undo it. ### Cause `app/Http/Middleware/CheckUserIsActive.php` — added after this audit, registered globally on both stacks in `bootstrap/app.php`: ```php if ($user instanceof User && $user->status !== UserStatus::Active) { ``` `users.status` is a plain string column with **no cast**. A request loads the row, so `$user->status` is the string `'active'`, and a string is never identical to an enum instance. The condition was true for everybody. Proven rather than assumed, against the live database: ``` status value : string(6) "active" isActive() : bool(true) middleware comparison (status !== UserStatus::Active) : bool(true) ``` ### Why the existing test passed anyway `tests/Feature/CheckUserIsActiveTest.php` already asserted that an active user gets a 200. It passed throughout. It created the user with `'status' => UserStatus::Active` — the enum instance — and never reloaded the model. With no cast on the column, the enum survived in memory, so the middleware compared an enum with an enum and agreed: ``` factory-assigned, in memory : enum(App\Enums\UserStatus::Active) loaded from the database : string(6) "active" ``` The test was written against the same wrong assumption as the code it guarded, so it could only ever confirm it. This is the sharpest example in the whole audit of a test that is evidence of nothing: it exercised a value production never holds. ### Fixed - Middleware asks `$user->isActive()` — the predicate the model already owns and the gate already uses. One rule, one place. - `CheckUserIsActiveTest` now persists `$status->value` and reads the row back with `fresh()`, so every case holds the object a request holds. Extended from 4 cases to 7, adding the web stack (which is the half the clinic noticed) and the JSON `account_inactive` code. All 7 pass. - `DatabaseRefreshTest` (added earlier in this audit) was flagging three suites that use `DatabaseTransactions`. That rolls back just as well, so the guard was too narrow — it now accepts either, because a guard that cries wolf is a guard people learn to ignore. Renamed to say what it actually checks. ### Not done, and needing a decision `database/migrations/2026_09_04_000001_fix_soft_delete_unique_constraints.php` is **pending** — it drops `users_email_unique` and the patient MRN unique index and replaces them with generated-column indexes so a soft-deleted row stops reserving its identifier. Nothing in the code references the new columns yet, so the application is consistent without it. Left unapplied deliberately: it is a live schema change on a clinical database, it was not asked for, and it needs the `mysql_migrations` credentials rather than `clinic_app`. It should be applied in a window, after a backup, not as a side effect of fixing a lockout. ### Migration applied, 2026-09-07 `2026_09_04_000001_fix_soft_delete_unique_constraints` — applied on the clinic's instruction, on the live database. **Before touching anything.** MySQL DDL commits implicitly and this migration is six statements across two tables, so a failure halfway leaves `users` changed and `patients` not. Everything that could make it fail was checked first: - MySQL 8.4.3 — virtual generated columns in a unique index are supported. - The index filters match exactly the two intended indexes and nothing else: `users_email_unique` on `users`, `patients_branch_id_mrn_unique` on `patients`. `PRIMARY` and the two `ulid` uniques are correctly left alone. - No duplicate emails among live users and no duplicate `(branch_id, mrn)` among live patients, so both new unique indexes could build. - Full `mysqldump` taken first — 79 tables, 5.9 MB, verified complete by its trailer — at `C:\laragon\backups\daily\pre-migration_2026-09-07_220359.sql`. The partial dump from a first attempt that tripped on an `EVENT` privilege was deleted rather than kept, so nothing ambiguous sits beside a known-good file. - Run over `--database=mysql_migrations`, because `clinic_app` holds no DDL. **After.** Applied in 4s. Verified: | | | |---|---| | `users` | `users_email_active_unique` present, old `users_email_unique` gone | | `patients` | `patients_branch_mrn_active_unique (branch_id_active, mrn_active)` present, old compound gone | | Data | 5 users, 9 patients, 11 invoices, 13 lab orders, 3,392 audit rows — unchanged | | Generated columns | live rows carry their value, the 3 soft-deleted patients carry NULL | The behaviour, proved by probe inserts inside a transaction that was rolled back (patient count still 9, user count still 5 afterwards): - Reusing a **soft-deleted** patient's MRN — **accepted**. That is the point of the migration: those three MRNs were reserved for ever. - Duplicating a **live** patient's MRN — **refused, duplicate key**. - Duplicating a **live** user's email — **refused, duplicate key**. The second and third matter more than the first. Loosening a uniqueness rule on a patient identifier is exactly how two people's records end up merged, so the guarantee was re-proved rather than assumed to have survived. 87 tests pass against the rebuilt schema (Patients module + the middleware suite). No migrations pending. Live site responding. --- ## Postscript, 2026-09-08 — both public pages were broken, and my audit said PASS Found while verifying the cPanel migration: a real QR token returned a redirect to the home page, an invented one returned the verification page. That was read first as a migration fault — a missing `APP_KEY` was a live problem at the same time — and it was not. It reproduces identically on this box. The migration was not the cause; it was the reason anyone looked. Two public, unauthenticated pages were broken for every visitor they exist for: | Page | Who reaches it | What happened | |---|---|---| | `/v/{token}`, `/verify/{number}/{code}` | anyone scanning a QR on a printed report | every **genuine** document redirected to the home page | | `/u/{token}` | a patient following an upload link — **5 live links** | every **genuine** link redirected to the home page | ### Cause A guest has no branch context. `EstablishClinicContext` reads the branch from the signed-in user, and there is no user, so `BranchContext` stays null — which `BranchScope` treats as an error, not as "no constraint", and rightly so. `DocumentVerifier` knew this. Its class docblock says *"It reads across branches, because a public request has no session and no branch"*, and its query called `withoutBranchScope()`. But that lifts the scope on the root query only. Eager-loaded relations are separate queries carrying their own scopes, so `with('issuer:id,name')` — a `User`, which is branch-owned — re-applied it and threw `BranchForbiddenException` the moment a row was actually found. The Portal had the same shape: the link row was read `withoutGlobalScopes()`, and then looking the patient up to greet them by first name ran an ordinary scoped query. `BranchForbiddenException` extends `DomainException`, whose web handler is `back()`. With no referrer that is the site root. So the failure was a redirect, not an error page — nothing in `laravel.log`, no 500, no alert. **And it failed backwards.** A token matching nothing never loaded a relation, so it rendered perfectly. Every forgery got a clean, confident "not verified"; every genuine report bounced to the front page. ### Why every test passed `DocumentVerificationPageTest` and `UploadPortalTest` both set a `BranchContext` in `beforeEach` — they have to, because issuing a document or a link needs one — and neither ever cleared it. So both suites exercised a guest-only endpoint with a branch a guest cannot have. 98 tests across these files, and not one of them was in the state the pages are actually used in. This is the same failure as the `CheckUserIsActive` lockout recorded above: a test that sets up the world and then asserts against its own setup rather than against production's. Twice now, in one audit. ### Fixed - `DocumentVerifier` — one `lookup()` method, wrapped in `BranchContext::acrossAllBranches()`, which suspends the scope for the whole read including relations and restores it on the way out even if it throws. That is what the docblock already claimed. - `UploadLinkService::resolve()` — sets the branch **from the link** when the request arrived without one, rather than lifting the scope. A token names one link, which belongs to one branch, so the request runs properly scoped and a link issued by branch A still cannot reach a patient in branch B. `acrossAllBranches()` would have worked here and would have thrown that away. ### Tests Nine new cases, in two `describe` blocks that clear the branch context first, so they run in the only state these endpoints are ever reached in. Verified in both directions — with the fix reverted, three fail in the verification suite and two in the portal suite, and the failures are the genuine-document ones. Gates after: Pint pass, PHPStan level 6 no errors, deptrac 0 violations, 98 tests across the six document and portal suites. ### The correction this forces `CMS-AUDIT-PLAN.md` recorded **Document authenticity — PASS**, on the strength of `ReportForgeryTest`, `DocumentAuthenticityTest` and `DocumentVerificationPageTest`. Those tests are good tests; they were run in conditions the feature never runs in, and I did not check that. The row now reads FIXED, and the Portal has a row of its own. The audit's own rule was not to treat a passing test as evidence of correctness. This is what it costs to break it.