Files
edr-platform/docs/SOLUTIONS.md

142 lines
9.3 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# EDR Passenger — Solutions
Concrete fixes for the confirmed findings in `docs/ISSUES.md`. Ordered by priority. Each references
the exact site and the intended change. Code sketches are illustrative, not drop-in patches.
**Test coverage backing these:** 28 automated tests (25 API `jest` + 3 UI Playwright) reproduce the
✅ findings. Fix a finding → its 🔴 test flips from "bug present" to failing; update the test to
assert the corrected behavior.
---
## P0 — deploy blockers (money creation/theft, broken booking)
### C-10 — Authenticated `POST /bookings` is broken
`bookings.service.ts:773` resolve unconditionally; delete the UUID-format gate:
```ts
// BEFORE: resolves only when passengerId is NOT a UUID (never fires for real IAM ids)
if (dto.passengerId && !dto.passengerId.match(/^[0-9a-f-]{36}$/i)) { }
// AFTER: always translate the authenticated identity → the Passenger.id
if (dto.passengerId) {
const passenger = await this.prisma.passenger.findUnique({
where: { iamUserId: dto.passengerId }, select: { id: true },
});
if (passenger) dto = { ...dto, passengerId: passenger.id };
// else: leave as-is only if it already IS a Passenger.id (guest/admin paths)
}
```
Keep the controller's "don't trust the body" intent (`bookings.controller.ts:528`) — it's correct to
take identity from the JWT; the service just has to map iamUserId → Passenger.id. Regression test:
`test/authed-booking-passengerid.e2e-spec.ts`.
### C-1 — Booking total is client-controlled
`bookings.service.ts:863-899`: stop trusting `reviewedTotalMinor`/`seatFareMinor`. Recompute the fare
server-side and reject a mismatch:
```ts
const server = fareCalculation.totalMinor;
if (dto.reviewedTotalMinor != null && Math.abs(dto.reviewedTotalMinor - server) > 1) {
throw new BadRequestException('Price changed — please review the updated fare');
}
resolvedTotalMinor = server; // never persist a client amount as the charge basis
```
Apply the same to `guest-booking.service.ts:206-245`.
### C-2 — Loyalty redemption unbounded + never deducted
`bookings.service.ts:1705` (and `:1028/:1249/:1451`): validate + debit inside the booking transaction:
```ts
const acct = await tx.loyaltyAccount.findUnique({ where: { passengerId } });
const pts = Math.min(dto.loyaltyRedemptionPoints ?? 0, acct?.pointsBalance ?? 0, MAX_REDEEM);
const loyaltyMinor = pts * POINTS_TO_MINOR;
await tx.loyaltyLedgerEntry.create({ data: { accountId: acct.id, delta: -pts, reason: 'REDEEMED', balanceAfter: acct.pointsBalance - pts } });
await tx.loyaltyAccount.update({ where: { id: acct.id }, data: { pointsBalance: { decrement: pts } } });
```
### C-3 — Wallet top-up: no ownership, no backing
- `wallet.controller.ts:34`: enforce `req.user` owns `:passengerId` (or is admin) before top-up.
- `wallet.service.ts:50`: only credit after a confirmed `PaymentIntent` (a top-up is a purchase).
- `wallet.controller.ts:23-24`: remove `@IsPublic()` from `GET /wallet/accounts`.
### C-4 — Payment amount never validated
- `intents.service.ts:541-548`: on `confirmedAmountMinor !== intent.amountMinor`, do NOT mark
SUCCEEDED — set a `AMOUNT_MISMATCH` state and alert. Populate `confirmedAmountMinor` in each
webhook handler (e.g. `waafi-webhook.service.ts:63`).
- `payments.service.ts:809` `finalizePaymentSuccess`: assert the settled amount equals
`booking.totalMinor` before confirming.
### C-5 — Late webhook resurrects an expired/cancelled booking
`payments.service.ts:809` `finalizePaymentSuccess`: refuse to confirm unless the booking is still
`PENDING_PAYMENT`; route a late success to the refund/again-available flow:
```ts
if (booking.status !== 'PENDING_PAYMENT') { await this.refundLatePayment(intent); return { alreadyFinalized: true }; }
```
### C-6 — Wallet double-spend (no row lock)
`payments.service.ts:461-484`: lock the row or use an atomic conditional update:
```ts
const res = await tx.$executeRaw`UPDATE passenger."WalletAccount"
SET "balanceMinor" = "balanceMinor" - ${total}
WHERE "passengerId" = ${booking.passengerId} AND "balanceMinor" >= ${total}`;
if (res === 0) return { success: false }; // insufficient / lost the race
```
Regression test: `critical-repro.e2e-spec.ts` (C-6, barrier-forced interleave).
### C-7 — Refund computed but never disbursed
`bookings.service.ts:2017-2027`: on `booking.cancelled`, actually disburse — credit the wallet or call
the provider refund — and drive `refundStatus PENDING → PROCESSING → COMPLETED`. Add a reconciliation
sweep for stuck `PENDING` rows.
### C-8 — Exchange-rate writes missing the ADMIN check (any passenger can write FX)
`fare-engine/currency.controller.ts:25,32`: add `@PassengerAdmin()` (+ `@ApiBearerAuth`) to the `PUT`
and `PATCH` handlers, matching the already-guarded `DELETE`. (Not unauthenticated — the global
JwtGuard requires a token; the gap is the missing *authorization*. Verified live: passenger → 200.)
### C-9 — `@Roles('ADMIN')` is dead
Register the guard globally so `@Roles` is enforced:
```ts
// app.module.ts providers
{ provide: APP_GUARD, useClass: RolesGuard }
```
…or convert `configurable-fare` / `segment-fare` / `system-config` controllers to the working
`@PassengerAdmin()`/`@PassengerStaff()` guards.
---
## P1 — pricing correctness (HIGH)
- **H-1 promo → negative total** (`fare-engine.service.ts:192`): `totalEtbMinor = Math.max(0, subtotal - discount)`; DTO `@Min(0) @Max(100)` on `percentOff`, `@Min(0)` on `amountOffMinor` (`promos.dto.ts:20,26`).
- **H-2 missing FX → 1.0** (`currency.service.ts:142`): remove the silent `return 1.0` — throw / block the quote so it fails closed.
- **H-3/H-4 FX divergence & unit confusion** (`currency.service.ts`): collapse the 4 routines into one `convert(fromMinor, from, to): {minor|major}` with one rounding + one fallback policy; give it a branded `Minor`/`Major` return type and audit every `amountMinor` assignment across the payment boundary.
- **H-5 `percentOff:0` treated as FIXED** (`fare-engine.service.ts:185`): use `promo.percentOff != null ? … : promo.amountOffMinor`.
- **H-6 `insuranceFeeMinor` dual meaning** (`fare-engine.service.ts:130,154,167`): split into `insuranceMultiplierBps` and `insuranceFeeMinor`; use one consistently.
- **H-7 domestic ETB fare × USD→ETB rate** (`fare-engine.service.ts:157`): don't apply a currency conversion to a domestic base fare — separate the minor-unit scaling from FX.
- **H-8 baggage ignores seat class** (`excess-baggage.service.ts:53`): `findFirst({ where: { seatClassId } })`.
- **H-9 baggage/supp skip conversion + DJF rounding** (`excess-baggage.service.ts:166`, `supplementary-charges.service.ts:132`): route through `convertMinorToChargeMajor`.
- **H-10 future-dated FX applied now** (`currency.service.ts:88,137`): add `effectiveDate: { lte: new Date() }` to the rate lookups.
- **H-11 non-deterministic fare resolution** (`fare-engine.service.ts:84,298`): add `orderBy: { validFrom: 'desc' }`; include `validFrom` in `SegmentFareRule`'s unique key (`schema.prisma:1113`) to allow dated versions.
- **H-12 divergent free-child rules** (`fare-engine.service.ts:172` vs `bookings.service.ts:1690` vs `:1622`): extract ONE `computeFare()` used by quote, booking, and payment.
- **H-13 promo silently dropped → overcharge** (`review/page.tsx:587`, `search.service.ts:940-970`): book the breakdown's discounted `totalMinor`, not the client-summed undiscounted per-pax fares — or return discounted per-pax fares. Fold into the C-1 fix (server recomputes the authoritative total incl. promo).
---
## P2 — config validation & safety (MEDIUM) / UI (LOW)
- **M-1 negative fares**: add `@Min(0)` to `baseFareMinor` (`schedules.dto.ts:85,95`) and `basePrice` (`seat-classes.dto.ts:29`).
- **M-2 promo bounds/date**: `@Min(0) @Max(100)` on `percentOff`, `@IsDateString()` on `validUntil` (`promos.dto.ts`); add min-spend / usage-limit / max-cap columns.
- **M-3 `PATCH /config` arbitrary**: replace the raw body with a whitelisted, typed DTO with per-key range checks (`system-config.controller.ts:34`).
- **M-4 past-date / double-booked schedules** (`schedules.service.ts:105,124`): reject past `departureAt`; widen the overlap check to the train across all routes.
- **M-5 deletes ignore references** (`stations`/`seat-classes`/`currencies`/`schedules` services): add referential guards before delete/disable; wrap the schedule cascade (`schedules.service.ts:438-485`) in a transaction.
- **M-6 non-atomic booking write** (`bookings.service.ts:883-926`): wrap create + confirmSeats + tier increment in one `$transaction`.
- **L-1 DJF shown with 2 decimals** (`portal/src/utils/format.ts:22`): format per `CHARGE_CURRENCY_DECIMALS` (DJF = 0).
- **L-2 client-side fare math** (`portal/src/utils/fare-utils.ts`, `review/page.tsx`): render only server-computed amounts; never submit a client-derived total (ties to C-1).
- **L-3 loyalty accrual currency** (`payments.service.ts:1062`): accrue from the actual charged amount/currency, not ETB minor.
---
## Suggested sequencing
1. **C-10, C-3, C-8, C-9** — quickest high-impact (a few lines each): unblock authenticated booking, stop free wallet credit, guard FX writes, enforce roles.
2. **C-1, C-2, C-4, C-5, C-6, C-7** — the money-integrity core (needs transactions + validation).
3. **H-2, H-3, H-4, H-7** — the FX/units foundation others compound on.
4. **H-1, H-5, H-8..H-12, M-1, M-2** — pricing correctness + validation.
5. **M-3..M-6, L-1..L-3** — config safety + UI consistency.