# MediCore — Architecture Review
Reviewed folder: `medicore28` (98 PHP/JS/CSS files, ~30k lines excluding the data dump SQL). All findings below were traced through actual source, not inferred — file and line references are given so they can be verified directly.

## 1. The granular permission system doesn't actually restrict anything (highest priority)

Settings has a fully built "🔐 Permissions" modal (`settings.php` ~line 611 onward) where an admin picks per-user, per-module access levels (view / full / custom actions), backed by a real `user_permissions` table and a `can($module, $action)` helper in `includes/auth.php`. It reads as a real access-control feature, and the UI text ("A default permission template is applied automatically") implies it is one.

It isn't. `can()` is called in exactly one place outside its own definition: `includes/header.php`, to decide which sidebar links to show. Every page (`require_once 'includes/header.php'` → `require_login()`) and every one of the 23 `api/*.php` files gate access with only `require_login()` (any logged-in user) or `require_admin()` (admin-only, for deletes). None of them call `can()` or `require_permission()`. So if an admin configures a cashier as "view only" on Products, that only hides the edit button from the sidebar/UI — the cashier can still POST `{"action":"save", ...}` straight to `api/products.php` and it will succeed, because the API never checks what the admin configured.

**Recommendation:** either enforce it or remove it. Enforcing it properly means adding a `require_permission($module, $action)` call at the top of every relevant `api/*.php` action (mirroring how `require_admin()` is already sprinkled through those files), keyed to the same module names already defined in `APP_MODULES`. That's a mechanical, low-risk change file-by-file. If granular permissions aren't actually needed yet, it's safer to pull the settings UI and `user_permissions` table out entirely than to leave a control that looks functional but isn't — that's the kind of gap that causes real incidents once staff numbers grow.

## 2. `require_admin()` silently locks out `super_admin`

`includes/auth.php` line 161:
```php
function require_admin(): void {
    require_login();
    if (($_SESSION['user_role'] ?? '') !== 'admin') {
```
This is a literal string comparison against `'admin'`. `is_admin()` two lines away correctly treats `super_admin` as admin (`in_array(..., ['admin','super_admin'])`), but `require_admin()` does not. `require_admin()` gates 23 API files — delete sale, delete purchase, delete customer/supplier, settings writes, generics/companies management, investments, etc. A `super_admin` account (the role the project added specifically to sit *above* admin, e.g. for permanent-delete in the recycle bin) will get a 403 on all of these.

**Recommendation:** change the check to `if (!is_admin())` so it reuses the already-correct helper, or explicitly allow both roles. One-line fix, but worth auditing after the change since it's been wrong long enough that some workaround may already exist elsewhere.

## 3. Fatal function redeclaration in `api/recycle.php`

`api/recycle.php` does `require_once '../includes/auth.php'` (which defines `is_super_admin()` at line 210), then defines its own top-level `function is_super_admin()` again at line 71 with no `function_exists()` guard. PHP does not allow redeclaring a function — this throws a fatal `Cannot redeclare is_super_admin()` error on every request to this file. Since this is the API that powers the entire Recycle Bin page (list/restore/permanent-delete), that page is likely non-functional as-is, or was written after a copy-paste that duplicated a helper already living in `auth.php`.

**Recommendation:** delete the duplicate definition in `recycle.php` (lines 70-73) and rely on the one in `auth.php`. Also worth a quick grep across the codebase for any other function defined in a page-level file that duplicates something in `includes/` — this is the kind of bug that happens when helper functions get copy-pasted into a new API file instead of imported.

## 4. Reference-number generation contradicts the project's own "non-negotiable" pattern

The project standard (and what `api/sales.php` and `api/purchases.php` actually do) is atomic INSERT → AUTO_INCREMENT → derive → UPDATE, specifically to avoid race conditions in numbering. But `includes/db.php`'s `next_ref()` helper still uses the old `SELECT COUNT(*) ... LIKE 'PREFIX-YEAR-%'` approach, and it's used for real inserts (not just UI previews) in three places:

- `api/requisitions.php:45` — requisition ref
- `api/purchase-returns.php:162` — PRN (purchase return number)
- `api/sale-returns.php:149` — SRN (sale return number)

`purchase_returns.prn` and `sales_returns.srn` both have `NOT NULL UNIQUE` constraints. Two concurrent returns hitting the same counter value will produce a duplicate-key exception, which bubbles up through the generic `catch (Throwable $e) { fail($e->getMessage()); }` handler as a raw SQL error shown to the user, rather than the intended atomic-numbering safety. Requisitions has a partial band-aid (checks if the ref already exists and appends `-His` time suffix), but returns have no such fallback.

**Recommendation:** rewrite `next_ref()` callers to use the same atomic INSERT→ID→derive→UPDATE pattern as `sales.php`/`purchases.php`, or at minimum wrap the return-insert in a retry loop that regenerates the ref on a unique-constraint violation. Low traffic makes this rare in practice, but it's exactly the kind of bug that shows up the first time two cashiers process returns at the same register bank at the same minute.

## 5. `medicore_schema.sql` is no longer the real schema

The schema file only defines 18 base tables and is missing everything added by the session's auto-migrations: `sales_hold`, `purchase_hold`, `sales_returns` (+ items), `purchase_returns` (+ items), `user_permissions`, `audit_logs`, all seven tables' soft-delete columns, `products.low_stock_alert` / `max_retail_disc`, `sales.round_adjustment` / `payable_amount`, the `super_admin` role, and the `customers.phone` unique index. It also still seeds `rounding_enabled`/`rounding_mode` as absent (those are only in `get_settings()` defaults, added after the schema file was last touched).

This isn't a correctness bug in the running app — the auto-migration pattern (defensive `information_schema` checks before `ALTER`) means these all get created lazily on first use. But it does mean there is no single source of truth for "what does the database look like." A fresh install from `medicore_schema.sql` alone would be missing entire tables until the specific page/action that triggers each migration is hit — and some of those triggers are non-obvious (e.g. `audit_logs` only appears once `log_action()` is first called; `user_permissions` only once someone opens the Permissions modal). If a database ever needs to be rebuilt from scratch, backed up structurally, or handed to another developer, this file will produce a subtly broken system.

**Recommendation:** regenerate `medicore_schema.sql` periodically from `mysqldump --no-data` against a fully-migrated instance, or better, keep the auto-migration blocks but also maintain the canonical CREATE TABLE statements in one place (e.g. `includes/db.php`) run once at bootstrap, rather than scattering `CREATE TABLE IF NOT EXISTS` across `api/holds.php`, `api/purchase-returns.php`, `api/sale-returns.php`, `api/settings.php`, and `includes/audit.php`. Auto-migration-on-demand is a reasonable choice for a single-tenant XAMPP deployment, but the schema should still be reconstructable from one file for backup/disaster-recovery purposes.

## 6. Dead and duplicate files left in the tree

Three files are orphaned — not linked from any nav, `href`, or `require`:

- `includes/db_.php` — a near-duplicate of `db.php`, but it points at a **different database** (`medicore_test` instead of `medicore`) and its `get_settings()` defaults are missing `rounding_mode`. Nothing currently includes it, but it's a landmine: if anyone ever typos `require_once '../includes/db_.php'` (easy to do, one character off), the app silently starts reading/writing the wrong database with no error.
- `dashboard1.php` and `smart-requisition1.php` — unlinked duplicates of `dashboard.php` and `product/smart-requisition.php`. Nothing points to them, so they're presumably earlier drafts kept around, but they'll confuse the next person who has to figure out which one is live (especially since `smart-requisition.php` had a real bug fix applied per the project history — it's unclear if `smart-requisition1.php` got the same fix).

**Recommendation:** delete `db_.php` outright (or if it needs to stay for a reason not visible in the code, rename it something unambiguous like `db.testenv.php.bak` and keep it outside the web root). Delete or clearly archive `dashboard1.php` and `smart-requisition1.php` — if they're meant to be a v2 in progress, a `/wip/` or `/_archive/` subfolder keeps them out of the way of anyone grepping the live tree.

## 7. CSRF token plumbing exists but was never wired up

`includes/footer.php` defines a `csrfToken()` JS helper that reads `document.querySelector('meta[name="csrf"]')`. No page ever sets that meta tag (`header.php` doesn't emit it), and `csrfToken()` is never called from `apiPost()` or anywhere else — it's dead code. The app's actual CSRF mitigation is `cookie_samesite: 'Lax'` in `includes/auth.php`, which does block cross-site POST-triggered cookie attachment in modern browsers and covers most of the practical risk for a same-origin XAMPP app, but it's not a complete defense (top-level GET-triggered actions, older browsers, and any future subdomain changes aren't covered).

**Recommendation:** low priority given SameSite=Lax already covers the realistic threat model for a single-tenant LAN pharmacy system, but either finish the token (emit the meta tag in `header.php`, send it as an `X-CSRF` header in `apiPost()`, verify it in each API file) or delete the unused `csrfToken()` function so it doesn't read as protection that isn't there.

## Summary — suggested fix order

1. Decide the fate of the permission system (enforce server-side or remove the UI) — this is the one with real security consequence.
2. Fix `require_admin()` to include `super_admin` — one line.
3. Remove the duplicate `is_super_admin()` in `api/recycle.php` — one line, currently breaking the Recycle Bin.
4. Delete `includes/db_.php` — removes a wrong-database landmine.
5. Fix the three `next_ref()` call sites to use atomic numbering, or add retry-on-duplicate.
6. Archive/delete `dashboard1.php`, `smart-requisition1.php`.
7. Regenerate `medicore_schema.sql` from a live instance; consider centralizing table creation.
8. Wire up or remove the CSRF token stub.

Everything else reviewed — the invoice/PO atomic-numbering pattern in `sales.php`/`purchases.php`, the soft-delete pattern, the offline-first IndexedDB/service-worker layer, and the settings auto-migration pattern in general — is implemented consistently with the architecture described in the project summary and didn't turn up issues.
