# Security Audit — July 2026

A full-portal security review carried out on 2026-07-20, and the fixes applied.
Five issues were found and all five were remediated. Each fix was verified against
the local instance, and for the live-only behaviours (H1, H2) with controlled
before/after tests that reproduced the original vulnerability and confirmed the fix.

## Summary

| # | Severity | Finding | Status |
|---|---|---|---|
| C1 | Critical | Unauthenticated arbitrary file upload → remote code execution | Fixed |
| H1 | High | Unauthenticated debtors API leaking customer financial data | Fixed |
| H2 | High | `Host`-header authentication bypass + insecure environment default | Fixed |
| M1 | Medium | `.git` directory web-accessible (source disclosure) | Fixed |
| M2 | Medium | phpinfo / composer installer / test scripts reachable | Fixed |

The injection surface was found to be **clean**: SQL is consistently parameterised
(no string-built queries anywhere, including the debtors API), no command injection,
and no reflected XSS in current code. Session cookies already carry `HttpOnly`,
`SameSite=Lax`, and `Secure` (on HTTPS).

---

## C1 — Arbitrary file upload → RCE

**Where:** the legacy POD / PDI / PO upload pages
(`CRUD/forms/pod/pod_upload/uploadpod.php`, `CRUD/forms/pdi/pdi/uploadpdi.php`,
`CRUD/customers/POs/uploadpos.php`) plus seven dead-but-reachable copies.

**Risk:** they ran `move_uploaded_file()` with the raw client filename and **no
extension or content check**, writing into a web-served folder. An attacker could
upload `shell.php` and execute code. Several guards used the short tag
`<? require … session.php ?>`; with `short_open_tag` off the guard was printed as
text and never ran, so those pages were also **unauthenticated**. Proven end-to-end
during the audit (an arbitrary file was uploaded with no session and fetched back).

**Fix:**
- New shared guard `CRUD/functions/upload_guard.php` — `store_uploaded_document()`:
  extension whitelist (`pdf/jpg/jpeg/png` only), magic-byte check (bytes must match
  the claimed type, defeating renamed scripts and polyglots), and `basename()` +
  destination check (no path traversal).
- The three live handlers rewritten to use it; the inert `<?` guards changed to
  `<?php` so authentication actually runs; `uploadpos.php` given an explicit target
  path instead of `""`.
- The seven dead orphan handlers deleted (unreferenced but URL-reachable).
- Defence-in-depth `.htaccess` in `pods/`, `POs/`, `pdi/pdi/` denying execution of
  any script that lands there (with a `<Files>` exemption for the legitimate handler
  pages that live in `POs/` and `pdi/pdi/`).

**Verified:** 11-case test — webshell `.php`, php-bytes-as-`.pdf`/`.png`, double
extension `x.php.pdf`, `../` traversal, `.htaccess`, null byte, and `.svg` all
rejected; legit pdf/jpg/png accepted; nothing escaped to disk. `.htaccess` serves
PDFs (200) but denies scripts (403).

---

## H1 — Unauthenticated debtors API

**Where:** `CRUD/customers/debtors/api/get_debtors.php`.

**Risk:** it included `bootstrap.php` and `database.php` but **not** `session.php`,
so it returned customer account names, outstanding balances, aging and contact
history to anyone who could reach the URL — no login required. Every other file in
the debtors module includes `session.php`; this one was the gap.

**Fix:** added `require_once …/CRUD/session.php;` after `bootstrap.php`, before any
query. `session.php` redirects an unauthenticated caller to the login page before
the endpoint touches the database.

**Verified:** old code leaked live balances unauthenticated; fixed code returns
`302 → /login.php` with zero data; normal authenticated use is unchanged.

---

## H2 — Host-header auth bypass + insecure environment default

**Where:** `CRUD/session.php`, `CRUD/database.php`, `CRUD/bootstrap.php`.

**Risk:** the environment was decided from the client-controllable `Host` header
(`SERVER_NAME`), and an unknown host defaulted to **`local`** — the mode in which
`session.php` skips authentication. A request to the live site with
`Host: anything.local` was treated as local dev and bypassed the login wall.
`SERVER_NAME` follows the `Host` header under Apache's default `UseCanonicalName Off`.

**Fix:**
- New `CRUD/env.php` with `portal_is_local_request()` — "local" now requires the
  connection to have terminated on a **loopback `SERVER_ADDR`** (127.0.0.1 / ::1),
  which Apache sets from the real socket and the client cannot forge.
- `session.php` and `database.php` derive "local" from this helper instead of the
  Host string.
- `bootstrap.php` uses it too, and the default for an unrecognised host was flipped
  from `local` to **`prod`** (the secure default: errors hidden, production DB).

**Verified:** the old `session.php` let a spoofed-host request reach page content
(auth bypassed); the fixed one blocks it, while genuine local dev (loopback) still
skips auth as before. The helper was unit-tested across loopback and public-IP cases.

**Live check after deploy:** confirm `SERVER_ADDR` on the live server is its public
IP, not `127.0.0.1` (a one-line `echo $_SERVER['SERVER_ADDR'];`). The fix assumes the
live box does not terminate requests on loopback (true for standard cPanel).

---

## M1 / M2 — Sensitive files reachable over HTTP

**Where:** `/.git/` served source-control metadata; `CRUD/z-dev-tools/phpinfo.php`,
`composer-setup.php`, and `CRUD/hire_list/test_*.php` were directly fetchable.

**Fix (block, not delete — closes HTTP reachability while keeping local dev tools):**
- Root `.htaccess`: `RedirectMatch 404` for `.git`/`.svn`/`.hg`/`.bzr`; `Require all
  denied` for sensitive extensions (`.sql .log .sh .bat .bak .ini .env .lock` …) and
  for `composer-setup.php` / `composer.*` / `phpinfo.php` / the `test_*.php` scripts.
  Added above the cPanel-managed sections (left untouched).
- `CRUD/z-dev-tools/.htaccess`: `Require all denied` for the whole dev-tooling folder.

**Verified:** `.git` now 404; dev/debug/composer/test files and `.sql` now 403; PDFs,
the upload handler pages, and normal app pages still 200 (no over-blocking).

---

---

## A1 / A2 — Broken access control (authorization) — follow-up pass

A second pass found the portal authenticated every page but authorised almost
none. `users_page_permissions` held 3 rules, all pointing at non-existent paths,
so any logged-in user could reach ~all 870 pages regardless of role.

**A1 — privilege escalation (the critical one):** `users/users_create.php`,
`_update.php`, `_delete.php`, `_index.php` had no manager check, so a logged-in
`driver` could POST `role=manager` and mint a manager account (or promote
themselves via `users_update`). 2 driver accounts exist, so this was live.

**A2 — everything open:** sensitive modules (debtors, prices, customers,
suppliers, reports, hire, HR, medical, credit, …) had no role checks at all.

**Fix — enforced role model (David confirmed driver/fitter are field staff):**
- `CRUD/auth.php`: `require_role(...)` (page-level guard) and
  `portal_field_role_may_access()` (the field-role allowlist).
- `CRUD/session.php`: **central default-deny** — a non-manager role may only
  reach the capture flows and their dependencies; every other page is
  manager-only. This is the single enforcement point, so no page can be left
  unguarded by omission. (The redirect target was also corrected from the
  non-existent `/CRUD/unauthorised.php` to `/unauthorised.php`.)
- `require_role('manager')` added to `users/*`, `customers/debtors/*`,
  `prices/*` as explicit belt-and-braces on top of the central check.

Field-role allowlist (derived by tracing what the capture flows actually call):
`forms/pod`, `forms/pdi`, `forms/timesheets`, `forms/fitters_report`, `fitter`,
`functions/ajax` (site lookups), `main` (landing), plus exact endpoints
`forms/templates_send_eml.php` (email a captured doc) and `forms/forms_dashboard.php`,
and `/logout.php`. Sensitive cross-links that some capture pages render
(edit-hire, edit-plant) are intentionally denied — clicking them lands on the
unauthorised page, which is correct.

**Verified:** `require_role()` and `portal_field_role_may_access()` unit-tested
across the full matrix (all sensitive modules denied for field roles; all
capture flows + traced dependencies allowed; managers and local dev unaffected).
A live field-role login can't be exercised locally (loopback skips the auth
layer), so enforcement was proven via the deterministic logic tests.

See [role-model-proposal.md](role-model-proposal.md) for the tier map and the
open per-role refinements.

---

## A3 — Second class of unrestricted file uploads (RCE) — follow-up pass

Beyond the legacy handlers in C1, a second pattern was found: handlers that store
an upload under a random name but keep the **client's file extension** with no
whitelist, into a web-served folder. `customer_documents_upload.php` and
`supplier_documents_upload.php` were confirmed exploitable — the target dir both
executed `.php` and allowed directory listing, so: upload `evil.php`, list the
folder to find the random name, request it → code runs. (Manager-only after A2,
but still RCE, and these dirs hold customer/supplier documents.)

**Fixes:**
- New `validate_uploaded_document()` in `upload_guard.php` — extension whitelist
  (pdf/jpg/png) + magic-byte check, returning the safe extension for handlers
  that use their own storage naming. `customer_documents_upload.php` and
  `supplier_documents_upload.php` now validate and store the *validated*
  extension, never the client's.
- **Defence-in-depth `.htaccess` sweep:** the deny-script + `Options -Indexes`
  guard was placed in **every upload storage directory** under `/CRUD` (23 in
  total, incl. those from C1). Dirs that also hold their own handler `.php`
  (e.g. `plant_list/LCs`, the `plant_lift_*` dirs) use a `<Files>` exemption so
  the handler still serves while uploaded scripts are denied. This neutralises
  the whole class regardless of per-handler validation.

**Remaining handlers — now done.** The ~26 other handlers were triaged and fixed:
- Safe already (hardcode the extension or whitelist): the batch handlers
  (`pod`/`pdi`/`ts`/`statements`/`internal_invoices`), `upload_supplier_invoice.php`,
  `ts_upload_save.php`, `signature_manage.php`, `pdf_sign/upload_pdf.php`,
  `fleet_documents_upload.php` — left as-is.
- Raw C1-style (inert `<?` guard + raw client filename): `plant_list/LCs/uploadLC.php`
  and the three `plant_specifications/plant_lift_*` handlers — fixed with `<?php`
  guard + `store_uploaded_documents()`. `uploadQLCs.php` (unauthenticated, zero
  references) was **deleted**.
- Client-extension in a constructed name: `hs/document_add.php`,
  `plant_list_works/works_document_upload.php`,
  `fitter/pdi_upload_handler.php`, `upload_supplier_invoice_form.php` — validated
  (`validate_uploaded_document()` or an extension whitelist matching the accepted
  types).
- Sanitised-name + client-extension cluster writing to `/CRUD/uploads/*`
  (mileage, riddor ×4, risk-assessment ×2, iso audit) — an inline extension
  whitelist now short-circuits the move; `/CRUD/uploads/` got a single recursive
  `.htaccess` covering the whole tree.

`store_uploaded_document()`/`validate_uploaded_document()` reject `.php`,
polyglots and traversal; every upload storage dir is execution-blocked; and the
one unauthenticated orphan was removed.

---

## Hardening pass — backups exposure, error leakage, login brute-force

**Old-code backups web-reachable (source disclosure / shadow endpoint).**
`/backups/` listed and served old copies of live pages — including a `need.php`
that still includes `session.php`, connects to the DB and processes `$_POST`, i.e.
a functioning hire-creation endpoint outside every fix applied to the real file.
Blocked via the root `.htaccess` (`RedirectMatch 404 ^/backups`). Now 404.

**Stack traces on live (info disclosure).** 336 files force `display_errors` on;
282 do so *before* including `bootstrap.php` (which corrects it per-env on prod —
safe), but **53 leaked** (set on after bootstrap, or with no bootstrap at all).
Those 53 were changed to an environment-aware form
(`SERVER_ADDR` loopback → on for local dev, else off) so live shows no traces
while local debugging is preserved. `bootstrap.php` (env-conditional, correct)
was left alone.

**No login brute-force protection.** `authenticate.php` had no throttling. Added a
`login_attempts` table (created on demand) and an IP-based limit: 10 failures in
15 minutes blocks further attempts (incl. correct credentials, so guessing can't
win during lockout); a successful login clears the IP's failures. It **degrades
gracefully** — if the attempts store is unavailable, login proceeds without the
throttle rather than locking everyone out. Its own `display_errors` leak was
fixed at the same time, and `login.php` shows a "too many attempts" message.

---

## Post-deploy pass — stored XSS & CSRF

**Stored XSS.** The codebase escapes output consistently (`h()` /
`htmlspecialchars` everywhere, server-side DataTables endpoints escape, the
client JS uses `escapeHtml()`). One page was the exception:
`plant_list/plantlist_index.php` rendered its whole table of free-text plant
fields (`Model`, `Serial_Number`, `fleet_Number`, `Category`, …) raw, so a
`<script>` typed into a plant record would run for anyone viewing the list.
Fixed — added an `h()` helper, wrapped every field, URL-encoded the PDF hrefs,
cast the id params to `(int)`.

**CSRF on GET-based actions.** The session cookie is `SameSite=Lax` (blocks
cross-site POSTs and sub-resource GETs), but 26 handlers performed a destructive
action on a plain GET (deletes, archive/reactivate, toggle-active,
collected/confirmed, complete, run-extract). Residual vector: a top-level
navigation an attacker triggers cross-site (crafted link / redirect). Fixed with
a shared guard — `block_cross_site_request()` in `CRUD/auth.php` — applied to all
26. It refuses requests whose `Sec-Fetch-Site` is `cross-site` (with an
`Origin`/`Referer` host fallback for older browsers), while allowing same-origin
clicks, direct navigation and header-less old browsers (SameSite=Lax backstop).
Local dev is skipped as usual. One of the 26, `messages/phone_call_complete.php`,
was also **unauthenticated** (no `session.php`) — that was added too.

Verified: cross-site → 403; same-origin / typed-URL / no-signal → allowed; all
26 lint clean and still reachable on local dev.

---

## Continuation — client-side XSS & dependency CVE

**Client-side (innerHTML) XSS.** A pass over the JS `innerHTML` template literals
found two that injected fetched contact data unescaped —
`missing_info_update_drivers.php` and `missing_info_update_PO.php` built an email
dropdown with `` `<option value="${c.email}">${c.name}</option>` `` — so a contact
name/email containing HTML would run as script. Fixed by building the options via
DOM APIs (`document.createElement` + `.value`/`.textContent`, which don't parse
HTML), matching the already-safe `loler_send_modal.php`. Other consumers
(`update_pl`, `update_oponly`, `update_history`, the risk-calc and cleanup panels)
were checked and are safe (DOM APIs or computed/server values).

**Dependency CVE (composer audit).** The root vendor tree is clean. The
`CRUD/invoice_review/vendor` tree — which the invoice burn/extract feature uses to
parse supplier-supplied PDFs — had **FPDI v2.6.6, vulnerable to CVE-2026-45802**
(memory-exhaustion / endless-loop DoS from a crafted PDF, fixed in 2.6.7+).
Updated that tree to FPDI v2.6.8 (TCPDF came along 6.11.2 → 6.11.3); `composer
audit` is now clean on both trees and the burn classes still load. Run
`composer audit` periodically to catch future advisories.

## Hardening pass 2 — idle timeout, password policy, mPDF review

Three lower-priority hardening items, done after the main vulnerability classes
were closed. None were open holes; these tighten policy/config.

**Session idle timeout (8h).** Sessions were a flat 24h with no inactivity
expiry, so a machine left logged in stayed logged in (e.g. over a weekend).
Added an app-level idle check in `CRUD/session.php` (authenticated section only):
if `now - $_SESSION['last_activity'] > 8h` the session is cleared, the cookie is
expired, and the user is redirected to `/login.php?err=4` ("signed out after a
period of inactivity"); otherwise `last_activity` is refreshed each request.
8 hours expires abandoned/overnight sessions without interrupting a normal
working day. Crucially the client-side keepalive pinger (`CRUD/keepalive.php`,
fired every 5 min) does **not** include `session.php`, so it keeps the cookie/gc
warm for a live tab but can never refresh `last_activity` — it cannot defeat the
idle timeout. New `err=4` branch added to `login.php`.

**Password policy (min 10, length over complexity).** `users_create.php`
enforced only a 6-character minimum. Raised to 10 characters with no forced
complexity rules (current NCSC/NIST guidance: length beats symbol-soup), plus a
form hint suggesting a three-word passphrase. There is no self-service password
change or reset flow (managers create accounts; `users_update.php` never touches
the password), so this is the only creation path to guard.

**mPDF SSRF/LFI review — no change needed.** The only app consumer of mPDF is
`CRUD/hs/lib/document_pdf.php` (H&S cover-page generator). Every dynamic value
reaching the cover HTML (title, approver, change reasons, dates) is passed
through `htmlspecialchars()`, so document metadata containing
`<img src="http://internal/...">` becomes inert text — it cannot inject a tag.
The only real `<img>`/image sources are server-controlled local paths (the
`invoice_header.png` letterhead and the app-generated watermarked signature).
No untrusted URL ever reaches mPDF, so there is no remote-fetch (SSRF) or
arbitrary-local-read (LFI) surface. The body merge goes through FPDI's PDF
parser, not HTML (and that CVE is already patched above). Locking down mPDF's
local-file access would break the legitimate letterhead/signature reads for no
security gain, so the usage is left as-is.

**Incidental fix.** `users_create.php` and `users_update.php` still forced
`display_errors` on unconditionally (missed by the earlier 53-file sweep because
of the 3-line `ini_set` block). Both switched to the same inline loopback-aware
toggle used elsewhere (errors on for `SERVER_ADDR` 127.0.0.1/::1, off otherwise).

## Unauthenticated endpoint sweep (2026-07-21, during the module review)

The module-by-module review surfaced that the original audit's auth checks — and my
own per-module health checks — only ever exercised **pages**. API endpoints and
handlers were never requested, so unauthenticated ones went unnoticed. A dedicated
sweep was run over every file lacking an auth guard.

**Method.** 53 files had no `session.php`. Triage discarded two categories: pure
libraries (only define functions — inert when requested directly, verified by
requesting several and getting 0 bytes) and `z-dev-tools/` (denied at the webserver).
That left **13 files that are active**, of which **11 are never included anywhere**,
i.e. directly reachable endpoints.

**Two genuine holes, same class as H1:**

- **`CRUD/maps/update_actions/update_single_location.php`** — included `bootstrap.php`
  and `database.php` but **not** `session.php`. Accepted `POST id/lat/lng` and ran
  `UPDATE site_address SET site_lat, site_long WHERE id_site_address = ?`. **Anyone
  could move any site's map coordinates.** SQL was parameterised, so this was an
  authorization failure, not injection.
- **`CRUD/hire_list/send_loler/fetch_loler_emails.php`** — called a bare
  `session_start()`, which starts a session but performs **no auth check**, so it
  looked protected while returning **customer contact names and email addresses**
  for any `hire_id`. Enumerable — a full harvest of the customer contact list.

**Also closed:** `forms/timesheets/timesheet_upload.php` — an **unreferenced,
unauthenticated file-upload endpoint** that trusted the client-supplied extension
(no content check) and wrote into a web-served directory. Now requires login and
validates by magic bytes, with the stored filename rebuilt from the *validated*
extension (dots stripped from the stem, so `shell.php` stores as `shell.png` and no
double extension can survive). It remains unreferenced and is a deletion candidate.

**Guarded for consistency** (lower risk, business-data reads or redirect shims):
`forms/pdi/pdi_next_number.php`, `hs/api_get_accreditation_sections.php`,
`hs/coshh/api_statements.php`, `hs/coshh/migrate_msds_tracking.php`,
`hs/risk_assessment/{gra,ms,ra}_edit.php`.

All 10 files verified: guard present, syntax clean, endpoints still return correct
data, and no regression across the portal.

> **Tooling note:** the inventory's auth detection originally just grepped for the
> string `session.php`, which produced **false negatives** — `invoice_review/internal_invoices/*.php`
> are authenticated via a local `config.php` that includes the guard. The detector in
> `CRUD/z-dev-tools/build_page_inventory.php` now follows includes two levels deep.

## Files changed

**New:** `CRUD/env.php`, `CRUD/functions/upload_guard.php`,
`CRUD/z-dev-tools/.htaccess`, `CRUD/forms/pod/pods/.htaccess`,
`CRUD/customers/POs/.htaccess`, `CRUD/forms/pdi/pdi/.htaccess`.

**Modified:** `.htaccess`, `CRUD/bootstrap.php`, `CRUD/session.php`,
`CRUD/database.php`, `CRUD/customers/debtors/api/get_debtors.php`,
`CRUD/forms/pod/pod_upload/uploadpod.php`, `CRUD/forms/pdi/pdi/uploadpdi.php`,
`CRUD/customers/POs/uploadpos.php`.

**Deleted (dead, URL-reachable upload handlers):**
`CRUD/forms/pod/pods/uploadpdI.php`, `CRUD/forms/pod/pods/uploadphp.php`,
`CRUD/forms/pod/pods/Uploads/{uploadpod,uploadpdi,uploadlc,uploadqlc}.php`,
`CRUD/forms/pdi/pdi/uploadphppdi.php`.