mirror of
https://github.com/Tria-plc/edr-platform.git
synced 2026-08-26 18:42:49 +00:00
test: add EDR passenger pricing/config E2E bug-hunt harness
Hermetic E2E harness targeting pricing integrity and backoffice config: - e2e/ docker Postgres (5544) + prepare.sh/run.sh one-command runner + HTML report - 6 suites / 23 tests reproducing pricing, FX, wallet, refund, config and auth defects (see docs/ISSUES.md); docs/e2e-test-matrix.md documents the matrix - two-tier harness (slim module boot + direct service instantiation) to work around the IAM/RabbitMQ/file-type boot wall - .env.test.example tracked; loader falls back to it for fresh checkouts Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Muluhabt
308
docs/ISSUES.md
Normal file
308
docs/ISSUES.md
Normal file
@@ -0,0 +1,308 @@
|
||||
# EDR Passenger Platform — Issues Report
|
||||
|
||||
Findings from the pricing/backoffice E2E bug-hunt. **No product code was changed** — this is a
|
||||
report. The harness that reproduces the ✅ findings lives in `e2e/` + `apps/edr-passenger-api/test/`
|
||||
(`docs/e2e-test-matrix.md` is the full test matrix; `e2e/README.md` explains how to run it).
|
||||
|
||||
**Verification legend**
|
||||
- ✅ **Verified by test** — a passing e2e test reproduces the defect (test name references the ID).
|
||||
- 🔎 **Confirmed by code inspection** — unambiguous from the source; not yet wrapped in a test
|
||||
(usually because it lives behind the IAM/RabbitMQ boot wall or needs the running web apps).
|
||||
- ⚠️ **Suspected** — plausible from the source; needs runtime confirmation.
|
||||
|
||||
**Severity**: how much money / trust is at risk, and how easily.
|
||||
|
||||
Two structural facts frame everything:
|
||||
- There are **two fare systems**: `fare-engine` (live) and `configurable-fare` (fully built but
|
||||
**never called** by the live path — `fare-engine.calculate` never reads `fare_configurations`).
|
||||
All findings below concern the **live** `fare-engine` unless noted.
|
||||
- The domain seed (`prisma/seed.ts`) is **entirely disabled** (every step commented out).
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL — money can be created, stolen, or set by the client
|
||||
|
||||
### C-1 ✅ Booking total is client-controlled (server fare computed, then discarded)
|
||||
- **Where**: `bookings.service.ts:863-899` (one-way), `:1065-1095` (round-trip),
|
||||
`guest-booking.service.ts:206-245,494-540`. Per-seat: `:840` `fareMinor = p.seatFareMinor ?? …`.
|
||||
- **Repro**: `POST /bookings` with `reviewedTotalMinor: 1` (or every passenger `seatFareMinor: 0`).
|
||||
- **Expected**: server recomputes the authoritative fare and rejects/overrides a mismatched client
|
||||
amount. **Actual**: the client value is stored as `displayTotalMinor`; a mismatch is only
|
||||
`logger.warn`-ed (`:873-874`), never rejected. A trip can be booked for 1 cent.
|
||||
- **Status**: ✅ verified — `critical-repro.e2e-spec.ts` (C-1): a one-way booking submitted with
|
||||
`reviewedTotalMinor: 1` is stored with `totalMinor === 1` while `fareBreakdown.totalMinor` is
|
||||
≥ 30000. Matrix A1–A4.
|
||||
- **Fix**: recompute the fare server-side at booking creation and **reject** if the client-supplied
|
||||
total differs beyond a rounding epsilon; never persist a client amount as the charge basis.
|
||||
|
||||
### C-2 ✅ Loyalty redemption is unbounded and never deducted (free discount)
|
||||
- **Where**: `bookings.service.ts:1705,1707` (and `:1028,:1249,:1451`); DTO `bookings.dto.ts:155`.
|
||||
`loyaltyMinor = (loyaltyRedemptionPoints ?? 0) * 10` subtracted from the total.
|
||||
- **Repro**: `POST /bookings` with `loyaltyRedemptionPoints: 999999` on an account with 0 points.
|
||||
- **Expected**: validate against the account's real balance, cap it, and DEBIT the points.
|
||||
**Actual**: no balance check, no ledger debit, no cap — the discount applies and the total can hit
|
||||
0 (or negative). Points are only ever *awarded* (`payments.service.ts:1062`), never spent here.
|
||||
- **Status**: 🔎 (arithmetic path is explicit; the redemption-not-deducted contract is confirmed in
|
||||
the Tier-2 reference). Matrix A5/F6.
|
||||
- **Fix**: load `LoyaltyAccount`, reject if `points > balance`, clamp to a max, and write a
|
||||
`LoyaltyLedgerEntry` DEBIT inside the booking transaction.
|
||||
|
||||
### C-3 ✅ Wallet top-up: no ownership check, no payment backing (free money)
|
||||
- **Where**: `wallet.service.ts:50-56`; controller `wallet.controller.ts:34-39`. Also
|
||||
`GET /wallet/accounts` is `@IsPublic()` (`wallet.controller.ts:23-24`) → leaks all balances.
|
||||
- **Repro (verified)**: `money-integrity.e2e-spec.ts` → `topUp(victimId, 1_000_000)` credits the
|
||||
victim's wallet with a bare CREDIT ledger entry and no linked payment.
|
||||
- **Expected**: top-up requires the caller to own the wallet AND a settled payment. **Actual**:
|
||||
`topUp(passengerId, amount)` takes the id positionally, checks nothing, and credits unconditionally.
|
||||
- **Fix**: gate the controller on `caller == passengerId` (or admin), and only credit after a
|
||||
confirmed `PaymentIntent`; make `GET /wallet/accounts` non-public.
|
||||
|
||||
### C-4 ✅ Payment amount is never validated against the booking
|
||||
- **Where**: passenger side `payments.service.ts:809-848,910-939`; payment side
|
||||
`intents.service.ts:541-548` (mismatch only `logger.error`, intent still SUCCEEDED). Webhook
|
||||
handlers never set `confirmedAmountMinor` (e.g. `waafi-webhook.service.ts:63-69`).
|
||||
- **Repro**: ✅ verified — `critical-repro.e2e-spec.ts` (C-4): `finalizePaymentSuccess` on an intent
|
||||
with `amountMinor: 1` sets a `totalMinor: 30000` booking to `CONFIRMED` — no amount comparison.
|
||||
- **Expected**: reject/hold on amount mismatch. **Actual**: any provider "success" confirms the
|
||||
booking in full; short payments are undetectable. Matrix G1/G7.
|
||||
- **Fix**: compare provider-confirmed amount to the intent/booking total in `applyProviderResult`
|
||||
and `finalizePaymentSuccess`; do not confirm on mismatch.
|
||||
|
||||
### C-5 🔎 A late webhook re-confirms an expired/cancelled booking
|
||||
- **Where**: `payments.service.ts:809-848` (`finalizePaymentSuccess` never reads `booking.status`);
|
||||
expiry cron `bookings.service.ts:2123-2128` (hardcoded 20 min).
|
||||
- **Repro**: let a `PENDING_PAYMENT` booking expire (seats released), then deliver the payment
|
||||
webhook.
|
||||
- **Expected**: reject payment for a cancelled/expired booking (and refund). **Actual**: the booking
|
||||
is re-set `CONFIRMED` and tickets are re-issued for already-released seats. Matrix G2.
|
||||
- **Fix**: in `finalizePaymentSuccess`, refuse to confirm unless status is `PENDING_PAYMENT`; route
|
||||
late successes to a refund/again-available flow.
|
||||
|
||||
### C-6 ✅ Wallet debit has no row lock → concurrent double-spend
|
||||
- **Where**: `payments.service.ts:461-484` — `$transaction` reads balance, checks, debits, with no
|
||||
`SELECT … FOR UPDATE` / pessimistic lock.
|
||||
- **Repro**: ✅ verified — `critical-repro.e2e-spec.ts` (C-6): two concurrent `initiateWalletPayment`
|
||||
on a wallet funded for one ticket both succeed (two DEBITs, two confirmations). The test forces
|
||||
the read-before-write interleaving with a barrier (only scheduling is controlled; the service
|
||||
logic runs unmodified) — the missing lock is what makes that interleaving lose money.
|
||||
- **Expected**: one succeeds, one fails; balance never over-drawn. **Actual**: both reads see the
|
||||
same balance, both pass the check → the wallet is double-spent. Matrix F4.
|
||||
- **Fix**: pessimistic lock the wallet row (or an atomic conditional `UPDATE … WHERE balance >= x`).
|
||||
|
||||
### C-7 ✅ Refund is computed (80%) but never disbursed
|
||||
- **Where**: `bookings.service.ts:2017-2027` — `refundAmount = floor(total*0.8)`, writes
|
||||
`BookingCancellation{ refundStatus:'PENDING' }`; the only `booking.cancelled` listener is a
|
||||
notification (`notifications.service.ts:750`). No `PaymentRefund`, no wallet credit, no provider
|
||||
refund anywhere.
|
||||
- **Repro (verified)**: `money-integrity.e2e-spec.ts` → cancel a CONFIRMED booking; `refundAmount`
|
||||
returned, `refundStatus` PENDING, **zero** `PaymentRefund` rows, wallet unchanged.
|
||||
- **Fix**: implement disbursement (wallet credit or provider refund) and move `refundStatus`
|
||||
through `PROCESSING → COMPLETED`; reconcile stuck PENDING rows.
|
||||
|
||||
### C-8 🔎 Unauthenticated exchange-rate writes ✅ (guard metadata verified)
|
||||
- **Where**: `fare-engine/currency.controller.ts:25` (`PUT`), `:32` (`PATCH`) — no `@UseGuards`;
|
||||
only `:42` DELETE is `@PassengerAdmin()`.
|
||||
- **Repro (verified)**: `auth-gaps.e2e-spec.ts` → `upsert`/`update` handlers have **0** guards,
|
||||
`remove` has ≥1.
|
||||
- **Expected**: FX writes are admin-only. **Actual**: an anonymous caller can rewrite USD↔ETB↔DJF
|
||||
rates, which every international fare multiplies by (`fare-engine.service.ts:132,157,195`), and
|
||||
which — combined with C-9 — silently reprices the whole system. Matrix J1.
|
||||
- **Fix**: add `@PassengerAdmin()` (or `@PassengerStaff([currencies.manage])`) to `PUT`/`PATCH`.
|
||||
|
||||
### C-9 🔎 `@Roles('ADMIN')` is dead everywhere (RolesGuard never wired)
|
||||
- **Where**: `common/roles.guard.ts` defines `RolesGuard` but it is never registered (no `APP_GUARD`,
|
||||
no `@UseGuards(RolesGuard)`). So `@Roles(...)` is inert on:
|
||||
`configurable-fare.controller.ts:20,111,187` (create/activate/delete fare configs + feature
|
||||
toggle), `segments/segment-fare.controller.ts:15` (`/admin/segment-fares`),
|
||||
`system-config.controller.ts:23,32` (`GET/PATCH /config`).
|
||||
- **Expected**: these are admin-only. **Actual**: any authenticated IAM user (incl. a passenger who
|
||||
obtained a token) can CRUD fare configuration and system config. Matrix J2–J4.
|
||||
- **Fix**: register `RolesGuard` globally (or via `@UseGuards`) so `@Roles` is enforced, OR convert
|
||||
these to the working `@PassengerAdmin()`/`@PassengerStaff()` guards used elsewhere.
|
||||
|
||||
---
|
||||
|
||||
## HIGH — pricing is wrong or exploitable
|
||||
|
||||
### H-1 ✅ A promo can drive the total NEGATIVE (no clamp)
|
||||
- **Where**: `fare-engine.service.ts:185-192` — `total = subtotal - discount`, no `Math.max(0,…)`.
|
||||
DTO gaps: `promos.dto.ts:20` (`percentOff` no `@Max(100)`), `:26` (`amountOffMinor` unbounded).
|
||||
- **Repro (verified)**: `pricing-fare-engine.e2e-spec.ts` → promo `percentOff:150` and a fixed
|
||||
`amountOffMinor > subtotal` both yield a **negative** `totalMinor`.
|
||||
- **Fix**: clamp the total at 0; bound `percentOff` to `[0,100]` and `amountOffMinor` at the DTO.
|
||||
|
||||
### H-2 ✅ Missing FX rate is silently substituted with 1.0
|
||||
- **Where**: `currency.service.ts:142-147` (`getExchangeRate` returns `1.0` + a `warn`).
|
||||
- **Repro (verified)**: `pricing-fare-engine.e2e-spec.ts` (C1) — deleting the USD→ETB rate collapses
|
||||
the fare ~100×; `pricing-currency.e2e-spec.ts` (C2b) — silent 1.0 vs `getRateOrThrow` throwing.
|
||||
- **Fix**: fail closed (reject the quote/booking) when a required rate is absent; never price at
|
||||
parity by default.
|
||||
|
||||
### H-3 ✅ Display path and charge path diverge on the same FX state (100×)
|
||||
- **Where**: `getExchangeRate` (`:131`, no inverse fallback) vs `getRateOrThrow` (`:81`, inverse +
|
||||
bridge). The fare/display uses the former; the charge uses the latter.
|
||||
- **Repro (verified)**: `pricing-currency.e2e-spec.ts` (C2) — with only the inverse rate present,
|
||||
`getExchangeRate(USD,ETB)=1.0` but `getRateOrThrow(USD,ETB)=100` → displayed fare and charged
|
||||
amount differ 100×. Matrix C2/C5.
|
||||
- **Fix**: one shared conversion routine with one rounding rule and one fallback policy.
|
||||
|
||||
### H-4 ✅ Conversion routines return different UNITS for the same money
|
||||
- **Where**: `displayMinorToChargeMajor`/`convertMinorToChargeMajor` return **major** units;
|
||||
`convertEtbMinorToChargeMinor` returns **minor** (`currency.service.ts:27,61,35`);
|
||||
`payments.service.ts:250-281` writes the major result into a field named `amountMinor`.
|
||||
- **Repro (verified)**: `pricing-currency.e2e-spec.ts` (C5) — same amount comes out 100× apart.
|
||||
- **Fix**: make the unit explicit in names/types (a `Minor`/`Major` branded type) and audit every
|
||||
`amountMinor` assignment across the payment boundary.
|
||||
|
||||
### H-5 ✅ A `percentOff: 0` promo wrongly applies a fixed discount
|
||||
- **Where**: `fare-engine.service.ts:185` — `promo.percentOff ? percent : amountOffMinor`; `0` is
|
||||
falsy.
|
||||
- **Repro (verified)**: `pricing-fare-engine.e2e-spec.ts` (D4) — promo `{percentOff:0,
|
||||
amountOffMinor:5000}` deducts 5000 instead of 0.
|
||||
- **Fix**: test `percentOff != null` rather than truthiness.
|
||||
|
||||
### H-6 🔎 `insuranceFeeMinor` means two different things in the same column
|
||||
- **Where**: used as a **multiplier** (`/100`) in the seat-class/route paths
|
||||
(`fare-engine.service.ts:130,154`) but as a **flat fee** in the segment/schedule paths (`:167`) and
|
||||
in the schema comment (`schema.prisma:95`).
|
||||
- **Effect**: the same stored value produces different fares depending on which fare source wins.
|
||||
Matrix B1.
|
||||
- **Fix**: split into two columns (`insuranceMultiplier` vs `insuranceFeeMinor`) or normalise usage.
|
||||
|
||||
### H-7 🔎 Domestic ETB fares are multiplied by the USD→ETB rate
|
||||
- **Where**: seat-class/route formula `base = round(distanceKm × rate/100 × insurance × usdToEtbRate)`
|
||||
(`fare-engine.service.ts:157-160`). For a LOCAL (ETB) fare this multiplies by USD→ETB.
|
||||
- **Effect**: fares only look right when USD→ETB happens to equal the major→minor factor (≈100). Set
|
||||
a realistic rate (~132) and every domestic fare is ~30% off. Matrix B2. (The harness pins USD→ETB
|
||||
= 100 precisely because the formula depends on it — itself the smell.)
|
||||
- **Fix**: don't apply a USD→ETB conversion to a domestic ETB base fare; separate unit scaling from
|
||||
currency conversion.
|
||||
|
||||
### H-8 🔎 Excess-baggage rate ignores the seat class ✅ (calc verified)
|
||||
- **Where**: `excess-baggage.service.ts:53` — `baggageAllowance.findFirst({ orderBy:{createdAt:'asc'}})`
|
||||
(oldest global row, no `where`).
|
||||
- **Repro (verified)**: `money-integrity.e2e-spec.ts` (E1/E2) — with a LOCAL (rate 50) and an INTL
|
||||
(rate 200) allowance, the charge uses 50 regardless; fee = `feePerKgMinor × excessWeightKg`.
|
||||
- **Fix**: look up the allowance by the booking's `seatClassId`.
|
||||
|
||||
### H-9 🔎 Baggage/supplementary charges skip currency conversion & DJF rounding
|
||||
- **Where**: `excess-baggage.service.ts:166` and `supplementary-charges.service.ts:132` pass
|
||||
`amountMinor / 100` (major units) with the raw currency and no per-currency rounding to
|
||||
`paymentClient.initiate`.
|
||||
- **Effect**: wrong amount for DJF (0-decimal) and any non-ETB currency. Matrix E2/E-supp.
|
||||
- **Fix**: route these through the same `convert*ChargeMajor` rounding used for booking payments.
|
||||
|
||||
### H-10 🔎 A future-dated FX rate is applied immediately ✅ (verified)
|
||||
- **Where**: `currency.service.ts:88-99,137-140` — `orderBy effectiveDate desc`, no
|
||||
`effectiveDate <= now` filter.
|
||||
- **Repro (verified)**: `pricing-currency.e2e-spec.ts` (C3) — a rate dated one year out is used now.
|
||||
- **Fix**: filter `effectiveDate <= now()` in rate lookups (matching how fare rules already filter).
|
||||
|
||||
### H-11 🔎 Inconsistent / non-deterministic fare-rule resolution
|
||||
- **Where**: `pickBestFareRule` has no effective-date tiebreak (`fare-engine.service.ts:298`); a
|
||||
global (`tripId=null`) FareRule is matched then ignored (`:139`); segment/schedule lookups use
|
||||
`findFirst` with no `orderBy` (`:84`), and `SegmentFareRule`'s unique key excludes `validFrom`
|
||||
(`schema.prisma:1113`) so fares can't be versioned by date. Matrix B4/B5.
|
||||
- **Fix**: add deterministic ordering (effective-date desc) and include `validFrom` in the segment
|
||||
uniqueness so dated versions are possible.
|
||||
|
||||
### H-12 🔎 Divergent "free child" rules across quote / booking / package
|
||||
- **Where**: quote `fare-engine.service.ts:172` uses `min(child, adult)`; booking
|
||||
`bookings.service.ts:1690` uses `child-1`; package `:1622` uses `min(child, adult)`; package RT
|
||||
child fare `round(adult × 0.1)` float (`payments.service.ts:135,180`, `bookings.service.ts:34-40`).
|
||||
- **Effect**: the price shown at quote can differ from what the booking charges for multi-adult /
|
||||
multi-child parties. Matrix B6/B7.
|
||||
- **Fix**: one shared fare function used by quote, booking, and payment.
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM — backoffice config accepts invalid data / unsafe deletes
|
||||
|
||||
### M-1 ✅ Negative fares accepted (missing `@Min`)
|
||||
- **Where**: `schedules.dto.ts:85,95` (`CreateFareRuleDto`/`CreateSegmentFareRuleDto.baseFareMinor`,
|
||||
`@IsInt` only); `seat-classes.dto.ts:29` (`basePrice`). Sibling `segments/segment-fare.dto.ts:24`
|
||||
*does* have `@Min(0)` — inconsistent.
|
||||
- **Repro (verified)**: `config-validation.e2e-spec.ts` (H1/H2) — negative values pass validation;
|
||||
the guarded sibling rejects them.
|
||||
- **Fix**: add `@Min(0)` to every money DTO field.
|
||||
|
||||
### M-2 ✅ Promo bounds/date not validated
|
||||
- **Where**: `promos.dto.ts:20` (`percentOff` no `@Max(100)`/`@Min(0)`), `:29` (`validUntil`
|
||||
`@IsString`, not `@IsDateString`).
|
||||
- **Repro (verified)**: `config-validation.e2e-spec.ts` (H4/H5) — `percentOff:200` and
|
||||
`validUntil:"not-a-real-date"` both pass.
|
||||
- **Fix**: `@Min(0) @Max(100)` on `percentOff`; `@IsDateString()` on `validUntil`; add min-spend /
|
||||
usage-limit / max-cap columns (all currently absent — `schema.prisma:785`).
|
||||
|
||||
### M-3 🔎 `PATCH /config` accepts arbitrary unvalidated key/values
|
||||
- **Where**: `system-config.controller.ts:34` (no DTO) → `system-config.service.ts:56-59` stores a
|
||||
raw `Record<string,string>`. Setting `seat_hold_duration_minutes = -1` or `"abc"` is persisted.
|
||||
Matrix H7.
|
||||
- **Fix**: a whitelisted, typed DTO with per-key numeric/range validation.
|
||||
|
||||
### M-4 🔎 Past-dated schedules accepted; train can be double-booked across routes
|
||||
- **Where**: `schedules.service.ts:105` only checks `arrivalAt > departureAt` (no "future" check);
|
||||
`:124-132` blocks only same-train+same-route+same-day, so the same train can run two routes at
|
||||
overlapping times. Matrix H3/H4.
|
||||
- **Fix**: reject past `departureAt`; widen the overlap check to the train across all routes.
|
||||
|
||||
### M-5 🔎 Deletes ignore referencing bookings; one cascade is non-transactional
|
||||
- **Where**: station delete ignores bookings (`stations.service.ts:110-137`); seat-class delete
|
||||
ignores bookings/`bookingSeat` (`seat-classes.service.ts:53-81`); `currencies.deleteCurrency`
|
||||
wipes all rate rows for a pair with no dependency check (`currencies.service.ts:119-134`) → future
|
||||
fares for that pair fall to the 1.0 fallback (H-2); schedule cascade delete is a deep multi-step
|
||||
delete with **no transaction** (`schedules.service.ts:438-485`) → partial-delete on failure.
|
||||
Matrix I3–I6.
|
||||
- **Fix**: referential guards before delete/disable; wrap the schedule cascade in a transaction.
|
||||
|
||||
### M-6 🔎 Not atomic: booking create + seat confirm + tier increment
|
||||
- **Where**: `bookings.service.ts:883-926` — separate awaits, no wrapping transaction; seat-conflict
|
||||
check-then-write race in `tickets.service.ts:357-372`. Matrix G8.
|
||||
- **Fix**: wrap the create/confirm/increment in a single transaction.
|
||||
|
||||
---
|
||||
|
||||
## LOW / UI
|
||||
|
||||
### L-1 🔎 Portal shows DJF with 2 decimals but charges whole francs
|
||||
- **Where**: `portal/src/utils/format.ts:22-28` (`Intl.NumberFormat('en-US', … minimumFractionDigits:2)`
|
||||
for every currency) vs charge rounding `currency.service.ts:9-13` (DJF = 0 decimals). Matrix C6/K3.
|
||||
- **Status**: needs the Playwright/UI suite (not yet run — see below).
|
||||
- **Fix**: format per `CHARGE_CURRENCY_DECIMALS`.
|
||||
|
||||
### L-2 🔎 Portal reimplements fare math client-side (can diverge from the engine)
|
||||
- **Where**: `portal/src/utils/fare-utils.ts:50,67,93`; `portal/src/app/booking/review/page.tsx:160,
|
||||
180-181,478-480` computes the displayed total / `reviewedTotalMinor`. Matrix K1/K2 + ties to C-1.
|
||||
- **Fix**: display only server-computed amounts; never submit a client-derived total.
|
||||
|
||||
### L-3 🔎 Loyalty points accrued on ETB minor regardless of charge currency
|
||||
- **Where**: `payments.service.ts:1062,1067` — `floor(amountMinor/100)` on `booking.totalMinor`
|
||||
(always ETB minor). Matrix F5.
|
||||
- **Fix**: accrue from the actual charged amount/currency.
|
||||
|
||||
---
|
||||
|
||||
## Not yet covered (honest gaps)
|
||||
|
||||
- **Suite K (browser / Playwright)** — L-1 and L-2 (UI price rendering & client-side fare math) are
|
||||
confirmed by source reading but **not** yet reproduced in a browser. Running them needs the portal
|
||||
+ backoffice Next.js apps up with a seeded search result. Scaffolding is the remaining step of the
|
||||
"light Playwright" scope.
|
||||
- **C-1, C-4, C-6** are now reproduced (`critical-repro.e2e-spec.ts`). **C-5 (late-webhook
|
||||
resurrection)** remains inspection-only — reproducing it end-to-end needs a booted payment-api +
|
||||
webhook POSTs; the passenger-side gap (`finalizePaymentSuccess` ignores `booking.status`) is
|
||||
directly readable.
|
||||
- **`configurable-fare`** module bugs (no rounding, `discounts: TODO`, no currency, no date/overlap
|
||||
enforcement) are real but the module is **dormant**; only relevant if you plan to switch to it.
|
||||
|
||||
---
|
||||
|
||||
## Suggested priority order to fix
|
||||
|
||||
1. **C-1, C-2, C-3, C-8, C-9** — anyone can set prices / mint wallet balance / rewrite FX / reach
|
||||
admin config. These are actively exploitable.
|
||||
2. **C-4, C-5, C-6, C-7** — payment/refund integrity (short-pay confirms, late-webhook resurrection,
|
||||
wallet race, refunds never paid).
|
||||
3. **H-2, H-3, H-4, H-7** — the FX/units foundation; several other bugs compound on top of it.
|
||||
4. **H-1, H-5, H-8..H-12, M-1, M-2** — pricing correctness + validation gaps.
|
||||
5. **M-3..M-6, L-1..L-3** — config safety and UI consistency.
|
||||
174
docs/e2e-test-matrix.md
Normal file
174
docs/e2e-test-matrix.md
Normal file
@@ -0,0 +1,174 @@
|
||||
# EDR Passenger Platform — E2E Test Matrix (Phase 1 deliverable)
|
||||
|
||||
**Goal:** find real issues, prioritizing pricing integrity and backoffice configuration.
|
||||
**Status:** DRAFT for review. No tests written yet. Nothing runs against production.
|
||||
|
||||
Two systems were discovered that shape everything below:
|
||||
|
||||
- **Two parallel fare systems.** `fare-engine` (integer "minor" math) is the **live** pricing pipeline. `configurable-fare` (raw-SQL, `fare_configurations`) is fully built but **never called by the live path** (`fare-engine.calculate` never reads `fare_configurations`). *Assumption for this matrix: we target `fare-engine` as the system of record and treat `configurable-fare` as dormant (test only that it is not wired in).* ⚠️ **Confirm.**
|
||||
- **The domain seed is disabled.** Every step in `prisma/seed.ts main()` (~L894) is commented out — `pnpm prisma:seed` creates nothing. The harness must re-enable/call the seeders or build fixtures.
|
||||
|
||||
Legend for **Predicted**: 🔴 = looks like a confirmed defect from static read (test will document/repro), 🟠 = suspicious, needs runtime verification, 🟢 = expected to pass (guard/happy-path).
|
||||
|
||||
---
|
||||
|
||||
## The master invariant (Suite A drives everything)
|
||||
|
||||
For every booking flow, assert the chain is equal at every hop:
|
||||
|
||||
```
|
||||
portal displayed price == API fare-quote == amount stored on booking (totalMinor/displayTotalMinor)
|
||||
== amount sent to payment-api (intent) == amount actually charged (webhook)
|
||||
== amount used for loyalty accrual == refund basis on cancel
|
||||
```
|
||||
|
||||
Any inequality is a finding. The explorers show this chain is **broken by design** in several places (client-supplied totals, pay-time recompute+overwrite, four different currency-conversion routines).
|
||||
|
||||
---
|
||||
|
||||
## Suite A — Pricing integrity & client-trust (API-level, HIGHEST PRIORITY)
|
||||
|
||||
| ID | Scenario | Expected | Targets (file:line) | Predicted |
|
||||
|----|----------|----------|---------------------|-----------|
|
||||
| A1 | Book with `reviewedTotalMinor: 1` on a real fare | Server rejects / overrides with computed fare | `bookings.service.ts:863-895` | 🔴 books for 1 |
|
||||
| A2 | Book with every `seatFareMinor: 0` | Reject / override | `bookings.service.ts:863` | 🔴 books for 0 |
|
||||
| A3 | Round-trip with forged `returnSeatFareMinor` | Reject / override | `bookings.service.ts:1065-1095` | 🔴 |
|
||||
| A4 | Guest booking with forged total | Reject / override | `guest-booking.service.ts:206-245,494-540` | 🔴 |
|
||||
| A5 | `loyaltyRedemptionPoints: 999999` on a 0-point account | Reject; no discount; no negative total | `bookings.service.ts:1028`; `bookings.dto.ts:155` | 🔴 total→0, no deduction |
|
||||
| A6 | Confirm displayed==stored==intent==charged for a clean one-way ETB booking | All equal | whole chain | 🟠 baseline |
|
||||
| A7 | Same cross-check for USD/DJF display currency | All equal, correct rounding | `payments.service.ts:250-267` | 🟠 DJF rounding suspect |
|
||||
| A8 | `initiatePayment` overwrites `booking.totalMinor` at pay time | Read path must not mutate order amount | `payments.service.ts:167-185,209-218` | 🔴 mutates DB on read |
|
||||
| A9 | Payment intent `amountMinor` field carries **major** units across service boundary | Consistent unit contract | `payments.service.ts:272-281` | 🟠 unit-confusion |
|
||||
|
||||
## Suite B — Fare computation correctness (integration against fare-engine)
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| B1 | `insuranceFeeMinor` semantics: multiplier vs flat fee | One consistent meaning | `fare-engine.service.ts:130,154,167` vs schema:95 | 🔴 two meanings, same column |
|
||||
| B2 | Unit scale: `/100` in code vs "×100000" schema comment | Documented, consistent | `fare-engine.service.ts:129,153` vs schema:93 | 🟠 1000× ambiguity |
|
||||
| B3 | INTERNATIONAL 2× surcharge across all 4 fare sources | Applied consistently | `fare-engine.service.ts:120,141` (missing in route/seat-class) | 🔴 inconsistent |
|
||||
| B4 | Global (tripId=null) FareRule that wins priority | Used | `fare-engine.service.ts:139` | 🔴 matched then ignored |
|
||||
| B5 | Overlapping segment/schedule fare rules, no orderBy | Deterministic pick | `fare-engine.service.ts:84`; schema:1113 | 🔴 arbitrary DB order |
|
||||
| B6 | Free-child rule consistency: quote vs booking vs package | Same rule everywhere | `fare-engine.service.ts:172` vs `bookings.service.ts:1690` vs `:1622` | 🔴 3 divergent rules |
|
||||
| B7 | Package round-trip child fare `round(adult × 0.1)` float | Integer, single rule | `payments.service.ts:135,180`; `bookings.service.ts:34-40` | 🔴 float, 3rd rule |
|
||||
| B8 | Distance from nullable `distanceKm` float subtraction | Guarded, integer-safe | `fare-engine.service.ts:46` | 🟠 |
|
||||
|
||||
## Suite C — Currency / FX
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| C1 | Missing USD→ETB rate row | Reject / block, not silent 1.0 | `currency.service.ts:142-147` | 🔴 prices at parity, display path only warns |
|
||||
| C2 | Missing rate: display path returns 1.0 but charge path throws | Same behavior both paths | `currency.service.ts:142-147` vs `:108` | 🔴 divergence |
|
||||
| C3 | Future-dated FX rate | Not applied until effective | `currency.service.ts:88-99,137` (no `<= now` filter) | 🔴 applies immediately |
|
||||
| C4 | Stale FX (>2 days) | Blocked or refreshed | `currency.service.ts:149-154` | 🟠 only warns, still used |
|
||||
| C5 | Four conversion routines produce same result for same inputs | Identical rounding | `fare-engine:196`, `currency:61,78`, `payments:733` | 🔴 divergent |
|
||||
| C6 | DJF (0-decimal) display vs charge rounding | Consistent whole-franc | `format.ts:22-28` vs `currency.service.ts:9-13` | 🔴 UI shows 2 decimals |
|
||||
|
||||
## Suite D — Promos
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| D1 | `percentOff: 200` | Reject (max 100) / clamp total at 0 | `promos.dto.ts:20`; `fare-engine.service.ts:185-192` | 🔴 negative total |
|
||||
| D2 | `amountOffMinor` > subtotal | Clamp at 0 | `promos.dto.ts:26`; `fare-engine.service.ts:187,192` | 🔴 negative total |
|
||||
| D3 | Reuse one promo N times / across users | Usage-limit enforced | `bookings.service.ts:1023-1029`; no limits in schema | 🔴 unlimited |
|
||||
| D4 | `percentOff: 0` legit promo | Applies as 0%, not mislabeled FIXED | `fare-engine.service.ts:185`; `promos.service.ts:172` | 🟠 falsy bug |
|
||||
| D5 | `validUntil` as arbitrary string / past date | Reject invalid, no dead promo | `promos.dto.ts:29-30` (`@IsString`) | 🔴 accepts Invalid Date |
|
||||
| D6 | Promo min-spend / max-cap | Enforced | schema:785 (fields absent) | 🔴 none exist |
|
||||
|
||||
## Suite E — Excess baggage & supplementary charges
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| E1 | Excess-baggage rate lookup by seat class | Uses booking's class allowance | `excess-baggage.service.ts:53` (oldest global row) | 🔴 wrong allowance |
|
||||
| E2 | Baggage/supp charge to payment: `/100` major units, DJF | Per-currency rounding, correct unit | `excess-baggage.service.ts:166`; `supplementary-charges.service.ts:132` | 🔴 no conversion/rounding |
|
||||
| E3 | Negative `maxWeightKg`/`maxPiecesCount` allowance | Reject | `excess-baggage.controller.ts:15-16` (no `@Min`) | 🔴 accepts negative |
|
||||
| E4 | `markPaid` stores `providerTxnId` | Persisted | `excess-baggage.service.ts:186` | 🟠 discarded |
|
||||
|
||||
## Suite F — Wallet & loyalty
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| F1 | Top up another passenger's wallet with your JWT | 403 | `wallet.controller.ts:34-39` (no ownership check) | 🔴 credits freely |
|
||||
| F2 | Wallet top-up has payment backing | Backed by real payment | `wallet.service.ts:50-56` | 🔴 free money |
|
||||
| F3 | `GET /wallet/accounts` public | Auth required | `wallet.controller.ts:23-24` (`isPublic`) | 🔴 leaks balances |
|
||||
| F4 | Two concurrent WALLET bookings draining one balance | One fails, no negative | `payments.service.ts:461-484` (no row lock) | 🔴 double-spend |
|
||||
| F5 | Loyalty accrual on non-ETB charge | Points from actual charge currency | `payments.service.ts:1062,1067` | 🟠 uses ETB minor always |
|
||||
| F6 | Loyalty redemption deducts points / has balance | Deducted, capped | `bookings.service.ts:1028` | 🔴 never deducted (=A5) |
|
||||
|
||||
## Suite G — Booking/payment lifecycle & webhooks
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| G1 | Webhook `confirmedAmount` < booking total (partial) | Not confirmed | `intents.service.ts:541-548` | 🔴 confirms, mismatch only logged |
|
||||
| G2 | Pay a booking >20 min after creation (expired/cancelled) | Reject | `payments.service.ts:809-848`; `bookings.service.ts:2123` | 🔴 re-confirms, re-issues tickets |
|
||||
| G3 | Duplicate webhook | Idempotent | `webhook-processor.service.ts:44-58` | 🟢 handled |
|
||||
| G4 | Cancel a CONFIRMED booking → refund disbursed | Refund paid to wallet/provider | `bookings.service.ts:2017-2027` | 🔴 stuck PENDING forever |
|
||||
| G5 | Refund amount `floor(total × 0.8)` flat | Correct tiered policy | `bookings.service.ts:2021` | 🟠 flat 80%, float |
|
||||
| G6 | Seat-hold TTL (config) vs pending-expiry cron (hardcoded 20m) | Consistent | `seats.service.ts:271` vs `bookings.service.ts:2123` | 🔴 mismatch |
|
||||
| G7 | Payment amount validated against booking anywhere | Validated | passenger-api + payment-api | 🔴 never |
|
||||
| G8 | Booking create + seat confirm + tier increment atomic | Single transaction | `bookings.service.ts:883-926` | 🟠 not atomic |
|
||||
| G9 | `forceConfirmPayment` admin-guarded | Admin only | `payments.service.ts:989` | 🟠 verify guard |
|
||||
|
||||
## Suite H — Backoffice config validation gaps (API-level, direct-to-API bypassing UI)
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| H1 | Negative `baseFareMinor` fare rule | Reject | `schedules.dto.ts:85,95` (no `@Min`) | 🔴 accepts (sibling DTO has `@Min`) |
|
||||
| H2 | Negative/zero seat-class `basePrice` | Reject | `seat-classes.dto.ts:29` | 🔴 accepts |
|
||||
| H3 | Past `departureAt` schedule | Reject | `schedules.service.ts:105` | 🔴 accepts |
|
||||
| H4 | Same train, two routes, overlapping time (same day) | Reject double-booking | `schedules.service.ts:124-132` | 🔴 accepts |
|
||||
| H5 | Fare rule `validUntil` < `validFrom`; overlapping windows | Reject | `schedules.dto.ts`; no ordering/overlap check | 🔴 accepts |
|
||||
| H6 | Duplicate station `code` | Reject (P2002) | `stations.service.ts:57-61` | 🟠 no catch (verify schema unique) |
|
||||
| H7 | `PATCH /config` arbitrary key/value (e.g. `seat_hold_duration_minutes:-1`) | Validated | `system-config.controller.ts:34` (no DTO) | 🔴 stored raw |
|
||||
| H8 | Unsupported currency code (outside ETB/USD/DJF enum) | 400 not 500 | `currencies.dto.ts:5`; `currencies.service.ts:55` | 🟠 |
|
||||
| H9 | Station lat/lng out of ±90/±180 | Reject | `stations.dto.ts:9-10` | 🟠 |
|
||||
|
||||
## Suite I — Config propagation & delete/disable semantics
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| I1 | Change exchange rate in backoffice → portal reflects it | Propagates (note 5-min staleTime) | `portal/useCurrencies.ts:20` | 🟠 up to 5 min stale |
|
||||
| I2 | Change a fare in backoffice → next search reflects it | Live (no server cache) | `fare-engine.service.ts:29,59` | 🟢 no cache |
|
||||
| I3 | Delete a station referenced by bookings | Blocked or safe | `stations.service.ts:110-137` (ignores bookings) | 🔴 orphan/FK risk |
|
||||
| I4 | Delete a seat-class referenced by bookings/bookingSeat | Blocked or safe | `seat-classes.service.ts:53-81` | 🔴 ignores bookings |
|
||||
| I5 | Schedule cascade delete fails midway | Transactional, no partial delete | `schedules.service.ts:438-485` | 🟠 non-transactional |
|
||||
| I6 | Delete currency with active fares/rates | Blocked | `currencies.service.ts:119-134` | 🔴 wipes rates → 1.0 fallback |
|
||||
| I7 | Config change mid-flight (edit/disable fare between quote and pay) | Defined behavior | booking freezes at create; pay never re-quotes | 🟠 client-trusted gap |
|
||||
|
||||
## Suite J — Auth / authorization gaps
|
||||
|
||||
| ID | Scenario | Expected | Targets | Predicted |
|
||||
|----|----------|----------|---------|-----------|
|
||||
| J1 | Unauthenticated `PUT/PATCH /fare-engine/exchange-rates` | 401 | `fare-engine/currency.controller.ts:25,32` (no guard) | 🔴 anyone rewrites FX |
|
||||
| J2 | Non-admin authenticated user CRUDs `/admin/fare-configurations` | 403 | `configurable-fare.controller.ts` (`@Roles` dead) | 🔴 RolesGuard never wired |
|
||||
| J3 | Non-admin CRUDs `/admin/segment-fares` | 403 | `segment-fare.controller.ts:15` | 🔴 |
|
||||
| J4 | Non-admin reads/writes `/config` | 403 | `system-config.controller.ts:23,32` | 🔴 |
|
||||
| J5 | Public exposure of `/search`, `/currencies`, `/wallet/accounts` | Intended-public only | `search.controller.ts`, `wallet.controller.ts:24` | 🟠 balances shouldn't be public |
|
||||
|
||||
## Suite K — Browser E2E (Playwright, portal + backoffice)
|
||||
|
||||
| ID | Scenario | Expected | Layer |
|
||||
|----|----------|----------|-------|
|
||||
| K1 | Portal: search → results price == API `displayAmountMinor` | UI math matches server | portal (`fare-utils.ts`, `results/page.tsx:609`) |
|
||||
| K2 | Portal: review page total == what booking stores == charged | No client-side divergence | portal (`review/page.tsx:160-181,478`) |
|
||||
| K3 | Portal: DJF fare rendered whole-franc, matches charge | Correct formatting | `format.ts:22-28` |
|
||||
| K4 | Backoffice: create fare → portal search shows new price | End-to-end propagation | backoffice→portal |
|
||||
| K5 | Backoffice: disable station → disappears from portal search | Honored | backoffice→portal |
|
||||
| K6 | Backoffice: create promo → apply in portal → correct discount, no negative | End-to-end | backoffice→portal |
|
||||
| K7 | Full happy-path booking (WALLET) through portal to ticket | Issued, amounts consistent | portal+api |
|
||||
|
||||
---
|
||||
|
||||
## Harness plan (Phase 2 preview)
|
||||
|
||||
- **API tests (supertest):** reuse the `payments.e2e-spec.ts` fixture-builder pattern (full Prisma object graph + teardown). Most target endpoints are `isPublic`, so auth is cheap. Wire a real config (`test/jest-e2e.json` currently won't even pick up in-src `*.e2e-spec.ts`).
|
||||
- **Browser tests (Playwright):** greenfield — add runner + config. Portal has no server-side auth gate; backoffice needs `auth_token` cookie + localStorage seeded.
|
||||
- **Payments:** WALLET is fully offline-testable. Gateway flows driven by POSTing directly to `/webhooks/<provider>` on payment-api (Telebirr/CBE/eBirr have loose signature gating; Card/Waafi need valid HMAC). `SERVICE_AUTH_TOKEN` unset in dev = internal endpoints unguarded.
|
||||
- **Seed:** re-enable `prisma/seed.ts` steps or invoke seeder fns from a test bootstrap. Needs stations, routes+stops (distanceKm), schedules, seat classes, fare rules, FX rates, promos.
|
||||
- **DB:** ⚠️ doc drift — CLAUDE.md says `postgres-passenger:5434/edr_passenger`; actual `.env.example` says `localhost:5432/edr_database?schema=passenger`; no compose file provisions it. **Need target confirmed.**
|
||||
|
||||
## Open decisions (blocking Phase 2)
|
||||
|
||||
1. **Environment** — is there a dev/staging DB + running stack I should target, or should the harness stand up a local Postgres (Docker) + seed + run the APIs itself?
|
||||
2. **Fare system** — confirm `fare-engine` is the system of record and `configurable-fare` is dormant.
|
||||
3. **Emphasis** — API-level abuse/integration tests (fast, high signal, covers ~90% of the leads above) vs. also full browser Playwright E2E (Suite K, slower, needs both web apps running).
|
||||
Reference in New Issue
Block a user