# Remaining Fixes — Turista Project

> Living list of outstanding items and recent security/bug cleanup.
> **Current test status:** 827 passed, 2621 assertions, 0 failed (Pest). PHPStan level 5 clean with `phpstan-baseline.neon`.
> **Last security pass:** 2026-07-06 — all high and medium findings from the 2026-06-22 audit and subsequent review were implemented or explicitly mitigated.
> **Last full audit:** 2026-07-23 — manual whole-project audit (controllers, models, services, security, config/ops, API resources, tests). Findings listed under Open Items.

---

## Intentionally Retained

| Item | Why it is still here |
|------|----------------------|
| Global `Model::unguard()` | Kept per project instruction (`AGENTS.md`). Removing it requires adding `$fillable`/`$guarded` to **every** model first. |

---

## Open Items

### Critical

| # | Location | Issue |
|---|----------|-------|
| 1 | `app/Services/AuthService.php:75` | Operator-precedence bug: `$credentials['whatsapp_number'] ?? null ? 'whatsapp_number' : 'email'` parses as `?? 'email'`. WhatsApp login produces `User::where('9665…', null)` → 500. WhatsApp login is completely broken. Needs parentheses. |

### High

| # | Location | Issue |
|---|----------|-------|
| 2 | `app/Services/PaymentService.php:59,160` | Wallet balance updates (`$customer->wallet +=/-=`) saved **without** `lockForUpdate()` (correct locked pattern exists in `ReservationService.php:212`). Concurrent payments/refunds can lose wallet updates while both `WalletTransaction` rows are written → wallet vs ledger divergence. |
| 3 | `app/Services/PromoCodeService.php:76` | `whereIn('owner_id', [$ownerId, null])`: `IN (…, NULL)` never matches NULL, so global promo codes (`owner_id` null) can never be validated. Global codes silently unusable. |
| 4 | `app/Services/Filter/FilterService.php:410` | Revenue search does `orWhereHas('invoice.reservation.customer', …->where('name', …))` on a `morphTo`, but `Customer` has no `name` column (name lives on `users`) → SQL error whenever the revenue search filter is used. |
| 5 | `app/Services/PromoCodeService.php:75` + `app/Models/PromoCode.php:57` | Codes generated uppercase-only, but lookup hashes raw user input (`hash('sha256', $code)`) with no `strtoupper`/normalization anywhere. Lowercase entry = "invalid promo code". Normalize at input or lookup. |

### Medium

| # | Location | Issue |
|---|----------|-------|
| 6 | `app/Services/PaymentService.php:92-94,55-62` | With `confirmed=true`, a refund larger than total paid minus refunded is allowed and the excess is credited straight into the customer's wallet. Unbounded wallet inflation guarded only by a boolean. |
| 7 | `app/Services/PaymentService.php:93` + `bootstrap/app.php:90` | `RefundRequiresConfirmationException` has no dedicated render handler → generic 500 instead of an actionable 4xx, defeating the designed "retry with `confirmed: true`" flow. |
| 8 | `app/Services/RevenueService.php:354-364` | `outstandingQuery` sums `invoices.remaining_amount > 0` with no status filter. `cancelReservation` sets `invoice->status = 'cancelled'` but leaves `remaining_amount`, so cancelled reservations permanently inflate the "outstanding" metric. |
| 9 | `app/Services/ReservationService.php:616-617 vs 639` | `persistFromPendingReservation` copies hold-time `total_price` but `createDocument()` recomputes pricing at confirm time. Price changes in between → `reservation.total_price` and `invoice.net_price` diverge, breaking paid/partially_paid math. |
| 10 | `app/Models/DiscountTemplate.php:90-101` + `app/Services/PricingService.php:53-58` | `isActiveForRange()` returns true if the template is active on **any single night**, then the discount applies to the **entire subtotal**. A 2-day promo discounts a 14-night stay in full. |
| 11 | `app/Services/PricingService.php:37-47` | Occasion branch of `calculateSubtotal()` uses `Occasion::splitRange()` over the full range, ignoring `$includePast` and the same-day 22:00 cutoff that `dateRange()` applies. Charged subtotal and booked availability rows can diverge. |
| 12 | `app/Http/Controllers/Admin/RoleController.php:66-85,95` | `update()` blocks permission edits on system roles but still allows **renaming** them — renaming `super_admin`/`admin` silently breaks `role:` middleware and `config('permissions.system_roles')`. `destroy()` deletes roles without checking for assigned users. |
| 13 | `app/Http/Controllers/UnitAvailabilityController.php:23-50` vs `CalendarController.php:29-35` | `index()` scopes employees only by owner, not assigned buildings, while the calendar endpoint restricts to assigned buildings. An employee can list availability + reservation IDs for all of the owner's buildings. |
| 14 | `app/Services/OtpService.php:73-77` + `app/Services/WhatsAppService.php:68-86,37-45` | If WhatsApp is disabled or credentials are missing, `send()` silently succeeds: OTP cached but never delivered, caller logs "OTP sent successfully". Users registering with WhatsApp can never verify, with no error surfaced. |
| 15 | `app/Services/PaymentService.php:137-156` | Fully-discounted reservations (`net_price = 0`) can never become `paid` — `paid_at`/`payment_status` only stamped on payment transactions. They show as outstanding debt in dashboards/reports forever. |
| 16 | `app/Console/Commands/ReleaseExpiredPendingReservations.php:20-29` | Entire expired-reservation sweep runs in one `DB::transaction()` with `lockForUpdate()` over a cursor; row locks held for the whole batch can block checkout traffic. Use per-row transactions. |
| 17 | `app/Http/Resources/UnitResource.php:30`, `BuildingResource.php:29` | `photos` calls `$this->getMedia('documents')` unconditionally. Reservation list/show loads `unit` but not `unit.media` → N+1 media query per reservation. |
| 18 | `app/Http/Resources/CustomerResource.php:16`, `OwnerResource.php:19` | `$this->user?->…` accessed outside `whenLoaded` → lazy-loads one query per row when `user` isn't eager loaded (e.g. `UnitAvailabilityController.php:34`). |
| 19 | `config/cors.php:32` | `supports_credentials` defaults to `true` in code but `.env.example` ships `false`, contradicting the 2026-06-22 audit doc. Any deployment missing the env var gets credentialed CORS. Safe default should be `false` in code. |

### Low

| # | Location | Issue |
|---|----------|-------|
| 20 | `app/Http/Controllers/Admin/UserController.php:75,97` + `StoreUserRequest.php:33` / `UpdateUserRequest.php:35` | A plain `admin` (not super-admin) can create/promote users to the `admin` role. Privilege-escalation path; needs an explicit product decision. |
| 21 | `app/Services/AuthService.php:31-34` | `findUserByContact` mixes `where`/`orWhere`: with both whatsapp + email supplied it returns the first row matching *either*, which in `verifyAccount` can select a different user than the OTP intended. |
| 22 | `app/Services/AuthService.php:171` | Email-only customer registration auto-sets `is_verified = true` with no email verification — inconsistency vs. the WhatsApp OTP path; confirm intent. |
| 23 | `app/Jobs/DispatchScheduledNotification.php:58-64,79` | Not idempotent: `notify()` runs before `markSent()`, so a crash/retry between them sends duplicates. Also `failed()` calls `->refresh()` which throws `ModelNotFoundException` if the row was deleted, breaking the recovery path — use `find($id)` + null-check. |
| 24 | `app/Notifications/ReservationReminder.php:14` | Missing `SerializesModels` (unlike sibling notifications); reminder can go out with stale reservation data. |
| 25 | `app/Services/SequenceService.php:28-31` | `lockForUpdate()->firstOrCreate()` can't lock a row that doesn't exist yet; two concurrent first uses race → one hits the unique index with an unhandled `QueryException`. Only mitigated by migration pre-seeding. |
| 26 | `app/Services/UnitAvailabilityService.php:240-244` | `releaseDates` throws "Reservation already started and can't be cancelled" whenever earliest booked date is past — also hit by `updateReservation` unit changes, so an overdue pending reservation can neither be cancelled nor moved, and the message is wrong for the edit case. |
| 27 | `app/Models/Customer.php:56-70` | `recordWalletTransaction()` never mutates the wallet; it snapshots `$this->wallet` into `balance_after`. Correctness depends on every caller saving the wallet first (PaymentService does so unlocked — see #2). Fragile API, no guard. |
| 28 | `app/Models/PendingReservation.php` | Has `occasion_id` column/FK but no `occasion()` relation (Reservation has one); `promoCode()` lacks `withTrashed()` while `Reservation::promoCode()` has it — soft-deleted promo silently becomes null on pending reservations. |
| 29 | `app/Models/DiscountTemplate.php:56-62` | `saving` hook derives dates from `duration_days` only when both dates are null; editing `duration_days` later leaves stale contradictory dates governing pricing. `duration_days = 0` would invert start/end. |
| 30 | `app/Models/ScheduledNotification.php:53-61` | `scopeDue()` filters on unindexed `send_at`; indexes are on `(sent_at, cancelled_at)` and `(reservation_id, cancelled_at)`. Due-scan becomes a partial/full scan as the table grows. |
| 31 | Slug/code unique constraints + SoftDeletes (`units.slug`, `buildings.slug`, `occasions.slug`, `promo_codes.code_hash`) | Soft-deleted rows still occupy unique keys, so reusing a deleted slug/code fails with a DB error instead of a validation message. (Promo generation handles it via `withTrashed()`; slugs don't.) |
| 32 | `app/Models/PromoCodeRedemption.php:22` | Only financial-adjacent write model without the `Auditable` trait; redemptions leave no audit trail. Also has redundant `$guarded = ['id']`. |
| 33 | `app/Models/Receipt.php` / `app/Models/Invoice.php` | `receipts.invoice_id` is unique (migration `2026_07_15_000003`) but `Invoice::receipts()` is still `hasMany` — relation type contradicts the 1:1 constraint and misleads callers into a DB error. |
| 34 | `app/Models/Owner.php:78` | `setVerificationStatus()` relies on shared-PK Owner↔User link (`belongsTo(User, 'id')`); an orphaned row silently breaks verification sync. Works, but fragile. |
| 35 | `app/Models/UnitAvailability.php` | `pending_reservation_id` column exists with FK but no `pendingReservation()` relation; missing casts on `unit_id`/`reservation_id`/`pending_reservation_id`. |
| 36 | `employees.building_id` (migration `2026_06_08_102709`) | Dead column — no `building()` relation; all building assignment goes through `building_employee` pivot. Leftover from pre-pivot design. |
| 37 | `app/Http/Controllers/ReceiptController.php:50-87` | `store()` is nearly dead: every reservation already gets a receipt via `createDocument()`, and the unique `receipts.invoice_id` means the endpoint can only ever hit the 422 path for normal data. Remove or document the legacy-invoice use case. |
| 38 | `app/Http/Controllers/UnitController.php:39-41,54,60` | Comment claims building-scoped index is "public marketplace browsing" but code `abort(403)`s non-owners; `$isOwnerRoute = … || $building->owner_id === $ownerId` is dead logic and `isPublic` is always `false`. Comment/code contradict — one is wrong. |
| 39 | `app/Services/ReservationService.php:342-361` | `updateReservation()` date-change path computes old/new sets via `dateRange()` (past/cutoff-filtered); past booked nights of a running stay are never released or repriced on extension → stale `booked` availability rows. |
| 40 | `app/Http/Controllers/Api/Admin/AdminController.php:17-22` | No `DB::transaction` between `User::create()` and `assignRole()`; failure leaves a verified, roleless admin user. |
| 41 | `app/Http/Controllers/Admin/RoleController.php:36` | `$request->boolean('include') === true || $request->input('include') === 'users'` — any truthy `?include=` string (e.g. `permissions`) also loads users. Sloppy but harmless. |
| 42 | `app/Services/PaymentService.php:70` + `app/Services/ReservationService.php:656` | `Notification::send()` inside `DB::transaction`. Safe only because the `database` queue has `after_commit => true`; with `QUEUE_CONNECTION=sync` a WhatsApp outage rolls back a completed payment/reservation. Consider `afterCommit()`. |
| 43 | `app/Helpers/SearchHelper.php:9` | `likeEscape` escapes with `\` but no query specifies `ESCAPE`; correct on MySQL, ineffective on SQLite (tests) — wildcard-injection protection silently differs by driver. |
| 44 | `app/Traits/HandlesMediaPhotos.php:56-62` | `replacePhotos` clears the collection *before* storing; if `storePhotos` then throws (`FileIsTooBig`) the model is left with zero photos. Also `BuildingController:98`/`UnitController:101` call `storePhotos` (not `OrFail`) letting `FileIsTooBig` become a 500. |
| 45 | `app/Traits/GeneratesUniqueSlug.php:21` | Check-then-act slug loop races under concurrency; loser hits the unique constraint instead of getting a suffixed slug. |
| 46 | `app/Services/DashboardMetrics.php:162-177` | Occupancy denominator only counts dates where `UnitAvailability` rows exist (created on demand) → untouched units/months invisible, occupancy systematically overstated. |
| 47 | `app/Services/PaymentService.php:80-88` / `ReservationService.php:169-179` | `lockForUpdate()` on aggregate `SUM()` queries is a no-op in MySQL; still correct because the invoice row is locked first, but the locking intent/comments are misleading. |
| 48 | `app/Http/Resources/PendingReservationResource.php` | Dead code — never referenced by any controller. Also the only resource exposing `photo_download_url`, so if the on-arrival document flow expects it, that feature is unwired. |
| 49 | `app/Mail/OtpMail.php:31-34` vs `resources/views/emails/otp.blade.php` | Mailable passes `context` but template never uses it; template is a single unstyled plain-text line with an English subject while notifications are Arabic — inconsistent OTP email UX. |
| 50 | API resources | Inconsistent `created_at`/`updated_at` serialization: some resources use `?->toIso8601String()`, most return raw Carbon → two response shapes for API consumers. |
| 51 | `BuildingResource.php:29` / `UnitResource.php:30` | Public photos read from the media collection named `'documents'` — the same collection name used for private reservation ID documents. Not a leak today (controller restricts by model type), but fragile naming. |
| 52 | `.env.example` vs `config/locations.php:5` | `SEED_COUNTRIES` read from env but absent from `.env.example`; silently falls back to `Libya`. |
| 53 | `.env.example:23` | `LOG_LEVEL=error` as shipped default hides warnings/deprecations in fresh installs (Laravel default is `debug`). |
| 54 | PHPStan baseline (`phpstan-baseline.neon`, 61 entries) | Mostly benign test-typing noise, but hides real minor app issues: broken PHPDoc `app/Http/Requests/SearchUnitsRequest.php:15`; wrong `@return` class `ListingFilterRequest.php:23` (`App\Http\Requests\ValidationRule` doesn't exist); int passed to `applyYearScope(string)` `UnitController.php:168`; non-exhaustive `match` on `payment_status` `FilterService.php:595` (currently guarded by `ListReceiptsRequest` validation — add a `default` arm). Recommend a baseline ratchet plan. |
| 55 | `app/Services/OtpService.php:70` | OTPs stored plaintext in cache. Acceptable given short TTL + throttling; hashing the cached value would be cheap hardening if the cache backend is shared. |

### Low / Deferred (pre-existing)

| Item | Why it is still open |
|------|----------------------|
| Mail driver defaults to `log` | You asked to skip mail configuration changes for now. Production deployments must set `MAIL_MAILER` to a real provider. |
| Password-reset email flow | Open since the 2026-06-22 audit; depends on the mail driver item above. |
| Employee verification flow hardening | Open since the 2026-06-22 audit. |
| Test coverage gaps | No test for `FilterService::applyToCustomerReceiptQuery` with invalid `payment_status` reaching the `match`; web-layer controllers covered only indirectly; no coverage tooling in CI. |

---

## Recently Completed

- **Reservation / on-arrival ID documents** moved to the private `local` disk and are now served only through the authorized `GET /api/v1/media/{media}/document` endpoint.
- **All public marketplace photos** (buildings, units, owner logos) remain on the public disk and are compressed on upload.
- **Upload size limits** aligned: `config/media-library.php` sets `max_file_size` to 20 MB, matching `PhotoFileRules`. Controllers now catch `FileIsTooBig` and return a validation error instead of a 500.
- **Account enumeration** removed from `verify-account` and `resend-otp` endpoints; missing users receive the same generic response as invalid OTPs.
- **Employee WhatsApp login enumeration** removed; unverified employees now receive the same generic `401 Invalid credentials.` as any other failed login, while the verify-account → force-change-password onboarding flow remains intact.
- **Password policy** strengthened to `min(12)->mixedCase()->numbers()->symbols()` via `Password::defaults()` in `AppServiceProvider`; factory and tests updated.
- **`verifyAccount`** no longer issues a fresh token for already-verified accounts.
- **`admin:create-super`** no longer accepts `--password`; it always prompts securely with `$this->secret('Password')`.
- **Scheduled commands** now use `->onOneServer()` for cross-server deduplication.
- **Dead `AppDatabaseChannel`** removed.
- **`locations:download`** now uses a pinned release URL, validates the JSON schema, and writes the output with `0750` directory / `0640` file permissions.
- All WhatsApp notifications (`PaymentProcessed`, `ReservationCreated`, `ReservationReminder`) now implement `ShouldQueue`.

---

## Verified Clean in the 2026-07-23 Audit

- **Mass-assignment convention:** zero violations — every Eloquent write uses `$request->validated()` / `$request->safe()->only()`; `NoRawRequestInputInEloquentWriteRule` passes with its own test.
- **Double-booking protection:** `UnitAvailabilityService::reserveDates()` locks rows, re-verifies counts, runs in transactions; promo usage limits re-checked under row locks.
- **Routing:** all routes resolve to existing controllers/methods; every state-changing action enforces `$this->authorize()`.
- **SQL injection:** all raw SQL is parameterized or fixed `match()` fragments.
- **June 2026 security audit findings:** verified fixed in code (IDOR guards, empty-scope `1 = 0`, security headers, Sanctum expiry, OTP response gating, generic login errors, password audit exclusion, private ID documents, super-admin seeder removal).
- **Tests:** 827/827 pass; critical flows (reservations, payments/refunds/wallet, invoices/receipts, promo codes, availability, authorization) all have dedicated passing coverage.

---

## Verification Commands

```bash
php artisan test
vendor/bin/pint --test
vendor/bin/phpstan analyse --no-progress --memory-limit=1G
npm run build
php artisan route:cache
php artisan optimize
composer audit
```

> **Notes:**
> - `php artisan optimize:clear` may report a missing MySQL `cache` table in local environments; this is an environment quirk and does not affect the test suite, which uses SQLite in-memory.
> - A queue worker must be running in any environment where WhatsApp notifications are enabled, because `PaymentProcessed`, `ReservationCreated`, and `ReservationReminder` are queued.
> - `->onOneServer()` requires the scheduler to use a shared cache store (e.g., Redis or database) in multi-server deployments.
> - PHP isn't on the system PATH in this environment; Pest was run via Herd's `php84/php.exe`. CI/devs on bare shells may need PATH setup.