# Refactor & Hardening Roadmap — 2026-06-29

## Frontend security remediation — COMPLETE (28/28 cards)

After the full 28-controller review (see `FRONTEND_REVIEW_BOARD.html`), every card was
remediated card-by-card via a verify-gated workflow flow (fix → `php -l` → authenticated
render smoke). Status is tracked live on the board with a **"تم الإصلاح ✓✓"** state +
per-finding strikethrough + a `fixedNote` per card.

- **Two passes. ~406 findings fixed, 41 accepted-as-by-design (with a reason on each), 0 left
  un-accounted.** Pass 1 cleared the bulk of CRITICAL/HIGH; pass 2 swept every remaining
  un-struck finding and either fixed it (i18n now landed in `ar`+`en`, env-conditional cookie
  `secure`, rate-limits, authored migrations, code cleanups, `strict_types` on the refactored
  components) or marked it **won't-fix** on the board (`◌`) with a short reason.
- **C27 C1+C2 (global guest-gate + booking-calendar guest-ACL) WERE applied** in pass 2 and
  verified: authenticated smoke 18/18 AND the unauthenticated `?real=1` preview still 200.
- **Accepted-by-design (~41, `◌` on board):** shared mobile-API (`api/`) contract changes
  (e.g. OTP length, base-model rules consumed by api resources), behavioral finance semantics
  needing a spec (float→BCMath, refund-zone logic), a few `strict_types` on 1000-line legacy
  controllers (runtime-coercion risk), and genuine by-design items (owner viewing their own
  IBAN, delete-works-after-quota-period, the 2-vs-1 group minimum per BR-G04).
- **Regression introduced & fixed:** the parallel mass-assignment work added `Shop::scenarios()`
  which called `$this->attributes()`; because `Shop` carries `MultiLanguageBehavior`, that
  re-entered `mlGetAttributes()` *during* the behavior's own `attach()` →
  `UnknownMethodException` on every page that touches a Shop (app-wide 500). Fixed by deriving
  the shopOwner scenario from the default scenario's attribute list minus `PRIVILEGED_FIELDS`
  (no `attributes()`/`getAttributes()` call). Final smoke: **17/17 green**.
- **Verification reality:** the render smoke + authenticated probes cover GET/render of the
  shop-portal surfaces; they do **NOT** exercise POST/write/finance flows. The new
  transaction/TOCTOU/IDOR guards on wallet, booking, group-booking and earnings should get
  focused integration/QA before production.



Supersedes the stale `CODE_ANALYSIS_REPORT.md` (2026-04-28) for prioritisation. Generated from
the baseline health audit (109 findings, all Critical/High re-verified against HEAD on branch
`tailwind-poc`). Tracks what's been done this engagement and what remains, with the reason each
open item is gated.

## BookingCalendarController thinning — service/presenter extraction (cards, in order)

Goal: shrink the fattest controller (`frontend/controllers/BookingCalendarController.php`,
1848 lines / 7 actions) by moving presentation → `BookingCalendarPresenter` and
data-access → focused services. Each card is a **behaviour-preserving** move, verified by
`php -l` + the authenticated render smoke. Route-local palette constants (`STATUS_COLOR`,
`MIXED_COLOR`, `SPECIALIST_PALETTE`) stay controller-owned and are **passed into services
as params** (services never own the route-local colours).

| Card | Move | Target | Status |
|---|---|---|---|
| C1 | leaf-mappers `customerName`(dup)/`customerMobile`/`customerAvatarUrl`/`agentNameOf`/`bookingCode` | `BookingCalendarPresenter` (public static) | ✅ done, smoke 17/17 |
| C2 | `agentTitle`/`agentAvatarUrl` (User mappers) | `BookingCalendarPresenter` | ✅ done; `statusChips` kept (route-local colour) |
| C5 | `walkInCustomers/Services/Agents/AgentServices/Timings` | **new `frontend/components/WalkInOptionsService`** | ✅ done |
| C3a | List view-model `buildList`/`soloRow`/`groupRow`/`collectionFor`/`settlementFor` | **new `frontend/components/BookingListService`** (colours via params) | ✅ done; smoke now covers `view=list`+`view=month` |
| C4 | `detailViewModel` (237 lines) | **new `frontend/components/BookingDetailViewModel`** (status hue via param) | ✅ done; verified by authed render of `/booking-calendar/detail?id=` (200, no error sig) + orphan C1 docblock cleaned |
| C3b | Day+Month view-model `buildDay`/`buildMonth`/`emptyDay`/`applyDemoClock`/`newGroupUrl`/`statusChips`/`groupSizesForDay` | **new `frontend/components/BookingCalendarViewModel`** (`STATUS_COLOR`+`SPECIALIST_PALETTE` via params; `month()` needs none) | ✅ done; authed render of walk-in/group/detail/day/month/list all clean |
| C6 | `bookingCodesByIds` | folded into `BookingCalendarViewModel` (private static, beside its sole caller `day()`) — **chose this over a new BookingQuery scope** (scope migration is one-model-per-PR; lower blast radius here) | ✅ done |

Controller: **1848 → 556 lines (−70%)**. 10 now-unused imports removed. All five new
`frontend/components/` units verified end-to-end against real bookings (HTTP 200, no PHP/Yii error
signature): `WalkInOptionsService`, `BookingListService`, `BookingDetailViewModel`,
`BookingCalendarViewModel` (+ `BookingCalendarPresenter` for the leaf-mappers). Smoke = 17/17.

**Calendar-controller thinning is COMPLETE.** What's left in the controller is genuinely
controller work: the 7 actions, `moveBooking()` (drag write path), and `resolveShopId()`.

### Still gated (NOT landed blind here) — financial / mobile-shared
- **C7 — Wallet TOCTOU** and **C8 — transaction-wrapping** (= roadmap T16/T17): these touch money
  and the mobile-shared API write paths and cannot be proven safe without a DB/integration env.
  Should each be a focused, separately-reviewed PR — see the "needs its own reviewed unit" section.

## Architectural units delivered (each establishes a reusable pattern)

| Unit | Pattern established | Verification |
|---|---|---|
| ActiveQuery named scopes (`BookingQuery`/`ShopQuery`) | named scope over inline `where()`, modelled on the live `ChargeQuery` | scope-equivalence harness (byte-identical SQL) + smoke |
| `BookingStatusPresenter` | domain status presenter → `Aurora` markup; dead AR badge helpers removed | delta-enumeration harness + smoke |
| `BookingCompletionService` | first `common/services/` domain service; pure-builder seam; "no inner transaction — caller owns it" | verbatim move + builder assertions + smoke |

## Security / cleanup fixes delivered

| # | Fix | Status |
|---|---|---|
| T5 | SQL injection in `HelperController` (4 actions parameterised) | ✅ done, verified |
| T6 | Hot-path index migration `m260629_120000_add_perf_indexes` | ✅ written — **run `php console/yii migrate` in Docker** |
| T7 | Debug module guarded `&& !YII_ENV_PROD` (never loads in prod) | ✅ done |
| T8 | Firebase key untracked + env-driven path + `.env.dist` placeholder | ✅ code done — **USER must rotate key in GCP + scrub git history** |
| T9 | Invoice PDFs untracked + gitignored | ✅ repo done — **disk copies + git history are an ops cleanup** |
| T10 | Phantom/dead `use` imports removed (3 api controllers) | ✅ done |
| T11 | `mt_rand` → `random_int` OTP (CSPRNG) | ✅ done |
| T12 | `actionTestSms`/`TestSms2` gated to non-prod | ✅ done |
| T15 | `User::rules()` mass-assignment footgun removed (behaviour-preserving) | ✅ done |
| T18 | `eval()` in `ExtendedMessageController` → `unquotePhpString()` parser | ✅ done |
| T20 | API `BookingController::actionIndex` eager-loads relations (N+1) | ✅ done, additive (no contract change) |
| T21 | HTTP security headers | ✅ already shipped via `SecurityHeadersBehavior` (HSTS/CSP intentionally opt-in) |
| T22 | Calendar drag-reschedule contract bug | ✅ already fixed in branch (`moveBooking()` reads `datetime`) |

## Open — USER / deployment only

- **T8 (rotation):** revoke the leaked Firebase service-account key in GCP; scrub it (+ the invoice PDFs) from git history (destructive — `git filter-repo`).
- **T13:** Apache `Options -Indexes` on the 4 vhosts (your revert restored `Indexes`; re-apply if wanted).
- **T14:** commit the untracked `aurora-forms.js` / `aurora-tabs.js` (referenced by `TailwindAsset` — deploy 404s without them).
- **Deployment:** set `YII_ENV=prod` in the production `.env`.

## Open — needs its own reviewed unit (financial / contract / structural — NOT safe to land blind here)

> No local DB (Docker-internal host), no api/mobile runtime, and the Codeception CLI is broken on
> PHP 8.5 — so payout/contract/structural changes cannot be proven safe in this environment. Each
> should be a focused PR with integration testing.

- **T16 — Wallet withdrawal TOCTOU** (`api/WalletController` ~306-359): wrap in a transaction + `SELECT … FOR UPDATE`. Financial + mobile-shared.
- **T17 — Transaction wrapping** of 5 multi-table write paths (`AgentsBookingsController::actionCreateWalkIn`, `BookingController::actionCreate`, `AgentsController::actionUpdate`, `ShopController` category pivot). Additive safety, but exercises write flows that need testing.
- **T19 — Frontend `ApiController` CSRF/CORS** (`accept-policies`): caller is unidentified (no frontend-JS caller found — possibly the mobile webview). Identify the caller first; same-origin → enable CSRF + drop CORS, cross-origin → pin the origin. Changing blind risks locking users out of policy acceptance.
- **T23 — `base/Shop.php` de-tiering**: remove the inverted `api\`/`backend\` imports + add a Gii regen-guard; consider relocating logic to the subclass. 1,134-line hand-mutated base — high blast radius.

## Open — follow-ups

- **T24 (docs):** rewrite `ai_specs/02_ARCHITECTURE/*` + `03_API/API_CONTRACTS.md` (currently describe Laravel/Sanctum/Blade — wrong framework) for Yii2; retire the stale `CODE_ANALYSIS_REPORT.md`.
- Continue the ActiveQuery-scope migration to the remaining ~40 empty query stubs (one model per PR).
- Fold the group-booking status-token logic in `agents-bookings/index.php` into `BookingStatusPresenter`.

## Verification reality (read before trusting "done")

- The index migration is **written, not run** (no host→DB access).
- The Firebase key + invoice PDFs are **untracked, not deleted from disk and not scrubbed from git history**.
- api/backend/console fixes are verified by `php -l` + reasoning; the frontend render smoke (15/15) covers `common/` autoload + the shop-portal pages but **does not exercise POST/completion/payout flows**.